From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37410) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxMq5-000680-7c for qemu-devel@nongnu.org; Thu, 11 Jul 2013 15:47:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UxMq3-0001S0-OF for qemu-devel@nongnu.org; Thu, 11 Jul 2013 15:47:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11470) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxMq3-0001Rv-Ge for qemu-devel@nongnu.org; Thu, 11 Jul 2013 15:47:39 -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 r6BJlcvL029602 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 11 Jul 2013 15:47:38 -0400 Message-ID: <51DF0BD9.3050403@redhat.com> Date: Thu, 11 Jul 2013 13:47:37 -0600 From: Eric Blake MIME-Version: 1.0 References: <1373363617-4723-1-git-send-email-kwolf@redhat.com> <1373363617-4723-9-git-send-email-kwolf@redhat.com> In-Reply-To: <1373363617-4723-9-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2QGWMBRUTAIBUCGGQATXO" Subject: Re: [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions 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) ------enig2QGWMBRUTAIBUCGGQATXO Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/09/2013 03:53 AM, Kevin Wolf wrote: > The discriminator for anonymous unions is the data type. This allows to= > have a union type that allows both of these: >=20 > { 'file': 'my_existing_block_device_id' } > { 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } } >=20 > Unions like this are specified in the schema with an empty dict as > discriminator. For this example you could take: >=20 > { 'union': 'BlockRef', > 'discriminator': {}, > 'data': { 'definition': 'BlockOptions' > 'reference': 'str' } } > { 'type': 'ExampleObject', > 'data: { 'file': 'BlockRef' } } Yay - a commit message that shows the new QMP wire format, and the qapi-schema.json that generated it. [Without reading the patch yet] I take it the 'data' of such a union must be completely distinguishable by type - the visitor for the 'file' key tries each subtype in turn ('BlockOptions' or 'str') until one does not have a parse error. And this changes the earlier patch that required 'discriminator' to be used only when 'base' was also present. Can it distinguish between two structs, or is the union only allowed to have one struct option and all other options are primitive types? Should we consider the use of arrays as a union member type? Hmm, I wonder if this new anonymous union can be useful in helping us write the self-description of introspection in Amos' patches. >=20 > Signed-off-by: Kevin Wolf > --- > include/qapi/qmp/qobject.h | 1 + > include/qapi/visitor-impl.h | 2 ++ > include/qapi/visitor.h | 3 +++ > qapi/qapi-visit-core.c | 9 +++++++++ > qapi/qmp-input-visitor.c | 14 ++++++++++++++ > qobject/qjson.c | 2 ++ > scripts/qapi-types.py | 42 +++++++++++++++++++++++++++++++++++++= +++ > scripts/qapi-visit.py | 47 +++++++++++++++++++++++++++++++++++++= ++++++++ > scripts/qapi.py | 28 +++++++++++++++++++++++++++ > 9 files changed, 148 insertions(+) > +++ b/scripts/qapi-visit.py > @@ -172,6 +172,49 @@ void visit_type_%(name)s(Visitor *m, %(name)s * ob= j, const char *name, Error **e > ''', > name=3Dname) > =20 > +def generate_visit_anon_union(name, members): > + ret =3D mcgen(''' > + > +void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name= , Error **errp) > +{ > + Error *err =3D NULL; > + > + if (!error_is_set(errp)) { > + visit_start_implicit_struct(m, (void**) obj, sizeof(%(name)s),= &err); > + visit_get_next_type(m, (int*) &(*obj)->kind, %(name)s_qtypes, = name, &err); > + switch ((*obj)->kind) { > +''', > + name=3Dname) > + > + for key in members: > + assert (members[key] in builtin_types > + or find_struct(members[key]) > + or find_union(members[key])), "Invalid anonymous union mem= ber" > + > + ret +=3D mcgen(''' > + case %(abbrev)s_KIND_%(enum)s: > + visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);= > + break; > +''', > + abbrev =3D de_camel_case(name).upper(), > + enum =3D c_fun(de_camel_case(key),False).upper(), > + c_type=3Dtype_name(members[key]), > + c_name=3Dc_fun(key)) Inconsistent spacing around '=3D' > + > + ret +=3D mcgen(''' > + default: > + abort(); Does this mean I can cause qemu to abort if I pass bogus information on the wire? Using your commit message example, { 'file': false } would hit the default case, right? > +++ b/scripts/qapi.py > @@ -17,6 +17,21 @@ builtin_types =3D [ > 'uint8', 'uint16', 'uint32', 'uint64' > ] > =20 > +builtin_type_qtypes =3D { > + 'str': 'QTYPE_QSTRING', > + 'int': 'QTYPE_QINT', > + 'number': 'QTYPE_QFLOAT', > + 'bool': 'QTYPE_QBOOL', > + 'int8': 'QTYPE_QINT', > + 'int16': 'QTYPE_QINT', > + 'int32': 'QTYPE_QINT', > + 'int64': 'QTYPE_QINT', > + 'uint8': 'QTYPE_QINT', > + 'uint16': 'QTYPE_QINT', > + 'uint32': 'QTYPE_QINT', > + 'uint64': 'QTYPE_QINT', What happens if I try to write a union in qapi-schema.json that has both a 'int8' and 'uint32' branch? Since both of those resolve to the QTYPE_QINT visitor, does that end up causing the C code to generate a switch statement with duplicate labels? > +} > + > def tokenize(data): > while len(data): > ch =3D data[0] > @@ -105,6 +120,7 @@ def parse_schema(fp): > if expr_eval.has_key('enum'): > add_enum(expr_eval['enum']) > elif expr_eval.has_key('union'): > + add_union(expr_eval) > add_enum('%sKind' % expr_eval['union']) > elif expr_eval.has_key('type'): > add_struct(expr_eval) > @@ -188,6 +204,7 @@ def type_name(name): > =20 > enum_types =3D [] > struct_types =3D [] > +union_types =3D [] Is it worth splitting the tracking of the union_types of qapi-schema.json into an independent patch? Does it make sense to allow a union type (possibly only an anonymous union type) in place of a struct type as a top-level type reference? That is, just as we now allow { 'command':'foo', 'data': 'StructType' }, would it make sense to allow { 'command':bar', 'data': 'AnonUnion' }? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2QGWMBRUTAIBUCGGQATXO 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/ iQEcBAEBCAAGBQJR3wvZAAoJEKeha0olJ0Nqqx8H/2kSfQbZu7bkC7EGeOJ48uMF V4GKBTOTwdT+oplXzH8z77SFz/TesWiMEoduzmPKa43JnKHZfBgMtjSgXIssrLIq Mu/zEq5LCvOHqCz30pR8AJ5amNGYXoWsZgWxUU/3qYea+zwUIshx9nTIMio07IRV /zGF9yqxt0UgI0VBVPkJGM35sMRzztJ4tBSe3tCDEI6fRAvU9M4So31JZA5nrEyJ w/LwXhuBM7+ZkIDRtaxCNT1akcGOkyKQ/zLIQnydUxI9awO/K+G4Z39xCjHMpWGm kMYPB2cxyNNYq0B+Cr5crs6mcHC6j0M4GLqEQKwgkq+9Uc4noaXZgMCGD1aUr2w= =927W -----END PGP SIGNATURE----- ------enig2QGWMBRUTAIBUCGGQATXO--