Re: [PATCH v6 02/11] error: auto propagated local_err

[Eric Blake <eblake@redhat.com>]

On 1/10/20 1:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.

s/with/with an/

> 
> It has three goals:
> 
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user

maybe s/&/and/ so it doesn't look like the C & operator.

> 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 & error_propagate: when we wrap

and again

> error_abort by local_err+error_propagate, resulting coredump will

s/,/, the/

> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all

s/allows/allows us/
s/all/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, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).

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

> - * Receive an error and pass it on to the caller:
> + * Receive an error and pass it on to the caller (DEPRECATED*):
>    *     Error *err = NULL;
>    *     foo(arg, &err);
>    *     if (err) {
> @@ -98,6 +98,50 @@
>    *     foo(arg, errp);
>    * for readability.
>    *
> + * DEPRECATED* This pattern is deprecated now, use ERRP_AUTO_PROPAGATE macro

s/use/use the/

> + * instead (defined below).
> + * It's deprecated because of two things:
> + *
> + * 1. Issue with error_abort & error_propagate: when we wrap error_abort by

s/&/and/

> + * local_err+error_propagate, resulting coredump will refer to error_propagate

s/,/, the/

> + * and not to the place where error happened.
> + *
> + * 2. A lot of extra code of the same pattern
> + *

> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function which use
> + * Error **errp parameter to report error. It's needed only in cases where we
> + * want to use error_prepend, error_append_hint or dereference *errp. It's
> + * still safe (but useless) in other cases.

It doesn't _have_ to be the first line to compile (we require C99+ 
compilers, which allow declarations after statements); but rather 
because it makes it easier for our Coccinelle conversion script to catch 
outliers.  But I think this text is okay, without calling out that extra 
information (maybe the commit message should mention it, though).

> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> + * local Error object, which will be automatically propagated to the original
> + * errp on function exit (see error_propagator_cleanup).
> + *
> + * After invocation of this macro it is always safe to dereference errp
> + * (as it's not NULL anymore) and to add information (by error_prepend or
> + * error_append_hint)
> + * (as, if it was error_fatal, we swapped it with a local_error to be
> + * propagated on cleanup).

double () () looks odd, as does the mid-sentence newline.

> + *
> + * Note: we don't wrap the error_abort case, as we want resulting coredump
> + * to point to the place where the error happened, not to error_propagate.
> + */
> +#define ERRP_AUTO_PROPAGATE()                                  \
> +    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
> +    errp = ((errp == NULL || *errp == error_fatal)             \
> +            ? &_auto_errp_prop.local_err : errp)
> +
>   /*
>    * Special error destination to abort on error.
>    * See error_setg() and error_propagate() for details.
> 

The macro itself is fine, my comments are solely on the commit message 
and comments.  Depending on how much cleanup Markus is willing to do 
rather than require a respin, you can add:

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

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