From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56267) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2iFr-0005l8-93 for qemu-devel@nongnu.org; Fri, 26 Jul 2013 09:40:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V2iFl-0007VW-IB for qemu-devel@nongnu.org; Fri, 26 Jul 2013 09:40:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28839) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2iFl-0007VO-As for qemu-devel@nongnu.org; Fri, 26 Jul 2013 09:40:17 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6QDeGeP022813 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 26 Jul 2013 09:40:16 -0400 Message-ID: <51F27C38.4020501@redhat.com> Date: Fri, 26 Jul 2013 07:40:08 -0600 From: Eric Blake MIME-Version: 1.0 References: <1374584606-5615-1-git-send-email-kwolf@redhat.com> <1374584606-5615-8-git-send-email-kwolf@redhat.com> In-Reply-To: <1374584606-5615-8-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mhvMQnfPgx64pBKAv0WHm1cxOGfgoXqaI" Subject: Re: [Qemu-devel] [PATCH 07/18] 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) --mhvMQnfPgx64pBKAv0WHm1cxOGfgoXqaI Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/23/2013 07:03 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" } MUCH nicer! >=20 > Signed-off-by: Kevin Wolf > --- > docs/qapi-code-gen.txt | 22 +++++++++++++ > scripts/qapi-types.py | 6 ++++ > scripts/qapi-visit.py | 86 ++++++++++++++++++++++++++++++++++++------= -------- > 3 files changed, 91 insertions(+), 23 deletions(-) >=20 > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index 555ca66..c187fda 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -103,6 +103,28 @@ And it looks like this on the wire: > "data" : { "backing-file": "/some/place/my-image", > "lazy-refcounts": true } } > =20 > + > +Flat union types avoid the nesting on the wire. They are used whenever= a > +specific field of the base type is declared as the discriminator ('typ= e' is > +then no longer generated). The discriminator must always be a string f= ield. Since an 'enum' is always sent over the wire as a string, is it appropriate to allow the discriminator to be an enum field instead of a 'str'? But that could be done in a followup patch; your initial usage is just fine with 'str'. Besides, if we allow the discriminator to have 'enum' type, that would imply that we want to guarantee coverage that all of the 'data' members of the union type correspond to the members of the union. On the other hand, that would be extra type safety - if we extend the enum that backs the discriminator, we'd immediately be reminded if we forgot to also extend the union based on that enum. Again, food for thought for a future patch, and not something needed for this one. > +The above example can then be modified as follows: > + > + { 'type': 'BlockdevCommonOptions', > + 'data': { 'driver': 'str' 'readonly': 'bool' } } Missing a comma. > +++ b/scripts/qapi-types.py > @@ -161,7 +161,9 @@ def generate_union(expr): > =20 > name =3D expr['union'] > typeinfo =3D expr['data'] > + > base =3D expr.get('base') > + discriminator =3D expr.get('discriminator') > =20 > ret =3D mcgen(''' > struct %(name)s > @@ -185,7 +187,11 @@ struct %(name)s > =20 > if base: > struct =3D find_struct(base) > + if discriminator: > + del struct['data'][discriminator] I asked before, but didn't get an answer; my question may just show my unfamiliarity with python. Is this modifying the original 'struct', such that other uses of the struct after this point will no longer contain the discriminator key? Or is it only modifying a copy of 'struct', with the original left intact? But based on the rest of your patch... > ret +=3D generate_struct_fields(struct['data']) > + else: > + assert not discriminator > =20 > ret +=3D mcgen(''' > }; > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index db6fa44..cd33e44 100644 > --- a/scripts/qapi-visit.py > @@ -157,9 +178,17 @@ def generate_visit_union(expr): > members =3D expr['data'] > =20 > base =3D expr.get('base') > + discriminator =3D expr.get('discriminator') > =20 > ret =3D generate_visit_enum('%sKind' % name, members.keys()) > =20 > + if base: > + base_fields =3D find_struct(base)['data'] > + if discriminator: > + base_fields =3D base_fields.copy() > + del base_fields[discriminator] =2E..here, you explicitly took a copy to be safe. So I suspect you are missing a .copy() above. I think my findings are easy fixes; so I'm okay if you fix them and then add: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --mhvMQnfPgx64pBKAv0WHm1cxOGfgoXqaI 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/ iQEcBAEBCAAGBQJR8nw4AAoJEKeha0olJ0NqIpAH/00VQFB92mki8izNjN/hTN0N gB+12cCGGR6o+jTTO2O0a8KHKtAWHeY8yPqvbCCGr15K5NqlO+ZcMbfXOkIvndWW o4pExXaHOlErU/4u0KUsK6Wdj2KW0XEijQJ/UI6wFQ6/rfC0qJeUnNCwHRs7FkZf lwhU4eJpbv+46oU6MoEXJWVn0Vn1I5ul+rHdtIjCmSCTnooCKS0/NMrKP4wBzKik IGtchhcv7zDInxFiXVw/d15MN0Kiswp3NI4efUR2/UnT1LayZa+Pz4FrKu82KxP5 GbIdcMg3QFd9G9+11KZ2nioKCIDclXAVOjzpr3+R0MbPHpojx2Udmc6H9tx99dg= =b7GC -----END PGP SIGNATURE----- --mhvMQnfPgx64pBKAv0WHm1cxOGfgoXqaI--