Daniel P. Berrangé <[email protected]> writes: > On Tue, Jun 30, 2026 at 01:55:52PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <[email protected]> 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 <[email protected]> >> > Tested-by: Peter Krempa <[email protected]> >> > Signed-off-by: Daniel P. Berrangé <[email protected]> >> > --- >> > 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
A wart you can easily cope with is still a wart. Oh well, we'll live. However, the wart should be documented. > 2. Not use both -monitor and -object on the same QEMU instance > so not have a clash between the two to begin with Management applications aware of the monitor QOM types should use -object / object-add exclusively. Human users will use -monitor to save typing. If they then need to plug an additional one at run time, they have no choice but use object-add. Again, we'll live. > 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. The risk is acceptable. The documentation complexity that comes with the wart bothers me. Could we use id_generate()? >> 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 ! Hope you brought insect repellent! >> 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. Makes sense. [...]
