[Qemu-devel] [PATCH v2 00/18] monitor: various code simplification and fixes

[Qemu-devel] [PATCH v2 00/18] monitor: various code simplification and fixes

[Marc-André Lureau]

Hi,

This series is a rebased subset of "[PATCH v3 00/38] RFC: monitor: add
asynchronous command type" with code cleanups and improvements that
are worth to consider for 3.0.

The series applies on master, and will conflict with the pending
series "[PATCH 0/9] monitor: enable OOB by default" from Peter.

In particular, reverting "qmp: isolate responses into io thread" is
quite intrusive, but is a nice simplification that is worth to
consider before modifying/maintaining it further. Also, "monitor: no
need to save need_resume" could be dropped if Peter "[PATCH 5/9]
monitor: suspend monitor instead of send CMD_DROP" is adopted (see
discussion about need_resume there).

The last patch, "RFC: qmp: rework 'id' handling" simplifies a bit
monitor "id" handling, and makes qemu-ga conform to the QMP
specification by copying "id" from the request in the reply.  This is
the opposite to Markus change "qmp qemu-ga: Revert change that
accidentally made qemu-ga accept 'id'".

(the first patch is already in Markus qapi-next branch, it is there
for patchew testing)

I would suggest the following patches for 3.0:
 - monitor: consitify qmp_send_response() QDict argument
 - qmp: constify qmp_is_oob()
 - qga: process_event() simplification and leak fix
 - qmp: drop json_parser_parse() wrapper
 - json-parser: simplify and avoid JSONParserContext allocation
 - json-parser: further simplify freeing JSONParserContext
 - qjson: report an error if there are multiple results
 - qjson: report error on unterminated string
 - qjson: return parsing error if unterminated input
 - json-parser: set an error if parsing returned NULL
 - tests: add a qmp success-response test
 - qga: process_event() simplification

(they apply cleanly when cherry-picked, the remaining cleanups could be
defered after the release)

v2:
- add a code comment about need_resume variable
- update "qga: process_event() simplification" patch with req/rsp
  variables
- add "qmp: drop json_parser_parse()" patch
- add JSONParserContext allocation simplification patches
- add "qjson: report an error if there are multiple results"
- add "qjson: report error on unterminated string"
- add "qjson: return parsing error if unterminated input"
- document json_parser_parse() return behaviour
- add r-b tags

Marc-André Lureau (18):
  tests: change /0.15/* tests to /qmp/*
  monitor: consitify qmp_send_response() QDict argument
  qmp: constify qmp_is_oob()
  Revert "qmp: isolate responses into io thread"
  monitor: no need to save need_resume
  qga: process_event() simplification and leak fix
  qmp: drop json_parser_parse() wrapper
  json-parser: simplify and avoid JSONParserContext allocation
  json-parser: further simplify freeing JSONParserContext
  qjson: report an error if there are multiple results
  qjson: report error on unterminated string
  qjson: return parsing error if unterminated input
  json-parser: set an error if parsing returned NULL
  json-lexer: make it safe to call multiple times
  tests: add a few qemu-qmp tests
  tests: add a qmp success-response test
  qga: process_event() simplification
  RFC: qmp: common 'id' handling & make QGA conform to QMP spec

 include/qapi/qmp/dispatch.h             |   2 +-
 include/qapi/qmp/json-parser.h          |   3 +-
 block.c                                 |   5 -
 monitor.c                               | 173 +++---------------------
 qapi/qmp-dispatch.c                     |  14 +-
 qapi/qobject-input-visitor.c            |   5 -
 qga/main.c                              |  66 +++------
 qobject/json-lexer.c                    |   5 +-
 qobject/json-parser.c                   |  59 +++-----
 qobject/json-streamer.c                 |   4 +-
 qobject/qjson.c                         |  24 +++-
 tests/check-qjson.c                     |  25 +++-
 tests/libqtest.c                        |   2 +-
 tests/qmp-test.c                        |  38 ++++++
 tests/test-qga.c                        |  13 +-
 tests/test-qmp-cmds.c                   |  27 +++-
 tests/qapi-schema/qapi-schema-test.json |   2 +
 tests/qapi-schema/qapi-schema-test.out  |   2 +
 18 files changed, 190 insertions(+), 279 deletions(-)

-- 
2.18.0.129.ge3331758f1

[Qemu-devel] [PATCH v2 01/18] tests: change /0.15/* tests to /qmp/*

[Marc-André Lureau]

Presumably 0.15 was the version it was first introduced, but
qmp keeps evolving. There is no point in having that version
as test prefix, 'qmp' makes more sense here.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20180326150916.9602-12-marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qmp-cmds.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index ba41a6161e..ab414fa0c9 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -286,11 +286,11 @@ int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
-    g_test_add_func("/0.15/dispatch_cmd", test_dispatch_cmd);
-    g_test_add_func("/0.15/dispatch_cmd_failure", test_dispatch_cmd_failure);
-    g_test_add_func("/0.15/dispatch_cmd_io", test_dispatch_cmd_io);
-    g_test_add_func("/0.15/dealloc_types", test_dealloc_types);
-    g_test_add_func("/0.15/dealloc_partial", test_dealloc_partial);
+    g_test_add_func("/qmp/dispatch_cmd", test_dispatch_cmd);
+    g_test_add_func("/qmp/dispatch_cmd_failure", test_dispatch_cmd_failure);
+    g_test_add_func("/qmp/dispatch_cmd_io", test_dispatch_cmd_io);
+    g_test_add_func("/qmp/dealloc_types", test_dealloc_types);
+    g_test_add_func("/qmp/dealloc_partial", test_dealloc_partial);
 
     test_qmp_init_marshal(&qmp_commands);
     g_test_run();
-- 
2.18.0.129.ge3331758f1

[Qemu-devel] [PATCH v2 02/18] monitor: consitify qmp_send_response() QDict argument

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index be29634a00..adc04a01d3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -504,9 +504,9 @@ int monitor_fprintf(FILE *stream, const char *fmt, ...)
     return 0;
 }
 
-static void qmp_send_response(Monitor *mon, QDict *rsp)
+static void qmp_send_response(Monitor *mon, const QDict *rsp)
 {
-    QObject *data = QOBJECT(rsp);
+    const QObject *data = QOBJECT(rsp);
     QString *json;
 
     json = mon->flags & MONITOR_USE_PRETTY ? qobject_to_json_pretty(data) :
-- 
2.18.0.129.ge3331758f1

[Qemu-devel] [PATCH v2 03/18] qmp: constify qmp_is_oob()

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/dispatch.h | 2 +-
 qapi/qmp-dispatch.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 4e2e749faf..68a528a9aa 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -50,7 +50,7 @@ bool qmp_has_success_response(const QmpCommand *cmd);
 QDict *qmp_error_response(Error *err);
 QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
                     bool allow_oob);
-bool qmp_is_oob(QDict *dict);
+bool qmp_is_oob(const QDict *dict);
 
 typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 6f2d466596..90ba5e3074 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -156,7 +156,7 @@ QDict *qmp_error_response(Error *err)
 /*
  * Does @qdict look like a command to be run out-of-band?
  */
-bool qmp_is_oob(QDict *dict)
+bool qmp_is_oob(const QDict *dict)
 {
     return qdict_haskey(dict, "exec-oob")
         && !qdict_haskey(dict, "execute");
-- 
2.18.0.129.ge3331758f1

[Qemu-devel] [PATCH v2 04/18] Revert "qmp: isolate responses into io thread"

[Marc-André Lureau]

This reverts commit abe3cd0ff7f774966da6842620806ab7576fe4f3.

There is no need to add an additional queue to send the reply to the
IOThread, because QMP response is thread safe, and chardev write path
is thread safe. It will schedule the watcher in the associated
IOThread.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 120 ++----------------------------------------------------
 1 file changed, 3 insertions(+), 117 deletions(-)

diff --git a/monitor.c b/monitor.c
index adc04a01d3..46d98010f7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -183,8 +183,6 @@ typedef struct {
     QemuMutex qmp_queue_lock;
     /* Input queue that holds all the parsed QMP requests */
     GQueue *qmp_requests;
-    /* Output queue contains all the QMP responses in order */
-    GQueue *qmp_responses;
 } MonitorQMP;
 
 /*
@@ -248,9 +246,6 @@ IOThread *mon_iothread;
 /* Bottom half to dispatch the requests received from I/O thread */
 QEMUBH *qmp_dispatcher_bh;
 
-/* Bottom half to deliver the responses back to clients */
-QEMUBH *qmp_respond_bh;
-
 struct QMPRequest {
     /* Owner of the request */
     Monitor *mon;
@@ -376,19 +371,10 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
     }
 }
 
-/* Caller must hold the mon->qmp.qmp_queue_lock */
-static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
-{
-    while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
-        qobject_unref((QDict *)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);
 }
 
@@ -519,85 +505,6 @@ static void qmp_send_response(Monitor *mon, const QDict *rsp)
     qobject_unref(json);
 }
 
-static void qmp_queue_response(Monitor *mon, QDict *rsp)
-{
-    if (mon->use_io_thread) {
-        /*
-         * Push a reference to the response queue.  The I/O thread
-         * drains that queue and emits.
-         */
-        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
-        g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp));
-        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
-        qemu_bh_schedule(qmp_respond_bh);
-    } else {
-        /*
-         * Not using monitor I/O thread, i.e. we are in the main thread.
-         * Emit right away.
-         */
-        qmp_send_response(mon, rsp);
-    }
-}
-
-struct QMPResponse {
-    Monitor *mon;
-    QDict *data;
-};
-typedef struct QMPResponse QMPResponse;
-
-static QDict *monitor_qmp_response_pop_one(Monitor *mon)
-{
-    QDict *data;
-
-    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
-    data = g_queue_pop_head(mon->qmp.qmp_responses);
-    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
-
-    return data;
-}
-
-static void monitor_qmp_response_flush(Monitor *mon)
-{
-    QDict *data;
-
-    while ((data = monitor_qmp_response_pop_one(mon))) {
-        qmp_send_response(mon, data);
-        qobject_unref(data);
-    }
-}
-
-/*
- * Pop a QMPResponse from any monitor's response queue into @response.
- * Return false if all the queues are empty; else true.
- */
-static bool monitor_qmp_response_pop_any(QMPResponse *response)
-{
-    Monitor *mon;
-    QDict *data = NULL;
-
-    qemu_mutex_lock(&monitor_lock);
-    QTAILQ_FOREACH(mon, &mon_list, entry) {
-        data = monitor_qmp_response_pop_one(mon);
-        if (data) {
-            response->mon = mon;
-            response->data = data;
-            break;
-        }
-    }
-    qemu_mutex_unlock(&monitor_lock);
-    return data != NULL;
-}
-
-static void monitor_qmp_bh_responder(void *opaque)
-{
-    QMPResponse response;
-
-    while (monitor_qmp_response_pop_any(&response)) {
-        qmp_send_response(response.mon, response.data);
-        qobject_unref(response.data);
-    }
-}
-
 static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     /* Limit guest-triggerable events to 1 per second */
     [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
@@ -621,7 +528,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)
     QTAILQ_FOREACH(mon, &mon_list, entry) {
         if (monitor_is_qmp(mon)
             && mon->qmp.commands != &qmp_cap_negotiation_commands) {
-            qmp_queue_response(mon, qdict);
+            qmp_send_response(mon, qdict);
         }
     }
 }
@@ -777,7 +684,6 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
     mon->skip_flush = skip_flush;
     mon->use_io_thread = use_io_thread;
     mon->qmp.qmp_requests = g_queue_new();
-    mon->qmp.qmp_responses = g_queue_new();
 }
 
 static void monitor_data_destroy(Monitor *mon)
@@ -792,9 +698,7 @@ static void monitor_data_destroy(Monitor *mon)
     qemu_mutex_destroy(&mon->mon_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);
 }
 
 char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
@@ -4100,7 +4004,7 @@ static void monitor_qmp_respond(Monitor *mon, QDict *rsp, QObject *id)
             qdict_put_obj(rsp, "id", qobject_ref(id));
         }
 
-        qmp_queue_response(mon, rsp);
+        qmp_send_response(mon, rsp);
     }
 }
 
@@ -4396,7 +4300,7 @@ static void monitor_qmp_event(void *opaque, int event)
         mon->qmp.commands = &qmp_cap_negotiation_commands;
         monitor_qmp_caps_reset(mon);
         data = qmp_greeting(mon);
-        qmp_queue_response(mon, data);
+        qmp_send_response(mon, data);
         qobject_unref(data);
         mon_refcount++;
         break;
@@ -4407,7 +4311,6 @@ static void monitor_qmp_event(void *opaque, int event)
          * stdio, it's possible that stdout is still open when stdin
          * is closed.
          */
-        monitor_qmp_response_flush(mon);
         monitor_qmp_cleanup_queues(mon);
         json_message_parser_destroy(&mon->qmp.parser);
         json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
@@ -4509,15 +4412,6 @@ static void monitor_iothread_init(void)
     qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
                                    monitor_qmp_bh_dispatcher,
                                    NULL);
-
-    /*
-     * The responder BH must be run in the monitor I/O thread, so that
-     * monitors that are using the I/O thread have their output
-     * written by the I/O thread.
-     */
-    qmp_respond_bh = aio_bh_new(monitor_get_aio_context(),
-                                monitor_qmp_bh_responder,
-                                NULL);
 }
 
 void monitor_init_globals(void)
@@ -4669,12 +4563,6 @@ void monitor_cleanup(void)
      */
     iothread_stop(mon_iothread);
 
-    /*
-     * Flush all response queues.  Note that even after this flush,
-     * data may remain in output buffers.
-     */
-    monitor_qmp_bh_responder(NULL);
-
     /* Flush output buffers and destroy monitors */
     qemu_mutex_lock(&monitor_lock);
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
@@ -4688,8 +4576,6 @@ void monitor_cleanup(void)
     /* QEMUBHs needs to be deleted before destroying the I/O thread */
     qemu_bh_delete(qmp_dispatcher_bh);
     qmp_dispatcher_bh = NULL;
-    qemu_bh_delete(qmp_respond_bh);
-    qmp_respond_bh = NULL;
 
     iothread_destroy(mon_iothread);
     mon_iothread = NULL;
-- 
2.18.0.129.ge3331758f1

[Qemu-devel] [PATCH v2 05/18] monitor: no need to save need_resume

[Marc-André Lureau]

There is no need for per-command need_resume granularity, it should
resume after running an non-oob command on oob-disabled monitor.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index 46d98010f7..961779032a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -257,12 +257,6 @@ struct QMPRequest {
      */
     QObject *req;
     Error *err;
-    /*
-     * 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;
 
@@ -4079,11 +4073,14 @@ static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any();
     QDict *rsp;
+    bool need_resume;
 
     if (!req_obj) {
         return;
     }
 
+    /*  qmp_oob_enabled() might change after "qmp_capabilities" */
+    need_resume = !qmp_oob_enabled(req_obj->mon);
     if (req_obj->req) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
         monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
@@ -4095,7 +4092,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
         qobject_unref(rsp);
     }
 
-    if (req_obj->need_resume) {
+    if (need_resume) {
         /* Pairs with the monitor_suspend() in handle_qmp_command() */
         monitor_resume(req_obj->mon);
     }
@@ -4147,7 +4144,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     req_obj->id = id;
     req_obj->req = req;
     req_obj->err = err;
-    req_obj->need_resume = false;
 
     /* Protect qmp_requests and fetching its length. */
     qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
@@ -4160,7 +4156,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
      */
     if (!qmp_oob_enabled(mon)) {
         monitor_suspend(mon);
-        req_obj->need_resume = true;
     } else {
         /* Drop the request if queue is full. */
         if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
-- 
2.18.0.129.ge3331758f1

[Qemu-devel] [PATCH v2 06/18] qga: process_event() simplification and leak fix

[Marc-André Lureau]

json_parser_parse_err() may return something else than a QDict, in
which case we loose the object. Let's keep track of the original
object to avoid leaks.

When an error occurs, "qdict" contains the response, but we still
check the "execute" key there. Untangle a bit this code, by having a
clear error path.

CC: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 qga/main.c | 54 +++++++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 537cc0e162..87372d40ef 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -600,42 +600,42 @@ static void process_command(GAState *s, QDict *req)
 static void process_event(JSONMessageParser *parser, GQueue *tokens)
 {
     GAState *s = container_of(parser, GAState, parser);
-    QDict *qdict;
+    QObject *obj;
+    QDict *req, *rsp;
     Error *err = NULL;
     int ret;
 
     g_assert(s && parser);
 
     g_debug("process_event: called");
-    qdict = qobject_to(QDict, json_parser_parse_err(tokens, NULL, &err));
-    if (err || !qdict) {
-        qobject_unref(qdict);
-        if (!err) {
-            g_warning("failed to parse event: unknown error");
-            error_setg(&err, QERR_JSON_PARSING);
-        } else {
-            g_warning("failed to parse event: %s", error_get_pretty(err));
-        }
-        qdict = qmp_error_response(err);
+    obj = json_parser_parse_err(tokens, NULL, &err);
+    if (err) {
+        goto err;
     }
-
-    /* handle host->guest commands */
-    if (qdict_haskey(qdict, "execute")) {
-        process_command(s, qdict);
-    } else {
-        if (!qdict_haskey(qdict, "error")) {
-            qobject_unref(qdict);
-            g_warning("unrecognized payload format");
-            error_setg(&err, QERR_UNSUPPORTED);
-            qdict = qmp_error_response(err);
-        }
-        ret = send_response(s, qdict);
-        if (ret < 0) {
-            g_warning("error sending error response: %s", strerror(-ret));
-        }
+    req = qobject_to(QDict, obj);
+    if (!req) {
+        error_setg(&err, QERR_JSON_PARSING);
+        goto err;
+    }
+    if (!qdict_haskey(req, "execute")) {
+        g_warning("unrecognized payload format");
+        error_setg(&err, QERR_UNSUPPORTED);
+        goto err;
     }
 
-    qobject_unref(qdict);
+    process_command(s, req);
+    qobject_unref(obj);
+    return;
+
+err:
+    g_warning("failed to parse event: %s", error_get_pretty(err));
+    rsp = qmp_error_response(err);
+    ret = send_response(s, rsp);
+    if (ret < 0) {
+        g_warning("error sending error response: %s", strerror(-ret));
+    }
+    qobject_unref(rsp);
+    qobject_unref(obj);
 }
 
 /* false return signals GAChannel to close the current client connection */
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 06/18] qga: process_event() simplification and leak fix

[Michael Roth]

Quoting Marc-André Lureau (2018-07-19 13:40:59)
> json_parser_parse_err() may return something else than a QDict, in
> which case we loose the object. Let's keep track of the original
> object to avoid leaks.
> 
> When an error occurs, "qdict" contains the response, but we still
> check the "execute" key there. Untangle a bit this code, by having a
> clear error path.
> 
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks, I've gone ahead and queued this one for 3.0/stable:
  https://github.com/mdroth/qemu/commits/qga

> ---
>  qga/main.c | 54 +++++++++++++++++++++++++++---------------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 537cc0e162..87372d40ef 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -600,42 +600,42 @@ static void process_command(GAState *s, QDict *req)
>  static void process_event(JSONMessageParser *parser, GQueue *tokens)
>  {
>      GAState *s = container_of(parser, GAState, parser);
> -    QDict *qdict;
> +    QObject *obj;
> +    QDict *req, *rsp;
>      Error *err = NULL;
>      int ret;
> 
>      g_assert(s && parser);
> 
>      g_debug("process_event: called");
> -    qdict = qobject_to(QDict, json_parser_parse_err(tokens, NULL, &err));
> -    if (err || !qdict) {
> -        qobject_unref(qdict);
> -        if (!err) {
> -            g_warning("failed to parse event: unknown error");
> -            error_setg(&err, QERR_JSON_PARSING);
> -        } else {
> -            g_warning("failed to parse event: %s", error_get_pretty(err));
> -        }
> -        qdict = qmp_error_response(err);
> +    obj = json_parser_parse_err(tokens, NULL, &err);
> +    if (err) {
> +        goto err;
>      }
> -
> -    /* handle host->guest commands */
> -    if (qdict_haskey(qdict, "execute")) {
> -        process_command(s, qdict);
> -    } else {
> -        if (!qdict_haskey(qdict, "error")) {
> -            qobject_unref(qdict);
> -            g_warning("unrecognized payload format");
> -            error_setg(&err, QERR_UNSUPPORTED);
> -            qdict = qmp_error_response(err);
> -        }
> -        ret = send_response(s, qdict);
> -        if (ret < 0) {
> -            g_warning("error sending error response: %s", strerror(-ret));
> -        }
> +    req = qobject_to(QDict, obj);
> +    if (!req) {
> +        error_setg(&err, QERR_JSON_PARSING);
> +        goto err;
> +    }
> +    if (!qdict_haskey(req, "execute")) {
> +        g_warning("unrecognized payload format");
> +        error_setg(&err, QERR_UNSUPPORTED);
> +        goto err;
>      }
> 
> -    qobject_unref(qdict);
> +    process_command(s, req);
> +    qobject_unref(obj);
> +    return;
> +
> +err:
> +    g_warning("failed to parse event: %s", error_get_pretty(err));
> +    rsp = qmp_error_response(err);
> +    ret = send_response(s, rsp);
> +    if (ret < 0) {
> +        g_warning("error sending error response: %s", strerror(-ret));
> +    }
> +    qobject_unref(rsp);
> +    qobject_unref(obj);
>  }
> 
>  /* false return signals GAChannel to close the current client connection */
> -- 
> 2.18.0.129.ge3331758f1
> 

[Qemu-devel] [PATCH v2 07/18] qmp: drop json_parser_parse() wrapper

[Marc-André Lureau]

Stupid wrapper that's used exactly once, in libqtest.c.  Let's use
json_parser_parse_err() there, and drop the wrapper.  Let's rename
json_parser_parse_err() to json_parser_parse() then.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/json-parser.h | 3 +--
 monitor.c                      | 4 ++--
 qga/main.c                     | 2 +-
 qobject/json-parser.c          | 7 +------
 qobject/qjson.c                | 2 +-
 tests/libqtest.c               | 2 +-
 6 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/include/qapi/qmp/json-parser.h b/include/qapi/qmp/json-parser.h
index 102f5c0068..a34209db7a 100644
--- a/include/qapi/qmp/json-parser.h
+++ b/include/qapi/qmp/json-parser.h
@@ -16,7 +16,6 @@
 
 #include "qemu-common.h"
 
-QObject *json_parser_parse(GQueue *tokens, va_list *ap);
-QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp);
+QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp);
 
 #endif
diff --git a/monitor.c b/monitor.c
index 961779032a..2abb3c2fc7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4113,9 +4113,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     Error *err = NULL;
     QMPRequest *req_obj;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
+    req = json_parser_parse(tokens, NULL, &err);
     if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
+        /* json_parser_parse() sucks: can fail without setting @err */
         error_setg(&err, QERR_JSON_PARSING);
     }
 
diff --git a/qga/main.c b/qga/main.c
index 87372d40ef..043f7c3ead 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -608,7 +608,7 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens)
     g_assert(s && parser);
 
     g_debug("process_event: called");
-    obj = json_parser_parse_err(tokens, NULL, &err);
+    obj = json_parser_parse(tokens, NULL, &err);
     if (err) {
         goto err;
     }
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index a5aa790d62..9a7004e680 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -575,12 +575,7 @@ static QObject *parse_value(JSONParserContext *ctxt, va_list *ap)
     }
 }
 
-QObject *json_parser_parse(GQueue *tokens, va_list *ap)
-{
-    return json_parser_parse_err(tokens, ap, NULL);
-}
-
-QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp)
+QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
 {
     JSONParserContext *ctxt = parser_context_new(tokens);
     QObject *result;
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 2f6a590e44..ee04e61096 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -36,7 +36,7 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
 {
     JSONParsingState *s = container_of(parser, JSONParsingState, parser);
 
-    s->result = json_parser_parse_err(tokens, s->ap, &s->err);
+    s->result = json_parser_parse(tokens, s->ap, &s->err);
 }
 
 QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 098af6aec4..3826b8baf3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -432,7 +432,7 @@ static void qmp_response(JSONMessageParser *parser, GQueue *tokens)
     QMPResponseParser *qmp = container_of(parser, QMPResponseParser, parser);
     QObject *obj;
 
-    obj = json_parser_parse(tokens, NULL);
+    obj = json_parser_parse(tokens, NULL, NULL);
     if (!obj) {
         fprintf(stderr, "QMP JSON response parsing failed\n");
         exit(1);
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 07/18] qmp: drop json_parser_parse() wrapper

[Markus Armbruster]

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

> Stupid wrapper that's used exactly once, in libqtest.c.  Let's use
> json_parser_parse_err() there, and drop the wrapper.  Let's rename
> json_parser_parse_err() to json_parser_parse() then.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v2 08/18] json-parser: simplify and avoid JSONParserContext allocation

[Marc-André Lureau]

parser_context_new/free() are only used from json_parser_parse(). We
can fold the code there and avoid an allocation altogether.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qobject/json-parser.c | 41 +++++++++--------------------------------
 1 file changed, 9 insertions(+), 32 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 9a7004e680..6baf73b4b9 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -244,33 +244,6 @@ static JSONToken *parser_context_peek_token(JSONParserContext *ctxt)
     return g_queue_peek_head(ctxt->buf);
 }
 
-static JSONParserContext *parser_context_new(GQueue *tokens)
-{
-    JSONParserContext *ctxt;
-
-    if (!tokens) {
-        return NULL;
-    }
-
-    ctxt = g_malloc0(sizeof(JSONParserContext));
-    ctxt->buf = tokens;
-
-    return ctxt;
-}
-
-/* to support error propagation, ctxt->err must be freed separately */
-static void parser_context_free(JSONParserContext *ctxt)
-{
-    if (ctxt) {
-        while (!g_queue_is_empty(ctxt->buf)) {
-            parser_context_pop_token(ctxt);
-        }
-        g_free(ctxt->current);
-        g_queue_free(ctxt->buf);
-        g_free(ctxt);
-    }
-}
-
 /**
  * Parsing rules
  */
@@ -577,18 +550,22 @@ static QObject *parse_value(JSONParserContext *ctxt, va_list *ap)
 
 QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
 {
-    JSONParserContext *ctxt = parser_context_new(tokens);
+    JSONParserContext ctxt = { .buf = tokens };
     QObject *result;
 
-    if (!ctxt) {
+    if (!tokens) {
         return NULL;
     }
 
-    result = parse_value(ctxt, ap);
+    result = parse_value(&ctxt, ap);
 
-    error_propagate(errp, ctxt->err);
+    error_propagate(errp, ctxt.err);
 
-    parser_context_free(ctxt);
+    while (!g_queue_is_empty(ctxt.buf)) {
+        parser_context_pop_token(&ctxt);
+    }
+    g_free(ctxt.current);
+    g_queue_free(ctxt.buf);
 
     return result;
 }
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 08/18] json-parser: simplify and avoid JSONParserContext allocation

[Markus Armbruster]

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

> parser_context_new/free() are only used from json_parser_parse(). We
> can fold the code there and avoid an allocation altogether.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qobject/json-parser.c | 41 +++++++++--------------------------------
>  1 file changed, 9 insertions(+), 32 deletions(-)
>
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 9a7004e680..6baf73b4b9 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -244,33 +244,6 @@ static JSONToken *parser_context_peek_token(JSONParserContext *ctxt)
>      return g_queue_peek_head(ctxt->buf);
>  }
>  
> -static JSONParserContext *parser_context_new(GQueue *tokens)
> -{
> -    JSONParserContext *ctxt;
> -
> -    if (!tokens) {
> -        return NULL;
> -    }
> -
> -    ctxt = g_malloc0(sizeof(JSONParserContext));
> -    ctxt->buf = tokens;
> -
> -    return ctxt;
> -}
> -
> -/* to support error propagation, ctxt->err must be freed separately */
> -static void parser_context_free(JSONParserContext *ctxt)
> -{
> -    if (ctxt) {
> -        while (!g_queue_is_empty(ctxt->buf)) {
> -            parser_context_pop_token(ctxt);
> -        }
> -        g_free(ctxt->current);
> -        g_queue_free(ctxt->buf);
> -        g_free(ctxt);
> -    }
> -}
> -
>  /**
>   * Parsing rules
>   */
> @@ -577,18 +550,22 @@ static QObject *parse_value(JSONParserContext *ctxt, va_list *ap)
>  
>  QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
>  {
> -    JSONParserContext *ctxt = parser_context_new(tokens);
> +    JSONParserContext ctxt = { .buf = tokens };
>      QObject *result;
>  
> -    if (!ctxt) {
> +    if (!tokens) {
>          return NULL;
>      }
>  
> -    result = parse_value(ctxt, ap);
> +    result = parse_value(&ctxt, ap);
>  
> -    error_propagate(errp, ctxt->err);
> +    error_propagate(errp, ctxt.err);
>  
> -    parser_context_free(ctxt);
> +    while (!g_queue_is_empty(ctxt.buf)) {
> +        parser_context_pop_token(&ctxt);
> +    }
> +    g_free(ctxt.current);
> +    g_queue_free(ctxt.buf);
>  
>      return result;
>  }

A lovely simplification.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v2 09/18] json-parser: further simplify freeing JSONParserContext

[Marc-André Lureau]

Use g_queue_free_full() directly.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qobject/json-parser.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 6baf73b4b9..0c0b478149 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -561,11 +561,8 @@ QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
 
     error_propagate(errp, ctxt.err);
 
-    while (!g_queue_is_empty(ctxt.buf)) {
-        parser_context_pop_token(&ctxt);
-    }
+    g_queue_free_full(ctxt.buf, g_free);
     g_free(ctxt.current);
-    g_queue_free(ctxt.buf);
 
     return result;
 }
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 09/18] json-parser: further simplify freeing JSONParserContext

[Markus Armbruster]

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

> Use g_queue_free_full() directly.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qobject/json-parser.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 6baf73b4b9..0c0b478149 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -561,11 +561,8 @@ QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
>  
>      error_propagate(errp, ctxt.err);
>  
> -    while (!g_queue_is_empty(ctxt.buf)) {
> -        parser_context_pop_token(&ctxt);
> -    }
> +    g_queue_free_full(ctxt.buf, g_free);
>      g_free(ctxt.current);
> -    g_queue_free(ctxt.buf);
>  
>      return result;
>  }

Enabled by our recent upgrade to GLib 2.40 (commit e7b3af81).

Dots into JSONParserContext, but it did so before the patch already.
Perhaps turning JSONParserContext into a proper abstract data type would
be slightly cleaner, but it doesn't seem worth the effort.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v2 10/18] qjson: report an error if there are multiple results

[Marc-André Lureau]

qobject_from_jsonv() returns a single object. Let's make sure that
during parsing we don't leak an intermediary object. Instead of
returning the last object, set a parsing error.

Also, the lexer/parser keeps consuming all the data. There might be an
error set earlier. Let's keep it and not call json_parser_parse() with
the remaining data. Eventually, we may teach the message parser to
stop consuming the data.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qobject/qjson.c     | 16 +++++++++++++++-
 tests/check-qjson.c | 11 +++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/qobject/qjson.c b/qobject/qjson.c
index ee04e61096..8a9d116150 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -22,6 +22,7 @@
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qnum.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qmp/qerror.h"
 #include "qemu/unicode.h"
 
 typedef struct JSONParsingState
@@ -36,7 +37,20 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
 {
     JSONParsingState *s = container_of(parser, JSONParsingState, parser);
 
-    s->result = json_parser_parse(tokens, s->ap, &s->err);
+    if (s->result || s->err) {
+        if (s->result) {
+            qobject_unref(s->result);
+            s->result = NULL;
+            if (!s->err) {
+                error_setg(&s->err, QERR_JSON_PARSING);
+            }
+        }
+        if (tokens) {
+            g_queue_free_full(tokens, g_free);
+        }
+    } else {
+        s->result = json_parser_parse(tokens, s->ap, &s->err);
+    }
 }
 
 QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index da582df3e9..7d3547e0cc 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1417,6 +1417,16 @@ static void limits_nesting(void)
     g_assert(obj == NULL);
 }
 
+static void multiple_objects(void)
+{
+    Error *err = NULL;
+    QObject *obj;
+
+    obj = qobject_from_json("{} {}", &err);
+    g_assert(obj == NULL);
+    error_free_or_abort(&err);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -1454,6 +1464,7 @@ int main(int argc, char **argv)
     g_test_add_func("/errors/invalid_dict_comma", invalid_dict_comma);
     g_test_add_func("/errors/unterminated/literal", unterminated_literal);
     g_test_add_func("/errors/limits/nesting", limits_nesting);
+    g_test_add_func("/errors/multiple_objects", multiple_objects);
 
     return g_test_run();
 }
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 10/18] qjson: report an error if there are multiple results

[Markus Armbruster]

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

> qobject_from_jsonv() returns a single object. Let's make sure that
> during parsing we don't leak an intermediary object. Instead of
> returning the last object, set a parsing error.
>
> Also, the lexer/parser keeps consuming all the data. There might be an
> error set earlier. Let's keep it and not call json_parser_parse() with
> the remaining data. Eventually, we may teach the message parser to
> stop consuming the data.

Took me a while to figure out what you mean :)

qobject_from_jsonv() feeds a complete string to the JSON parser, with
json_message_parser_feed().  This actually feeds one character after the
other to the lexer, via json_lexer_feed_char().

Whenever a character completes a token, the lexer feeds that token to
the streamer via a callback that is always json_message_process_token().

The attentive reader may wonder what it does with trailing characters
that aren't a complete token.  More on that below.

The streamer accumulates tokens, counting various parenthesis.  When all
counts are zero or any is negative, the group is complete, and is fed to
the parser via another callback.  That callback is parse_json() here.
There are more elsewhere.

The attentive reader may wonder what it does with trailing tokens that
aren't a complete group.  More on that below.

The callbacks all pass the tokens to json_parser_parse() to do the
actual parsing.  Returns the abstract syntax tree as QObject on success.

Note that the callback can be called any number of times.

In my opinion, this is over-engineered and under-thought.  There's more
flexibility than we're using, and the associated complexity breeds bugs.

In particular, parse_json() is wrong:

    s->result = json_parser_parse(tokens, s->ap, &s->err);

If the previous call succeeded, we leak its result.  This is the leak
you mentioned.

If any previous call set an error, we pass &s->err pointing to non-null,
which is forbidden.  If json_parser_parse() passed it to error_setg(),
this would crash.  Since it passes it only to error_propagate(), all
errors but the first one are ignored.  Latent crash bug.

If the last call succeeds and any previous call failed, the function
simultaneously succeeds and fails: we return both a result and set an
error.

Let's demonstrate these bugs before we fix them, by inserting the
appended patch before this one.

Are the other callbacks similarly buggy?  Turns out they're okay:

* handle_qmp_command() consumes result and error on each call.

* process_event() does, too.

* qmp_response() treats errors as fatal, and asserts "no previous
  response".

On trailing tokens that don't form a group: they get silently ignored,
as demonstrated by check-qjson test cases unterminated_array(),
unterminated_array_comma(), unterminated_dict(),
unterminated_dict_comma(), all marked BUG.

On trailing characters that don't form a token: they get silently
ignored, as demonstrated by check-qjson test cases
unterminated_string(), unterminated_sq_string(), unterminated_escape(),
all marked BUG, except when they aren't, as in test case
unterminated_literal().

The BUG marks are all gone at the end of this series.  I guess you're
fixing all that :)

Note that these "trailing characters / tokens are silently ignored" bugs
*do* affect the other callbacks.  Reproducer for handle_qmp_command():

    $ echo -e '{ "execute": "qmp_capabilities" }\n{ "execute": "query-name" }\n"unterminated' | socat UNIX:test-qmp STDIO
    {"QMP": {"version": {"qemu": {"micro": 90, "minor": 12, "major": 2}, "package": "v3.0.0-rc1-20-g6a024cd461"}, "capabilities": ["oob"]}}
    {"return": {}}
    {"return": {}}

Note there's no error reported for the last line.

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qobject/qjson.c     | 16 +++++++++++++++-
>  tests/check-qjson.c | 11 +++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index ee04e61096..8a9d116150 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -22,6 +22,7 @@
>  #include "qapi/qmp/qlist.h"
>  #include "qapi/qmp/qnum.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qerror.h"
>  #include "qemu/unicode.h"
>  
>  typedef struct JSONParsingState
> @@ -36,7 +37,20 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
>  {
>      JSONParsingState *s = container_of(parser, JSONParsingState, parser);
>  
> -    s->result = json_parser_parse(tokens, s->ap, &s->err);
> +    if (s->result || s->err) {
> +        if (s->result) {
> +            qobject_unref(s->result);
> +            s->result = NULL;
> +            if (!s->err) {

Conditional is necessary because a previous call to json_parser_parse()
could have set s->err already.  Without it, error_setg() would fail the
assertion in error_setv() then.  Subtle.

> +                error_setg(&s->err, QERR_JSON_PARSING);

Hmm.  Is this really "Invalid JSON syntax"?  In a way, it is, because
RFC 7159 defines JSON-text as a single value.  Still, a friendlier error
message like "Expecting at most one JSON value" woun't hurt.

What if !tokens?  That shouldn't be an error.

> +            }
> +        }
> +        if (tokens) {
> +            g_queue_free_full(tokens, g_free);

g_queue_free_full(NULL, ...) doesn't work?!?  That would be lame.

> +        }
> +    } else {
> +        s->result = json_parser_parse(tokens, s->ap, &s->err);
> +    }
>  }

What about this:

       JSONParsingState *s = container_of(parser, JSONParsingState, parser);
       Error *err = NULL;

       if (!tokens) {
           return;
       }
       if (s->result || s->err) {
           if (s->result) {
               qobject_unref(s->result);
               s->result = NULL;
               error_setg(&err, "Expecting at most one JSON value");
           }
           g_queue_free_full(tokens, g_free);
       } else {
           s->result = json_parser_parse(tokens, s->ap, &err);
       }
       error_propagate(&s->err, err);
       assert(!s->result != !s->err);

>  
>  QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index da582df3e9..7d3547e0cc 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -1417,6 +1417,16 @@ static void limits_nesting(void)
>      g_assert(obj == NULL);
>  }
>  
> +static void multiple_objects(void)
> +{
> +    Error *err = NULL;
> +    QObject *obj;
> +
> +    obj = qobject_from_json("{} {}", &err);
> +    g_assert(obj == NULL);
> +    error_free_or_abort(&err);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -1454,6 +1464,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/errors/invalid_dict_comma", invalid_dict_comma);
>      g_test_add_func("/errors/unterminated/literal", unterminated_literal);
>      g_test_add_func("/errors/limits/nesting", limits_nesting);
> +    g_test_add_func("/errors/multiple_objects", multiple_objects);
>  
>      return g_test_run();
>  }

>From 18064b774ea7a79a9dbabe5cf75458f0326070d5 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Fri, 20 Jul 2018 10:22:41 +0200
Subject: [PATCH] check-qjson: Cover multiple JSON objects in same string

qobject_from_json() & friends misbehave when the JSON text has more
than one JSON value.  Add test coverage to demonstrate the bugs.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qjson.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index da582df3e9..c5fd439263 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1417,6 +1417,25 @@ static void limits_nesting(void)
     g_assert(obj == NULL);
 }
 
+static void multiple_objects(void)
+{
+    Error *err = NULL;
+    QObject *obj;
+
+    /* BUG this leaks the syntax tree for "false" */
+    obj = qobject_from_json("false true", &err);
+    g_assert(qbool_get_bool(qobject_to(QBool, obj)));
+    g_assert(!err);
+    qobject_unref(obj);
+
+    /* BUG simultaneously succeeds and fails */
+    /* BUG calls json_parser_parse() with errp pointing to non-null */
+    obj = qobject_from_json("} true", &err);
+    g_assert(qbool_get_bool(qobject_to(QBool, obj)));
+    error_free_or_abort(&err);
+    qobject_unref(obj);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -1454,6 +1473,7 @@ int main(int argc, char **argv)
     g_test_add_func("/errors/invalid_dict_comma", invalid_dict_comma);
     g_test_add_func("/errors/unterminated/literal", unterminated_literal);
     g_test_add_func("/errors/limits/nesting", limits_nesting);
+    g_test_add_func("/errors/multiple_objects", multiple_objects);
 
     return g_test_run();
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 10/18] qjson: report an error if there are multiple results

[Marc-André Lureau]

Hi

On Fri, Jul 20, 2018 at 10:49 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> qobject_from_jsonv() returns a single object. Let's make sure that
>> during parsing we don't leak an intermediary object. Instead of
>> returning the last object, set a parsing error.
>>
>> Also, the lexer/parser keeps consuming all the data. There might be an
>> error set earlier. Let's keep it and not call json_parser_parse() with
>> the remaining data. Eventually, we may teach the message parser to
>> stop consuming the data.
>
> Took me a while to figure out what you mean :)
>
> qobject_from_jsonv() feeds a complete string to the JSON parser, with
> json_message_parser_feed().  This actually feeds one character after the
> other to the lexer, via json_lexer_feed_char().
>
> Whenever a character completes a token, the lexer feeds that token to
> the streamer via a callback that is always json_message_process_token().
>
> The attentive reader may wonder what it does with trailing characters
> that aren't a complete token.  More on that below.
>
> The streamer accumulates tokens, counting various parenthesis.  When all
> counts are zero or any is negative, the group is complete, and is fed to
> the parser via another callback.  That callback is parse_json() here.
> There are more elsewhere.
>
> The attentive reader may wonder what it does with trailing tokens that
> aren't a complete group.  More on that below.
>
> The callbacks all pass the tokens to json_parser_parse() to do the
> actual parsing.  Returns the abstract syntax tree as QObject on success.
>
> Note that the callback can be called any number of times.
>
> In my opinion, this is over-engineered and under-thought.  There's more
> flexibility than we're using, and the associated complexity breeds bugs.
>
> In particular, parse_json() is wrong:
>
>     s->result = json_parser_parse(tokens, s->ap, &s->err);
>
> If the previous call succeeded, we leak its result.  This is the leak
> you mentioned.
>
> If any previous call set an error, we pass &s->err pointing to non-null,
> which is forbidden.  If json_parser_parse() passed it to error_setg(),
> this would crash.  Since it passes it only to error_propagate(), all
> errors but the first one are ignored.  Latent crash bug.
>
> If the last call succeeds and any previous call failed, the function
> simultaneously succeeds and fails: we return both a result and set an
> error.
>
> Let's demonstrate these bugs before we fix them, by inserting the
> appended patch before this one.
>
> Are the other callbacks similarly buggy?  Turns out they're okay:
>
> * handle_qmp_command() consumes result and error on each call.
>
> * process_event() does, too.
>
> * qmp_response() treats errors as fatal, and asserts "no previous
>   response".
>
> On trailing tokens that don't form a group: they get silently ignored,
> as demonstrated by check-qjson test cases unterminated_array(),
> unterminated_array_comma(), unterminated_dict(),
> unterminated_dict_comma(), all marked BUG.
>
> On trailing characters that don't form a token: they get silently
> ignored, as demonstrated by check-qjson test cases
> unterminated_string(), unterminated_sq_string(), unterminated_escape(),
> all marked BUG, except when they aren't, as in test case
> unterminated_literal().
>
> The BUG marks are all gone at the end of this series.  I guess you're
> fixing all that :)
>
> Note that these "trailing characters / tokens are silently ignored" bugs
> *do* affect the other callbacks.  Reproducer for handle_qmp_command():
>
>     $ echo -e '{ "execute": "qmp_capabilities" }\n{ "execute": "query-name" }\n"unterminated' | socat UNIX:test-qmp STDIO
>     {"QMP": {"version": {"qemu": {"micro": 90, "minor": 12, "major": 2}, "package": "v3.0.0-rc1-20-g6a024cd461"}, "capabilities": ["oob"]}}
>     {"return": {}}
>     {"return": {}}
>
> Note there's no error reported for the last line.
>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  qobject/qjson.c     | 16 +++++++++++++++-
>>  tests/check-qjson.c | 11 +++++++++++
>>  2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/qobject/qjson.c b/qobject/qjson.c
>> index ee04e61096..8a9d116150 100644
>> --- a/qobject/qjson.c
>> +++ b/qobject/qjson.c
>> @@ -22,6 +22,7 @@
>>  #include "qapi/qmp/qlist.h"
>>  #include "qapi/qmp/qnum.h"
>>  #include "qapi/qmp/qstring.h"
>> +#include "qapi/qmp/qerror.h"
>>  #include "qemu/unicode.h"
>>
>>  typedef struct JSONParsingState
>> @@ -36,7 +37,20 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
>>  {
>>      JSONParsingState *s = container_of(parser, JSONParsingState, parser);
>>
>> -    s->result = json_parser_parse(tokens, s->ap, &s->err);
>> +    if (s->result || s->err) {
>> +        if (s->result) {
>> +            qobject_unref(s->result);
>> +            s->result = NULL;
>> +            if (!s->err) {
>
> Conditional is necessary because a previous call to json_parser_parse()
> could have set s->err already.  Without it, error_setg() would fail the
> assertion in error_setv() then.  Subtle.
>
>> +                error_setg(&s->err, QERR_JSON_PARSING);
>
> Hmm.  Is this really "Invalid JSON syntax"?  In a way, it is, because
> RFC 7159 defines JSON-text as a single value.  Still, a friendlier error
> message like "Expecting at most one JSON value" woun't hurt.
>
> What if !tokens?  That shouldn't be an error.
>
>> +            }
>> +        }
>> +        if (tokens) {
>> +            g_queue_free_full(tokens, g_free);
>
> g_queue_free_full(NULL, ...) doesn't work?!?  That would be lame.

It warns and return.

We could use g_clear_pointer(&tokens, g_queue_free_full)...

>
>> +        }
>> +    } else {
>> +        s->result = json_parser_parse(tokens, s->ap, &s->err);
>> +    }
>>  }
>
> What about this:
>
>        JSONParsingState *s = container_of(parser, JSONParsingState, parser);
>        Error *err = NULL;
>
>        if (!tokens) {
>            return;
>        }

I would rather leave handling of NULL tokens to json_parser_parse()
for consistency with other callers.

>        if (s->result || s->err) {
>            if (s->result) {
>                qobject_unref(s->result);
>                s->result = NULL;
>                error_setg(&err, "Expecting at most one JSON value");
>            }
>            g_queue_free_full(tokens, g_free);

missing null check

>        } else {
>            s->result = json_parser_parse(tokens, s->ap, &err);
>        }
>        error_propagate(&s->err, err);

how do you ensure s->err is not already set?

>        assert(!s->result != !s->err);
>
>>
>>  QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
>> index da582df3e9..7d3547e0cc 100644
>> --- a/tests/check-qjson.c
>> +++ b/tests/check-qjson.c
>> @@ -1417,6 +1417,16 @@ static void limits_nesting(void)
>>      g_assert(obj == NULL);
>>  }
>>
>> +static void multiple_objects(void)
>> +{
>> +    Error *err = NULL;
>> +    QObject *obj;
>> +
>> +    obj = qobject_from_json("{} {}", &err);
>> +    g_assert(obj == NULL);
>> +    error_free_or_abort(&err);
>> +}
>> +
>>  int main(int argc, char **argv)
>>  {
>>      g_test_init(&argc, &argv, NULL);
>> @@ -1454,6 +1464,7 @@ int main(int argc, char **argv)
>>      g_test_add_func("/errors/invalid_dict_comma", invalid_dict_comma);
>>      g_test_add_func("/errors/unterminated/literal", unterminated_literal);
>>      g_test_add_func("/errors/limits/nesting", limits_nesting);
>> +    g_test_add_func("/errors/multiple_objects", multiple_objects);
>>
>>      return g_test_run();
>>  }
>
> From 18064b774ea7a79a9dbabe5cf75458f0326070d5 Mon Sep 17 00:00:00 2001
> From: Markus Armbruster <armbru@redhat.com>
> Date: Fri, 20 Jul 2018 10:22:41 +0200
> Subject: [PATCH] check-qjson: Cover multiple JSON objects in same string
>
> qobject_from_json() & friends misbehave when the JSON text has more
> than one JSON value.  Add test coverage to demonstrate the bugs.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

test looks better than mine, should I include it in the series before the fix?

> ---
>  tests/check-qjson.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index da582df3e9..c5fd439263 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -1417,6 +1417,25 @@ static void limits_nesting(void)
>      g_assert(obj == NULL);
>  }
>
> +static void multiple_objects(void)
> +{
> +    Error *err = NULL;
> +    QObject *obj;
> +
> +    /* BUG this leaks the syntax tree for "false" */
> +    obj = qobject_from_json("false true", &err);
> +    g_assert(qbool_get_bool(qobject_to(QBool, obj)));
> +    g_assert(!err);
> +    qobject_unref(obj);
> +
> +    /* BUG simultaneously succeeds and fails */
> +    /* BUG calls json_parser_parse() with errp pointing to non-null */
> +    obj = qobject_from_json("} true", &err);
> +    g_assert(qbool_get_bool(qobject_to(QBool, obj)));
> +    error_free_or_abort(&err);
> +    qobject_unref(obj);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -1454,6 +1473,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/errors/invalid_dict_comma", invalid_dict_comma);
>      g_test_add_func("/errors/unterminated/literal", unterminated_literal);
>      g_test_add_func("/errors/limits/nesting", limits_nesting);
> +    g_test_add_func("/errors/multiple_objects", multiple_objects);
>
>      return g_test_run();
>  }
> --
> 2.17.1
>

Re: [Qemu-devel] [PATCH v2 10/18] qjson: report an error if there are multiple results

[Markus Armbruster]

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

> Hi
>
> On Fri, Jul 20, 2018 at 10:49 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>>> qobject_from_jsonv() returns a single object. Let's make sure that
>>> during parsing we don't leak an intermediary object. Instead of
>>> returning the last object, set a parsing error.
>>>
>>> Also, the lexer/parser keeps consuming all the data. There might be an
>>> error set earlier. Let's keep it and not call json_parser_parse() with
>>> the remaining data. Eventually, we may teach the message parser to
>>> stop consuming the data.
>>
>> Took me a while to figure out what you mean :)
>>
>> qobject_from_jsonv() feeds a complete string to the JSON parser, with
>> json_message_parser_feed().  This actually feeds one character after the
>> other to the lexer, via json_lexer_feed_char().
>>
>> Whenever a character completes a token, the lexer feeds that token to
>> the streamer via a callback that is always json_message_process_token().
>>
>> The attentive reader may wonder what it does with trailing characters
>> that aren't a complete token.  More on that below.
>>
>> The streamer accumulates tokens, counting various parenthesis.  When all
>> counts are zero or any is negative, the group is complete, and is fed to
>> the parser via another callback.  That callback is parse_json() here.
>> There are more elsewhere.
>>
>> The attentive reader may wonder what it does with trailing tokens that
>> aren't a complete group.  More on that below.
>>
>> The callbacks all pass the tokens to json_parser_parse() to do the
>> actual parsing.  Returns the abstract syntax tree as QObject on success.
>>
>> Note that the callback can be called any number of times.
>>
>> In my opinion, this is over-engineered and under-thought.  There's more
>> flexibility than we're using, and the associated complexity breeds bugs.

Two obvious simplifications come to mind:

* Call json_message_process_token() directly.

* Move the call json_parser_parse() into the streamer, and have the
  streamer pass QObject * instead of GQueue * to its callback.

I'll post patches for your consideration, but first I'll continue to
review your series.

>> In particular, parse_json() is wrong:
>>
>>     s->result = json_parser_parse(tokens, s->ap, &s->err);
>>
>> If the previous call succeeded, we leak its result.  This is the leak
>> you mentioned.
>>
>> If any previous call set an error, we pass &s->err pointing to non-null,
>> which is forbidden.  If json_parser_parse() passed it to error_setg(),
>> this would crash.  Since it passes it only to error_propagate(), all
>> errors but the first one are ignored.  Latent crash bug.
>>
>> If the last call succeeds and any previous call failed, the function
>> simultaneously succeeds and fails: we return both a result and set an
>> error.
>>
>> Let's demonstrate these bugs before we fix them, by inserting the
>> appended patch before this one.
>>
>> Are the other callbacks similarly buggy?  Turns out they're okay:
>>
>> * handle_qmp_command() consumes result and error on each call.
>>
>> * process_event() does, too.
>>
>> * qmp_response() treats errors as fatal, and asserts "no previous
>>   response".
>>
>> On trailing tokens that don't form a group: they get silently ignored,
>> as demonstrated by check-qjson test cases unterminated_array(),
>> unterminated_array_comma(), unterminated_dict(),
>> unterminated_dict_comma(), all marked BUG.
>>
>> On trailing characters that don't form a token: they get silently
>> ignored, as demonstrated by check-qjson test cases
>> unterminated_string(), unterminated_sq_string(), unterminated_escape(),
>> all marked BUG, except when they aren't, as in test case
>> unterminated_literal().
>>
>> The BUG marks are all gone at the end of this series.  I guess you're
>> fixing all that :)
>>
>> Note that these "trailing characters / tokens are silently ignored" bugs
>> *do* affect the other callbacks.  Reproducer for handle_qmp_command():
>>
>>     $ echo -e '{ "execute": "qmp_capabilities" }\n{ "execute": "query-name" }\n"unterminated' | socat UNIX:test-qmp STDIO
>>     {"QMP": {"version": {"qemu": {"micro": 90, "minor": 12, "major": 2}, "package": "v3.0.0-rc1-20-g6a024cd461"}, "capabilities": ["oob"]}}
>>     {"return": {}}
>>     {"return": {}}
>>
>> Note there's no error reported for the last line.
>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  qobject/qjson.c     | 16 +++++++++++++++-
>>>  tests/check-qjson.c | 11 +++++++++++
>>>  2 files changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qobject/qjson.c b/qobject/qjson.c
>>> index ee04e61096..8a9d116150 100644
>>> --- a/qobject/qjson.c
>>> +++ b/qobject/qjson.c
>>> @@ -22,6 +22,7 @@
>>>  #include "qapi/qmp/qlist.h"
>>>  #include "qapi/qmp/qnum.h"
>>>  #include "qapi/qmp/qstring.h"
>>> +#include "qapi/qmp/qerror.h"
>>>  #include "qemu/unicode.h"
>>>
>>>  typedef struct JSONParsingState
>>> @@ -36,7 +37,20 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
>>>  {
>>>      JSONParsingState *s = container_of(parser, JSONParsingState, parser);
>>>
>>> -    s->result = json_parser_parse(tokens, s->ap, &s->err);
>>> +    if (s->result || s->err) {
>>> +        if (s->result) {
>>> +            qobject_unref(s->result);
>>> +            s->result = NULL;
>>> +            if (!s->err) {
>>
>> Conditional is necessary because a previous call to json_parser_parse()
>> could have set s->err already.  Without it, error_setg() would fail the
>> assertion in error_setv() then.  Subtle.
>>
>>> +                error_setg(&s->err, QERR_JSON_PARSING);
>>
>> Hmm.  Is this really "Invalid JSON syntax"?  In a way, it is, because
>> RFC 7159 defines JSON-text as a single value.  Still, a friendlier error
>> message like "Expecting at most one JSON value" woun't hurt.
>>
>> What if !tokens?  That shouldn't be an error.
>>
>>> +            }
>>> +        }
>>> +        if (tokens) {
>>> +            g_queue_free_full(tokens, g_free);
>>
>> g_queue_free_full(NULL, ...) doesn't work?!?  That would be lame.
>
> It warns and return.

Lame.

> We could use g_clear_pointer(&tokens, g_queue_free_full)...

Slightly terser, but the reader better knows g_clear_pointer().  So far,
we don't use it.

>>> +        }
>>> +    } else {
>>> +        s->result = json_parser_parse(tokens, s->ap, &s->err);
>>> +    }
>>>  }
>>
>> What about this:
>>
>>        JSONParsingState *s = container_of(parser, JSONParsingState, parser);
>>        Error *err = NULL;
>>
>>        if (!tokens) {
>>            return;
>>        }
>
> I would rather leave handling of NULL tokens to json_parser_parse()
> for consistency with other callers.

Fair point.

I expect the issue to evaporate when I simplify the parser interface.

>>        if (s->result || s->err) {
>>            if (s->result) {
>>                qobject_unref(s->result);
>>                s->result = NULL;
>>                error_setg(&err, "Expecting at most one JSON value");
>>            }
>>            g_queue_free_full(tokens, g_free);
>
> missing null check

@tokens can't be null here.

>>        } else {
>>            s->result = json_parser_parse(tokens, s->ap, &err);
>>        }
>>        error_propagate(&s->err, err);
>
> how do you ensure s->err is not already set?

error_propagate() is fine with that, unlike error_setg().

>>        assert(!s->result != !s->err);
>>
>>>
>>>  QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>>> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
>>> index da582df3e9..7d3547e0cc 100644
>>> --- a/tests/check-qjson.c
>>> +++ b/tests/check-qjson.c
>>> @@ -1417,6 +1417,16 @@ static void limits_nesting(void)
>>>      g_assert(obj == NULL);
>>>  }
>>>
>>> +static void multiple_objects(void)
>>> +{
>>> +    Error *err = NULL;
>>> +    QObject *obj;
>>> +
>>> +    obj = qobject_from_json("{} {}", &err);
>>> +    g_assert(obj == NULL);
>>> +    error_free_or_abort(&err);
>>> +}
>>> +
>>>  int main(int argc, char **argv)
>>>  {
>>>      g_test_init(&argc, &argv, NULL);
>>> @@ -1454,6 +1464,7 @@ int main(int argc, char **argv)
>>>      g_test_add_func("/errors/invalid_dict_comma", invalid_dict_comma);
>>>      g_test_add_func("/errors/unterminated/literal", unterminated_literal);
>>>      g_test_add_func("/errors/limits/nesting", limits_nesting);
>>> +    g_test_add_func("/errors/multiple_objects", multiple_objects);
>>>
>>>      return g_test_run();
>>>  }
>>
>> From 18064b774ea7a79a9dbabe5cf75458f0326070d5 Mon Sep 17 00:00:00 2001
>> From: Markus Armbruster <armbru@redhat.com>
>> Date: Fri, 20 Jul 2018 10:22:41 +0200
>> Subject: [PATCH] check-qjson: Cover multiple JSON objects in same string
>>
>> qobject_from_json() & friends misbehave when the JSON text has more
>> than one JSON value.  Add test coverage to demonstrate the bugs.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> test looks better than mine, should I include it in the series before the fix?

Yes, please.

[Qemu-devel] [PATCH v2 11/18] qjson: report error on unterminated string

[Marc-André Lureau]

An unterminated string will make parser emit an error (tokens ==
NULL). Let's report it.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qobject/qjson.c     | 3 +++
 tests/check-qjson.c | 6 +++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/qobject/qjson.c b/qobject/qjson.c
index 8a9d116150..01218c9ad6 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -37,6 +37,9 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
 {
     JSONParsingState *s = container_of(parser, JSONParsingState, parser);
 
+    if (!tokens && !s->err) {
+        error_setg(&s->err, QERR_JSON_PARSING);
+    }
     if (s->result || s->err) {
         if (s->result) {
             qobject_unref(s->result);
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 7d3547e0cc..e602abda11 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1315,7 +1315,7 @@ static void unterminated_string(void)
 {
     Error *err = NULL;
     QObject *obj = qobject_from_json("\"abc", &err);
-    g_assert(!err);             /* BUG */
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
@@ -1323,7 +1323,7 @@ static void unterminated_sq_string(void)
 {
     Error *err = NULL;
     QObject *obj = qobject_from_json("'abc", &err);
-    g_assert(!err);             /* BUG */
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
@@ -1331,7 +1331,7 @@ static void unterminated_escape(void)
 {
     Error *err = NULL;
     QObject *obj = qobject_from_json("\"abc\\\"", &err);
-    g_assert(!err);             /* BUG */
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 11/18] qjson: report error on unterminated string

[Markus Armbruster]

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

> An unterminated string will make parser emit an error (tokens ==
> NULL). Let's report it.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qobject/qjson.c     | 3 +++
>  tests/check-qjson.c | 6 +++---
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index 8a9d116150..01218c9ad6 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -37,6 +37,9 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
>  {
>      JSONParsingState *s = container_of(parser, JSONParsingState, parser);
>  
> +    if (!tokens && !s->err) {
> +        error_setg(&s->err, QERR_JSON_PARSING);
> +    }
>      if (s->result || s->err) {
>          if (s->result) {
>              qobject_unref(s->result);

This doesn't fix the JSON parser, it "fixes" one of its users!  Other
users remain broken.  Reproducer for QMP (already mentioned in my review
of the previous patch):

    $ echo -e '{ "execute": "qmp_capabilities" }\n{ "execute": "query-name" }\n"unterminated' | socat UNIX:test-qmp STDIO
    {"QMP": {"version": {"qemu": {"micro": 90, "minor": 12, "major": 2}, "package": "v3.0.0-rc1-20-g6a024cd461"}, "capabilities": ["oob"]}}
    {"return": {}}
    {"return": {}}

Note there's no error reported for the last line.

The simplification of the JSON parser I have in mind might make this
easy to fix properly.  I'll look into it.

[...]

[Qemu-devel] [PATCH v2 12/18] qjson: return parsing error if unterminated input

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qobject/json-streamer.c | 4 +++-
 qobject/qjson.c         | 5 ++++-
 tests/check-qjson.c     | 8 ++++----
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index c51c2021f9..065c551332 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -126,7 +126,9 @@ int json_message_parser_feed(JSONMessageParser *parser,
 
 int json_message_parser_flush(JSONMessageParser *parser)
 {
-    return json_lexer_flush(&parser->lexer);
+    int ret = json_lexer_flush(&parser->lexer);
+
+    return ret ?: g_queue_get_length(parser->tokens);
 }
 
 void json_message_parser_destroy(JSONMessageParser *parser)
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 01218c9ad6..8afdc1e06a 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -64,7 +64,10 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
 
     json_message_parser_init(&state.parser, parse_json);
     json_message_parser_feed(&state.parser, string, strlen(string));
-    json_message_parser_flush(&state.parser);
+    if (json_message_parser_flush(&state.parser) != 0 &&
+        !state.err) {
+        error_setg(&state.err, QERR_JSON_PARSING);
+    }
     json_message_parser_destroy(&state.parser);
 
     error_propagate(errp, state.err);
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index e602abda11..d0144ba93c 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1339,7 +1339,7 @@ static void unterminated_array(void)
 {
     Error *err = NULL;
     QObject *obj = qobject_from_json("[32", &err);
-    g_assert(!err);             /* BUG */
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
@@ -1347,7 +1347,7 @@ static void unterminated_array_comma(void)
 {
     Error *err = NULL;
     QObject *obj = qobject_from_json("[32,", &err);
-    g_assert(!err);             /* BUG */
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
@@ -1363,7 +1363,7 @@ static void unterminated_dict(void)
 {
     Error *err = NULL;
     QObject *obj = qobject_from_json("{'abc':32", &err);
-    g_assert(!err);             /* BUG */
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
@@ -1371,7 +1371,7 @@ static void unterminated_dict_comma(void)
 {
     Error *err = NULL;
     QObject *obj = qobject_from_json("{'abc':32,", &err);
-    g_assert(!err);             /* BUG */
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 12/18] qjson: return parsing error if unterminated input

[Markus Armbruster]

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

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qobject/json-streamer.c | 4 +++-
>  qobject/qjson.c         | 5 ++++-
>  tests/check-qjson.c     | 8 ++++----
>  3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
> index c51c2021f9..065c551332 100644
> --- a/qobject/json-streamer.c
> +++ b/qobject/json-streamer.c
> @@ -126,7 +126,9 @@ int json_message_parser_feed(JSONMessageParser *parser,
>  
>  int json_message_parser_flush(JSONMessageParser *parser)
>  {
> -    return json_lexer_flush(&parser->lexer);
> +    int ret = json_lexer_flush(&parser->lexer);
> +
> +    return ret ?: g_queue_get_length(parser->tokens);
>  }
>  
>  void json_message_parser_destroy(JSONMessageParser *parser)
> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index 01218c9ad6..8afdc1e06a 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -64,7 +64,10 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>  
>      json_message_parser_init(&state.parser, parse_json);
>      json_message_parser_feed(&state.parser, string, strlen(string));
> -    json_message_parser_flush(&state.parser);
> +    if (json_message_parser_flush(&state.parser) != 0 &&
> +        !state.err) {
> +        error_setg(&state.err, QERR_JSON_PARSING);
> +    }
>      json_message_parser_destroy(&state.parser);
>  
>      error_propagate(errp, state.err);

Again, this leaves other users broken.  Reproducer for QMP:

    $ echo -e '{ "execute": "qmp_capabilities" }\n{ "execute": "query-name" }\n[' | socat UNIX:/work/armbru/images/test-qmp STDIO
    {"QMP": {"version": {"qemu": {"micro": 90, "minor": 12, "major": 2}, "package": "v3.0.0-rc1-21-g975ad3dcf2"}, "capabilities": ["oob"]}}
    {"return": {}}
    {"return": {}}

Note there's no error reported for the last line.

The simplification of the JSON parser I have in mind might make this
easy to fix properly.  I'll look into it.

[Qemu-devel] [PATCH v2 13/18] json-parser: set an error if parsing returned NULL

[Marc-André Lureau]

Let's make json_parser_parse_err() suck less, and simplify caller
error handling.

 * monitor.c handle_qmp_command(): drop workaround

 * qga/main.c process_event(): improve error report, QERR_JSON_PARSING
   case is handled by json_parser_parse_err() now.

 * qobject/json-parser.c json_parser_parse(): Ignores the error.

 * qobject/qjson.c qobject_from_jsonv() via parse_json()
  - qobject_from_json()
    ~ block.c parse_json_filename() - removed workaround
    ~ block/rbd.c - abort (generated json - should never fail)
    ~ qapi/qobject-input-visitor.c - removed workaround
    ~ tests/check-qjson.c - assert or crash
    ~ tests/test-visitor-serialization.c - assert or crash

  - qobject_from_jsonf()
    Now dies in error_handle_fatal() instead of the assertion.
    Only used in tests, ok.

  - tests/test-qobject-input-visitor.c
  - tests/libqtest.c qmp_fd_sendv()
    Passes &error_abort, does nothing when qobject_from_jsonv() returns
    null.  The fix makes this catch invalid JSON instead of silently
    ignoring it.

Update the function documentation for possible return values.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block.c                      |  5 -----
 monitor.c                    |  4 ----
 qapi/qobject-input-visitor.c |  5 -----
 qga/main.c                   |  2 +-
 qobject/json-parser.c        | 16 +++++++++++++++-
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index a2fe05ea96..42eaa8b7dc 100644
--- a/block.c
+++ b/block.c
@@ -1478,11 +1478,6 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
 
     options_obj = qobject_from_json(filename, errp);
     if (!options_obj) {
-        /* Work around qobject_from_json() lossage TODO fix that */
-        if (errp && !*errp) {
-            error_setg(errp, "Could not parse the JSON options");
-            return NULL;
-        }
         error_prepend(errp, "Could not parse the JSON options: ");
         return NULL;
     }
diff --git a/monitor.c b/monitor.c
index 2abb3c2fc7..5a41a230b9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4114,10 +4114,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     QMPRequest *req_obj;
 
     req = json_parser_parse(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
 
     qdict = qobject_to(QDict, req);
     if (qdict) {
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index da57f4cc24..3e88b27f9e 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -725,11 +725,6 @@ Visitor *qobject_input_visitor_new_str(const char *str,
     if (is_json) {
         obj = qobject_from_json(str, errp);
         if (!obj) {
-            /* Work around qobject_from_json() lossage TODO fix that */
-            if (errp && !*errp) {
-                error_setg(errp, "JSON parse error");
-                return NULL;
-            }
             return NULL;
         }
         args = qobject_to(QDict, obj);
diff --git a/qga/main.c b/qga/main.c
index 043f7c3ead..9032bb0c7a 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -614,7 +614,7 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens)
     }
     req = qobject_to(QDict, obj);
     if (!req) {
-        error_setg(&err, QERR_JSON_PARSING);
+        error_setg(&err, "Input must be a JSON object");
         goto err;
     }
     if (!qdict_haskey(req, "execute")) {
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 0c0b478149..c3b0c216cf 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -24,6 +24,7 @@
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-lexer.h"
 #include "qapi/qmp/json-streamer.h"
+#include "qapi/qmp/qerror.h"
 
 typedef struct JSONParserContext
 {
@@ -548,6 +549,14 @@ static QObject *parse_value(JSONParserContext *ctxt, va_list *ap)
     }
 }
 
+/**
+ * json_parser_parse:
+ *
+ * If @tokens is null, return null.
+ * Else if @tokens parse okay, return the parse tree.
+ * Else set an error and return null.
+ *
+ **/
 QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
 {
     JSONParserContext ctxt = { .buf = tokens };
@@ -559,7 +568,12 @@ QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
 
     result = parse_value(&ctxt, ap);
 
-    error_propagate(errp, ctxt.err);
+    if (!result && !ctxt.err) {
+        /* TODO: improve error reporting */
+        error_setg(errp, QERR_JSON_PARSING);
+    } else {
+        error_propagate(errp, ctxt.err);
+    }
 
     g_queue_free_full(ctxt.buf, g_free);
     g_free(ctxt.current);
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 13/18] json-parser: set an error if parsing returned NULL

[Markus Armbruster]

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

> Let's make json_parser_parse_err() suck less, and simplify caller
> error handling.

Let's state what's wrong with it first, like this:

  json_parser_parse_err() returns null on empty input and on parse error.
  In the latter case, it sometimes, but not always sets an error.  This
  sucks.  Fix it to always set an error, and simplify callers.

>  * monitor.c handle_qmp_command(): drop workaround
>
>  * qga/main.c process_event(): improve error report, QERR_JSON_PARSING
>    case is handled by json_parser_parse_err() now.
>
>  * qobject/json-parser.c json_parser_parse(): Ignores the error.
>
>  * qobject/qjson.c qobject_from_jsonv() via parse_json()
>   - qobject_from_json()
>     ~ block.c parse_json_filename() - removed workaround
>     ~ block/rbd.c - abort (generated json - should never fail)

Actually, we improve the failure from "qlist_size() crashes" to "the new
error_setg() aborts".

>     ~ qapi/qobject-input-visitor.c - removed workaround
>     ~ tests/check-qjson.c - assert or crash
>     ~ tests/test-visitor-serialization.c - assert or crash
>
>   - qobject_from_jsonf()
>     Now dies in error_handle_fatal() instead of the assertion.
>     Only used in tests, ok.
>
>   - tests/test-qobject-input-visitor.c
>   - tests/libqtest.c qmp_fd_sendv()
>     Passes &error_abort, does nothing when qobject_from_jsonv() returns
>     null.  The fix makes this catch invalid JSON instead of silently
>     ignoring it.
>
> Update the function documentation for possible return values.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  block.c                      |  5 -----
>  monitor.c                    |  4 ----
>  qapi/qobject-input-visitor.c |  5 -----
>  qga/main.c                   |  2 +-
>  qobject/json-parser.c        | 16 +++++++++++++++-
>  5 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index a2fe05ea96..42eaa8b7dc 100644
> --- a/block.c
> +++ b/block.c
> @@ -1478,11 +1478,6 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
>  
>      options_obj = qobject_from_json(filename, errp);
>      if (!options_obj) {
> -        /* Work around qobject_from_json() lossage TODO fix that */
> -        if (errp && !*errp) {
> -            error_setg(errp, "Could not parse the JSON options");
> -            return NULL;
> -        }
>          error_prepend(errp, "Could not parse the JSON options: ");

Crashes if qobject_from_json() returns null without setting an error,
e.g. when input is empty:

    (gdb) p parse_json_filename("json: ", &error_abort)
    [Thread 0x7fffd5bb8700 (LWP 10318) exited]

    Thread 1 "upstream-qemu" received signal SIGSEGV, Segmentation fault.
    error_vprepend (errp=0x555556850110 <error_abort>, 
        fmt=0x555556044a30 "Could not parse the JSON options: ", ap=0x7fffffffd9b0)
        at /work/armbru/qemu/util/error.c:136
    136	    g_string_append(newmsg, (*errp)->msg);


>          return NULL;
>      }
> diff --git a/monitor.c b/monitor.c
> index 2abb3c2fc7..5a41a230b9 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4114,10 +4114,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QMPRequest *req_obj;
>  
>      req = json_parser_parse(tokens, NULL, &err);
> -    if (!req && !err) {
> -        /* json_parser_parse() sucks: can fail without setting @err */
> -        error_setg(&err, QERR_JSON_PARSING);
> -    }
>  
>      qdict = qobject_to(QDict, req);
>      if (qdict) {

What if json_parser_parse() returns null without setting an error?  It
can when @tokens is null.  Hmm...

    $ gdb --args upstream-qemu -nodefaults -S -display none -qmp stdio
    [...]
    (gdb) r
    [...]
    {"QMP": {"version": {"qemu": {"micro": 90, "minor": 12, "major": 2}, "package": "v3.0.0-rc1-23-ga18a0aefe5"}, "capabilities": []}}
    [New Thread 0x7fffd5c38700 (LWP 11878)]
    @
    upstream-qemu: /work/armbru/qemu/monitor.c:4088: monitor_qmp_bh_dispatcher: Assertion `req_obj->err' failed.

    Thread 1 "upstream-qemu" received signal SIGABRT, Aborted.
    0x00007fffec607feb in raise () from /lib64/libc.so.6
    [...]
    (gdb) bt
    #0  0x00007fffec607feb in raise () at /lib64/libc.so.6
    #1  0x00007fffec5f25c1 in abort () at /lib64/libc.so.6
    #2  0x00007fffec5f2491 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
    #3  0x00007fffec600752 in  () at /lib64/libc.so.6
    #4  0x0000555555874236 in monitor_qmp_bh_dispatcher (data=0x0)
        at /work/armbru/qemu/monitor.c:4088
    [...]

Crash on lexical error.  See review of json_parser_parse() below.

We lack test coverage.  Patch appended.

> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index da57f4cc24..3e88b27f9e 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -725,11 +725,6 @@ Visitor *qobject_input_visitor_new_str(const char *str,
>      if (is_json) {
>          obj = qobject_from_json(str, errp);
>          if (!obj) {
> -            /* Work around qobject_from_json() lossage TODO fix that */
> -            if (errp && !*errp) {
> -                error_setg(errp, "JSON parse error");
> -                return NULL;
> -            }
>              return NULL;
>          }
>          args = qobject_to(QDict, obj);

Likewise, what if json_parser_parse() returns null without setting an
error?

> diff --git a/qga/main.c b/qga/main.c
> index 043f7c3ead..9032bb0c7a 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -614,7 +614,7 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens)
>      }
>      req = qobject_to(QDict, obj);
>      if (!req) {
> -        error_setg(&err, QERR_JSON_PARSING);
> +        error_setg(&err, "Input must be a JSON object");

Commit message explains why this is appropriate.  Good.

>          goto err;
>      }
>      if (!qdict_haskey(req, "execute")) {
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 0c0b478149..c3b0c216cf 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -24,6 +24,7 @@
>  #include "qapi/qmp/json-parser.h"
>  #include "qapi/qmp/json-lexer.h"
>  #include "qapi/qmp/json-streamer.h"
> +#include "qapi/qmp/qerror.h"
>  
>  typedef struct JSONParserContext
>  {
> @@ -548,6 +549,14 @@ static QObject *parse_value(JSONParserContext *ctxt, va_list *ap)
>      }
>  }
>  
> +/**
> + * json_parser_parse:
> + *
> + * If @tokens is null, return null.
> + * Else if @tokens parse okay, return the parse tree.
> + * Else set an error and return null.
> + *
> + **/

We end doc comments with an unadorned */, in accordance with the GTK-Doc
manual.

The @tokens argument always comes from json_message_process_token():

    out_emit_bad:
        /*
         * Clear out token list and tell the parser to emit an error
         * indication by passing it a NULL list
         */
        json_message_free_tokens(parser);
    out_emit:
        /* send current list of tokens to parser and reset tokenizer */
        parser->brace_count = 0;
        parser->bracket_count = 0;
        /* parser->emit takes ownership of parser->tokens.  Remove our own
         * reference to parser->tokens before handing it out to parser->emit.
         */
        tokens = parser->tokens;
        parser->tokens = g_queue_new();
        parser->emit(parser, tokens);

parser->tokens is null exactly when we pass out_emit_bad.

Thus, the intent of null @tokens is to signal a lexical error.

Aside: there's no way to pass information on the error along with this
signal, but that's not this patch's problem.

>  QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
>  {
>      JSONParserContext ctxt = { .buf = tokens };
> @@ -559,7 +568,12 @@ QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
       QObject *result;

       if (!tokens) {
           return NULL;
       }

This does not match the intent declared in json_message_process_token().

>  
>      result = parse_value(&ctxt, ap);
>  
> -    error_propagate(errp, ctxt.err);
> +    if (!result && !ctxt.err) {
> +        /* TODO: improve error reporting */

I'd point out the issue explicitly here: parse_value() can return null
without setting ctxt.err.  No need to make more specific claims.
Although A quick eye over suggests parse_escape() is to blame.

> +        error_setg(errp, QERR_JSON_PARSING);
> +    } else {
> +        error_propagate(errp, ctxt.err);
> +    }
>  
>      g_queue_free_full(ctxt.buf, g_free);
>      g_free(ctxt.current);

I hope the JSON parser simplifications I have in mind will help take
care of this patch's issues, too.


>From f469aabeb5950d4891fbf5fa3a76ea333f540848 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Mon, 23 Jul 2018 10:08:29 +0200
Subject: [PATCH] check-qjson: Cover blank and lexically erroneous input

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qjson.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index d0144ba93c..3acd5a37a3 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1306,8 +1306,27 @@ static void simple_varargs(void)
 
 static void empty_input(void)
 {
-    const char *empty = "";
-    QObject *obj = qobject_from_json(empty, &error_abort);
+    QObject *obj = qobject_from_json("", &error_abort);
+    g_assert(obj == NULL);
+}
+
+static void blank_input(void)
+{
+    QObject *obj = qobject_from_json("", &error_abort);
+    g_assert(obj == NULL);
+}
+
+static void junk_input(void)
+{
+    Error *err = NULL;
+    QObject *obj;
+
+    obj = qobject_from_json("@", &err);
+    error_free_or_abort(&err);
+    g_assert(obj == NULL);
+
+    obj = qobject_from_json("{@", &err);
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
@@ -1452,7 +1471,9 @@ int main(int argc, char **argv)
 
     g_test_add_func("/varargs/simple_varargs", simple_varargs);
 
-    g_test_add_func("/errors/empty_input", empty_input);
+    g_test_add_func("/errors/empty", empty_input);
+    g_test_add_func("/errors/blank", blank_input);
+    g_test_add_func("/errors/junk", junk_input);
     g_test_add_func("/errors/unterminated/string", unterminated_string);
     g_test_add_func("/errors/unterminated/escape", unterminated_escape);
     g_test_add_func("/errors/unterminated/sq_string", unterminated_sq_string);
-- 
2.17.1

[Qemu-devel] [PATCH v2 14/18] json-lexer: make it safe to call multiple times

[Marc-André Lureau]

We can easily avoid the burden of checking if the lexer was
initialized prior to calling destroy by the caller, let's do it.

This allows simplification in state tracking in later patches of the
qmp-async RFC series.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qobject/json-lexer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index 980ba159d6..0eaba43a2c 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -386,5 +386,8 @@ int json_lexer_flush(JSONLexer *lexer)
 
 void json_lexer_destroy(JSONLexer *lexer)
 {
-    g_string_free(lexer->token, true);
+    if (lexer->token) {
+        g_string_free(lexer->token, true);
+        lexer->token = NULL;
+    }
 }
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 14/18] json-lexer: make it safe to call multiple times

[Markus Armbruster]

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

> We can easily avoid the burden of checking if the lexer was
> initialized prior to calling destroy by the caller, let's do it.
>
> This allows simplification in state tracking in later patches of the
> qmp-async RFC series.

Please include the patch in that series, where I can see its utility.

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qobject/json-lexer.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
> index 980ba159d6..0eaba43a2c 100644
> --- a/qobject/json-lexer.c
> +++ b/qobject/json-lexer.c
> @@ -386,5 +386,8 @@ int json_lexer_flush(JSONLexer *lexer)
>  
>  void json_lexer_destroy(JSONLexer *lexer)
>  {
> -    g_string_free(lexer->token, true);
> +    if (lexer->token) {
> +        g_string_free(lexer->token, true);
> +        lexer->token = NULL;
> +    }
>  }

[Qemu-devel] [PATCH v2 15/18] tests: add a few qemu-qmp tests

[Marc-André Lureau]

These 2 tests exhibited two qmp bugs that were fixed in 2.7
(series from commit e64c75a9752c5d0fd64eb2e684c656a5ea7d03c6 to
commit 1382d4abdf9619985e4078e37e49e487cea9935e)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/qmp-test.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index b9774084f8..54611e587f 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -249,7 +249,40 @@ static void test_qmp_oob(void)
     recv_cmd_id(qts, "blocks-2");
     recv_cmd_id(qts, "err-2");
     cleanup_blocking_cmd();
+}
+
+static void test_object_add_without_props(void)
+{
+    QTestState *qts;
+    QDict *ret;
+
+    qts = qtest_init(common_args);
 
+    ret = qtest_qmp(qts,
+                    "{'execute': 'object-add', 'arguments':"
+                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1' } }");
+    g_assert_nonnull(ret);
+
+    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
+
+    qobject_unref(ret);
+    qtest_quit(qts);
+}
+
+static void test_qom_set_without_value(void)
+{
+    QTestState *qts;
+    QDict *ret;
+
+    qts = qtest_init(common_args);
+
+    ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
+                    " { 'path': '/machine', 'property': 'rtc-time' } }");
+    g_assert_nonnull(ret);
+
+    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
+
+    qobject_unref(ret);
     qtest_quit(qts);
 }
 
@@ -479,8 +512,13 @@ int main(int argc, char *argv[])
 
     g_test_init(&argc, &argv, NULL);
 
+    qtest_add_func("qmp/object-add-without-props",
+                   test_object_add_without_props);
+    qtest_add_func("qmp/qom-set-without-value",
+                   test_qom_set_without_value);
     qtest_add_func("qmp/protocol", test_qmp_protocol);
     qtest_add_func("qmp/oob", test_qmp_oob);
+
     qmp_schema_init(&schema);
     add_query_tests(&schema);
     qtest_add_func("qmp/preconfig", test_qmp_preconfig);
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 15/18] tests: add a few qemu-qmp tests

[Markus Armbruster]

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

> These 2 tests exhibited two qmp bugs that were fixed in 2.7
> (series from commit e64c75a9752c5d0fd64eb2e684c656a5ea7d03c6 to
> commit 1382d4abdf9619985e4078e37e49e487cea9935e)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/qmp-test.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index b9774084f8..54611e587f 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -249,7 +249,40 @@ static void test_qmp_oob(void)
>      recv_cmd_id(qts, "blocks-2");
>      recv_cmd_id(qts, "err-2");
>      cleanup_blocking_cmd();
> +}
> +
> +static void test_object_add_without_props(void)
> +{
> +    QTestState *qts;
> +    QDict *ret;
> +
> +    qts = qtest_init(common_args);
>  
> +    ret = qtest_qmp(qts,
> +                    "{'execute': 'object-add', 'arguments':"
> +                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1' } }");
> +    g_assert_nonnull(ret);
> +
> +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> +
> +    qobject_unref(ret);
> +    qtest_quit(qts);
> +}
> +
> +static void test_qom_set_without_value(void)
> +{
> +    QTestState *qts;
> +    QDict *ret;
> +
> +    qts = qtest_init(common_args);
> +
> +    ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
> +                    " { 'path': '/machine', 'property': 'rtc-time' } }");
> +    g_assert_nonnull(ret);
> +
> +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> +
> +    qobject_unref(ret);
>      qtest_quit(qts);
>  }
>  
> @@ -479,8 +512,13 @@ int main(int argc, char *argv[])
>  
>      g_test_init(&argc, &argv, NULL);
>  
> +    qtest_add_func("qmp/object-add-without-props",
> +                   test_object_add_without_props);
> +    qtest_add_func("qmp/qom-set-without-value",
> +                   test_qom_set_without_value);
>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>      qtest_add_func("qmp/oob", test_qmp_oob);
> +
>      qmp_schema_init(&schema);
>      add_query_tests(&schema);
>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);

I've since moved tests that aren't protocol-related into qmp-cmd-test.

As discussed in review of v1, test_object_add_without_props() isn't
protocol-related.  It tests a bug in qmp_object_add() we fixed in commit
e64c75a975.  Sadly, we don't have systematic object-add tests.  This
lone test can go into qmp-cmd-test for want of a better home.

test_qom_set_without_value() isn't about qom-set, it's about a bug in
infrastructure used by the QMP core, fixed in commit c489780203.  We
covered the bug in infrastructure unit tests (commit bce3035a44).  If we
want to test it at the QMP level as well, the test could go into
qmp-test.  Do we want to?

In my quick rebase, I added both tests to qmp-cmd-test.c out of
laziness.

[Qemu-devel] [PATCH v2 16/18] tests: add a qmp success-response test

[Marc-André Lureau]

Verify the usage of this schema feature and the API behaviour.  This
should be the only case where qmp_dispatch() returns NULL without
error.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qmp-cmds.c                   | 17 +++++++++++++++++
 tests/qapi-schema/qapi-schema-test.json |  2 ++
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index ab414fa0c9..8d5100a324 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -32,6 +32,10 @@ void qmp_test_flags_command(Error **errp)
 {
 }
 
+void qmp_cmd_success_response(Error **errp)
+{
+}
+
 Empty2 *qmp_user_def_cmd0(Error **errp)
 {
     return g_new0(Empty2, 1);
@@ -153,6 +157,17 @@ static void test_dispatch_cmd_failure(void)
     qobject_unref(req);
 }
 
+static void test_dispatch_cmd_success_response(void)
+{
+    QDict *req = qdict_new();
+    QDict *resp;
+
+    qdict_put_str(req, "execute", "cmd-success-response");
+    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+    assert(resp == NULL);
+    qobject_unref(req);
+}
+
 static QObject *test_qmp_dispatch(QDict *req)
 {
     QDict *resp;
@@ -289,6 +304,8 @@ int main(int argc, char **argv)
     g_test_add_func("/qmp/dispatch_cmd", test_dispatch_cmd);
     g_test_add_func("/qmp/dispatch_cmd_failure", test_dispatch_cmd_failure);
     g_test_add_func("/qmp/dispatch_cmd_io", test_dispatch_cmd_io);
+    g_test_add_func("/qmp/dispatch_cmd_success_response",
+                    test_dispatch_cmd_success_response);
     g_test_add_func("/qmp/dealloc_types", test_dealloc_types);
     g_test_add_func("/qmp/dealloc_partial", test_dealloc_partial);
 
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 11aa4c8f8d..fb03163430 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -137,6 +137,8 @@
   'data': {'ud1a': 'UserDefOne', '*ud1b': 'UserDefOne'},
   'returns': 'UserDefTwo' }
 
+{ 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
+
 # Returning a non-dictionary requires a name from the whitelist
 { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
   'returns': 'int' }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 0da92455da..218ac7d556 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -156,6 +156,8 @@ object q_obj_user_def_cmd2-arg
     member ud1b: UserDefOne optional=True
 command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
    gen=True success_response=True boxed=False oob=False preconfig=False
+command cmd-success-response None -> None
+   gen=True success_response=False boxed=False oob=False preconfig=False
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True
-- 
2.18.0.129.ge3331758f1

[Qemu-devel] [PATCH v2 17/18] qga: process_event() simplification

[Marc-André Lureau]

Simplify the code around qmp_dispatch():
- rely on qmp_dispatch/check_obj() for message checking
- have a single send_response() point
- constify send_response() argument

It changes a couple of error messages:

* When @req isn't a dictionary, from
    Invalid JSON syntax
  to
    QMP input must be a JSON object

* When @req lacks member "execute", from
    this feature or command is not currently supported
  to
    QMP input lacks member 'execute'

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/main.c | 54 +++++++++++++-----------------------------------------
 1 file changed, 13 insertions(+), 41 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 9032bb0c7a..f5c772314f 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -545,15 +545,15 @@ fail:
 #endif
 }
 
-static int send_response(GAState *s, QDict *payload)
+static int send_response(GAState *s, const QDict *rsp)
 {
     const char *buf;
     QString *payload_qstr, *response_qstr;
     GIOStatus status;
 
-    g_assert(payload && s->channel);
+    g_assert(rsp && s->channel);
 
-    payload_qstr = qobject_to_json(QOBJECT(payload));
+    payload_qstr = qobject_to_json(QOBJECT(rsp));
     if (!payload_qstr) {
         return -EINVAL;
     }
@@ -579,63 +579,35 @@ static int send_response(GAState *s, QDict *payload)
     return 0;
 }
 
-static void process_command(GAState *s, QDict *req)
-{
-    QDict *rsp;
-    int ret;
-
-    g_assert(req);
-    g_debug("processing command");
-    rsp = qmp_dispatch(&ga_commands, QOBJECT(req), false);
-    if (rsp) {
-        ret = send_response(s, rsp);
-        if (ret < 0) {
-            g_warning("error sending response: %s", strerror(-ret));
-        }
-        qobject_unref(rsp);
-    }
-}
-
 /* handle requests/control events coming in over the channel */
 static void process_event(JSONMessageParser *parser, GQueue *tokens)
 {
     GAState *s = container_of(parser, GAState, parser);
-    QObject *obj;
-    QDict *req, *rsp;
+    QObject *req;
+    QDict *rsp = NULL;
     Error *err = NULL;
     int ret;
 
     g_assert(s && parser);
 
     g_debug("process_event: called");
-    obj = json_parser_parse(tokens, NULL, &err);
+
+    req = json_parser_parse(tokens, NULL, &err);
     if (err) {
-        goto err;
-    }
-    req = qobject_to(QDict, obj);
-    if (!req) {
-        error_setg(&err, "Input must be a JSON object");
-        goto err;
-    }
-    if (!qdict_haskey(req, "execute")) {
-        g_warning("unrecognized payload format");
-        error_setg(&err, QERR_UNSUPPORTED);
-        goto err;
+        rsp = qmp_error_response(err);
+        goto end;
     }
 
-    process_command(s, req);
-    qobject_unref(obj);
-    return;
+    g_debug("processing command");
+    rsp = qmp_dispatch(&ga_commands, req, false);
 
-err:
-    g_warning("failed to parse event: %s", error_get_pretty(err));
-    rsp = qmp_error_response(err);
+end:
     ret = send_response(s, rsp);
     if (ret < 0) {
         g_warning("error sending error response: %s", strerror(-ret));
     }
     qobject_unref(rsp);
-    qobject_unref(obj);
+    qobject_unref(req);
 }
 
 /* false return signals GAChannel to close the current client connection */
-- 
2.18.0.129.ge3331758f1

[Qemu-devel] [PATCH v2 18/18] RFC: qmp: common 'id' handling & make QGA conform to QMP spec

[Marc-André Lureau]

Let qmp_dispatch() copy the 'id' field. That way any qmp client will
conform to the specification, including QGA. Furthermore, it
simplifies the work for qemu monitor.

CC: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c           | 30 +++++++++++-------------------
 qapi/qmp-dispatch.c | 12 +++++++++---
 tests/test-qga.c    | 13 +++++--------
 3 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5a41a230b9..11249e7018 100644
--- a/monitor.c
+++ b/monitor.c
@@ -249,8 +249,6 @@ QEMUBH *qmp_dispatcher_bh;
 struct QMPRequest {
     /* Owner of the request */
     Monitor *mon;
-    /* "id" field of the request */
-    QObject *id;
     /*
      * Request object to be handled or Error to be reported
      * (exactly one of them is non-null)
@@ -351,7 +349,6 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
 
 static void qmp_request_free(QMPRequest *req)
 {
-    qobject_unref(req->id);
     qobject_unref(req->req);
     error_free(req->err);
     g_free(req);
@@ -3991,18 +3988,14 @@ static int monitor_can_read(void *opaque)
  * Null @rsp can only happen for commands with QCO_NO_SUCCESS_RESP.
  * Nothing is emitted then.
  */
-static void monitor_qmp_respond(Monitor *mon, QDict *rsp, QObject *id)
+static void monitor_qmp_respond(Monitor *mon, QDict *rsp)
 {
     if (rsp) {
-        if (id) {
-            qdict_put_obj(rsp, "id", qobject_ref(id));
-        }
-
         qmp_send_response(mon, rsp);
     }
 }
 
-static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
+static void monitor_qmp_dispatch(Monitor *mon, QObject *req)
 {
     Monitor *old_mon;
     QDict *rsp;
@@ -4027,7 +4020,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
         }
     }
 
-    monitor_qmp_respond(mon, rsp, id);
+    monitor_qmp_respond(mon, rsp);
     qobject_unref(rsp);
 }
 
@@ -4082,13 +4075,15 @@ static void monitor_qmp_bh_dispatcher(void *data)
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
     need_resume = !qmp_oob_enabled(req_obj->mon);
     if (req_obj->req) {
-        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
-        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+        QDict *qdict = qobject_to(QDict, req_obj->req);
+        QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
+        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
+        monitor_qmp_dispatch(req_obj->mon, req_obj->req);
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
         req_obj->err = NULL;
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);
+        monitor_qmp_respond(req_obj->mon, rsp);
         qobject_unref(rsp);
     }
 
@@ -4117,8 +4112,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 
     qdict = qobject_to(QDict, req);
     if (qdict) {
-        id = qobject_ref(qdict_get(qdict, "id"));
-        qdict_del(qdict, "id");
+        id = qdict_get(qdict, "id");
     } /* else will fail qmp_dispatch() */
 
     if (req && trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
@@ -4129,15 +4123,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 
     if (qdict && qmp_is_oob(qdict)) {
         /* OOB commands are executed immediately */
-        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id)
-                                          ?: "");
-        monitor_qmp_dispatch(mon, req, id);
+        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) ?: "");
+        monitor_qmp_dispatch(mon, req);
         return;
     }
 
     req_obj = g_new0(QMPRequest, 1);
     req_obj->mon = mon;
-    req_obj->id = id;
     req_obj->req = req;
     req_obj->err = err;
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 90ba5e3074..e714cfb8ff 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -59,6 +59,8 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
                            "QMP input member 'arguments' must be an object");
                 return NULL;
             }
+        } else if (!strcmp(arg_name, "id")) {
+            continue;
         } else {
             error_setg(errp, "QMP input member '%s' is unexpected",
                        arg_name);
@@ -166,11 +168,11 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
                     bool allow_oob)
 {
     Error *err = NULL;
-    QObject *ret;
+    QDict *dict = qobject_to(QDict, request);
+    QObject *id = dict ? qdict_get(dict, "id") : NULL;
+    QObject *ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
     QDict *rsp;
 
-    ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
-
     if (err) {
         rsp = qmp_error_response(err);
     } else if (ret) {
@@ -181,5 +183,9 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
         rsp = NULL;
     }
 
+    if (rsp && id) {
+        qdict_put_obj(rsp, "id", qobject_ref(id));
+    }
+
     return rsp;
 }
diff --git a/tests/test-qga.c b/tests/test-qga.c
index d638b1571a..4ee8b405f4 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -227,18 +227,15 @@ static void test_qga_ping(gconstpointer fix)
     qobject_unref(ret);
 }
 
-static void test_qga_invalid_id(gconstpointer fix)
+static void test_qga_id(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *error;
-    const char *class;
+    QDict *ret;
 
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}");
     g_assert_nonnull(ret);
-
-    error = qdict_get_qdict(ret, "error");
-    class = qdict_get_try_str(error, "class");
-    g_assert_cmpstr(class, ==, "GenericError");
+    qmp_assert_no_error(ret);
+    g_assert_cmpint(qdict_get_int(ret, "id"), ==, 1);
 
     qobject_unref(ret);
 }
@@ -1014,7 +1011,7 @@ int main(int argc, char **argv)
     g_test_add_data_func("/qga/file-ops", &fix, test_qga_file_ops);
     g_test_add_data_func("/qga/file-write-read", &fix, test_qga_file_write_read);
     g_test_add_data_func("/qga/get-time", &fix, test_qga_get_time);
-    g_test_add_data_func("/qga/invalid-id", &fix, test_qga_invalid_id);
+    g_test_add_data_func("/qga/id", &fix, test_qga_id);
     g_test_add_data_func("/qga/invalid-oob", &fix, test_qga_invalid_oob);
     g_test_add_data_func("/qga/invalid-cmd", &fix, test_qga_invalid_cmd);
     g_test_add_data_func("/qga/invalid-args", &fix, test_qga_invalid_args);
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH v2 18/18] RFC: qmp: common 'id' handling & make QGA conform to QMP spec

[Markus Armbruster]

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

> Let qmp_dispatch() copy the 'id' field. That way any qmp client will
> conform to the specification, including QGA. Furthermore, it
> simplifies the work for qemu monitor.
>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c           | 30 +++++++++++-------------------
>  qapi/qmp-dispatch.c | 12 +++++++++---
>  tests/test-qga.c    | 13 +++++--------
>  3 files changed, 25 insertions(+), 30 deletions(-)

I support adding this feature to QGA.

Needs Mike Roth's Ack or R-by.

> diff --git a/monitor.c b/monitor.c
> index 5a41a230b9..11249e7018 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -249,8 +249,6 @@ QEMUBH *qmp_dispatcher_bh;
>  struct QMPRequest {
>      /* Owner of the request */
>      Monitor *mon;
> -    /* "id" field of the request */
> -    QObject *id;
>      /*
>       * Request object to be handled or Error to be reported
>       * (exactly one of them is non-null)
> @@ -351,7 +349,6 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>  
>  static void qmp_request_free(QMPRequest *req)
>  {
> -    qobject_unref(req->id);
>      qobject_unref(req->req);
>      error_free(req->err);
>      g_free(req);
> @@ -3991,18 +3988,14 @@ static int monitor_can_read(void *opaque)
>   * Null @rsp can only happen for commands with QCO_NO_SUCCESS_RESP.
>   * Nothing is emitted then.
>   */
> -static void monitor_qmp_respond(Monitor *mon, QDict *rsp, QObject *id)
> +static void monitor_qmp_respond(Monitor *mon, QDict *rsp)
>  {
>      if (rsp) {
> -        if (id) {
> -            qdict_put_obj(rsp, "id", qobject_ref(id));
> -        }
> -
>          qmp_send_response(mon, rsp);
>      }
>  }
>  
> -static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
> +static void monitor_qmp_dispatch(Monitor *mon, QObject *req)
>  {
>      Monitor *old_mon;
>      QDict *rsp;
> @@ -4027,7 +4020,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
>          }
>      }
>  
> -    monitor_qmp_respond(mon, rsp, id);
> +    monitor_qmp_respond(mon, rsp);
>      qobject_unref(rsp);
>  }
>  
> @@ -4082,13 +4075,15 @@ static void monitor_qmp_bh_dispatcher(void *data)
>      /*  qmp_oob_enabled() might change after "qmp_capabilities" */
>      need_resume = !qmp_oob_enabled(req_obj->mon);
>      if (req_obj->req) {
> -        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> -        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
> +        QDict *qdict = qobject_to(QDict, req_obj->req);
> +        QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
> +        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
> +        monitor_qmp_dispatch(req_obj->mon, req_obj->req);
>      } else {
>          assert(req_obj->err);
>          rsp = qmp_error_response(req_obj->err);
>          req_obj->err = NULL;
> -        monitor_qmp_respond(req_obj->mon, rsp, NULL);
> +        monitor_qmp_respond(req_obj->mon, rsp);
>          qobject_unref(rsp);
>      }
>  
> @@ -4117,8 +4112,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>  
>      qdict = qobject_to(QDict, req);
>      if (qdict) {
> -        id = qobject_ref(qdict_get(qdict, "id"));
> -        qdict_del(qdict, "id");
> +        id = qdict_get(qdict, "id");

Write of @id.

>      } /* else will fail qmp_dispatch() */
>  
>      if (req && trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
           QString *req_json = qobject_to_json(req);
           trace_handle_qmp_command(mon, qstring_get_str(req_json));
           qobject_unref(req_json);
       }
> @@ -4129,15 +4123,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>  
>      if (qdict && qmp_is_oob(qdict)) {
>          /* OOB commands are executed immediately */
> -        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id)
> -                                          ?: "");
> -        monitor_qmp_dispatch(mon, req, id);
> +        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) ?: "");
> +        monitor_qmp_dispatch(mon, req);

First use of @id.

>      }
>  
>      req_obj = g_new0(QMPRequest, 1);
>      req_obj->mon = mon;
> -    req_obj->id = id;
>      req_obj->req = req;
>      req_obj->err = err;
>  
       /* Protect qmp_requests and fetching its length. */
       qemu_mutex_lock(&mon->qmp.qmp_queue_lock);

       /*
        * If OOB is not enabled on the current monitor, we'll emulate the
        * old behavior that we won't process the current monitor any more
        * until it has responded.  This helps make sure that as long as
        * OOB is not enabled, the server will never drop any command.
        */
       if (!qmp_oob_enabled(mon)) {
           monitor_suspend(mon);
       } else {
           /* Drop the request if queue is full. */
           if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
               qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
               /*
                * FIXME @id's scope is just @mon, and broadcasting it is
                * wrong.  If another monitor's client has a command with
                * the same ID in flight, the event will incorrectly claim
                * that command was dropped.
                */
               qapi_event_send_command_dropped(id,
                                               COMMAND_DROP_REASON_QUEUE_FULL,
                                               &error_abort);
               qmp_request_free(req_obj);
               return;

Second use of @id, will go away when we get rid of flawed event
COMMAND_DROPPED.

           }
       }

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

Comment needs an update: @id is no longer transferred.

        * etc. will be delivered to the handler side.
        */
       g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
       qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);

       /* Kick the dispatcher routine */
       qemu_bh_schedule(qmp_dispatcher_bh);
   }

Once COMMAND_DROPPED is gone, the assignment to @id should go next to
its only remaining use.

Perhaps we should remove it before this patch to reduce churn.

> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 90ba5e3074..e714cfb8ff 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -59,6 +59,8 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
>                             "QMP input member 'arguments' must be an object");
>                  return NULL;
>              }
> +        } else if (!strcmp(arg_name, "id")) {
> +            continue;
>          } else {
>              error_setg(errp, "QMP input member '%s' is unexpected",
>                         arg_name);
> @@ -166,11 +168,11 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
>                      bool allow_oob)
>  {
>      Error *err = NULL;
> -    QObject *ret;
> +    QDict *dict = qobject_to(QDict, request);
> +    QObject *id = dict ? qdict_get(dict, "id") : NULL;
> +    QObject *ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
>      QDict *rsp;
>  
> -    ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
> -
>      if (err) {

This separates the call from its error check.  I don't like that.
Recommend:

       QObject *ret;
  +    QDict *dict = qobject_to(QDict, request);
  +    QObject *id = dict ? qdict_get(dict, "id") : NULL;
       QDict *rsp;
   
       ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
  -
       if (err) {

>          rsp = qmp_error_response(err);
>      } else if (ret) {
> @@ -181,5 +183,9 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
>          rsp = NULL;
>      }
>  
> +    if (rsp && id) {
> +        qdict_put_obj(rsp, "id", qobject_ref(id));
> +    }
> +
>      return rsp;
>  }
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index d638b1571a..4ee8b405f4 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -227,18 +227,15 @@ static void test_qga_ping(gconstpointer fix)
>      qobject_unref(ret);
>  }
>  
> -static void test_qga_invalid_id(gconstpointer fix)
> +static void test_qga_id(gconstpointer fix)
>  {
>      const TestFixture *fixture = fix;
> -    QDict *ret, *error;
> -    const char *class;
> +    QDict *ret;
>  
>      ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}");
>      g_assert_nonnull(ret);
> -
> -    error = qdict_get_qdict(ret, "error");
> -    class = qdict_get_try_str(error, "class");
> -    g_assert_cmpstr(class, ==, "GenericError");
> +    qmp_assert_no_error(ret);
> +    g_assert_cmpint(qdict_get_int(ret, "id"), ==, 1);
>  
>      qobject_unref(ret);
>  }
> @@ -1014,7 +1011,7 @@ int main(int argc, char **argv)
>      g_test_add_data_func("/qga/file-ops", &fix, test_qga_file_ops);
>      g_test_add_data_func("/qga/file-write-read", &fix, test_qga_file_write_read);
>      g_test_add_data_func("/qga/get-time", &fix, test_qga_get_time);
> -    g_test_add_data_func("/qga/invalid-id", &fix, test_qga_invalid_id);
> +    g_test_add_data_func("/qga/id", &fix, test_qga_id);
>      g_test_add_data_func("/qga/invalid-oob", &fix, test_qga_invalid_oob);
>      g_test_add_data_func("/qga/invalid-cmd", &fix, test_qga_invalid_cmd);
>      g_test_add_data_func("/qga/invalid-args", &fix, test_qga_invalid_args);

I thought "doc update missing", but then I checked qmp-spec.txt, and
it doesn't mention "id" works only for QEMU. not for QGA.  And then I
realized you pointed that out in your commit message.

With the comment updated:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 00/18] monitor: various code simplification and fixes

[Markus Armbruster]

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

> Hi,
>
> This series is a rebased subset of "[PATCH v3 00/38] RFC: monitor: add
> asynchronous command type" with code cleanups and improvements that
> are worth to consider for 3.0.
>
> The series applies on master, and will conflict with the pending
> series "[PATCH 0/9] monitor: enable OOB by default" from Peter.
>
> In particular, reverting "qmp: isolate responses into io thread" is
> quite intrusive, but is a nice simplification that is worth to
> consider before modifying/maintaining it further. Also, "monitor: no
> need to save need_resume" could be dropped if Peter "[PATCH 5/9]
> monitor: suspend monitor instead of send CMD_DROP" is adopted (see
> discussion about need_resume there).
>
> The last patch, "RFC: qmp: rework 'id' handling" simplifies a bit
> monitor "id" handling, and makes qemu-ga conform to the QMP
> specification by copying "id" from the request in the reply.  This is
> the opposite to Markus change "qmp qemu-ga: Revert change that
> accidentally made qemu-ga accept 'id'".
>
> (the first patch is already in Markus qapi-next branch, it is there
> for patchew testing)
[...]

I took the liberty to rebase this onto my "[PATCH 00/56] json: Fixes,
error reporting improvements, cleanups", which is by the way also
available at
http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/json-fixes

> Marc-André Lureau (18):
>   tests: change /0.15/* tests to /qmp/*

Kept (not really part of this series, as noted above)

>   monitor: consitify qmp_send_response() QDict argument
>   qmp: constify qmp_is_oob()
>   Revert "qmp: isolate responses into io thread"
>   monitor: no need to save need_resume

Kept, has my R-by.

I figure the last two need to be considered in context of OOB work that
hasn't landed, yet.  Discussed in review of v1.

>   qga: process_event() simplification and leak fix

Already in master as commit ae7da1e5f6.

>   qmp: drop json_parser_parse() wrapper

Replaced by my series.

>   json-parser: simplify and avoid JSONParserContext allocation

Picked into my series.

>   json-parser: further simplify freeing JSONParserContext
>   qjson: report an error if there are multiple results
>   qjson: report error on unterminated string
>   qjson: return parsing error if unterminated input
>   json-parser: set an error if parsing returned NULL

Replaced by my series.

>   json-lexer: make it safe to call multiple times
>   tests: add a few qemu-qmp tests
>   tests: add a qmp success-response test
>   qga: process_event() simplification
>   RFC: qmp: common 'id' handling & make QGA conform to QMP spec

Kept, only the success-response test has my R-by.

Result:
http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/elmarco-monitor

[...]