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.
>
> 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);
+ }
>
> > +
> > + return name;
> > +}
> > +
> > int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
> > void *opaque)
> > {
> > @@ -1522,10 +1532,18 @@ static void monitor_readline_flush(void *opaque)
> > monitor_flush(&mon->common);
> > }
> >
> > -void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp)
> > +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);
> > + }
> > +
> > if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
> > g_free(mon);
> > return;
> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> > index a5c4aba306..2060311b03 100644
> > --- a/monitor/monitor-internal.h
> > +++ b/monitor/monitor-internal.h
> > @@ -108,6 +108,8 @@ struct Monitor {
> > bool skip_flush;
> > bool use_io_thread;
> >
> > + char *id;
>
> Needs a comment. In case you doubt it: explaining monitor IDs in my
> review of the commit message took me ~120 words.
OK, I learned a lot from your comments~
Thanks~
Chen
>
> > +
> > char *mon_cpu_path;
> > QTAILQ_ENTRY(Monitor) entry;
> >
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 00b93ed612..dafb4ad8b0 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -732,7 +732,7 @@ int monitor_init(MonitorOptions *opts, bool allow_hmp,
> > Error **errp)
> >
> > switch (opts->mode) {
> > case MONITOR_MODE_CONTROL:
> > - monitor_init_qmp(chr, opts->pretty, errp);
> > + monitor_init_qmp(chr, opts->pretty, opts->id, errp);
> > break;
> > case MONITOR_MODE_READLINE:
> > if (!allow_hmp) {
> > @@ -743,7 +743,7 @@ int monitor_init(MonitorOptions *opts, bool allow_hmp,
> > Error **errp)
> > error_setg(errp, "'pretty' is not compatible with HMP
> > monitors");
> > return -1;
> > }
> > - monitor_init_hmp(chr, true, errp);
> > + monitor_init_hmp(chr, true, opts->id, errp);
> > break;
> > default:
> > g_assert_not_reached();
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index 687019811f..b210850a15 100644
> > --- a/monitor/qmp.c
> > +++ b/monitor/qmp.c
> > @@ -56,6 +56,7 @@
> > * Access must be atomic for thread safety.
> > */
> > static bool qmp_dispatcher_co_busy = true;
> > +static int mon_qmp_id_counter;
> >
> > struct QMPRequest {
> > /* Owner of the request */
> > @@ -76,6 +77,14 @@ static bool qmp_oob_enabled(MonitorQMP *mon)
> > return mon->capab[QMP_CAPABILITY_OOB];
> > }
> >
> > +static char *monitor_qmp_get_id(void)
> > +{
> > + int id = qatomic_fetch_inc(&mon_qmp_id_counter);
>
> Again, why atomic?
>
> > + char *name = g_strdup_printf("mon_default_qmp_%d", id);
>
> Again, can clash with the user's IDs.
>
> > +
> > + return name;
> > +}
> > +
> > static void monitor_qmp_caps_reset(MonitorQMP *mon)
> > {
> > memset(mon->capab_offered, 0, sizeof(mon->capab_offered));
> > @@ -513,10 +522,17 @@ static void monitor_qmp_setup_handlers_bh(void
> > *opaque)
> > monitor_list_append(&mon->common);
> > }
> >
> > -void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
> > +void monitor_init_qmp(Chardev *chr, bool pretty, const char *id, Error
> > **errp)
> > {
> > MonitorQMP *mon = g_new0(MonitorQMP, 1);
> >
> > + if (!id) {
> > + g_autofree char *mon_id = monitor_qmp_get_id();
> > + mon->common.id = g_strdup(mon_id);
> > + } else {
> > + mon->common.id = g_strdup(id);
> > + }
> > +
> > if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) {
> > g_free(mon);
> > return;
> > diff --git a/stubs/monitor-internal.c b/stubs/monitor-internal.c
> > index 4fece49d53..325a559e62 100644
> > --- a/stubs/monitor-internal.c
> > +++ b/stubs/monitor-internal.c
> > @@ -8,6 +8,7 @@ int monitor_get_fd(Monitor *mon, const char *name, Error
> > **errp)
> > return -1;
> > }
> >
> > -void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp)
> > +void monitor_init_hmp(Chardev *chr, bool use_readline, const char *id,
> > + Error **errp)
> > {
> > }
>
