[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc |
Date: |
Sat, 15 Jun 2019 22:31:12 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) |
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,