From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55806) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5F9M-000599-L6 for qemu-devel@nongnu.org; Wed, 17 Jun 2015 11:21:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5F9H-0002Xw-8W for qemu-devel@nongnu.org; Wed, 17 Jun 2015 11:21:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44886) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5F9H-0002Xs-26 for qemu-devel@nongnu.org; Wed, 17 Jun 2015 11:21:07 -0400 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 41679385A78 for ; Wed, 17 Jun 2015 15:21:06 +0000 (UTC) Message-ID: <5581905C.5060406@redhat.com> Date: Wed, 17 Jun 2015 09:21:00 -0600 From: Eric Blake MIME-Version: 1.0 References: <1434525861-21768-1-git-send-email-mst@redhat.com> <1434525861-21768-2-git-send-email-mst@redhat.com> In-Reply-To: <1434525861-21768-2-git-send-email-mst@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="GBJ7qMVofFeMD1L0eV7BiJIIkkRHJKRaP" Subject: Re: [Qemu-devel] [PATCH v2 1/3] error: don't rely on pointer comparisons List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, dgilbert@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --GBJ7qMVofFeMD1L0eV7BiJIIkkRHJKRaP Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/17/2015 01:24 AM, Michael S. Tsirkin wrote: > makes it possible to copy error_abort pointers, > not just pass them on directly. >=20 > This is needed because follow-up patches add support for > Error *local_err =3D ...; > as a way to set an abort-on-error pointer, which requires that we have > more than just a global error_abort abort-on-error pointer, but that an= y > number of pointers all resolve to something specific. >=20 > Add an assert statement when class is retrieved, to make sure we still > get a core-dump if we (somehow) attempt to output the abort errp by > mistake. >=20 > Signed-off-by: Michael S. Tsirkin > Reviewed-by: Eric Blake I think you made enough changes from v1 (functional change of an added assertion, but also better justification via improved commit message) that I would have dropped R-b if I were the one submitting it. Your argument of aiding gdb debugging of error objects by making the pointer point somewhere valid makes sense, when compared to my hack of an invalid pointer that would segfault even when trying to view it through gdb. The added assertion in this version definitely helps avoid code making the mistake of dereferencing the magic error pointer. > --- > util/error.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) >=20 > diff --git a/util/error.c b/util/error.c > index 14f4351..e10cb34 100644 > --- a/util/error.c > +++ b/util/error.c > @@ -20,7 +20,13 @@ struct Error > ErrorClass err_class; > }; > =20 > -Error *error_abort; > +static Error error_abort_st =3D { .err_class =3D ERROR_CLASS_MAX }; > +Error *error_abort =3D &error_abort_st; > + > +static bool error_is_abort(Error **errp) > +{ > + return errp && *errp =3D=3D error_abort; However, now I don't like the subject line. This is still a pointer comparison (just a different pointer than before). So while I'm now happy with the state of the code, I think a better commit message would b= e: error: don't rely on address of global variable The old implementation used the address of a global pointer variable (&error_abort, type Error**) as a sentinel. This patch changes to using the value of the global pointer variable itself (error_abort, type Error*), so that the sentinel value can be easily copied to other Error* pointers, regardless of their address. The new sentinel points to an actual object, in case it is inspected through a debugger, although working code should never dereference it. This is needed because...[snipped] > @@ -144,6 +150,7 @@ Error *error_copy(const Error *err) > =20 > ErrorClass error_get_class(const Error *err) > { > + assert(err->err_class < ERROR_CLASS_MAX); I also think you should add this assert in error_copy() a few lines above, as well as in error_get_pretty() and error_free() a few lines belo= w. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --GBJ7qMVofFeMD1L0eV7BiJIIkkRHJKRaP 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/ iQEcBAEBCAAGBQJVgZBdAAoJEKeha0olJ0NqB9wIAKZNhfcFrFVnZ2tgs9h7DHWs zH0D4qW/5ng/1Wor8UkM/d699T8hO4aHJ+U7c16e8UHVJbhuJoODdqBQdeZOWdV/ Hkjv7s7B4cj/EKITnqQT1soEx0IAA4PlK3rNGtPj2hC9uqKfdPZyYDRb6hD7k6IZ DR/R3J33quiNFbM0MTUd0AwoBxWL38zp/PohLboaJf6WfcOIVSJ2ctj71yWn+WHq VWvu34mozC6bv716PtmeMHylHIMhpFQifxeCA1TafCa1rKaFJ0rCtQKgYClIOiMe 3by6fNCVAmPvLsEF3hJTI6wjyIhGw00n0QzuWVmMU7++JoqcfP17msZmm7MMTqM= =KTwr -----END PGP SIGNATURE----- --GBJ7qMVofFeMD1L0eV7BiJIIkkRHJKRaP--