Re: [Qemu-devel] [RFC v6 16/27] monitor: separate QMP parser and dispatcher

[Peter Xu]

On Mon, Jan 08, 2018 at 05:09:16PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:46PM +0800, Peter Xu wrote:
> > Originally QMP goes throw these steps:
> 
> s/throw/through/
> 
> > 
> >   JSON Parser --> QMP Dispatcher --> Respond
> >       /|\    (2)                (3)     |
> >    (1) |                               \|/ (4)
> >        +---------  main thread  --------+
> > 
> > This patch does this:
> > 
> >   JSON Parser     QMP Dispatcher --> Respond
> >       /|\ |           /|\       (4)     |
> >        |  | (2)        | (3)            |  (5)
> >    (1) |  +----->      |               \|/
> >        +---------  main thread  <-------+
> > 
> > So the parsing job and the dispatching job is isolated now.  It gives us
> > a chance in following up patches to totally move the parser outside.
> > 
> > The isloation is done using one QEMUBH. Only one dispatcher QEMUBH is
> 
> s/isloation/isolation/
> 
> > @@ -3897,30 +3920,39 @@ static void monitor_qmp_respond(Monitor *mon, 
> > QObject *rsp,
> >      qobject_decref(rsp);
> >  }
> >  
> > -static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > +struct QMPRequest {
> > +    /* Owner of the request */
> > +    Monitor *mon;
> > +    /* "id" field of the request */
> > +    QObject *id;
> > +    /* Request object to be handled */
> > +    QObject *req;
> > +    /*
> > +     * Whether we need to resume the monitor afterward.  This flag is
> > +     * used to emulate the old QMP server behavior that only one
> > +     * command is executed for each time.
> 
> s/only one command is executed for each time/the current command must
> complete before the next one executes/

Will address all the grammar issues.

> 
> > +     */
> > +    bool need_resume;
> 
> This isn't really a per-request decision so a QMPRequest field is not
> necessary.  If "oob" is enabled then we dispatch commands without
> waiting.  If "oob" is disabled then we complete the current command
> before dispatching the next one.

I explicitly added this to make sure the suspend and resume will be
paired up.  Actually there is at least one exception, which is the
initial "qmp_capabilities" command especially when used to enable the
OOB capability. For that case, OOB is not enabled before execution,
while OOB is enabled during the execution and after running the
command.  If without this field, monitor can hang.

> 
> If you want to keep this, I don't mind, but the field isn't necessary.

According to above, I would still like to keep it.

> 
> > @@ -4150,6 +4292,15 @@ static void monitor_iothread_init(void)
> >  {
> >      mon_global.mon_iothread = iothread_create("mon_iothread",
> >                                                &error_abort);
> > +
> > +    /*
> > +     * This MUST be on main loop thread since we have commands that
> > +     * have assumption to be run on main loop thread (Yeah, we'd
> > +     * better remove this assumption in the future).
> 
> "we'd better ..." usually means that it will be necessary.  It is
> stronger than "it would be nice to ...".  I'm not sure which one you
> mean here.
> 
> There doesn't seem to be any immediate need or plan to run regular
> commands outside the main loop.  I'm not aware of active work to do
> that.  So what is this comment trying to say?

Yeah I mean "it would be nice to", I'll switch.

This comment explains why dispatcher bottom half is bound to the
default main thread and it cannot be bound to anything else.  Thanks,

-- 
Peter Xu