[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/5] add qemuAgentCommand()



(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__ */


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]