Hi On Sun, Apr 8, 2018 at 5:02 AM, Peter Xu <pet...@redhat.com> wrote: > On Wed, Apr 04, 2018 at 03:58:56PM +0200, Marc-André Lureau wrote: >> Hi Peter >> >> On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu <pet...@redhat.com> wrote: >> > On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote: >> >> Hi >> >> >> >> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu <pet...@redhat.com> wrote: >> >> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote: >> >> >> 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? >> >> > >> >> > You are right. I missed that, sorry... >> >> > >> >> > Would this be a simple workaround (but hopefully efficient) solution? >> >> > >> >> > diff --git a/monitor.c b/monitor.c >> >> > index 77f4c41cfa..99641c0c6d 100644 >> >> > --- a/monitor.c >> >> > +++ b/monitor.c >> >> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest; >> >> > * 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) >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool >> >> > hack_curmon) >> >> > { >> >> > Monitor *mon, *old_mon; >> >> > QObject *req, *rsp = NULL, *id; >> >> > @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest >> >> > *req_obj) >> >> > QDECREF(req_json); >> >> > } >> >> > >> >> > - old_mon = cur_mon; >> >> > - cur_mon = mon; >> >> > + if (hack_curmon) { >> >> > + old_mon = cur_mon; >> >> > + cur_mon = mon; >> >> > + } >> >> > >> >> > rsp = qmp_dispatch(mon->qmp.commands, req); >> >> > >> >> > - cur_mon = old_mon; >> >> > + if (hack_curmon) { >> >> > + cur_mon = old_mon; >> >> > + } >> >> > >> >> > if (mon->qmp.commands == &qmp_cap_negotiation_commands) { >> >> > qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error"); >> >> > @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data) >> >> > >> >> > if (req_obj) { >> >> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) >> >> > ?: ""); >> >> > - monitor_qmp_dispatch_one(req_obj); >> >> > + monitor_qmp_dispatch_one(req_obj, true); >> >> > /* Reschedule instead of looping so the main loop stays >> >> > responsive */ >> >> > qemu_bh_schedule(mon_global.qmp_dispatcher_bh); >> >> > } >> >> > @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser >> >> > *parser, GQueue *tokens) >> >> > /* Out-Of-Band (OOB) requests are executed directly in parser. >> >> > */ >> >> > >> >> > trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id) >> >> > ?: ""); >> >> > - monitor_qmp_dispatch_one(req_obj); >> >> > + monitor_qmp_dispatch_one(req_obj, false); >> >> > return; >> >> > } >> >> > >> >> > Then we forbit touching that evil cur_mon in OOB-capable command >> >> > handlers. Thanks, >> >> >> >> That's not easy to enforce though, afaict it is being used for: >> >> - error reporting decision >> > >> > IMO this should not be a problem, since any QMP handler (including >> > OOB-capable ones) will be with an Error** there, so logically speaking >> > people should never call things like error_report() in that. >> > >> >> - file & socket lookup (fd: & /dev/fdset etc) >> > >> > I suppose only very rare commands will use it? It'll be a big problem >> > to solve when we want to completely remove cur_mon though. >> > >> >> - the current state of the monitor / list of commands, cpu_path, >> >> capabilities.. >> > >> > This is very rare to be used too? Most commands should not use them AFAIU. >> > >> >> >> >> Wouldn't it be simpler to make it per-thread? I think it could also >> >> use helpers to push/pop the current monitor. >> > >> > Anyway I think yes this is still a good option (though the cur_mon >> > logic will be a bit more complicated). >> > >> > Do you plan to post some patch about this, or do you want me to do >> > this? I suppose we'll change the qemu_thread_create() a bit to pass >> > the cur_mon inside, and I suppose this might be better material after >> > 2.12 release if OOB is off now. >> >> Have you looked at making cur_mon per-thread? > > Above was my idea, nothing else has been done. > > Please feel free to post a patch for this, or I'll do this after 2.12 > release.
If it's fixed after 2.12, I think we should document the race as a known issue. -- Marc-André Lureau