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