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

[Max Reitz <mreitz@redhat.com>]

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.)

> 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