[sheepdog] [RFC v5 024/126] error: auto propagated local_err

[Markus Armbruster]

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

> 04.12.2019 17:59, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov at virtuozzo.com> writes:
>> 
>>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>>> functions with errp OUT parameter.
>>>
>>> It has three goals:
>>>
>>> 1. Fix issue with error_fatal & 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 & error_propagate: when we wrap
>>> error_abort by local_err+error_propagate, resulting coredump will
>>> refer to error_propagate and not to the place where error happened.
>> 
>> I get what you mean, but I have plenty of context.
>> 
>>> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>>> local_err+error_propagate pattern, which will definitely fix the issue)
>> 
>> The parenthesis is not part of the goal.
>> 
>>> [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.).
>> 
>> The paragraph talks about two cases: 1. and 2.+3. 
>
> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
> fix achieve 2 and 3 by one action.
>
>> Makes me think we
>> want two paragraphs, each illustrated with an example.
>> 
>> What about you provide the examples, and then I try to polish the prose?
>
> 1: error_fatal problem
>
> Assume the following code flow:
>
> int f1(errp) {
>     ...
>     ret = f2(errp);
>     if (ret < 0) {
>        error_append_hint(errp, "very useful hint");
>        return ret;
>     }
>     ...
> }
>
> Now, if we call f1 with &error_fatal argument and f2 fails, the program
> will exit immediately inside f2, when setting the errp. User will not
> see the hint.
>
> So, in this case we should use local_err.

How does this example look after the transformation?

> 2: error_abort problem
>
> Now, consider functions without return value. We normally use local_err
> variable to catch failures:
>
> void f1(errp) {
>     Error *local_err = NULL;
>     ...
>     f2(&local_err);
>     if (local_err) {
>         error_propagate(errp, local_err);
>         return;
>     }
>     ...
> }
>
> Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
> crash dump will point to error_propagate, not to the failure point in f2,
> which complicates debugging.
>
> So, we should never wrap error_abort by local_err.

Likewise.

>
> ===
>
> Our solution:
>
> - Fixes [1.], adding invocation of new macro into functions with error_appen_hint/error_prepend,
>    New macro will wrap error_fatal.
> - Fixes [2.], by switching from hand-written local_err to smart macro, which never
>    wraps error_abort.
> - Handles [3.], by switching to macro, which is less code
> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra propagations
>    (in fact, error_propagate is called, but returns immediately on first if (!local_err))