From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59834) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1f0NAb-0001YR-FE for qemu-devel@nongnu.org; Mon, 26 Mar 2018 04:07:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1f0NAW-0008RO-Da for qemu-devel@nongnu.org; Mon, 26 Mar 2018 04:07:57 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:34698 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1f0NAW-0008Qr-7r for qemu-devel@nongnu.org; Mon, 26 Mar 2018 04:07:52 -0400 Date: Mon, 26 Mar 2018 16:07:38 +0800 From: Peter Xu Message-ID: <20180326080738.GA17789@xz-mi> References: <20180309090006.10018-1-peterx@redhat.com> <20180309090006.10018-15-peterx@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: QEMU , Laurent Vivier , Fam Zheng , Juan Quintela , Michael Roth , Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini , "Dr . David Alan Gilbert" On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-Andr=C3=A9 Lureau wrote: [...] > > +/* > > + * Dispatch one single QMP request. The function will free the req_o= bj > > + * and objects inside it before return. > > + */ > > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj) > > { > > - QObject *req, *rsp =3D NULL, *id =3D NULL; > > + Monitor *mon, *old_mon; > > + QObject *req, *rsp =3D NULL, *id; > > QDict *qdict =3D NULL; > > - MonitorQMP *mon_qmp =3D container_of(parser, MonitorQMP, parser)= ; > > - Monitor *old_mon, *mon =3D container_of(mon_qmp, Monitor, qmp); > > - > > - Error *err =3D NULL; > > + bool need_resume; > > > > - req =3D 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 =3D req_obj->req; > > + mon =3D req_obj->mon; > > + id =3D req_obj->id; > > + need_resume =3D req_obj->need_resume; > > > > - qdict =3D qobject_to_qdict(req); > > - if (qdict) { > > - id =3D 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 =3D qobject_to_json(req); > > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParse= r *parser, GQueue *tokens) > > old_mon =3D cur_mon; > > cur_mon =3D mon; >=20 > 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, --=20 Peter Xu