Re: [Qemu-devel] [RFC 00/15] Error API: Flag errors in *errp even if errors are being ignored

[Eric Blake]

On 06/13/2017 11:52 AM, Eduardo Habkost wrote:

> The Proposal
> ------------
> 
> I'm proposing replacing NULL errp with a special macro:
> IGNORE_ERRORS.  The macro will trigger special behavior in the
> error API that will make it not save any error information in the
> error pointer, but still keep track of boolean error state in
> *errp.
> 
> This will allow us to simplify the documented method to propagate errors
> from:
> 
>     Error *err = NULL;
>     foo(arg, &err);
>     if (err) {
>         handle the error...
>         error_propagate(errp, err);
>     }
> 
> to:
> 
>     foo(arg, errp);
>     if (ERR_IS_SET(errp)) {
>         handle the error...
>     }

Seems kind of neat to me!


> 
> This way, we can make ERR_IS_SET work even if errp was
> IGNORE_ERRORS.  The ERR_IS_* macros are reimplemented as:
> 
>   #define ERR_IS_SET(e) (*(e) && *(e) != &ignored_error_unset)
>   #define ERR_IS_IGNORED(e) (!(e) || *(e) == &ignored_error_unset || *(e) == 
> &ignored_error_set)

Where the initial NULL checks go away if you get your way with a final
patch.

> 
> Ensuring errp is never NULL
> ---------------------------
> 
> The last patch on this series changes the (Error **errp)
> parameters in functions to (Error *errp[static 1]), just to help
> validate the existing code, as clang warns about NULL arguments
> on that case.  I don't think we should apply that patch, though,
> because the "[static 1]" syntax confuses Coccinelle.

Have you filed a bug report to the Coccinelle folks?  But yeah, it is
rather arcane C99 syntax that you don't see much of in common code.

> (Probably the easiest solution for that is to add assert(errp)
> lines to the ERR_IS_*() macros.)

Or even having the macros use a forced dereference through errp->xxx may
at least be enough for Coverity to catch things.

> Git branch
> ----------
> 
> This series depend on a few extra cleanups that I didn't submit
> to qemu-devel yet.  A git branch including this series is
> available at:
> 
>   git://github.com/ehabkost/qemu-hacks.git work/err-api-rework-ignore-ptr-v1
> 
> Eduardo Habkost (15):
>   tests: Test cases for error API
>   error: New IGNORE_ERRORS macro
>   Add qapi/error.h includes on files that will need it
>   [coccinelle] Use IGNORE_ERRORS instead of NULL as errp argument
>   qapi: Use IGNORE_ERRORS instead of NULL on generated code
>   test-qapi-util: Use IGNORE_ERRORS instead of NULL
>   Manual changes to use IGNORE_ERRORS instead of NULL
>   error: New ERR_IS_* macros for checking Error** values
>   [coccinelle] Use ERR_IS_* macros
>   test-qapi-util: Use ERR_IS_* macros
>   Manual changes to use ERR_IS_* macros
>   error: Make IGNORED_ERRORS not a NULL pointer
>   rdma: Simplify var declaration to avoid confusing Coccinelle
>   [coccinelle] Eliminate unnecessary local_err/error_propagate() usage
>   [test only] Use 'Error *err[static 1]' instead of 'Error **errp' to
>     catch NULL errp arguments

I have not reviewed the series yet, but the idea looks promising.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature