[Qemu-devel] [PATCH] QMP: Save default control monitor for emitting async events

[Qemu-devel] [PATCH] QMP: Save default control monitor for emitting async events

[Adam Litke]

When using a control/QMP monitor in tandem with a regular monitor, asynchronous
messages can get lost depending on the order of the QEMU program arguments.
QEMU events issued by monitor_protocol_event() always go to cur_mon.  If the
user monitor was specified on the command line first (or it has ,default), the
message will be directed to the user monitor (not the QMP monitor).

One solution is to save the default QMP session in another monitor pointer (ala
cur_mon) and always direct asynchronous events to that monitor...

Signed-off-by: Adam Litke <agl@us.ibm.com>

diff --git a/monitor.c b/monitor.c
index 134ed15..794f6ba 100644
--- a/monitor.c
+++ b/monitor.c
@@ -128,6 +128,7 @@ static const mon_cmd_t mon_cmds[];
 static const mon_cmd_t info_cmds[];
 
 Monitor *cur_mon = NULL;
+Monitor *cur_mon_control = NULL;
 
 static void monitor_command_cb(Monitor *mon, const char *cmdline,
                                void *opaque);
@@ -334,11 +335,11 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
 {
     QDict *qmp;
     const char *event_name;
-    Monitor *mon = cur_mon;
+    Monitor *mon = cur_mon_control;
 
     assert(event < QEVENT_MAX);
 
-    if (!monitor_ctrl_mode(mon))
+    if (!mon)
         return;
 
     switch (event) {
@@ -4283,6 +4284,9 @@ void monitor_init(CharDriverState *chr, int flags)
     QLIST_INSERT_HEAD(&mon_list, mon, entry);
     if (!cur_mon || (flags & MONITOR_IS_DEFAULT))
         cur_mon = mon;
+    if (!cur_mon_control || (flags & MONITOR_IS_DEFAULT))
+        if (flags & MONITOR_USE_CONTROL)
+            cur_mon_control = mon;
 }
 
 static void bdrv_password_cb(Monitor *mon, const char *password, void *opaque)
diff --git a/monitor.h b/monitor.h
index 556239a..a343589 100644
--- a/monitor.h
+++ b/monitor.h
@@ -7,6 +7,7 @@
 #include "block.h"
 
 extern Monitor *cur_mon;
+extern Monitor *cur_mon_control;
 
 /* flags for monitor_init */
 #define MONITOR_IS_DEFAULT    0x01
diff --git a/qemu-tool.c b/qemu-tool.c
index 18b48af..cfe03d6 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -34,6 +34,7 @@ void qemu_service_io(void)
 }
 
 Monitor *cur_mon;
+Monitor *cur_mon_control;
 
 void monitor_printf(Monitor *mon, const char *fmt, ...)
 {


-- 
Thanks,
Adam

[Qemu-devel] Re: [PATCH] QMP: Save default control monitor for emitting async events

[Luiz Capitulino]

On Thu, 14 Jan 2010 15:20:10 -0600
Adam Litke <agl@us.ibm.com> wrote:

> When using a control/QMP monitor in tandem with a regular monitor, asynchronous
> messages can get lost depending on the order of the QEMU program arguments.
> QEMU events issued by monitor_protocol_event() always go to cur_mon.  If the
> user monitor was specified on the command line first (or it has ,default), the
> message will be directed to the user monitor (not the QMP monitor).

 I think we have two problems here:

1. Async messages are only delivered for the default Monitor, so if the QMP
Monitor is not the default one it won't get them (not a bug if well
documented, but it's annoying)

2. On a multiple QMP Monitor setup, only one of them will get async messages

 This patch fixes 1. but the best fix would be to solve both problems,
as QMP Monitors have to be equally capable IMO.

 There's an array with all Monitors IIRC, maybe we could loop through it
and delivery the message to each QMP ones of them?

> One solution is to save the default QMP session in another monitor pointer (ala
> cur_mon) and always direct asynchronous events to that monitor...
> 
> Signed-off-by: Adam Litke <agl@us.ibm.com>
> 
> diff --git a/monitor.c b/monitor.c
> index 134ed15..794f6ba 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -128,6 +128,7 @@ static const mon_cmd_t mon_cmds[];
>  static const mon_cmd_t info_cmds[];
>  
>  Monitor *cur_mon = NULL;
> +Monitor *cur_mon_control = NULL;
>  
>  static void monitor_command_cb(Monitor *mon, const char *cmdline,
>                                 void *opaque);
> @@ -334,11 +335,11 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>  {
>      QDict *qmp;
>      const char *event_name;
> -    Monitor *mon = cur_mon;
> +    Monitor *mon = cur_mon_control;
>  
>      assert(event < QEVENT_MAX);
>  
> -    if (!monitor_ctrl_mode(mon))
> +    if (!mon)
>          return;
>  
>      switch (event) {
> @@ -4283,6 +4284,9 @@ void monitor_init(CharDriverState *chr, int flags)
>      QLIST_INSERT_HEAD(&mon_list, mon, entry);
>      if (!cur_mon || (flags & MONITOR_IS_DEFAULT))
>          cur_mon = mon;
> +    if (!cur_mon_control || (flags & MONITOR_IS_DEFAULT))
> +        if (flags & MONITOR_USE_CONTROL)
> +            cur_mon_control = mon;
>  }
>  
>  static void bdrv_password_cb(Monitor *mon, const char *password, void *opaque)
> diff --git a/monitor.h b/monitor.h
> index 556239a..a343589 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -7,6 +7,7 @@
>  #include "block.h"
>  
>  extern Monitor *cur_mon;
> +extern Monitor *cur_mon_control;
>  
>  /* flags for monitor_init */
>  #define MONITOR_IS_DEFAULT    0x01
> diff --git a/qemu-tool.c b/qemu-tool.c
> index 18b48af..cfe03d6 100644
> --- a/qemu-tool.c
> +++ b/qemu-tool.c
> @@ -34,6 +34,7 @@ void qemu_service_io(void)
>  }
>  
>  Monitor *cur_mon;
> +Monitor *cur_mon_control;
>  
>  void monitor_printf(Monitor *mon, const char *fmt, ...)
>  {
> 
> 

[Qemu-devel] Re: [PATCH] QMP: Save default control monitor for emitting async events

[Adam Litke]

On Fri, 2010-01-15 at 11:38 -0200, Luiz Capitulino wrote:
> On Thu, 14 Jan 2010 15:20:10 -0600
> Adam Litke <agl@us.ibm.com> wrote:
> 
> > When using a control/QMP monitor in tandem with a regular monitor, asynchronous
> > messages can get lost depending on the order of the QEMU program arguments.
> > QEMU events issued by monitor_protocol_event() always go to cur_mon.  If the
> > user monitor was specified on the command line first (or it has ,default), the
> > message will be directed to the user monitor (not the QMP monitor).
> 
>  I think we have two problems here:
> 
> 1. Async messages are only delivered for the default Monitor, so if the QMP
> Monitor is not the default one it won't get them (not a bug if well
> documented, but it's annoying)
> 
> 2. On a multiple QMP Monitor setup, only one of them will get async messages
> 
>  This patch fixes 1. but the best fix would be to solve both problems,
> as QMP Monitors have to be equally capable IMO.
> 
>  There's an array with all Monitors IIRC, maybe we could loop through it
> and delivery the message to each QMP ones of them?

Sure.  This was the other way I was considering.  I'll spin up a patch
and we can see how it looks.


-- 
Thanks,
Adam

[Qemu-devel] [PATCH] QMP: Emit asynchronous events on all QMP monitors

[Adam Litke]

When using a control/QMP monitor in tandem with a regular monitor, asynchronous
messages can get lost depending on the order of the QEMU program arguments.
QEMU events issued by monitor_protocol_event() always go to cur_mon.  If the
user monitor was specified on the command line first (or it has ,default), the
message will be directed to the user monitor (not the QMP monitor).
Additionally, only one QMP session is currently able to receive async messages.

To avoid this confusion, scan through the list of monitors and emit the message
on each QMP monitor.

Signed-off-by: Adam Litke <agl@us.ibm.com>

diff --git a/monitor.c b/monitor.c
index 134ed15..06c8bf0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -334,13 +334,10 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
 {
     QDict *qmp;
     const char *event_name;
-    Monitor *mon = cur_mon;
+    Monitor *mon;
 
     assert(event < QEVENT_MAX);
 
-    if (!monitor_ctrl_mode(mon))
-        return;
-
     switch (event) {
         case QEVENT_DEBUG:
             event_name = "DEBUG";
@@ -373,7 +370,12 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         qdict_put_obj(qmp, "data", data);
     }
 
-    monitor_json_emitter(mon, QOBJECT(qmp));
+    QLIST_FOREACH(mon, &mon_list, entry) {
+        if (!monitor_ctrl_mode(mon))
+            return;
+
+        monitor_json_emitter(mon, QOBJECT(qmp));
+    }
     QDECREF(qmp);
 }
 


-- 
Thanks,
Adam

[Qemu-devel] Re: [PATCH] QMP: Emit asynchronous events on all QMP monitors

[Luiz Capitulino]

On Fri, 15 Jan 2010 08:34:02 -0600
Adam Litke <agl@us.ibm.com> wrote:

> When using a control/QMP monitor in tandem with a regular monitor, asynchronous
> messages can get lost depending on the order of the QEMU program arguments.
> QEMU events issued by monitor_protocol_event() always go to cur_mon.  If the
> user monitor was specified on the command line first (or it has ,default), the
> message will be directed to the user monitor (not the QMP monitor).
> Additionally, only one QMP session is currently able to receive async messages.
> 
> To avoid this confusion, scan through the list of monitors and emit the message
> on each QMP monitor.
> 
> Signed-off-by: Adam Litke <agl@us.ibm.com>
> 
> diff --git a/monitor.c b/monitor.c
> index 134ed15..06c8bf0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -334,13 +334,10 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>  {
>      QDict *qmp;
>      const char *event_name;
> -    Monitor *mon = cur_mon;
> +    Monitor *mon;
>  
>      assert(event < QEVENT_MAX);
>  
> -    if (!monitor_ctrl_mode(mon))
> -        return;
> -
>      switch (event) {
>          case QEVENT_DEBUG:
>              event_name = "DEBUG";
> @@ -373,7 +370,12 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>          qdict_put_obj(qmp, "data", data);
>      }
>  
> -    monitor_json_emitter(mon, QOBJECT(qmp));
> +    QLIST_FOREACH(mon, &mon_list, entry) {
> +        if (!monitor_ctrl_mode(mon))
> +            return;
> +
> +        monitor_json_emitter(mon, QOBJECT(qmp));
> +    }

 The function will return on the first !QMP Monitor, the
right QLIST_FOREACH() body is:

if (monitor_ctrl_mode(mon)) {
 monitor_json_emitter(mon, QOBJECT(qmp));
}

 I'll ACK the respin.

>      QDECREF(qmp);
>  }
>  
> 
> 

[Qemu-devel] Re: [PATCH][RESPIN] QMP: Emit asynchronous events on all QMP monitors

[Adam Litke]

On Fri, 2010-01-15 at 13:00 -0200, Luiz Capitulino wrote:
>  The function will return on the first !QMP Monitor, the
> right QLIST_FOREACH() body is:
> 
> if (monitor_ctrl_mode(mon)) {
>  monitor_json_emitter(mon, QOBJECT(qmp));
> }
> 
>  I'll ACK the respin.

Ah right, of course.  Thanks and here it is.

When using a control/QMP monitor in tandem with a regular monitor, asynchronous
messages can get lost depending on the order of the QEMU program arguments.
QEMU events issued by monitor_protocol_event() always go to cur_mon.  If the
user monitor was specified on the command line first (or it has ,default), the
message will be directed to the user monitor (not the QMP monitor).
Additionally, only one QMP session is currently able to receive async messages.

To avoid this confusion, scan through the list of monitors and emit the message
on each QMP monitor.

Signed-off-by: Adam Litke <agl@us.ibm.com>
Acked-by: Luiz Capitulino <lcapitulino@redhat.com>

diff --git a/monitor.c b/monitor.c
index 134ed15..6a2c7fb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -334,13 +334,10 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
 {
     QDict *qmp;
     const char *event_name;
-    Monitor *mon = cur_mon;
+    Monitor *mon;
 
     assert(event < QEVENT_MAX);
 
-    if (!monitor_ctrl_mode(mon))
-        return;
-
     switch (event) {
         case QEVENT_DEBUG:
             event_name = "DEBUG";
@@ -373,7 +370,11 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         qdict_put_obj(qmp, "data", data);
     }
 
-    monitor_json_emitter(mon, QOBJECT(qmp));
+    QLIST_FOREACH(mon, &mon_list, entry) {
+        if (monitor_ctrl_mode(mon)) {
+            monitor_json_emitter(mon, QOBJECT(qmp));
+        }
+    }
     QDECREF(qmp);
 }
 


-- 
Thanks,
Adam

[Qemu-devel] Re: [PATCH][RESPIN] QMP: Emit asynchronous events on all QMP monitors

[Luiz Capitulino]

On Fri, 15 Jan 2010 09:16:03 -0600
Adam Litke <agl@us.ibm.com> wrote:

> On Fri, 2010-01-15 at 13:00 -0200, Luiz Capitulino wrote:
> >  The function will return on the first !QMP Monitor, the
> > right QLIST_FOREACH() body is:
> > 
> > if (monitor_ctrl_mode(mon)) {
> >  monitor_json_emitter(mon, QOBJECT(qmp));
> > }
> > 
> >  I'll ACK the respin.
> 
> Ah right, of course.  Thanks and here it is.
> 
> When using a control/QMP monitor in tandem with a regular monitor, asynchronous
> messages can get lost depending on the order of the QEMU program arguments.
> QEMU events issued by monitor_protocol_event() always go to cur_mon.  If the
> user monitor was specified on the command line first (or it has ,default), the
> message will be directed to the user monitor (not the QMP monitor).
> Additionally, only one QMP session is currently able to receive async messages.
> 
> To avoid this confusion, scan through the list of monitors and emit the message
> on each QMP monitor.
> 
> Signed-off-by: Adam Litke <agl@us.ibm.com>
> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>

 Now it's good, hope Anthony's script can get a patch from a thread,
this is probably good for stable too.

 Btw, we have a lot of QMP tasks in case you're interested :-)

> 
> diff --git a/monitor.c b/monitor.c
> index 134ed15..6a2c7fb 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -334,13 +334,10 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>  {
>      QDict *qmp;
>      const char *event_name;
> -    Monitor *mon = cur_mon;
> +    Monitor *mon;
>  
>      assert(event < QEVENT_MAX);
>  
> -    if (!monitor_ctrl_mode(mon))
> -        return;
> -
>      switch (event) {
>          case QEVENT_DEBUG:
>              event_name = "DEBUG";
> @@ -373,7 +370,11 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>          qdict_put_obj(qmp, "data", data);
>      }
>  
> -    monitor_json_emitter(mon, QOBJECT(qmp));
> +    QLIST_FOREACH(mon, &mon_list, entry) {
> +        if (monitor_ctrl_mode(mon)) {
> +            monitor_json_emitter(mon, QOBJECT(qmp));
> +        }
> +    }
>      QDECREF(qmp);
>  }
>  
> 
> 

Re: [Qemu-devel] [PATCH] QMP: Emit asynchronous events on all QMP monitors

[Anthony Liguori]

On 01/15/2010 08:34 AM, Adam Litke wrote:
> When using a control/QMP monitor in tandem with a regular monitor, asynchronous
> messages can get lost depending on the order of the QEMU program arguments.
> QEMU events issued by monitor_protocol_event() always go to cur_mon.  If the
> user monitor was specified on the command line first (or it has ,default), the
> message will be directed to the user monitor (not the QMP monitor).
> Additionally, only one QMP session is currently able to receive async messages.
>
> To avoid this confusion, scan through the list of monitors and emit the message
> on each QMP monitor.
>
> Signed-off-by: Adam Litke<agl@us.ibm.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> diff --git a/monitor.c b/monitor.c
> index 134ed15..06c8bf0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -334,13 +334,10 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>   {
>       QDict *qmp;
>       const char *event_name;
> -    Monitor *mon = cur_mon;
> +    Monitor *mon;
>
>       assert(event<  QEVENT_MAX);
>
> -    if (!monitor_ctrl_mode(mon))
> -        return;
> -
>       switch (event) {
>           case QEVENT_DEBUG:
>               event_name = "DEBUG";
> @@ -373,7 +370,12 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>           qdict_put_obj(qmp, "data", data);
>       }
>
> -    monitor_json_emitter(mon, QOBJECT(qmp));
> +    QLIST_FOREACH(mon,&mon_list, entry) {
> +        if (!monitor_ctrl_mode(mon))
> +            return;
> +
> +        monitor_json_emitter(mon, QOBJECT(qmp));
> +    }
>       QDECREF(qmp);
>   }
>
>
>
>