[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC] error: auto propagated local_err
From: |
Daniel P . Berrangé |
Subject: |
Re: [RFC] error: auto propagated local_err |
Date: |
Thu, 19 Sep 2019 17:51:00 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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 :|
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, (continued)
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Vladimir Sementsov-Ogievskiy, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Eric Blake, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Kevin Wolf, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Vladimir Sementsov-Ogievskiy, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Eric Blake, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Daniel P . Berrangé, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Eric Blake, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Vladimir Sementsov-Ogievskiy, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Daniel P . Berrangé, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Vladimir Sementsov-Ogievskiy, 2019/09/19
- Re: [RFC] error: auto propagated local_err,
Daniel P . Berrangé <=
- Re: [RFC] error: auto propagated local_err, Eric Blake, 2019/09/20
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Vladimir Sementsov-Ogievskiy, 2019/09/19
- Re: [qemu-s390x] [Qemu-devel] [RFC] error: auto propagated local_err, no-reply, 2019/09/18
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, David Hildenbrand, 2019/09/19
- Re: [qemu-s390x] [RFC] error: auto propagated local_err, Max Reitz, 2019/09/19