Kevin Wolf <kw...@redhat.com> writes: > Am 14.06.2019 um 11:06 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> >> > monitor.c mixes a lot of different things in a single file: The core >> > monitor infrastructure, HMP infrastrcture, QMP infrastructure, and the >> > implementation of several HMP and QMP commands. Almost worse, struct >> > Monitor mixes state for HMP, for QMP, and state actually shared between >> > all monitors. monitor.c must be linked with a system emulator and even >> > requires per-target compilation because some of the commands it >> > implements access system emulator state. >> > >> > The reason why I care about this is that I'm working on a protoype for a >> > storage daemon, which wants to use QMP (but probably not HMP) and >> > obviously doesn't have any system emulator state. So I'm interested in >> > some core monitor parts that can be linked to non-system-emulator tools. >> > >> > This series first creates separate structs MonitorQMP and MonitorHMP >> > which inherit from Monitor, and then moves the associated infrastructure >> > code into separate source files. >> > >> > While the split is probably not perfect, I think it's an improvement of >> > the current state even for QEMU proper, and it's good enough so I can >> > link my storage daemon against just monitor/core.o and monitor/qmp.o and >> > get a useless QMP monitor that parses the JSON input and rejects >> > everything as an unknown command. >> > >> > Next I'll try to teach it a subset of QMP commands that can actually be >> > supported in a tool, but while there will be a few follow-up patches to >> > achieve this, I don't expect that this work will bring up much that >> > needs to be changed in the splitting process done in this series. >> >> I think I can address the remaining rather minor issues without a >> respin. Please let me know if you disagree with any of my remarks. > > Feel free to make the changes you suggested, possibly with the exception > of the #includes in monitor-internal.h where I think you're only > partially right (see my reply there). > > Please also consider fixing the commit message typo I pointed out for > patch 15.
Done. Result in my public repository https://repo.or.cz/qemu/armbru.git tag pull-monitor-2019-06-15, just in case you want to run your eyes over it. Incremental diff appended. monitor/hmp-cmds.c | 5 ++--- monitor/hmp.c | 13 +++++++------ monitor/misc.c | 27 ++++++--------------------- monitor/monitor-internal.h | 14 +++++--------- monitor/monitor.c | 10 +++------- monitor/qmp.c | 5 +++-- 6 files changed, 26 insertions(+), 48 deletions(-) diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 712737cd18..c283dde0e9 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -24,7 +24,7 @@ #include "qemu/option.h" #include "qemu/timer.h" #include "qemu/sockets.h" -#include "monitor/monitor.h" +#include "monitor/monitor-internal.h" #include "monitor/qdev.h" #include "qapi/error.h" #include "qapi/opts-visitor.h" @@ -1943,8 +1943,7 @@ static void hmp_change_read_arg(void *opaque, const char *password, void hmp_change(Monitor *mon, const QDict *qdict) { - /* FIXME Make MonitorHMP public and use container_of */ - MonitorHMP *hmp_mon = (MonitorHMP *) mon; + MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common); const char *device = qdict_get_str(qdict, "device"); const char *target = qdict_get_str(qdict, "target"); const char *arg = qdict_get_try_str(qdict, "arg"); diff --git a/monitor/hmp.c b/monitor/hmp.c index 43185a7445..5349a81307 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -24,18 +24,17 @@ #include "qemu/osdep.h" #include "monitor-internal.h" - #include "qapi/error.h" +#include "qapi/qmp/qdict.h" #include "qapi/qmp/qnum.h" - #include "qemu/config-file.h" #include "qemu/ctype.h" +#include "qemu/cutils.h" #include "qemu/log.h" #include "qemu/option.h" #include "qemu/units.h" #include "sysemu/block-backend.h" #include "sysemu/sysemu.h" - #include "trace.h" static void monitor_command_cb(void *opaque, const char *cmdline, @@ -1279,8 +1278,10 @@ static void monitor_find_completion(void *opaque, return; } - /* if the line ends with a space, it means we want to complete the - * next arg */ + /* + * if the line ends with a space, it means we want to complete the + * next arg + */ len = strlen(cmdline); if (len > 0 && qemu_isspace(cmdline[len - 1])) { if (nb_args >= MAX_ARGS) { @@ -1395,7 +1396,7 @@ static void monitor_readline_flush(void *opaque) void monitor_init_hmp(Chardev *chr, bool use_readline) { - MonitorHMP *mon = g_malloc0(sizeof(*mon)); + MonitorHMP *mon = g_new0(MonitorHMP, 1); monitor_data_init(&mon->common, false, false, false); qemu_chr_fe_init(&mon->common.chr, chr, &error_abort); diff --git a/monitor/misc.c b/monitor/misc.c index 49d8c445c4..10f24673f8 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -35,18 +35,12 @@ #include "exec/gdbstub.h" #include "net/net.h" #include "net/slirp.h" -#include "chardev/char-fe.h" -#include "chardev/char-io.h" #include "chardev/char-mux.h" #include "ui/qemu-spice.h" #include "sysemu/numa.h" -#include "monitor/monitor.h" -#include "qemu/config-file.h" #include "qemu/ctype.h" -#include "qemu/readline.h" #include "ui/console.h" #include "ui/input.h" -#include "sysemu/block-backend.h" #include "audio/audio.h" #include "disas/disas.h" #include "sysemu/balloon.h" @@ -58,11 +52,7 @@ #include "sysemu/tpm.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" -#include "qapi/qmp/qnum.h" #include "qapi/qmp/qstring.h" -#include "qapi/qmp/qjson.h" -#include "qapi/qmp/json-parser.h" -#include "qapi/qmp/qlist.h" #include "qom/object_interfaces.h" #include "trace/control.h" #include "monitor/hmp-target.h" @@ -71,7 +61,6 @@ #endif #include "exec/memory.h" #include "exec/exec-all.h" -#include "qemu/log.h" #include "qemu/option.h" #include "hmp.h" #include "qemu/thread.h" @@ -81,9 +70,7 @@ #include "qapi/error.h" #include "qapi/qmp-event.h" #include "qapi/qapi-introspect.h" -#include "sysemu/qtest.h" #include "sysemu/cpus.h" -#include "sysemu/iothread.h" #include "qemu/cutils.h" #include "tcg/tcg.h" @@ -2336,14 +2323,12 @@ compare_mon_cmd(const void *a, const void *b) static void sortcmdlist(void) { - int array_num; - int elem_size = sizeof(HMPCommand); - - array_num = sizeof(hmp_cmds)/elem_size-1; - qsort((void *)hmp_cmds, array_num, elem_size, compare_mon_cmd); - - array_num = sizeof(hmp_info_cmds)/elem_size-1; - qsort((void *)hmp_info_cmds, array_num, elem_size, compare_mon_cmd); + qsort(hmp_cmds, ARRAY_SIZE(hmp_cmds) - 1, + sizeof(*hmp_cmds), + compare_mon_cmd); + qsort(hmp_info_cmds, ARRAY_SIZE(hmp_info_cmds) - 1, + sizeof(*hmp_info_cmds), + compare_mon_cmd); } void monitor_init_globals(void) diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 333ebf89e4..7760b22ba3 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -22,19 +22,15 @@ * THE SOFTWARE. */ -#ifndef MONITOR_INT_H -#define MONITOR_INT_H +#ifndef MONITOR_INTERNAL_H +#define MONITOR_INTERNAL_H +#include "chardev/char-fe.h" #include "monitor/monitor.h" -#include "qemu/cutils.h" - -#include "qapi/qmp/qdict.h" -#include "qapi/qmp/json-parser.h" -#include "qapi/qmp/dispatch.h" #include "qapi/qapi-types-misc.h" - +#include "qapi/qmp/dispatch.h" +#include "qapi/qmp/json-parser.h" #include "qemu/readline.h" -#include "chardev/char-fe.h" #include "sysemu/iothread.h" /* diff --git a/monitor/monitor.c b/monitor/monitor.c index 01d8fb5d30..3ef28171c0 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -24,15 +24,13 @@ #include "qemu/osdep.h" #include "monitor-internal.h" - #include "qapi/error.h" #include "qapi/qapi-emit-events.h" +#include "qapi/qmp/qdict.h" #include "qapi/qmp/qstring.h" - #include "qemu/error-report.h" #include "qemu/option.h" #include "sysemu/qtest.h" - #include "trace.h" /* @@ -545,11 +543,9 @@ void monitor_data_destroy(Monitor *mon) g_free(mon->mon_cpu_path); qemu_chr_fe_deinit(&mon->chr, false); if (monitor_is_qmp(mon)) { - MonitorQMP *qmp_mon = container_of(mon, MonitorQMP, common); - monitor_data_destroy_qmp(qmp_mon); + monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common)); } else { - MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common); - readline_free(hmp_mon->rs); + readline_free(container_of(mon, MonitorHMP, common)->rs); } qobject_unref(mon->outbuf); qemu_mutex_destroy(&mon->mon_lock); diff --git a/monitor/qmp.c b/monitor/qmp.c index 7258f2b088..e1b196217d 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -28,9 +28,10 @@ #include "monitor-internal.h" #include "qapi/error.h" #include "qapi/qapi-commands-misc.h" +#include "qapi/qmp/qdict.h" #include "qapi/qmp/qjson.h" -#include "qapi/qmp/qstring.h" #include "qapi/qmp/qlist.h" +#include "qapi/qmp/qstring.h" #include "trace.h" struct QMPRequest { @@ -365,7 +366,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) void monitor_init_qmp(Chardev *chr, bool pretty) { - MonitorQMP *mon = g_malloc0(sizeof(*mon)); + MonitorQMP *mon = g_new0(MonitorQMP, 1); /* Note: we run QMP monitor in I/O thread when @chr supports that */ monitor_data_init(&mon->common, true, false,