From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57332) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLvSn-0007TY-Li for qemu-devel@nongnu.org; Wed, 20 Jan 2016 11:18:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLvSj-0005Pp-EX for qemu-devel@nongnu.org; Wed, 20 Jan 2016 11:18:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35166) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLvSj-0005Pl-71 for qemu-devel@nongnu.org; Wed, 20 Jan 2016 11:18:25 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 6D082344EB1 for ; Wed, 20 Jan 2016 16:18:24 +0000 (UTC) References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-6-git-send-email-eblake@redhat.com> <87egdcwemf.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <569FB34F.1030006@redhat.com> Date: Wed, 20 Jan 2016 09:18:23 -0700 MIME-Version: 1.0 In-Reply-To: <87egdcwemf.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="LQNe1915RJXPn3GePVH3CHSXbpDqCn1O5" Subject: Re: [Qemu-devel] [PATCH v9 05/37] vl: Improve use of qapi visitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --LQNe1915RJXPn3GePVH3CHSXbpDqCn1O5 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/20/2016 06:43 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Cache the visitor in a local variable instead of repeatedly >> calling the accessor. Pass NULL for the visit_start_struct() >> object (which matches the fact that we were already passing 0 >> for the size argument, because we aren't using the visit to >> allocate a qapi struct). Guarantee that visit_end_struct() >> is called if visit_start_struct() succeeded. >=20 > Three separate things --- you're pushing it now :) Two of them the same as in 4/37. So, among the five items, I did a split in two based on file. I could split in the other direction: fix hmp and vl to cache their visitor fix hmp and vl to pass NULL to avoid pointless allocation fix vl to guarantee visit_end_struct >=20 > Impact of not calling visit_end_struct()? Assertion failures after patch 24. :) Basically, I wanted to see whether the code base could get simpler if we always enforced visit start/end grouping, and there were only a couple of outliers that needed fixing (here, and in patch 7). >> >> qdict_del(pdict, "qom-type"); >> - visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err); >> + visit_type_str(v, &type, "qom-type", &err); >> if (err) { >> goto out; >> } >=20 > If we get here, we must call visit_end_struct(). >=20 >> if (!type_predicate(type)) { >> + visit_end_struct(v, NULL); >=20 > Here, you add the previously missing visit_end_struct() to the error > path. >=20 >> goto out; >> } >> >> qdict_del(pdict, "id"); >> - visit_type_str(opts_get_visitor(ov), &id, "id", &err); >> + visit_type_str(v, &id, "id", &err); >> if (err) { >> - goto out; >> + goto out_end; >=20 > Here, you skip to the function's cleanup, which now includes > visit_end_struct(). >=20 > Such a mix is can be a sign the cleanup isn't quite in the right order.= >=20 > When we take this error path to out_end, then: >=20 > out_end: > visit_end_struct(v, &err_end); // called, as we must > if (!err && err_end) { // !err is false > qmp_object_del(id, NULL); // not called > } > error_propagate(&err, err_end); // since err, this is > // error_free(err_end) Correct. And it gets simpler later on, when visit_end_struct() loses the errp argument (patch 33). >=20 >> } >> >> - object_add(type, id, pdict, opts_get_visitor(ov), &err); >> - if (err) { >> - goto out; >> - } >> - visit_end_struct(opts_get_visitor(ov), &err); >> - if (err) { >> + object_add(type, id, pdict, v, &err); >> + >> +out_end: >> + visit_end_struct(v, &err_end); >=20 > visit_end_struct() must be called when visit_start_struct() succeeded. > All error paths after that success pass through here, except for one, > and that one calls visit_end_struct(). Okay. >=20 >> + if (!err && err_end) { >> qmp_object_del(id, NULL); >> } >=20 > qmp_object_del() must be called when we fail after object_add() > succeeded. That's what the condition does. >=20 >> + error_propagate(&err, err_end); >> >> out: >=20 > Cleanup below here must be done always. >=20 >> opts_visitor_cleanup(ov); >=20 > The only reason I'm not asking you to rewrite this mess is that you're > already rewriting it later in this series. So I think you found patch 33. >=20 >> @@ -2867,7 +2870,6 @@ out: >> QDECREF(pdict); >> g_free(id); >> g_free(type); >> - g_free(dummy); >> if (err) { >> error_report_err(err); >> return -1; >=20 > I wonder whether splitting this and the previous patch along the change= > rather than the file would come out nicer: >=20 > 1. Cache the visitor >=20 > 2. Suppress the pointless allocation >=20 > 3. Fix to call visit_end_struct() >=20 What do you know - we're thinking on the same lines :) [And now you know I just replied to your email in the order that I read it, rather than reading the whole thing first] --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --LQNe1915RJXPn3GePVH3CHSXbpDqCn1O5 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/ iQEcBAEBCAAGBQJWn7NPAAoJEKeha0olJ0NqZ0kH/Ai8Kgq7faTiEjRS2Ji0HFio jFDcREyXGnajJmKvbLAfIkIJA2UvA9Y7q3Yr4/oVMnakprDExQpud7bCf53YH9io hEbTWxAtl2c1AY5RHuazO5l5qgLGruOIUMciGi/EPlfElR9qbkJuY0FK0vcxe6wd vUACkycscsLuavPLlsGKCTS3LmALkAShCoT2c0+hBKm4D70k88v4VO15IOmvz1tE WfAOfU5mPVFErDd8tSKhg5SZJLgCbeLY6dPa3UXByfYbQUxR6zgyyCAxJSs9gVbv rG290c/BDLhsbQwK615mOYxbkvO11RsoNdcwOotft0MPHtmaBl5WJCax0XT7WWQ= =/am2 -----END PGP SIGNATURE----- --LQNe1915RJXPn3GePVH3CHSXbpDqCn1O5--