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