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.

v3:
- Assert monitor_is_qmp() before casting to MonitorQMP in two places
- Added note that HMP doesn't currently use iothread to the
  documentation of MonitorHMP
- Removed unnecessary memset() in monitor_data_init()
- Removed Monitor.cmd_table instead of moving it to MonitorHMP. Renamed
  the tables to have an hmp_ prefix.
- monitor_int.h of v2 becomes monitor-internal.h now
- Cleaned up #include directives in new files
- Moved some more functions between files
- Removed monitor_init() in favour of public monitor_init_hmp/qmp()
- Deprecate -mon control=readline,pretty=on|off
- Improved several commit messages

v2:
- Fix coding style while moving files to make checkpatch happier
- Updated file name references in docs/devel/writing-qmp-commands.txt
- Updated MAINTAINERS for moved and newly created files
- Created monitor/trace-events instead of using the root directory one
- Move {hmp,qmp}.c to monitor/{hmp,qmp}-cmds.c

Kevin Wolf (15):
  monitor: Remove unused password prompting fields
  monitor: Split monitor_init in HMP and QMP function
  monitor: Make MonitorQMP a child class of Monitor
  monitor: Create MonitorHMP with readline state
  monitor: Remove Monitor.cmd_table indirection
  monitor: Rename HMP command type and tables
  Move monitor.c to monitor/misc.c
  monitor: Move {hmp,qmp}.c to monitor/{hmp,qmp}-cmds.c
  monitor: Create monitor-internal.h with common definitions
  monitor: Split out monitor/qmp.c
  monitor: Split out monitor/hmp.c
  monitor: Split out monitor/monitor.c
  monitor: Split Monitor.flags into separate bools
  monitor: Replace monitor_init() with monitor_init_{hmp,qmp}()
  vl: Deprecate -mon pretty=... for HMP monitors

 docs/devel/writing-qmp-commands.txt |   11 +-
 include/monitor/monitor.h           |   15 +-
 monitor/monitor-internal.h          |  187 ++
 chardev/char.c                      |    2 +-
 gdbstub.c                           |    2 +-
 monitor.c                           | 4729 ---------------------------
 hmp.c => monitor/hmp-cmds.c         |    6 +-
 monitor/hmp.c                       | 1415 ++++++++
 monitor/misc.c                      | 2368 ++++++++++++++
 monitor/monitor.c                   |  632 ++++
 qmp.c => monitor/qmp-cmds.c         |    2 +-
 monitor/qmp.c                       |  403 +++
 stubs/monitor.c                     |    6 +-
 tests/test-util-sockets.c           |    3 +-
 vl.c                                |   28 +-
 MAINTAINERS                         |   13 +-
 Makefile.objs                       |    4 +-
 Makefile.target                     |    3 +-
 hmp-commands.hx                     |    2 +-
 monitor/Makefile.objs               |    3 +
 monitor/trace-events                |   15 +
 qemu-deprecated.texi                |    6 +
 trace-events                        |   10 -
 23 files changed, 5091 insertions(+), 4774 deletions(-)
 create mode 100644 monitor/monitor-internal.h
 delete mode 100644 monitor.c
 rename hmp.c => monitor/hmp-cmds.c (99%)
 create mode 100644 monitor/hmp.c
 create mode 100644 monitor/misc.c
 create mode 100644 monitor/monitor.c
 rename qmp.c => monitor/qmp-cmds.c (99%)
 create mode 100644 monitor/qmp.c
 create mode 100644 monitor/Makefile.objs
 create mode 100644 monitor/trace-events

-- 
2.20.1

Patchew URL: https://patchew.org/QEMU/20190613153405.24769-1-kwolf@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190613153405.24769-1-kwolf@redhat.com
Subject: [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
6d522fb vl: Deprecate -mon pretty=... for HMP monitors
e34ac59 monitor: Replace monitor_init() with monitor_init_{hmp, qmp}()
e75b96d monitor: Split Monitor.flags into separate bools
24a36f9 monitor: Split out monitor/monitor.c
27c4127 monitor: Split out monitor/hmp.c
26f727a monitor: Split out monitor/qmp.c
b2685af monitor: Create monitor-internal.h with common definitions
e1f6a6e monitor: Move {hmp, qmp}.c to monitor/{hmp, qmp}-cmds.c
ea374f7 Move monitor.c to monitor/misc.c
21e390e monitor: Rename HMP command type and tables
3335bfc monitor: Remove Monitor.cmd_table indirection
b935214 monitor: Create MonitorHMP with readline state
0800382 monitor: Make MonitorQMP a child class of Monitor
7c3f4f7 monitor: Split monitor_init in HMP and QMP function
6e68255 monitor: Remove unused password prompting fields

=== OUTPUT BEGIN ===
1/15 Checking commit 6e68255df41e (monitor: Remove unused password prompting fields)
2/15 Checking commit 7c3f4f792c86 (monitor: Split monitor_init in HMP and QMP function)
3/15 Checking commit 080038232dc8 (monitor: Make MonitorQMP a child class of Monitor)
4/15 Checking commit b935214b2c2c (monitor: Create MonitorHMP with readline state)
5/15 Checking commit 3335bfc40060 (monitor: Remove Monitor.cmd_table indirection)
6/15 Checking commit 21e390e01266 (monitor: Rename HMP command type and tables)
ERROR: spaces required around that '/' (ctx:VxV)
#226: FILE: monitor.c:4543:
+    array_num = sizeof(hmp_cmds)/elem_size-1;
                                 ^

ERROR: spaces required around that '-' (ctx:VxV)
#226: FILE: monitor.c:4543:
+    array_num = sizeof(hmp_cmds)/elem_size-1;
                                           ^

ERROR: spaces required around that '/' (ctx:VxV)
#231: FILE: monitor.c:4546:
+    array_num = sizeof(hmp_info_cmds)/elem_size-1;
                                      ^

ERROR: spaces required around that '-' (ctx:VxV)
#231: FILE: monitor.c:4546:
+    array_num = sizeof(hmp_info_cmds)/elem_size-1;
                                                ^

total: 4 errors, 0 warnings, 194 lines checked

Patch 6/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

7/15 Checking commit ea374f7b2aad (Move monitor.c to monitor/misc.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#80: 
new file mode 100644

total: 0 errors, 1 warnings, 78 lines checked

Patch 7/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/15 Checking commit e1f6a6ef1c06 (monitor: Move {hmp, qmp}.c to monitor/{hmp, qmp}-cmds.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#104: 
rename from hmp.c

total: 0 errors, 1 warnings, 73 lines checked

Patch 8/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/15 Checking commit b2685af3a437 (monitor: Create monitor-internal.h with common definitions)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#175: 
new file mode 100644

total: 0 errors, 1 warnings, 290 lines checked

Patch 9/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/15 Checking commit 26f727a38094 (monitor: Split out monitor/qmp.c)
ERROR: return is not a function, parentheses are not required
#564: FILE: monitor/monitor-internal.h:153:
+    return (mon->flags & MONITOR_USE_CONTROL);

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#590: 
new file mode 100644

total: 1 errors, 1 warnings, 954 lines checked

Patch 10/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

11/15 Checking commit 27c412749fec (monitor: Split out monitor/hmp.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#28: 
new file mode 100644

ERROR: consider using qemu_strtoull in preference to strtoull
#432: FILE: monitor/hmp.c:400:
+        n = strtoull(pch, &p, 0);

WARNING: Block comments use a leading /* on a separate line
#1314: FILE: monitor/hmp.c:1282:
+    /* if the line ends with a space, it means we want to complete the

WARNING: Block comments use a trailing */ on a separate line
#1315: FILE: monitor/hmp.c:1283:
+     * next arg */

total: 1 errors, 3 warnings, 2919 lines checked

Patch 11/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

12/15 Checking commit 24a36f947bcb (monitor: Split out monitor/monitor.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#725: 
new file mode 100644

WARNING: Block comments use a leading /* on a separate line
#1364: FILE: monitor/monitor.c:635:
+        { /* end of list */ }

total: 0 errors, 2 warnings, 1316 lines checked

Patch 12/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
13/15 Checking commit e75b96deb320 (monitor: Split Monitor.flags into separate bools)
14/15 Checking commit e34ac5963a50 (monitor: Replace monitor_init() with monitor_init_{hmp, qmp}())
15/15 Checking commit 6d522fb23163 (vl: Deprecate -mon pretty=... for HMP monitors)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190613153405.24769-1-kwolf@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Kevin Wolf <kwolf@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.

Thanks for helping out with the monitor code!  I know it's rather crusty
in places.

Dave, I'll take this through my tree, if you don't mind.

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

Kevin

Kevin Wolf <kwolf@redhat.com> writes:

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

Am 15.06.2019 um 22:31 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 14.06.2019 um 11:06 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@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.

I didn't actually apply the patch or checkout your branch, but at least
from reading the diff it looks okay.

Kevin