Eduardo Habkost <ehabk...@redhat.com> writes: > On Fri, Jun 30, 2017 at 01:40:58PM +0200, Markus Armbruster wrote: > [...] >> >> I doubt the macros make the bug fixing materially easier, and I doubt >> they can reduce future bugs of this kind. What they can do is letting >> us get rid of error_propagate() boilerplate with relative ease. >> >> 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 think the first order of business is to figure out whether we want to >> pursue returning success/failure. > > About this, I'm unsure. Returning error information in two separate > locations (the return value and *errp) makes it easier to introduce bugs > that are hard to detect.
I sympathize with this argument. It's exactly what that made us avoid returning a success/failure indication. Except when we don't actually avoid it: * Functions returning a pointer typically return non-null on success, null on failure. Has inconsistency between return value and Error setting been a problem in practice? Nope. * Quite a few functions return 0 on success, -errno on failure, to let callers handle different errors differently. Has inconsistency been a problem in practice? Nope again. Aside: the original Error plan was to have callers check ErrorClass, but that didn't work out. * Functions with a side effect typically either do their side effect and succeed, or do nothing and fail. Inconsistency between side effect and claimed success is theoretically possible no matter how success is claimed: it's possible if the function returns success/failure, it's possible if it sets an Error on failure and doesn't on success, and it's possible if it does both. My point is: returning void instead of success/failure gets rid only of a part of a theoretical problem, which turns out not much of a problem in practice. > Especially when the tree is an inconsistent > state where we mix -1/0, -errno/0, FALSE/TRUE, NULL/non-NULL and void > functions. This is basically ret<0/ret>=0, !ret/ret, void. Getting rid of void would improve matters, wouldn't it?
