[Qemu-devel] [PATCH 00/12] RFC: monitor: various code simplification and fixes

[Qemu-devel] [PATCH 00/12] RFC: 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)

Marc-André Lureau (12):
  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
  json-parser: always set an error if return 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: rework 'id' handling

 include/qapi/qmp/dispatch.h             |   2 +-
 block.c                                 |   5 -
 monitor.c                               | 171 +++---------------------
 qapi/qmp-dispatch.c                     |  12 +-
 qapi/qobject-input-visitor.c            |   5 -
 qga/main.c                              |  66 +++------
 qobject/json-lexer.c                    |   5 +-
 qobject/json-parser.c                   |   8 +-
 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 +
 13 files changed, 129 insertions(+), 227 deletions(-)

-- 
2.18.0.rc1

[Qemu-devel] [PATCH 01/12] 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.rc1

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

[Marc-André Lureau]

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

diff --git a/monitor.c b/monitor.c
index 3c9c97b73f..fc481d902d 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.rc1

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

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@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.rc1

[Qemu-devel] [PATCH 04/12] 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>
---
 monitor.c | 120 ++----------------------------------------------------
 1 file changed, 3 insertions(+), 117 deletions(-)

diff --git a/monitor.c b/monitor.c
index fc481d902d..462cf96f7b 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);
     }
 }
 
@@ -4395,7 +4299,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;
@@ -4406,7 +4310,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);
@@ -4508,15 +4411,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)
@@ -4668,12 +4562,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) {
@@ -4687,8 +4575,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.rc1

[Qemu-devel] [PATCH 05/12] 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>
---
 monitor.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index 462cf96f7b..ec40a80d81 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,13 @@ 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;
     }
 
+    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);
@@ -4094,7 +4090,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);
     }
@@ -4146,7 +4142,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);
@@ -4159,7 +4154,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.rc1

[Qemu-devel] [PATCH 06/12] 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.

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

diff --git a/qga/main.c b/qga/main.c
index 537cc0e162..0784761605 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -600,6 +600,7 @@ static void process_command(GAState *s, QDict *req)
 static void process_event(JSONMessageParser *parser, GQueue *tokens)
 {
     GAState *s = container_of(parser, GAState, parser);
+    QObject *obj;
     QDict *qdict;
     Error *err = NULL;
     int ret;
@@ -607,35 +608,34 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens)
     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));
-        }
+    qdict = qobject_to(QDict, obj);
+    if (!qdict) {
+        error_setg(&err, QERR_JSON_PARSING);
+        goto err;
+    }
+    if (!qdict_haskey(qdict, "execute")) {
+        g_warning("unrecognized payload format");
+        error_setg(&err, QERR_UNSUPPORTED);
+        goto err;
     }
 
+    process_command(s, qdict);
+    qobject_unref(obj);
+    return;
+
+err:
+    g_warning("failed to parse event: %s", error_get_pretty(err));
+    qdict = qmp_error_response(err);
+    ret = send_response(s, qdict);
+    if (ret < 0) {
+        g_warning("error sending error response: %s", strerror(-ret));
+    }
     qobject_unref(qdict);
+    qobject_unref(obj);
 }
 
 /* false return signals GAChannel to close the current client connection */
-- 
2.18.0.rc1

[Qemu-devel] [PATCH 07/12] json-parser: always set an error if return NULL

[Marc-André Lureau]

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

 * qga/main.c process_event() doesn't need further changes after
   previous cleanup.

 * 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 - abort, ok
    ~ tests/test-visitor-serialization.c - abort, ok

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

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

diff --git a/block.c b/block.c
index ac8b3a3511..9046d66465 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 ec40a80d81..a3efe96d1d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4112,10 +4112,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     QMPRequest *req_obj;
 
     req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
 
     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/qobject/json-parser.c b/qobject/json-parser.c
index a5aa790d62..82c3167629 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
 {
@@ -591,7 +592,12 @@ QObject *json_parser_parse_err(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);
+    }
 
     parser_context_free(ctxt);
 
-- 
2.18.0.rc1

[Qemu-devel] [PATCH 08/12] 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.rc1

[Qemu-devel] [PATCH 09/12] 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 ceaf4a6789..084c5edff0 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -249,7 +249,39 @@ 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 +511,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);
@@ -488,5 +525,6 @@ int main(int argc, char *argv[])
     ret = g_test_run();
 
     qmp_schema_cleanup(&schema);
+
     return ret;
 }
-- 
2.18.0.rc1

[Qemu-devel] [PATCH 10/12] 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>
---
 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.rc1

[Qemu-devel] [PATCH 11/12] 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
- rsp/req variable renaming for clarity

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

diff --git a/qga/main.c b/qga/main.c
index 0784761605..b0a88ee5cf 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 *qdict;
+    QObject *req;
+    QDict *rsp = NULL;
     Error *err = NULL;
     int ret;
 
     g_assert(s && parser);
 
     g_debug("process_event: called");
-    obj = json_parser_parse_err(tokens, NULL, &err);
+
+    req = json_parser_parse_err(tokens, NULL, &err);
     if (err) {
-        goto err;
-    }
-    qdict = qobject_to(QDict, obj);
-    if (!qdict) {
-        error_setg(&err, QERR_JSON_PARSING);
-        goto err;
-    }
-    if (!qdict_haskey(qdict, "execute")) {
-        g_warning("unrecognized payload format");
-        error_setg(&err, QERR_UNSUPPORTED);
-        goto err;
+        rsp = qmp_error_response(err);
+        goto end;
     }
 
-    process_command(s, qdict);
-    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));
-    qdict = qmp_error_response(err);
-    ret = send_response(s, qdict);
+end:
+    ret = send_response(s, rsp);
     if (ret < 0) {
         g_warning("error sending error response: %s", strerror(-ret));
     }
-    qobject_unref(qdict);
-    qobject_unref(obj);
+    qobject_unref(rsp);
+    qobject_unref(req);
 }
 
 /* false return signals GAChannel to close the current client connection */
-- 
2.18.0.rc1

[Qemu-devel] [PATCH 12/12] RFC: qmp: rework 'id' handling

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c           | 31 ++++++++++++-------------------
 qapi/qmp-dispatch.c | 10 ++++++++--
 tests/test-qga.c    | 13 +++++--------
 3 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/monitor.c b/monitor.c
index a3efe96d1d..bf192697e4 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);
 }
 
@@ -4081,12 +4074,15 @@ static void monitor_qmp_bh_dispatcher(void *data)
 
     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);
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);
+        req_obj->err = NULL;
+        monitor_qmp_respond(req_obj->mon, rsp);
         qobject_unref(rsp);
     }
 
@@ -4115,8 +4111,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 (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
@@ -4127,15 +4122,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..acea0fcfcd 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,8 +168,8 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
                     bool allow_oob)
 {
     Error *err = NULL;
-    QObject *ret;
-    QDict *rsp;
+    QDict *rsp, *dict = qobject_to(QDict, request);
+    QObject *ret, *id = dict ? qdict_get(dict, "id") : NULL;
 
     ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
 
@@ -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.rc1

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

[Markus Armbruster]

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

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 3c9c97b73f..fc481d902d 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) :

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

Re: [Qemu-devel] [PATCH 03/12] qmp: constify qmp_is_oob()

[Markus Armbruster]

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

> Signed-off-by: Marc-André Lureau <marcandre.lureau@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");

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

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

[Markus Armbruster]

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

> 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>
> ---
>  monitor.c | 120 ++----------------------------------------------------
>  1 file changed, 3 insertions(+), 117 deletions(-)

The diffstat speaks for itself.

The revert conflicts pretty badly, so I'm going over it hunk by hunk.

>
> diff --git a/monitor.c b/monitor.c
> index fc481d902d..462cf96f7b 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;
>  
>  /*

Not reverted:

       bool skip_flush;
       bool use_io_thr;

  +    /* We can't access guest memory when holding the lock */
       QemuMutex out_lock;
       QString *outbuf;
       guint out_watch;

Commit dc7cbcd8fae moved it elswhere.  Besides, it's a keeper.  It
shouldn't have been in commit abe3cd0ff7f in the first place.

Good.

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

Not reverted:

  @@ -416,7 +421,8 @@ int monitor_fprintf(FILE *stream, const char *fmt, ...)
       return 0;
   }

  -static void monitor_json_emitter(Monitor *mon, const QObject *data)
  +static void monitor_json_emitter_raw(Monitor *mon,
  +                                     QObject *data)
   {
       QString *json;

Commit 65e3fe6743a renamed it once more, to qmp_send_response().

Good.

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

These are followup fixes from commit 6d2d563f8cc "qmp: cleanup qmp
queues properly".  I think you're reverting exactly its response queue
part, mostly here, but there's one more below.

Good.

> @@ -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 },

Again, conflicts due to followup fixes, notably

    40687eb741a monitor: rename *_pop_one to *_pop_any
    c73a843b4a8 monitor: flush qmp responses when CLOSED
    65e3fe6743a qmp: Replace monitor_json_emitter{,raw}() by qmp_{queue,send}_response()

Good, I think.

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

The patch you revert doesn't have a hunk here, because it achieved the
change by replacing monitor_json_emitter().  The old one became
monitor_json_emitter_raw().  Both have been renamed since.  You switch
back to the old one.

Good.

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

A hunk with out a conflict, awesome!

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

The first deletion is due to the response queue part of 6d2d563f8cc
"qmp: cleanup qmp queues properly".  See note above.

Good.

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

Same argument as for monitor_qapi_event_emit().

Good.

> @@ -4395,7 +4299,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;

Likewise.

> @@ -4406,7 +4310,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);

Another bit of commit c73a843b4a8 that needs to go.

Only refactorings remain.  Perhaps c73a843b4a8 should've been split.
Doesn't matter now.

Good.

> @@ -4508,15 +4411,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);
>  }
>  

Only trivial conflicts.

>  void monitor_init_globals(void)
> @@ -4668,12 +4562,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) {

Likewise.

> @@ -4687,8 +4575,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;

Likewise.

Looks good to me, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

However, Peter has argued for keeping the response queue:

  For my own opinion, I'll see it a bit awkward (as I mentioned) to
  revert the response queue part.  Again, it's mostly working well
  there, we just fix things up when needed.  It's not really a big part
  of code to maintain, and it still looks sane to me to have such an
  isolation so that we can have the dispatcher totally separate from the
  chardev IO path (I'd say if I design a QMP interface from scratch,
  I'll do that from the first day).  So I am not against reverting that
  part at all, I just don't see much gain from that as well, especially
  if we want to make QMP more modular in the future.

The argument worth considering here is "isolating the dispatcher from
the chardev IO path".  Opinions?

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

[Marc-André Lureau]

Hi

On Thu, Jul 12, 2018 at 3:14 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> 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>
>> ---
>>  monitor.c | 120 ++----------------------------------------------------
>>  1 file changed, 3 insertions(+), 117 deletions(-)
>
> The diffstat speaks for itself.
>
> The revert conflicts pretty badly, so I'm going over it hunk by hunk.
>
>>
>> diff --git a/monitor.c b/monitor.c
>> index fc481d902d..462cf96f7b 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;
>>
>>  /*
>
> Not reverted:
>
>        bool skip_flush;
>        bool use_io_thr;
>
>   +    /* We can't access guest memory when holding the lock */
>        QemuMutex out_lock;
>        QString *outbuf;
>        guint out_watch;
>
> Commit dc7cbcd8fae moved it elswhere.  Besides, it's a keeper.  It
> shouldn't have been in commit abe3cd0ff7f in the first place.
>
> Good.
>
>> @@ -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;
>
> Not reverted:
>
>   @@ -416,7 +421,8 @@ int monitor_fprintf(FILE *stream, const char *fmt, ...)
>        return 0;
>    }
>
>   -static void monitor_json_emitter(Monitor *mon, const QObject *data)
>   +static void monitor_json_emitter_raw(Monitor *mon,
>   +                                     QObject *data)
>    {
>        QString *json;
>
> Commit 65e3fe6743a renamed it once more, to qmp_send_response().
>
> Good.
>
>> @@ -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);
>>  }
>>
>
> These are followup fixes from commit 6d2d563f8cc "qmp: cleanup qmp
> queues properly".  I think you're reverting exactly its response queue
> part, mostly here, but there's one more below.
>
> Good.
>
>> @@ -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 },
>
> Again, conflicts due to followup fixes, notably
>
>     40687eb741a monitor: rename *_pop_one to *_pop_any
>     c73a843b4a8 monitor: flush qmp responses when CLOSED
>     65e3fe6743a qmp: Replace monitor_json_emitter{,raw}() by qmp_{queue,send}_response()
>
> Good, I think.
>
>> @@ -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);
>>          }
>>      }
>>  }
>
> The patch you revert doesn't have a hunk here, because it achieved the
> change by replacing monitor_json_emitter().  The old one became
> monitor_json_emitter_raw().  Both have been renamed since.  You switch
> back to the old one.
>
> Good.
>
>> @@ -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)
>
> A hunk with out a conflict, awesome!
>
>> @@ -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);
>>  }
>>
>
> The first deletion is due to the response queue part of 6d2d563f8cc
> "qmp: cleanup qmp queues properly".  See note above.
>
> Good.
>
>>  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);
>>      }
>>  }
>>
>
> Same argument as for monitor_qapi_event_emit().
>
> Good.
>
>> @@ -4395,7 +4299,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;
>
> Likewise.
>
>> @@ -4406,7 +4310,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);
>
> Another bit of commit c73a843b4a8 that needs to go.
>
> Only refactorings remain.  Perhaps c73a843b4a8 should've been split.
> Doesn't matter now.
>
> Good.
>
>> @@ -4508,15 +4411,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);
>>  }
>>
>
> Only trivial conflicts.
>
>>  void monitor_init_globals(void)
>> @@ -4668,12 +4562,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) {
>
> Likewise.
>
>> @@ -4687,8 +4575,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;
>
> Likewise.
>
> Looks good to me, so
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

thanks for the review,

>
> However, Peter has argued for keeping the response queue:
>
>   For my own opinion, I'll see it a bit awkward (as I mentioned) to
>   revert the response queue part.  Again, it's mostly working well
>   there, we just fix things up when needed.  It's not really a big part
>   of code to maintain, and it still looks sane to me to have such an
>   isolation so that we can have the dispatcher totally separate from the
>   chardev IO path (I'd say if I design a QMP interface from scratch,
>   I'll do that from the first day).  So I am not against reverting that
>   part at all, I just don't see much gain from that as well, especially
>   if we want to make QMP more modular in the future.
>
> The argument worth considering here is "isolating the dispatcher from
> the chardev IO path".  Opinions?

Imho, this is extra unneeded work. The queue is necessary on the
receive side, to dispatch between iothread and main loop, but there
isn't such need for sending the response stream.

Also, there is already a sending "buffer queue": it's "outbuf". To me,
they are duplicating the response buffering, thus making things
unnecessarily more complicated.

If we needed, we could modify monitor_flush_locked() to add a sending
watch only. This would have mostly the same effect as scheduling the
bh for processing the qmp_responses queue. But since chardev sending
is thread-safe, it's not a problem to save the watch handler switch,
and try sending directly from the iothread.

it's not critical, so we can delay this cleanup to 3.1. (I wish it
would have been merged early during this cycle, like most of my
pending series, but hey, we have more important things to do ;)


-- 
Marc-André Lureau

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

[Peter Xu]

On Thu, Jul 12, 2018 at 03:32:51PM +0200, Marc-André Lureau wrote:

[...]

> >
> > However, Peter has argued for keeping the response queue:
> >
> >   For my own opinion, I'll see it a bit awkward (as I mentioned) to
> >   revert the response queue part.  Again, it's mostly working well
> >   there, we just fix things up when needed.  It's not really a big part
> >   of code to maintain, and it still looks sane to me to have such an
> >   isolation so that we can have the dispatcher totally separate from the
> >   chardev IO path (I'd say if I design a QMP interface from scratch,
> >   I'll do that from the first day).  So I am not against reverting that
> >   part at all, I just don't see much gain from that as well, especially
> >   if we want to make QMP more modular in the future.
> >
> > The argument worth considering here is "isolating the dispatcher from
> > the chardev IO path".  Opinions?
> 
> Imho, this is extra unneeded work. The queue is necessary on the
> receive side, to dispatch between iothread and main loop, but there
> isn't such need for sending the response stream.
> 
> Also, there is already a sending "buffer queue": it's "outbuf". To me,
> they are duplicating the response buffering, thus making things
> unnecessarily more complicated.

Yeah actually this reminded me about the fact that we are still using
unlimited buffer size for the out_buf.  IMHO we should make it a
limited size after 3.0, then AFAICT if without current qmp response
queue we'll need to introduce some similar thing to cache responses
then when the out_buf is full.

IMHO the response queue looks more like the correct place that we
should do the flow control, and the out_buf could be majorly used to
do the JSON->string convertion (so basically IMHO the out_buf
limitation should be the size of the maximum JSON object that we'll
have).

> 
> If we needed, we could modify monitor_flush_locked() to add a sending
> watch only. This would have mostly the same effect as scheduling the
> bh for processing the qmp_responses queue. But since chardev sending
> is thread-safe, it's not a problem to save the watch handler switch,
> and try sending directly from the iothread.

Actually this worries me a bit on complexity - if we remove the
response queue then the response IO can be run either in main thread
or in iothread (e.g., after we add a watch).  For me, it's not really
as straightforward as we just only run IOs only in the iothread
always.  Again, we'd better do enough testings to make sure it's
always working well.  I just hope we can just avoid bothering with it.

> 
> it's not critical, so we can delay this cleanup to 3.1. (I wish it
> would have been merged early during this cycle, like most of my
> pending series, but hey, we have more important things to do ;)

Fully agree on this.

Thanks!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 05/12] monitor: no need to save need_resume

[Markus Armbruster]

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

> 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>
> ---
>  monitor.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 462cf96f7b..ec40a80d81 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;

Note for later: this comment goes away without a replacement.

>  };
>  typedef struct QMPRequest QMPRequest;
>  
> @@ -4079,11 +4073,13 @@ 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;
>      }
>  
> +    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);
> @@ -4094,7 +4090,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() */

Why not

       if (!qmp_oob_enabled(req_obj->mon)) {

and drop the variable?  Perhaps you keep the variable to have its name
provide a hint on why we resume.  But even with that hint and the "pairs
with" comment, it's still less than clear.  What about...

>          monitor_resume(req_obj->mon);
>      }
> @@ -4146,7 +4142,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);
> @@ -4159,7 +4154,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>       */
>      if (!qmp_oob_enabled(mon)) {

... adding a replacement for the deleted comment here, say

           /*
           * Emulate traditional QMP server behavior: read next command
           * only after current command completed.  Pairs with
           * monitor_resume() in monitor_qmp_bh_dispatcher().
           */

Would you then be okay with cutting out the variable?

>          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) {

Your patch works fine as far as I can tell, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

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

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

Should this leak fix go into 3.0?

> When an error occurs, "qdict" contains the response, but we still
> check the "execute" key there.

Harmless.

>                                Untangle a bit this code, by having a
> clear error path.

Untangling might make sense anyway.  Let's see.

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/main.c | 50 +++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 537cc0e162..0784761605 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -600,6 +600,7 @@ static void process_command(GAState *s, QDict *req)
>  static void process_event(JSONMessageParser *parser, GQueue *tokens)
>  {
>      GAState *s = container_of(parser, GAState, parser);
> +    QObject *obj;
>      QDict *qdict;
>      Error *err = NULL;
>      int ret;
> @@ -607,35 +608,34 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens)
>      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));
> -        }
> +    qdict = qobject_to(QDict, obj);
> +    if (!qdict) {
> +        error_setg(&err, QERR_JSON_PARSING);
> +        goto err;
> +    }
> +    if (!qdict_haskey(qdict, "execute")) {
> +        g_warning("unrecognized payload format");
> +        error_setg(&err, QERR_UNSUPPORTED);
> +        goto err;
>      }
>  
> +    process_command(s, qdict);
> +    qobject_unref(obj);
> +    return;
> +
> +err:
> +    g_warning("failed to parse event: %s", error_get_pretty(err));
> +    qdict = qmp_error_response(err);
> +    ret = send_response(s, qdict);
> +    if (ret < 0) {
> +        g_warning("error sending error response: %s", strerror(-ret));
> +    }
>      qobject_unref(qdict);
> +    qobject_unref(obj);
>  }
>  
>  /* false return signals GAChannel to close the current client connection */

Control flow is much improved.  Took me a minute to convince myself the
reference counting is okay: qdict is a weak reference before qdict =
qmp_error_response(), and becomes strong there.  Suggest to use a new
variable @err_rsp for the latter purpose.

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

Re: [Qemu-devel] [PATCH 05/12] monitor: no need to save need_resume

[Peter Xu]

On Tue, Jul 17, 2018 at 07:38:32AM +0200, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > 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>
> > ---
> >  monitor.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 462cf96f7b..ec40a80d81 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;
> 
> Note for later: this comment goes away without a replacement.
> 
> >  };
> >  typedef struct QMPRequest QMPRequest;
> >  
> > @@ -4079,11 +4073,13 @@ 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;
> >      }
> >  
> > +    need_resume = !qmp_oob_enabled(req_obj->mon);

[1]

> >      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);
> > @@ -4094,7 +4090,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() */
> 
> Why not
> 
>        if (!qmp_oob_enabled(req_obj->mon)) {
> 
> and drop the variable?  Perhaps you keep the variable to have its name
> provide a hint on why we resume.  But even with that hint and the "pairs
> with" comment, it's still less than clear.  What about...

IMO it's because the value of qmp_oob_enabled(req_obj->mon) might
change along the way, and we need to cache the old result to decide
whether we'll need to resume the monitor.  The trick is that when we
send the first "qmp_capabilities" command to enable out-of-band, then
when at [1] we will get a FALSE (since out-of-band is off by default)
while when reach here we'll get a TRUE instead (after we handled the
"qmp_capabilities" command, out-of-band is enabled).

So if we really want to remove the req_obj->need_resume variable or
anything similar, IMHO we'd better add some comment at [1] mentioning
about this trick.

Thanks,

> 
> >          monitor_resume(req_obj->mon);
> >      }
> > @@ -4146,7 +4142,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);
> > @@ -4159,7 +4154,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >       */
> >      if (!qmp_oob_enabled(mon)) {
> 
> ... adding a replacement for the deleted comment here, say
> 
>            /*
>            * Emulate traditional QMP server behavior: read next command
>            * only after current command completed.  Pairs with
>            * monitor_resume() in monitor_qmp_bh_dispatcher().
>            */
> 
> Would you then be okay with cutting out the variable?
> 
> >          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) {
> 
> Your patch works fine as far as I can tell, so
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 07/12] json-parser: always set an error if return 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.

Missing:

   * monitor.c handle_qmp_command(): drop workaround

>  * qga/main.c process_event() doesn't need further changes after
>    previous cleanup.

"Doesn't need further changes" yes, but I think it could use one.
Consider:

    obj = json_parser_parse_err(tokens, NULL, &err);
    if (err) {
        goto err;
    }
    qdict = qobject_to(QDict, obj);
    if (!qdict) {
        error_setg(&err, QERR_JSON_PARSING);
        goto err;
    }

Before this patch, we get to the error_setg() when
json_parser_parse_err() fails without setting an error, and when it
succeeds but returns anything but a dictionary.  QERR_JSON_PARSING is
appropriate for the first case, but not the second.

This patch removes the first case.  Please improve the error message.

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

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.

>  * 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 - abort, ok

To be precise, we pass &error_abort and either assert or crash when it
returns null.  Okay.  Same for other instances of "abort, ok" below.

There are a few instances that don't abort.  First one when !utf8_out:

        obj = qobject_from_json(json_in, utf8_out ? &error_abort : NULL);
        if (utf8_out) {
            str = qobject_to(QString, obj);
            g_assert(str);
            g_assert_cmpstr(qstring_get_str(str), ==, utf8_out);
        } else {
            g_assert(!obj);
        }
        qobject_unref(obj);

It ignores the error.  Okay.

Next one:

    static void unterminated_string(void)
    {
        Error *err = NULL;
        QObject *obj = qobject_from_json("\"abc", &err);
        g_assert(!err);             /* BUG */
        g_assert(obj == NULL);
    }

This patch should fix the BUG.  We'll see at [*] below why it doens't.

>     ~ tests/test-visitor-serialization.c - abort, ok
>
>   - qobject_from_jsonf()

This is called qobject_from_jsonf_nofail() since commit f3e9cc9f7a1, and
became the obvious wrapper around qobject_from_vjsonf_nofail() in commit
74670550ee0.  Please mention both new names.

I guess the commit message needs more updates for these recent changes
below, but I'm glossing over that now, along with much of the patch,
because I think I've found something more serious, explained at the end
of the patch.

>     Now dies in error_handle_fatal() instead of the assertion.

Which assertion?  Ah, the one in qobject_from_vjsonf_nofail().

The assertion is now dead, isn't it?

>     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.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  block.c                      | 5 -----
>  monitor.c                    | 4 ----
>  qapi/qobject-input-visitor.c | 5 -----
>  qobject/json-parser.c        | 8 +++++++-
>  4 files changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index ac8b3a3511..9046d66465 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 ec40a80d81..a3efe96d1d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4112,10 +4112,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QMPRequest *req_obj;
>  
>      req = json_parser_parse_err(tokens, NULL, &err);
> -    if (!req && !err) {
> -        /* json_parser_parse_err() sucks: can fail without setting @err */
> -        error_setg(&err, QERR_JSON_PARSING);
> -    }
>  
>      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/qobject/json-parser.c b/qobject/json-parser.c
> index a5aa790d62..82c3167629 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
>  {
> @@ -591,7 +592,12 @@ QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp)
   QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp)
   {
       JSONParserContext *ctxt = parser_context_new(tokens);
       QObject *result;

       if (!ctxt) {
           return NULL;

[*] Still returns null without setting an error.  Two cases lumped into
one: "no tokens due to empty input (not an error)", and "no tokens due
to lexical error".  This is not the spot to distinguish the two, it
needs to be done in its callers.  I figure the sane contract for
json_parser_parse_err() would be

* If @tokens is null, return null.
* Else if @tokens parse okay, return the parse tree.
* Else set an error and return null.

But then "always set an error if return NULL" is not possible.  Ideas?

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

Re: [Qemu-devel] [PATCH 08/12] 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.
>
> 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;
> +    }
>  }

Is this just on general principles[*], or does it enable simplifactions
later in this series?

I asked the same question for its predecessor, and you answered

    It allowed further simplification later in the series. I don't know if
    this is still necessary after the rebase and other changes [...]

Do you have an updated answer?

Re: [Qemu-devel] [PATCH 09/12] 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 ceaf4a6789..084c5edff0 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -249,7 +249,39 @@ 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' } }");

Please break lines between arguments instead of within.  More of the
same below.

> +    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 +511,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);
> @@ -488,5 +525,6 @@ int main(int argc, char *argv[])
>      ret = g_test_run();
>  
>      qmp_schema_cleanup(&schema);
> +
>      return ret;
>  }

Is this hunk intentional?

Taking a step back: the test cases look good, but is this file an
appropriate home?  The file comment states it's about "QMP protocol test
cases".  These test cases test commands, not the protocol.

I figure test_qom_set_without_value() belongs to qom-test.c.

test_object_add_without_props() could go into a memory backend test
collection, or an object-add test collection.  Sadly, neither exists.
We could have a qmp command test collection as a home of last resort.
Temptation to just throw a few random test cases there instead of
covering (a set of related) commands with a proper test case collection.

As is, your patch turns qmp-test.c into such a home of last resort.  If
that's what we want, we should update the file comment.  But I think I'd
rather have a separate qmp-cmd-test.c.

Thoughts?

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

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2018 at 7:53 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> 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.
>
> Should this leak fix go into 3.0?

It has been there for a while, but it could be fixed for 3.0 indeed.

>
>> When an error occurs, "qdict" contains the response, but we still
>> check the "execute" key there.
>
> Harmless.
>
>>                                Untangle a bit this code, by having a
>> clear error path.
>
> Untangling might make sense anyway.  Let's see.
>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  qga/main.c | 50 +++++++++++++++++++++++++-------------------------
>>  1 file changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/qga/main.c b/qga/main.c
>> index 537cc0e162..0784761605 100644
>> --- a/qga/main.c
>> +++ b/qga/main.c
>> @@ -600,6 +600,7 @@ static void process_command(GAState *s, QDict *req)
>>  static void process_event(JSONMessageParser *parser, GQueue *tokens)
>>  {
>>      GAState *s = container_of(parser, GAState, parser);
>> +    QObject *obj;
>>      QDict *qdict;
>>      Error *err = NULL;
>>      int ret;
>> @@ -607,35 +608,34 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens)
>>      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));
>> -        }
>> +    qdict = qobject_to(QDict, obj);
>> +    if (!qdict) {
>> +        error_setg(&err, QERR_JSON_PARSING);
>> +        goto err;
>> +    }
>> +    if (!qdict_haskey(qdict, "execute")) {
>> +        g_warning("unrecognized payload format");
>> +        error_setg(&err, QERR_UNSUPPORTED);
>> +        goto err;
>>      }
>>
>> +    process_command(s, qdict);
>> +    qobject_unref(obj);
>> +    return;
>> +
>> +err:
>> +    g_warning("failed to parse event: %s", error_get_pretty(err));
>> +    qdict = qmp_error_response(err);
>> +    ret = send_response(s, qdict);
>> +    if (ret < 0) {
>> +        g_warning("error sending error response: %s", strerror(-ret));
>> +    }
>>      qobject_unref(qdict);
>> +    qobject_unref(obj);
>>  }
>>
>>  /* false return signals GAChannel to close the current client connection */
>
> Control flow is much improved.  Took me a minute to convince myself the
> reference counting is okay: qdict is a weak reference before qdict =
> qmp_error_response(), and becomes strong there.  Suggest to use a new
> variable @err_rsp for the latter purpose.

Yes, the code is further improved in patch 11.

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

thanks


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 09/12] tests: add a few qemu-qmp tests

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2018 at 10:01 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 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 ceaf4a6789..084c5edff0 100644
>> --- a/tests/qmp-test.c
>> +++ b/tests/qmp-test.c
>> @@ -249,7 +249,39 @@ 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' } }");
>
> Please break lines between arguments instead of within.  More of the
> same below.

Sorry I am not sure I understand what you want. It's broken between
argument "execute" and "arguments", how do you want to change it?

>
>> +    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 +511,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);
>> @@ -488,5 +525,6 @@ int main(int argc, char *argv[])
>>      ret = g_test_run();
>>
>>      qmp_schema_cleanup(&schema);
>> +
>>      return ret;
>>  }
>
> Is this hunk intentional?
>

no

> Taking a step back: the test cases look good, but is this file an
> appropriate home?  The file comment states it's about "QMP protocol test
> cases".  These test cases test commands, not the protocol.

Actually, the tests were written to test the qapi/qmp fixes mentionned
in commit message. So they were more "protocol" related than command
related.

>
> I figure test_qom_set_without_value() belongs to qom-test.c.
>
> test_object_add_without_props() could go into a memory backend test
> collection, or an object-add test collection.  Sadly, neither exists.
> We could have a qmp command test collection as a home of last resort.
> Temptation to just throw a few random test cases there instead of
> covering (a set of related) commands with a proper test case collection.
>
> As is, your patch turns qmp-test.c into such a home of last resort.  If
> that's what we want, we should update the file comment.  But I think I'd
> rather have a separate qmp-cmd-test.c.
>
> Thoughts?

It's somewhat blurry lines to me, it would be simpler to just use qmp-test.

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

[Markus Armbruster]

Mike, I got a bug fix for you to consider for 3.0.

Marc-André, there's one remark for you inline.

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

> Hi
>
> On Tue, Jul 17, 2018 at 7:53 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>>> 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.
>>
>> Should this leak fix go into 3.0?
>
> It has been there for a while, but it could be fixed for 3.0 indeed.
>
>>
>>> When an error occurs, "qdict" contains the response, but we still
>>> check the "execute" key there.
>>
>> Harmless.
>>
>>>                                Untangle a bit this code, by having a
>>> clear error path.
>>
>> Untangling might make sense anyway.  Let's see.
>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  qga/main.c | 50 +++++++++++++++++++++++++-------------------------
>>>  1 file changed, 25 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/qga/main.c b/qga/main.c
>>> index 537cc0e162..0784761605 100644
>>> --- a/qga/main.c
>>> +++ b/qga/main.c
>>> @@ -600,6 +600,7 @@ static void process_command(GAState *s, QDict *req)
>>>  static void process_event(JSONMessageParser *parser, GQueue *tokens)
>>>  {
>>>      GAState *s = container_of(parser, GAState, parser);
>>> +    QObject *obj;
>>>      QDict *qdict;
>>>      Error *err = NULL;
>>>      int ret;
>>> @@ -607,35 +608,34 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens)
>>>      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));
>>> -        }
>>> +    qdict = qobject_to(QDict, obj);
>>> +    if (!qdict) {
>>> +        error_setg(&err, QERR_JSON_PARSING);
>>> +        goto err;
>>> +    }
>>> +    if (!qdict_haskey(qdict, "execute")) {
>>> +        g_warning("unrecognized payload format");
>>> +        error_setg(&err, QERR_UNSUPPORTED);
>>> +        goto err;
>>>      }
>>>
>>> +    process_command(s, qdict);
>>> +    qobject_unref(obj);
>>> +    return;
>>> +
>>> +err:
>>> +    g_warning("failed to parse event: %s", error_get_pretty(err));
>>> +    qdict = qmp_error_response(err);
>>> +    ret = send_response(s, qdict);
>>> +    if (ret < 0) {
>>> +        g_warning("error sending error response: %s", strerror(-ret));
>>> +    }
>>>      qobject_unref(qdict);
>>> +    qobject_unref(obj);
>>>  }
>>>
>>>  /* false return signals GAChannel to close the current client connection */
>>
>> Control flow is much improved.  Took me a minute to convince myself the
>> reference counting is okay: qdict is a weak reference before qdict =
>> qmp_error_response(), and becomes strong there.  Suggest to use a new
>> variable @err_rsp for the latter purpose.
>
> Yes, the code is further improved in patch 11.

Looking... yes, it looks quite nice after PATCH 11.  Whether to make the
intermediate state after this patch a bit nicer just because we can is
Mike's call to make.

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

Re: [Qemu-devel] [PATCH 09/12] tests: add a few qemu-qmp tests

[Markus Armbruster]

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

> Hi
>
> On Tue, Jul 17, 2018 at 10:01 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> 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 ceaf4a6789..084c5edff0 100644
>>> --- a/tests/qmp-test.c
>>> +++ b/tests/qmp-test.c
>>> @@ -249,7 +249,39 @@ 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' } }");
>>
>> Please break lines between arguments instead of within.  More of the
>> same below.
>
> Sorry I am not sure I understand what you want. It's broken between
> argument "execute" and "arguments", how do you want to change it?

The line is broken in the middle of the second argument to qtest_qmp().

Line breaks I'd like better:

    ret = qtest_qmp(qts,
                    "{'execute': 'object-add',"
                    " 'arguments': {'qom-type': 'memory-backend-ram',
                    "               'id': 'ram1'}}");

    ret = qtest_qmp(qts,
                    "{'execute': 'object-add',"
                    " 'arguments': {"
                    "    'qom-type': 'memory-backend-ram', 'id': 'ram1'}}");

    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 +511,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);
>>> @@ -488,5 +525,6 @@ int main(int argc, char *argv[])
>>>      ret = g_test_run();
>>>
>>>      qmp_schema_cleanup(&schema);
>>> +
>>>      return ret;
>>>  }
>>
>> Is this hunk intentional?
>>
>
> no

Easy enough to drop :)

>> Taking a step back: the test cases look good, but is this file an
>> appropriate home?  The file comment states it's about "QMP protocol test
>> cases".  These test cases test commands, not the protocol.
>
> Actually, the tests were written to test the qapi/qmp fixes mentionned
> in commit message. So they were more "protocol" related than command
> related.

Let's see...

1. test_object_add_without_props() tests the bug fixed in commit
   e64c75a975 "qmp: fix object-add assert() without props"

   object-add's parameter @props is optional, qmp_object_add() fails to
   default it to {}, which user_creatable_add_type() requires.  This is
   a bug in the command handler.  It went unnoticed because object-add
   lacks systematic tests.  You add one test case, which is welcome, but
   what we really want is at systematic coverage of all paths through
   qmp_object_add().

   Aside: we want that for every QMP command.  We have it for next to
   none of them.

   Anyway, neither the bug nor its test are about the QMP protocol.

2. test_qom_set_without_value() tests the bug fixed in commit c489780203
   "qapi: Fix crash when 'any' or 'null' parameter is missing"

   The QMP input visitor neglects to check qmp_input_get_object() for
   failure.  The bug is *not* in qom-set; qom-set is merely part of the
   reproducer.  You're right, this case is 'more "protocol" related than
   command related.'

   We do have tests for the QMP input visitor.  You fixed them to cover
   this case in the very next commit bce3035a44
   "tests/test-qmp-input-strict: Cover missing struct members".  That's
   a unit test.

   Should we test it once more at the QMP level in a qtest?  If yes,
   then we should test all the errors the visitor should report once
   there, shouldn't we?  Mind, I'm not saying *you* should.

>>
>> I figure test_qom_set_without_value() belongs to qom-test.c.
>>
>> test_object_add_without_props() could go into a memory backend test
>> collection, or an object-add test collection.  Sadly, neither exists.
>> We could have a qmp command test collection as a home of last resort.
>> Temptation to just throw a few random test cases there instead of
>> covering (a set of related) commands with a proper test case collection.
>>
>> As is, your patch turns qmp-test.c into such a home of last resort.  If
>> that's what we want, we should update the file comment.  But I think I'd
>> rather have a separate qmp-cmd-test.c.
>>
>> Thoughts?
>
> It's somewhat blurry lines to me, it would be simpler to just use qmp-test.

Simpler, yes, but creating a new file isn't really hard, either.

In current master, qmp-test.c has just under 500 lines.  It's already
one of the larger files in tests/.  Certainly not too big, but I'd
prefer to keep it focused.

I defocused it some myself when I added the query smoke tests (commit
e4a426e75e tests/qmp-test: Add generic, basic test of query commands).
It was simpler to just use qmp-test.  I shouldn't have.

I'll prepare a patch to split qmp-test.c into a command-independent part
(protocol, infrastructure) and a part about commands.

Re: [Qemu-devel] [PATCH 10/12] tests: add a qmp success-response test

[Markus Armbruster]

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

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

Re: [Qemu-devel] [PATCH 11/12] qga: process_event() simplification

[Markus Armbruster]

Copying the maintainer Michael Roth.

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

> 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
> - rsp/req variable renaming for clarity
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/main.c | 58 ++++++++++++++----------------------------------------
>  1 file changed, 15 insertions(+), 43 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 0784761605..b0a88ee5cf 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 *qdict;
> +    QObject *req;
> +    QDict *rsp = NULL;
>      Error *err = NULL;
>      int ret;
>  
>      g_assert(s && parser);
>  
>      g_debug("process_event: called");
> -    obj = json_parser_parse_err(tokens, NULL, &err);
> +
> +    req = json_parser_parse_err(tokens, NULL, &err);
>      if (err) {
> -        goto err;
> -    }
> -    qdict = qobject_to(QDict, obj);
> -    if (!qdict) {
> -        error_setg(&err, QERR_JSON_PARSING);
> -        goto err;
> -    }
> -    if (!qdict_haskey(qdict, "execute")) {
> -        g_warning("unrecognized payload format");
> -        error_setg(&err, QERR_UNSUPPORTED);
> -        goto err;
> +        rsp = qmp_error_response(err);
> +        goto end;
>      }
>  
> -    process_command(s, qdict);
> -    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));
> -    qdict = qmp_error_response(err);
> -    ret = send_response(s, qdict);
> +end:
> +    ret = send_response(s, rsp);
>      if (ret < 0) {
>          g_warning("error sending error response: %s", strerror(-ret));
>      }
> -    qobject_unref(qdict);
> -    qobject_unref(obj);
> +    qobject_unref(rsp);
> +    qobject_unref(req);
>  }
>  
>  /* false return signals GAChannel to close the current client connection */

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

Improvement, except the QMP part is a bit odd.

Let's mention the error message improvments in the commit message.

Re: [Qemu-devel] [PATCH 12/12] RFC: qmp: rework 'id' handling

[Markus Armbruster]

Copying the guest agent maintainer Michael Roth.

Patch needs a rebase.

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.

Before this patch, users of the common core shared by QMP and QGA have
to do extra work to conform to qmp-spec.txt.  QMP does, QGA doesn't.
I'm inclined to consider that a bug.

Judging from your description, this patch moves that work into the
shared part.  The patch is therefore not just a rework of 'id' handling,
it's a QGA bug fix, if you're so inclined, else a QGA improvement.

Thus, 1. please rephrase the commit message accordingly, and 2. the
patch needs Michael's approval.

>                                              Furthermore, it
> simplifies the work for qemu monitor.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c           | 31 ++++++++++++-------------------
>  qapi/qmp-dispatch.c | 10 ++++++++--
>  tests/test-qga.c    | 13 +++++--------
>  3 files changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index a3efe96d1d..bf192697e4 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);
>  }
>  
> @@ -4081,12 +4074,15 @@ static void monitor_qmp_bh_dispatcher(void *data)
>  
>      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);
> -        monitor_qmp_respond(req_obj->mon, rsp, NULL);
> +        req_obj->err = NULL;
> +        monitor_qmp_respond(req_obj->mon, rsp);

Error response without ID before and after the patch.  Okay.

>          qobject_unref(rsp);
>      }
>  
> @@ -4115,8 +4111,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() */

Two users of @id remain: trace_monitor_qmp_cmd_out_of_band(), visible
below, and qapi_event_send_command_dropped().  We currently plan to kill
the latter.  The former is guarded by if (qdict && qmp_is_oob(qdict)),
and could therefore safely use qdict_get(qdict, "id").  Together, this
will let us delete the whole conditional here.

>  
>      if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> @@ -4127,15 +4122,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..acea0fcfcd 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,8 +168,8 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
>                      bool allow_oob)
>  {
>      Error *err = NULL;
> -    QObject *ret;
> -    QDict *rsp;
> +    QDict *rsp, *dict = qobject_to(QDict, request);
> +    QObject *ret, *id = dict ? qdict_get(dict, "id") : NULL;

I dislike mixing initialized and uninitialized variables in the same
declaration.

>  
>      ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
>  
> @@ -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;
>  }

This puts the ID (if any) into the response.  Good.

The monitor sends command responses only with monitor_qmp_respond().
It's called on two places:

* monitor_qmp_dispatch()

  Gets its response from qmp_dispatch(), which now puts the ID (if any).

* monitor_qmp_bh_dispatcher()

  Error response without ID before and after the patch (see above).

Good.

Not visible in the patch: QGA.  It sends command responses only with
send_response().  Called only in process_event().  Two cases:

* json_parser_parse_err() sets an error

  Error response without ID before and after the patch

* qmp_dispatch()

  Now puts the ID (if any).  This is the externally visible change.

Good.

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

Re: [Qemu-devel] [PATCH 07/12] json-parser: always set an error if return NULL

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2018 at 9:06 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Let's make json_parser_parse_err() suck less, and simplify caller
>> error handling.
>
> Missing:
>
>    * monitor.c handle_qmp_command(): drop workaround
>
>>  * qga/main.c process_event() doesn't need further changes after
>>    previous cleanup.
>
> "Doesn't need further changes" yes, but I think it could use one.
> Consider:
>
>     obj = json_parser_parse_err(tokens, NULL, &err);
>     if (err) {
>         goto err;
>     }
>     qdict = qobject_to(QDict, obj);
>     if (!qdict) {
>         error_setg(&err, QERR_JSON_PARSING);
>         goto err;
>     }
>
> Before this patch, we get to the error_setg() when
> json_parser_parse_err() fails without setting an error, and when it
> succeeds but returns anything but a dictionary.  QERR_JSON_PARSING is
> appropriate for the first case, but not the second.
>
> This patch removes the first case.  Please improve the error message.
>

ok, replaced with

error_setg(&err, "Input must be a JSON object");

>>  * qobject/json-parser.c json_parser_parse()
>>    Ignores the error.
>
> 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.
>
>>  * 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 - abort, ok
>
> To be precise, we pass &error_abort and either assert or crash when it
> returns null.  Okay.  Same for other instances of "abort, ok" below.
>
> There are a few instances that don't abort.  First one when !utf8_out:
>
>         obj = qobject_from_json(json_in, utf8_out ? &error_abort : NULL);
>         if (utf8_out) {
>             str = qobject_to(QString, obj);
>             g_assert(str);
>             g_assert_cmpstr(qstring_get_str(str), ==, utf8_out);
>         } else {
>             g_assert(!obj);
>         }
>         qobject_unref(obj);
>
> It ignores the error.  Okay.
>
> Next one:
>
>     static void unterminated_string(void)
>     {
>         Error *err = NULL;
>         QObject *obj = qobject_from_json("\"abc", &err);
>         g_assert(!err);             /* BUG */
>         g_assert(obj == NULL);
>     }
>
> This patch should fix the BUG.  We'll see at [*] below why it doens't.
>
>>     ~ tests/test-visitor-serialization.c - abort, ok
>>
>>   - qobject_from_jsonf()
>
> This is called qobject_from_jsonf_nofail() since commit f3e9cc9f7a1, and
> became the obvious wrapper around qobject_from_vjsonf_nofail() in commit
> 74670550ee0.  Please mention both new names.

Hmm this is not upstream yet :)

> I guess the commit message needs more updates for these recent changes
> below, but I'm glossing over that now, along with much of the patch,
> because I think I've found something more serious, explained at the end
> of the patch.
>
>>     Now dies in error_handle_fatal() instead of the assertion.
>
> Which assertion?  Ah, the one in qobject_from_vjsonf_nofail().
>
> The assertion is now dead, isn't it?

not upstream at least

>
>>     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.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  block.c                      | 5 -----
>>  monitor.c                    | 4 ----
>>  qapi/qobject-input-visitor.c | 5 -----
>>  qobject/json-parser.c        | 8 +++++++-
>>  4 files changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index ac8b3a3511..9046d66465 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 ec40a80d81..a3efe96d1d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4112,10 +4112,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>>      QMPRequest *req_obj;
>>
>>      req = json_parser_parse_err(tokens, NULL, &err);
>> -    if (!req && !err) {
>> -        /* json_parser_parse_err() sucks: can fail without setting @err */
>> -        error_setg(&err, QERR_JSON_PARSING);
>> -    }
>>
>>      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/qobject/json-parser.c b/qobject/json-parser.c
>> index a5aa790d62..82c3167629 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
>>  {
>> @@ -591,7 +592,12 @@ QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp)
>    QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp)
>    {
>        JSONParserContext *ctxt = parser_context_new(tokens);
>        QObject *result;
>
>        if (!ctxt) {
>            return NULL;
>
> [*] Still returns null without setting an error.  Two cases lumped into
> one: "no tokens due to empty input (not an error)", and "no tokens due
> to lexical error".  This is not the spot to distinguish the two, it
> needs to be done in its callers.  I figure the sane contract for

I tried to tackle that in the next iteration, but it's harder than it
looks like somehow ;)

> json_parser_parse_err() would be
>
> * If @tokens is null, return null.
> * Else if @tokens parse okay, return the parse tree.
> * Else set an error and return null.
>
> But then "always set an error if return NULL" is not possible.  Ideas?

That's okay, I'll document the function that way


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



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 09/12] tests: add a few qemu-qmp tests

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2018 at 3:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Hi
>>
>> On Tue, Jul 17, 2018 at 10:01 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> 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 ceaf4a6789..084c5edff0 100644
>>>> --- a/tests/qmp-test.c
>>>> +++ b/tests/qmp-test.c
>>>> @@ -249,7 +249,39 @@ 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' } }");
>>>
>>> Please break lines between arguments instead of within.  More of the
>>> same below.
>>
>> Sorry I am not sure I understand what you want. It's broken between
>> argument "execute" and "arguments", how do you want to change it?
>
> The line is broken in the middle of the second argument to qtest_qmp().
>
> Line breaks I'd like better:
>
>     ret = qtest_qmp(qts,
>                     "{'execute': 'object-add',"
>                     " 'arguments': {'qom-type': 'memory-backend-ram',
>                     "               'id': 'ram1'}}");
>
>     ret = qtest_qmp(qts,
>                     "{'execute': 'object-add',"
>                     " 'arguments': {"
>                     "    'qom-type': 'memory-backend-ram', 'id': 'ram1'}}");
>
>     ret = qtest_qmp(qts,
>                     "{'execute': 'object-add', 'arguments':"
>                     " {'qom-type': 'memory-backend-ram', 'id': 'ram1'}}");
>

ok (I picked the last, they all look equally bad/good to me ;)

>>>> +    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 +511,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);
>>>> @@ -488,5 +525,6 @@ int main(int argc, char *argv[])
>>>>      ret = g_test_run();
>>>>
>>>>      qmp_schema_cleanup(&schema);
>>>> +
>>>>      return ret;
>>>>  }
>>>
>>> Is this hunk intentional?
>>>
>>
>> no
>
> Easy enough to drop :)
>
>>> Taking a step back: the test cases look good, but is this file an
>>> appropriate home?  The file comment states it's about "QMP protocol test
>>> cases".  These test cases test commands, not the protocol.
>>
>> Actually, the tests were written to test the qapi/qmp fixes mentionned
>> in commit message. So they were more "protocol" related than command
>> related.
>
> Let's see...
>
> 1. test_object_add_without_props() tests the bug fixed in commit
>    e64c75a975 "qmp: fix object-add assert() without props"
>
>    object-add's parameter @props is optional, qmp_object_add() fails to
>    default it to {}, which user_creatable_add_type() requires.  This is
>    a bug in the command handler.  It went unnoticed because object-add
>    lacks systematic tests.  You add one test case, which is welcome, but
>    what we really want is at systematic coverage of all paths through
>    qmp_object_add().
>
>    Aside: we want that for every QMP command.  We have it for next to
>    none of them.
>
>    Anyway, neither the bug nor its test are about the QMP protocol.

you are right

>
> 2. test_qom_set_without_value() tests the bug fixed in commit c489780203
>    "qapi: Fix crash when 'any' or 'null' parameter is missing"
>
>    The QMP input visitor neglects to check qmp_input_get_object() for
>    failure.  The bug is *not* in qom-set; qom-set is merely part of the
>    reproducer.  You're right, this case is 'more "protocol" related than
>    command related.'
>
>    We do have tests for the QMP input visitor.  You fixed them to cover
>    this case in the very next commit bce3035a44
>    "tests/test-qmp-input-strict: Cover missing struct members".  That's
>    a unit test.
>
>    Should we test it once more at the QMP level in a qtest?  If yes,
>    then we should test all the errors the visitor should report once
>    there, shouldn't we?  Mind, I'm not saying *you* should.
>

As you said commit bce3035a44 covers it at a lower/unit level. The QMP
test is the user level, and the original reproducer. Both have values,
even if they reach the same condition. Up to you if you prefer to drop
it.

>>>
>>> I figure test_qom_set_without_value() belongs to qom-test.c.
>>>
>>> test_object_add_without_props() could go into a memory backend test
>>> collection, or an object-add test collection.  Sadly, neither exists.
>>> We could have a qmp command test collection as a home of last resort.
>>> Temptation to just throw a few random test cases there instead of
>>> covering (a set of related) commands with a proper test case collection.
>>>
>>> As is, your patch turns qmp-test.c into such a home of last resort.  If
>>> that's what we want, we should update the file comment.  But I think I'd
>>> rather have a separate qmp-cmd-test.c.
>>>
>>> Thoughts?
>>
>> It's somewhat blurry lines to me, it would be simpler to just use qmp-test.
>
> Simpler, yes, but creating a new file isn't really hard, either.
>
> In current master, qmp-test.c has just under 500 lines.  It's already
> one of the larger files in tests/.  Certainly not too big, but I'd
> prefer to keep it focused.
>
> I defocused it some myself when I added the query smoke tests (commit
> e4a426e75e tests/qmp-test: Add generic, basic test of query commands).
> It was simpler to just use qmp-test.  I shouldn't have.
>
> I'll prepare a patch to split qmp-test.c into a command-independent part
> (protocol, infrastructure) and a part about commands.
>

Fine with me, although it could be done after this commit is applied I suppose.

I will keep it in the series for now, let you decide how to handle it

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 12/12] RFC: qmp: rework 'id' handling

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2018 at 6:05 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Copying the guest agent maintainer Michael Roth.
>
> Patch needs a rebase.
>
> 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.
>
> Before this patch, users of the common core shared by QMP and QGA have
> to do extra work to conform to qmp-spec.txt.  QMP does, QGA doesn't.
> I'm inclined to consider that a bug.
>
> Judging from your description, this patch moves that work into the
> shared part.  The patch is therefore not just a rework of 'id' handling,
> it's a QGA bug fix, if you're so inclined, else a QGA improvement.
>
> Thus, 1. please rephrase the commit message accordingly, and 2. the
> patch needs Michael's approval.
>

ok

>>                                              Furthermore, it
>> simplifies the work for qemu monitor.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  monitor.c           | 31 ++++++++++++-------------------
>>  qapi/qmp-dispatch.c | 10 ++++++++--
>>  tests/test-qga.c    | 13 +++++--------
>>  3 files changed, 25 insertions(+), 29 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index a3efe96d1d..bf192697e4 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);
>>  }
>>
>> @@ -4081,12 +4074,15 @@ static void monitor_qmp_bh_dispatcher(void *data)
>>
>>      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);
>> -        monitor_qmp_respond(req_obj->mon, rsp, NULL);
>> +        req_obj->err = NULL;
>> +        monitor_qmp_respond(req_obj->mon, rsp);
>
> Error response without ID before and after the patch.  Okay.
>
>>          qobject_unref(rsp);
>>      }
>>
>> @@ -4115,8 +4111,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() */
>
> Two users of @id remain: trace_monitor_qmp_cmd_out_of_band(), visible
> below, and qapi_event_send_command_dropped().  We currently plan to kill
> the latter.  The former is guarded by if (qdict && qmp_is_oob(qdict)),
> and could therefore safely use qdict_get(qdict, "id").  Together, this
> will let us delete the whole conditional here.
>
>>
>>      if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
>> @@ -4127,15 +4122,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..acea0fcfcd 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,8 +168,8 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
>>                      bool allow_oob)
>>  {
>>      Error *err = NULL;
>> -    QObject *ret;
>> -    QDict *rsp;
>> +    QDict *rsp, *dict = qobject_to(QDict, request);
>> +    QObject *ret, *id = dict ? qdict_get(dict, "id") : NULL;
>
> I dislike mixing initialized and uninitialized variables in the same
> declaration.

ok

>
>>
>>      ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
>>
>> @@ -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;
>>  }
>
> This puts the ID (if any) into the response.  Good.
>
> The monitor sends command responses only with monitor_qmp_respond().
> It's called on two places:
>
> * monitor_qmp_dispatch()
>
>   Gets its response from qmp_dispatch(), which now puts the ID (if any).
>
> * monitor_qmp_bh_dispatcher()
>
>   Error response without ID before and after the patch (see above).
>
> Good.
>
> Not visible in the patch: QGA.  It sends command responses only with
> send_response().  Called only in process_event().  Two cases:
>
> * json_parser_parse_err() sets an error
>
>   Error response without ID before and after the patch

It should be the same as qemu monitor (no qdict returned by json_parser_parse())

>
> * qmp_dispatch()
>
>   Now puts the ID (if any).  This is the externally visible change.
>
> Good.
>
>> 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);
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 07/12] json-parser: always set an error if return NULL

[Markus Armbruster]

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

> Hi
>
> On Tue, Jul 17, 2018 at 9:06 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>>> Let's make json_parser_parse_err() suck less, and simplify caller
>>> error handling.
>>
>> Missing:
>>
>>    * monitor.c handle_qmp_command(): drop workaround
>>
>>>  * qga/main.c process_event() doesn't need further changes after
>>>    previous cleanup.
>>
>> "Doesn't need further changes" yes, but I think it could use one.
>> Consider:
>>
>>     obj = json_parser_parse_err(tokens, NULL, &err);
>>     if (err) {
>>         goto err;
>>     }
>>     qdict = qobject_to(QDict, obj);
>>     if (!qdict) {
>>         error_setg(&err, QERR_JSON_PARSING);
>>         goto err;
>>     }
>>
>> Before this patch, we get to the error_setg() when
>> json_parser_parse_err() fails without setting an error, and when it
>> succeeds but returns anything but a dictionary.  QERR_JSON_PARSING is
>> appropriate for the first case, but not the second.
>>
>> This patch removes the first case.  Please improve the error message.
>>
>
> ok, replaced with
>
> error_setg(&err, "Input must be a JSON object");

Works for me.

>>>  * qobject/json-parser.c json_parser_parse()
>>>    Ignores the error.
>>
>> 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.
>>
>>>  * 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 - abort, ok
>>
>> To be precise, we pass &error_abort and either assert or crash when it
>> returns null.  Okay.  Same for other instances of "abort, ok" below.
>>
>> There are a few instances that don't abort.  First one when !utf8_out:
>>
>>         obj = qobject_from_json(json_in, utf8_out ? &error_abort : NULL);
>>         if (utf8_out) {
>>             str = qobject_to(QString, obj);
>>             g_assert(str);
>>             g_assert_cmpstr(qstring_get_str(str), ==, utf8_out);
>>         } else {
>>             g_assert(!obj);
>>         }
>>         qobject_unref(obj);
>>
>> It ignores the error.  Okay.
>>
>> Next one:
>>
>>     static void unterminated_string(void)
>>     {
>>         Error *err = NULL;
>>         QObject *obj = qobject_from_json("\"abc", &err);
>>         g_assert(!err);             /* BUG */
>>         g_assert(obj == NULL);
>>     }
>>
>> This patch should fix the BUG.  We'll see at [*] below why it doens't.
>>
>>>     ~ tests/test-visitor-serialization.c - abort, ok
>>>
>>>   - qobject_from_jsonf()
>>
>> This is called qobject_from_jsonf_nofail() since commit f3e9cc9f7a1, and
>> became the obvious wrapper around qobject_from_vjsonf_nofail() in commit
>> 74670550ee0.  Please mention both new names.
>
> Hmm this is not upstream yet :)

Uh, I applied your series on top of my "[PATCH 00/20] tests:
Compile-time format string checking for libqtest.h" for review, then
promptly forgot my base isn't upstream, yet.

I consider my series ready, but decided to hold onto it until 3.1 opens
up.

Hope we'll remember these semantic conflicts when we assemble our series
for 3.1.

>> I guess the commit message needs more updates for these recent changes
>> below, but I'm glossing over that now, along with much of the patch,
>> because I think I've found something more serious, explained at the end
>> of the patch.
>>
>>>     Now dies in error_handle_fatal() instead of the assertion.
>>
>> Which assertion?  Ah, the one in qobject_from_vjsonf_nofail().
>>
>> The assertion is now dead, isn't it?
>
> not upstream at least

I applied to master and tried to determine "the assertion" by squinting
at the code, no luck.  No need to help me with it, I'll simply try again
with your respin.

>>>     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.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  block.c                      | 5 -----
>>>  monitor.c                    | 4 ----
>>>  qapi/qobject-input-visitor.c | 5 -----
>>>  qobject/json-parser.c        | 8 +++++++-
>>>  4 files changed, 7 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index ac8b3a3511..9046d66465 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 ec40a80d81..a3efe96d1d 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -4112,10 +4112,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>>>      QMPRequest *req_obj;
>>>
>>>      req = json_parser_parse_err(tokens, NULL, &err);
>>> -    if (!req && !err) {
>>> -        /* json_parser_parse_err() sucks: can fail without setting @err */
>>> -        error_setg(&err, QERR_JSON_PARSING);
>>> -    }
>>>
>>>      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/qobject/json-parser.c b/qobject/json-parser.c
>>> index a5aa790d62..82c3167629 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
>>>  {
>>> @@ -591,7 +592,12 @@ QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp)
>>    QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp)
>>    {
>>        JSONParserContext *ctxt = parser_context_new(tokens);
>>        QObject *result;
>>
>>        if (!ctxt) {
>>            return NULL;
>>
>> [*] Still returns null without setting an error.  Two cases lumped into
>> one: "no tokens due to empty input (not an error)", and "no tokens due
>> to lexical error".  This is not the spot to distinguish the two, it
>> needs to be done in its callers.  I figure the sane contract for
>
> I tried to tackle that in the next iteration, but it's harder than it
> looks like somehow ;)

Doesn't surprise me.

>> json_parser_parse_err() would be
>>
>> * If @tokens is null, return null.
>> * Else if @tokens parse okay, return the parse tree.
>> * Else set an error and return null.
>>
>> But then "always set an error if return NULL" is not possible.  Ideas?
>
> That's okay, I'll document the function that way
>
>
>>
>>        }
>>>
>>>      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);
>>> +    }
>>>
>>>      parser_context_free(ctxt);
>>