Re: [Qemu-devel] [PATCH v5 02/20] qapi.py: add a simple #ifdef conditional

[Eric Blake]

On 09/07/2016 08:40 AM, Markus Armbruster wrote:
>>>>>> Yet another option is to add 'ifdef' keys to the definitions
>>>>>> themselves, i.e.
>>>>>>
>>>>>>     { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>>>>>>       'ifdef': 'TARGET_ARM' }

Seems like it might be useful; could even be expanded to:

{ 'command': 'query-arm-x86', 'returns': ['Foo'],
  'ifdef': [ 'TARGET_ARM', 'TARGET_X86' ] }

unless you are okay with it instead being:

{ 'command': 'query-arm-x86', 'returns': ['Foo'],
  'ifdef': 'TARGET_ARM || TARGET_X86' }

Either way, we need to make sure that whatever we put here is easy to
translate into the appropriate generated code; and keeping in mind that
while '#if A || B' makes sense to the preprocessor, '#ifdef A || B' does
not.

>>>> Fixable the same way as always when a definition needs to grow
>>>> additional properties, but the syntax only provides a single value: make
>>>> that single value an object, and the old non-object value syntactic
>>>> sugar for the equivalent object value.  We've previously discussed this
>>>> technique in the context of giving command arguments default values.
>>>> I'm not saying this is what we should do here, only pointing out it's
>>>> possible.
>>>>
>>>
>>> Ok, but let's find something, if possible simple and convenient, no?
>>
>> I agree it needs to be simple, both the interface (QAPI language) and
>> the implementation.  However, I don't like "first past the post".  I
>> prefer to explore the design space a bit.

Another nice thing about expanding it by changing 'name':'typestr' to be
sugar for 'name':{'type':'typestr'} is that QMP introspection is already
prepared to expose a dictionary of attributes (type being one, and
ifdef'ness being another) per member, so we already would have a nice
mapping between qapi JSON files (a member name tied to a dictionary) and
the generated output.

>>
>> So let me explore the "add 'ifdef' keys to definitions" corner of the
>> QAPI language design space.
>>
>> Easily done for top-level definitions, because they're all JSON objects.
>> Could even add it to the include directive if we wanted to.
>>
>> Less easily done for enumeration, struct, union and alternate members.
>> Note that command and event arguments specified inline are a special
>> case of struct members.
>>

and ideally, whatever we can do for a named struct, we can also do for
an anonymous inlined struct for the command and event arguments.

> 
> I ported qmp-commands.hx's #if to qapi-schema.json.  The TARGET_FOO are
> poisoned, so I commented them out.  There's a CONFIG_SPICE left, which
> will do for testing.

Obviously, we'd have to find a way to work around the poisoned ifdef
names, but the idea makes some sense to me.

> 
> I also turned key 'gen': false into 'if': false.  Possibly a bad idea.

Maybe, maybe not. We have so few that it's easy to convert them all in
one go, at which point you are just giving directives to the code
generator: if the 'if' key is present, it controls what gets omitted
(either the omitted code is gated by #if (or #ifdef) of the gate string,
or 'if':false means the generator omits the code altogether).

> @@ -475,16 +475,18 @@ arguments for the user's function out of an input 
> QDict, calls the
>  user's function, and if it succeeded, builds an output QObject from
>  its return value.
>  
> +FIXME document 'if'
> +

And in particular, what the string must look like (will it be fed to
generated #if or to generated #ifdef?  What if you have more than one
conditional?)

> +++ b/qapi-schema.json
> @@ -1269,7 +1269,9 @@
>  #
>  # Since: 0.14.0
>  ##
> -{ 'command': 'query-spice', 'returns': 'SpiceInfo' }
> +{ 'command': 'query-spice',
> +  'if': 'defined(CONFIG_SPICE)',
> +  'returns': 'SpiceInfo' }

So the idea here is that qemu built without SPICE support would
completely lack the query-spice command.  This is currently detectable
during 'query-commands' but NOT detectable during 'query-qmp-schema';
but after the patch, we could begin to make the introspection likewise
hide the unused command.  Sounds useful.

>  
>  ##
>  # @BalloonInfo:
> @@ -2355,6 +2357,7 @@
>  # Since: 2.5
>  ##
>  { 'command': 'dump-skeys',
> +#  'if': 'defined(TARGET_S390X)',
>    'data': { 'filename': 'str' } }

And here you ran into the poisoned variable problem.  Obviously
something to be solved if we like the overall idea.

> +++ b/scripts/qapi-introspect.py
> @@ -30,8 +30,6 @@ def to_json(obj, level=0):
>          ret = '{' + ', '.join(elts) + '}'
>      else:
>          assert False                # not implemented
> -    if level == 1:
> -        ret = '\n' + ret
>      return ret
>  
>  
> @@ -69,8 +67,15 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
>  extern const char %(c_name)s[];
>  ''',
>                            c_name=c_name(name))
> -        lines = to_json(jsons).split('\n')
> -        c_string = '\n    '.join([to_c_string(line) for line in lines])
> +        c_string = '"["'
> +        for i in jsons:
> +            js, genif = i
> +            # FIXME blank lines are off
> +            c_string += gen_pp_if(genif or True)
> +            c_string += '\n    ' + to_c_string(to_json(js) + ', ')
> +            c_string += gen_pp_endif(genif or True)

And this change to introspection output shows the true power - by having
the conditional expression be part of the .json QAPI file, we can now
reflect the conditions through ALL parts of the generated code, instead
of having discrepancies between query-commands vs. query-qmp-schema.

At any rate, for a quick day's hack, I like this approach for feeling
like it fits in with the QAPI language, rather than being bolted on as
additional non-JSON syntax.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature