Re: [PATCH v3 1/5] monitor: store monitor id and dynamic flag in Monitor struct

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Apr 07, 2026 at 09:32:45AM +0200, Christian Brauner wrote:
> Add 'id', 'dynamic', and 'dead' fields to struct Monitor. The id field
> stores the monitor identifier from MonitorOptions which was previously
> parsed but discarded. The dynamic flag marks monitors created at runtime
> via the upcoming monitor-add command, and the dead flag will be used for
> deferred destruction during monitor-remove.
> 
> Extend monitor_init_qmp() to accept id and dynamic parameters so these
> are set before the monitor is added to mon_list.
> 
> For iothread monitors, move monitor_list_append() from the setup BH to
> the caller so monitor_find_by_id() can detect duplicates immediately.
> Without this, two concurrent monitor-add calls could both pass the
> duplicate check before either BH runs.  This means the monitor is now
> visible in mon_list before its chardev handlers are set up, which was
> not the case before.  This is safe because the request queue is still
> empty (no chardev handlers means no monitor_qmp_read(), so the
> dispatcher finds nothing to dispatch) and event broadcast is handled
> below.
> 
> This requires initializing mon->commands = &qmp_cap_negotiation_commands
> before monitor_list_append().  Without it, commands is NULL (from
> g_new0) and monitor_qapi_event_emit() would not skip the monitor during
> event broadcast -- its check is specifically for the
> qmp_cap_negotiation_commands pointer, so a NULL falls through to
> qmp_send_response() on an uninitialized monitor.  CHR_EVENT_OPENED sets
> commands to the same value again later.
> 
> Add monitor_find_by_id() to look up monitors by identifier.  The lookup
> takes monitor_lock to serialize with the I/O thread BH that modifies
> mon_list, but releases it before returning.  The caller must hold the
> BQL to ensure the returned pointer remains valid since only BQL holders
> can destroy monitors.
> 
> Free the id string in monitor_data_destroy().
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> ---
>  include/monitor/monitor.h  |  3 ++-
>  monitor/monitor-internal.h |  5 +++++
>  monitor/monitor.c          | 21 ++++++++++++++++++++-
>  monitor/qmp.c              | 11 ++++++++---
>  4 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 296690e1f1..7a2bb603e4 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -19,7 +19,8 @@ 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_qmp(Chardev *chr, bool pretty, const char *id,
> +                      bool dynamic, Error **errp);
>  void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp);
>  int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp);
>  int monitor_init_opts(QemuOpts *opts, Error **errp);
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index feca111ae3..4896812d4e 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -98,7 +98,10 @@ struct Monitor {
>      bool is_qmp;
>      bool skip_flush;
>      bool use_io_thread;
> +    bool dynamic;           /* true if created via monitor-add */
> +    bool dead;              /* awaiting drain after monitor-remove */

Neither of these fields appear to be used for anything in this
patch. If they're used in a later patch, then add them at time
of use.

>  
> +    char *id;               /* NULL for unnamed CLI monitors */
>      char *mon_cpu_path;
>      QTAILQ_ENTRY(Monitor) entry;
>  
> @@ -181,6 +184,8 @@ void monitor_data_destroy_qmp(MonitorQMP *mon);
>  void coroutine_fn monitor_qmp_dispatcher_co(void *data);
>  void qmp_dispatcher_co_wake(void);
>  
> +Monitor *monitor_find_by_id(const char *id);
> +
>  int get_monitor_def(Monitor *mon, int64_t *pval, const char *name);
>  void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
>  int hmp_compare_cmd(const char *name, const char *list);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 00b93ed612..7144255e12 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -622,6 +622,7 @@ void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
>  
>  void monitor_data_destroy(Monitor *mon)
>  {
> +    g_free(mon->id);
>      g_free(mon->mon_cpu_path);
>      qemu_chr_fe_deinit(&mon->chr, false);
>      if (monitor_is_qmp(mon)) {
> @@ -633,6 +634,24 @@ void monitor_data_destroy(Monitor *mon)
>      qemu_mutex_destroy(&mon->mon_lock);
>  }
>  
> +/*
> + * Look up a monitor by its id.  The monitor_lock is released before
> + * returning, so the caller must hold the BQL to ensure the returned
> + * pointer remains valid (only BQL holders can destroy monitors).
> + */
> +Monitor *monitor_find_by_id(const char *id)
> +{
> +    Monitor *mon;
> +
> +    QEMU_LOCK_GUARD(&monitor_lock);
> +    QTAILQ_FOREACH(mon, &mon_list, entry) {
> +        if (mon->id && strcmp(mon->id, id) == 0) {
> +            return mon;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  void monitor_cleanup(void)
>  {
>      /*
> @@ -732,7 +751,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, false, errp);
>          break;
>      case MONITOR_MODE_READLINE:
>          if (!allow_hmp) {
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 687019811f..afbe2283d6 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -510,10 +510,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
>      qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
>                               monitor_qmp_read, monitor_qmp_event,
>                               NULL, &mon->common, context, true);
> -    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,
> +                      bool dynamic, Error **errp)
>  {
>      MonitorQMP *mon = g_new0(MonitorQMP, 1);
>  
> @@ -527,12 +527,16 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>      monitor_data_init(&mon->common, true, false,
>                        qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
>  
> +    mon->common.id = g_strdup(id);
> +    mon->common.dynamic = dynamic;
>      mon->pretty = pretty;
>  
>      qemu_mutex_init(&mon->qmp_queue_lock);
>      mon->qmp_requests = g_queue_new();
>  
>      json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL);
> +    /* Prevent event broadcast to an uninitialized monitor. */
> +    mon->commands = &qmp_cap_negotiation_commands;
>      if (mon->common.use_io_thread) {
>          /*
>           * Make sure the old iowatch is gone.  It's possible when
> @@ -551,7 +555,8 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>           */
>          aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
>                                  monitor_qmp_setup_handlers_bh, mon);
> -        /* The bottom half will add @mon to @mon_list */
> +        /* Synchronous insert for immediate duplicate detection. */
> +        monitor_list_append(&mon->common);
>      } else {
>          qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
>                                   monitor_qmp_read, monitor_qmp_event,
> 
> -- 
> 2.47.3
> 

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