Re: [PATCH 3/6] qapi: Generate command registration stuff into separate files

From: Markus Armbruster <armbru@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org, kwolf@pond.sub.org, mdroth@linux.vnet.ibm.com
Subject: Re: [PATCH 3/6] qapi: Generate command registration stuff into separate files
Date: Wed, 27 Nov 2019 17:12:23 +0100	[thread overview]
Message-ID: <87sgm9ti1k.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <d9fc4c2a-61d0-43b9-e792-35f5f5d39798@redhat.com> (Eric Blake's message of "Wed, 20 Nov 2019 13:26:10 -0600")

Eric Blake <eblake@redhat.com> writes:

> On 11/20/19 12:25 PM, Markus Armbruster wrote:
>> Having to include qapi-commands.h just for qmp_init_marshal() is
>> suboptimal.  Generate it into separate files.  This lets
>> monitor/misc.c, qga/main.c, and the generated qapi-commands-FOO.h
>> include less.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/docs/devel/qapi-code-gen.txt
>> @@ -1493,6 +1493,10 @@ $(prefix)qapi-commands.c: Command marshal/dispatch functions for each
>>   $(prefix)qapi-commands.h: Function prototypes for the QMP commands
>>                             specified in the schema
>>   +$(prefix)qapi-init-commands.h - Command initialization prototype
>> +
>> +$(prefix)qapi-init-commands.h - Command initialization code
>
> Looks like you meant s/h/c/

I did.  Thanks!

>> +    #endif /* EXAMPLE_QAPI_INIT_COMMANDS_H */
>> +    $ cat qapi-generated/example-qapi-init-commands.
>> +[Uninteresting stuff omitted...]
>
> missing a 'c'

Yes.

>> +++ b/Makefile
>
>>   -QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h
>> qga-qapi-visit.h qga-qapi-commands.h)
>> +QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qapi-commands.h qga-qapi-init-commands.h)
>
> Worth using \ for line-wrapping?

Hmm, it's used only in the following line, could just as well inline
there and line-wrap that:

    $(qga-obj-y): $(QGALIB_GEN)

Hmm^2, why do we even have that line?  The compiler-generated .d should
take care of dependencies.  Dig, dig, ...

    commit 016c77ad62a8ad607dd4349d8cb8ad1365bab831
    Author: Michael Roth <mdroth@linux.vnet.ibm.com>
    Date:   Tue Jul 26 11:39:24 2011 -0500

        Makefile: add missing deps on $(GENERATED_HEADERS)

        This fixes a build issue with make -j6+ due to qapi-generated files
        being built before $(GENERATED_HEADERS) have been created.

        Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
        Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
        Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Yes, but the same issue exists for all the other generated headers, and
we solve it differently: put them into $(generated-files-y), have
Makefile depend on them.

I'll clean this up on top.  No need to beautify lines now that will go
away then.

> With those addressed,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!