[Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests)

[Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests)

[Peter Xu]

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

-- 
2.14.3

[Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression

[Peter Xu]

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 <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/monitor.c b/monitor.c
index 77f4c41cfa..849fa23bf9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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

[Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly

[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

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 <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 59 insertions(+), 20 deletions(-)

diff --git a/monitor.c b/monitor.c
index 849fa23bf9..eba98df9da 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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

[Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"

[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.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/monitor/monitor.h |  1 +
 monitor.c                 | 22 ++++++++++++++++++++--
 vl.c                      |  5 +++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 0cb0538a31..d6ab70cae2 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -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);
 
diff --git a/monitor.c b/monitor.c
index eba98df9da..d77ccc8785 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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) {
+        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);
+        }
+    }
 
-    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 */ }
     },
diff --git a/vl.c b/vl.c
index c81cc86607..5fd01bd5f6 100644
--- a/vl.c
+++ b/vl.c
@@ -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

[Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true"

[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 <peterx@redhat.com>
---
 scripts/qapi/common.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 2c05e3c284..3e14bc41f2 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -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

[Qemu-devel] [PATCH for-2.12 5/8] tests: let qapi-schema tests detect oob

[Peter Xu]

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 <eblake@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 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(-)

diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 430b5a87db..63058b1590 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -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
diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index ee3b34e623..82213aa51d 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -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
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index a79935e8c3..862678f8f4 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -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
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 012e7fc06a..4f43370017 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -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
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 10e68b01d9..c1a144ba29 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -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

[Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema

[Peter Xu]

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

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 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

diff --git a/tests/Makefile.include b/tests/Makefile.include
index eb218a9539..3b9a5e31a2 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -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
diff --git a/tests/qapi-schema/oob-test.err b/tests/qapi-schema/oob-test.err
new file mode 100644
index 0000000000..35b60f7480
--- /dev/null
+++ b/tests/qapi-schema/oob-test.err
@@ -0,0 +1 @@
+tests/qapi-schema/oob-test.json:2: 'allow-oob' of command 'oob-command-1' should only use true value
diff --git a/tests/qapi-schema/oob-test.exit b/tests/qapi-schema/oob-test.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/oob-test.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/oob-test.json b/tests/qapi-schema/oob-test.json
new file mode 100644
index 0000000000..da9635920f
--- /dev/null
+++ b/tests/qapi-schema/oob-test.json
@@ -0,0 +1,2 @@
+# Check against oob illegal value
+{ 'command': 'oob-command-1', 'allow-oob': 'some-string' }
diff --git a/tests/qapi-schema/oob-test.out b/tests/qapi-schema/oob-test.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index c72dbd8050..06e30f452e 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -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:
 #
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 4f43370017..467577d770 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -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
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 93fbbb1b73..db690cc5ae 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -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

[Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format()

[Peter Xu]

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 <peterx@redhat.com>
---
 tests/libqtest.c | 14 +++++++++++---
 tests/libqtest.h | 14 ++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 200b2b9e92..d2af1b17f0 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -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);
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 811169453a..1f3605ce73 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -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

[Qemu-devel] [PATCH for-2.12 8/8] tests: qmp-test: add test for new "x-oob"

[Peter Xu]

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 <peterx@redhat.com>
---
 tests/qmp-test.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 558e83540c..4d15b0fca5 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -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)
+{
+    QTestState *qts;
+    QDict *resp, *q;
+    int acks = 0;
+    const QListEntry *entry;
+    QList *capabilities;
+    QString *qstr;
+    const char *cmd_id;
+    const char *qmp_params = "-chardev socket,path=%s,nowait,id=char0 "
+                             "-mon chardev=char0,mode=control,x-oob=on";
+
+    qts = qtest_init_with_qmp_format(common_args, qmp_params);
+
+    /* Ignore the greeting message. */
+    resp = qtest_qmp_receive(qts);
+    q = qdict_get_qdict(resp, "QMP");
+    g_assert(q);
+    capabilities = qdict_get_qlist(q, "capabilities");
+    g_assert(capabilities && !qlist_empty(capabilities));
+    entry = qlist_first(capabilities);
+    g_assert(entry);
+    qstr = qobject_to(QString, entry->value);
+    g_assert(qstr);
+    g_assert_cmpstr(qstring_get_str(qstr), ==, "oob");
+    QDECREF(resp);
+
+    /* Try a fake capability, it should fail. */
+    resp = qtest_qmp(qts,
+                     "{ 'execute': 'qmp_capabilities', "
+                     "  'arguments': { 'enable': [ 'cap-does-not-exist' ] } }");
+    g_assert(qdict_haskey(resp, "error"));
+    QDECREF(resp);
+
+    /* Now, enable OOB in current QMP session, it should succeed. */
+    resp = qtest_qmp(qts,
+                     "{ 'execute': 'qmp_capabilities', "
+                     "  'arguments': { 'enable': [ 'oob' ] } }");
+    g_assert(qdict_haskey(resp, "return"));
+    QDECREF(resp);
+
+    /*
+     * Try any command that does not support OOB but with OOB flag. We
+     * should get failure.
+     */
+    resp = qtest_qmp(qts,
+                     "{ 'execute': 'query-cpus',"
+                     "  'control': { 'run-oob': true } }");
+    g_assert(qdict_haskey(resp, "error"));
+    QDECREF(resp);
+
+    /*
+     * First send the "x-oob-test" command with lock=true and
+     * oob=false, it should hang the dispatcher and main thread;
+     * later, we send another lock=false with oob=true to continue
+     * that thread processing.  Finally we should receive replies from
+     * both commands.
+     */
+    qtest_async_qmp(qts,
+                    "{ 'execute': 'x-oob-test',"
+                    "  'arguments': { 'lock': true }, "
+                    "  'id': 'lock-cmd'}");
+    qtest_async_qmp(qts,
+                    "{ 'execute': 'x-oob-test', "
+                    "  'arguments': { 'lock': false }, "
+                    "  'control': { 'run-oob': true }, "
+                    "  'id': 'unlock-cmd' }");
+
+    /* Ignore all events.  Wait for 2 acks */
+    while (acks < 2) {
+        resp = qtest_qmp_receive(qts);
+        cmd_id = qdict_get_str(resp, "id");
+        if (!g_strcmp0(cmd_id, "lock-cmd") ||
+            !g_strcmp0(cmd_id, "unlock-cmd")) {
+            acks++;
+        }
+        QDECREF(resp);
+    }
+
+    qtest_quit(qts);
+}
+
 static int query_error_class(const char *cmd)
 {
     static struct {
@@ -318,6 +401,7 @@ int main(int argc, char *argv[])
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("qmp/protocol", test_qmp_protocol);
+    qtest_add_func("qmp/oob", test_qmp_oob);
     qmp_schema_init(&schema);
     add_query_tests(&schema);
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression

[Marc-André Lureau]

On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> 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 <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  monitor.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 77f4c41cfa..849fa23bf9 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -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
>

Re: [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly

[Marc-André Lureau]

On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> 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 <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 59 insertions(+), 20 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 849fa23bf9..eba98df9da 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -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
>

Re: [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"

[Marc-André Lureau]

Hi

On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> 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 <peterx@redhat.com>
> ---
>  include/monitor/monitor.h |  1 +
>  monitor.c                 | 22 ++++++++++++++++++++--
>  vl.c                      |  5 +++++
>  3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 0cb0538a31..d6ab70cae2 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -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);
>
> diff --git a/monitor.c b/monitor.c
> index eba98df9da..d77ccc8785 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -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) {
> +        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);
> +        }
> +    }

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

>
> -    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 */ }
>      },
> diff --git a/vl.c b/vl.c
> index c81cc86607..5fd01bd5f6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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
>

Other than that, it looks fine.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true"

[Marc-André Lureau]

On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> 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 <peterx@redhat.com>

Not exactly required imho, but why not:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  scripts/qapi/common.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 2c05e3c284..3e14bc41f2 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -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
>

Re: [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"

[Peter Xu]

On Mon, Mar 26, 2018 at 11:10:30AM +0200, Marc-André Lureau wrote:

[...]

> > @@ -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);
> > +        }
> > +        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()

But there can be other callers for monitor_init() so I would assume
checking it here would be safer (though for now indeed mon_init_func()
is the only one that can pass OOB flag in).

[...]

> Other than that, it looks fine.
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks for your quick reviews!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-2.12 5/8] tests: let qapi-schema tests detect oob

[Marc-André Lureau]

On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> 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 <eblake@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  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(-)
>
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 430b5a87db..63058b1590 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -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
> diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
> index ee3b34e623..82213aa51d 100644
> --- a/tests/qapi-schema/ident-with-escape.out
> +++ b/tests/qapi-schema/ident-with-escape.out
> @@ -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
> diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
> index a79935e8c3..862678f8f4 100644
> --- a/tests/qapi-schema/indented-expr.out
> +++ b/tests/qapi-schema/indented-expr.out
> @@ -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
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 012e7fc06a..4f43370017 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -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
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 10e68b01d9..c1a144ba29 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -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
>

Re: [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format()

[Marc-André Lureau]

On Mon, Mar 26, 2018 at 8:39 AM, Peter Xu <peterx@redhat.com> wrote:
> 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 <peterx@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  tests/libqtest.c | 14 +++++++++++---
>  tests/libqtest.h | 14 ++++++++++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 200b2b9e92..d2af1b17f0 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -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);
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 811169453a..1f3605ce73 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -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
>

Re: [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema

[Marc-André Lureau]

On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> It simply tests the new OOB capability, and make sure the QAPISchema can
> parse it correctly.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  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
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index eb218a9539..3b9a5e31a2 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -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
> diff --git a/tests/qapi-schema/oob-test.err b/tests/qapi-schema/oob-test.err
> new file mode 100644
> index 0000000000..35b60f7480
> --- /dev/null
> +++ b/tests/qapi-schema/oob-test.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/oob-test.json:2: 'allow-oob' of command 'oob-command-1' should only use true value
> diff --git a/tests/qapi-schema/oob-test.exit b/tests/qapi-schema/oob-test.exit
> new file mode 100644
> index 0000000000..d00491fd7e
> --- /dev/null
> +++ b/tests/qapi-schema/oob-test.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/oob-test.json b/tests/qapi-schema/oob-test.json
> new file mode 100644
> index 0000000000..da9635920f
> --- /dev/null
> +++ b/tests/qapi-schema/oob-test.json
> @@ -0,0 +1,2 @@
> +# Check against oob illegal value
> +{ 'command': 'oob-command-1', 'allow-oob': 'some-string' }
> diff --git a/tests/qapi-schema/oob-test.out b/tests/qapi-schema/oob-test.out
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index c72dbd8050..06e30f452e 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -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:
>  #
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 4f43370017..467577d770 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -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
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index 93fbbb1b73..db690cc5ae 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -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
>

Re: [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests)

[Christian Borntraeger]

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

Christian



On 03/26/2018 08:38 AM, Peter Xu wrote:
> 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
> 

Re: [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression

[Eric Blake]

On 03/26/2018 01:38 AM, Peter Xu wrote:
> When someone sents a command before QMP handshake, error was like this:

s/sents/sends/
s/error was/the error used to be/

> 
>   {"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 <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   monitor.c | 23 ++++++++---------------
>   1 file changed, 8 insertions(+), 15 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly

[Eric Blake]

On 03/26/2018 01:38 AM, Peter Xu wrote:
> 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

Why here? [1]

> - 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.

> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 59 insertions(+), 20 deletions(-)
> 

> +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...

> +
> +/* 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));

...here,

> +    }
> +}
> +
> +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);

and here.

> @@ -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?

>           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--;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true"

[Eric Blake]

On 03/26/2018 04:11 AM, Marc-André Lureau wrote:
> On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
>> 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 <peterx@redhat.com>
> 
> Not exactly required imho, but why not:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Not technically required, but does add some consistency to the QAPI parser.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"

[Eric Blake]

On 03/26/2018 04:10 AM, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
>> 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.

Worth documenting in the commit message at least that even when OOB is 
enabled, the client must STILL opt-in to using it by replying correctly 
to qmp_capabilities, as well as mention that in the future, we may 
remove x-oob and rely on JUST qmp_capabilities for enabling OOB.


>> @@ -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);
>> +        }
>> +        if (use_readline) {
>> +            error_report("Monitor Out-Of-band is only supported by QMP");
>> +            exit(1);

Should these two checks be swapped?  Otherwise, if you use a MUX-typed 
chardev for HMP, the message implies that switching chardev backend 
might make it work, even though if you actually do that you'd then get 
the failure for not being QMP.

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

To do that, monitor_init() should change signatures to take Error 
**errp.  Probably worth doing if you spin a v2 of this series (adding 
the parameter can be done as a separate patch, although there are only 5 
callers in the tree so adjusting the callers at the same time is 
probably not that hard to review).

> 
> Other than that, it looks fine.
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Okay, I'll see how my review goes on the rest of the series before 
deciding whether to request a v2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema

[Eric Blake]

On 03/26/2018 01:38 AM, Peter Xu wrote:
> It simply tests the new OOB capability, and make sure the QAPISchema can
> parse it correctly.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   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

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format()

[Eric Blake]

On 03/26/2018 01:39 AM, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>   tests/libqtest.c | 14 +++++++++++---
>   tests/libqtest.h | 14 ++++++++++++++
>   2 files changed, 25 insertions(+), 3 deletions(-)
> 

[Reviewing in reverse order; you may want to look at 
scripts/git.orderfile for how to present your patches in a more logical 
manner with .h changes first.]

 > diff --git a/tests/libqtest.h b/tests/libqtest.h
 > index 811169453a..1f3605ce73 100644
 > --- a/tests/libqtest.h
 > +++ b/tests/libqtest.h
 > @@ -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.

Not your fault, but I'm already not a fan of 'extra_args'; it would be 
better to make these functions take an array of arguments, or even use 
varargs, instead of relying on shell word splitting.  Our testsuite is a 
gaping security hole if executed in a directory where a shell 
metacharacter causes the shell word splitting to do something different 
than planned.

 > + * @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);

Ouch - you didn't use any attribute to mark the format string so that 
the compiler can enforce that it is treated as a printf-style argument. 
I wonder if it would have been better to just have a 'bool use_oob' 
parameter rather than playing ugly games with passing printf-style 
format arguments around.

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

> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 200b2b9e92..d2af1b17f0 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -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);

And in addition to my earlier comment about not using a compiler 
attribute, you aren't even bothering to assert that the caller didn't 
pass in a garbage string, which can really lead to weird breakages.

>   
>       /* 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,

Again, if you used the idea of a 'bool use_oob',  this would look more like:

"-qmp unix:%s,nowait%s ",
...
qmp_socket_path, use_oob ? "x-oob=on" : "",

which is a lot more limited in scope to prevent auditing nightmares, 
while no less powerful for what you are actually using this new function 
for.

>                                     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");
> +}

There are so few callers of qtest_init_without_qmp_handshake() that I'd 
just add the parameter to the existing function and update its two 
callers, instead of adding yet another forwarding wrapper.  Especially 
if it is just adding a 'bool use_oob' parameter.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12 8/8] tests: qmp-test: add test for new "x-oob"

[Eric Blake]

On 03/26/2018 01:39 AM, Peter Xu wrote:
> Test the new OOB capability. It's mostly the reverted OOB test, but

Helpful to mention which commit id has the reverts you are restoring. 
Looks like 4fd78ad. Are you also restoring the tests reverted in cc79760 
and a4f9092?

> 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 <peterx@redhat.com>
> ---
>   tests/qmp-test.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 84 insertions(+)
> 

> +++ b/tests/qmp-test.c
> @@ -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)
> +{
> +    QTestState *qts;
> +    QDict *resp, *q;
> +    int acks = 0;
> +    const QListEntry *entry;
> +    QList *capabilities;
> +    QString *qstr;
> +    const char *cmd_id;
> +    const char *qmp_params = "-chardev socket,path=%s,nowait,id=char0 "
> +                             "-mon chardev=char0,mode=control,x-oob=on";

Again, I'd rather fold this into 7/8 qtest_init_without_qmp_handshake by 
changing "-qmp unix:%s,nowait" into "-chardev 
socket,path=%s,nowait,id=char0 -mon chardev=char0,mode=control" for ALL 
tests (as that always works), then have a bool that decides whether we 
also append ",x-oob=on" as needed.

> +
> +    qts = qtest_init_with_qmp_format(common_args, qmp_params);
> +
> +    /* Ignore the greeting message. */

Comment is wrong - you aren't ignoring it, but...

> +    resp = qtest_qmp_receive(qts);
> +    q = qdict_get_qdict(resp, "QMP");
> +    g_assert(q);
> +    capabilities = qdict_get_qlist(q, "capabilities");
> +    g_assert(capabilities && !qlist_empty(capabilities));

...actually inspecting what it contains.

> +    entry = qlist_first(capabilities);
> +    g_assert(entry);
> +    qstr = qobject_to(QString, entry->value);
> +    g_assert(qstr);
> +    g_assert_cmpstr(qstring_get_str(qstr), ==, "oob");
> +    QDECREF(resp);
> +

In the interest of getting this in -rc1, I will probably try and make 
the changes I've mentioned in this thread, and post a v2 along those 
lines; if you like my changes, I can send the pull request on my Tuesday 
morning instead of waiting for another round of back-and-forth.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format()

[Eric Blake]

On 03/26/2018 01:39 AM, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>   tests/libqtest.c | 14 +++++++++++---
>   tests/libqtest.h | 14 ++++++++++++++
>   2 files changed, 25 insertions(+), 3 deletions(-)
> 

> +    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);
>   

Memory leak of qmp_params.  Avoided in my rewrite to use bool instead of 
a format string.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests)

[Eric Blake]

On 03/26/2018 01:38 AM, Peter Xu wrote:
> 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"

Queuing 1, 4-6 without further changes.  I'll repost a v2 of 2, 3, my 
rewrite of 7, and 8.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests)

[Peter Xu]

On Mon, Mar 26, 2018 at 12:18:28PM +0200, Christian Borntraeger wrote:
> Thanks for the quick fixing. This series on top of master
> works fine on s390. (make check and iotests)

Appreciate the quick follow up.  Thanks Christian.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly

[Peter Xu]

On Mon, Mar 26, 2018 at 02:42:23PM -0500, Eric Blake wrote:
> On 03/26/2018 01:38 AM, Peter Xu wrote:
> > 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
> 
> Why here? [1]

Yes it can be omitted. We can do it either here or closing, the only
difference should be that when added here there's more possibility
that the pending commands (requests from disconnected clients) be
executed rather than dropped.

Here I did the cleanup on both places.  Drop any of them would be fine
too.

> 
> > - 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.
> 
> > Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
> >   1 file changed, 59 insertions(+), 20 deletions(-)
> > 
> 
> > +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...

Yes we can omit that.

> 
> > +
> > +/* 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));
> 
> ...here,
> 
> > +    }
> > +}
> > +
> > +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);
> 
> and here.
> 
> > @@ -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?

(answered above)

Thanks,

-- 
Peter Xu