From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55401) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aM0m7-0005Ts-DU for qemu-devel@nongnu.org; Wed, 20 Jan 2016 16:58:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aM0m3-0004Tv-CS for qemu-devel@nongnu.org; Wed, 20 Jan 2016 16:58:47 -0500 References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-19-git-send-email-eblake@redhat.com> <87mvs0kr9m.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56A0030F.4090008@redhat.com> Date: Wed, 20 Jan 2016 14:58:39 -0700 MIME-Version: 1.0 In-Reply-To: <87mvs0kr9m.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="1TBJ7dvOachlsAUFiwfRwQt4DiIFxHOx8" Subject: Re: [Qemu-devel] [PATCH v9 18/37] qapi: Drop unused error argument for list and implicit struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Michael Roth , Alexander Graf , qemu-devel@nongnu.org, "open list:sPAPR" , marcandre.lureau@redhat.com, David Gibson This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --1TBJ7dvOachlsAUFiwfRwQt4DiIFxHOx8 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/20/2016 12:03 PM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> No backend was setting an error when ending the visit of a list >> or implicit struct. >=20 > That's a lie: qmp_input_end_list() does. But it shouldn't, as you > explain below. Rephrase the commit message? I'm not sure why you call it a lie - qmp_input_end_list() will not set an error unless it is mistakenly paired with a push(struct), which none of our code base does. Or put another way, although qmp_input_pop() [called by qmp_input_end_list()] has a signature that can set an error, closer inspection shows that it will only do so when invoked to close out a struct, and not when closing out a list. But that's a blatant programmer mismatch, which none of our code base does, so no well-formed use of visitors can cause qmp_input_end_list() to set an error. >=20 >> Make the callers a bit easier to follow by >> making this a part of the contract, and removing the errp >> argument - callers can then unconditionally end an object as >> part of cleanup without having to think about whether a second >> error is dominated by a first, because there is no second error. >> >> The only addition of &error_abort in this patch, in the function >> qmp_input_end_list(), will never trigger unless a programming >> bug creates a push(struct)/pop(list) or push(list)/pop(struct) >> mismatch. I'm open to wording suggestions. Maybe replace all of the above with: None of the existing .end_implicit_struct() implementations use errp. And of the existing .end_list() implementations, only qmp_input_end_list() even uses errp, but closer inspection shows that it will never be modified (errp is only passed to qmp_input_pop(), which will only set an error if the corresponding push was a struct rather than a list). We can turn that internal usage into an &error_abort, to protect against programmer mistakes of push(struct)/pop(list) or push(list)/pop(struct) mismatch. With that done, we can then make all public uses of visit_end_implicit_struct() and visit_end_list() easier to follow by removing the errp argument and making error-free operation part of the contract. Callers can then unconditionally end an object as part of cleanup without having to think about whether a second error is dominated by a first, because there is no possibility of a second error. >> >> A later patch will then tackle the larger task of splitting >> visit_end_struct(), which can indeed set an error (and that >> cleanup will also have the side-effect of removing the use of >> error_abort added here). >> >> Signed-off-by: Eric Blake >> Reviewed-by: Marc-Andr=C3=A9 Lureau >=20 > Patch looks good. I like the simplification. Would help to split this into two patches, one switching from qmp_input_pop(errp) into qmp_input_pop(&error_abort), and the other then removing unused errp argument? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --1TBJ7dvOachlsAUFiwfRwQt4DiIFxHOx8 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/ iQEcBAEBCAAGBQJWoAMPAAoJEKeha0olJ0Nq1HAH/3YDYDuMLLjHOo/Wrj5YzTf4 xGP9qh/+D8mKs9q5PKnHCr0C2kt7GtooDTuqNWNZW3Se9NMO5uRZpQP6D+6NCkHT DNDgsu7tVScuFiHj2P5VpWjSdpjee/UrU8YuB01KsOsWDPHTFv2u9ubw+SD0L/t0 4v9dpdW5o2pD1Bf6Q8RUARwzPY1FLFhh7LVI7AjzsfvsykolgIAhHcwchucuHZy0 +vuqPw0okqGrIL4qKecM/Ludsz6hQ/NDKfBLDVt0NPcTz2KubNR8OWgKoMr6eirR Sw0hdRZaUVIJGf7p5JlJv0DOKCK72FsYH7laeZaOaClZmaNbJgZ+hrPtQrpDDq8= =TqB1 -----END PGP SIGNATURE----- --1TBJ7dvOachlsAUFiwfRwQt4DiIFxHOx8--