Re: [PATCH v6 05/34] monitor: minimal conversion of monitors to QOM

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

On Fri, Jul 03, 2026 at 02:48:14PM +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. The Monitor base class is
> > abstract since only the HMP/QMP variants should ever be
> > created.
> >
> > When created through the existing QemuOpts interfaces, the
> > new internal QOM object will get assigned a dynamic ID with
> > the format "compat_monitorNNN" which is the historical
> > QemuOpts ID naming pattern.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  chardev/char.c             |  2 +-
> >  gdbstub/system.c           |  2 +-
> >  include/monitor/monitor.h  | 18 +++++++++++++++---
> >  monitor/hmp.c              | 31 ++++++++++++++++++++++++++++---
> >  monitor/monitor-internal.h | 18 ++++++++++++++++--
> >  monitor/monitor.c          | 31 +++++++++++++++++++++++++++----
> >  monitor/qmp-cmds.c         | 15 ++++++++-------
> >  monitor/qmp.c              | 31 ++++++++++++++++++++++++++++---
> >  stubs/monitor-internal.c   |  3 ++-
> >  system/vl.c                |  7 +++----
> >  10 files changed, 129 insertions(+), 29 deletions(-)
> >
> > diff --git a/chardev/char.c b/chardev/char.c
> > index 813c04d953..c71ffa7963 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -805,7 +805,7 @@ static Chardev *qemu_chr_new_from_name(const char *label, const char *filename,
> >  
> >      if (qemu_opt_get_bool(opts, "mux", 0)) {
> >          assert(permit_mux_mon);
> > -        monitor_new_hmp(chr, true, &err);
> > +        monitor_new_hmp(NULL, chr, true, &err);
> 
> Yet another way to create a monitor, not covered by the commit message.
>
> We get here for legacy chardev syntax "mon:...", I think.  Where exactly
> in the external interface this is accepted I'm not sure.  It's
> documented for -serial.  See qemu-options.hx.

Yes, I confirmed this codepath hits for -serial (which means it will
apply to -parallel too, but no one cares about that )

> There is no "mon" QemuOpts.
> 
> You pass NULL to monitor_new_hmp(), which makes it create the object as
> /objects/compat_monitorNNN.

Yes.

> > diff --git a/gdbstub/system.c b/gdbstub/system.c
> > index 18f6a62b2e..fda8ef9352 100644
> > --- a/gdbstub/system.c
> > +++ b/gdbstub/system.c
> > @@ -389,7 +389,7 @@ bool gdbserver_start(const char *device, Error **errp)
> >          /* Initialize a monitor terminal for gdb */
> >          mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB,
> >                                     NULL, NULL, &error_abort);
> > -        monitor_new_hmp(mon_chr, false, &error_abort);
> > +        monitor_new_hmp(NULL, mon_chr, false, &error_abort);
> 
> And yet another, also no "mon" QemuOpts, and also
> /objects/compat_monitorNNN.

Yep, triggered with -gdbstub  arg


> > diff --git a/system/vl.c b/system/vl.c
> > index 6643242729..4737c03872 100644
> > --- a/system/vl.c
> > +++ b/system/vl.c
> > @@ -1252,7 +1252,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
> >  
> >  static void monitor_parse(const char *str, const char *mode, bool pretty)
> >  {
> > -    static int monitor_device_index = 0;
> >      QemuOpts *opts;
> >      const char *p;
> >      char label[32];
> > @@ -1260,8 +1259,9 @@ static void monitor_parse(const char *str, const char *mode, bool pretty)
> >      if (strstart(str, "chardev:", &p)) {
> >          snprintf(label, sizeof(label), "%s", p);
> >      } else {
> > -        snprintf(label, sizeof(label), "compat_monitor%d",
> > -                 monitor_device_index);
> > +        g_autofree char *id = monitor_compat_id();
> > +        assert(strlen(id) < sizeof(label));
> > +        memcpy(label, id, strlen(id) + 1);
> >          opts = qemu_chr_parse_compat(label, str, true);
> >          if (!opts) {
> >              error_report("parse error: %s", str);
> > @@ -1277,7 +1277,6 @@ static void monitor_parse(const char *str, const char *mode, bool pretty)
> >      } else {
> >          assert(pretty == false);
> >      }
> > -    monitor_device_index++;
> 
> Before the patch, we increment the NNN in "compat_monitorNNN" exactly
> here.
> 
> Afterwards, we increment it in monitor_compat_id(), which gets called
> from here and several additional places.
> 
> If any of the additional places execute before an execution of the
> original place, NNN change.
> 
> Feels tolerable, but needs a mention in the commit message.

Yes, will mention the change in NNN incrementing.

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