On 19.09.19 11:14, Vladimir Sementsov-Ogievskiy wrote: > 19.09.2019 11:59, Max Reitz wrote: >> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote: >>> Hi all! >>> >>> Here is a proposal (three of them, actually) of auto propagation for >>> local_err, to not call error_propagate on every exit point, when we >>> deal with local_err. >>> >>> It also may help make Greg's series[1] about error_append_hint smaller. >>> >>> See definitions and examples below. >>> >>> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like >>> it, the idea will touch same code (and may be more). >>> >>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>> --- >>> include/qapi/error.h | 102 +++++++++++++++++++++++++++++++++++++++++++ >>> block.c | 63 ++++++++++++-------------- >>> block/backup.c | 8 +++- >>> block/gluster.c | 7 +++ >>> 4 files changed, 144 insertions(+), 36 deletions(-) >> >> If the combination of “if (local_err) { error_propagate(...); ... }” is >> what’s cumbersome, can’t this be done simpler by adding an >> error_propagate() variant with a return value? >> >> i.e. >> >> bool has_error_then_propagate(Error **errp, Error *err) >> { >> if (!err) { >> return false; >> } >> error_propagate(errp, err); >> return true; >> } >> >> And then turn all instances of >> >> if (local_err) { >> error_propagate(errp, local_err); >> ... >> } >> >> into >> >> if (has_error_then_propagate(errp, local_err)) { >> ... >> } >> >> ? >> >> Max >> > > No, originally cumbersome is introducing local_err in a lot of new places by > Greg's series. MAKE_ERRP_SAFE macro makes it as simple as one macro call > instead (in each function where we need local_err).
Does it need more than one local_err per function? Because if it didn’t, I’d find one “Error *local_err;” simpler than one macro incantation. (It has the same LoC, and it makes code readers ask the same question: “Why do we need it?” Which has the same answer for both; but one is immediately readable code, whereas the other is a macro.) > Also, auto-propagation seems correct thing to do, which fits good into > g_auto concept, so even without any macro it just allows to drop several > error_propagate > calls (or may be several goto statements to do one error_propagate call) into > one definitions. It's the same story like with g_autofree vs g_free. I don’t see the advantage here. You have to do the if () statement anyway, so it isn’t like you’re saving any LoC. In addition, I personally don’t find code hidden through __attribute__((cleanup)) easier to understand than explicitly written code. It isn’t like I don’t like __attribute__((cleanup)). But it does count under “magic” in my book, and thus I’d avoid it if it doesn’t bring actual advantages. (It does bring actual advantages for things like auto-freeing memory, auto-closing FDs, or zeroing secret buffers before freeing them. In all those cases, you save LoC, and you prevent accidental leakage. I don’t quite see such advantages here, but I may well be mistaken.) Now Kevin has given an actual advantage, which is that local_err complicates debugging. I’ve never had that problem myself, but that would indeed be an advantage that may warrant some magic. Max