Re: [PATCH v5 05/35] monitor: minimal conversion of monitors to QOM

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> 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

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.

[...]