Re: [Qemu-devel] [RFC v2 13/22] monitor: separate QMP parser and dispatcher

[Stefan Hajnoczi]

On Fri, Sep 29, 2017 at 11:38:35AM +0800, Peter Xu wrote:
> Originally QMP is going throw these steps:

s/is going throw/goes 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
> used for all the monitors.
> 
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  monitor.c | 156 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 133 insertions(+), 23 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 7b76dff5ad..1e9a6cb6a5 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -208,10 +208,14 @@ struct Monitor {
>      mon_cmd_t *cmd_table;
>      QLIST_HEAD(,mon_fd_t) fds;
>      QTAILQ_ENTRY(Monitor) entry;
> +    /* Input queue that hangs all the parsed QMP requests */

s/hangs/holds/

> +static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
> +                               void *opaque)
> +{
> +    QObject *req, *id = NULL;
> +    QDict *qdict = NULL;
> +    Monitor *mon = opaque;
> +    Error *err = NULL;
> +    QMPRequest *req_obj;
> +
> +    req = json_parser_parse_err(tokens, NULL, &err);
> +    if (!req && !err) {
> +        /* json_parser_parse_err() sucks: can fail without setting @err */
> +        error_setg(&err, QERR_JSON_PARSING);
> +    }
> +    if (err) {
> +        monitor_qmp_respond(mon, NULL, err, NULL);
> +        qobject_decref(req);

Is there a return statement missing here?

> +    }
> +
> +    qdict = qobject_to_qdict(req);
> +    if (qdict) {
> +        id = qdict_get(qdict, "id");
> +        qobject_incref(id);
> +        qdict_del(qdict, "id");
> +    } /* else will fail qmp_dispatch() */
> +
> +    req_obj = g_new0(QMPRequest, 1);
> +    req_obj->mon = mon;
> +    req_obj->id = id;
> +    req_obj->req = req;
> +
> +    /*
> +     * Put the request to the end of queue so that requests will be
> +     * handled in time order.  Ownership for req_obj, req, id,
> +     * etc. will be delivered to the handler side.
> +     */
> +    g_queue_push_tail(mon->qmp_requests, req_obj);
> +
> +    /* Kick the dispatcher routine */
> +    qemu_bh_schedule(mon_global.qmp_dispatcher_bh);

How is thread-safety ensured when accessing qmp_requests?