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

[Markus Armbruster <armbru@redhat.com>]

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

> 05.12.2019 17:58, Vladimir Sementsov-Ogievskiy wrote:
>> 05.12.2019 15:36, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> 04.12.2019 17:59, Markus Armbruster wrote:
>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@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?
>> 
>> 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.
>
> and if it was error_abort, it is not wrapped, so will crash where failed.

We still need to avoid passing on unwrapped @errp when we want to ignore
some errors, as in fit_load_fdt(), but that should be quite rare.
Doesn't invalidate your approach.

[...]