Re: [Qemu-devel] [PATCH v2 22/29] qapi: Generate separate .h, .c for each module

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/11/2018 03:36 AM, Markus Armbruster wrote:
>> Our qapi-schema.json is composed of modules connected by include
>> directives, but the generated code is monolithic all the same: one
>> qapi-types.h with all the types, one qapi-visit.h with all the
>> visitors, and so forth.  These monolithic headers get included all
>> over the place.  In my "build everything" tree, adding a QAPI type
>> recompiles about 4800 out of 5100 objects.
>>
>> We wouldn't write such monolithic headers by hand.  It stands to
>> reason that we shouldn't generate them, either.
>>
>> Split up generated qapi-types.h to mirror the schema's modular
>> structure: one header per module.  Name the main module's header
>> qapi-types.h, and sub-module D/B.json's header D/qapi-types-B.h.
>>
>> Mirror the schema's includes in the headers, so that qapi-types.h gets
>> you everything exactly as before.  If you need less, you can include
>> one or more of the sub-module headers.  To be exploited shortly.
>>
>> Split up qapi-types.c, qapi-visit.h, qapi-visit.c, qmp-commands.h,
>> qmp-commands.c, qapi-event.h, qapi-event.c the same way.
>> qmp-introspect.h, qmp-introspect.c and qapi.texi remain monolithic.
>
> Make sense.
>
>>
>> The split of qmp-commands.c duplicates static helper function
>> qmp_marshal_output_str() in qapi-commands-char.c and
>> qapi-commands-misc.c.  This happens when commands returning the same
>> type occur in multiple modules.  Not worth avoiding.
>
> As long as it is static, and neither .c file includes the other, we're
> fine (gdb may have a harder time figuring out which copy to put a
> breakpoint on, but that's not too terrible).
>
>>
>> Since I'm going to rename qapi-event.[ch] to qapi-events.[ch], and
>> qmp-commands.[ch] to qapi-commands.[ch], name the shards that way
>> already, to reduce churn.  This requires temporary hacks in
>> commands.py and events.py.  They'll go away with the rename.
>
> The planned rename makes sense; prepping now is fine.
>
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   .gitignore               |  60 ++++++++++++++++++++++++
>>   Makefile                 | 120 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   Makefile.objs            |  65 ++++++++++++++++++++++++-
>>   scripts/qapi/commands.py |  35 +++++++++-----
>>   scripts/qapi/common.py   |  21 +++++++--
>>   scripts/qapi/events.py   |  19 ++++++--
>>   6 files changed, 300 insertions(+), 20 deletions(-)
>>
>> diff --git a/.gitignore b/.gitignore
>> index 9477a08b6b..42c57998fd 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -31,7 +31,67 @@
>>   /qapi-gen-timestamp
>>   /qapi-builtin-types.[ch]
>>   /qapi-builtin-visit.[ch]
>> +/qapi/qapi-commands-block-core.[ch]
>> +/qapi/qapi-commands-block.[ch]
>
> Is it worth using a glob instead of specific patterns, as in:
>
> /qapi/qapi-commands-*.[ch]
>
> Maybe being precise doesn't hurt, if we aren't adding sub-modules all
> that often; but being generous with a glob makes it less likely that a
> future module addition will forget to update .gitignore.

We should simply ditch building in-tree.  I wanted to post patches for
that for quite some time.

>> +++ b/Makefile
>> @@ -92,10 +92,70 @@ include $(SRC_PATH)/rules.mak
>>   GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
>>   GENERATED_FILES += qapi-builtin-types.h qapi-builtin-types.c
>>   GENERATED_FILES += qapi-types.h qapi-types.c
>> +GENERATED_FILES += qapi/qapi-types-block-core.h qapi/qapi-types-block-core.c
>
> For the makefile, I'd suggest using an intermediate list of module
> names, and then using make string operations to expand it into larger
> lists, to cut down on the repetition.  Something like (untested):
>
> QAPI_MODULES = block-core block char common ...
> GENERATED_FILES += $(addprefix qapi/qapi-types-,$(addsuffix
> .c,$(QAPI_MODULES))
> GENERATED_FILES += $(addprefix qapi/qapi-types-,$(addsuffix
> .h,$(QAPI_MODULES))

I did it the stupid way to save time.  Improvements welcome :)

>> @@ -525,10 +585,70 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json 
>> $(SRC_PATH)/qapi/common.json \
>>     qapi-builtin-types.c qapi-builtin-types.h \
>>   qapi-types.c qapi-types.h \
>> +qapi/qapi-types-block-core.c qapi/qapi-types-block-core.h \
>> +qapi/qapi-types-block.c qapi/qapi-types-block.h \
>
> And repeating the list of filenames; again, an intermediate variable
> would let you do:
>
> QAPI_GENERATED_FILES = ...
> GENERATED_FILES += $(QAPI_GENERATED_FILES)
>
> $(QAPI_GENERATED_FILES): ...
>
>>   qmp-introspect.h qmp-introspect.c \
>>   qapi-doc.texi: \
>>   qapi-gen-timestamp ;
>
> Not only does it cut down on the repetition, but it will make adding a
> future module easier.
>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 2813e984fd..7a55d45669 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -3,8 +3,56 @@
>>   stub-obj-y = stubs/ crypto/
>>   util-obj-y = util/ qobject/ qapi/
>>   util-obj-y += qapi-builtin-types.o
>> +util-obj-y += qapi-types.o
>> +util-obj-y += qapi/qapi-types-block-core.o
>
> Perhaps this can also exploit make text manipulation?
>
> Otherwise looks good.

Thanks!