On Tue, Jun 30, 2026 at 01:55:52PM +0200, Markus Armbruster wrote:
Daniel P. Berrangé <berrange@redhat.com> writes:
This introduces a Monitor QOM object, with MonitorHMP and MonitorQMP subclasses. This is the bare minimum conversion of just the type declarations and replacing g_new/g_free with object_new/object_unref.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/monitor/monitor.h | 11 ++++++++++- monitor/hmp.c | 29 +++++++++++++++++++++++++++-- monitor/monitor-internal.h | 18 ++++++++++++++++-- monitor/monitor.c | 18 ++++++++++++++++-- monitor/qmp-cmds.c | 15 ++++++++------- monitor/qmp.c | 29 +++++++++++++++++++++++++++-- 6 files changed, 104 insertions(+), 16 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index b9642b58ba..2e9f9e12e9 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -5,8 +5,17 @@ #include "qapi/qapi-types-misc.h" #include "qemu/readline.h" #include "exec/hwaddr.h" +#include "qom/object.h" + +#define TYPE_MONITOR "monitor" +OBJECT_DECLARE_TYPE(Monitor, MonitorClass, MONITOR); + +#define TYPE_MONITOR_HMP "monitor-hmp" +OBJECT_DECLARE_TYPE(MonitorHMP, MonitorHMPClass, MONITOR_HMP); + +#define TYPE_MONITOR_QMP "monitor-qmp" +OBJECT_DECLARE_TYPE(MonitorQMP, MonitorQMPClass, MONITOR_QMP);
-typedef struct MonitorHMP MonitorHMP; typedef struct MonitorOptions MonitorOptions;
#define QMP_REQ_QUEUE_LEN_MAX 8 diff --git a/monitor/hmp.c b/monitor/hmp.c index 4e4468424a..81047d2513 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -43,6 +43,20 @@ #include "system/block-backend.h" #include "trace.h"
+OBJECT_DEFINE_TYPE(MonitorHMP, monitor_hmp, MONITOR_HMP, MONITOR); + +static void monitor_hmp_finalize(Object *obj) +{ +} + +static void monitor_hmp_class_init(ObjectClass *cls, const void *data) +{ +} + +static void monitor_hmp_init(Object *obj) +{ +} + static void monitor_command_cb(void *opaque, const char *cmdline, void *readline_opaque) { @@ -1526,10 +1540,21 @@ static void monitor_readline_flush(void *opaque)
void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp) { - MonitorHMP *mon = g_new0(MonitorHMP, 1); + MonitorHMP *mon; + static int counter; + g_autofree char *id = g_strdup_printf("hmpcompat%d", counter++);
Hmm. The system picking IDs is problematic when they can clash with the user's IDs. If we had an ounce of common sense, we'd restrict both across the board so they cannot clash. But we don't.
We need an ID here, because we need to make the new object the child of something (actually: child of /objects/), which requires a child name.
Non-problem with -object / object-add, because @id is mandatory there.
Non-problem with -device / device_add, because we use separate parents for devices with and without @id (/machine/peripheral/ and /machine/peripheral-anon/, plus the /machine/unattached/ orphanage).
Example for an existing problem:
$ qemu-system-x86_64 -nodefaults -monitor stdio -chardev null,id=chr0 -mon id=compat_monitor0,chardev=chr0 qemu-system-x86_64: -mon id=compat_monitor0,chardev=chr0: Duplicate ID 'compat_monitor0' for mon
Example for a problem created by this series:
$ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio -chardev null,id=chr0 -object monitor-hmp,id=hmpcompat0,chardev=chr0 qemu-system-x86_64: -monitor stdio: attempt to add duplicate property 'hmpcompat0' to object (type 'container')
I readily admit that these clashes are *unlikely*. Still, do we really want to define an interface that claims to let you pick any ID, then rejects some of them sometimes? Feels rather 1990s to me. At the very least, cover the wart in the commit message.
The way I looked at it was aything using -object with the new monitor-qmp/monitor-hmp types is new code. They can: 1. Trivially abide the warning about "hmpcompatNN" / "qmpcompatNN" being internal usage for compat syntax 2. Not use both -monitor and -object on the same QEMU instance so not have a clash between the two to begin with The remaining danger where is some existing code using -object with a *non-monitor* type, and calling it "hmpcompatNN" / "qmpcompatNN", which would be insanity. Never say never, but I think that's an acceptable risk.
This existing problem example leads me to the next mess: interaction with monitors' *other* ID.
qemu-system-FOO's -mon accepts an optional "id" parameter. It goes into its QemuOpts, and from there into MonitorOptions member @id.
Arrrggggggghh. That is not documented for -mon at all AFAICT $ qemu-system-x86_64 -help 2>&1 | grep -- -mon -monitor dev redirect the monitor to char device 'dev' -qmp dev like -monitor but opens in 'control' mode -mon [chardev=]name[,mode=readline|control][,pretty[=on|off]] but yeah as you say, it exists, and in fact libvirt even uses it despite it being undocumented.
qemu-storage-daemon's --monitor is similar, except it bypasses QemuOpts.
qemu-system-FOO provides convenience options -monitor, -qmp, -qmp-pretty.
Their argument may refer to an existing chardev by ID, like "chardev:ID". This creates a monitor with that same[*] QemuOpts and MonitorOptions ID.
Else, their argument is character device configuration in legacy syntax, like "stdio". This creates both a monitor and a character device, with ID "compat_monitorN", where N counts up from zero. The character device is visible in "info chardev", as always.
Aside: in both cases we use the same ID for two different objects, which feels unadvisable.
Agreed, that's awful.
Aside: we have code checking whether a QemuOpts or character device ID starts with "compat_monitor", which is horryfying.
Eww.
Your series does not mess with this at all. Understandable; I stay out of this swamp when I can, too.
I didn't realize the swap was there in this case !
However, it results in monitors having two IDs, namely the one in MonitorOptions, and the one in /object/. This is confusing.
Perhaps we should we'd get rid of the one in MonitorOptions. May well be more trouble than it's worth.
Could we at least make the two IDs the same?
Since -mon has an existing ID, we might as well pass it through to use as the child prop name for the objects.
diff --git a/monitor/monitor.c b/monitor/monitor.c index a87597e606..a497c25c54 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -73,6 +73,20 @@ static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */ MonitorList mon_list; static bool monitor_destroyed;
+OBJECT_DEFINE_TYPE(Monitor, monitor, MONITOR, OBJECT); + +static void monitor_finalize(Object *obj) +{ +} + +static void monitor_class_init(ObjectClass *cls, const void *data) +{ +} + +static void monitor_init(Object *obj) +{ +} + Monitor *monitor_cur(void) { Monitor *mon; @@ -598,7 +612,7 @@ void monitor_list_append(Monitor *mon)
if (mon) { monitor_data_destroy(mon); - g_free(mon); + object_unparent(OBJECT(mon)); } }
@@ -680,7 +694,7 @@ void monitor_cleanup(void) monitor_flush(mon); monitor_data_destroy(mon); qemu_mutex_lock(&monitor_lock); - g_free(mon); + object_unparent(OBJECT(mon)); } qemu_mutex_unlock(&monitor_lock);
Hmm...
{"execute": "qom-list-types", "arguments": {"implements": "monitor"}} {"return": [{"name": "monitor-hmp", "parent": "monitor"}, {"name": "monitor-qmp", "parent": "monitor"}, {"name": "monitor", "parent": "object"}]}
Shouldn't type "monitor" be abstract?
Yes it absolutely should be, and I'd swear it *was* abstract at some point in my work, but I guess I lost it along the way. 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 :|