[Qemu-devel] [PATCH 0/2] qemu_agent: Switch to event driven impl

[Qemu-devel] [PATCH 0/2] qemu_agent: Switch to event driven impl

[Michal Privoznik]

Qemu fixed some qemu-ga commands reporting bogus success:

   http://www.mail-archive.com/qemu-devel@nongnu.org/msg110261.html

Therefore we need an implementation which won't wait for qemu-ga to reply and
hence block indefinitely. Fortunately, we can take events on qemu monitor as
indication of success.

Michal Privoznik (2):
  qemu_agent: Add some more debug prints
  qemu_agent: Wait for events instead of agent response

 src/qemu/qemu_agent.c   |   52 +++++++++++++++++++++++++++++++++++++++++-----
 src/qemu/qemu_agent.h   |    9 ++++++++
 src/qemu/qemu_process.c |    9 ++++++++
 3 files changed, 64 insertions(+), 6 deletions(-)

-- 
1.7.8.5

[Qemu-devel] [PATCH 1/2] qemu_agent: Add some more debug prints

[Michal Privoznik]

for agent ref count and qemuProcessHandleAgentDestroy
---
 src/qemu/qemu_agent.c   |    3 ++-
 src/qemu/qemu_process.c |    2 ++
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index bc4ceff..36610c2 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -155,13 +155,14 @@ static void qemuAgentFree(qemuAgentPtr mon)
 int qemuAgentRef(qemuAgentPtr mon)
 {
     mon->refs++;
+    VIR_DEBUG("%d", mon->refs);
     return mon->refs;
 }
 
 int qemuAgentUnref(qemuAgentPtr mon)
 {
     mon->refs--;
-
+    VIR_DEBUG("%d", mon->refs);
     if (mon->refs == 0) {
         qemuAgentUnlock(mon);
         qemuAgentFree(mon);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 544eed5..2b47bf6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -167,6 +167,8 @@ static void qemuProcessHandleAgentDestroy(qemuAgentPtr agent,
 {
     qemuDomainObjPrivatePtr priv;
 
+    VIR_DEBUG("Received destroy agent=%p vm=%p", agent, vm);
+
     virDomainObjLock(vm);
     priv = vm->privateData;
     if (priv->agent == agent)
-- 
1.7.8.5

Re: [Qemu-devel] [libvirt] [PATCH 1/2] qemu_agent: Add some more debug prints

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

On 06/15/2012 10:10 AM, Michal Privoznik wrote:
> for agent ref count and qemuProcessHandleAgentDestroy
> ---
>  src/qemu/qemu_agent.c   |    3 ++-
>  src/qemu/qemu_process.c |    2 ++
>  2 files changed, 4 insertions(+), 1 deletions(-)

ACK.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[Qemu-devel] [PATCH 2/2] qemu_agent: Wait for events instead of agent response

[Michal Privoznik]

With latest changes to qemu-ga success on some commands is not reported
anymore, e.g. guest-shutdown or guest-suspend-*. However, errors are
still being reported. Therefore, we need to find different source of
indication if operation was successful. Events.
---
 src/qemu/qemu_agent.c   |   49 ++++++++++++++++++++++++++++++++++++++++++----
 src/qemu/qemu_agent.h   |    9 ++++++++
 src/qemu/qemu_process.c |    7 ++++++
 3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 36610c2..95aec67 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -107,6 +107,11 @@ struct _qemuAgent {
     /* If anything went wrong, this will be fed back
      * the next monitor msg */
     virError lastError;
+
+    /* Some guest agent commands don't return anything
+     * but fire up an event on qemu monitor instead.
+     * Take that as indication of successful completion */
+    qemuAgentEvent await_event;
 };
 
 #if DEBUG_RAW_IO
@@ -826,6 +831,13 @@ void qemuAgentClose(qemuAgentPtr mon)
         VIR_FORCE_CLOSE(mon->fd);
     }
 
+    /* If there is somebody waiting for a message
+     * wake him up. No message will arrive anyway. */
+    if (mon->msg && !mon->msg->finished) {
+        mon->msg->finished = 1;
+        virCondSignal(&mon->notify);
+    }
+
     if (qemuAgentUnref(mon) > 0)
         qemuAgentUnlock(mon);
 }
@@ -982,6 +994,7 @@ qemuAgentCommand(qemuAgentPtr mon,
     int ret = -1;
     qemuAgentMessage msg;
     char *cmdstr = NULL;
+    int await_event = mon->await_event;
 
     *reply = NULL;
 
@@ -1010,10 +1023,16 @@ qemuAgentCommand(qemuAgentPtr mon,
               ret, msg.rxObject);
 
     if (ret == 0) {
+        /* If we haven't obtained any reply but we wait for an
+         * event, then don't report this as error */
         if (!msg.rxObject) {
-            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                            _("Missing monitor reply object"));
-            ret = -1;
+            if (await_event) {
+                VIR_DEBUG("Woken up by event %d", await_event);
+            } else {
+                qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                                _("Missing monitor reply object"));
+                ret = -1;
+            }
         } else {
             *reply = msg.rxObject;
         }
@@ -1238,6 +1257,24 @@ error:
     return NULL;
 }
 
+void qemuAgentNotifyEvent(qemuAgentPtr mon,
+                          qemuAgentEvent event)
+{
+    VIR_DEBUG("mon=%p event=%d", mon, event);
+    if (mon->await_event == event) {
+        VIR_DEBUG("Wakening up a tragedian");
+        mon->await_event = QEMU_AGENT_EVENT_NONE;
+        /* somebody waiting for this event, wake him up. */
+        if (mon->msg && !mon->msg->finished) {
+            mon->msg->finished = 1;
+            virCondSignal(&mon->notify);
+        }
+    } else {
+        /* shouldn't happen but one never knows */
+        VIR_WARN("Received unexpected event %d", event);
+    }
+}
+
 VIR_ENUM_DECL(qemuAgentShutdownMode);
 
 VIR_ENUM_IMPL(qemuAgentShutdownMode,
@@ -1257,9 +1294,10 @@ int qemuAgentShutdown(qemuAgentPtr mon,
     if (!cmd)
         return -1;
 
+    mon->await_event = QEMU_AGENT_EVENT_SHUTDOWN;
     ret = qemuAgentCommand(mon, cmd, &reply);
 
-    if (ret == 0)
+    if (reply && ret == 0)
         ret = qemuAgentCheckError(cmd, reply);
 
     virJSONValueFree(cmd);
@@ -1362,9 +1400,10 @@ qemuAgentSuspend(qemuAgentPtr mon,
     if (!cmd)
         return -1;
 
+    mon->await_event = QEMU_AGENT_EVENT_SUSPEND;
     ret = qemuAgentCommand(mon, cmd, &reply);
 
-    if (ret == 0)
+    if (reply && ret == 0)
         ret = qemuAgentCheckError(cmd, reply);
 
     virJSONValueFree(cmd);
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 98c23b0..0816d90 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -56,6 +56,15 @@ int qemuAgentUnref(qemuAgentPtr mon) ATTRIBUTE_RETURN_CHECK;
 void qemuAgentClose(qemuAgentPtr mon);
 
 typedef enum {
+    QEMU_AGENT_EVENT_NONE = 0,
+    QEMU_AGENT_EVENT_SHUTDOWN,
+    QEMU_AGENT_EVENT_SUSPEND
+} qemuAgentEvent;
+
+void qemuAgentNotifyEvent(qemuAgentPtr mon,
+                          qemuAgentEvent event);
+
+typedef enum {
     QEMU_AGENT_SHUTDOWN_POWERDOWN,
     QEMU_AGENT_SHUTDOWN_REBOOT,
     QEMU_AGENT_SHUTDOWN_HALT,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 2b47bf6..1df3637 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -665,6 +665,9 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
                  vm->def->name);
     }
 
+    if (priv->agent)
+        qemuAgentNotifyEvent(priv->agent, QEMU_AGENT_EVENT_SHUTDOWN);
+
     qemuProcessShutdownOrReboot(driver, vm);
 
 unlock:
@@ -1118,6 +1121,7 @@ qemuProcessHandlePMSuspend(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
     event = virDomainEventPMSuspendNewFromObj(vm);
 
     if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
+        qemuDomainObjPrivatePtr priv = vm->privateData;
         VIR_DEBUG("Transitioned guest %s to pmsuspended state due to "
                   "QMP suspend event", vm->def->name);
 
@@ -1128,6 +1132,9 @@ qemuProcessHandlePMSuspend(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
             VIR_WARN("Unable to save status on vm %s after suspend event",
                      vm->def->name);
         }
+
+        if (priv->agent)
+            qemuAgentNotifyEvent(priv->agent, QEMU_AGENT_EVENT_SUSPEND);
     }
 
     virDomainObjUnlock(vm);
-- 
1.7.8.5

Re: [Qemu-devel] [libvirt] [PATCH 2/2] qemu_agent: Wait for events instead of agent response

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

On 06/15/2012 10:10 AM, Michal Privoznik wrote:
> With latest changes to qemu-ga success on some commands is not reported
> anymore, e.g. guest-shutdown or guest-suspend-*. However, errors are
> still being reported. Therefore, we need to find different source of
> indication if operation was successful. Events.
> ---
>  src/qemu/qemu_agent.c   |   49 ++++++++++++++++++++++++++++++++++++++++++----
>  src/qemu/qemu_agent.h   |    9 ++++++++
>  src/qemu/qemu_process.c |    7 ++++++
>  3 files changed, 60 insertions(+), 5 deletions(-)
> 

> @@ -1238,6 +1257,24 @@ error:
>      return NULL;
>  }
>  
> +void qemuAgentNotifyEvent(qemuAgentPtr mon,
> +                          qemuAgentEvent event)
> +{
> +    VIR_DEBUG("mon=%p event=%d", mon, event);
> +    if (mon->await_event == event) {
> +        VIR_DEBUG("Wakening up a tragedian");

s/Wakening/Waking/

ACK.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [libvirt] [PATCH 2/2] qemu_agent: Wait for events instead of agent response

[Michal Privoznik]

On 15.06.2012 19:16, Eric Blake wrote:
> On 06/15/2012 10:10 AM, Michal Privoznik wrote:
>> With latest changes to qemu-ga success on some commands is not reported
>> anymore, e.g. guest-shutdown or guest-suspend-*. However, errors are
>> still being reported. Therefore, we need to find different source of
>> indication if operation was successful. Events.
>> ---
>>  src/qemu/qemu_agent.c   |   49 ++++++++++++++++++++++++++++++++++++++++++----
>>  src/qemu/qemu_agent.h   |    9 ++++++++
>>  src/qemu/qemu_process.c |    7 ++++++
>>  3 files changed, 60 insertions(+), 5 deletions(-)
>>
> 
>> @@ -1238,6 +1257,24 @@ error:
>>      return NULL;
>>  }
>>  
>> +void qemuAgentNotifyEvent(qemuAgentPtr mon,
>> +                          qemuAgentEvent event)
>> +{
>> +    VIR_DEBUG("mon=%p event=%d", mon, event);
>> +    if (mon->await_event == event) {
>> +        VIR_DEBUG("Wakening up a tragedian");
> 
> s/Wakening/Waking/
> 
> ACK.
> 
> 

Fixed and pushed. Thanks.

Michal