From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56384) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoZSx-0002y8-37 for qemu-devel@nongnu.org; Tue, 20 Oct 2015 12:08:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoZSs-00056F-VE for qemu-devel@nongnu.org; Tue, 20 Oct 2015 12:08:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42328) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoZSs-000567-Kl for qemu-devel@nongnu.org; Tue, 20 Oct 2015 12:08:42 -0400 References: <1444968943-11254-1-git-send-email-eblake@redhat.com> <1444968943-11254-6-git-send-email-eblake@redhat.com> <8737x591xz.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56266704.2060403@redhat.com> Date: Tue, 20 Oct 2015 10:08:36 -0600 MIME-Version: 1.0 In-Reply-To: <8737x591xz.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="G6hHLjjjCmXPK6EuhhA2n02G5CMv35Vxu" Subject: Re: [Qemu-devel] [PATCH v9 05/17] qapi: Unbox base members List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Luiz Capitulino , Gerd Hoffmann , qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --G6hHLjjjCmXPK6EuhhA2n02G5CMv35Vxu Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/20/2015 06:09 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Rather than storing a base class as a pointer to a box, just >> store the fields of that base class in the same order, so that >> a child struct can be safely cast to its parent. ^^^^ >> Compare to the earlier commit 1e6c1616a "qapi: Generate a nicer >> struct for flat unions". >=20 > Should we mention the struct can be cast to its base? As in the ^^^ sentence above? Or did you mean somewhere additionally in the comments in the code below? >> -def gen_struct_fields(members): >> +def gen_struct_fields(members, base, nested=3DFalse): >> ret =3D '' >> + if base: >> + if not nested: >> + ret +=3D mcgen(''' >> + /* Members inherited from %(c_name)s: */ >> +''', >> + c_name=3Dbase.c_name()) >> + ret +=3D gen_struct_fields(base.local_members, base.base, Tru= e) >> + if not nested: >> + ret +=3D mcgen(''' >> + /* Own members: */ >> +''') >=20 > Before: gen_struct_fields() emits *own* fields. >=20 > After: it emits *all* fields. >=20 > Since the signature changes, all callers are visible in the patch, and > can be reviewed. >=20 > Code looks a bit awkward, but I don't have better ideas. Perhaps we ca= n > do better when we fuse gen_struct() and gen_union(). I haven't tried to do that fusion yet; but we are probably quite close to it. I'll see whether it is worth adding on as an 18/17 in this subset of the series, or saving for later. > This is gen_union(). >=20 >> ''', >> c_name=3Dc_name(name)) >> if base: >> - ret +=3D mcgen(''' >> - /* Members inherited from %(c_name)s: */ >> -''', >> - c_name=3Dc_name(base.name)) >> - ret +=3D gen_struct_fields(base.members) >> - ret +=3D mcgen(''' >> - /* Own members: */ >> -''') >> + ret +=3D gen_struct_fields([], base) >> else: >> ret +=3D mcgen(''' >> %(c_type)s kind; >=20 > Before: emit base fields, then own fields. >=20 > After: emit all fields. Good, but please mention in the commit message= > that you're touching gen_union(). You could also do this part in a > separate patch, but that's probably overkill. Sure, I can improve the commit message, and maybe split the patch. The idea at play is that both structs and unions want to emit all fields, as well as strategic comments about which fields are inherited, so a shared function is ideal for that; this patch moved code from gen_union() into gen_struct_fields() so that gen_struct() could reuse it. >> if base: >> - ret +=3D gen_visit_implicit_struct(base) >> + ret +=3D gen_visit_struct_fields(base.name, base.base, >> + base.local_members) >> >> ret +=3D mcgen(''' >> >=20 > This change looks innocent enough on first glance, but it's actually > quite hairy. >=20 > =3D Before =3D >=20 > The visit_type_FOO_fields() are generated in QAPISchema visit order, > i.e. right when QAPISchemaObjectType 'FOO' is visited. >=20 > The visit_type_implicit_FOO() are generated on demand, right before > their first use. It's used by visit_type_STRUCT_fields() when STRUCT > has base FOO, and by visit_type_UNION() when flat UNION has a member > FOO. >=20 > Aside: only flat unions, because simple unions are an ugly special case= > here. To be unified. >=20 > Generating visit_type_implicit_FOO() on demand makes sense, because the= > function isn't always needed. >=20 > Since visit_type_implicit_FOO() calls visit_type_FOO_fields(), and the > former can be generated before the latter, we may need a forward > declaration. struct_fields_seen is used to detect this. See commit > 8c3f8e7. >=20 > =3D After =3D >=20 > visit_type_implicit_FOO() is now used only when flat UNION has a member= > FOO. May need a forward declaration of visit_type_FOO_fields() anyway.= >=20 > visit_type_FOO_fields() is now also generated on demand, right before > first use other than visit_type_implicit_FOO(). Weird. >=20 > The resulting code motion makes the change to generated code difficult > to review. >=20 > Generating visit_type_FOO_fields() on demand makes less sense, because > the function is always needed. All it can accomplish is saving a few > forward declarations, at the cost of making gen_visit_struct_fields() > recursive, which begs the recursion termination question, and less > uniform generated code. >=20 > The naive answer to the recursion termination question is that types > can't contain themselves, therefore we can't ever get into a cycle. > That begs the next question: why do we need the if name in > struct_fields_seen conditional then? We do need it (turning it into an= > assertion promptly fails it), but I can't tell why offhand. >=20 > I'm sure I could figure out why this works, but I don't want to. Let's= > keep the code simple instead: keep generating visit_type_FOO_fields() i= n > QAPISchema visit order, emit necessary forward declarations. >=20 > Suggest to factor the code to emit a forward declaration out of > gen_visit_implicit_struct() and reuse it in gen_visit_struct_fields(). Sounds like I need to split this patch then, anyways. I'll see what I can come up with for v10. >> +++ b/tests/qapi-schema/qapi-schema-test.json >> @@ -40,6 +40,10 @@ >> 'data': { 'string0': 'str', >> 'dict1': 'UserDefTwoDict' } } >> >> +# ensure that we don't have an artificial collision on 'base' >> +{ 'struct': 'UserDefThree', >> + 'base': 'UserDefOne', 'data': { 'base': 'str' } } >> + > This is the positive test I challenged above. I can drop it if you don't like it; I felt better with it, but as you say, it only proves that 'base' is usable as a member name, and not that someone won't pick some other name when refactoring back into a boxed base. See also my comments below about using containers (rather than boxes or inlined members), where we may need to deal with the naming clash anyways. >> @@ -218,9 +216,11 @@ static void channel_event(int event, SpiceChannel= EventInfo *info) >> } >> >> if (info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) { >> - add_addr_info(client->base, (struct sockaddr *)&info->paddr_e= xt, >> + add_addr_info((SpiceBasicInfo *)client, >=20 > Ah, you're already exploiting the ability to cast to the base type! Absolutely :) >=20 > Idea: should we generate a type-safe macro or inline function for this?= Hmm. DO_UPCAST() (and its more powerful underlying container_of()) doesn't fit here, because we inlined the fields rather than directly including the base. Below, I'm using this qapi: {'struct':'Parent', 'data':{'i':'int'}} {'struct':'Child', 'base':'Parent', 'data':{'b':'bool'}} What we have without this patch is a box that requires separate allocation (two layers of pointers, the boxing that we don't want): struct Child { Parent *base; bool b; }; And this patch takes things to be completely inlined (since that's what we did earlier with flat unions), so that there is no intermediate structure in the child (and thus nothing for DO_UPCAST() to grab): struct Child { /* Inherited fields from Parent */ int i; /* own fields */ bool b; }; But there is a third possible layout (if we do it, we should do it for flat unions as well), which is using the base as a container rather than a box, where at least we don't need separate allocation: struct Child { Parent base; bool b; }; or similarly, but in longhand: struct Child { struct { int i; } base; bool b; }; but then we are back to having to avoid a collision with the C name 'base' with a QMP name in own fields, and all client code has to spell out child->base.i (instead of the boxed child->base->i, or the inlined child->i). There's also the ugly approach of exposing things in a dual naming system via an anonymous union and struct: struct Child { union { struct { int i; }; Parent base; }; bool b; }; which would allow 'child->i' to be the same storage as 'child->base.i', so that clients can use the short spelling when accessing fields but also have handy access to the base member for DO_UPCAST(). I'm not sure I want to go there, though. Taking your idea from another review message, you mentioned that we could allow 'base' by tweaking c_name() to munge the user's member 'base' into 'q_base', while reserving 'base' for ourselves; or we could use '_base' for our own use, which shouldn't collide with user's names (user names should not start with underscore except for downstream names which have double-underscore). Or use '_b' instead of 'base' - the point remains that if we want to avoid a collision but still use the container approach, we could pick the C name appropriately. But ultimately, we should either commit to the name munging pattern, or outlaw the name that we plan to use internally, or stick to inlined members that don't require munging in the first place. Meanwhile, if we decide that we like the convenience of inlined data, you are correct in the idea that we could have the qapi generator automatically spit out an appropriate type-specific upcast inline function for each type with a base. So given the same example, this might mean: static inline Parent *qapi_Child_to_Parent(Child *child) { return (Parent*)child; } But while such a representation would add compiler type-safety (hiding the cast in generated code, where we can prove the generator knew it was safe, and so that clients don't have to cast), it also adds verbosity. I can't think of any representation that would be shorter than making the clients do the cast, short of using a container rather than inline approach. Even foo(qapi_baseof_Child(child), blah) is longer than foo((Parent *)child, blah). Preferences? > Turned out nicely, I anticipated more churn. Yes, that was my feeling as well - we haven't quite used base classes in enough places to make the conversion painful - but the longer we wait, the more usage of base classes will sneak into .json files making a future conversion to a new layout more painful. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --G6hHLjjjCmXPK6EuhhA2n02G5CMv35Vxu 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/ iQEcBAEBCAAGBQJWJmcEAAoJEKeha0olJ0NquMwH+wbFBACGguQu08ajDE1Z+kHY 6O15Jy0Ueyfr04NwHAgh7dzgwsK/cX3eilAKiSDfGOfyzzbN+C5fVNSOPABdmjxg +GcImJQMceHX3X9Wtj8O5gp5xhAkBl/+dxDBmbq3HzGDDr1xXWgSw1jiToNE9KVV O7Fft7yqCfVBU/7rM1a5DOzi5cZnchk4AGmvzy2cDpVIznlq9nmRIcHaTpAwbaEO +vQ9/wyylF6vI59jJ1aUnuPC8iSNyQ/eRqWrw7Q0ut3o9eojnz1X0hhvahoqWI8V 4FabHjRiLyT4RpDEgs/7m1LooE1h3nCihEfpHxJ1qkWVkpdSDMTgqlMYTOIxuuM= =ez55 -----END PGP SIGNATURE----- --G6hHLjjjCmXPK6EuhhA2n02G5CMv35Vxu--