Hi On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <pet...@redhat.com> wrote: > 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.
But monitor_qmp_dispatch_one() is called from iothread if the command is oob, so cur_mon may be updated while another command is running in main thread, or am I wrong? > > I would be far more than glad to see cur_mon go away one day. > > Thanks, > > -- > Peter Xu -- Marc-André Lureau