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

[Max Reitz <mreitz@redhat.com>]

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 <vsementsov@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