From 88e40e43476ca483367576cd8dbffbfa71a0bfc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 6 Dec 2018 00:37:31 +0400 Subject: monitor: inline ambiguous helper functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function were not named with "mon_iothread", or following the AIO vs GMainContext distinction. Inline them instead. Signed-off-by: Marc-André Lureau Reviewed-by: Peter Xu Reviewed-by: Markus Armbruster Message-Id: <20181205203737.9011-2-marcandre.lureau@redhat.com> Signed-off-by: Markus Armbruster --- monitor.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/monitor.c b/monitor.c index d39390c2f2..d531e8ccc9 100644 --- a/monitor.c +++ b/monitor.c @@ -4453,16 +4453,6 @@ static void sortcmdlist(void) qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd); } -static GMainContext *monitor_get_io_context(void) -{ - return iothread_get_g_main_context(mon_iothread); -} - -static AioContext *monitor_get_aio_context(void) -{ - return iothread_get_aio_context(mon_iothread); -} - static void monitor_iothread_init(void) { mon_iothread = iothread_create("mon_iothread", &error_abort); @@ -4549,7 +4539,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) GMainContext *context; assert(mon->use_io_thread); - context = monitor_get_io_context(); + context = iothread_get_g_main_context(mon_iothread); assert(context); qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read, monitor_qmp_event, NULL, mon, context, true); @@ -4601,7 +4591,7 @@ void monitor_init(Chardev *chr, int flags) * since chardev might be running in the monitor I/O * thread. Schedule a bottom half. */ - aio_bh_schedule_oneshot(monitor_get_aio_context(), + aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread), monitor_qmp_setup_handlers_bh, mon); /* The bottom half will add @mon to @mon_list */ return; -- cgit v1.2.3 From ef12a703b7d4894a851cbf3e94cca8fa25fc43f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 6 Dec 2018 00:37:32 +0400 Subject: monitor: accept chardev input from iothread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chardev backends may not handle safely IO events from concurrent threads (may not handle I/O events from concurrent threads safely, only the write path is since commit > 9005b2a7589540a3733b3abdcfbccfe7746cd1a1). Better to wake up the chardev from the monitor IO thread if it's being used as the chardev context. Unify code paths by using a BH in all cases. Drop the now redundant aio_notify() call. Clean up control flow not to rely on mon->use_io_thread implying monitor_is_qmp(mon). Signed-off-by: Marc-André Lureau Reviewed-by: Peter Xu Reviewed-by: Markus Armbruster Message-Id: <20181205203737.9011-3-marcandre.lureau@redhat.com> Signed-off-by: Markus Armbruster --- monitor.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/monitor.c b/monitor.c index d531e8ccc9..79afe99079 100644 --- a/monitor.c +++ b/monitor.c @@ -4297,7 +4297,7 @@ int monitor_suspend(Monitor *mon) atomic_inc(&mon->suspend_cnt); - if (monitor_is_qmp(mon) && mon->use_io_thread) { + if (mon->use_io_thread) { /* * Kick I/O thread to make sure this takes effect. It'll be * evaluated again in prepare() of the watch object. @@ -4309,6 +4309,13 @@ int monitor_suspend(Monitor *mon) return 0; } +static void monitor_accept_input(void *opaque) +{ + Monitor *mon = opaque; + + qemu_chr_fe_accept_input(&mon->chr); +} + void monitor_resume(Monitor *mon) { if (monitor_is_hmp_non_interactive(mon)) { @@ -4316,20 +4323,22 @@ 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 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)); - } + AioContext *ctx; + + if (mon->use_io_thread) { + ctx = iothread_get_aio_context(mon_iothread); } else { + ctx = qemu_get_aio_context(); + } + + if (!monitor_is_qmp(mon)) { assert(mon->rs); readline_show_prompt(mon->rs); } - qemu_chr_fe_accept_input(&mon->chr); + + aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon); } + trace_monitor_suspend(mon, -1); } -- cgit v1.2.3 From 9ac3788b0be368e2826a8e9adadc1ada87d89273 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 6 Dec 2018 00:37:33 +0400 Subject: char: add a QEMU_CHAR_FEATURE_GCONTEXT flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QEMU_CHAR_FEATURE_GCONTEXT declares the character device can switch GMainContext. Assert we don't switch context when the character device doesn't provide this feature. Character device users must not violate this restriction. In particular, user configurations that violate them must be rejected. Existing frontend that rely on context switching would now assert() if the backend doesn't allow it (instead of silently producing undesired events in the default context). Following patches improve the situation by reporting an error earlier instead, on the frontend side. Signed-off-by: Marc-André Lureau Message-Id: <20181205203737.9011-4-marcandre.lureau@redhat.com> Signed-off-by: Markus Armbruster --- chardev/char.c | 11 +++++++++++ include/chardev/char.h | 3 +++ 2 files changed, 14 insertions(+) diff --git a/chardev/char.c b/chardev/char.c index 152dde5327..ccba36bafb 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -193,6 +193,8 @@ void qemu_chr_be_update_read_handlers(Chardev *s, { ChardevClass *cc = CHARDEV_GET_CLASS(s); + assert(qemu_chr_has_feature(s, QEMU_CHAR_FEATURE_GCONTEXT) + || !context); s->gcontext = context; if (cc->chr_update_read_handler) { cc->chr_update_read_handler(s); @@ -240,6 +242,15 @@ static void char_init(Object *obj) chr->logfd = -1; qemu_mutex_init(&chr->chr_write_lock); + + /* + * Assume if chr_update_read_handler is implemented it will + * take the updated gcontext into account. + */ + if (CHARDEV_GET_CLASS(chr)->chr_update_read_handler) { + qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT); + } + } static int null_chr_write(Chardev *chr, const uint8_t *buf, int len) diff --git a/include/chardev/char.h b/include/chardev/char.h index 7becd8c80c..014566c3de 100644 --- a/include/chardev/char.h +++ b/include/chardev/char.h @@ -47,6 +47,9 @@ typedef enum { QEMU_CHAR_FEATURE_FD_PASS, /* Whether replay or record mode is enabled */ QEMU_CHAR_FEATURE_REPLAY, + /* Whether the gcontext can be changed after calling + * qemu_chr_be_update_read_handlers() */ + QEMU_CHAR_FEATURE_GCONTEXT, QEMU_CHAR_FEATURE_LAST, } ChardevFeature; -- cgit v1.2.3 From a9a0d9b99a347b1413728992726f2bacf76d48ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 6 Dec 2018 00:37:34 +0400 Subject: monitor: check if chardev can switch gcontext for OOB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not all backends are able to switch gcontext. Those backends cannot drive a OOB monitor (the monitor would then be blocking on main thread). For example, ringbuf, spice, or more esoteric input chardevs like braille or MUX. We already forbid MUX because not all frontends are ready to run outside main loop. Replace that by a context-switching feature check. Signed-off-by: Marc-André Lureau Message-Id: <20181205203737.9011-5-marcandre.lureau@redhat.com> [Error condition simplified, commit message adjusted accordingly] Signed-off-by: Markus Armbruster --- monitor.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/monitor.c b/monitor.c index 79afe99079..a1329d8a86 100644 --- a/monitor.c +++ b/monitor.c @@ -4562,9 +4562,10 @@ void monitor_init(Chardev *chr, int flags) bool use_oob = flags & MONITOR_USE_OOB; if (use_oob) { - if (CHARDEV_IS_MUX(chr)) { + if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) { error_report("Monitor out-of-band is not supported with " - "MUX typed chardev backend"); + "%s typed chardev backend", + object_get_typename(OBJECT(chr))); exit(1); } if (use_readline) { -- cgit v1.2.3 From 269d25cdeb3acecf0033d6809dd7e23ebbc78ce9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 6 Dec 2018 00:37:35 +0400 Subject: colo: check chardev can switch context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit COLO uses a worker context (iothread) to drive the chardev. All backends are not able to switch the context, let's report an error in this case. Signed-off-by: Marc-André Lureau Message-Id: <20181205203737.9011-6-marcandre.lureau@redhat.com> Reviewed-by: Li Zhijian Reviewed-by: Zhang Chen Signed-off-by: Markus Armbruster --- net/colo-compare.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/net/colo-compare.c b/net/colo-compare.c index a39191d522..9156ab3349 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -957,6 +957,12 @@ static int find_and_check_chardev(Chardev **chr, return 1; } + if (!qemu_chr_has_feature(*chr, QEMU_CHAR_FEATURE_GCONTEXT)) { + error_setg(errp, "chardev \"%s\" cannot switch context", + chr_name); + return 1; + } + return 0; } -- cgit v1.2.3 From 8dac00bbfbca1fa793b8ad410a4373e97599102d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 6 Dec 2018 00:37:36 +0400 Subject: monitor: prevent inserting new monitors after cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit monitor_cleanup() is one of the last things main() calls before it returns. In the following patch, monitor_cleanup() will release the monitor_lock during flushing. There may be pending commands to insert new monitors, which would modify the mon_list during iteration, and the clean-up could thus miss those new insertions. Add a monitor_destroyed global to check if monitor_cleanup() has been already called. In this case, don't insert the new monitor in the list, but free it instead. A cleaner solution would involve the main thread telling other threads to terminate, waiting for their termination. Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster Message-Id: <20181205203737.9011-7-marcandre.lureau@redhat.com> Signed-off-by: Markus Armbruster --- monitor.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/monitor.c b/monitor.c index a1329d8a86..d353c0e645 100644 --- a/monitor.c +++ b/monitor.c @@ -263,10 +263,11 @@ typedef struct QMPRequest QMPRequest; /* QMP checker flags */ #define QMP_ACCEPT_UNKNOWNS 1 -/* Protects mon_list, monitor_qapi_event_state. */ +/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */ static QemuMutex monitor_lock; static GHashTable *monitor_qapi_event_state; static QTAILQ_HEAD(mon_list, Monitor) mon_list; +static bool monitor_destroyed; /* Protects mon_fdsets */ static QemuMutex mon_fdsets_lock; @@ -4538,8 +4539,21 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) static void monitor_list_append(Monitor *mon) { qemu_mutex_lock(&monitor_lock); - QTAILQ_INSERT_HEAD(&mon_list, mon, entry); + /* + * This prevents inserting new monitors during monitor_cleanup(). + * A cleaner solution would involve the main thread telling other + * threads to terminate, waiting for their termination. + */ + if (!monitor_destroyed) { + QTAILQ_INSERT_HEAD(&mon_list, mon, entry); + mon = NULL; + } qemu_mutex_unlock(&monitor_lock); + + if (mon) { + monitor_data_destroy(mon); + g_free(mon); + } } static void monitor_qmp_setup_handlers_bh(void *opaque) @@ -4634,6 +4648,7 @@ void monitor_cleanup(void) /* Flush output buffers and destroy monitors */ qemu_mutex_lock(&monitor_lock); + monitor_destroyed = true; QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) { QTAILQ_REMOVE(&mon_list, mon, entry); monitor_flush(mon); -- cgit v1.2.3 From 34f1f3e06d8824740d3bc41556f8300f0fb463cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 6 Dec 2018 00:37:37 +0400 Subject: monitor: avoid potential dead-lock when cleaning up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a monitor is connected to a Spice chardev, the monitor cleanup can dead-lock: #0 0x00007f43446637fd in __lll_lock_wait () at /lib64/libpthread.so.0 #1 0x00007f434465ccf4 in pthread_mutex_lock () at /lib64/libpthread.so.0 #2 0x0000556dd79f22ba in qemu_mutex_lock_impl (mutex=0x556dd81c9220 , file=0x556dd7ae3648 "/home/elmarco/src/qq/monitor.c", line=645) at /home/elmarco/src/qq/util/qemu-thread-posix.c:66 #3 0x0000556dd7431bd5 in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x556dd9abc850, errp=0x7fffb7bbddd8) at /home/elmarco/src/qq/monitor.c:645 #4 0x0000556dd79d476b in qapi_event_send_spice_disconnected (server=0x556dd98ee760, client=0x556ddaaa8560, errp=0x556dd82180d0 ) at qapi/qapi-events-ui.c:149 #5 0x0000556dd7870fc1 in channel_event (event=3, info=0x556ddad1b590) at /home/elmarco/src/qq/ui/spice-core.c:235 #6 0x00007f434560a6bb in reds_handle_channel_event (reds=, event=3, info=0x556ddad1b590) at reds.c:316 #7 0x00007f43455f393b in main_dispatcher_self_handle_channel_event (info=0x556ddad1b590, event=3, self=0x556dd9a7d8c0) at main-dispatcher.c:197 #8 0x00007f43455f393b in main_dispatcher_channel_event (self=0x556dd9a7d8c0, event=event@entry=3, info=0x556ddad1b590) at main-dispatcher.c:197 #9 0x00007f4345612833 in red_stream_push_channel_event (s=s@entry=0x556ddae2ef40, event=event@entry=3) at red-stream.c:414 #10 0x00007f434561286b in red_stream_free (s=0x556ddae2ef40) at red-stream.c:388 #11 0x00007f43455f9ddc in red_channel_client_finalize (object=0x556dd9bb21a0) at red-channel-client.c:347 #12 0x00007f434b5f9fb9 in g_object_unref () at /lib64/libgobject-2.0.so.0 #13 0x00007f43455fc212 in red_channel_client_push (rcc=0x556dd9bb21a0) at red-channel-client.c:1341 #14 0x0000556dd76081ba in spice_port_set_fe_open (chr=0x556dd9925e20, fe_open=0) at /home/elmarco/src/qq/chardev/spice.c:241 #15 0x0000556dd796d74a in qemu_chr_fe_set_open (be=0x556dd9a37c00, fe_open=0) at /home/elmarco/src/qq/chardev/char-fe.c:340 #16 0x0000556dd796d4d9 in qemu_chr_fe_set_handlers (b=0x556dd9a37c00, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true) at /home/elmarco/src/qq/chardev/char-fe.c:280 #17 0x0000556dd796d359 in qemu_chr_fe_deinit (b=0x556dd9a37c00, del=false) at /home/elmarco/src/qq/chardev/char-fe.c:233 #18 0x0000556dd7432240 in monitor_data_destroy (mon=0x556dd9a37c00) at /home/elmarco/src/qq/monitor.c:786 #19 0x0000556dd743b968 in monitor_cleanup () at /home/elmarco/src/qq/monitor.c:4683 #20 0x0000556dd75ce776 in main (argc=3, argv=0x7fffb7bbe458, envp=0x7fffb7bbe478) at /home/elmarco/src/qq/vl.c:4660 Because spice code tries to emit a "disconnected" signal on the monitors. Fix this dead-lock by releasing the monitor lock for flush/destroy. monitor_lock protects mon_list, monitor_qapi_event_state and monitor_destroyed. monitor_flush() and monitor_data_destroy() don't access any of those variables. monitor_cleanup()'s loop is safe because it uses QTAILQ_FOREACH_SAFE(), and no further monitor can be added after calling monitor_cleanup() thanks to monitor_destroyed check in monitor_list_append(). Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster Message-Id: <20181205203737.9011-8-marcandre.lureau@redhat.com> Signed-off-by: Markus Armbruster --- monitor.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/monitor.c b/monitor.c index d353c0e645..5ce2c58b76 100644 --- a/monitor.c +++ b/monitor.c @@ -4651,8 +4651,11 @@ void monitor_cleanup(void) monitor_destroyed = true; QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) { QTAILQ_REMOVE(&mon_list, mon, entry); + /* Permit QAPI event emission from character frontend release */ + qemu_mutex_unlock(&monitor_lock); monitor_flush(mon); monitor_data_destroy(mon); + qemu_mutex_lock(&monitor_lock); g_free(mon); } qemu_mutex_unlock(&monitor_lock); -- cgit v1.2.3 From 9ab84470ffc2781df3acd4607bc6d2ae64d6d7e3 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 9 Oct 2018 14:27:13 +0800 Subject: monitor: Suspend monitor instead dropping commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a QMP client sends in-band commands more quickly that we can process them, we can either queue them without limit (QUEUE), drop commands when the queue is full (DROP), or suspend receiving commands when the queue is full (SUSPEND). None of them is ideal: * QUEUE lets a misbehaving client make QEMU eat memory without bounds. Not such a hot idea. * With DROP, the client has to cope with dropped in-band commands. To inform the client, we send a COMMAND_DROPPED event then. The event is flawed by design in two ways: it's ambiguous (see commit d621cfe0a17), and it brings back the "eat memory without bounds" problem. * With SUSPEND, the client has to manage the flow of in-band commands to keep the monitor available for out-of-band commands. We currently DROP. Switch to SUSPEND. Managing the flow of in-band commands to keep the monitor available for out-of-band commands isn't really hard: just count the number of "outstanding" in-band commands (commands sent minus replies received), and if it exceeds the limit, hold back additional ones until it drops below the limit again. Note that we need to be careful pairing the suspend with a resume, or else the monitor will hang, possibly forever. And here since we need to make sure both: (1) popping request from the req queue, and (2) reading length of the req queue will be in the same critical section, we let the pop function take the corresponding queue lock when there is a request, then we release the lock from the caller. Reviewed-by: Marc-André Lureau Signed-off-by: Peter Xu Message-Id: <20181009062718.1914-2-peterx@redhat.com> Signed-off-by: Markus Armbruster --- docs/interop/qmp-spec.txt | 5 +++-- include/monitor/monitor.h | 2 ++ monitor.c | 52 +++++++++++++++++++++-------------------------- qapi/misc.json | 40 ------------------------------------ 4 files changed, 28 insertions(+), 71 deletions(-) diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt index 8f7da0245d..67e44a8120 100644 --- a/docs/interop/qmp-spec.txt +++ b/docs/interop/qmp-spec.txt @@ -130,8 +130,9 @@ to pass "id" with out-of-band commands. Passing it with all commands is recommended for clients that accept capability "oob". If the client sends in-band commands faster than the server can -execute them, the server will eventually drop commands to limit the -queue length. The sever sends event COMMAND_DROPPED then. +execute them, the server will stop reading the requests from the QMP +channel until the request queue length is reduced to an acceptable +range. Only a few commands support out-of-band execution. The ones that do have "allow-oob": true in output of query-qmp-schema. diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 6fd2c53b09..0c0a37d8cb 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -15,6 +15,8 @@ extern __thread Monitor *cur_mon; #define MONITOR_USE_PRETTY 0x08 #define MONITOR_USE_OOB 0x10 +#define QMP_REQ_QUEUE_LEN_MAX 8 + bool monitor_cur_is_qmp(void); void monitor_init_globals(void); diff --git a/monitor.c b/monitor.c index 5ce2c58b76..d2529260ed 100644 --- a/monitor.c +++ b/monitor.c @@ -4110,8 +4110,12 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) * processing commands only on a very busy monitor. To achieve that, * when we process one request on a specific monitor, we put that * monitor to the end of mon_list queue. + * + * Note: if the function returned with non-NULL, then the caller will + * be with mon->qmp.qmp_queue_lock held, and the caller is responsible + * to release it. */ -static QMPRequest *monitor_qmp_requests_pop_any(void) +static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void) { QMPRequest *req_obj = NULL; Monitor *mon; @@ -4121,10 +4125,11 @@ static QMPRequest *monitor_qmp_requests_pop_any(void) QTAILQ_FOREACH(mon, &mon_list, entry) { qemu_mutex_lock(&mon->qmp.qmp_queue_lock); req_obj = g_queue_pop_head(mon->qmp.qmp_requests); - qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); if (req_obj) { + /* With the lock of corresponding queue held */ break; } + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); } if (req_obj) { @@ -4143,30 +4148,34 @@ static QMPRequest *monitor_qmp_requests_pop_any(void) static void monitor_qmp_bh_dispatcher(void *data) { - QMPRequest *req_obj = monitor_qmp_requests_pop_any(); + QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock(); QDict *rsp; bool need_resume; + Monitor *mon; if (!req_obj) { return; } + mon = req_obj->mon; /* qmp_oob_enabled() might change after "qmp_capabilities" */ - need_resume = !qmp_oob_enabled(req_obj->mon); + need_resume = !qmp_oob_enabled(mon) || + mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1; + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); 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); + monitor_qmp_dispatch(mon, req_obj->req, req_obj->id); } else { assert(req_obj->err); rsp = qmp_error_response(req_obj->err); req_obj->err = NULL; - monitor_qmp_respond(req_obj->mon, rsp, NULL); + monitor_qmp_respond(mon, rsp, NULL); qobject_unref(rsp); } if (need_resume) { /* Pairs with the monitor_suspend() in handle_qmp_command() */ - monitor_resume(req_obj->mon); + monitor_resume(mon); } qmp_request_free(req_obj); @@ -4174,8 +4183,6 @@ static void monitor_qmp_bh_dispatcher(void *data) qemu_bh_schedule(qmp_dispatcher_bh); } -#define QMP_REQ_QUEUE_LEN_MAX (8) - static void handle_qmp_command(void *opaque, QObject *req, Error *err) { Monitor *mon = opaque; @@ -4217,28 +4224,14 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err) qemu_mutex_lock(&mon->qmp.qmp_queue_lock); /* - * If OOB is not enabled on the current monitor, we'll emulate the - * old behavior that we won't process the current monitor any more - * until it has responded. This helps make sure that as long as - * OOB is not enabled, the server will never drop any command. + * Suspend the monitor when we can't queue more requests after + * this one. Dequeuing in monitor_qmp_bh_dispatcher() will resume + * it. Note that when OOB is disabled, we queue at most one + * command, for backward compatibility. */ - if (!qmp_oob_enabled(mon)) { + if (!qmp_oob_enabled(mon) || + mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) { monitor_suspend(mon); - } else { - /* 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); - qmp_request_free(req_obj); - return; - } } /* @@ -4246,6 +4239,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err) * handled in time order. Ownership for req_obj, req, id, * etc. will be delivered to the handler side. */ + assert(mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX); g_queue_push_tail(mon->qmp.qmp_requests, req_obj); qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); diff --git a/qapi/misc.json b/qapi/misc.json index 45121492dd..4211a732f3 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -3444,46 +3444,6 @@ ## { 'command': 'query-sev-capabilities', 'returns': 'SevCapability' } -## -# @CommandDropReason: -# -# Reasons that caused one command to be dropped. -# -# @queue-full: the command queue is full. This can only occur when -# the client sends a new non-oob command before the -# response to the previous non-oob command has been -# received. -# -# Since: 2.12 -## -{ 'enum': 'CommandDropReason', - 'data': [ 'queue-full' ] } - -## -# @COMMAND_DROPPED: -# -# Emitted when a command is dropped due to some reason. Commands can -# only be dropped when the oob capability is enabled. -# -# @id: The dropped command's "id" field. -# FIXME Broken by design. 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. -# -# @reason: The reason why the command is dropped. -# -# Since: 2.12 -# -# Example: -# -# { "event": "COMMAND_DROPPED", -# "data": {"result": {"id": "libvirt-102", -# "reason": "queue-full" } } } -# -## -{ 'event': 'COMMAND_DROPPED' , - 'data': { 'id': 'any', 'reason': 'CommandDropReason' } } - ## # @set-numa-node: # -- cgit v1.2.3 From 8258292e18c39480b64eba9f3551ab772ce29b5d Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 9 Oct 2018 14:27:15 +0800 Subject: monitor: Remove "x-oob", offer capability "oob" unconditionally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Out-of-band command execution was introduced in commit cf869d53172. Unfortunately, we ran into a regression, and had to turn it into an experimental option for 2.12 (commit be933ffc23). http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html The regression has since been fixed (commit 951702f39c7 "monitor: bind dispatch bh to iohandler context"). A thorough re-review of OOB commands led to a few more issues, which have also been addressed. This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"), and makes QMP monitors again offer capability "oob" whenever they can provide it, i.e. when the monitor's character device is capable of running in an I/O thread. Some trivial touch-up in the test code is required to make sure qmp-test won't break. Reviewed-by: Markus Armbruster Reviewed-by: Marc-André Lureau Signed-off-by: Peter Xu Message-Id: <20181009062718.1914-4-peterx@redhat.com> [Conflict with "monitor: check if chardev can switch gcontext for OOB" resolved, commit message updated] Signed-off-by: Markus Armbruster --- include/monitor/monitor.h | 1 - monitor.c | 23 +++++------------------ tests/libqtest.c | 2 +- tests/qmp-test.c | 2 +- vl.c | 5 ----- 5 files changed, 7 insertions(+), 26 deletions(-) diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 0c0a37d8cb..c1b40a9cac 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -13,7 +13,6 @@ extern __thread Monitor *cur_mon; #define MONITOR_USE_READLINE 0x02 #define MONITOR_USE_CONTROL 0x04 #define MONITOR_USE_PRETTY 0x08 -#define MONITOR_USE_OOB 0x10 #define QMP_REQ_QUEUE_LEN_MAX 8 diff --git a/monitor.c b/monitor.c index d2529260ed..6e81b09294 100644 --- a/monitor.c +++ b/monitor.c @@ -4567,22 +4567,12 @@ void monitor_init(Chardev *chr, int flags) { Monitor *mon = g_malloc(sizeof(*mon)); bool use_readline = flags & MONITOR_USE_READLINE; - bool use_oob = flags & MONITOR_USE_OOB; - - if (use_oob) { - if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) { - error_report("Monitor out-of-band is not supported with " - "%s typed chardev backend", - object_get_typename(OBJECT(chr))); - exit(1); - } - if (use_readline) { - error_report("Monitor out-of-band is only supported by QMP"); - exit(1); - } - } - monitor_data_init(mon, false, use_oob); + /* Note: we run QMP monitor in I/O thread when @chr supports that */ + monitor_data_init(mon, false, + (flags & MONITOR_USE_CONTROL) + && qemu_chr_has_feature(chr, + QEMU_CHAR_FEATURE_GCONTEXT)); qemu_chr_fe_init(&mon->chr, chr, &error_abort); mon->flags = flags; @@ -4677,9 +4667,6 @@ QemuOptsList qemu_mon_opts = { },{ .name = "pretty", .type = QEMU_OPT_BOOL, - },{ - .name = "x-oob", - .type = QEMU_OPT_BOOL, }, { /* end of list */ } }, diff --git a/tests/libqtest.c b/tests/libqtest.c index 75e07e16e7..225a123649 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -224,7 +224,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, "-display none " "%s", qemu_binary, socket_path, getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null", - qmp_socket_path, use_oob ? ",x-oob=on" : "", + qmp_socket_path, "", extra_args ?: ""); g_test_message("starting QEMU: %s", command); diff --git a/tests/qmp-test.c b/tests/qmp-test.c index 7517be4654..5ba22af6a0 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -116,7 +116,7 @@ static void test_qmp_protocol(void) g_assert(q); test_version(qdict_get(q, "version")); capabilities = qdict_get_qlist(q, "capabilities"); - g_assert(capabilities && qlist_empty(capabilities)); + g_assert(capabilities); qobject_unref(resp); /* Test valid command before handshake */ diff --git a/vl.c b/vl.c index a5ae5f23d2..2a8b2ee16d 100644 --- a/vl.c +++ b/vl.c @@ -2322,11 +2322,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp) if (qemu_opt_get_bool(opts, "pretty", 0)) flags |= MONITOR_USE_PRETTY; - /* OOB is off by default */ - if (qemu_opt_get_bool(opts, "x-oob", 0)) { - flags |= MONITOR_USE_OOB; - } - chardev = qemu_opt_get(opts, "chardev"); if (!chardev) { error_report("chardev is required"); -- cgit v1.2.3 From 192f26a74c57ca2997f6ca8e3a2fd24e943c7918 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 9 Oct 2018 14:27:16 +0800 Subject: Revert "tests: Add parameter to qtest_init_without_qmp_handshake" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a. Meanwhile, revert one line from fa198ad9bdef to make sure qtest_init_without_qmp_handshake() will only pass in one parameter. Reviewed-by: Markus Armbruster Reviewed-by: Marc-André Lureau Signed-off-by: Peter Xu Message-Id: <20181009062718.1914-5-peterx@redhat.com> Signed-off-by: Markus Armbruster --- tests/libqtest.c | 9 ++++----- tests/libqtest.h | 4 +--- tests/qmp-test.c | 4 ++-- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/tests/libqtest.c b/tests/libqtest.c index 225a123649..43be078518 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -187,8 +187,7 @@ static const char *qtest_qemu_binary(void) return qemu_bin; } -QTestState *qtest_init_without_qmp_handshake(bool use_oob, - const char *extra_args) +QTestState *qtest_init_without_qmp_handshake(const char *extra_args) { QTestState *s; int sock, qmpsock, i; @@ -219,12 +218,12 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, "-qtest unix:%s,nowait " "-qtest-log %s " "-chardev socket,path=%s,nowait,id=char0 " - "-mon chardev=char0,mode=control%s " + "-mon chardev=char0,mode=control " "-machine accel=qtest " "-display none " "%s", qemu_binary, socket_path, getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null", - qmp_socket_path, "", + qmp_socket_path, extra_args ?: ""); g_test_message("starting QEMU: %s", command); @@ -266,7 +265,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, QTestState *qtest_init(const char *extra_args) { - QTestState *s = qtest_init_without_qmp_handshake(false, extra_args); + QTestState *s = qtest_init_without_qmp_handshake(extra_args); QDict *greeting; /* Read the QMP greeting and then do the handshake */ diff --git a/tests/libqtest.h b/tests/libqtest.h index ed88ff99d5..96a6c01352 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -55,14 +55,12 @@ QTestState *qtest_init(const char *extra_args); /** * qtest_init_without_qmp_handshake: - * @use_oob: true to have the server advertise OOB support * @extra_args: other arguments to pass to QEMU. CAUTION: these * arguments are subject to word splitting and shell evaluation. * * Returns: #QTestState instance. */ -QTestState *qtest_init_without_qmp_handshake(bool use_oob, - const char *extra_args); +QTestState *qtest_init_without_qmp_handshake(const char *extra_args); /** * qtest_quit: diff --git a/tests/qmp-test.c b/tests/qmp-test.c index 5ba22af6a0..48a4fa791a 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -108,7 +108,7 @@ static void test_qmp_protocol(void) QList *capabilities; QTestState *qts; - qts = qtest_init_without_qmp_handshake(false, common_args); + qts = qtest_init_without_qmp_handshake(common_args); /* Test greeting */ resp = qtest_qmp_receive(qts); @@ -219,7 +219,7 @@ static void test_qmp_oob(void) QList *capabilities; QString *qstr; - qts = qtest_init_without_qmp_handshake(true, common_args); + qts = qtest_init_without_qmp_handshake(common_args); /* Check the greeting message. */ resp = qtest_qmp_receive(qts); -- cgit v1.2.3 From c55f070b7195cee4e06998c10f57f13c7df98dbd Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 9 Oct 2018 14:27:17 +0800 Subject: tests: add oob functional test for test-qmp-cmds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Straightforward test just to let the test-qmp-cmds be complete. Reviewed-by: Marc-André Lureau Signed-off-by: Peter Xu Message-Id: <20181009062718.1914-6-peterx@redhat.com> Signed-off-by: Markus Armbruster --- tests/test-qmp-cmds.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c index 4ab2b6e5ce..481cb069ca 100644 --- a/tests/test-qmp-cmds.c +++ b/tests/test-qmp-cmds.c @@ -126,6 +126,21 @@ static void test_dispatch_cmd(void) qobject_unref(req); } +static void test_dispatch_cmd_oob(void) +{ + QDict *req = qdict_new(); + QDict *resp; + + qdict_put_str(req, "exec-oob", "test-flags-command"); + + resp = qmp_dispatch(&qmp_commands, QOBJECT(req), true); + assert(resp != NULL); + assert(!qdict_haskey(resp, "error")); + + qobject_unref(resp); + qobject_unref(req); +} + /* test commands that return an error due to invalid parameters */ static void test_dispatch_cmd_failure(void) { @@ -302,6 +317,7 @@ int main(int argc, char **argv) g_test_init(&argc, &argv, NULL); g_test_add_func("/qmp/dispatch_cmd", test_dispatch_cmd); + g_test_add_func("/qmp/dispatch_cmd_oob", test_dispatch_cmd_oob); g_test_add_func("/qmp/dispatch_cmd_failure", test_dispatch_cmd_failure); g_test_add_func("/qmp/dispatch_cmd_io", test_dispatch_cmd_io); g_test_add_func("/qmp/dispatch_cmd_success_response", -- cgit v1.2.3