From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41078) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLwH2-000721-PY for qemu-devel@nongnu.org; Wed, 20 Jan 2016 12:10:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLwGy-0000ej-Ok for qemu-devel@nongnu.org; Wed, 20 Jan 2016 12:10:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40327) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLwGy-0000ef-GV for qemu-devel@nongnu.org; Wed, 20 Jan 2016 12:10:20 -0500 References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-8-git-send-email-eblake@redhat.com> <87y4bks2g7.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <569FBF7B.6090408@redhat.com> Date: Wed, 20 Jan 2016 10:10:19 -0700 MIME-Version: 1.0 In-Reply-To: <87y4bks2g7.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JI133Itb1RN9wjD8CXsMKOghi8SpeFHJ2" Subject: Re: [Qemu-devel] [PATCH v9 07/37] qapi: Improve generated event 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, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --JI133Itb1RN9wjD8CXsMKOghi8SpeFHJ2 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/20/2016 08:19 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> All other successful clients of visit_start_struct() were paired >> with an unconditional visit_end_struct(); but the generated >> code for events was relying on qmp_output_visitor_cleanup() to >> work on an incomplete visit. >=20 > Disgusting, isn't it? :) This, along with the fix in 5/37, were the two places that had to be fixed to avoid assertions in patch 24, when I turned on stricter enforcing of cleanup only on an evenly matched visit. >=20 >> Alter the code to guarantee that >> the struct is completed, which will make a future patch to >> split visit_end_struct() easier to reason about. While at it, >> drop some assertions and comments that are not present in other >> uses of the qmp output visitor, and pass NULL rather than "" as >> the 'kind' parameter (matching most other uses where obj is NULL). >> >> Signed-off-by: Eric Blake >> >> --- >> v9: save churn in declaration order for later series on boxed params, >> drop Marc-Andre's R-b >> v8: no change >> v7: place earlier in series, adjust handling of 'kind' >> v6: new patch >> >> If desired, I can defer the hunk re-ordering the declaration of >> obj to later in the series where it actually comes in handy. Dead sentence leftover from v8; as mentioned above, I DID sink the declaration reordering to a later series for v9. But it's after the ---, so it gets trimmed automatically by 'git am'. >> ret +=3D gen_err_check() >> - ret +=3D gen_visit_fields(arg_type.members, need_cast=3DTrue)= >> + ret +=3D gen_visit_fields(arg_type.members, need_cast=3DTrue,= >> + label=3D'out_obj') >=20 > On error, we now go to out_obj rather than out. Some fields will be > unvisited then (possibly all), and err will be set. >=20 > Now I get to figure out what this change changes. >=20 >> ret +=3D mcgen(''' >> +out_obj: >> visit_end_struct(v, &err); >> if (err) { >> goto out; >> } >=20 > Good: we actually call visit_end_struct() as we should. >=20 > Not so good: if we get here via the error exit of gen_visit_fields(), > err is set. If visit_end_struct() tries to set another error... Oops. It all gets cleaned up in 33 when visit_end_struct() loses the errp argument, but in the meantime, I think the most robust way to write this would be: out_obj: visit_end_struct(v, err ? NULL : &err); if (err) { =2E.. >=20 > I guess the idea is to go from gen_visit_fields() failure through > visit_end_struct() here to out. Correct? Yes. >> +++ b/scripts/qapi.py >> @@ -1636,7 +1636,8 @@ def gen_err_check(label=3D'out', skiperr=3DFalse= ): >> label=3Dlabel) >> >> >> -def gen_visit_fields(members, prefix=3D'', need_cast=3DFalse, skiperr= =3DFalse): >> +def gen_visit_fields(members, prefix=3D'', need_cast=3DFalse, skiperr= =3DFalse, >> + label=3D'out'): >=20 > Probably clearer than label=3DNone, but duplicates gen_err_check()'s > default. Fine with me. >=20 >> ret =3D '' >> if skiperr: >> errparg =3D 'NULL' > else: > errparg =3D '&err' >=20 > for memb in members: > if memb.optional: > ret +=3D mcgen(''' > if (visit_optional(v, "%(name)s", &%(prefix)shas_%(c_name)s)) { > ''', > prefix=3Dprefix, c_name=3Dc_name(memb.name)= , > name=3Dmemb.name, errp=3Derrparg) > push_indent() >=20 > errp=3Derrparg unused here. Not this patch's job to clean up. Bah. Commit 5cdc8831 missed it, due to repeated refactoring. I'm a bit surprised that pep8 didn't complain. Okay, I'm adding it as a separate cleanup. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --JI133Itb1RN9wjD8CXsMKOghi8SpeFHJ2 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/ iQEcBAEBCAAGBQJWn797AAoJEKeha0olJ0NqHuQH/3uxy0SUOH3E1optLuLBnY1h 5FDPxQyiGUUqe6fhwmYGOTrepNKtS5GG2UOOT1FWipLv6RYnx0QlQiKO50CDbUBd m+qMJK9tUMnTP4i9HkmPTZbHe9Jo3Cx22y0oYQxYjFzZKg9353lJIIpTVMcmDYLE 0OspXVfxoaJihnPwOdW+fCzxfB4w6XARbIgnj2q3HG4nq0gl7cmqZ9G3TeGWifjz kMja0auTES9J0/dKH7hmx1WuC3kWHYJ4fM1KUJFcUnkF7A2oCR8aBGSzcNS/IVsu ssgJp6glpEctBcpqig5/ZmLlyelkRVCYh9sp7PkfJc79QlZZb9Hw5gEudCtJ9XE= =8cFT -----END PGP SIGNATURE----- --JI133Itb1RN9wjD8CXsMKOghi8SpeFHJ2--