Re: [PATCH V8 06/15] monitor: assign unique default ID to anonymous monitors

[Markus Armbruster <armbru@redhat.com>]

Zhang Chen <zhangckid@gmail.com> writes:

> On Wed, Jun 3, 2026 at 1:02 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Zhang Chen <zhangckid@gmail.com> writes:
>>
>> > Currently, dynamically or implicitly created monitors (such as those
>> > generated via '-monitor stdio', GDB stub terminals, or mux chardevs)
>> > leave the 'id' field in the Monitor struct as NULL.
>>
>> There is no @id member in struct Monitor.
>
> Yes, I try to add it in this patch.
>
>>
>> >                                                     But the original
>> > @MonitorOptions have the @id field.
>>
>> QAPI type MonitorOptions has an optional member @id.
>>
>> Instances are only created by monitor_init_opts() from a QemuOpts, and
>> member @id is set to the QemuOpts's @id, which is also optional.
>>
>> monitor_init_opts() is only called during startup on behalf of CLI
>> option -mon, its shorthands -monitor, -qmp, -qmp-pretty, and for default
>> monitors.
>>
>> -mon accepts an "id" parameter, but it's optional.
>>
>> The others make up an "id".  For instance, the default monitor usually
>> gets "compat_monitor0".
>>
>> Aside: there's code that checks whether a non-null QemuOpts "id" starts
>> with "compat_monitor".  That's horrifying.
>>
>> As far as I can tell, the only way to create a monitor with null @id in
>> MonitorOptions is -mon without "id".
>>
>> This is a swamp.  Instead of messing with MonitorOptions member @id, ...
>
> I don't quite understand here, the "compat_monitor0" has been saved in
> the struct chardev's label field. I know each monitor need to bind to a chardev.
> In principle, the "compat_monitor0" mark belonging to the chardev, not
> the monitor.
> Why is it forbidden to replace monitor with chardev?
> So I think the monitor need to a unique ID field in the struct monitor.
>
>>
>> >                                     This makes it difficult to track,
>> > debug, or associate monitors with specific iothreads or
>> > infrastructure components.
>> >
>> > Fix this by introducing monitor_get_id(), which uses an atomic counter
>> > to generate a unique fallback name ("mon_default_X") when 'id' is
>> > not explicitly supplied in MonitorOptions. Update all internal HMP
>> > and QMP initialization paths to propagate and store this string.
>>
>> ... you create yet another ID that is used for nothing but referring to
>> monitors from IOThreadHolder.  Hmm.
>>
>> Is it really necessary that every monitor has an ID?  If a user want
>> one, they can simply avoid -mon without id.
>
> I think yes, the iothread ref/unref is the first user.

What would happen if we used MonitorOptions @id instead of adding yet
another monitor ID?  IOThreadHolderMonitor then has to use
MonitorOptions @id, and query-iothreads can then identify the monitor
holding the I/O thread only when the user assigned a monitor ID.

The only way *not* to assign one is -mon without "id".

I'd expect human users to use the shorthands instead of -mon, and I'd
expect management applications to use -mon with "id".  So, monitors
without "id" should be rare.

But even if they are rare, and probably unwise, they're not impossible.
What are the consequences?

I can't see truly bad ones.  If you want query-iothreads to identify
your monitors, just give them IDs!  That's why I'm doubting the need for
yet another monitor ID.

Note that entire issue goes away if we can make monitors QOM objects
before the interface becomes unchangable.

>> If it's not necessary, ignore the remainder of my review.
>>
>> > Signed-off-by: Zhang Chen <zhangckid@gmail.com>
>> > ---
>> >  chardev/char.c             |  2 +-
>> >  gdbstub/system.c           |  3 ++-
>> >  include/monitor/monitor.h  |  6 ++++--
>> >  monitor/hmp.c              | 20 +++++++++++++++++++-
>> >  monitor/monitor-internal.h |  2 ++
>> >  monitor/monitor.c          |  4 ++--
>> >  monitor/qmp.c              | 18 +++++++++++++++++-
>> >  stubs/monitor-internal.c   |  3 ++-
>> >  8 files changed, 49 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/chardev/char.c b/chardev/char.c
>> > index ca8b37ed8d..f057247001 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_init_hmp(chr, true, &err);
>> > +        monitor_init_hmp(chr, true, NULL, &err);
>> >          if (err) {
>> >              error_report_err(err);
>> >              object_unparent(OBJECT(chr));
>> > diff --git a/gdbstub/system.c b/gdbstub/system.c
>> > index e86c5870ab..50f934fde3 100644
>> > --- a/gdbstub/system.c
>> > +++ b/gdbstub/system.c
>> > @@ -388,7 +388,8 @@ 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_init_hmp(mon_chr, false, &error_abort);
>> > +
>> > +        monitor_init_hmp(mon_chr, false, NULL, &error_abort);
>> >      } else {
>> >          qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
>> >          mon_chr = gdbserver_system_state.mon_chr;
>> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> > index 55649a8664..c6ccd34cda 100644
>> > --- a/include/monitor/monitor.h
>> > +++ b/include/monitor/monitor.h
>> > @@ -19,11 +19,13 @@ bool monitor_cur_is_qmp(void);
>> >
>> >  void monitor_init_globals(void);
>> >  void monitor_init_globals_core(void);
>> > -void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp);
>> > -void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp);
>> > +void monitor_init_qmp(Chardev *chr, bool pretty, const char *id, Error **errp);
>> > +void monitor_init_hmp(Chardev *chr, bool use_readline, const char *id,
>> > +                      Error **errp);
>> >  int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp);
>> >  int monitor_init_opts(QemuOpts *opts, Error **errp);
>> >  void monitor_cleanup(void);
>> > +char *monitor_get_id(void);
>> >
>> >  int monitor_suspend(Monitor *mon);
>> >  void monitor_resume(Monitor *mon);
>> > diff --git a/monitor/hmp.c b/monitor/hmp.c
>> > index cc4390486e..47f57f69e5 100644
>> > --- a/monitor/hmp.c
>> > +++ b/monitor/hmp.c
>> > @@ -43,6 +43,8 @@
>> >  #include "system/block-backend.h"
>> >  #include "trace.h"
>> >
>> > +static int mon_hmp_id_counter;
>> > +
>> >  static void monitor_command_cb(void *opaque, const char *cmdline,
>> >                                 void *readline_opaque)
>> >  {
>> > @@ -65,6 +67,14 @@ void monitor_read_command(MonitorHMP *mon, int show_prompt)
>> >      }
>> >  }
>> >
>> > +static char *monitor_hmp_get_id(void)
>> > +{
>> > +    int id = qatomic_fetch_inc(&mon_hmp_id_counter);
>>
>> Why atomic?  As far as I can see, this is only called during startup in
>> the main thread.  Even if we merge dynamic monitor creation, the monitor
>> command to create a monitor will also run in the main thread.
>
> Yes, you are right.
> Will change it to:
> int id = mon_hmp_id_counter++;
>
>
>>
>> > +    char *name = g_strdup_printf("mon_default_hmp_%d", id);
>>
>> Can clash with the user's monitor IDs.
>
> Just run it when the user didn't input thie monitor ID:
>
> +void monitor_init_hmp(Chardev *chr, bool use_readline, const char *id,
> +                      Error **errp)
>  {
>      MonitorHMP *mon = g_new0(MonitorHMP, 1);
>
> +    if (!id) {
> +        g_autofree char *mon_id =  monitor_hmp_get_id();
> +        mon->common.id = g_strdup(mon_id);
> +    } else {
> +        mon->common.id = g_strdup(id);
> +    }

I'm not sure I was able to express myself with sufficient clarity, so
let me try again.

Something like

     -mon mode=control,chardev=qmp2 -chardev socket,id=qmp2,server=on,wait=off,path=test-qmp

will make your code generate monitor-name "mon_default_qmp_0".

Something like

    -mon mode=control,chardev=qmp2,id=mon_default_qmp_0 -chardev socket,id=qmp2,server=on,wait=off,path=test-qmp

will *also* produce monitor-name "mon_default_qmp_0".

Thus, your auto-generated monitor names can clash with the user's.
That's bad.


While testing this, I ran into a crash:

    $ qemu-system-x86_64 -S -nodefaults -display none -qmp stdio -mon mode=control,chardev=qmp2 -chardev socket,id=qmp2,server=on,wait=off,path=test-qmp
    Segmentation fault         (core dumped) qemu-system-x86_64 -S -nodefaults -display none -qmp stdio -mon mode=control,chardev=qmp2 -chardev socket,id=qmp2,server=on,wait=off,path=test-qmp

Stack backtrace:

    #0  0x0000555555f09eae in aio_bh_enqueue (bh=0x555557a18760, new_flags=10)
        at ../util/async.c:95
    #1  0x0000555555f0a084 in aio_bh_schedule_oneshot_full
        (ctx=0x0, cb=0x555555df6733 <monitor_qmp_setup_handlers_bh>, opaque=0x555557791cc0, name=0x55555648240f "monitor_qmp_setup_handlers_bh")
        at ../util/async.c:141
    #2  0x0000555555df69bf in monitor_init_qmp
        (chr=0x555557a17e60, pretty=false, id=0x0, errp=0x7fffffffd860)
        at ../monitor/qmp.c:569
    #3  0x0000555555df4e73 in monitor_init
        (opts=0x555557a18c60, allow_hmp=true, errp=0x7fffffffd860)
        at ../monitor/monitor.c:751
    #4  0x0000555555df5028 in monitor_init_opts
        (opts=0x555557790600, errp=0x555557646870 <error_fatal>)
        at ../monitor/monitor.c:784
    #5  0x0000555555a668fb in mon_init_func
        (opaque=0x0, opts=0x555557790600, errp=0x555557646870 <error_fatal>)
        at ../system/vl.c:1249
    #6  0x0000555555efb375 in qemu_opts_foreach
        (list=0x555557517da0 <qemu_mon_opts>, func=0x555555a668d4 <mon_init_func>, opaque=0x0, errp=0x555557646870 <error_fatal>) at ../util/qemu-option.c:1148
    #7  0x0000555555a68e1c in qemu_create_late_backends () at ../system/vl.c:2117
    #8  0x0000555555a6e1e2 in qemu_init (argc=11, argv=0x7fffffffdc98)
        at ../system/vl.c:3822
    #9  0x0000555555e170a8 in main (argc=11, argv=0x7fffffffdc98)
        at ../system/main.c:71

I did not bisect to find the commit that breaks this.

[...]