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