From: Christian Brauner <brauner@kernel.org> The removal sequence is: 1. Remove from mon_list under monitor_lock. This must happen before disconnecting chardev handlers to prevent event broadcast from calling monitor_flush_locked() after the gcontext reset, which would create an out_watch on the wrong GMainContext (see monitor_cancel_out_watch()). 2. Cancel any pending out_watch while gcontext still points to the correct context. 3. Disconnect chardev handlers, passing context=NULL and close the connection. 4. Drain pending requests from any in-flight monitor_qmp_read(). 5. Destroy the monitor object Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org> [DB: extracted from a larger commit and refactored to apply to the new monitor class structure. Remove 'self delete' feature which requires complex special-case code] Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/monitor-internal.h | 2 ++ monitor/monitor.c | 22 ++++++++++++++++ monitor/qmp.c | 54 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 76 insertions(+), 2 deletions(-) diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index a82e1aacb6..5522e05464 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -178,6 +178,7 @@ struct MonitorQMP { Monitor parent_obj; JSONMessageParser parser; bool pretty; + bool setup_pending; /* iothread BH has not yet set up chardev handlers */ /* * When a client connects, we're in capabilities negotiation mode. * @commands is &qmp_cap_negotiation_commands then. When command @@ -206,6 +207,7 @@ extern MonitorList mon_list; bool monitor_requires_iothread(const Monitor *mon); int monitor_can_read(void *opaque); +void monitor_cancel_out_watch(Monitor *mon); void monitor_list_append(Monitor *mon); void monitor_fdsets_cleanup(void); int monitor_set_cpu(Monitor *mon, int cpu_index); diff --git a/monitor/monitor.c b/monitor/monitor.c index 0e1c623555..b155f58546 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -177,6 +177,28 @@ static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond, return G_SOURCE_REMOVE; } +/* Cancel a pending out_watch GSource. Caller must hold mon_lock. */ +void monitor_cancel_out_watch(Monitor *mon) +{ + if (mon->out_watch) { + GMainContext *ctx = NULL; + GSource *src; + + if (monitor_requires_iothread(mon)) { + ctx = iothread_get_g_main_context(mon_iothread); + } + src = g_main_context_find_source_by_id(ctx, mon->out_watch); + if (!src && ctx) { + /* Handler disconnect may have reset gcontext to NULL. */ + src = g_main_context_find_source_by_id(NULL, mon->out_watch); + } + if (src) { + g_source_destroy(src); + } + mon->out_watch = 0; + } +} + /* Caller must hold mon->mon_lock */ void monitor_flush_locked(Monitor *mon) { diff --git a/monitor/qmp.c b/monitor/qmp.c index 2131be82c7..5301927f09 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -184,6 +184,12 @@ static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon) } } +static void monitor_qmp_drain_queue(MonitorQMP *mon) +{ + QEMU_LOCK_GUARD(&mon->qmp_queue_lock); + monitor_qmp_cleanup_req_queue_locked(mon); +} + static void monitor_qmp_cleanup_queue_and_resume(MonitorQMP *mon) { QEMU_LOCK_GUARD(&mon->qmp_queue_lock); @@ -597,6 +603,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) monitor_qmp_read, monitor_qmp_event, NULL, &mon->parent_obj, context, true); monitor_list_append(&mon->parent_obj); + qatomic_set(&mon->setup_pending, false); } void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) @@ -645,6 +652,7 @@ static void monitor_qmp_complete(UserCreatable *uc, Error **errp) * since chardev might be running in the monitor I/O * thread. Schedule a bottom half. */ + mon->setup_pending = true; 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 */ @@ -656,8 +664,14 @@ static void monitor_qmp_complete(UserCreatable *uc, Error **errp) } } +static void monitor_qmp_iothread_quiesce(void *opaque) +{ + /* No-op: synchronization point only */ +} + static bool monitor_qmp_prepare_delete(UserCreatable *uc, Error **errp) { + Monitor *mon = MONITOR(uc); MonitorQMP *qmp = MONITOR_QMP(uc); if (monitor_qmp_dispatcher_is_servicing(qmp)) { @@ -665,8 +679,44 @@ static bool monitor_qmp_prepare_delete(UserCreatable *uc, Error **errp) return false; } - error_setg(errp, "Deleting QMP monitors is not supported"); - return false; + if (qatomic_read(&qmp->setup_pending)) { + error_setg(errp, "monitor is still initializing"); + return false; + } + + /* Remove from mon_list before chardev disconnect. */ + WITH_QEMU_LOCK_GUARD(&monitor_lock) { + QTAILQ_REMOVE(&mon_list, mon, entry); + } + + /* Cancel out_watch while gcontext still points to the right ctx. */ + WITH_QEMU_LOCK_GUARD(&mon->mon_lock) { + monitor_cancel_out_watch(mon); + } + + qemu_chr_fe_set_handlers(&mon->chr, NULL, NULL, NULL, NULL, + NULL, NULL, true); + + /* Drain requests from any in-flight monitor_qmp_read(). */ + monitor_qmp_drain_queue(qmp); + + WITH_QEMU_LOCK_GUARD(&mon->mon_lock) { + /* Disable flushes before cancel -- gcontext is already wrong. */ + qemu_chr_fe_set_open(&mon->chr, false); + monitor_cancel_out_watch(mon); + } + + /* Synchronize with in-flight iothread callbacks. */ + if (monitor_requires_iothread(mon)) { + aio_wait_bh_oneshot(iothread_get_aio_context(mon_iothread), + monitor_qmp_iothread_quiesce, NULL); + } + + /* Catch requests from a racing monitor_qmp_read(). */ + monitor_qmp_drain_queue(qmp); + monitor_fdsets_cleanup(); + + return true; } static void monitor_qmp_accept_input(Monitor *mon) -- 2.54.0