On Mon, May 18, 2026 at 7:57 PM Markus Armbruster <[email protected]> wrote:
>
> Zhang Chen <[email protected]> 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 <[email protected]>
> > ---
> > 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.
Yes, you are right.
And here looks has same issue for multi iothreads:
static void monitor_iothread_init(void)
{
mon_iothread = iothread_create("mon_iothread", &error_abort);
}
If yes, I will fix it by the way in next version.
Back to this patch, the "struct Monitor" doesn't seem to be a unique
field like id or qom path.
We need to add this field or any comments here?
Then, as discussed in patch 02/14, I will add a new type for the monitor:
"IO_THREAD_HOLDER_KIND_MONITOR"
Thanks
Chen
>
> > + 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 {
>
