Kevin Wolf <kw...@redhat.com> writes:
> Am 29.01.2020 um 09:35 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@redhat.com> writes:
>>
>> > misc.json contains definitions that are related to the system emulator,
>> > so it can't be used for other tools like the storage daemon. This patch
>> > moves basic functionality that is shared between all tools (and mostly
>> > related to the monitor itself) into a new control.json, which could be
>> > used in tools as well.
>> >
>> > Signed-off-by: Kevin Wolf <kw...@redhat.com>
>> > ---
>> > qapi/control.json | 218 +++++++++++++++++++++++++++++++++++++
>> > qapi/misc.json | 212 ------------------------------------
>> > qapi/qapi-schema.json | 1 +
>> > monitor/monitor-internal.h | 1 +
>> > monitor/hmp-cmds.c | 1 +
>> > monitor/misc.c | 1 +
>> > monitor/qmp-cmds.c | 1 +
>> > monitor/qmp.c | 2 +-
>> > tests/qtest/qmp-test.c | 2 +-
>> > ui/gtk.c | 1 +
>> > qapi/Makefile.objs | 6 +-
>> > 11 files changed, 229 insertions(+), 217 deletions(-)
>> > create mode 100644 qapi/control.json
>> >
>> > diff --git a/qapi/control.json b/qapi/control.json
>> > new file mode 100644
>> > index 0000000000..a82a18da1a
>> > --- /dev/null
>> > +++ b/qapi/control.json
>> > @@ -0,0 +1,218 @@
>> > +# -*- Mode: Python -*-
>> > +#
>> > +
>>
>> Let's add a copyright notice:
>>
>> # Copyright (C) 2011-2020 Red Hat, Inc.
>> #
>> # This work is licensed under the terms of the GNU GPL, version 2 or
>> later.
>> # See the COPYING file in the top-level directory.
>
> I'm not adding anything new, but just moving code from a file that
> doesn't have a copyright notice. In fact, almost none of the schema
> files have a copyright notice. I'm not comfortable adding legal
> assertions without verifying that they are correct, and certainly not as
> a side-effect of a code movement patch. This would be an unrelated
> change.
>
> I suggest that we leave this patch as is, and if you think copyright
> notices should be added, the correct information can be tracked down
> and added consistently for all schema files in a separate series.
There is nothing to be tracked down. Anything that lacks an explicit
copyright notice is under GPLv2+, as per LICENSE.
>> > +##
>> > +# = Monitor definitions (shared between system emulator and tools)
>> > +##
>>
>> This comment does double-duty: it's for readers of this source file, and
>> for readers of generated docs/interop/qemu-qmp-ref.*. It's okay for
>> the former, but not the latter, as the resulting table of contents
>> shows:
>> [...]
>
>> Proposed header:
>>
>> # = QMP monitor control
>
> Works for me.
>
>> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
>> > index 9751b11f8f..61fd91ede7 100644
>> > --- a/qapi/qapi-schema.json
>> > +++ b/qapi/qapi-schema.json
>> > @@ -103,6 +103,7 @@
>> > { 'include': 'qdev.json' }
>> > { 'include': 'machine.json' }
>> > { 'include': 'machine-target.json' }
>> > +{ 'include': 'control.json' }
>> > { 'include': 'misc.json' }
>> > { 'include': 'misc-target.json' }
>> > { 'include': 'audio.json' }
>>
>> This determines position within docs/interop/qemu-qmp-ref.*. Next to
>> misc.json is the least change. Perhaps putting it next to
>> introspect.json would be better.
>>
>> If we split @quit off control.json, then we could include the .json
>> providing @quit next to @stop & friends. Again, I'm not demanding such
>> a split.
>
> I'll put it before introspect.json for now.
>
> I don't think the whole order is overly meaningful and it could use some
> rearrangement in general. Again, unrelated to this series.
Yes, the order could use some love.
>> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
>> > index d78f5ca190..4d402ded85 100644
>> > --- a/monitor/monitor-internal.h
>> > +++ b/monitor/monitor-internal.h
>> > @@ -27,6 +27,7 @@
>> >
>> > #include "chardev/char-fe.h"
>> > #include "monitor/monitor.h"
>> > +#include "qapi/qapi-types-control.h"
>> > #include "qapi/qmp/dispatch.h"
>> > #include "qapi/qmp/json-parser.h"
>> > #include "qemu/readline.h"
>> > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> > index d0e0af893a..abb052836b 100644
>> > --- a/monitor/hmp-cmds.c
>> > +++ b/monitor/hmp-cmds.c
>> > @@ -33,6 +33,7 @@
>> > #include "qapi/qapi-commands-char.h"
>> > #include "qapi/qapi-commands-migration.h"
>> > #include "qapi/qapi-commands-misc.h"
>> > +#include "qapi/qapi-commands-control.h"
>> > #include "qapi/qapi-commands-net.h"
>> > #include "qapi/qapi-commands-rocker.h"
>> > #include "qapi/qapi-commands-run-state.h"
>>
>> Please keep the qapi/qapi-commands-* sorted, like you do ...
>
> It is sorted! It's exactly where you would expect "-monitor"... *sigh*
>
> /me goes back to finding each #include and moving it.
> /me hates renaming header files.
>
>> > diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
>> > index a8f1f4c35e..20fcc37c2c 100644
>> > --- a/qapi/Makefile.objs
>> > +++ b/qapi/Makefile.objs
>> > @@ -5,9 +5,9 @@ util-obj-y += opts-visitor.o qapi-clone-visitor.o
>> > util-obj-y += qmp-event.o
>> > util-obj-y += qapi-util.o
>> >
>> > -QAPI_COMMON_MODULES = audio authz block-core block char common crypto
>> > -QAPI_COMMON_MODULES += dump error introspect job machine migration misc
>> > net
>> > -QAPI_COMMON_MODULES += qdev qom rdma rocker run-state sockets tpm
>> > +QAPI_COMMON_MODULES = audio authz block-core block char common control
>> > crypto
>> > +QAPI_COMMON_MODULES += dump error introspect job machine migration misc
>> > +QAPI_COMMON_MODULES += net qdev qom rdma rocker run-state sockets tpm
>> > QAPI_COMMON_MODULES += trace transaction ui
>> > QAPI_TARGET_MODULES = machine-target misc-target
>> > QAPI_MODULES = $(QAPI_COMMON_MODULES) $(QAPI_TARGET_MODULES)
>>
>> With the comments and the include directives tidied up:
>> Reviewed-by: Markus Armbruster <arm...@redhat.com>
>
> Thanks. (I assume this means even without the copyright header.)
I'd prefer with, but I'll accept without.
