From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:59040)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1Z5brL-0001IJ-Pe
	for qemu-devel@nongnu.org; Thu, 18 Jun 2015 11:36:08 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1Z5brI-0008Oh-7N
	for qemu-devel@nongnu.org; Thu, 18 Jun 2015 11:36:07 -0400
Received: from mx1.redhat.com ([209.132.183.28]:41994)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1Z5brI-0008OW-1q
	for qemu-devel@nongnu.org; Thu, 18 Jun 2015 11:36:04 -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 8DD6F38887D
	for <qemu-devel@nongnu.org>; Thu, 18 Jun 2015 15:36:03 +0000 (UTC)
From: Markus Armbruster <armbru@redhat.com>
References: <1434525861-21768-1-git-send-email-mst@redhat.com>
	<1434525861-21768-2-git-send-email-mst@redhat.com>
	<5581951F.9050802@redhat.com>
Date: Thu, 18 Jun 2015 17:36:00 +0200
In-Reply-To: <5581951F.9050802@redhat.com> (Eric Blake's message of "Wed, 17
	Jun 2015 09:41:19 -0600")
Message-ID: <87twu5rpun.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: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Eric Blake <eblake@redhat.com>
Cc: kwolf@redhat.com, qemu-devel@nongnu.org, dgilbert@redhat.com, "Michael S. Tsirkin" <mst@redhat.com>

Eric Blake <eblake@redhat.com> writes:

> 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.
>> 
>
>> @@ -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 I pointed out on 3/3, this breaks code that does:
>
> if (local_err) {
>     error_propagate(errp, local_err);
>     ...
> }
>
> now that local_err is non-NULL when errp is error_abort.  But what if
> you alter the semantics, and have error_propagate return a bool (true if
> an error was propagated, false if no error or caller didn't care):
>
> bool error_propagate(Error **dst_errp, Error *local_err)
> {
>     if (error_is_abort(&local_err)) {
>         assert(error_is_abort(dst_errp);
>         return false;
>     }
>     if (local_err && error_is_abort(dst_errp)) {
>         error_report_err(local_err);
>         abort();
>     }
>     if (dst_errp && !*dst_errp) {
>         *dst_errp = local_err;
>         return true;
>     }
>     if (local_err) {
>         error_free(local_err);
>     }
>     return false;
> }
>
> then callers can be modified to this idiom (also has the benefit of
> being one line shorter):
>
> if (error_propagate(errp, local_err)) {
>     ...
> }

Caution!  The condition you need to test here is "an error has been
stored into local_err", *not* "an error was propagated".  Different when
errp is NULL and local_err has an error.