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
- file & socket lookup (fd: & /dev/fdset etc)
- the current state of the monitor / list of commands, cpu_path, capabilities..
Wouldn't it be simpler to make it per-thread? I think it could also
use helpers to push/pop the current monitor.
--
Marc-André Lureau
