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

[Vladimir Sementsov-Ogievskiy]

05.12.2019 15:36, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 
>> 04.12.2019 17:59, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <address@hidden> 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?

Good point.

int f1(errp) {
    ERRP_AUTO_PROPAGATE();
    ...
    ret = f2(errp);
    if (ret < 0) {
       error_append_hint(errp, "very useful hint");
       return ret;
    }
    ...
}

- nothing changed, only add macro at start. But now errp is safe, if it was
error_fatal it is wrapped by local error, and will only call exit on automatic
propagation on f1 finish.

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

And here:

void f1(errp) {
     ERRP_AUTO_PROPAGATE();
     ...
     f2(errp);
     if (*errp) {
         return;
     }
     ...

- if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
   local error is automatically propagated to original one.

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


-- 
Best regards,
Vladimir