Am 12.06.2019 um 15:11 hat Markus Armbruster geschrieben: > Kevin Wolf <kw...@redhat.com> writes: > > > Move QMP infrastructure from monitor/misc.c to monitor/qmp.c. This is > > code that can be shared for all targets, so compile it only once. > > Less code compiled per target, yay! > > > The amount of function and particularly extern variables in > > monitor_int.h is probably a bit larger than it needs to be, but this way > > no non-trivial code modifications are needed. The interfaces between QMP > > and the monitor core can be cleaned up later. > > That's okay. > > I have to admit I naively expected the previous patch moved everything > to the new header we need in a header for splitting up monitor/misc.c. > How did you decide what to move to the header in which patch?
The previous patch moved only the Monitor{HMP,QMP} data structures and their dependencies as I was sure these would be shared. Everything else was added to address linker complaints as I was going. I'll clarify this in the commit message of the previous patch. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > > --- > > include/monitor/monitor.h | 1 + > > monitor/monitor_int.h | 30 ++- > > monitor/misc.c | 394 +------------------------------------ > > monitor/qmp.c | 404 ++++++++++++++++++++++++++++++++++++++ > > Makefile.objs | 1 + > > monitor/Makefile.objs | 1 + > > monitor/trace-events | 4 +- > > 7 files changed, 448 insertions(+), 387 deletions(-) > > create mode 100644 monitor/qmp.c > > > > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h > > index 1ba354f811..7bbab05320 100644 > > --- a/include/monitor/monitor.h > > +++ b/include/monitor/monitor.h > > @@ -21,6 +21,7 @@ bool monitor_cur_is_qmp(void); > > > > void monitor_init_globals(void); > > void monitor_init(Chardev *chr, int flags); > > +void monitor_init_qmp(Chardev *chr, int flags); > > Why does this one go to the non-internal header? Most callers already know whether they want QMP or HMP, so they can just directly create the right thing instead of going through the monitor_init() wrapper. If you prefer, I can move it to the internal header, though. It's not called externally yet. > > void monitor_cleanup(void); > > > > int monitor_suspend(Monitor *mon); > > diff --git a/monitor/monitor_int.h b/monitor/monitor_int.h > > index 7122418955..4aabee54e1 100644 > > --- a/monitor/monitor_int.h > > +++ b/monitor/monitor_int.h > > @@ -30,10 +30,11 @@ > > > > #include "qapi/qmp/qdict.h" > > #include "qapi/qmp/json-parser.h" > > -#include "qapi/qapi-commands.h" > > +#include "qapi/qmp/dispatch.h" > > This part should be squashed into the previous patch. You'll > additionally need qapi/qapi-types-misc.h for QMP_CAPABILITY__MAX there, > or keep monitor/monitor.h, even though you need it only here for > MONITOR_USE_CONTROL. Yes, already happened while addressing the comments you had for the header. > > > > #include "qemu/readline.h" > > #include "chardev/char-fe.h" > > +#include "sysemu/iothread.h" > > Perhaps IOThread should be typedef'ed in qemu/typedefs.h. I'm not > asking you to do that. > > > > > /* > > * Supported types: > > @@ -145,4 +146,31 @@ typedef struct { > > GQueue *qmp_requests; > > } MonitorQMP; > > > > +/** > > + * Is @mon a QMP monitor? > > + */ > > +static inline bool monitor_is_qmp(const Monitor *mon) > > +{ > > + return (mon->flags & MONITOR_USE_CONTROL); > > +} > > + > > +typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList; > > +extern IOThread *mon_iothread; > > +extern QEMUBH *qmp_dispatcher_bh; > > +extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands; > > +extern QemuMutex monitor_lock; > > +extern MonitorList mon_list; > > +extern int mon_refcount; > > + > > +int monitor_puts(Monitor *mon, const char *str); > > +void monitor_data_init(Monitor *mon, int flags, bool skip_flush, > > + bool use_io_thread); > > +int monitor_can_read(void *opaque); > > +void monitor_list_append(Monitor *mon); > > +void monitor_fdsets_cleanup(void); > > + > > +void qmp_send_response(MonitorQMP *mon, const QDict *rsp); > > +void monitor_data_destroy_qmp(MonitorQMP *mon); > > +void monitor_qmp_bh_dispatcher(void *data); > > + > > #endif > > I trust you these are indeed all needed. The linker said so. :-) > > diff --git a/monitor/qmp.c b/monitor/qmp.c > > new file mode 100644 > > index 0000000000..d425b0f2ba > > --- /dev/null > > +++ b/monitor/qmp.c > > @@ -0,0 +1,404 @@ > > +/* > > + * QEMU monitor > > + * > > + * Copyright (c) 2003-2004 Fabrice Bellard > > I'm pretty confident nothing in this file is actually due to Fabrice. > > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > copy > > + * of this software and associated documentation files (the "Software"), > > to deal > > + * in the Software without restriction, including without limitation the > > rights > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or > > sell > > + * copies of the Software, and to permit persons to whom the Software is > > + * furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be included > > in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > > OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > FROM, > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > IN > > + * THE SOFTWARE. > > + */ > > We inherit the non-standard license regardless. Lesson learned: do not > add substantial code to source files with a non-standard license. Yes. Nothing I can change here now, though. The legally safe way for splitting files is to copy the license header. > > + > > +#include "qemu/osdep.h" > > +#include "monitor_int.h" > > + > > +#include "chardev/char-io.h" > > + > > +#include "qapi/error.h" > > +#include "qapi/qmp/qjson.h" > > +#include "qapi/qmp/qstring.h" > > +#include "qapi/qmp/qlist.h" > > +#include "qapi/qapi-commands-misc.h" > > + > > +#include "trace.h" > > Please sort the includes alphabetically (except qemu/osdep.h, of > course). In other words, move monitor_int.h down and remove the empty lines? I like separating header files by "category", but that's a matter of taste. I'll follow yours if you tell me whether I understood it correctly. > > + > > +struct QMPRequest { > > + /* Owner of the request */ > > + MonitorQMP *mon; > > + /* > > + * Request object to be handled or Error to be reported > > + * (exactly one of them is non-null) > > + */ > > + QObject *req; > > + Error *err; > > +}; > > +typedef struct QMPRequest QMPRequest; > > Please fuse these two. This is a code motion patch. Are you sure? [y/n] Kevin