From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37310) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZI3LS-0005lC-Q9 for qemu-devel@nongnu.org; Wed, 22 Jul 2015 19:22:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZI3LO-0001Z1-AX for qemu-devel@nongnu.org; Wed, 22 Jul 2015 19:22:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37978) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZI3LO-0001Ym-1G for qemu-devel@nongnu.org; Wed, 22 Jul 2015 19:22:34 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-28-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <55B01923.9050907@redhat.com> Date: Wed, 22 Jul 2015 16:28:51 -0600 MIME-Version: 1.0 In-Reply-To: <1435782155-31412-28-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="WLlw1HHSKSpW1cHX2I0KMtrXhGDb8MWLG" Subject: Re: [Qemu-devel] [PATCH RFC v2 27/47] qapi-visit: Convert to QAPISchemaVisitor, fixing bugs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, berto@igalia.com, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --WLlw1HHSKSpW1cHX2I0KMtrXhGDb8MWLG Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/01/2015 02:22 PM, Markus Armbruster wrote: > Fixes flat unions to visit the base's base members (the previous > commit merely added them to the struct). Same test case. >=20 > Patch's effect on visit_type_UserDefFlatUnion(): >=20 > static void visit_type_UserDefFlatUnion_fields(Visitor *m, UserDef= FlatUnion **obj, Error **errp) > { > Error *err =3D NULL; >=20 > + visit_type_int(m, &(*obj)->integer, "integer", &err); > + if (err) { > + goto out; > + } > visit_type_str(m, &(*obj)->string, "string", &err); > if (err) { > goto out; >=20 > Test cases updated for the bug fix. >=20 > Fixes alternates to generate a visitor for their implicit enumeration > type. None of them are currently used, obviously. Example: > block-core.json's BlockdevRef now generates > visit_type_BlockdevRefKind(). >=20 > Signed-off-by: Markus Armbruster > --- > scripts/qapi-visit.py | 254 ++++++++++++------------= -------- > tests/qapi-schema/qapi-schema-test.json | 3 - > tests/test-qmp-input-strict.c | 2 +- > tests/test-qmp-input-visitor.c | 4 +- > 4 files changed, 100 insertions(+), 163 deletions(-) Another conversion that results in a fairly large diffstat to the generated files: qapi-visit.c | 4542 ++++++++++++++++++------------------ qapi-visit.h | 256 -- qga/qapi-generated/qga-qapi-visit.c | 88 qga/qapi-generated/qga-qapi-visit.h | 36 4 files changed, 2355 insertions(+), 2567 deletions(-) Same complaints as in 26/47, where splitting some of the cleanups into separate patches would make it easier to validate that the final conversion is correct. Here, a very common thing in the generated .c file is that you end up swapping the order of visit_type_foo and visit_type_fooList, for any time when foo is an enum. [1] >=20 > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index a52a572..135e7c1 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -12,7 +12,6 @@ > # This work is licensed under the terms of the GNU GPL, version 2. > # See the COPYING file in the top-level directory. > =20 > -from ordereddict import OrderedDict > from qapi import * > import re > =20 > @@ -24,13 +23,13 @@ def generate_visit_implicit_struct(type): > return '' > implicit_structs_seen.add(type) > ret =3D '' > - if type not in struct_fields_seen: > + if type.name not in struct_fields_seen: > # Need a forward declaration > ret +=3D mcgen(''' > =20 > static void visit_type_%(c_type)s_fields(Visitor *m, %(c_type)s **obj,= Error **errp); > ''', > - c_type=3Dtype_name(type)) > + c_type=3Dtype.c_name()) This looks a little fishy on first read; why are we calling type.c_name() (and not type.c_type()) when assigning to a placeholder named %(c_type)s? But on second thought, it looks correct: since we really do want the name of the type (and not the magic '*' pointer suffix= ). Still, it might be nicer to name things %(c_name)s here; and in that case, it's more of a pre-existing cleanup that might be better floating into one of your earlier patches. > =20 > ret +=3D mcgen(''' > =20 > @@ -46,7 +45,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *m= , %(c_type)s **obj, Error * > error_propagate(errp, err); > } > ''', > - c_type=3Dtype_name(type)) > + c_type=3Dtype.c_name()) same here. > return ret > =20 > def generate_visit_struct_fields(name, members, base =3D None): > @@ -74,24 +73,24 @@ if (err) { > goto out; > } > ''', > - type=3Dtype_name(base), c_name=3Dc_name('base')) > + type=3Dbase.c_name(), c_name=3Dc_name('base')) And this one's pointless: c_name('base') =3D=3D 'base'. Pointless since commit 622f557 introduced type inheritance. Why do we even need %(c_name)s if we are always passing a constant string? Oh, and that means our generator has a collision bug that none of my added tests have exposed yet: you cannot have a base class and simultaneously add a member named 'base': { 'struct': 'Base', 'data': { 'i': 'int' } } { 'struct': 'Sub', 'base': 'Base', 'data': { 'base': 'str' } } because the generated C code is trying to use the name 'base' for its own purposes. I guess that means more pre-req patches to the series to expose the bug, and either tighten the parser to reject things for now (easiest) or update the generator to not collide (harder, and fine for a later series). By the way, now that we are emitting flat unions in such a way that you can cast to the base class, why don't we change our C code to do likewise? That is, where we now have this generated C: struct BlockdevOptionsGenericFormat { BlockdevRef *file; }; struct BlockdevOptionsGenericCOWFormat { BlockdevOptionsGenericFormat *base; bool has_backing; BlockdevRef *backing; }; why can't we instead have an unboxed representation: struct BlockdevOptionsGenericFormat { BlockdevRef *file; }; /* This struct can be cast to BlockdevOptionsGenericFormat */ struct BlockdevOptionsGenericCOWFormat { BlockdevRef *file; /* end of fields from base class BlockdevOptionsGenericFormat */ bool has_backing; BlockdevRef *backing; }; where client code that was referring to o->base->file now refers to o->fi= le. > +def gen_visit_union(name, base, variants): > + ret =3D '' > =20 > if base: > - assert discriminator > - base_fields =3D find_struct(base)['data'].copy() > - del base_fields[discriminator] > - ret +=3D generate_visit_struct_fields(name, base_fields) > + members =3D [m for m in base.members if m !=3D variants.tag_me= mber] > + ret +=3D generate_visit_struct_fields(name, members) Why not just visit ALL base class members, unconditionally? > =20 > - if discriminator: > - for key in members: > - ret +=3D generate_visit_implicit_struct(members[key]) > + for var in variants.variants: > + if var.flat: > + ret +=3D generate_visit_implicit_struct(var.type) Okay, I see where you are using .flat from the initial parse. I still think it is a bit odd that you are defining '.flat' for each 'variant' within 'variants', even though, for a given 'variants', all members will have the same setting of '.flat'. That makes me wonder if '.flat' should belong instead to the top-level 'variants' struct rather than to each 'variant' member. But again I wonder what would happen if you had instead normalized the input of simple unions into always having an implicit struct (with single member 'data'), so that by the time you get here, you only have to deal with a single representation of unions instead of having to still emit different things for flat vs. simple (since on the wire, we already proved simple is shorthand that can be duplicated by a flat union= ). > =20 > ret +=3D mcgen(''' > =20 > @@ -300,41 +268,39 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s= **obj, const char *name, Error > ''', > name=3Dc_name(name)) > =20 > - if not discriminator: > - tag =3D 'kind' > - disc_key =3D "type" > - else: > - tag =3D discriminator > - disc_key =3D discriminator > + disc_key =3D variants.tag_member.name > + if not variants.tag_name: > + # we pointlessly use a different key for simple unions We could fix that (as a separate patch); wonder how much C code it would affect. A lot of these things that we can alter in generated code are certainly easier to see now that we have a clean generator :) > +def gen_visit_decl(name, scalar=3DFalse): > + c_type =3D c_name(name) + ' *' > + if not scalar: > + c_type +=3D '*' > return mcgen(''' > - > -void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, = Error **errp); > +void visit_type_%(c_name)s(Visitor *m, %(c_type)sobj, const char *name= , Error **errp); > ''', > - name=3Dc_name(name)) > + c_name=3Dc_name(name), c_type=3Dc_type) Nice way to consolidate several near-identical copies. > + > +class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): > + def __init__(self): > + self.decl =3D None > + self.defn =3D None > + self.btin =3D None > + def visit_begin(self): > + self.decl =3D '' > + self.defn =3D '' > + self.btin =3D guardstart('QAPI_VISIT_BUILTIN_VISITOR_DECL') > + def visit_end(self): > + # to avoid header dependency hell, we always generate > + # declarations for built-in types in our header files and > + # simply guard them > + self.btin +=3D guardend('QAPI_VISIT_BUILTIN_VISITOR_DECL') > + self.decl =3D self.btin + self.decl > + self.btin =3D None > + # ...this doesn't work for cases where we link in multiple > + # objects that have the functions defined, so we use > + # do_builtins (option -b) to provide control And once again, as in 26/47, this floats the .h file to have all builtin representations in one chunk (for continuity with pre-patch), but fails to do the same for the .c code... > + def visit_enum_type(self, name, info, values): > + self.decl +=3D gen_visit_decl(name, scalar=3DTrue) > + self.defn +=3D generate_visit_enum(name) > + def visit_array_type(self, name, info, element_type): > + decl =3D gen_visit_decl(name) > + defn =3D gen_visit_list(name, element_type) > + if isinstance(element_type, QAPISchemaBuiltinType): > + self.btin +=3D decl > + if do_builtins: > + self.defn +=3D defn =2E..where the builtins are now interleaved with everything else instead of bunched together, making the generated diff larger and more confusing than necessary. > + else: > + self.decl +=3D decl > + self.defn +=3D defn > + def visit_object_type(self, name, info, base, members, variants): > + if info: > + self.decl +=3D gen_visit_decl(name) > + if variants: > + self.defn +=3D gen_visit_union(name, base, variants) Worth adding 'assert not members'? > + else: > + self.defn +=3D gen_visit_struct(name, base, members) Or maybe we can someday consolidate these two into a single gen_visit_object, that handles all members and variants in a uniform manner, instead of our current differences. I wonder how much C code would be impacted? > + def visit_alternate_type(self, name, info, variants): > + self.decl +=3D gen_visit_decl(name) > + self.defn +=3D gen_visit_alternate(name, variants) > =20 > do_builtins =3D False > =20 > @@ -442,56 +428,10 @@ fdecl.write(mcgen(''' > ''', > prefix=3Dprefix)) > =20 > -exprs =3D QAPISchema(input_file).get_exprs() > - > -# to avoid header dependency hell, we always generate declarations > -# for built-in types in our header files and simply guard them > -fdecl.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DECL")) > -for typename in builtin_types.keys(): > - fdecl.write(generate_declaration(typename, builtin_type=3DTrue)) > -fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL")) > - > -# ...this doesn't work for cases where we link in multiple objects tha= t > -# have the functions defined, so we use -b option to provide control > -# over these cases > -if do_builtins: > - for typename in builtin_types.keys(): > - fdef.write(generate_visit_list(typename)) Again, a well-placed sorted() over these two loops in a pre-req patch will minimize the churn on the builtins. > - > -for expr in exprs: > - if expr.has_key('struct'): > - ret =3D generate_visit_struct(expr) > - ret +=3D generate_visit_list(expr['struct']) > - fdef.write(ret) > - > - ret =3D generate_declaration(expr['struct']) > - fdecl.write(ret) > - elif expr.has_key('union'): > - ret =3D generate_visit_union(expr) > - ret +=3D generate_visit_list(expr['union']) > - fdef.write(ret) > - > - enum_define =3D discriminator_find_enum_define(expr) > - ret =3D "" > - if not enum_define: > - ret =3D generate_decl_enum('%sKind' % expr['union']) Nice that the new visitor automatically visits any implicit enum, without us having to special case it. > - ret +=3D generate_declaration(expr['union']) > - fdecl.write(ret) > - elif expr.has_key('alternate'): > - ret =3D generate_visit_alternate(expr['alternate'], expr['data= ']) > - ret +=3D generate_visit_list(expr['alternate']) > - fdef.write(ret) > - > - ret =3D generate_decl_enum('%sKind' % expr['alternate']) > - ret +=3D generate_declaration(expr['alternate']) > - fdecl.write(ret) > - elif expr.has_key('enum'): > - ret =3D generate_visit_list(expr['enum']) > - ret +=3D generate_visit_enum(expr['enum']) [1] swapping these two lines in a pre-req patch will minimize the churn of this conversion. > - fdef.write(ret) > - > - ret =3D generate_decl_enum(expr['enum']) > - ret +=3D generate_enum_declaration(expr['enum']) > - fdecl.write(ret) > +schema =3D QAPISchema(input_file) > +gen =3D QAPISchemaGenVisitVisitor() > +schema.visit(gen) > +fdef.write(gen.defn) > +fdecl.write(gen.decl) Again, overall impression is that your series is headed in the right direction. And nice that the TODOs in the testsuite pointed out what this fixes, for visiting indirect bases. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --WLlw1HHSKSpW1cHX2I0KMtrXhGDb8MWLG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVsBkjAAoJEKeha0olJ0NqY4IH/0lXyujVjOoQBR+xs50APKD6 7tqsdngRinFAYVTHMAG9QBeWE6xEEUKZo+Hgd4MqKETvVm03Wp9ggH62/GCMEN/G 6aUeJhs5fwXHrNRhPKww4x2MMtltmEHRhkXOPm6cUCOciuNSJ8LgqDazcdXRr5sQ WZGd3/TSOFepS/jppNoK+i2JniRmiY+DgXB82oiy+RNzzdeBMtrWFqzgdjuzejEB mobu+zUMbjsKj2jtTX0KO6xFw8xMRZg5mHgWQ7SDNCuFBHJipyVhqD3zlm2zziqK y+DUZ6QaWMHqfdUtwSqSjMRcYAHG3J2wDCb7HLXiyXmKNRFOEZpvOk3Slr5hyrE= =7LpI -----END PGP SIGNATURE----- --WLlw1HHSKSpW1cHX2I0KMtrXhGDb8MWLG--