Re: [Qemu-ppc] [RFC] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>>>> ---
>>>>    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