From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44379) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5cOh-0007ep-AE for qemu-devel@nongnu.org; Thu, 18 Jun 2015 12:10:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5cOc-0000i0-8z for qemu-devel@nongnu.org; Thu, 18 Jun 2015 12:10:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53099) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5cOc-0000hm-0L for qemu-devel@nongnu.org; Thu, 18 Jun 2015 12:10:30 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id BB2F0372500 for ; Thu, 18 Jun 2015 16:10:29 +0000 (UTC) From: Markus Armbruster References: <1434525861-21768-1-git-send-email-mst@redhat.com> <1434525861-21768-2-git-send-email-mst@redhat.com> Date: Thu, 18 Jun 2015 18:10:27 +0200 In-Reply-To: <1434525861-21768-2-git-send-email-mst@redhat.com> (Michael S. Tsirkin's message of "Wed, 17 Jun 2015 09:24:43 +0200") Message-ID: <87fv5pro98.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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" Cc: kwolf@redhat.com, qemu-devel@nongnu.org, dgilbert@redhat.com "Michael S. Tsirkin" writes: > makes it possible to copy error_abort pointers, > not just pass them on directly. Humor me, and start your sentences with a capital letter :) > This is needed because follow-up patches add support for > Error *local_err = ...; > 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 any > number of pointers all resolve to something specific. > > 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. Description could be clearer, but let's discuss the actual patches first. > > Signed-off-by: Michael S. Tsirkin > Reviewed-by: Eric Blake > --- > util/error.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > 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; > }; > > -Error *error_abort; > +static Error error_abort_st = { .err_class = ERROR_CLASS_MAX }; > +Error *error_abort = &error_abort_st; > + > +static bool error_is_abort(Error **errp) > +{ > + return errp && *errp == error_abort; > +} If anything changes the value of error_abort, we're now screwed. > > void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...) > { > @@ -40,7 +46,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...) > va_end(ap); > err->err_class = err_class; > > - if (errp == &error_abort) { > + if (error_is_abort(errp)) { > error_report_err(err); > abort(); > } > @@ -76,7 +82,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class, > va_end(ap); > err->err_class = err_class; > > - if (errp == &error_abort) { > + if (error_is_abort(errp)) { > error_report_err(err); > abort(); > } > @@ -121,7 +127,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class, > va_end(ap); > err->err_class = err_class; > > - if (errp == &error_abort) { > + if (error_is_abort(errp)) { > error_report_err(err); > abort(); > } > @@ -144,6 +150,7 @@ Error *error_copy(const Error *err) > > ErrorClass error_get_class(const Error *err) > { > + assert(err->err_class < ERROR_CLASS_MAX); The assertion makes some sense independent of the rest of this series. It's not as tight as it could be when the compiler makes ErrorClass signed. > return err->err_class; > } > > @@ -168,7 +175,7 @@ void error_free(Error *err) > > void error_propagate(Error **dst_errp, Error *local_err) > { > - if (local_err && dst_errp == &error_abort) { > + if (local_err && error_is_abort(dst_errp)) { > error_report_err(local_err); > abort(); > } else if (dst_errp && !*dst_errp) { As Eric pointed out, this isn't quite right. Your use of ERROR_CLASS_MAX is unobvious, and needs an explanatory comment somewhere. I'd put it right next to its definition if it wasn't defined implicitly.