From c06982122036546fcbfe40f5b22ae7088d28c9a2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:27 +0200 Subject: qmp: Say "out-of-band" instead of "Out-Of-Band" Affects documentation and a few error messages. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-2-armbru@redhat.com> --- monitor.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 3a0ea0c..208d7c7 100644 --- a/monitor.c +++ b/monitor.c @@ -1268,11 +1268,11 @@ static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list, case QMP_CAPABILITY_OOB: if (!mon->use_io_thr) { /* - * Out-Of-Band only works with monitors that are + * Out-of-band only works with monitors that are * running on dedicated IOThread. */ error_setg(errp, "This monitor does not support " - "Out-Of-Band (OOB)"); + "out-of-band (OOB)"); return; } break; @@ -1320,7 +1320,7 @@ static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp) if (qmp_is_oob(req)) { if (!qmp_oob_enabled(mon)) { - error_setg(errp, "Please enable Out-Of-Band first " + error_setg(errp, "Please enable out-of-band first " "for the session during capabilities negotiation"); return false; } @@ -4294,7 +4294,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) /* When OOB is enabled, the "id" field is mandatory. */ if (qmp_oob_enabled(mon) && !id) { - error_setg(&err, "Out-Of-Band capability requires that " + error_setg(&err, "Out-of-band capability requires that " "every command contains an 'id' field"); goto err; } @@ -4308,7 +4308,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) qdict_del(qdict, "id"); if (qmp_is_oob(qdict)) { - /* Out-Of-Band (OOB) requests are executed directly in parser. */ + /* 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); @@ -4684,12 +4684,12 @@ void monitor_init(Chardev *chr, int flags) if (use_oob) { if (CHARDEV_IS_MUX(chr)) { - error_report("Monitor Out-Of-Band is not supported with " + error_report("Monitor out-of-band is not supported with " "MUX typed chardev backend"); exit(1); } if (use_readline) { - error_report("Monitor Out-Of-band is only supported by QMP"); + error_report("Monitor out-of-band is only supported by QMP"); exit(1); } } -- cgit v1.1 From c5f57ed026bbe6817b17dbc842e17fd23da1b607 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:28 +0200 Subject: monitor: Spell "I/O thread" consistently in comments Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-3-armbru@redhat.com> --- monitor.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 208d7c7..4f4f309 100644 --- a/monitor.c +++ b/monitor.c @@ -243,7 +243,7 @@ struct Monitor { /* Let's add monitor global variables to this struct. */ static struct { IOThread *mon_iothread; - /* Bottom half to dispatch the requests received from IO thread */ + /* Bottom half to dispatch the requests received from I/O thread */ QEMUBH *qmp_dispatcher_bh; /* Bottom half to deliver the responses back to clients */ QEMUBH *qmp_respond_bh; @@ -518,7 +518,7 @@ static void monitor_json_emitter(Monitor *mon, QObject *data) { if (mon->use_io_thr) { /* - * If using IO thread, we need to queue the item so that IO + * If using I/O thread, we need to queue the item so that I/O * thread will do the rest for us. Take refcount so that * caller won't free the data (which will be finally freed in * responder thread). @@ -529,7 +529,7 @@ static void monitor_json_emitter(Monitor *mon, QObject *data) qemu_bh_schedule(mon_global.qmp_respond_bh); } else { /* - * If not using monitor IO thread, then we are in main thread. + * If not using monitor I/O thread, then we are in main thread. * Do the emission right away. */ monitor_json_emitter_raw(mon, data); @@ -1269,7 +1269,7 @@ static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list, if (!mon->use_io_thr) { /* * Out-of-band only works with monitors that are - * running on dedicated IOThread. + * running on dedicated I/O thread. */ error_setg(errp, "This monitor does not support " "out-of-band (OOB)"); @@ -4403,7 +4403,7 @@ int monitor_suspend(Monitor *mon) if (monitor_is_qmp(mon)) { /* - * Kick iothread to make sure this takes effect. It'll be + * Kick I/O thread to make sure this takes effect. It'll be * evaluated again in prepare() of the watch object. */ aio_notify(iothread_get_aio_context(mon_global.mon_iothread)); @@ -4422,7 +4422,7 @@ void monitor_resume(Monitor *mon) if (atomic_dec_fetch(&mon->suspend_cnt) == 0) { if (monitor_is_qmp(mon)) { /* - * For QMP monitors that are running in IOThread, let's + * For QMP monitors that are running in I/O thread, let's * kick the thread in case it's sleeping. */ if (mon->use_io_thr) { @@ -4446,7 +4446,7 @@ static QObject *get_qmp_greeting(Monitor *mon) for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) { if (!mon->use_io_thr && cap == QMP_CAPABILITY_OOB) { - /* Monitors that are not using IOThread won't support OOB */ + /* Monitors that are not using I/O thread won't support OOB */ continue; } qlist_append_str(cap_list, QMPCapability_str(cap)); @@ -4587,9 +4587,9 @@ static void monitor_iothread_init(void) NULL); /* - * Unlike the dispatcher BH, this must be run on the monitor IO - * thread, so that monitors that are using IO thread will make - * sure read/write operations are all done on the IO thread. + * Unlike the dispatcher BH, this must be run on the monitor I/O + * thread, so that monitors that are using I/O thread will make + * sure read/write operations are all done on the I/O thread. */ mon_global.qmp_respond_bh = aio_bh_new(monitor_get_aio_context(), monitor_qmp_bh_responder, @@ -4661,7 +4661,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) if (mon->use_io_thr) { /* * When use_io_thr is set, we use the global shared dedicated - * IO thread for this monitor to handle input/output. + * I/O thread for this monitor to handle input/output. */ context = monitor_get_io_context(); /* We should have inited globals before reaching here. */ @@ -4718,7 +4718,7 @@ void monitor_init(Chardev *chr, int flags) /* * We can't call qemu_chr_fe_set_handlers() directly here * since during the procedure the chardev will be active - * and running in monitor iothread, while we'll still do + * and running in monitor I/O thread, while we'll still do * something before returning from it, which is a possible * race too. To avoid that, we just create a BH to setup * the handlers. @@ -4745,16 +4745,16 @@ void monitor_cleanup(void) Monitor *mon, *next; /* - * We need to explicitly stop the iothread (but not destroy it), - * cleanup the monitor resources, then destroy the iothread since + * We need to explicitly stop the I/O thread (but not destroy it), + * cleanup the monitor resources, then destroy the I/O thread since * we need to unregister from chardev below in * monitor_data_destroy(), and chardev is not thread-safe yet */ iothread_stop(mon_global.mon_iothread); /* - * After we have IOThread to send responses, it's possible that - * when we stop the IOThread there are still replies queued in the + * After we have I/O thread to send responses, it's possible that + * when we stop the I/O thread there are still replies queued in the * responder queue. Flush all of them. Note that even after this * flush it's still possible that out buffer is not flushed. * It'll be done in below monitor_flush() as the last resort. @@ -4770,7 +4770,7 @@ void monitor_cleanup(void) } qemu_mutex_unlock(&monitor_lock); - /* QEMUBHs needs to be deleted before destroying the IOThread. */ + /* QEMUBHs needs to be deleted before destroying the I/O thread */ qemu_bh_delete(mon_global.qmp_dispatcher_bh); mon_global.qmp_dispatcher_bh = NULL; qemu_bh_delete(mon_global.qmp_respond_bh); -- cgit v1.1 From d621cfe0a177978b17711a712293221294430f53 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:30 +0200 Subject: qmp: Document COMMAND_DROPPED design flaw Events are broadcast to all monitors. If another monitor's client has a command with the same ID in flight, the event will incorrectly claim that command was dropped. This must be fixed before out-of-band execution can graduate from "experimental". Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-5-armbru@redhat.com> --- monitor.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 4f4f309..3ad89fe 100644 --- a/monitor.c +++ b/monitor.c @@ -4331,6 +4331,12 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) /* Drop the request if queue is full. */ if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) { qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); + /* + * FIXME @id's scope is just @mon, and broadcasting it is + * wrong. If another monitor's client has a command with + * the same ID in flight, the event will incorrectly claim + * that command was dropped. + */ qapi_event_send_command_dropped(id, COMMAND_DROP_REASON_QUEUE_FULL, &error_abort); -- cgit v1.1 From 80cd93bd966bbb0907caa7f1d5676342f27f8f9e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:33 +0200 Subject: qmp: Make "id" optional again even in "oob" monitors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit cf869d53172 "qmp: support out-of-band (oob) execution" made "id" mandatory for all commands when the client accepted capability "oob". This is rather onerous when you play with QMP by hand, and unnecessarily so: only out-of-band commands need an ID for reliable matching of response to command. Revert that part of commit cf869d53172 for now, but have documentation advise on the need to use "id" with out-of-band commands. Signed-off-by: Markus Armbruster Message-Id: <20180703085358.13941-8-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé Reviewed-by: Eric Blake --- monitor.c | 7 ------- 1 file changed, 7 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 3ad89fe..0dd6e22 100644 --- a/monitor.c +++ b/monitor.c @@ -4292,13 +4292,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) id = qdict_get(qdict, "id"); - /* When OOB is enabled, the "id" field is mandatory. */ - if (qmp_oob_enabled(mon) && !id) { - error_setg(&err, "Out-of-band capability requires that " - "every command contains an 'id' field"); - goto err; - } - req_obj = g_new0(QMPRequest, 1); req_obj->mon = mon; req_obj->id = qobject_ref(id); -- cgit v1.1 From 0fa39d0b0374b983535de8591e5e561401d1d5c6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:35 +0200 Subject: qmp qemu-ga: Revert change that accidentally made qemu-ga accept "id" Commit cf869d53172 "qmp: support out-of-band (oob) execution" changed how we check "id": Note that in the patch I exported qmp_dispatch_check_obj() to be used to check the request earlier, and at the same time allowed "id" field to be there since actually we always allow that. The part after "and" is ill-advised: it makes qemu-ga accept and ignore "id". Revert. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-10-armbru@redhat.com> --- monitor.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 0dd6e22..7245ee3 100644 --- a/monitor.c +++ b/monitor.c @@ -4264,7 +4264,7 @@ static void monitor_qmp_bh_dispatcher(void *data) static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) { QObject *req, *id = NULL; - QDict *qdict = NULL; + QDict *qdict; MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser); Monitor *mon = container_of(mon_qmp, Monitor, qmp); Error *err = NULL; @@ -4279,6 +4279,12 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) goto err; } + qdict = qobject_to(QDict, req); + if (qdict) { + id = qobject_ref(qdict_get(qdict, "id")); + qdict_del(qdict, "id"); + } /* else will fail qmp_dispatch() */ + /* Check against the request in general layout */ qdict = qmp_dispatch_check_obj(req, &err); if (!qdict) { @@ -4290,16 +4296,12 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) goto err; } - id = qdict_get(qdict, "id"); - req_obj = g_new0(QMPRequest, 1); req_obj->mon = mon; - req_obj->id = qobject_ref(id); + req_obj->id = id; req_obj->req = req; req_obj->need_resume = false; - qdict_del(qdict, "id"); - if (qmp_is_oob(qdict)) { /* Out-of-band (OOB) requests are executed directly in parser. */ trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id) -- cgit v1.1 From 674ed7228f03150d15703961ea2a59cd744f3beb Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:37 +0200 Subject: qmp qemu-ga: Fix qemu-ga not to accept "control" Commit cf869d53172 "qmp: support out-of-band (oob) execution" accidentally made qemu-ga accept and ignore "control". Fix that. Out-of-band execution in a monitor that doesn't support it now fails with {"error": {"class": "GenericError", "desc": "QMP input member 'control' is unexpected"}} instead of {"error": {"class": "GenericError", "desc": "Please enable out-of-band first for the session during capabilities negotiation"}} The old description is suboptimal when out-of-band cannot not be enabled, or the command doesn't support out-of-band execution. The new description is a bit unspecific, but it'll do. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-12-armbru@redhat.com> --- monitor.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 7245ee3..2e443bb 100644 --- a/monitor.c +++ b/monitor.c @@ -1319,11 +1319,6 @@ static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp) } if (qmp_is_oob(req)) { - if (!qmp_oob_enabled(mon)) { - error_setg(errp, "Please enable out-of-band first " - "for the session during capabilities negotiation"); - return false; - } if (!(cmd->options & QCO_ALLOW_OOB)) { error_setg(errp, "The command %s does not support OOB", command); @@ -4195,7 +4190,7 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) old_mon = cur_mon; cur_mon = mon; - rsp = qmp_dispatch(mon->qmp.commands, req); + rsp = qmp_dispatch(mon->qmp.commands, req, qmp_oob_enabled(mon)); cur_mon = old_mon; @@ -4286,7 +4281,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) } /* else will fail qmp_dispatch() */ /* Check against the request in general layout */ - qdict = qmp_dispatch_check_obj(req, &err); + qdict = qmp_dispatch_check_obj(req, qmp_oob_enabled(mon), &err); if (!qdict) { goto err; } -- cgit v1.1 From 00ecec151d2323e742af94cccf2de77025f3c0c1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:38 +0200 Subject: qmp: Redo how the client requests out-of-band execution Commit cf869d53172 "qmp: support out-of-band (oob) execution" added a general mechanism for command-independent arguments just for an out-of-band flag: The "control" key is introduced to store this extra flag. "control" field is used to store arguments that are shared by all the commands, rather than command specific arguments. Let "run-oob" be the first. However, it failed to reject unknown members of "control". For instance, in QMP command {"execute": "query-name", "id": 42, "control": {"crap": true}} "crap" gets silently ignored. Instead of fixing this, revert the general "control" mechanism (because YAGNI), and do it the way I initially proposed, with key "exec-oob". Simpler code, simpler interface. An out-of-band command {"execute": "migrate-pause", "id": 42, "control": {"run-oob": true}} becomes {"exec-oob": "migrate-pause", "id": 42} Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-13-armbru@redhat.com> [Commit message typo fixed] --- monitor.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 2e443bb..3bf3e68 100644 --- a/monitor.c +++ b/monitor.c @@ -1301,6 +1301,9 @@ static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp) command = qdict_get_try_str(req, "execute"); if (!command) { + command = qdict_get_try_str(req, "exec-oob"); + } + if (!command) { error_setg(errp, "Command field 'execute' missing"); return false; } -- cgit v1.1 From 45434ba47b3e3ccc2ac76674bb3988a152554bf2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:39 +0200 Subject: qmp: Revert change to handle_qmp_command tracepoint Commit 71da4667db6 "monitor: separate QMP parser and dispatcher" moved the handle_qmp_command tracepoint from handle_qmp_command() to monitor_qmp_dispatch_one(). This delays tracing from enqueue time to dequeue time. Revert that. Dequeue remains adequately visible via tracepoint monitor_qmp_cmd_in_band. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-14-armbru@redhat.com> --- monitor.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 3bf3e68..3cc4b07 100644 --- a/monitor.c +++ b/monitor.c @@ -4184,12 +4184,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) g_free(req_obj); - if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) { - QString *req_json = qobject_to_json(req); - trace_handle_qmp_command(mon, qstring_get_str(req_json)); - qobject_unref(req_json); - } - old_mon = cur_mon; cur_mon = mon; @@ -4283,6 +4277,12 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) qdict_del(qdict, "id"); } /* else will fail qmp_dispatch() */ + if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) { + QString *req_json = qobject_to_json(req); + trace_handle_qmp_command(mon, qstring_get_str(req_json)); + qobject_unref(req_json); + } + /* Check against the request in general layout */ qdict = qmp_dispatch_check_obj(req, qmp_oob_enabled(mon), &err); if (!qdict) { -- cgit v1.1 From 05f7274c8bd4a958f6673d28bf9bca0a9cf09c76 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:40 +0200 Subject: qmp: Always free QMPRequest with qmp_request_free() monitor_qmp_dispatch_one() frees a QMPRequest manually, because it needs to keep a reference to ->id. Premature optimization. Take an additional reference so we can use qmp_request_free(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-15-armbru@redhat.com> --- monitor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 3cc4b07..875647f 100644 --- a/monitor.c +++ b/monitor.c @@ -4182,8 +4182,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) id = req_obj->id; need_resume = req_obj->need_resume; - g_free(req_obj); - old_mon = cur_mon; cur_mon = mon; @@ -4192,14 +4190,14 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) cur_mon = old_mon; /* Respond if necessary */ - monitor_qmp_respond(mon, rsp, NULL, id); + monitor_qmp_respond(mon, rsp, NULL, qobject_ref(id)); /* This pairs with the monitor_suspend() in handle_qmp_command(). */ if (need_resume) { monitor_resume(mon); } - qobject_unref(req); + qmp_request_free(req_obj); } /* -- cgit v1.1 From b27314567d4cd8e204a18feba60d3341fb2d1aed Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:41 +0200 Subject: qmp: Simplify code around monitor_qmp_dispatch_one() Change monitor_qmp_dispatch_one() to take its parameters unwrapped, move monitor_resume() to the one caller that needs it, rename the function to monitor_qmp_dispatch(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-16-armbru@redhat.com> --- monitor.c | 58 ++++++++++++++++++++++++---------------------------------- 1 file changed, 24 insertions(+), 34 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 875647f..37ca4e7 100644 --- a/monitor.c +++ b/monitor.c @@ -4167,20 +4167,10 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp, qobject_unref(rsp); } -/* - * 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(Monitor *mon, QObject *req, QObject *id) { - Monitor *mon, *old_mon; - QObject *req, *rsp = NULL, *id; - bool need_resume; - - req = req_obj->req; - mon = req_obj->mon; - id = req_obj->id; - need_resume = req_obj->need_resume; + Monitor *old_mon; + QObject *rsp; old_mon = cur_mon; cur_mon = mon; @@ -4189,15 +4179,7 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) cur_mon = old_mon; - /* Respond if necessary */ monitor_qmp_respond(mon, rsp, NULL, qobject_ref(id)); - - /* This pairs with the monitor_suspend() in handle_qmp_command(). */ - if (need_resume) { - monitor_resume(mon); - } - - qmp_request_free(req_obj); } /* @@ -4241,12 +4223,20 @@ static void monitor_qmp_bh_dispatcher(void *data) { QMPRequest *req_obj = monitor_qmp_requests_pop_any(); - if (req_obj) { - trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: ""); - monitor_qmp_dispatch_one(req_obj); - /* Reschedule instead of looping so the main loop stays responsive */ - qemu_bh_schedule(mon_global.qmp_dispatcher_bh); + if (!req_obj) { + return; } + + trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: ""); + monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id); + if (req_obj->need_resume) { + /* Pairs with the monitor_suspend() in handle_qmp_command() */ + monitor_resume(req_obj->mon); + } + qmp_request_free(req_obj); + + /* Reschedule instead of looping so the main loop stays responsive */ + qemu_bh_schedule(mon_global.qmp_dispatcher_bh); } #define QMP_REQ_QUEUE_LEN_MAX (8) @@ -4292,20 +4282,20 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) goto err; } - req_obj = g_new0(QMPRequest, 1); - req_obj->mon = mon; - req_obj->id = id; - req_obj->req = req; - req_obj->need_resume = false; - if (qmp_is_oob(qdict)) { /* Out-of-band (OOB) requests are executed directly in parser. */ - trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id) + trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) ?: ""); - monitor_qmp_dispatch_one(req_obj); + monitor_qmp_dispatch(mon, req, id); return; } + req_obj = g_new0(QMPRequest, 1); + req_obj->mon = mon; + req_obj->id = id; + req_obj->req = req; + req_obj->need_resume = false; + /* Protect qmp_requests and fetching its length. */ qemu_mutex_lock(&mon->qmp.qmp_queue_lock); -- cgit v1.1 From e8f4a22168f573633f31fad3d6bfcbe5f0259b28 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:42 +0200 Subject: tests/qmp-test: Demonstrate QMP errors jumping the queue When OOB is enabled, out-of-band commands are executed right away, everything else is queued. This lets out-of-band commands "jump the queue". However, certain errors are always reported right away, and therefore can jump the queue even when the erroneous input does not request out-of-band execution. These errors are pretty unlikely to occur in production, but it's wrong all the same. Mark FIXME. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Peter Xu Message-Id: <20180703085358.13941-17-armbru@redhat.com> --- monitor.c | 1 + 1 file changed, 1 insertion(+) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 37ca4e7..c49214c 100644 --- a/monitor.c +++ b/monitor.c @@ -4339,6 +4339,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) return; err: + /* FIXME overtakes queued in-band commands, wrong when !qmp_is_oob() */ monitor_qmp_respond(mon, NULL, err, NULL); qobject_unref(req); } -- cgit v1.1 From 69240fe62d1ae02257bc0694a11c478b10378948 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:43 +0200 Subject: qmp: Don't let malformed in-band commands jump the queue handle_qmp_command() reports certain errors right away. This is wrong when OOB is enabled, because the errors can "jump the queue" then, as the previous commit demonstrates. To fix, we need to delay errors until dispatch. Do that for semantic errors, mostly by reverting ill-advised parts of commit cf869d53172 "qmp: support out-of-band (oob) execution". Bonus: doesn't run qmp_dispatch_check_obj() twice, once in handle_qmp_command(), and again in do_qmp_dispatch(). That's also due to commit cf869d53172. The next commit will fix queue jumping for syntax errors. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-18-armbru@redhat.com> --- monitor.c | 79 +++++++++++++++------------------------------------------------ 1 file changed, 18 insertions(+), 61 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index c49214c..be2a856 100644 --- a/monitor.c +++ b/monitor.c @@ -1290,48 +1290,6 @@ static void qmp_caps_apply(Monitor *mon, QMPCapabilityList *list) } } -/* - * Return true if check successful, or false otherwise. When false is - * returned, detailed error will be in errp if provided. - */ -static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp) -{ - const char *command; - QmpCommand *cmd; - - command = qdict_get_try_str(req, "execute"); - if (!command) { - command = qdict_get_try_str(req, "exec-oob"); - } - if (!command) { - error_setg(errp, "Command field 'execute' missing"); - return false; - } - - cmd = qmp_find_command(mon->qmp.commands, command); - if (!cmd) { - if (mon->qmp.commands == &qmp_cap_negotiation_commands) { - error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND, - "Expecting capabilities negotiation " - "with 'qmp_capabilities'"); - } else { - error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND, - "The command %s has not been found", command); - } - return false; - } - - if (qmp_is_oob(req)) { - if (!(cmd->options & QCO_ALLOW_OOB)) { - error_setg(errp, "The command %s does not support OOB", - command); - return false; - } - } - - return true; -} - void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable, Error **errp) { @@ -4171,6 +4129,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) { Monitor *old_mon; QObject *rsp; + QDict *error; old_mon = cur_mon; cur_mon = mon; @@ -4179,6 +4138,19 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) cur_mon = old_mon; + if (mon->qmp.commands == &qmp_cap_negotiation_commands) { + error = qdict_get_qdict(qobject_to(QDict, rsp), "error"); + if (error + && !g_strcmp0(qdict_get_try_str(error, "class"), + QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) { + /* Provide a more useful error message */ + qdict_del(error, "desc"); + qdict_put_str(error, "desc", "Expecting capabilities negotiation" + " with 'qmp_capabilities'"); + } + } + + /* Respond if necessary */ monitor_qmp_respond(mon, rsp, NULL, qobject_ref(id)); } @@ -4256,7 +4228,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) error_setg(&err, QERR_JSON_PARSING); } if (err) { - goto err; + assert(!req); + monitor_qmp_respond(mon, NULL, err, NULL); + return; } qdict = qobject_to(QDict, req); @@ -4271,18 +4245,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) qobject_unref(req_json); } - /* Check against the request in general layout */ - qdict = qmp_dispatch_check_obj(req, qmp_oob_enabled(mon), &err); - if (!qdict) { - goto err; - } - - /* Check against OOB specific */ - if (!qmp_cmd_oob_check(mon, qdict, &err)) { - goto err; - } - - if (qmp_is_oob(qdict)) { + if (qdict && qmp_is_oob(qdict)) { /* Out-of-band (OOB) requests are executed directly in parser. */ trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) ?: ""); @@ -4336,12 +4299,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) /* Kick the dispatcher routine */ qemu_bh_schedule(mon_global.qmp_dispatcher_bh); - return; - -err: - /* FIXME overtakes queued in-band commands, wrong when !qmp_is_oob() */ - monitor_qmp_respond(mon, NULL, err, NULL); - qobject_unref(req); } static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size) -- cgit v1.1 From 1cc37471525d03f963bc71d724f0dc9eab888fc1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:44 +0200 Subject: qmp: Don't let JSON errors jump the queue handle_qmp_command() reports JSON syntax errors right away. This is wrong when OOB is enabled, because the errors can "jump the queue" then. The previous commit fixed the same bug for semantic errors, by delaying the checking until dispatch. We can't delay the checking, so delay the reporting. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-19-armbru@redhat.com> --- monitor.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index be2a856..ea54213 100644 --- a/monitor.c +++ b/monitor.c @@ -254,8 +254,12 @@ struct QMPRequest { Monitor *mon; /* "id" field of the request */ QObject *id; - /* Request object to be handled */ + /* + * Request object to be handled or Error to be reported + * (exactly one of them is non-null) + */ QObject *req; + Error *err; /* * Whether we need to resume the monitor afterward. This flag is * used to emulate the old QMP server behavior that the current @@ -360,6 +364,7 @@ static void qmp_request_free(QMPRequest *req) { qobject_unref(req->id); qobject_unref(req->req); + error_free(req->err); g_free(req); } @@ -4199,8 +4204,14 @@ static void monitor_qmp_bh_dispatcher(void *data) return; } - trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: ""); - monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id); + if (req_obj->req) { + trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: ""); + monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id); + } else { + assert(req_obj->err); + monitor_qmp_respond(req_obj->mon, NULL, req_obj->err, NULL); + } + if (req_obj->need_resume) { /* Pairs with the monitor_suspend() in handle_qmp_command() */ monitor_resume(req_obj->mon); @@ -4227,11 +4238,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) /* json_parser_parse_err() sucks: can fail without setting @err */ error_setg(&err, QERR_JSON_PARSING); } - if (err) { - assert(!req); - monitor_qmp_respond(mon, NULL, err, NULL); - return; - } qdict = qobject_to(QDict, req); if (qdict) { @@ -4257,6 +4263,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) req_obj->mon = mon; req_obj->id = id; req_obj->req = req; + req_obj->err = err; req_obj->need_resume = false; /* Protect qmp_requests and fetching its length. */ -- cgit v1.1 From f91dc2a0d05eb211a18b8569ad42fb350dbf0b9f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:45 +0200 Subject: monitor: Rename use_io_thr to use_io_thread Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-20-armbru@redhat.com> --- monitor.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index ea54213..b3c5dcc 100644 --- a/monitor.c +++ b/monitor.c @@ -207,11 +207,11 @@ struct Monitor { int flags; int suspend_cnt; /* Needs to be accessed atomically */ bool skip_flush; - bool use_io_thr; + bool use_io_thread; /* * State used only in the thread "owning" the monitor. - * If @use_io_thr, this is mon_global.mon_iothread. + * If @use_io_thread, this is mon_global.mon_iothread. * Else, it's the main thread. * These members can be safely accessed without locks. */ @@ -521,7 +521,7 @@ static void monitor_json_emitter_raw(Monitor *mon, static void monitor_json_emitter(Monitor *mon, QObject *data) { - if (mon->use_io_thr) { + if (mon->use_io_thread) { /* * If using I/O thread, we need to queue the item so that I/O * thread will do the rest for us. Take refcount so that @@ -767,7 +767,7 @@ static void monitor_qapi_event_init(void) static void handle_hmp_command(Monitor *mon, const char *cmdline); static void monitor_data_init(Monitor *mon, bool skip_flush, - bool use_io_thr) + bool use_io_thread) { memset(mon, 0, sizeof(Monitor)); qemu_mutex_init(&mon->mon_lock); @@ -776,7 +776,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush, /* Use *mon_cmds by default. */ mon->cmd_table = mon_cmds; mon->skip_flush = skip_flush; - mon->use_io_thr = use_io_thr; + mon->use_io_thread = use_io_thread; mon->qmp.qmp_requests = g_queue_new(); mon->qmp.qmp_responses = g_queue_new(); } @@ -1271,7 +1271,7 @@ static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list, assert(list->value < QMP_CAPABILITY__MAX); switch (list->value) { case QMP_CAPABILITY_OOB: - if (!mon->use_io_thr) { + if (!mon->use_io_thread) { /* * Out-of-band only works with monitors that are * running on dedicated I/O thread. @@ -4377,7 +4377,7 @@ void monitor_resume(Monitor *mon) * For QMP monitors that are running in I/O thread, let's * kick the thread in case it's sleeping. */ - if (mon->use_io_thr) { + if (mon->use_io_thread) { aio_notify(iothread_get_aio_context(mon_global.mon_iothread)); } } else { @@ -4397,7 +4397,7 @@ static QObject *get_qmp_greeting(Monitor *mon) qmp_marshal_query_version(NULL, &ver, NULL); for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) { - if (!mon->use_io_thr && cap == QMP_CAPABILITY_OOB) { + if (!mon->use_io_thread && cap == QMP_CAPABILITY_OOB) { /* Monitors that are not using I/O thread won't support OOB */ continue; } @@ -4610,9 +4610,9 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) Monitor *mon = opaque; GMainContext *context; - if (mon->use_io_thr) { + if (mon->use_io_thread) { /* - * When use_io_thr is set, we use the global shared dedicated + * When @use_io_thread is set, we use the global shared dedicated * I/O thread for this monitor to handle input/output. */ context = monitor_get_io_context(); @@ -4661,7 +4661,7 @@ void monitor_init(Chardev *chr, int flags) if (monitor_is_qmp(mon)) { qemu_chr_fe_set_echo(&mon->chr, true); json_message_parser_init(&mon->qmp.parser, handle_qmp_command); - if (mon->use_io_thr) { + if (mon->use_io_thread) { /* * Make sure the old iowatch is gone. It's possible when * e.g. the chardev is in client mode, with wait=on. -- cgit v1.1 From cab5ad86b41af6deead6f9bd6edc8a2353eb3e90 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:46 +0200 Subject: monitor: Peel off @mon_global wrapper Wrapping global variables in a struct without a use for the wrapper struct buys us nothing but longer lines. Unwrap them. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-21-armbru@redhat.com> --- monitor.c | 60 +++++++++++++++++++++++++++++------------------------------- 1 file changed, 29 insertions(+), 31 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index b3c5dcc..26de90e 100644 --- a/monitor.c +++ b/monitor.c @@ -211,7 +211,7 @@ struct Monitor { /* * State used only in the thread "owning" the monitor. - * If @use_io_thread, this is mon_global.mon_iothread. + * If @use_io_thread, this is @mon_iothread. * Else, it's the main thread. * These members can be safely accessed without locks. */ @@ -240,14 +240,13 @@ struct Monitor { int mux_out; }; -/* Let's add monitor global variables to this struct. */ -static struct { - IOThread *mon_iothread; - /* Bottom half to dispatch the requests received from I/O thread */ - QEMUBH *qmp_dispatcher_bh; - /* Bottom half to deliver the responses back to clients */ - QEMUBH *qmp_respond_bh; -} mon_global; +IOThread *mon_iothread; + +/* Bottom half to dispatch the requests received from I/O thread */ +QEMUBH *qmp_dispatcher_bh; + +/* Bottom half to deliver the responses back to clients */ +QEMUBH *qmp_respond_bh; struct QMPRequest { /* Owner of the request */ @@ -531,7 +530,7 @@ static void monitor_json_emitter(Monitor *mon, QObject *data) qemu_mutex_lock(&mon->qmp.qmp_queue_lock); g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(data)); qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); - qemu_bh_schedule(mon_global.qmp_respond_bh); + qemu_bh_schedule(qmp_respond_bh); } else { /* * If not using monitor I/O thread, then we are in main thread. @@ -4219,7 +4218,7 @@ static void monitor_qmp_bh_dispatcher(void *data) qmp_request_free(req_obj); /* Reschedule instead of looping so the main loop stays responsive */ - qemu_bh_schedule(mon_global.qmp_dispatcher_bh); + qemu_bh_schedule(qmp_dispatcher_bh); } #define QMP_REQ_QUEUE_LEN_MAX (8) @@ -4305,7 +4304,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); /* Kick the dispatcher routine */ - qemu_bh_schedule(mon_global.qmp_dispatcher_bh); + qemu_bh_schedule(qmp_dispatcher_bh); } static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size) @@ -4358,7 +4357,7 @@ int monitor_suspend(Monitor *mon) * Kick I/O thread to make sure this takes effect. It'll be * evaluated again in prepare() of the watch object. */ - aio_notify(iothread_get_aio_context(mon_global.mon_iothread)); + aio_notify(iothread_get_aio_context(mon_iothread)); } trace_monitor_suspend(mon, 1); @@ -4378,7 +4377,7 @@ void monitor_resume(Monitor *mon) * kick the thread in case it's sleeping. */ if (mon->use_io_thread) { - aio_notify(iothread_get_aio_context(mon_global.mon_iothread)); + aio_notify(iothread_get_aio_context(mon_iothread)); } } else { assert(mon->rs); @@ -4516,36 +4515,35 @@ static void sortcmdlist(void) static GMainContext *monitor_get_io_context(void) { - return iothread_get_g_main_context(mon_global.mon_iothread); + return iothread_get_g_main_context(mon_iothread); } static AioContext *monitor_get_aio_context(void) { - return iothread_get_aio_context(mon_global.mon_iothread); + return iothread_get_aio_context(mon_iothread); } static void monitor_iothread_init(void) { - mon_global.mon_iothread = iothread_create("mon_iothread", - &error_abort); + mon_iothread = iothread_create("mon_iothread", &error_abort); /* * This MUST be on main loop thread since we have commands that * have assumption to be run on main loop thread. It would be * nice that one day we can remove this assumption in the future. */ - mon_global.qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(), - monitor_qmp_bh_dispatcher, - NULL); + qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(), + monitor_qmp_bh_dispatcher, + NULL); /* * Unlike the dispatcher BH, this must be run on the monitor I/O * thread, so that monitors that are using I/O thread will make * sure read/write operations are all done on the I/O thread. */ - mon_global.qmp_respond_bh = aio_bh_new(monitor_get_aio_context(), - monitor_qmp_bh_responder, - NULL); + qmp_respond_bh = aio_bh_new(monitor_get_aio_context(), + monitor_qmp_bh_responder, + NULL); } void monitor_init_globals(void) @@ -4702,7 +4700,7 @@ void monitor_cleanup(void) * we need to unregister from chardev below in * monitor_data_destroy(), and chardev is not thread-safe yet */ - iothread_stop(mon_global.mon_iothread); + iothread_stop(mon_iothread); /* * After we have I/O thread to send responses, it's possible that @@ -4723,13 +4721,13 @@ void monitor_cleanup(void) qemu_mutex_unlock(&monitor_lock); /* QEMUBHs needs to be deleted before destroying the I/O thread */ - qemu_bh_delete(mon_global.qmp_dispatcher_bh); - mon_global.qmp_dispatcher_bh = NULL; - qemu_bh_delete(mon_global.qmp_respond_bh); - mon_global.qmp_respond_bh = NULL; + qemu_bh_delete(qmp_dispatcher_bh); + qmp_dispatcher_bh = NULL; + qemu_bh_delete(qmp_respond_bh); + qmp_respond_bh = NULL; - iothread_destroy(mon_global.mon_iothread); - mon_global.mon_iothread = NULL; + iothread_destroy(mon_iothread); + mon_iothread = NULL; } QemuOptsList qemu_mon_opts = { -- cgit v1.1 From cee32796cadc9510ee00f029a933009df7a28ae2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:48 +0200 Subject: qmp: De-duplicate error response building All callers of qmp_build_error_object() duplicate the code to wrap it in a response object. Replace it by qmp_error_response() that captures the duplicated code, including error_free(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-23-armbru@redhat.com> --- monitor.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 26de90e..6284efe 100644 --- a/monitor.c +++ b/monitor.c @@ -4107,14 +4107,9 @@ static int monitor_can_read(void *opaque) static void monitor_qmp_respond(Monitor *mon, QObject *rsp, Error *err, QObject *id) { - QDict *qdict = NULL; - if (err) { assert(!rsp); - qdict = qdict_new(); - qdict_put_obj(qdict, "error", qmp_build_error_object(err)); - error_free(err); - rsp = QOBJECT(qdict); + rsp = QOBJECT(qmp_error_response(err)); } if (rsp) { -- cgit v1.1 From d43b16945afa8457b03aee543a110c4ff0b7f070 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:49 +0200 Subject: qmp: Use QDict * instead of QObject * for response objects By using the more specific type, we get fewer downcasts. The downcasts are safe, but not obviously so, at least not locally. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-24-armbru@redhat.com> --- monitor.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 6284efe..8237b1c 100644 --- a/monitor.c +++ b/monitor.c @@ -379,7 +379,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon) static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon) { while (!g_queue_is_empty(mon->qmp.qmp_responses)) { - qobject_unref((QObject *)g_queue_pop_head(mon->qmp.qmp_responses)); + qobject_unref((QDict *)g_queue_pop_head(mon->qmp.qmp_responses)); } } @@ -528,7 +528,8 @@ static void monitor_json_emitter(Monitor *mon, QObject *data) * responder thread). */ qemu_mutex_lock(&mon->qmp.qmp_queue_lock); - g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(data)); + g_queue_push_tail(mon->qmp.qmp_responses, + qobject_ref(qobject_to(QDict, data))); qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); qemu_bh_schedule(qmp_respond_bh); } else { @@ -542,13 +543,13 @@ static void monitor_json_emitter(Monitor *mon, QObject *data) struct QMPResponse { Monitor *mon; - QObject *data; + QDict *data; }; typedef struct QMPResponse QMPResponse; -static QObject *monitor_qmp_response_pop_one(Monitor *mon) +static QDict *monitor_qmp_response_pop_one(Monitor *mon) { - QObject *data; + QDict *data; qemu_mutex_lock(&mon->qmp.qmp_queue_lock); data = g_queue_pop_head(mon->qmp.qmp_responses); @@ -559,10 +560,10 @@ static QObject *monitor_qmp_response_pop_one(Monitor *mon) static void monitor_qmp_response_flush(Monitor *mon) { - QObject *data; + QDict *data; while ((data = monitor_qmp_response_pop_one(mon))) { - monitor_json_emitter_raw(mon, data); + monitor_json_emitter_raw(mon, QOBJECT(data)); qobject_unref(data); } } @@ -574,7 +575,7 @@ static void monitor_qmp_response_flush(Monitor *mon) static bool monitor_qmp_response_pop_any(QMPResponse *response) { Monitor *mon; - QObject *data = NULL; + QDict *data = NULL; qemu_mutex_lock(&monitor_lock); QTAILQ_FOREACH(mon, &mon_list, entry) { @@ -594,7 +595,7 @@ static void monitor_qmp_bh_responder(void *opaque) QMPResponse response; while (monitor_qmp_response_pop_any(&response)) { - monitor_json_emitter_raw(response.mon, response.data); + monitor_json_emitter_raw(response.mon, QOBJECT(response.data)); qobject_unref(response.data); } } @@ -4104,20 +4105,20 @@ static int monitor_can_read(void *opaque) * 2. rsp, err, and id may be NULL. * 3. If err != NULL then rsp must be NULL. */ -static void monitor_qmp_respond(Monitor *mon, QObject *rsp, +static void monitor_qmp_respond(Monitor *mon, QDict *rsp, Error *err, QObject *id) { if (err) { assert(!rsp); - rsp = QOBJECT(qmp_error_response(err)); + rsp = qmp_error_response(err); } if (rsp) { if (id) { - qdict_put_obj(qobject_to(QDict, rsp), "id", qobject_ref(id)); + qdict_put_obj(rsp, "id", qobject_ref(id)); } - monitor_json_emitter(mon, rsp); + monitor_json_emitter(mon, QOBJECT(rsp)); } qobject_unref(id); @@ -4127,7 +4128,7 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp, static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) { Monitor *old_mon; - QObject *rsp; + QDict *rsp; QDict *error; old_mon = cur_mon; @@ -4138,7 +4139,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) cur_mon = old_mon; if (mon->qmp.commands == &qmp_cap_negotiation_commands) { - error = qdict_get_qdict(qobject_to(QDict, rsp), "error"); + error = qdict_get_qdict(rsp, "error"); if (error && !g_strcmp0(qdict_get_try_str(error, "class"), QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) { -- cgit v1.1 From 65e3fe6743af08bd0bc79b3d6158e91d572afc57 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:50 +0200 Subject: qmp: Replace monitor_json_emitter{,raw}() by qmp_{queue,send}_response() monitor_json_emitter() and monitor_json_emitter_raw() are unnecessarily general: they can send arbitrary JSON values, even though we only ever use them for QMP, which may send only JSON objects. Specialize the argument from QObject * to QDict *, and rename to qmp_queue_response(), qmp_send_response(). All callers but one lose an upcast. The lone exception gains a downcast; the next commit will get rid of it. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-25-armbru@redhat.com> --- monitor.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 8237b1c..50dc57c 100644 --- a/monitor.c +++ b/monitor.c @@ -503,9 +503,9 @@ int monitor_fprintf(FILE *stream, const char *fmt, ...) return 0; } -static void monitor_json_emitter_raw(Monitor *mon, - QObject *data) +static void qmp_send_response(Monitor *mon, QDict *rsp) { + QObject *data = QOBJECT(rsp); QString *json; json = mon->flags & MONITOR_USE_PRETTY ? qobject_to_json_pretty(data) : @@ -518,7 +518,7 @@ static void monitor_json_emitter_raw(Monitor *mon, qobject_unref(json); } -static void monitor_json_emitter(Monitor *mon, QObject *data) +static void qmp_queue_response(Monitor *mon, QDict *rsp) { if (mon->use_io_thread) { /* @@ -528,8 +528,7 @@ static void monitor_json_emitter(Monitor *mon, QObject *data) * responder thread). */ qemu_mutex_lock(&mon->qmp.qmp_queue_lock); - g_queue_push_tail(mon->qmp.qmp_responses, - qobject_ref(qobject_to(QDict, data))); + g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp)); qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); qemu_bh_schedule(qmp_respond_bh); } else { @@ -537,7 +536,7 @@ static void monitor_json_emitter(Monitor *mon, QObject *data) * If not using monitor I/O thread, then we are in main thread. * Do the emission right away. */ - monitor_json_emitter_raw(mon, data); + qmp_send_response(mon, rsp); } } @@ -563,7 +562,7 @@ static void monitor_qmp_response_flush(Monitor *mon) QDict *data; while ((data = monitor_qmp_response_pop_one(mon))) { - monitor_json_emitter_raw(mon, QOBJECT(data)); + qmp_send_response(mon, data); qobject_unref(data); } } @@ -595,7 +594,7 @@ static void monitor_qmp_bh_responder(void *opaque) QMPResponse response; while (monitor_qmp_response_pop_any(&response)) { - monitor_json_emitter_raw(response.mon, QOBJECT(response.data)); + qmp_send_response(response.mon, response.data); qobject_unref(response.data); } } @@ -622,7 +621,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict) QTAILQ_FOREACH(mon, &mon_list, entry) { if (monitor_is_qmp(mon) && mon->qmp.commands != &qmp_cap_negotiation_commands) { - monitor_json_emitter(mon, QOBJECT(qdict)); + qmp_queue_response(mon, qdict); } } } @@ -4118,7 +4117,7 @@ static void monitor_qmp_respond(Monitor *mon, QDict *rsp, qdict_put_obj(rsp, "id", qobject_ref(id)); } - monitor_json_emitter(mon, QOBJECT(rsp)); + qmp_queue_response(mon, rsp); } qobject_unref(id); @@ -4418,7 +4417,7 @@ static void monitor_qmp_event(void *opaque, int event) mon->qmp.commands = &qmp_cap_negotiation_commands; monitor_qmp_caps_reset(mon); data = get_qmp_greeting(mon); - monitor_json_emitter(mon, data); + qmp_queue_response(mon, qobject_to(QDict, data)); qobject_unref(data); mon_refcount++; break; -- cgit v1.1 From 1816604b62f4c5ac1032374a941638f2fe199104 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:51 +0200 Subject: qmp: Replace get_qmp_greeting() by qmp_greeting() get_qmp_greeting() returns a QDict * as QObject *. It's caller converts it right back. Return QDict * instead. While there, rename to qmp_greeting(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-26-armbru@redhat.com> --- monitor.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 50dc57c..e5e9f49 100644 --- a/monitor.c +++ b/monitor.c @@ -4382,7 +4382,7 @@ void monitor_resume(Monitor *mon) trace_monitor_suspend(mon, -1); } -static QObject *get_qmp_greeting(Monitor *mon) +static QDict *qmp_greeting(Monitor *mon) { QList *cap_list = qlist_new(); QObject *ver = NULL; @@ -4398,8 +4398,9 @@ static QObject *get_qmp_greeting(Monitor *mon) qlist_append_str(cap_list, QMPCapability_str(cap)); } - return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': %p}}", - ver, cap_list); + return qdict_from_jsonf_nofail( + "{'QMP': {'version': %p, 'capabilities': %p}}", + ver, cap_list); } static void monitor_qmp_caps_reset(Monitor *mon) @@ -4409,15 +4410,15 @@ static void monitor_qmp_caps_reset(Monitor *mon) static void monitor_qmp_event(void *opaque, int event) { - QObject *data; + QDict *data; Monitor *mon = opaque; switch (event) { case CHR_EVENT_OPENED: mon->qmp.commands = &qmp_cap_negotiation_commands; monitor_qmp_caps_reset(mon); - data = get_qmp_greeting(mon); - qmp_queue_response(mon, qobject_to(QDict, data)); + data = qmp_greeting(mon); + qmp_queue_response(mon, data); qobject_unref(data); mon_refcount++; break; -- cgit v1.1 From 7cb2123f226a88fddcb9a3673c658c818ee6daed Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:52 +0200 Subject: qmp: Simplify monitor_qmp_respond() monitor_qmp_respond() takes both a response object and an error object. If an error object is non-null, the response object must be null, and the response is built from the error object. Of the two callers, one always passes a null response object, and one a null error object. Move building the response object from the error object to the latter, and drop the error object parameter. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-27-armbru@redhat.com> --- monitor.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index e5e9f49..1da7b8b 100644 --- a/monitor.c +++ b/monitor.c @@ -4100,18 +4100,12 @@ static int monitor_can_read(void *opaque) } /* - * 1. This function takes ownership of rsp, err, and id. - * 2. rsp, err, and id may be NULL. - * 3. If err != NULL then rsp must be NULL. + * Emit QMP response @rsp with ID @id to @mon. + * Null @rsp can only happen for commands with QCO_NO_SUCCESS_RESP. + * Nothing is emitted then. */ -static void monitor_qmp_respond(Monitor *mon, QDict *rsp, - Error *err, QObject *id) +static void monitor_qmp_respond(Monitor *mon, QDict *rsp, QObject *id) { - if (err) { - assert(!rsp); - rsp = qmp_error_response(err); - } - if (rsp) { if (id) { qdict_put_obj(rsp, "id", qobject_ref(id)); @@ -4119,9 +4113,6 @@ static void monitor_qmp_respond(Monitor *mon, QDict *rsp, qmp_queue_response(mon, rsp); } - - qobject_unref(id); - qobject_unref(rsp); } static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) @@ -4149,8 +4140,8 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) } } - /* Respond if necessary */ - monitor_qmp_respond(mon, rsp, NULL, qobject_ref(id)); + monitor_qmp_respond(mon, rsp, id); + qobject_unref(rsp); } /* @@ -4193,6 +4184,7 @@ static QMPRequest *monitor_qmp_requests_pop_any(void) static void monitor_qmp_bh_dispatcher(void *data) { QMPRequest *req_obj = monitor_qmp_requests_pop_any(); + QDict *rsp; if (!req_obj) { return; @@ -4203,7 +4195,9 @@ static void monitor_qmp_bh_dispatcher(void *data) monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id); } else { assert(req_obj->err); - monitor_qmp_respond(req_obj->mon, NULL, req_obj->err, NULL); + rsp = qmp_error_response(req_obj->err); + monitor_qmp_respond(req_obj->mon, rsp, NULL); + qobject_unref(rsp); } if (req_obj->need_resume) { -- cgit v1.1 From 279f9e0840d08db8e0bb3806b6eb4f2f60cb104f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:56 +0200 Subject: qmp: Clean up capability negotiation after commit 02130314d8c qmp_greeting() offers capabilities to the client, and qmp_qmp_capabilities() accepts or denies capabilities requested by the client. The two compute the set of available capabilities independently. Not nice. Clean this up as follows. Compute available capabilities just once in monitor_qmp_caps_reset(), and store them in Monitor member qmp.capab_offered[]. Have qmp_greeting() and qmp_qmp_capabilities() use that. Both are now oblivious of capability details. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-31-armbru@redhat.com> --- monitor.c | 89 +++++++++++++++++++++++++++------------------------------------ 1 file changed, 38 insertions(+), 51 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 1da7b8b..f8b88ba 100644 --- a/monitor.c +++ b/monitor.c @@ -173,7 +173,8 @@ typedef struct { * mode. */ QmpCommandList *commands; - bool qmp_caps[QMP_CAPABILITY__MAX]; + bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */ + bool capab[QMP_CAPABILITY__MAX]; /* offered and accepted */ /* * Protects qmp request/response queue. Please take monitor_lock * first when used together. @@ -1253,52 +1254,56 @@ static void monitor_init_qmp_commands(void) qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG); } -static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap) +static bool qmp_oob_enabled(Monitor *mon) { - return mon->qmp.qmp_caps[cap]; + return mon->qmp.capab[QMP_CAPABILITY_OOB]; } -static bool qmp_oob_enabled(Monitor *mon) +static void monitor_qmp_caps_reset(Monitor *mon) { - return qmp_cap_enabled(mon, QMP_CAPABILITY_OOB); + memset(mon->qmp.capab_offered, 0, sizeof(mon->qmp.capab_offered)); + memset(mon->qmp.capab, 0, sizeof(mon->qmp.capab)); + mon->qmp.capab_offered[QMP_CAPABILITY_OOB] = mon->use_io_thread; } -static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list, - Error **errp) +/* + * Accept QMP capabilities in @list for @mon. + * On success, set mon->qmp.capab[], and return true. + * On error, set @errp, and return false. + */ +static bool qmp_caps_accept(Monitor *mon, QMPCapabilityList *list, + Error **errp) { + GString *unavailable = NULL; + bool capab[QMP_CAPABILITY__MAX]; + + memset(capab, 0, sizeof(capab)); + for (; list; list = list->next) { - assert(list->value < QMP_CAPABILITY__MAX); - switch (list->value) { - case QMP_CAPABILITY_OOB: - if (!mon->use_io_thread) { - /* - * Out-of-band only works with monitors that are - * running on dedicated I/O thread. - */ - error_setg(errp, "This monitor does not support " - "out-of-band (OOB)"); - return; + if (!mon->qmp.capab_offered[list->value]) { + if (!unavailable) { + unavailable = g_string_new(QMPCapability_str(list->value)); + } else { + g_string_append_printf(unavailable, ", %s", + QMPCapability_str(list->value)); } - break; - default: - break; } + capab[list->value] = true; } -} -/* This function should only be called after capabilities are checked. */ -static void qmp_caps_apply(Monitor *mon, QMPCapabilityList *list) -{ - for (; list; list = list->next) { - mon->qmp.qmp_caps[list->value] = true; + if (unavailable) { + error_setg(errp, "Capability %s not available", unavailable->str); + g_string_free(unavailable, true); + return false; } + + memcpy(mon->qmp.capab, capab, sizeof(capab)); + return true; } void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable, Error **errp) { - Error *local_err = NULL; - if (cur_mon->qmp.commands == &qmp_commands) { error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND, "Capabilities negotiation is already complete, command " @@ -1306,19 +1311,8 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable, return; } - /* Enable QMP capabilities provided by the client if applicable. */ - if (has_enable) { - qmp_caps_check(cur_mon, enable, &local_err); - if (local_err) { - /* - * Failed check on any of the capabilities will fail the - * entire command (and thus not apply any of the other - * capabilities that were also requested). - */ - error_propagate(errp, local_err); - return; - } - qmp_caps_apply(cur_mon, enable); + if (!qmp_caps_accept(cur_mon, enable, errp)) { + return; } cur_mon->qmp.commands = &qmp_commands; @@ -4385,11 +4379,9 @@ static QDict *qmp_greeting(Monitor *mon) qmp_marshal_query_version(NULL, &ver, NULL); for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) { - if (!mon->use_io_thread && cap == QMP_CAPABILITY_OOB) { - /* Monitors that are not using I/O thread won't support OOB */ - continue; + if (mon->qmp.capab_offered[cap]) { + qlist_append_str(cap_list, QMPCapability_str(cap)); } - qlist_append_str(cap_list, QMPCapability_str(cap)); } return qdict_from_jsonf_nofail( @@ -4397,11 +4389,6 @@ static QDict *qmp_greeting(Monitor *mon) ver, cap_list); } -static void monitor_qmp_caps_reset(Monitor *mon) -{ - memset(mon->qmp.qmp_caps, 0, sizeof(mon->qmp.qmp_caps)); -} - static void monitor_qmp_event(void *opaque, int event) { QDict *data; -- cgit v1.1 From 774a6b67a409edf23a9b4b02da9ccbfc62e27cae Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:57 +0200 Subject: monitor: Improve some comments Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-32-armbru@redhat.com> --- monitor.c | 100 ++++++++++++++++++++++++++++---------------------------------- 1 file changed, 45 insertions(+), 55 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index f8b88ba..5117953 100644 --- a/monitor.c +++ b/monitor.c @@ -169,15 +169,16 @@ typedef struct { JSONMessageParser parser; /* * When a client connects, we're in capabilities negotiation mode. - * When command qmp_capabilities succeeds, we go into command - * mode. + * @commands is &qmp_cap_negotiation_commands then. When command + * qmp_capabilities succeeds, we go into command mode, and + * @command becomes &qmp_commands. */ QmpCommandList *commands; bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */ bool capab[QMP_CAPABILITY__MAX]; /* offered and accepted */ /* - * Protects qmp request/response queue. Please take monitor_lock - * first when used together. + * Protects qmp request/response queue. + * Take monitor_lock first when you need both. */ QemuMutex qmp_queue_lock; /* Input queue that holds all the parsed QMP requests */ @@ -232,7 +233,7 @@ struct Monitor { QemuMutex mon_lock; /* - * Fields that are protected by the per-monitor lock. + * Members that are protected by the per-monitor lock */ QLIST_HEAD(, mon_fd_t) fds; QString *outbuf; @@ -241,6 +242,7 @@ struct Monitor { int mux_out; }; +/* Shared monitor I/O thread */ IOThread *mon_iothread; /* Bottom half to dispatch the requests received from I/O thread */ @@ -302,9 +304,9 @@ static inline bool monitor_is_qmp(const Monitor *mon) } /** - * Whether @mon is using readline? Note: not all HMP monitors use - * readline, e.g., gdbserver has a non-interactive HMP monitor, so - * readline is not used there. + * Is @mon is using readline? + * Note: not all HMP monitors use readline, e.g., gdbserver has a + * non-interactive HMP monitor, so readline is not used there. */ static inline bool monitor_uses_readline(const Monitor *mon) { @@ -318,14 +320,12 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon) /* * Return the clock to use for recording an event's time. + * It's QEMU_CLOCK_REALTIME, except for qtests it's + * QEMU_CLOCK_VIRTUAL, to support testing rate limits. * Beware: result is invalid before configure_accelerator(). */ static inline QEMUClockType monitor_get_event_clock(void) { - /* - * This allows us to perform tests on the monitor queues to verify - * that the rate limits are enforced. - */ return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME; } @@ -368,7 +368,7 @@ static void qmp_request_free(QMPRequest *req) g_free(req); } -/* Must with the mon->qmp.qmp_queue_lock held */ +/* Caller must hold mon->qmp.qmp_queue_lock */ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon) { while (!g_queue_is_empty(mon->qmp.qmp_requests)) { @@ -376,7 +376,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon) } } -/* Must with the mon->qmp.qmp_queue_lock held */ +/* Caller must hold the mon->qmp.qmp_queue_lock */ static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon) { while (!g_queue_is_empty(mon->qmp.qmp_responses)) { @@ -407,7 +407,7 @@ static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, return FALSE; } -/* Called with mon->mon_lock held. */ +/* Caller must hold mon->mon_lock */ static void monitor_flush_locked(Monitor *mon) { int rc; @@ -523,10 +523,8 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp) { if (mon->use_io_thread) { /* - * If using I/O thread, we need to queue the item so that I/O - * thread will do the rest for us. Take refcount so that - * caller won't free the data (which will be finally freed in - * responder thread). + * Push a reference to the response queue. The I/O thread + * drains that queue and emits. */ qemu_mutex_lock(&mon->qmp.qmp_queue_lock); g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp)); @@ -534,8 +532,8 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp) qemu_bh_schedule(qmp_respond_bh); } else { /* - * If not using monitor I/O thread, then we are in main thread. - * Do the emission right away. + * Not using monitor I/O thread, i.e. we are in the main thread. + * Emit right away. */ qmp_send_response(mon, rsp); } @@ -611,8 +609,9 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = { }; /* - * Emits the event to every monitor instance, @event is only used for trace - * Called with monitor_lock held. + * Broadcast an event to all monitors. + * @qdict is the event object. Its member "event" must match @event. + * Caller must hold monitor_lock. */ static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict) { @@ -981,8 +980,7 @@ static int parse_cmdline(const char *cmdline, } /* - * Returns true if the command can be executed in preconfig mode - * i.e. it has the 'p' flag. + * Can command @cmd be executed in preconfig state? */ static bool cmd_can_preconfig(const mon_cmd_t *cmd) { @@ -2221,7 +2219,7 @@ void qmp_getfd(const char *fdname, Error **errp) tmp_fd = monfd->fd; monfd->fd = fd; qemu_mutex_unlock(&cur_mon->mon_lock); - /* Make sure close() is out of critical section */ + /* Make sure close() is outside critical section */ close(tmp_fd); return; } @@ -2250,7 +2248,7 @@ void qmp_closefd(const char *fdname, Error **errp) g_free(monfd->name); g_free(monfd); qemu_mutex_unlock(&cur_mon->mon_lock); - /* Make sure close() is out of critical section */ + /* Make sure close() is outside critical section */ close(tmp_fd); return; } @@ -4139,7 +4137,8 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) } /* - * Pop one QMP request from monitor queues, return NULL if not found. + * Pop a QMP request from a monitor request queue. + * Return the request, or NULL all request queues are empty. * We are using round-robin fashion to pop the request, to avoid * processing commands only on a very busy monitor. To achieve that, * when we process one request on a specific monitor, we put that @@ -4234,7 +4233,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) } if (qdict && qmp_is_oob(qdict)) { - /* Out-of-band (OOB) requests are executed directly in parser. */ + /* OOB commands are executed immediately */ trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) ?: ""); monitor_qmp_dispatch(mon, req, id); @@ -4356,8 +4355,8 @@ void monitor_resume(Monitor *mon) if (atomic_dec_fetch(&mon->suspend_cnt) == 0) { if (monitor_is_qmp(mon)) { /* - * For QMP monitors that are running in I/O thread, let's - * kick the thread in case it's sleeping. + * For QMP monitors that are running in the I/O thread, + * let's kick the thread in case it's sleeping. */ if (mon->use_io_thread) { aio_notify(iothread_get_aio_context(mon_iothread)); @@ -4505,18 +4504,18 @@ static void monitor_iothread_init(void) mon_iothread = iothread_create("mon_iothread", &error_abort); /* - * This MUST be on main loop thread since we have commands that - * have assumption to be run on main loop thread. It would be - * nice that one day we can remove this assumption in the future. + * The dispatcher BH must run in the main loop thread, since we + * have commands assuming that context. It would be nice to get + * rid of those assumptions. */ qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(), monitor_qmp_bh_dispatcher, NULL); /* - * Unlike the dispatcher BH, this must be run on the monitor I/O - * thread, so that monitors that are using I/O thread will make - * sure read/write operations are all done on the I/O thread. + * The responder BH must be run in the monitor I/O thread, so that + * monitors that are using the I/O thread have their output + * written by the I/O thread. */ qmp_respond_bh = aio_bh_new(monitor_get_aio_context(), monitor_qmp_bh_responder, @@ -4586,15 +4585,11 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) GMainContext *context; if (mon->use_io_thread) { - /* - * When @use_io_thread is set, we use the global shared dedicated - * I/O thread for this monitor to handle input/output. - */ + /* Use @mon_iothread context */ context = monitor_get_io_context(); - /* We should have inited globals before reaching here. */ assert(context); } else { - /* The default main loop, which is the main thread */ + /* Use default main loop context */ context = NULL; } @@ -4644,15 +4639,12 @@ void monitor_init(Chardev *chr, int flags) remove_fd_in_watch(chr); /* * We can't call qemu_chr_fe_set_handlers() directly here - * since during the procedure the chardev will be active - * and running in monitor I/O thread, while we'll still do - * something before returning from it, which is a possible - * race too. To avoid that, we just create a BH to setup - * the handlers. + * since chardev might be running in the monitor I/O + * thread. Schedule a bottom half. */ aio_bh_schedule_oneshot(monitor_get_aio_context(), monitor_qmp_setup_handlers_bh, mon); - /* We'll add this to mon_list in the BH when setup done */ + /* The bottom half will add @mon to @mon_list */ return; } else { qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, @@ -4673,21 +4665,19 @@ void monitor_cleanup(void) /* * We need to explicitly stop the I/O thread (but not destroy it), - * cleanup the monitor resources, then destroy the I/O thread since + * clean up the monitor resources, then destroy the I/O thread since * we need to unregister from chardev below in * monitor_data_destroy(), and chardev is not thread-safe yet */ iothread_stop(mon_iothread); /* - * After we have I/O thread to send responses, it's possible that - * when we stop the I/O thread there are still replies queued in the - * responder queue. Flush all of them. Note that even after this - * flush it's still possible that out buffer is not flushed. - * It'll be done in below monitor_flush() as the last resort. + * Flush all response queues. Note that even after this flush, + * data may remain in output buffers. */ monitor_qmp_bh_responder(NULL); + /* Flush output buffers and destroy monitors */ qemu_mutex_lock(&monitor_lock); QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) { QTAILQ_REMOVE(&mon_list, mon, entry); -- cgit v1.1