Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc
Am 15.06.2019 um 22:31 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > Am 14.06.2019 um 11:06 hat Markus Armbruster geschrieben: > >> Kevin Wolf 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
Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc
Kevin Wolf writes: > Am 14.06.2019 um 11:06 hat Markus Armbruster geschrieben: >> Kevin Wolf 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/m
Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc
Am 14.06.2019 um 11:06 hat Markus Armbruster geschrieben: > Kevin Wolf 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
Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc
Kevin Wolf 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.
Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc
Patchew URL: https://patchew.org/QEMU/20190613153405.24769-1-kw...@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20190613153405.24769-1-kw...@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: +