Markus Armbruster <arm...@redhat.com> writes: > Marc-André Lureau <marcandre.lur...@gmail.com> writes: > >> Hi >> >> On Tue, Sep 6, 2016 at 7:58 PM Markus Armbruster <arm...@redhat.com> wrote: >> >>> QAPI language design issues, copying Eric. >>> >>> Marc-André Lureau <marcandre.lur...@gmail.com> writes: >>> >>> > Hi >>> > >>> > On Tue, Sep 6, 2016 at 5:00 PM Markus Armbruster <arm...@redhat.com> >>> 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 }