[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispa
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher |
Date: |
Mon, 26 Mar 2018 10:33:27 +0200 |
Hi
On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <address@hidden> 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
- [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, (continued)
- [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, Peter Xu, 2018/03/09
- Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, Eric Blake, 2018/03/10
- Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, Marc-André Lureau, 2018/03/21
- Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, Dr. David Alan Gilbert, 2018/03/21
- Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, Eric Blake, 2018/03/21
- Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, Marc-André Lureau, 2018/03/21
- Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, Peter Xu, 2018/03/22
- Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, Eric Blake, 2018/03/22
Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher, Marc-André Lureau, 2018/03/23
[Qemu-devel] [PATCH v8 16/23] monitor: send event when command queue full, Peter Xu, 2018/03/09
[Qemu-devel] [PATCH v8 15/23] qmp: add new event "command-dropped", Peter Xu, 2018/03/09
[Qemu-devel] [PATCH v8 17/23] qapi: introduce new cmd option "allow-oob", Peter Xu, 2018/03/09