From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42197) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYzDX-0005Pj-QD for qemu-devel@nongnu.org; Thu, 25 Feb 2016 11:56:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYzDT-0005XY-NP for qemu-devel@nongnu.org; Thu, 25 Feb 2016 11:56:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46513) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYzDT-0005Wv-ES for qemu-devel@nongnu.org; Thu, 25 Feb 2016 11:56:39 -0500 References: <1456262075-3311-1-git-send-email-eblake@redhat.com> <1456262075-3311-3-git-send-email-eblake@redhat.com> <87vb5expg9.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56CF3245.3080805@redhat.com> Date: Thu, 25 Feb 2016 09:56:37 -0700 MIME-Version: 1.0 In-Reply-To: <87vb5expg9.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wb2HjgfTABRG815S2AXCeNRCUX8T8ni3o" Subject: Re: [Qemu-devel] [PATCH 2/3] qapi-visit: Expose visit_type_FOO_fields() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --wb2HjgfTABRG815S2AXCeNRCUX8T8ni3o Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/24/2016 05:28 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Dan Berrange reported a case where he needs to work with a >> QCryptoBlockOptions union type using the OptsVisitor, but only >> visit one of the branches of that type (the discriminator is not >> visited directly, but learned externally). When things were >> boxed, it was easy: just visit the variant directly, which took >> care of both allocating the variant and visiting its fields. But >> now that things are unboxed, we need a way to visit the fields >> without allocation, done by exposing visit_type_FOO_fields() to >> the user. Of course, this should only be done for objects, not >> lists, so we need another flag to gen_visit_decl(). >> >> Since the function is now public, we no longer need to preserve >> topological ordering via struct_fields_seen. >> >> Signed-off-by: Eric Blake >> >> --- >> Minor conflicts with pending series "qapi implicit types"; I can >> rebase whichever series gets reviewed second. Sounds like you want this in first; I'll respin a single series with both sets of patches, with this at the front. >> --- >> scripts/qapi-visit.py | 47 +++++++++++++-----------------------------= ----- >> 1 file changed, 13 insertions(+), 34 deletions(-) >> >=20 > Let me review how this thing works for an object type FOO before your > patch. >=20 > gen_visit_object() generates visit_type_FOO() with external linkage, an= d > gen_visit_struct_fields() generates visit_type_FOO_fields() with > internal linkage. >=20 > gen_visit_decl() generates a declaration of visit_type_FOO(), and > gen_visit_fields_decl() generates one for visit_type_FOO_fields() unles= s > it's already in scope. >=20 > visit_type_FOO_fields() is always called by visit_type_FOO(), and > sometimes called elsewhere. >=20 > We generate visit_type_FOO_fields() right before visit_type_FOO(), so > it's in scope there. Anything that generates uses elsewhere must call > gen_visit_fields_decl(). >=20 > Your patch generates visit_type_FOO_fields() declarations into the > header, which renders the "if already in scope" logic useless, along > with the need to call gen_visit_fields_decl() before generating > visit_type_FOO_fields() uses outside visit_type_FOO(). Correct. I'll try and incorporate some of this text in the commit message for better justification. >> +def gen_visit_decl(name, scalar=3DFalse, list=3DFalse): >> + ret =3D '' >> c_type =3D c_name(name) + ' *' >> if not scalar: >> + if not list: >> + ret +=3D mcgen(''' >> +void visit_type_%(c_name)s_fields(Visitor *v, %(c_type)sobj, Error **= errp); >> +''', >> + c_name=3Dc_name(name), c_type=3Dc_type) >=20 > This folds gen_visit_fields_decl() into gen_visit_decl() with the > necessary changes: drop the "is already in scope" conditional, switch t= o > external linkage. >=20 > Since gen_visit_decl() is called for non-object types as well, it gets = a > new optional parameter to suppress generation of > visit_type_FOO_fields(). The parameter is named @list, which makes no > sense to me. See more below. >=20 > I don't like do-everything functions with suppressor flags much. Can w= e > structure this as a set of do-one-thing functions? Possibly with > convenience functions collecting common sets. Sure; gen_visit_decl() stays unchanged, and gen_visit_decl_fields() would be new and called only for objects. >=20 > def visit_alternate_type(self, name, info, variants): > self.decl +=3D gen_visit_decl(name) >=20 > Bug: here too. Indeed - the declaration doesn't hurt, but there is no implementation to back up the declaration, so it's better if I fix things to not add the declaration. v2 will fix it. >=20 > $ grep _BlockdevRef_fields q*[ch] > qapi-visit.h:void visit_type_BlockdevRef_fields(Visitor *v, Blockde= vRef *obj, Error **errp); >=20 > self.defn +=3D gen_visit_alternate(name, variants) >=20 > Your choice of an optional argument to keep things unchanged effectivel= y > hid the places that changed. Failed to fool me today, but don't expect= > to remain lucky that way :) >=20 > One more thing: I never liked the name _fields. C struct and union > types don't have fields, they have members. Likewiese, JSON objects. > We shouldn't gratitously invent terminology. We've done that quite a > bit around QMP (JSON object vs. C QDict, JSON array vs. QList, QFLoat i= s > really a double, ...). Cleaning that up completely is probably hopeles= s > by now. But we can rename visit_type_FOO_fields() to something more > sensible, say visit_type_FOO_members(), or even > visit_type_FOO_unboxed(). visit_type_FOO_members() sounds slightly nicer to me. I'll use that terminology in v2 to see how things look. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --wb2HjgfTABRG815S2AXCeNRCUX8T8ni3o 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/ iQEcBAEBCAAGBQJWzzJFAAoJEKeha0olJ0NqhR8H+wTf5cSJDasMm4iOwyPfjylv 0LzIZYUsmpbpZWqDbkyPWMP4djYF+EVCNqzA0m00EwN8QEBz8BbXUScH+W/irm7j qHbXqtQ9Df14zIA6XDu6eydnh7OhKk2/4wGp9GQY05kmXmmFbBza/kbvcwCujVkg kg3e7cZweissBJlmwDIKvOomRohgdTZAQ/DbCwNPLeAqdTuve6zHflG7txaimQPa rZLZS73rfSlsIhMuPQclTa/uI4qLuTjPNjqeKmtzlU5oaUgDXJF2XUVm5aMRvwzH AIUwTSM9LXyEk01MyTNUtkAl5H0isSY9AVDkV47xe8ouBnT9XJQvARNzPP6kpqA= =VMPu -----END PGP SIGNATURE----- --wb2HjgfTABRG815S2AXCeNRCUX8T8ni3o--