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