Re: [PATCH v5 32/35] monitor: add support for auto-deleting monitors upon close

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jul 01, 2026 at 08:32:06AM +0200, Markus Armbruster wrote:
> 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?

The use is is that someone/something wants to spawn a script that
does some job with the QEMU monitor.  The thing that spawns the
script adds the new monitor and launches the script. Having
auto-delete means that you do not need to then keep track of that
script to perform cleanup of the dynamically added monitor. It
gives you "do the right thing" behaviour automatically when the
script exits, closing its monitor connection.

The initial series proposed by Christian supported the ability
to run "object-del' on the monitor itself - a "self delete"
essentially. That is very awkward from the code POV, as it
required special case hanlding to ensure the QMP response to
the delete action got sent on the socket before the delete
action took place. It also made it impossible to then delete
the character device.

Auto-delete gives us a better solution with less code complexity.

> >> 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?

object-del can fail if

 * An object with the specified "id" does not exist. That shouldn't
   happen in this case but harmless if it odes.
 * object_del command tries to delete the monitor that
   is servicing the object_del command. Cannot happen with
   auto-delete
 * the monitor has not finished initializing it BH with
   chardev handlers. Cannot happen if we know we have a
   live connection already.

chardev-del can fail if

 * The chardev with "id" does not exist. SHouldn't happen
   but is harmless if it does
 * The chardev reports it is "busy" - aka the frontend
   is still connected - we just deleted it so cannot happen
 * Record/replay is in use - a niche use case

So I don't think errors are a problem.

> Do we always want to delete both monitor and character device?

IMHO yes they are a pair whose lifetime should be tied together
for normal use.

> >> 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.

I think they'd be interesting for some "embedded" usage of
QEMU such as libguestfs where you want QEMU to go away
entirely when the process disconnects. But we have a differnt
mechanism via '-run-with  exit-with-parent=on' these days so
we probably don't need two mechanisms for the same concept.

> >> +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?

Yes.

> Is this worth a comment?

Sure.

> >>  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.

When I got this wrong initially bad things happened :-)

> 
> >> +             *
> >> +             * 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?

Yes.

> 
> >> +             */
> >> +            object_ref(mon);
> 
> This is the object_ref() I referred to above.

Yes.

> 
> >> +            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>
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|