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

[Kevin Wolf]

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.

> > +##
> > +# = 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.

> > 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.)

Kevin