Marc-André Lureau <marcandre.lur...@gmail.com> writes: > Hi > > On Thu, Aug 17, 2017 at 3:55 PM, Markus Armbruster <arm...@redhat.com> wrote: >> Marc-André Lureau <marcandre.lur...@redhat.com> 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
