05.12.2019 17:58, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2019 15:36, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
>>
>>> 04.12.2019 17:59, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsement...@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.
>
>>
>>>
>>> ===
>>>
>>> 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
