[Qemu-devel] [PATCH v4 00/10] monitor: various code simplification and fixes

[Qemu-devel] [PATCH v4 00/10] 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".

v4:
- splitted "tests: add a few qmp tests"
- fix success-response crash regression in "qga: process_event() simplification"
- add r-b tags from Michael

v3:
- updated "tests: add a few qmp tests" to move tests to new files
- comment update in "qmp: common 'id' handling & make QGA conform to QMP spec"
- commit message updates, add r-b tags

Marc-André Lureau (10):
  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
  json-lexer: make it safe to call destroy multiple times
  tests: add qmp/object-add-without-props test
  tests: add qmp/qom-set-without-value test
  tests: add a qmp success-response test
  qga: process_event() simplification
  qmp: common 'id' handling & make QGA conform to QMP spec

 include/qapi/qmp/dispatch.h             |   2 +-
 monitor.c                               | 170 +++---------------------
 qapi/qmp-dispatch.c                     |  12 +-
 qga/main.c                              |  57 +++-----
 qobject/json-lexer.c                    |   5 +-
 tests/qmp-cmd-test.c                    |  31 +++++
 tests/qmp-test.c                        |  18 +++
 tests/test-qga.c                        |  13 +-
 tests/test-qmp-cmds.c                   |  17 +++
 tests/qapi-schema/qapi-schema-test.json |   2 +
 tests/qapi-schema/qapi-schema-test.out  |   2 +
 11 files changed, 125 insertions(+), 204 deletions(-)

-- 
2.19.0.rc0.48.gb9dfa238d5

[Qemu-devel] [PATCH v4 01/10] monitor: consitify qmp_send_response() QDict argument

[Marc-André Lureau]

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

diff --git a/monitor.c b/monitor.c
index 021c11b1bf..a1db4db487 100644
--- a/monitor.c
+++ b/monitor.c
@@ -503,9 +503,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.19.0.rc0.48.gb9dfa238d5

[Qemu-devel] [PATCH v4 02/10] qmp: constify qmp_is_oob()

[Marc-André Lureau]

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

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 4e2e749faf..68a528a9aa 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -50,7 +50,7 @@ bool qmp_has_success_response(const QmpCommand *cmd);
 QDict *qmp_error_response(Error *err);
 QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
                     bool allow_oob);
-bool qmp_is_oob(QDict *dict);
+bool qmp_is_oob(const QDict *dict);
 
 typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index d8da1a62de..1d922e04f7 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -155,7 +155,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.19.0.rc0.48.gb9dfa238d5

[Qemu-devel] [PATCH v4 03/10] Revert "qmp: isolate responses into io thread"

[Marc-André Lureau]

This reverts commit abe3cd0ff7f774966da6842620806ab7576fe4f3.

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

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

diff --git a/monitor.c b/monitor.c
index a1db4db487..084900c602 100644
--- a/monitor.c
+++ b/monitor.c
@@ -182,8 +182,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;
 
 /*
@@ -247,9 +245,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;
@@ -375,19 +370,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);
 }
 
@@ -518,85 +504,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 },
@@ -620,7 +527,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);
         }
     }
 }
@@ -818,7 +725,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)
@@ -833,9 +739,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,
@@ -4152,7 +4056,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);
     }
 }
 
@@ -4444,7 +4348,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;
@@ -4455,7 +4359,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,
@@ -4558,15 +4461,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)
@@ -4719,12 +4613,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) {
@@ -4738,8 +4626,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.19.0.rc0.48.gb9dfa238d5

[Qemu-devel] [PATCH v4 04/10] monitor: no need to save need_resume

[Marc-André Lureau]

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

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

diff --git a/monitor.c b/monitor.c
index 084900c602..295866d3ec 100644
--- a/monitor.c
+++ b/monitor.c
@@ -256,12 +256,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;
 
@@ -4131,11 +4125,14 @@ static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any();
     QDict *rsp;
+    bool need_resume;
 
     if (!req_obj) {
         return;
     }
 
+    /*  qmp_oob_enabled() might change after "qmp_capabilities" */
+    need_resume = !qmp_oob_enabled(req_obj->mon);
     if (req_obj->req) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
         monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
@@ -4147,7 +4144,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);
     }
@@ -4195,7 +4192,6 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
     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);
@@ -4208,7 +4204,6 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
      */
     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.19.0.rc0.48.gb9dfa238d5

[Qemu-devel] [PATCH v4 05/10] json-lexer: make it safe to call destroy 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 the qmp-async RFC
series, the patch "qmp: add QmpSession" can call qmp_session_destroy()
multiple time, which in turns calls json_lexer_destroy().

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 e1745a3d95..39969047f4 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -351,5 +351,8 @@ void 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.19.0.rc0.48.gb9dfa238d5

Re: [Qemu-devel] [PATCH v4 05/10] json-lexer: make it safe to call destroy 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 the qmp-async RFC
> series, the patch "qmp: add QmpSession" can call qmp_session_destroy()
> multiple time, which in turns calls json_lexer_destroy().
>
> 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 e1745a3d95..39969047f4 100644
> --- a/qobject/json-lexer.c
> +++ b/qobject/json-lexer.c
> @@ -351,5 +351,8 @@ void 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;
> +    }
>  }

Please stick this patch into the series its commit message refers to.

[Qemu-devel] [PATCH v4 06/10] tests: add qmp/object-add-without-props test

[Marc-André Lureau]

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

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

diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
index c5b70df974..3ba8f68476 100644
--- a/tests/qmp-cmd-test.c
+++ b/tests/qmp-cmd-test.c
@@ -19,6 +19,15 @@
 
 const char common_args[] = "-nodefaults -machine none";
 
+static const char *get_error_class(QDict *resp)
+{
+    QDict *error = qdict_get_qdict(resp, "error");
+    const char *desc = qdict_get_try_str(error, "desc");
+
+    g_assert(desc);
+    return error ? qdict_get_try_str(error, "class") : NULL;
+}
+
 /* Query smoke tests */
 
 static int query_error_class(const char *cmd)
@@ -197,6 +206,24 @@ static void add_query_tests(QmpSchema *schema)
     }
 }
 
+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);
+}
+
 int main(int argc, char *argv[])
 {
     QmpSchema schema;
@@ -206,6 +233,10 @@ int main(int argc, char *argv[])
 
     qmp_schema_init(&schema);
     add_query_tests(&schema);
+
+    qtest_add_func("qmp/object-add-without-props",
+                   test_object_add_without_props);
+
     ret = g_test_run();
 
     qmp_schema_cleanup(&schema);
-- 
2.19.0.rc0.48.gb9dfa238d5

Re: [Qemu-devel] [PATCH v4 06/10] tests: add qmp/object-add-without-props test

[Markus Armbruster]

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

> test_object_add_without_props() tests a bug in qmp_object_add() we
> fixed in commit e64c75a975.  Sadly, we don't have systematic
> object-add tests.  This lone test can go into qmp-cmd-test for want of
> a better home.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/qmp-cmd-test.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
> index c5b70df974..3ba8f68476 100644
> --- a/tests/qmp-cmd-test.c
> +++ b/tests/qmp-cmd-test.c
> @@ -19,6 +19,15 @@
>  
>  const char common_args[] = "-nodefaults -machine none";
>  
> +static const char *get_error_class(QDict *resp)
> +{
> +    QDict *error = qdict_get_qdict(resp, "error");
> +    const char *desc = qdict_get_try_str(error, "desc");
> +
> +    g_assert(desc);
> +    return error ? qdict_get_try_str(error, "class") : NULL;
> +}
> +
>  /* Query smoke tests */
>  
>  static int query_error_class(const char *cmd)

Copied from qmp-test.c.  It should be factored out instead.  Where to
put it?  libqtest.c isn't quite right, as the function could
theoretically be useful in unit tests as well, but I guess it would do
for now.

Asserting presence of "desc" makes little sense outside qmp-test.c
protocol tests, but it doesn't hurt, either.

Grep shows more possible users in tests/drive_del-test.c and
tests/test-qga.c.

> @@ -197,6 +206,24 @@ static void add_query_tests(QmpSchema *schema)
>      }
>  }
>  
> +static void test_object_add_without_props(void)
> +{
> +    QTestState *qts;
> +    QDict *ret;

qmp-test.c and qmp-cmd-test.c commonly use @resp for the response.

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

What's wrong with g_assert(!ret)?

> +
> +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> +
> +    qobject_unref(ret);

Permit me to digress.

When you expect success, you check @resp like this:

    ret = qdict_get_qdict(resp, "return");
    ... laborously check @ret against expectations ...

If you feel pedantically thorough, you can throw in

    g_assert(!qdict_haskey(resp, "error");

When you expect failure, you check like this:

    error = qdict_get_qdict(resp, "error");
    class = qdict_get_try_str(error, "class");
    g_assert_cmpstr(class, ==, "GenericError");

and perhaps

    g_assert(!qdict_haskey(resp, "return");

get_error_class() saves a little typing in the failure case.  It's still
an awfully verbose overall, and the checking is full of holes more often
than not.  There's got to be a better way.

> +    qtest_quit(qts);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      QmpSchema schema;
> @@ -206,6 +233,10 @@ int main(int argc, char *argv[])
>  
>      qmp_schema_init(&schema);
>      add_query_tests(&schema);
> +
> +    qtest_add_func("qmp/object-add-without-props",
> +                   test_object_add_without_props);
> +
>      ret = g_test_run();
>  
>      qmp_schema_cleanup(&schema);

May I have a TODO comment asking for coverage of generic object-add
failure modes?

Re: [Qemu-devel] [PATCH v4 06/10] tests: add qmp/object-add-without-props test

[Marc-André Lureau]

Hi
On Thu, Aug 30, 2018 at 3:01 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > test_object_add_without_props() tests a bug in qmp_object_add() we
> > fixed in commit e64c75a975.  Sadly, we don't have systematic
> > object-add tests.  This lone test can go into qmp-cmd-test for want of
> > a better home.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/qmp-cmd-test.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
> > index c5b70df974..3ba8f68476 100644
> > --- a/tests/qmp-cmd-test.c
> > +++ b/tests/qmp-cmd-test.c
> > @@ -19,6 +19,15 @@
> >
> >  const char common_args[] = "-nodefaults -machine none";
> >
> > +static const char *get_error_class(QDict *resp)
> > +{
> > +    QDict *error = qdict_get_qdict(resp, "error");
> > +    const char *desc = qdict_get_try_str(error, "desc");
> > +
> > +    g_assert(desc);
> > +    return error ? qdict_get_try_str(error, "class") : NULL;
> > +}
> > +
> >  /* Query smoke tests */
> >
> >  static int query_error_class(const char *cmd)
>
> Copied from qmp-test.c.  It should be factored out instead.  Where to
> put it?  libqtest.c isn't quite right, as the function could
> theoretically be useful in unit tests as well, but I guess it would do
> for now.
>
> Asserting presence of "desc" makes little sense outside qmp-test.c
> protocol tests, but it doesn't hurt, either.
>
> Grep shows more possible users in tests/drive_del-test.c and
> tests/test-qga.c.

ok

>
> > @@ -197,6 +206,24 @@ static void add_query_tests(QmpSchema *schema)
> >      }
> >  }
> >
> > +static void test_object_add_without_props(void)
> > +{
> > +    QTestState *qts;
> > +    QDict *ret;
>
> qmp-test.c and qmp-cmd-test.c commonly use @resp for the response.

ok

>
> > +
> > +    qts = qtest_init(common_args);
> > +
> > +    ret = qtest_qmp(qts,
> > +                    "{'execute': 'object-add', 'arguments':"
> > +                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1' } }");
> > +    g_assert_nonnull(ret);
>
> What's wrong with g_assert(!ret)?

nothing wrong, but g_assert_nonnull is slightly more readable, both in
code and in error produced.

>
> > +
> > +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> > +
> > +    qobject_unref(ret);
>
> Permit me to digress.
>
> When you expect success, you check @resp like this:
>
>     ret = qdict_get_qdict(resp, "return");
>     ... laborously check @ret against expectations ...
>
> If you feel pedantically thorough, you can throw in
>
>     g_assert(!qdict_haskey(resp, "error");
>
> When you expect failure, you check like this:
>
>     error = qdict_get_qdict(resp, "error");
>     class = qdict_get_try_str(error, "class");
>     g_assert_cmpstr(class, ==, "GenericError");
>
> and perhaps
>
>     g_assert(!qdict_haskey(resp, "return");
>
> get_error_class() saves a little typing in the failure case.  It's still
> an awfully verbose overall, and the checking is full of holes more often
> than not.  There's got to be a better way.
>

what about?
/**
 * qmp_assert_error_class:
 * @rsp: QMP response to check for error
 * @class: an error class
 *
 * Assert the response has the given error class and discard @rsp.
 */
void qmp_assert_error_class(QDict *rsp, const char *class)
{
    QDict *error = qdict_get_qdict(rsp, "error");

    g_assert_cmpstr(qdict_get_try_str(error, "class"), ==, class);
    g_assert_nonnull(qdict_get_try_str(error, "desc"));
    g_assert_null(qdict_get(error, "return"));

    qobject_unref(rsp);
}


> > +    qtest_quit(qts);
> > +}
> > +
> >  int main(int argc, char *argv[])
> >  {
> >      QmpSchema schema;
> > @@ -206,6 +233,10 @@ int main(int argc, char *argv[])
> >
> >      qmp_schema_init(&schema);
> >      add_query_tests(&schema);
> > +
> > +    qtest_add_func("qmp/object-add-without-props",
> > +                   test_object_add_without_props);
> > +
> >      ret = g_test_run();
> >
> >      qmp_schema_cleanup(&schema);
>
> May I have a TODO comment asking for coverage of generic object-add
> failure modes?

You mean checking for other kind of failures? ok


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v4 06/10] tests: add qmp/object-add-without-props test

[Markus Armbruster]

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

> Hi
> On Thu, Aug 30, 2018 at 3:01 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > test_object_add_without_props() tests a bug in qmp_object_add() we
>> > fixed in commit e64c75a975.  Sadly, we don't have systematic
>> > object-add tests.  This lone test can go into qmp-cmd-test for want of
>> > a better home.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  tests/qmp-cmd-test.c | 31 +++++++++++++++++++++++++++++++
>> >  1 file changed, 31 insertions(+)
>> >
>> > diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
>> > index c5b70df974..3ba8f68476 100644
>> > --- a/tests/qmp-cmd-test.c
>> > +++ b/tests/qmp-cmd-test.c
>> > @@ -19,6 +19,15 @@
>> >
>> >  const char common_args[] = "-nodefaults -machine none";
>> >
>> > +static const char *get_error_class(QDict *resp)
>> > +{
>> > +    QDict *error = qdict_get_qdict(resp, "error");
>> > +    const char *desc = qdict_get_try_str(error, "desc");
>> > +
>> > +    g_assert(desc);
>> > +    return error ? qdict_get_try_str(error, "class") : NULL;
>> > +}
>> > +
>> >  /* Query smoke tests */
>> >
>> >  static int query_error_class(const char *cmd)
>>
>> Copied from qmp-test.c.  It should be factored out instead.  Where to
>> put it?  libqtest.c isn't quite right, as the function could
>> theoretically be useful in unit tests as well, but I guess it would do
>> for now.
>>
>> Asserting presence of "desc" makes little sense outside qmp-test.c
>> protocol tests, but it doesn't hurt, either.
>>
>> Grep shows more possible users in tests/drive_del-test.c and
>> tests/test-qga.c.
>
> ok
>
>>
>> > @@ -197,6 +206,24 @@ static void add_query_tests(QmpSchema *schema)
>> >      }
>> >  }
>> >
>> > +static void test_object_add_without_props(void)
>> > +{
>> > +    QTestState *qts;
>> > +    QDict *ret;
>>
>> qmp-test.c and qmp-cmd-test.c commonly use @resp for the response.
>
> ok
>
>>
>> > +
>> > +    qts = qtest_init(common_args);
>> > +
>> > +    ret = qtest_qmp(qts,
>> > +                    "{'execute': 'object-add', 'arguments':"
>> > +                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1' } }");
>> > +    g_assert_nonnull(ret);
>>
>> What's wrong with g_assert(!ret)?
>
> nothing wrong, but g_assert_nonnull is slightly more readable, both in
> code and in error produced.

I beg to differ.  assert(!ret) is idiomatic C.

I file g_assert_nonnull() under "redundant crap GLib dumps into my
limited identifier memory just because it can", along with similar
beauties g_assert_false(), guint, gconstpointer, ...

>> > +
>> > +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
>> > +
>> > +    qobject_unref(ret);
>>
>> Permit me to digress.
>>
>> When you expect success, you check @resp like this:
>>
>>     ret = qdict_get_qdict(resp, "return");
>>     ... laborously check @ret against expectations ...
>>
>> If you feel pedantically thorough, you can throw in
>>
>>     g_assert(!qdict_haskey(resp, "error");
>>
>> When you expect failure, you check like this:
>>
>>     error = qdict_get_qdict(resp, "error");
>>     class = qdict_get_try_str(error, "class");
>>     g_assert_cmpstr(class, ==, "GenericError");
>>
>> and perhaps
>>
>>     g_assert(!qdict_haskey(resp, "return");
>>
>> get_error_class() saves a little typing in the failure case.  It's still
>> an awfully verbose overall, and the checking is full of holes more often
>> than not.  There's got to be a better way.
>>
>
> what about?
> /**
>  * qmp_assert_error_class:
>  * @rsp: QMP response to check for error
>  * @class: an error class
>  *
>  * Assert the response has the given error class and discard @rsp.
>  */
> void qmp_assert_error_class(QDict *rsp, const char *class)
> {
>     QDict *error = qdict_get_qdict(rsp, "error");
>
>     g_assert_cmpstr(qdict_get_try_str(error, "class"), ==, class);
>     g_assert_nonnull(qdict_get_try_str(error, "desc"));
>     g_assert_null(qdict_get(error, "return"));
>
>     qobject_unref(rsp);
> }

Drawback: the assertion messages no longer point to the test that broke.
Do we care?

>> > +    qtest_quit(qts);
>> > +}
>> > +
>> >  int main(int argc, char *argv[])
>> >  {
>> >      QmpSchema schema;
>> > @@ -206,6 +233,10 @@ int main(int argc, char *argv[])
>> >
>> >      qmp_schema_init(&schema);
>> >      add_query_tests(&schema);
>> > +
>> > +    qtest_add_func("qmp/object-add-without-props",
>> > +                   test_object_add_without_props);
>> > +
>> >      ret = g_test_run();
>> >
>> >      qmp_schema_cleanup(&schema);
>>
>> May I have a TODO comment asking for coverage of generic object-add
>> failure modes?
>
> You mean checking for other kind of failures? ok

Yes.  Your commit message admits "we don't have systematic object-add
tests".  The TODO comment copies that to the code.  It only asks for
negative tests, because these are the only generic ones (I think), and
object-specific tests should go elsewhere.

[Qemu-devel] [PATCH v4 07/10] tests: add qmp/qom-set-without-value test

[Marc-André Lureau]

test_qom_set_without_value() is about a bug in infrastructure used by
the QMP core, fixed in commit c489780203.  We covered the bug in
infrastructure unit tests (commit bce3035a44).  I wrote that test
earlier, to cover QMP level as well, the test could go into qmp-test.

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

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 4ae2245484..fdfe73b6d2 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
     qtest_quit(qs);
 }
 
+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);
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
@@ -355,6 +372,7 @@ int main(int argc, char *argv[])
     qtest_add_func("qmp/protocol", test_qmp_protocol);
     qtest_add_func("qmp/oob", test_qmp_oob);
     qtest_add_func("qmp/preconfig", test_qmp_preconfig);
+    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
 
     return g_test_run();
 }
-- 
2.19.0.rc0.48.gb9dfa238d5

Re: [Qemu-devel] [PATCH v4 07/10] tests: add qmp/qom-set-without-value test

[Markus Armbruster]

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

> test_qom_set_without_value() is about a bug in infrastructure used by
> the QMP core, fixed in commit c489780203.  We covered the bug in
> infrastructure unit tests (commit bce3035a44).  I wrote that test
> earlier, to cover QMP level as well, the test could go into qmp-test.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/qmp-test.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index 4ae2245484..fdfe73b6d2 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
>      qtest_quit(qs);
>  }
>  
> +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);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -355,6 +372,7 @@ int main(int argc, char *argv[])
>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>      qtest_add_func("qmp/oob", test_qmp_oob);
>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>  
>      return g_test_run();
>  }

What we're testing here is a missing 'any' parameter.  The test should
be named accordingly.  Perhaps:

       qtest_add_func("qmp/missing-any", test_missing_any);

I can't bring myself to care for this test.  There are countless ways to
run a command with invalid parameters, and this is just one.  The only
thing that sets it apart is it used to tickle a visitor bug we've long
fixed, complete with a unit test to prevent regressions.  But the cost
of carrying the test is pretty low, so if you really want it, then
taking it without further argument is probably cheaper even if it's as
redundant as I suspect it is.

Re: [Qemu-devel] [PATCH v4 07/10] tests: add qmp/qom-set-without-value test

[Marc-André Lureau]

Hi

On Thu, Aug 30, 2018 at 3:05 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> test_qom_set_without_value() is about a bug in infrastructure used by
>> the QMP core, fixed in commit c489780203.  We covered the bug in
>> infrastructure unit tests (commit bce3035a44).  I wrote that test
>> earlier, to cover QMP level as well, the test could go into qmp-test.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  tests/qmp-test.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> index 4ae2245484..fdfe73b6d2 100644
>> --- a/tests/qmp-test.c
>> +++ b/tests/qmp-test.c
>> @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
>>      qtest_quit(qs);
>>  }
>>
>> +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);
>> +}
>> +
>>  int main(int argc, char *argv[])
>>  {
>>      g_test_init(&argc, &argv, NULL);
>> @@ -355,6 +372,7 @@ int main(int argc, char *argv[])
>>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>>      qtest_add_func("qmp/oob", test_qmp_oob);
>>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>>
>>      return g_test_run();
>>  }
>
> What we're testing here is a missing 'any' parameter.  The test should
> be named accordingly.  Perhaps:
>
>        qtest_add_func("qmp/missing-any", test_missing_any);
>
> I can't bring myself to care for this test.  There are countless ways to
> run a command with invalid parameters, and this is just one.  The only
> thing that sets it apart is it used to tickle a visitor bug we've long
> fixed, complete with a unit test to prevent regressions.  But the cost
> of carrying the test is pretty low, so if you really want it, then
> taking it without further argument is probably cheaper even if it's as
> redundant as I suspect it is.

Yes, I would rather have it than drop it. It covers the QMP level as well.

Re: [Qemu-devel] [PATCH v4 07/10] tests: add qmp/qom-set-without-value test

[Marc-André Lureau]

Hi

On Thu, Aug 30, 2018 at 3:05 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > test_qom_set_without_value() is about a bug in infrastructure used by
> > the QMP core, fixed in commit c489780203.  We covered the bug in
> > infrastructure unit tests (commit bce3035a44).  I wrote that test
> > earlier, to cover QMP level as well, the test could go into qmp-test.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/qmp-test.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> > index 4ae2245484..fdfe73b6d2 100644
> > --- a/tests/qmp-test.c
> > +++ b/tests/qmp-test.c
> > @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
> >      qtest_quit(qs);
> >  }
> >
> > +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);
> > +}
> > +
> >  int main(int argc, char *argv[])
> >  {
> >      g_test_init(&argc, &argv, NULL);
> > @@ -355,6 +372,7 @@ int main(int argc, char *argv[])
> >      qtest_add_func("qmp/protocol", test_qmp_protocol);
> >      qtest_add_func("qmp/oob", test_qmp_oob);
> >      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
> > +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
> >
> >      return g_test_run();
> >  }
>
> What we're testing here is a missing 'any' parameter.  The test should
> be named accordingly.  Perhaps:
>
>        qtest_add_func("qmp/missing-any", test_missing_any);

Eh, the inner visitor fix was about an 'any' parameter missing.

But the test is more about about checking the behaviour of qom-set
without 'value' argument. I would not rename it based on the internal
bug that was fixed.

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v4 07/10] tests: add qmp/qom-set-without-value test

[Markus Armbruster]

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

> Hi
>
> On Thu, Aug 30, 2018 at 3:05 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > test_qom_set_without_value() is about a bug in infrastructure used by
>> > the QMP core, fixed in commit c489780203.  We covered the bug in
>> > infrastructure unit tests (commit bce3035a44).  I wrote that test
>> > earlier, to cover QMP level as well, the test could go into qmp-test.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  tests/qmp-test.c | 18 ++++++++++++++++++
>> >  1 file changed, 18 insertions(+)
>> >
>> > diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> > index 4ae2245484..fdfe73b6d2 100644
>> > --- a/tests/qmp-test.c
>> > +++ b/tests/qmp-test.c
>> > @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
>> >      qtest_quit(qs);
>> >  }
>> >
>> > +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);
>> > +}
>> > +
>> >  int main(int argc, char *argv[])
>> >  {
>> >      g_test_init(&argc, &argv, NULL);
>> > @@ -355,6 +372,7 @@ int main(int argc, char *argv[])
>> >      qtest_add_func("qmp/protocol", test_qmp_protocol);
>> >      qtest_add_func("qmp/oob", test_qmp_oob);
>> >      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>> > +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>> >
>> >      return g_test_run();
>> >  }
>>
>> What we're testing here is a missing 'any' parameter.  The test should
>> be named accordingly.  Perhaps:
>>
>>        qtest_add_func("qmp/missing-any", test_missing_any);
>
> Eh, the inner visitor fix was about an 'any' parameter missing.
>
> But the test is more about about checking the behaviour of qom-set
> without 'value' argument. I would not rename it based on the internal
> bug that was fixed.

Missing arguments are caught by the QObject input visitor on behalf of
the generated command marshalling code.  The command handler isn't
called then.  Thus, qom-set doesn't get to behave!  All the test really
tests is that

* the visitor behaves (redundant with test-qobject-input-visitor.c's
  test_visitor_in_fail_struct_missing()),

* the generated marshalling code handles visitor failure (I guess that's
  worth covering here),

* and the QMP core handles command failure (redundant with numerous
  other tests, but covering it here once more won't hurt).

The test could just as well any other command with a mandatory argument
of type 'any', such as qom-set or object-add.

Same for arguments of inadmissible JSON type.

Having written all this, I now think the test should be named
"qmp/invalid-arg".

[Qemu-devel] [PATCH v4 08/10] 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.19.0.rc0.48.gb9dfa238d5

Re: [Qemu-devel] [PATCH v4 08/10] 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.

Scratch "without error".

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

With the commit message corrected:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v4 09/10] qga: process_event() simplification

[Marc-André Lureau]

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

It changes a couple of error messages:

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

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

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

diff --git a/qga/main.c b/qga/main.c
index 6d70242d05..6596408cbb 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -544,15 +544,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;
     }
@@ -578,58 +578,31 @@ 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(void *opaque, QObject *obj, Error *err)
 {
     GAState *s = opaque;
-    QDict *req, *rsp;
+    QDict *rsp;
     int ret;
 
     g_debug("process_event: called");
     assert(!obj != !err);
     if (err) {
-        goto err;
-    }
-    req = qobject_to(QDict, obj);
-    if (!req) {
-        error_setg(&err, "Input must be a JSON object");
-        goto err;
-    }
-    if (!qdict_haskey(req, "execute")) {
-        g_warning("unrecognized payload format");
-        error_setg(&err, QERR_UNSUPPORTED);
-        goto err;
+        rsp = qmp_error_response(err);
+        goto end;
     }
 
-    process_command(s, req);
-    qobject_unref(obj);
-    return;
+    g_debug("processing command");
+    rsp = qmp_dispatch(&ga_commands, obj, false);
 
-err:
-    g_warning("failed to parse event: %s", error_get_pretty(err));
-    rsp = qmp_error_response(err);
-    ret = send_response(s, rsp);
-    if (ret < 0) {
-        g_warning("error sending error response: %s", strerror(-ret));
+end:
+    if (rsp) {
+        ret = send_response(s, rsp);
+        if (ret < 0) {
+            g_warning("error sending error response: %s", strerror(-ret));
+        }
+        qobject_unref(rsp);
     }
-    qobject_unref(rsp);
     qobject_unref(obj);
 }
 
-- 
2.19.0.rc0.48.gb9dfa238d5

Re: [Qemu-devel] [PATCH v4 09/10] qga: process_event() simplification

[Markus Armbruster]

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
>
> It changes a couple of error messages:
>
> * When @req isn't a dictionary, from
>     Invalid JSON syntax
>   to
>     QMP input must be a JSON object
>
> * When @req lacks member "execute", from
>     this feature or command is not currently supported
>   to
>     QMP input lacks member 'execute'
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

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

Re: [Qemu-devel] [PATCH v4 09/10] qga: process_event() simplification

[Marc-André Lureau]

Hi
On Thu, Aug 30, 2018 at 6:03 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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
> >
> > It changes a couple of error messages:
> >
> > * When @req isn't a dictionary, from
> >     Invalid JSON syntax
> >   to
> >     QMP input must be a JSON object
> >
> > * When @req lacks member "execute", from
> >     this feature or command is not currently supported
> >   to
> >     QMP input lacks member 'execute'
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>

Can someone take the patch? thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v4 09/10] qga: process_event() simplification

[Markus Armbruster]

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

> Hi
> On Thu, Aug 30, 2018 at 6:03 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> 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
>> >
>> > It changes a couple of error messages:
>> >
>> > * When @req isn't a dictionary, from
>> >     Invalid JSON syntax
>> >   to
>> >     QMP input must be a JSON object
>> >
>> > * When @req lacks member "execute", from
>> >     this feature or command is not currently supported
>> >   to
>> >     QMP input lacks member 'execute'
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>
> Can someone take the patch? thanks

Michael?

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

[Marc-André Lureau]

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

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

diff --git a/monitor.c b/monitor.c
index 295866d3ec..7126e403b0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -248,8 +248,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)
@@ -350,7 +348,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);
@@ -4043,18 +4040,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;
@@ -4079,7 +4072,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);
 }
 
@@ -4134,13 +4127,15 @@ static void monitor_qmp_bh_dispatcher(void *data)
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
     need_resume = !qmp_oob_enabled(req_obj->mon);
     if (req_obj->req) {
-        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
-        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+        QDict *qdict = qobject_to(QDict, req_obj->req);
+        QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
+        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
+        monitor_qmp_dispatch(req_obj->mon, req_obj->req);
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
         req_obj->err = NULL;
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);
+        monitor_qmp_respond(req_obj->mon, rsp);
         qobject_unref(rsp);
     }
 
@@ -4167,8 +4162,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 
     qdict = qobject_to(QDict, req);
     if (qdict) {
-        id = qobject_ref(qdict_get(qdict, "id"));
-        qdict_del(qdict, "id");
+        id = qdict_get(qdict, "id");
     } /* else will fail qmp_dispatch() */
 
     if (req && trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
@@ -4179,17 +4173,14 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 
     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);
         qobject_unref(req);
-        qobject_unref(id);
         return;
     }
 
     req_obj = g_new0(QMPRequest, 1);
     req_obj->mon = mon;
-    req_obj->id = id;
     req_obj->req = req;
     req_obj->err = err;
 
@@ -4224,7 +4215,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 
     /*
      * Put the request to the end of queue so that requests will be
-     * handled in time order.  Ownership for req_obj, req, id,
+     * handled in time order.  Ownership for req_obj, req,
      * etc. will be delivered to the handler side.
      */
     g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 1d922e04f7..5f812bb9f2 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -58,6 +58,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);
@@ -165,11 +167,11 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
                     bool allow_oob)
 {
     Error *err = NULL;
-    QObject *ret;
+    QDict *dict = qobject_to(QDict, request);
+    QObject *ret, *id = dict ? qdict_get(dict, "id") : NULL;
     QDict *rsp;
 
     ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
-
     if (err) {
         rsp = qmp_error_response(err);
     } else if (ret) {
@@ -180,5 +182,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 f69cdf6c03..e00c62f6f7 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -225,18 +225,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);
 }
@@ -997,7 +994,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.19.0.rc0.48.gb9dfa238d5

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

[Markus Armbruster]

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

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

Let's squash in

diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
index 8f7da0245d..5fdfb2a4d0 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -109,6 +109,7 @@ or
   command execution, it is optional and will be part of the response
   if provided.  The "id" member can be any json-value.  A json-number
   incremented for each successive command works fine.
+- Older versions of the QEMU Guest agent do not support "id".
 
 2.3.1 Out-of-band execution
 ---------------------------

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

[Marc-André Lureau]

On Sat, Sep 1, 2018 at 12:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Let qmp_dispatch() copy the 'id' field. That way any qmp client will
>> conform to the specification, including QGA. Furthermore, it
>> simplifies the work for qemu monitor.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> ---
>>  monitor.c           | 33 ++++++++++++---------------------
>>  qapi/qmp-dispatch.c | 10 ++++++++--
>>  tests/test-qga.c    | 13 +++++--------
>>  3 files changed, 25 insertions(+), 31 deletions(-)
>
> Let's squash in
>
> diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
> index 8f7da0245d..5fdfb2a4d0 100644
> --- a/docs/interop/qmp-spec.txt
> +++ b/docs/interop/qmp-spec.txt
> @@ -109,6 +109,7 @@ or
>    command execution, it is optional and will be part of the response
>    if provided.  The "id" member can be any json-value.  A json-number
>    incremented for each successive command works fine.
> +- Older versions of the QEMU Guest agent do not support "id".

Good idea. Are you taking and updating the patch, or Michael?

thanks

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

[Markus Armbruster]

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

> On Sat, Sep 1, 2018 at 12:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>>> Let qmp_dispatch() copy the 'id' field. That way any qmp client will
>>> conform to the specification, including QGA. Furthermore, it
>>> simplifies the work for qemu monitor.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> ---
>>>  monitor.c           | 33 ++++++++++++---------------------
>>>  qapi/qmp-dispatch.c | 10 ++++++++--
>>>  tests/test-qga.c    | 13 +++++--------
>>>  3 files changed, 25 insertions(+), 31 deletions(-)
>>
>> Let's squash in
>>
>> diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
>> index 8f7da0245d..5fdfb2a4d0 100644
>> --- a/docs/interop/qmp-spec.txt
>> +++ b/docs/interop/qmp-spec.txt
>> @@ -109,6 +109,7 @@ or
>>    command execution, it is optional and will be part of the response
>>    if provided.  The "id" member can be any json-value.  A json-number
>>    incremented for each successive command works fine.
>> +- Older versions of the QEMU Guest agent do not support "id".
>
> Good idea. Are you taking and updating the patch, or Michael?

I left the QGA stuff out of my "[PULL 0/6] Monitor patches for
2018-09-01".  If it's still pending when I do my next pull request for
QMP stuff, I can include it.  Of course, Michael is welcome to take it
earlier.

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

[Marc-André Lureau]

Hi

On Sat, Sep 1, 2018 at 4:06 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> On Sat, Sep 1, 2018 at 12:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >
> >> Let qmp_dispatch() copy the 'id' field. That way any qmp client will
> >> conform to the specification, including QGA. Furthermore, it
> >> simplifies the work for qemu monitor.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> ---
> >>  monitor.c           | 33 ++++++++++++---------------------
> >>  qapi/qmp-dispatch.c | 10 ++++++++--
> >>  tests/test-qga.c    | 13 +++++--------
> >>  3 files changed, 25 insertions(+), 31 deletions(-)
> >
> > Let's squash in
> >
> > diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
> > index 8f7da0245d..5fdfb2a4d0 100644
> > --- a/docs/interop/qmp-spec.txt
> > +++ b/docs/interop/qmp-spec.txt
> > @@ -109,6 +109,7 @@ or
> >    command execution, it is optional and will be part of the response
> >    if provided.  The "id" member can be any json-value.  A json-number
> >    incremented for each successive command works fine.
> > +- Older versions of the QEMU Guest agent do not support "id".
>
> Good idea. Are you taking and updating the patch, or Michael?
>

any of you taking the patch?

thanks

-- 
Marc-André Lureau

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

[Markus Armbruster]

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

> Hi
>
> On Sat, Sep 1, 2018 at 4:06 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> On Sat, Sep 1, 2018 at 12:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> > Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>> >
>> >> Let qmp_dispatch() copy the 'id' field. That way any qmp client will
>> >> conform to the specification, including QGA. Furthermore, it
>> >> simplifies the work for qemu monitor.
>> >>
>> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> >> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> >> ---
>> >>  monitor.c           | 33 ++++++++++++---------------------
>> >>  qapi/qmp-dispatch.c | 10 ++++++++--
>> >>  tests/test-qga.c    | 13 +++++--------
>> >>  3 files changed, 25 insertions(+), 31 deletions(-)
>> >
>> > Let's squash in
>> >
>> > diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
>> > index 8f7da0245d..5fdfb2a4d0 100644
>> > --- a/docs/interop/qmp-spec.txt
>> > +++ b/docs/interop/qmp-spec.txt
>> > @@ -109,6 +109,7 @@ or
>> >    command execution, it is optional and will be part of the response
>> >    if provided.  The "id" member can be any json-value.  A json-number
>> >    incremented for each successive command works fine.
>> > +- Older versions of the QEMU Guest agent do not support "id".
>>
>> Good idea. Are you taking and updating the patch, or Michael?
>>
>
> any of you taking the patch?

Quoting myself (Message-ID: <871sabf999.fsf@dusky.pond.sub.org>):

    I left the QGA stuff out of my "[PULL 0/6] Monitor patches for
    2018-09-01".  If it's still pending when I do my next pull request for
    QMP stuff, I can include it.  Of course, Michael is welcome to take it
    earlier.

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

[Markus Armbruster]

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

> Hi,
>
> This series is a rebased subset of "[PATCH v3 00/38] RFC: monitor: add
> asynchronous command type".

PATCH 01-04 are core monitor work Peter will need to make progress.
Queued.
http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/monitor-next

PATCH 05 I'd like you to keep in your qmp-async series.

PATCH 06-07 could use a bit of work.

PATCH 08-10 are related to QGA.  Michael, would you like me to handle
them?

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

[Michael Roth]

Quoting Markus Armbruster (2018-08-30 09:21:11)
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > Hi,
> >
> > This series is a rebased subset of "[PATCH v3 00/38] RFC: monitor: add
> > asynchronous command type".
> 
> PATCH 01-04 are core monitor work Peter will need to make progress.
> Queued.
> http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/monitor-next
> 
> PATCH 05 I'd like you to keep in your qmp-async series.
> 
> PATCH 06-07 could use a bit of work.
> 
> PATCH 08-10 are related to QGA.  Michael, would you like me to handle
> them?

That's fine by me, can take them through my tree too if you'd prefer,
just let me know.