Re: [RFC] error: auto propagated local_err

[Daniel P . Berrangé]

On Thu, Sep 19, 2019 at 04:16:25PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 18:50, Daniel P. Berrangé wrote:
> > On Thu, Sep 19, 2019 at 10:24:20AM -0500, Eric Blake wrote:
> >> On 9/19/19 9:49 AM, Daniel P. Berrangé wrote:
> >>
> >>>> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
> >>>> **errp parameter is dirt-simple to explain.  It has no performance
> >>>> penalty if the user passed in a normal error or error_abort (the cost of
> >>>> an 'if' hidden in the macro is probably negligible compared to
> >>>> everything else we do), and has no semantic penalty if the user passed
> >>>> in NULL or error_fatal (we now get the behavior we want with less
> >>>> boilerplate).
> >>>>
> >>>> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
> >>>> can I omit it?' does not provide the same simplicity.
> >>>
> >>> The flipside is that MAKE_ERRP_SAFE hides a bunch of logic, so you don't
> >>> really know what its doing without looking at it, and this is QEMU
> >>> custom concept so one more thing to learn for new contributors.
> >>>
> >>> While I think it is a nice trick, personally I think we would be better
> >>> off if we simply used a code pattern which does not require de-referencing
> >>> 'errp' at all, aside from exceptional cases. IOW, no added macro in 95%
> >>> of all our methods using Error **errp.
> >>
> >> If 100% of our callsites use the macro, then new contributors will
> >> quickly learn by observation alone that the macro usage must be
> >> important on any new function taking Error **errp, whether or not they
> >> actually read the macro to see what it does.  If only 5% of our
> >> callsites use the macro, it's harder to argue that a new user will pick
> >> up on the nuances by observation alone (presumably, our docs would also
> >> spell it out, but we know that not everyone reads those...).
> > 
> > To get some slightly less made-up stats, I did some searching:
> > 
> >    - 4408  methods with an 'errp' parameter declared
> > 
> >       git grep 'Error \*\*errp'|  wc -l
> > 
> >    - 696 methods with an 'Error *local' declared (what other names
> >      do we use for this?)
> > 
> >       git grep 'Error \*local' | wc -l
> > 
> >    - 1243 methods with an 'errp' parameter which have void
> >      return value (fuzzy number - my matching is quite crude)
> > 
> >       git grep 'Error \*\*errp'|  grep -E '(^| )void' | wc -l
> > 
> >    - 11 methods using error_append_hint with a local_err
> > 
> >       git grep append_hint | grep local | wc -l
> 
> why do you count only with local? Greg's series is here to bring local to all
> functions with append_hint:

I hadn't noticed the scope of Greg's series :-)

> 
> # git grep append_hint | wc -l
> 85



> Also, conversion to use macro everywhere may be done (seems so) by coccinelle 
> script.
> But conversion which you mean, only by hand I think. Converting 1243 methods 
> by hand
> is a huge task..

Yeah, it would be a non-negligible amount of work.

> I think there are three consistent ways:
> 
> 1. Use macro everywhere
> 2. Drop error_append_hint
> 3. Drop error_fatal

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|