Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()

[Eric Blake <eblake@redhat.com>]

On 7/7/20 11:50 AM, Markus Armbruster wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with an errp OUT parameter.
> 
> It has three goals:
> 
> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user

the user

> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
> 
> 2. Fix issue with error_abort and error_propagate: when we wrap
> error_abort by local_err+error_propagate, the resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
> the local_err+error_propagate pattern, which will definitely fix the
> issue) [Reported by Kevin Wolf]
> 
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
> 
> To achieve these goals, later patches will add invocations
> of this macro at the start of functions with either use
> error_prepend/error_append_hint (solving 1) or which use
> local_err+error_propagate to check errors, switching those
> functions to use *errp instead (solving 2 and 3).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> [Comments merged properly with recent commit "error: Document Error
> API usage rules", and edited for clarity.  Put ERRP_AUTO_PROPAGATE()
> before its helpers, and touch up style.  Commit message tweaked.]
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/qapi/error.h | 160 ++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 141 insertions(+), 19 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3fed49747d..c865a7d2f1 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h

> @@ -128,18 +122,26 @@
>    *         handle the error...
>    *     }
>    * when it doesn't, say a void function:
> + *     ERRP_AUTO_PROPAGATE();
> + *     foo(arg, errp);
> + *     if (*errp) {
> + *         handle the error...
> + *     }
> + * More on ERRP_AUTO_PROPAGATE() below.
> + *
> + * Code predating ERRP_AUTO_PROPAGATE() still exits, and looks like this:

exists

>    *     Error *err = NULL;
>    *     foo(arg, &err);
>    *     if (err) {
>    *         handle the error...
> - *         error_propagate(errp, err);
> + *         error_propagate(errp, err); // deprecated
>    *     }
> - * Do *not* "optimize" this to
> + * Avoid in new code.  Do *not* "optimize" it to
>    *     foo(arg, errp);
>    *     if (*errp) { // WRONG!
>    *         handle the error...
>    *     }
> - * because errp may be NULL!
> + * because errp may be NULL!  Guard with ERRP_AUTO_PROPAGATE().

maybe:

because errp may be NULL without the ERRP_AUTO_PROPAGATE() guard.

>    *
>    * But when all you do with the error is pass it on, please use
>    *     foo(arg, errp);
> @@ -158,6 +160,19 @@
>    *         handle the error...
>    *     }
>    *
> + * Pass an existing error to the caller:

> + * = Converting to ERRP_AUTO_PROPAGATE() =
> + *
> + * To convert a function to use ERRP_AUTO_PROPAGATE():
> + *
> + * 0. If the Error ** parameter is not named @errp, rename it to
> + *    @errp.
> + *
> + * 1. Add an ERRP_AUTO_PROPAGATE() invocation, by convention right at
> + *    the beginning of the function.  This makes @errp safe to use.
> + *
> + * 2. Replace &err by errp, and err by *errp.  Delete local variable
> + *    @err.
> + *
> + * 3. Delete error_propagate(errp, *errp), replace
> + *    error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
> + *

Why a comma here?

> + * 4. Ensure @errp is valid at return: when you destroy *errp, set
> + *    errp = NULL.
> + *
> + * Example:
> + *
> + *     bool fn(..., Error **errp)
> + *     {
> + *         Error *err = NULL;
> + *
> + *         foo(arg, &err);
> + *         if (err) {
> + *             handle the error...
> + *             error_propagate(errp, err);
> + *             return false;
> + *         }
> + *         ...
> + *     }
> + *
> + * becomes
> + *
> + *     bool fn(..., Error **errp)
> + *     {
> + *         ERRP_AUTO_PROPAGATE();
> + *
> + *         foo(arg, errp);
> + *         if (*errp) {
> + *             handle the error...
> + *             return false;
> + *         }
> + *         ...
> + *     }

Do we want the example to show the use of error_free and *errp = NULL?

Otherwise, this is looking good to me.  It will need a tweak if we go 
with the shorter name ERRP_GUARD, but I like that idea.

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