On 19.09.19 12:03, Vladimir Sementsov-Ogievskiy wrote: > 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
I did, because it would part of the if () statement in my proposal. > And your example don't match Greg's case, there no "if (local_err)" in it, Ah, right, I see. OK then. > 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) True. [...] >> 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. Although after some more consideration I realized this probably cannot be achieved with this series, actually. Max
