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

[Peter Xu]

On Fri, Sep 15, 2017 at 04:55:51PM +0100, Dr. David Alan Gilbert wrote:
> * 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.
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> 
> I don't understand how this enforces the allowoob, that is it stops
> other commands being called with run-oob=true

Here's what I thought:

OOB commands are executed directly in the parser, and currently we
only have one single parser/IO thread, then we can't have two oob
commands in parallel, can we? Say, if we got one OOB command, we won't
handle anything else (no matter OOB or non-OOB) before we finished
processing that OOB command.

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

Maybe I misread the comment... Even with current patch, all OOB
commands will be run from the same thread/context (which is the newly
created parser thread), right?  Thanks,

-- 
Peter Xu