Marc-André Lureau <marcandre.lur...@redhat.com> 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 :)