Re: [qemu-s390x] [RFC] error: auto propagated local_err

[Daniel P . Berrangé]

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


This suggests to me, that if we used the 'return 0 / return -1' convention
to indicate failure for the methods which currently return void, we could
potentially only have 11 cases where a local error object is genuinely
needed. 

If my numbers are at all accurate, I still believe we're better off
changing the return type and eliminating essentially all use of local
error variables, as void funcs/local error objects appear to be the
minority coding pattern.


> > There are lots of neat things we could do with auto-cleanup functions we
> > I think we need to be wary of hiding too much cleverness behind macros
> > when doing so overall.
> 
> The benefit of getting rid of the 'Error *local_err = NULL; ...
> error_propagate()' boilerplate is worth the cleverness, in my opinion,
> but especially if also accompanied by CI coverage that we abide by our
> new rules.

At least we're both wanting to eliminate the local error + propagation.
The difference is whether we're genuinely eliminating it, or just hiding
it from view via auto-cleanup & macro magic

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 :|