This determines whether a human monitor runs with readline for interactive use, or without readline for non-interactive use by the GDB stub. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/hmp.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/monitor/hmp.c b/monitor/hmp.c index 4ab81b5904..d5905e2279 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -50,12 +50,37 @@ static void monitor_hmp_finalize(Object *obj) { } +static bool monitor_hmp_get_readline(Object *obj, Error **errp) +{ + MonitorHMP *mon = MONITOR_HMP(obj); + + return mon->use_readline; +} + +static void monitor_hmp_set_readline(Object *obj, bool val, Error **errp) +{ + MonitorHMP *mon = MONITOR_HMP(obj); + + mon->use_readline = val; +} + static void monitor_hmp_class_init(ObjectClass *cls, const void *data) { + object_class_property_add_bool(cls, "readline", + monitor_hmp_get_readline, + monitor_hmp_set_readline); } static void monitor_hmp_init(Object *obj) { + MonitorHMP *hmp = MONITOR_HMP(obj); + + /* + * Default to common case for external HMP use, + * as opposed to non-interactive internal use + * from gdbstub + */ + hmp->use_readline = true; } static void monitor_command_cb(void *opaque, const char *cmdline, @@ -1550,6 +1575,7 @@ void monitor_new_hmp(const char *chardev_id, bool use_readline, Error **errp) id, errp, "chardev", chardev_id, + "readline", use_readline ? "yes" : "no", NULL); if (!obj) { @@ -1566,7 +1592,6 @@ void monitor_new_hmp(const char *chardev_id, bool use_readline, Error **errp) monitor_data_init(&mon->parent_obj, false, false, false); - mon->use_readline = use_readline; if (mon->use_readline) { mon->rs = readline_init(monitor_readline_printf, monitor_readline_flush, -- 2.54.0

This determines whether the QMP JSON responses are pretty printed with newlines and indentation, or compact with no extra whitespace. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/qmp.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/monitor/qmp.c b/monitor/qmp.c index 3a4e2ae0e7..ae6317b09d 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -77,8 +77,25 @@ static void monitor_qmp_finalize(Object *obj) { } +static bool monitor_qmp_get_pretty(Object *obj, Error **errp) +{ + MonitorQMP *mon = MONITOR_QMP(obj); + + return mon->pretty; +} + +static void monitor_qmp_set_pretty(Object *obj, bool val, Error **errp) +{ + MonitorQMP *mon = MONITOR_QMP(obj); + + mon->pretty = val; +} + static void monitor_qmp_class_init(ObjectClass *cls, const void *data) { + object_class_property_add_bool(cls, "pretty", + monitor_qmp_get_pretty, + monitor_qmp_set_pretty); } static void monitor_qmp_init(Object *obj) @@ -537,6 +554,7 @@ void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) id, errp, "chardev", chardev_id, + "pretty", pretty ? "yes" : "no", NULL); if (!obj) { @@ -557,8 +575,6 @@ void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) qemu_chr_has_feature(mon->parent_obj.chr.chr, QEMU_CHAR_FEATURE_GCONTEXT)); - mon->pretty = pretty; - qemu_mutex_init(&mon->qmp_queue_lock); mon->qmp_requests = g_queue_new(); -- 2.54.0

The 'skip_flush' field is set on the dummy throwaway HMP monitor object created by QMP's 'human-monitor-command', as an indication not to try to write data to the chardev. Instead the QMP command impl will grab the data straight out of the in-memory buffer. The flag is redundant, however, as the monitor code could instead simply check the 'fe_is_open' field on the CharFrontend, which will be false in the same scenarios that 'skip_flush' is true. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/hmp.c | 2 +- monitor/monitor-internal.h | 4 +--- monitor/monitor.c | 11 +++++++---- monitor/qmp-cmds.c | 2 +- monitor/qmp.c | 2 +- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/monitor/hmp.c b/monitor/hmp.c index d5905e2279..516aa25d4c 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -1590,7 +1590,7 @@ void monitor_new_hmp(const char *chardev_id, bool use_readline, Error **errp) return; } - monitor_data_init(&mon->parent_obj, false, false, false); + monitor_data_init(&mon->parent_obj, false, false); if (mon->use_readline) { mon->rs = readline_init(monitor_readline_printf, diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 145d52fd71..d34f9be139 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -112,7 +112,6 @@ struct Monitor { CharFrontend chr; int suspend_cnt; /* Needs to be accessed atomically */ bool is_qmp; - bool skip_flush; bool use_io_thread; char *mon_cpu_path; @@ -194,8 +193,7 @@ extern QemuMutex monitor_lock; extern MonitorList mon_list; void monitor_complete(Monitor *mon, Error **errp); -void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush, - bool use_io_thread); +void monitor_data_init(Monitor *mon, bool is_qmp, bool use_io_thread); void monitor_data_destroy(Monitor *mon); int monitor_can_read(void *opaque); void monitor_list_append(Monitor *mon); diff --git a/monitor/monitor.c b/monitor/monitor.c index 1f1f5fe9fe..e2b652daa8 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -187,7 +187,12 @@ void monitor_flush_locked(Monitor *mon) size_t len; const char *buf; - if (mon->skip_flush) { + /* + * When used by QMP human-monitor-command, no chardev + * will be connected, as we want to just collect the + * output in the buffer + */ + if (!mon->chr.fe_is_open) { return; } @@ -641,8 +646,7 @@ static void monitor_iothread_init(void) mon_iothread = iothread_create("mon_iothread", &error_abort); } -void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush, - bool use_io_thread) +void monitor_data_init(Monitor *mon, bool is_qmp, bool use_io_thread) { if (use_io_thread && !mon_iothread) { monitor_iothread_init(); @@ -650,7 +654,6 @@ void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush, qemu_mutex_init(&mon->mon_lock); mon->is_qmp = is_qmp; mon->outbuf = g_string_new(NULL); - mon->skip_flush = skip_flush; mon->use_io_thread = use_io_thread; } diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index bfde769ef0..0d9adad288 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -168,7 +168,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, char *output = NULL; MonitorHMP *hmp = MONITOR_HMP(object_new(TYPE_MONITOR_HMP)); - monitor_data_init(&hmp->parent_obj, false, true, false); + monitor_data_init(&hmp->parent_obj, false, false); if (has_cpu_index) { int ret = monitor_set_cpu(&hmp->parent_obj, cpu_index); diff --git a/monitor/qmp.c b/monitor/qmp.c index ae6317b09d..689cbb804c 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -571,7 +571,7 @@ void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) qemu_chr_fe_set_echo(&mon->parent_obj.chr, true); /* Note: we run QMP monitor in I/O thread when @chr supports that */ - monitor_data_init(&mon->parent_obj, true, false, + monitor_data_init(&mon->parent_obj, true, qemu_chr_has_feature(mon->parent_obj.chr.chr, QEMU_CHAR_FEATURE_GCONTEXT)); -- 2.54.0

Start to take advantage of QOM, by using object init and finalize methods to replace monitor_data_init and monitor_data_destroy. A standalone helper is provided to enable the I/O thread for QMP where appropriate for the chardev backend. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/hmp.c | 6 +++-- monitor/monitor-internal.h | 3 +-- monitor/monitor.c | 46 +++++++++++++------------------------- monitor/qmp-cmds.c | 3 --- monitor/qmp.c | 34 +++++++++++++++------------- 5 files changed, 40 insertions(+), 52 deletions(-) diff --git a/monitor/hmp.c b/monitor/hmp.c index 516aa25d4c..2bb2333da5 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -48,6 +48,10 @@ OBJECT_DEFINE_TYPE(MonitorHMP, monitor_hmp, MONITOR_HMP, MONITOR); static void monitor_hmp_finalize(Object *obj) { + MonitorHMP *mon = MONITOR_HMP(obj); + if (mon->rs) { + readline_free(mon->rs); + } } static bool monitor_hmp_get_readline(Object *obj, Error **errp) @@ -1590,8 +1594,6 @@ void monitor_new_hmp(const char *chardev_id, bool use_readline, Error **errp) return; } - monitor_data_init(&mon->parent_obj, false, false); - if (mon->use_readline) { mon->rs = readline_init(monitor_readline_printf, monitor_readline_flush, diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index d34f9be139..6a4c3b0f37 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -193,8 +193,7 @@ extern QemuMutex monitor_lock; extern MonitorList mon_list; void monitor_complete(Monitor *mon, Error **errp); -void monitor_data_init(Monitor *mon, bool is_qmp, bool use_io_thread); -void monitor_data_destroy(Monitor *mon); +void monitor_iothread_init(Monitor *mon); int monitor_can_read(void *opaque); void monitor_list_append(Monitor *mon); void monitor_fdsets_cleanup(void); diff --git a/monitor/monitor.c b/monitor/monitor.c index e2b652daa8..ae36e996e9 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -80,6 +80,10 @@ static void monitor_finalize(Object *obj) Monitor *mon = MONITOR(obj); g_free(mon->chardev_id); + g_free(mon->mon_cpu_path); + qemu_chr_fe_deinit(&mon->chr, false); + g_string_free(mon->outbuf, true); + qemu_mutex_destroy(&mon->mon_lock); } static char *monitor_get_chardev_id(Object *obj, Error **errp) @@ -105,6 +109,11 @@ static void monitor_class_init(ObjectClass *cls, const void *data) static void monitor_init(Object *obj) { + Monitor *mon = MONITOR(obj); + + qemu_mutex_init(&mon->mon_lock); + mon->is_qmp = !!object_dynamic_cast(obj, TYPE_MONITOR_QMP); + mon->outbuf = g_string_new(NULL); } Monitor *monitor_cur(void) @@ -636,38 +645,16 @@ void monitor_list_append(Monitor *mon) qemu_mutex_unlock(&monitor_lock); if (mon) { - monitor_data_destroy(mon); object_unparent(OBJECT(mon)); } } -static void monitor_iothread_init(void) -{ - mon_iothread = iothread_create("mon_iothread", &error_abort); -} - -void monitor_data_init(Monitor *mon, bool is_qmp, bool use_io_thread) +void monitor_iothread_init(Monitor *mon) { - if (use_io_thread && !mon_iothread) { - monitor_iothread_init(); + if (!mon_iothread) { + mon_iothread = iothread_create("mon_iothread", &error_abort); } - qemu_mutex_init(&mon->mon_lock); - mon->is_qmp = is_qmp; - mon->outbuf = g_string_new(NULL); - mon->use_io_thread = use_io_thread; -} - -void monitor_data_destroy(Monitor *mon) -{ - g_free(mon->mon_cpu_path); - qemu_chr_fe_deinit(&mon->chr, false); - if (monitor_is_qmp(mon)) { - monitor_data_destroy_qmp(container_of(mon, MonitorQMP, parent_obj)); - } else { - readline_free(container_of(mon, MonitorHMP, parent_obj)->rs); - } - g_string_free(mon->outbuf, true); - qemu_mutex_destroy(&mon->mon_lock); + mon->use_io_thread = true; } void monitor_cleanup(void) @@ -685,7 +672,7 @@ void monitor_cleanup(void) * Letting the iothread continue while shutting down the dispatcher * means that new requests may still be coming in. This is okay, * we'll just leave them in the queue without sending a response - * and monitor_data_destroy() will free them. + * and object finalization will free them. */ WITH_QEMU_LOCK_GUARD(&monitor_lock) { qmp_dispatcher_co_shutdown = true; @@ -699,8 +686,8 @@ void monitor_cleanup(void) /* * We need to explicitly stop the I/O thread (but not destroy it), * clean up the monitor resources, then destroy the I/O thread since - * we need to unregister from chardev below in - * monitor_data_destroy(), and chardev is not thread-safe yet + * we need to unregister from chardev below in object + * finalization, and chardev is not thread-safe yet */ if (mon_iothread) { iothread_stop(mon_iothread); @@ -715,7 +702,6 @@ void monitor_cleanup(void) /* Permit QAPI event emission from character frontend release */ qemu_mutex_unlock(&monitor_lock); monitor_flush(mon); - monitor_data_destroy(mon); qemu_mutex_lock(&monitor_lock); object_unparent(OBJECT(mon)); } diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index 0d9adad288..6cb0b587fb 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -168,8 +168,6 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, char *output = NULL; MonitorHMP *hmp = MONITOR_HMP(object_new(TYPE_MONITOR_HMP)); - monitor_data_init(&hmp->parent_obj, false, false); - if (has_cpu_index) { int ret = monitor_set_cpu(&hmp->parent_obj, cpu_index); if (ret < 0) { @@ -186,7 +184,6 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, } out: - monitor_data_destroy(&hmp->parent_obj); object_unref(hmp); return output; } diff --git a/monitor/qmp.c b/monitor/qmp.c index 689cbb804c..6e23f7f4ed 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -73,8 +73,16 @@ QmpCommandList qmp_commands, qmp_cap_negotiation_commands; OBJECT_DEFINE_TYPE(MonitorQMP, monitor_qmp, MONITOR_QMP, MONITOR); +static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon); + static void monitor_qmp_finalize(Object *obj) { + MonitorQMP *mon = MONITOR_QMP(obj); + + json_message_parser_destroy(&mon->parser); + qemu_mutex_destroy(&mon->qmp_queue_lock); + monitor_qmp_cleanup_req_queue_locked(mon); + g_queue_free(mon->qmp_requests); } static bool monitor_qmp_get_pretty(Object *obj, Error **errp) @@ -98,8 +106,15 @@ static void monitor_qmp_class_init(ObjectClass *cls, const void *data) monitor_qmp_set_pretty); } +static void handle_qmp_command(void *opaque, QObject *req, Error *err); static void monitor_qmp_init(Object *obj) { + MonitorQMP *mon = MONITOR_QMP(obj); + + qemu_mutex_init(&mon->qmp_queue_lock); + mon->qmp_requests = g_queue_new(); + + json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL); } static bool qmp_oob_enabled(MonitorQMP *mon) @@ -522,14 +537,6 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent event) } } -void monitor_data_destroy_qmp(MonitorQMP *mon) -{ - json_message_parser_destroy(&mon->parser); - qemu_mutex_destroy(&mon->qmp_queue_lock); - monitor_qmp_cleanup_req_queue_locked(mon); - g_queue_free(mon->qmp_requests); -} - static void monitor_qmp_setup_handlers_bh(void *opaque) { MonitorQMP *mon = opaque; @@ -571,14 +578,11 @@ void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) qemu_chr_fe_set_echo(&mon->parent_obj.chr, true); /* Note: we run QMP monitor in I/O thread when @chr supports that */ - monitor_data_init(&mon->parent_obj, true, - qemu_chr_has_feature(mon->parent_obj.chr.chr, - QEMU_CHAR_FEATURE_GCONTEXT)); - - qemu_mutex_init(&mon->qmp_queue_lock); - mon->qmp_requests = g_queue_new(); + if (qemu_chr_has_feature(mon->parent_obj.chr.chr, + QEMU_CHAR_FEATURE_GCONTEXT)) { + monitor_iothread_init(&mon->parent_obj); + } - json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL); if (mon->parent_obj.use_io_thread) { /* * Make sure the old iowatch is gone. It's possible when -- 2.54.0

This removes the need for using monitor_is_qmp() to check the subclass type, which is an anti-pattern. Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/hmp.c | 13 +++++++++++++ monitor/monitor-internal.h | 7 +++++++ monitor/monitor.c | 11 ++++------- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/monitor/hmp.c b/monitor/hmp.c index 2bb2333da5..08055839a4 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -68,11 +68,18 @@ static void monitor_hmp_set_readline(Object *obj, bool val, Error **errp) mon->use_readline = val; } +int monitor_hmp_vprintf(Monitor *mon, const char *fmt, va_list ap) + G_GNUC_PRINTF(2, 0); + static void monitor_hmp_class_init(ObjectClass *cls, const void *data) { + MonitorClass *moncls = MONITOR_CLASS(cls); + object_class_property_add_bool(cls, "readline", monitor_hmp_get_readline, monitor_hmp_set_readline); + + moncls->vprintf = monitor_hmp_vprintf; } static void monitor_hmp_init(Object *obj) @@ -87,6 +94,12 @@ static void monitor_hmp_init(Object *obj) hmp->use_readline = true; } +int monitor_hmp_vprintf(Monitor *mon, const char *fmt, va_list ap) +{ + g_autofree char *buf = g_strdup_vprintf(fmt, ap); + return monitor_puts(mon, buf); +} + static void monitor_command_cb(void *opaque, const char *cmdline, void *readline_opaque) { diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 6a4c3b0f37..23cef68153 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -104,6 +104,13 @@ typedef struct HMPCommand { struct MonitorClass { ObjectClass parent_class; + + /* + * If non-NULL, the monitor is able to print messages + * for attention of the client user + */ + int (*vprintf)(Monitor *mon, const char *fmt, va_list ap) + G_GNUC_PRINTF(2, 0); }; struct Monitor { diff --git a/monitor/monitor.c b/monitor/monitor.c index ae36e996e9..1ab04a19fa 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -261,21 +261,18 @@ int monitor_puts(Monitor *mon, const char *str) int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { - char *buf; - int n; + MonitorClass *moncls; if (!mon) { return -1; } - if (monitor_is_qmp(mon)) { + moncls = MONITOR_GET_CLASS(mon); + if (!moncls->vprintf) { return -1; } - buf = g_strdup_vprintf(fmt, ap); - n = monitor_puts(mon, buf); - g_free(buf); - return n; + return moncls->vprintf(mon, fmt, ap); } int monitor_printf(Monitor *mon, const char *fmt, ...) -- 2.54.0

This removes the need for using monitor_is_qmp() to check the subclass type, which is an anti-pattern. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/monitor/monitor.h | 1 + monitor/monitor-internal.h | 5 +++++ monitor/monitor.c | 15 +++------------ monitor/qmp.c | 20 ++++++++++++++++++++ 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index a135b3a590..8cb3db677b 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -3,6 +3,7 @@ #include "block/block.h" #include "qapi/qapi-types-misc.h" +#include "qapi/qapi-emit-events.h" #include "qemu/readline.h" #include "exec/hwaddr.h" #include "qom/object.h" diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 23cef68153..763b5c9625 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -111,6 +111,11 @@ struct MonitorClass { */ int (*vprintf)(Monitor *mon, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0); + /* + * If non-NULL, the monitor is able to send event + * notifications back to the client + */ + void (*emit_event)(Monitor *mon, QAPIEvent event, QDict *qdict); }; struct Monitor { diff --git a/monitor/monitor.c b/monitor/monitor.c index 1ab04a19fa..93fe542f23 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -345,22 +345,13 @@ static inline QEMUClockType monitor_get_event_clock(void) static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict) { Monitor *mon; - MonitorQMP *qmp_mon; trace_monitor_protocol_event_emit(event, qdict); QTAILQ_FOREACH(mon, &mon_list, entry) { - if (!monitor_is_qmp(mon)) { - continue; + MonitorClass *cls = MONITOR_GET_CLASS(mon); + if (cls->emit_event) { + cls->emit_event(mon, event, qdict); } - - qmp_mon = container_of(mon, MonitorQMP, parent_obj); - { - QEMU_LOCK_GUARD(&mon->mon_lock); - if (qmp_mon->commands == &qmp_cap_negotiation_commands) { - continue; - } - } - qmp_send_response(qmp_mon, qdict); } } diff --git a/monitor/qmp.c b/monitor/qmp.c index 6e23f7f4ed..b21dfbe578 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -99,11 +99,17 @@ static void monitor_qmp_set_pretty(Object *obj, bool val, Error **errp) mon->pretty = val; } +static void monitor_qmp_emit_event(Monitor *mon, QAPIEvent event, QDict *qdict); + static void monitor_qmp_class_init(ObjectClass *cls, const void *data) { + MonitorClass *moncls = MONITOR_CLASS(cls); + object_class_property_add_bool(cls, "pretty", monitor_qmp_get_pretty, monitor_qmp_set_pretty); + + moncls->emit_event = monitor_qmp_emit_event; } static void handle_qmp_command(void *opaque, QObject *req, Error *err); @@ -117,6 +123,20 @@ static void monitor_qmp_init(Object *obj) json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL); } +static void monitor_qmp_emit_event(Monitor *mon, QAPIEvent event, QDict *qdict) +{ + MonitorQMP *qmp = MONITOR_QMP(mon); + + WITH_QEMU_LOCK_GUARD(&mon->mon_lock) { + if (qmp->commands == &qmp_cap_negotiation_commands) { + return; + } + } + + qmp_send_response(qmp, qdict); +} + + static bool qmp_oob_enabled(MonitorQMP *mon) { return mon->capab[QMP_CAPABILITY_OOB]; -- 2.54.0

This removes the need for using monitor_is_qmp() to check the subclass type, which is an anti-pattern. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/hmp.c | 16 ++++++++++++++++ monitor/monitor-internal.h | 5 +++++ monitor/monitor.c | 12 +++--------- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/monitor/hmp.c b/monitor/hmp.c index 08055839a4..1da9370ead 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -70,6 +70,7 @@ static void monitor_hmp_set_readline(Object *obj, bool val, Error **errp) int monitor_hmp_vprintf(Monitor *mon, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0); +static void monitor_hmp_accept_input(Monitor *mon); static void monitor_hmp_class_init(ObjectClass *cls, const void *data) { @@ -80,6 +81,7 @@ static void monitor_hmp_class_init(ObjectClass *cls, const void *data) monitor_hmp_set_readline); moncls->vprintf = monitor_hmp_vprintf; + moncls->accept_input = monitor_hmp_accept_input; } static void monitor_hmp_init(Object *obj) @@ -100,6 +102,20 @@ int monitor_hmp_vprintf(Monitor *mon, const char *fmt, va_list ap) return monitor_puts(mon, buf); } +static void monitor_hmp_accept_input(Monitor *mon) +{ + qemu_mutex_lock(&mon->mon_lock); + if (mon->reset_seen) { + MonitorHMP *hmp = MONITOR_HMP(mon); + assert(hmp->rs); + readline_restart(hmp->rs); + qemu_mutex_unlock(&mon->mon_lock); + readline_show_prompt(hmp->rs); + } else { + qemu_mutex_unlock(&mon->mon_lock); + } +} + static void monitor_command_cb(void *opaque, const char *cmdline, void *readline_opaque) { diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 763b5c9625..592f146331 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -116,6 +116,11 @@ struct MonitorClass { * notifications back to the client */ void (*emit_event)(Monitor *mon, QAPIEvent event, QDict *qdict); + /* + * If non-NULL, perform any actions needed to prepare + * the monitor to accept further client input + */ + void (*accept_input)(Monitor *mon); }; struct Monitor { diff --git a/monitor/monitor.c b/monitor/monitor.c index 93fe542f23..4928e5201e 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -575,16 +575,10 @@ int monitor_suspend(Monitor *mon) static void monitor_accept_input(void *opaque) { Monitor *mon = opaque; + MonitorClass *cls = MONITOR_GET_CLASS(mon); - qemu_mutex_lock(&mon->mon_lock); - if (!monitor_is_qmp(mon) && mon->reset_seen) { - MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, parent_obj); - assert(hmp_mon->rs); - readline_restart(hmp_mon->rs); - qemu_mutex_unlock(&mon->mon_lock); - readline_show_prompt(hmp_mon->rs); - } else { - qemu_mutex_unlock(&mon->mon_lock); + if (cls->accept_input) { + cls->accept_input(mon); } qemu_chr_fe_accept_input(&mon->chr); -- 2.54.0

Introducing a virtual "requires_iothread" method allows the code to automatically initialize the I/O thread during object completion. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/monitor-internal.h | 14 +++++++++----- monitor/monitor.c | 22 ++++++++++++---------- monitor/qmp.c | 20 +++++++++++--------- 3 files changed, 32 insertions(+), 24 deletions(-) diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 592f146331..efd21b8912 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -121,6 +121,12 @@ struct MonitorClass { * the monitor to accept further client input */ void (*accept_input)(Monitor *mon); + + /* + * If non-NULL and returns true, then an I/O thread + * is required for processing the monitor + */ + bool (*requires_iothread)(const Monitor *mon); }; struct Monitor { @@ -129,7 +135,6 @@ struct Monitor { CharFrontend chr; int suspend_cnt; /* Needs to be accessed atomically */ bool is_qmp; - bool use_io_thread; char *mon_cpu_path; QTAILQ_ENTRY(Monitor) entry; @@ -159,9 +164,8 @@ struct MonitorHMP { bool use_readline; /* * State used only in the thread "owning" the monitor. - * If @use_io_thread, this is @mon_iothread. (This does not actually happen - * in the current state of the code.) - * Else, it's the main thread. + * This is currently always the main thread, since + * HMP does not allow use of the I/O thread at this time. * These members can be safely accessed without locks. */ ReadLineState *rs; @@ -210,7 +214,7 @@ extern QemuMutex monitor_lock; extern MonitorList mon_list; void monitor_complete(Monitor *mon, Error **errp); -void monitor_iothread_init(Monitor *mon); +bool monitor_requires_iothread(const Monitor *mon); int monitor_can_read(void *opaque); void monitor_list_append(Monitor *mon); void monitor_fdsets_cleanup(void); diff --git a/monitor/monitor.c b/monitor/monitor.c index 4928e5201e..a700d747fa 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -159,6 +159,12 @@ bool monitor_cur_is_qmp(void) return cur_mon && monitor_is_qmp(cur_mon); } +bool monitor_requires_iothread(const Monitor *mon) +{ + MonitorClass *cls = MONITOR_GET_CLASS(mon); + return cls->requires_iothread && cls->requires_iothread(mon); +} + /** * Is @mon is using readline? * Note: not all HMP monitors use readline, e.g., gdbserver has a @@ -560,7 +566,7 @@ int monitor_suspend(Monitor *mon) qatomic_inc(&mon->suspend_cnt); - if (mon->use_io_thread) { + if (monitor_requires_iothread(mon)) { /* * Kick I/O thread to make sure this takes effect. It'll be * evaluated again in prepare() of the watch object. @@ -593,7 +599,7 @@ void monitor_resume(Monitor *mon) if (qatomic_dec_fetch(&mon->suspend_cnt) == 0) { AioContext *ctx; - if (mon->use_io_thread) { + if (monitor_requires_iothread(mon)) { ctx = iothread_get_aio_context(mon_iothread); } else { ctx = qemu_get_aio_context(); @@ -631,14 +637,6 @@ void monitor_list_append(Monitor *mon) } } -void monitor_iothread_init(Monitor *mon) -{ - if (!mon_iothread) { - mon_iothread = iothread_create("mon_iothread", &error_abort); - } - mon->use_io_thread = true; -} - void monitor_cleanup(void) { /* @@ -733,6 +731,10 @@ void monitor_complete(Monitor *mon, Error **errp) return; } } + + if (monitor_requires_iothread(mon) && !mon_iothread) { + mon_iothread = iothread_create("mon_iothread", &error_abort); + } } int monitor_new(MonitorOptions *opts, bool allow_hmp, Error **errp) diff --git a/monitor/qmp.c b/monitor/qmp.c index b21dfbe578..95b2c02497 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -100,6 +100,7 @@ static void monitor_qmp_set_pretty(Object *obj, bool val, Error **errp) } static void monitor_qmp_emit_event(Monitor *mon, QAPIEvent event, QDict *qdict); +static bool monitor_qmp_requires_iothread(const Monitor *mon); static void monitor_qmp_class_init(ObjectClass *cls, const void *data) { @@ -110,6 +111,7 @@ static void monitor_qmp_class_init(ObjectClass *cls, const void *data) monitor_qmp_set_pretty); moncls->emit_event = monitor_qmp_emit_event; + moncls->requires_iothread = monitor_qmp_requires_iothread; } static void handle_qmp_command(void *opaque, QObject *req, Error *err); @@ -136,6 +138,11 @@ static void monitor_qmp_emit_event(Monitor *mon, QAPIEvent event, QDict *qdict) qmp_send_response(qmp, qdict); } +static bool monitor_qmp_requires_iothread(const Monitor *mon) +{ + return qemu_chr_has_feature(mon->chr.chr, + QEMU_CHAR_FEATURE_GCONTEXT); +} static bool qmp_oob_enabled(MonitorQMP *mon) { @@ -146,7 +153,8 @@ static void monitor_qmp_caps_reset(MonitorQMP *mon) { memset(mon->capab_offered, 0, sizeof(mon->capab_offered)); memset(mon->capab, 0, sizeof(mon->capab)); - mon->capab_offered[QMP_CAPABILITY_OOB] = mon->parent_obj.use_io_thread; + mon->capab_offered[QMP_CAPABILITY_OOB] = + monitor_requires_iothread(MONITOR(mon)); } static void qmp_request_free(QMPRequest *req) @@ -562,7 +570,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) MonitorQMP *mon = opaque; GMainContext *context; - assert(mon->parent_obj.use_io_thread); + assert(monitor_requires_iothread(MONITOR(mon))); context = iothread_get_g_main_context(mon_iothread); assert(context); qemu_chr_fe_set_handlers(&mon->parent_obj.chr, monitor_can_read, @@ -597,13 +605,7 @@ void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) qemu_chr_fe_set_echo(&mon->parent_obj.chr, true); - /* Note: we run QMP monitor in I/O thread when @chr supports that */ - if (qemu_chr_has_feature(mon->parent_obj.chr.chr, - QEMU_CHAR_FEATURE_GCONTEXT)) { - monitor_iothread_init(&mon->parent_obj); - } - - if (mon->parent_obj.use_io_thread) { + if (monitor_requires_iothread(MONITOR(mon))) { /* * Make sure the old iowatch is gone. It's possible when * e.g. the chardev is in client mode, with wait=on. -- 2.54.0

This eliminates a use of monitor_is_qmp() from the QMP coroutine dispatch path. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/qmp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/monitor/qmp.c b/monitor/qmp.c index 95b2c02497..5cf38c6273 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -277,11 +277,12 @@ static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void) MonitorQMP *qmp_mon; QTAILQ_FOREACH(mon, &mon_list, entry) { - if (!monitor_is_qmp(mon)) { + qmp_mon = MONITOR_QMP( + object_dynamic_cast(OBJECT(mon), TYPE_MONITOR_QMP)); + if (!qmp_mon) { continue; } - qmp_mon = container_of(mon, MonitorQMP, parent_obj); qemu_mutex_lock(&qmp_mon->qmp_queue_lock); req_obj = g_queue_pop_head(qmp_mon->qmp_requests); if (req_obj) { -- 2.54.0

This eliminates a use of monitor_is_qmp() from the error reporting path. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- util/error-report.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/util/error-report.c b/util/error-report.c index f832ad9b6b..f333af9249 100644 --- a/util/error-report.c +++ b/util/error-report.c @@ -231,16 +231,15 @@ char *real_time_iso8601(void) G_GNUC_PRINTF(2, 0) static void vreport(report_type type, const char *fmt, va_list ap) { - Monitor *cur = monitor_cur(); - gchar *timestr; - /* * When current monitor is QMP, messages must go to stderr - * and have prefixes added + * and have prefixes added, so we cast to HMP, leaving 'cur' + * as NULL in QMP case */ - if (monitor_cur_is_qmp()) { - cur = NULL; - } + Monitor *cur = MONITOR( + object_dynamic_cast(OBJECT(monitor_cur()), TYPE_MONITOR_HMP)); + gchar *timestr; + if (!cur) { qemu_flockfile(stderr); } -- 2.54.0

The previous patch dropped the only remaining use of monitor_cur_is_qmp. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/monitor/monitor.h | 1 - monitor/monitor.c | 10 ---------- stubs/monitor-core.c | 5 ----- tests/unit/test-util-sockets.c | 1 - tools/qemu-vnc/stubs.c | 5 ----- 5 files changed, 22 deletions(-) diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 8cb3db677b..3f730cffee 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -25,7 +25,6 @@ extern QemuOptsList qemu_mon_opts; Monitor *monitor_cur(void); Monitor *monitor_set_cur(Coroutine *co, Monitor *mon); -bool monitor_cur_is_qmp(void); void monitor_init_globals(void); void monitor_init_globals_core(void); diff --git a/monitor/monitor.c b/monitor/monitor.c index a700d747fa..44b1d5ea7f 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -149,16 +149,6 @@ Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) return old_monitor; } -/** - * Is the current monitor, if any, a QMP monitor? - */ -bool monitor_cur_is_qmp(void) -{ - Monitor *cur_mon = monitor_cur(); - - return cur_mon && monitor_is_qmp(cur_mon); -} - bool monitor_requires_iothread(const Monitor *mon) { MonitorClass *cls = MONITOR_GET_CLASS(mon); diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c index 078a5012e9..a7c32297c9 100644 --- a/stubs/monitor-core.c +++ b/stubs/monitor-core.c @@ -7,11 +7,6 @@ Monitor *monitor_cur(void) return NULL; } -bool monitor_cur_is_qmp(void) -{ - return false; -} - Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { return NULL; diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c index b9f2453e29..ee66d727c3 100644 --- a/tests/unit/test-util-sockets.c +++ b/tests/unit/test-util-sockets.c @@ -74,7 +74,6 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp) Monitor *monitor_cur(void) { return cur_mon; } Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { abort(); } int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); } -bool monitor_cur_is_qmp(void) { abort(); }; #ifndef _WIN32 static void test_socket_fd_pass_name_good(void) diff --git a/tools/qemu-vnc/stubs.c b/tools/qemu-vnc/stubs.c index a865ce85f0..1c82d8cff4 100644 --- a/tools/qemu-vnc/stubs.c +++ b/tools/qemu-vnc/stubs.c @@ -36,11 +36,6 @@ Monitor *monitor_cur(void) return NULL; } -bool monitor_cur_is_qmp(void) -{ - return false; -} - Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { return NULL; -- 2.54.0

Rather than asserting monitor_is_qmp(), use a QOM cast via MONITOR_QMP which performs an assert already. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/qmp-cmds-control.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c index af6a2a118b..8fe0876a3d 100644 --- a/monitor/qmp-cmds-control.c +++ b/monitor/qmp-cmds-control.c @@ -72,11 +72,7 @@ static bool qmp_caps_accept(MonitorQMP *mon, QMPCapabilityList *list, void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable, Error **errp) { - Monitor *cur_mon = monitor_cur(); - MonitorQMP *mon; - - assert(monitor_is_qmp(cur_mon)); - mon = container_of(cur_mon, MonitorQMP, parent_obj); + MonitorQMP *mon = MONITOR_QMP(monitor_cur()); if (mon->commands == &qmp_commands) { error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND, @@ -122,11 +118,7 @@ static void query_commands_cb(const QmpCommand *cmd, void *opaque) CommandInfoList *qmp_query_commands(Error **errp) { CommandInfoList *list = NULL; - Monitor *cur_mon = monitor_cur(); - MonitorQMP *mon; - - assert(monitor_is_qmp(cur_mon)); - mon = container_of(cur_mon, MonitorQMP, parent_obj); + MonitorQMP *mon = MONITOR_QMP(monitor_cur()); qmp_for_each_command(mon->commands, query_commands_cb, &list); -- 2.54.0

Rather than checking !monitor_is_qmp(), use a dynamic cast to check for HMP. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monitor/monitor.c b/monitor/monitor.c index 44b1d5ea7f..578aca70d4 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -167,7 +167,7 @@ static inline bool monitor_uses_readline(const MonitorHMP *mon) static inline bool monitor_is_hmp_non_interactive(const Monitor *mon) { - if (monitor_is_qmp(mon)) { + if (!object_dynamic_cast(OBJECT(mon), TYPE_MONITOR_HMP)) { return false; } -- 2.54.0

The previous patch dropped the only remaining use of monitor_is_qmp. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/monitor-internal.h | 9 --------- monitor/monitor.c | 1 - 2 files changed, 10 deletions(-) diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index efd21b8912..fe5703af6d 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -134,7 +134,6 @@ struct Monitor { char *chardev_id; CharFrontend chr; int suspend_cnt; /* Needs to be accessed atomically */ - bool is_qmp; char *mon_cpu_path; QTAILQ_ENTRY(Monitor) entry; @@ -197,14 +196,6 @@ struct MonitorQMP { GQueue *qmp_requests; }; -/** - * Is @mon a QMP monitor? - */ -static inline bool monitor_is_qmp(const Monitor *mon) -{ - return mon->is_qmp; -} - typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList; extern IOThread *mon_iothread; extern Coroutine *qmp_dispatcher_co; diff --git a/monitor/monitor.c b/monitor/monitor.c index 578aca70d4..0cace985f0 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -112,7 +112,6 @@ static void monitor_init(Object *obj) Monitor *mon = MONITOR(obj); qemu_mutex_init(&mon->mon_lock); - mon->is_qmp = !!object_dynamic_cast(obj, TYPE_MONITOR_QMP); mon->outbuf = g_string_new(NULL); } -- 2.54.0

The monitor_is_hmp_non_interactive method is used by monitor_suspend and monitor_resume, to make them a no-op if the HMP does not use readline. There are only a handful of callers of suspend/resume and they can be made to skip the call when readline is not present. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/monitor/monitor.h | 2 +- migration/migration-hmp-cmds.c | 5 ++++- monitor/hmp-cmds.c | 5 ++++- monitor/hmp.c | 11 ++++++++--- monitor/monitor.c | 30 +----------------------------- 5 files changed, 18 insertions(+), 35 deletions(-) diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 3f730cffee..ffda343585 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -34,7 +34,7 @@ int monitor_new(MonitorOptions *opts, bool allow_hmp, Error **errp); int monitor_new_opts(QemuOpts *opts, Error **errp); void monitor_cleanup(void); -int monitor_suspend(Monitor *mon); +void monitor_suspend(Monitor *mon); void monitor_resume(Monitor *mon); int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp); diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 8b385f560e..502ca704da 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -19,6 +19,7 @@ #include "monitor/hmp.h" #include "monitor/hmp-completion.h" #include "monitor/monitor.h" +#include "monitor/monitor-internal.h" #include "qapi/error.h" #include "qapi/qapi-commands-migration.h" #include "qapi/qapi-visit-migration.h" @@ -853,12 +854,14 @@ void hmp_migrate(Monitor *mon, const QDict *qdict) if (!detach) { HMPMigrationStatus *status; + MonitorHMP *hmp = MONITOR_HMP(mon); - if (monitor_suspend(mon) < 0) { + if (!hmp->use_readline) { monitor_printf(mon, "terminal does not allow synchronous " "migration, continuing detached\n"); return; } + monitor_suspend(mon); status = g_malloc0(sizeof(*status)); status->mon = mon; diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index e139caeba9..e9fb8d827a 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -129,7 +129,10 @@ void hmp_info_version(Monitor *mon, const QDict *qdict) void hmp_quit(Monitor *mon, const QDict *qdict) { - monitor_suspend(mon); + MonitorHMP *hmp = MONITOR_HMP(mon); + if (hmp->use_readline) { + monitor_suspend(mon); + } qmp_quit(NULL); } diff --git a/monitor/hmp.c b/monitor/hmp.c index 1da9370ead..b8cccdcde3 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -1528,13 +1528,16 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size) static void monitor_event(void *opaque, QEMUChrEvent event) { Monitor *mon = opaque; + MonitorHMP *hmp = MONITOR_HMP(mon); switch (event) { case CHR_EVENT_MUX_IN: qemu_mutex_lock(&mon->mon_lock); if (mon->mux_out) { mon->mux_out = 0; - monitor_resume(mon); + if (hmp->use_readline) { + monitor_resume(mon); + } } qemu_mutex_unlock(&mon->mon_lock); break; @@ -1547,7 +1550,9 @@ static void monitor_event(void *opaque, QEMUChrEvent event) } else { monitor_flush_locked(mon); } - monitor_suspend(mon); + if (hmp->use_readline) { + monitor_suspend(mon); + } mon->mux_out = 1; } qemu_mutex_unlock(&mon->mon_lock); @@ -1558,7 +1563,7 @@ static void monitor_event(void *opaque, QEMUChrEvent event) "information\n", QEMU_VERSION); qemu_mutex_lock(&mon->mon_lock); mon->reset_seen = 1; - if (!mon->mux_out) { + if (!mon->mux_out && hmp->use_readline) { /* Suspend-resume forces the prompt to be printed. */ monitor_suspend(mon); monitor_resume(mon); diff --git a/monitor/monitor.c b/monitor/monitor.c index 0cace985f0..c401e23b7b 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -154,25 +154,6 @@ bool monitor_requires_iothread(const Monitor *mon) return cls->requires_iothread && cls->requires_iothread(mon); } -/** - * Is @mon is using readline? - * Note: not all HMP monitors use readline, e.g., gdbserver has a - * non-interactive HMP monitor, so readline is not used there. - */ -static inline bool monitor_uses_readline(const MonitorHMP *mon) -{ - return mon->use_readline; -} - -static inline bool monitor_is_hmp_non_interactive(const Monitor *mon) -{ - if (!object_dynamic_cast(OBJECT(mon), TYPE_MONITOR_HMP)) { - return false; - } - - return !monitor_uses_readline(container_of(mon, MonitorHMP, parent_obj)); -} - static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond, void *opaque) { @@ -547,12 +528,8 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b) return TRUE; } -int monitor_suspend(Monitor *mon) +void monitor_suspend(Monitor *mon) { - if (monitor_is_hmp_non_interactive(mon)) { - return -ENOTTY; - } - qatomic_inc(&mon->suspend_cnt); if (monitor_requires_iothread(mon)) { @@ -564,7 +541,6 @@ int monitor_suspend(Monitor *mon) } trace_monitor_suspend(mon, 1); - return 0; } static void monitor_accept_input(void *opaque) @@ -581,10 +557,6 @@ static void monitor_accept_input(void *opaque) void monitor_resume(Monitor *mon) { - if (monitor_is_hmp_non_interactive(mon)) { - return; - } - if (qatomic_dec_fetch(&mon->suspend_cnt) == 0) { AioContext *ctx; -- 2.54.0

Implement the user creatable QOM interface and define the monitor-qmp and monitor-hmp types in QAPI. This unlocks the ability to create them on the command line with -object or in HMP/QMP with object_add. For example: $QEMU -chardev stdio,id=monchr0 -object monitor-hmp,id=mon0,chrdev=monchr0 Initially the "prepare_delete" callback is hardcoded to return an error which means -object and object_add can be used, but object_del will fail. Support for deleting monitors will be introduced in subsequent commits. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/hmp.c | 70 ++++++++++++++++++++++++-------------- monitor/monitor-internal.h | 1 - monitor/monitor.c | 14 ++++++-- monitor/qmp.c | 46 +++++++++++++++++-------- qapi/qom.json | 43 +++++++++++++++++++++++ stubs/monitor-internal.c | 1 + system/vl.c | 8 ++++- 7 files changed, 138 insertions(+), 45 deletions(-) diff --git a/monitor/hmp.c b/monitor/hmp.c index b8cccdcde3..2cd508f1ee 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -40,6 +40,7 @@ #include "qemu/base-arch-defs.h" #include "qemu/target-info.h" #include "qemu/units.h" +#include "qom/object_interfaces.h" #include "exec/gdbstub.h" #include "system/block-backend.h" #include "trace.h" @@ -71,10 +72,13 @@ static void monitor_hmp_set_readline(Object *obj, bool val, Error **errp) int monitor_hmp_vprintf(Monitor *mon, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0); static void monitor_hmp_accept_input(Monitor *mon); +static void monitor_hmp_complete(UserCreatable *uc, Error **errp); +static bool monitor_hmp_prepare_delete(UserCreatable *uc, Error **errp); static void monitor_hmp_class_init(ObjectClass *cls, const void *data) { MonitorClass *moncls = MONITOR_CLASS(cls); + UserCreatableClass *ucc = USER_CREATABLE_CLASS(cls); object_class_property_add_bool(cls, "readline", monitor_hmp_get_readline, @@ -82,6 +86,9 @@ static void monitor_hmp_class_init(ObjectClass *cls, const void *data) moncls->vprintf = monitor_hmp_vprintf; moncls->accept_input = monitor_hmp_accept_input; + + ucc->complete = monitor_hmp_complete; + ucc->prepare_delete = monitor_hmp_prepare_delete; } static void monitor_hmp_init(Object *obj) @@ -1604,42 +1611,53 @@ static void monitor_readline_flush(void *opaque) void monitor_new_hmp(const char *chardev_id, bool use_readline, Error **errp) { - ERRP_GUARD(); - MonitorHMP *mon; static int counter; g_autofree char *id = g_strdup_printf("hmpcompat%d", counter++); - Object *obj = object_new_with_props(TYPE_MONITOR_HMP, - object_get_objects_root(), - id, - errp, - "chardev", chardev_id, - "readline", use_readline ? "yes" : "no", - NULL); - - if (!obj) { - return; - } + object_new_with_props(TYPE_MONITOR_HMP, + object_get_objects_root(), + id, + errp, + "chardev", chardev_id, + "readline", use_readline ? "yes" : "no", + NULL); +} - mon = MONITOR_HMP(obj); +static void monitor_hmp_complete(UserCreatable *uc, Error **errp) +{ + MonitorHMP *mon = MONITOR_HMP(uc); + UserCreatableClass *ucc_parent = + USER_CREATABLE_CLASS( + object_class_get_parent( + OBJECT_CLASS(MONITOR_HMP_GET_CLASS(mon)))); + ERRP_GUARD(); - monitor_complete(MONITOR(mon), errp); + ucc_parent->complete(uc, errp); if (*errp) { - object_unparent(OBJECT(mon)); return; } - if (mon->use_readline) { - mon->rs = readline_init(monitor_readline_printf, - monitor_readline_flush, - mon, - monitor_find_completion); - monitor_read_command(mon, 0); + if (mon->parent_obj.chardev_id) { + if (mon->use_readline) { + mon->rs = readline_init(monitor_readline_printf, + monitor_readline_flush, + mon, + monitor_find_completion); + monitor_read_command(mon, 0); + } + + qemu_chr_fe_set_handlers(&mon->parent_obj.chr, + monitor_can_read, + monitor_read, + monitor_event, NULL, + &mon->parent_obj, NULL, true); + monitor_list_append(&mon->parent_obj); } +} - qemu_chr_fe_set_handlers(&mon->parent_obj.chr, - monitor_can_read, monitor_read, monitor_event, - NULL, &mon->parent_obj, NULL, true); - monitor_list_append(&mon->parent_obj); +static bool monitor_hmp_prepare_delete(UserCreatable *uc, Error **errp) +{ + error_setg(errp, "Deleting HMP monitors is not supported"); + return false; } /** diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index fe5703af6d..012d442a20 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -204,7 +204,6 @@ extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands; extern QemuMutex monitor_lock; extern MonitorList mon_list; -void monitor_complete(Monitor *mon, Error **errp); bool monitor_requires_iothread(const Monitor *mon); int monitor_can_read(void *opaque); void monitor_list_append(Monitor *mon); diff --git a/monitor/monitor.c b/monitor/monitor.c index c401e23b7b..9257dae0be 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -29,6 +29,7 @@ #include "qapi/qapi-emit-events.h" #include "qapi/qapi-visit-control.h" #include "qobject/qdict.h" +#include "qom/object_interfaces.h" #include "qemu/error-report.h" #include "qemu/option.h" #include "system/qtest.h" @@ -73,7 +74,8 @@ static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */ MonitorList mon_list; static bool monitor_destroyed; -OBJECT_DEFINE_TYPE(Monitor, monitor, MONITOR, OBJECT); +OBJECT_DEFINE_TYPE_EXTENDED(Monitor, monitor, MONITOR, OBJECT, true, + { TYPE_USER_CREATABLE }, {}); static void monitor_finalize(Object *obj) { @@ -100,11 +102,17 @@ static void monitor_set_chardev_id(Object *obj, const char *str, Error **errp) mon->chardev_id = g_strdup(str); } +static void monitor_complete(UserCreatable *uc, Error **errp); + static void monitor_class_init(ObjectClass *cls, const void *data) { + UserCreatableClass *ucc = USER_CREATABLE_CLASS(cls); + object_class_property_add_str(cls, "chardev", monitor_get_chardev_id, monitor_set_chardev_id); + + ucc->complete = monitor_complete; } static void monitor_init(Object *obj) @@ -679,8 +687,10 @@ void monitor_init_globals(void) aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); } -void monitor_complete(Monitor *mon, Error **errp) +static void monitor_complete(UserCreatable *uc, Error **errp) { + Monitor *mon = MONITOR(uc); + if (mon->chardev_id) { Chardev *chr = qemu_chr_find(mon->chardev_id); if (chr == NULL) { diff --git a/monitor/qmp.c b/monitor/qmp.c index 5cf38c6273..ff39b8cda4 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -31,6 +31,7 @@ #include "qobject/qdict.h" #include "qobject/qjson.h" #include "qobject/qlist.h" +#include "qom/object_interfaces.h" #include "trace.h" /* @@ -101,10 +102,13 @@ static void monitor_qmp_set_pretty(Object *obj, bool val, Error **errp) static void monitor_qmp_emit_event(Monitor *mon, QAPIEvent event, QDict *qdict); static bool monitor_qmp_requires_iothread(const Monitor *mon); +static void monitor_qmp_complete(UserCreatable *uc, Error **errp); +static bool monitor_qmp_prepare_delete(UserCreatable *uc, Error **errp); static void monitor_qmp_class_init(ObjectClass *cls, const void *data) { MonitorClass *moncls = MONITOR_CLASS(cls); + UserCreatableClass *ucc = USER_CREATABLE_CLASS(cls); object_class_property_add_bool(cls, "pretty", monitor_qmp_get_pretty, @@ -112,6 +116,9 @@ static void monitor_qmp_class_init(ObjectClass *cls, const void *data) moncls->emit_event = monitor_qmp_emit_event; moncls->requires_iothread = monitor_qmp_requires_iothread; + + ucc->complete = monitor_qmp_complete; + ucc->prepare_delete = monitor_qmp_prepare_delete; } static void handle_qmp_command(void *opaque, QObject *req, Error *err); @@ -582,25 +589,28 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) { - MonitorQMP *mon; static int counter; g_autofree char *id = g_strdup_printf("qmpcompat%d", counter++); - Object *obj = object_new_with_props(TYPE_MONITOR_QMP, - object_get_objects_root(), - id, - errp, - "chardev", chardev_id, - "pretty", pretty ? "yes" : "no", - NULL); - - if (!obj) { - return; - } + object_new_with_props(TYPE_MONITOR_QMP, + object_get_objects_root(), + id, + errp, + "chardev", chardev_id, + "pretty", pretty ? "yes" : "no", + NULL); +} - mon = MONITOR_QMP(obj); - monitor_complete(MONITOR(mon), errp); +static void monitor_qmp_complete(UserCreatable *uc, Error **errp) +{ + MonitorQMP *mon = MONITOR_QMP(uc); + UserCreatableClass *ucc_parent = + USER_CREATABLE_CLASS( + object_class_get_parent( + OBJECT_CLASS(MONITOR_QMP_GET_CLASS(mon)))); + ERRP_GUARD(); + + ucc_parent->complete(uc, errp); if (*errp) { - object_unparent(OBJECT(mon)); return; } @@ -632,3 +642,9 @@ void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) monitor_list_append(&mon->parent_obj); } } + +static bool monitor_qmp_prepare_delete(UserCreatable *uc, Error **errp) +{ + error_setg(errp, "Deleting QMP monitors is not supported"); + return false; +} diff --git a/qapi/qom.json b/qapi/qom.json index dd45ac1087..6ed510858e 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -1187,6 +1187,45 @@ 'data': { '*cpu-affinity': ['uint16'], '*node-affinity': ['uint16'] } } +## +# @MonitorProperties: +# +# Properties for all monitors +# +# @chardev: ID of the character device providing the monitor transport +# +# Since: 11.1 +## +{ 'struct': 'MonitorProperties', + 'data': { 'chardev': 'str' }} + +## +# @MonitorHMPProperties: +# +# Properties for the HMP monitor +# +# @readline: whether to enable readline for interactive command +# usage (default: enabled) +# +# Since: 11.1 +## +{ 'struct': 'MonitorHMPProperties', + 'base': 'MonitorProperties', + 'data': { '*readline': 'bool' } } + +## +# @MonitorQMPProperties: +# +# Properties for the QMP monitor +# +# @pretty: whether to pretty print JSON responses (default: disabled) +# +# Since: 11.1 +## +{ 'struct': 'MonitorQMPProperties', + 'base': 'MonitorProperties', + 'data': { '*pretty': 'bool' } } + ## # @ObjectType: # @@ -1237,6 +1276,8 @@ 'memory-backend-ram', { 'name': 'memory-backend-shm', 'if': 'CONFIG_POSIX' }, + 'monitor-hmp', + 'monitor-qmp', 'pef-guest', { 'name': 'pr-manager-helper', 'if': 'CONFIG_LINUX' }, @@ -1315,6 +1356,8 @@ 'memory-backend-ram': 'MemoryBackendProperties', 'memory-backend-shm': { 'type': 'MemoryBackendShmProperties', 'if': 'CONFIG_POSIX' }, + 'monitor-hmp': {'type': 'MonitorHMPProperties' }, + 'monitor-qmp': {'type': 'MonitorQMPProperties' }, 'pr-manager-helper': { 'type': 'PrManagerHelperProperties', 'if': 'CONFIG_LINUX' }, 'qtest': 'QtestProperties', diff --git a/stubs/monitor-internal.c b/stubs/monitor-internal.c index 51db7588b9..59ccc4b35c 100644 --- a/stubs/monitor-internal.c +++ b/stubs/monitor-internal.c @@ -10,4 +10,5 @@ int monitor_get_fd(Monitor *mon, const char *name, Error **errp) void monitor_new_hmp(const char *chardev_id, bool use_readline, Error **errp) { + g_assert_not_reached(); } diff --git a/system/vl.c b/system/vl.c index 21ef2dac51..86f09a8b5c 100644 --- a/system/vl.c +++ b/system/vl.c @@ -1829,6 +1829,10 @@ static void object_option_add_visitor(Visitor *v) { ObjectOption *opt = g_new0(ObjectOption, 1); visit_type_ObjectOptions(v, NULL, &opt->opts, &error_fatal); + if (opt->opts->qom_type == OBJECT_TYPE_MONITOR_HMP || + opt->opts->qom_type == OBJECT_TYPE_MONITOR_QMP) { + default_monitor = 0; + } QTAILQ_INSERT_TAIL(&object_opts, opt, next); } @@ -1970,7 +1974,9 @@ static bool object_create_early(const char *type) /* Reason: property "chardev" */ if (g_str_equal(type, "rng-egd") || - g_str_equal(type, "qtest")) { + g_str_equal(type, "qtest") || + g_str_equal(type, "monitor-hmp") || + g_str_equal(type, "monitor-qmp")) { return false; } -- 2.54.0

Add new docs for the `-object monitor-hmp` and `-object monitor-qmp` options, updating `-mon` to state that it is legacy syntax sugar for the new `-object` args. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- qemu-options.hx | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/qemu-options.hx b/qemu-options.hx index 96ae41f787..031417b79d 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4959,6 +4959,16 @@ SRST -mon chardev=mon1,mode=control,pretty=on enables the QMP monitor on localhost port 4444 with pretty-printing. + + The use of ``-mon mode=readline`` is historical syntax sugar + for the new ``-object monitor-hmp`` option, each use of which + creates an object with the ID ``hmpcompatNNN`` where ``NNN`` is + a counter starting from 0. + + The use of ``-mon mode=control`` is historical syntax sugar + for the new ``-object monitor-qmp`` option, each use of which + creates an object with the ID ``qmpcompatNNN`` where ``NNN`` is + a counter starting from 0. ERST DEF("debugcon", HAS_ARG, QEMU_OPTION_debugcon, \ @@ -5709,6 +5719,34 @@ SRST they are specified. Note that the 'id' property must be set. These objects are placed in the '/objects' path. + ``-object monitor-hmp,id=id,chardev=chardv_id,readline=on|off`` + Set up a monitor running the Human Monitor Protocol, + connected to the chardev ``chrid``. + + The ``id`` parameter is a unique ID that can be used + to dynamically delete the monitor at runtime. + + The ``readline`` parameter, which defaults to ``on``, + controls whether the monitor provides interactive + prompts + + ``-object monitor-qmp,id=id,chardev=chardev_id,pretty=on|off`` + Set up a monitor running the QEMU Monitor Protocol, + connected to the chardev ``chrid``. + + The ``id`` parameter is a unique ID that can be used + to dynamically delete the monitor at runtime. Note + that monitors created using the historical syntax + will be allocated IDs following the pattern ``monNN``. + Mixing ``-object`` with the historical monitor syntax is + discouraged. + + The ``pretty`` parameter, which defaults to ``off``, + controls whether the monitor responses are pretty + printed as multi-line indented JSON, as opposed to + constrained to a single line without extraneous + whitespace. + ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,offset=offset,readonly=on|off,rom=on|off|auto`` Creates a memory file backend object, which can be used to back the guest RAM with huge pages. -- 2.54.0

From: Christian Brauner <brauner@kernel.org> Test the object-add/object-del QMP commands with the monitor-qmp object type. - Basic lifecycle: chardev-add -> object-add -> object-del -> chardev-remove - Error: object-add with nonexistent chardev - Error: second monitor on same chardev (chardev already in use) - Removal of CLI-created QMP monitor succeeds - Error: object-remove on HMP monitor - Re-add after remove: same id and chardev reusable after removal Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org> [DB: modified to use object-add/object-del, removing redundant scenarios already handled by object-add/del code] Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/qtest/qmp-test.c | 174 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/tests/qtest/qmp-test.c b/tests/qtest/qmp-test.c index edf0886787..3a325a04e0 100644 --- a/tests/qtest/qmp-test.c +++ b/tests/qtest/qmp-test.c @@ -337,6 +337,174 @@ static void test_qmp_missing_any_arg(void) qtest_quit(qts); } +static void test_qmp_monitor_add_remove(void) +{ + QTestState *qts; + QDict *resp; + + qts = qtest_init(common_args); + + /* Create a null chardev for the dynamic monitor */ + resp = qtest_qmp(qts, + "{'execute': 'chardev-add'," + " 'arguments': {'id': 'monitor-chardev'," + " 'backend': {'type': 'null'," + " 'data': {}}}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + /* Add a dynamic monitor */ + resp = qtest_qmp(qts, + "{'execute': 'object-add'," + " 'arguments': {'qom-type': 'monitor-qmp'," + " 'id': 'dyn-mon'," + " 'chardev': 'monitor-chardev'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + /* Remove the dynamic monitor */ + resp = qtest_qmp(qts, + "{'execute': 'object-del'," + " 'arguments': {'id': 'dyn-mon'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + /* Add again after remove -- same id and chardev should work */ + resp = qtest_qmp(qts, + "{'execute': 'object-add'," + " 'arguments': {'qom-type': 'monitor-qmp'," + " 'id': 'dyn-mon'," + " 'chardev': 'monitor-chardev'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + /* Clean up */ + resp = qtest_qmp(qts, + "{'execute': 'object-del'," + " 'arguments': {'id': 'dyn-mon'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + resp = qtest_qmp(qts, + "{'execute': 'chardev-remove'," + " 'arguments': {'id': 'monitor-chardev'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + qtest_quit(qts); +} + +static void test_qmp_monitor_error_paths(void) +{ + QTestState *qts; + QDict *resp; + + qts = qtest_init(common_args); + + /* Error: chardev does not exist */ + resp = qtest_qmp(qts, + "{'execute': 'object-add'," + " 'arguments': {'qom-type': 'monitor-qmp'," + " 'id': 'bad-mon'," + " 'chardev': 'nonexistent'}}"); + qmp_expect_error_and_unref(resp, "GenericError"); + + qtest_quit(qts); +} + +static void test_qmp_monitor_chardev_in_use(void) +{ + QTestState *qts; + QDict *resp; + + qts = qtest_init(common_args); + + /* Create a null chardev */ + resp = qtest_qmp(qts, + "{'execute': 'chardev-add'," + " 'arguments': {'id': 'shared-chr'," + " 'backend': {'type': 'null'," + " 'data': {}}}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + /* Attach first monitor */ + resp = qtest_qmp(qts, + "{'execute': 'object-add'," + " 'arguments': {'qom-type': 'monitor-qmp'," + " 'id': 'mon-1'," + " 'chardev': 'shared-chr'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + /* Error: second monitor on the same chardev */ + resp = qtest_qmp(qts, + "{'execute': 'object-add'," + " 'arguments': {'qom-type': 'monitor-qmp'," + " 'id': 'mon-2'," + " 'chardev': 'shared-chr'}}"); + qmp_expect_error_and_unref(resp, "GenericError"); + + /* Clean up */ + resp = qtest_qmp(qts, + "{'execute': 'object-del'," + " 'arguments': {'id': 'mon-1'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + resp = qtest_qmp(qts, + "{'execute': 'chardev-remove'," + " 'arguments': {'id': 'shared-chr'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + qtest_quit(qts); +} + +static void test_qmp_monitor_remove_cli(void) +{ + QTestState *qts; + QDict *resp; + + /* Launch with a named CLI monitor on a null chardev */ + qts = qtest_initf("%s -chardev null,id=cli-chr" + " -object monitor-qmp,id=cli-mon,chardev=cli-chr", + common_args); + + /* CLI-created QMP monitors can be removed */ + resp = qtest_qmp(qts, + "{'execute': 'object-del'," + " 'arguments': {'id': 'cli-mon'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + resp = qtest_qmp(qts, + "{'execute': 'chardev-remove'," + " 'arguments': {'id': 'cli-chr'}}"); + g_assert(qdict_haskey(resp, "return")); + qobject_unref(resp); + + qtest_quit(qts); +} + +static void test_qmp_monitor_remove_hmp(void) +{ + QTestState *qts; + QDict *resp; + + qts = qtest_initf("%s -chardev null,id=hmp-chr" + " -object monitor-hmp,id=hmp-mon,chardev=hmp-chr", + common_args); + + /* Error: object_del must reject HMP monitors */ + resp = qtest_qmp(qts, + "{'execute': 'object-del'," + " 'arguments': {'id': 'hmp-mon'}}"); + qmp_expect_error_and_unref(resp, "GenericError"); + + qtest_quit(qts); +} + int main(int argc, char *argv[]) { g_test_init(&argc, &argv, NULL); @@ -348,6 +516,12 @@ int main(int argc, char *argv[]) #endif qtest_add_func("qmp/preconfig", test_qmp_preconfig); qtest_add_func("qmp/missing-any-arg", test_qmp_missing_any_arg); + qtest_add_func("qmp/monitor-add-remove", test_qmp_monitor_add_remove); + qtest_add_func("qmp/monitor-error-paths", test_qmp_monitor_error_paths); + qtest_add_func("qmp/monitor-chardev-in-use", + test_qmp_monitor_chardev_in_use); + qtest_add_func("qmp/monitor-remove-cli", test_qmp_monitor_remove_cli); + qtest_add_func("qmp/monitor-remove-hmp", test_qmp_monitor_remove_hmp); return g_test_run(); } -- 2.54.0

From: Christian Brauner <brauner@kernel.org> Add functional tests that exercise dynamic monitor hotplug with real socket connections: - Hotplug cycle: chardev-add a unix socket, object-add, connect to the socket, receive the QMP greeting, negotiate capabilities, send query-version, disconnect, remove the monitor and chardev, then repeat the entire cycle a second time to verify cleanup and reuse. - Self-removal: a dynamically-added monitor sends object-del targeting itself, verifying that the request is rejected - Large response: send query-qmp-schema on a dynamic monitor to exercise the output buffer flush path with a large response payload. - Events after negotiation: trigger STOP/RESUME events via the main monitor and verify they are delivered on the dynamic monitor. This complements the qtest unit tests by verifying that a real QMP client can connect to a dynamically-added monitor and exchange messages. Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org> [DB: modified to use object-add/object-del; adjust self-removal test to validate rejection of request] Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- MAINTAINERS | 1 + tests/functional/generic/meson.build | 1 + .../generic/test_monitor_hotplug.py | 168 ++++++++++++++++++ 3 files changed, 170 insertions(+) create mode 100755 tests/functional/generic/test_monitor_hotplug.py diff --git a/MAINTAINERS b/MAINTAINERS index 748ec77beb..86bb043674 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3615,6 +3615,7 @@ F: docs/interop/*qmp-* F: scripts/qmp/ F: tests/qtest/qmp-test.c F: tests/qtest/qmp-cmd-test.c +F: tests/functional/generic/test_monitor_hotplug.py T: git https://repo.or.cz/qemu/armbru.git qapi-next qtest diff --git a/tests/functional/generic/meson.build b/tests/functional/generic/meson.build index 09763c5d22..c94105c62e 100644 --- a/tests/functional/generic/meson.build +++ b/tests/functional/generic/meson.build @@ -4,6 +4,7 @@ tests_generic_system = [ 'empty_cpu_model', 'info_usernet', 'linters', + 'monitor_hotplug', 'version', 'vnc', ] diff --git a/tests/functional/generic/test_monitor_hotplug.py b/tests/functional/generic/test_monitor_hotplug.py new file mode 100755 index 0000000000..5d8a159eb0 --- /dev/null +++ b/tests/functional/generic/test_monitor_hotplug.py @@ -0,0 +1,168 @@ +#!/usr/bin/env python3 +# +# SPDX-License-Identifier: GPL-2.0-or-later +# +# Functional test for dynamic QMP monitor hotplug +# +# Copyright (c) 2026 Christian Brauner + +import os + +from qemu_test import QemuSystemTest + +from qemu.qmp.legacy import QEMUMonitorProtocol + + +class MonitorHotplug(QemuSystemTest): + + def setUp(self): + super().setUp() + sock_dir = self.socket_dir() + self._sock_path = os.path.join(sock_dir.name, 'hotplug.sock') + + def _add_monitor(self): + """Create a chardev + monitor and return the socket path.""" + sock = self._sock_path + self.vm.cmd('chardev-add', id='hotplug-chr', backend={ + 'type': 'socket', + 'data': { + 'addr': { + 'type': 'unix', + 'data': {'path': sock} + }, + 'server': True, + 'wait': False, + } + }) + self.vm.cmd('object-add', id='hotplug-mon', + qom_type='monitor-qmp', + chardev='hotplug-chr') + return sock + + def _remove_monitor(self): + """Remove the monitor + chardev.""" + self.vm.cmd('object-del', id='hotplug-mon') + self.vm.cmd('chardev-remove', id='hotplug-chr') + + def _connect_and_handshake(self, sock_path): + """ + Connect to the dynamic monitor socket, perform the QMP + greeting and capability negotiation, send a command, then + disconnect. + """ + qmp = QEMUMonitorProtocol(sock_path) + + # connect(negotiate=True) receives the greeting, validates it, + # and sends qmp_capabilities automatically. + greeting = qmp.connect(negotiate=True) + self.assertIn('QMP', greeting) + self.assertIn('version', greeting['QMP']) + self.assertIn('capabilities', greeting['QMP']) + + # Send a real command to prove the session is fully functional + resp = qmp.cmd_obj({'execute': 'query-version'}) + self.assertIn('return', resp) + self.assertIn('qemu', resp['return']) + + qmp.close() + + def test_hotplug_cycle(self): + """ + Hotplug a monitor, do the full QMP handshake, unplug it, + then repeat the whole cycle a second time. + """ + self.set_machine('none') + self.vm.add_args('-nodefaults') + self.vm.launch() + + # First cycle + sock = self._add_monitor() + self._connect_and_handshake(sock) + self._remove_monitor() + + # Second cycle -- same ids, same path, must work + sock = self._add_monitor() + self._connect_and_handshake(sock) + self._remove_monitor() + + def test_self_removal(self): + """ + A dynamically-added monitor sends object-del targeting + itself. Verify the request is rejected, but the monitor + can still be deleted from outside its own context. + """ + self.set_machine('none') + self.vm.add_args('-nodefaults') + self.vm.launch() + + sock = self._add_monitor() + + qmp = QEMUMonitorProtocol(sock) + greeting = qmp.connect(negotiate=True) + self.assertIn('QMP', greeting) + + # Self-removal: the dynamic monitor raises error + resp = qmp.cmd_obj({'execute': 'object-del', + 'arguments': {'id': 'hotplug-mon'}}) + self.assertIn('error', resp) + + qmp.close() + + resp = self.vm.cmd('object-del', id='hotplug-mon') + + # Clean up the chardev + self.vm.cmd('chardev-remove', id='hotplug-chr') + + def test_large_response(self): + """ + Send a command with a large response (query-qmp-schema) on a + dynamically-added monitor to exercise the output buffer flush + path. + """ + self.set_machine('none') + self.vm.add_args('-nodefaults') + self.vm.launch() + + sock = self._add_monitor() + + qmp = QEMUMonitorProtocol(sock) + qmp.connect(negotiate=True) + + resp = qmp.cmd_obj({'execute': 'query-qmp-schema'}) + self.assertIn('return', resp) + self.assertIsInstance(resp['return'], list) + self.assertGreater(len(resp['return']), 0) + + qmp.close() + self._remove_monitor() + + def test_events_after_negotiation(self): + """ + Verify that QMP events are delivered on a dynamically-added + monitor after capability negotiation completes. + """ + self.set_machine('none') + self.vm.add_args('-nodefaults') + self.vm.launch() + + sock = self._add_monitor() + + qmp = QEMUMonitorProtocol(sock) + qmp.connect(negotiate=True) + + # Trigger a STOP event via the main monitor, then read it + # from the dynamic monitor. + self.vm.cmd('stop') + resp = qmp.pull_event(wait=True) + self.assertEqual(resp['event'], 'STOP') + + self.vm.cmd('cont') + resp = qmp.pull_event(wait=True) + self.assertEqual(resp['event'], 'RESUME') + + qmp.close() + self._remove_monitor() + + +if __name__ == '__main__': + QemuSystemTest.main() -- 2.54.0

When unplugging a monitor there is a careful synchronization dance between the monitor handling the "object-del" command and the command processing for the monitor being deleted. The stress test runs a busy loop of 'query-qmp-schema' on a second monitor, while the primary monitor requests its deletion. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .../generic/test_monitor_hotplug.py | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/functional/generic/test_monitor_hotplug.py b/tests/functional/generic/test_monitor_hotplug.py index 5d8a159eb0..03087faafc 100755 --- a/tests/functional/generic/test_monitor_hotplug.py +++ b/tests/functional/generic/test_monitor_hotplug.py @@ -6,11 +6,16 @@ # # Copyright (c) 2026 Christian Brauner +import asyncio import os +import random +import threading +import time from qemu_test import QemuSystemTest from qemu.qmp.legacy import QEMUMonitorProtocol +from qemu.qmp import QMPClient class MonitorHotplug(QemuSystemTest): @@ -163,6 +168,63 @@ def test_events_after_negotiation(self): qmp.close() self._remove_monitor() + def stress_mon(self, sock): + async def main(): + qmp = QMPClient('testvm') + await qmp.connect(sock) + # Run query-version in a tight loop so that the + # monitor thread/dispatcher is very busy at the + # time we try to delete the monitor + while True: + try: + # A command which returns alot of data to make + # it more likely we're in the I/O reply path + # when deleting the monitor + res = await qmp.execute('query-qmp-schema') + # Some commands which generate async events + # as those can trigger different code paths + res = await qmp.execute('stop') + res = await qmp.execute('cont') + except: + # we'll get here if the monitor is terminated + # by QEMU in which case we must disconnect + # out side, but.... + try: + await qmp.disconnect() + except (ConnectionResetError, EOFError, BrokenPipeError): + # ... disconnect() will probably see + # errors too, but we must try to call it + # regardless to cleanup asyncio state + # and prevent python warnings at GC time + pass + return + asyncio.run(main()) + + def test_hotplug_stress(self): + """ + Repeatedly hotplug and unplug a monitor, while another thread + concurrently issues commands on that monitor. This stresses + the synchronization with the monitor thread during cleanup + """ + self.set_machine('none') + self.vm.add_args('-nodefaults') + self.vm.launch() + + # Each loop sleeps at most 0.5 seconds, so this should + # give an upper bound of approx 5 seconds execution + # time which is reasonable to run by default + repeat = 10 + for i in range(repeat): + # First cycle + sock = self._add_monitor() + print ("# stress cycle %02d/%02d" % (i, repeat)) + stress = threading.Thread(target=self.stress_mon, args=[sock]) + stress.start() + # Sleep upto 1/2 second to vary the races + time.sleep(random.random() / 0.5) + self._remove_monitor() + stress.join() + if __name__ == '__main__': QemuSystemTest.main() -- 2.54.0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- qom/object_interfaces.c | 6 ++++++ qom/trace-events | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index 6faa0b2fd9..2e618a7623 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -20,12 +20,14 @@ #include "qapi/opts-visitor.h" #include "qemu/config-file.h" #include "qemu/keyval.h" +#include "trace.h" bool user_creatable_complete(UserCreatable *uc, Error **errp) { UserCreatableClass *ucc = USER_CREATABLE_GET_CLASS(uc); ERRP_GUARD(); + trace_user_creatable_complete(uc, object_get_typename(OBJECT(uc))); if (ucc->complete) { ucc->complete(uc, errp); } @@ -37,9 +39,13 @@ bool user_creatable_prepare_delete(UserCreatable *uc, Error **errp) UserCreatableClass *ucc = USER_CREATABLE_GET_CLASS(uc); ERRP_GUARD(); + trace_user_creatable_prepare_delete(uc, object_get_typename(OBJECT(uc))); if (ucc->prepare_delete) { ucc->prepare_delete(uc, errp); } + trace_user_creatable_prepare_delete_result( + uc, object_get_typename(OBJECT(uc)), + *errp ? error_get_pretty(*errp) : NULL); return !*errp; } diff --git a/qom/trace-events b/qom/trace-events index 44c63e72af..bac8472618 100644 --- a/qom/trace-events +++ b/qom/trace-events @@ -11,3 +11,8 @@ object_property_del_child(void *obj, const char *type, void *child, const char * object_property_parse(void *obj, const char *type, const char *name, const char *value) "obj=%p type=%s prop=%s value=%s" object_class_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "type=%s->%s (%s:%d:%s)" object_class_property_add(const char *type, const char *name, void *value) "type=%s name=%s value=%p" + +# object_interfaces.c +user_creatable_complete(void *obj, const char *type) "obj=%p type=%s" +user_creatable_prepare_delete(void *obj, const char *type) "obj=%p type=%s" +user_creatable_prepare_delete_result(void *obj, const char *type, const char *msg) "obj=%p type=%s msg=%s" -- 2.54.0

The default monitor is usually a long lived object that will exist for the entire lifetime of the VM. A monitor can only service a single client at a time though, and so it might be desirable to hotplug additional monitors at runtime for specific tasks. If doing that, however, there is a need to remove the monitor when it is no longer needed. Allowing a client to run "object-del" against its own monitor adds complex edge cases, as it would be desirable to send the QMP response despite the monitor sending it being deleted. Doing "object-del" alone will also result in orphaning a character device backend instance, as there is no opportunity to run the companion "chardev-del" command. A simpler way to ensure cleanup is to add the concept of auto-deleting monitor objects. Specifically when the "CHR_EVENT_CLOSED" event is emitted, the equivalent of "object-del" + "chardev-del" can be run internally. Since the transient client has already droppped its monitor connection, there is no synchronization to be concerned about. This is implemented via a new "close-action=none|delete" property on the 'monitor-qmp' object. This concept could be extended with further actions in future, for example: * close-action=shutdown - graceful guest shutdown * close-action=terminate - immediate guest poweroff * close-action=stop - pause guest CPUs while the monitor is not connected to any client This is left as an exercise for future interested contributors. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/monitor-internal.h | 3 + monitor/qmp.c | 59 +++++++++++++++++++ qapi/qom.json | 21 ++++++- qemu-options.hx | 10 +++- .../generic/test_monitor_hotplug.py | 53 +++++++++++++++-- 5 files changed, 140 insertions(+), 6 deletions(-) diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 2e8e5ec721..165002f535 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -28,6 +28,7 @@ #include "chardev/char-fe.h" #include "monitor/monitor.h" #include "qapi/qapi-types-control.h" +#include "qapi/qapi-types-qom.h" #include "qapi/qmp-registry.h" #include "qobject/json-parser.h" #include "qemu/readline.h" @@ -179,7 +180,9 @@ struct MonitorQMP { Monitor parent_obj; JSONMessageParser parser; bool pretty; + MonitorQMPCloseAction close_action; bool setup_pending; /* iothread BH has not yet set up chardev handlers */ + bool delete_pending; /* close_action has started 'delete' process */ /* * When a client connects, we're in capabilities negotiation mode. * @commands is &qmp_cap_negotiation_commands then. When command diff --git a/monitor/qmp.c b/monitor/qmp.c index d471428488..905f924ea6 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -28,6 +28,7 @@ #include "monitor-internal.h" #include "qapi/error.h" #include "qapi/qapi-commands-control.h" +#include "qapi/qapi-commands-char.h" #include "qobject/qdict.h" #include "qobject/qjson.h" #include "qobject/qlist.h" @@ -103,6 +104,20 @@ static void monitor_qmp_set_pretty(Object *obj, bool val, Error **errp) mon->pretty = val; } +static int monitor_qmp_get_close_action(Object *obj, Error **errp) +{ + MonitorQMP *mon = MONITOR_QMP(obj); + + return mon->close_action; +} + +static void monitor_qmp_set_close_action(Object *obj, int val, Error **errp) +{ + MonitorQMP *mon = MONITOR_QMP(obj); + + mon->close_action = val; +} + static void monitor_qmp_emit_event(Monitor *mon, QAPIEvent event, QDict *qdict); static bool monitor_qmp_requires_iothread(const Monitor *mon); static void monitor_qmp_complete(UserCreatable *uc, Error **errp); @@ -117,6 +132,11 @@ static void monitor_qmp_class_init(ObjectClass *cls, const void *data) object_class_property_add_bool(cls, "pretty", monitor_qmp_get_pretty, monitor_qmp_set_pretty); + object_class_property_add_enum(cls, "close-action", + "MonitorQMPCloseAction", + &MonitorQMPCloseAction_lookup, + monitor_qmp_get_close_action, + monitor_qmp_set_close_action); moncls->emit_event = monitor_qmp_emit_event; moncls->requires_iothread = monitor_qmp_requires_iothread; @@ -550,11 +570,33 @@ static QDict *qmp_greeting(MonitorQMP *mon) ver, cap_list); } +static void monitor_qmp_self_delete_bh(void *opaque) +{ + MonitorQMP *mon = opaque; + g_autofree char *mon_id = object_property_get_child_name( + object_get_objects_root(), OBJECT(mon)); + g_autofree char *chardev_id = g_strdup(mon->parent_obj.chardev_id); + Error *local_error = NULL; + + g_assert(mon_id); + + user_creatable_del(mon_id, &local_error); + if (local_error != NULL) { + error_report_err(local_error); + } else { + qmp_chardev_remove(chardev_id, NULL); + } +} + static void monitor_qmp_event(void *opaque, QEMUChrEvent event) { QDict *data; MonitorQMP *mon = opaque; + if (mon->delete_pending) { + return; + } + switch (event) { case CHR_EVENT_OPENED: WITH_QEMU_LOCK_GUARD(&mon->parent_obj.mon_lock) { @@ -577,6 +619,23 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent event) json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL); monitor_fdsets_cleanup(); + switch (mon->close_action) { + case MONITOR_QMP_CLOSE_ACTION_NONE: + break; /* nada */ + case MONITOR_QMP_CLOSE_ACTION_DELETE: + mon->delete_pending = true; + /* + * Do NOT run in the AIO context associated with the + * monitor. We need to run in the default AIO context + * which is the same context in which 'qmp_object_del' + * will execute + */ + aio_bh_schedule_oneshot(qemu_get_aio_context(), + monitor_qmp_self_delete_bh, mon); + break; + default: + g_assert_not_reached(); + } break; case CHR_EVENT_BREAK: case CHR_EVENT_MUX_IN: diff --git a/qapi/qom.json b/qapi/qom.json index 6ed510858e..63335b8fd0 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -1213,18 +1213,37 @@ 'base': 'MonitorProperties', 'data': { '*readline': 'bool' } } + +## +# @MonitorQMPCloseAction: +# +# Action to take when the character device backend is +# closed. +# +# @none: take no action (the default) +# @delete: delete both the 'monitor-qmp' object and its associated +# character device backend object +# +# Since 11.1 +## +{ 'enum' : 'MonitorQMPCloseAction', + 'data': ['none', 'delete'] } + ## # @MonitorQMPProperties: # # Properties for the QMP monitor # # @pretty: whether to pretty print JSON responses (default: disabled) +# @close-action: action to take when the character device backend +# is closed (default: none) # # Since: 11.1 ## { 'struct': 'MonitorQMPProperties', 'base': 'MonitorProperties', - 'data': { '*pretty': 'bool' } } + 'data': { '*pretty': 'bool', + '*close-action': 'MonitorQMPCloseAction' } } ## # @ObjectType: diff --git a/qemu-options.hx b/qemu-options.hx index 031417b79d..848a53ea34 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -5730,7 +5730,7 @@ SRST controls whether the monitor provides interactive prompts - ``-object monitor-qmp,id=id,chardev=chardev_id,pretty=on|off`` + ``-object monitor-qmp,id=id,chardev=chardev_id,pretty=on|off,close-action=none|delete`` Set up a monitor running the QEMU Monitor Protocol, connected to the chardev ``chrid``. @@ -5747,6 +5747,14 @@ SRST constrained to a single line without extraneous whitespace. + The ``close-action`` parameter, which defaults to ``none``, + controls what happens when the connection to the monitor + is terminated by the user. If set to ``delete``, then the + ``monitor-qmp`` object and its associated character + device are both immediately deleted. This can be useful + if an extra monitor was hotplugged for a specific task + and should be unplugged when completed. + ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,offset=offset,readonly=on|off,rom=on|off|auto`` Creates a memory file backend object, which can be used to back the guest RAM with huge pages. diff --git a/tests/functional/generic/test_monitor_hotplug.py b/tests/functional/generic/test_monitor_hotplug.py index 03087faafc..f7d72a77c2 100755 --- a/tests/functional/generic/test_monitor_hotplug.py +++ b/tests/functional/generic/test_monitor_hotplug.py @@ -25,7 +25,7 @@ def setUp(self): sock_dir = self.socket_dir() self._sock_path = os.path.join(sock_dir.name, 'hotplug.sock') - def _add_monitor(self): + def _add_monitor(self, autodelete=False): """Create a chardev + monitor and return the socket path.""" sock = self._sock_path self.vm.cmd('chardev-add', id='hotplug-chr', backend={ @@ -39,9 +39,15 @@ def _add_monitor(self): 'wait': False, } }) - self.vm.cmd('object-add', id='hotplug-mon', - qom_type='monitor-qmp', - chardev='hotplug-chr') + if autodelete: + self.vm.cmd('object-add', id='hotplug-mon', + qom_type='monitor-qmp', + chardev='hotplug-chr', + close_action='delete') + else: + self.vm.cmd('object-add', id='hotplug-mon', + qom_type='monitor-qmp', + chardev='hotplug-chr') return sock def _remove_monitor(self): @@ -118,6 +124,45 @@ def test_self_removal(self): # Clean up the chardev self.vm.cmd('chardev-remove', id='hotplug-chr') + def test_auto_delete(self): + """ + A dynamically-added monitor is configured with 'close-action=delete' + should see itself deleted when the client is closed. + """ + self.set_machine('none') + self.vm.add_args('-nodefaults') + self.vm.launch() + + sock = self._add_monitor(autodelete=True) + + cdevs = [c["label"] for c in self.vm.cmd('query-chardev')] + objs = [o["name"] for o in self.vm.cmd('qom-list', path='/objects')] + assert ('hotplug-chr' in cdevs) + assert ('hotplug-mon' in objs) + + qmp = QEMUMonitorProtocol(sock) + greeting = qmp.connect(negotiate=True) + self.assertIn('QMP', greeting) + + cdevs = [c["label"] for c in self.vm.cmd('query-chardev')] + objs = [o["name"] for o in self.vm.cmd('qom-list', path='/objects')] + assert ('hotplug-chr' in cdevs) + assert ('hotplug-mon' in objs) + + qmp.close() + + for i in range(10): + cdevs = [c["label"] for c in self.vm.cmd('query-chardev')] + if 'hotplug-chr' not in cdevs: + break + # Sleep upto 1/2 second to vary the races + time.sleep(random.random() / 0.5) + + cdevs = [c["label"] for c in self.vm.cmd('query-chardev')] + objs = [o["name"] for o in self.vm.cmd('qom-list', path='/objects')] + assert ('hotplug-chr' not in cdevs) + assert ('hotplug-mon' not in objs) + def test_large_response(self): """ Send a command with a large response (query-qmp-schema) on a -- 2.54.0