(2012/08/09 11:08), Eric Blake wrote:
> On 08/07/2012 06:04 PM, MATSUDA, Daiki wrote:
>> Add qemuAgentCommand() for general qemu agent command.
>>
>> include/libvirt/libvirt-qemu.h | 5 +++++
>> src/qemu/qemu_agent.c | 38 ++++++++++++++++++++++++++++++++++++++
>> src/qemu/qemu_agent.h | 5 +++++
>> 3 files changed, 48 insertions(+)
>>
>
> Causes a compiler warning:
>
> CC libvirt_driver_qemu_impl_la-qemu_agent.lo
> qemu/qemu_agent.c: In function 'qemuAgentQemuAgentCommand':
> qemu/qemu_agent.c:1411:9: error: variable 'seconds' set but not used
> [-Werror=unused-but-set-variable]
Sorry, it has a mistake on calling qemuAgentCommand() with not seconds but timeout.
>> diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h
>> index a37f897..ffc5ae5 100644
>> --- a/include/libvirt/libvirt-qemu.h
>> +++ b/include/libvirt/libvirt-qemu.h
>> @@ -44,6 +44,11 @@ virDomainPtr virDomainQemuAttach(virConnectPtr domain,
>> unsigned int pid_value,
>> unsigned int flags);
>>
>> +typedef enum {
>> + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK = -1,
>> + VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT = 0,
>> +} virDomainQemuAgentCommandTimeoutFlags;
>
> These aren't flags, but special timeout values. Maybe s/Flags/Values/
>
>
>> +
>> +int qemuAgentQemuAgentCommand(qemuAgentPtr mon,
>
> This name sounds funny; maybe qemuAgentArbitraryCommand might be nicer.
>
>> + const char *cmd_str,
>> + char **result,
>> + int timeout)
>> +{
>> + int ret = -1;
>> + int seconds;
>> + virJSONValuePtr cmd;
>> + virJSONValuePtr reply = NULL;
>> +
>> + cmd = virJSONValueFromString(cmd_str);
>> + if (!cmd)
>> + return ret;
>> +
>> + if (result == NULL) {
>> + seconds = 0;
>> + } else if (timeout == VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) {
>> + seconds = 5;
>
> Why 5 seconds here,
Becuase I realize the infinite waiting as you suggested.
> * If @result is NULL, then don't wait for a result (and @timeout
> * must be 0). Otherwise, wait for @timeout seconds for a
> * successful result to be populated into *@result, with
> * VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK (-1) meaning to block
> * forever waiting for a result.
But it is difficult. So, I used 'repeat' tag and 5 seconds is default value.
#define QEMU_AGENT_WAIT_TIME (1000ull * 5)
>> + } else {
>> + seconds = timeout;
>
> but the user's timeout here? If you are looping while waiting for a
> response, then 5 seconds is too long between loops; you will feel
> unresponsive; and if the user passes a timeout of 10, you end up waiting
> 10 seconds.
>
>> + }
>> +
>> +repeat:
>> + ret = qemuAgentCommand(mon, cmd, &reply, timeout);
>
> Won't work - this starts a new agent command every time repeat is
> reached, rather than waiting for the completion of an existing command.
>
>> +
>> + if (ret == 0) {
>> + ret = qemuAgentCheckError(cmd, reply);
>> + *result = virJSONValueToString(reply);
>> + } else {
>> + if (timeout == VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) goto repeat;
>
> Typical formatting would be to split this to two lines:
>
> if (timeout == VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK)
> goto repeat;
>
>> + *result = NULL;
>> + }
>> +
>> + virJSONValueFree(cmd);
>> + virJSONValueFree(reply);
>> + return ret;
>> +}
>> diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
>> index 2fdebb2..fc19c2f 100644
>> --- a/src/qemu/qemu_agent.h
>> +++ b/src/qemu/qemu_agent.h
>> @@ -77,4 +77,9 @@ int qemuAgentFSThaw(qemuAgentPtr mon);
>>
>> int qemuAgentSuspend(qemuAgentPtr mon,
>> unsigned int target);
>> +
>> +int qemuAgentQemuAgentCommand(qemuAgentPtr mon,
>> + const char *cmd,
>> + char **result,
>> + int timeout);
>> #endif /* __QEMU_AGENT_H__ */
>>
>
> Definitely needs some work before this function will handle timeout
> properly.
>
And I attached fixed patch, but it is not completed. After receiving all comments,
I will resend new patches.
include/libvirt/libvirt-qemu.h | 5 ++++
src/qemu/qemu_agent.c | 47 ++++++++++++++++++++++++++++++++++++++++-
src/qemu/qemu_agent.h | 5 ++++
3 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h
index a37f897..ffc5ae5 100644
--- a/include/libvirt/libvirt-qemu.h
+++ b/include/libvirt/libvirt-qemu.h
@@ -44,6 +44,11 @@ virDomainPtr virDomainQemuAttach(virConnectPtr domain,
unsigned int pid_value,
unsigned int flags);
+typedef enum {
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK = -1,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT = 0,
+} virDomainQemuAgentCommandTimeoutValues;
+
# ifdef __cplusplus
}
# endif
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 7699042..1cfcafc 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -830,7 +830,8 @@ void qemuAgentClose(qemuAgentPtr mon)
virObjectUnref(mon);
}
-#define QEMU_AGENT_WAIT_TIME (1000ull * 5)
+#define QEMU_AGENT_WAIT_DEFAULT 5
+#define QEMU_AGENT_WAIT_TIME (1000ull * QEMU_AGENT_WAIT_DEFAULT)
/**
* qemuAgentSend:
@@ -1401,3 +1401,47 @@ qemuAgentSuspend(qemuAgentPtr mon,
virJSONValueFree(reply);
return ret;
}
+
+int qemuAgentArbitraryCommand(qemuAgentPtr mon,
+ const char *cmd_str,
+ char **result,
+ int timeout)
+{
+ int ret = -1;
+ int seconds;
+ virJSONValuePtr cmd;
+ virJSONValuePtr reply = NULL;
+
+ if (timeout < -1)
+ return ret;
+
+ cmd = virJSONValueFromString(cmd_str);
+ if (!cmd)
+ return ret;
+
+ if (result == NULL) {
+ seconds = VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT;
+ } else if (timeout == VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) {
+ seconds = QEMU_AGENT_WAIT_DEFAULT;
+ } else {
+ seconds = timeout;
+ }
+
+repeat:
+ ret = qemuAgentCommand(mon, cmd, &reply, seconds);
+
+ if (ret == 0) {
+ ret = qemuAgentCheckError(cmd, reply);
+ *result = virJSONValueToString(reply);
+ } else {
+ if (timeout == VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) {
+ virJSONValueFree(reply);
+ goto repeat;
+ }
+ *result = NULL;
+ }
+
+ virJSONValueFree(cmd);
+ virJSONValueFree(reply);
+ return ret;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 2fdebb2..fc19c2f 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -77,4 +77,9 @@ int qemuAgentFSThaw(qemuAgentPtr mon);
int qemuAgentSuspend(qemuAgentPtr mon,
unsigned int target);
+
+int qemuAgentArbitraryCommand(qemuAgentPtr mon,
+ const char *cmd,
+ char **result,
+ int timeout);
#endif /* __QEMU_AGENT_H__ */
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list