From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54167) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yd102-0000Eq-QT for qemu-devel@nongnu.org; Tue, 31 Mar 2015 14:34:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yd0zz-0000OZ-JT for qemu-devel@nongnu.org; Tue, 31 Mar 2015 14:34:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47207) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yd0zz-0000NX-CI for qemu-devel@nongnu.org; Tue, 31 Mar 2015 14:34:51 -0400 Date: Tue, 31 Mar 2015 20:34:46 +0200 From: Kevin Wolf Message-ID: <20150331183446.GF4748@noname.redhat.com> References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-7-git-send-email-eblake@redhat.com> <87h9t76gng.fsf@blackfin.pond.sub.org> <55142006.50606@redhat.com> <20150331171320.GD4748@noname.redhat.com> <551AE45F.70801@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="5G06lTa6Jq83wMTw" Content-Disposition: inline In-Reply-To: <551AE45F.70801@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: lcapitulino@redhat.com, famz@redhat.com, Markus Armbruster , wenchaoqemu@gmail.com, qemu-devel@nongnu.org --5G06lTa6Jq83wMTw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 31.03.2015 um 20:15 hat Eric Blake geschrieben: > On 03/31/2015 11:13 AM, Kevin Wolf wrote: >=20 > >> [3] > >> { 'union':'SimpleAndSafe', 'discriminator':'MyEnum', > >> 'data':{ 'one':'str', 'two':'int' }} > >> > >> {"execute":"foo", "arguments":{'type':'one', 'data':'hello'}} > >=20 > > This would overload 'discriminator' with two different meanings: > >=20 > > * In case [1] it refers to one key in the JSON object which contains the > > name of the union branch to select. That is, it is the _name_ of the > > key used as a discriminator. > >=20 > > * In case [3], the 'type' key is used in the JSON object to select a > > union branch. 'discriminator' describes the _valid values_ of it, i.e. > > the branch names. > >=20 > > We shouldn't mix these meanings. If you need [3], we could call it > > 'discriminator-type' or something like that. If both options are > > consistently used for simple and flat unions, you end up with these > > rules: > >=20 >=20 > Nice idea. Good thing I haven't actually implemented the extension yet. >=20 > > * 'discriminator' is the key that is used to select the union branch. > >=20 > > - For flat unions it is required and must refer to an explicitly > > declared key in the base type. > >=20 > > - For simple unions it is optional and defaults to 'type'. The key is > > implicitly created in the union type. > >=20 > > * 'discrimiator-type' describes the valid values of 'discriminator', > > either by referring to an enum type or to 'str'. >=20 > Referring to 'str' should only be allowed for implicit enums. We > already require it to resolve to an enum for flat unions. >=20 > >=20 > > - For flat unions, this must match the type of the explicit > > declaration of the discriminator field. It is optional and defauls > > to the only valid value. > >=20 > > - For simple unions it is optional, too, and defaults to 'str'. > >=20 > > - If it is the name of an enum type, that enum type is reused and the > > declared union branches must match the valid values of the enum. > >=20 > > - If it is 'str', a new enum is generated, and all the declared union > > branches are used as valid values. >=20 > Sounds reasonable to me. But even with this approach, I still don't > know that we have room for a 'base' type on a simple union. I guess the > joy of future extensions is that we can make legal whatever we want, and > that this series is adding enough tests to make it obvious whether those > new additions break any existing test cases. I think we have room for it, but probably still no use case. And they are kind of ugly with mixing top-level fields for the base type and nested ones for the branch-specific fields. (To be honest, I find all non-flat unions ugly, but mixing is even uglier.) So I certainly don't object to removing simple unions with base. > > There's quite some duplication in it where we need to make sure that the > > schema matches in all places, but without an explicit declaration of the > > disriminator field in simple unions, this seems to be the best I can > > come up with. >=20 > Yeah, it's a bit more verbose than the overloaded version, but may be > worth it. >=20 > >=20 > >> But the existing, unused-except-in-testsuite, notion of a simple union > >> with a base class looks like: > >> > >> [4] > >> { 'type':'Shared', 'data':{'common':'int'}} > >> { 'union':'SimpleWithBase', 'base':'Shared', > >> 'data':{ 'one':'str', 'two':'int' }} > >> > >> {"execute":"foo", "arguments":{'common':1, 'type':'one', 'data':'hello= '}} > >> > >> If we were to allow the addition of 'discriminator':'EnumType' to a > >> simple union [3], but then add that discriminator to an existing case = of > >> a simple union with base [4], it would look like: > >> > >> { 'type':'Shared', 'data':{'common':'int'}} > >> { 'union':'SimpleWithBaseAndDiscriminator', 'base':'Shared', > >> 'discriminator':'MyEnum', > >> 'data':{ 'one':'str', 'two':'int' }} > >> > >> Yuck. That is indistinguishable from flat unions [1], except by wheth= er > >> discriminator names an enum type or a member of the base class. > >=20 > > Which could even be ambiguous, couldn't it? >=20 > My whole argument for why I _don't_ want to allow base types for simple > unions. Essentially the problem is that we distinguish between simple and flat unions only by looking at currently arbitrarily restricted options. So another option to solve it would be a boolean 'flat', which we would set as false for all existing simple unions, and let it default to true. That would give us nicer QMP for new unions, too. (I still don't think we need to save simple unions with base types, but it's a thought I wanted to mention anyway because I like flat unions.) > >>> In particular, I can define simple unions in terms of flat ones by > >>> restricting all union cases to a single member named 'data'. They're > >>> not implemented that way, but that's a historical accident. Simple > >>> unions are a redundant concept. > >> > >> Cool. Or more concretely, > >> > >> { 'union': 'Simple', 'data': { 'one': 'str', 'two': 'int' } } > >> > >> is identical on the wire to: > >> > >> { 'enum': 'MyEnum', 'data': ['one', 'two'] } > >> { 'type': 'Base', 'data': { 'type': 'MyEnum' } } > >> { 'type': 'Branch1', 'data': { 'data': 'str' } } > >> { 'type': 'Branch2', 'data': { 'data': 'int' } } > >> { 'union': 'Flat', 'base': 'Base', 'discriminator': 'type', > >> 'data': { 'one': 'Branch1', 'two': 'Branch2' } } > >=20 > > Perhaps we should expose all unions for schema introspection in a way > > similar to this (possibly not splitting out Branch1 and Branch2 as > > independent types, though). We would just have to give a name to > > implicitly generated enums and base types. >=20 > So maybe we update the schema to allow anonymous types. That is: >=20 > { 'union': 'Simple', > 'data': { 'one': { 'name': 'str', 'value': 'int' } } } >=20 > would be nicer than the current requirement of: >=20 > { 'type': 'Branch1', 'data': { 'name': 'str', 'value': 'int' } } > { 'union': 'Simple', > 'data': { 'one': 'Branch1' } } Hm... You just removed them, right? Maybe my suggestion isn't that constructive then. :-) Kevin --5G06lTa6Jq83wMTw Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVGujGAAoJEH8JsnLIjy/WHb0P/Rq3Rm47fooc7qHXwOklck4Q dOkkGqkBDPUrQxQn19wUsIFTuLGT+ATa7yQJHJ7YeARYsE20hgxV0QGBKJxg0+zH jndFr5Sgk2f7QbWoO3tXBEEgd4/znEkCuRVY9t/X0m3YHhCAxZkZ6kGvHkWmX2PE 1Nue2F6lRmjS943JvISKYo7hdJaVfLTmaFAsTTuwuRHiy6WSlkCJMEfBzcEVDcS4 O3O9MGS6QvQnnF0/X95tDVnNVVUt0tt9WkbTOpYshZRxA8rlapOoPXoG9Q0oQp7+ PjnKBnksPLC32GTkK+mPmdiq1Y9eVSfbQfgi6zg2H6K+ONjOx5K29J5/88oyWyQa jHoLCdNLfYZtMt52h21y/rXDBW6JvOEQopr6uopDCDRQOl+zBt6nyR7Ng1gAKRdg JCYiZ+6Vhcoz9MjYLwh7Hp8/Ef94k90nSmwwQRXiZUvcBDFIyrq6CfVt4WLIFspP 188/2t15/qkPRy3+Xs0l3icAlwNrOZw+XaEUwS7WKGYM29W53xkFPKktk+kSdM3I 1WV78jRWOoky9HJFeYzDLq7mHUVsOG6D7Nx1CeIH74ehh0wqrR/FZlpTAPcos0Od mWvfr5a8ZNTnzUi4mr0ZJ6yJN1VgDR2lECUNXydD1z6GYJOCSnxSBoMAFA1oerOE KjYPY0t8TIKMSyG+pUBt =qf+N -----END PGP SIGNATURE----- --5G06lTa6Jq83wMTw--