All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Christian Brauner <brauner@kernel.org>
Cc: qemu-devel@nongnu.org, "Markus Armbruster" <armbru@redhat.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Fabiano Rosas" <farosas@suse.de>,
	"Laurent Vivier" <lvivier@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Thomas Huth" <th.huth+qemu@posteo.eu>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>
Subject: Re: [PATCH v4 0/5] monitor: add dynamic QMP monitor hotplug support
Date: Thu, 9 Apr 2026 20:43:40 +0100	[thread overview]
Message-ID: <adgBbHDDcEhh9LpQ@redhat.com> (raw)
In-Reply-To: <20260409-work-qmp-monitor-hotplug-v4-0-89c4fdf69df1@kernel.org>

On Thu, Apr 09, 2026 at 09:18:17AM +0200, Christian Brauner wrote:
> QEMU supports runtime hotplug for chardevs, devices, block backends,
> and netdevs. Monitors are the only major subsystem that lacks this --
> all QMP monitors must be configured at launch via -mon or -qmp CLI
> options.
> 
> This series adds monitor-add, monitor-remove, and query-monitors QMP
> commands so that management tools can create independent QMP sessions
> on demand at runtime.

There's nothing inherently wrong with your proposal. It is following
the standard historical design practice we've taken for enabling
hotplug/unplug of all the other backend types. Conceptually though
our historical practice is more of an accident of evolution rather
than an ideal state.

By this I mean we have a general purpose object framework
internally and commands '-object', and 'object_add' / 'object_del'
for hotplug/unplug in QMP and HMP. Conceptually these could handle
all of our objects, devices, netdev chardevs, monitors, etc, etc,
removing the need for the specialized add/remove QMP commands for
each backend type, and the specialized CLI argsf. 

The blocker has been lack of time to convert everything/anything,
since while the conversions are not especially difficult they are
certainly time consuming due to the sheer number of types that
exist in many cases.


I'm thinking the monitor, however, is a decent opportunity to
"do the right thing" from a design POV. We only have three classes
needed in QOM, the monitor generic base class, a HMP subclass and
a QMP subclass.

If we convert QMP/HMP into QOM objects, we could then use the
pre-existing object_add/object_del commands in both HMP and QMP.

I'm not asking you to do that conversion work though. I've
been doing some experimentation myself today to test the
viability of this idea and it looks promising.

Given we've got an entire 4 month dev cycle before these
proposed monitor-add/remove commands could get into a QEMU
release, I think it is worth trialling the QOM conversion
for a short while.

> I've implemented a QMP-to-Varlink bridge in systemd. This allows
> systemd-vmspawn to control virtual machines and containers through a
> unified Varlink interface. Varlink allows protocol upgrades. For
> example, it is possible to switch from Varlink to say http. I'm
> allowing Varlink clients to upgrade from the Varlink interface to the
> native QMP interface. Such clients get a new monitor assigned that
> allows them to manage the virtual machine directly via QMP. The main
> monitor remains unaffected and tied to the generic Varlink interface. We
> can't pre-allocate monitors as we have no control over how many protocol
> upgrades we actually get but it won't just be one. And having unused
> monitors around really isn't ideal either.
> 
> Having the ability to hotplug monitors would really be helpful. I'm not
> yet super well-versed in qemu internals so this might be done wrong but
> testing works so far.
> 
> My systemd patch that triggered this is at
> https://github.com/systemd/systemd/pull/41449.
> 
> The usage pattern mirrors chardev hotplug:
> 
>   -> chardev-add id=qmp-extra backend=socket,...
>   -> monitor-add id=extra-qmp chardev=qmp-extra
>   [client connects to socket, gets QMP greeting, negotiates, sends commands]
>   -> monitor-remove id=extra-qmp
>   -> chardev-remove id=qmp-extra

With my idea the usage pattern would be basically the same
flow:

  -> chardev-add id=qmp-extra backend=socket,...
  -> object-add qom-type=monitor-qmp id=extra-qmp chardev=qmp-extra
  [client connects to socket, gets QMP greeting, negotiates, sends commands]
  -> object-del id=extra-qmp
  -> chardev-remove id=qmp-extra

> Patches 1-2 add the data model (id field in Monitor) and the
> infrastructure for safe per-monitor destruction without shutting down
> the shared dispatcher coroutine.
> 
> Patch 3 adds the QAPI schema and command handlers.
> 
> Patches 4-5 add qtest unit tests and a functional e2e test that
> performs a full hotplug -> connect -> handshake -> unplug cycle.

Patches 2, 4 and 5 would still be desirable even with the QOM
conversion I'm thinking about.

Patch 1 would be redundant since QOM would give us an ID facility
as standard.

Patch 3 would be redundant since we'd use object_add/object_del
instead.

> 
> > meson test "qtest-x86_64/qmp-test" "func-x86_64-monitor_hotplug" -v
> ninja: Entering directory `/home/brauner/src/git/qemu/build'
> [45/45] Linking target qemu-img
> 1/2 qemu:func-quick+func-x86_64 / func-x86_64-monitor_hotplug        RUNNING
> >>> QEMU_TEST_QEMU_IMG=/home/brauner/src/git/qemu/build/qemu-img ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MESON_BUILD_ROOT=/home/brauner/src/git/qemu/build LD_LIBRARY_PATH=/home/brauner/src/git/qemu/build/tests/tcg/plugins:/home/brauner/src/git/qemu/build/contrib/plugins:/home/brauner/src/go/deps/raft/.libs/:/home/brauner/src/go/deps/cowsql/.libs/ MALLOC_PERTURB_=165 PYTHONPATH=/home/brauner/src/git/qemu/python:/home/brauner/src/git/qemu/tests/functional QEMU_TEST_GDB=/usr/bin/gdb QEMU_TEST_QEMU_BINARY=/home/brauner/src/git/qemu/build/qemu-system-x86_64 MESON_TEST_ITERATION=1 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 RUST_BACKTRACE=1 /home/brauner/src/git/qemu/build/pyvenv/bin/python3 /home/brauner/src/git/qemu/tests/functional/generic/test_monitor_hotplug.py
> 2/2 qemu:qtest+qtest-x86_64 / qtest-x86_64/qmp-test                  RUNNING
> >>> ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MALLOC_PERTURB_=244 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 PYTHON=/home/brauner/src/git/qemu/build/pyvenv/bin/python3 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/home/brauner/src/git/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_BINARY=./qemu-system-x86_64 MESON_TEST_ITERATION=1 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 RUST_BACKTRACE=1 /home/brauner/src/git/qemu/build/tests/qtest/qmp-test --tap -k
> ▶ 1/2 test_monitor_hotplug.MonitorHotplug.test_events_after_negotiation OK
> ▶ 2/2 /x86_64/qmp/protocol                                           OK
> ▶ 1/2 test_monitor_hotplug.MonitorHotplug.test_hotplug_cycle         OK
> ▶ 1/2 test_monitor_hotplug.MonitorHotplug.test_large_response        OK
> ▶ 2/2 /x86_64/qmp/oob                                                OK
> ▶ 1/2 test_monitor_hotplug.MonitorHotplug.test_self_removal          OK
> 1/2 qemu:func-quick+func-x86_64 / func-x86_64-monitor_hotplug        OK              0.30s   4 subtests passed
> 
> ▶ 2/2 /x86_64/qmp/preconfig                                          OK
> ▶ 2/2 /x86_64/qmp/missing-any-arg                                    OK
> ▶ 2/2 /x86_64/qmp/monitor-add-remove                                 OK
> ▶ 2/2 /x86_64/qmp/monitor-error-paths                                OK
> ▶ 2/2 /x86_64/qmp/monitor-chardev-in-use                             OK
> ▶ 2/2 /x86_64/qmp/monitor-remove-cli                                 OK
> ▶ 2/2 /x86_64/qmp/monitor-remove-hmp                                 OK
> 2/2 qemu:qtest+qtest-x86_64 / qtest-x86_64/qmp-test                  OK              1.09s   9 subtests passed
> 
> Ok:                 2
> Expected Fail:      0
> Fail:               0
> Unexpected Pass:    0
> Skipped:            0
> Timeout:            0
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
> Changes in v4:
> - Move 'dead' field from patch 1 to patch 2 where it is first used.
> - Allow removal of any QMP monitor, drop the 'dynamic' gate in
>   qmp_monitor_remove(). Drop the 'dynamic' field from struct Monitor
>   and from MonitorInfo entirely since it no longer serves a purpose.
> - Auto-generate monitor ids ("mon0", "mon1", ...) for QMP monitors
>   created via CLI without an explicit id, so every QMP monitor is
>   addressable by monitor-remove and always appears with an id in
>   query-monitors output.
> - Change Since: 11.0 to Since: 11.1 throughout the QAPI schema.
> - Drop "GenericError" from QAPI error descriptions.
> - Update monitor-remove QAPI doc to reflect that any QMP monitor can
>   be removed, not just dynamically added ones.
> - Update qtest: test_qmp_monitor_remove_cli now expects success
>   instead of error.
> - Link to v3: https://patch.msgid.link/20260407-work-qmp-monitor-hotplug-v3-0-cb259800fffb@kernel.org
> 
> Changes in v3:
> - Use SPDX license identifier in functional test.
> - Use framework's socket_dir() helper instead of manual tempfile
>   handling for socket paths in the functional test, drop tearDown().
> - Tighten struct field comments in monitor-internal.h.
> - Wrap long qtest_add_func() registration line.
> - Link to v2: https://patch.msgid.link/20260405-work-qmp-monitor-hotplug-v2-0-ad5bedd2917a@kernel.org
> 
> Changes in v2:
> - Fix use-after-free in self-removal path: skip monitor_resume() when
>   the monitor is dead to avoid scheduling a BH against a monitor that
>   is about to be freed by monitor_qmp_destroy().
> - Hold monitor_lock in monitor_find_by_id() to prevent races with
>   the I/O thread BH that appends to mon_list.
> - Deduplicate monitor-remove commit message: trim the gcontext/out_watch
>   explanation that repeated the infrastructure commit, reference
>   monitor_cancel_out_watch() instead.
> - Add missing test descriptions to patch 4 (chardev-in-use, CLI monitor
>   rejection, HMP monitor rejection) and patch 5 (self-removal, large
>   response, events).
> - Fix cover letter wording.
> - Link to v1: https://patch.msgid.link/20260402-work-qmp-monitor-hotplug-v1-0-6313a5cdd574@kernel.org
> 
> ---
> Christian Brauner (5):
>       monitor: store monitor id in Monitor struct
>       monitor/qmp: add infrastructure for safe dynamic monitor removal
>       qapi: add monitor-add, monitor-remove, query-monitors commands
>       tests/qtest: add tests for dynamic monitor add/remove
>       tests/functional: add e2e test for dynamic QMP monitor hotplug
> 
>  include/monitor/monitor.h                        |   3 +-
>  monitor/monitor-internal.h                       |  10 ++
>  monitor/monitor.c                                |  70 ++++++--
>  monitor/qmp-cmds-control.c                       | 105 ++++++++++++
>  monitor/qmp.c                                    |  81 +++++++++-
>  qapi/control.json                                |  97 ++++++++++++
>  tests/functional/generic/meson.build             |   1 +
>  tests/functional/generic/test_monitor_hotplug.py | 170 ++++++++++++++++++++
>  tests/qtest/qmp-test.c                           | 193 +++++++++++++++++++++++
>  9 files changed, 714 insertions(+), 16 deletions(-)
> ---
> base-commit: 6d3e9dddefdd2e8e6a4942ba9399df0e47df21ed
> change-id: 20260402-work-qmp-monitor-hotplug-fba7c618e3db
> 

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



  parent reply	other threads:[~2026-04-09 19:44 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-09  7:18 [PATCH v4 0/5] monitor: add dynamic QMP monitor hotplug support Christian Brauner
2026-04-09  7:18 ` [PATCH v4 1/5] monitor: store monitor id in Monitor struct Christian Brauner
2026-04-09  7:18 ` [PATCH v4 2/5] monitor/qmp: add infrastructure for safe dynamic monitor removal Christian Brauner
2026-04-09  7:18 ` [PATCH v4 3/5] qapi: add monitor-add, monitor-remove, query-monitors commands Christian Brauner
2026-04-09  7:18 ` [PATCH v4 4/5] tests/qtest: add tests for dynamic monitor add/remove Christian Brauner
2026-04-09 13:01   ` Fabiano Rosas
2026-04-09  7:18 ` [PATCH v4 5/5] tests/functional: add e2e test for dynamic QMP monitor hotplug Christian Brauner
2026-04-09 19:43 ` Daniel P. Berrangé [this message]
2026-04-10  7:36   ` [PATCH v4 0/5] monitor: add dynamic QMP monitor hotplug support Christian Brauner
2026-04-10 16:25     ` Daniel P. Berrangé

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=adgBbHDDcEhh9LpQ@redhat.com \
    --to=berrange@redhat.com \
    --cc=armbru@redhat.com \
    --cc=brauner@kernel.org \
    --cc=eblake@redhat.com \
    --cc=farosas@suse.de \
    --cc=lvivier@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=philmd@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=th.huth+qemu@posteo.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.