From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50407) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxHg5-0002YH-Nd for qemu-devel@nongnu.org; Thu, 11 Jul 2013 10:17:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UxHg4-0002VL-0a for qemu-devel@nongnu.org; Thu, 11 Jul 2013 10:17:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54639) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxHg3-0002VD-PA for qemu-devel@nongnu.org; Thu, 11 Jul 2013 10:16:59 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6BEGvSZ017898 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 11 Jul 2013 10:16:58 -0400 Message-ID: <51DEBE58.5000009@redhat.com> Date: Thu, 11 Jul 2013 08:16:56 -0600 From: Eric Blake MIME-Version: 1.0 References: <1373363617-4723-1-git-send-email-kwolf@redhat.com> <1373363617-4723-7-git-send-email-kwolf@redhat.com> In-Reply-To: <1373363617-4723-7-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2MQGTQHQAIPMFNLLODKAH" Subject: Re: [Qemu-devel] [RFC PATCH 06/11] qapi: Flat unions with arbitrary discriminator List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: armbru@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2MQGTQHQAIPMFNLLODKAH Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/09/2013 03:53 AM, Kevin Wolf wrote: > Instead of the rather verbose syntax that distinguishes base and > subclass fields... >=20 > { "type": "file", > "read-only": true, > "data": { > "filename": "test" > } } >=20 > ...we can now have both in the same namespace, allowing a more direct > mapping of the command line, and moving fields between the common base > and subclasses without breaking the API: >=20 > { "driver": "file", > "read-only": true, > "filename": "test" } [prior to reading the patch body] I assume that the corresponding qapi-schema.json change that matches the above wire context is: { 'type': 'Base', 'data': { 'read-only': 'bool' } } { 'type': 'File', 'data': { 'filename': 'str' } } { 'union': 'Example', 'base': 'Base', 'discriminator': 'driver', 'data': { 'file': 'File' } } with the new 'discriminator' element stating that an alternate name (rather than the default 'type' name) is in use. Do you allow flattened unions always, or only when 'discriminator' was present? Aren't you really flattening TWO structs? That is, extending the above, we are going from: { 'command': 'test', 'data': { 'driver': 'Example' } } which is previously called as: { "execute": "test", "arguments": { "driver": { "type": "file", "read-only": true, "data": { "filename": "test" } } } } to the shorter: { "execute": "test", "arguments": { "driver": "file", "read-only": true, "filename": "test" } } [...now after reading the patch body] Oh - you ONLY support a discriminator IF you pass a 'base' class, where the discriminator has to be one of the fields named in the base class, necessarily of type 'str'. In other words, the qapi-schema.json needs to look more like: { 'type': 'Base', 'data': { 'read-only': 'bool', 'driver': 'str' } } { 'type': 'File', 'data': { 'filename': 'str' } } { 'union': 'Example', 'base': 'Base', 'discriminator': 'driver', 'data': { 'file': 'File' } } If I'm off (or even if I'm right), more details in the commit message about the qapi description that leads to the final QMP wire contents would be helpful. >=20 > Signed-off-by: Kevin Wolf > --- > scripts/qapi-types.py | 10 +++--- > scripts/qapi-visit.py | 91 ++++++++++++++++++++++++++++++++++++-------= -------- > 2 files changed, 70 insertions(+), 31 deletions(-) >=20 > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 960065b..db2f533 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -162,10 +162,8 @@ def generate_union(expr): > name =3D expr['union'] > typeinfo =3D expr['data'] > =20 > - if expr.has_key('base'): > - base =3D expr['base'] > - else: > - base =3D None > + base =3D expr.get('base') Should this simplification be squashed into patch 2/11? (Patch 4/11 has a similar construct that might also be simplified.) > + discriminator =3D expr.get('discriminator') > =20 > ret =3D mcgen(''' > struct %(name)s > @@ -189,7 +187,11 @@ struct %(name)s > =20 > if base: > struct =3D find_struct(base) > + if discriminator: > + del struct['data'][discriminator] Here my lack of python expertise shows, in what may be a dumb question: Does find_struct(base) return a reference to the original object (oops - modifying 'struct' here means that the next call to find_struct(base) sees a smaller object), or a fully-cloned object (phew - our deletion is to our local copy, but future callers get a fresh clone that still see the discriminator)? Meanwhile, should you assert that the discriminator is of type 'str'? Actually, I suppose it might be nice to support a discriminator of an enum type (to document the set of valid strings); but the point still remains that the discriminator field is probably not going to work as a bool, integer, or struct type. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2MQGTQHQAIPMFNLLODKAH Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJR3r5YAAoJEKeha0olJ0Nqh6gH/3+0NMYorkbAGyuBkDWVe6b9 beoj7KxOYLsmSwMnuJrwtrWwHroi5L1AA7LvYsCa2iJqWgs2A9yNBDfy6158jlrc GItZQ3E1/b/vLz6QBgkhfHVAWOviDt9NA3JjCYBDZeWcJW4F9XXyRvH7oDwwuR6y UKB/r+rpPQMtEVUVJSttdZ1Amk2Cina/lzIsk2X1FTFNJn6X7F+A2SXVyDNmrqic Dbw1V8fzvpBZYNI4aDMuUWmC+tpVU0oA5r4583s6BbBD4dH7FMpPWPJ8Uxoot2Xo M4OSPPFkg97lXhr0qRwyBpO5O3FV+RfSualFjrYEItt6DNeDPD0Db3m0C/qxQLQ= =wIru -----END PGP SIGNATURE----- ------enig2MQGTQHQAIPMFNLLODKAH--