On Mon, Jun 8, 2026 at 6:41 PM Markus Armbruster <[email protected]> wrote:
>
> Zhang Chen <[email protected]> writes:
>
> > On Wed, Jun 3, 2026 at 1:02 AM Markus Armbruster <[email protected]> wrote:
> >>
> >> Zhang Chen <[email protected]> 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 <[email protected]>
> >> > ---
> >> > 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.
>
OK, I got it. In the next version, I will remove the
monitor_hmp/qmp_get_id() and
keep the NULL ID for the monitors, and use the "mon->chr->label" as
the backup ID
for the monitor.
For example:
/* monitor/monitor.c */
void monitor_data_init(Monitor *mon, Chardev *chr, bool is_qmp, bool skip_flush,
bool use_io_thread)
{
mon->chr = chr;
if (use_io_thread && !mon_iothread) {
monitor_iothread_init();
const char *holder_name = mon->id ? mon->id : chr->label;
--------------> As the backup ID
IOThreadHolder holder = {
.type = IO_THREAD_HOLDER_KIND_MONITOR_NAME,
.u.monitor_name.monitor_name = (char *)holder_name,
};
mon->ctx = iothread_ref_and_get_aio_context(mon_iothread, &holder);
} else {
mon->ctx = qemu_get_aio_context();
--------------------------> Fix the Segmentation fault you found,
thank you.
}
}
Any comments?
Thanks
Chen
>
> 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.
>
> [...]
>
