On Fri, Jul 03, 2026 at 02:48:14PM +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. 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é <[email protected]>
> > ---
> > 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 :|
