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

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

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

A big pile of patches can look scary, but what really drags out review
is oversized non-trivial patches like PATCH 07.  I can take quick,
refreshing breaks much more easily between patches than within a big &
hairy one.

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

The way things work, QMP needs to be defined in a single schema.
Doesn't mean we have to generate monoliths from it.

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