[v6,05/12] qmp: Call monitor_set_cur() only in qmp_dispatch()
diff mbox series

Message ID 20200528153742.274164-6-kwolf@redhat.com
State New
Headers show
Series
  • monitor: Optionally run handlers in coroutines
Related show

Commit Message

Kevin Wolf May 28, 2020, 3:37 p.m. UTC
The correct way to set the current monitor for a coroutine handler is
different that for a blocking handler, so monitor_set_cur() can only be
called in qmp_dispatch().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qapi/qmp/dispatch.h | 3 ++-
 monitor/qmp.c               | 7 +------
 qapi/qmp-dispatch.c         | 8 +++++++-
 qga/main.c                  | 2 +-
 stubs/monitor-core.c        | 4 ++++
 tests/test-qmp-cmds.c       | 6 +++---
 6 files changed, 18 insertions(+), 12 deletions(-)

Comments

Eric Blake May 28, 2020, 6:42 p.m. UTC | #1
On 5/28/20 10:37 AM, Kevin Wolf wrote:
> The correct way to set the current monitor for a coroutine handler is
> different that for a blocking handler, so monitor_set_cur() can only be

s/that/than/

> called in qmp_dispatch().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/qapi/qmp/dispatch.h | 3 ++-
>   monitor/qmp.c               | 7 +------
>   qapi/qmp-dispatch.c         | 8 +++++++-
>   qga/main.c                  | 2 +-
>   stubs/monitor-core.c        | 4 ++++
>   tests/test-qmp-cmds.c       | 6 +++---
>   6 files changed, 18 insertions(+), 12 deletions(-)
> 

> +++ b/stubs/monitor-core.c
> @@ -8,6 +8,10 @@ Monitor *monitor_cur(void)
>       return NULL;
>   }
>   
> +void monitor_set_cur(Monitor *mon)
> +{
> +}
> +

Should this stub assert(!mon)?

Otherwise looks reasonable.
Markus Armbruster Aug. 4, 2020, 1:17 p.m. UTC | #2
Kevin Wolf <kwolf@redhat.com> writes:

> The correct way to set the current monitor for a coroutine handler is
> different that for a blocking handler, so monitor_set_cur() can only be

will be different

> called in qmp_dispatch().

needs to be called in

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qapi/qmp/dispatch.h | 3 ++-
>  monitor/qmp.c               | 7 +------
>  qapi/qmp-dispatch.c         | 8 +++++++-
>  qga/main.c                  | 2 +-
>  stubs/monitor-core.c        | 4 ++++
>  tests/test-qmp-cmds.c       | 6 +++---
>  6 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 5a9cf82472..0c2f467028 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -14,6 +14,7 @@
>  #ifndef QAPI_QMP_DISPATCH_H
>  #define QAPI_QMP_DISPATCH_H
>  
> +#include "monitor/monitor.h"
>  #include "qemu/queue.h"
>  
>  typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
> @@ -49,7 +50,7 @@ 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,
> -                    bool allow_oob);
> +                    bool allow_oob, Monitor *cur_mon);
>  bool qmp_is_oob(const QDict *dict);
>  
>  typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque);

Tying dispatch to the current monitor when two out of three users don't
have a monitor is ugly.  Let's not worry about that now.

> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index a04c512e3a..922fdb5541 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -138,12 +138,7 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>      QDict *rsp;
>      QDict *error;
>  
> -    assert(monitor_cur() == NULL);
> -    monitor_set_cur(&mon->common);
> -
> -    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
> -
> -    monitor_set_cur(NULL);
> +    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), &mon->common);

Long line.

>  
>      if (mon->commands == &qmp_cap_negotiation_commands) {
>          error = qdict_get_qdict(rsp, "error");
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 79347e0864..2fdbc0fba4 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -89,7 +89,7 @@ bool qmp_is_oob(const QDict *dict)
>  }
>  
>  QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> -                    bool allow_oob)
> +                    bool allow_oob, Monitor *cur_mon)
>  {
>      Error *err = NULL;
>      bool oob;
> @@ -152,7 +152,13 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
>          args = qdict_get_qdict(dict, "arguments");
>          qobject_ref(args);
>      }
> +
> +    assert(monitor_cur() == NULL);
> +    monitor_set_cur(cur_mon);
> +
>      cmd->fn(args, &ret, &err);
> +
> +    monitor_set_cur(NULL);
>      qobject_unref(args);
>      if (err) {
>          /* or assert(!ret) after reviewing all handlers: */
> diff --git a/qga/main.c b/qga/main.c
> index f0e454f28d..1042c4e7d3 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -574,7 +574,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(&ga_commands, obj, false, NULL);
>  
>  end:
>      ret = send_response(s, rsp);
> diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> index 0cd2d864b2..e493df1027 100644
> --- a/stubs/monitor-core.c
> +++ b/stubs/monitor-core.c
> @@ -8,6 +8,10 @@ Monitor *monitor_cur(void)
>      return NULL;
>  }
>  
> +void monitor_set_cur(Monitor *mon)
> +{
> +}
> +
>  void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>  {
>  }
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index d12ff47e26..5f1b245e19 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -152,7 +152,7 @@ static QObject *do_qmp_dispatch(bool allow_oob, const char *template, ...)
>      req = qdict_from_vjsonf_nofail(template, ap);
>      va_end(ap);
>  
> -    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob);
> +    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
>      g_assert(resp);
>      ret = qdict_get(resp, "return");
>      g_assert(ret);
> @@ -175,7 +175,7 @@ static void do_qmp_dispatch_error(bool allow_oob, ErrorClass cls,
>      req = qdict_from_vjsonf_nofail(template, ap);
>      va_end(ap);
>  
> -    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob);
> +    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
>      g_assert(resp);
>      error = qdict_get_qdict(resp, "error");
>      g_assert(error);
> @@ -231,7 +231,7 @@ static void test_dispatch_cmd_success_response(void)
>      QDict *resp;
>  
>      qdict_put_str(req, "execute", "cmd-success-response");
> -    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
> +    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false, NULL);
>      g_assert_null(resp);
>      qobject_unref(req);
>  }

This works because qmp_dispatch() doesn't bother to check its @mon
argument is sane, and the test's commands don't use monitor_cur().

Patch
diff mbox series

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 5a9cf82472..0c2f467028 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -14,6 +14,7 @@ 
 #ifndef QAPI_QMP_DISPATCH_H
 #define QAPI_QMP_DISPATCH_H
 
+#include "monitor/monitor.h"
 #include "qemu/queue.h"
 
 typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
@@ -49,7 +50,7 @@  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,
-                    bool allow_oob);
+                    bool allow_oob, Monitor *cur_mon);
 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 a04c512e3a..922fdb5541 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -138,12 +138,7 @@  static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
     QDict *rsp;
     QDict *error;
 
-    assert(monitor_cur() == NULL);
-    monitor_set_cur(&mon->common);
-
-    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
-
-    monitor_set_cur(NULL);
+    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), &mon->common);
 
     if (mon->commands == &qmp_cap_negotiation_commands) {
         error = qdict_get_qdict(rsp, "error");
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 79347e0864..2fdbc0fba4 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -89,7 +89,7 @@  bool qmp_is_oob(const QDict *dict)
 }
 
 QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
-                    bool allow_oob)
+                    bool allow_oob, Monitor *cur_mon)
 {
     Error *err = NULL;
     bool oob;
@@ -152,7 +152,13 @@  QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
         args = qdict_get_qdict(dict, "arguments");
         qobject_ref(args);
     }
+
+    assert(monitor_cur() == NULL);
+    monitor_set_cur(cur_mon);
+
     cmd->fn(args, &ret, &err);
+
+    monitor_set_cur(NULL);
     qobject_unref(args);
     if (err) {
         /* or assert(!ret) after reviewing all handlers: */
diff --git a/qga/main.c b/qga/main.c
index f0e454f28d..1042c4e7d3 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -574,7 +574,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(&ga_commands, obj, false, NULL);
 
 end:
     ret = send_response(s, rsp);
diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index 0cd2d864b2..e493df1027 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -8,6 +8,10 @@  Monitor *monitor_cur(void)
     return NULL;
 }
 
+void monitor_set_cur(Monitor *mon)
+{
+}
+
 void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
 {
 }
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index d12ff47e26..5f1b245e19 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -152,7 +152,7 @@  static QObject *do_qmp_dispatch(bool allow_oob, const char *template, ...)
     req = qdict_from_vjsonf_nofail(template, ap);
     va_end(ap);
 
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob);
+    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
     g_assert(resp);
     ret = qdict_get(resp, "return");
     g_assert(ret);
@@ -175,7 +175,7 @@  static void do_qmp_dispatch_error(bool allow_oob, ErrorClass cls,
     req = qdict_from_vjsonf_nofail(template, ap);
     va_end(ap);
 
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob);
+    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
     g_assert(resp);
     error = qdict_get_qdict(resp, "error");
     g_assert(error);
@@ -231,7 +231,7 @@  static void test_dispatch_cmd_success_response(void)
     QDict *resp;
 
     qdict_put_str(req, "execute", "cmd-success-response");
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false, NULL);
     g_assert_null(resp);
     qobject_unref(req);
 }