From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48660) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2jWi-0000jR-Ii for qemu-devel@nongnu.org; Fri, 26 Jul 2013 11:01:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V2jWh-00017q-0l for qemu-devel@nongnu.org; Fri, 26 Jul 2013 11:01:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25244) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2jWg-00017X-QC for qemu-devel@nongnu.org; Fri, 26 Jul 2013 11:01:50 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6QF1nvU010277 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 26 Jul 2013 11:01:49 -0400 Date: Fri, 26 Jul 2013 17:01:46 +0200 From: Kevin Wolf Message-ID: <20130726150146.GB18446@dhcp-200-207.str.redhat.com> References: <1374584606-5615-1-git-send-email-kwolf@redhat.com> <1374584606-5615-8-git-send-email-kwolf@redhat.com> <51F27C38.4020501@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51F27C38.4020501@redhat.com> 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: Eric Blake Cc: armbru@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, lcapitulino@redhat.com Am 26.07.2013 um 15:40 hat Eric Blake geschrieben: > On 07/23/2013 07:03 AM, Kevin Wolf wrote: > > Instead of the rather verbose syntax that distinguishes base and > > subclass fields... > > > > { "type": "file", > > "read-only": true, > > "data": { > > "filename": "test" > > } } > > > > ...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: > > > > { "driver": "file", > > "read-only": true, > > "filename": "test" } > > MUCH nicer! > > > > > 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(-) > > > > > 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 } } > > > > + > > +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 ('type' is > > +then no longer generated). The discriminator must always be a string field. > > 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. There's nothing to add to this, I would certainly support such a future patch. > > +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): > > > > name = expr['union'] > > typeinfo = expr['data'] > > + > > base = expr.get('base') > > + discriminator = expr.get('discriminator') > > > > ret = mcgen(''' > > struct %(name)s > > @@ -185,7 +187,11 @@ struct %(name)s > > > > if base: > > struct = 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... Sorry, this is in fact my own unfamiliarity with Python, combined with failure to fix all cases when you pointed it out. The only reason I didn't reply to that part of your review was that I thought it would be obvious when I send a fixed version. Well, except if I don't. I've changed this hunk now to match the other one: @@ -184,8 +186,13 @@ struct %(name)s ''') if base: - struct = find_struct(base) - ret += generate_struct_fields(struct['data']) + base_fields = find_struct(base)['data'] + if discriminator: + base_fields = base_fields.copy() + del base_fields[discriminator] + ret += generate_struct_fields(base_fields) + else: + assert not discriminator ret += mcgen(''' }; > I think my findings are easy fixes; so I'm okay if you fix them and then > add: > > Reviewed-by: Eric Blake Thanks, Eric. Kevin