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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

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/

> +     */
> +    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.

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

> @@ -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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]