Re: [Qemu-devel] [PATCH RFC 2/3] error: allow local errors to trigger abort

[Eric Blake <eblake@redhat.com>]

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

On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> It's a common idiom:
> 
>     Error *local_err = NULL;
>     ....
>     foo(&local_err);
>     ...
>     if (local_err) {
>         error_propagate(errp, local_err);
>         return;
>     }
> 
> Unfortunately it means that call to foo(&local_err) will
> not abort even if errp is set to error_abort.
> 
> Instead, we get an abort at error_propagate which is too late.

That is, the quality of the stack trace is degraded in that it no longer
pinpoints the actual cause of failure.

> 
> To fix, add an API to check errp and set local_err to error_abort
> if errp is error_abort.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qapi/error.h | 5 +++++
>  util/error.c         | 5 +++++
>  2 files changed, 10 insertions(+)

I like the idea.

> +++ b/util/error.c
> @@ -28,6 +28,11 @@ static bool error_is_abort(Error **errp)
>      return errp && *errp && (*errp)->err_class == ERROR_CLASS_MAX;
>  }
>  
> +Error *error_init_local(Error **errp)
> +{
> +    return error_is_abort(errp) ? *errp : NULL;

What you have works, but see also my ideas in my (latest) reply to 1/3
where you could use pointer equality on error_abort (rather than on
&error_abort) as the key factor.  After all, the way you have
implemented things so far, when taking the *errp branch, you are still
effectively returning the error_abort global pointer (you haven't yet
used anything that required struct copying of the contents of error_abort).

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]