Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> The correct way to set the current monitor for a coroutine handler will
> be different than for a blocking handler, so monitor_set_cur() needs to
> be called in qmp_dispatch().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qapi/qmp/dispatch.h | 3 ++-
>  monitor/qmp.c               | 8 +-------
>  qapi/qmp-dispatch.c         | 8 +++++++-
>  qga/main.c                  | 2 +-
>  stubs/monitor-core.c        | 5 +++++
>  tests/test-qmp-cmds.c       | 6 +++---
>  6 files changed, 19 insertions(+), 13 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);
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 8469970c69..922fdb5541 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -135,16 +135,10 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict 
> *rsp)
>  
>  static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>  {
> -    Monitor *old_mon;
>      QDict *rsp;
>      QDict *error;
>  
> -    old_mon = monitor_set_cur(&mon->common);
> -    assert(old_mon == NULL);
> -
> -    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.  Happy to wrap it in my tree.  A few more in PATCH 08-11.

>  
>      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 3febf3b0fd..241779a1d6 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -578,7 +578,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);

I dislike how this entangles qemu-ga even more with the monitor.  But I
want to get this wrapped more than I want it to be spotless.  Perhaps I
can clean up some on top.

[...]