Re: [Qemu-devel] [PATCH v8 18/23] qmp: support out-of-band (oob) execution

[Peter Xu]

On Thu, Mar 22, 2018 at 11:22:14AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Mar 9, 2018 at 10:00 AM, Peter Xu <address@hidden> 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.
> >
> > Note that in the patch I exported qmp_dispatch_check_obj() to be used to
> > check the request earlier, and at the same time allowed "id" field to be
> > there since actually we always allow that.
> >
> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  include/qapi/qmp/dispatch.h |  2 ++
> >  monitor.c                   | 84 
> > ++++++++++++++++++++++++++++++++++++++++-----
> >  qapi/qmp-dispatch.c         | 33 +++++++++++++++++-
> >  trace-events                |  2 ++
> >  4 files changed, 111 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index 26eb13ff41..ffb4652f71 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -48,6 +48,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);
> >  QObject *qmp_build_error_object(Error *err);
> > +QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp);
> > +bool qmp_is_oob(QDict *dict);
> >
> >  typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
> >
> > diff --git a/monitor.c b/monitor.c
> > index 4d57a8d341..5c8afe9f50 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1110,6 +1110,44 @@ static void qmp_caps_apply(Monitor *mon, 
> > QMPCapabilityList *list)
> >      }
> >  }
> >
> > +/*
> > + * Return true if check successful, or false otherwise.  When false is
> > + * returned, detailed error will be in errp if provided.
> > + */
> > +static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp)
> > +{
> > +    const char *command;
> > +    QmpCommand *cmd;
> > +
> > +    command = qdict_get_try_str(req, "execute");
> > +    if (!command) {
> > +        error_setg(errp, "Command field 'execute' missing");
> > +        return false;
> > +    }
> > +
> > +    cmd = qmp_find_command(mon->qmp.commands, command);
> > +    if (!cmd) {
> > +        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> > +                  "The command %s has not been found", command);
> > +        return false;
> > +    }
> > +
> > +    if (qmp_is_oob(req)) {
> > +        if (!qmp_oob_enabled(mon)) {
> > +            error_setg(errp, "Please enable Out-Of-Band first "
> > +                       "for the session during capabilities negociation");
> > +            return false;
> > +        }
> > +        if (!(cmd->options & QCO_ALLOW_OOB)) {
> > +            error_setg(errp, "The command %s does not support OOB",
> > +                       command);
> > +            return false;
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >  void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
> >                            Error **errp)
> >  {
> > @@ -4001,6 +4039,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >      QMPRequest *req_obj = monitor_qmp_requests_pop_one();
> >
> >      if (req_obj) {
> > +        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: 
> > "");
> >          monitor_qmp_dispatch_one(req_obj);
> >          /* Reschedule instead of looping so the main loop stays responsive 
> > */
> >          qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> > @@ -4024,17 +4063,31 @@ static void handle_qmp_command(JSONMessageParser 
> > *parser, GQueue *tokens)
> >          error_setg(&err, QERR_JSON_PARSING);
> >      }
> >      if (err) {
> > -        monitor_qmp_respond(mon, NULL, err, NULL);
> > -        qobject_decref(req);
> > -        return;
> > +        goto err;
> > +    }
> > +
> > +    /* Check against the request in general layout */
> > +    qdict = qmp_dispatch_check_obj(req, &err);
> > +    if (!qdict) {
> > +        goto err;
> >      }
> >
> > -    qdict = qobject_to_qdict(req);
> > -    if (qdict) {
> > -        id = qdict_get(qdict, "id");
> > -        qobject_incref(id);
> > -        qdict_del(qdict, "id");
> > -    } /* else will fail qmp_dispatch() */
> > +    /* Check against OOB specific */
> > +    if (!qmp_cmd_oob_check(mon, qdict, &err)) {
> > +        goto err;
> > +    }
> 
> This check happens before monitor_qmp_dispatch_one(), where there is
> now dead code for COMMAND_NOT_FOUND error handling (changing the error
> description), since it will error here.
> 
> A solution is to move the error manipulation code to monitor_qmp_respond():

So yes, now it'll be checked earlier now since the allow-oob is bound
to per command, then we'll need that info earlier here.  While I kept
the other check untouched since there'll be other places that calls
qmp_dispatch() too.

I'm just wondering would there be any problem even without below change?

Thanks,

> 
> 
> diff --git a/monitor.c b/monitor.c
> index 52b4c77ac3..df09a1657d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3997,6 +3997,18 @@ static void monitor_qmp_respond(Monitor *mon,
> QObject *rsp,
>              qdict_put_obj(qobject_to(QDict, rsp), "id", id);
>          }
> 
> +        if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> +            qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> +            if (qdict
> +                && !g_strcmp0(qdict_get_try_str(qdict, "class"),
> +
> QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) {
> +                /* Provide a more useful error message */
> +                qdict_del(qdict, "desc");
> +                qdict_put_str(qdict, "desc", "Expecting capabilities
> negotiation"
> +                              " with 'qmp_capabilities'");
> +            }
> +        }
> +
>          monitor_json_emitter(mon, rsp);
>      }
> 
> @@ -4022,7 +4034,6 @@ static void monitor_qmp_dispatch_one(QMPRequest
> *req_obj, bool oob_cmd)
>  {
>      Monitor *mon, *old_mon;
>      QObject *req, *rsp = NULL, *id;
> -    QDict *qdict = NULL;
>      bool need_resume;
> 
>      req = req_obj->req;
> @@ -4045,18 +4056,6 @@ static void monitor_qmp_dispatch_one(QMPRequest
> *req_obj, bool oob_cmd)
> 
>      cur_mon = old_mon;
> 
> -    if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> -        qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> -        if (qdict
> -            && !g_strcmp0(qdict_get_try_str(qdict, "class"),
> -                    QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) {
> -            /* Provide a more useful error message */
> -            qdict_del(qdict, "desc");
> -            qdict_put_str(qdict, "desc", "Expecting capabilities negotiation"
> -                          " with 'qmp_capabilities'");
> -        }
> -    }
> -
>      /* Respond if necessary */
>      monitor_qmp_respond(mon, rsp, NULL, id);
> 
> 
> > +
> > +    id = qdict_get(qdict, "id");
> > +
> > +    /* When OOB is enabled, the "id" field is mandatory. */
> > +    if (qmp_oob_enabled(mon) && !id) {
> > +        error_setg(&err, "Out-Of-Band capability requires that "
> > +                   "every command contains an 'id' field");
> > +        goto err;
> > +    }
> > +
> > +    qobject_incref(id);
> > +    qdict_del(qdict, "id");
> >
> >      req_obj = g_new0(QMPRequest, 1);
> >      req_obj->mon = mon;
> > @@ -4042,6 +4095,14 @@ static void handle_qmp_command(JSONMessageParser 
> > *parser, GQueue *tokens)
> >      req_obj->req = req;
> >      req_obj->need_resume = false;
> >
> > +    if (qmp_is_oob(qdict)) {
> > +        /* Out-Of-Band (OOB) requests are executed directly in parser. */
> > +        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
> > +                                          ?: "");
> > +        monitor_qmp_dispatch_one(req_obj);
> > +        return;
> > +    }
> > +
> >      /* Protect qmp_requests and fetching its length. */
> >      qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> >
> > @@ -4078,6 +4139,11 @@ static void handle_qmp_command(JSONMessageParser 
> > *parser, GQueue *tokens)
> >
> >      /* Kick the dispatcher routine */
> >      qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> > +    return;
> > +
> > +err:
> > +    monitor_qmp_respond(mon, NULL, err, NULL);
> > +    qobject_decref(req);
> >  }
> >
> >  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 e31ac4be1f..9a2e18c768 100644
> > --- a/qapi/qmp-dispatch.c
> > +++ b/qapi/qmp-dispatch.c
> > @@ -17,8 +17,9 @@
> >  #include "qapi/qmp/json-parser.h"
> >  #include "qapi/qmp/qdict.h"
> >  #include "qapi/qmp/qjson.h"
> > +#include "qapi/qmp/qbool.h"
> >
> > -static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
> > +QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
> >  {
> >      const QDictEntry *ent;
> >      const char *arg_name;
> > @@ -50,6 +51,14 @@ 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, "id")) {
> > +            continue;
> > +        } else if (!strcmp(arg_name, "control")) {
> > +            if (qobject_type(arg_obj) != QTYPE_QDICT) {
> > +                error_setg(errp,
> > +                           "QMP input member 'control' must be a dict");
> > +                return NULL;
> > +            }
> >          } else {
> >              error_setg(errp, "QMP input member '%s' is unexpected",
> >                         arg_name);
> > @@ -120,6 +129,28 @@ 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(QDict *dict)
> > +{
> > +    QBool *bool_obj;
> > +
> > +    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 fe10c3b487..bd036da278 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -48,6 +48,8 @@ monitor_protocol_event_queue(uint32_t event, void *qdict, 
> > uint64_t rate) "event=
> >  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_suspend(void *ptr, int cnt) "mon %p: %d"
> > +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.14.3
> >
> >
> 
> 
> 
> -- 
> Marc-André Lureau

-- 
Peter Xu