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 <pet...@redhat.com>
> > ---
> > 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
