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

[Markus Armbruster <armbru@xxxxxxxxxx>]

Eric Blake <eblake@xxxxxxxxxx> writes:

> On 7/7/20 11:50 AM, Markus Armbruster wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
>>
>> 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 <vsementsov@xxxxxxxxxxxxx>
>> Reviewed-by: Paul Durrant <paul@xxxxxxx>
>> Reviewed-by: Greg Kurz <groug@xxxxxxxx>
>> Reviewed-by: Eric Blake <eblake@xxxxxxxxxx>
>> [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@xxxxxxxxxx>
>> ---
>>   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!