Re: [Qemu-devel] [PATCH v2 1/3] error: don't rely on pointer comparisons

[Markus Armbruster]

Eric Blake <address@hidden> 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.