Re: [PATCH V7 06/14] monitor: Update tracking iothread users with holder name

[Markus Armbruster <armbru@redhat.com>]

Zhang Chen <zhangckid@gmail.com> writes:

> Since the Monitor struct is not a QOM Object, we cannot use
> object_get_canonical_path(). Instead, this patch uses the monitor's
> name (or a default type-based name) as the holder identifier like QOM
> Object.
>
> Because Daniel Berrangé's "[PATCH RFC 00/17] monitor: turn QMP and HMP into
> QOM objects" under review, when that series gets merged, the kind monitor
> can go away. Current patch assume the monitor already been a QOM Object
> in IOthread. Will keep looking at Daniel Berrangé's patch about monitor
> QOM in case any future changes needed here.

Feels like this patch should not be merged as is.

> Cache the AioContext in the Monitor struct to avoid repeated calls to
> iothread_get_aio_context() and ensure symmetrical ref/unref during
> monitor lifecycle.
>
> Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> ---
>  monitor/monitor-internal.h |  3 +++
>  monitor/monitor.c          | 24 ++++++++++++++++++++++--
>  monitor/qmp.c              |  3 ++-
>  3 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index a5c4aba306..3e2c141c27 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -125,6 +125,9 @@ struct Monitor {
>      guint out_watch;
>      int mux_out;
>      int reset_seen;
> +
> +    /* iothread context */
> +    AioContext *ctx;
>  };
>  
>  struct MonitorHMP {
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 00b93ed612..b6efe776d6 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -529,7 +529,7 @@ int monitor_suspend(Monitor *mon)
>           * Kick I/O thread to make sure this takes effect.  It'll be
>           * evaluated again in prepare() of the watch object.
>           */
> -        aio_notify(iothread_get_aio_context(mon_iothread));
> +        aio_notify(mon->ctx);
>      }
>  
>      trace_monitor_suspend(mon, 1);
> @@ -564,7 +564,7 @@ void monitor_resume(Monitor *mon)
>          AioContext *ctx;
>  
>          if (mon->use_io_thread) {
> -            ctx = iothread_get_aio_context(mon_iothread);
> +            ctx = mon->ctx;
>          } else {
>              ctx = qemu_get_aio_context();
>          }
> @@ -612,6 +612,18 @@ void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
>  {
>      if (use_io_thread && !mon_iothread) {
>          monitor_iothread_init();
> +        /*
> +         * Because of current Monitor is not a QOM Object,
> +         * so using OBJECT(mon) is undefined behavior and may crash.
> +         * Try using a hard-coded future implementation of the qom path instead.
> +         * (Like the name of the "mon_iothread").
> +         * long-term solution would be making Monitor QOM, after that change
> +         * here to:
> +         * g_autofree path = object_get_canonical_path(OBJECT(mon));
> +         */
> +        g_autofree char *path = g_strdup(is_qmp ? "/monitor/qmp_mon0" :
> +                                                  "/monitor/hmp_mon0");

This is wrong.

First, inventing QOM paths is cheating.  Fine for RFC patches, but not
for merging.

Second, we can have multiple QMP and multiple HMP monitors.  For
instance

    $ qemu-system-x86_64 -S -display none -monitor stdio -chardev "socket,id=chr-mon1,path=hmp-sock,server=on,wait=off" -mon "id=mon1,chardev=chr-mon1,mode=readline"

runs QEMU with an HMP monitor on stdio, and another one on Unix domain
socket hmp-sock.

> +        mon->ctx = iothread_ref_and_get_aio_context(mon_iothread, path);
>      }
>      qemu_mutex_init(&mon->mon_lock);
>      mon->is_qmp = is_qmp;
> @@ -631,6 +643,14 @@ void monitor_data_destroy(Monitor *mon)
>      }
>      g_string_free(mon->outbuf, true);
>      qemu_mutex_destroy(&mon->mon_lock);
> +
> +    if (mon->ctx && mon_iothread) {
> +        g_autofree char *path = g_strdup(monitor_is_qmp(mon) ?
> +                                        "/monitor/qmp_mon0" :
> +                                        "/monitor/hmp_mon0");
> +        iothread_put_aio_context(mon_iothread, path);
> +        mon->ctx = NULL;
> +    }
>  }
>  
>  void monitor_cleanup(void)
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 687019811f..8e4a775fac 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -549,7 +549,8 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>           * since chardev might be running in the monitor I/O
>           * thread.  Schedule a bottom half.
>           */
> -        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> +        Monitor *mon_p = &mon->common;
> +        aio_bh_schedule_oneshot(mon_p->ctx,
>                                  monitor_qmp_setup_handlers_bh, mon);
>          /* The bottom half will add @mon to @mon_list */
>      } else {