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

[Peter Xu]

On Thu, Oct 12, 2017 at 01:50:45PM +0100, Stefan Hajnoczi wrote:
> 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/

Fixed.

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

Fixed.

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

Hmm... Very possible!

Fixed.

> 
> > +    }
> > +
> > +    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?

It's a GQueue.  I assume GQueue is thread safe itself as long as
g_thread_init() is called?

Thanks,

-- 
Peter Xu