[Qemu-devel] [PATCH v3 0/9] monitor: various code simplification and fixes

[Qemu-devel] [PATCH v3 0/9] 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".

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 (9):
  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 a few qmp tests
  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                              |  47 ++-----
 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, 119 insertions(+), 200 deletions(-)

-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH v3 1/9] 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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH v3 2/9] 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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH v3 3/9] 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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH v3 4/9] 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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH v3 5/9] 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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH v3 6/9] tests: add a few qmp tests

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

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-cmd-test.c | 31 +++++++++++++++++++++++++++++++
 tests/qmp-test.c     | 18 ++++++++++++++++++
 2 files changed, 49 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);
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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH v3 7/9] 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.547.g1d89318c48

[Qemu-devel] [PATCH v3 8/9] 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'

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

diff --git a/qga/main.c b/qga/main.c
index 6d70242d05..f0ec035996 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,53 +578,24 @@ 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);
+end:
     ret = send_response(s, rsp);
     if (ret < 0) {
         g_warning("error sending error response: %s", strerror(-ret));
-- 
2.18.0.547.g1d89318c48

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

[Marc-André Lureau]

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

CC: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.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.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Peter Xu]

On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote:
> 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>

Note that this series/patch still conflict with the "enable
out-of-band by default" series.

  [PATCH v6 00/13] monitor: enable OOB by default

I'm not against this patch to be merged since it has its r-b, but I
feel like we'd better judge on whether we still like the response
queue first, in case one day we'll need to add these things back.

When there could be functional changes around the code path I would
think we'd better keep the cleanup patches postponed a bit until those
functional changes are settled.  For now the functional part is decide
how to fix up the rest of out-of-band issues (my proposal is in the
series above which should solve everything that is related to
out-of-band to be fixed; if there is more, I'll continue to work on
it), whether we should enable it by default for 3.1 (my answer
is... yes...), and what to do with it.

If we found that it's too hard to enable it by default, I'm thinking
whether we can make it a persistent flag for monitor (maybe turning
the "x-oob" into a real "oob" and keep it, then we don't turn it on by
default), then we can let libvirt start working with out-of-band with
the flag.  After all it's actually working mostly (the pending issues
are only things like flow control for malicious/buggy clients, but
libvirt never had such an issue with it).

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 1/9] monitor: consitify qmp_send_response() QDict argument

[Thomas Huth]

On 2018-08-25 15:57, Marc-André Lureau wrote:
> 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) :
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/9] qmp: constify qmp_is_oob()

[Thomas Huth]

On 2018-08-25 15:57, Marc-André Lureau wrote:
> 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");
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 6/9] tests: add a few qmp tests

[Thomas Huth]

On 2018-08-25 15:57, Marc-André Lureau wrote:
> 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.
> 
> 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-cmd-test.c | 31 +++++++++++++++++++++++++++++++
>  tests/qmp-test.c     | 18 ++++++++++++++++++
>  2 files changed, 49 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 8/9] qga: process_event() simplification

[Michael Roth]

Quoting Marc-André Lureau (2018-08-25 08:57:23)
> 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'
> 
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/main.c | 47 +++++++++--------------------------------------
>  1 file changed, 9 insertions(+), 38 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 6d70242d05..f0ec035996 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,53 +578,24 @@ 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);
> -    }

We used to check here for the success-response=false scenario (e.g.
guest-shutdown qga command). Now we pass the result of qmp_dispatch()
directly to send_response(), which looks like that might cause an
assertion now. Do we need to add handling for this?

> -}
> -
>  /* 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);
> +end:
>      ret = send_response(s, rsp);
>      if (ret < 0) {
>          g_warning("error sending error response: %s", strerror(-ret));
> -- 
> 2.18.0.547.g1d89318c48
> 

Re: [Qemu-devel] [PATCH v3 8/9] qga: process_event() simplification

[Marc-André Lureau]

Hi
On Tue, Aug 28, 2018 at 2:05 AM Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>
> Quoting Marc-André Lureau (2018-08-25 08:57:23)
> > 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'
> >
> > CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  qga/main.c | 47 +++++++++--------------------------------------
> >  1 file changed, 9 insertions(+), 38 deletions(-)
> >
> > diff --git a/qga/main.c b/qga/main.c
> > index 6d70242d05..f0ec035996 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,53 +578,24 @@ 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);
> > -    }
>
> We used to check here for the success-response=false scenario (e.g.
> guest-shutdown qga command). Now we pass the result of qmp_dispatch()
> directly to send_response(), which looks like that might cause an
> assertion now. Do we need to add handling for this?

Good catch! fixing it:

end:
    if (rsp) {
        ret = send_response(s, rsp);
        if (ret < 0) {
            g_warning("error sending error response: %s", strerror(-ret));
        }
        qobject_unref(rsp);
    }

ack, with that?

> > -}
> > -
> >  /* 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);
> > +end:
> >      ret = send_response(s, rsp);
> >      if (ret < 0) {
> >          g_warning("error sending error response: %s", strerror(-ret));
> > --
> > 2.18.0.547.g1d89318c48
> >
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote:
>> 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>
>
> Note that this series/patch still conflict with the "enable
> out-of-band by default" series.
>
>   [PATCH v6 00/13] monitor: enable OOB by default

Yes.

> I'm not against this patch to be merged since it has its r-b, but I
> feel like we'd better judge on whether we still like the response
> queue first, in case one day we'll need to add these things back.

Let's not worry about things that may or may not happen at some
indeterminate time in the future.

However:

> When there could be functional changes around the code path I would
> think we'd better keep the cleanup patches postponed a bit until those
> functional changes are settled.  For now the functional part is decide
> how to fix up the rest of out-of-band issues (my proposal is in the
> series above which should solve everything that is related to
> out-of-band to be fixed; if there is more, I'll continue to work on
> it), whether we should enable it by default for 3.1 (my answer
> is... yes...), and what to do with it.

I agree the important job is to finish OOB.

Sometimes, it's better to clean up first.  Sometimes, it's not.

Right now, the response queue is a useless complication, and
Marc-André's PATCH 3+4 get rid of it.  Lovely.  I understand this
conflicts with your OOB work.  The question is whether your work
fundamentally needs the response queue or not.

If your OOB work happens to be coded for the response queue, but the
problem could also be solved without the response queue, then the OOB
job doesn't fundamentally need the response queue.

Unless that's the case, getting rid of the response queue is unnecessary
churn.

If it is the case, we still need to consider effort.  Which order is
less total work?  Which order gets us to the goal faster?

Can you guys agree on answers to these questions, or do you need me to
answer them?

Restating the questions:

1. Can you think of a way to do what Peter's OOB series does, but
without the response queue?

2. If you can, what's easier / cheaper / faster:

   a. Merge Marc-André's patches to get rid of response queue, rewrite
      OOB series without response queue on top.

   b. Merge Peter's OOB series with response queue, rewrite patches to
      get rid of response queue on top.

> If we found that it's too hard to enable it by default, I'm thinking
> whether we can make it a persistent flag for monitor (maybe turning
> the "x-oob" into a real "oob" and keep it, then we don't turn it on by
> default), then we can let libvirt start working with out-of-band with
> the flag.  After all it's actually working mostly (the pending issues
> are only things like flow control for malicious/buggy clients, but
> libvirt never had such an issue with it).

The OOB job isn't complete without working flow control.  Nevertheless,
I'm willing to consider enabling OOB without working flow control.

Re: [Qemu-devel] [PATCH v3 0/9] 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 1-2 queued.  Thanks!

Re: [Qemu-devel] [PATCH v3 8/9] qga: process_event() simplification

[Michael Roth]

Quoting Marc-André Lureau (2018-08-28 06:56:51)
> Hi
> On Tue, Aug 28, 2018 at 2:05 AM Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> >
> > Quoting Marc-André Lureau (2018-08-25 08:57:23)
> > > 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'
> > >
> > > CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  qga/main.c | 47 +++++++++--------------------------------------
> > >  1 file changed, 9 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/qga/main.c b/qga/main.c
> > > index 6d70242d05..f0ec035996 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,53 +578,24 @@ 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);
> > > -    }
> >
> > We used to check here for the success-response=false scenario (e.g.
> > guest-shutdown qga command). Now we pass the result of qmp_dispatch()
> > directly to send_response(), which looks like that might cause an
> > assertion now. Do we need to add handling for this?
> 
> Good catch! fixing it:
> 
> end:
>     if (rsp) {
>         ret = send_response(s, rsp);
>         if (ret < 0) {
>             g_warning("error sending error response: %s", strerror(-ret));
>         }
>         qobject_unref(rsp);
>     }
> 
> ack, with that?

Looks good! With that:

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> 
> > > -}
> > > -
> > >  /* 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);
> > > +end:
> > >      ret = send_response(s, rsp);
> > >      if (ret < 0) {
> > >          g_warning("error sending error response: %s", strerror(-ret));
> > > --
> > > 2.18.0.547.g1d89318c48
> > >
> >
> 
> 
> -- 
> Marc-André Lureau
> 

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

[Michael Roth]

Quoting Marc-André Lureau (2018-08-25 08:57:24)
> Let qmp_dispatch() copy the 'id' field. That way any qmp client will
> conform to the specification, including QGA. Furthermore, it
> simplifies the work for qemu monitor.
> 
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 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.18.0.547.g1d89318c48
> 

Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Peter Xu]

On Tue, Aug 28, 2018 at 05:46:29PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote:
> >> 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>
> >
> > Note that this series/patch still conflict with the "enable
> > out-of-band by default" series.
> >
> >   [PATCH v6 00/13] monitor: enable OOB by default
> 
> Yes.
> 
> > I'm not against this patch to be merged since it has its r-b, but I
> > feel like we'd better judge on whether we still like the response
> > queue first, in case one day we'll need to add these things back.
> 
> Let's not worry about things that may or may not happen at some
> indeterminate time in the future.

It might not be that "far future"...  Please see below.

> 
> However:
> 
> > When there could be functional changes around the code path I would
> > think we'd better keep the cleanup patches postponed a bit until those
> > functional changes are settled.  For now the functional part is decide
> > how to fix up the rest of out-of-band issues (my proposal is in the
> > series above which should solve everything that is related to
> > out-of-band to be fixed; if there is more, I'll continue to work on
> > it), whether we should enable it by default for 3.1 (my answer
> > is... yes...), and what to do with it.
> 
> I agree the important job is to finish OOB.
> 
> Sometimes, it's better to clean up first.  Sometimes, it's not.
> 
> Right now, the response queue is a useless complication, and
> Marc-André's PATCH 3+4 get rid of it.  Lovely.  I understand this
> conflicts with your OOB work.  The question is whether your work
> fundamentally needs the response queue or not.

Just to clarify a bit... I prefer to keep the response queue not
because it's conflicting my existing work but because I think we might
get use of it even in the near future.  I stated it here on the
possibility that we might use the response queue to solve the
unlimited monitor out_buf issue here:

https://patchwork.kernel.org/patch/10511471/#22110771

Quotes:

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

Let's imagine what we need if we want to limit the out_buf: (1) To
limit the out_buf, we need somewhere to cache the responses that we
want to put into out_buf but we can't when the out_buf is full -
that's mostly the response queue now.  (2) Since we will need to queue
these items onto out_buf when out_buf is not full, we'll possibly need
something like a bottom half to kickoff when out_buf is able to handle
more data - that's mostly the bottom half of the response queue.

AFAIU the rest to do is very possible only that we set a limit to the
out_buf but only if response queue is there...

I didn't really work on the out_buf since I didn't want to further
expand the out-of-band work (since it's already going far away before
it settles down first...), and after all the out_buf issue is nothing
related to the out-of-band work and it's there for a long time.
However that's the major reason that I might prefer to keep the queue
now.

[1]

> 
> If your OOB work happens to be coded for the response queue, but the
> problem could also be solved without the response queue, then the OOB
> job doesn't fundamentally need the response queue.

Yes, I think the OOB work itself does not need the response queue now.

> 
> Unless that's the case, getting rid of the response queue is unnecessary
> churn.
> 
> If it is the case, we still need to consider effort.  Which order is
> less total work?  Which order gets us to the goal faster?
> 
> Can you guys agree on answers to these questions, or do you need me to
> answer them?
> 
> Restating the questions:
> 
> 1. Can you think of a way to do what Peter's OOB series does, but
> without the response queue?
> 
> 2. If you can, what's easier / cheaper / faster:
> 
>    a. Merge Marc-André's patches to get rid of response queue, rewrite
>       OOB series without response queue on top.
> 
>    b. Merge Peter's OOB series with response queue, rewrite patches to
>       get rid of response queue on top.

Let's have a quick look at above [1], if it's not a good reason (or
even it's still unclear) then let's drop the queue.  It'll be
perfectly fine I rebase my work upon Marc-André's.  After all the only
reason to keep the response queue for me is to save time (for anyone
who will be working with monitors).  If we spend too much time on
judging whether we should keep the queue (we've already spent some)
then it's already a waste of time...  It does not worth it IMO.

> 
> > If we found that it's too hard to enable it by default, I'm thinking
> > whether we can make it a persistent flag for monitor (maybe turning
> > the "x-oob" into a real "oob" and keep it, then we don't turn it on by
> > default), then we can let libvirt start working with out-of-band with
> > the flag.  After all it's actually working mostly (the pending issues
> > are only things like flow control for malicious/buggy clients, but
> > libvirt never had such an issue with it).
> 
> The OOB job isn't complete without working flow control.  Nevertheless,
> I'm willing to consider enabling OOB without working flow control.

That'll be great.  Thanks!

(Though I think the current OOB series should have addressed all but
 the out_buf flow control issue, right?)

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Aug 28, 2018 at 05:46:29PM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote:
>> >> 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>
>> >
>> > Note that this series/patch still conflict with the "enable
>> > out-of-band by default" series.
>> >
>> >   [PATCH v6 00/13] monitor: enable OOB by default
>> 
>> Yes.
>> 
>> > I'm not against this patch to be merged since it has its r-b, but I
>> > feel like we'd better judge on whether we still like the response
>> > queue first, in case one day we'll need to add these things back.
>> 
>> Let's not worry about things that may or may not happen at some
>> indeterminate time in the future.
>
> It might not be that "far future"...  Please see below.
>
>> 
>> However:
>> 
>> > When there could be functional changes around the code path I would
>> > think we'd better keep the cleanup patches postponed a bit until those
>> > functional changes are settled.  For now the functional part is decide
>> > how to fix up the rest of out-of-band issues (my proposal is in the
>> > series above which should solve everything that is related to
>> > out-of-band to be fixed; if there is more, I'll continue to work on
>> > it), whether we should enable it by default for 3.1 (my answer
>> > is... yes...), and what to do with it.
>> 
>> I agree the important job is to finish OOB.
>> 
>> Sometimes, it's better to clean up first.  Sometimes, it's not.
>> 
>> Right now, the response queue is a useless complication, and
>> Marc-André's PATCH 3+4 get rid of it.  Lovely.  I understand this
>> conflicts with your OOB work.  The question is whether your work
>> fundamentally needs the response queue or not.
>
> Just to clarify a bit... I prefer to keep the response queue not
> because it's conflicting my existing work but because I think we might
> get use of it even in the near future.  I stated it here on the
> possibility that we might use the response queue to solve the
> unlimited monitor out_buf issue here:
>
> https://patchwork.kernel.org/patch/10511471/#22110771
>
> Quotes:
>
>         ...
>         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).
>         ...
>
> Let's imagine what we need if we want to limit the out_buf: (1) To
> limit the out_buf, we need somewhere to cache the responses that we
> want to put into out_buf but we can't when the out_buf is full -
> that's mostly the response queue now.  (2) Since we will need to queue
> these items onto out_buf when out_buf is not full, we'll possibly need
> something like a bottom half to kickoff when out_buf is able to handle
> more data - that's mostly the bottom half of the response queue.
>
> AFAIU the rest to do is very possible only that we set a limit to the
> out_buf but only if response queue is there...

Limiting outbuf only to have the same data pile up earlier in the
pipeline doesn't seem helpful.  We need to throttle production of
output.  A simple way to do that is throttling input.  See below.

> I didn't really work on the out_buf since I didn't want to further
> expand the out-of-band work (since it's already going far away before
> it settles down first...), and after all the out_buf issue is nothing
> related to the out-of-band work and it's there for a long time.
> However that's the major reason that I might prefer to keep the queue
> now.
>
> [1]

Let's review what we have.

QMP flow control is about limiting the amount of data QEMU buffers on
behalf of a QMP client.

This is about robustness, not security.  There are countless ways a QMP
client can make QEMU use more memory.  We want to cope with accidents,
not stop attacks.

A common and simple way to do flow control is to throttle receiving.
Unless the transport buffers way too much (which would be insane), the
peer will soon notice, and can adapt.

QMP input flows from the character device to commands.  It is converted
from text to QObject along the way.  Output flows from commands to the
character device.  It is converted from QObject to text along the way.

When the client sends input faster than QEMU can execute them, flow
control should kick in to stop adding more input to the pileup.  When
QEMU produces output faster than the client can receive it, flow control
should kick in to stop adding more output to the pileup.  We can do that
indirectly, by stopping input.

Input buffering:

* The chardev buffers 4KiB (I think).  Small enough to be ignored.

* The JSON parser buffers one partial JSON value as a token list, up to
  a limit in the order of 64MiB.  Weasel words "in the order", because
  it measures memory consumption only indirectly.  The limit is
  ridicilously generous for a control plane purpose like QMP.

* When the partial JSON value becomes complete, the JSON parser converts
  it to a QObject, then frees the token list.

* Without OOB, the QMP core buffers one complete QObject (the command)
  until we're done with the command.  The JSON parser's buffer should be
  empty then, because the QMP core suspends reading from the char device
  while it deals with a command.

* With OOB, the QMP core buffers up to 8 in-band commands and one
  out-of-band command.

  When the in-band command buffer is full, we currently drop further
  in-band commands, but that's a bad idea, and we're going to suspend
  reading from the char device instead.  Once we do, the JSON parser's
  buffer should be empty when the in-band command buffer is full.  The
  remainder of my analysis assumes we suspend rather than drop.

Output buffering:

* Traditionally, the QMP core buffers one QObject while it converts it
  to text.  It buffers an unlimited amount of text in mon->outbuf.

* Adding a request queue doesn't by itself change how much data is
  buffered, only when it's converted from QObject to text.  If the
  bottom half doing the conversion runs quickly, nothing changes.  If it
  gets delayed for some reason, QObjects can pile up in the response
  queue before they get converted and moved to mon->outbuf.

Summary of flow control:

* We limit input buffers and stop reading input when the limit is
  exceeded.  This stops input piling up.

* We do not limit output at all.

Ideally, we'd keep track of combined input and output buffer size, and
throttle input to keep it under control.  But separate limits for
individual buffers could be good enough, and might be simpler.

>> If your OOB work happens to be coded for the response queue, but the
>> problem could also be solved without the response queue, then the OOB
>> job doesn't fundamentally need the response queue.
>
> Yes, I think the OOB work itself does not need the response queue now.

Understood.

>> Unless that's the case, getting rid of the response queue is unnecessary
>> churn.
>> 
>> If it is the case, we still need to consider effort.  Which order is
>> less total work?  Which order gets us to the goal faster?
>> 
>> Can you guys agree on answers to these questions, or do you need me to
>> answer them?
>> 
>> Restating the questions:
>> 
>> 1. Can you think of a way to do what Peter's OOB series does, but
>> without the response queue?
>> 
>> 2. If you can, what's easier / cheaper / faster:
>> 
>>    a. Merge Marc-André's patches to get rid of response queue, rewrite
>>       OOB series without response queue on top.
>> 
>>    b. Merge Peter's OOB series with response queue, rewrite patches to
>>       get rid of response queue on top.
>
> Let's have a quick look at above [1], if it's not a good reason (or
> even it's still unclear) then let's drop the queue.  It'll be
> perfectly fine I rebase my work upon Marc-André's.  After all the only
> reason to keep the response queue for me is to save time (for anyone
> who will be working with monitors).  If we spend too much time on
> judging whether we should keep the queue (we've already spent some)
> then it's already a waste of time...  It does not worth it IMO.

Time spent on coming up with a high-level plan for flow control is time
well spent.

Sometimes, you have to explore and experiment before you can come up
with a high-level plan that has a reasonable chance of working.  No
license to skip the "think before you hack" step entirely.

Here's the simplest (and possibly naive) plan I can think of: if
mon->outbuf exceeds a limit, suspend reading input.  No response queue.
Would it have a reasonable chance of working?

>> > If we found that it's too hard to enable it by default, I'm thinking
>> > whether we can make it a persistent flag for monitor (maybe turning
>> > the "x-oob" into a real "oob" and keep it, then we don't turn it on by
>> > default), then we can let libvirt start working with out-of-band with
>> > the flag.  After all it's actually working mostly (the pending issues
>> > are only things like flow control for malicious/buggy clients, but
>> > libvirt never had such an issue with it).
>> 
>> The OOB job isn't complete without working flow control.  Nevertheless,
>> I'm willing to consider enabling OOB without working flow control.
>
> That'll be great.  Thanks!
>
> (Though I think the current OOB series should have addressed all but
>  the out_buf flow control issue, right?)

I hope so, but I haven't been able to review it, yet :)

Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Peter Xu]

On Wed, Aug 29, 2018 at 10:55:51AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Aug 28, 2018 at 05:46:29PM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote:
> >> >> 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>
> >> >
> >> > Note that this series/patch still conflict with the "enable
> >> > out-of-band by default" series.
> >> >
> >> >   [PATCH v6 00/13] monitor: enable OOB by default
> >> 
> >> Yes.
> >> 
> >> > I'm not against this patch to be merged since it has its r-b, but I
> >> > feel like we'd better judge on whether we still like the response
> >> > queue first, in case one day we'll need to add these things back.
> >> 
> >> Let's not worry about things that may or may not happen at some
> >> indeterminate time in the future.
> >
> > It might not be that "far future"...  Please see below.
> >
> >> 
> >> However:
> >> 
> >> > When there could be functional changes around the code path I would
> >> > think we'd better keep the cleanup patches postponed a bit until those
> >> > functional changes are settled.  For now the functional part is decide
> >> > how to fix up the rest of out-of-band issues (my proposal is in the
> >> > series above which should solve everything that is related to
> >> > out-of-band to be fixed; if there is more, I'll continue to work on
> >> > it), whether we should enable it by default for 3.1 (my answer
> >> > is... yes...), and what to do with it.
> >> 
> >> I agree the important job is to finish OOB.
> >> 
> >> Sometimes, it's better to clean up first.  Sometimes, it's not.
> >> 
> >> Right now, the response queue is a useless complication, and
> >> Marc-André's PATCH 3+4 get rid of it.  Lovely.  I understand this
> >> conflicts with your OOB work.  The question is whether your work
> >> fundamentally needs the response queue or not.
> >
> > Just to clarify a bit... I prefer to keep the response queue not
> > because it's conflicting my existing work but because I think we might
> > get use of it even in the near future.  I stated it here on the
> > possibility that we might use the response queue to solve the
> > unlimited monitor out_buf issue here:
> >
> > https://patchwork.kernel.org/patch/10511471/#22110771
> >
> > Quotes:
> >
> >         ...
> >         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).
> >         ...
> >
> > Let's imagine what we need if we want to limit the out_buf: (1) To
> > limit the out_buf, we need somewhere to cache the responses that we
> > want to put into out_buf but we can't when the out_buf is full -
> > that's mostly the response queue now.  (2) Since we will need to queue
> > these items onto out_buf when out_buf is not full, we'll possibly need
> > something like a bottom half to kickoff when out_buf is able to handle
> > more data - that's mostly the bottom half of the response queue.
> >
> > AFAIU the rest to do is very possible only that we set a limit to the
> > out_buf but only if response queue is there...
> 
> Limiting outbuf only to have the same data pile up earlier in the
> pipeline doesn't seem helpful.  We need to throttle production of
> output.  A simple way to do that is throttling input.  See below.
> 
> > I didn't really work on the out_buf since I didn't want to further
> > expand the out-of-band work (since it's already going far away before
> > it settles down first...), and after all the out_buf issue is nothing
> > related to the out-of-band work and it's there for a long time.
> > However that's the major reason that I might prefer to keep the queue
> > now.
> >
> > [1]
> 
> Let's review what we have.
> 
> QMP flow control is about limiting the amount of data QEMU buffers on
> behalf of a QMP client.
> 
> This is about robustness, not security.  There are countless ways a QMP
> client can make QEMU use more memory.  We want to cope with accidents,
> not stop attacks.
> 
> A common and simple way to do flow control is to throttle receiving.
> Unless the transport buffers way too much (which would be insane), the
> peer will soon notice, and can adapt.
> 
> QMP input flows from the character device to commands.  It is converted
> from text to QObject along the way.  Output flows from commands to the
> character device.  It is converted from QObject to text along the way.
> 
> When the client sends input faster than QEMU can execute them, flow
> control should kick in to stop adding more input to the pileup.  When
> QEMU produces output faster than the client can receive it, flow control
> should kick in to stop adding more output to the pileup.  We can do that
> indirectly, by stopping input.
> 
> Input buffering:
> 
> * The chardev buffers 4KiB (I think).  Small enough to be ignored.
> 
> * The JSON parser buffers one partial JSON value as a token list, up to
>   a limit in the order of 64MiB.  Weasel words "in the order", because
>   it measures memory consumption only indirectly.  The limit is
>   ridicilously generous for a control plane purpose like QMP.
> 
> * When the partial JSON value becomes complete, the JSON parser converts
>   it to a QObject, then frees the token list.
> 
> * Without OOB, the QMP core buffers one complete QObject (the command)
>   until we're done with the command.  The JSON parser's buffer should be
>   empty then, because the QMP core suspends reading from the char device
>   while it deals with a command.
> 
> * With OOB, the QMP core buffers up to 8 in-band commands and one
>   out-of-band command.
> 
>   When the in-band command buffer is full, we currently drop further
>   in-band commands, but that's a bad idea, and we're going to suspend
>   reading from the char device instead.  Once we do, the JSON parser's
>   buffer should be empty when the in-band command buffer is full.  The
>   remainder of my analysis assumes we suspend rather than drop.
> 
> Output buffering:
> 
> * Traditionally, the QMP core buffers one QObject while it converts it
>   to text.  It buffers an unlimited amount of text in mon->outbuf.
> 
> * Adding a request queue doesn't by itself change how much data is
>   buffered, only when it's converted from QObject to text.  If the
>   bottom half doing the conversion runs quickly, nothing changes.  If it
>   gets delayed for some reason, QObjects can pile up in the response
>   queue before they get converted and moved to mon->outbuf.
> 
> Summary of flow control:
> 
> * We limit input buffers and stop reading input when the limit is
>   exceeded.  This stops input piling up.
> 
> * We do not limit output at all.
> 
> Ideally, we'd keep track of combined input and output buffer size, and
> throttle input to keep it under control.  But separate limits for
> individual buffers could be good enough, and might be simpler.

(thanks for the summary)

> 
> >> If your OOB work happens to be coded for the response queue, but the
> >> problem could also be solved without the response queue, then the OOB
> >> job doesn't fundamentally need the response queue.
> >
> > Yes, I think the OOB work itself does not need the response queue now.
> 
> Understood.
> 
> >> Unless that's the case, getting rid of the response queue is unnecessary
> >> churn.
> >> 
> >> If it is the case, we still need to consider effort.  Which order is
> >> less total work?  Which order gets us to the goal faster?
> >> 
> >> Can you guys agree on answers to these questions, or do you need me to
> >> answer them?
> >> 
> >> Restating the questions:
> >> 
> >> 1. Can you think of a way to do what Peter's OOB series does, but
> >> without the response queue?
> >> 
> >> 2. If you can, what's easier / cheaper / faster:
> >> 
> >>    a. Merge Marc-André's patches to get rid of response queue, rewrite
> >>       OOB series without response queue on top.
> >> 
> >>    b. Merge Peter's OOB series with response queue, rewrite patches to
> >>       get rid of response queue on top.
> >
> > Let's have a quick look at above [1], if it's not a good reason (or
> > even it's still unclear) then let's drop the queue.  It'll be
> > perfectly fine I rebase my work upon Marc-André's.  After all the only
> > reason to keep the response queue for me is to save time (for anyone
> > who will be working with monitors).  If we spend too much time on
> > judging whether we should keep the queue (we've already spent some)
> > then it's already a waste of time...  It does not worth it IMO.
> 
> Time spent on coming up with a high-level plan for flow control is time
> well spent.
> 
> Sometimes, you have to explore and experiment before you can come up
> with a high-level plan that has a reasonable chance of working.  No
> license to skip the "think before you hack" step entirely.
> 
> Here's the simplest (and possibly naive) plan I can think of: if
> mon->outbuf exceeds a limit, suspend reading input.  No response queue.
> Would it have a reasonable chance of working?

Hmm I think it works...  Let's assume:

- M: threshold size for outbuf
- A: size of current outbuf
- B: size of a new response message (and assume A+B>M, so the flow
     control will trigger)

I think that queue is not a must if we don't restrict the buffer that
much - for example we can just queue the JSON object into outbuf when
we receive the new message with size B (after queuing, we might get
A+B>M, then it's slightly bigger than the limit threshold), now we
suspend the monitor.

If we want to have a very strict buffer size limitation for outbuf so
the outbuf never use more than M, we can't just queue it since it will
overflow, then we need to stop the input and cache the object
somewhere (e.g., the response queue).

So I think now I agree with you that the response queue is not
required if we think the first solution is okay for us.

Thanks,

> 
> >> > If we found that it's too hard to enable it by default, I'm thinking
> >> > whether we can make it a persistent flag for monitor (maybe turning
> >> > the "x-oob" into a real "oob" and keep it, then we don't turn it on by
> >> > default), then we can let libvirt start working with out-of-band with
> >> > the flag.  After all it's actually working mostly (the pending issues
> >> > are only things like flow control for malicious/buggy clients, but
> >> > libvirt never had such an issue with it).
> >> 
> >> The OOB job isn't complete without working flow control.  Nevertheless,
> >> I'm willing to consider enabling OOB without working flow control.
> >
> > That'll be great.  Thanks!
> >
> > (Though I think the current OOB series should have addressed all but
> >  the out_buf flow control issue, right?)
> 
> I hope so, but I haven't been able to review it, yet :)

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Aug 29, 2018 at 10:55:51AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Tue, Aug 28, 2018 at 05:46:29PM +0200, Markus Armbruster wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote:
>> >> >> 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>
>> >> >
>> >> > Note that this series/patch still conflict with the "enable
>> >> > out-of-band by default" series.
>> >> >
>> >> >   [PATCH v6 00/13] monitor: enable OOB by default
>> >> 
>> >> Yes.
>> >> 
>> >> > I'm not against this patch to be merged since it has its r-b, but I
>> >> > feel like we'd better judge on whether we still like the response
>> >> > queue first, in case one day we'll need to add these things back.
>> >> 
>> >> Let's not worry about things that may or may not happen at some
>> >> indeterminate time in the future.
>> >
>> > It might not be that "far future"...  Please see below.
>> >
>> >> 
>> >> However:
>> >> 
>> >> > When there could be functional changes around the code path I would
>> >> > think we'd better keep the cleanup patches postponed a bit until those
>> >> > functional changes are settled.  For now the functional part is decide
>> >> > how to fix up the rest of out-of-band issues (my proposal is in the
>> >> > series above which should solve everything that is related to
>> >> > out-of-band to be fixed; if there is more, I'll continue to work on
>> >> > it), whether we should enable it by default for 3.1 (my answer
>> >> > is... yes...), and what to do with it.
>> >> 
>> >> I agree the important job is to finish OOB.
>> >> 
>> >> Sometimes, it's better to clean up first.  Sometimes, it's not.
>> >> 
>> >> Right now, the response queue is a useless complication, and
>> >> Marc-André's PATCH 3+4 get rid of it.  Lovely.  I understand this
>> >> conflicts with your OOB work.  The question is whether your work
>> >> fundamentally needs the response queue or not.
>> >
>> > Just to clarify a bit... I prefer to keep the response queue not
>> > because it's conflicting my existing work but because I think we might
>> > get use of it even in the near future.  I stated it here on the
>> > possibility that we might use the response queue to solve the
>> > unlimited monitor out_buf issue here:
>> >
>> > https://patchwork.kernel.org/patch/10511471/#22110771
>> >
>> > Quotes:
>> >
>> >         ...
>> >         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).
>> >         ...
>> >
>> > Let's imagine what we need if we want to limit the out_buf: (1) To
>> > limit the out_buf, we need somewhere to cache the responses that we
>> > want to put into out_buf but we can't when the out_buf is full -
>> > that's mostly the response queue now.  (2) Since we will need to queue
>> > these items onto out_buf when out_buf is not full, we'll possibly need
>> > something like a bottom half to kickoff when out_buf is able to handle
>> > more data - that's mostly the bottom half of the response queue.
>> >
>> > AFAIU the rest to do is very possible only that we set a limit to the
>> > out_buf but only if response queue is there...
>> 
>> Limiting outbuf only to have the same data pile up earlier in the
>> pipeline doesn't seem helpful.  We need to throttle production of
>> output.  A simple way to do that is throttling input.  See below.
>> 
>> > I didn't really work on the out_buf since I didn't want to further
>> > expand the out-of-band work (since it's already going far away before
>> > it settles down first...), and after all the out_buf issue is nothing
>> > related to the out-of-band work and it's there for a long time.
>> > However that's the major reason that I might prefer to keep the queue
>> > now.
>> >
>> > [1]
>> 
>> Let's review what we have.
>> 
>> QMP flow control is about limiting the amount of data QEMU buffers on
>> behalf of a QMP client.
>> 
>> This is about robustness, not security.  There are countless ways a QMP
>> client can make QEMU use more memory.  We want to cope with accidents,
>> not stop attacks.
>> 
>> A common and simple way to do flow control is to throttle receiving.
>> Unless the transport buffers way too much (which would be insane), the
>> peer will soon notice, and can adapt.
>> 
>> QMP input flows from the character device to commands.  It is converted
>> from text to QObject along the way.  Output flows from commands to the
>> character device.  It is converted from QObject to text along the way.
>> 
>> When the client sends input faster than QEMU can execute them, flow
>> control should kick in to stop adding more input to the pileup.  When
>> QEMU produces output faster than the client can receive it, flow control
>> should kick in to stop adding more output to the pileup.  We can do that
>> indirectly, by stopping input.
>> 
>> Input buffering:
>> 
>> * The chardev buffers 4KiB (I think).  Small enough to be ignored.
>> 
>> * The JSON parser buffers one partial JSON value as a token list, up to
>>   a limit in the order of 64MiB.  Weasel words "in the order", because
>>   it measures memory consumption only indirectly.  The limit is
>>   ridicilously generous for a control plane purpose like QMP.
>> 
>> * When the partial JSON value becomes complete, the JSON parser converts
>>   it to a QObject, then frees the token list.
>> 
>> * Without OOB, the QMP core buffers one complete QObject (the command)
>>   until we're done with the command.  The JSON parser's buffer should be
>>   empty then, because the QMP core suspends reading from the char device
>>   while it deals with a command.
>> 
>> * With OOB, the QMP core buffers up to 8 in-band commands and one
>>   out-of-band command.
>> 
>>   When the in-band command buffer is full, we currently drop further
>>   in-band commands, but that's a bad idea, and we're going to suspend
>>   reading from the char device instead.  Once we do, the JSON parser's
>>   buffer should be empty when the in-band command buffer is full.  The
>>   remainder of my analysis assumes we suspend rather than drop.
>> 
>> Output buffering:
>> 
>> * Traditionally, the QMP core buffers one QObject while it converts it
>>   to text.  It buffers an unlimited amount of text in mon->outbuf.
>> 
>> * Adding a request queue doesn't by itself change how much data is
>>   buffered, only when it's converted from QObject to text.  If the
>>   bottom half doing the conversion runs quickly, nothing changes.  If it
>>   gets delayed for some reason, QObjects can pile up in the response
>>   queue before they get converted and moved to mon->outbuf.
>> 
>> Summary of flow control:
>> 
>> * We limit input buffers and stop reading input when the limit is
>>   exceeded.  This stops input piling up.
>> 
>> * We do not limit output at all.
>> 
>> Ideally, we'd keep track of combined input and output buffer size, and
>> throttle input to keep it under control.  But separate limits for
>> individual buffers could be good enough, and might be simpler.
>
> (thanks for the summary)
>
>> 
>> >> If your OOB work happens to be coded for the response queue, but the
>> >> problem could also be solved without the response queue, then the OOB
>> >> job doesn't fundamentally need the response queue.
>> >
>> > Yes, I think the OOB work itself does not need the response queue now.
>> 
>> Understood.
>> 
>> >> Unless that's the case, getting rid of the response queue is unnecessary
>> >> churn.
>> >> 
>> >> If it is the case, we still need to consider effort.  Which order is
>> >> less total work?  Which order gets us to the goal faster?
>> >> 
>> >> Can you guys agree on answers to these questions, or do you need me to
>> >> answer them?
>> >> 
>> >> Restating the questions:
>> >> 
>> >> 1. Can you think of a way to do what Peter's OOB series does, but
>> >> without the response queue?
>> >> 
>> >> 2. If you can, what's easier / cheaper / faster:
>> >> 
>> >>    a. Merge Marc-André's patches to get rid of response queue, rewrite
>> >>       OOB series without response queue on top.
>> >> 
>> >>    b. Merge Peter's OOB series with response queue, rewrite patches to
>> >>       get rid of response queue on top.
>> >
>> > Let's have a quick look at above [1], if it's not a good reason (or
>> > even it's still unclear) then let's drop the queue.  It'll be
>> > perfectly fine I rebase my work upon Marc-André's.  After all the only
>> > reason to keep the response queue for me is to save time (for anyone
>> > who will be working with monitors).  If we spend too much time on
>> > judging whether we should keep the queue (we've already spent some)
>> > then it's already a waste of time...  It does not worth it IMO.
>> 
>> Time spent on coming up with a high-level plan for flow control is time
>> well spent.
>> 
>> Sometimes, you have to explore and experiment before you can come up
>> with a high-level plan that has a reasonable chance of working.  No
>> license to skip the "think before you hack" step entirely.
>> 
>> Here's the simplest (and possibly naive) plan I can think of: if
>> mon->outbuf exceeds a limit, suspend reading input.  No response queue.
>> Would it have a reasonable chance of working?
>
> Hmm I think it works...  Let's assume:
>
> - M: threshold size for outbuf
> - A: size of current outbuf
> - B: size of a new response message (and assume A+B>M, so the flow
>      control will trigger)
>
> I think that queue is not a must if we don't restrict the buffer that
> much - for example we can just queue the JSON object into outbuf when
> we receive the new message with size B (after queuing, we might get
> A+B>M, then it's slightly bigger than the limit threshold), now we
> suspend the monitor.

Yes.

M is a threshold for suspending the monitor, not a hard size limit.

Since the response QObjects has to go *somewhere*, we can just as well
stuff it into mon->outbuf.

The actual mon-outbuf size A may exceed the threshold M, but only while
the monitor is suspended.

> If we want to have a very strict buffer size limitation for outbuf so
> the outbuf never use more than M, we can't just queue it since it will
> overflow, then we need to stop the input and cache the object
> somewhere (e.g., the response queue).

Yes, but that doesn't actually limit memory use: instead of mon->outbuf
growing without bound, we now have response queue growing without bound.
Actual memory use changes only if one of the two representations QObject
and text is more compact.

Unscientific experiment: I instrumented qobject_to_json() to measure
size of its QObject input and text output (patch appended).  For
query-qmp-schema's output, I measure ~9.8MiB for QObject, and ~119KiB
for text:

    ### qobject_to_json(0x556653e7ac30)
    ###    #objs   #bytes QType
    ###        0        0 none
    ###      568        0 qnull
    ###        0        0 qnum
    ###    11876   540253 qstring
    ###     2301  9677496 qdict
    ###      509    86344 qlist
    ###        2       48 qbool
    ### 0x556653e7ac30 uses 10304141 vs. 121759

Most of the QObject's memory is occupied by QDicts.  The way they're
implemented is wasteful (known issue).  But even with a magical QDict
implementation that uses no memory at all, the QObject would still use
five times the memory of text.

My experiment suggests that to conserve memory, we should convert
responses to text right away.

> So I think now I agree with you that the response queue is not
> required if we think the first solution is okay for us.

Cool.  Can you explore that and post patches?

>
> Thanks,
>
>> 
>> >> > If we found that it's too hard to enable it by default, I'm thinking
>> >> > whether we can make it a persistent flag for monitor (maybe turning
>> >> > the "x-oob" into a real "oob" and keep it, then we don't turn it on by
>> >> > default), then we can let libvirt start working with out-of-band with
>> >> > the flag.  After all it's actually working mostly (the pending issues
>> >> > are only things like flow control for malicious/buggy clients, but
>> >> > libvirt never had such an issue with it).
>> >> 
>> >> The OOB job isn't complete without working flow control.  Nevertheless,
>> >> I'm willing to consider enabling OOB without working flow control.
>> >
>> > That'll be great.  Thanks!
>> >
>> > (Though I think the current OOB series should have addressed all but
>> >  the out_buf flow control issue, right?)
>> 
>> I hope so, but I haven't been able to review it, yet :)
>
> Regards,


diff --git a/qobject/qjson.c b/qobject/qjson.c
index db36101f3b..92fa87330e 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -203,6 +203,9 @@ static void to_json_list_iter(QObject *obj, void *opaque)
     s->count++;
 }
 
+static size_t sz[QTYPE__MAX];
+static int hist[QTYPE__MAX];
+
 static void to_json(const QObject *obj, QString *str, int pretty, int indent)
 {
     switch (qobject_type(obj)) {
@@ -211,6 +214,7 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
         break;
     case QTYPE_QNUM: {
         QNum *val = qobject_to(QNum, obj);
+        sz[QTYPE_QNUM] += sizeof(*val);
         char *buffer = qnum_to_string(val);
         qstring_append(str, buffer);
         g_free(buffer);
@@ -218,6 +222,7 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
     }
     case QTYPE_QSTRING: {
         QString *val = qobject_to(QString, obj);
+        sz[QTYPE_QSTRING] += sizeof(*val) + val->capacity;
         const char *ptr;
         int cp;
         char buf[16];
@@ -275,6 +280,7 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
     case QTYPE_QDICT: {
         ToJsonIterState s;
         QDict *val = qobject_to(QDict, obj);
+        sz[QTYPE_QDICT] += sizeof(*val) + val->size * sizeof(QDictEntry);
 
         s.count = 0;
         s.str = str;
@@ -294,6 +300,7 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
     case QTYPE_QLIST: {
         ToJsonIterState s;
         QList *val = qobject_to(QList, obj);
+        sz[QTYPE_QLIST] += sizeof(*val) + qlist_size(val) * sizeof(QListEntry);
 
         s.count = 0;
         s.str = str;
@@ -312,6 +319,7 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
     }
     case QTYPE_QBOOL: {
         QBool *val = qobject_to(QBool, obj);
+        sz[QTYPE_QBOOL] += sizeof(*val);
 
         if (qbool_get_bool(val)) {
             qstring_append(str, "true");
@@ -323,13 +331,25 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
     default:
         abort();
     }
+    hist[qobject_type(obj)]++;
 }
 
 QString *qobject_to_json(const QObject *obj)
 {
     QString *str = qstring_new();
 
+    memset(&sz, 0, sizeof(sz));
+    memset(&hist, 0, sizeof(hist));
+    printf("### qobject_to_json(%p)\n", obj);
+    printf("### %8s %8s QType\n", "#objs", "#bytes");
     to_json(obj, str, 0, 0);
+    size_t totsz = 0;
+    for (int i = 0; i < QTYPE__MAX; i++) {
+        printf("### %8d %8zd %s\n", hist[i], sz[i], QType_str(i));
+        totsz += sz[i];
+    }
+    printf("### %p uses %zd vs. %zd\n", obj, totsz, strlen(str->string));
+    printf("### %.60s\n", str->string);
 
     return str;
 }

Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Marc-André Lureau]

Hi

On Wed, Aug 29, 2018 at 2:51 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> > So I think now I agree with you that the response queue is not
> > required if we think the first solution is okay for us.
>
> Cool.  Can you explore that and post patches?
>

As we agree to drop the response queue, I am sending rebased v4, with
qga fixes and r-b from Michael, and tests splitted.

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Peter Xu]

On Wed, Aug 29, 2018 at 02:40:39PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Aug 29, 2018 at 10:55:51AM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Tue, Aug 28, 2018 at 05:46:29PM +0200, Markus Armbruster wrote:
> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >> 
> >> >> > On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote:
> >> >> >> 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>
> >> >> >
> >> >> > Note that this series/patch still conflict with the "enable
> >> >> > out-of-band by default" series.
> >> >> >
> >> >> >   [PATCH v6 00/13] monitor: enable OOB by default
> >> >> 
> >> >> Yes.
> >> >> 
> >> >> > I'm not against this patch to be merged since it has its r-b, but I
> >> >> > feel like we'd better judge on whether we still like the response
> >> >> > queue first, in case one day we'll need to add these things back.
> >> >> 
> >> >> Let's not worry about things that may or may not happen at some
> >> >> indeterminate time in the future.
> >> >
> >> > It might not be that "far future"...  Please see below.
> >> >
> >> >> 
> >> >> However:
> >> >> 
> >> >> > When there could be functional changes around the code path I would
> >> >> > think we'd better keep the cleanup patches postponed a bit until those
> >> >> > functional changes are settled.  For now the functional part is decide
> >> >> > how to fix up the rest of out-of-band issues (my proposal is in the
> >> >> > series above which should solve everything that is related to
> >> >> > out-of-band to be fixed; if there is more, I'll continue to work on
> >> >> > it), whether we should enable it by default for 3.1 (my answer
> >> >> > is... yes...), and what to do with it.
> >> >> 
> >> >> I agree the important job is to finish OOB.
> >> >> 
> >> >> Sometimes, it's better to clean up first.  Sometimes, it's not.
> >> >> 
> >> >> Right now, the response queue is a useless complication, and
> >> >> Marc-André's PATCH 3+4 get rid of it.  Lovely.  I understand this
> >> >> conflicts with your OOB work.  The question is whether your work
> >> >> fundamentally needs the response queue or not.
> >> >
> >> > Just to clarify a bit... I prefer to keep the response queue not
> >> > because it's conflicting my existing work but because I think we might
> >> > get use of it even in the near future.  I stated it here on the
> >> > possibility that we might use the response queue to solve the
> >> > unlimited monitor out_buf issue here:
> >> >
> >> > https://patchwork.kernel.org/patch/10511471/#22110771
> >> >
> >> > Quotes:
> >> >
> >> >         ...
> >> >         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).
> >> >         ...
> >> >
> >> > Let's imagine what we need if we want to limit the out_buf: (1) To
> >> > limit the out_buf, we need somewhere to cache the responses that we
> >> > want to put into out_buf but we can't when the out_buf is full -
> >> > that's mostly the response queue now.  (2) Since we will need to queue
> >> > these items onto out_buf when out_buf is not full, we'll possibly need
> >> > something like a bottom half to kickoff when out_buf is able to handle
> >> > more data - that's mostly the bottom half of the response queue.
> >> >
> >> > AFAIU the rest to do is very possible only that we set a limit to the
> >> > out_buf but only if response queue is there...
> >> 
> >> Limiting outbuf only to have the same data pile up earlier in the
> >> pipeline doesn't seem helpful.  We need to throttle production of
> >> output.  A simple way to do that is throttling input.  See below.
> >> 
> >> > I didn't really work on the out_buf since I didn't want to further
> >> > expand the out-of-band work (since it's already going far away before
> >> > it settles down first...), and after all the out_buf issue is nothing
> >> > related to the out-of-band work and it's there for a long time.
> >> > However that's the major reason that I might prefer to keep the queue
> >> > now.
> >> >
> >> > [1]
> >> 
> >> Let's review what we have.
> >> 
> >> QMP flow control is about limiting the amount of data QEMU buffers on
> >> behalf of a QMP client.
> >> 
> >> This is about robustness, not security.  There are countless ways a QMP
> >> client can make QEMU use more memory.  We want to cope with accidents,
> >> not stop attacks.
> >> 
> >> A common and simple way to do flow control is to throttle receiving.
> >> Unless the transport buffers way too much (which would be insane), the
> >> peer will soon notice, and can adapt.
> >> 
> >> QMP input flows from the character device to commands.  It is converted
> >> from text to QObject along the way.  Output flows from commands to the
> >> character device.  It is converted from QObject to text along the way.
> >> 
> >> When the client sends input faster than QEMU can execute them, flow
> >> control should kick in to stop adding more input to the pileup.  When
> >> QEMU produces output faster than the client can receive it, flow control
> >> should kick in to stop adding more output to the pileup.  We can do that
> >> indirectly, by stopping input.
> >> 
> >> Input buffering:
> >> 
> >> * The chardev buffers 4KiB (I think).  Small enough to be ignored.
> >> 
> >> * The JSON parser buffers one partial JSON value as a token list, up to
> >>   a limit in the order of 64MiB.  Weasel words "in the order", because
> >>   it measures memory consumption only indirectly.  The limit is
> >>   ridicilously generous for a control plane purpose like QMP.
> >> 
> >> * When the partial JSON value becomes complete, the JSON parser converts
> >>   it to a QObject, then frees the token list.
> >> 
> >> * Without OOB, the QMP core buffers one complete QObject (the command)
> >>   until we're done with the command.  The JSON parser's buffer should be
> >>   empty then, because the QMP core suspends reading from the char device
> >>   while it deals with a command.
> >> 
> >> * With OOB, the QMP core buffers up to 8 in-band commands and one
> >>   out-of-band command.
> >> 
> >>   When the in-band command buffer is full, we currently drop further
> >>   in-band commands, but that's a bad idea, and we're going to suspend
> >>   reading from the char device instead.  Once we do, the JSON parser's
> >>   buffer should be empty when the in-band command buffer is full.  The
> >>   remainder of my analysis assumes we suspend rather than drop.
> >> 
> >> Output buffering:
> >> 
> >> * Traditionally, the QMP core buffers one QObject while it converts it
> >>   to text.  It buffers an unlimited amount of text in mon->outbuf.
> >> 
> >> * Adding a request queue doesn't by itself change how much data is
> >>   buffered, only when it's converted from QObject to text.  If the
> >>   bottom half doing the conversion runs quickly, nothing changes.  If it
> >>   gets delayed for some reason, QObjects can pile up in the response
> >>   queue before they get converted and moved to mon->outbuf.
> >> 
> >> Summary of flow control:
> >> 
> >> * We limit input buffers and stop reading input when the limit is
> >>   exceeded.  This stops input piling up.
> >> 
> >> * We do not limit output at all.
> >> 
> >> Ideally, we'd keep track of combined input and output buffer size, and
> >> throttle input to keep it under control.  But separate limits for
> >> individual buffers could be good enough, and might be simpler.
> >
> > (thanks for the summary)
> >
> >> 
> >> >> If your OOB work happens to be coded for the response queue, but the
> >> >> problem could also be solved without the response queue, then the OOB
> >> >> job doesn't fundamentally need the response queue.
> >> >
> >> > Yes, I think the OOB work itself does not need the response queue now.
> >> 
> >> Understood.
> >> 
> >> >> Unless that's the case, getting rid of the response queue is unnecessary
> >> >> churn.
> >> >> 
> >> >> If it is the case, we still need to consider effort.  Which order is
> >> >> less total work?  Which order gets us to the goal faster?
> >> >> 
> >> >> Can you guys agree on answers to these questions, or do you need me to
> >> >> answer them?
> >> >> 
> >> >> Restating the questions:
> >> >> 
> >> >> 1. Can you think of a way to do what Peter's OOB series does, but
> >> >> without the response queue?
> >> >> 
> >> >> 2. If you can, what's easier / cheaper / faster:
> >> >> 
> >> >>    a. Merge Marc-André's patches to get rid of response queue, rewrite
> >> >>       OOB series without response queue on top.
> >> >> 
> >> >>    b. Merge Peter's OOB series with response queue, rewrite patches to
> >> >>       get rid of response queue on top.
> >> >
> >> > Let's have a quick look at above [1], if it's not a good reason (or
> >> > even it's still unclear) then let's drop the queue.  It'll be
> >> > perfectly fine I rebase my work upon Marc-André's.  After all the only
> >> > reason to keep the response queue for me is to save time (for anyone
> >> > who will be working with monitors).  If we spend too much time on
> >> > judging whether we should keep the queue (we've already spent some)
> >> > then it's already a waste of time...  It does not worth it IMO.
> >> 
> >> Time spent on coming up with a high-level plan for flow control is time
> >> well spent.
> >> 
> >> Sometimes, you have to explore and experiment before you can come up
> >> with a high-level plan that has a reasonable chance of working.  No
> >> license to skip the "think before you hack" step entirely.
> >> 
> >> Here's the simplest (and possibly naive) plan I can think of: if
> >> mon->outbuf exceeds a limit, suspend reading input.  No response queue.
> >> Would it have a reasonable chance of working?
> >
> > Hmm I think it works...  Let's assume:
> >
> > - M: threshold size for outbuf
> > - A: size of current outbuf
> > - B: size of a new response message (and assume A+B>M, so the flow
> >      control will trigger)
> >
> > I think that queue is not a must if we don't restrict the buffer that
> > much - for example we can just queue the JSON object into outbuf when
> > we receive the new message with size B (after queuing, we might get
> > A+B>M, then it's slightly bigger than the limit threshold), now we
> > suspend the monitor.
> 
> Yes.
> 
> M is a threshold for suspending the monitor, not a hard size limit.
> 
> Since the response QObjects has to go *somewhere*, we can just as well
> stuff it into mon->outbuf.
> 
> The actual mon-outbuf size A may exceed the threshold M, but only while
> the monitor is suspended.
> 
> > If we want to have a very strict buffer size limitation for outbuf so
> > the outbuf never use more than M, we can't just queue it since it will
> > overflow, then we need to stop the input and cache the object
> > somewhere (e.g., the response queue).
> 
> Yes, but that doesn't actually limit memory use: instead of mon->outbuf
> growing without bound, we now have response queue growing without bound.
> Actual memory use changes only if one of the two representations QObject
> and text is more compact.
> 
> Unscientific experiment: I instrumented qobject_to_json() to measure
> size of its QObject input and text output (patch appended).  For
> query-qmp-schema's output, I measure ~9.8MiB for QObject, and ~119KiB
> for text:
> 
>     ### qobject_to_json(0x556653e7ac30)
>     ###    #objs   #bytes QType
>     ###        0        0 none
>     ###      568        0 qnull
>     ###        0        0 qnum
>     ###    11876   540253 qstring
>     ###     2301  9677496 qdict
>     ###      509    86344 qlist
>     ###        2       48 qbool
>     ### 0x556653e7ac30 uses 10304141 vs. 121759
> 
> Most of the QObject's memory is occupied by QDicts.  The way they're
> implemented is wasteful (known issue).  But even with a magical QDict
> implementation that uses no memory at all, the QObject would still use
> five times the memory of text.
> 
> My experiment suggests that to conserve memory, we should convert
> responses to text right away.

>From memory saving POV, yes.  But keeping them as objects bring us
flexibility and isolation, e.g., we can simply drop one event as we
want (rather than dropping the whole outbuf), or suddenly we want to
insert a new key due to some reason.  But these requirements have no
real usage so far.  So I admit it's possibly me that have thought too
much and I decided to not struggle. :)

(I never consider perf for QMP yet... so actually spending more memory
 is not a problem to me; however possibility to use up the memory is
 of course another thing...)

Now I only hope we won't have other bug triggered due to start using
the multi-threading of output buffer after we remove the queue.

> 
> > So I think now I agree with you that the response queue is not
> > required if we think the first solution is okay for us.
> 
> Cool.  Can you explore that and post patches?

I think what I can do here is rebasing my work after Marc-Andre's
patches merged.  Please let me know if there's anything else I can do.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Aug 29, 2018 at 02:40:39PM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Wed, Aug 29, 2018 at 10:55:51AM +0200, Markus Armbruster wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > On Tue, Aug 28, 2018 at 05:46:29PM +0200, Markus Armbruster wrote:
>> >> >> Peter Xu <peterx@redhat.com> writes:
>> >> >> 
>> >> >> > On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote:
>> >> >> >> 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>
>> >> >> >
>> >> >> > Note that this series/patch still conflict with the "enable
>> >> >> > out-of-band by default" series.
>> >> >> >
>> >> >> >   [PATCH v6 00/13] monitor: enable OOB by default
>> >> >> 
>> >> >> Yes.
>> >> >> 
>> >> >> > I'm not against this patch to be merged since it has its r-b, but I
>> >> >> > feel like we'd better judge on whether we still like the response
>> >> >> > queue first, in case one day we'll need to add these things back.
>> >> >> 
>> >> >> Let's not worry about things that may or may not happen at some
>> >> >> indeterminate time in the future.
>> >> >
>> >> > It might not be that "far future"...  Please see below.
>> >> >
>> >> >> 
>> >> >> However:
>> >> >> 
>> >> >> > When there could be functional changes around the code path I would
>> >> >> > think we'd better keep the cleanup patches postponed a bit until those
>> >> >> > functional changes are settled.  For now the functional part is decide
>> >> >> > how to fix up the rest of out-of-band issues (my proposal is in the
>> >> >> > series above which should solve everything that is related to
>> >> >> > out-of-band to be fixed; if there is more, I'll continue to work on
>> >> >> > it), whether we should enable it by default for 3.1 (my answer
>> >> >> > is... yes...), and what to do with it.
>> >> >> 
>> >> >> I agree the important job is to finish OOB.
>> >> >> 
>> >> >> Sometimes, it's better to clean up first.  Sometimes, it's not.
>> >> >> 
>> >> >> Right now, the response queue is a useless complication, and
>> >> >> Marc-André's PATCH 3+4 get rid of it.  Lovely.  I understand this
>> >> >> conflicts with your OOB work.  The question is whether your work
>> >> >> fundamentally needs the response queue or not.
>> >> >
>> >> > Just to clarify a bit... I prefer to keep the response queue not
>> >> > because it's conflicting my existing work but because I think we might
>> >> > get use of it even in the near future.  I stated it here on the
>> >> > possibility that we might use the response queue to solve the
>> >> > unlimited monitor out_buf issue here:
>> >> >
>> >> > https://patchwork.kernel.org/patch/10511471/#22110771
>> >> >
>> >> > Quotes:
>> >> >
>> >> >         ...
>> >> >         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).
>> >> >         ...
>> >> >
>> >> > Let's imagine what we need if we want to limit the out_buf: (1) To
>> >> > limit the out_buf, we need somewhere to cache the responses that we
>> >> > want to put into out_buf but we can't when the out_buf is full -
>> >> > that's mostly the response queue now.  (2) Since we will need to queue
>> >> > these items onto out_buf when out_buf is not full, we'll possibly need
>> >> > something like a bottom half to kickoff when out_buf is able to handle
>> >> > more data - that's mostly the bottom half of the response queue.
>> >> >
>> >> > AFAIU the rest to do is very possible only that we set a limit to the
>> >> > out_buf but only if response queue is there...
>> >> 
>> >> Limiting outbuf only to have the same data pile up earlier in the
>> >> pipeline doesn't seem helpful.  We need to throttle production of
>> >> output.  A simple way to do that is throttling input.  See below.
>> >> 
>> >> > I didn't really work on the out_buf since I didn't want to further
>> >> > expand the out-of-band work (since it's already going far away before
>> >> > it settles down first...), and after all the out_buf issue is nothing
>> >> > related to the out-of-band work and it's there for a long time.
>> >> > However that's the major reason that I might prefer to keep the queue
>> >> > now.
>> >> >
>> >> > [1]
>> >> 
>> >> Let's review what we have.
>> >> 
>> >> QMP flow control is about limiting the amount of data QEMU buffers on
>> >> behalf of a QMP client.
>> >> 
>> >> This is about robustness, not security.  There are countless ways a QMP
>> >> client can make QEMU use more memory.  We want to cope with accidents,
>> >> not stop attacks.
>> >> 
>> >> A common and simple way to do flow control is to throttle receiving.
>> >> Unless the transport buffers way too much (which would be insane), the
>> >> peer will soon notice, and can adapt.
>> >> 
>> >> QMP input flows from the character device to commands.  It is converted
>> >> from text to QObject along the way.  Output flows from commands to the
>> >> character device.  It is converted from QObject to text along the way.
>> >> 
>> >> When the client sends input faster than QEMU can execute them, flow
>> >> control should kick in to stop adding more input to the pileup.  When
>> >> QEMU produces output faster than the client can receive it, flow control
>> >> should kick in to stop adding more output to the pileup.  We can do that
>> >> indirectly, by stopping input.
>> >> 
>> >> Input buffering:
>> >> 
>> >> * The chardev buffers 4KiB (I think).  Small enough to be ignored.
>> >> 
>> >> * The JSON parser buffers one partial JSON value as a token list, up to
>> >>   a limit in the order of 64MiB.  Weasel words "in the order", because
>> >>   it measures memory consumption only indirectly.  The limit is
>> >>   ridicilously generous for a control plane purpose like QMP.
>> >> 
>> >> * When the partial JSON value becomes complete, the JSON parser converts
>> >>   it to a QObject, then frees the token list.
>> >> 
>> >> * Without OOB, the QMP core buffers one complete QObject (the command)
>> >>   until we're done with the command.  The JSON parser's buffer should be
>> >>   empty then, because the QMP core suspends reading from the char device
>> >>   while it deals with a command.
>> >> 
>> >> * With OOB, the QMP core buffers up to 8 in-band commands and one
>> >>   out-of-band command.
>> >> 
>> >>   When the in-band command buffer is full, we currently drop further
>> >>   in-band commands, but that's a bad idea, and we're going to suspend
>> >>   reading from the char device instead.  Once we do, the JSON parser's
>> >>   buffer should be empty when the in-band command buffer is full.  The
>> >>   remainder of my analysis assumes we suspend rather than drop.
>> >> 
>> >> Output buffering:
>> >> 
>> >> * Traditionally, the QMP core buffers one QObject while it converts it
>> >>   to text.  It buffers an unlimited amount of text in mon->outbuf.
>> >> 
>> >> * Adding a request queue doesn't by itself change how much data is
>> >>   buffered, only when it's converted from QObject to text.  If the
>> >>   bottom half doing the conversion runs quickly, nothing changes.  If it
>> >>   gets delayed for some reason, QObjects can pile up in the response
>> >>   queue before they get converted and moved to mon->outbuf.
>> >> 
>> >> Summary of flow control:
>> >> 
>> >> * We limit input buffers and stop reading input when the limit is
>> >>   exceeded.  This stops input piling up.
>> >> 
>> >> * We do not limit output at all.
>> >> 
>> >> Ideally, we'd keep track of combined input and output buffer size, and
>> >> throttle input to keep it under control.  But separate limits for
>> >> individual buffers could be good enough, and might be simpler.
>> >
>> > (thanks for the summary)
>> >
>> >> 
>> >> >> If your OOB work happens to be coded for the response queue, but the
>> >> >> problem could also be solved without the response queue, then the OOB
>> >> >> job doesn't fundamentally need the response queue.
>> >> >
>> >> > Yes, I think the OOB work itself does not need the response queue now.
>> >> 
>> >> Understood.
>> >> 
>> >> >> Unless that's the case, getting rid of the response queue is unnecessary
>> >> >> churn.
>> >> >> 
>> >> >> If it is the case, we still need to consider effort.  Which order is
>> >> >> less total work?  Which order gets us to the goal faster?
>> >> >> 
>> >> >> Can you guys agree on answers to these questions, or do you need me to
>> >> >> answer them?
>> >> >> 
>> >> >> Restating the questions:
>> >> >> 
>> >> >> 1. Can you think of a way to do what Peter's OOB series does, but
>> >> >> without the response queue?
>> >> >> 
>> >> >> 2. If you can, what's easier / cheaper / faster:
>> >> >> 
>> >> >>    a. Merge Marc-André's patches to get rid of response queue, rewrite
>> >> >>       OOB series without response queue on top.
>> >> >> 
>> >> >>    b. Merge Peter's OOB series with response queue, rewrite patches to
>> >> >>       get rid of response queue on top.
>> >> >
>> >> > Let's have a quick look at above [1], if it's not a good reason (or
>> >> > even it's still unclear) then let's drop the queue.  It'll be
>> >> > perfectly fine I rebase my work upon Marc-André's.  After all the only
>> >> > reason to keep the response queue for me is to save time (for anyone
>> >> > who will be working with monitors).  If we spend too much time on
>> >> > judging whether we should keep the queue (we've already spent some)
>> >> > then it's already a waste of time...  It does not worth it IMO.
>> >> 
>> >> Time spent on coming up with a high-level plan for flow control is time
>> >> well spent.
>> >> 
>> >> Sometimes, you have to explore and experiment before you can come up
>> >> with a high-level plan that has a reasonable chance of working.  No
>> >> license to skip the "think before you hack" step entirely.
>> >> 
>> >> Here's the simplest (and possibly naive) plan I can think of: if
>> >> mon->outbuf exceeds a limit, suspend reading input.  No response queue.
>> >> Would it have a reasonable chance of working?
>> >
>> > Hmm I think it works...  Let's assume:
>> >
>> > - M: threshold size for outbuf
>> > - A: size of current outbuf
>> > - B: size of a new response message (and assume A+B>M, so the flow
>> >      control will trigger)
>> >
>> > I think that queue is not a must if we don't restrict the buffer that
>> > much - for example we can just queue the JSON object into outbuf when
>> > we receive the new message with size B (after queuing, we might get
>> > A+B>M, then it's slightly bigger than the limit threshold), now we
>> > suspend the monitor.
>> 
>> Yes.
>> 
>> M is a threshold for suspending the monitor, not a hard size limit.
>> 
>> Since the response QObjects has to go *somewhere*, we can just as well
>> stuff it into mon->outbuf.
>> 
>> The actual mon-outbuf size A may exceed the threshold M, but only while
>> the monitor is suspended.
>> 
>> > If we want to have a very strict buffer size limitation for outbuf so
>> > the outbuf never use more than M, we can't just queue it since it will
>> > overflow, then we need to stop the input and cache the object
>> > somewhere (e.g., the response queue).
>> 
>> Yes, but that doesn't actually limit memory use: instead of mon->outbuf
>> growing without bound, we now have response queue growing without bound.
>> Actual memory use changes only if one of the two representations QObject
>> and text is more compact.
>> 
>> Unscientific experiment: I instrumented qobject_to_json() to measure
>> size of its QObject input and text output (patch appended).  For
>> query-qmp-schema's output, I measure ~9.8MiB for QObject, and ~119KiB
>> for text:
>> 
>>     ### qobject_to_json(0x556653e7ac30)
>>     ###    #objs   #bytes QType
>>     ###        0        0 none
>>     ###      568        0 qnull
>>     ###        0        0 qnum
>>     ###    11876   540253 qstring
>>     ###     2301  9677496 qdict
>>     ###      509    86344 qlist
>>     ###        2       48 qbool
>>     ### 0x556653e7ac30 uses 10304141 vs. 121759
>> 
>> Most of the QObject's memory is occupied by QDicts.  The way they're
>> implemented is wasteful (known issue).  But even with a magical QDict
>> implementation that uses no memory at all, the QObject would still use
>> five times the memory of text.
>> 
>> My experiment suggests that to conserve memory, we should convert
>> responses to text right away.
>
> From memory saving POV, yes.  But keeping them as objects bring us
> flexibility and isolation, e.g., we can simply drop one event as we
> want (rather than dropping the whole outbuf), or suddenly we want to
> insert a new key due to some reason.  But these requirements have no
> real usage so far.  So I admit it's possibly me that have thought too
> much and I decided to not struggle. :)

YAGNI :)

Letting out-of-band responses (and perhaps certain events) overtake
in-band responses in the response queue would be kind of cool.  To be
actually useful, the response queue had to be non-empty.  Your code
empties it into mon->outbuf at the first opportunity.  That would have
to be changed.  Refilling mon->outbuf only when it gets empty would be
suboptimal, because it can split up writes to the underlying character
device.  Still, the response queue would be non-empty only when the QMP
client is slow to receive output.  Does that happen in practice?  My
point is: the additional complexity would be real, but the gains seem
dubious.

> (I never consider perf for QMP yet... so actually spending more memory
>  is not a problem to me; however possibility to use up the memory is
>  of course another thing...)
>
> Now I only hope we won't have other bug triggered due to start using
> the multi-threading of output buffer after we remove the queue.

That'll be Marc-André's fault then, for assuring us the "QMP response is
thread safe, and chardev write path is thread safe" right in his commit
message of PATCH 3 ;)

>> > So I think now I agree with you that the response queue is not
>> > required if we think the first solution is okay for us.
>> 
>> Cool.  Can you explore that and post patches?
>
> I think what I can do here is rebasing my work after Marc-Andre's
> patches merged.  Please let me know if there's anything else I can do.
>
> Thanks,

Sounds like a plan.