From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54596) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uxe8D-0008CH-Lp for qemu-devel@nongnu.org; Fri, 12 Jul 2013 10:15:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uxe8B-0000ez-Lx for qemu-devel@nongnu.org; Fri, 12 Jul 2013 10:15:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:23447) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uxe8B-0000dy-F8 for qemu-devel@nongnu.org; Fri, 12 Jul 2013 10:15:31 -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 r6CEFUlZ028625 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 12 Jul 2013 10:15:30 -0400 Message-ID: <51E00F80.7020208@redhat.com> Date: Fri, 12 Jul 2013 08:15:28 -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> <51DF0BD9.3050403@redhat.com> <20130712085507.GA2533@dhcp-200-207.str.redhat.com> In-Reply-To: <20130712085507.GA2533@dhcp-200-207.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2HRNRDVVGWJGNXKOQSUNS" 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) ------enig2HRNRDVVGWJGNXKOQSUNS Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/12/2013 02:55 AM, Kevin Wolf wrote: > Am 11.07.2013 um 21:47 hat Eric Blake geschrieben: >> 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: >>> >>> { 'file': 'my_existing_block_device_id' } >>> { 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } = } >>> >>> Unions like this are specified in the schema with an empty dict as >>> discriminator. For this example you could take: >>> >>> { '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. >=20 > Yeah, sorry about these missing examples and detailed documentation. I > wanted to get an RFC out quickly and probably underestimated how > important it is even for reviewing an RFC. The higher volume the list, the more important documentation is :) >=20 > However, I think keeping this information only in the commit messages > isn't right either. Do we have any of the schema syntax documented > anywhere? Should I create something in docs/(specs/)? We have qemu/docs/qapi-code-gen.txt - and you are right - that file NEEDS to be updated with this series :) >=20 >> [Without reading the patch yet] >> I take it the 'data' of such a union must be completely distinguishabl= e >> by type - the visitor for the 'file' key tries each subtype in turn >> ('BlockOptions' or 'str') until one does not have a parse error. >=20 > Not exactly. It asks the visitor for the type of the next item and then= > uses a lookup table to get the right discriminator enum value for it. More efficient than trying every visitor; but same idea of the lookup table being used to say which visitor will succeed. >=20 >> And >> this changes the earlier patch that required 'discriminator' to be use= d >> only when 'base' was also present. Can it distinguish between two >> structs, or is the union only allowed to have one struct option and al= l >> other options are primitive types? Should we consider the use of arra= ys >> as a union member type? >=20 > As the types it uses for lookup are QObject types and all structs map t= o > QDict, only one struct is allowed. Worth documenting. >=20 > This makes sense anyway, because allowing different structs would mean > that you potentially get ambiguities when structs look alike (perhaps > only differing in optional fields or something like that). >=20 > As soon as we find a good use case for anonymous unions of two structs,= > I might change my opinion, though. ;-) >=20 > Arrays are a logical extension of this, though I don't have any use for= > them now. I wouldn't expect them to be too hard to implement. I don't mind leaving them out for now; as you said, it should be easy enough to add at the point where the schema makes it worth doing. >=20 >> Hmm, I wonder if this new anonymous union can be useful in helping us >> write the self-description of introspection in Amos' patches. >=20 > Hm, what's the exact problem there? I know I read that someone (Paolo?)= > wrote that it was impossible, but I didn't immediately understand why > and didn't feel like thinking a lot about either. Indeed: if we write a self-describing schema for introspection, then 'enum' uses 'data':['str'], while 'type', 'union', and 'command' use 'data':'QDict'. If we make an anonymous union with discriminator based on enum/type/union/command, then the type of data is determined based on the discriminator, with an array involved. But it was more than just what data type is tied to 'data', it was also the fact that qapi-schema.json contains free-form dictionaries (arbitrary keys, each with a type), while QAPI tries to describe the wire format of all possible keys, where only values are arbitrary strings. Hence I've been arguing all along that we need a post-processing step that turns qapi-schema's 'key':'Type' into the QMP wire struct { 'name':'key', 'type':'Type' }, so that 'name' and 'type' are fixed keys. I think both issues are at hand - how to represent each field (whether as short-hand straight from the .json file or a long-hand struct) and how to distinguish between qapi metatypes (which are effectively a union between 'type'/'command'/...). >>> + >>> + ret +=3D mcgen(''' >>> + default: >>> + abort(); >> >> Does this mean I can cause qemu to abort if I pass bogus information o= n >> the wire? Using your commit message example, >> >> { 'file': false } >> >> would hit the default case, right? >=20 > No, it wouldn't, but that case fails in an interesting way anyway. :-) >=20 > We would look up the enum kind in BlockRef_qtypes[QTYPE_BOOL], which > isn't defined and therefore 0. This happens to be the enum value for > BLOCK_REF_KIND_DEFINITION. So I guess what you get is an error message > like "invalid argument type: got bool, expected dict". As long as it is an error message and not a qemu crash, I'm okay. I should be able to throw valid JSON but garbage contents at the parser, without the parser falling over. >=20 >> 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 > Sounds sensible to me. But do we have a use case for it? Not sure yet; again, one of those things that should be easy enough to defer until we have a use case, and where introspection may be such a use case. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2HRNRDVVGWJGNXKOQSUNS 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/ iQEcBAEBCAAGBQJR4A+AAAoJEKeha0olJ0NqiHIIAJtK1jOsMpIf0Adcc4onxjmf X88X52z5p0SPYqIn4tOKY6Q8qB9ao+WAPSLjB2SiQhB13sYYk0GEIZnoNHe46Sb2 hHXfQmkWZu5cqfg66001/xsBll9nbWxkWjQT58X/6ConyXJiSV2sgjPB0y2An5YU pqWciVF5WR9m9T9yKqNxHSKKojY/U63u57ETNHppF/zw9AHsHcYMVHUVVUD07pOJ ZHwTazWRi0ozOe3L4Y0k9g1Cq38qKk6phA1hhKchjwghuwjXGkm6H3gmuY+6m9yx 8TOxb9rt5aJrZTdv6ZrS+hS2x+69LHmfiLqvLwSHh+jjW5+MYrWwAMxFmGGD0fM= =z8J3 -----END PGP SIGNATURE----- ------enig2HRNRDVVGWJGNXKOQSUNS--