Re: [Qemu-devel] [PATCH v2 24/29] qapi: Empty out qapi-schema.json

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 02/11/2018 03:36 AM, Markus Armbruster wrote:
>> The previous commit improved compile time by including less of the
>> generated QAPI headers.  This is impossible for stuff defined directly
>> in qapi-schema.json, because that ends up in headers that that pull in
>> everything.
>>
>> Move everything but include directives from qapi-schema.json to new
>> sub-module qapi/misc.json, then include just the "misc" shard where
>> possible.
>>
>> It's not possible everywhere, except:
>
> Odd wording, did you mean one of:
>
> It's possible everywhere, except:
> It's almost possible everywhere, except:

The former.

>> * monitor.c needs qmp-command.h to get qmp_init_marshall()
>
> s/marshall/marshal/

Yes.

>> * monitor.c, ui/vnc.c and the generated qapi-event-FOO.c need
>>    qapi-event.h to get enum QAPIEvent
>>
>> Perhaps we'll get rid of those some other day.
>>
>> Adding a type to qapi/migration.json now recompiles some 120 instead
>> of 2300 out of 5100 objects.
>
> Getting better :)
>
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   .gitignore                         |    4 +
>>   Makefile                           |    9 +
>>   Makefile.objs                      |    4 +
>>   arch_init.c                        |    2 +-
>>   balloon.c                          |    2 +-
>>   block/iscsi.c                      |    2 +-
>>   cpus.c                             |    2 +-
>>   dump.c                             |    4 +-
>>   hmp.c                              |   10 +-
>>   hw/acpi/core.c                     |    2 +-
>>   hw/acpi/cpu.c                      |    2 +-
>>   hw/acpi/memory_hotplug.c           |    2 +-
>>   hw/acpi/vmgenid.c                  |    2 +-
>>   hw/core/qdev.c                     |    2 +-
>>   hw/i386/xen/xen-hvm.c              |    2 +-
>>   hw/ipmi/ipmi.c                     |    2 +-
>>   hw/pci/pci-stub.c                  |    2 +-
>>   hw/pci/pci.c                       |    2 +-
>>   hw/ppc/spapr_rtc.c                 |    2 +-
>>   hw/s390x/s390-skeys.c              |    2 +-
>>   hw/timer/mc146818rtc.c             |    4 +-
>>   hw/virtio/virtio-balloon.c         |    2 +-
>>   hw/watchdog/watchdog.c             |    2 +-
>>   include/hw/qdev-properties.h       |    3 +-
>>   include/monitor/monitor.h          |    2 +-
>>   include/sysemu/arch_init.h         |    2 +-
>>   include/sysemu/balloon.h           |    2 +-
>>   include/sysemu/dump.h              |    2 +-
>>   include/sysemu/hostmem.h           |    2 +-
>>   include/sysemu/replay.h            |    3 +-
>>   iothread.c                         |    2 +-
>>   migration/savevm.c                 |    3 +-
>>   numa.c                             |    4 +-
>
> Mostly mechanical,
>
>>   qapi-schema.json                   | 3098 +-----------------------------------
>>   qapi/misc.json                     | 3090 +++++++++++++++++++++++++++++++++++
>
> A huge move of content (git won't flag it as a rename, though ;(
>
> But why is the new file smaller than what was removed from the old
> file? Let's see...[1]
>
>>   qapi/run-state.json                |   10 +
>>   qdev-monitor.c                     |    2 +-
>>   qmp.c                              |    4 +-
>>   stubs/uuid.c                       |    2 +-
>>   stubs/vmgenid.c                    |    2 +-
>>   stubs/xen-hvm.c                    |    2 +-
>>   target/arm/monitor.c               |    3 +-
>>   target/i386/cpu.c                  |    4 +-
>>   tests/qmp-test.c                   |    3 +-
>>   tests/test-qobject-input-visitor.c |    2 +-
>>   tests/test-visitor-serialization.c |    1 -
>>   ui/gtk.c                           |    2 +-
>>   util/qemu-config.c                 |    2 +-
>>   vl.c                               |    4 +-
>>   49 files changed, 3181 insertions(+), 3144 deletions(-)
>>   create mode 100644 qapi/misc.json
>
> Huge email, but reviewing is not too hard.
>
>>
>> diff --git a/.gitignore b/.gitignore
>> index 42c57998fd..7f162e862f 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -38,6 +38,7 @@
>>   /qapi/qapi-commands-crypto.[ch]
>>   /qapi/qapi-commands-introspect.[ch]
>>   /qapi/qapi-commands-migration.[ch]
>> +/qapi/qapi-commands-misc.[ch]
>
> If my comments on the earlier patch result in a glob here, you
> wouldn't need these changes.
>
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -99,6 +99,7 @@ GENERATED_FILES += qapi/qapi-types-common.h qapi/qapi-types-common.c
>>   GENERATED_FILES += qapi/qapi-types-crypto.h qapi/qapi-types-crypto.c
>>   GENERATED_FILES += qapi/qapi-types-introspect.h qapi/qapi-types-introspect.c
>>   GENERATED_FILES += qapi/qapi-types-migration.h qapi/qapi-types-migration.c
>> +GENERATED_FILES += qapi/qapi-types-misc.h qapi/qapi-types-misc.c
>>   GENERATED_FILES += qapi/qapi-types-net.h qapi/qapi-types-net.c
>>   GENERATED_FILES += qapi/qapi-types-rocker.h qapi/qapi-types-rocker.c
>>   GENERATED_FILES += qapi/qapi-types-run-state.h qapi/qapi-types-run-state.c
>> @@ -116,6 +117,7 @@ GENERATED_FILES += qapi/qapi-visit-common.h qapi/qapi-visit-common.c
>>   GENERATED_FILES += qapi/qapi-visit-crypto.h qapi/qapi-visit-crypto.c
>>   GENERATED_FILES += qapi/qapi-visit-introspect.h qapi/qapi-visit-introspect.c
>>   GENERATED_FILES += qapi/qapi-visit-migration.h qapi/qapi-visit-migration.c
>> +GENERATED_FILES += qapi/qapi-visit-misc.h qapi/qapi-visit-misc.c
>
> And again, my comments about using intermediate variables and make
> text substitution would result in less duplication here.
>
>> +++ b/Makefile.objs
>> @@ -11,6 +11,7 @@ util-obj-y += qapi/qapi-types-common.o
>>   util-obj-y += qapi/qapi-types-crypto.o
>>   util-obj-y += qapi/qapi-types-introspect.o
>>   util-obj-y += qapi/qapi-types-migration.o
>> +util-obj-y += qapi/qapi-types-misc.o
>
> and possibly here too.
>
>> +++ b/hmp.c
>> @@ -23,13 +23,21 @@
>>   #include "qemu/config-file.h"
>>   #include "qemu/option.h"
>>   #include "qemu/timer.h"
>> -#include "qmp-commands.h"
>>   #include "qemu/sockets.h"
>>   #include "monitor/monitor.h"
>>   #include "monitor/qdev.h"
>>   #include "qapi/error.h"
>>   #include "qapi/opts-visitor.h"
>>   #include "qapi-builtin-visit.h"
>> +#include "qapi/qapi-commands-block.h"
>> +#include "qapi/qapi-commands-char.h"
>> +#include "qapi/qapi-commands-migration.h"
>> +#include "qapi/qapi-commands-misc.h"
>> +#include "qapi/qapi-commands-net.h"
>> +#include "qapi/qapi-commands-rocker.h"
>> +#include "qapi/qapi-commands-run-state.h"
>> +#include "qapi/qapi-commands-tpm.h"
>> +#include "qapi/qapi-commands-ui.h"
>
> Well, not every file gets a smaller list of includes ;)
>
>> +++ b/qapi-schema.json
>> @@ -92,3100 +92,4 @@
>>   { 'include': 'qapi/transaction.json' }
>>   { 'include': 'qapi/trace.json' }
>>   { 'include': 'qapi/introspect.json' }
>> -
>> -##
>> -# = Miscellanea
>> -##
>
>> -# Since: 2.11
>> -##
>> -{ 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
>> +{ 'include': 'qapi/misc.json' }
>
> [1] Conflict magnet. We'd better be prepared for some rebasing to get
> this patch series through.  See what the last item is here...
>
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> new file mode 100644
>> index 0000000000..225631bf7d
>> --- /dev/null
>> +++ b/qapi/misc.json
>> @@ -0,0 +1,3090 @@
>> +# -*- Mode: Python -*-
>> +#
>> +
>
> Should we be thinking about copyright/license statements in the QAPI
> submodule files?  But that's a topic for another series.
>
>
>> +# Since: 2.9
>> +##
>> +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
>> diff --git a/qapi/run-state.json b/qapi/run-state.json
>
> [1] ...and oops - you already hit one of those
> conflicts. watchdog-set-action disappeared in the move.  Or did it?
> [2]
>
>> index bca46a8785..a27c3c2a93 100644
>> --- a/qapi/run-state.json
>> +++ b/qapi/run-state.json
>> @@ -283,6 +283,16 @@
>>     'data': [ 'reset', 'shutdown', 'poweroff', 'pause', 'debug', 'none',
>>               'inject-nmi' ] }
>>   +
>> +##
>> +# @watchdog-set-action:
>> +#
>> +# Set watchdog action
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
>> +
>
> [2] Oh, you MOVED it to a different file.  Let's fix that separately,
> so that THIS patch can be just 1-1 file motion, not 1-N (making
> rebasing easier).

Okay.

>>   ##
>>   # @GUEST_PANICKED:
>>   #
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 846238175f..b8f6bc3f7e 100644
>> --- a/qdev-monitor.c
>
> Looks reasonable, but not quite ready for my R-b until v3.