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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jul 01, 2026 at 03:19:53PM +0200, Markus Armbruster wrote:
> 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()?

Yes,  we could.

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