From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49932) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhd5z-00020U-QU for qemu-devel@nongnu.org; Wed, 07 Sep 2016 09:40:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhd5u-0000Wr-P8 for qemu-devel@nongnu.org; Wed, 07 Sep 2016 09:40:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44944) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhd5u-0000Wf-GB for qemu-devel@nongnu.org; Wed, 07 Sep 2016 09:40:50 -0400 From: Markus Armbruster References: <20160817165757.23486-1-marcandre.lureau@redhat.com> <20160817165757.23486-3-marcandre.lureau@redhat.com> <87fupd5hpi.fsf@dusky.pond.sub.org> <87vay9uijd.fsf@dusky.pond.sub.org> <87d1kgjdzh.fsf@dusky.pond.sub.org> Date: Wed, 07 Sep 2016 15:40:46 +0200 In-Reply-To: <87d1kgjdzh.fsf@dusky.pond.sub.org> (Markus Armbruster's message of "Wed, 07 Sep 2016 10:44:34 +0200") Message-ID: <87zinjde01.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 02/20] qapi.py: add a simple #ifdef conditional List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: qemu-devel@nongnu.org, Eric Blake Markus Armbruster writes: > Marc-Andr=C3=A9 Lureau writes: > >> Hi >> >> On Tue, Sep 6, 2016 at 7:58 PM Markus Armbruster wro= te: >> >>> QAPI language design issues, copying Eric. >>> >>> Marc-Andr=C3=A9 Lureau writes: >>> >>> > Hi >>> > >>> > On Tue, Sep 6, 2016 at 5:00 PM Markus Armbruster >>> wrote: [...] >>> >> Yet another option is to add 'ifdef' keys to the definitions >>> >> themselves, i.e. >>> >> >>> >> { 'command': 'query-gic-capabilities', 'returns': ['GICCapabilit= y'], >>> >> 'ifdef': 'TARGET_ARM' } >>> >> >>> > >>> > That's the worst of all options imho, as it makes it hard to filter o= ut >>> > unions/enums, ex: >>> > >>> > @ -3446,8 +3466,10 @@ >>> > 'testdev': 'ChardevCommon', >>> > 'stdio' : 'ChardevStdio', >>> > 'console': 'ChardevCommon', >>> > +#ifdef CONFIG_SPICE >>> > 'spicevmc' : >>> 'ChardevSpiceChannel', >>> > 'spiceport' : 'ChardevSpicePo= rt', >>> > +#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' mem= ber 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. =20 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 QDi= ct, calls the user's function, and if it succeeded, builds an output QObject from its return value. =20 +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: =20 { 'command': 'netdev_add', - 'data': {'type': 'str', 'id': 'str'}, - 'gen': false } + 'if': false, + 'data': {'type': 'str', 'id': 'str'} } =20 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' } =20 ## # @BalloonInfo: @@ -2355,6 +2357,7 @@ # Since: 2.5 ## { 'command': 'dump-skeys', +# 'if': 'defined(TARGET_S390X)', 'data': { 'filename': 'str' } } =20 ## @@ -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'} } =20 ## # @netdev_del: @@ -4455,6 +4458,7 @@ # Since: 2.1 ## { 'command': 'rtc-reset-reinjection' } +# 'if': 'defined(TARGET_I386)' =20 # 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'] +} =20 ## # 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() =20 ## # @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 =3D None =20 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 =3D gen_pp_if(genif) + pp_endif =3D gen_pp_endif(genif) + self.decl +=3D pp_if + self.defn +=3D pp_if # FIXME blank lines are off self.decl +=3D 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 +=3D gen_marshal_decl(name) self.defn +=3D gen_marshal(name, arg_type, boxed, ret_type) if not middle_mode: + self._regy +=3D pp_if self._regy +=3D gen_register_command(name, success_response) + self._regy +=3D pp_endif + self.decl +=3D pp_endif + self.defn +=3D pp_endif =20 =20 middle_mode =3D 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=3D0): ret =3D '{' + ', '.join(elts) + '}' else: assert False # not implemented - if level =3D=3D 1: - ret =3D '\n' + ret return ret =20 =20 @@ -69,8 +67,15 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor): extern const char %(c_name)s[]; ''', c_name=3Dc_name(name)) - lines =3D to_json(jsons).split('\n') - c_string =3D '\n '.join([to_c_string(line) for line in lines]) + c_string =3D '"["' + for i in jsons: + js, genif =3D i + # FIXME blank lines are off + c_string +=3D gen_pp_if(genif or True) + c_string +=3D '\n ' + to_c_string(to_json(js) + ', ') + c_string +=3D gen_pp_endif(genif or True) + # FIXME trailing comma (JSON sucks) + c_string +=3D '\n "]"' self.defn =3D mcgen(''' const char %(c_name)s[] =3D %(c_string)s; ''', @@ -111,12 +116,12 @@ const char %(c_name)s[] =3D %(c_string)s; return '[' + self._use_type(typ.element_type) + ']' return self._name(typ.name) =20 - def _gen_json(self, name, mtype, obj): + def _gen_json(self, name, mtype, obj, genif=3DTrue): if mtype not in ('command', 'event', 'builtin', 'array'): name =3D self._name(name) obj['name'] =3D name obj['meta-type'] =3D mtype - self._jsons.append(obj) + self._jsons.append((obj, genif)) =20 def _gen_member(self, member): ret =3D {'name': member.name, 'type': self._use_type(member.type)} @@ -154,12 +159,13 @@ const char %(c_name)s[] =3D %(c_string)s; for m in variants.variants]}) =20 def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + genif, success_response, boxed): arg_type =3D arg_type or self._schema.the_empty_object_type ret_type =3D 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) =20 def visit_event(self, name, info, arg_type, boxed): arg_type =3D 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=3D[= ]): raise QAPIExprError(info, "Unknown key '%s' in %s '%s'" % (key, meta, name)) - if (key =3D=3D 'gen' or key =3D=3D 'success-response') and value i= s not False: + if (key =3D=3D '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 val= ue" + % (key, meta, name)) + if (key =3D=3D 'success-response') and value is not False: raise QAPIExprError(info, "'%s' of %s '%s' should only use false val= ue" % (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', 'box= ed']) + ['data', 'returns', 'if', 'success-response', 'boxe= d']) 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 =20 def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + genif, success_response, boxed): pass =20 def visit_event(self, name, info, arg_type, boxed): @@ -1180,8 +1186,8 @@ class QAPISchemaAlternateType(QAPISchemaType): =20 =20 class QAPISchemaCommand(QAPISchemaEntity): - def __init__(self, name, info, arg_type, ret_type, gen, success_respon= se, - 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 =3D None self._ret_type_name =3D ret_type self.ret_type =3D None - self.gen =3D gen + self.genif =3D genif self.success_response =3D success_response self.boxed =3D boxed =20 @@ -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.boxe= d) =20 =20 class QAPISchemaEvent(QAPISchemaEntity): @@ -1419,17 +1425,20 @@ class QAPISchema(object): name =3D expr['command'] data =3D expr.get('data') rets =3D expr.get('returns') - gen =3D expr.get('gen', True) + genif =3D expr.get('if', True) success_response =3D expr.get('success-response', True) boxed =3D expr.get('boxed', False) if isinstance(data, OrderedDict): + # TODO apply genif to the implicit object type data =3D 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) =3D=3D 1 rets =3D 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)) =20 def _def_event(self, expr, info): name =3D expr['event'] @@ -1704,6 +1713,17 @@ def gen_params(arg_type, boxed, extra): return ret =20 =20 +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' shoul= d 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 }