From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41259) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zf9AR-00063x-6y for qemu-devel@nongnu.org; Thu, 24 Sep 2015 12:14:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zf9AN-0001DE-Un for qemu-devel@nongnu.org; Thu, 24 Sep 2015 12:14:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51582) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zf9AN-0001D2-NK for qemu-devel@nongnu.org; Thu, 24 Sep 2015 12:14:39 -0400 References: <1442872682-6523-1-git-send-email-eblake@redhat.com> <1442872682-6523-8-git-send-email-eblake@redhat.com> <87r3lneuet.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <5604216D.2010509@redhat.com> Date: Thu, 24 Sep 2015 10:14:37 -0600 MIME-Version: 1.0 In-Reply-To: <87r3lneuet.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="uILnNkw8NvHgS4dNfRPKvEe7E5JAmRCGp" Subject: Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, DirtY.iCE.hu@gmail.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --uILnNkw8NvHgS4dNfRPKvEe7E5JAmRCGp Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/24/2015 08:58 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Due to the existing semantics of the error_set() family, >> generated sequences in the qapi visitors such as: >> >> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >> if (!err) { >> visit_type_FOO_fields(m, obj, errp); >> visit_end_implicit_struct(m, &err); >> } >> error_propagate(errp, err); >=20 > Indentation seems off. Intentional? No, probably due to rebase churn (I reindented generated code in 9/46, but the series as I worked on it wasn't always in the order presented here). Will fix. >=20 >> >> are risky: if visit_type_FOO_fields() sets errp, and then >> visit_end_implicit_struct() also encounters an error, the >> attempt to overwrite the first error will cause an abort(). >> Obviously, we weren't triggering this situation (none of the >> existing callbacks for visit_end_implicit_struct() currently >> try to set an error), but it is better to not even cause the >> problem in the first place. >=20 > The code works, but it sets a problematic example. >=20 >> Meanwhile, in spite of the poor documentation of the qapi >> visitors, we want to guarantee that if a visit_start_*() >> succeeds, then the matching visit_end_*() will be called. >=20 > Agreed. >=20 >> The options are to either propagate and clear a local err >> multiple times: >> >> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >> if (!err) { >> visit_type_FOO_fields(m, obj, &err); >> if (err) { >> error_propagate(errp, err); >> err =3D NULL; >> } >> visit_end_implicit_struct(m, &err); >> } >> error_propagate(errp, err); More poor indentation on my part. >> >> or, as this patch does, just pass in NULL to ignore further >> errors once an error has occurred. >> >> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >> if (!err) { >> visit_type_FOO_fields(m, obj, &err); >> visit_end_implicit_struct(m, err ? NULL : &err); >> } >> error_propagate(errp, err); >=20 > Hmmmmm... not sure we do this anywhere else, yet. The ternary isn't > exactly pretty, but the intent to ignore additional error is clear > enough, I think. >=20 > If we elect to adopt this new error handling pattern, we should perhaps= > document it in error.h. >=20 > Third option: drop visit_end_implicit_struct()'s errp parameter. If we= > find a compelling use for it, we'll put it back and solve the problem. >=20 Ooh, interesting idea. It changes the contract - but since the contract isn't (yet) documented, and happens to work with existing uses without a contract, it could indeed be nicer. It would have knock-on effects to 24/46 where I first try documenting the contract. >> visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), = &err); >> if (!err) { >> - visit_type_%(c_type)s_fields(m, obj, errp); >> - visit_end_implicit_struct(m, &err); >> + visit_type_%(c_type)s_fields(m, obj, &err); >> + visit_end_implicit_struct(m, err ? NULL : &err); >> visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_= name)s), &err); >> if (!err) { >> if (*obj) { >> - visit_type_%(c_name)s_fields(m, obj, errp); >> + visit_type_%(c_name)s_fields(m, obj, &err); >> } >> - visit_end_struct(m, &err); >> + visit_end_struct(m, err ? NULL : &err); >> } >> error_propagate(errp, err); >> } >=20 > Oh, it's about visit_end_struct(), too. Commit message only talks abou= t > visit_end_implicit_struct(). >=20 > In particular, "none of the existing callbacks for > visit_end_implicit_struct() currently try to set an error". Does that > hold for visit_end_struct() callbacks, too? I'm fairly certain that ALL of the visit_end_* callbacks were similar in nature, but you've prompted me to re-audit things and update the commit message to be absolutely clear about it. >=20 >> @@ -175,9 +175,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s = **obj, const char *name, Error >> visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err); >> } >> >> - error_propagate(errp, err); >> - err =3D NULL; >> - visit_end_list(m, &err); >> + visit_end_list(m, err ? NULL : &err); >> out: >> error_propagate(errp, err); >> } >=20 > Likewise. Does it hold for visit_end_list() callbacks, too? >=20 > Looks like you switch from option 1 to option 2 here. Your slate isn't= > as clean as the commit message suggests :) Consistency is nice, but documenting where we started from to get to the consistent state would be even nicer. Point taken. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --uILnNkw8NvHgS4dNfRPKvEe7E5JAmRCGp 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/ iQEcBAEBCAAGBQJWBCFuAAoJEKeha0olJ0NqNc0IAKq4dmuO9e/MUyPbxyqwQduo 89yNW18l+KT1taN2mhgHeuvrN+yP0CNlfsbLIKSQyscUZ5oe20t/diowxfrxexds zfWoQuTt4OttU8EX5tyEIwBlepnSe4FwQ9sx4pBG5OGdc4A+hLUqxrtui17Y4HqP 4PfpZR/EJCoKW6Tp3DzT6uwWt+AtFFr4BQ+dOVS8Fa1LcOJMcv9PzugXgNWAJUEM 9Cxi356+t2igTaETHJT1YaIPDk2RuDXCHgDNHNLCtsHR+Umw+94h4mQnNMWMjDcP MrlJ/C8Iyom4gS/uPc4nSh9wtifIxPGuSdQlmSt8xJWG2YmfHz4ZLFmDm4b01rQ= =Oy3j -----END PGP SIGNATURE----- --uILnNkw8NvHgS4dNfRPKvEe7E5JAmRCGp--