Monitor: some oob related patches (fixes, new param, tests)

When someone sents a command before QMP handshake, error was like this:

{"execute": "query-cpus"}
{"error": {"class": "CommandNotFound", "desc":

            "Expecting capabilities negotiation with 'qmp_capabilities'"}}

While after cf869d5317 it becomes:

{"execute": "query-cpus"}
{"error": {"class": "CommandNotFound", "desc":

            "The command query-cpus has not been found"}}

Fix it back to the nicer one.

Fixes cf869d5317 ("qmp: support out-of-band (oob) execution", 2018-03-19)

Reported-by Marc-André Lureau <[email protected]>

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

---
 monitor.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

monitor.c +8 -16

index 77f4c41cfa..849fa23bf9 100644
@@ -1203,8 +1203,14 @@ static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp)
 
     cmd = qmp_find_command(mon->qmp.commands, command);
     if (!cmd) {
-        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
-                  "The command %s has not been found", command);
+        if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
+            error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+                      "Expecting capabilities negotiation "
+                      "with 'qmp_capabilities'");
+        } else {
+            error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+                      "The command %s has not been found", command);
+        }
         return false;
     }
 

@@ -4027,7 +4033,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
     Monitor *mon, *old_mon;
     QObject *req, *rsp = NULL, *id;
-    QDict *qdict = NULL;
     bool need_resume;
 
     req = req_obj->req;

@@ -4050,18 +4055,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 
     cur_mon = old_mon;
 
-    if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
-        qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
-        if (qdict
-            && !g_strcmp0(qdict_get_try_str(qdict, "class"),
-                    QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) {
-            /* Provide a more useful error message */
-            qdict_del(qdict, "desc");
-            qdict_put_str(qdict, "desc", "Expecting capabilities negotiation"
-                          " with 'qmp_capabilities'");
-        }
-    }
-
     /* Respond if necessary */
     monitor_qmp_respond(mon, rsp, NULL, id);
 
--

2.14.3

When someone sents a command before QMP handshake, error was like this:

{"execute": "query-cpus"}
{"error": {"class": "CommandNotFound", "desc":

             "Expecting capabilities negotiation with 'qmp_capabilities'"}}

While after cf869d5317 it becomes:

{"execute": "query-cpus"}
{"error": {"class": "CommandNotFound", "desc":

             "The command query-cpus has not been found"}}

Fix it back to the nicer one.

Fixes cf869d5317 ("qmp: support out-of-band (oob) execution", 2018-03-19)

Reported-by Marc-André Lureau <[email protected]>

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

---
  monitor.c | 23 ++++++++---------------
  1 file changed, 8 insertions(+), 15 deletions(-)

Marc-André Lureau reported that we can have this happen:

1. client1 connects, send command C1
2. client1 disconnects before getting response for C1
3. client2 connects, who might receive response of C1

However client2 should not receive remaining responses for client1.

Basically, we should clean up the request/response queue elements when:

- before a session established
- after a session is closed
- before destroying the queues

Some helpers are introduced to achieve that. We need to make sure we're with the lock when operating on those queues.

Reported-by Marc-André Lureau <[email protected]>

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

---
 monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 59 insertions(+), 20 deletions(-)

monitor.c +59 -21

index 849fa23bf9..eba98df9da 100644
@@ -234,6 +234,22 @@ static struct {
     QEMUBH *qmp_respond_bh;
 } mon_global;
 
+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;
+
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 

@@ -310,6 +326,43 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
     }
 }
 
+static void qmp_request_free(QMPRequest *req)
+{
+    qobject_decref(req->id);
+    qobject_decref(req->req);
+    g_free(req);
+}
+
+static void qmp_response_free(QObject *obj)
+{
+    qobject_decref(obj);
+}
+
+/* Must with the mon->qmp.qmp_queue_lock held */
+static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
+{
+    while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
+        qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests));
+    }
+}
+
+/* Must with the mon->qmp.qmp_queue_lock held */
+static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
+{
+    while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
+        qmp_response_free(g_queue_pop_head(mon->qmp.qmp_responses));
+    }
+}
+
+static void monitor_qmp_cleanup_queues(Monitor *mon)
+{
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    monitor_qmp_cleanup_req_queue_locked(mon);
+    monitor_qmp_cleanup_resp_queue_locked(mon);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+}
+
+
 static void monitor_flush_locked(Monitor *mon);
 
 static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,

@@ -497,7 +550,7 @@ static void monitor_qmp_bh_responder(void *opaque)
             break;
         }
         monitor_json_emitter_raw(response.mon, response.data);
-        qobject_decref(response.data);
+        qmp_response_free(response.data);
     }
 }
 

@@ -701,6 +754,8 @@ static void monitor_data_destroy(Monitor *mon)
     QDECREF(mon->outbuf);
     qemu_mutex_destroy(&mon->out_lock);
     qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
+    monitor_qmp_cleanup_req_queue_locked(mon);
+    monitor_qmp_cleanup_resp_queue_locked(mon);
     g_queue_free(mon->qmp.qmp_requests);
     g_queue_free(mon->qmp.qmp_responses);
 }

@@ -4009,22 +4064,6 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
     qobject_decref(rsp);
 }
 
-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.

@@ -4191,9 +4230,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
             qapi_event_send_command_dropped(id,
                                             COMMAND_DROP_REASON_QUEUE_FULL,
                                             &error_abort);
-            qobject_decref(id);
-            qobject_decref(req);
-            g_free(req_obj);
+            qmp_request_free(req_obj);
             return;
         }
     }

@@ -4327,6 +4364,7 @@ static void monitor_qmp_event(void *opaque, int event)
 
     switch (event) {
     case CHR_EVENT_OPENED:
+        monitor_qmp_cleanup_queues(mon);
         mon->qmp.commands = &qmp_cap_negotiation_commands;
         monitor_qmp_caps_reset(mon);
         data = get_qmp_greeting(mon);

@@ -4335,6 +4373,7 @@ static void monitor_qmp_event(void *opaque, int event)
         mon_refcount++;
         break;
     case CHR_EVENT_CLOSED:
+        monitor_qmp_cleanup_queues(mon);
         json_message_parser_destroy(&mon->qmp.parser);
         json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
         mon_refcount--;
--

2.14.3

Marc-André Lureau reported that we can have this happen:

1. client1 connects, send command C1
2. client1 disconnects before getting response for C1
3. client2 connects, who might receive response of C1

However client2 should not receive remaining responses for client1.

Basically, we should clean up the request/response queue elements when:

- before a session established

- after a session is closed
- before destroying the queues

Some helpers are introduced to achieve that. We need to make sure we're with the lock when operating on those queues.

Reported-by Marc-André Lureau <[email protected]>

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

---
  monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
  1 file changed, 59 insertions(+), 20 deletions(-)

monitor.c +1 -0

+static void qmp_request_free(QMPRequest *req)

monitor.c +4 -0

+
+/* Must with the mon->qmp.qmp_queue_lock held */
+static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
+{

monitor.c +12 -0

+    }
+}
+
+static void monitor_qmp_cleanup_queues(Monitor *mon)
+{
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    monitor_qmp_cleanup_req_queue_locked(mon);
+    monitor_qmp_cleanup_resp_queue_locked(mon);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+}
+
+
  static void monitor_flush_locked(Monitor *mon);
 
  static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,

monitor.c +1 -0

          mon->qmp.commands = &qmp_cap_negotiation_commands;
          monitor_qmp_caps_reset(mon);
          data = get_qmp_greeting(mon);

@@ -4335,6 +4373,7 @@ static void monitor_qmp_event(void *opaque, int event)
          mon_refcount++;
          break;
      case CHR_EVENT_CLOSED:
+        monitor_qmp_cleanup_queues(mon);
          json_message_parser_destroy(&mon->qmp.parser);
          json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
          mon_refcount--;

Quoted 6 lines · Peter Xu

Marc-André Lureau reported that we can have this happen:

1. client1 connects, send command C1
2. client1 disconnects before getting response for C1
3. client2 connects, who might receive response of C1

Quoted 5 lines

However client2 should not receive remaining responses for client1.

Basically, we should clean up the request/response queue elements when:

- before a session established

Why here? [1]

Quoted 7 lines

- after a session is closed
- before destroying the queues

Some helpers are introduced to achieve that. We need to make sure we're with the lock when operating on those queues.

It would also be helpful to mention that the patch includes code motion to declare struct QMPRequest earlier in the file.

Quoted 6 lines

Reported-by Marc-André Lureau <[email protected]>

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

---
  monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
  1 file changed, 59 insertions(+), 20 deletions(-)

monitor.c int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, · line 339

Quoted 11 lines

monitor.c +11 -0

+static void qmp_request_free(QMPRequest *req)
+{
+    qobject_decref(req->id);
+    qobject_decref(req->req);
+    g_free(req);
+}
+
+static void qmp_response_free(QObject *obj)
+{
+    qobject_decref(obj);
+}

Why do we need this function? Unless you plan to add to it in later patches, I'd rather just inline things and directly call qobject_decref() at the callsites...

Quoted 5 lines

monitor.c +4 -0

+
+/* Must with the mon->qmp.qmp_queue_lock held */
+static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
+{

monitor.c int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, · line 353

Quoted 10 lines

monitor.c +10 -0

+    while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
+        qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests));
+    }
+}
+
+/* Must with the mon->qmp.qmp_queue_lock held */
+static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
+{
+    while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
+        qmp_response_free(g_queue_pop_head(mon->qmp.qmp_responses));

...here,

Quoted 15 lines

monitor.c +12 -0

+    }
+}
+
+static void monitor_qmp_cleanup_queues(Monitor *mon)
+{
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    monitor_qmp_cleanup_req_queue_locked(mon);
+    monitor_qmp_cleanup_resp_queue_locked(mon);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+}
+
+
  static void monitor_flush_locked(Monitor *mon);
  static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,

monitor.c static void monitor_qmp_bh_responder(void *opaque) · line 553

Quoted 5 lines

monitor.c +1 -1

@@ -497,7 +550,7 @@ static void monitor_qmp_bh_responder(void *opaque)
              break;
          }
          monitor_json_emitter_raw(response.mon, response.data);
-        qobject_decref(response.data);
+        qmp_response_free(response.data);

and here.

monitor.c static void monitor_qmp_event(void *opaque, int event) · line 4366

Quoted 4 lines

monitor.c +1 -0

@@ -4327,6 +4364,7 @@ static void monitor_qmp_event(void *opaque, int event)
      switch (event) {
      case CHR_EVENT_OPENED:
+        monitor_qmp_cleanup_queues(mon);

[1] How would something be queued to need cleanup at this point, if we already start with a clean queue before the first monitor, and if all monitor close actions clean the queue?

Add new parameter to optionally enable Out-Of-Band for a QMP server.

An example command line:

  ./qemu-system-x86_64 -chardev stdio,id=char0 \
                       -mon chardev=char0,mode=control,x-oob=on

By default, Out-Of-Band is off.

It is not allowed if either MUX or non-QMP is detected, since Out-Of-Band is currently only for QMP, and non-MUX chardev backends.

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

---
 include/monitor/monitor.h |  1 +
 monitor.c                 | 22 ++++++++++++++++++++--
 vl.c                      |  5 +++++
 3 files changed, 26 insertions(+), 2 deletions(-)

include/monitor/monitor.h +1 -0

index 0cb0538a31..d6ab70cae2 100644
@@ -13,6 +13,7 @@ extern Monitor *cur_mon;
 #define MONITOR_USE_READLINE  0x02
 #define MONITOR_USE_CONTROL   0x04
 #define MONITOR_USE_PRETTY    0x08
+#define MONITOR_USE_OOB       0x10
 
 bool monitor_cur_is_qmp(void);
 

monitor.c +5 -0

index eba98df9da..d77ccc8785 100644
@@ -36,6 +36,7 @@
 #include "net/slirp.h"
 #include "chardev/char-fe.h"
 #include "chardev/char-io.h"
+#include "chardev/char-mux.h"
 #include "ui/qemu-spice.h"
 #include "sysemu/numa.h"
 #include "monitor/monitor.h"

@@ -4568,12 +4569,26 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
 void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
+    bool use_readline = flags & MONITOR_USE_READLINE;
+    bool use_oob = flags & MONITOR_USE_OOB;
+
+    if (use_oob) {

monitor.c +5 -2

-    monitor_data_init(mon, false, false);
+    monitor_data_init(mon, false, use_oob);
 
     qemu_chr_fe_init(&mon->chr, chr, &error_abort);
     mon->flags = flags;
-    if (flags & MONITOR_USE_READLINE) {
+    if (use_readline) {
         mon->rs = readline_init(monitor_readline_printf,
                                 monitor_readline_flush,
                                 mon,

@@ -4669,6 +4684,9 @@ QemuOptsList qemu_mon_opts = {
         },{
             .name = "pretty",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "x-oob",
+            .type = QEMU_OPT_BOOL,
         },
         { /* end of list */ }
     },

vl.c +5 -1

index c81cc86607..5fd01bd5f6 100644
@@ -2404,6 +2404,11 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
     if (qemu_opt_get_bool(opts, "pretty", 0))
         flags |= MONITOR_USE_PRETTY;
 
+    /* OOB is off by default */
+    if (qemu_opt_get_bool(opts, "x-oob", 0)) {
+        flags |= MONITOR_USE_OOB;
+    }
+
     chardev = qemu_opt_get(opts, "chardev");
     chr = qemu_chr_find(chardev);
     if (chr == NULL) {
--

2.14.3

Quoted 13 lines

monitor.c +9 -0

@@ -4568,12 +4569,26 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
 void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
+    bool use_readline = flags & MONITOR_USE_READLINE;
+    bool use_oob = flags & MONITOR_USE_OOB;
+
+    if (use_oob) {
+        if (CHARDEV_IS_MUX(chr)) {
+            error_report("Monitor Out-Of-Band is not supported with "
+                         "MUX typed chardev backend");
+            exit(1);
+        }

monitor.c static void monitor_qmp_setup_handlers_bh(void *opaque) · line 4585

Quoted 5 lines

monitor.c +5 -0

+        if (use_readline) {
+            error_report("Monitor Out-Of-band is only supported by QMP");
+            exit(1);
+        }
+    }

I would rather see the error reporting / exit in vl.c:mon_init_func() function, to have a single place for exit()

Other than that, it looks fine.

Reviewed-by Marc-André Lureau <[email protected]>

Hi

Quoted 11 lines · Peter Xu

Add new parameter to optionally enable Out-Of-Band for a QMP server.

An example command line:

   ./qemu-system-x86_64 -chardev stdio,id=char0 \
                        -mon chardev=char0,mode=control,x-oob=on

By default, Out-Of-Band is off.

It is not allowed if either MUX or non-QMP is detected, since Out-Of-Band is currently only for QMP, and non-MUX chardev backends.

Quoted 8 lines

monitor.c +4 -0

@@ -4568,12 +4569,26 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
  void monitor_init(Chardev *chr, int flags)
  {
      Monitor *mon = g_malloc(sizeof(*mon));
+    bool use_readline = flags & MONITOR_USE_READLINE;
+    bool use_oob = flags & MONITOR_USE_OOB;
+
+    if (use_oob) {

monitor.c static void monitor_qmp_setup_handlers_bh(void *opaque) · line 4583

Quoted 8 lines

monitor.c +8 -0

+        if (CHARDEV_IS_MUX(chr)) {
+            error_report("Monitor Out-Of-Band is not supported with "
+                         "MUX typed chardev backend");
+            exit(1);
+        }
+        if (use_readline) {
+            error_report("Monitor Out-Of-band is only supported by QMP");
+            exit(1);

Review static void monitor_qmp_setup_handlers_bh(void *opaque) · line 4585

Quoted 2 lines

(quoted diff) +2 -0

+        }
+    }

I would rather see the error reporting / exit in vl.c:mon_init_func() function, to have a single place for exit()

Other than that, it looks fine.

Reviewed-by Marc-André Lureau <[email protected]>

It was missed in the first version of OOB series. We should check this to make sure we throw the right error when fault value is passed in.

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

---
 scripts/qapi/common.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

scripts/qapi/common.py +1 -2

index 2c05e3c284..3e14bc41f2 100644
@@ -872,7 +872,7 @@ def check_keys(expr_elem, meta, required, optional=[]):
             raise QAPISemError(info,
                                "'%s' of %s '%s' should only use false value"
                                % (key, meta, name))
-        if key == 'boxed' and value is not True:
+        if (key == 'boxed' or key == 'allow-oob') and value is not True:
             raise QAPISemError(info,
                                "'%s' of %s '%s' should only use true value"
                                % (key, meta, name))
--

2.14.3

Quoted 4 lines · Peter Xu

It was missed in the first version of OOB series. We should check this to make sure we throw the right error when fault value is passed in.

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

Not exactly required imho, but why not:

Reviewed-by Marc-André Lureau <[email protected]>

The allow_oob parameter was passed in but not used in tests. Now reflect that in the tests, so we need to touch up other command testers with that new change.

Reviewed-by Eric Blake <[email protected]>

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

---
 tests/qapi-schema/doc-good.out          |  4 ++--
 tests/qapi-schema/ident-with-escape.out |  2 +-
 tests/qapi-schema/indented-expr.out     |  4 ++--
 tests/qapi-schema/qapi-schema-test.out  | 18 +++++++++---------
 tests/qapi-schema/test-qapi.py          |  4 ++--
 5 files changed, 16 insertions(+), 16 deletions(-)

tests/qapi-schema/doc-good.out +2 -2

index 430b5a87db..63058b1590 100644
@@ -28,9 +28,9 @@ object q_obj_cmd-arg
     member arg2: str optional=True
     member arg3: bool optional=False
 command cmd q_obj_cmd-arg -> Object
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 command cmd-boxed Object -> None
-   gen=True success_response=True boxed=True
+   gen=True success_response=True boxed=True oob=False
 doc freeform
     body=
 = Section

tests/qapi-schema/ident-with-escape.out +1 -1

index ee3b34e623..82213aa51d 100644
@@ -5,4 +5,4 @@ module ident-with-escape.json
 object q_obj_fooA-arg
     member bar1: str optional=False
 command fooA q_obj_fooA-arg -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False

tests/qapi-schema/indented-expr.out +2 -2

index a79935e8c3..862678f8f4 100644
@@ -3,6 +3,6 @@ enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
     prefix QTYPE
 module indented-expr.json
 command eins None -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 command zwei None -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False

tests/qapi-schema/qapi-schema-test.out +9 -9

index 012e7fc06a..4f43370017 100644
@@ -16,7 +16,7 @@ object Empty1
 object Empty2
     base Empty1
 command user_def_cmd0 Empty2 -> Empty2
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 enum QEnumTwo ['value1', 'value2']
     prefix QENUM_TWO
 object UserDefOne

@@ -143,29 +143,29 @@ object UserDefNativeListUnion
     case sizes: q_obj_sizeList-wrapper
     case any: q_obj_anyList-wrapper
 command user_def_cmd None -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 object q_obj_user_def_cmd1-arg
     member ud1a: UserDefOne optional=False
 command user_def_cmd1 q_obj_user_def_cmd1-arg -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 object q_obj_user_def_cmd2-arg
     member ud1a: UserDefOne optional=False
     member ud1b: UserDefOne optional=True
 command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True
 command guest-get-time q_obj_guest-get-time-arg -> int
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 object q_obj_guest-sync-arg
     member arg: any optional=False
 command guest-sync q_obj_guest-sync-arg -> any
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 command boxed-struct UserDefZero -> None
-   gen=True success_response=True boxed=True
+   gen=True success_response=True boxed=True oob=False
 command boxed-union UserDefNativeListUnion -> None
-   gen=True success_response=True boxed=True
+   gen=True success_response=True boxed=True oob=False
 object UserDefOptions
     member i64: intList optional=True
     member u64: uint64List optional=True

@@ -229,4 +229,4 @@ object q_obj___org.qemu_x-command-arg
     member c: __org.qemu_x-Union2 optional=False
     member d: __org.qemu_x-Alt optional=False
 command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False

tests/qapi-schema/test-qapi.py +2 -3

index 10e68b01d9..c1a144ba29 100644
@@ -45,8 +45,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
                       gen, success_response, boxed, allow_oob):
         print('command %s %s -> %s' % \
               (name, arg_type and arg_type.name, ret_type and ret_type.name))
-        print('   gen=%s success_response=%s boxed=%s' % \
-              (gen, success_response, boxed))
+        print('   gen=%s success_response=%s boxed=%s oob=%s' % \
+              (gen, success_response, boxed, allow_oob))
 
     def visit_event(self, name, info, arg_type, boxed):
         print('event %s %s' % (name, arg_type and arg_type.name))
--

2.14.3

It simply tests the new OOB capability, and make sure the QAPISchema can parse it correctly.

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

---
 tests/Makefile.include                  | 1 +
 tests/qapi-schema/oob-test.err          | 1 +
 tests/qapi-schema/oob-test.exit         | 1 +
 tests/qapi-schema/oob-test.json         | 2 ++
 tests/qapi-schema/oob-test.out          | 0
 tests/qapi-schema/qapi-schema-test.json | 3 +++
 tests/qapi-schema/qapi-schema-test.out  | 2 ++
 tests/test-qmp-cmds.c                   | 4 ++++
 8 files changed, 14 insertions(+)
 create mode 100644 tests/qapi-schema/oob-test.err
 create mode 100644 tests/qapi-schema/oob-test.exit
 create mode 100644 tests/qapi-schema/oob-test.json
 create mode 100644 tests/qapi-schema/oob-test.out

tests/Makefile.include +1 -0

index eb218a9539..3b9a5e31a2 100644
@@ -523,6 +523,7 @@ qapi-schema += missing-comma-object.json
 qapi-schema += missing-type.json
 qapi-schema += nested-struct-data.json
 qapi-schema += non-objects.json
+qapi-schema += oob-test.json
 qapi-schema += pragma-doc-required-crap.json
 qapi-schema += pragma-extra-junk.json
 qapi-schema += pragma-name-case-whitelist-crap.json

tests/qapi-schema/oob-test.err new file +1 -0

new file mode 100644
index 0000000000..35b60f7480
@@ -0,0 +1 @@
+tests/qapi-schema/oob-test.json:2: 'allow-oob' of command 'oob-command-1' should only use true value

tests/qapi-schema/oob-test.exit new file +1 -0

new file mode 100644
index 0000000000..d00491fd7e
@@ -0,0 +1 @@
+1

tests/qapi-schema/oob-test.json new file +2 -0

new file mode 100644
index 0000000000..da9635920f
@@ -0,0 +1,2 @@
+# Check against oob illegal value
+{ 'command': 'oob-command-1', 'allow-oob': 'some-string' }

tests/qapi-schema/oob-test.out new file +0 -0

new file mode 100644
index 0000000000..e69de29bb2

tests/qapi-schema/qapi-schema-test.json +3 -0

index c72dbd8050..06e30f452e 100644
@@ -139,6 +139,9 @@
 { 'command': 'boxed-struct', 'boxed': true, 'data': 'UserDefZero' }
 { 'command': 'boxed-union', 'data': 'UserDefNativeListUnion', 'boxed': true }
 
+# Smoke test on Out-Of-Band
+{ 'command': 'an-oob-command', 'allow-oob': true }
+
 # For testing integer range flattening in opts-visitor. The following schema
 # corresponds to the option format:
 #

tests/qapi-schema/qapi-schema-test.out +2 -0

index 4f43370017..467577d770 100644
@@ -166,6 +166,8 @@ command boxed-struct UserDefZero -> None
    gen=True success_response=True boxed=True oob=False
 command boxed-union UserDefNativeListUnion -> None
    gen=True success_response=True boxed=True oob=False
+command an-oob-command None -> None
+   gen=True success_response=True boxed=False oob=True
 object UserDefOptions
     member i64: intList optional=True
     member u64: uint64List optional=True

tests/test-qmp-cmds.c +4 -1

index 93fbbb1b73..db690cc5ae 100644
@@ -16,6 +16,10 @@ void qmp_user_def_cmd(Error **errp)
 {
 }
 
+void qmp_an_oob_command(Error **errp)
+{
+}
+
 Empty2 *qmp_user_def_cmd0(Error **errp)
 {
     return g_new0(Empty2, 1);
--

2.14.3

It simply tests the new OOB capability, and make sure the QAPISchema can parse it correctly.

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

---
  tests/Makefile.include                  | 1 +
  tests/qapi-schema/oob-test.err          | 1 +
  tests/qapi-schema/oob-test.exit         | 1 +
  tests/qapi-schema/oob-test.json         | 2 ++
  tests/qapi-schema/oob-test.out          | 0
  tests/qapi-schema/qapi-schema-test.json | 3 +++
  tests/qapi-schema/qapi-schema-test.out  | 2 ++
  tests/test-qmp-cmds.c                   | 4 ++++
  8 files changed, 14 insertions(+)

create mode 100644 tests/qapi-schema/oob-test.err create mode 100644 tests/qapi-schema/oob-test.exit create mode 100644 tests/qapi-schema/oob-test.json create mode 100644 tests/qapi-schema/oob-test.out

It is abstracted from qtest_init_without_qmp_handshake(). It works just like qtest_init_without_qmp_handshake() but further it would allow the caller to specify the QMP parameter.

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

---
 tests/libqtest.c | 14 +++++++++++---
 tests/libqtest.h | 14 ++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

tests/libqtest.c +11 -3

index 200b2b9e92..d2af1b17f0 100644
@@ -166,19 +166,22 @@ static const char *qtest_qemu_binary(void)
     return qemu_bin;
 }
 
-QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+QTestState *qtest_init_with_qmp_format(const char *extra_args,
+                                       const char *qmp_format)
 {
     QTestState *s;
     int sock, qmpsock, i;
     gchar *socket_path;
     gchar *qmp_socket_path;
     gchar *command;
+    gchar *qmp_params;
     const char *qemu_binary = qtest_qemu_binary();
 
     s = g_new(QTestState, 1);
 
     socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
     qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
+    qmp_params = g_strdup_printf(qmp_format, qmp_socket_path);
 
     /* It's possible that if an earlier test run crashed it might
      * have left a stale unix socket lying around. Delete any

@@ -199,12 +202,12 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
         command = g_strdup_printf("exec %s "
                                   "-qtest unix:%s,nowait "
                                   "-qtest-log %s "
-                                  "-qmp unix:%s,nowait "
+                                  "%s "
                                   "-machine accel=qtest "
                                   "-display none "
                                   "%s", qemu_binary, socket_path,
                                   getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
-                                  qmp_socket_path,
+                                  qmp_params,
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
         exit(1);

@@ -237,6 +240,11 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     return s;
 }
 
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+{
+    return qtest_init_with_qmp_format(extra_args, "-qmp unix:%s,nowait");
+}
+
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s = qtest_init_without_qmp_handshake(extra_args);

tests/libqtest.h +14 -1

index 811169453a..1f3605ce73 100644
@@ -62,6 +62,20 @@ QTestState *qtest_init(const char *extra_args);
  */
 QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
 
+/**
+ * qtest_init_with_qmp_format:
+ * @extra_args: other arguments to pass to QEMU.
+ * @qmp_format: format of QMP parameters, should contain one "%s"
+ *              field so that the socket path will be filled later.
+ *
+ * Note that this function will work just like
+ * qtest_init_without_qmp_handshake(), so no QMP handshake will be done.
+ *
+ * Returns: #QTestState instance.
+ */
+QTestState *qtest_init_with_qmp_format(const char *extra_args,
+                                       const char *qmp_format);
+
 /**
  * qtest_quit:
  * @s: #QTestState instance to operate on.
--

2.14.3

It is abstracted from qtest_init_without_qmp_handshake(). It works just like qtest_init_without_qmp_handshake() but further it would allow the caller to specify the QMP parameter.

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

---
  tests/libqtest.c | 14 +++++++++++---
  tests/libqtest.h | 14 ++++++++++++++
  2 files changed, 25 insertions(+), 3 deletions(-)

tests/libqtest.h +1 -0

+
  /**
   * qtest_quit:
   * @s: #QTestState instance to operate on.

tests/libqtest.c +2 -1

index 200b2b9e92..d2af1b17f0 100644
@@ -166,19 +166,22 @@ static const char *qtest_qemu_binary(void)
      return qemu_bin;
  }
 
-QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+QTestState *qtest_init_with_qmp_format(const char *extra_args,
+                                       const char *qmp_format)
  {
      QTestState *s;
      int sock, qmpsock, i;
      gchar *socket_path;

tests/libqtest.c +0 -0

      /* It's possible that if an earlier test run crashed it might
       * have left a stale unix socket lying around. Delete any

@@ -199,12 +202,12 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
          command = g_strdup_printf("exec %s "

It is abstracted from qtest_init_without_qmp_handshake(). It works just like qtest_init_without_qmp_handshake() but further it would allow the caller to specify the QMP parameter.

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

---
  tests/libqtest.c | 14 +++++++++++---
  tests/libqtest.h | 14 ++++++++++++++
  2 files changed, 25 insertions(+), 3 deletions(-)

Test the new OOB capability. It's mostly the reverted OOB test, but

differs in that:

- It uses the new qtest_init_with_qmp_format() to create the monitor with the new monitor parameter "-mon x-oob"
- Squashed the capability tests on greeting message
- Don't use qtest_global any more, instead use self-maintained QTestState, which is the trend

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

---
  tests/qmp-test.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 84 insertions(+)

+++ b/tests/qmp-test.c

tests/qmp-test.c +2 -0

@@ -134,6 +134,89 @@ static void test_qmp_protocol(void)
      qtest_quit(qts);
  }
 
+/* Tests for Out-Of-Band support. */
+static void test_qmp_oob(void)

I suppose these are all good even for 2.12, so marked in subject. Tested with "make check" for all targets on x86_64, and iotest -raw.

Patch 1 fixes one OOB error message regression reported by Marc-Andre.

Patch 2 fixes one potential OOB problem when more than one clients are there, reported by Marc-Andre (too).

Patch 3 introduce "-mon x-oob=on" parameter to allow user to explicitly enable Out-Of-Band for a specific monitor.

Patch 4-6 are qapi-schema fixes and tests for Out-Of-Band.

Patch 7-8 add back the OOB test on the new parameter (with more enhancements).

Please review, thanks.

Peter Xu (8):
qmp: fix qmp_capabilities error regression
qmp: cleanup qmp queues properly
monitor: new parameter "x-oob"
qapi: restrict allow-oob value to be "true"
tests: let qapi-schema tests detect oob
tests: add oob-test for qapi-schema
tests: introduce qtest_init_with_qmp_format()
tests: qmp-test: add test for new "x-oob"

 include/monitor/monitor.h               |   1 +
 monitor.c                               | 124 ++++++++++++++++++++++----------
 scripts/qapi/common.py                  |   2 +-
 tests/Makefile.include                  |   1 +
 tests/libqtest.c                        |  14 +++-
 tests/libqtest.h                        |  14 ++++
 tests/qapi-schema/doc-good.out          |   4 +-
 tests/qapi-schema/ident-with-escape.out |   2 +-
 tests/qapi-schema/indented-expr.out     |   4 +-
 tests/qapi-schema/oob-test.err          |   1 +
 tests/qapi-schema/oob-test.exit         |   1 +
 tests/qapi-schema/oob-test.json         |   2 +
 tests/qapi-schema/oob-test.out          |   0
 tests/qapi-schema/qapi-schema-test.json |   3 +
 tests/qapi-schema/qapi-schema-test.out  |  20 +++---
 tests/qapi-schema/test-qapi.py          |   4 +-
 tests/qmp-test.c                        |  84 ++++++++++++++++++++++
 tests/test-qmp-cmds.c                   |   4 ++
 vl.c                                    |   5 ++
 19 files changed, 233 insertions(+), 57 deletions(-)
 create mode 100644 tests/qapi-schema/oob-test.err
 create mode 100644 tests/qapi-schema/oob-test.exit
 create mode 100644 tests/qapi-schema/oob-test.json
 create mode 100644 tests/qapi-schema/oob-test.out

Thanks for the quick fixing. This series on top of master works fine on s390. (make check and iotests)

I suppose these are all good even for 2.12, so marked in subject. Tested with "make check" for all targets on x86_64, and iotest -raw.

Patch 1 fixes one OOB error message regression reported by Marc-Andre.

Patch 2 fixes one potential OOB problem when more than one clients are there, reported by Marc-Andre (too).

Patch 3 introduce "-mon x-oob=on" parameter to allow user to explicitly enable Out-Of-Band for a specific monitor.

Patch 4-6 are qapi-schema fixes and tests for Out-Of-Band.

Patch 7-8 add back the OOB test on the new parameter (with more enhancements).

Please review, thanks.

Peter Xu (8):
qmp: fix qmp_capabilities error regression
qmp: cleanup qmp queues properly
monitor: new parameter "x-oob"
qapi: restrict allow-oob value to be "true"
tests: let qapi-schema tests detect oob
tests: add oob-test for qapi-schema
tests: introduce qtest_init_with_qmp_format()
tests: qmp-test: add test for new "x-oob"