Re: [Qemu-devel] [PATCH v3 00/50] Hi,

[Markus Armbruster]

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

> 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.
>
> 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(). The main difference with v2, is
> the addition of a per-target schema in "build-sys: add a target
> schema". This removes the NEED_CPU_H hack, and keep most of the qapi
> files in common build.
>
> 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.
>
> Comments welcome,
>
> v3:
> - rebased (qlit is now merged upstream)
> - solve the per-target #ifdef problem by using a target.json
>   and new qapi generated target files
> - update some commit messages based on Markus review
> - more schema error reporting
> - move the ifcond argument closer to info/doc
> - use mcgen() in gen_if()/gen_endif()
> - simplify "modify to_qlit() to take an optional suffix"
> - fix generated qlit indentation
> - fix temporary build break by merging #if types & visitors patch
> - fix some redundant condtionals generation
> - change enum visitor to take QAPISchemaMember
> - reject unknown dictionnary keys in { .., 'if': ..}
> - split qapi test visitor print() with trailing ',' trick

Things I like:

* How you add conditionals to the QAPI language

* How the generated code changes

Things I don't like so much:

* A few commit messages are just too terse

* Code and its test added in separate patches

* ifcond_decorator

  I guess can either accept it, or come up with a better solution.

* Unconventional handling of sugared forms (see my review of PATCH 28).

* Addressing complilation consistency problems (see my review of PATCH
  17) before addressing them by splitting off target-specific generated
  files (PATCH 39..)

* Manual splitting of the generated monolith with pragma unit, -i and
  -u.  I guess can either accept it, or come up with a better solution.

Additionally, there are couple of minor things here and there, and a few
questions.  The usual patch review business.

Overall, there's much more for me to like than to dislike :)