From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLjKl-0002br-H2 for qemu-devel@nongnu.org; Thu, 06 Mar 2014 20:12:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLjKg-0000XL-7J for qemu-devel@nongnu.org; Thu, 06 Mar 2014 20:12:19 -0500 Received: from mail-pd0-x230.google.com ([2607:f8b0:400e:c02::230]:61636) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLjKf-0000XF-Nx for qemu-devel@nongnu.org; Thu, 06 Mar 2014 20:12:14 -0500 Received: by mail-pd0-f176.google.com with SMTP id r10so3314153pdi.35 for ; Thu, 06 Mar 2014 17:12:12 -0800 (PST) Message-ID: <53191CDD.8030504@gmail.com> Date: Fri, 07 Mar 2014 09:11:57 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1393987480-37579-1-git-send-email-wenchaoqemu@gmail.com> <1393987480-37579-8-git-send-email-wenchaoqemu@gmail.com> <87y50nbuoc.fsf@blackfin.pond.sub.org> <531861F9.1020608@gmail.com> <20140306080358.7fac2825@redhat.com> In-Reply-To: <20140306080358.7fac2825@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V9 07/10] qapi script: support enum type as discriminator in union List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, mdroth@linux.vnet.ibm.com, Markus Armbruster , qemu-devel@nongnu.org 于 2014/3/6 21:03, Luiz Capitulino 写道: > On Thu, 06 Mar 2014 19:54:33 +0800 > Wenchao Xia wrote: > >> 于 2014/3/6 16:25, Markus Armbruster 写道: >>> Wenchao Xia writes: >>> >>>> By default, any union will automatically generate a enum type as >>>> "[UnionName]Kind" in C code, and it is duplicated when the discriminator >>>> is specified as a pre-defined enum type in schema. After this patch, >>>> the pre-defined enum type will be really used as the switch case >>>> condition in generated C code, if discriminator is an enum field. >>>> >>>> Signed-off-by: Wenchao Xia >>>> Reviewed-by: Eric Blake >>>> --- >>>> docs/qapi-code-gen.txt | 8 ++++- >>>> scripts/qapi-types.py | 18 +++++++++--- >>>> scripts/qapi-visit.py | 29 +++++++++++++------ >>>> scripts/qapi.py | 32 +++++++++++++++++++++- >>>> tests/Makefile | 2 +- >>>> tests/qapi-schema/flat-union-reverse-define.exit | 1 + >>>> tests/qapi-schema/flat-union-reverse-define.json | 17 +++++++++++ >>>> tests/qapi-schema/flat-union-reverse-define.out | 9 ++++++ >>>> 8 files changed, 99 insertions(+), 17 deletions(-) >>>> create mode 100644 tests/qapi-schema/flat-union-reverse-define.err >>>> create mode 100644 tests/qapi-schema/flat-union-reverse-define.exit >>>> create mode 100644 tests/qapi-schema/flat-union-reverse-define.json >>>> create mode 100644 tests/qapi-schema/flat-union-reverse-define.out >>>> >>>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt >>>> index 0728f36..a2e7921 100644 >>>> --- a/docs/qapi-code-gen.txt >>>> +++ b/docs/qapi-code-gen.txt >>>> @@ -123,11 +123,15 @@ And it looks like this on the wire: >>>> >>>> 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. >>>> +then no longer generated). The discriminator can be a string field or a >>>> +predefined enum field. If it is a string field, a hidden enum type will be >>>> +generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check >>>> +will be done to verify the correctness. It is recommended to use an enum field. >>>> The above example can then be modified as follows: >>>> >>>> + { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] } >>>> { 'type': 'BlockdevCommonOptions', >>>> - 'data': { 'driver': 'str', 'readonly': 'bool' } } >>>> + 'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } } >>>> { 'union': 'BlockdevOptions', >>>> 'base': 'BlockdevCommonOptions', >>>> 'discriminator': 'driver', >>>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py >>>> index 5885bac..10864ef 100644 >>>> --- a/scripts/qapi-types.py >>>> +++ b/scripts/qapi-types.py >>>> @@ -201,14 +201,21 @@ def generate_union(expr): >>>> base = expr.get('base') >>>> discriminator = expr.get('discriminator') >>>> >>>> + enum_define = discriminator_find_enum_define(expr) >>>> + if enum_define: >>>> + discriminator_type_name = enum_define['enum_name'] >>>> + else: >>>> + discriminator_type_name = '%sKind' % (name) >>>> + >>>> ret = mcgen(''' >>>> struct %(name)s >>>> { >>>> - %(name)sKind kind; >>>> + %(discriminator_type_name)s kind; >>>> union { >>>> void *data; >>>> ''', >>>> - name=name) >>>> + name=name, >>>> + discriminator_type_name=discriminator_type_name) >>>> >>>> for key in typeinfo: >>>> ret += mcgen(''' >>>> @@ -389,8 +396,11 @@ for expr in exprs: >>>> fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) >>>> elif expr.has_key('union'): >>>> ret += generate_fwd_struct(expr['union'], expr['data']) + "\n" >>>> - ret += generate_enum('%sKind' % expr['union'], expr['data'].keys()) >>>> - fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys())) >>>> + enum_define = discriminator_find_enum_define(expr) >>>> + if not enum_define: >>>> + ret += generate_enum('%sKind' % expr['union'], expr['data'].keys()) >>>> + fdef.write(generate_enum_lookup('%sKind' % expr['union'], >>>> + expr['data'].keys())) >>>> if expr.get('discriminator') == {}: >>>> fdef.write(generate_anon_union_qtypes(expr)) >>>> else: >>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py >>>> index 0baaf60..45ce3a9 100644 >>>> --- a/scripts/qapi-visit.py >>>> +++ b/scripts/qapi-visit.py >>>> @@ -259,10 +259,16 @@ def generate_visit_union(expr): >>>> assert not base >>>> return generate_visit_anon_union(name, members) >>>> >>>> - # There will always be a discriminator in the C switch code, by default it >>>> - # is an enum type generated silently as "'%sKind' % (name)" >>>> - ret = generate_visit_enum('%sKind' % name, members.keys()) >>>> - disc_type = '%sKind' % (name) >>>> + enum_define = discriminator_find_enum_define(expr) >>>> + if enum_define: >>>> + # Use the enum type as discriminator >>>> + ret = "" >>>> + disc_type = enum_define['enum_name'] >>>> + else: >>>> + # There will always be a discriminator in the C switch code, by default it >>>> + # is an enum type generated silently as "'%sKind' % (name)" >>>> + ret = generate_visit_enum('%sKind' % name, members.keys()) >>>> + disc_type = '%sKind' % (name) >>>> >>>> if base: >>>> base_fields = find_struct(base)['data'] >>>> @@ -298,15 +304,16 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** >>>> pop_indent() >>>> >>>> if not discriminator: >>>> - desc_type = "type" >>>> + disc_key = "type" >>>> else: >>>> - desc_type = discriminator >>>> + disc_key = discriminator >>>> ret += mcgen(''' >>>> - visit_type_%(name)sKind(m,&(*obj)->kind, "%(type)s",&err); >>>> + visit_type_%(disc_type)s(m,&(*obj)->kind, "%(disc_key)s",&err); >>>> if (!err) { >>>> switch ((*obj)->kind) { >>>> ''', >>>> - name=name, type=desc_type) >>>> + disc_type = disc_type, >>>> + disc_key = disc_key) >>>> >>>> for key in members: >>>> if not discriminator: >>>> @@ -517,7 +524,11 @@ for expr in exprs: >>>> ret += generate_visit_list(expr['union'], expr['data']) >>>> fdef.write(ret) >>>> >>>> - ret = generate_decl_enum('%sKind' % expr['union'], expr['data'].keys()) >>>> + enum_define = discriminator_find_enum_define(expr) >>>> + ret = "" >>>> + if not enum_define: >>>> + ret = generate_decl_enum('%sKind' % expr['union'], >>>> + expr['data'].keys()) >>>> ret += generate_declaration(expr['union'], expr['data']) >>>> fdecl.write(ret) >>>> elif expr.has_key('enum'): >>>> diff --git a/scripts/qapi.py b/scripts/qapi.py >>>> index eebc8a7..0a504c4 100644 >>>> --- a/scripts/qapi.py >>>> +++ b/scripts/qapi.py >>>> @@ -180,6 +180,25 @@ def find_base_fields(base): >>>> return None >>>> return base_struct_define['data'] >>>> >>>> +# Return the discriminator enum define if discriminator is specified as an >>>> +# enum type, otherwise return None. >>>> +def discriminator_find_enum_define(expr): >>>> + base = expr.get('base') >>>> + discriminator = expr.get('discriminator') >>>> + >>>> + if not (discriminator and base): >>>> + return None >>>> + >>>> + base_fields = find_base_fields(base) >>>> + if not base_fields: >>>> + return None >>>> + >>>> + discriminator_type = base_fields.get(discriminator) >>>> + if not discriminator_type: >>>> + return None >>>> + >>>> + return find_enum(discriminator_type) >>>> + >>>> def check_union(expr, expr_info): >>>> name = expr['union'] >>>> base = expr.get('base') >>>> @@ -254,11 +273,22 @@ def parse_schema(fp): >>>> add_enum(expr['enum'], expr['data']) >>>> elif expr.has_key('union'): >>>> add_union(expr) >>>> - add_enum('%sKind' % expr['union']) >>>> elif expr.has_key('type'): >>>> add_struct(expr) >>>> exprs.append(expr) >>>> >>>> + # Try again for hidden UnionKind enum >>>> + for expr_elem in schema.exprs: >>>> + expr = expr_elem['expr'] >>>> + if expr.has_key('union'): >>>> + try: >>>> + enum_define = discriminator_find_enum_define(expr) >>>> + except QAPIExprError, e: >>>> + print>>sys.stderr, e >>>> + exit(1) >>> How can we get QAPIExprError here? >>> >> Ops, for this version Exception wouldn't happen, I forgot >> to remove the "try except". It should be simply: >> >> + # Try again for hidden UnionKind enum >> + for expr_elem in schema.exprs: >> + expr = expr_elem['expr'] >> + if expr.has_key('union'): >> + enum_define = discriminator_find_enum_define(expr) >> >> Luiz, do you mind apply this series and correct above directly? > I do. Please, respin 07/10 only and send it as a reply to this thread. > Modified and rebased, hope the line removed in qapi.py wouldn't interrupt patch 9,10's appliance. >>>> + if not enum_define: >>>> + add_enum('%sKind' % expr['union']) >>>> + >>>> try: >>>> check_exprs(schema) >>>> except QAPIExprError, e: >>> [...]