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

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

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.

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.

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

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>
---
 monitor/monitor-internal.h                    |  3 +
 monitor/qmp.c                                 | 75 +++++++++++++++++
 qapi/qom.json                                 | 21 ++++-
 .../generic/test_monitor_hotplug.py           | 83 ++++++++++++++++++-
 4 files changed, 177 insertions(+), 5 deletions(-)

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 5522e05464..23829f32f9 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 5301927f09..1057b0d12b 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);
+    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;
+    }
+
     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 */
+        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
+             *
+             * Hold an extra ref in case a separate monitor races
+             * with the BH by processing an explicit 'object-del'
+             */
+            object_ref(mon);
+            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:
diff --git a/qapi/qom.json b/qapi/qom.json
index 6affb70a59..b4c49a2e8b 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
+# closed.
+#
+# @none: take no action (the default)
+# @delete: delete both the 'monitor-qmp' object and its associated
+#          character device backend object
+#
+# Since 11.1
+##
+{ 'enum' : 'MonitorQMPCloseAction',
+  'data': ['none', 'delete'] }
+
 ##
 # @MonitorQMPProperties:
 #
 # Properties for the QMP monitor
 #
 # @pretty: whether to pretty print JSON responses (default: disabled)
+# @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/tests/functional/generic/test_monitor_hotplug.py b/tests/functional/generic/test_monitor_hotplug.py
index 55af8a238f..86092e781c 100755
--- a/tests/functional/generic/test_monitor_hotplug.py
+++ b/tests/functional/generic/test_monitor_hotplug.py
@@ -25,7 +25,7 @@ def setUp(self):
         sock_dir = self.socket_dir()
         self._sock_path = os.path.join(sock_dir.name, 'hotplug.sock')
 
-    def _add_monitor(self):
+    def _add_monitor(self, autodelete=False):
         """Create a chardev + monitor and return the socket path."""
         sock = self._sock_path
         self.vm.cmd('chardev-add', id='hotplug-chr', backend={
@@ -39,9 +39,15 @@ def _add_monitor(self):
                 'wait': False,
             }
         })
-        self.vm.cmd('object-add', id='hotplug-mon',
-                    qom_type='monitor-qmp',
-                    chardev='hotplug-chr')
+        if autodelete:
+            self.vm.cmd('object-add', id='hotplug-mon',
+                        qom_type='monitor-qmp',
+                        chardev='hotplug-chr',
+                        close_action='delete')
+        else:
+            self.vm.cmd('object-add', id='hotplug-mon',
+                        qom_type='monitor-qmp',
+                        chardev='hotplug-chr')
         return sock
 
     def _remove_monitor(self):
@@ -118,6 +124,75 @@ def test_self_removal(self):
         # Clean up the chardev
         self.vm.cmd('chardev-remove', id='hotplug-chr')
 
+    def test_auto_delete(self):
+        """
+        A dynamically-added monitor configured with 'close-action=delete'
+        should see itself deleted when the client is closed.
+        """
+        self.set_machine('none')
+        self.vm.add_args('-nodefaults')
+        self.vm.launch()
+
+        sock = self._add_monitor(autodelete=True)
+
+        cdevs = [c["label"] for c in self.vm.cmd('query-chardev')]
+        objs = [o["name"] for o in self.vm.cmd('qom-list', path='/objects')]
+        assert ('hotplug-chr' in cdevs)
+        assert ('hotplug-mon' in objs)
+
+        qmp = QEMUMonitorProtocol(sock)
+        greeting = qmp.connect(negotiate=True)
+        self.assertIn('QMP', greeting)
+
+        cdevs = [c["label"] for c in self.vm.cmd('query-chardev')]
+        objs = [o["name"] for o in self.vm.cmd('qom-list', path='/objects')]
+        assert ('hotplug-chr' in cdevs)
+        assert ('hotplug-mon' in objs)
+
+        qmp.close()
+
+        # Wait upto 10 seconds max for chardev to auto-delete, which
+        # is hopefully enough for reliability under high load
+        for i in range(int(10 / 0.2)):
+            cdevs = [c["label"] for c in self.vm.cmd('query-chardev')]
+            if 'hotplug-chr' not in cdevs:
+                break
+            # Wait a little more then try again
+            time.sleep(0.2)
+
+        cdevs = [c["label"] for c in self.vm.cmd('query-chardev')]
+        objs = [o["name"] for o in self.vm.cmd('qom-list', path='/objects')]
+        assert ('hotplug-chr' not in cdevs)
+        assert ('hotplug-mon' not in objs)
+
+    def test_reconnect(self):
+        """
+        A dynamically-added monitor configured without 'close-action'
+        should allow reconnects after the client is closed.
+        """
+        self.set_machine('none')
+        self.vm.add_args('-nodefaults')
+        self.vm.launch()
+
+        sock = self._add_monitor()
+
+        qmp = QEMUMonitorProtocol(sock)
+        qmp.connect(negotiate=True)
+
+        resp = qmp.cmd_obj({'execute': 'query-chardev'})
+        self.assertIn('return', resp)
+
+        qmp.close()
+
+        qmp = QEMUMonitorProtocol(sock)
+        qmp.connect(negotiate=True)
+
+        resp = qmp.cmd_obj({'execute': 'query-chardev'})
+        self.assertIn('return', resp)
+
+        qmp.close()
+        self._remove_monitor()
+
     def test_large_response(self):
         """
         Send a command with a large response (query-qmp-schema) on a
-- 
2.54.0