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

[Markus Armbruster <armbru@redhat.com>]

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.

>                                                     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, ...

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

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.

> +    char *name = g_strdup_printf("mon_default_hmp_%d", id);

Can clash with the user's monitor IDs.

> +
> +    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.

> +
>      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)
>  {
>  }