Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher

[Peter Xu]

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 <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?

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,

-- 
Peter Xu