The field name 'parent_obj' is standard practice for QOM structs so align the HMP monitor. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/hmp-cmds.c | 2 +- monitor/hmp.c | 43 ++++++++++++++++++++------------------ monitor/monitor-internal.h | 2 +- monitor/monitor.c | 6 +++--- monitor/qmp-cmds.c | 10 ++++----- ui/ui-hmp-cmds.c | 2 +- 6 files changed, 34 insertions(+), 31 deletions(-) diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 443b8c785d..e139caeba9 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -287,7 +287,7 @@ void hmp_info_sync_profile(Monitor *mon, const QDict *qdict) void hmp_info_history(Monitor *mon, const QDict *qdict) { - MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common); + MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, parent_obj); int i; const char *str; diff --git a/monitor/hmp.c b/monitor/hmp.c index cc4390486e..3dde52ddbe 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -48,9 +48,9 @@ static void monitor_command_cb(void *opaque, const char *cmdline, { MonitorHMP *mon = opaque; - monitor_suspend(&mon->common); + monitor_suspend(&mon->parent_obj); handle_hmp_command(mon, cmdline); - monitor_resume(&mon->common); + monitor_resume(&mon->parent_obj); } void monitor_read_command(MonitorHMP *mon, int show_prompt) @@ -73,7 +73,7 @@ int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func, /* prompt is printed on return from the command handler */ return 0; } else { - monitor_printf(&mon->common, + monitor_printf(&mon->parent_obj, "terminal does not support password prompting\n"); return -ENOTTY; } @@ -695,7 +695,7 @@ static const HMPCommand *monitor_parse_command(MonitorHMP *hmp_mon, const char **cmdp, HMPCommand *table) { - Monitor *mon = &hmp_mon->common; + Monitor *mon = &hmp_mon->parent_obj; const char *p; const HMPCommand *cmd; char cmdname[256]; @@ -1188,35 +1188,37 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline) if (!cmd->cmd && !cmd->cmd_info_hrt) { /* FIXME: is it useful to try autoload modules here ??? */ - monitor_printf(&mon->common, "Command \"%.*s\" is not available.\n", + monitor_printf(&mon->parent_obj, "Command \"%.*s\" is not available.\n", (int)(cmdline - cmd_start), cmd_start); return; } - qdict = monitor_parse_arguments(&mon->common, &cmdline, cmd); + qdict = monitor_parse_arguments(&mon->parent_obj, &cmdline, cmd); if (!qdict) { while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) { cmdline--; } - monitor_printf(&mon->common, "Try \"help %.*s\" for more information\n", + monitor_printf(&mon->parent_obj, + "Try \"help %.*s\" for more information\n", (int)(cmdline - cmd_start), cmd_start); return; } if (!cmd->coroutine) { /* old_mon is non-NULL when called from qmp_human_monitor_command() */ - Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(), &mon->common); - handle_hmp_command_exec(&mon->common, cmd, qdict); + Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(), + &mon->parent_obj); + handle_hmp_command_exec(&mon->parent_obj, cmd, qdict); monitor_set_cur(qemu_coroutine_self(), old_mon); } else { HandleHmpCommandCo data = { - .mon = &mon->common, + .mon = &mon->parent_obj, .cmd = cmd, .qdict = qdict, .done = false, }; Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data); - monitor_set_cur(co, &mon->common); + monitor_set_cur(co, &mon->parent_obj); aio_co_enter(qemu_get_aio_context(), co); AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done); } @@ -1434,7 +1436,7 @@ cleanup: static void monitor_read(void *opaque, const uint8_t *buf, int size) { - MonitorHMP *mon = container_of(opaque, MonitorHMP, common); + MonitorHMP *mon = container_of(opaque, MonitorHMP, parent_obj); int i; if (mon->rs) { @@ -1443,7 +1445,7 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size) } } else { if (size == 0 || buf[size - 1] != 0) { - monitor_printf(&mon->common, "corrupted command\n"); + monitor_printf(&mon->parent_obj, "corrupted command\n"); } else { handle_hmp_command(mon, (char *)buf); } @@ -1512,26 +1514,26 @@ static void G_GNUC_PRINTF(2, 3) monitor_readline_printf(void *opaque, MonitorHMP *mon = opaque; va_list ap; va_start(ap, fmt); - monitor_vprintf(&mon->common, fmt, ap); + monitor_vprintf(&mon->parent_obj, fmt, ap); va_end(ap); } static void monitor_readline_flush(void *opaque) { MonitorHMP *mon = opaque; - monitor_flush(&mon->common); + monitor_flush(&mon->parent_obj); } void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp) { MonitorHMP *mon = g_new0(MonitorHMP, 1); - if (!qemu_chr_fe_init(&mon->common.chr, chr, errp)) { + if (!qemu_chr_fe_init(&mon->parent_obj.chr, chr, errp)) { g_free(mon); return; } - monitor_data_init(&mon->common, false, false, false); + monitor_data_init(&mon->parent_obj, false, false, false); mon->use_readline = use_readline; if (mon->use_readline) { @@ -1542,9 +1544,10 @@ void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp) monitor_read_command(mon, 0); } - qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read, monitor_read, - monitor_event, NULL, &mon->common, NULL, true); - monitor_list_append(&mon->common); + 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); } /** diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index a5c4aba306..eb301ea796 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -128,7 +128,7 @@ struct Monitor { }; struct MonitorHMP { - Monitor common; + Monitor parent_obj; bool use_readline; /* * State used only in the thread "owning" the monitor. diff --git a/monitor/monitor.c b/monitor/monitor.c index 00b93ed612..2a1b511ff4 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -132,7 +132,7 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon) return false; } - return !monitor_uses_readline(container_of(mon, MonitorHMP, common)); + return !monitor_uses_readline(container_of(mon, MonitorHMP, parent_obj)); } static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond, @@ -542,7 +542,7 @@ static void monitor_accept_input(void *opaque) qemu_mutex_lock(&mon->mon_lock); if (!monitor_is_qmp(mon) && mon->reset_seen) { - MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common); + MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, parent_obj); assert(hmp_mon->rs); readline_restart(hmp_mon->rs); qemu_mutex_unlock(&mon->mon_lock); @@ -627,7 +627,7 @@ void monitor_data_destroy(Monitor *mon) if (monitor_is_qmp(mon)) { monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common)); } else { - readline_free(container_of(mon, MonitorHMP, common)->rs); + readline_free(container_of(mon, MonitorHMP, parent_obj)->rs); } g_string_free(mon->outbuf, true); qemu_mutex_destroy(&mon->mon_lock); diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index 0c409c27dc..aa9ee8a391 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -168,10 +168,10 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, char *output = NULL; MonitorHMP hmp = {}; - monitor_data_init(&hmp.common, false, true, false); + monitor_data_init(&hmp.parent_obj, false, true, false); if (has_cpu_index) { - int ret = monitor_set_cpu(&hmp.common, cpu_index); + int ret = monitor_set_cpu(&hmp.parent_obj, cpu_index); if (ret < 0) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index", "a CPU number"); @@ -181,12 +181,12 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, handle_hmp_command(&hmp, command_line); - WITH_QEMU_LOCK_GUARD(&hmp.common.mon_lock) { - output = g_strdup(hmp.common.outbuf->str); + WITH_QEMU_LOCK_GUARD(&hmp.parent_obj.mon_lock) { + output = g_strdup(hmp.parent_obj.outbuf->str); } out: - monitor_data_destroy(&hmp.common); + monitor_data_destroy(&hmp.parent_obj); return output; } diff --git a/ui/ui-hmp-cmds.c b/ui/ui-hmp-cmds.c index ee3e731d07..06f4030ce4 100644 --- a/ui/ui-hmp-cmds.c +++ b/ui/ui-hmp-cmds.c @@ -343,7 +343,7 @@ void hmp_change_vnc(Monitor *mon, const char *device, const char *target, return; } if (!arg) { - MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common); + MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, parent_obj); monitor_read_password(hmp_mon, hmp_change_read_arg, NULL); } else { qmp_change_vnc_password(arg, errp); -- 2.54.0

The current "monitor_init" functions will clash with the methods of the same name that are required by QOM. To ease the transition to QOM, rename them out of the way. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- chardev/char.c | 2 +- gdbstub/system.c | 2 +- include/monitor/monitor.h | 8 ++++---- monitor/hmp.c | 2 +- monitor/monitor.c | 10 +++++----- monitor/qmp.c | 2 +- storage-daemon/qemu-storage-daemon.c | 2 +- stubs/monitor-internal.c | 2 +- system/vl.c | 2 +- 9 files changed, 16 insertions(+), 16 deletions(-) diff --git a/chardev/char.c b/chardev/char.c index ca8b37ed8d..813c04d953 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -805,7 +805,7 @@ static Chardev *qemu_chr_new_from_name(const char *label, const char *filename, if (qemu_opt_get_bool(opts, "mux", 0)) { assert(permit_mux_mon); - monitor_init_hmp(chr, true, &err); + monitor_new_hmp(chr, true, &err); if (err) { error_report_err(err); object_unparent(OBJECT(chr)); diff --git a/gdbstub/system.c b/gdbstub/system.c index 2063b63b2f..18f6a62b2e 100644 --- a/gdbstub/system.c +++ b/gdbstub/system.c @@ -389,7 +389,7 @@ bool gdbserver_start(const char *device, Error **errp) /* Initialize a monitor terminal for gdb */ mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB, NULL, NULL, &error_abort); - monitor_init_hmp(mon_chr, false, &error_abort); + monitor_new_hmp(mon_chr, false, &error_abort); } else { qemu_chr_fe_deinit(&gdbserver_system_state.chr, true); mon_chr = gdbserver_system_state.mon_chr; diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 55649a8664..b9642b58ba 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -19,10 +19,10 @@ bool monitor_cur_is_qmp(void); void monitor_init_globals(void); void monitor_init_globals_core(void); -void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp); -void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp); -int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp); -int monitor_init_opts(QemuOpts *opts, Error **errp); +void monitor_new_qmp(Chardev *chr, bool pretty, Error **errp); +void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp); +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); diff --git a/monitor/hmp.c b/monitor/hmp.c index 3dde52ddbe..4e4468424a 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -1524,7 +1524,7 @@ static void monitor_readline_flush(void *opaque) monitor_flush(&mon->parent_obj); } -void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp) +void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp) { MonitorHMP *mon = g_new0(MonitorHMP, 1); diff --git a/monitor/monitor.c b/monitor/monitor.c index 0b7b464ebe..a87597e606 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -715,7 +715,7 @@ void monitor_init_globals(void) aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); } -int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp) +int monitor_new(MonitorOptions *opts, bool allow_hmp, Error **errp) { ERRP_GUARD(); Chardev *chr; @@ -732,7 +732,7 @@ int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp) switch (opts->mode) { case MONITOR_MODE_CONTROL: - monitor_init_qmp(chr, opts->pretty, errp); + monitor_new_qmp(chr, opts->pretty, errp); break; case MONITOR_MODE_READLINE: if (!allow_hmp) { @@ -743,7 +743,7 @@ int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp) error_setg(errp, "'pretty' is not compatible with HMP monitors"); return -1; } - monitor_init_hmp(chr, true, errp); + monitor_new_hmp(chr, true, errp); break; default: g_assert_not_reached(); @@ -752,7 +752,7 @@ int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp) return *errp ? -1 : 0; } -int monitor_init_opts(QemuOpts *opts, Error **errp) +int monitor_new_opts(QemuOpts *opts, Error **errp) { Visitor *v; MonitorOptions *options; @@ -765,7 +765,7 @@ int monitor_init_opts(QemuOpts *opts, Error **errp) return -1; } - ret = monitor_init(options, true, errp); + ret = monitor_new(options, true, errp); qapi_free_MonitorOptions(options); return ret; } diff --git a/monitor/qmp.c b/monitor/qmp.c index 27934206db..cb28a95efd 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -513,7 +513,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) monitor_list_append(&mon->parent_obj); } -void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp) +void monitor_new_qmp(Chardev *chr, bool pretty, Error **errp) { MonitorQMP *mon = g_new0(MonitorQMP, 1); diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c index eb72561358..50dbfbd97a 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -330,7 +330,7 @@ static void process_options(int argc, char *argv[], bool pre_init_pass) visit_free(v); /* TODO Catch duplicate monitor IDs */ - monitor_init(monitor, false, &error_fatal); + monitor_new(monitor, false, &error_fatal); qapi_free_MonitorOptions(monitor); break; } diff --git a/stubs/monitor-internal.c b/stubs/monitor-internal.c index 4fece49d53..23d58da184 100644 --- a/stubs/monitor-internal.c +++ b/stubs/monitor-internal.c @@ -8,6 +8,6 @@ int monitor_get_fd(Monitor *mon, const char *name, Error **errp) return -1; } -void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp) +void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp) { } diff --git a/system/vl.c b/system/vl.c index 1c0da7df29..fb36140aac 100644 --- a/system/vl.c +++ b/system/vl.c @@ -1247,7 +1247,7 @@ static int fsdev_init_func(void *opaque, QemuOpts *opts, Error **errp) static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp) { - return monitor_init_opts(opts, errp); + return monitor_new_opts(opts, errp); } static void monitor_parse(const char *str, const char *mode, bool pretty) -- 2.54.0

This is associates both QMP and HMP monitors with a character device backend. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- chardev/char.c | 3 ++- gdbstub/system.c | 4 ++-- include/monitor/monitor.h | 4 ++-- monitor/hmp.c | 10 ++++++-- monitor/monitor-internal.h | 2 ++ monitor/monitor.c | 47 ++++++++++++++++++++++++++++++-------- monitor/qmp.c | 18 ++++++++++----- stubs/monitor-internal.c | 2 +- 8 files changed, 67 insertions(+), 23 deletions(-) diff --git a/chardev/char.c b/chardev/char.c index 813c04d953..981fbb1aaa 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -804,8 +804,9 @@ static Chardev *qemu_chr_new_from_name(const char *label, const char *filename, } if (qemu_opt_get_bool(opts, "mux", 0)) { + const char *chardev_id = qemu_opts_id(opts); assert(permit_mux_mon); - monitor_new_hmp(chr, true, &err); + monitor_new_hmp(chardev_id, true, &err); if (err) { error_report_err(err); object_unparent(OBJECT(chr)); diff --git a/gdbstub/system.c b/gdbstub/system.c index 18f6a62b2e..80175e7fa2 100644 --- a/gdbstub/system.c +++ b/gdbstub/system.c @@ -387,9 +387,9 @@ bool gdbserver_start(const char *device, Error **errp) qemu_add_vm_change_state_handler(gdb_vm_state_change, NULL); /* Initialize a monitor terminal for gdb */ - mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB, + mon_chr = qemu_chardev_new("gdbchrdev0", TYPE_CHARDEV_GDB, NULL, NULL, &error_abort); - monitor_new_hmp(mon_chr, false, &error_abort); + monitor_new_hmp("gdbchrdev0", false, &error_abort); } else { qemu_chr_fe_deinit(&gdbserver_system_state.chr, true); mon_chr = gdbserver_system_state.mon_chr; diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 2e9f9e12e9..a135b3a590 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -28,8 +28,8 @@ bool monitor_cur_is_qmp(void); void monitor_init_globals(void); void monitor_init_globals_core(void); -void monitor_new_qmp(Chardev *chr, bool pretty, Error **errp); -void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp); +void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp); +void monitor_new_hmp(const char *chardev_id, bool use_readline, Error **errp); int monitor_new(MonitorOptions *opts, bool allow_hmp, Error **errp); int monitor_new_opts(QemuOpts *opts, Error **errp); void monitor_cleanup(void); diff --git a/monitor/hmp.c b/monitor/hmp.c index 81047d2513..4ab81b5904 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -23,6 +23,7 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include <dirent.h> #include "hw/core/qdev.h" #include "hw/core/sysemu-cpu-ops.h" @@ -1538,8 +1539,9 @@ static void monitor_readline_flush(void *opaque) monitor_flush(&mon->parent_obj); } -void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp) +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++); @@ -1547,13 +1549,17 @@ void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp) object_get_objects_root(), id, errp, + "chardev", chardev_id, NULL); + if (!obj) { return; } + mon = MONITOR_HMP(obj); - if (!qemu_chr_fe_init(&mon->parent_obj.chr, chr, errp)) { + monitor_complete(MONITOR(mon), errp); + if (*errp) { object_unparent(OBJECT(mon)); return; } diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 05c1f2f5e0..145d52fd71 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -108,6 +108,7 @@ struct MonitorClass { struct Monitor { Object parent; + char *chardev_id; CharFrontend chr; int suspend_cnt; /* Needs to be accessed atomically */ bool is_qmp; @@ -192,6 +193,7 @@ extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands; 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_destroy(Monitor *mon); diff --git a/monitor/monitor.c b/monitor/monitor.c index a497c25c54..747ca27fd5 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -77,10 +77,31 @@ OBJECT_DEFINE_TYPE(Monitor, monitor, MONITOR, OBJECT); static void monitor_finalize(Object *obj) { + Monitor *mon = MONITOR(obj); + + g_free(mon->chardev_id); +} + +static char *monitor_get_chardev_id(Object *obj, Error **errp) +{ + Monitor *mon = MONITOR(obj); + + return g_strdup(mon->chardev_id); +} + +static void monitor_set_chardev_id(Object *obj, const char *str, Error **errp) +{ + Monitor *mon = MONITOR(obj); + + g_free(mon->chardev_id); + mon->chardev_id = g_strdup(str); } static void monitor_class_init(ObjectClass *cls, const void *data) { + object_class_property_add_str(cls, "chardev", + monitor_get_chardev_id, + monitor_set_chardev_id); } static void monitor_init(Object *obj) @@ -729,16 +750,24 @@ void monitor_init_globals(void) aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); } -int monitor_new(MonitorOptions *opts, bool allow_hmp, Error **errp) +void monitor_complete(Monitor *mon, Error **errp) { - ERRP_GUARD(); - Chardev *chr; + if (mon->chardev_id) { + Chardev *chr = qemu_chr_find(mon->chardev_id); + if (chr == NULL) { + error_setg(errp, "chardev \"%s\" not found", mon->chardev_id); + return; + } - chr = qemu_chr_find(opts->chardev); - if (chr == NULL) { - error_setg(errp, "chardev \"%s\" not found", opts->chardev); - return -1; + if (!qemu_chr_fe_init(&mon->chr, chr, errp)) { + return; + } } +} + +int monitor_new(MonitorOptions *opts, bool allow_hmp, Error **errp) +{ + ERRP_GUARD(); if (!opts->has_mode) { opts->mode = allow_hmp ? MONITOR_MODE_READLINE : MONITOR_MODE_CONTROL; @@ -746,7 +775,7 @@ int monitor_new(MonitorOptions *opts, bool allow_hmp, Error **errp) switch (opts->mode) { case MONITOR_MODE_CONTROL: - monitor_new_qmp(chr, opts->pretty, errp); + monitor_new_qmp(opts->chardev, opts->pretty, errp); break; case MONITOR_MODE_READLINE: if (!allow_hmp) { @@ -757,7 +786,7 @@ int monitor_new(MonitorOptions *opts, bool allow_hmp, Error **errp) error_setg(errp, "'pretty' is not compatible with HMP monitors"); return -1; } - monitor_new_hmp(chr, true, errp); + monitor_new_hmp(opts->chardev, true, errp); break; default: g_assert_not_reached(); diff --git a/monitor/qmp.c b/monitor/qmp.c index 5231ed506a..0abf2677f9 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -527,8 +527,9 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) monitor_list_append(&mon->parent_obj); } -void monitor_new_qmp(Chardev *chr, bool pretty, Error **errp) +void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) { + ERRP_GUARD(); MonitorQMP *mon; static int counter; g_autofree char *id = g_strdup_printf("qmpcompat%d", counter++); @@ -536,21 +537,26 @@ void monitor_new_qmp(Chardev *chr, bool pretty, Error **errp) object_get_objects_root(), id, errp, + "chardev", chardev_id, NULL); + if (!obj) { return; } - mon = MONITOR_QMP(obj); - if (!qemu_chr_fe_init(&mon->parent_obj.chr, chr, errp)) { + mon = MONITOR_QMP(obj); + monitor_complete(MONITOR(mon), errp); + if (*errp) { object_unparent(OBJECT(mon)); return; } + 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, - qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)); + qemu_chr_has_feature(mon->parent_obj.chr.chr, + QEMU_CHAR_FEATURE_GCONTEXT)); mon->pretty = pretty; @@ -563,12 +569,12 @@ void monitor_new_qmp(Chardev *chr, bool pretty, Error **errp) * Make sure the old iowatch is gone. It's possible when * e.g. the chardev is in client mode, with wait=on. */ - remove_fd_in_watch(chr); + remove_fd_in_watch(mon->parent_obj.chr.chr); /* * Clean up listener IO sources early to prevent racy fd * handling between the main thread and the I/O thread. */ - remove_listener_fd_in_watch(chr); + remove_listener_fd_in_watch(mon->parent_obj.chr.chr); /* * We can't call qemu_chr_fe_set_handlers() directly here * since chardev might be running in the monitor I/O diff --git a/stubs/monitor-internal.c b/stubs/monitor-internal.c index 23d58da184..51db7588b9 100644 --- a/stubs/monitor-internal.c +++ b/stubs/monitor-internal.c @@ -8,6 +8,6 @@ int monitor_get_fd(Monitor *mon, const char *name, Error **errp) return -1; } -void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp) +void monitor_new_hmp(const char *chardev_id, bool use_readline, Error **errp) { } -- 2.54.0

This determines whether the QMP JSON responses are pretty printed with newlines and indentation, or compact with no extra whitespace. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> 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 0abf2677f9..2a8532ef96 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) @@ -538,6 +555,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) { @@ -558,8 +576,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

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. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> 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 e3e5623a4b..76df379a2c 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) @@ -106,6 +110,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) @@ -637,38 +646,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) @@ -686,7 +673,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; @@ -700,8 +687,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); @@ -716,7 +703,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 e3318b57db..694d303215 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; @@ -572,14 +579,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: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> 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 8b70380d64..6f0353d9f1 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -346,22 +346,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 694d303215..ec17050a70 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

Introducing a virtual "requires_iothread" method allows the code to automatically initialize the I/O thread during object completion. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> 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 2f16cd3053..b788669f1f 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -160,6 +160,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 @@ -561,7 +567,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. @@ -594,7 +600,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(); @@ -632,14 +638,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) { /* @@ -734,6 +732,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 ec17050a70..93f576c19d 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, @@ -598,13 +606,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 error reporting path. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> 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

Rather than asserting monitor_is_qmp(), use a QOM cast via MONITOR_QMP which performs an assert already. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> 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

The previous patch dropped the only remaining use of monitor_is_qmp. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> 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 15e49d78d7..6ed586fc4f 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -113,7 +113,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

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. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> 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 f88364a574..bf5e8a184a 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) { @@ -101,11 +103,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) @@ -680,8 +688,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 d2a03bb7e3..0e1c3eb52b 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); @@ -583,25 +590,28 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) { ERRP_GUARD(); - 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; } @@ -633,3 +643,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..6affb70a59 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 line editing +# (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 fb36140aac..79ed2be449 100644 --- a/system/vl.c +++ b/system/vl.c @@ -1830,6 +1830,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); } @@ -1971,7 +1975,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

From: Christian Brauner <brauner@kernel.org> If an 'object_del' command for a QMP monitor arrives targetting the current monitor, reject this request. If the current monitor is deleted, it will be impossible to send any reply and the client won't be able to remove the corresponding chardev backend. Note, it is not possible to rely on checking monitor_cur() because if 'object_del' is called via human-monitor-command, monitor_cur() will reflect the temporary HMP, not the QMP target that needs to be checked. Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org> [DB: extracted monitor tracking from larger commit; added logic to monitor_qmp_prepare_delete to reject request] Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/qmp.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/monitor/qmp.c b/monitor/qmp.c index 0e1c3eb52b..b0535dec68 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -72,6 +72,9 @@ typedef struct QMPRequest QMPRequest; QmpCommandList qmp_commands, qmp_cap_negotiation_commands; +/* Monitor being serviced by the dispatcher. Protected by BQL. */ +static MonitorQMP *qmp_dispatcher_current_mon; + OBJECT_DEFINE_TYPE(MonitorQMP, monitor_qmp, MONITOR_QMP, MONITOR); static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon); @@ -369,6 +372,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data) */ mon = req_obj->mon; + qmp_dispatcher_current_mon = mon; /* * We need to resume the monitor if handle_qmp_command() @@ -429,6 +433,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data) } qmp_request_free(req_obj); + qmp_dispatcher_current_mon = NULL; } qatomic_set(&qmp_dispatcher_co, NULL); } @@ -573,6 +578,11 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent event) } } +static bool monitor_qmp_dispatcher_is_servicing(MonitorQMP *mon) +{ + return qmp_dispatcher_current_mon == mon; +} + static void monitor_qmp_setup_handlers_bh(void *opaque) { MonitorQMP *mon = opaque; @@ -646,6 +656,13 @@ static void monitor_qmp_complete(UserCreatable *uc, Error **errp) static bool monitor_qmp_prepare_delete(UserCreatable *uc, Error **errp) { + MonitorQMP *qmp = MONITOR_QMP(uc); + + if (monitor_qmp_dispatcher_is_servicing(qmp)) { + error_setg(errp, "Cannot delete the current QMP monitor"); + return false; + } + error_setg(errp, "Deleting QMP monitors is not supported"); return false; } -- 2.54.0

From: Christian Brauner <brauner@kernel.org> The removal sequence is: 1. Remove from mon_list under monitor_lock. This must happen before disconnecting chardev handlers to prevent event broadcast from calling monitor_flush_locked() after the gcontext reset, which would create an out_watch on the wrong GMainContext (see monitor_cancel_out_watch()). 2. Cancel any pending out_watch while gcontext still points to the correct context. 3. Disconnect chardev handlers, passing context=NULL and close the connection. 4. Drain pending requests from any in-flight monitor_qmp_read(). 5. Destroy the monitor object Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org> [DB: extracted from a larger commit and refactored to apply to the new monitor class structure. Remove 'self delete' feature which requires complex special-case code] Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/monitor-internal.h | 2 ++ monitor/monitor.c | 22 ++++++++++++++++ monitor/qmp.c | 54 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 76 insertions(+), 2 deletions(-) diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index a82e1aacb6..5522e05464 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -178,6 +178,7 @@ struct MonitorQMP { Monitor parent_obj; JSONMessageParser parser; bool pretty; + bool setup_pending; /* iothread BH has not yet set up chardev handlers */ /* * When a client connects, we're in capabilities negotiation mode. * @commands is &qmp_cap_negotiation_commands then. When command @@ -206,6 +207,7 @@ extern MonitorList mon_list; bool monitor_requires_iothread(const Monitor *mon); int monitor_can_read(void *opaque); +void monitor_cancel_out_watch(Monitor *mon); void monitor_list_append(Monitor *mon); void monitor_fdsets_cleanup(void); int monitor_set_cpu(Monitor *mon, int cpu_index); diff --git a/monitor/monitor.c b/monitor/monitor.c index 0e1c623555..b155f58546 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -177,6 +177,28 @@ static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond, return G_SOURCE_REMOVE; } +/* Cancel a pending out_watch GSource. Caller must hold mon_lock. */ +void monitor_cancel_out_watch(Monitor *mon) +{ + if (mon->out_watch) { + GMainContext *ctx = NULL; + GSource *src; + + if (monitor_requires_iothread(mon)) { + ctx = iothread_get_g_main_context(mon_iothread); + } + src = g_main_context_find_source_by_id(ctx, mon->out_watch); + if (!src && ctx) { + /* Handler disconnect may have reset gcontext to NULL. */ + src = g_main_context_find_source_by_id(NULL, mon->out_watch); + } + if (src) { + g_source_destroy(src); + } + mon->out_watch = 0; + } +} + /* Caller must hold mon->mon_lock */ void monitor_flush_locked(Monitor *mon) { diff --git a/monitor/qmp.c b/monitor/qmp.c index 2131be82c7..5301927f09 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -184,6 +184,12 @@ static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon) } } +static void monitor_qmp_drain_queue(MonitorQMP *mon) +{ + QEMU_LOCK_GUARD(&mon->qmp_queue_lock); + monitor_qmp_cleanup_req_queue_locked(mon); +} + static void monitor_qmp_cleanup_queue_and_resume(MonitorQMP *mon) { QEMU_LOCK_GUARD(&mon->qmp_queue_lock); @@ -597,6 +603,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) monitor_qmp_read, monitor_qmp_event, NULL, &mon->parent_obj, context, true); monitor_list_append(&mon->parent_obj); + qatomic_set(&mon->setup_pending, false); } void monitor_new_qmp(const char *chardev_id, bool pretty, Error **errp) @@ -645,6 +652,7 @@ static void monitor_qmp_complete(UserCreatable *uc, Error **errp) * since chardev might be running in the monitor I/O * thread. Schedule a bottom half. */ + mon->setup_pending = true; aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread), monitor_qmp_setup_handlers_bh, mon); /* The bottom half will add @mon to @mon_list */ @@ -656,8 +664,14 @@ static void monitor_qmp_complete(UserCreatable *uc, Error **errp) } } +static void monitor_qmp_iothread_quiesce(void *opaque) +{ + /* No-op: synchronization point only */ +} + static bool monitor_qmp_prepare_delete(UserCreatable *uc, Error **errp) { + Monitor *mon = MONITOR(uc); MonitorQMP *qmp = MONITOR_QMP(uc); if (monitor_qmp_dispatcher_is_servicing(qmp)) { @@ -665,8 +679,44 @@ static bool monitor_qmp_prepare_delete(UserCreatable *uc, Error **errp) return false; } - error_setg(errp, "Deleting QMP monitors is not supported"); - return false; + if (qatomic_read(&qmp->setup_pending)) { + error_setg(errp, "monitor is still initializing"); + return false; + } + + /* Remove from mon_list before chardev disconnect. */ + WITH_QEMU_LOCK_GUARD(&monitor_lock) { + QTAILQ_REMOVE(&mon_list, mon, entry); + } + + /* Cancel out_watch while gcontext still points to the right ctx. */ + WITH_QEMU_LOCK_GUARD(&mon->mon_lock) { + monitor_cancel_out_watch(mon); + } + + qemu_chr_fe_set_handlers(&mon->chr, NULL, NULL, NULL, NULL, + NULL, NULL, true); + + /* Drain requests from any in-flight monitor_qmp_read(). */ + monitor_qmp_drain_queue(qmp); + + WITH_QEMU_LOCK_GUARD(&mon->mon_lock) { + /* Disable flushes before cancel -- gcontext is already wrong. */ + qemu_chr_fe_set_open(&mon->chr, false); + monitor_cancel_out_watch(mon); + } + + /* Synchronize with in-flight iothread callbacks. */ + if (monitor_requires_iothread(mon)) { + aio_wait_bh_oneshot(iothread_get_aio_context(mon_iothread), + monitor_qmp_iothread_quiesce, NULL); + } + + /* Catch requests from a racing monitor_qmp_read(). */ + monitor_qmp_drain_queue(qmp); + monitor_fdsets_cleanup(); + + return true; } static void monitor_qmp_accept_input(Monitor *mon) -- 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] Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> 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 93df53d87f..c74ffe56ae 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3616,6 +3616,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

The 'user_creatable_del' method doesn't want the 'Object *', but instead the "id" of the object to be deleted. While most of its work could be done with the "Object *" alone, purging the QemuOpts values needs the "id". The "id" is not recorded against an Object though, it is indirectly stored as a property linking the parent and child objects together. Add an accessor that can fetch this property name. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/qom/object.h | 10 ++++++++++ qom/object.c | 17 +++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/include/qom/object.h b/include/qom/object.h index 11f55613fc..216d2a30f6 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -1759,6 +1759,16 @@ ObjectProperty *object_property_try_add_child(Object *obj, const char *name, ObjectProperty *object_property_add_child(Object *obj, const char *name, Object *child); +/** + * object_property_get_child_name: + * @obj: the object that owns the property + * @child: the object referenced by the child property + * + * Return the property name against which @child is registered + * with @obj, or NULL if @obj is not a parent of @child. + */ +char *object_property_get_child_name(Object *obj, Object *child); + typedef enum { /* Unref the link pointer when the property is deleted */ OBJ_PROP_LINK_STRONG = 0x1, diff --git a/qom/object.c b/qom/object.c index f79b2cf361..66bbd1488b 100644 --- a/qom/object.c +++ b/qom/object.c @@ -1952,6 +1952,23 @@ object_property_add_child(Object *obj, const char *name, return object_property_try_add_child(obj, name, child, &error_abort); } +char *object_property_get_child_name(Object *obj, Object *child) +{ + ObjectProperty *prop; + GHashTableIter iter; + gpointer key, value; + + g_hash_table_iter_init(&iter, obj->properties); + while (g_hash_table_iter_next(&iter, &key, &value)) { + prop = value; + if (object_property_is_child(prop) && prop->opaque == child) { + return g_strdup(prop->name); + } + } + return NULL; +} + + void object_property_allow_set_link(const Object *obj, const char *name, Object *val, Error **errp) { -- 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. Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/monitor-internal.h | 3 + monitor/qmp.c | 75 +++++++++++++++++ qapi/qom.json | 21 ++++- .../generic/test_monitor_hotplug.py | 83 ++++++++++++++++++- 4 files changed, 177 insertions(+), 5 deletions(-) diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 5522e05464..23829f32f9 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" @@ -178,7 +179,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 5301927f09..1057b0d12b 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,45 @@ 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; + + if (!mon_id) { + /* Another monitor raced & ran 'object-del' on 'mon' + * before this BH got scheduled, so we have a ref on + * mon but it is already unparented. + */ + object_unref(mon); + return; + } + + user_creatable_del(mon_id, &local_error); + object_unref(mon); + 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; + /* Protect against race if a client drops & quickly + * reconnects - we'll have the delete BH scheduled + * so must not honour a new open request + */ + if (mon->delete_pending) { + return; + } + switch (event) { case CHR_EVENT_OPENED: WITH_QEMU_LOCK_GUARD(&mon->parent_obj.mon_lock) { @@ -577,6 +631,27 @@ 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 + * + * Hold an extra ref in case a separate monitor races + * with the BH by processing an explicit 'object-del' + */ + object_ref(mon); + 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 6affb70a59..b4c49a2e8b 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/tests/functional/generic/test_monitor_hotplug.py b/tests/functional/generic/test_monitor_hotplug.py index 55af8a238f..86092e781c 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,75 @@ 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 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() + + # Wait upto 10 seconds max for chardev to auto-delete, which + # is hopefully enough for reliability under high load + for i in range(int(10 / 0.2)): + cdevs = [c["label"] for c in self.vm.cmd('query-chardev')] + if 'hotplug-chr' not in cdevs: + break + # Wait a little more then try again + time.sleep(0.2) + + 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_reconnect(self): + """ + A dynamically-added monitor configured without 'close-action' + should allow reconnects after the client is closed. + """ + 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-chardev'}) + self.assertIn('return', resp) + + qmp.close() + + qmp = QEMUMonitorProtocol(sock) + qmp.connect(negotiate=True) + + resp = qmp.cmd_obj({'execute': 'query-chardev'}) + self.assertIn('return', resp) + + qmp.close() + self._remove_monitor() + def test_large_response(self): """ Send a command with a large response (query-qmp-schema) on a -- 2.54.0

Use the new preferred low level option for configuring the QMP service in libqtest and the python Machine class used by tests. This will avoid triggering deprecation warnings after the subsequent commit. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- python/qemu/machine/machine.py | 4 ++-- tests/qtest/libqtest.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index ebb58d5b68..5f4845f5fb 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -304,8 +304,8 @@ def _base_args(self) -> List[str]: ) else: moncdev = f"socket,id=mon,path={self._monitor_address}" - args.extend(['-chardev', moncdev, '-mon', - 'chardev=mon,mode=control']) + args.extend(['-chardev', moncdev, '-object', + 'monitor-qmp,id=qmp,chardev=mon']) if self._machine is not None: args.extend(['-machine', self._machine]) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 4e22c66b75..c33c799c92 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -460,7 +460,7 @@ gchar *qtest_qemu_args(const char *extra_args) "-qtest unix:%s " "-qtest-log %s " "-chardev socket,path=%s,id=char0 " - "-mon chardev=char0,mode=control " + "-object monitor-qmp,id=qmp0,chardev=char0 " "-display none " "-audio none " "%s " -- 2.54.0

The high level `-qmp` and `-monitor` options can remain as convenience wrappers, but the low level `-mon` is completed obsoleted by the new `-object` support with 'monitor-qmp' and 'monitor-hmp' types. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Tested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/about/deprecated.rst | 10 ++++++++++ docs/devel/writing-monitor-commands.rst | 4 ++-- docs/system/arm/xenpvh.rst | 4 ++-- docs/system/i386/xen.rst | 3 ++- docs/system/i386/xenpvh.rst | 4 ++-- qemu-options.hx | 11 ++++++----- system/vl.c | 2 ++ 7 files changed, 26 insertions(+), 12 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 97750f5edc..8370fc5680 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -61,6 +61,16 @@ The ``debug-threads`` option of the ``-name`` argument is now ignored. Thread naming is unconditionally enabled for all platforms where it is supported. +``-mon`` option (since 11.1) +'''''''''''''''''''''''''''' + +The ``-mon`` option was the generic mechanism for creating monitor objects +if the convenience ``-qmp`` or ``-monitor`` options were not flexible +enough. The monitor objects have been converted to QOM, so ``-mon mode=readline`` +is replaced by ``-object monitor-hmp`` and ``-mon mode=control`` is replaced +by ``-object monitor-qmp``. The short convenience options are not deprecated, +only ``-mon``. + QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst index 930da5cd06..7ae7efe327 100644 --- a/docs/devel/writing-monitor-commands.rst +++ b/docs/devel/writing-monitor-commands.rst @@ -52,8 +52,8 @@ shown here. First, QEMU should be started like this:: # qemu-system-TARGET [...] \ - -chardev socket,id=qmp,port=4444,host=localhost,server=on \ - -mon chardev=qmp,mode=control,pretty=on + -chardev socket,id=chrqmp0,port=4444,host=localhost,server=on \ + -object monitor-qmp,chardev=chrqmp0,pretty=on,id=qmp0 Then, in a different terminal:: diff --git a/docs/system/arm/xenpvh.rst b/docs/system/arm/xenpvh.rst index 430ac2c02e..511af63e3d 100644 --- a/docs/system/arm/xenpvh.rst +++ b/docs/system/arm/xenpvh.rst @@ -27,9 +27,9 @@ Sample QEMU xenpvh commands for running and connecting with Xen: qemu-system-aarch64 -xen-domid 1 \ -chardev socket,id=libxl-cmd,path=qmp-libxl-1,server=on,wait=off \ - -mon chardev=libxl-cmd,mode=control \ + -object monitor-qmp,id=qmp0,chardev=libxl-cmd \ -chardev socket,id=libxenstat-cmd,path=qmp-libxenstat-1,server=on,wait=off \ - -mon chardev=libxenstat-cmd,mode=control \ + -object monitor-qmp,id=qmp1,chardev=libxenstat-cmd \ -xen-attach -name guest0 -vnc none -display none -nographic \ -machine xenpvh -m 1301 \ -chardev socket,id=chrtpm,path=tmp/vtpm2/swtpm-sock \ diff --git a/docs/system/i386/xen.rst b/docs/system/i386/xen.rst index 46db5f34c1..6ff5cdb1e4 100644 --- a/docs/system/i386/xen.rst +++ b/docs/system/i386/xen.rst @@ -79,7 +79,8 @@ of type ``xen-console`` to connect to it. For the Xen console equivalent of the handy ``-serial mon:stdio`` option, for example: .. parsed-literal:: - -chardev stdio,mux=on,id=char0,signal=off -mon char0 \\ + -chardev stdio,mux=on,id=char0,signal=off \\ + -object monitor-hmp,chardev=char0,id=hmp0 \\ -device xen-console,chardev=char0 The Xen network device is ``xen-net-device``, which becomes the default NIC diff --git a/docs/system/i386/xenpvh.rst b/docs/system/i386/xenpvh.rst index 354250f073..904778e3f5 100644 --- a/docs/system/i386/xenpvh.rst +++ b/docs/system/i386/xenpvh.rst @@ -33,9 +33,9 @@ case you need to construct one manually: qemu-system-i386 -xen-domid 3 -no-shutdown \ -chardev socket,id=libxl-cmd,path=/var/run/xen/qmp-libxl-3,server=on,wait=off \ - -mon chardev=libxl-cmd,mode=control \ + -object monitor-qmp,id=qmp0,chardev=libxl-cmd \ -chardev socket,id=libxenstat-cmd,path=/var/run/xen/qmp-libxenstat-3,server=on,wait=off \ - -mon chardev=libxenstat-cmd,mode=control \ + -object monitor-qmp,id=qmp1,chardev=libxenstat-cmd \ -nodefaults \ -no-user-config \ -xen-attach -name g0 \ diff --git a/qemu-options.hx b/qemu-options.hx index a804d5b880..999dca7444 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4137,7 +4137,7 @@ The general form of a character device option is: :: -chardev stdio,mux=on,id=char0 \ - -mon chardev=char0,mode=readline \ + -object monitor-hmp,id=hmp0,chardev=char0 \ -serial chardev:char0 \ -serial chardev:char0 @@ -4149,7 +4149,7 @@ The general form of a character device option is: :: -chardev stdio,mux=on,id=char0 \ - -mon chardev=char0,mode=readline \ + -object monitor-hmp,id=hmp0,chardev=char0 \ -parallel chardev:char0 \ -chardev tcp,...,mux=on,id=char1 \ -serial chardev:char1 \ @@ -4950,7 +4950,8 @@ SRST -qmp tcp:localhost:4444,server=on,wait=off Not all options are configurable via this syntax; for maximum - flexibility use the ``-mon`` option and an accompanying ``-chardev``. + flexibility use ``-object monitor-qmp`` and an accompanying + ``-chardev``. ERST DEF("qmp-pretty", HAS_ARG, QEMU_OPTION_qmp_pretty, \ @@ -4981,12 +4982,12 @@ SRST enables the QMP monitor on localhost port 4444 with pretty-printing. - The use of ``-mon mode=readline`` is historical syntax sugar + The use of ``-mon mode=readline`` is deprecated 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 + The use of ``-mon mode=control`` is deprecated 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. diff --git a/system/vl.c b/system/vl.c index 79ed2be449..a7b6d37452 100644 --- a/system/vl.c +++ b/system/vl.c @@ -3240,6 +3240,8 @@ void qemu_init(int argc, char **argv) default_monitor = 0; break; case QEMU_OPTION_mon: + warn_report_once("'-mon' is deprecated, use '-object' with " + "'monitor-hmp' or 'monitor-qmp' types instead"); if (!qemu_opts_parse_noisily(qemu_find_opts("mon"), optarg, true)) { exit(1); -- 2.54.0