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

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 07.07.2020 19:50, 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
>> 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>
>
> Ok, I see you have mostly rewritten the big comment

Guilty as charged...  I was happy with the contents you provided (and
grateful for it), but our parallel work caused some redundancy.  I went
beyond a minimal merge to get a something that reads as one coherent
text.

> and not only in this patch, so, I go and read the whole comment on top of 
> these series.
>
> =================================
>
>    * Pass an existing error to the caller with the message modified:
>    *     error_propagate_prepend(errp, err,
>    *                             "Could not frobnicate '%s': ", name);
>    * This is more concise than
>    *     error_propagate(errp, err); // don't do this
>    *     error_prepend(errp, "Could not frobnicate '%s': ", name);
>    * and works even when @errp is &error_fatal.
>
> - the latter doesn't consider ERRP_AUTO_PROPAGATE: as we know, that 
> ERRP_AUTO_PROPAGATE should be used when we use error_prepend, the latter 
> should look like
>
>
> ERRP_AUTO_PROPAGATE();
> ...
> error_propagate(errp, err); // don't do this
> error_prepend(errp, "Could not frobnicate '%s': ", name);
>
> - and it works even when @errp is &error_fatal, so the 
> error_propagate_prepend now is just a shortcut, not the only correct way.

I can duplicate the advice from the paragraph preceding it, like this:

     * This is rarely needed.  When @err is a local variable, use of
     * ERRP_GUARD() commonly results in more readable code.
     * Where it is needed, it is more concise than
     *     error_propagate(errp, err); // don't do this
     *     error_prepend(errp, "Could not frobnicate '%s': ", name);
     * and works even when @errp is &error_fatal.

> Still, the text is formally correct as is, and may be improved later.
>
> =================================
>
>    * 2. Replace &err by errp, and err by *errp.  Delete local variable
>    *    @err.
>
> - hmm a bit not obvious,, It can be local_err.

Yes, but I trust the reader can make that mental jump.

> It can be (in some rare cases) still needed to handle the error locally, not 
> passing to the caller..

I didn't think of this.

What about

     * To convert a function to use ERRP_GUARD(), assuming the local
     * variable it propagates to @errp is called @err:
     [...]
     * 2. Replace &err by errp, and err by *errp.  Delete local variable
     *    @err if it s now unused.

Nope, still no good, if we replace like that, @err *will* be unused, and
the locally handled error will leak to the caller.

No time left for wordsmithing; let's improve on top.

> may be just something like "Assume local Error *err variable is used to get 
> errors from called functions and than propagated to caller's errp" before 
> paragraph [2.] will help.
>
>
>    *
>    * 3. Delete error_propagate(errp, *errp), replace
>    *    error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
>    *
>    * 4. Ensure @errp is valid at return: when you destroy *errp, set
>    *    errp = NULL.
>
> =================================
>
>
> May be good to add note about ERRP_AUTO_PROPAGATE() into comment above 
> error_append_hint (and error_(v)prepend)).

Good point.

> =================================
>
>   /*
>    * Make @errp parameter easier to use regardless of argument value
>
> may be s/argument/its/
>
>    *
>    * This macro is for use right at the beginning of a function that
>    * takes an Error **errp parameter to pass errors to its caller.  The
>    * parameter must be named @errp.
>    *
>    * It must be used when the function dereferences @errp or passes
>    * @errp to error_prepend(), error_vprepend(), or error_append_hint().
>    * It is safe to use even when it's not needed, but please avoid
>    * cluttering the source with useless code.
>    *
>    * If @errp is NULL or &error_fatal, rewrite it to point to a local
>    * Error variable, which will be automatically propagated to the
>    * original @errp on function exit.
>    *
>    * Note: &error_abort is not rewritten, because that would move the
>    * abort from the place where the error is created to the place where
>    * it's propagated.
>    */
>
> =================================
>
>
> All these are minor, the documentation is good as is, thank you!

Thanks for your review, and your patience!