Re: [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 14.06.2019 um 11:06 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> 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,