From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45791) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVlgW-0000Ly-5O for qemu-devel@nongnu.org; Tue, 16 Feb 2016 14:53:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVlgQ-00063W-UP for qemu-devel@nongnu.org; Tue, 16 Feb 2016 14:53:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33697) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVlgQ-00063Q-KT for qemu-devel@nongnu.org; Tue, 16 Feb 2016 14:53:14 -0500 References: <1455582057-27565-1-git-send-email-eblake@redhat.com> <1455582057-27565-11-git-send-email-eblake@redhat.com> <87io1oo4o5.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56C37E29.6040500@redhat.com> Date: Tue, 16 Feb 2016 12:53:13 -0700 MIME-Version: 1.0 In-Reply-To: <87io1oo4o5.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="CKudvLCUwwVbvQ2OErTP70pJShj7m7Tn5" Subject: Re: [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate 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) --CKudvLCUwwVbvQ2OErTP70pJShj7m7Tn5 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/16/2016 12:07 PM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> There's no reason to do two malloc's for an alternate type visiting >> a QAPI struct; let's just inline the struct directly as the C union >> branch of the struct. >> >> Surprisingly, no clients were actually using the struct member prior >> to this patch; some testsuite coverage is added to avoid future >> regressions. >> >> Ultimately, we want to do the same treatment for QAPI unions, but >> as that touches a lot more client code, it is better as a separate >> patch. So in the meantime, I had to hack in a way to test if we >> are visiting an alternate type, within qapi-types:gen_variants(); >> the hack is possible because an earlier patch guaranteed that all >> alternates have at least two branches, with at most one object >> branch; the hack will go away in a later patch. >=20 > Suggest: >=20 > Ultimately, we want to do the same treatment for QAPI unions, but as > that touches a lot more client code, it is better as a separate patch= =2E > The two share gen_variants(), and I had to hack in a way to test if w= e > are visiting an alternate type: look for a non-object branch. This > works because alternates have at least two branches, with at most one= > object branch, and unions have only object branches. The hack will g= o > away in a later patch. Nicer. >=20 >> The generated code difference to qapi-types.h is relatively small, >> made possible by a new c_type(is_member) parameter in qapi.py: >=20 > Let's drop the "made possible" clause here. I was trying to document the new is_member parameter somewhere in the commit message, but I can agree that it could be its own paragraph rather than a clause here. >=20 > This is in addition to visit_type_BlockdevOptions(), so we need another= > name. >=20 > I can't quite see how the function is tied to alternates, though. >=20 I'm open to naming suggestions. Also, I noticed that we have 'visit_type_FOO_fields' and 'visit_type_implicit_FOO'; so I didn't know whether to name it 'visit_type_alternate_FOO' or 'visit_type_FOO_alternate'; it gets more interesting later in the series when I completely delete 'visit_type_implicit_FOO'. >> >> and use it like this: >> >> | switch ((*obj)->type) { >> | case QTYPE_QDICT: >> |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &= err); >> |+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.def= inition, &err); >=20 > Let's compare the two functions. First visit_type_BlockdevOptions(): >=20 >=20 > Now visit_type_alternate_BlockdevOptions(), with differences annotated > with //: >=20 > static void visit_type_alternate_BlockdevOptions(Visitor *v, > const char *name, > BlockdevOptions *obj, // one * less= Yep, because we no longer need to malloc a second object, so we no longer need to propagate a change to obj back to the caller. > Error **errp) > { > Error *err =3D NULL; >=20 > visit_start_struct(v, name, NULL, 0, &err); // NULL instead of = &obj > // suppresses mallo= c > if (err) { > goto out; > } > // null check dropped (obj can't be null) > visit_type_BlockdevOptions_fields(v, obj, &err); Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again, because we have one less level of indirection, and 7/13 reduced the indirection required in visit_type_FOO_fields()). > // visit_start_union() + switch dropped > error_propagate(errp, err); > err =3D NULL; > visit_end_struct(v, &err); > out: > error_propagate(errp, err); > } >=20 > Why can we drop visit_start_union() + switch? visit_start_union() is dropped because its only purpose was to determine if the dealloc visitor needs to visit the default branch. When we had a separate allocation, we did not want to visit the branch if the discriminator was not successfully parsed, because otherwise we would dereference NULL. But now that we don't have a second pointer allocation, we no longer have anything to dealloc, and we can no longer dereference NULL. Explained better in 12/13, where I delete visit_start_union() altogether. But maybe I could keep it in this patch in the meantime, to minimize confusion. Dropped switch, on the other hand, looks to be a genuine bug. Eeek. That should absolutely be present, and it proves that our testsuite is not strong enough for not catching me on it. And now that you've made me think about it, maybe I have yet another idea. Right now, we've split the visit of local members into visit_type_FOO_fields(), while leaving the variant members to be visited in visit_type_FOO() visit_type_FOO_fields() is static, so we can change it without impacting the entire tree; I could add a bool parameter to that function, and write= : visit_type_FOO() { visit_start_struct(obj) visit_type_FOO_fields(, false) visit_end_struct() } visit_type_FOO_fields(, wrap) { if (wrap) { visit_start_struct(NULL) } visit local fields visit variant fields if (wrap) { visit_end_struct() } } and for the variant type that includes FOO as one of its options, visit_type_VARIANT() { allocate (currently spelled visit_start_implicit_struct, but see 13/13)= switch (type) { case QTYPE_QINT: visit_type_int(); break case QTYPE_QDICT: visit_type_FOO_fields(, true); break } visit_end } or not even create a new function, and just have: visit_type_FOO() { visit_start_struct(obj) visit_type_FOO_fields() visit_end_struct() } visit_type_FOO_fields() { visit local fields visit variant fields } and for the variant type, visit_type_VARIANT() { allocate (currently spelled visit_start_implicit_struct, but see 13/13)= switch (type) { case QTYPE_QINT: visit_type_int(); break case QTYPE_QDICT: visit_start_struct(NULL) visit_type_FOO_fields() visit_end_struct() break } visit_end_implicit } where the logic gets a bit more complicated to do correct error handling.= >> @@ -984,8 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType): >> assert not self.is_implicit() >> return QAPISchemaType.c_name(self) >> >> - def c_type(self, is_param=3DFalse): >> + def c_type(self, is_param=3DFalse, is_member=3DFalse): >> assert not self.is_implicit() >> + if is_member: >> + return c_name(self.name) >> return QAPISchemaType.c_type(self) >=20 > To understand this, you have to peel off OO abstraction: > QAPISchemaType.c_type(self) returns c_name(self.name) + pointer_suffix.= >=20 > I think we should keep it peeled off in the source code. By this, you mean I should do: def c_type(self, is_param=3DFalse, is_member=3DFalse): assert not self.is_implicit() if is_member: return c_name(self.name) return c_name(self.name) + pointer_suffix ? or that I should hoist the suppression of pointer_suffix into the parent class? >=20 >> >> def json_type(self): > [tests/ diff looks good] good for what they caught, but they didn't catch enough :) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --CKudvLCUwwVbvQ2OErTP70pJShj7m7Tn5 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/ iQEcBAEBCAAGBQJWw34pAAoJEKeha0olJ0NqtfEH/AlZMvP51x1ikU5QBsxfzDXw ftT4CeydIr6Qt33XpKE38bwVt3dai7tc34BPS5Qa5ys2uPmAImQ0tfMQ2yIEfvdp 5iamUj1Js7RfkADtd4ajyq5m4GcsURgl1El03w16i6bqS8BsIBO4sCHKkieC7v8v k63RxrlRMK7dckFHQtBe+IlU5Z+CmGV4/QmEBSCnmTWcRe7I3OXq9Xl73WezPMee tqc09KDlyyx8jnOuoaJkmR7XOP+g+9TPcqJKEIrTCUCYY47hZzqdMT70gD1tDYKS i8Wqfo8AjWOF1U7pd2OP0htGxX3ujAknrl6aMx56NCJI6+g9T6+Mc5MyMent/NU= =QPM8 -----END PGP SIGNATURE----- --CKudvLCUwwVbvQ2OErTP70pJShj7m7Tn5--