qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 00/25] monitor: add asynchronous command type
@ 2019-11-08 15:00 Marc-André Lureau
  2019-11-08 15:00 ` [PATCH v6 01/25] qmp: constify QmpCommand and list Marc-André Lureau
                   ` (25 more replies)
  0 siblings, 26 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 internal async command solution
instead. By introducing a session context and a command return
handler, QMP handlers can:
- defer the return, allowing the mainloop to reenter
- return only to the caller (instead of the broadcast event reply)
- optionnally allow cancellation when the client is gone
- track on-going qapi command(s) per session

This does not introduce new QMP APIs or client visible changes, the
command are handled in order, and the reply still come in order (even
when handlers finish out of order).

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.

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.

v6:
- includes PPM save code improvements (will be sent seperatly,
  included for convenience for now)
- remove qmp_return_get_monitor() need, which is problematic with the
  QMP session over HMP monitor ("monitor: teach HMP about asynchronous
  commands")
- rebased

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 (25):
  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()
  console: add graphic_hw_update_done()
  ppm-save: pass opened fd
  ui: add pixman image g_autoptr support
  object: add g_autoptr support
  screendump: replace FILE with QIOChannel and fix close()/qemu_close()
  osdep: add qemu_unlink()
  screendump: use qemu_unlink()
  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/qapi/qmp/dispatch.h             |  89 +++++++++-
 include/qapi/qmp/json-parser.h          |   7 +-
 include/qemu/osdep.h                    |   1 +
 include/qom/object.h                    |   3 +
 include/ui/console.h                    |   4 +
 include/ui/qemu-pixman.h                |   2 +
 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                           |  64 +++----
 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                | 149 ++++++++++++++---
 scripts/qapi/doc.py                     |   2 +-
 scripts/qapi/expr.py                    |   4 +-
 scripts/qapi/introspect.py              |   2 +-
 scripts/qapi/schema.py                  |  11 +-
 tests/qapi-schema/qapi-schema-test.json |   5 +
 tests/qapi-schema/qapi-schema-test.out  |   8 +
 tests/qapi-schema/test-qapi.py          |   7 +-
 tests/test-qmp-cmds.c                   | 206 +++++++++++++++++++----
 ui/console.c                            | 165 ++++++++++++------
 ui/trace-events                         |   2 +-
 util/osdep.c                            |  15 ++
 35 files changed, 937 insertions(+), 312 deletions(-)

-- 
2.24.0



^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v6 01/25] qmp: constify QmpCommand and list
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
@ 2019-11-08 15:00 ` Marc-André Lureau
  2019-11-08 16:50   ` Damien Hedde
  2019-11-08 15:01 ` [PATCH v6 02/25] json-lexer: make it safe to call destroy multiple times Marc-André Lureau
                   ` (24 subsequent siblings)
  25 siblings, 1 reply; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 3baa15f3bf..3052bfe8f1 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 d78f5ca190..3e7dac5910 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -132,7 +132,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 bc264b3c9b..857399c5fe 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);
@@ -164,7 +164,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 02/25] json-lexer: make it safe to call destroy multiple times
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
  2019-11-08 15:00 ` [PATCH v6 01/25] qmp: constify QmpCommand and list Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 16:04   ` Damien Hedde
  2019-11-08 15:01 ` [PATCH v6 03/25] qmp: add QmpSession Marc-André Lureau
                   ` (23 subsequent siblings)
  25 siblings, 1 reply; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 03/25] qmp: add QmpSession
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
  2019-11-08 15:00 ` [PATCH v6 01/25] qmp: constify QmpCommand and list Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 02/25] json-lexer: make it safe to call destroy multiple times Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 04/25] QmpSession: add a return callback Marc-André Lureau
                   ` (22 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 3052bfe8f1..bb33ca73cf 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 3e7dac5910..b15266e77b 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -132,7 +132,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 12898b6448..ecf21d9786 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -263,7 +263,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 9d9e5d8b27..24ccdeb4a4 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 857399c5fe..0e6b003dd8 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -164,15 +164,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 27b0afe55a..962c2cb2e1 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -147,44 +147,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"));
 
@@ -198,36 +206,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 04/25] QmpSession: add a return callback
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (2 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 03/25] qmp: add QmpSession Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 05/25] QmpSession: add json parser and use it in qga Marc-André Lureau
                   ` (21 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 24ccdeb4a4..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 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 0e6b003dd8..1314f4929f 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -164,17 +164,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);
@@ -189,12 +200,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 962c2cb2e1..cca15e79ba 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -143,22 +143,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);
 }
@@ -167,82 +168,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 05/25] QmpSession: add json parser and use it in qga
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (3 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 04/25] QmpSession: add a return callback Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 06/25] monitor: use qmp session to parse json feed Marc-André Lureau
                   ` (20 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 1314f4929f..f8c491924d 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -164,6 +164,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)
@@ -171,6 +188,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;
 }
@@ -183,6 +201,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 06/25] monitor: use qmp session to parse json feed
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (4 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 05/25] QmpSession: add json parser and use it in qga Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 07/25] qga: simplify dispatch_return_cb Marc-André Lureau
                   ` (19 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 b15266e77b..b8994f896a 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -124,7 +124,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 f8c491924d..b004d7506f 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -183,12 +183,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 cca15e79ba..d9623d10b6 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -155,7 +155,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);
@@ -169,7 +169,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);
@@ -191,7 +191,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);
@@ -217,7 +218,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);
@@ -238,7 +239,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 07/25] qga: simplify dispatch_return_cb
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (5 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 06/25] monitor: use qmp session to parse json feed Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 08/25] QmpSession: introduce QmpReturn Marc-André Lureau
                   ` (18 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 08/25] QmpSession: introduce QmpReturn
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (6 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 07/25] qga: simplify dispatch_return_cb Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 09/25] qmp: simplify qmp_return_error() Marc-André Lureau
                   ` (17 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 b004d7506f..188b5680b6 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -19,6 +19,46 @@
 #include "sysemu/runstate.h"
 #include "qapi/qmp/qbool.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)
 {
@@ -144,17 +184,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?
  */
@@ -171,9 +200,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);
     }
@@ -209,25 +236,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 09/25] qmp: simplify qmp_return_error()
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (7 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 08/25] QmpSession: introduce QmpReturn Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 10/25] QmpSession: keep a queue of pending commands Marc-André Lureau
                   ` (16 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 188b5680b6..fcf6cb0bf8 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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 10/25] QmpSession: keep a queue of pending commands
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (8 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 09/25] qmp: simplify qmp_return_error() Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 11/25] QmpSession: return orderly Marc-André Lureau
                   ` (15 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 fcf6cb0bf8..aed5c91057 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);
 }
 
@@ -220,17 +237,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 11/25] QmpSession: return orderly
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (9 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 10/25] QmpSession: keep a queue of pending commands Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 12/25] qmp: introduce asynchronous command type Marc-André Lureau
                   ` (14 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 aed5c91057..3eb7e4b610 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 d9623d10b6..213df7e53a 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -361,6 +361,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)
 {
@@ -374,6 +406,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 12/25] qmp: introduce asynchronous command type
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (10 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 11/25] QmpSession: return orderly Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 13/25] scripts: learn 'async' qapi commands Marc-André Lureau
                   ` (13 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 3eb7e4b610..85e99671a9 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);
@@ -225,14 +225,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);
@@ -305,7 +310,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 13/25] scripts: learn 'async' qapi commands
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (11 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 12/25] qmp: introduce asynchronous command type Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 14/25] qmp: add qmp_return_is_cancelled() Marc-André Lureau
                   ` (12 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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                | 149 ++++++++++++++++++++----
 scripts/qapi/doc.py                     |   2 +-
 scripts/qapi/expr.py                    |   4 +-
 scripts/qapi/introspect.py              |   2 +-
 scripts/qapi/schema.py                  |  11 +-
 tests/qapi-schema/qapi-schema-test.json |   5 +
 tests/qapi-schema/qapi-schema-test.out  |   8 ++
 tests/qapi-schema/test-qapi.py          |   7 +-
 tests/test-qmp-cmds.c                   |  60 ++++++++++
 9 files changed, 212 insertions(+), 36 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index ab98e504f3..301b1a24d8 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -17,18 +17,35 @@ from qapi.common import *
 from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext
 
 
-def gen_command_decl(name, arg_type, boxed, ret_type):
-    return mcgen('''
+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);
+ ''',
+                    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, 'Error **errp'))
+                     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
@@ -39,7 +56,13 @@ def gen_call(name, arg_type, boxed, ret_type):
             if memb.optional:
                 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 = '
@@ -61,6 +84,51 @@ 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('''
 
@@ -84,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 = boxed or (arg_type and not arg_type.is_empty())
 
     ret = mcgen('''
@@ -105,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;
 ''',
@@ -154,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);
 ''')
 
@@ -194,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:
@@ -203,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
 
@@ -278,7 +373,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      features, asyn):
         if not gen:
             return
         # FIXME: If T is a user-defined type, the user is responsible
@@ -292,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/doc.py b/scripts/qapi/doc.py
index 6f1c17f71f..375978f04f 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -265,7 +265,7 @@ class QAPISchemaGenDocVisitor(QAPISchemaVisitor):
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      features, asyn):
         doc = self.cur_doc
         self._gen.add(texi_msg('Command', doc, ifcond,
                                texi_arguments(doc,
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index d7a289eded..db4956133d 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -89,7 +89,7 @@ def check_flags(expr, info):
         if key in expr and expr[key] is not False:
             raise QAPISemError(
                 info, "flag '%s' may only use false value" % key)
-    for key in ['boxed', 'allow-oob', 'allow-preconfig']:
+    for key in ['boxed', 'allow-oob', 'allow-preconfig', 'async']:
         if key in expr and expr[key] is not True:
             raise QAPISemError(
                 info, "flag '%s' may only use true value" % key)
@@ -344,7 +344,7 @@ def check_exprs(exprs):
                        ['command'],
                        ['data', 'returns', 'boxed', 'if', 'features',
                         'gen', 'success-response', 'allow-oob',
-                        'allow-preconfig'])
+                        'allow-preconfig', 'async'])
             normalize_members(expr.get('data'))
             check_command(expr, info)
         elif meta == 'event':
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b3a463dd8b..f28c96afa3 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -212,7 +212,7 @@ const QLitObject %(c_name)s = %(c_string)s;
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      features, 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/scripts/qapi/schema.py b/scripts/qapi/schema.py
index cf0045f34e..2e1d5278bf 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -128,7 +128,7 @@ class QAPISchemaVisitor(object):
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      features, asyn):
         pass
 
     def visit_event(self, name, info, ifcond, arg_type, boxed):
@@ -678,7 +678,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
 
     def __init__(self, name, info, doc, ifcond, arg_type, ret_type,
                  gen, success_response, boxed, allow_oob, allow_preconfig,
-                 features):
+                 features, asyn):
         QAPISchemaEntity.__init__(self, name, info, doc, ifcond, features)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -691,6 +691,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)
@@ -733,7 +734,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
                               self.gen, self.success_response,
                               self.boxed, self.allow_oob,
                               self.allow_preconfig,
-                              self.features)
+                              self.features, self.asyn)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1016,6 +1017,7 @@ class QAPISchema(object):
         allow_preconfig = expr.get('allow-preconfig', False)
         ifcond = expr.get('if')
         features = expr.get('features', [])
+        asyn = expr.get('async', False)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, ifcond, 'arg', self._make_members(data, info))
@@ -1025,7 +1027,8 @@ class QAPISchema(object):
         self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, rets,
                                            gen, success_response,
                                            boxed, allow_oob, allow_preconfig,
-                                           self._make_features(features, info)))
+                                           self._make_features(features, info),
+                                           asyn))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 9abf175fe0..58d2164c7e 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -148,6 +148,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 3660e75a48..19d2d202b9 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -217,6 +217,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 bad14edb47..1914b7d5ff 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -70,12 +70,13 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      features, 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)
         self._print_features(features)
 
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 213df7e53a..75ef30f6ea 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);
@@ -394,6 +416,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);
@@ -407,6 +466,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 14/25] qmp: add qmp_return_is_cancelled()
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (12 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 13/25] scripts: learn 'async' qapi commands Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 15/25] console: add graphic_hw_update_done() Marc-André Lureau
                   ` (11 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 85e99671a9..e5c1505b05 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 75ef30f6ea..66ad8726fe 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);
 }
 
@@ -453,6 +465,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);
@@ -467,6 +503,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 15/25] console: add graphic_hw_update_done()
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (13 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 14/25] qmp: add qmp_return_is_cancelled() Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 16/25] ppm-save: pass opened fd Marc-André Lureau
                   ` (10 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 f7fdc4901e..3ce2e57b8f 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -109,7 +109,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;
@@ -137,7 +137,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)) {
@@ -158,6 +158,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 cd7eb39d20..6d43b7433c 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1181,6 +1181,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 16/25] ppm-save: pass opened fd
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (14 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 15/25] console: add graphic_hw_update_done() Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 17/25] ui: add pixman image g_autoptr support Marc-André Lureau
                   ` (9 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

This will allow to pre-open the file before running the async finish
handler and avoid potential monitor fdset races.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c    | 45 ++++++++++++++++++++++-----------------------
 ui/trace-events |  2 +-
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 3c941528d2..77d62fe76d 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -193,6 +193,7 @@ 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 bool ppm_save(int fd, DisplaySurface *ds, Error **errp);
 
 static void gui_update(void *opaque)
 {
@@ -308,29 +309,22 @@ void graphic_hw_invalidate(QemuConsole *con)
     }
 }
 
-static void ppm_save(const char *filename, DisplaySurface *ds,
-                     Error **errp)
+static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
 {
     int width = pixman_image_get_width(ds->image);
     int height = pixman_image_get_height(ds->image);
-    int fd;
     FILE *f;
     int y;
     int ret;
     pixman_image_t *linebuf;
+    bool success = false;
 
-    trace_ppm_save(filename, ds);
-    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666);
-    if (fd == -1) {
-        error_setg(errp, "failed to open file '%s': %s", filename,
-                   strerror(errno));
-        return;
-    }
+    trace_ppm_save(fd, ds);
     f = fdopen(fd, "wb");
     ret = fprintf(f, "P6\n%d %d\n%d\n", width, height, 255);
     if (ret < 0) {
         linebuf = NULL;
-        goto write_err;
+        goto end;
     }
     linebuf = qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
     for (y = 0; y < height; y++) {
@@ -339,21 +333,16 @@ static void ppm_save(const char *filename, DisplaySurface *ds,
         ret = fwrite(pixman_image_get_data(linebuf), 1,
                      pixman_image_get_stride(linebuf), f);
         (void)ret;
-        if (ferror(f)) {
-            goto write_err;
-        }
+        success = !ferror(f);
     }
 
-out:
+end:
+    if (!success) {
+        error_setg(errp, "failed to write to PPM file: %s", strerror(errno));
+    }
     qemu_pixman_image_unref(linebuf);
     fclose(f);
-    return;
-
-write_err:
-    error_setg(errp, "failed to write to file '%s': %s", filename,
-               strerror(errno));
-    unlink(filename);
-    goto out;
+    return success;
 }
 
 void qmp_screendump(const char *filename, bool has_device, const char *device,
@@ -361,6 +350,7 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
 {
     QemuConsole *con;
     DisplaySurface *surface;
+    int fd;
 
     if (has_device) {
         con = qemu_console_lookup_by_device_name(device, has_head ? head : 0,
@@ -387,7 +377,16 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
         return;
     }
 
-    ppm_save(filename, surface, errp);
+    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666);
+    if (fd == -1) {
+        error_setg(errp, "failed to open file '%s': %s", filename,
+                   strerror(errno));
+        return;
+    }
+
+    if (!ppm_save(fd, surface, errp)) {
+        unlink(filename);
+    }
 }
 
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata)
diff --git a/ui/trace-events b/ui/trace-events
index 63de72a798..0dcda393c1 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -15,7 +15,7 @@ displaysurface_create_pixman(void *display_surface) "surface=%p"
 displaysurface_free(void *display_surface) "surface=%p"
 displaychangelistener_register(void *dcl, const char *name) "%p [ %s ]"
 displaychangelistener_unregister(void *dcl, const char *name) "%p [ %s ]"
-ppm_save(const char *filename, void *display_surface) "%s surface=%p"
+ppm_save(int fd, void *display_surface) "fd=%d surface=%p"
 
 # gtk.c
 # gtk-gl-area.c
-- 
2.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 17/25] ui: add pixman image g_autoptr support
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (15 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 16/25] ppm-save: pass opened fd Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 18/25] object: add " Marc-André Lureau
                   ` (8 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/qemu-pixman.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index 0668109305..3b7cf70157 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -90,4 +90,6 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
                               pixman_color_t *bgcol,
                               int x, int y, int cw, int ch);
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(pixman_image_t, qemu_pixman_image_unref)
+
 #endif /* QEMU_PIXMAN_H */
-- 
2.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 18/25] object: add g_autoptr support
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (16 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 17/25] ui: add pixman image g_autoptr support Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 19/25] screendump: replace FILE with QIOChannel and fix close()/qemu_close() Marc-André Lureau
                   ` (7 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qom/object.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 128d00c77f..f96a44be64 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1747,4 +1747,7 @@ Object *container_get(Object *root, const char *path);
  * Returns the instance_size of the given @typename.
  */
 size_t object_type_get_instance_size(const char *typename);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(Object, object_unref)
+
 #endif
-- 
2.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 19/25] screendump: replace FILE with QIOChannel and fix close()/qemu_close()
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (17 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 18/25] object: add " Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 20/25] osdep: add qemu_unlink() Marc-André Lureau
                   ` (6 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

The file opened for ppm_save() may be a /dev/fdset, in which case a
dup fd is added to the fdset. It should be removed by calling
qemu_close(), instead of the implicit close() on fclose().

I don't see a convenient way to solve that with stdio streams, so I
switched the code to QIOChannel which uses qemu_close().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 77d62fe76d..587edf4ed4 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 "io/channel-file.h"
 
 #define DEFAULT_BACKSCROLL 512
 #define CONSOLE_CURSOR_PERIOD 500
@@ -313,36 +314,31 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
 {
     int width = pixman_image_get_width(ds->image);
     int height = pixman_image_get_height(ds->image);
-    FILE *f;
+    g_autoptr(Object) ioc = OBJECT(qio_channel_file_new_fd(fd));
+    g_autofree char *header = NULL;
+    g_autoptr(pixman_image_t) linebuf = NULL;
+    g_autoptr(GError) error = NULL;
     int y;
-    int ret;
-    pixman_image_t *linebuf;
-    bool success = false;
 
     trace_ppm_save(fd, ds);
-    f = fdopen(fd, "wb");
-    ret = fprintf(f, "P6\n%d %d\n%d\n", width, height, 255);
-    if (ret < 0) {
-        linebuf = NULL;
-        goto end;
+
+    header = g_strdup_printf("P6\n%d %d\n%d\n", width, height, 255);
+    if (qio_channel_write_all(QIO_CHANNEL(ioc),
+                              header, strlen(header), errp) < 0) {
+        return false;
     }
+
     linebuf = qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
     for (y = 0; y < height; y++) {
         qemu_pixman_linebuf_fill(linebuf, ds->image, width, 0, y);
-        clearerr(f);
-        ret = fwrite(pixman_image_get_data(linebuf), 1,
-                     pixman_image_get_stride(linebuf), f);
-        (void)ret;
-        success = !ferror(f);
+        if (qio_channel_write_all(QIO_CHANNEL(ioc),
+                                  (char *)pixman_image_get_data(linebuf),
+                                  pixman_image_get_stride(linebuf), errp) < 0) {
+            return false;
+        }
     }
 
-end:
-    if (!success) {
-        error_setg(errp, "failed to write to PPM file: %s", strerror(errno));
-    }
-    qemu_pixman_image_unref(linebuf);
-    fclose(f);
-    return success;
+    return true;
 }
 
 void qmp_screendump(const char *filename, bool has_device, const char *device,
-- 
2.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 20/25] osdep: add qemu_unlink()
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (18 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 19/25] screendump: replace FILE with QIOChannel and fix close()/qemu_close() Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 21/25] screendump: use qemu_unlink() Marc-André Lureau
                   ` (5 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

Add a helper function to match qemu_open() which may return files
under the /dev/fdset prefix. Those shouldn't be removed, since it's
only a qemu namespace.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/osdep.h |  1 +
 util/osdep.c         | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 0f97d68586..9bd3dcfd13 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -462,6 +462,7 @@ int qemu_mprotect_none(void *addr, size_t size);
 
 int qemu_open(const char *name, int flags, ...);
 int qemu_close(int fd);
+int qemu_unlink(const char *name);
 #ifndef _WIN32
 int qemu_dup(int fd);
 #endif
diff --git a/util/osdep.c b/util/osdep.c
index 3f04326040..f7d06050f7 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -370,6 +370,21 @@ int qemu_close(int fd)
     return close(fd);
 }
 
+/*
+ * Delete a file from the filesystem, unless the filename is /dev/fdset/...
+ *
+ * Returns: On success, zero is returned.  On error, -1 is returned,
+ * and errno is set appropriately.
+ */
+int qemu_unlink(const char *name)
+{
+    if (g_str_has_prefix(name, "/dev/fdset/")) {
+        return 0;
+    }
+
+    return unlink(name);
+}
+
 /*
  * A variant of write(2) which handles partial write.
  *
-- 
2.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 21/25] screendump: use qemu_unlink()
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (19 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 20/25] osdep: add qemu_unlink() Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 22/25] console: make screendump asynchronous Marc-André Lureau
                   ` (4 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

Don't attempt to remove /dev/fdset files.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/console.c b/ui/console.c
index 587edf4ed4..e6ac462aa0 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -381,7 +381,7 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
     }
 
     if (!ppm_save(fd, surface, errp)) {
-        unlink(filename);
+        qemu_unlink(filename);
     }
 }
 
-- 
2.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 22/25] console: make screendump asynchronous
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (20 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 21/25] screendump: use qemu_unlink() Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 23/25] monitor: start making qmp_human_monitor_command() asynchronous Marc-André Lureau
                   ` (3 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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         | 117 +++++++++++++++++++++++++++++++++++++++----
 4 files changed, 113 insertions(+), 12 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 b2551c16d1..c52e78fedf 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2308,7 +2308,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 e04525d8b4..148ae4c062 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 e6ac462aa0..fd87605b7c 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -34,6 +34,7 @@
 #include "trace.h"
 #include "exec/memory.h"
 #include "io/channel-file.h"
+#include "monitor/monitor.h"
 
 #define DEFAULT_BACKSCROLL 512
 #define CONSOLE_CURSOR_PERIOD 500
@@ -118,6 +119,13 @@ typedef enum {
     TEXT_CONSOLE_FIXED_SIZE
 } console_type_t;
 
+struct qmp_screendump {
+    int fd;
+    gchar *filename;
+    QmpReturn *ret;
+    QLIST_ENTRY(qmp_screendump) link;
+};
+
 struct QemuConsole {
     Object parent;
 
@@ -168,6 +176,8 @@ struct QemuConsole {
     uint8_t out_fifo_buf[16];
     QEMUTimer *kbd_timer;
 
+    QLIST_HEAD(, qmp_screendump) qmp_screendumps;
+
     QTAILQ_ENTRY(QemuConsole) next;
 };
 
@@ -261,8 +271,51 @@ 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;
+
+    if (qmp_return_is_cancelled(dump->ret)) {
+        goto cleanup;
+    }
+
+    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.
+         */
+        if (!ppm_save(dump->fd, surface, &err)) {
+            qemu_unlink(dump->filename);
+        }
+        dump->fd = -1;
+    }
+
+    if (err) {
+        qmp_return_error(dump->ret, err);
+    } else {
+        qmp_screendump_return(dump->ret);
+    }
+
+cleanup:
+    if (dump->fd != -1) {
+        qemu_close(dump->fd);
+    }
+    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)
@@ -341,31 +394,42 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
     return true;
 }
 
-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;
-    int fd;
+    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);
+    int fd;
+
+    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) {
@@ -385,6 +449,38 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
     }
 }
 
+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);
+    int fd;
+
+    if (!con) {
+        qmp_return_error(qret, err);
+        return;
+    }
+
+    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666);
+    if (fd == -1) {
+        error_setg(&err, "failed to open file '%s': %s",
+                   filename, strerror(errno));
+        qmp_return_error(qret, err);
+        return;
+    }
+
+    dump = g_new(struct qmp_screendump, 1);
+    dump->fd = fd;
+    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) {
@@ -1295,6 +1391,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.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 23/25] monitor: start making qmp_human_monitor_command() asynchronous
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (21 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 22/25] console: make screendump asynchronous Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 24/25] monitor: teach HMP about asynchronous commands Marc-André Lureau
                   ` (2 subsequent siblings)
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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 bb33ca73cf..3617f855f5 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 33b94e3589..15a8bc0d0d 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1054,7 +1054,8 @@
 { 'command': 'human-monitor-command',
   'data': {'command-line': 'str', '*cpu-index': 'int'},
   'returns': 'str',
-  'features': [ 'savevm-monitor-nodes' ] }
+  'features': [ 'savevm-monitor-nodes' ],
+  'async': true }
 
 ##
 # @change:
-- 
2.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 24/25] monitor: teach HMP about asynchronous commands
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (22 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 23/25] monitor: start making qmp_human_monitor_command() asynchronous Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-11-08 15:01 ` [PATCH v6 25/25] hmp: call the asynchronous QMP screendump to fix outdated/glitches Marc-André Lureau
  2019-12-13 16:03 ` [PATCH v6 00/25] monitor: add asynchronous command type Kevin Wolf
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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>
---
 monitor/hmp.c              | 110 +++++++++++++++++++++++++++++++++++--
 monitor/misc.c             |  40 --------------
 monitor/monitor-internal.h |   9 ++-
 3 files changed, 113 insertions(+), 46 deletions(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index 8942e28933..07f305141d 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/runstate.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 3617f855f5..ed672db650 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 b8994f896a..d13da7db31 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -72,7 +72,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
@@ -80,6 +83,7 @@ typedef struct HMPCommand {
      */
     struct HMPCommand *sub_table;
     void (*command_completion)(ReadLineState *rs, int nb_args, const char *str);
+    bool async;
 } HMPCommand;
 
 struct Monitor {
@@ -120,6 +124,8 @@ struct MonitorHMP {
      * These members can be safely accessed without locks.
      */
     ReadLineState *rs;
+    QmpReturn *for_qmp_command;
+    QmpSession qmp_session;
 };
 
 typedef struct {
@@ -175,7 +181,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
-- 
2.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH v6 25/25] hmp: call the asynchronous QMP screendump to fix outdated/glitches
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (23 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 24/25] monitor: teach HMP about asynchronous commands Marc-André Lureau
@ 2019-11-08 15:01 ` Marc-André Lureau
  2019-12-13 16:03 ` [PATCH v6 00/25] monitor: add asynchronous command type Kevin Wolf
  25 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2019-11-08 15:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Marc-André Lureau, armbru, kraxel

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         | 32 --------------------------------
 4 files changed, 6 insertions(+), 40 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index cfcc044ce4..82b9236deb 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 c52e78fedf..93b061cf4f 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2301,15 +2301,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 fd87605b7c..a6f8e34e61 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -417,38 +417,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);
-    int fd;
-
-    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;
-    }
-
-    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666);
-    if (fd == -1) {
-        error_setg(errp, "failed to open file '%s': %s", filename,
-                   strerror(errno));
-        return;
-    }
-
-    if (!ppm_save(fd, surface, errp)) {
-        qemu_unlink(filename);
-    }
-}
-
 void qmp_screendump(const char *filename,
                     bool has_device, const char *device,
                     bool has_head, int64_t head,
-- 
2.24.0



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 02/25] json-lexer: make it safe to call destroy multiple times
  2019-11-08 15:01 ` [PATCH v6 02/25] json-lexer: make it safe to call destroy multiple times Marc-André Lureau
@ 2019-11-08 16:04   ` Damien Hedde
  0 siblings, 0 replies; 37+ messages in thread
From: Damien Hedde @ 2019-11-08 16:04 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel; +Cc: armbru, kraxel



On 11/8/19 4:01 PM, Marc-André Lureau wrote:
> 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;
> +    }
>  }
> 

Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>

--
Damien


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 01/25] qmp: constify QmpCommand and list
  2019-11-08 15:00 ` [PATCH v6 01/25] qmp: constify QmpCommand and list Marc-André Lureau
@ 2019-11-08 16:50   ` Damien Hedde
  2020-02-17 11:47     ` Marc-André Lureau
  0 siblings, 1 reply; 37+ messages in thread
From: Damien Hedde @ 2019-11-08 16:50 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel; +Cc: armbru, kraxel



On 11/8/19 4:00 PM, Marc-André Lureau wrote:
> 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 3baa15f3bf..3052bfe8f1 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 d78f5ca190..3e7dac5910 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -132,7 +132,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 bc264b3c9b..857399c5fe 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);
> @@ -164,7 +164,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));
>  }
> 

Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>

Damien


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 00/25] monitor: add asynchronous command type
  2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
                   ` (24 preceding siblings ...)
  2019-11-08 15:01 ` [PATCH v6 25/25] hmp: call the asynchronous QMP screendump to fix outdated/glitches Marc-André Lureau
@ 2019-12-13 16:03 ` Kevin Wolf
  2019-12-13 16:28   ` Marc-André Lureau
  25 siblings, 1 reply; 37+ messages in thread
From: Kevin Wolf @ 2019-12-13 16:03 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: kraxel, qemu-devel, armbru

Am 08.11.2019 um 16:00 hat Marc-André Lureau geschrieben:
> The following series implements an internal async command solution
> instead. By introducing a session context and a command return
> handler, QMP handlers can:
> - defer the return, allowing the mainloop to reenter
> - return only to the caller (instead of the broadcast event reply)
> - optionnally allow cancellation when the client is gone
> - track on-going qapi command(s) per session

This requires major changes to existing QMP command handlers. Did you
consider at least optionally providing a way where instead of using the
new async_fn, QMP already creates a coroutine in which the handler is
executed?

At least for some of the block layer commands, we could simply enable
this without changing any of the code and would automatically get rid of
blocking the guest while the command is doing I/O. If we need to
implement .async_fn, in contrast, it would be quite a bit of boiler
plate code for each single handler to create a coroutine for the real
handler and to wrap all parameters in a struct.

I started playing a bit with this and it didn't look too bad, but we
can't move every command handler to a coroutine without auditing it, so
I would have had to add a new option to the QAPI schema - and at that
point I thought that I might end up duplicating a lot of your code if I
continued. So I'm now trying to get your opinion or advice before I
continue with anything in that direction.

> This does not introduce new QMP APIs or client visible changes, the
> command are handled in order, and the reply still come in order (even
> when handlers finish out of order).
> 
> 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.

This part would certainly become simpler with coroutines (the marshaller
could stay unchanged).

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

After converting it to QIOChannel like you already do, I/O would
automatically become asynchronous when run in a coroutine.

If you don't want this yet, but only fix the BZ, I guess you could delay
that patch until later and just have a single yield and reenter of the
command handler coroutine like this:

    co = qemu_coroutine_self();
    aio_co_schedule(qemu_coroutine_get_aio_context(co), co);
    qemu_coroutine_yield();

Kevin



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 00/25] monitor: add asynchronous command type
  2019-12-13 16:03 ` [PATCH v6 00/25] monitor: add asynchronous command type Kevin Wolf
@ 2019-12-13 16:28   ` Marc-André Lureau
  2019-12-16 12:07     ` Kevin Wolf
  0 siblings, 1 reply; 37+ messages in thread
From: Marc-André Lureau @ 2019-12-13 16:28 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Markus Armbruster, Gerd Hoffmann, QEMU

Hi

On Fri, Dec 13, 2019 at 8:04 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 08.11.2019 um 16:00 hat Marc-André Lureau geschrieben:
> > The following series implements an internal async command solution
> > instead. By introducing a session context and a command return
> > handler, QMP handlers can:
> > - defer the return, allowing the mainloop to reenter
> > - return only to the caller (instead of the broadcast event reply)
> > - optionnally allow cancellation when the client is gone
> > - track on-going qapi command(s) per session
>
> This requires major changes to existing QMP command handlers. Did you
> consider at least optionally providing a way where instead of using the
> new async_fn, QMP already creates a coroutine in which the handler is
> executed?

Yes, but I don't see how this could be done without the basic callback
infrastructure I propose here. Also forcing existing code to be
translated to coroutine-aware is probably even more complicated.

>
> At least for some of the block layer commands, we could simply enable
> this without changing any of the code and would automatically get rid of
> blocking the guest while the command is doing I/O. If we need to
> implement .async_fn, in contrast, it would be quite a bit of boiler
> plate code for each single handler to create a coroutine for the real
> handler and to wrap all parameters in a struct.

We could have the generator do that for you eventually, and spawn the coroutine.

>
> I started playing a bit with this and it didn't look too bad, but we
> can't move every command handler to a coroutine without auditing it, so
> I would have had to add a new option to the QAPI schema - and at that
> point I thought that I might end up duplicating a lot of your code if I
> continued. So I'm now trying to get your opinion or advice before I
> continue with anything in that direction.

thanks for looking at this old series!

>
> > This does not introduce new QMP APIs or client visible changes, the
> > command are handled in order, and the reply still come in order (even
> > when handlers finish out of order).
> >
> > 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.
>
> This part would certainly become simpler with coroutines (the marshaller
> could stay unchanged).

That's not much change honestly. I am not sure sneaking a coroutine
behind its back is going to be simpler, I would need to look at it.

>
> > 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.
>
> After converting it to QIOChannel like you already do, I/O would
> automatically become asynchronous when run in a coroutine.
>
> If you don't want this yet, but only fix the BZ, I guess you could delay
> that patch until later and just have a single yield and reenter of the
> command handler coroutine like this:
>
>     co = qemu_coroutine_self();
>     aio_co_schedule(qemu_coroutine_get_aio_context(co), co);
>     qemu_coroutine_yield();
>

If various places of code start doing that, we are in trouble, the
stack may grow, cancellation becomes hairy.

Furthermore, in the case of screendump, IO is not necessarily within
the coroutine context. In this case, we need to wait for the QXL
device to "flush" the screen. Communicating this event back to the
coroutine isn't simpler than what I propose here.

Thanks!

-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 00/25] monitor: add asynchronous command type
  2019-12-13 16:28   ` Marc-André Lureau
@ 2019-12-16 12:07     ` Kevin Wolf
  2020-01-06 18:21       ` Marc-André Lureau
  0 siblings, 1 reply; 37+ messages in thread
From: Kevin Wolf @ 2019-12-16 12:07 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: Markus Armbruster, Gerd Hoffmann, QEMU

Am 13.12.2019 um 17:28 hat Marc-André Lureau geschrieben:
> On Fri, Dec 13, 2019 at 8:04 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 08.11.2019 um 16:00 hat Marc-André Lureau geschrieben:
> > > The following series implements an internal async command solution
> > > instead. By introducing a session context and a command return
> > > handler, QMP handlers can:
> > > - defer the return, allowing the mainloop to reenter
> > > - return only to the caller (instead of the broadcast event reply)
> > > - optionnally allow cancellation when the client is gone
> > > - track on-going qapi command(s) per session
> >
> > This requires major changes to existing QMP command handlers. Did you
> > consider at least optionally providing a way where instead of using the
> > new async_fn, QMP already creates a coroutine in which the handler is
> > executed?
> 
> Yes, but I don't see how this could be done without the basic callback
> infrastructure I propose here. Also forcing existing code to be
> translated to coroutine-aware is probably even more complicated.

I'll attach what I hacked up last week after discussing a problem with
Markus and Max. As you probably expected, screendump isn't really my
main motivation to look into this. The specific command we discussed was
block_resize, which can potentially block the monitor for a while, but
I'm sure that many more block commands have the same problem.

What my patch does is moving everything into a coroutine. This is wrong
because not everything can be run in a coroutine, so it needs to be made
optional (like you did with your async flag).

The problem isn't with completely coroutine-unaware code, though: That
one would just work, even if not taking advantage from the coroutine. A
potential problem exists with code that behaves differently when run in
a coroutine or outside of coroutine context (generally by checking
qemu_in_coroutine())), or calls of coroutine-unaware code into such
functions.

Running some command handlers outside of coroutine context wouldn't be
hard to add to my patch (basically just a BH), but I haven't looked into
the QAPI side of making it an option.

> > At least for some of the block layer commands, we could simply enable
> > this without changing any of the code and would automatically get rid of
> > blocking the guest while the command is doing I/O. If we need to
> > implement .async_fn, in contrast, it would be quite a bit of boiler
> > plate code for each single handler to create a coroutine for the real
> > handler and to wrap all parameters in a struct.
> 
> We could have the generator do that for you eventually, and spawn the
> coroutine.

Yes, if we need both, that's an option. I'd like to explore first if the
callback-based approach is actually needed, though.

> > > 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.
> >
> > After converting it to QIOChannel like you already do, I/O would
> > automatically become asynchronous when run in a coroutine.
> >
> > If you don't want this yet, but only fix the BZ, I guess you could delay
> > that patch until later and just have a single yield and reenter of the
> > command handler coroutine like this:
> >
> >     co = qemu_coroutine_self();
> >     aio_co_schedule(qemu_coroutine_get_aio_context(co), co);
> >     qemu_coroutine_yield();

(This specific code is wrong, I misread your patches. You don't want to
immediately reenter the coroutine, but only in graphic_hw_update_done().)

> If various places of code start doing that, we are in trouble, the
> stack may grow, cancellation becomes hairy.

I don't understand. How does the coroutine-based approach differ from
what your series does? Basically, instead of splitting qmp_screendump()
in two parts, you keep the existing single function, just with a
qemu_coroutine_yield() in the middle, and instead of calling
qmp_screendump_finish() you wake up the coroutine.

Yes, instead of malloc'ing a struct qmp_screendump, you would keep
things on the stack, but that doesn't make the stack grow as these
things are already on the stack in qmp_screendump().

> Furthermore, in the case of screendump, IO is not necessarily within
> the coroutine context. In this case, we need to wait for the QXL
> device to "flush" the screen. Communicating this event back to the
> coroutine isn't simpler than what I propose here.

Waiting for something in a coroutine means calling
qemu_coroutine_yield() and then letting the event handler call
aio_co_wake() to resume the coroutine. It's really simple. I'm pretty
sure that if qmp_screendump() is called in a coroutine, your I/O would
automatically end up in the coroutine context unless you do something
specifically to avoid it.

Kevin


From 8d4b8ba34d33d3bbf3f0a21703928eb68895e169 Mon Sep 17 00:00:00 2001
From: Kevin Wolf <kwolf@redhat.com>
Date: Thu, 12 Dec 2019 16:32:27 +0100
Subject: [PATCH] qmp: Move dispatcher to a coroutine

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qapi/qmp/dispatch.h |  2 +
 monitor/monitor-internal.h  |  5 ++-
 monitor/monitor.c           | 24 +++++++----
 monitor/qmp.c               | 80 +++++++++++++++++++++++--------------
 qapi/qmp-dispatch.c         |  4 ++
 5 files changed, 75 insertions(+), 40 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 9aa426a398..4088255463 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -29,6 +29,8 @@ typedef enum QmpCommandOptions
 typedef struct QmpCommand
 {
     const char *name;
+    /* Runs outside of coroutine context for OOB commands, but in coroutine context
+     * for everything else. */
     QmpCommandFunc *fn;
     QmpCommandOptions options;
     QTAILQ_ENTRY(QmpCommand) node;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index d78f5ca190..7389b6a56c 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -154,7 +154,8 @@ static inline bool monitor_is_qmp(const Monitor *mon)
 
 typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
 extern IOThread *mon_iothread;
-extern QEMUBH *qmp_dispatcher_bh;
+extern Coroutine *qmp_dispatcher_co;
+extern bool qmp_dispatcher_co_busy;
 extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
 extern QemuMutex monitor_lock;
 extern MonitorList mon_list;
@@ -172,7 +173,7 @@ void monitor_fdsets_cleanup(void);
 
 void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
 void monitor_data_destroy_qmp(MonitorQMP *mon);
-void monitor_qmp_bh_dispatcher(void *data);
+void coroutine_fn monitor_qmp_dispatcher_co(void *data);
 
 int get_monitor_def(int64_t *pval, const char *name);
 void help_cmd(Monitor *mon, const char *name);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 12898b6448..8f90d0b46c 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -53,8 +53,9 @@ typedef struct {
 /* Shared monitor I/O thread */
 IOThread *mon_iothread;
 
-/* Bottom half to dispatch the requests received from I/O thread */
-QEMUBH *qmp_dispatcher_bh;
+/* Coroutine to dispatch the requests received from I/O thread */
+Coroutine *qmp_dispatcher_co;
+bool qmp_dispatcher_co_busy;
 
 /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
 QemuMutex monitor_lock;
@@ -579,9 +580,16 @@ void monitor_cleanup(void)
     }
     qemu_mutex_unlock(&monitor_lock);
 
-    /* QEMUBHs needs to be deleted before destroying the I/O thread */
-    qemu_bh_delete(qmp_dispatcher_bh);
-    qmp_dispatcher_bh = NULL;
+    /* The dispatcher needs to stop before destroying the I/O thread */
+    if (!atomic_mb_read(&qmp_dispatcher_co_busy)) {
+        aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
+        qmp_dispatcher_co = NULL;
+    }
+
+    AIO_WAIT_WHILE(qemu_get_aio_context(),
+                   aio_bh_poll(iohandler_get_aio_context()) ||
+                   atomic_mb_read(&qmp_dispatcher_co_busy));
+
     if (mon_iothread) {
         iothread_destroy(mon_iothread);
         mon_iothread = NULL;
@@ -604,9 +612,9 @@ void monitor_init_globals_core(void)
      * have commands assuming that context.  It would be nice to get
      * rid of those assumptions.
      */
-    qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
-                                   monitor_qmp_bh_dispatcher,
-                                   NULL);
+    qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL);
+    atomic_mb_set(&qmp_dispatcher_co_busy, true);
+    aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
 }
 
 QemuOptsList qemu_mon_opts = {
diff --git a/monitor/qmp.c b/monitor/qmp.c
index b67a8e7d1f..f5dc77cd0a 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -133,6 +133,8 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
     }
 }
 
+/* Runs outside of coroutine context for OOB commands, but in coroutine context
+ * for everything else. */
 static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
 {
     Monitor *old_mon;
@@ -211,43 +213,59 @@ static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void)
     return req_obj;
 }
 
-void monitor_qmp_bh_dispatcher(void *data)
+void coroutine_fn monitor_qmp_dispatcher_co(void *data)
 {
-    QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
+    QMPRequest *req_obj = NULL;
     QDict *rsp;
     bool need_resume;
     MonitorQMP *mon;
 
-    if (!req_obj) {
-        return;
-    }
+    while (true) {
+        assert(atomic_mb_read(&qmp_dispatcher_co_busy) == true);
+
+        while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) {
+            /* Wait to be reentered from handle_qmp_command, or terminate if
+             * qmp_dispatcher_co has been reset to NULL */
+            atomic_mb_set(&qmp_dispatcher_co_busy, false);
+            if (qmp_dispatcher_co) {
+                qemu_coroutine_yield();
+            } else {
+                return;
+            }
+            atomic_mb_set(&qmp_dispatcher_co_busy, true);
+        }
 
-    mon = req_obj->mon;
-    /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(mon) ||
-        mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
-    qemu_mutex_unlock(&mon->qmp_queue_lock);
-    if (req_obj->req) {
-        QDict *qdict = qobject_to(QDict, req_obj->req);
-        QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
-        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
-        monitor_qmp_dispatch(mon, req_obj->req);
-    } else {
-        assert(req_obj->err);
-        rsp = qmp_error_response(req_obj->err);
-        req_obj->err = NULL;
-        monitor_qmp_respond(mon, rsp);
-        qobject_unref(rsp);
-    }
+        aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co);
+        qemu_coroutine_yield();
+
+        mon = req_obj->mon;
+        /*  qmp_oob_enabled() might change after "qmp_capabilities" */
+        need_resume = !qmp_oob_enabled(mon) ||
+            mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
+        qemu_mutex_unlock(&mon->qmp_queue_lock);
+        if (req_obj->req) {
+            QDict *qdict = qobject_to(QDict, req_obj->req);
+            QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
+            trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
+            monitor_qmp_dispatch(mon, req_obj->req);
+        } else {
+            assert(req_obj->err);
+            rsp = qmp_error_response(req_obj->err);
+            req_obj->err = NULL;
+            monitor_qmp_respond(mon, rsp);
+            qobject_unref(rsp);
+        }
 
-    if (need_resume) {
-        /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(&mon->common);
-    }
-    qmp_request_free(req_obj);
+        if (need_resume) {
+            /* Pairs with the monitor_suspend() in handle_qmp_command() */
+            monitor_resume(&mon->common);
+        }
+        qmp_request_free(req_obj);
 
-    /* Reschedule instead of looping so the main loop stays responsive */
-    qemu_bh_schedule(qmp_dispatcher_bh);
+        /* Reschedule instead of looping so the main loop stays responsive */
+        aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
+        qemu_coroutine_yield();
+    }
 }
 
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
@@ -308,7 +326,9 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
     qemu_mutex_unlock(&mon->qmp_queue_lock);
 
     /* Kick the dispatcher routine */
-    qemu_bh_schedule(qmp_dispatcher_bh);
+    if (!atomic_mb_read(&qmp_dispatcher_co_busy)) {
+        aio_co_wake(qmp_dispatcher_co);
+    }
 }
 
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index bc264b3c9b..1dbc152307 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -75,6 +75,8 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
     return dict;
 }
 
+/* Runs outside of coroutine context for OOB commands, but in coroutine context
+ * for everything else. */
 static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
                                 bool allow_oob, Error **errp)
 {
@@ -164,6 +166,8 @@ bool qmp_is_oob(const QDict *dict)
         && !qdict_haskey(dict, "execute");
 }
 
+/* Runs outside of coroutine context for OOB commands, but in coroutine context
+ * for everything else. */
 QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
                     bool allow_oob)
 {
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 00/25] monitor: add asynchronous command type
  2019-12-16 12:07     ` Kevin Wolf
@ 2020-01-06 18:21       ` Marc-André Lureau
  2020-01-07  5:17         ` Kevin Wolf
  0 siblings, 1 reply; 37+ messages in thread
From: Marc-André Lureau @ 2020-01-06 18:21 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Markus Armbruster, Gerd Hoffmann, QEMU

Hi Kevin,

On Mon, Dec 16, 2019 at 4:07 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 13.12.2019 um 17:28 hat Marc-André Lureau geschrieben:
> > On Fri, Dec 13, 2019 at 8:04 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > Am 08.11.2019 um 16:00 hat Marc-André Lureau geschrieben:
> > > > The following series implements an internal async command solution
> > > > instead. By introducing a session context and a command return
> > > > handler, QMP handlers can:
> > > > - defer the return, allowing the mainloop to reenter
> > > > - return only to the caller (instead of the broadcast event reply)
> > > > - optionnally allow cancellation when the client is gone
> > > > - track on-going qapi command(s) per session
> > >
> > > This requires major changes to existing QMP command handlers. Did you
> > > consider at least optionally providing a way where instead of using the
> > > new async_fn, QMP already creates a coroutine in which the handler is
> > > executed?
> >
> > Yes, but I don't see how this could be done without the basic callback
> > infrastructure I propose here. Also forcing existing code to be
> > translated to coroutine-aware is probably even more complicated.
>
> I'll attach what I hacked up last week after discussing a problem with
> Markus and Max. As you probably expected, screendump isn't really my
> main motivation to look into this. The specific command we discussed was
> block_resize, which can potentially block the monitor for a while, but
> I'm sure that many more block commands have the same problem.

Thanks, that helps me to understand your proposal (I had something
more complicated in mind)

> What my patch does is moving everything into a coroutine. This is wrong
> because not everything can be run in a coroutine, so it needs to be made
> optional (like you did with your async flag).

"everything" is a bit too much ;) You proposal is to replace
qmp_dispatch_bh by a coroutine version (except for OOB commands). This
is nice because indeed, it allows to reenter the mainloop with a
simple yield in QMP commands. It is also simpler than my "async"
proposal, because some of the state is part of the coroutine, and
because it doesn't allow QMP commands concurrency (beside existing
OOB).

Iow, coroutine (for async) + oob (for concurrency) make my proposal
kinda obsolete. I can only regret that a simple callback-based
solution looked simpler to me than one that mixes both threads &
coroutines, but I don't mind if everybody likes it better :) I can
definitely see the point for block commands, which rely on coroutines
anyway, and qemu is already that complex in general.

>
> The problem isn't with completely coroutine-unaware code, though: That
> one would just work, even if not taking advantage from the coroutine. A
> potential problem exists with code that behaves differently when run in
> a coroutine or outside of coroutine context (generally by checking
> qemu_in_coroutine())), or calls of coroutine-unaware code into such
> functions.
>
> Running some command handlers outside of coroutine context wouldn't be
> hard to add to my patch (basically just a BH), but I haven't looked into
> the QAPI side of making it an option.

Yes, I think we should have a 'coroutine': true, for commands that
should be run with a coroutine.

Or perhaps replace existing allow-oob with 'dispatch':
- 'bh' (default)
- 'coroutine'
- 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at
this point)

Your patch looks quite good to me, but make check hangs. Have you looked at it?

-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 00/25] monitor: add asynchronous command type
  2020-01-06 18:21       ` Marc-André Lureau
@ 2020-01-07  5:17         ` Kevin Wolf
  2020-01-07 12:11           ` Marc-André Lureau
  0 siblings, 1 reply; 37+ messages in thread
From: Kevin Wolf @ 2020-01-07  5:17 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: Markus Armbruster, Gerd Hoffmann, QEMU

Am 06.01.2020 um 19:21 hat Marc-André Lureau geschrieben:
> > What my patch does is moving everything into a coroutine. This is wrong
> > because not everything can be run in a coroutine, so it needs to be made
> > optional (like you did with your async flag).
> 
> "everything" is a bit too much ;) You proposal is to replace
> qmp_dispatch_bh by a coroutine version (except for OOB commands). This
> is nice because indeed, it allows to reenter the mainloop with a
> simple yield in QMP commands. It is also simpler than my "async"
> proposal, because some of the state is part of the coroutine, and
> because it doesn't allow QMP commands concurrency (beside existing
> OOB).
> 
> Iow, coroutine (for async) + oob (for concurrency) make my proposal
> kinda obsolete. I can only regret that a simple callback-based
> solution looked simpler to me than one that mixes both threads &
> coroutines, but I don't mind if everybody likes it better :) I can
> definitely see the point for block commands, which rely on coroutines
> anyway, and qemu is already that complex in general.

Callbacks are indeed simple enough for implementing the infrastructure,
but for the users they only look simple as long as they do trivial
things. :-)

Anyway, now that you have seen my POC hack, do you agree that this
should help solving the screendump problem, too?

> > The problem isn't with completely coroutine-unaware code, though: That
> > one would just work, even if not taking advantage from the coroutine. A
> > potential problem exists with code that behaves differently when run in
> > a coroutine or outside of coroutine context (generally by checking
> > qemu_in_coroutine())), or calls of coroutine-unaware code into such
> > functions.
> >
> > Running some command handlers outside of coroutine context wouldn't be
> > hard to add to my patch (basically just a BH), but I haven't looked into
> > the QAPI side of making it an option.
> 
> Yes, I think we should have a 'coroutine': true, for commands that
> should be run with a coroutine.
> 
> Or perhaps replace existing allow-oob with 'dispatch':
> - 'bh' (default)
> - 'coroutine'
> - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at
> this point)

If it's "at this point", then making it two separate bools would make
more sense. But I seem to remember that OOB handlers are fundamentally
not supposed to block, so coroutine support would be pointless for them
and an enum could work.

I'll defer to Markus on this one.

> Your patch looks quite good to me, but make check hangs. Have you
> looked at it?

I'm not sure if I was looking at some qemu-iotests cases or make check,
but yes, I did see a hang. My case was a QMP command that just doesn't
work correctly inside a coroutine without modifications, so requiring
'coroutine': true would fix it.

Kevin



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 00/25] monitor: add asynchronous command type
  2020-01-07  5:17         ` Kevin Wolf
@ 2020-01-07 12:11           ` Marc-André Lureau
  2020-01-13 15:58             ` Markus Armbruster
  0 siblings, 1 reply; 37+ messages in thread
From: Marc-André Lureau @ 2020-01-07 12:11 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Markus Armbruster, Gerd Hoffmann, QEMU

Hi

On Tue, Jan 7, 2020 at 9:17 AM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 06.01.2020 um 19:21 hat Marc-André Lureau geschrieben:
> > > What my patch does is moving everything into a coroutine. This is wrong
> > > because not everything can be run in a coroutine, so it needs to be made
> > > optional (like you did with your async flag).
> >
> > "everything" is a bit too much ;) You proposal is to replace
> > qmp_dispatch_bh by a coroutine version (except for OOB commands). This
> > is nice because indeed, it allows to reenter the mainloop with a
> > simple yield in QMP commands. It is also simpler than my "async"
> > proposal, because some of the state is part of the coroutine, and
> > because it doesn't allow QMP commands concurrency (beside existing
> > OOB).
> >
> > Iow, coroutine (for async) + oob (for concurrency) make my proposal
> > kinda obsolete. I can only regret that a simple callback-based
> > solution looked simpler to me than one that mixes both threads &
> > coroutines, but I don't mind if everybody likes it better :) I can
> > definitely see the point for block commands, which rely on coroutines
> > anyway, and qemu is already that complex in general.
>
> Callbacks are indeed simple enough for implementing the infrastructure,
> but for the users they only look simple as long as they do trivial
> things. :-)
>
> Anyway, now that you have seen my POC hack, do you agree that this
> should help solving the screendump problem, too?

Yes, and I will work on it as soon as you have a working patch series
or branch :)

>
> > > The problem isn't with completely coroutine-unaware code, though: That
> > > one would just work, even if not taking advantage from the coroutine. A
> > > potential problem exists with code that behaves differently when run in
> > > a coroutine or outside of coroutine context (generally by checking
> > > qemu_in_coroutine())), or calls of coroutine-unaware code into such
> > > functions.
> > >
> > > Running some command handlers outside of coroutine context wouldn't be
> > > hard to add to my patch (basically just a BH), but I haven't looked into
> > > the QAPI side of making it an option.
> >
> > Yes, I think we should have a 'coroutine': true, for commands that
> > should be run with a coroutine.
> >
> > Or perhaps replace existing allow-oob with 'dispatch':
> > - 'bh' (default)
> > - 'coroutine'
> > - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at
> > this point)
>
> If it's "at this point", then making it two separate bools would make
> more sense. But I seem to remember that OOB handlers are fundamentally
> not supposed to block, so coroutine support would be pointless for them
> and an enum could work.

I think so too

>
> I'll defer to Markus on this one.

Yup, Markus should take a look at your proposal and give some
guidance. And hopefully, it won't take >2y.

>
> > Your patch looks quite good to me, but make check hangs. Have you
> > looked at it?
>
> I'm not sure if I was looking at some qemu-iotests cases or make check,
> but yes, I did see a hang. My case was a QMP command that just doesn't
> work correctly inside a coroutine without modifications, so requiring
> 'coroutine': true would fix it.

ok, then I suggest you do a "minimal" patch series that works.

thanks!

-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 00/25] monitor: add asynchronous command type
  2020-01-07 12:11           ` Marc-André Lureau
@ 2020-01-13 15:58             ` Markus Armbruster
  2020-01-13 16:54               ` Kevin Wolf
  0 siblings, 1 reply; 37+ messages in thread
From: Markus Armbruster @ 2020-01-13 15:58 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: Kevin Wolf, Gerd Hoffmann, QEMU

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

> Hi
>
> On Tue, Jan 7, 2020 at 9:17 AM Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> Am 06.01.2020 um 19:21 hat Marc-André Lureau geschrieben:
>> > > What my patch does is moving everything into a coroutine. This is wrong
>> > > because not everything can be run in a coroutine, so it needs to be made
>> > > optional (like you did with your async flag).
>> >
>> > "everything" is a bit too much ;) You proposal is to replace
>> > qmp_dispatch_bh by a coroutine version (except for OOB commands). This
>> > is nice because indeed, it allows to reenter the mainloop with a
>> > simple yield in QMP commands. It is also simpler than my "async"
>> > proposal, because some of the state is part of the coroutine, and
>> > because it doesn't allow QMP commands concurrency (beside existing
>> > OOB).
>> >
>> > Iow, coroutine (for async) + oob (for concurrency) make my proposal
>> > kinda obsolete. I can only regret that a simple callback-based
>> > solution looked simpler to me than one that mixes both threads &
>> > coroutines, but I don't mind if everybody likes it better :) I can
>> > definitely see the point for block commands, which rely on coroutines
>> > anyway, and qemu is already that complex in general.
>>
>> Callbacks are indeed simple enough for implementing the infrastructure,
>> but for the users they only look simple as long as they do trivial
>> things. :-)
>>
>> Anyway, now that you have seen my POC hack, do you agree that this
>> should help solving the screendump problem, too?
>
> Yes, and I will work on it as soon as you have a working patch series
> or branch :)
>
>>
>> > > The problem isn't with completely coroutine-unaware code, though: That
>> > > one would just work, even if not taking advantage from the coroutine. A
>> > > potential problem exists with code that behaves differently when run in
>> > > a coroutine or outside of coroutine context (generally by checking
>> > > qemu_in_coroutine())), or calls of coroutine-unaware code into such
>> > > functions.
>> > >
>> > > Running some command handlers outside of coroutine context wouldn't be
>> > > hard to add to my patch (basically just a BH), but I haven't looked into
>> > > the QAPI side of making it an option.
>> >
>> > Yes, I think we should have a 'coroutine': true, for commands that
>> > should be run with a coroutine.
>> >
>> > Or perhaps replace existing allow-oob with 'dispatch':
>> > - 'bh' (default)
>> > - 'coroutine'
>> > - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at
>> > this point)
>>
>> If it's "at this point", then making it two separate bools would make
>> more sense. But I seem to remember that OOB handlers are fundamentally
>> not supposed to block, so coroutine support would be pointless for them
>> and an enum could work.
>
> I think so too
>
>>
>> I'll defer to Markus on this one.
>
> Yup, Markus should take a look at your proposal and give some
> guidance. And hopefully, it won't take >2y.

Is it "[PATCH 0/4] qmp: Optionally run handlers in coroutines"?

[...]



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 00/25] monitor: add asynchronous command type
  2020-01-13 15:58             ` Markus Armbruster
@ 2020-01-13 16:54               ` Kevin Wolf
  0 siblings, 0 replies; 37+ messages in thread
From: Kevin Wolf @ 2020-01-13 16:54 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Marc-André Lureau, Gerd Hoffmann, QEMU

Am 13.01.2020 um 16:58 hat Markus Armbruster geschrieben:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 
> > Hi
> >
> > On Tue, Jan 7, 2020 at 9:17 AM Kevin Wolf <kwolf@redhat.com> wrote:
> >>
> >> Am 06.01.2020 um 19:21 hat Marc-André Lureau geschrieben:
> >> > > What my patch does is moving everything into a coroutine. This is wrong
> >> > > because not everything can be run in a coroutine, so it needs to be made
> >> > > optional (like you did with your async flag).
> >> >
> >> > "everything" is a bit too much ;) You proposal is to replace
> >> > qmp_dispatch_bh by a coroutine version (except for OOB commands). This
> >> > is nice because indeed, it allows to reenter the mainloop with a
> >> > simple yield in QMP commands. It is also simpler than my "async"
> >> > proposal, because some of the state is part of the coroutine, and
> >> > because it doesn't allow QMP commands concurrency (beside existing
> >> > OOB).
> >> >
> >> > Iow, coroutine (for async) + oob (for concurrency) make my proposal
> >> > kinda obsolete. I can only regret that a simple callback-based
> >> > solution looked simpler to me than one that mixes both threads &
> >> > coroutines, but I don't mind if everybody likes it better :) I can
> >> > definitely see the point for block commands, which rely on coroutines
> >> > anyway, and qemu is already that complex in general.
> >>
> >> Callbacks are indeed simple enough for implementing the infrastructure,
> >> but for the users they only look simple as long as they do trivial
> >> things. :-)
> >>
> >> Anyway, now that you have seen my POC hack, do you agree that this
> >> should help solving the screendump problem, too?
> >
> > Yes, and I will work on it as soon as you have a working patch series
> > or branch :)
> >
> >>
> >> > > The problem isn't with completely coroutine-unaware code, though: That
> >> > > one would just work, even if not taking advantage from the coroutine. A
> >> > > potential problem exists with code that behaves differently when run in
> >> > > a coroutine or outside of coroutine context (generally by checking
> >> > > qemu_in_coroutine())), or calls of coroutine-unaware code into such
> >> > > functions.
> >> > >
> >> > > Running some command handlers outside of coroutine context wouldn't be
> >> > > hard to add to my patch (basically just a BH), but I haven't looked into
> >> > > the QAPI side of making it an option.
> >> >
> >> > Yes, I think we should have a 'coroutine': true, for commands that
> >> > should be run with a coroutine.
> >> >
> >> > Or perhaps replace existing allow-oob with 'dispatch':
> >> > - 'bh' (default)
> >> > - 'coroutine'
> >> > - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at
> >> > this point)
> >>
> >> If it's "at this point", then making it two separate bools would make
> >> more sense. But I seem to remember that OOB handlers are fundamentally
> >> not supposed to block, so coroutine support would be pointless for them
> >> and an enum could work.
> >
> > I think so too
> >
> >>
> >> I'll defer to Markus on this one.
> >
> > Yup, Markus should take a look at your proposal and give some
> > guidance. And hopefully, it won't take >2y.
> 
> Is it "[PATCH 0/4] qmp: Optionally run handlers in coroutines"?

Yes, that's the one.

Kevin



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v6 01/25] qmp: constify QmpCommand and list
  2019-11-08 16:50   ` Damien Hedde
@ 2020-02-17 11:47     ` Marc-André Lureau
  0 siblings, 0 replies; 37+ messages in thread
From: Marc-André Lureau @ 2020-02-17 11:47 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Damien Hedde, QEMU, Gerd Hoffmann

Hi Markus,

Can you review/queue this patch?

thanks

On Fri, Nov 8, 2019 at 5:52 PM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
>
> On 11/8/19 4:00 PM, Marc-André Lureau wrote:
> > 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 3baa15f3bf..3052bfe8f1 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 d78f5ca190..3e7dac5910 100644
> > --- a/monitor/monitor-internal.h
> > +++ b/monitor/monitor-internal.h
> > @@ -132,7 +132,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 bc264b3c9b..857399c5fe 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);
> > @@ -164,7 +164,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));
> >  }
> >
>
> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
>
> Damien
>


-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, other threads:[~2020-02-17 11:53 UTC | newest]

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 15:00 [PATCH v6 00/25] monitor: add asynchronous command type Marc-André Lureau
2019-11-08 15:00 ` [PATCH v6 01/25] qmp: constify QmpCommand and list Marc-André Lureau
2019-11-08 16:50   ` Damien Hedde
2020-02-17 11:47     ` Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 02/25] json-lexer: make it safe to call destroy multiple times Marc-André Lureau
2019-11-08 16:04   ` Damien Hedde
2019-11-08 15:01 ` [PATCH v6 03/25] qmp: add QmpSession Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 04/25] QmpSession: add a return callback Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 05/25] QmpSession: add json parser and use it in qga Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 06/25] monitor: use qmp session to parse json feed Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 07/25] qga: simplify dispatch_return_cb Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 08/25] QmpSession: introduce QmpReturn Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 09/25] qmp: simplify qmp_return_error() Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 10/25] QmpSession: keep a queue of pending commands Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 11/25] QmpSession: return orderly Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 12/25] qmp: introduce asynchronous command type Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 13/25] scripts: learn 'async' qapi commands Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 14/25] qmp: add qmp_return_is_cancelled() Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 15/25] console: add graphic_hw_update_done() Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 16/25] ppm-save: pass opened fd Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 17/25] ui: add pixman image g_autoptr support Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 18/25] object: add " Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 19/25] screendump: replace FILE with QIOChannel and fix close()/qemu_close() Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 20/25] osdep: add qemu_unlink() Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 21/25] screendump: use qemu_unlink() Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 22/25] console: make screendump asynchronous Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 23/25] monitor: start making qmp_human_monitor_command() asynchronous Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 24/25] monitor: teach HMP about asynchronous commands Marc-André Lureau
2019-11-08 15:01 ` [PATCH v6 25/25] hmp: call the asynchronous QMP screendump to fix outdated/glitches Marc-André Lureau
2019-12-13 16:03 ` [PATCH v6 00/25] monitor: add asynchronous command type Kevin Wolf
2019-12-13 16:28   ` Marc-André Lureau
2019-12-16 12:07     ` Kevin Wolf
2020-01-06 18:21       ` Marc-André Lureau
2020-01-07  5:17         ` Kevin Wolf
2020-01-07 12:11           ` Marc-André Lureau
2020-01-13 15:58             ` Markus Armbruster
2020-01-13 16:54               ` Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).