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
