Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, Jan 18, 2018 at 02:34:39PM -0600, Eric Blake wrote:
> On 01/18/2018 09:55 AM, Philippe Mathieu-Daudé wrote:
> > On Thu, Jan 18, 2018 at 9:03 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> On 18/01/2018 12:21, Philippe Mathieu-Daudé wrote:
> >>>> I'm not a fan of bool return types, in general (because "!" is often
> >>>> success while "< 0" is failure) and especially when there is an Error**;
> >>>> I disagree with commit 9d3b155186.  But the function is not in an area I
> >>>> maintain so I'm queuing this, thanks.
> >>> Do you prefer "if (local_err)" and "if (errp && *errp)" ?
> >>
> >> The latter is wrong.  I do prefer
> > 
> > Ok so my 253674981e24 missed that train too.

Oops.  I thought we had Coccinelle scripts to detect that
pattern.

> 
> Markus has expressed as desire, as the error maintainer, to make errp
> functions return a useful value for less boilerplate, and at one point
> was even debating about Coccinelle scripts to make the conversion
> easier.  Perhaps int with -1 is more reliable than bool for that useful
> value, but this is definitely a topic of past discussion.
> 
> By the way, if (local_err) is definitely preferable; 'if (errp &&
> *errp)' means that your behavior is different depending on whether the
> caller wanted to ignore the error, and not whether you wanted to handle
> the error.
> 
> > 
> >>
> >>     if (local_err) {
> >>         error_propagate(errp, local_err);
> >>         return;
> >>     }
> 
> Yes, that's the right boilerplate if you don't have a return value witness.
> 
> >>
> >> or maybe (but only if there is a meaning to a zero vs. positive return
> >> value, or if errno is an important part of the returned Error *)
> >>
> >>     ret = f(..., errp);
> >>     if (ret < 0) {
> >>         return;
> >>     }
> >>
> >>> I wondered once if a macro might improve this pattern but thought the
> >>> code would get more obscure.
> >>
> >> Eduardo had a series to avoid error_propagate, where NULL was replaced
> >> by a (non-NULL) IGNORED_ERRORS macro.  Then you could do:
> >>
> >>     f(..., errp);
> >>     if (error_is_set(errp)) {
> >>         return;
> >>     }
> >>
> >> See here:
> >> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03139.html
> > 
> > This series never hit /master!
> > 
> > Reading the thread I'm not sure what was the expected outcome.
> 
> And since Markus may not answer this thread for a while, I'm still not
> sure if there is any expected outcome.

Quoting Markus on that discussion:

"If we switch to returning success/failure (which also gets rid of the
boilerplate), then the macros may still let us get rid of boilerplate
more quickly, for some additional churn.  Worthwhile?  Depends on how
long the return value change takes us."

I guess the lack of activity switching the code to returning
success/failure is enough evidence that the switch will take us
forever?

We can do some effort to document the preferred convention to
return success/failure, but I don't think we will be able to
convert the existing void/ret/bool functions to a single style
(whatever it is) in a reasonable time.

That said, IMO returning 0/-1 or true/false is always preferred
to returning void, so there's no need to add more local_err
boilerplate code.

-- 
Eduardo