On Thu, Sep 19, 2019 at 10:21:44AM +0000, Vladimir Sementsov-Ogievskiy wrote: > 19.09.2019 13:09, Daniel P. Berrangé wrote: > > On Thu, Sep 19, 2019 at 11:17:20AM +0200, Kevin Wolf wrote: > >> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben: > >>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>> + */ > >>>> +#define MAKE_ERRP_SAFE(errp) \ > >>>> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \ > >>>> +if ((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) > >>>> { \ > >>>> + (errp) = &__auto_errp_prop.local_err; \ > >>>> +} > >>> > >>> Not written to take a trailing semicolon in the caller. > >>> > >>> You could even set __auto_errp_prop unconditionally rather than trying > >>> to reuse incoming errp (the difference being that error_propagate() gets > >>> called more frequently). > >> > >> I think this difference is actually a problem. > >> > >> When debugging things, I hate error_propagate(). It means that the Error > >> (specifically its fields src/func/line) points to the outermost > >> error_propagate() rather than the place where the error really happened. > >> It also makes error_abort completely useless because at the point where > >> the process gets aborted, the interesting information is already lost. > > > > Yeah, that is very frustrating. For personal development you can eventually > > figure it out, but with customer support requests, all you get is the > > stack trace and you've no core file to investigate, so you're stuck. > > IOW, as a general principle, we should strive to minimize the usage > > of error propagate. > > > > The key reason why we typically need the propagate calls, is because > > we allow the passed in Error **errp parameter to be NULL, while at the > > same time we want to inspect it to see if its contents are non-NULL > > to detect failure. I venture to suggest that this is flawed API > > design on our part. We should not need to inspect the error object > > to see if a method call fails - the return value can be used for > > this purpose. > > > > IOW, instead of this pattern with typically 'void' methods > > > > extern void somemethod(Error **errp); > > > > void thismethod(Error **errp) { > > Error *local_err = NULL; > > ... > > somemethod(&local_err); > > if (local_err) { > > error_propagate(errp, local_err); > > return; > > } > > ... > > } > > > > We should be preferring > > > > extern int somemethod(Error **errp); > > > > int thismethod(Error **errp) { > > ... > > if (somemethod(errp) < 0) { > > return -1; > > } > > ... > > } > > > > ie only using the Error object to bubble up the *details* of > > the error, not as the mechanism for finding if an error occurred. > > > > There is of course a downside with this approach, in that a > > method might set 'errp' but return 0, or not set 'errp' but > > return -1. I think this is outweighed by the simpler code > > pattern overall though. > > > > > > Agree. But seems that such change may be only done by hand.. Hmm, on the > other hand, > why not. May be I'll try do it for some parts of block layer. > > Still, error_append_hint needs local_err in case of error_fatal.
Yep, fortunately that usage is comparatively rare vs use of error_propagate in general. To be clear I don't have any objections to your overall idea of using attribute cleanup to simplify error propagation. As you say, it can still be useful for the error_append_hint, even if we use the code pattern I suggest more widely to eliminate propagation where possible. 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 :|