Re: [Qemu-devel] [PATCH 00/26] qapi: add #if pre-processor conditions to generated code

[Marc-André Lureau]

Hi

On Thu, Aug 17, 2017 at 3:55 PM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> Hi,
>>
>> In order to clean-up some hacks in qapi (having to unregister commands
>> at runtime), I proposed a "[PATCH v5 02/20] qapi.py: add a simple #ifdef 
>> condition"
>>
>> (see http://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03106.html).
>>
>> However, we decided to drop that patch from the series and solve the
>> problem later. The main issues were:
>> - the syntax was awkward to the JSON schema and documentation
>> - the evaluation of the condition was done in the qapi scripts, with
>>   very limited capability
>> - each target/config would need different generated files.
>>
>> Instead, it could defer the #if evaluation to the C-preprocessor.
>>
>> With this series, top-level qapi JSON entity can take 'if' keys:
>>
>> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
>>   'if': 'defined(TEST_IF_STRUCT)' }
>>
>> Members can be exploded as dictionnary with 'type'/'if' keys:
>>
>> { 'struct': 'TestIfStruct', 'data':
>>   { 'foo': 'int',
>>     'bar': { 'type': 'int', 'if': 'defined(TEST_IF_STRUCT_BAR)'} } }
>>
>> Enum values can be exploded as dictionnary with 'type'/'if' keys:
>>
>> { 'enum': 'TestIfEnum', 'data':
>>   [ 'foo',
>>     { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ] }
>>
>> A good benefit from having conditional schema is that introspection
>> will reflect more accurately the capability of the server. Another
>> benefit is that it may help to remove some dead code when disabling a
>> functionality.
>
> This is the main benefit.  Until we realize it, introspection remains
> seriously hobbled.
>
> A few closing remarks.
>
> The general approach "generate the #if for the compiler to evaluate"
> looks sound.
>
> I haven't been able to fully review how it's integrated into the QAPI
> language and how the generators implement it.  I hope a bit of judicious
> patch splitting will help me over the hump.

I have done some patch splitting, that doubles the number of patches though ;)

>
> As so often, solving one problem makes other problems more visible.  In
> this case, the problem that we generate a monolith, and pay for that
> with compile time.  More of it once we compile the monolith per target.

Indeed, it would be nice to improve that soon after.

>
>> Starting from patch "qapi: add conditions to VNC type/commands/events
>> on the schema", the series demonstrate adding conditions, in order to
>> remove qmp_unregister_commands_hack(). However, it feels like I
>> cheated a little bit by using per-target NEED_CPU_H in the headers to
>> avoid #define poison. The alternative could be to split the headers in
>> common/target?
>
> Yup, we could really use a way to modularize the generated code.
>
> If your work leads us to ideas on how to crack the monolith, great.  If
> not, we'll have to decide whether we can live with the compile time hit.
> I'd rather not block your work on cracking the monolith.

I think we could start by splitting the json schemas.

>
>> There are a lot more things we could make conditional in the QAPI
>> schema, like pci/kvm/xen/numa/vde/slirp/posix/win32/vsock/lzo etc etc,
>> however I am still evaluating the implication of such changes both
>> externally and internally, for those interested, I can share my wip
>> branch.
>
> You provide the infrastructure and useful examples of use.  Good enough,
> no need to hunt down *all* uses right away.
>

I am sending a new version,

thanks

-- 
Marc-André Lureau