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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]

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)) {
    ...
}

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]