From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44529) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLXy7-0006LU-P1 for qemu-devel@nongnu.org; Thu, 06 Mar 2014 08:04:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLXy3-0001fq-0k for qemu-devel@nongnu.org; Thu, 06 Mar 2014 08:04:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:7144) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLXy2-0001ep-Oc for qemu-devel@nongnu.org; Thu, 06 Mar 2014 08:04:06 -0500 Date: Thu, 6 Mar 2014 08:03:58 -0500 From: Luiz Capitulino Message-ID: <20140306080358.7fac2825@redhat.com> In-Reply-To: <531861F9.1020608@gmail.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: Wenchao Xia Cc: kwolf@redhat.com, mdroth@linux.vnet.ibm.com, Markus Armbruster , qemu-devel@nongnu.org On Thu, 06 Mar 2014 19:54:33 +0800 Wenchao Xia wrote: > =E4=BA=8E 2014/3/6 16:25, Markus Armbruster =E5=86=99=E9=81=93: > > 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 discriminat= or > >> 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: > >> =20 > >> Flat union types avoid the nesting on the wire. They are used wheneve= r a > >> specific field of the base type is declared as the discriminator ('ty= pe' 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 wi= ll be > >> +generated as "[UNION_NAME]Kind". If it is an enum field, a compile ti= me check > >> +will be done to verify the correctness. It is recommended to use an e= num field. > >> The above example can then be modified as follows: > >> =20 > >> + { '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 =3D expr.get('base') > >> discriminator =3D expr.get('discriminator') > >> =20 > >> + enum_define =3D discriminator_find_enum_define(expr) > >> + if enum_define: > >> + discriminator_type_name =3D enum_define['enum_name'] > >> + else: > >> + discriminator_type_name =3D '%sKind' % (name) > >> + > >> ret =3D mcgen(''' > >> struct %(name)s > >> { > >> - %(name)sKind kind; > >> + %(discriminator_type_name)s kind; > >> union { > >> void *data; > >> ''', > >> - name=3Dname) > >> + name=3Dname, > >> + discriminator_type_name=3Ddiscriminator_type_name) > >> =20 > >> for key in typeinfo: > >> ret +=3D mcgen(''' > >> @@ -389,8 +396,11 @@ for expr in exprs: > >> fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) > >> elif expr.has_key('union'): > >> ret +=3D generate_fwd_struct(expr['union'], expr['data']) + "= \n" > >> - ret +=3D generate_enum('%sKind' % expr['union'], expr['data']= .keys()) > >> - fdef.write(generate_enum_lookup('%sKind' % expr['union'], exp= r['data'].keys())) > >> + enum_define =3D discriminator_find_enum_define(expr) > >> + if not enum_define: > >> + ret +=3D generate_enum('%sKind' % expr['union'], expr['da= ta'].keys()) > >> + fdef.write(generate_enum_lookup('%sKind' % expr['union'], > >> + expr['data'].keys())) > >> if expr.get('discriminator') =3D=3D {}: > >> 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) > >> =20 > >> - # There will always be a discriminator in the C switch code, by d= efault it > >> - # is an enum type generated silently as "'%sKind' % (name)" > >> - ret =3D generate_visit_enum('%sKind' % name, members.keys()) > >> - disc_type =3D '%sKind' % (name) > >> + enum_define =3D discriminator_find_enum_define(expr) > >> + if enum_define: > >> + # Use the enum type as discriminator > >> + ret =3D "" > >> + disc_type =3D 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 =3D generate_visit_enum('%sKind' % name, members.keys()) > >> + disc_type =3D '%sKind' % (name) > >> =20 > >> if base: > >> base_fields =3D find_struct(base)['data'] > >> @@ -298,15 +304,16 @@ void visit_type_%(name)s(Visitor *m, %(name)s **= obj, const char *name, Error ** > >> pop_indent() > >> =20 > >> if not discriminator: > >> - desc_type =3D "type" > >> + disc_key =3D "type" > >> else: > >> - desc_type =3D discriminator > >> + disc_key =3D discriminator > >> ret +=3D mcgen(''' > >> - visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err); > >> + visit_type_%(disc_type)s(m, &(*obj)->kind, "%(disc_key)s", &e= rr); > >> if (!err) { > >> switch ((*obj)->kind) { > >> ''', > >> - name=3Dname, type=3Ddesc_type) > >> + disc_type =3D disc_type, > >> + disc_key =3D disc_key) > >> =20 > >> for key in members: > >> if not discriminator: > >> @@ -517,7 +524,11 @@ for expr in exprs: > >> ret +=3D generate_visit_list(expr['union'], expr['data']) > >> fdef.write(ret) > >> =20 > >> - ret =3D generate_decl_enum('%sKind' % expr['union'], expr['da= ta'].keys()) > >> + enum_define =3D discriminator_find_enum_define(expr) > >> + ret =3D "" > >> + if not enum_define: > >> + ret =3D generate_decl_enum('%sKind' % expr['union'], > >> + expr['data'].keys()) > >> ret +=3D 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'] > >> =20 > >> +# Return the discriminator enum define if discriminator is specified = as an > >> +# enum type, otherwise return None. > >> +def discriminator_find_enum_define(expr): > >> + base =3D expr.get('base') > >> + discriminator =3D expr.get('discriminator') > >> + > >> + if not (discriminator and base): > >> + return None > >> + > >> + base_fields =3D find_base_fields(base) > >> + if not base_fields: > >> + return None > >> + > >> + discriminator_type =3D base_fields.get(discriminator) > >> + if not discriminator_type: > >> + return None > >> + > >> + return find_enum(discriminator_type) > >> + > >> def check_union(expr, expr_info): > >> name =3D expr['union'] > >> base =3D 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) > >> =20 > >> + # Try again for hidden UnionKind enum > >> + for expr_elem in schema.exprs: > >> + expr =3D expr_elem['expr'] > >> + if expr.has_key('union'): > >> + try: > >> + enum_define =3D 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: >=20 > + # Try again for hidden UnionKind enum > + for expr_elem in schema.exprs: > + expr =3D expr_elem['expr'] > + if expr.has_key('union'): > + enum_define =3D discriminator_find_enum_define(expr) >=20 > 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. > >> + if not enum_define: > >> + add_enum('%sKind' % expr['union']) > >> + > >> try: > >> check_exprs(schema) > >> except QAPIExprError, e: > > [...] >=20