[email protected] 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 <[email protected]> >> Signed-off-by: Daniel P. Berrangé <[email protected]> >> Message-ID: <[email protected]> >> >> 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: <[email protected]> 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 <[email protected]>
