Re: [PATCH v2 2/4] qapi: Create module 'control'

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 29.01.2020 um 09:35 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> 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 <address@hidden>
>> > ---
>> >  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 <address@hidden>
>
> Thanks. (I assume this means even without the copyright header.)

I'd prefer with, but I'll accept without.