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

[Eric Blake <eblake@redhat.com>]

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

On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> makes it possible to copy error_abort pointers,
> not just pass them on directly.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  util/error.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/util/error.c b/util/error.c
> index 14f4351..ccf29ea 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;

Looking at this a bit further, I still wonder if we can do a slightly
better job of coming up with something that will SIGSEGV (or SIGBUS) if
we (accidentally) try to dereference the pointer (similar to how SIG_IGN
is (sighandler_t)1) - because we know that the abort object should never
be dereferenced.  Something like:

    Error *error_abort = (Error *)1;

with no need for error_abort_st.  (Might have to spell it as Error
    *error_abort = (void*)(intptr_t)1;
to shut up compiler warnings)

> +
> +static bool error_is_abort(Error **errp)
> +{
> +    return errp && *errp && (*errp)->err_class == ERROR_CLASS_MAX;
> +}

and this would be:

    return errp && *errp == error_abort;

The rest of this patch is still good.  Then in patch 2, you'd have:

Error *error_init_local(Error **errp)
{
    return error_is_abort(errp) ? error_abort : NULL;
}

That is, you still use pointer equality, but at one less level of
indirection (equality at the Error* level, not the Error** level).

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