On Fri, Sep 15, 2017 at 04:55:51PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu ([email protected]) 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 <[email protected]>
>
> 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
