On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
[...]
> > +/*
> > + * Dispatch one single QMP request. The function will free the req_obj
> > + * and objects inside it before return.
> > + */
> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > {
> > - QObject *req, *rsp = NULL, *id = NULL;
> > + Monitor *mon, *old_mon;
> > + QObject *req, *rsp = NULL, *id;
> > QDict *qdict = NULL;
> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> > -
> > - Error *err = NULL;
> > + bool need_resume;
> >
> > - 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) {
> > - goto err_out;
> > - }
> > + req = req_obj->req;
> > + mon = req_obj->mon;
> > + id = req_obj->id;
> > + need_resume = req_obj->need_resume;
> >
> > - qdict = qobject_to_qdict(req);
> > - if (qdict) {
> > - id = qdict_get(qdict, "id");
> > - qobject_incref(id);
> > - qdict_del(qdict, "id");
> > - } /* else will fail qmp_dispatch() */
> > + g_free(req_obj);
> >
> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> > QString *req_json = qobject_to_json(req);
> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser
> > *parser, GQueue *tokens)
> > old_mon = cur_mon;
> > cur_mon = mon;
>
> There is another issue with this series, since cur_mon is global (and
> not protected), an oob command may change the cur_mon while another
> command is running in the main thread with unexpected consequences. I
> don't have a clear idea what is the best way to solve it. Making the
> variable per-thread, or going all the way to get rid of cur_mon (my
> preference, but much harder)
IMHO it is fine too.
Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
which is still running in main thread. So AFAICT all the cur_mon
references are in main thread, and monitor IOThread does not modify
that variable at all. Then we should probably be safe.
I would be far more than glad to see cur_mon go away one day.
Thanks,
--
Peter Xu
