19.09.2019 12:33, Max Reitz wrote: > 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.)
Not the same, you didn't count error_propagate And your example don't match Greg's case, there no "if (local_err)" in it, just "error_append_hint(errp)", which don't work for error_fatal and error_abort (Yes, I agree with Kevin, that it should work only for error_fatal) > >> 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 > -- Best regards, Vladimir
