Re: [Qemu-devel] [RFC 14/15] qmp: support out-of-band (oob) execution

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@redhat.com) wrote:
> Having "allow-oob" to true for a command does not mean that this command
> will always be run in out-of-band mode.  The out-of-band quick path will
> only be executed if we specify the extra "run-oob" flag when sending the
> QMP request:
> 
>   { "execute": "command-that-allows-oob",
>     "arguments": { ... },
>     "control": { "run-oob": true } }
> 
> The "control" key is introduced to store this extra flag.  "control"
> field is used to store arguments that are shared by all the commands,
> rather than command specific arguments.  Let "run-oob" be the first.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

I don't understand how this enforces the allowoob, that is it stops
other commands being called with run-oob=true

> ---
>  docs/devel/qapi-code-gen.txt | 10 ++++++++++
>  include/qapi/qmp/dispatch.h  |  1 +
>  monitor.c                    | 11 +++++++++++
>  qapi/qmp-dispatch.c          | 34 ++++++++++++++++++++++++++++++++++
>  trace-events                 |  2 ++
>  5 files changed, 58 insertions(+)
> 
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 61fa167..47d16bb 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -665,6 +665,16 @@ allowed to run out-of-band can also be introspected using
>  query-qmp-schema command.  Please see the section "Client JSON
>  Protocol introspection" for more information.
>  
> +To execute a command in out-of-band way, we need to specify the
> +"control" field in the request, with "run-oob" set to true. Example:
> +
> + => { "execute": "command-support-oob",
> +      "arguments": { ... },
> +      "control": { "run-oob": true } }
> + <= { "return": { } }
> +
> +Without it, even the commands that supports out-of-band execution will
> +still be run in-band.
>  
>  === Events ===
>  
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index b767988..ee2b8ce 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -49,6 +49,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);
>  QObject *qmp_build_error_object(Error *err);
> +bool qmp_is_oob(const QObject *request);
>  
>  typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
>  
> diff --git a/monitor.c b/monitor.c
> index 599ea36..cb96204 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3928,6 +3928,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
>          if (!req_obj) {
>              break;
>          }
> +        trace_monitor_qmp_cmd_in_band(qobject_get_str(req_obj->id));
>          monitor_qmp_dispatch_one(req_obj);
>      }
>  }
> @@ -3963,6 +3964,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
>      req_obj->id = id;
>      req_obj->req = req;
>  
> +    if (qmp_is_oob(req)) {
> +        /*
> +         * Trigger fast-path to handle the out-of-band request, by
> +         * executing the command directly in parser.
> +         */
> +        trace_monitor_qmp_cmd_out_of_band(qobject_get_str(req_obj->id));
> +        monitor_qmp_dispatch_one(req_obj);
> +        return;
> +    }

I wonder if there is a way to run all allowoob commands in this way;
the only difference being if they're not started with run-oob
you wiat for completion of !oob commands.
That way the allowoob commands are always run from the same
thread/context which feels like it should simplify them.

Dave

>      /*
>       * Put the request to the end of queue so that requests will be
>       * handled in time order.  Ownership for req_obj, req, id,
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index b41fa17..9a05dfa 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -52,6 +52,12 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>                             "QMP input member 'arguments' must be an object");
>                  return NULL;
>              }
> +        } else if (!strcmp(arg_name, "control")) {
> +            if (qobject_type(arg_obj) != QTYPE_QDICT) {
> +                error_setg(errp,
> +                           "QMP input member 'control' must be an object");
> +                return NULL;
> +            }
>          } else {
>              error_setg(errp, "QMP input member '%s' is unexpected",
>                         arg_name);
> @@ -122,6 +128,34 @@ QObject *qmp_build_error_object(Error *err)
>                                error_get_pretty(err));
>  }
>  
> +/*
> + * Detect whether a request should be run out-of-band, by quickly
> + * peeking at whether we have: { "control": { "run-oob": True } }. By
> + * default commands are run in-band.
> + */
> +bool qmp_is_oob(const QObject *request)
> +{
> +    QDict *dict;
> +    QBool *bool_obj;
> +
> +    dict = qobject_to_qdict(request);
> +    if (!dict) {
> +        return false;
> +    }
> +
> +    dict = qdict_get_qdict(dict, "control");
> +    if (!dict) {
> +        return false;
> +    }
> +
> +    bool_obj = qobject_to_qbool(qdict_get(dict, "run-oob"));
> +    if (!bool_obj) {
> +        return false;
> +    }
> +
> +    return qbool_get_bool(bool_obj);
> +}
> +
>  QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request)
>  {
>      Error *err = NULL;
> diff --git a/trace-events b/trace-events
> index 1f50f56..f7900a6 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -47,6 +47,8 @@ monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
>  monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64
>  handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s"
>  handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
> +monitor_qmp_cmd_in_band(const char *id) "%s"
> +monitor_qmp_cmd_out_of_band(const char *id) "%s"
>  
>  # dma-helpers.c
>  dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK