[Qemu-devel] [PATCH v5 00/20] monitor: add asynchronous command type

[Qemu-devel] [PATCH v5 00/20] monitor: add asynchronous command type

[Marc-André Lureau]

Hi,

HMP and QMP commands are handled synchronously in qemu today. But
there are benefits allowing the command handler to re-enter the main
loop if the command cannot be handled synchronously, or if it is
long-lasting. Some bugs such as rhbz#1230527 are difficult to solve
without it.

The common solution is to use a pair of command+event in this case.
But this approach has a number of issues:
- you can't "fix" an existing command: you need a new API, and ad-hoc
  documentation for that command+signal association, and old/broken
  command deprecation
- since the reply event is broadcasted and 'id' is used for matching the
  request, it may conflict with other clients request 'id' space
- it is arguably less efficient and elegant (weird API, useless return
  in most cases, broadcast reply, no cancelling on disconnect etc)

The following series implements an async command solution instead. By
introducing a session context and a command return handler, it can:
- defer the return, allowing the mainloop to reenter
- return only to the caller (instead of broadcast events for reply)
- optionnally allow cancellation when the client is gone
- track on-going qapi command(s) per client/session

and without introduction of new QMP APIs or client visible change.

Existing qemu commands can be gradually replaced by async:true
variants when needed, while carefully reviewing the concurrency
aspects. The async:true commands marshaller helpers are splitted in
half, the calling and return functions. The command is called with a
QmpReturn context, that can return immediately or later, using the
generated return helper, which allows for a step-by-step conversion.

The screendump command is converted to an async:true version to solve
rhbz#1230527. The command shows basic cancellation (this could be
extended if needed). It could be further improved to do asynchronous
IO writes as well.

v5:
- rebased

v4:
- rebased, mostly adapting to new OOB code
  (there was not much feedback in v3 for the async command part,
   but preliminary patches got merged!)
- drop the RFC status

v3:
- complete rework, dropping the asynchronous commands visibility from
  the protocol side entirely (until there is a real need for it)
- rebased, with a few preliminary cleanup patches
- teach asynchronous commands to HMP

v2:
- documentation fixes and improvements
- fix calling async commands sync without id
- fix bad hmp monitor assert
- add a few extra asserts
- add async with no-id failure and screendump test

Marc-André Lureau (20):
  qmp: constify QmpCommand and list
  json-lexer: make it safe to call destroy multiple times
  qmp: add QmpSession
  QmpSession: add a return callback
  QmpSession: add json parser and use it in qga
  monitor: use qmp session to parse json feed
  qga: simplify dispatch_return_cb
  QmpSession: introduce QmpReturn
  qmp: simplify qmp_return_error()
  QmpSession: keep a queue of pending commands
  QmpSession: return orderly
  qmp: introduce asynchronous command type
  scripts: learn 'async' qapi commands
  qmp: add qmp_return_is_cancelled()
  monitor: add qmp_return_get_monitor()
  console: add graphic_hw_update_done()
  console: make screendump asynchronous
  monitor: start making qmp_human_monitor_command() asynchronous
  monitor: teach HMP about asynchronous commands
  hmp: call the asynchronous QMP screendump to fix outdated/glitches

 hmp-commands.hx                         |   3 +-
 hw/display/qxl-render.c                 |   9 +-
 hw/display/qxl.c                        |   1 +
 include/monitor/monitor.h               |   3 +
 include/qapi/qmp/dispatch.h             |  89 +++++++++-
 include/qapi/qmp/json-parser.h          |   7 +-
 include/ui/console.h                    |   4 +
 monitor/hmp-cmds.c                      |   6 +-
 monitor/hmp.c                           | 110 +++++++++++-
 monitor/misc.c                          |  46 +----
 monitor/monitor-internal.h              |  12 +-
 monitor/monitor.c                       |   2 +-
 monitor/qmp.c                           |  79 ++++-----
 qapi/misc.json                          |   3 +-
 qapi/qmp-dispatch.c                     | 214 +++++++++++++++++++-----
 qapi/qmp-registry.c                     |  33 +++-
 qapi/ui.json                            |   3 +-
 qga/commands.c                          |   2 +-
 qga/main.c                              |  51 ++----
 qobject/json-lexer.c                    |   5 +-
 qobject/json-streamer.c                 |   3 +-
 scripts/qapi/commands.py                | 151 ++++++++++++++---
 scripts/qapi/common.py                  |  15 +-
 scripts/qapi/doc.py                     |   3 +-
 scripts/qapi/introspect.py              |   3 +-
 tests/qapi-schema/qapi-schema-test.json |   5 +
 tests/qapi-schema/qapi-schema-test.out  |   8 +
 tests/qapi-schema/test-qapi.py          |   8 +-
 tests/test-qmp-cmds.c                   | 206 +++++++++++++++++++----
 ui/console.c                            | 100 +++++++++--
 30 files changed, 908 insertions(+), 276 deletions(-)

-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 01/20] qmp: constify QmpCommand and list

[Marc-André Lureau]

Since 0b69f6f72ce47a37a749b056b6d5ec64c61f11e8 "qapi: remove
qmp_unregister_command()", the command list can be declared const.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/dispatch.h | 9 +++++----
 monitor/misc.c              | 2 +-
 monitor/monitor-internal.h  | 2 +-
 qapi/qmp-dispatch.c         | 6 +++---
 qapi/qmp-registry.c         | 6 +++---
 qga/commands.c              | 2 +-
 qga/main.c                  | 6 +++---
 7 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 9aa426a398..5a9cf82472 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -39,7 +39,8 @@ typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
 
 void qmp_register_command(QmpCommandList *cmds, const char *name,
                           QmpCommandFunc *fn, QmpCommandOptions options);
-QmpCommand *qmp_find_command(QmpCommandList *cmds, const char *name);
+const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
+                                   const char *name);
 void qmp_disable_command(QmpCommandList *cmds, const char *name);
 void qmp_enable_command(QmpCommandList *cmds, const char *name);
 
@@ -47,13 +48,13 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QDict *qmp_error_response(Error *err);
-QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
+QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
                     bool allow_oob);
 bool qmp_is_oob(const QDict *dict);
 
-typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
+typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque);
 
-void qmp_for_each_command(QmpCommandList *cmds, qmp_cmd_callback_fn fn,
+void qmp_for_each_command(const QmpCommandList *cmds, qmp_cmd_callback_fn fn,
                           void *opaque);
 
 #endif
diff --git a/monitor/misc.c b/monitor/misc.c
index 00338c002a..a0fc5111c5 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -230,7 +230,7 @@ static void hmp_info_help(Monitor *mon, const QDict *qdict)
     help_cmd(mon, "info");
 }
 
-static void query_commands_cb(QmpCommand *cmd, void *opaque)
+static void query_commands_cb(const QmpCommand *cmd, void *opaque)
 {
     CommandInfoList *info, **list = opaque;
 
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 7760b22ba3..b0a028dbf8 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -133,7 +133,7 @@ typedef struct {
      * qmp_capabilities succeeds, we go into command mode, and
      * @command becomes &qmp_commands.
      */
-    QmpCommandList *commands;
+    const QmpCommandList *commands;
     bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
     bool capab[QMP_CAPABILITY__MAX];         /* offered and accepted */
     /*
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index e2c366e09e..f9d43046aa 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -75,14 +75,14 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
     return dict;
 }
 
-static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
+static QObject *do_qmp_dispatch(const QmpCommandList *cmds, QObject *request,
                                 bool allow_oob, Error **errp)
 {
     Error *local_err = NULL;
     bool oob;
     const char *command;
     QDict *args, *dict;
-    QmpCommand *cmd;
+    const QmpCommand *cmd;
     QObject *ret = NULL;
 
     dict = qmp_dispatch_check_obj(request, allow_oob, errp);
@@ -163,7 +163,7 @@ bool qmp_is_oob(const QDict *dict)
         && !qdict_haskey(dict, "execute");
 }
 
-QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
+QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
                     bool allow_oob)
 {
     Error *err = NULL;
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index ca00f74795..d0f9a1d3e3 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -27,7 +27,7 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
     QTAILQ_INSERT_TAIL(cmds, cmd, node);
 }
 
-QmpCommand *qmp_find_command(QmpCommandList *cmds, const char *name)
+const QmpCommand *qmp_find_command(const QmpCommandList *cmds, const char *name)
 {
     QmpCommand *cmd;
 
@@ -77,10 +77,10 @@ bool qmp_has_success_response(const QmpCommand *cmd)
     return !(cmd->options & QCO_NO_SUCCESS_RESP);
 }
 
-void qmp_for_each_command(QmpCommandList *cmds, qmp_cmd_callback_fn fn,
+void qmp_for_each_command(const QmpCommandList *cmds, qmp_cmd_callback_fn fn,
                           void *opaque)
 {
-    QmpCommand *cmd;
+    const QmpCommand *cmd;
 
     QTAILQ_FOREACH(cmd, cmds, node) {
         fn(cmd, opaque);
diff --git a/qga/commands.c b/qga/commands.c
index 0c7d1385c2..05e9ab6c3d 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -54,7 +54,7 @@ void qmp_guest_ping(Error **errp)
     slog("guest-ping called");
 }
 
-static void qmp_command_info(QmpCommand *cmd, void *opaque)
+static void qmp_command_info(const QmpCommand *cmd, void *opaque)
 {
     GuestAgentInfo *info = opaque;
     GuestAgentCommandInfo *cmd_info;
diff --git a/qga/main.c b/qga/main.c
index c35c2a2120..f23614528e 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -359,7 +359,7 @@ static gint ga_strcmp(gconstpointer str1, gconstpointer str2)
 }
 
 /* disable commands that aren't safe for fsfreeze */
-static void ga_disable_non_whitelisted(QmpCommand *cmd, void *opaque)
+static void ga_disable_non_whitelisted(const QmpCommand *cmd, void *opaque)
 {
     bool whitelisted = false;
     int i = 0;
@@ -378,7 +378,7 @@ static void ga_disable_non_whitelisted(QmpCommand *cmd, void *opaque)
 }
 
 /* [re-]enable all commands, except those explicitly blacklisted by user */
-static void ga_enable_non_blacklisted(QmpCommand *cmd, void *opaque)
+static void ga_enable_non_blacklisted(const QmpCommand *cmd, void *opaque)
 {
     GList *blacklist = opaque;
     const char *name = qmp_command_name(cmd);
@@ -918,7 +918,7 @@ int64_t ga_get_fd_handle(GAState *s, Error **errp)
     return handle;
 }
 
-static void ga_print_cmd(QmpCommand *cmd, void *opaque)
+static void ga_print_cmd(const QmpCommand *cmd, void *opaque)
 {
     printf("%s\n", qmp_command_name(cmd));
 }
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 02/20] 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 with the following patch,
"qmp: add QmpSession" can call qmp_session_destroy() multiple times,
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 632320d72d..fa7a2c43a8 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -361,5 +361,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.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 03/20] qmp: add QmpSession

[Marc-André Lureau]

This structure will hold various data related to a QMP client session:
the list of commands, the parser, the callbacks, the pending
operations...

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/dispatch.h | 10 +++++++++-
 monitor/misc.c              |  6 +++---
 monitor/monitor-internal.h  |  2 +-
 monitor/monitor.c           |  2 +-
 monitor/qmp.c               |  8 +++++---
 qapi/qmp-dispatch.c         | 15 ++++++++++++---
 qga/main.c                  |  5 ++++-
 tests/test-qmp-cmds.c       | 28 ++++++++++++++++++++++------
 8 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 5a9cf82472..3b53cfd788 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -37,10 +37,18 @@ typedef struct QmpCommand
 
 typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
 
+typedef struct QmpSession QmpSession;
+
+struct QmpSession {
+    const QmpCommandList *cmds;
+};
+
 void qmp_register_command(QmpCommandList *cmds, const char *name,
                           QmpCommandFunc *fn, QmpCommandOptions options);
 const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
                                    const char *name);
+void qmp_session_init(QmpSession *session, const QmpCommandList *cmds);
+void qmp_session_destroy(QmpSession *session);
 void qmp_disable_command(QmpCommandList *cmds, const char *name);
 void qmp_enable_command(QmpCommandList *cmds, const char *name);
 
@@ -48,7 +56,7 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QDict *qmp_error_response(Error *err);
-QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
+QDict *qmp_dispatch(QmpSession *session, QObject *request,
                     bool allow_oob);
 bool qmp_is_oob(const QDict *dict);
 
diff --git a/monitor/misc.c b/monitor/misc.c
index a0fc5111c5..a23c1b8ba4 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -253,7 +253,7 @@ CommandInfoList *qmp_query_commands(Error **errp)
     assert(monitor_is_qmp(cur_mon));
     mon = container_of(cur_mon, MonitorQMP, common);
 
-    qmp_for_each_command(mon->commands, query_commands_cb, &list);
+    qmp_for_each_command(mon->session.cmds, query_commands_cb, &list);
 
     return list;
 }
@@ -363,7 +363,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
     assert(monitor_is_qmp(cur_mon));
     mon = container_of(cur_mon, MonitorQMP, common);
 
-    if (mon->commands == &qmp_commands) {
+    if (mon->session.cmds == &qmp_commands) {
         error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
                   "Capabilities negotiation is already complete, command "
                   "ignored");
@@ -374,7 +374,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
         return;
     }
 
-    mon->commands = &qmp_commands;
+    mon->session.cmds = &qmp_commands;
 }
 
 /* Set the current CPU defined by the user. Callers must hold BQL. */
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index b0a028dbf8..65d587eafb 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -133,7 +133,7 @@ typedef struct {
      * qmp_capabilities succeeds, we go into command mode, and
      * @command becomes &qmp_commands.
      */
-    const QmpCommandList *commands;
+    QmpSession session;
     bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
     bool capab[QMP_CAPABILITY__MAX];         /* offered and accepted */
     /*
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 3ef28171c0..9d918c9952 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -262,7 +262,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)
         }
 
         qmp_mon = container_of(mon, MonitorQMP, common);
-        if (qmp_mon->commands != &qmp_cap_negotiation_commands) {
+        if (qmp_mon->session.cmds != &qmp_cap_negotiation_commands) {
             qmp_send_response(qmp_mon, qdict);
         }
     }
diff --git a/monitor/qmp.c b/monitor/qmp.c
index e1b196217d..dd72a0d8cf 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -117,11 +117,11 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
     old_mon = cur_mon;
     cur_mon = &mon->common;
 
-    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
+    rsp = qmp_dispatch(&mon->session, req, qmp_oob_enabled(mon));
 
     cur_mon = old_mon;
 
-    if (mon->commands == &qmp_cap_negotiation_commands) {
+    if (mon->session.cmds == &qmp_cap_negotiation_commands) {
         error = qdict_get_qdict(rsp, "error");
         if (error
             && !g_strcmp0(qdict_get_try_str(error, "class"),
@@ -318,7 +318,7 @@ static void monitor_qmp_event(void *opaque, int event)
 
     switch (event) {
     case CHR_EVENT_OPENED:
-        mon->commands = &qmp_cap_negotiation_commands;
+        qmp_session_init(&mon->session, &qmp_cap_negotiation_commands);
         monitor_qmp_caps_reset(mon);
         data = qmp_greeting(mon);
         qmp_send_response(mon, data);
@@ -333,6 +333,7 @@ static void monitor_qmp_event(void *opaque, int event)
          * is closed.
          */
         monitor_qmp_cleanup_queues(mon);
+        qmp_session_destroy(&mon->session);
         json_message_parser_destroy(&mon->parser);
         json_message_parser_init(&mon->parser, handle_qmp_command,
                                  mon, NULL);
@@ -344,6 +345,7 @@ static void monitor_qmp_event(void *opaque, int event)
 
 void monitor_data_destroy_qmp(MonitorQMP *mon)
 {
+    qmp_session_destroy(&mon->session);
     json_message_parser_destroy(&mon->parser);
     qemu_mutex_destroy(&mon->qmp_queue_lock);
     monitor_qmp_cleanup_req_queue_locked(mon);
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index f9d43046aa..98a82ac33c 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -163,15 +163,24 @@ bool qmp_is_oob(const QDict *dict)
         && !qdict_haskey(dict, "execute");
 }
 
-QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
-                    bool allow_oob)
+void qmp_session_init(QmpSession *session, const QmpCommandList *cmds)
+{
+    session->cmds = cmds;
+}
+
+void qmp_session_destroy(QmpSession *session)
+{
+    session->cmds = NULL;
+}
+
+QDict *qmp_dispatch(QmpSession *session, QObject *request, bool allow_oob)
 {
     Error *err = NULL;
     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);
+    ret = do_qmp_dispatch(session->cmds, request, allow_oob, &err);
     if (err) {
         rsp = qmp_error_response(err);
     } else if (ret) {
diff --git a/qga/main.c b/qga/main.c
index f23614528e..61190db5f3 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -74,6 +74,7 @@ typedef struct GAPersistentState {
 typedef struct GAConfig GAConfig;
 
 struct GAState {
+    QmpSession session;
     JSONMessageParser parser;
     GMainLoop *main_loop;
     GAChannel *channel;
@@ -572,7 +573,7 @@ static void process_event(void *opaque, QObject *obj, Error *err)
     }
 
     g_debug("processing command");
-    rsp = qmp_dispatch(&ga_commands, obj, false);
+    rsp = qmp_dispatch(&s->session, obj, false);
 
 end:
     ret = send_response(s, rsp);
@@ -1338,6 +1339,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
     json_message_parser_init(&s->parser, process_event, s, NULL);
+    qmp_session_init(&s->session, &ga_commands);
 
 #ifndef _WIN32
     if (!register_signal_handlers()) {
@@ -1369,6 +1371,7 @@ static void cleanup_agent(GAState *s)
     CloseHandle(s->wakeup_event);
 #endif
     if (s->command_state) {
+        qmp_session_destroy(&s->session);
         ga_command_state_cleanup_all(s->command_state);
         ga_command_state_free(s->command_state);
         json_message_parser_destroy(&s->parser);
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index ab389f42da..e2fe04ee8b 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -119,44 +119,52 @@ __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
 /* test commands with no input and no return value */
 static void test_dispatch_cmd(void)
 {
+    QmpSession session = { 0, };
     QDict *req = qdict_new();
     QDict *resp;
 
+    qmp_session_init(&session, &qmp_commands);
     qdict_put_str(req, "execute", "user_def_cmd");
 
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+    resp = qmp_dispatch(&session, QOBJECT(req), false);
     assert(resp != NULL);
     assert(!qdict_haskey(resp, "error"));
 
     qobject_unref(resp);
     qobject_unref(req);
+    qmp_session_destroy(&session);
 }
 
 static void test_dispatch_cmd_oob(void)
 {
+    QmpSession session = { 0, };
     QDict *req = qdict_new();
     QDict *resp;
 
+    qmp_session_init(&session, &qmp_commands);
     qdict_put_str(req, "exec-oob", "test-flags-command");
 
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), true);
+    resp = qmp_dispatch(&session, QOBJECT(req), true);
     assert(resp != NULL);
     assert(!qdict_haskey(resp, "error"));
 
     qobject_unref(resp);
     qobject_unref(req);
+    qmp_session_destroy(&session);
 }
 
 /* test commands that return an error due to invalid parameters */
 static void test_dispatch_cmd_failure(void)
 {
+    QmpSession session = { 0, };
     QDict *req = qdict_new();
     QDict *args = qdict_new();
     QDict *resp;
 
+    qmp_session_init(&session, &qmp_commands);
     qdict_put_str(req, "execute", "user_def_cmd2");
 
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+    resp = qmp_dispatch(&session, QOBJECT(req), false);
     assert(resp != NULL);
     assert(qdict_haskey(resp, "error"));
 
@@ -170,36 +178,44 @@ static void test_dispatch_cmd_failure(void)
 
     qdict_put_str(req, "execute", "user_def_cmd");
 
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+    resp = qmp_dispatch(&session, QOBJECT(req), false);
     assert(resp != NULL);
     assert(qdict_haskey(resp, "error"));
 
     qobject_unref(resp);
     qobject_unref(req);
+    qmp_session_destroy(&session);
 }
 
 static void test_dispatch_cmd_success_response(void)
 {
+    QmpSession session = { 0, };
     QDict *req = qdict_new();
     QDict *resp;
 
+    qmp_session_init(&session, &qmp_commands);
     qdict_put_str(req, "execute", "cmd-success-response");
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+    resp = qmp_dispatch(&session, QOBJECT(req), false);
     g_assert_null(resp);
     qobject_unref(req);
+    qmp_session_destroy(&session);
 }
 
+
 static QObject *test_qmp_dispatch(QDict *req)
 {
+    QmpSession session = { 0, };
     QDict *resp;
     QObject *ret;
 
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+    qmp_session_init(&session, &qmp_commands);
+    resp = qmp_dispatch(&session, QOBJECT(req), false);
     assert(resp && !qdict_haskey(resp, "error"));
     ret = qdict_get(resp, "return");
     assert(ret);
     qobject_ref(ret);
     qobject_unref(resp);
+    qmp_session_destroy(&session);
     return ret;
 }
 
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 04/20] QmpSession: add a return callback

[Marc-André Lureau]

Introduce a return_cb to allow delaying finishing the dispatch
and sending the response asynchronously. For now, this is just
modifying qmp_dispatch() to call the callback synchronously.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/dispatch.h | 10 ++++--
 monitor/qmp.c               | 47 ++++++++++---------------
 qapi/qmp-dispatch.c         | 22 +++++++++---
 qga/main.c                  | 34 +++++++++++-------
 tests/test-qmp-cmds.c       | 69 ++++++++++++++++++-------------------
 5 files changed, 98 insertions(+), 84 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 3b53cfd788..d1ce631a93 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -38,16 +38,20 @@ typedef struct QmpCommand
 typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
 
 typedef struct QmpSession QmpSession;
+typedef void (QmpDispatchReturn) (QmpSession *session, QDict *rsp);
 
 struct QmpSession {
     const QmpCommandList *cmds;
+    QmpDispatchReturn *return_cb;
 };
 
 void qmp_register_command(QmpCommandList *cmds, const char *name,
                           QmpCommandFunc *fn, QmpCommandOptions options);
 const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
                                    const char *name);
-void qmp_session_init(QmpSession *session, const QmpCommandList *cmds);
+void qmp_session_init(QmpSession *session,
+                      const QmpCommandList *cmds,
+                      QmpDispatchReturn *return_cb);
 void qmp_session_destroy(QmpSession *session);
 void qmp_disable_command(QmpCommandList *cmds, const char *name);
 void qmp_enable_command(QmpCommandList *cmds, const char *name);
@@ -56,8 +60,8 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QDict *qmp_error_response(Error *err);
-QDict *qmp_dispatch(QmpSession *session, QObject *request,
-                    bool allow_oob);
+void qmp_dispatch(QmpSession *session, QObject *request,
+                  bool allow_oob);
 bool qmp_is_oob(const QDict *dict);
 
 typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque);
diff --git a/monitor/qmp.c b/monitor/qmp.c
index dd72a0d8cf..b215cb70f3 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -96,45 +96,35 @@ void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
     qobject_unref(json);
 }
 
-/*
- * Emit QMP response @rsp with ID @id to @mon.
- * Null @rsp can only happen for commands with QCO_NO_SUCCESS_RESP.
- * Nothing is emitted then.
- */
-static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
+static void dispatch_return_cb(QmpSession *session, QDict *rsp)
 {
-    if (rsp) {
-        qmp_send_response(mon, rsp);
+    MonitorQMP *mon = container_of(session, MonitorQMP, session);
+
+    if (mon->session.cmds == &qmp_cap_negotiation_commands) {
+        QDict *error = qdict_get_qdict(rsp, "error");
+        if (error
+            && !g_strcmp0(qdict_get_try_str(error, "class"),
+                          QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) {
+            /* Provide a more useful error message */
+            qdict_del(error, "desc");
+            qdict_put_str(error, "desc", "Expecting capabilities negotiation"
+                          " with 'qmp_capabilities'");
+        }
     }
+
+    qmp_send_response(mon, rsp);
 }
 
 static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
 {
     Monitor *old_mon;
-    QDict *rsp;
-    QDict *error;
 
     old_mon = cur_mon;
     cur_mon = &mon->common;
 
-    rsp = qmp_dispatch(&mon->session, req, qmp_oob_enabled(mon));
+    qmp_dispatch(&mon->session, req, qmp_oob_enabled(mon));
 
     cur_mon = old_mon;
-
-    if (mon->session.cmds == &qmp_cap_negotiation_commands) {
-        error = qdict_get_qdict(rsp, "error");
-        if (error
-            && !g_strcmp0(qdict_get_try_str(error, "class"),
-                    QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) {
-            /* Provide a more useful error message */
-            qdict_del(error, "desc");
-            qdict_put_str(error, "desc", "Expecting capabilities negotiation"
-                          " with 'qmp_capabilities'");
-        }
-    }
-
-    monitor_qmp_respond(mon, rsp);
-    qobject_unref(rsp);
 }
 
 /*
@@ -211,7 +201,7 @@ void monitor_qmp_bh_dispatcher(void *data)
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
         req_obj->err = NULL;
-        monitor_qmp_respond(mon, rsp);
+        qmp_send_response(req_obj->mon, rsp);
         qobject_unref(rsp);
     }
 
@@ -318,7 +308,8 @@ static void monitor_qmp_event(void *opaque, int event)
 
     switch (event) {
     case CHR_EVENT_OPENED:
-        qmp_session_init(&mon->session, &qmp_cap_negotiation_commands);
+        qmp_session_init(&mon->session,
+                         &qmp_cap_negotiation_commands, dispatch_return_cb);
         monitor_qmp_caps_reset(mon);
         data = qmp_greeting(mon);
         qmp_send_response(mon, data);
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 98a82ac33c..37b058cf97 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -163,17 +163,28 @@ bool qmp_is_oob(const QDict *dict)
         && !qdict_haskey(dict, "execute");
 }
 
-void qmp_session_init(QmpSession *session, const QmpCommandList *cmds)
+void qmp_session_init(QmpSession *session,
+                      const QmpCommandList *cmds,
+                      QmpDispatchReturn *return_cb)
 {
+    assert(return_cb);
+    assert(!session->return_cb);
+
     session->cmds = cmds;
+    session->return_cb = return_cb;
 }
 
 void qmp_session_destroy(QmpSession *session)
 {
+    if (!session->return_cb) {
+        return;
+    }
+
     session->cmds = NULL;
+    session->return_cb = NULL;
 }
 
-QDict *qmp_dispatch(QmpSession *session, QObject *request, bool allow_oob)
+void qmp_dispatch(QmpSession *session, QObject *request, bool allow_oob)
 {
     Error *err = NULL;
     QDict *dict = qobject_to(QDict, request);
@@ -188,12 +199,13 @@ QDict *qmp_dispatch(QmpSession *session, QObject *request, bool allow_oob)
         qdict_put_obj(rsp, "return", ret);
     } else {
         /* Can only happen for commands with QCO_NO_SUCCESS_RESP */
-        rsp = NULL;
+        return;
     }
 
-    if (rsp && id) {
+    if (id) {
         qdict_put_obj(rsp, "id", qobject_ref(id));
     }
 
-    return rsp;
+    session->return_cb(session, rsp);
+    qobject_unref(rsp);
 }
diff --git a/qga/main.c b/qga/main.c
index 61190db5f3..c291d06491 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -558,29 +558,37 @@ static int send_response(GAState *s, const QDict *rsp)
     return 0;
 }
 
+static void dispatch_return_cb(QmpSession *session, QDict *rsp)
+{
+    GAState *s = container_of(session, GAState, session);
+    int ret = send_response(s, rsp);
+    if (ret < 0) {
+        g_warning("error sending response: %s", strerror(-ret));
+    }
+}
+
 /* handle requests/control events coming in over the channel */
 static void process_event(void *opaque, QObject *obj, Error *err)
 {
     GAState *s = opaque;
-    QDict *rsp;
     int ret;
 
     g_debug("process_event: called");
     assert(!obj != !err);
-    if (err) {
-        rsp = qmp_error_response(err);
-        goto end;
-    }
 
-    g_debug("processing command");
-    rsp = qmp_dispatch(&s->session, obj, false);
+    if (err) {
+        QDict *rsp = qmp_error_response(err);
 
-end:
-    ret = send_response(s, rsp);
-    if (ret < 0) {
-        g_warning("error sending error response: %s", strerror(-ret));
+        ret = send_response(s, rsp);
+        if (ret < 0) {
+            g_warning("error sending error response: %s", strerror(-ret));
+        }
+        qobject_unref(rsp);
+    } else {
+        g_debug("processing command");
+        qmp_dispatch(&s->session, obj, false);
     }
-    qobject_unref(rsp);
+
     qobject_unref(obj);
 }
 
@@ -1339,7 +1347,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
     json_message_parser_init(&s->parser, process_event, s, NULL);
-    qmp_session_init(&s->session, &ga_commands);
+    qmp_session_init(&s->session, &ga_commands, dispatch_return_cb);
 
 #ifndef _WIN32
     if (!register_signal_handlers()) {
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index e2fe04ee8b..7b3bccc091 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -115,22 +115,23 @@ __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
     return ret;
 }
 
+static void dispatch_cmd_return(QmpSession *session, QDict *resp)
+{
+    assert(resp != NULL);
+    assert(!qdict_haskey(resp, "error"));
+}
 
 /* test commands with no input and no return value */
 static void test_dispatch_cmd(void)
 {
     QmpSession session = { 0, };
     QDict *req = qdict_new();
-    QDict *resp;
 
-    qmp_session_init(&session, &qmp_commands);
+    qmp_session_init(&session, &qmp_commands, dispatch_cmd_return);
     qdict_put_str(req, "execute", "user_def_cmd");
 
-    resp = qmp_dispatch(&session, QOBJECT(req), false);
-    assert(resp != NULL);
-    assert(!qdict_haskey(resp, "error"));
+    qmp_dispatch(&session, QOBJECT(req), false);
 
-    qobject_unref(resp);
     qobject_unref(req);
     qmp_session_destroy(&session);
 }
@@ -139,82 +140,80 @@ static void test_dispatch_cmd_oob(void)
 {
     QmpSession session = { 0, };
     QDict *req = qdict_new();
-    QDict *resp;
 
-    qmp_session_init(&session, &qmp_commands);
+    qmp_session_init(&session, &qmp_commands, dispatch_cmd_return);
     qdict_put_str(req, "exec-oob", "test-flags-command");
 
-    resp = qmp_dispatch(&session, QOBJECT(req), true);
-    assert(resp != NULL);
-    assert(!qdict_haskey(resp, "error"));
+    qmp_dispatch(&session, QOBJECT(req), true);
 
-    qobject_unref(resp);
     qobject_unref(req);
     qmp_session_destroy(&session);
 }
 
+static void dispatch_cmd_failure_return(QmpSession *session, QDict *resp)
+{
+    assert(resp != NULL);
+    assert(qdict_haskey(resp, "error"));
+}
+
 /* test commands that return an error due to invalid parameters */
 static void test_dispatch_cmd_failure(void)
 {
     QmpSession session = { 0, };
     QDict *req = qdict_new();
     QDict *args = qdict_new();
-    QDict *resp;
 
-    qmp_session_init(&session, &qmp_commands);
+    qmp_session_init(&session, &qmp_commands, dispatch_cmd_failure_return);
     qdict_put_str(req, "execute", "user_def_cmd2");
 
-    resp = qmp_dispatch(&session, QOBJECT(req), false);
-    assert(resp != NULL);
-    assert(qdict_haskey(resp, "error"));
+    qmp_dispatch(&session, QOBJECT(req), false);
 
-    qobject_unref(resp);
     qobject_unref(req);
 
     /* check that with extra arguments it throws an error */
     req = qdict_new();
     qdict_put_int(args, "a", 66);
     qdict_put(req, "arguments", args);
-
     qdict_put_str(req, "execute", "user_def_cmd");
 
-    resp = qmp_dispatch(&session, QOBJECT(req), false);
-    assert(resp != NULL);
-    assert(qdict_haskey(resp, "error"));
+    qmp_dispatch(&session, QOBJECT(req), false);
 
-    qobject_unref(resp);
     qobject_unref(req);
     qmp_session_destroy(&session);
 }
 
+static QObject *dispatch_ret;
+
 static void test_dispatch_cmd_success_response(void)
 {
     QmpSession session = { 0, };
     QDict *req = qdict_new();
-    QDict *resp;
 
-    qmp_session_init(&session, &qmp_commands);
+    qmp_session_init(&session, &qmp_commands, (QmpDispatchReturn *)abort);
     qdict_put_str(req, "execute", "cmd-success-response");
-    resp = qmp_dispatch(&session, QOBJECT(req), false);
-    g_assert_null(resp);
+
+    qmp_dispatch(&session, QOBJECT(req), false);
+
     qobject_unref(req);
     qmp_session_destroy(&session);
 }
 
+static void dispatch_return(QmpSession *session, QDict *resp)
+{
+    assert(resp && !qdict_haskey(resp, "error"));
+    dispatch_ret = qdict_get(resp, "return");
+    qobject_ref(dispatch_ret);
+}
 
 static QObject *test_qmp_dispatch(QDict *req)
 {
     QmpSession session = { 0, };
-    QDict *resp;
     QObject *ret;
 
-    qmp_session_init(&session, &qmp_commands);
-    resp = qmp_dispatch(&session, QOBJECT(req), false);
-    assert(resp && !qdict_haskey(resp, "error"));
-    ret = qdict_get(resp, "return");
-    assert(ret);
-    qobject_ref(ret);
-    qobject_unref(resp);
+    qmp_session_init(&session, &qmp_commands, dispatch_return);
+    qmp_dispatch(&session, QOBJECT(req), false);
+    ret = dispatch_ret;
+    dispatch_ret = NULL;
     qmp_session_destroy(&session);
     return ret;
 }
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 05/20] QmpSession: add json parser and use it in qga

[Marc-André Lureau]

Move JSON parser to QmpSession, and implement a simple handler to check
the parsed tokens and call qmp_dispatch(). This is enough for a simple
QMP client, like QGA.

The QEMU monitor has more complicated handling of dispatching which
will be addressed in a following patch to benefit from more common
code.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/dispatch.h |  7 +++++++
 qapi/qmp-dispatch.c         | 19 +++++++++++++++++++
 qga/main.c                  | 31 +------------------------------
 3 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index d1ce631a93..c84edff7d2 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -15,6 +15,7 @@
 #define QAPI_QMP_DISPATCH_H
 
 #include "qemu/queue.h"
+#include "qapi/qmp/json-parser.h"
 
 typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
 
@@ -42,6 +43,7 @@ typedef void (QmpDispatchReturn) (QmpSession *session, QDict *rsp);
 
 struct QmpSession {
     const QmpCommandList *cmds;
+    JSONMessageParser parser;
     QmpDispatchReturn *return_cb;
 };
 
@@ -52,6 +54,11 @@ const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
 void qmp_session_init(QmpSession *session,
                       const QmpCommandList *cmds,
                       QmpDispatchReturn *return_cb);
+static inline void
+qmp_session_feed(QmpSession *session, const char *buf, size_t count)
+{
+    json_message_parser_feed(&session->parser, buf, count);
+}
 void qmp_session_destroy(QmpSession *session);
 void qmp_disable_command(QmpCommandList *cmds, const char *name);
 void qmp_enable_command(QmpCommandList *cmds, const char *name);
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 37b058cf97..803ec626cd 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -163,6 +163,23 @@ bool qmp_is_oob(const QDict *dict)
         && !qdict_haskey(dict, "execute");
 }
 
+static void qmp_json_emit(void *opaque, QObject *obj, Error *err)
+{
+    QmpSession *session = opaque;
+
+    assert(!obj != !err);
+
+    if (err) {
+        QDict *rsp = qmp_error_response(err);
+        session->return_cb(session, rsp);
+        qobject_unref(rsp);
+    } else {
+        qmp_dispatch(session, obj, false);
+    }
+
+    qobject_unref(obj);
+}
+
 void qmp_session_init(QmpSession *session,
                       const QmpCommandList *cmds,
                       QmpDispatchReturn *return_cb)
@@ -170,6 +187,7 @@ void qmp_session_init(QmpSession *session,
     assert(return_cb);
     assert(!session->return_cb);
 
+    json_message_parser_init(&session->parser, qmp_json_emit, session, NULL);
     session->cmds = cmds;
     session->return_cb = return_cb;
 }
@@ -182,6 +200,7 @@ void qmp_session_destroy(QmpSession *session)
 
     session->cmds = NULL;
     session->return_cb = NULL;
+    json_message_parser_destroy(&session->parser);
 }
 
 void qmp_dispatch(QmpSession *session, QObject *request, bool allow_oob)
diff --git a/qga/main.c b/qga/main.c
index c291d06491..057368eb16 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -19,7 +19,6 @@
 #include <sys/wait.h>
 #endif
 #include "qemu-common.h"
-#include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qstring.h"
@@ -75,7 +74,6 @@ typedef struct GAConfig GAConfig;
 
 struct GAState {
     QmpSession session;
-    JSONMessageParser parser;
     GMainLoop *main_loop;
     GAChannel *channel;
     bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
@@ -567,31 +565,6 @@ static void dispatch_return_cb(QmpSession *session, QDict *rsp)
     }
 }
 
-/* handle requests/control events coming in over the channel */
-static void process_event(void *opaque, QObject *obj, Error *err)
-{
-    GAState *s = opaque;
-    int ret;
-
-    g_debug("process_event: called");
-    assert(!obj != !err);
-
-    if (err) {
-        QDict *rsp = qmp_error_response(err);
-
-        ret = send_response(s, rsp);
-        if (ret < 0) {
-            g_warning("error sending error response: %s", strerror(-ret));
-        }
-        qobject_unref(rsp);
-    } else {
-        g_debug("processing command");
-        qmp_dispatch(&s->session, obj, false);
-    }
-
-    qobject_unref(obj);
-}
-
 /* false return signals GAChannel to close the current client connection */
 static gboolean channel_event_cb(GIOCondition condition, gpointer data)
 {
@@ -607,7 +580,7 @@ static gboolean channel_event_cb(GIOCondition condition, gpointer data)
     case G_IO_STATUS_NORMAL:
         buf[count] = 0;
         g_debug("read data, count: %d, data: %s", (int)count, buf);
-        json_message_parser_feed(&s->parser, (char *)buf, (int)count);
+        qmp_session_feed(&s->session, (char *)buf, (int)count);
         break;
     case G_IO_STATUS_EOF:
         g_debug("received EOF");
@@ -1346,7 +1319,6 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
     s->command_state = ga_command_state_new();
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
-    json_message_parser_init(&s->parser, process_event, s, NULL);
     qmp_session_init(&s->session, &ga_commands, dispatch_return_cb);
 
 #ifndef _WIN32
@@ -1382,7 +1354,6 @@ static void cleanup_agent(GAState *s)
         qmp_session_destroy(&s->session);
         ga_command_state_cleanup_all(s->command_state);
         ga_command_state_free(s->command_state);
-        json_message_parser_destroy(&s->parser);
     }
     g_free(s->pstate_filepath);
     g_free(s->state_filepath_isfrozen);
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 06/20] monitor: use qmp session to parse json feed

[Marc-André Lureau]

Use the QmpSession json parser introduced in previous patch to
generalize the handling in both qemu & qemu-ga. Unfortunately, since
the introduction of OOB, it's not as common as it was before that. We
may want to move some of OOB logic in common qmp-dispatch.c/QmpSession
though.

The QEMU monitor has peculiar handling of the stream of commands, for
OOB command processing, which can be solved by overriding the json
emit callback.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/dispatch.h    |  1 +
 include/qapi/qmp/json-parser.h |  7 ++++---
 monitor/monitor-internal.h     |  1 -
 monitor/qmp.c                  | 13 +++++--------
 qapi/qmp-dispatch.c            |  4 +++-
 qga/main.c                     |  2 +-
 qobject/json-streamer.c        |  3 +--
 tests/test-qmp-cmds.c          | 11 ++++++-----
 8 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index c84edff7d2..b3ca6c9ff2 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -53,6 +53,7 @@ const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
                                    const char *name);
 void qmp_session_init(QmpSession *session,
                       const QmpCommandList *cmds,
+                      JSONMessageEmit *emit,
                       QmpDispatchReturn *return_cb);
 static inline void
 qmp_session_feed(QmpSession *session, const char *buf, size_t count)
diff --git a/include/qapi/qmp/json-parser.h b/include/qapi/qmp/json-parser.h
index 7345a9bd5c..6f168e8007 100644
--- a/include/qapi/qmp/json-parser.h
+++ b/include/qapi/qmp/json-parser.h
@@ -14,6 +14,8 @@
 #ifndef QAPI_QMP_JSON_PARSER_H
 #define QAPI_QMP_JSON_PARSER_H
 
+typedef void (JSONMessageEmit)(void *opaque, QObject *json, Error *err);
+
 typedef struct JSONLexer {
     int start_state, state;
     GString *token;
@@ -21,7 +23,7 @@ typedef struct JSONLexer {
 } JSONLexer;
 
 typedef struct JSONMessageParser {
-    void (*emit)(void *opaque, QObject *json, Error *err);
+    JSONMessageEmit *emit;
     void *opaque;
     va_list *ap;
     JSONLexer lexer;
@@ -32,8 +34,7 @@ typedef struct JSONMessageParser {
 } JSONMessageParser;
 
 void json_message_parser_init(JSONMessageParser *parser,
-                              void (*emit)(void *opaque, QObject *json,
-                                           Error *err),
+                              JSONMessageEmit *emit,
                               void *opaque, va_list *ap);
 
 void json_message_parser_feed(JSONMessageParser *parser,
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 65d587eafb..65cf668b20 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -125,7 +125,6 @@ struct MonitorHMP {
 
 typedef struct {
     Monitor common;
-    JSONMessageParser parser;
     bool pretty;
     /*
      * When a client connects, we're in capabilities negotiation mode.
diff --git a/monitor/qmp.c b/monitor/qmp.c
index b215cb70f3..cd29494e28 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -217,7 +217,7 @@ void monitor_qmp_bh_dispatcher(void *data)
 
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 {
-    MonitorQMP *mon = opaque;
+    MonitorQMP *mon = container_of(opaque, MonitorQMP, session);
     QObject *id = NULL;
     QDict *qdict;
     QMPRequest *req_obj;
@@ -279,7 +279,7 @@ static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
     MonitorQMP *mon = opaque;
 
-    json_message_parser_feed(&mon->parser, (const char *) buf, size);
+    qmp_session_feed(&mon->session, (const char *) buf, size);
 }
 
 static QDict *qmp_greeting(MonitorQMP *mon)
@@ -309,7 +309,9 @@ static void monitor_qmp_event(void *opaque, int event)
     switch (event) {
     case CHR_EVENT_OPENED:
         qmp_session_init(&mon->session,
-                         &qmp_cap_negotiation_commands, dispatch_return_cb);
+                         &qmp_cap_negotiation_commands,
+                         handle_qmp_command,
+                         dispatch_return_cb);
         monitor_qmp_caps_reset(mon);
         data = qmp_greeting(mon);
         qmp_send_response(mon, data);
@@ -325,9 +327,6 @@ static void monitor_qmp_event(void *opaque, int event)
          */
         monitor_qmp_cleanup_queues(mon);
         qmp_session_destroy(&mon->session);
-        json_message_parser_destroy(&mon->parser);
-        json_message_parser_init(&mon->parser, handle_qmp_command,
-                                 mon, NULL);
         mon_refcount--;
         monitor_fdsets_cleanup();
         break;
@@ -337,7 +336,6 @@ static void monitor_qmp_event(void *opaque, int event)
 void monitor_data_destroy_qmp(MonitorQMP *mon)
 {
     qmp_session_destroy(&mon->session);
-    json_message_parser_destroy(&mon->parser);
     qemu_mutex_destroy(&mon->qmp_queue_lock);
     monitor_qmp_cleanup_req_queue_locked(mon);
     g_queue_free(mon->qmp_requests);
@@ -373,7 +371,6 @@ void monitor_init_qmp(Chardev *chr, bool pretty)
     qemu_chr_fe_init(&mon->common.chr, chr, &error_abort);
     qemu_chr_fe_set_echo(&mon->common.chr, true);
 
-    json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL);
     if (mon->common.use_io_thread) {
         /*
          * Make sure the old iowatch is gone.  It's possible when
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 803ec626cd..f2c376d005 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -182,12 +182,14 @@ static void qmp_json_emit(void *opaque, QObject *obj, Error *err)
 
 void qmp_session_init(QmpSession *session,
                       const QmpCommandList *cmds,
+                      JSONMessageEmit *emit,
                       QmpDispatchReturn *return_cb)
 {
     assert(return_cb);
     assert(!session->return_cb);
 
-    json_message_parser_init(&session->parser, qmp_json_emit, session, NULL);
+    json_message_parser_init(&session->parser, emit ?: qmp_json_emit,
+                             session, NULL);
     session->cmds = cmds;
     session->return_cb = return_cb;
 }
diff --git a/qga/main.c b/qga/main.c
index 057368eb16..b005550c70 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1319,7 +1319,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
     s->command_state = ga_command_state_new();
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
-    qmp_session_init(&s->session, &ga_commands, dispatch_return_cb);
+    qmp_session_init(&s->session, &ga_commands, NULL, dispatch_return_cb);
 
 #ifndef _WIN32
     if (!register_signal_handlers()) {
diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index 47dd7ea576..2a440f2a9e 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -100,8 +100,7 @@ out_emit:
 }
 
 void json_message_parser_init(JSONMessageParser *parser,
-                              void (*emit)(void *opaque, QObject *json,
-                                           Error *err),
+                              JSONMessageEmit *emit,
                               void *opaque, va_list *ap)
 {
     parser->emit = emit;
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 7b3bccc091..8e46f88f6f 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -127,7 +127,7 @@ static void test_dispatch_cmd(void)
     QmpSession session = { 0, };
     QDict *req = qdict_new();
 
-    qmp_session_init(&session, &qmp_commands, dispatch_cmd_return);
+    qmp_session_init(&session, &qmp_commands, NULL, dispatch_cmd_return);
     qdict_put_str(req, "execute", "user_def_cmd");
 
     qmp_dispatch(&session, QOBJECT(req), false);
@@ -141,7 +141,7 @@ static void test_dispatch_cmd_oob(void)
     QmpSession session = { 0, };
     QDict *req = qdict_new();
 
-    qmp_session_init(&session, &qmp_commands, dispatch_cmd_return);
+    qmp_session_init(&session, &qmp_commands, NULL, dispatch_cmd_return);
     qdict_put_str(req, "exec-oob", "test-flags-command");
 
     qmp_dispatch(&session, QOBJECT(req), true);
@@ -163,7 +163,8 @@ static void test_dispatch_cmd_failure(void)
     QDict *req = qdict_new();
     QDict *args = qdict_new();
 
-    qmp_session_init(&session, &qmp_commands, dispatch_cmd_failure_return);
+    qmp_session_init(&session, &qmp_commands, NULL,
+                     dispatch_cmd_failure_return);
     qdict_put_str(req, "execute", "user_def_cmd2");
 
     qmp_dispatch(&session, QOBJECT(req), false);
@@ -189,7 +190,7 @@ static void test_dispatch_cmd_success_response(void)
     QmpSession session = { 0, };
     QDict *req = qdict_new();
 
-    qmp_session_init(&session, &qmp_commands, (QmpDispatchReturn *)abort);
+    qmp_session_init(&session, &qmp_commands, NULL, (QmpDispatchReturn *)abort);
     qdict_put_str(req, "execute", "cmd-success-response");
 
     qmp_dispatch(&session, QOBJECT(req), false);
@@ -210,7 +211,7 @@ static QObject *test_qmp_dispatch(QDict *req)
     QmpSession session = { 0, };
     QObject *ret;
 
-    qmp_session_init(&session, &qmp_commands, dispatch_return);
+    qmp_session_init(&session, &qmp_commands, NULL, dispatch_return);
     qmp_dispatch(&session, QOBJECT(req), false);
     ret = dispatch_ret;
     dispatch_ret = NULL;
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 07/20] qga: simplify dispatch_return_cb

[Marc-André Lureau]

Fold send_response().

qobject_to_json() can't return NULL (it will crash if allocation
failed, either in memcpy() or abort from g_realloc()).

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

diff --git a/qga/main.c b/qga/main.c
index b005550c70..66fe7ac3de 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -522,8 +522,9 @@ fail:
 #endif
 }
 
-static int send_response(GAState *s, const QDict *rsp)
+static void dispatch_return_cb(QmpSession *session, QDict *rsp)
 {
+    GAState *s = container_of(session, GAState, session);
     const char *buf;
     QString *payload_qstr, *response_qstr;
     GIOStatus status;
@@ -531,9 +532,6 @@ static int send_response(GAState *s, const QDict *rsp)
     g_assert(rsp && s->channel);
 
     payload_qstr = qobject_to_json(QOBJECT(rsp));
-    if (!payload_qstr) {
-        return -EINVAL;
-    }
 
     if (s->delimit_response) {
         s->delimit_response = false;
@@ -550,18 +548,7 @@ static int send_response(GAState *s, const QDict *rsp)
     status = ga_channel_write_all(s->channel, buf, strlen(buf));
     qobject_unref(response_qstr);
     if (status != G_IO_STATUS_NORMAL) {
-        return -EIO;
-    }
-
-    return 0;
-}
-
-static void dispatch_return_cb(QmpSession *session, QDict *rsp)
-{
-    GAState *s = container_of(session, GAState, session);
-    int ret = send_response(s, rsp);
-    if (ret < 0) {
-        g_warning("error sending response: %s", strerror(-ret));
+        g_warning("Failed sending response");
     }
 }
 
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 08/20] QmpSession: introduce QmpReturn

[Marc-André Lureau]

QmpReturn (and associated functions) is used during synchronous
dispatch return for now. It helps to factor out some code for
handling a reply context.

In the following patches, the QmpReturn will be the basis upon which
asynchronous reply will be handled: it will hold the context for a QMP
command reply.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/dispatch.h | 34 ++++++++++++++++-
 monitor/qmp.c               |  6 +--
 qapi/qmp-dispatch.c         | 74 ++++++++++++++++++++++---------------
 3 files changed, 79 insertions(+), 35 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index b3ca6c9ff2..6c0d21968e 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -17,6 +17,8 @@
 #include "qemu/queue.h"
 #include "qapi/qmp/json-parser.h"
 
+typedef struct QmpReturn QmpReturn;
+
 typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
 
 typedef enum QmpCommandOptions
@@ -47,6 +49,37 @@ struct QmpSession {
     QmpDispatchReturn *return_cb;
 };
 
+struct QmpReturn {
+    QmpSession *session;
+    QDict *rsp;
+};
+
+/**
+ * qmp_return_new:
+ *
+ * Allocates and initializes a QmpReturn.
+ */
+QmpReturn *qmp_return_new(QmpSession *session, const QObject *req);
+
+/**
+ * qmp_return_free:
+ *
+ * Free a QmpReturn. This shouldn't be needed if you actually return
+ * with qmp_return{_error}.
+ */
+void qmp_return_free(QmpReturn *qret);
+
+/**
+ * qmp_return{_error}:
+ *
+ * Construct the command reply, and call the
+ * return_cb() associated with the session.
+ *
+ * Finally, free the QmpReturn.
+ */
+void qmp_return(QmpReturn *qret, QObject *rsp);
+void qmp_return_error(QmpReturn *qret, Error *err);
+
 void qmp_register_command(QmpCommandList *cmds, const char *name,
                           QmpCommandFunc *fn, QmpCommandOptions options);
 const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
@@ -67,7 +100,6 @@ void qmp_enable_command(QmpCommandList *cmds, const char *name);
 bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
-QDict *qmp_error_response(Error *err);
 void qmp_dispatch(QmpSession *session, QObject *request,
                   bool allow_oob);
 bool qmp_is_oob(const QDict *dict);
diff --git a/monitor/qmp.c b/monitor/qmp.c
index cd29494e28..056ad7b68b 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -179,7 +179,6 @@ static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void)
 void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
-    QDict *rsp;
     bool need_resume;
     MonitorQMP *mon;
 
@@ -198,11 +197,10 @@ void monitor_qmp_bh_dispatcher(void *data)
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
         monitor_qmp_dispatch(mon, req_obj->req);
     } else {
+        QmpSession *session = &req_obj->mon->session;
         assert(req_obj->err);
-        rsp = qmp_error_response(req_obj->err);
+        qmp_return_error(qmp_return_new(session, req_obj->req), req_obj->err);
         req_obj->err = NULL;
-        qmp_send_response(req_obj->mon, rsp);
-        qobject_unref(rsp);
     }
 
     if (need_resume) {
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index f2c376d005..405cb291b1 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -19,6 +19,46 @@
 #include "qapi/qmp/qbool.h"
 #include "sysemu/sysemu.h"
 
+QmpReturn *qmp_return_new(QmpSession *session, const QObject *request)
+{
+    QmpReturn *qret = g_new0(QmpReturn, 1);
+    const QDict *req = qobject_to(QDict, request);
+    QObject *id = req ? qdict_get(req, "id") : NULL;
+
+    qret->session = session;
+    qret->rsp = qdict_new();
+    if (id) {
+        qobject_ref(id);
+        qdict_put_obj(qret->rsp, "id", id);
+    }
+
+    return qret;
+}
+
+void qmp_return_free(QmpReturn *qret)
+{
+    qobject_unref(qret->rsp);
+    g_free(qret);
+}
+
+void qmp_return(QmpReturn *qret, QObject *rsp)
+{
+    qdict_put_obj(qret->rsp, "return", rsp ?: QOBJECT(qdict_new()));
+    qret->session->return_cb(qret->session, qret->rsp);
+    qmp_return_free(qret);
+}
+
+void qmp_return_error(QmpReturn *qret, Error *err)
+{
+    qdict_put_obj(qret->rsp, "error",
+                  qobject_from_jsonf_nofail("{ 'class': %s, 'desc': %s }",
+                      QapiErrorClass_str(error_get_class(err)),
+                      error_get_pretty(err)));
+    error_free(err);
+    qret->session->return_cb(qret->session, qret->rsp);
+    qmp_return_free(qret);
+}
+
 static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
                                      Error **errp)
 {
@@ -143,17 +183,6 @@ static QObject *do_qmp_dispatch(const QmpCommandList *cmds, QObject *request,
     return ret;
 }
 
-QDict *qmp_error_response(Error *err)
-{
-    QDict *rsp;
-
-    rsp = qdict_from_jsonf_nofail("{ 'error': { 'class': %s, 'desc': %s } }",
-                                  QapiErrorClass_str(error_get_class(err)),
-                                  error_get_pretty(err));
-    error_free(err);
-    return rsp;
-}
-
 /*
  * Does @qdict look like a command to be run out-of-band?
  */
@@ -170,9 +199,7 @@ static void qmp_json_emit(void *opaque, QObject *obj, Error *err)
     assert(!obj != !err);
 
     if (err) {
-        QDict *rsp = qmp_error_response(err);
-        session->return_cb(session, rsp);
-        qobject_unref(rsp);
+        qmp_return_error(qmp_return_new(session, obj), err);
     } else {
         qmp_dispatch(session, obj, false);
     }
@@ -208,25 +235,12 @@ void qmp_session_destroy(QmpSession *session)
 void qmp_dispatch(QmpSession *session, QObject *request, bool allow_oob)
 {
     Error *err = NULL;
-    QDict *dict = qobject_to(QDict, request);
-    QObject *ret, *id = dict ? qdict_get(dict, "id") : NULL;
-    QDict *rsp;
+    QObject *ret;
 
     ret = do_qmp_dispatch(session->cmds, request, allow_oob, &err);
     if (err) {
-        rsp = qmp_error_response(err);
+        qmp_return_error(qmp_return_new(session, request), err);
     } else if (ret) {
-        rsp = qdict_new();
-        qdict_put_obj(rsp, "return", ret);
-    } else {
-        /* Can only happen for commands with QCO_NO_SUCCESS_RESP */
-        return;
+        qmp_return(qmp_return_new(session, request), ret);
     }
-
-    if (id) {
-        qdict_put_obj(rsp, "id", qobject_ref(id));
-    }
-
-    session->return_cb(session, rsp);
-    qobject_unref(rsp);
 }
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 09/20] qmp: simplify qmp_return_error()

[Marc-André Lureau]

It's simple, probably more efficient, to hand-craft the dict.

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

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 405cb291b1..5f75dc27bd 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -50,10 +50,10 @@ void qmp_return(QmpReturn *qret, QObject *rsp)
 
 void qmp_return_error(QmpReturn *qret, Error *err)
 {
-    qdict_put_obj(qret->rsp, "error",
-                  qobject_from_jsonf_nofail("{ 'class': %s, 'desc': %s }",
-                      QapiErrorClass_str(error_get_class(err)),
-                      error_get_pretty(err)));
+    QDict *qdict = qdict_new();
+    qdict_put_str(qdict, "class", QapiErrorClass_str(error_get_class(err)));
+    qdict_put_str(qdict, "desc", error_get_pretty(err));
+    qdict_put_obj(qret->rsp, "error", QOBJECT(qdict));
     error_free(err);
     qret->session->return_cb(qret->session, qret->rsp);
     qmp_return_free(qret);
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 10/20] QmpSession: keep a queue of pending commands

[Marc-André Lureau]

The following commit will introduce asynchronous commands. Let's keep
the session aware of the pending commands, so we can do interesting
things like order the replies, or cancel pending operations when the
client is gone.

The queue needs a lock, since QmpReturn may be called from any thread.

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

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 6c0d21968e..7c9de9780d 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -16,6 +16,7 @@
 
 #include "qemu/queue.h"
 #include "qapi/qmp/json-parser.h"
+#include "qemu/thread.h"
 
 typedef struct QmpReturn QmpReturn;
 
@@ -47,11 +48,14 @@ struct QmpSession {
     const QmpCommandList *cmds;
     JSONMessageParser parser;
     QmpDispatchReturn *return_cb;
+    QemuMutex pending_lock;
+    QTAILQ_HEAD(, QmpReturn) pending;
 };
 
 struct QmpReturn {
     QmpSession *session;
     QDict *rsp;
+    QTAILQ_ENTRY(QmpReturn) entry;
 };
 
 /**
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 5f75dc27bd..4699a6715b 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -32,11 +32,24 @@ QmpReturn *qmp_return_new(QmpSession *session, const QObject *request)
         qdict_put_obj(qret->rsp, "id", id);
     }
 
+    qemu_mutex_lock(&session->pending_lock);
+    QTAILQ_INSERT_TAIL(&session->pending, qret, entry);
+    qemu_mutex_unlock(&session->pending_lock);
+
     return qret;
 }
 
 void qmp_return_free(QmpReturn *qret)
 {
+    QmpSession *session = qret->session;
+
+    if (session) {
+        qemu_mutex_lock(&session->pending_lock);
+    }
+    QTAILQ_REMOVE(&session->pending, qret, entry);
+    if (session) {
+        qemu_mutex_unlock(&session->pending_lock);
+    }
     qobject_unref(qret->rsp);
     g_free(qret);
 }
@@ -44,7 +57,9 @@ void qmp_return_free(QmpReturn *qret)
 void qmp_return(QmpReturn *qret, QObject *rsp)
 {
     qdict_put_obj(qret->rsp, "return", rsp ?: QOBJECT(qdict_new()));
-    qret->session->return_cb(qret->session, qret->rsp);
+    if (qret->session) {
+        qret->session->return_cb(qret->session, qret->rsp);
+    }
     qmp_return_free(qret);
 }
 
@@ -55,7 +70,9 @@ void qmp_return_error(QmpReturn *qret, Error *err)
     qdict_put_str(qdict, "desc", error_get_pretty(err));
     qdict_put_obj(qret->rsp, "error", QOBJECT(qdict));
     error_free(err);
-    qret->session->return_cb(qret->session, qret->rsp);
+    if (qret->session) {
+        qret->session->return_cb(qret->session, qret->rsp);
+    }
     qmp_return_free(qret);
 }
 
@@ -219,17 +236,28 @@ void qmp_session_init(QmpSession *session,
                              session, NULL);
     session->cmds = cmds;
     session->return_cb = return_cb;
+    qemu_mutex_init(&session->pending_lock);
+    QTAILQ_INIT(&session->pending);
 }
 
 void qmp_session_destroy(QmpSession *session)
 {
+    QmpReturn *ret, *next;
+
     if (!session->return_cb) {
         return;
     }
 
+    qemu_mutex_lock(&session->pending_lock);
+    QTAILQ_FOREACH_SAFE(ret, &session->pending, entry, next) {
+        ret->session = NULL;
+        QTAILQ_REMOVE(&session->pending, ret, entry);
+    }
+    qemu_mutex_unlock(&session->pending_lock);
     session->cmds = NULL;
     session->return_cb = NULL;
     json_message_parser_destroy(&session->parser);
+    qemu_mutex_destroy(&session->pending_lock);
 }
 
 void qmp_dispatch(QmpSession *session, QObject *request, bool allow_oob)
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 11/20] QmpSession: return orderly

[Marc-André Lureau]

QEMU will gain support for asynchronous commands, and may thus finish
commands in various order. However, the clients expect replies in
order. Let's enforce ordering of replies in QmpReturn: starting from
the older command, process each pending QmpReturn, and return until
reaching one that is unfinished.

Or if the command is OOB, it should return immediately.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/dispatch.h |  2 ++
 qapi/qmp-dispatch.c         | 61 ++++++++++++++++++++++++++++++-------
 tests/test-qmp-cmds.c       | 33 ++++++++++++++++++++
 3 files changed, 85 insertions(+), 11 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 7c9de9780d..92d6fd1afb 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -55,6 +55,8 @@ struct QmpSession {
 struct QmpReturn {
     QmpSession *session;
     QDict *rsp;
+    bool oob;
+    bool finished;
     QTAILQ_ENTRY(QmpReturn) entry;
 };
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 4699a6715b..546a6c9f7b 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -25,6 +25,7 @@ QmpReturn *qmp_return_new(QmpSession *session, const QObject *request)
     const QDict *req = qobject_to(QDict, request);
     QObject *id = req ? qdict_get(req, "id") : NULL;
 
+    qret->oob = req ? qmp_is_oob(req) : false;
     qret->session = session;
     qret->rsp = qdict_new();
     if (id) {
@@ -39,6 +40,15 @@ QmpReturn *qmp_return_new(QmpSession *session, const QObject *request)
     return qret;
 }
 
+static void qmp_return_free_with_lock(QmpReturn *qret)
+{
+    if (qret->session) {
+        QTAILQ_REMOVE(&qret->session->pending, qret, entry);
+    }
+    qobject_unref(qret->rsp);
+    g_free(qret);
+}
+
 void qmp_return_free(QmpReturn *qret)
 {
     QmpSession *session = qret->session;
@@ -46,21 +56,53 @@ void qmp_return_free(QmpReturn *qret)
     if (session) {
         qemu_mutex_lock(&session->pending_lock);
     }
-    QTAILQ_REMOVE(&session->pending, qret, entry);
+
+    qmp_return_free_with_lock(qret);
+
     if (session) {
         qemu_mutex_unlock(&session->pending_lock);
     }
-    qobject_unref(qret->rsp);
-    g_free(qret);
+}
+
+static void qmp_return_orderly(QmpReturn *qret)
+{
+    QmpSession *session = qret->session;
+    QmpReturn *ret, *next;
+
+    if (!session) {
+        /* the session was destroyed before return, discard */
+        qmp_return_free(qret);
+        return;
+    }
+    if (qret->oob) {
+        session->return_cb(session, qret->rsp);
+        qmp_return_free(qret);
+        return;
+    }
+
+    qret->finished = true;
+
+    qemu_mutex_lock(&session->pending_lock);
+    /*
+     * Process the list of pending and call return_cb until reaching
+     * an unfinished.
+     */
+    QTAILQ_FOREACH_SAFE(ret, &session->pending, entry, next) {
+        if (!ret->finished) {
+            break;
+        }
+        session->return_cb(session, ret->rsp);
+        ret->session = session;
+        qmp_return_free_with_lock(ret);
+    }
+
+    qemu_mutex_unlock(&session->pending_lock);
 }
 
 void qmp_return(QmpReturn *qret, QObject *rsp)
 {
     qdict_put_obj(qret->rsp, "return", rsp ?: QOBJECT(qdict_new()));
-    if (qret->session) {
-        qret->session->return_cb(qret->session, qret->rsp);
-    }
-    qmp_return_free(qret);
+    qmp_return_orderly(qret);
 }
 
 void qmp_return_error(QmpReturn *qret, Error *err)
@@ -70,10 +112,7 @@ void qmp_return_error(QmpReturn *qret, Error *err)
     qdict_put_str(qdict, "desc", error_get_pretty(err));
     qdict_put_obj(qret->rsp, "error", QOBJECT(qdict));
     error_free(err);
-    if (qret->session) {
-        qret->session->return_cb(qret->session, qret->rsp);
-    }
-    qmp_return_free(qret);
+    qmp_return_orderly(qret);
 }
 
 static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 8e46f88f6f..ece8726e96 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -333,6 +333,38 @@ static void test_dealloc_partial(void)
     qapi_free_UserDefTwo(ud2);
 }
 
+typedef struct QmpReturnOrderly {
+    QmpSession session;
+    int returns;
+} QmpReturnOrderly;
+
+static void dispatch_return_orderly(QmpSession *session, QDict *resp)
+{
+    QmpReturnOrderly *o = container_of(session, QmpReturnOrderly, session);
+
+    o->returns++;
+}
+
+static void test_qmp_return_orderly(void)
+{
+    QDict *dict = qdict_new();
+    QmpReturnOrderly o = { { 0 }, };
+    QmpReturn *r1, *r2, *r3;
+
+    qmp_session_init(&o.session, &qmp_commands, NULL, dispatch_return_orderly);
+    r1 = qmp_return_new(&o.session, NULL);
+    qdict_put_str(dict, "exec-oob", "test");
+    r2 = qmp_return_new(&o.session, QOBJECT(dict));
+    r3 = qmp_return_new(&o.session, NULL);
+    qmp_return(r3, NULL);
+    g_assert_cmpint(o.returns, ==, 0);
+    qmp_return(r2, NULL);
+    g_assert_cmpint(o.returns, ==, 1);
+    qmp_return(r1, NULL);
+    g_assert_cmpint(o.returns, ==, 3);
+    qmp_session_destroy(&o.session);
+    qobject_unref(dict);
+}
 
 int main(int argc, char **argv)
 {
@@ -346,6 +378,7 @@ int main(int argc, char **argv)
                     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);
+    g_test_add_func("/qmp/return_orderly", test_qmp_return_orderly);
 
     test_qmp_init_marshal(&qmp_commands);
     g_test_run();
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 12/20] qmp: introduce asynchronous command type

[Marc-André Lureau]

Add a new type of command, QmpCommandFuncAsync: those commands can
return later thanks to QmpReturn. This commit introduces the new type
and register function and teach qmp_dipatch() to call it without
qmp_return(). The async_fn callback will be responsible for calling
qmp_return(), either synchronously or asynchronously.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/dispatch.h | 10 +++++++++-
 qapi/qmp-dispatch.c         | 27 ++++++++++++++++-----------
 qapi/qmp-registry.c         | 27 ++++++++++++++++++++++++---
 3 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 92d6fd1afb..6aef0abc70 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -21,6 +21,7 @@
 typedef struct QmpReturn QmpReturn;
 
 typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
+typedef void (QmpCommandAsyncFunc)(QDict *, QmpReturn *);
 
 typedef enum QmpCommandOptions
 {
@@ -28,12 +29,16 @@ typedef enum QmpCommandOptions
     QCO_NO_SUCCESS_RESP       =  (1U << 0),
     QCO_ALLOW_OOB             =  (1U << 1),
     QCO_ALLOW_PRECONFIG       =  (1U << 2),
+    QCO_ASYNC                 =  (1U << 3),
 } QmpCommandOptions;
 
 typedef struct QmpCommand
 {
     const char *name;
-    QmpCommandFunc *fn;
+    union {
+        QmpCommandFunc *fn;
+        QmpCommandAsyncFunc *async_fn;
+    };
     QmpCommandOptions options;
     QTAILQ_ENTRY(QmpCommand) node;
     bool enabled;
@@ -88,6 +93,9 @@ void qmp_return_error(QmpReturn *qret, Error *err);
 
 void qmp_register_command(QmpCommandList *cmds, const char *name,
                           QmpCommandFunc *fn, QmpCommandOptions options);
+void qmp_register_async_command(QmpCommandList *cmds, const char *name,
+                                QmpCommandAsyncFunc *fn,
+                                QmpCommandOptions options);
 const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
                                    const char *name);
 void qmp_session_init(QmpSession *session,
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 546a6c9f7b..1f493af67a 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -171,7 +171,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
     return dict;
 }
 
-static QObject *do_qmp_dispatch(const QmpCommandList *cmds, QObject *request,
+static QObject *do_qmp_dispatch(QmpSession *session, QObject *request,
                                 bool allow_oob, Error **errp)
 {
     Error *local_err = NULL;
@@ -193,7 +193,7 @@ static QObject *do_qmp_dispatch(const QmpCommandList *cmds, QObject *request,
         command = qdict_get_str(dict, "exec-oob");
         oob = true;
     }
-    cmd = qmp_find_command(cmds, command);
+    cmd = qmp_find_command(session->cmds, command);
     if (cmd == NULL) {
         error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
                   "The command %s has not been found", command);
@@ -224,14 +224,19 @@ static QObject *do_qmp_dispatch(const QmpCommandList *cmds, QObject *request,
         qobject_ref(args);
     }
 
-    cmd->fn(args, &ret, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    } else if (cmd->options & QCO_NO_SUCCESS_RESP) {
-        g_assert(!ret);
-    } else if (!ret) {
-        /* TODO turn into assertion */
-        ret = QOBJECT(qdict_new());
+
+    if (cmd->options & QCO_ASYNC) {
+        cmd->async_fn(args, qmp_return_new(session, request));
+    } else {
+        cmd->fn(args, &ret, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+        } else if (cmd->options & QCO_NO_SUCCESS_RESP) {
+            g_assert(!ret);
+        } else if (!ret) {
+            /* TODO turn into assertion */
+            ret = QOBJECT(qdict_new());
+        }
     }
 
     qobject_unref(args);
@@ -304,7 +309,7 @@ void qmp_dispatch(QmpSession *session, QObject *request, bool allow_oob)
     Error *err = NULL;
     QObject *ret;
 
-    ret = do_qmp_dispatch(session->cmds, request, allow_oob, &err);
+    ret = do_qmp_dispatch(session, request, allow_oob, &err);
     if (err) {
         qmp_return_error(qmp_return_new(session, request), err);
     } else if (ret) {
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index d0f9a1d3e3..0f3d521ce5 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -15,16 +15,37 @@
 #include "qemu/osdep.h"
 #include "qapi/qmp/dispatch.h"
 
-void qmp_register_command(QmpCommandList *cmds, const char *name,
-                          QmpCommandFunc *fn, QmpCommandOptions options)
+
+static QmpCommand *qmp_command_new(QmpCommandList *cmds, const char *name,
+                                   QmpCommandOptions options)
 {
     QmpCommand *cmd = g_malloc0(sizeof(*cmd));
 
     cmd->name = name;
-    cmd->fn = fn;
     cmd->enabled = true;
     cmd->options = options;
     QTAILQ_INSERT_TAIL(cmds, cmd, node);
+
+    return cmd;
+}
+
+
+void qmp_register_command(QmpCommandList *cmds, const char *name,
+                          QmpCommandFunc *fn, QmpCommandOptions options)
+{
+    QmpCommand *cmd = qmp_command_new(cmds, name, options);
+
+    assert(!(options & QCO_ASYNC));
+    cmd->fn = fn;
+}
+
+void qmp_register_async_command(QmpCommandList *cmds, const char *name,
+                            QmpCommandAsyncFunc *fn, QmpCommandOptions options)
+{
+    QmpCommand *cmd = qmp_command_new(cmds, name, options);
+
+    assert(options & QCO_ASYNC);
+    cmd->async_fn = fn;
 }
 
 const QmpCommand *qmp_find_command(const QmpCommandList *cmds, const char *name)
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 13/20] scripts: learn 'async' qapi commands

[Marc-André Lureau]

Commands with the 'async' key will be registered as async type (see
related commit), and will allow a synchronous (in scope callback) or
asynchronous return (out-of-scope when ready, in idle etc) by keeping
the given QmpReturn and calling qmp_return function later.

Ex:
  { 'command': 'foo-async,
    'data': {'arg': 'str'},
    'returns': 'Foo',
    'async': true }

generates the following marshaller:

void qmp_marshal_foo_async(QDict *args, QmpReturn *qret)
{
    Error *err = NULL;
    Visitor *v;
    q_obj_foo_async_arg arg = {0};

    v = qmp_input_visitor_new(QOBJECT(args), true);
    visit_start_struct(v, NULL, NULL, 0, &err);
    if (err) {
        goto out;
    }
    visit_type_q_obj_foo_async_arg_members(v, &arg, &err);
    if (!err) {
        visit_check_struct(v, &err);
    }
    visit_end_struct(v, NULL);
    if (err) {
        goto out;
    }

    qmp_foo_async(arg.arg, qret);

out:
    if (err) {
        qmp_return_error(qret, err);
    }
    visit_free(v);
    v = qapi_dealloc_visitor_new();
    visit_start_struct(v, NULL, NULL, 0, NULL);
    visit_type_q_obj_foo_async_arg_members(v, &arg, NULL);
    visit_end_struct(v, NULL);
    visit_free(v);
}

and a return helper:

void qmp_foo_async_return(QmpReturn *qret, Foo *ret_in)
{
    Error *err = NULL;
    QObject *ret_out = NULL;

    qmp_marshal_output_Foo(ret_in, &ret_out, &err);

    if (err) {
        qmp_return_error(qret, err);
    } else {
        qmp_return(qret, ret_out);
    }
}

The dispatched function may call the return helper within the calling
scope or delay the return. To return an error, it can call
qmp_return_error() directly instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi/commands.py                | 151 ++++++++++++++++++++----
 scripts/qapi/common.py                  |  15 ++-
 scripts/qapi/doc.py                     |   3 +-
 scripts/qapi/introspect.py              |   3 +-
 tests/qapi-schema/qapi-schema-test.json |   5 +
 tests/qapi-schema/qapi-schema-test.out  |   8 ++
 tests/qapi-schema/test-qapi.py          |   8 +-
 tests/test-qmp-cmds.c                   |  60 ++++++++++
 8 files changed, 218 insertions(+), 35 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index b929e07be4..4c200c04ca 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -16,18 +16,36 @@ See the COPYING file in the top-level directory.
 from qapi.common import *
 
 
-def gen_command_decl(name, arg_type, boxed, ret_type):
-    return mcgen('''
-%(c_type)s qmp_%(c_name)s(%(params)s);
+def gen_command_decl(name, arg_type, boxed, ret_type, success_response, asyn):
+    if asyn:
+        extra = "QmpReturn *qret"
+    else:
+        extra = 'Error **errp'
+
+    if asyn:
+        ret = mcgen('''
+void qmp_%(name)s(%(params)s);
 ''',
-                 c_type=(ret_type and ret_type.c_type()) or 'void',
-                 c_name=c_name(name),
-                 params=build_params(arg_type, boxed, 'Error **errp'))
+                     name=c_name(name),
+                     params=build_params(arg_type, boxed, extra))
+        if success_response:
+            ret += mcgen('''
+void qmp_%(name)s_return(QmpReturn *qret%(c_type)s);
+''',
+                        c_type=(", " + ret_type.c_type() if ret_type else ""),
+                        name=c_name(name))
 
+        return ret
+    else:
+        return mcgen('''
+%(c_type)s qmp_%(c_name)s(%(params)s);
+''',
+                     c_type=(ret_type and ret_type.c_type()) or 'void',
+                     c_name=c_name(name),
+                     params=build_params(arg_type, boxed, extra))
 
-def gen_call(name, arg_type, boxed, ret_type):
-    ret = ''
 
+def gen_argstr(arg_type, boxed):
     argstr = ''
     if boxed:
         assert arg_type and not arg_type.is_empty()
@@ -39,6 +57,13 @@ def gen_call(name, arg_type, boxed, ret_type):
                 argstr += 'arg.has_%s, ' % c_name(memb.name)
             argstr += 'arg.%s, ' % c_name(memb.name)
 
+    return argstr
+
+
+def gen_call(name, arg_type, boxed, ret_type):
+    ret = ''
+
+    argstr = gen_argstr(arg_type, boxed)
     lhs = ''
     if ret_type:
         lhs = 'retval = '
@@ -60,6 +85,50 @@ def gen_call(name, arg_type, boxed, ret_type):
     return ret
 
 
+def gen_async_call(name, arg_type, boxed):
+    argstr = gen_argstr(arg_type, boxed)
+
+    push_indent()
+    ret = mcgen('''
+
+qmp_%(c_name)s(%(args)sqret);
+''',
+                c_name=c_name(name), args=argstr)
+
+    pop_indent()
+    return ret
+
+
+def gen_async_return(name, ret_type):
+    if ret_type:
+        return mcgen('''
+
+void qmp_%(c_name)s_return(QmpReturn *qret, %(ret_type)s ret_in)
+{
+    Error *err = NULL;
+    QObject *ret_out = NULL;
+
+    qmp_marshal_output_%(ret_c_name)s(ret_in, &ret_out, &err);
+
+    if (err) {
+        qmp_return_error(qret, err);
+    } else {
+        qmp_return(qret, ret_out);
+    }
+}
+''',
+                     c_name=c_name(name),
+                     ret_type=ret_type.c_type(), ret_c_name=ret_type.c_name())
+    else:
+        return mcgen('''
+
+void qmp_%(c_name)s_return(QmpReturn *qret)
+{
+    qmp_return(qret, QOBJECT(qdict_new()));
+}
+''',
+                     c_name=c_name(name))
+
 def gen_marshal_output(ret_type):
     return mcgen('''
 
@@ -83,19 +152,22 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
                  c_type=ret_type.c_type(), c_name=ret_type.c_name())
 
 
-def build_marshal_proto(name):
-    return ('void qmp_marshal_%s(QDict *args, QObject **ret, Error **errp)'
-            % c_name(name))
+def build_marshal_proto(name, asyn):
+    if asyn:
+        tmpl = 'void qmp_marshal_%s(QDict *args, QmpReturn *qret)'
+    else:
+        tmpl = 'void qmp_marshal_%s(QDict *args, QObject **ret, Error **errp)'
+    return tmpl % c_name(name)
 
 
-def gen_marshal_decl(name):
+def gen_marshal_decl(name, asyn):
     return mcgen('''
 %(proto)s;
 ''',
-                 proto=build_marshal_proto(name))
+                 proto=build_marshal_proto(name, asyn))
 
 
-def gen_marshal(name, arg_type, boxed, ret_type):
+def gen_marshal(name, arg_type, boxed, ret_type, asyn):
     have_args = arg_type and not arg_type.is_empty()
 
     ret = mcgen('''
@@ -104,9 +176,9 @@ def gen_marshal(name, arg_type, boxed, ret_type):
 {
     Error *err = NULL;
 ''',
-                proto=build_marshal_proto(name))
+                proto=build_marshal_proto(name, asyn))
 
-    if ret_type:
+    if ret_type and not asyn:
         ret += mcgen('''
     %(c_type)s retval;
 ''',
@@ -153,12 +225,28 @@ def gen_marshal(name, arg_type, boxed, ret_type):
     }
 ''')
 
-    ret += gen_call(name, arg_type, boxed, ret_type)
+    if asyn:
+        ret += gen_async_call(name, arg_type, boxed)
+    else:
+        ret += gen_call(name, arg_type, boxed, ret_type)
 
     ret += mcgen('''
 
 out:
+''')
+
+    if asyn:
+        ret += mcgen('''
+    if (err) {
+        qmp_return_error(qret, err);
+    }
+''')
+    else:
+        ret += mcgen('''
     error_propagate(errp, err);
+''')
+
+    ret += mcgen('''
     visit_free(v);
 ''')
 
@@ -193,7 +281,8 @@ out:
     return ret
 
 
-def gen_register_command(name, success_response, allow_oob, allow_preconfig):
+def gen_register_command(name, success_response, allow_oob, allow_preconfig,
+                         asyn):
     options = []
 
     if not success_response:
@@ -202,17 +291,24 @@ def gen_register_command(name, success_response, allow_oob, allow_preconfig):
         options += ['QCO_ALLOW_OOB']
     if allow_preconfig:
         options += ['QCO_ALLOW_PRECONFIG']
+    if asyn:
+        options += ['QCO_ASYNC']
 
     if not options:
         options = ['QCO_NO_OPTIONS']
 
     options = " | ".join(options)
 
+    if asyn:
+        regfn = 'qmp_register_async_command'
+    else:
+        regfn = 'qmp_register_command'
+
     ret = mcgen('''
-    qmp_register_command(cmds, "%(name)s",
+    %(regfn)s(cmds, "%(name)s",
                          qmp_marshal_%(c_name)s, %(opts)s);
 ''',
-                name=name, c_name=c_name(name),
+                regfn=regfn, name=name, c_name=c_name(name),
                 opts=options)
     return ret
 
@@ -276,7 +372,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
         genc.add(gen_registry(self._regy.get_content(), self._prefix))
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      asyn):
         if not gen:
             return
         # FIXME: If T is a user-defined type, the user is responsible
@@ -290,11 +387,15 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
                            self._genh, self._genc, self._regy):
                 self._genc.add(gen_marshal_output(ret_type))
         with ifcontext(ifcond, self._genh, self._genc, self._regy):
-            self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
-            self._genh.add(gen_marshal_decl(name))
-            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
+            self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type,
+                                            success_response, asyn))
+            self._genh.add(gen_marshal_decl(name, asyn))
+            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type, asyn))
+            if asyn and success_response:
+                self._genc.add(gen_async_return(name, ret_type))
             self._regy.add(gen_register_command(name, success_response,
-                                                allow_oob, allow_preconfig))
+                                                allow_oob, allow_preconfig,
+                                                asyn))
 
 
 def gen_commands(schema, output_dir, prefix):
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index d61bfdc526..1f698e91d1 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1138,7 +1138,7 @@ def check_exprs(exprs):
             meta = 'command'
             check_keys(expr_elem, 'command', [],
                        ['data', 'returns', 'gen', 'success-response',
-                        'boxed', 'allow-oob', 'allow-preconfig', 'if'])
+                        'boxed', 'allow-oob', 'allow-preconfig', 'if', 'async'])
             normalize_members(expr.get('data'))
         elif 'event' in expr:
             meta = 'event'
@@ -1283,7 +1283,8 @@ class QAPISchemaVisitor(object):
         pass
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      asyn):
         pass
 
     def visit_event(self, name, info, ifcond, arg_type, boxed):
@@ -1697,7 +1698,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
 
 class QAPISchemaCommand(QAPISchemaEntity):
     def __init__(self, name, info, doc, ifcond, arg_type, ret_type,
-                 gen, success_response, boxed, allow_oob, allow_preconfig):
+                 gen, success_response, boxed, allow_oob, allow_preconfig,
+                 asyn):
         QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -1710,6 +1712,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         self.boxed = boxed
         self.allow_oob = allow_oob
         self.allow_preconfig = allow_preconfig
+        self.asyn = asyn
 
     def check(self, schema):
         QAPISchemaEntity.check(self, schema)
@@ -1736,7 +1739,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
                               self.arg_type, self.ret_type,
                               self.gen, self.success_response,
                               self.boxed, self.allow_oob,
-                              self.allow_preconfig)
+                              self.allow_preconfig, self.asyn)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1989,6 +1992,7 @@ class QAPISchema(object):
         allow_oob = expr.get('allow-oob', False)
         allow_preconfig = expr.get('allow-preconfig', False)
         ifcond = expr.get('if')
+        asyn = expr.get('async', False)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, doc, ifcond, 'arg', self._make_members(data, info))
@@ -1997,7 +2001,8 @@ class QAPISchema(object):
             rets = self._make_array_type(rets[0], info)
         self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, rets,
                                            gen, success_response,
-                                           boxed, allow_oob, allow_preconfig))
+                                           boxed, allow_oob, allow_preconfig,
+                                           asyn))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 5fc0fc7e06..6fe280448a 100755
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -249,7 +249,8 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
                                body=texi_entity(doc, 'Members', ifcond)))
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      asyn):
         doc = self.cur_doc
         if boxed:
             body = texi_body(doc)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index f62cf0a2e1..e618e36d38 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -206,7 +206,8 @@ const QLitObject %(c_name)s = %(c_string)s;
                            for m in variants.variants]}, ifcond)
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      asyn):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         obj = {'arg-type': self._use_type(arg_type),
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index c6d59acc3e..593eefa10b 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -143,6 +143,11 @@
 
 { 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
 
+{ 'command': 'cmd-async', 'data': {'filename': 'str'},
+  'returns': 'Empty2', 'async': true }
+{ 'command': 'cmd-success-response-async', 'data': {'filename': 'str'},
+  'async': true, '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 85d510bc00..75aee842b9 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -208,6 +208,14 @@ 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_cmd-async-arg
+    member filename: str optional=False
+command cmd-async q_obj_cmd-async-arg -> Empty2
+   gen=True success_response=True boxed=False oob=False preconfig=False async=True
+object q_obj_cmd-success-response-async-arg
+    member filename: str optional=False
+command cmd-success-response-async q_obj_cmd-success-response-async-arg -> None
+   gen=True success_response=False boxed=False oob=False preconfig=False async=True
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index b0f770b9bd..c88cc91763 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -60,12 +60,14 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         self._print_if(ifcond)
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      asyn):
         print('command %s %s -> %s'
               % (name, arg_type and arg_type.name,
                  ret_type and ret_type.name))
-        print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
-              % (gen, success_response, boxed, allow_oob, allow_preconfig))
+        print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s%s'
+              % (gen, success_response, boxed, allow_oob, allow_preconfig,
+                 ' async=True' if asyn else ''))
         self._print_if(ifcond)
 
     def visit_event(self, name, info, ifcond, arg_type, boxed):
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index ece8726e96..f567ac2fb0 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -34,6 +34,28 @@ void qmp_cmd_success_response(Error **errp)
 {
 }
 
+static gboolean cmd_async_idle(gpointer user_data)
+{
+    QmpReturn *qret = user_data;
+
+    qmp_cmd_async_return(qret, g_new0(Empty2, 1));
+
+    return G_SOURCE_REMOVE;
+}
+
+void qmp_cmd_async(const char *filename, QmpReturn *qret)
+{
+    g_idle_add(cmd_async_idle, qret);
+}
+
+void qmp_cmd_success_response_async(const char *filename, QmpReturn *qret)
+{
+    Error *err = NULL;
+
+    error_setg(&err, "no response, but error ok");
+    qmp_return_error(qret, err);
+}
+
 Empty2 *qmp_user_def_cmd0(Error **errp)
 {
     return g_new0(Empty2, 1);
@@ -366,6 +388,43 @@ static void test_qmp_return_orderly(void)
     qobject_unref(dict);
 }
 
+typedef struct QmpReturnAsync {
+    QmpSession session;
+    GMainLoop *loop;
+} QmpReturnAsync;
+
+static void dispatch_return_async(QmpSession *session, QDict *resp)
+{
+    QmpReturnAsync *a = container_of(session, QmpReturnAsync, session);
+
+    g_main_loop_quit(a->loop);
+    g_main_loop_unref(a->loop);
+    a->loop = NULL;
+}
+
+static void test_qmp_return_async(void)
+{
+    QmpReturnAsync a = { { 0, } , };
+    QDict *args = qdict_new();
+    QDict *req = qdict_new();
+
+    a.loop = g_main_loop_new(NULL, TRUE);
+    qmp_session_init(&a.session, &qmp_commands,
+                    NULL, dispatch_return_async);
+
+    qdict_put_str(args, "filename", "test-filename");
+    qdict_put_str(req, "execute", "cmd-async");
+    qdict_put(req, "arguments", args);
+    qmp_dispatch(&a.session, QOBJECT(req), false);
+    g_assert(a.loop);
+
+    g_main_loop_run(a.loop);
+    g_assert(!a.loop);
+
+    qmp_session_destroy(&a.session);
+    qobject_unref(req);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -379,6 +438,7 @@ int main(int argc, char **argv)
     g_test_add_func("/qmp/dealloc_types", test_dealloc_types);
     g_test_add_func("/qmp/dealloc_partial", test_dealloc_partial);
     g_test_add_func("/qmp/return_orderly", test_qmp_return_orderly);
+    g_test_add_func("/qmp/return_async", test_qmp_return_async);
 
     test_qmp_init_marshal(&qmp_commands);
     g_test_run();
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 14/20] qmp: add qmp_return_is_cancelled()

[Marc-André Lureau]

If the client is gone, and the session finished, no need to
return. The async handler can use this information to avoid
unnecessary work and exit earlier.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/dispatch.h |  8 ++++++++
 qapi/qmp-dispatch.c         | 10 ++++++++++
 tests/test-qmp-cmds.c       | 39 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 6aef0abc70..6673902e95 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -91,6 +91,14 @@ void qmp_return_free(QmpReturn *qret);
 void qmp_return(QmpReturn *qret, QObject *rsp);
 void qmp_return_error(QmpReturn *qret, Error *err);
 
+/*
+ * @qmp_return_is_cancelled:
+ *
+ * Return true if the QmpReturn is cancelled, and free the QmpReturn
+ * in this case.
+ */
+bool qmp_return_is_cancelled(QmpReturn *qret);
+
 void qmp_register_command(QmpCommandList *cmds, const char *name,
                           QmpCommandFunc *fn, QmpCommandOptions options);
 void qmp_register_async_command(QmpCommandList *cmds, const char *name,
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 1f493af67a..8653c17901 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -64,6 +64,16 @@ void qmp_return_free(QmpReturn *qret)
     }
 }
 
+bool qmp_return_is_cancelled(QmpReturn *qret)
+{
+    if (!qret->session) {
+        qmp_return_free(qret);
+        return true;
+    }
+
+    return false;
+}
+
 static void qmp_return_orderly(QmpReturn *qret)
 {
     QmpSession *session = qret->session;
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index f567ac2fb0..d4c27f0be1 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -34,17 +34,29 @@ void qmp_cmd_success_response(Error **errp)
 {
 }
 
+static GMainLoop *loop;
+
 static gboolean cmd_async_idle(gpointer user_data)
 {
     QmpReturn *qret = user_data;
 
-    qmp_cmd_async_return(qret, g_new0(Empty2, 1));
+    if (!qret->session) {
+        g_assert(qmp_return_is_cancelled(qret));
+        g_main_loop_quit(loop);
+        g_main_loop_unref(loop);
+        loop = NULL;
+    } else {
+        qmp_cmd_async_return(qret, g_new0(Empty2, 1));
+    }
 
     return G_SOURCE_REMOVE;
 }
 
 void qmp_cmd_async(const char *filename, QmpReturn *qret)
 {
+    if (g_str_equal(filename, "cancel")) {
+        qmp_session_destroy(qret->session);
+    }
     g_idle_add(cmd_async_idle, qret);
 }
 
@@ -425,6 +437,30 @@ static void test_qmp_return_async(void)
     qobject_unref(req);
 }
 
+static void test_qmp_return_async_cancel(void)
+{
+    QmpReturnAsync a = { { 0, }, };
+    QDict *args = qdict_new();
+    QDict *req = qdict_new();
+
+    a.loop = g_main_loop_new(NULL, TRUE);
+    qmp_session_init(&a.session, &qmp_commands,
+                     NULL, dispatch_return_async);
+
+    qdict_put_str(args, "filename", "cancel");
+    qdict_put_str(req, "execute", "cmd-async");
+    qdict_put(req, "arguments", args);
+    qmp_dispatch(&a.session, QOBJECT(req), false);
+    g_assert(a.loop);
+
+    loop = a.loop;
+    g_main_loop_run(loop);
+    g_assert(!loop);
+
+    qmp_session_destroy(&a.session);
+    qobject_unref(req);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -439,6 +475,7 @@ int main(int argc, char **argv)
     g_test_add_func("/qmp/dealloc_partial", test_dealloc_partial);
     g_test_add_func("/qmp/return_orderly", test_qmp_return_orderly);
     g_test_add_func("/qmp/return_async", test_qmp_return_async);
+    g_test_add_func("/qmp/return_async_cancel", test_qmp_return_async_cancel);
 
     test_qmp_init_marshal(&qmp_commands);
     g_test_run();
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 15/20] monitor: add qmp_return_get_monitor()

[Marc-André Lureau]

If necessary, add an helper that can be used to retrieve the
associated monitor. This is useful for asynchronous commands that may
have to update cur_mon for various reasons.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/monitor/monitor.h |  3 +++
 monitor/qmp.c             | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index a81eeff5f8..6a2907a366 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -4,6 +4,7 @@
 #include "block/block.h"
 #include "qapi/qapi-types-misc.h"
 #include "qemu/readline.h"
+#include "qapi/qmp/dispatch.h"
 
 extern __thread Monitor *cur_mon;
 typedef struct MonitorHMP MonitorHMP;
@@ -43,4 +44,6 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
 void monitor_fdset_dup_fd_remove(int dup_fd);
 int64_t monitor_fdset_dup_fd_find(int dup_fd);
 
+Monitor *qmp_return_get_monitor(QmpReturn *qret);
+
 #endif /* MONITOR_H */
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 056ad7b68b..df8b9d8d4f 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -390,3 +390,14 @@ void monitor_init_qmp(Chardev *chr, bool pretty)
         monitor_list_append(&mon->common);
     }
 }
+
+Monitor *qmp_return_get_monitor(QmpReturn *qret)
+{
+    MonitorQMP *mon;
+
+    if (!qret->session) {
+        return NULL;
+    }
+    mon = container_of(qret->session, MonitorQMP, session);
+    return &mon->common;
+}
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 16/20] console: add graphic_hw_update_done()

[Marc-André Lureau]

Add a function to be called when a graphic update is done.

Declare the QXL renderer as async: render_update_cookie_num counts the
number of outstanding updates, and graphic_hw_update_done() is called
when it reaches none.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl-render.c | 9 +++++++--
 hw/display/qxl.c        | 1 +
 include/ui/console.h    | 2 ++
 ui/console.c            | 9 +++++++++
 4 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index 14ad2b352d..102fa0b7e9 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -108,7 +108,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
                                                 qxl->guest_primary.surface.mem,
                                                 MEMSLOT_GROUP_GUEST);
         if (!qxl->guest_primary.data) {
-            return;
+            goto end;
         }
         qxl_set_rect_to_surface(qxl, &qxl->dirty[0]);
         qxl->num_dirty_rects = 1;
@@ -136,7 +136,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
     }
 
     if (!qxl->guest_primary.data) {
-        return;
+        goto end;
     }
     for (i = 0; i < qxl->num_dirty_rects; i++) {
         if (qemu_spice_rect_is_empty(qxl->dirty+i)) {
@@ -157,6 +157,11 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
                        qxl->dirty[i].bottom - qxl->dirty[i].top);
     }
     qxl->num_dirty_rects = 0;
+
+end:
+    if (qxl->render_update_cookie_num == 0) {
+        graphic_hw_update_done(qxl->ssd.dcl.con);
+    }
 }
 
 /*
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 98c7410032..188399acd1 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1178,6 +1178,7 @@ static const QXLInterface qxl_interface = {
 
 static const GraphicHwOps qxl_ops = {
     .gfx_update  = qxl_hw_update,
+    .gfx_update_async = true,
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
diff --git a/include/ui/console.h b/include/ui/console.h
index f981696848..281f9c145b 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -365,6 +365,7 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
 typedef struct GraphicHwOps {
     void (*invalidate)(void *opaque);
     void (*gfx_update)(void *opaque);
+    bool gfx_update_async; /* if true, calls graphic_hw_update_done() */
     void (*text_update)(void *opaque, console_ch_t *text);
     void (*update_interval)(void *opaque, uint64_t interval);
     int (*ui_info)(void *opaque, uint32_t head, QemuUIInfo *info);
@@ -380,6 +381,7 @@ void graphic_console_set_hwops(QemuConsole *con,
 void graphic_console_close(QemuConsole *con);
 
 void graphic_hw_update(QemuConsole *con);
+void graphic_hw_update_done(QemuConsole *con);
 void graphic_hw_invalidate(QemuConsole *con);
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata);
 void graphic_hw_gl_block(QemuConsole *con, bool block);
diff --git a/ui/console.c b/ui/console.c
index 82d1ddac9c..3c941528d2 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -259,13 +259,22 @@ static void gui_setup_refresh(DisplayState *ds)
     ds->have_text = have_text;
 }
 
+void graphic_hw_update_done(QemuConsole *con)
+{
+}
+
 void graphic_hw_update(QemuConsole *con)
 {
+    bool async = false;
     if (!con) {
         con = active_console;
     }
     if (con && con->hw_ops->gfx_update) {
         con->hw_ops->gfx_update(con->hw);
+        async = con->hw_ops->gfx_update_async;
+    }
+    if (!async) {
+        graphic_hw_update_done(con);
     }
 }
 
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 17/20] console: make screendump asynchronous

[Marc-André Lureau]

Make screendump asynchronous to provide correct screendumps.

For now, HMP doesn't have async support, so it has to remain
synchronous and potentially incorrect to avoid races (following
patches will add HMP asynchronous commands)

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1230527

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/console.h |   3 ++
 monitor/hmp-cmds.c   |   2 +-
 qapi/ui.json         |   3 +-
 ui/console.c         | 103 +++++++++++++++++++++++++++++++++++++++----
 4 files changed, 100 insertions(+), 11 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 281f9c145b..a1935557cc 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -74,6 +74,9 @@ typedef struct MouseTransformInfo {
 } MouseTransformInfo;
 
 void hmp_mouse_set(Monitor *mon, const QDict *qdict);
+void hmp_screendump_sync(const char *filename,
+                         bool has_device, const char *device,
+                         bool has_head, int64_t head, Error **errp);
 
 /* keysym is a unicode code except for special keys (see QEMU_KEY_xxx
    constants) */
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 5ca3ebe942..50a25a1ddc 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2341,7 +2341,7 @@ void hmp_screendump(Monitor *mon, const QDict *qdict)
     int64_t head = qdict_get_try_int(qdict, "head", 0);
     Error *err = NULL;
 
-    qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
+    hmp_screendump_sync(filename, id != NULL, id, id != NULL, head, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/qapi/ui.json b/qapi/ui.json
index 59e412139a..cbb3979172 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -96,7 +96,8 @@
 #
 ##
 { 'command': 'screendump',
-  'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
+  'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
+  'async': true }
 
 ##
 # == Spice
diff --git a/ui/console.c b/ui/console.c
index 3c941528d2..29c850c31c 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -33,6 +33,7 @@
 #include "chardev/char-fe.h"
 #include "trace.h"
 #include "exec/memory.h"
+#include "monitor/monitor.h"
 
 #define DEFAULT_BACKSCROLL 512
 #define CONSOLE_CURSOR_PERIOD 500
@@ -117,6 +118,12 @@ typedef enum {
     TEXT_CONSOLE_FIXED_SIZE
 } console_type_t;
 
+struct qmp_screendump {
+    gchar *filename;
+    QmpReturn *ret;
+    QLIST_ENTRY(qmp_screendump) link;
+};
+
 struct QemuConsole {
     Object parent;
 
@@ -167,6 +174,8 @@ struct QemuConsole {
     uint8_t out_fifo_buf[16];
     QEMUTimer *kbd_timer;
 
+    QLIST_HEAD(, qmp_screendump) qmp_screendumps;
+
     QTAILQ_ENTRY(QemuConsole) next;
 };
 
@@ -193,6 +202,8 @@ static void dpy_refresh(DisplayState *s);
 static DisplayState *get_alloc_displaystate(void);
 static void text_console_update_cursor_timer(void);
 static void text_console_update_cursor(void *opaque);
+static void ppm_save(const char *filename, DisplaySurface *ds,
+                     Error **errp);
 
 static void gui_update(void *opaque)
 {
@@ -259,8 +270,48 @@ static void gui_setup_refresh(DisplayState *ds)
     ds->have_text = have_text;
 }
 
+static void qmp_screendump_finish(QemuConsole *con, struct qmp_screendump *dump)
+{
+    Error *err = NULL;
+    DisplaySurface *surface;
+    Monitor *prev_mon = cur_mon;
+
+    if (qmp_return_is_cancelled(dump->ret)) {
+        goto cleanup;
+    }
+
+    cur_mon = qmp_return_get_monitor(dump->ret);
+    surface = qemu_console_surface(con);
+    if (!surface) {
+        error_setg(&err, "no surface");
+    } else {
+        /*
+         * FIXME: async save with coroutine? it would have to copy or
+         * lock the surface.
+         */
+        ppm_save(dump->filename, surface, &err);
+    }
+
+    if (err) {
+        qmp_return_error(dump->ret, err);
+    } else {
+        qmp_screendump_return(dump->ret);
+    }
+    cur_mon = prev_mon;
+
+cleanup:
+    g_free(dump->filename);
+    QLIST_REMOVE(dump, link);
+    g_free(dump);
+}
+
 void graphic_hw_update_done(QemuConsole *con)
 {
+    struct qmp_screendump *dump, *next;
+
+    QLIST_FOREACH_SAFE(dump, &con->qmp_screendumps, link, next) {
+        qmp_screendump_finish(con, dump);
+    }
 }
 
 void graphic_hw_update(QemuConsole *con)
@@ -356,30 +407,41 @@ write_err:
     goto out;
 }
 
-void qmp_screendump(const char *filename, bool has_device, const char *device,
-                    bool has_head, int64_t head, Error **errp)
+
+static QemuConsole *get_console(bool has_device, const char *device,
+                                bool has_head, int64_t head, Error **errp)
 {
-    QemuConsole *con;
-    DisplaySurface *surface;
+    QemuConsole *con = NULL;
 
     if (has_device) {
         con = qemu_console_lookup_by_device_name(device, has_head ? head : 0,
                                                  errp);
-        if (!con) {
-            return;
-        }
     } else {
         if (has_head) {
             error_setg(errp, "'head' must be specified together with 'device'");
-            return;
+            return NULL;
         }
         con = qemu_console_lookup_by_index(0);
         if (!con) {
             error_setg(errp, "There is no console to take a screendump from");
-            return;
         }
     }
 
+    return con;
+}
+
+void hmp_screendump_sync(const char *filename,
+                         bool has_device, const char *device,
+                         bool has_head, int64_t head, Error **errp)
+{
+    DisplaySurface *surface;
+    QemuConsole *con = get_console(has_device, device, has_head, head, errp);
+
+    if (!con) {
+        return;
+    }
+    /* This may not complete the drawing with Spice, you may have
+     * glitches or outdated dumps, use qmp instead! */
     graphic_hw_update(con);
     surface = qemu_console_surface(con);
     if (!surface) {
@@ -390,6 +452,28 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
     ppm_save(filename, surface, errp);
 }
 
+void qmp_screendump(const char *filename,
+                    bool has_device, const char *device,
+                    bool has_head, int64_t head,
+                    QmpReturn *qret)
+{
+    Error *err = NULL;
+    struct qmp_screendump *dump = NULL;
+    QemuConsole *con = get_console(has_device, device, has_head, head, &err);
+
+    if (!con) {
+        qmp_return_error(qret, err);
+        return;
+    }
+
+    dump = g_new(struct qmp_screendump, 1);
+    dump->filename = g_strdup(filename);
+    dump->ret = qret;
+    QLIST_INSERT_HEAD(&con->qmp_screendumps, dump, link);
+
+    graphic_hw_update(con);
+}
+
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata)
 {
     if (!con) {
@@ -1300,6 +1384,7 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
     obj = object_new(TYPE_QEMU_CONSOLE);
     s = QEMU_CONSOLE(obj);
     s->head = head;
+    QLIST_INIT(&s->qmp_screendumps);
     object_property_add_link(obj, "device", TYPE_DEVICE,
                              (Object **)&s->device,
                              object_property_allow_set_link,
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 18/20] monitor: start making qmp_human_monitor_command() asynchronous

[Marc-André Lureau]

This prepares the work for HMP commands to be asynchronous.

Start making QMP human-monitor-command asynchronous, although
QmpReturn is used synchronously on error or after
handle_hmp_command().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor/misc.c | 14 ++++++++------
 qapi/misc.json |  3 ++-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/monitor/misc.c b/monitor/misc.c
index a23c1b8ba4..0645667e1b 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -115,8 +115,8 @@ static QLIST_HEAD(, MonFdset) mon_fdsets;
 
 static HMPCommand hmp_info_cmds[];
 
-char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
-                                int64_t cpu_index, Error **errp)
+void qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
+                               int64_t cpu_index, QmpReturn *qret)
 {
     char *output = NULL;
     Monitor *old_mon;
@@ -130,15 +130,15 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     if (has_cpu_index) {
         int ret = monitor_set_cpu(cpu_index);
         if (ret < 0) {
-            cur_mon = old_mon;
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
+            Error *err = NULL;
+            error_setg(&err, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
                        "a CPU number");
+            qmp_return_error(qret, err);
             goto out;
         }
     }
 
     handle_hmp_command(&hmp, command_line);
-    cur_mon = old_mon;
 
     qemu_mutex_lock(&hmp.common.mon_lock);
     if (qstring_get_length(hmp.common.outbuf) > 0) {
@@ -148,9 +148,11 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     }
     qemu_mutex_unlock(&hmp.common.mon_lock);
 
+    qmp_human_monitor_command_return(qret, output);
+
 out:
+    cur_mon = old_mon;
     monitor_data_destroy(&hmp.common);
-    return output;
 }
 
 /**
diff --git a/qapi/misc.json b/qapi/misc.json
index a7fba7230c..8c6ca46b8b 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1047,7 +1047,8 @@
 ##
 { 'command': 'human-monitor-command',
   'data': {'command-line': 'str', '*cpu-index': 'int'},
-  'returns': 'str' }
+  'returns': 'str',
+  'async': true }
 
 ##
 # @change:
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 19/20] monitor: teach HMP about asynchronous commands

[Marc-André Lureau]

Similar to how we handle both synchronous and asynchronous commands in
QMP, HMP gains a new async_cmd() that will allow the command to
complete asynchronously. For interactive reasons, and command
ordering, the HMP monitor is suspended until the asynchronous command
completes.

It is expected that HMP async commands will be implemented re-using
QMP async commands counterparts, so it reuses the QmpSession/QmpReturn
for context handling (instead of introducing HmpSession/HmpReturn and
having to convert from one to the other as we call QMP counterparts).

hmp_dispatch_return_cb() will handle printing the result to the
current monitor. It may have different ways to print the QmpReturn
result to the current monitor. Currently, only error reporting is
implemented.

QMP human-monitor-command is modified to deal with an async HMP
commands too. It creates a temporary session, and the return callback
will return asynchronously to the original QMP command and destroy the
temporary monitor when hmp->for_qmp_command is set.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/monitor/monitor.h  |   2 +-
 monitor/hmp.c              | 110 +++++++++++++++++++++++++++++++++++--
 monitor/misc.c             |  40 --------------
 monitor/monitor-internal.h |   9 ++-
 monitor/qmp.c              |  14 +++--
 5 files changed, 123 insertions(+), 52 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 6a2907a366..5968b52fe2 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -44,6 +44,6 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
 void monitor_fdset_dup_fd_remove(int dup_fd);
 int64_t monitor_fdset_dup_fd_find(int dup_fd);
 
-Monitor *qmp_return_get_monitor(QmpReturn *qret);
+Monitor *qmp_return_get_monitor(QmpReturn *qret, bool hmp);
 
 #endif /* MONITOR_H */
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 5223661e82..b2b6dce8fb 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -26,11 +26,15 @@
 #include <dirent.h>
 #include "monitor-internal.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-misc.h"
+#include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qstring.h"
 #include "qemu/config-file.h"
 #include "qemu/ctype.h"
 #include "qemu/cutils.h"
+#include "qemu/error-report.h"
 #include "qemu/log.h"
 #include "qemu/option.h"
 #include "qemu/units.h"
@@ -38,6 +42,8 @@
 #include "sysemu/sysemu.h"
 #include "trace.h"
 
+static bool handle_hmp_command(MonitorHMP *mon, const char *cmdline);
+
 static void monitor_command_cb(void *opaque, const char *cmdline,
                                void *readline_opaque)
 {
@@ -1056,7 +1062,7 @@ fail:
     return NULL;
 }
 
-void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
+static bool handle_hmp_command(MonitorHMP *mon, const char *cmdline)
 {
     QDict *qdict;
     const HMPCommand *cmd;
@@ -1066,7 +1072,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
 
     cmd = monitor_parse_command(mon, cmdline, &cmdline, hmp_cmds);
     if (!cmd) {
-        return;
+        return false;
     }
 
     qdict = monitor_parse_arguments(&mon->common, &cmdline, cmd);
@@ -1076,11 +1082,19 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
         }
         monitor_printf(&mon->common, "Try \"help %.*s\" for more information\n",
                        (int)(cmdline - cmd_start), cmd_start);
-        return;
+        return false;
     }
 
-    cmd->cmd(&mon->common, qdict);
+    if (cmd->async) {
+        QmpReturn *qret = qmp_return_new(&mon->qmp_session, NULL);
+        monitor_suspend(&mon->common);
+        cmd->async_cmd(&mon->common, qdict, qret);
+    } else {
+        cmd->cmd(&mon->common, qdict);
+    }
     qobject_unref(qdict);
+
+    return cmd->async;
 }
 
 static void cmd_completion(MonitorHMP *mon, const char *name, const char *list)
@@ -1395,6 +1409,59 @@ static void monitor_readline_flush(void *opaque)
     monitor_flush(&mon->common);
 }
 
+static void free_hmp_monitor(void *opaque)
+{
+    MonitorHMP *hmp = opaque;
+
+    qmp_session_destroy(&hmp->qmp_session);
+    monitor_data_destroy(&hmp->common);
+    g_free(hmp);
+}
+
+static AioContext *monitor_get_aio_context(void)
+{
+    return iothread_get_aio_context(mon_iothread);
+}
+
+static void qmp_human_monitor_command_finish(MonitorHMP *hmp, QmpReturn *qret)
+{
+    char *output;
+
+    qemu_mutex_lock(&hmp->common.mon_lock);
+    if (qstring_get_length(hmp->common.outbuf) > 0) {
+        output = g_strdup(qstring_get_str(hmp->common.outbuf));
+    } else {
+        output = g_strdup("");
+    }
+    qemu_mutex_unlock(&hmp->common.mon_lock);
+
+    qmp_human_monitor_command_return(qret, output);
+
+    if (hmp->for_qmp_command) {
+        aio_bh_schedule_oneshot(monitor_get_aio_context(),
+                                free_hmp_monitor, hmp);
+    }
+}
+
+static void hmp_dispatch_return_cb(QmpSession *session, QDict *rsp)
+{
+    MonitorHMP *hmp = container_of(session, MonitorHMP, qmp_session);
+    QDict *err = qdict_get_qdict(rsp, "error");
+    Monitor *old_mon = cur_mon;
+
+    cur_mon = &hmp->common;
+    if (err) {
+        error_report("%s", qdict_get_str(err, "desc"));
+    } /* XXX: else, report depending on command */
+
+    if (hmp->for_qmp_command) {
+        qmp_human_monitor_command_finish(hmp, hmp->for_qmp_command);
+    } else {
+        monitor_resume(&hmp->common);
+    }
+    cur_mon = old_mon;
+}
+
 void monitor_init_hmp(Chardev *chr, bool use_readline)
 {
     MonitorHMP *mon = g_new0(MonitorHMP, 1);
@@ -1411,7 +1478,42 @@ void monitor_init_hmp(Chardev *chr, bool use_readline)
         monitor_read_command(mon, 0);
     }
 
+    qmp_session_init(&mon->qmp_session,
+                     NULL, NULL, hmp_dispatch_return_cb);
     qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read, monitor_read,
                              monitor_event, NULL, &mon->common, NULL, true);
     monitor_list_append(&mon->common);
 }
+
+void qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
+                               int64_t cpu_index, QmpReturn *qret)
+{
+    Monitor *old_mon;
+    MonitorHMP *hmp = g_new0(MonitorHMP, 1);
+
+    monitor_data_init(&hmp->common, false, true, false);
+    qmp_session_init(&hmp->qmp_session, NULL, NULL, hmp_dispatch_return_cb);
+    hmp->for_qmp_command = qret;
+
+    old_mon = cur_mon;
+    cur_mon = &hmp->common;
+
+    if (has_cpu_index) {
+        int ret = monitor_set_cpu(cpu_index);
+        if (ret < 0) {
+            Error *err = NULL;
+            error_setg(&err, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
+                       "a CPU number");
+            qmp_return_error(qret, err);
+            free_hmp_monitor(hmp);
+            goto out;
+        }
+    }
+
+    if (!handle_hmp_command(hmp, command_line)) {
+        qmp_human_monitor_command_finish(hmp, qret);
+    }
+
+out:
+    cur_mon = old_mon;
+}
diff --git a/monitor/misc.c b/monitor/misc.c
index 0645667e1b..19a2919154 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -115,46 +115,6 @@ static QLIST_HEAD(, MonFdset) mon_fdsets;
 
 static HMPCommand hmp_info_cmds[];
 
-void qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
-                               int64_t cpu_index, QmpReturn *qret)
-{
-    char *output = NULL;
-    Monitor *old_mon;
-    MonitorHMP hmp = {};
-
-    monitor_data_init(&hmp.common, false, true, false);
-
-    old_mon = cur_mon;
-    cur_mon = &hmp.common;
-
-    if (has_cpu_index) {
-        int ret = monitor_set_cpu(cpu_index);
-        if (ret < 0) {
-            Error *err = NULL;
-            error_setg(&err, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
-                       "a CPU number");
-            qmp_return_error(qret, err);
-            goto out;
-        }
-    }
-
-    handle_hmp_command(&hmp, command_line);
-
-    qemu_mutex_lock(&hmp.common.mon_lock);
-    if (qstring_get_length(hmp.common.outbuf) > 0) {
-        output = g_strdup(qstring_get_str(hmp.common.outbuf));
-    } else {
-        output = g_strdup("");
-    }
-    qemu_mutex_unlock(&hmp.common.mon_lock);
-
-    qmp_human_monitor_command_return(qret, output);
-
-out:
-    cur_mon = old_mon;
-    monitor_data_destroy(&hmp.common);
-}
-
 /**
  * Is @name in the '|' separated list of names @list?
  */
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 65cf668b20..cab2712187 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -73,7 +73,10 @@ typedef struct HMPCommand {
     const char *params;
     const char *help;
     const char *flags; /* p=preconfig */
-    void (*cmd)(Monitor *mon, const QDict *qdict);
+    union {
+        void (*cmd)(Monitor *mon, const QDict *qdict);
+        void (*async_cmd)(Monitor *mon, const QDict *qdict, QmpReturn *qret);
+    };
     /*
      * @sub_table is a list of 2nd level of commands. If it does not exist,
      * cmd should be used. If it exists, sub_table[?].cmd should be
@@ -81,6 +84,7 @@ typedef struct HMPCommand {
      */
     struct HMPCommand *sub_table;
     void (*command_completion)(ReadLineState *rs, int nb_args, const char *str);
+    bool async;
 } HMPCommand;
 
 struct Monitor {
@@ -121,6 +125,8 @@ struct MonitorHMP {
      * These members can be safely accessed without locks.
      */
     ReadLineState *rs;
+    QmpReturn *for_qmp_command;
+    QmpSession qmp_session;
 };
 
 typedef struct {
@@ -176,7 +182,6 @@ void monitor_qmp_bh_dispatcher(void *data);
 
 int get_monitor_def(int64_t *pval, const char *name);
 void help_cmd(Monitor *mon, const char *name);
-void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
 int hmp_compare_cmd(const char *name, const char *list);
 
 #endif
diff --git a/monitor/qmp.c b/monitor/qmp.c
index df8b9d8d4f..cadaf77515 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -391,13 +391,17 @@ void monitor_init_qmp(Chardev *chr, bool pretty)
     }
 }
 
-Monitor *qmp_return_get_monitor(QmpReturn *qret)
+Monitor *qmp_return_get_monitor(QmpReturn *qret, bool hmp)
 {
-    MonitorQMP *mon;
-
     if (!qret->session) {
         return NULL;
     }
-    mon = container_of(qret->session, MonitorQMP, session);
-    return &mon->common;
+
+    if (hmp) {
+        MonitorHMP *hmp = container_of(qret->session, MonitorHMP, qmp_session);
+        return &hmp->common;
+    } else {
+        MonitorQMP *qmp = container_of(qret->session, MonitorQMP, session);
+        return &qmp->common;
+    }
 }
-- 
2.22.0.428.g6d5b264208

[Qemu-devel] [PATCH v5 20/20] hmp: call the asynchronous QMP screendump to fix outdated/glitches

[Marc-André Lureau]

In order to fix the bad screendumps (same as rhbz#1230527), call into
the asynchonous version of the QMP command.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hmp-commands.hx      |  3 ++-
 include/ui/console.h |  5 ++---
 monitor/hmp-cmds.c   |  6 ++----
 ui/console.c         | 24 +-----------------------
 4 files changed, 7 insertions(+), 31 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index bfa5681dd2..fa559d6a4e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -278,7 +278,8 @@ ETEXI
         .params     = "filename [device [head]]",
         .help       = "save screen from head 'head' of display device 'device' "
                       "into PPM image 'filename'",
-        .cmd        = hmp_screendump,
+        .async_cmd  = hmp_screendump_async,
+        .async      = true,
     },
 
 STEXI
diff --git a/include/ui/console.h b/include/ui/console.h
index a1935557cc..d0a2a2066f 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -6,6 +6,7 @@
 #include "qemu/notify.h"
 #include "qemu/error-report.h"
 #include "qapi/qapi-types-ui.h"
+#include "qapi/qmp/dispatch.h"
 
 #ifdef CONFIG_OPENGL
 # include <epoxy/gl.h>
@@ -74,9 +75,7 @@ typedef struct MouseTransformInfo {
 } MouseTransformInfo;
 
 void hmp_mouse_set(Monitor *mon, const QDict *qdict);
-void hmp_screendump_sync(const char *filename,
-                         bool has_device, const char *device,
-                         bool has_head, int64_t head, Error **errp);
+void hmp_screendump_async(Monitor *mon, const QDict *qdict, QmpReturn *qret);
 
 /* keysym is a unicode code except for special keys (see QEMU_KEY_xxx
    constants) */
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 50a25a1ddc..e2af0e6307 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2334,15 +2334,13 @@ err_out:
     goto out;
 }
 
-void hmp_screendump(Monitor *mon, const QDict *qdict)
+void hmp_screendump_async(Monitor *mon, const QDict *qdict, QmpReturn *qret)
 {
     const char *filename = qdict_get_str(qdict, "filename");
     const char *id = qdict_get_try_str(qdict, "device");
     int64_t head = qdict_get_try_int(qdict, "head", 0);
-    Error *err = NULL;
 
-    hmp_screendump_sync(filename, id != NULL, id, id != NULL, head, &err);
-    hmp_handle_error(mon, &err);
+    qmp_screendump(filename, id != NULL, id, id != NULL, head, qret);
 }
 
 void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
diff --git a/ui/console.c b/ui/console.c
index 29c850c31c..7436b153b9 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -280,7 +280,7 @@ static void qmp_screendump_finish(QemuConsole *con, struct qmp_screendump *dump)
         goto cleanup;
     }
 
-    cur_mon = qmp_return_get_monitor(dump->ret);
+    cur_mon = qmp_return_get_monitor(dump->ret, true);
     surface = qemu_console_surface(con);
     if (!surface) {
         error_setg(&err, "no surface");
@@ -430,28 +430,6 @@ static QemuConsole *get_console(bool has_device, const char *device,
     return con;
 }
 
-void hmp_screendump_sync(const char *filename,
-                         bool has_device, const char *device,
-                         bool has_head, int64_t head, Error **errp)
-{
-    DisplaySurface *surface;
-    QemuConsole *con = get_console(has_device, device, has_head, head, errp);
-
-    if (!con) {
-        return;
-    }
-    /* This may not complete the drawing with Spice, you may have
-     * glitches or outdated dumps, use qmp instead! */
-    graphic_hw_update(con);
-    surface = qemu_console_surface(con);
-    if (!surface) {
-        error_setg(errp, "no surface");
-        return;
-    }
-
-    ppm_save(filename, surface, errp);
-}
-
 void qmp_screendump(const char *filename,
                     bool has_device, const char *device,
                     bool has_head, int64_t head,
-- 
2.22.0.428.g6d5b264208