QMP: out-of-band (OOB) execution support

Update both the developer and spec for the new QMP OOB (Out-Of-Band) command.

Signed-off-by Peter Xu <[email protected]>

---
  docs/devel/qapi-code-gen.txt | 65 ++++++++++++++++++++++++++++++++++++++++----
  docs/interop/qmp-spec.txt    | 30 +++++++++++++++++---
  2 files changed, 86 insertions(+), 9 deletions(-)

+"control" field in the request, with "run-oob" set to true. Example:

docs/devel/qapi-code-gen.txt +3 -0

+
+ => { "execute": "command-support-oob",
+      "arguments": { ... },

+declared to support Out-Of-Band execution when 'allow-oob' field is

+set to true when defining the command. With that, it can be run in an

docs/devel/qapi-code-gen.txt +4 -0

+Out-Of-Band way if 'run-oob' is specified in 'control' field of
+command request.
+
+Under normal QMP command execution, the following apply to each

+- They do not take the BQL during execution.

docs/devel/qapi-code-gen.txt +11 -0

+
+OOB command handlers must satisfy the following conditions:
+
+- It executes extremely fast,
+- It does not take any lock, or, it can take very small locks if all
+  critical regions also follow the rules for OOB command handler code,
+- It does not invoke system calls that may block,
+- It does not access guest RAM that may block when userfaultfd is
+  enabled for postcopy live migration.
+
+If in doubt, do not implement OOB execution support.
 
  === Events ===

@@ -739,10 +792,12 @@ references by name.
  QAPI schema definitions not reachable that way are omitted.
 
  The SchemaInfo for a command has meta-type "command", and variant

docs/devel/qapi-code-gen.txt +0 -0

  If the command takes no arguments, "arg-type" names an object type
  without members.  Likewise, if the command returns nothing, "ret-type"

docs/interop/qmp-spec.txt +0 -0

index f8b5356015..9a208589f6 100644

docs/interop/qmp-spec.txt +8 -0

+  "Issuing Commands" section below.  Not all commands allow this "oob"
+  execution.  The "query-qmp-schema" command can be used to inspect
+  which commands support "oob" execution.
+
+QMP clients can get a list of supported QMP capabilities of the QMP
+server in the greeting message mentioned above.  By default, all the
+capabilities are off.  To enable any QMP capabilities, the QMP client
+needs to send the "qmp_capabilities" command with an extra parameter

docs/interop/qmp-spec.txt +3 -0

+  response of an "oob" command can overtake prior in-band commands.
+  To enable "oob" handling of a particular command, just provide a
+  control field with: { "control": { "run-oob": true } }
 
  2.4 Commands Responses
  ----------------------

Quoted 8 lines · Peter Xu

Update both the developer and spec for the new QMP OOB (Out-Of-Band) command.

Signed-off-by Peter Xu <[email protected]>

---
  docs/devel/qapi-code-gen.txt | 65 ++++++++++++++++++++++++++++++++++++++++----
  docs/interop/qmp-spec.txt    | 30 +++++++++++++++++---
  2 files changed, 86 insertions(+), 9 deletions(-)

If all goes well, I'm planning on taking this series through my QAPI tree, with a pull request either late today or early Monday in order to make the soft freeze deadline. We'll see what my review turns up, but hopefully at this point it's either clean or minor enough tweaks that I can polish it without a v9.

Quoted 6 lines

docs/interop/qmp-spec.txt +1 -1

@@ -102,10 +113,16 @@ The format for command execution is:
    required. Each command documents what contents will be considered
    valid when handling the json-argument
  - The "id" member is a transaction identification associated with the
-  command execution, it is optional and will be part of the response if
+  command execution.  It is required if OOB is enabled, and optional

docs/interop/qmp-spec.txt The format for command execution is: · line 117

Quoted 1 line

docs/interop/qmp-spec.txt +1 -0

+  if not.  The same "id" field will be part of the response if

Ambiguous on whether this is the per-command 'run-oob', or whether this is the generic QMP capability requested during qmp_capabilities. I think the intent is that if you enabled the QMP capability up front, ALL commands must have an "id", even if they are run in-band without 'run-oob', because the 'id' in the response is what will distinguish whether a later OOB request overtook a pending in-band reply.

docs/interop/qmp-spec.txt The format for command execution is: · line 122

Quoted 6 lines

docs/interop/qmp-spec.txt +2 -0

    provided. The "id" member can be any json-value, although most
    clients merely use a json-number incremented for each successive
    command
+- The "control" member is optional, and currently only used for
+  "out-of-band" execution ("oob" as shortcut). The handling or

A bit late to be introducing "oob" as shortcut, given that you've already used the abbreviation oob earlier in the document.

docs/interop/qmp-spec.txt The format for command execution is: · line 135

Quoted 12 lines

docs/interop/qmp-spec.txt +7 -0

+  response of an "oob" command can overtake prior in-band commands.
+  To enable "oob" handling of a particular command, just provide a
+  control field with: { "control": { "run-oob": true } }
  2.4 Commands Responses
  ----------------------

@@ -113,6 +130,11 @@ The format for command execution is:
  There are two possible responses which the Server will issue as the result
  of a command execution: success or error.
+As long as the commands were issued with a proper "id" field, then the
+same "id" field will be attached in the corresponding response message
+so that requests and responses can match.  Clients should drop all the
+responses that are with unknown "id" field.

s/are with/have an/

I've got quite a few wording tweaks, but the general concept is good. If you need to respin for other reasons, feel free to make those improvements, but I also don't mind making tweaks myself as part of queuing for a pull request, so:

Reviewed-by Eric Blake <[email protected]>

A quick way to fetch string from qobject when it's a QString.

Reviewed-by Fam Zheng <[email protected]>

Reviewed-by Stefan Hajnoczi <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
  include/qapi/qmp/qstring.h |  1 +
  qobject/qstring.c          | 11 +++++++++++
  2 files changed, 12 insertions(+)

+++ b/qobject/qstring.c

qobject/qstring.c +0 -0

@@ -137,6 +137,17 @@ const char *qstring_get_try_str(const QString *qstring)
      return qstring ? qstring_get_str(qstring) : NULL;
  }

In monitor_qmp_read(), we have the hack to temporarily replace the cur_mon pointer. Now we move this hack deeper inside the QMP dispatcher routine since the Monitor pointer can be actually obtained using container_of() upon the parser object, just like most of the other JSON parser users do.

This does not make much sense as a single patch. However, this will be a big step for the next patch, when the QMP dispatcher routine will be split from the QMP parser.

Reviewed-by Stefan Hajnoczi <[email protected]>

Reviewed-by Eric Blake <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
  monitor.c | 17 ++++++++++-------
  1 file changed, 10 insertions(+), 7 deletions(-)

For each Monitor, add one field "use_io_thr" to show whether it will be using the dedicated monitor IO thread to handle input/output. When set, monitor IO parsing work will be offloaded to the dedicated monitor IO thread, rather than the original main loop thread.

This only works for QMP. HMP will always be run on the main loop thread.

Currently we're still keeping use_io_thr off always. Will turn it on later at some point.

One thing to mention is that we cannot set use_io_thr for every QMP monitor. The problem is that MUXed typed chardevs may not work well with it now. When MUX is used, frontend of chardev can be the monitor plus something else. The only thing we know would be safe to be run outside main thread so far is the monitor frontend. All the rest of the frontends should still be run in main thread only.

Signed-off-by Peter Xu <[email protected]>

---
  monitor.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
  1 file changed, 90 insertions(+), 8 deletions(-)

There were no QMP capabilities defined. Define the first "oob" as capability to allow out-of-band messages.

After this patch, we will allow QMP clients to enable QMP capabilities when sending the first "qmp_capabilities" command. Originally we are starting QMP session with no arguments like:

   { "execute": "qmp_capabilities" }

Now we can enable some QMP capabilities using (take OOB as example, which is the only one capability that we support):

{ "execute": "qmp_capabilities",

     "argument": { "enable": [ "oob" ] } }

When the "argument" key is not provided, no capability is enabled.

For capability "oob", the monitor needs to be run on dedicated IO

thread, otherwise the command will fail. For example, trying to enable OOB on a MUXed typed QMP monitor will fail.

One thing to mention is that, QMP capabilities are per-monitor, and also

when the connection is closed due to some reason, the capabilities will be reset.

Also, touch up qmp-test.c to test the new bits.

Signed-off-by Peter Xu <[email protected]>

---
  monitor.c        | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
  qapi/misc.json   | 32 ++++++++++++++++++++---
  tests/qmp-test.c | 10 +++++++-
  3 files changed, 110 insertions(+), 9 deletions(-)

A tiny refactoring, preparing to split the QMP dispatcher away.

Reviewed-by Fam Zheng <[email protected]>

Reviewed-by Stefan Hajnoczi <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
  monitor.c | 50 +++++++++++++++++++++++++++++++++-----------------
  1 file changed, 33 insertions(+), 17 deletions(-)

This patches allows QMP monitors to be suspended/resumed.

One thing to mention is that for QMPs that are using IOThreads, we need an explicit kick for the IOThread in case it is sleeping.

Meanwhile, we need to take special care on non-interactive HMPs. Currently only gdbserver is using that. For these monitors, we still don't allow suspend/resume operations.

Since at it, add traces for the operations.

Signed-off-by Peter Xu <[email protected]>

---
  monitor.c    | 47 ++++++++++++++++++++++++++++++++++++++++++++---
  trace-events |  1 +
  2 files changed, 45 insertions(+), 3 deletions(-)

Originally QMP goes through these steps:

JSON Parser --> QMP Dispatcher --> Respond

       /|\    (2)                (3)     |
    (1) |                               \|/ (4)
        +---------  main thread  --------+

This patch does this:

   JSON Parser     QMP Dispatcher --> Respond
       /|\ |           /|\       (4)     |
        |  | (2)        | (3)            |  (5)
    (1) |  +----->      |               \|/
        +---------  main thread  <-------+

So the parsing job and the dispatching job is isolated now. It gives us a chance in following up patches to totally move the parser outside.

The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is used for all the monitors.

Reviewed-by Stefan Hajnoczi <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
  monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
  1 file changed, 178 insertions(+), 23 deletions(-)

      if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
          QString *req_json = qobject_to_json(req);

+/*
+ * Pop one QMP request from monitor queues, return NULL if not found.
+ * We are using round-robin fashion to pop the request, to avoid
+ * processing command only on a very busy monitor. To achieve that,

+ * when we processed one request on specific monitor, we put that

+ * it is responded. This helps make sure that as long as OOB is

Originally QMP goes through these steps:

JSON Parser --> QMP Dispatcher --> Respond

      /|\    (2)                (3)     |
   (1) |                               \|/ (4)
       +---------  main thread  --------+

This patch does this:

JSON Parser QMP Dispatcher --> Respond

      /|\ |           /|\       (4)     |
       |  | (2)        | (3)            |  (5)
   (1) |  +----->      |               \|/
       +---------  main thread  <-------+

So the parsing job and the dispatching job is isolated now. It gives us a chance in following up patches to totally move the parser outside.

The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is used for all the monitors.

Reviewed-by Stefan Hajnoczi <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 178 insertions(+), 23 deletions(-)

monitor.c +145 -23

index de9343be87..5104e5db07 100644
@@ -172,6 +172,13 @@ typedef struct {
      */
     QmpCommandList *commands;
     bool qmp_caps[QMP_CAPABILITY__MAX];
+    /*
+     * Protects qmp request/response queue.  Please take monitor_lock
+     * first when used together.
+     */
+    QemuMutex qmp_queue_lock;
+    /* Input queue that holds all the parsed QMP requests */
+    GQueue *qmp_requests;
 } MonitorQMP;
 
 /*

@@ -218,6 +225,8 @@ struct Monitor {
 /* Let's add monitor global variables to this struct. */
 static struct {
     IOThread *mon_iothread;
+    /* Bottom half to dispatch the requests received from IO thread */
+    QEMUBH *qmp_dispatcher_bh;
 } mon_global;
 
 /* QMP checker flags */

@@ -600,11 +609,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
 {
     memset(mon, 0, sizeof(Monitor));
     qemu_mutex_init(&mon->out_lock);
+    qemu_mutex_init(&mon->qmp.qmp_queue_lock);
     mon->outbuf = qstring_new();
     /* Use *mon_cmds by default. */
     mon->cmd_table = mon_cmds;
     mon->skip_flush = skip_flush;
     mon->use_io_thr = use_io_thr;
+    mon->qmp.qmp_requests = g_queue_new();
 }
 
 static void monitor_data_destroy(Monitor *mon)

@@ -617,6 +628,8 @@ static void monitor_data_destroy(Monitor *mon)
     readline_free(mon->rs);
     QDECREF(mon->outbuf);
     qemu_mutex_destroy(&mon->out_lock);
+    qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
+    g_queue_free(mon->qmp.qmp_requests);
 }
 
 char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,

@@ -1056,6 +1069,16 @@ static void monitor_init_qmp_commands(void)
                          qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
 }
 
+static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap)
+{
+    return mon->qmp.qmp_caps[cap];
+}
+
+static bool qmp_oob_enabled(Monitor *mon)
+{
+    return qmp_cap_enabled(mon, QMP_CAPABILITY_OOB);
+}
+
 static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list,
                            Error **errp)
 {

@@ -3866,30 +3889,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
     qobject_decref(rsp);
 }
 
-static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+struct QMPRequest {
+    /* Owner of the request */
+    Monitor *mon;
+    /* "id" field of the request */
+    QObject *id;
+    /* Request object to be handled */
+    QObject *req;
+    /*
+     * Whether we need to resume the monitor afterward.  This flag is
+     * used to emulate the old QMP server behavior that the current
+     * command must be completed before execution of the next one.
+     */
+    bool need_resume;
+};
+typedef struct QMPRequest QMPRequest;
+
+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);
 
     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);

@@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     old_mon = cur_mon;
     cur_mon = mon;
 
-    rsp = qmp_dispatch(cur_mon->qmp.commands, req);
+    rsp = qmp_dispatch(mon->qmp.commands, req);
 
     cur_mon = old_mon;
 

@@ -3916,12 +3948,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         }
     }
 
-err_out:
-    monitor_qmp_respond(mon, rsp, err, id);
+    /* Respond if necessary */
+    monitor_qmp_respond(mon, rsp, NULL, id);
+
+    /* This pairs with the monitor_suspend() in handle_qmp_command(). */
+    if (need_resume) {
+        monitor_resume(mon);
+    }
 
     qobject_decref(req);
 }
 
+/*
+ * Pop one QMP request from monitor queues, return NULL if not found.
+ * We are using round-robin fashion to pop the request, to avoid
+ * processing command only on a very busy monitor.  To achieve that,
+ * when we processed one request on specific monitor, we put that
+ * monitor to the end of mon_list queue.
+ */
+static QMPRequest *monitor_qmp_requests_pop_one(void)
+{
+    QMPRequest *req_obj = NULL;
+    Monitor *mon;
+
+    qemu_mutex_lock(&monitor_lock);
+
+    QTAILQ_FOREACH(mon, &mon_list, entry) {
+        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+        req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
+        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        if (req_obj) {
+            break;
+        }
+    }
+
+    if (req_obj) {
+        /*
+         * We found one request on the monitor. Degrade this monitor's
+         * priority to lowest by re-inserting it to end of queue.
+         */
+        QTAILQ_REMOVE(&mon_list, mon, entry);
+        QTAILQ_INSERT_TAIL(&mon_list, mon, entry);
+    }
+
+    qemu_mutex_unlock(&monitor_lock);
+
+    return req_obj;
+}
+
+static void monitor_qmp_bh_dispatcher(void *data)
+{
+    QMPRequest *req_obj = monitor_qmp_requests_pop_one();
+
+    if (req_obj) {
+        monitor_qmp_dispatch_one(req_obj);
+        /* Reschedule instead of looping so the main loop stays responsive */
+        qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+    }
+}
+
+static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+{
+    QObject *req, *id = NULL;
+    QDict *qdict = NULL;
+    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
+    Monitor *mon = container_of(mon_qmp, Monitor, qmp);
+    Error *err = NULL;
+    QMPRequest *req_obj;
+
+    req = json_parser_parse_err(tokens, NULL, &err);
+    if (!req && !err) {
+        /* json_parser_parse_err() sucks: can fail without setting @err */
+        error_setg(&err, QERR_JSON_PARSING);
+    }
+    if (err) {
+        monitor_qmp_respond(mon, NULL, err, NULL);
+        qobject_decref(req);
+        return;
+    }
+
+    qdict = qobject_to_qdict(req);
+    if (qdict) {
+        id = qdict_get(qdict, "id");
+        qobject_incref(id);
+        qdict_del(qdict, "id");
+    } /* else will fail qmp_dispatch() */
+
+    req_obj = g_new0(QMPRequest, 1);
+    req_obj->mon = mon;
+    req_obj->id = id;
+    req_obj->req = req;
+    req_obj->need_resume = false;
+
+    /*
+     * If OOB is not enabled on current monitor, we'll emulate the old
+     * behavior that we won't process current monitor any more until
+     * it is responded.  This helps make sure that as long as OOB is

monitor.c +23 -1

+     * etc. will be delivered to the handler side.
+     */
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+
+    /* Kick the dispatcher routine */
+    qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+}
+
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
     Monitor *mon = opaque;

@@ -4134,6 +4276,15 @@ static void monitor_iothread_init(void)
 {
     mon_global.mon_iothread = iothread_create("mon_iothread",
                                               &error_abort);
+
+    /*
+     * This MUST be on main loop thread since we have commands that
+     * have assumption to be run on main loop thread.  It would be
+     * nice that one day we can remove this assumption in the future.
+     */
+    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
+                                              monitor_qmp_bh_dispatcher,
+                                              NULL);
 }
 
 void monitor_init_globals(void)

@@ -4280,6 +4431,10 @@ void monitor_cleanup(void)
     }
     qemu_mutex_unlock(&monitor_lock);
 
+    /* QEMUBHs needs to be deleted before destroying the IOThread. */
+    qemu_bh_delete(mon_global.qmp_dispatcher_bh);
+    mon_global.qmp_dispatcher_bh = NULL;
+
     iothread_destroy(mon_global.mon_iothread);
     mon_global.mon_iothread = NULL;
 }
--

2.14.3

Hi

Quoted 272 lines · Peter Xu

Originally QMP goes through these steps:

JSON Parser --> QMP Dispatcher --> Respond

      /|\    (2)                (3)     |
   (1) |                               \|/ (4)
       +---------  main thread  --------+

This patch does this:

JSON Parser QMP Dispatcher --> Respond

      /|\ |           /|\       (4)     |
       |  | (2)        | (3)            |  (5)
   (1) |  +----->      |               \|/
       +---------  main thread  <-------+

So the parsing job and the dispatching job is isolated now. It gives us a chance in following up patches to totally move the parser outside.

The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is used for all the monitors.

Reviewed-by Stefan Hajnoczi <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 178 insertions(+), 23 deletions(-)

monitor.c +152 -23

index de9343be87..5104e5db07 100644
@@ -172,6 +172,13 @@ typedef struct {
      */
     QmpCommandList *commands;
     bool qmp_caps[QMP_CAPABILITY__MAX];
+    /*
+     * Protects qmp request/response queue.  Please take monitor_lock
+     * first when used together.
+     */
+    QemuMutex qmp_queue_lock;
+    /* Input queue that holds all the parsed QMP requests */
+    GQueue *qmp_requests;
 } MonitorQMP;
 
 /*

@@ -218,6 +225,8 @@ struct Monitor {
 /* Let's add monitor global variables to this struct. */
 static struct {
     IOThread *mon_iothread;
+    /* Bottom half to dispatch the requests received from IO thread */
+    QEMUBH *qmp_dispatcher_bh;
 } mon_global;
 
 /* QMP checker flags */

@@ -600,11 +609,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
 {
     memset(mon, 0, sizeof(Monitor));
     qemu_mutex_init(&mon->out_lock);
+    qemu_mutex_init(&mon->qmp.qmp_queue_lock);
     mon->outbuf = qstring_new();
     /* Use *mon_cmds by default. */
     mon->cmd_table = mon_cmds;
     mon->skip_flush = skip_flush;
     mon->use_io_thr = use_io_thr;
+    mon->qmp.qmp_requests = g_queue_new();
 }
 
 static void monitor_data_destroy(Monitor *mon)

@@ -617,6 +628,8 @@ static void monitor_data_destroy(Monitor *mon)
     readline_free(mon->rs);
     QDECREF(mon->outbuf);
     qemu_mutex_destroy(&mon->out_lock);
+    qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
+    g_queue_free(mon->qmp.qmp_requests);
 }
 
 char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,

@@ -1056,6 +1069,16 @@ static void monitor_init_qmp_commands(void)
                          qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
 }
 
+static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap)
+{
+    return mon->qmp.qmp_caps[cap];
+}
+
+static bool qmp_oob_enabled(Monitor *mon)
+{
+    return qmp_cap_enabled(mon, QMP_CAPABILITY_OOB);
+}
+
 static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list,
                            Error **errp)
 {

@@ -3866,30 +3889,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
     qobject_decref(rsp);
 }
 
-static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+struct QMPRequest {
+    /* Owner of the request */
+    Monitor *mon;
+    /* "id" field of the request */
+    QObject *id;
+    /* Request object to be handled */
+    QObject *req;
+    /*
+     * Whether we need to resume the monitor afterward.  This flag is
+     * used to emulate the old QMP server behavior that the current
+     * command must be completed before execution of the next one.
+     */
+    bool need_resume;
+};
+typedef struct QMPRequest QMPRequest;
+
+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);
 
     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);

@@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     old_mon = cur_mon;
     cur_mon = mon;
 
-    rsp = qmp_dispatch(cur_mon->qmp.commands, req);
+    rsp = qmp_dispatch(mon->qmp.commands, req);
 
     cur_mon = old_mon;
 

@@ -3916,12 +3948,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         }
     }
 
-err_out:
-    monitor_qmp_respond(mon, rsp, err, id);
+    /* Respond if necessary */
+    monitor_qmp_respond(mon, rsp, NULL, id);
+
+    /* This pairs with the monitor_suspend() in handle_qmp_command(). */
+    if (need_resume) {
+        monitor_resume(mon);
+    }
 
     qobject_decref(req);
 }
 
+/*
+ * Pop one QMP request from monitor queues, return NULL if not found.
+ * We are using round-robin fashion to pop the request, to avoid
+ * processing command only on a very busy monitor.  To achieve that,
+ * when we processed one request on specific monitor, we put that
+ * monitor to the end of mon_list queue.
+ */
+static QMPRequest *monitor_qmp_requests_pop_one(void)
+{
+    QMPRequest *req_obj = NULL;
+    Monitor *mon;
+
+    qemu_mutex_lock(&monitor_lock);
+
+    QTAILQ_FOREACH(mon, &mon_list, entry) {
+        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+        req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
+        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        if (req_obj) {
+            break;
+        }
+    }
+
+    if (req_obj) {
+        /*
+         * We found one request on the monitor. Degrade this monitor's
+         * priority to lowest by re-inserting it to end of queue.
+         */
+        QTAILQ_REMOVE(&mon_list, mon, entry);
+        QTAILQ_INSERT_TAIL(&mon_list, mon, entry);
+    }
+
+    qemu_mutex_unlock(&monitor_lock);
+
+    return req_obj;
+}
+
+static void monitor_qmp_bh_dispatcher(void *data)
+{
+    QMPRequest *req_obj = monitor_qmp_requests_pop_one();
+
+    if (req_obj) {
+        monitor_qmp_dispatch_one(req_obj);
+        /* Reschedule instead of looping so the main loop stays responsive */
+        qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+    }
+}
+
+static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+{
+    QObject *req, *id = NULL;
+    QDict *qdict = NULL;
+    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
+    Monitor *mon = container_of(mon_qmp, Monitor, qmp);
+    Error *err = NULL;
+    QMPRequest *req_obj;
+
+    req = json_parser_parse_err(tokens, NULL, &err);
+    if (!req && !err) {
+        /* json_parser_parse_err() sucks: can fail without setting @err */
+        error_setg(&err, QERR_JSON_PARSING);
+    }
+    if (err) {
+        monitor_qmp_respond(mon, NULL, err, NULL);
+        qobject_decref(req);
+        return;
+    }
+
+    qdict = qobject_to_qdict(req);
+    if (qdict) {
+        id = qdict_get(qdict, "id");
+        qobject_incref(id);
+        qdict_del(qdict, "id");
+    } /* else will fail qmp_dispatch() */
+
+    req_obj = g_new0(QMPRequest, 1);
+    req_obj->mon = mon;
+    req_obj->id = id;
+    req_obj->req = req;
+    req_obj->need_resume = false;
+
+    /*
+     * If OOB is not enabled on current monitor, we'll emulate the old
+     * behavior that we won't process current monitor any more until
+     * it is responded.  This helps make sure that as long as OOB is
+     * not enabled, the server will never drop any command.
+     */
+    if (!qmp_oob_enabled(mon)) {
+        monitor_suspend(mon);
+        req_obj->need_resume = true;
+    }
+

monitor.c static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 4056

Quoted 3 lines

monitor.c +3 -0

+    /*
+     * Put the request to the end of queue so that requests will be
+     * handled in time order.  Ownership for req_obj, req, id,

I think the order is not respected if subsequent messages have errors (in either json parsing, dispatch_check_obj, oob_check). So if I enable oob, and queue a few command, then send a bad command/message, I won't be able to tell for which command.

Quoted 33 lines

monitor.c +19 -0

+     * etc. will be delivered to the handler side.
+     */
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+
+    /* Kick the dispatcher routine */
+    qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+}
+
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
     Monitor *mon = opaque;

@@ -4134,6 +4276,15 @@ static void monitor_iothread_init(void)
 {
     mon_global.mon_iothread = iothread_create("mon_iothread",
                                               &error_abort);
+
+    /*
+     * This MUST be on main loop thread since we have commands that
+     * have assumption to be run on main loop thread.  It would be
+     * nice that one day we can remove this assumption in the future.
+     */
+    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
+                                              monitor_qmp_bh_dispatcher,
+                                              NULL);
 }
 
 void monitor_init_globals(void)

@@ -4280,6 +4431,10 @@ void monitor_cleanup(void)
     }
     qemu_mutex_unlock(&monitor_lock);
 

Quoted 11 lines

monitor.c +4 -1

+    /* QEMUBHs needs to be deleted before destroying the IOThread. */
+    qemu_bh_delete(mon_global.qmp_dispatcher_bh);
+    mon_global.qmp_dispatcher_bh = NULL;
+
     iothread_destroy(mon_global.mon_iothread);
     mon_global.mon_iothread = NULL;
 }
--

2.14.3

Signature · 1 line

Marc-André Lureau

Quoted 20 lines

Quoted 20 lines

So the parsing job and the dispatching job is isolated now. It gives us a chance in following up patches to totally move the parser outside.

The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is used for all the monitors.

monitor.c +12 -0

+
+    /*
+     * If OOB is not enabled on current monitor, we'll emulate the old
+     * behavior that we won't process current monitor any more until
+     * it is responded.  This helps make sure that as long as OOB is
+     * not enabled, the server will never drop any command.
+     */
+    if (!qmp_oob_enabled(mon)) {
+        monitor_suspend(mon);
+        req_obj->need_resume = true;
+    }
+

Quoted 8 lines

monitor.c static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 4056

Quoted 3 lines

monitor.c +3 -0

+    /*
+     * Put the request to the end of queue so that requests will be
+     * handled in time order.  Ownership for req_obj, req, id,

I think the order is not respected if subsequent messages have errors (in either json parsing, dispatch_check_obj, oob_check). So if I enable oob, and queue a few command, then send a bad command/message, I won't be able to tell for which command.

Doesn't OOB insist on having an ID field with the command?

Quoted 31 lines · Dr. David Alan Gilbert

Quoted 21 lines

Quoted 21 lines

So the parsing job and the dispatching job is isolated now. It gives us a chance in following up patches to totally move the parser outside.

The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is used for all the monitors.

monitor.c +13 -0

+
+    /*
+     * If OOB is not enabled on current monitor, we'll emulate the old
+     * behavior that we won't process current monitor any more until
+     * it is responded.  This helps make sure that as long as OOB is
+     * not enabled, the server will never drop any command.
+     */
+    if (!qmp_oob_enabled(mon)) {
+        monitor_suspend(mon);
+        req_obj->need_resume = true;
+    }
+
+    /*

Quoted 8 lines

monitor.c static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 4056

Quoted 2 lines

monitor.c +2 -0

+     * Put the request to the end of queue so that requests will be
+     * handled in time order.  Ownership for req_obj, req, id,

I think the order is not respected if subsequent messages have errors (in either json parsing, dispatch_check_obj, oob_check). So if I enable oob, and queue a few command, then send a bad command/message, I won't be able to tell for which command.

Quoted 1 line

Doesn't OOB insist on having an ID field with the command?

OOB insists on an id field - but there is the situation that SOME errors occur even before the id field has been encountered (for example, if you send non-JSON, the parser gets all confused - it has to emit SOME error, but that error can't refer to an id because it wasn't able to parse one yet). A well-formed client will never do this, but we MUST be robust even in the face of a malicious client (or even a well-intentioned client but a noisy communication medium that manages to corrupt bytes). I'm not sure if the testsuite adequately covers what happens in this scenario. Peter?

Hi

Quoted 36 lines · Eric Blake

Quoted 31 lines · Dr. David Alan Gilbert

Quoted 21 lines

Quoted 21 lines

So the parsing job and the dispatching job is isolated now. It gives us a chance in following up patches to totally move the parser outside.

The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is used for all the monitors.

monitor.c +13 -0

+
+    /*
+     * If OOB is not enabled on current monitor, we'll emulate the old
+     * behavior that we won't process current monitor any more until
+     * it is responded.  This helps make sure that as long as OOB is
+     * not enabled, the server will never drop any command.
+     */
+    if (!qmp_oob_enabled(mon)) {
+        monitor_suspend(mon);
+        req_obj->need_resume = true;
+    }
+
+    /*

Quoted 8 lines

monitor.c static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 4056

Quoted 2 lines

monitor.c +2 -0

+     * Put the request to the end of queue so that requests will be
+     * handled in time order.  Ownership for req_obj, req, id,

I think the order is not respected if subsequent messages have errors (in either json parsing, dispatch_check_obj, oob_check). So if I enable oob, and queue a few command, then send a bad command/message, I won't be able to tell for which command.

Quoted 1 line

Doesn't OOB insist on having an ID field with the command?

Quoted 8 lines

OOB insists on an id field - but there is the situation that SOME errors occur even before the id field has been encountered (for example, if you send non-JSON, the parser gets all confused - it has to emit SOME error, but that error can't refer to an id because it wasn't able to parse one yet). A well-formed client will never do this, but we MUST be robust even in the face of a malicious client (or even a well-intentioned client but a noisy communication medium that manages to corrupt bytes). I'm not sure if the testsuite adequately covers what happens in this scenario. Peter?

I think a solution would be to queue the error reply (the reply only) instead of replying directly.

Another problem I see with the current solution is that pending commands are not discarded when a new client connects. So I suppose a new client can receive replies for commands it didn't make, with id namespace that may conflict with its own. breaking ordering etc. A possible solution is to mark the pending request to not send the reply somehow?

Quoted 12 lines

Quoted 11 lines

Quoted 1 line

Doesn't OOB insist on having an ID field with the command?

OOB insists on an id field - but there is the situation that SOME errors occur even before the id field has been encountered (for example, if you send non-JSON, the parser gets all confused - it has to emit SOME error, but that error can't refer to an id because it wasn't able to parse one yet). A well-formed client will never do this, but we MUST be robust even in the face of a malicious client (or even a well-intentioned client but a noisy communication medium that manages to corrupt bytes). I'm not sure if the testsuite adequately covers what happens in this scenario. Peter?

Quoted 2 lines

I think a solution would be to queue the error reply (the reply only) instead of replying directly.

IMHO this should be fine.

Note that to be compatible with existing QMP we'll suspend the monitor if OOB is not enabled in the session on receiving the first command. It means even if another faulty command is sent after the good one, it'll not be read by QMP parser until the first one is fully complete.

Since I'm working on some test patches after all, I'll try to add a test case for this to see whether Eric would like them too.

Quoted 8 lines

Another problem I see with the current solution is that pending commands are not discarded when a new client connects. So I suppose a new client can receive replies for commands it didn't make, with id namespace that may conflict with its own. breaking ordering etc. A possible solution is to mark the pending request to not send the reply somehow?

Yeah, to be simpler - maybe we can even drop the commands that have not yet been dispatched?

I'll treat a command as "complete" only until the client receives a response, otherwise if a client disconnects before receiving a reply then I think it's fine the behavior is undefined - IMHO it's fine either the QMP server executes the command or not (and no matter what, we drop the responses). Would that work?

Originally QMP goes through these steps:

JSON Parser --> QMP Dispatcher --> Respond

      /|\    (2)                (3)     |
   (1) |                               \|/ (4)
       +---------  main thread  --------+

This patch does this:

JSON Parser QMP Dispatcher --> Respond

      /|\ |           /|\       (4)     |
       |  | (2)        | (3)            |  (5)
   (1) |  +----->      |               \|/
       +---------  main thread  <-------+

So the parsing job and the dispatching job is isolated now. It gives us a chance in following up patches to totally move the parser outside.

The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is used for all the monitors.

Reviewed-by Stefan Hajnoczi <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 178 insertions(+), 23 deletions(-)

monitor.c +52 -20

index de9343be87..5104e5db07 100644
@@ -172,6 +172,13 @@ typedef struct {
      */
     QmpCommandList *commands;
     bool qmp_caps[QMP_CAPABILITY__MAX];
+    /*
+     * Protects qmp request/response queue.  Please take monitor_lock
+     * first when used together.
+     */
+    QemuMutex qmp_queue_lock;
+    /* Input queue that holds all the parsed QMP requests */
+    GQueue *qmp_requests;
 } MonitorQMP;
 
 /*

@@ -218,6 +225,8 @@ struct Monitor {
 /* Let's add monitor global variables to this struct. */
 static struct {
     IOThread *mon_iothread;
+    /* Bottom half to dispatch the requests received from IO thread */
+    QEMUBH *qmp_dispatcher_bh;
 } mon_global;
 
 /* QMP checker flags */

@@ -600,11 +609,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
 {
     memset(mon, 0, sizeof(Monitor));
     qemu_mutex_init(&mon->out_lock);
+    qemu_mutex_init(&mon->qmp.qmp_queue_lock);
     mon->outbuf = qstring_new();
     /* Use *mon_cmds by default. */
     mon->cmd_table = mon_cmds;
     mon->skip_flush = skip_flush;
     mon->use_io_thr = use_io_thr;
+    mon->qmp.qmp_requests = g_queue_new();
 }
 
 static void monitor_data_destroy(Monitor *mon)

@@ -617,6 +628,8 @@ static void monitor_data_destroy(Monitor *mon)
     readline_free(mon->rs);
     QDECREF(mon->outbuf);
     qemu_mutex_destroy(&mon->out_lock);
+    qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
+    g_queue_free(mon->qmp.qmp_requests);
 }
 
 char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,

@@ -1056,6 +1069,16 @@ static void monitor_init_qmp_commands(void)
                          qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
 }
 
+static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap)
+{
+    return mon->qmp.qmp_caps[cap];
+}
+
+static bool qmp_oob_enabled(Monitor *mon)
+{
+    return qmp_cap_enabled(mon, QMP_CAPABILITY_OOB);
+}
+
 static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list,
                            Error **errp)
 {

@@ -3866,30 +3889,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
     qobject_decref(rsp);
 }
 
-static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+struct QMPRequest {
+    /* Owner of the request */
+    Monitor *mon;
+    /* "id" field of the request */
+    QObject *id;
+    /* Request object to be handled */
+    QObject *req;
+    /*
+     * Whether we need to resume the monitor afterward.  This flag is
+     * used to emulate the old QMP server behavior that the current
+     * command must be completed before execution of the next one.
+     */
+    bool need_resume;
+};
+typedef struct QMPRequest QMPRequest;
+
+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);
 
     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);

monitor.c +126 -4

-    rsp = qmp_dispatch(cur_mon->qmp.commands, req);
+    rsp = qmp_dispatch(mon->qmp.commands, req);
 
     cur_mon = old_mon;
 

@@ -3916,12 +3948,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         }
     }
 
-err_out:
-    monitor_qmp_respond(mon, rsp, err, id);
+    /* Respond if necessary */
+    monitor_qmp_respond(mon, rsp, NULL, id);
+
+    /* This pairs with the monitor_suspend() in handle_qmp_command(). */
+    if (need_resume) {
+        monitor_resume(mon);
+    }
 
     qobject_decref(req);
 }
 
+/*
+ * Pop one QMP request from monitor queues, return NULL if not found.
+ * We are using round-robin fashion to pop the request, to avoid
+ * processing command only on a very busy monitor.  To achieve that,
+ * when we processed one request on specific monitor, we put that
+ * monitor to the end of mon_list queue.
+ */
+static QMPRequest *monitor_qmp_requests_pop_one(void)
+{
+    QMPRequest *req_obj = NULL;
+    Monitor *mon;
+
+    qemu_mutex_lock(&monitor_lock);
+
+    QTAILQ_FOREACH(mon, &mon_list, entry) {
+        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+        req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
+        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        if (req_obj) {
+            break;
+        }
+    }
+
+    if (req_obj) {
+        /*
+         * We found one request on the monitor. Degrade this monitor's
+         * priority to lowest by re-inserting it to end of queue.
+         */
+        QTAILQ_REMOVE(&mon_list, mon, entry);
+        QTAILQ_INSERT_TAIL(&mon_list, mon, entry);
+    }
+
+    qemu_mutex_unlock(&monitor_lock);
+
+    return req_obj;
+}
+
+static void monitor_qmp_bh_dispatcher(void *data)
+{
+    QMPRequest *req_obj = monitor_qmp_requests_pop_one();
+
+    if (req_obj) {
+        monitor_qmp_dispatch_one(req_obj);
+        /* Reschedule instead of looping so the main loop stays responsive */
+        qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+    }
+}
+
+static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+{
+    QObject *req, *id = NULL;
+    QDict *qdict = NULL;
+    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
+    Monitor *mon = container_of(mon_qmp, Monitor, qmp);
+    Error *err = NULL;
+    QMPRequest *req_obj;
+
+    req = json_parser_parse_err(tokens, NULL, &err);
+    if (!req && !err) {
+        /* json_parser_parse_err() sucks: can fail without setting @err */
+        error_setg(&err, QERR_JSON_PARSING);
+    }
+    if (err) {
+        monitor_qmp_respond(mon, NULL, err, NULL);
+        qobject_decref(req);
+        return;
+    }
+
+    qdict = qobject_to_qdict(req);
+    if (qdict) {
+        id = qdict_get(qdict, "id");
+        qobject_incref(id);
+        qdict_del(qdict, "id");
+    } /* else will fail qmp_dispatch() */
+
+    req_obj = g_new0(QMPRequest, 1);
+    req_obj->mon = mon;
+    req_obj->id = id;
+    req_obj->req = req;
+    req_obj->need_resume = false;
+
+    /*
+     * If OOB is not enabled on current monitor, we'll emulate the old
+     * behavior that we won't process current monitor any more until
+     * it is responded.  This helps make sure that as long as OOB is
+     * not enabled, the server will never drop any command.
+     */
+    if (!qmp_oob_enabled(mon)) {
+        monitor_suspend(mon);
+        req_obj->need_resume = true;
+    }
+
+    /*
+     * Put the request to the end of queue so that requests will be
+     * handled in time order.  Ownership for req_obj, req, id,
+     * etc. will be delivered to the handler side.
+     */
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+
+    /* Kick the dispatcher routine */
+    qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+}
+
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
     Monitor *mon = opaque;

@@ -4134,6 +4276,15 @@ static void monitor_iothread_init(void)
 {
     mon_global.mon_iothread = iothread_create("mon_iothread",
                                               &error_abort);
+
+    /*
+     * This MUST be on main loop thread since we have commands that
+     * have assumption to be run on main loop thread.  It would be
+     * nice that one day we can remove this assumption in the future.
+     */
+    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
+                                              monitor_qmp_bh_dispatcher,
+                                              NULL);
 }
 
 void monitor_init_globals(void)

@@ -4280,6 +4431,10 @@ void monitor_cleanup(void)
     }
     qemu_mutex_unlock(&monitor_lock);
 
+    /* QEMUBHs needs to be deleted before destroying the IOThread. */
+    qemu_bh_delete(mon_global.qmp_dispatcher_bh);
+    mon_global.qmp_dispatcher_bh = NULL;
+
     iothread_destroy(mon_global.mon_iothread);
     mon_global.mon_iothread = NULL;
 }
--

2.14.3

Quoted 37 lines

monitor.c +13 -19

+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);

monitor.c static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 3933

Quoted 5 lines

     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);

monitor.c +0 -0

@@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     old_mon = cur_mon;
     cur_mon = mon;

There is another issue with this series, since cur_mon is global (and not protected), an oob command may change the cur_mon while another command is running in the main thread with unexpected consequences. I don't have a clear idea what is the best way to solve it. Making the variable per-thread, or going all the way to get rid of cur_mon (my preference, but much harder)

On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:

[...]

Quoted 37 lines

Quoted 37 lines

monitor.c +13 -19

+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);

Quoted 12 lines

monitor.c static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 3933

Quoted 5 lines

     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);

monitor.c +0 -0

@@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     old_mon = cur_mon;
     cur_mon = mon;

There is another issue with this series, since cur_mon is global (and not protected), an oob command may change the cur_mon while another command is running in the main thread with unexpected consequences. I don't have a clear idea what is the best way to solve it. Making the variable per-thread, or going all the way to get rid of cur_mon (my preference, but much harder)

IMHO it is fine too.

Note that this cur_mon operation is in monitor_qmp_dispatch_one() now, which is still running in main thread. So AFAICT all the cur_mon references are in main thread, and monitor IOThread does not modify that variable at all. Then we should probably be safe.

I would be far more than glad to see cur_mon go away one day.

Thanks,

--
Peter Xu

Hi

Quoted 56 lines · Peter Xu

On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:

[...]

Quoted 37 lines

Quoted 37 lines

(quoted diff) +13 -19

+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);

Quoted 12 lines

Review static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 3933

Quoted 5 lines

     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);

(quoted diff) +0 -0

@@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     old_mon = cur_mon;
     cur_mon = mon;

There is another issue with this series, since cur_mon is global (and not protected), an oob command may change the cur_mon while another command is running in the main thread with unexpected consequences. I don't have a clear idea what is the best way to solve it. Making the variable per-thread, or going all the way to get rid of cur_mon (my preference, but much harder)

IMHO it is fine too.

Quoted 4 lines

Note that this cur_mon operation is in monitor_qmp_dispatch_one() now, which is still running in main thread. So AFAICT all the cur_mon references are in main thread, and monitor IOThread does not modify that variable at all. Then we should probably be safe.

But monitor_qmp_dispatch_one() is called from iothread if the command is oob, so cur_mon may be updated while another command is running in main thread, or am I wrong?

Quoted 59 lines · Marc-André Lureau

Hi

Quoted 56 lines · Peter Xu

On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:

[...]

Quoted 37 lines

Quoted 37 lines

(quoted diff) +13 -19

+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);

Quoted 12 lines

Review static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 3933

Quoted 5 lines

     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);

(quoted diff) +0 -0

@@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     old_mon = cur_mon;
     cur_mon = mon;

There is another issue with this series, since cur_mon is global (and not protected), an oob command may change the cur_mon while another command is running in the main thread with unexpected consequences. I don't have a clear idea what is the best way to solve it. Making the variable per-thread, or going all the way to get rid of cur_mon (my preference, but much harder)

IMHO it is fine too.

Quoted 8 lines

Quoted 4 lines

Note that this cur_mon operation is in monitor_qmp_dispatch_one() now, which is still running in main thread. So AFAICT all the cur_mon references are in main thread, and monitor IOThread does not modify that variable at all. Then we should probably be safe.

But monitor_qmp_dispatch_one() is called from iothread if the command is oob, so cur_mon may be updated while another command is running in main thread, or am I wrong?

You are right. I missed that, sorry...

Would this be a simple workaround (but hopefully efficient) solution?

monitor.c +10 -6

index 77f4c41cfa..99641c0c6d 100644
@@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
  * Dispatch one single QMP request. The function will free the req_obj
  * and objects inside it before return.
  */
-static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
 {
     Monitor *mon, *old_mon;
     QObject *req, *rsp = NULL, *id;

@@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
         QDECREF(req_json);
     }
 
-    old_mon = cur_mon;
-    cur_mon = mon;
+    if (hack_curmon) {
+        old_mon = cur_mon;
+        cur_mon = mon;
+    }
 
     rsp = qmp_dispatch(mon->qmp.commands, req);
 
-    cur_mon = old_mon;
+    if (hack_curmon) {
+        cur_mon = old_mon;
+    }
 
     if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
         qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");

@@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
 
     if (req_obj) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
-        monitor_qmp_dispatch_one(req_obj);
+        monitor_qmp_dispatch_one(req_obj, true);
         /* Reschedule instead of looping so the main loop stays responsive */
         qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
     }

@@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         /* Out-Of-Band (OOB) requests are executed directly in parser. */
         trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
                                           ?: "");
-        monitor_qmp_dispatch_one(req_obj);
+        monitor_qmp_dispatch_one(req_obj, false);
         return;
     }
 

Then we forbit touching that evil cur_mon in OOB-capable command handlers. Thanks,

Hi

Quoted 124 lines · Peter Xu

Quoted 59 lines · Marc-André Lureau

Hi

Quoted 56 lines · Peter Xu

On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:

[...]

Quoted 37 lines

Quoted 37 lines

(quoted diff) +13 -19

+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);

Quoted 12 lines

Review static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 3933

Quoted 5 lines

     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);

(quoted diff) +0 -0

@@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     old_mon = cur_mon;
     cur_mon = mon;

There is another issue with this series, since cur_mon is global (and not protected), an oob command may change the cur_mon while another command is running in the main thread with unexpected consequences. I don't have a clear idea what is the best way to solve it. Making the variable per-thread, or going all the way to get rid of cur_mon (my preference, but much harder)

IMHO it is fine too.

Quoted 8 lines

Quoted 4 lines

Note that this cur_mon operation is in monitor_qmp_dispatch_one() now, which is still running in main thread. So AFAICT all the cur_mon references are in main thread, and monitor IOThread does not modify that variable at all. Then we should probably be safe.

But monitor_qmp_dispatch_one() is called from iothread if the command is oob, so cur_mon may be updated while another command is running in main thread, or am I wrong?

You are right. I missed that, sorry...

Would this be a simple workaround (but hopefully efficient) solution?

monitor.c +10 -6

index 77f4c41cfa..99641c0c6d 100644
@@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
  * Dispatch one single QMP request. The function will free the req_obj
  * and objects inside it before return.
  */
-static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
 {
     Monitor *mon, *old_mon;
     QObject *req, *rsp = NULL, *id;

@@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
         QDECREF(req_json);
     }
 
-    old_mon = cur_mon;
-    cur_mon = mon;
+    if (hack_curmon) {
+        old_mon = cur_mon;
+        cur_mon = mon;
+    }
 
     rsp = qmp_dispatch(mon->qmp.commands, req);
 
-    cur_mon = old_mon;
+    if (hack_curmon) {
+        cur_mon = old_mon;
+    }
 
     if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
         qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");

@@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
 
     if (req_obj) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
-        monitor_qmp_dispatch_one(req_obj);
+        monitor_qmp_dispatch_one(req_obj, true);
         /* Reschedule instead of looping so the main loop stays responsive */
         qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
     }

@@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         /* Out-Of-Band (OOB) requests are executed directly in parser. */
         trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
                                           ?: "");
-        monitor_qmp_dispatch_one(req_obj);
+        monitor_qmp_dispatch_one(req_obj, false);
         return;
     }
 

Quoted 2 lines

Then we forbit touching that evil cur_mon in OOB-capable command handlers. Thanks,

That's not easy to enforce though, afaict it is being used for:
- error reporting decision

- file & socket lookup (fd: & /dev/fdset etc)

- the current state of the monitor / list of commands, cpu_path, capabilities..

Wouldn't it be simpler to make it per-thread? I think it could also use helpers to push/pop the current monitor.

Quoted 127 lines · Marc-André Lureau

Hi

Quoted 124 lines · Peter Xu

Quoted 59 lines · Marc-André Lureau

Hi

Quoted 56 lines · Peter Xu

On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:

[...]

Quoted 37 lines

Quoted 37 lines

(quoted diff) +13 -19

+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);

Quoted 12 lines

Review static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) · line 3933

Quoted 5 lines

     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);

(quoted diff) +0 -0

@@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     old_mon = cur_mon;
     cur_mon = mon;

There is another issue with this series, since cur_mon is global (and not protected), an oob command may change the cur_mon while another command is running in the main thread with unexpected consequences. I don't have a clear idea what is the best way to solve it. Making the variable per-thread, or going all the way to get rid of cur_mon (my preference, but much harder)

IMHO it is fine too.

Quoted 8 lines

Quoted 4 lines

Note that this cur_mon operation is in monitor_qmp_dispatch_one() now, which is still running in main thread. So AFAICT all the cur_mon references are in main thread, and monitor IOThread does not modify that variable at all. Then we should probably be safe.

But monitor_qmp_dispatch_one() is called from iothread if the command is oob, so cur_mon may be updated while another command is running in main thread, or am I wrong?

You are right. I missed that, sorry...

Would this be a simple workaround (but hopefully efficient) solution?

monitor.c +10 -6

index 77f4c41cfa..99641c0c6d 100644
@@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
  * Dispatch one single QMP request. The function will free the req_obj
  * and objects inside it before return.
  */
-static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
 {
     Monitor *mon, *old_mon;
     QObject *req, *rsp = NULL, *id;

@@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
         QDECREF(req_json);
     }
 
-    old_mon = cur_mon;
-    cur_mon = mon;
+    if (hack_curmon) {
+        old_mon = cur_mon;
+        cur_mon = mon;
+    }
 
     rsp = qmp_dispatch(mon->qmp.commands, req);
 
-    cur_mon = old_mon;
+    if (hack_curmon) {
+        cur_mon = old_mon;
+    }
 
     if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
         qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");

@@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
 
     if (req_obj) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
-        monitor_qmp_dispatch_one(req_obj);
+        monitor_qmp_dispatch_one(req_obj, true);
         /* Reschedule instead of looping so the main loop stays responsive */
         qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
     }

@@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         /* Out-Of-Band (OOB) requests are executed directly in parser. */
         trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
                                           ?: "");
-        monitor_qmp_dispatch_one(req_obj);
+        monitor_qmp_dispatch_one(req_obj, false);
         return;
     }
 

Quoted 5 lines

Quoted 2 lines

Then we forbit touching that evil cur_mon in OOB-capable command handlers. Thanks,

That's not easy to enforce though, afaict it is being used for:
- error reporting decision

IMO this should not be a problem, since any QMP handler (including OOB-capable ones) will be with an Error** there, so logically speaking people should never call things like error_report() in that.

Quoted 1 line

- file & socket lookup (fd: & /dev/fdset etc)

I suppose only very rare commands will use it? It'll be a big problem to solve when we want to completely remove cur_mon though.

Quoted 1 line

- the current state of the monitor / list of commands, cpu_path, capabilities..

This is very rare to be used too? Most commands should not use them AFAIU.

Quoted 3 lines

Wouldn't it be simpler to make it per-thread? I think it could also use helpers to push/pop the current monitor.