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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Marc-André Lureau <address@hidden> writes:
>
>> Hi
>>
>> On Tue, Sep 6, 2016 at 7:58 PM Markus Armbruster <address@hidden> wrote:
>>
>>> QAPI language design issues, copying Eric.
>>>
>>> Marc-André Lureau <address@hidden> writes:
>>>
>>> > Hi
>>> >
>>> > On Tue, Sep 6, 2016 at 5:00 PM Markus Armbruster <address@hidden>
>>> wrote:
[...]
>>> >> Yet another option is to add 'ifdef' keys to the definitions
>>> >> themselves, i.e.
>>> >>
>>> >>     { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>>> >>       'ifdef': 'TARGET_ARM' }
>>> >>
>>> >
>>> > That's the worst of all options imho, as it makes it hard to filter out
>>> > unions/enums, ex:
>>> >
>>> >  @ -3446,8 +3466,10 @@
>>> >                                         'testdev': 'ChardevCommon',
>>> >                                         'stdio'  : 'ChardevStdio',
>>> >                                         'console': 'ChardevCommon',
>>> > +#ifdef CONFIG_SPICE
>>> >                                         'spicevmc' :
>>> 'ChardevSpiceChannel',
>>> >                                         'spiceport' : 'ChardevSpicePort',
>>> > +#endif
>>> >                                         'vc'     : 'ChardevVC',
>>> >                                         'ringbuf': 'ChardevRingbuf',
>>>
>>> Point taken.
>>>
>>> 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.
>
> 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.
>
> The "can't specify additional stuff for struct members" problem isn't
> new.  We hacked around it to specify "optional": encode it into the
> member name.  Doesn't scale.  We need to solve the problem to be able to
> specify default values, and we already decided how: have an JSON object
> instead of a mere JSON string, make the string syntax sugar for {
> 'type': STRING }.  See commit 6b5abc7 and the discussion in qemu-devel
> leading up to it.  For consistency, we'll do it for union and alternate
> members, too.
>
> That leaves just enumeration members.  The same technique applies.
>
> If I remember correctly, we only need conditional commands right now, to
> avoid regressing query-commands.  The more complicated member work could
> be done later.

To gauge whether this idea is practical, I implemented key 'if' for
commands.  It's just a sketch, and has a number of issues, which I
marked FIXME.

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.

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

Anyway, diffstat isn't bad:

 docs/qapi-code-gen.txt                     | 14 ++++++-----
 qapi-schema.json                           | 15 ++++++++---
 qapi/introspect.json                       |  2 +-
 scripts/qapi-commands.py                   | 12 +++++++--
 scripts/qapi-introspect.py                 | 22 ++++++++++------
 scripts/qapi.py                            | 40 ++++++++++++++++++++++--------
 tests/qapi-schema/type-bypass-bad-gen.err  |  2 +-
 tests/qapi-schema/type-bypass-bad-gen.json |  4 +--
 8 files changed, 77 insertions(+), 34 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index de298dc..93e99d8 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -423,7 +423,7 @@ part of a Client JSON Protocol command.  The 'data' member 
is optional
 and defaults to {} (an empty dictionary).  If present, it must be the
 string name of a complex type, or a dictionary that declares an
 anonymous type with the same semantics as a 'struct' expression, with
-one exception noted below when 'gen' is used.
+one exception noted below when 'if': false is used.
 
 The 'returns' member describes what will appear in the "return" member
 of a Client JSON Protocol reply on successful completion of a command.
@@ -431,8 +431,8 @@ The member is optional from the command declaration; if 
absent, the
 "return" member will be an empty dictionary.  If 'returns' is present,
 it must be the string name of a complex or built-in type, a
 one-element array containing the name of a complex or built-in type,
-with one exception noted below when 'gen' is used.  Although it is
-permitted to have the 'returns' member name a built-in type or an
+with one exception noted below when 'if':false is used.  Although it
+is permitted to have the 'returns' member name a built-in type or an
 array of built-in types, any command that does this cannot be extended
 to return additional information in the future; thus, new commands
 should strongly consider returning a dictionary-based type or an array
@@ -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'
+
 In rare cases, QAPI cannot express a type-safe representation of a
 corresponding Client JSON Protocol command.  You then have to suppress
-generation of a marshalling function by including a key 'gen' with
+generation of a marshalling function by including a key 'if' with
 boolean value false, and instead write your own function.  Please try
 to avoid adding new commands that rely on this, and instead use
 type-safe unions.  For an example of this usage:
 
  { 'command': 'netdev_add',
-   'data': {'type': 'str', 'id': 'str'},
-   'gen': false }
+   'if': false,
+   'data': {'type': 'str', 'id': 'str'} }
 
 Normally, the QAPI schema is used to describe synchronous exchanges,
 where a response is expected.  But in some cases, the action of a
diff --git a/qapi-schema.json b/qapi-schema.json
index c4f3674..ad0559e 100644
--- a/qapi-schema.json
+++ 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' }
 
 ##
 # @BalloonInfo:
@@ -2355,6 +2357,7 @@
 # Since: 2.5
 ##
 { 'command': 'dump-skeys',
+#  'if': 'defined(TARGET_S390X)',
   'data': { 'filename': 'str' } }
 
 ##
@@ -2380,8 +2383,8 @@
 #          If @type is not a valid network backend, DeviceNotFound
 ##
 { 'command': 'netdev_add',
-  'data': {'type': 'str', 'id': 'str'},
-  'gen': false }                # so we can get the additional arguments
+  'if': false,                  # so we can get the additional arguments
+  'data': {'type': 'str', 'id': 'str'} }
 
 ##
 # @netdev_del:
@@ -4455,6 +4458,7 @@
 # Since: 2.1
 ##
 { 'command': 'rtc-reset-reinjection' }
+#  'if': 'defined(TARGET_I386)'
 
 # Rocker ethernet network switch
 { 'include': 'qapi/rocker.json' }
@@ -4525,7 +4529,10 @@
 #
 # Since: 2.6
 ##
-{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
+{ 'command': 'query-gic-capabilities',
+#  'if': 'defined(TARGET_ARM)',
+  'returns': ['GICCapability']
+}
 
 ##
 # CpuInstanceProperties
diff --git a/qapi/introspect.json b/qapi/introspect.json
index 3fd81fb..b8f421a 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -46,7 +46,7 @@
 ##
 { 'command': 'query-qmp-schema',
   'returns': [ 'SchemaInfo' ],
-  'gen': false }                # just to simplify qmp_query_json()
+  'if': false }                 # just to simplify qmp_query_json()
 
 ##
 # @SchemaMetaType
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index a06a2c4..f34e4cc 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -215,9 +215,13 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
         self._visited_ret_types = None
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
-        if not gen:
+                      genif, success_response, boxed):
+        if genif is False:
             return
+        pp_if = gen_pp_if(genif)
+        pp_endif = gen_pp_endif(genif)
+        self.decl += pp_if
+        self.defn += pp_if      # FIXME blank lines are off
         self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
         if ret_type and ret_type not in self._visited_ret_types:
             self._visited_ret_types.add(ret_type)
@@ -226,7 +230,11 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
             self.decl += gen_marshal_decl(name)
         self.defn += gen_marshal(name, arg_type, boxed, ret_type)
         if not middle_mode:
+            self._regy += pp_if
             self._regy += gen_register_command(name, success_response)
+            self._regy += pp_endif
+        self.decl += pp_endif
+        self.defn += pp_endif
 
 
 middle_mode = False
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index 541644e..0d8cec7 100644
--- a/scripts/qapi-introspect.py
+++ 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)
+        # FIXME trailing comma (JSON sucks)
+        c_string += '\n    "]"'
         self.defn = mcgen('''
 const char %(c_name)s[] = %(c_string)s;
 ''',
@@ -111,12 +116,12 @@ const char %(c_name)s[] = %(c_string)s;
             return '[' + self._use_type(typ.element_type) + ']'
         return self._name(typ.name)
 
-    def _gen_json(self, name, mtype, obj):
+    def _gen_json(self, name, mtype, obj, genif=True):
         if mtype not in ('command', 'event', 'builtin', 'array'):
             name = self._name(name)
         obj['name'] = name
         obj['meta-type'] = mtype
-        self._jsons.append(obj)
+        self._jsons.append((obj, genif))
 
     def _gen_member(self, member):
         ret = {'name': member.name, 'type': self._use_type(member.type)}
@@ -154,12 +159,13 @@ const char %(c_name)s[] = %(c_string)s;
                                     for m in variants.variants]})
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      genif, success_response, boxed):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         self._gen_json(name, 'command',
                        {'arg-type': self._use_type(arg_type),
-                        'ret-type': self._use_type(ret_type)})
+                        'ret-type': self._use_type(ret_type)},
+                       genif)
 
     def visit_event(self, name, info, arg_type, boxed):
         arg_type = arg_type or self._schema.the_empty_object_type
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 21bc32f..6c0cf9f 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -698,7 +698,13 @@ def check_keys(expr_elem, meta, required, optional=[]):
             raise QAPIExprError(info,
                                 "Unknown key '%s' in %s '%s'"
                                 % (key, meta, name))
-        if (key == 'gen' or key == 'success-response') and value is not False:
+        if (key == 'if'
+            and value is not False and not isinstance(value, str)):
+            # FIXME update error message
+            raise QAPIExprError(info,
+                                "'%s' of %s '%s' should only use false value"
+                                % (key, meta, name))
+        if (key == 'success-response') and value is not False:
             raise QAPIExprError(info,
                                 "'%s' of %s '%s' should only use false value"
                                 % (key, meta, name))
@@ -737,7 +743,7 @@ def check_exprs(exprs):
             add_struct(expr, info)
         elif 'command' in expr:
             check_keys(expr_elem, 'command', [],
-                       ['data', 'returns', 'gen', 'success-response', 'boxed'])
+                       ['data', 'returns', 'if', 'success-response', 'boxed'])
             add_name(expr['command'], info, 'command')
         elif 'event' in expr:
             check_keys(expr_elem, 'event', [], ['data', 'boxed'])
@@ -838,7 +844,7 @@ class QAPISchemaVisitor(object):
         pass
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      genif, success_response, boxed):
         pass
 
     def visit_event(self, name, info, arg_type, boxed):
@@ -1180,8 +1186,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
 
 
 class QAPISchemaCommand(QAPISchemaEntity):
-    def __init__(self, name, info, arg_type, ret_type, gen, success_response,
-                 boxed):
+    def __init__(self, name, info, arg_type, ret_type,
+                 genif, success_response, boxed):
         QAPISchemaEntity.__init__(self, name, info)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -1189,7 +1195,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         self.arg_type = None
         self._ret_type_name = ret_type
         self.ret_type = None
-        self.gen = gen
+        self.genif = genif
         self.success_response = success_response
         self.boxed = boxed
 
@@ -1216,7 +1222,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
     def visit(self, visitor):
         visitor.visit_command(self.name, self.info,
                               self.arg_type, self.ret_type,
-                              self.gen, self.success_response, self.boxed)
+                              self.genif, self.success_response, self.boxed)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1419,17 +1425,20 @@ class QAPISchema(object):
         name = expr['command']
         data = expr.get('data')
         rets = expr.get('returns')
-        gen = expr.get('gen', True)
+        genif = expr.get('if', True)
         success_response = expr.get('success-response', True)
         boxed = expr.get('boxed', False)
         if isinstance(data, OrderedDict):
+            # TODO apply genif to the implicit object type
             data = self._make_implicit_object_type(
                 name, info, 'arg', self._make_members(data, info))
         if isinstance(rets, list):
+            # TODO apply genif to the implicit array type
+            # TODO disjunction of all the genif
             assert len(rets) == 1
             rets = self._make_array_type(rets[0], info)
-        self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
-                                           success_response, boxed))
+        self._def_entity(QAPISchemaCommand(name, info, data, rets,
+                                           genif, success_response, boxed))
 
     def _def_event(self, expr, info):
         name = expr['event']
@@ -1704,6 +1713,17 @@ def gen_params(arg_type, boxed, extra):
     return ret
 
 
+def gen_pp_if(cond):
+    if cond is True:
+        return ''
+    return '\n#if ' + cond + '\n'
+
+
+def gen_pp_endif(cond):
+    if cond is True:
+        return ''
+    return '\n#endif  /* ' + cond + ' */\n'
+
 #
 # Common command line parsing
 #
diff --git a/tests/qapi-schema/type-bypass-bad-gen.err 
b/tests/qapi-schema/type-bypass-bad-gen.err
index a83c3c6..cca25f1 100644
--- a/tests/qapi-schema/type-bypass-bad-gen.err
+++ b/tests/qapi-schema/type-bypass-bad-gen.err
@@ -1 +1 @@
-tests/qapi-schema/type-bypass-bad-gen.json:2: 'gen' of command 'foo' should 
only use false value
+tests/qapi-schema/type-bypass-bad-gen.json:2: 'if' of command 'foo' should 
only use false value
diff --git a/tests/qapi-schema/type-bypass-bad-gen.json 
b/tests/qapi-schema/type-bypass-bad-gen.json
index e8dec34..637b11f 100644
--- a/tests/qapi-schema/type-bypass-bad-gen.json
+++ b/tests/qapi-schema/type-bypass-bad-gen.json
@@ -1,2 +1,2 @@
-# 'gen' should only appear with value false
-{ 'command': 'foo', 'gen': 'whatever' }
+# 'if' should only appear with value false FIXME or str
+{ 'command': 'foo', 'if': null }