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