marcandre.lureau@redhat.com writes:
The default monitor is usually a long lived object that will exist for the entire lifetime of the VM. A monitor can only service a single client at a time though, and so it might be desirable to hotplug additional monitors at runtime for specific tasks. If doing that, however, there is a need to remove the monitor when it is no longer needed.
Whatever adds the additional monitor can also delete it. The fact that you propose other means suggests you believe this would be cumbersome in practice. Why?
Allowing a client to run "object-del" against its own monitor adds complex edge cases, as it would be desirable to send the QMP response despite the monitor sending it being deleted. Doing "object-del" alone will also result in orphaning a character device backend instance, as there is no opportunity to run the companion "chardev-del" command.
A simpler way to ensure cleanup is to add the concept of auto-deleting monitor objects. Specifically when the "CHR_EVENT_CLOSED" event is emitted, the equivalent of "object-del" + "chardev-del" can be run internally. Since the transient client has already droppped its monitor connection, there is no synchronization to be concerned about.
If object-del or chardev-del fail, there's no way to report the error. Can they fail? Do we always want to delete both monitor and character device?
This is implemented via a new "close-action=none|delete" property on the 'monitor-qmp' object. This concept could be extended with further actions in future, for example:
* close-action=shutdown - graceful guest shutdown * close-action=terminate - immediate guest poweroff * close-action=stop - pause guest CPUs while the monitor is not connected to any client
I'm not sure these would be useful and safe. Mentioning them here is okay regardless.
This is left as an exercise for future interested contributors.
Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Message-ID: <20260624173752.2928717-33-berrange@redhat.com>
diff --git a/qapi/qom.json b/qapi/qom.json index 6affb70a594..b4c49a2e8b9 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -1213,18 +1213,37 @@ 'base': 'MonitorProperties', 'data': { '*readline': 'bool' } }
+ +## +# @MonitorQMPCloseAction: +# +# Action to take when the character device backend is
No line break here.
+# closed. +# +# @none: take no action (the default)
The type cannot specify a default, only its users can. Scratch (the default). Blank line between member descriptions, please.
+# @delete: delete both the 'monitor-qmp' object and its associated +# character device backend object
Format like # @delete: delete both the 'monitor-qmp' object and its associated # character device backend object
+# +# Since 11.1
should be "Since: 11.1" (worth a new check perhaps)
docs/devel/qapi-code-gen.rst: Section tags are case-sensitive and end with a colon. They are only recognized after a blank line. Good example:: # # Since: 7.1 Bad examples (all ordinary paragraphs):: # since: 7.1 # Since 7.1 # Since : 7.1 There are a few bad ones in master. I posted a patch: Subject: [PATCH] qapi: Fix misspelled section tags in doc comments Date: Wed, 1 Jul 2026 08:11:36 +0200 Message-ID: <20260701061136.798815-1-armbru@redhat.com> Code to detect and reject such errors would have to carefully avoid false negatives.
+## +{ 'enum' : 'MonitorQMPCloseAction',
extra space, vs the usual '{ 'enum':'.
Yes.
+ 'data': ['none', 'delete'] } + ## # @MonitorQMPProperties: # # Properties for the QMP monitor # # @pretty: whether to pretty print JSON responses (default: disabled)
(default: false) See my review of PATCH 22. Blank line between member descriptions, please.
+# @close-action: action to take when the character device backend +# is closed (default: none)
Format like # @close-action: action to take when the character device backend is # closed (default: none)
# # Since: 11.1 ## { 'struct': 'MonitorQMPProperties', 'base': 'MonitorProperties', - 'data': { '*pretty': 'bool' } } + 'data': { '*pretty': 'bool', + '*close-action': 'MonitorQMPCloseAction' } }
## # @ObjectType: diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 5522e05464b..23829f32f9a 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -28,6 +28,7 @@ #include "chardev/char-fe.h" #include "monitor/monitor.h" #include "qapi/qapi-types-control.h" +#include "qapi/qapi-types-qom.h" #include "qapi/qmp-registry.h" #include "qobject/json-parser.h" #include "qemu/readline.h" @@ -178,7 +179,9 @@ struct MonitorQMP { Monitor parent_obj; JSONMessageParser parser; bool pretty; + MonitorQMPCloseAction close_action; bool setup_pending; /* iothread BH has not yet set up chardev handlers */ + bool delete_pending; /* close_action has started 'delete' process */ /* * When a client connects, we're in capabilities negotiation mode. * @commands is &qmp_cap_negotiation_commands then. When command diff --git a/monitor/qmp.c b/monitor/qmp.c index 5301927f09f..1057b0d12ba 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -28,6 +28,7 @@ #include "monitor-internal.h" #include "qapi/error.h" #include "qapi/qapi-commands-control.h" +#include "qapi/qapi-commands-char.h" #include "qobject/qdict.h" #include "qobject/qjson.h" #include "qobject/qlist.h" @@ -103,6 +104,20 @@ static void monitor_qmp_set_pretty(Object *obj, bool val, Error **errp) mon->pretty = val; }
+static int monitor_qmp_get_close_action(Object *obj, Error **errp) +{ + MonitorQMP *mon = MONITOR_QMP(obj); + + return mon->close_action; +} + +static void monitor_qmp_set_close_action(Object *obj, int val, Error **errp) +{ + MonitorQMP *mon = MONITOR_QMP(obj); + + mon->close_action = val; +} + static void monitor_qmp_emit_event(Monitor *mon, QAPIEvent event, QDict *qdict); static bool monitor_qmp_requires_iothread(const Monitor *mon); static void monitor_qmp_complete(UserCreatable *uc, Error **errp); @@ -117,6 +132,11 @@ static void monitor_qmp_class_init(ObjectClass *cls, const void *data) object_class_property_add_bool(cls, "pretty", monitor_qmp_get_pretty, monitor_qmp_set_pretty); + object_class_property_add_enum(cls, "close-action", + "MonitorQMPCloseAction", + &MonitorQMPCloseAction_lookup, + monitor_qmp_get_close_action, + monitor_qmp_set_close_action);
moncls->emit_event = monitor_qmp_emit_event; moncls->requires_iothread = monitor_qmp_requires_iothread; @@ -550,11 +570,45 @@ static QDict *qmp_greeting(MonitorQMP *mon) ver, cap_list); }
+static void monitor_qmp_self_delete_bh(void *opaque) +{ + MonitorQMP *mon = opaque; + g_autofree char *mon_id = object_property_get_child_name( + object_get_objects_root(), OBJECT(mon)); + g_autofree char *chardev_id = g_strdup(mon->parent_obj.chardev_id); + Error *local_error = NULL; + + if (!mon_id) { + /* Another monitor raced & ran 'object-del' on 'mon' + * before this BH got scheduled, so we have a ref on + * mon but it is already unparented. + */ + object_unref(mon); + return; + } + + user_creatable_del(mon_id, &local_error); + object_unref(mon);
This object_unref() and the one above pair with the object_ref() in monitor_qmp_event() case MONITOR_QMP_CLOSE_ACTION_DELETE. Correct? Is this worth a comment?
+ if (local_error != NULL) { + error_report_err(local_error); + } else { + qmp_chardev_remove(chardev_id, NULL); + } +} + static void monitor_qmp_event(void *opaque, QEMUChrEvent event) { QDict *data; MonitorQMP *mon = opaque;
+ /* Protect against race if a client drops & quickly + * reconnects - we'll have the delete BH scheduled + * so must not honour a new open request + */ + if (mon->delete_pending) { + return; + }
Consider "something happens between setting @delete_pending and monitor destruction that triggers the event" and "same thing happens after destruction". Any observable differences in behavior? Are there any other monitor interactions that might need similar special-casing when @delete_pending?
+ switch (event) { case CHR_EVENT_OPENED: WITH_QEMU_LOCK_GUARD(&mon->parent_obj.mon_lock) { @@ -577,6 +631,27 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent event) json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL); monitor_fdsets_cleanup(); + switch (mon->close_action) { + case MONITOR_QMP_CLOSE_ACTION_NONE: + break; /* nada */
Useless comment :)
+ case MONITOR_QMP_CLOSE_ACTION_DELETE: + mon->delete_pending = true; + /* + * Do NOT run in the AIO context associated with the + * monitor. We need to run in the default AIO context + * which is the same context in which 'qmp_object_del' + * will execute
I believe you. I don't have the headspace right now to confirm this myself.
+ * + * Hold an extra ref in case a separate monitor races + * with the BH by processing an explicit 'object-del'
The extra ref ensures the object still lives when the BH runs. Correct?
+ */ + object_ref(mon);
This is the object_ref() I referred to above.
+ aio_bh_schedule_oneshot(qemu_get_aio_context(), + monitor_qmp_self_delete_bh, mon); + break; + default: + g_assert_not_reached(); + } break; case CHR_EVENT_BREAK: case CHR_EVENT_MUX_IN:
[...]
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>