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