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