Eric Blake <ebl...@redhat.com> writes:
> On 7/7/20 11:50 AM, Markus Armbruster wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsement...@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
Yes.
>> 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 <vsement...@virtuozzo.com>
>> Reviewed-by: Paul Durrant <p...@xen.org>
>> Reviewed-by: Greg Kurz <gr...@kaod.org>
>> Reviewed-by: Eric Blake <ebl...@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 <arm...@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
Fixing...
>> * 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.
Sold.
>> *
>> * 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?
Editing accident.
>> + * 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?
Yes, but we're running out of time, so let's do it in the series that
introduces the usage to the code.
> 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.
Tweaking away...
Thanks!
