Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 9/19/19 8:03 AM, Kevin Wolf wrote: >> >> Interesting, that to handle error_append_hint problem, we don't need to >> create local_err in case of errp==NULL either.. >> >> So, possibly, we need the following steps: >> >> 1. implement MAKE_ERRP_SAFE_FOR_HINT (which only leave "*(errp) == >> error_fatal" in the if condition >> 2. rebase Greg's series on it, to fix hints for fatal errors >> 3. implement MAKE_ERRP_SAFE_FOR_DEREFERENCE (which only leave "(errp) == >> NULL" in the if condition >> 4. convert all (almost all) local_err usage to use >> MAKE_ERRP_SAFE_FOR_DEREFERENCE, which will >> fix problem with error_abort (and also drop a lot of calls of >> error_propagate) Why do we need two macros? A single MAKE_ERRP_SAFE that covers both NULL and _fatal at the same time is sufficient. You can then always use append_hint and/or *errp as you wish, and the error_propagate() call is only used in two situations: caller passed NULL (so we free the error as ignored), caller passed _fatal (so the hint got appended before we propagate it to flag a more useful message). >> 5. merely convert "void func(.., errp)" to "int func(.., errp)" and drop >> MAKE_ERRP_SAFE_FOR_DEREFERENCE() >> magic, together with dereferencing. That's orthogonal, but may also be useful cleanups. I don't think we need to drop the macro, though. > > Long macro names, but as the parameter will always only be "errp", it > fits easily on a line, so this is fine. > > I think I like this plan. > > Kevin > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC] error: auto propagated local_err
19.09.2019 16:40, Eric Blake wrote:
> On 9/19/19 4:17 AM, 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.
>
> Okay, based on that, I see the following desirable semantics:
>
> Caller: one of 4 calling paradigms:
>
> pass errp=NULL (we don't care about failures)
> pass errp=_abort (we want to abort() as soon as possible as close
> to the real problem as possible)
> pass errp=_fatal (we want to exit(), but only after collecting as
> much information as possible)
> pass errp = anything else (we are collecting an error for other reasons,
> we may report it or let the caller decide or ...)
>
> Callee: we want a SINGLE paradigm:
>
> func (Error **errp)
> {
> MAKE_ERRP_SAFE();
>
> now we can pass errp to any child function, test '*errp', or do
> anything else, and we DON'T have to call error_propagate.
>
> I think that means we need:
>
> #define MAKE_ERRP_SAFE() \
>g_auto(...) __auto_errp = { .errp = errp }; \
>do { \
> if (!errp || errp == _fatal) { errp = &__auto_errp.local; } \
>} while (0)
>
> So back to the caller semantics:
>
> if the caller passed NULL, we've redirected errp locally so that we can
> use *errp at will; the auto-cleanup will free our local error.
>
> if the caller passed _abort, we keep errp unchanged. *errp tests
> will never trigger, because we'll have already aborted in the child on
> the original errp, giving developers the best stack trace.
>
> if the caller passed _fatal, we redirect errp. auto-cleanup will
> then error_propagate that back to the caller, producing as much nice
> information as possible.
>
> if the caller passed anything else, we keep errp unchanged, so no extra
> error_propagate in the mix.
>
>>
>> So I'd really like to restrict the use of error_propagate() to places
>> where it's absolutely necessary. Unless, of course, you can fix these
>> practical problems that error_propagate() causes for debugging.
>>
>> In fact, in the context of Greg's series, I think we really only need to
>> support hints for error_fatal, which are cases that users are supposed
>> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
>> are things that are never supposed to happen. A good stack trace is more
>> important there than adding a hint to the message.
>
> We also want to handle the caller passing NULL, so that we no longer
> have to introduce 'Error *local_error = NULL' everywhere.
>
With my plan of two different macro, I at least messed the case when we need
both dereferencing and hints, which means third macro, or one macro with
parameters,
saying what to wrap.
And my aim was to follow the idea of "do propagation only if it really
necessary in this case".
But may be you are right, and we shouldn't care so much.
1. What is bad, if we wrap NULL, when we only want to use hints?
Seems nothing. Some extra actions on error path, but who cares about it?
2. What is bad, if we wrap error_fatal, when we only want to dereference, and
don't use hints?
Seems nothing again, on error path we will return from higher level, and a bit
of extra work, but nothing worse..
So I tend to agree. But honestly, I didn't understand first part of Kevin's
paragraph against propagation,
so, may be he have more reasons to minimize number of cases when we propagate.
To the same topic, of minimization: should we always call MAKE_ERRP_SAFE at
function top, or only
in block, where it is needed (assume, we dereference it only inside some "if"
or "while"?
Kevin, is something bad in propagation, when it not related to error_abort?
--
Best regards,
Vladimir
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 9/19/19 4:17 AM, 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.
Okay, based on that, I see the following desirable semantics:
Caller: one of 4 calling paradigms:
pass errp=NULL (we don't care about failures)
pass errp=_abort (we want to abort() as soon as possible as close
to the real problem as possible)
pass errp=_fatal (we want to exit(), but only after collecting as
much information as possible)
pass errp = anything else (we are collecting an error for other reasons,
we may report it or let the caller decide or ...)
Callee: we want a SINGLE paradigm:
func (Error **errp)
{
MAKE_ERRP_SAFE();
now we can pass errp to any child function, test '*errp', or do
anything else, and we DON'T have to call error_propagate.
I think that means we need:
#define MAKE_ERRP_SAFE() \
g_auto(...) __auto_errp = { .errp = errp }; \
do { \
if (!errp || errp == _fatal) { errp = &__auto_errp.local; } \
} while (0)
So back to the caller semantics:
if the caller passed NULL, we've redirected errp locally so that we can
use *errp at will; the auto-cleanup will free our local error.
if the caller passed _abort, we keep errp unchanged. *errp tests
will never trigger, because we'll have already aborted in the child on
the original errp, giving developers the best stack trace.
if the caller passed _fatal, we redirect errp. auto-cleanup will
then error_propagate that back to the caller, producing as much nice
information as possible.
if the caller passed anything else, we keep errp unchanged, so no extra
error_propagate in the mix.
>
> So I'd really like to restrict the use of error_propagate() to places
> where it's absolutely necessary. Unless, of course, you can fix these
> practical problems that error_propagate() causes for debugging.
>
> In fact, in the context of Greg's series, I think we really only need to
> support hints for error_fatal, which are cases that users are supposed
> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
> are things that are never supposed to happen. A good stack trace is more
> important there than adding a hint to the message.
We also want to handle the caller passing NULL, so that we no longer
have to introduce 'Error *local_error = NULL' everywhere.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 9/19/19 1:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> "The evaluations of the initialization list expressions are
>> indeterminately sequenced with respect to one another and thus the order
>> in which any side effects occur is unspecified."
>>
>> which does not bode well for the assignment to __auto_errp_prop.errp.
>> All changes to errp would have to be within the same initializer. Maybe:
>>
>> #define MAKE_ERRP_SAFE() \
>>g_auto(ErrorPropagator) __auto_errp_prop = { \
>> .local_err = (__auto_errp_prop.err = errp, \
>> (errp = &__auto_errp_prop.local_err), NULL) }
>
> Is it guaranteed that .errp will not be initialized to NULL after evaluating
> of
> .local_err initializer?
Probably not.
>
>>
>> but by the time you get that complicated, just using a statement is
>> easier to read.
Either two declarations (the second being an unused dummy variable
declared solely for its initializer's side-effects) or a declaration and
a statement are the only sane ways I can see to provide guaranteed
ordering. It's hidden behind a macro, so I don't care which.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC] error: auto propagated local_err
19.09.2019 16:03, Kevin Wolf wrote:
> Am 19.09.2019 um 14:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 19.09.2019 12:17, 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.
>>>
>>> So I'd really like to restrict the use of error_propagate() to places
>>> where it's absolutely necessary. Unless, of course, you can fix these
>>> practical problems that error_propagate() causes for debugging.
>>>
>>> In fact, in the context of Greg's series, I think we really only need to
>>> support hints for error_fatal, which are cases that users are supposed
>>> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
>>> are things that are never supposed to happen. A good stack trace is more
>>> important there than adding a hint to the message.
>>>
>>
>> Interesting, that to handle error_append_hint problem, we don't need to
>> create local_err in case of errp==NULL either..
>>
>> So, possibly, we need the following steps:
>>
>> 1. implement MAKE_ERRP_SAFE_FOR_HINT (which only leave "*(errp) ==
>> error_fatal" in the if condition
>> 2. rebase Greg's series on it, to fix hints for fatal errors
>> 3. implement MAKE_ERRP_SAFE_FOR_DEREFERENCE (which only leave "(errp) ==
>> NULL" in the if condition
>> 4. convert all (almost all) local_err usage to use
>> MAKE_ERRP_SAFE_FOR_DEREFERENCE, which will
>> fix problem with error_abort (and also drop a lot of calls of
>> error_propagate)
>> 5. merely convert "void func(.., errp)" to "int func(.., errp)" and drop
>> MAKE_ERRP_SAFE_FOR_DEREFERENCE()
>> magic, together with dereferencing.
>
> Long macro names, but as the parameter will always only be "errp", it
> fits easily on a line, so this is fine.
Yes, I wanted to stress their meaning in plan..
Other variants, I can imagine:
MAKE_ERRP_SAFE_FOR_DEREFERENCE
WRAP_ERRP_FOR_DEREFERENCE
WRAP_NULL_ERRP
MAKE_ERRP_SAFE_FOR_HINT
WRAP_ERRP_FOR_HINT
WRAP_FATAL_ERRP
>
> I think I like this plan.
>
> Kevin
>
--
Best regards,
Vladimir
Re: [Qemu-devel] [RFC] error: auto propagated local_err
Am 19.09.2019 um 14:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 19.09.2019 12:17, 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.
> >
> > So I'd really like to restrict the use of error_propagate() to places
> > where it's absolutely necessary. Unless, of course, you can fix these
> > practical problems that error_propagate() causes for debugging.
> >
> > In fact, in the context of Greg's series, I think we really only need to
> > support hints for error_fatal, which are cases that users are supposed
> > to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
> > are things that are never supposed to happen. A good stack trace is more
> > important there than adding a hint to the message.
> >
>
> Interesting, that to handle error_append_hint problem, we don't need to
> create local_err in case of errp==NULL either..
>
> So, possibly, we need the following steps:
>
> 1. implement MAKE_ERRP_SAFE_FOR_HINT (which only leave "*(errp) ==
> error_fatal" in the if condition
> 2. rebase Greg's series on it, to fix hints for fatal errors
> 3. implement MAKE_ERRP_SAFE_FOR_DEREFERENCE (which only leave "(errp) ==
> NULL" in the if condition
> 4. convert all (almost all) local_err usage to use
> MAKE_ERRP_SAFE_FOR_DEREFERENCE, which will
> fix problem with error_abort (and also drop a lot of calls of
> error_propagate)
> 5. merely convert "void func(.., errp)" to "int func(.., errp)" and drop
> MAKE_ERRP_SAFE_FOR_DEREFERENCE()
> magic, together with dereferencing.
Long macro names, but as the parameter will always only be "errp", it
fits easily on a line, so this is fine.
I think I like this plan.
Kevin
Re: [Qemu-devel] [RFC] error: auto propagated local_err
19.09.2019 12:17, 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.
>
> So I'd really like to restrict the use of error_propagate() to places
> where it's absolutely necessary. Unless, of course, you can fix these
> practical problems that error_propagate() causes for debugging.
>
> In fact, in the context of Greg's series, I think we really only need to
> support hints for error_fatal, which are cases that users are supposed
> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
> are things that are never supposed to happen. A good stack trace is more
> important there than adding a hint to the message.
>
Interesting, that to handle error_append_hint problem, we don't need to
create local_err in case of errp==NULL either..
So, possibly, we need the following steps:
1. implement MAKE_ERRP_SAFE_FOR_HINT (which only leave "*(errp) == error_fatal"
in the if condition
2. rebase Greg's series on it, to fix hints for fatal errors
3. implement MAKE_ERRP_SAFE_FOR_DEREFERENCE (which only leave "(errp) == NULL"
in the if condition
4. convert all (almost all) local_err usage to use
MAKE_ERRP_SAFE_FOR_DEREFERENCE, which will
fix problem with error_abort (and also drop a lot of calls of
error_propagate)
5. merely convert "void func(.., errp)" to "int func(.., errp)" and drop
MAKE_ERRP_SAFE_FOR_DEREFERENCE()
magic, together with dereferencing.
--
Best regards,
Vladimir
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On Thu, Sep 19, 2019 at 10:21:44AM +, 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(_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 :|
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 19.09.19 12:03, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 12:33, Max Reitz wrote:
>> On 19.09.19 11:14, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.09.2019 11:59, Max Reitz wrote:
On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a proposal (three of them, actually) of auto propagation for
> local_err, to not call error_propagate on every exit point, when we
> deal with local_err.
>
> It also may help make Greg's series[1] about error_append_hint smaller.
>
> See definitions and examples below.
>
> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
> it, the idea will touch same code (and may be more).
>
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>include/qapi/error.h | 102 +++
>block.c | 63 --
>block/backup.c | 8 +++-
>block/gluster.c | 7 +++
>4 files changed, 144 insertions(+), 36 deletions(-)
If the combination of “if (local_err) { error_propagate(...); ... }” is
what’s cumbersome, can’t this be done simpler by adding an
error_propagate() variant with a return value?
i.e.
bool has_error_then_propagate(Error **errp, Error *err)
{
if (!err) {
return false;
}
error_propagate(errp, err);
return true;
}
And then turn all instances of
if (local_err) {
error_propagate(errp, local_err);
...
}
into
if (has_error_then_propagate(errp, local_err)) {
...
}
?
Max
>>>
>>> No, originally cumbersome is introducing local_err in a lot of new places by
>>> Greg's series. MAKE_ERRP_SAFE macro makes it as simple as one macro call
>>> instead (in each function where we need local_err).
>>
>> Does it need more than one local_err per function?
>>
>> Because if it didn’t, I’d find one “Error *local_err;” simpler than one
>> macro incantation.
>>
>> (It has the same LoC, and it makes code readers ask the same question:
>> “Why do we need it?” Which has the same answer for both; but one is
>> immediately readable code, whereas the other is a macro.)
>
> Not the same, you didn't count error_propagate
I did, because it would part of the if () statement in my proposal.
> And your example don't match Greg's case, there no "if (local_err)" in it,
Ah, right, I see. OK then.
> just "error_append_hint(errp)", which don't work for error_fatal and
> error_abort
> (Yes, I agree with Kevin, that it should work only for error_fatal)
True.
[...]
>> Now Kevin has given an actual advantage, which is that local_err
>> complicates debugging. I’ve never had that problem myself, but that
>> would indeed be an advantage that may warrant some magic.
Although after some more consideration I realized this probably cannot
be achieved with this series, actually.
Max
Re: [Qemu-devel] [RFC] error: auto propagated local_err
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(_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.
--
Best regards,
Vladimir
Re: [Qemu-devel] [RFC] error: auto propagated local_err
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(_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.
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-devel] [RFC] error: auto propagated local_err
On Thu, 19 Sep 2019 09:28:11 +
Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 11:59, Greg Kurz wrote:
> > On Wed, 18 Sep 2019 16:02:44 +0300
> > Vladimir Sementsov-Ogievskiy wrote:
> >
> >> Hi all!
> >>
> >> Here is a proposal (three of them, actually) of auto propagation for
> >> local_err, to not call error_propagate on every exit point, when we
> >> deal with local_err.
> >>
> >> It also may help make Greg's series[1] about error_append_hint smaller.
> >>
> >
> > This will depend on whether we reach a consensus soon enough (soft
> > freeze for 4.2 is 2019-10-29). Otherwise, I think my series should
> > be merged as is, and auto-propagation to be delayed to 4.3.
> >
> >> See definitions and examples below.
> >>
> >> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
> >> it, the idea will touch same code (and may be more).
> >>
> >
> > When we have a good auto-propagation mechanism available, I guess
> > this can be beneficial for most of the code, not only the places
> > where we add hints (or prepend something, see below).
> >
> >> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >> ---
> >> include/qapi/error.h | 102 +++
> >> block.c | 63 --
> >> block/backup.c | 8 +++-
> >> block/gluster.c | 7 +++
> >> 4 files changed, 144 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/include/qapi/error.h b/include/qapi/error.h
> >> index 3f95141a01..083e061014 100644
> >> --- a/include/qapi/error.h
> >> +++ b/include/qapi/error.h
> >> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
> >> ErrorClass err_class, const char *fmt, ...)
> >> GCC_FMT_ATTR(6, 7);
> >>
> >> +typedef struct ErrorPropagator {
> >> +Error **errp;
> >> +Error *local_err;
> >> +} ErrorPropagator;
> >> +
> >> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> >> +{
> >> +if (prop->local_err) {
> >> +error_propagate(prop->errp, prop->local_err);
> >
> > We also have error_propagate_prepend(), which is basically a variant of
> > error_prepend()+error_propagate() that can cope with _fatal. This
> > was introduced by Markus in commit 4b5766488fd3, for similar reasons that
> > motivated my series. It has ~30 users in the tree.
> >
> > There's no such thing a generic cleanup function with a printf-like
> > interface, so all of these should be converted back to error_prepend()
> > if we go for auto-propagation.
> >
> > Aside from propagation, one can sometime choose to call things like
> > error_free() or error_free_or_abort()... Auto-propagation should
> > detect that and not call error_propagate() in this case.
>
> Hmm, for example, qmp_eject, which error_free or error_propagate..
> We can leave such cases as is, not many of them. Or introduce
> safe_errp_free(Error **errp), which will zero pointer after freeing.
>
Maybe even turning error_free() to take an Error ** ? It looks
safe to zero out a dangling pointer. Of course the API change
would need to be propagated to all error_* functions that
explicitly call error_free().
> >
> >> +}
> >> +}
> >> +
> >> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator,
> >> error_propagator_cleanup);
> >> +
> >> +/*
> >> + * ErrorPropagationPair
> >> + *
> >> + * [Error *local_err, Error **errp]
> >> + *
> >> + * First element is local_err, second is original errp, which is
> >> propagation
> >> + * target. Yes, errp has a bit another type, so it should be converted.
> >> + *
> >> + * ErrorPropagationPair may be used as errp, which points to local_err,
> >> + * as it's type is compatible.
> >> + */
> >> +typedef Error *ErrorPropagationPair[2];
> >> +
> >> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair
> >> *arr)
> >> +{
> >> +Error *local_err = (*arr)[0];
> >> +Error **errp = (Error **)(*arr)[1];
> >> +
> >> +if (local_err) {
> >> +error_propagate(errp, local_err);
> >> +}
> >> +}
> >> +
> >> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
> >> + error_propagation_pair_cleanup);
> >> +
> >> +/*
> >> + * DEF_AUTO_ERRP
> >> + *
> >> + * Define auto_errp variable, which may be used instead of errp, and
> >> + * *auto_errp may be safely checked to be zero or not, and may be safely
> >> + * used for error_append_hint(). auto_errp is automatically propagated
> >> + * to errp at function exit.
> >> + */
> >> +#define DEF_AUTO_ERRP(auto_errp, errp) \
> >> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
> >> +
> >> +
> >> +/*
> >> + * Another variant:
> >> + * Pros:
> >> + * - normal structure instead of cheating with array
> >> + * - we can directly use errp, if it's not NULL and don't point to
> >> + * error_abort or error_fatal
> >> + * Cons:
> >> + * - we need to
Re: [Qemu-devel] [RFC] error: auto propagated local_err
19.09.2019 12:33, Max Reitz wrote:
> On 19.09.19 11:14, Vladimir Sementsov-Ogievskiy wrote:
>> 19.09.2019 11:59, Max Reitz wrote:
>>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
Hi all!
Here is a proposal (three of them, actually) of auto propagation for
local_err, to not call error_propagate on every exit point, when we
deal with local_err.
It also may help make Greg's series[1] about error_append_hint smaller.
See definitions and examples below.
I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
it, the idea will touch same code (and may be more).
[1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/qapi/error.h | 102 +++
block.c | 63 --
block/backup.c | 8 +++-
block/gluster.c | 7 +++
4 files changed, 144 insertions(+), 36 deletions(-)
>>>
>>> If the combination of “if (local_err) { error_propagate(...); ... }” is
>>> what’s cumbersome, can’t this be done simpler by adding an
>>> error_propagate() variant with a return value?
>>>
>>> i.e.
>>>
>>> bool has_error_then_propagate(Error **errp, Error *err)
>>> {
>>> if (!err) {
>>> return false;
>>> }
>>> error_propagate(errp, err);
>>> return true;
>>> }
>>>
>>> And then turn all instances of
>>>
>>> if (local_err) {
>>> error_propagate(errp, local_err);
>>> ...
>>> }
>>>
>>> into
>>>
>>> if (has_error_then_propagate(errp, local_err)) {
>>> ...
>>> }
>>>
>>> ?
>>>
>>> Max
>>>
>>
>> No, originally cumbersome is introducing local_err in a lot of new places by
>> Greg's series. MAKE_ERRP_SAFE macro makes it as simple as one macro call
>> instead (in each function where we need local_err).
>
> Does it need more than one local_err per function?
>
> Because if it didn’t, I’d find one “Error *local_err;” simpler than one
> macro incantation.
>
> (It has the same LoC, and it makes code readers ask the same question:
> “Why do we need it?” Which has the same answer for both; but one is
> immediately readable code, whereas the other is a macro.)
Not the same, you didn't count error_propagate
And your example don't match Greg's case, there no "if (local_err)" in it,
just "error_append_hint(errp)", which don't work for error_fatal and error_abort
(Yes, I agree with Kevin, that it should work only for error_fatal)
>
>> Also, auto-propagation seems correct thing to do, which fits good into
>> g_auto concept, so even without any macro it just allows to drop several
>> error_propagate
>> calls (or may be several goto statements to do one error_propagate call) into
>> one definitions. It's the same story like with g_autofree vs g_free.
>
> I don’t see the advantage here. You have to do the if () statement
> anyway, so it isn’t like you’re saving any LoC. In addition, I
> personally don’t find code hidden through __attribute__((cleanup))
> easier to understand than explicitly written code.
>
> It isn’t like I don’t like __attribute__((cleanup)). But it does count
> under “magic” in my book, and thus I’d avoid it if it doesn’t bring
> actual advantages. (It does bring actual advantages for things like
> auto-freeing memory, auto-closing FDs, or zeroing secret buffers before
> freeing them. In all those cases, you save LoC, and you prevent
> accidental leakage. I don’t quite see such advantages here, but I may
> well be mistaken.)
>
>
> Now Kevin has given an actual advantage, which is that local_err
> complicates debugging. I’ve never had that problem myself, but that
> would indeed be an advantage that may warrant some magic.
>
> Max
>
--
Best regards,
Vladimir
Re: [Qemu-devel] [RFC] error: auto propagated local_err
19.09.2019 12:17, 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.
Hmm, never tested it, but looking at the code I can't understand how can that
be. src/func/line are set in error_setg.. and in error_propagate() we just
set the errp of the caller, src/func/line unchanged.
Still, I see that these useful fields are printed only for error_abort, for
which we usually have coredump, which provides a lot more information.
> It also makes error_abort completely useless because at the point where
> the process gets aborted, the interesting information is already lost.
Aha, understand this point, error_abort just don't work as desired, if
we swap it by local_err. And we can fix it by using macro: never create
local_err for error_abort, let it abort exactly on error_setg.
>
> So I'd really like to restrict the use of error_propagate() to places
> where it's absolutely necessary. Unless, of course, you can fix these
> practical problems that error_propagate() causes for debugging.
>
> In fact, in the context of Greg's series, I think we really only need to
> support hints for error_fatal, which are cases that users are supposed
> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
> are things that are never supposed to happen. A good stack trace is more
> important there than adding a hint to the message.
>
Agreed
--
Best regards,
Vladimir
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 19.09.19 11:14, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 11:59, Max Reitz wrote:
>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is a proposal (three of them, actually) of auto propagation for
>>> local_err, to not call error_propagate on every exit point, when we
>>> deal with local_err.
>>>
>>> It also may help make Greg's series[1] about error_append_hint smaller.
>>>
>>> See definitions and examples below.
>>>
>>> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
>>> it, the idea will touch same code (and may be more).
>>>
>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> include/qapi/error.h | 102 +++
>>> block.c | 63 --
>>> block/backup.c | 8 +++-
>>> block/gluster.c | 7 +++
>>> 4 files changed, 144 insertions(+), 36 deletions(-)
>>
>> If the combination of “if (local_err) { error_propagate(...); ... }” is
>> what’s cumbersome, can’t this be done simpler by adding an
>> error_propagate() variant with a return value?
>>
>> i.e.
>>
>> bool has_error_then_propagate(Error **errp, Error *err)
>> {
>> if (!err) {
>> return false;
>> }
>> error_propagate(errp, err);
>> return true;
>> }
>>
>> And then turn all instances of
>>
>> if (local_err) {
>> error_propagate(errp, local_err);
>> ...
>> }
>>
>> into
>>
>> if (has_error_then_propagate(errp, local_err)) {
>> ...
>> }
>>
>> ?
>>
>> Max
>>
>
> No, originally cumbersome is introducing local_err in a lot of new places by
> Greg's series. MAKE_ERRP_SAFE macro makes it as simple as one macro call
> instead (in each function where we need local_err).
Does it need more than one local_err per function?
Because if it didn’t, I’d find one “Error *local_err;” simpler than one
macro incantation.
(It has the same LoC, and it makes code readers ask the same question:
“Why do we need it?” Which has the same answer for both; but one is
immediately readable code, whereas the other is a macro.)
> Also, auto-propagation seems correct thing to do, which fits good into
> g_auto concept, so even without any macro it just allows to drop several
> error_propagate
> calls (or may be several goto statements to do one error_propagate call) into
> one definitions. It's the same story like with g_autofree vs g_free.
I don’t see the advantage here. You have to do the if () statement
anyway, so it isn’t like you’re saving any LoC. In addition, I
personally don’t find code hidden through __attribute__((cleanup))
easier to understand than explicitly written code.
It isn’t like I don’t like __attribute__((cleanup)). But it does count
under “magic” in my book, and thus I’d avoid it if it doesn’t bring
actual advantages. (It does bring actual advantages for things like
auto-freeing memory, auto-closing FDs, or zeroing secret buffers before
freeing them. In all those cases, you save LoC, and you prevent
accidental leakage. I don’t quite see such advantages here, but I may
well be mistaken.)
Now Kevin has given an actual advantage, which is that local_err
complicates debugging. I’ve never had that problem myself, but that
would indeed be an advantage that may warrant some magic.
Max
Re: [Qemu-devel] [RFC] error: auto propagated local_err
19.09.2019 11:59, Greg Kurz wrote:
> On Wed, 18 Sep 2019 16:02:44 +0300
> Vladimir Sementsov-Ogievskiy wrote:
>
>> Hi all!
>>
>> Here is a proposal (three of them, actually) of auto propagation for
>> local_err, to not call error_propagate on every exit point, when we
>> deal with local_err.
>>
>> It also may help make Greg's series[1] about error_append_hint smaller.
>>
>
> This will depend on whether we reach a consensus soon enough (soft
> freeze for 4.2 is 2019-10-29). Otherwise, I think my series should
> be merged as is, and auto-propagation to be delayed to 4.3.
>
>> See definitions and examples below.
>>
>> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
>> it, the idea will touch same code (and may be more).
>>
>
> When we have a good auto-propagation mechanism available, I guess
> this can be beneficial for most of the code, not only the places
> where we add hints (or prepend something, see below).
>
>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> include/qapi/error.h | 102 +++
>> block.c | 63 --
>> block/backup.c | 8 +++-
>> block/gluster.c | 7 +++
>> 4 files changed, 144 insertions(+), 36 deletions(-)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 3f95141a01..083e061014 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
>> ErrorClass err_class, const char *fmt, ...)
>> GCC_FMT_ATTR(6, 7);
>>
>> +typedef struct ErrorPropagator {
>> +Error **errp;
>> +Error *local_err;
>> +} ErrorPropagator;
>> +
>> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>> +{
>> +if (prop->local_err) {
>> +error_propagate(prop->errp, prop->local_err);
>
> We also have error_propagate_prepend(), which is basically a variant of
> error_prepend()+error_propagate() that can cope with _fatal. This
> was introduced by Markus in commit 4b5766488fd3, for similar reasons that
> motivated my series. It has ~30 users in the tree.
>
> There's no such thing a generic cleanup function with a printf-like
> interface, so all of these should be converted back to error_prepend()
> if we go for auto-propagation.
>
> Aside from propagation, one can sometime choose to call things like
> error_free() or error_free_or_abort()... Auto-propagation should
> detect that and not call error_propagate() in this case.
Hmm, for example, qmp_eject, which error_free or error_propagate..
We can leave such cases as is, not many of them. Or introduce
safe_errp_free(Error **errp), which will zero pointer after freeing.
>
>> +}
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>> +
>> +/*
>> + * ErrorPropagationPair
>> + *
>> + * [Error *local_err, Error **errp]
>> + *
>> + * First element is local_err, second is original errp, which is propagation
>> + * target. Yes, errp has a bit another type, so it should be converted.
>> + *
>> + * ErrorPropagationPair may be used as errp, which points to local_err,
>> + * as it's type is compatible.
>> + */
>> +typedef Error *ErrorPropagationPair[2];
>> +
>> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr)
>> +{
>> +Error *local_err = (*arr)[0];
>> +Error **errp = (Error **)(*arr)[1];
>> +
>> +if (local_err) {
>> +error_propagate(errp, local_err);
>> +}
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
>> + error_propagation_pair_cleanup);
>> +
>> +/*
>> + * DEF_AUTO_ERRP
>> + *
>> + * Define auto_errp variable, which may be used instead of errp, and
>> + * *auto_errp may be safely checked to be zero or not, and may be safely
>> + * used for error_append_hint(). auto_errp is automatically propagated
>> + * to errp at function exit.
>> + */
>> +#define DEF_AUTO_ERRP(auto_errp, errp) \
>> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
>> +
>> +
>> +/*
>> + * Another variant:
>> + * Pros:
>> + * - normal structure instead of cheating with array
>> + * - we can directly use errp, if it's not NULL and don't point to
>> + * error_abort or error_fatal
>> + * Cons:
>> + * - we need to define two variables instead of one
>> + */
>> +typedef struct ErrorPropagationStruct {
>> +Error *local_err;
>> +Error **errp;
>> +} ErrorPropagationStruct;
>> +
>> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
>> *prop)
>> +{
>> +if (prop->local_err) {
>> +error_propagate(prop->errp, prop->local_err);
>> +}
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
>> + error_propagation_struct_cleanup);
>> +
>> +#define DEF_AUTO_ERRP_V2(auto_errp,
Re: [Qemu-devel] [RFC] error: auto propagated local_err
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.
So I'd really like to restrict the use of error_propagate() to places
where it's absolutely necessary. Unless, of course, you can fix these
practical problems that error_propagate() causes for debugging.
In fact, in the context of Greg's series, I think we really only need to
support hints for error_fatal, which are cases that users are supposed
to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
are things that are never supposed to happen. A good stack trace is more
important there than adding a hint to the message.
Kevin
Re: [Qemu-devel] [RFC] error: auto propagated local_err
19.09.2019 11:59, Max Reitz wrote:
> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is a proposal (three of them, actually) of auto propagation for
>> local_err, to not call error_propagate on every exit point, when we
>> deal with local_err.
>>
>> It also may help make Greg's series[1] about error_append_hint smaller.
>>
>> See definitions and examples below.
>>
>> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
>> it, the idea will touch same code (and may be more).
>>
>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> include/qapi/error.h | 102 +++
>> block.c | 63 --
>> block/backup.c | 8 +++-
>> block/gluster.c | 7 +++
>> 4 files changed, 144 insertions(+), 36 deletions(-)
>
> If the combination of “if (local_err) { error_propagate(...); ... }” is
> what’s cumbersome, can’t this be done simpler by adding an
> error_propagate() variant with a return value?
>
> i.e.
>
> bool has_error_then_propagate(Error **errp, Error *err)
> {
> if (!err) {
> return false;
> }
> error_propagate(errp, err);
> return true;
> }
>
> And then turn all instances of
>
> if (local_err) {
> error_propagate(errp, local_err);
> ...
> }
>
> into
>
> if (has_error_then_propagate(errp, local_err)) {
> ...
> }
>
> ?
>
> Max
>
No, originally cumbersome is introducing local_err in a lot of new places by
Greg's series. MAKE_ERRP_SAFE macro makes it as simple as one macro call
instead (in each function where we need local_err).
Also, auto-propagation seems correct thing to do, which fits good into
g_auto concept, so even without any macro it just allows to drop several
error_propagate
calls (or may be several goto statements to do one error_propagate call) into
one definitions. It's the same story like with g_autofree vs g_free.
--
Best regards,
Vladimir
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On Wed, 18 Sep 2019 16:02:44 +0300
Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a proposal (three of them, actually) of auto propagation for
> local_err, to not call error_propagate on every exit point, when we
> deal with local_err.
>
> It also may help make Greg's series[1] about error_append_hint smaller.
>
This will depend on whether we reach a consensus soon enough (soft
freeze for 4.2 is 2019-10-29). Otherwise, I think my series should
be merged as is, and auto-propagation to be delayed to 4.3.
> See definitions and examples below.
>
> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
> it, the idea will touch same code (and may be more).
>
When we have a good auto-propagation mechanism available, I guess
this can be beneficial for most of the code, not only the places
where we add hints (or prepend something, see below).
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> include/qapi/error.h | 102 +++
> block.c | 63 --
> block/backup.c | 8 +++-
> block/gluster.c | 7 +++
> 4 files changed, 144 insertions(+), 36 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..083e061014 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
> ErrorClass err_class, const char *fmt, ...)
> GCC_FMT_ATTR(6, 7);
>
> +typedef struct ErrorPropagator {
> +Error **errp;
> +Error *local_err;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
We also have error_propagate_prepend(), which is basically a variant of
error_prepend()+error_propagate() that can cope with _fatal. This
was introduced by Markus in commit 4b5766488fd3, for similar reasons that
motivated my series. It has ~30 users in the tree.
There's no such thing a generic cleanup function with a printf-like
interface, so all of these should be converted back to error_prepend()
if we go for auto-propagation.
Aside from propagation, one can sometime choose to call things like
error_free() or error_free_or_abort()... Auto-propagation should
detect that and not call error_propagate() in this case.
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ErrorPropagationPair
> + *
> + * [Error *local_err, Error **errp]
> + *
> + * First element is local_err, second is original errp, which is propagation
> + * target. Yes, errp has a bit another type, so it should be converted.
> + *
> + * ErrorPropagationPair may be used as errp, which points to local_err,
> + * as it's type is compatible.
> + */
> +typedef Error *ErrorPropagationPair[2];
> +
> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr)
> +{
> +Error *local_err = (*arr)[0];
> +Error **errp = (Error **)(*arr)[1];
> +
> +if (local_err) {
> +error_propagate(errp, local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
> + error_propagation_pair_cleanup);
> +
> +/*
> + * DEF_AUTO_ERRP
> + *
> + * Define auto_errp variable, which may be used instead of errp, and
> + * *auto_errp may be safely checked to be zero or not, and may be safely
> + * used for error_append_hint(). auto_errp is automatically propagated
> + * to errp at function exit.
> + */
> +#define DEF_AUTO_ERRP(auto_errp, errp) \
> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
> +
> +
> +/*
> + * Another variant:
> + * Pros:
> + * - normal structure instead of cheating with array
> + * - we can directly use errp, if it's not NULL and don't point to
> + * error_abort or error_fatal
> + * Cons:
> + * - we need to define two variables instead of one
> + */
> +typedef struct ErrorPropagationStruct {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagationStruct;
> +
> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
> *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
> + error_propagation_struct_cleanup);
> +
> +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
> +Error **auto_errp = \
> +((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal)
> ? \
> +&__auto_errp_prop.local_err : \
> +(errp);
> +
> +/*
> + * Third variant:
> + * Pros:
> + * - simpler movement for functions which don't have local_err yet
> + * the
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a proposal (three of them, actually) of auto propagation for
> local_err, to not call error_propagate on every exit point, when we
> deal with local_err.
>
> It also may help make Greg's series[1] about error_append_hint smaller.
>
> See definitions and examples below.
>
> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
> it, the idea will touch same code (and may be more).
>
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> include/qapi/error.h | 102 +++
> block.c | 63 --
> block/backup.c | 8 +++-
> block/gluster.c | 7 +++
> 4 files changed, 144 insertions(+), 36 deletions(-)
If the combination of “if (local_err) { error_propagate(...); ... }” is
what’s cumbersome, can’t this be done simpler by adding an
error_propagate() variant with a return value?
i.e.
bool has_error_then_propagate(Error **errp, Error *err)
{
if (!err) {
return false;
}
error_propagate(errp, err);
return true;
}
And then turn all instances of
if (local_err) {
error_propagate(errp, local_err);
...
}
into
if (has_error_then_propagate(errp, local_err)) {
...
}
?
Max
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 19.09.19 10:20, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 10:53, David Hildenbrand wrote:
>> On 19.09.19 09:41, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.09.2019 10:32, David Hildenbrand wrote:
On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a proposal (three of them, actually) of auto propagation for
> local_err, to not call error_propagate on every exit point, when we
> deal with local_err.
>
> It also may help make Greg's series[1] about error_append_hint smaller.
>
> See definitions and examples below.
>
> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
> it, the idea will touch same code (and may be more).
>
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>include/qapi/error.h | 102 +++
>block.c | 63 --
>block/backup.c | 8 +++-
>block/gluster.c | 7 +++
>4 files changed, 144 insertions(+), 36 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..083e061014 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
>ErrorClass err_class, const char *fmt, ...)
>GCC_FMT_ATTR(6, 7);
>
> +typedef struct ErrorPropagator {
> +Error **errp;
> +Error *local_err;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator,
> error_propagator_cleanup);
> +
> +/*
> + * ErrorPropagationPair
> + *
> + * [Error *local_err, Error **errp]
> + *
> + * First element is local_err, second is original errp, which is
> propagation
> + * target. Yes, errp has a bit another type, so it should be converted.
> + *
> + * ErrorPropagationPair may be used as errp, which points to local_err,
> + * as it's type is compatible.
> + */
> +typedef Error *ErrorPropagationPair[2];
> +
> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair
> *arr)
> +{
> +Error *local_err = (*arr)[0];
> +Error **errp = (Error **)(*arr)[1];
> +
> +if (local_err) {
> +error_propagate(errp, local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
> + error_propagation_pair_cleanup);
> +
> +/*
> + * DEF_AUTO_ERRP
> + *
> + * Define auto_errp variable, which may be used instead of errp, and
> + * *auto_errp may be safely checked to be zero or not, and may be safely
> + * used for error_append_hint(). auto_errp is automatically propagated
> + * to errp at function exit.
> + */
> +#define DEF_AUTO_ERRP(auto_errp, errp) \
> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
> +
> +
> +/*
> + * Another variant:
> + * Pros:
> + * - normal structure instead of cheating with array
> + * - we can directly use errp, if it's not NULL and don't point to
> + * error_abort or error_fatal
> + * Cons:
> + * - we need to define two variables instead of one
> + */
> +typedef struct ErrorPropagationStruct {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagationStruct;
> +
> +static inline void
> error_propagation_struct_cleanup(ErrorPropagationStruct *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
> + error_propagation_struct_cleanup);
> +
> +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp =
> (errp)}; \
> +Error **auto_errp = \
> +((errp) == NULL || *(errp) == error_abort || *(errp) ==
> error_fatal) ? \
> +&__auto_errp_prop.local_err : \
> +(errp);
> +
> +/*
> + * Third variant:
> + * Pros:
> + * - simpler movement for functions which don't have local_err yet
> + * the only thing to do is to call one macro at function start.
> + * This extremely simplifies Greg's series
> + * Cons:
> + * - looks like errp shadowing.. Still seems safe.
> + * - must be after all definitions of local variables and before any
> + * code.
>
Re: [Qemu-devel] [RFC] error: auto propagated local_err
19.09.2019 10:53, David Hildenbrand wrote:
> On 19.09.19 09:41, Vladimir Sementsov-Ogievskiy wrote:
>> 19.09.2019 10:32, David Hildenbrand wrote:
>>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
Hi all!
Here is a proposal (three of them, actually) of auto propagation for
local_err, to not call error_propagate on every exit point, when we
deal with local_err.
It also may help make Greg's series[1] about error_append_hint smaller.
See definitions and examples below.
I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
it, the idea will touch same code (and may be more).
[1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/qapi/error.h | 102 +++
block.c | 63 --
block/backup.c | 8 +++-
block/gluster.c | 7 +++
4 files changed, 144 insertions(+), 36 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3f95141a01..083e061014 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
ErrorClass err_class, const char *fmt, ...)
GCC_FMT_ATTR(6, 7);
+typedef struct ErrorPropagator {
+Error **errp;
+Error *local_err;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+if (prop->local_err) {
+error_propagate(prop->errp, prop->local_err);
+}
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator,
error_propagator_cleanup);
+
+/*
+ * ErrorPropagationPair
+ *
+ * [Error *local_err, Error **errp]
+ *
+ * First element is local_err, second is original errp, which is
propagation
+ * target. Yes, errp has a bit another type, so it should be converted.
+ *
+ * ErrorPropagationPair may be used as errp, which points to local_err,
+ * as it's type is compatible.
+ */
+typedef Error *ErrorPropagationPair[2];
+
+static inline void error_propagation_pair_cleanup(ErrorPropagationPair
*arr)
+{
+Error *local_err = (*arr)[0];
+Error **errp = (Error **)(*arr)[1];
+
+if (local_err) {
+error_propagate(errp, local_err);
+}
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
+ error_propagation_pair_cleanup);
+
+/*
+ * DEF_AUTO_ERRP
+ *
+ * Define auto_errp variable, which may be used instead of errp, and
+ * *auto_errp may be safely checked to be zero or not, and may be safely
+ * used for error_append_hint(). auto_errp is automatically propagated
+ * to errp at function exit.
+ */
+#define DEF_AUTO_ERRP(auto_errp, errp) \
+g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
+
+
+/*
+ * Another variant:
+ * Pros:
+ * - normal structure instead of cheating with array
+ * - we can directly use errp, if it's not NULL and don't point to
+ * error_abort or error_fatal
+ * Cons:
+ * - we need to define two variables instead of one
+ */
+typedef struct ErrorPropagationStruct {
+Error *local_err;
+Error **errp;
+} ErrorPropagationStruct;
+
+static inline void
error_propagation_struct_cleanup(ErrorPropagationStruct *prop)
+{
+if (prop->local_err) {
+error_propagate(prop->errp, prop->local_err);
+}
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
+ error_propagation_struct_cleanup);
+
+#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
+g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)};
\
+Error **auto_errp = \
+((errp) == NULL || *(errp) == error_abort || *(errp) ==
error_fatal) ? \
+&__auto_errp_prop.local_err : \
+(errp);
+
+/*
+ * Third variant:
+ * Pros:
+ * - simpler movement for functions which don't have local_err yet
+ * the only thing to do is to call one macro at function start.
+ * This extremely simplifies Greg's series
+ * Cons:
+ * - looks like errp shadowing.. Still seems safe.
+ * - must be after all definitions of local variables and before any
+ * code.
+ * - like v2, several statements in one open macro
+ */
+#define MAKE_ERRP_SAFE(errp) \
+g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
+if
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 19.09.19 09:41, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 10:32, David Hildenbrand wrote:
>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is a proposal (three of them, actually) of auto propagation for
>>> local_err, to not call error_propagate on every exit point, when we
>>> deal with local_err.
>>>
>>> It also may help make Greg's series[1] about error_append_hint smaller.
>>>
>>> See definitions and examples below.
>>>
>>> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
>>> it, the idea will touch same code (and may be more).
>>>
>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> include/qapi/error.h | 102 +++
>>> block.c | 63 --
>>> block/backup.c | 8 +++-
>>> block/gluster.c | 7 +++
>>> 4 files changed, 144 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index 3f95141a01..083e061014 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
>>> ErrorClass err_class, const char *fmt, ...)
>>> GCC_FMT_ATTR(6, 7);
>>>
>>> +typedef struct ErrorPropagator {
>>> +Error **errp;
>>> +Error *local_err;
>>> +} ErrorPropagator;
>>> +
>>> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>>> +{
>>> +if (prop->local_err) {
>>> +error_propagate(prop->errp, prop->local_err);
>>> +}
>>> +}
>>> +
>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator,
>>> error_propagator_cleanup);
>>> +
>>> +/*
>>> + * ErrorPropagationPair
>>> + *
>>> + * [Error *local_err, Error **errp]
>>> + *
>>> + * First element is local_err, second is original errp, which is
>>> propagation
>>> + * target. Yes, errp has a bit another type, so it should be converted.
>>> + *
>>> + * ErrorPropagationPair may be used as errp, which points to local_err,
>>> + * as it's type is compatible.
>>> + */
>>> +typedef Error *ErrorPropagationPair[2];
>>> +
>>> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair
>>> *arr)
>>> +{
>>> +Error *local_err = (*arr)[0];
>>> +Error **errp = (Error **)(*arr)[1];
>>> +
>>> +if (local_err) {
>>> +error_propagate(errp, local_err);
>>> +}
>>> +}
>>> +
>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
>>> + error_propagation_pair_cleanup);
>>> +
>>> +/*
>>> + * DEF_AUTO_ERRP
>>> + *
>>> + * Define auto_errp variable, which may be used instead of errp, and
>>> + * *auto_errp may be safely checked to be zero or not, and may be safely
>>> + * used for error_append_hint(). auto_errp is automatically propagated
>>> + * to errp at function exit.
>>> + */
>>> +#define DEF_AUTO_ERRP(auto_errp, errp) \
>>> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
>>> +
>>> +
>>> +/*
>>> + * Another variant:
>>> + * Pros:
>>> + * - normal structure instead of cheating with array
>>> + * - we can directly use errp, if it's not NULL and don't point to
>>> + * error_abort or error_fatal
>>> + * Cons:
>>> + * - we need to define two variables instead of one
>>> + */
>>> +typedef struct ErrorPropagationStruct {
>>> +Error *local_err;
>>> +Error **errp;
>>> +} ErrorPropagationStruct;
>>> +
>>> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
>>> *prop)
>>> +{
>>> +if (prop->local_err) {
>>> +error_propagate(prop->errp, prop->local_err);
>>> +}
>>> +}
>>> +
>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
>>> + error_propagation_struct_cleanup);
>>> +
>>> +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
>>> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
>>> +Error **auto_errp = \
>>> +((errp) == NULL || *(errp) == error_abort || *(errp) ==
>>> error_fatal) ? \
>>> +&__auto_errp_prop.local_err : \
>>> +(errp);
>>> +
>>> +/*
>>> + * Third variant:
>>> + * Pros:
>>> + * - simpler movement for functions which don't have local_err yet
>>> + * the only thing to do is to call one macro at function start.
>>> + * This extremely simplifies Greg's series
>>> + * Cons:
>>> + * - looks like errp shadowing.. Still seems safe.
>>> + * - must be after all definitions of local variables and before any
>>> + * code.
>>> + * - like v2, several statements in one open macro
>>> + */
>>> +#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; \
>>> +}
>>
>>
>> Using that idea, what about something like this:
>>
>>
Re: [Qemu-devel] [RFC] error: auto propagated local_err
19.09.2019 10:32, David Hildenbrand wrote:
> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is a proposal (three of them, actually) of auto propagation for
>> local_err, to not call error_propagate on every exit point, when we
>> deal with local_err.
>>
>> It also may help make Greg's series[1] about error_append_hint smaller.
>>
>> See definitions and examples below.
>>
>> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
>> it, the idea will touch same code (and may be more).
>>
>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> include/qapi/error.h | 102 +++
>> block.c | 63 --
>> block/backup.c | 8 +++-
>> block/gluster.c | 7 +++
>> 4 files changed, 144 insertions(+), 36 deletions(-)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 3f95141a01..083e061014 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
>> ErrorClass err_class, const char *fmt, ...)
>> GCC_FMT_ATTR(6, 7);
>>
>> +typedef struct ErrorPropagator {
>> +Error **errp;
>> +Error *local_err;
>> +} ErrorPropagator;
>> +
>> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>> +{
>> +if (prop->local_err) {
>> +error_propagate(prop->errp, prop->local_err);
>> +}
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>> +
>> +/*
>> + * ErrorPropagationPair
>> + *
>> + * [Error *local_err, Error **errp]
>> + *
>> + * First element is local_err, second is original errp, which is propagation
>> + * target. Yes, errp has a bit another type, so it should be converted.
>> + *
>> + * ErrorPropagationPair may be used as errp, which points to local_err,
>> + * as it's type is compatible.
>> + */
>> +typedef Error *ErrorPropagationPair[2];
>> +
>> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr)
>> +{
>> +Error *local_err = (*arr)[0];
>> +Error **errp = (Error **)(*arr)[1];
>> +
>> +if (local_err) {
>> +error_propagate(errp, local_err);
>> +}
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
>> + error_propagation_pair_cleanup);
>> +
>> +/*
>> + * DEF_AUTO_ERRP
>> + *
>> + * Define auto_errp variable, which may be used instead of errp, and
>> + * *auto_errp may be safely checked to be zero or not, and may be safely
>> + * used for error_append_hint(). auto_errp is automatically propagated
>> + * to errp at function exit.
>> + */
>> +#define DEF_AUTO_ERRP(auto_errp, errp) \
>> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
>> +
>> +
>> +/*
>> + * Another variant:
>> + * Pros:
>> + * - normal structure instead of cheating with array
>> + * - we can directly use errp, if it's not NULL and don't point to
>> + * error_abort or error_fatal
>> + * Cons:
>> + * - we need to define two variables instead of one
>> + */
>> +typedef struct ErrorPropagationStruct {
>> +Error *local_err;
>> +Error **errp;
>> +} ErrorPropagationStruct;
>> +
>> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
>> *prop)
>> +{
>> +if (prop->local_err) {
>> +error_propagate(prop->errp, prop->local_err);
>> +}
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
>> + error_propagation_struct_cleanup);
>> +
>> +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
>> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
>> +Error **auto_errp = \
>> +((errp) == NULL || *(errp) == error_abort || *(errp) ==
>> error_fatal) ? \
>> +&__auto_errp_prop.local_err : \
>> +(errp);
>> +
>> +/*
>> + * Third variant:
>> + * Pros:
>> + * - simpler movement for functions which don't have local_err yet
>> + * the only thing to do is to call one macro at function start.
>> + * This extremely simplifies Greg's series
>> + * Cons:
>> + * - looks like errp shadowing.. Still seems safe.
>> + * - must be after all definitions of local variables and before any
>> + * code.
>> + * - like v2, several statements in one open macro
>> + */
>> +#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; \
>> +}
>
>
> Using that idea, what about something like this:
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 8bfb6684cb..043ad69f8b 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -58,22 +58,42 @@ S390CPU
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a proposal (three of them, actually) of auto propagation for
> local_err, to not call error_propagate on every exit point, when we
> deal with local_err.
>
> It also may help make Greg's series[1] about error_append_hint smaller.
>
> See definitions and examples below.
>
> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
> it, the idea will touch same code (and may be more).
>
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> include/qapi/error.h | 102 +++
> block.c | 63 --
> block/backup.c | 8 +++-
> block/gluster.c | 7 +++
> 4 files changed, 144 insertions(+), 36 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..083e061014 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
> ErrorClass err_class, const char *fmt, ...)
> GCC_FMT_ATTR(6, 7);
>
> +typedef struct ErrorPropagator {
> +Error **errp;
> +Error *local_err;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ErrorPropagationPair
> + *
> + * [Error *local_err, Error **errp]
> + *
> + * First element is local_err, second is original errp, which is propagation
> + * target. Yes, errp has a bit another type, so it should be converted.
> + *
> + * ErrorPropagationPair may be used as errp, which points to local_err,
> + * as it's type is compatible.
> + */
> +typedef Error *ErrorPropagationPair[2];
> +
> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr)
> +{
> +Error *local_err = (*arr)[0];
> +Error **errp = (Error **)(*arr)[1];
> +
> +if (local_err) {
> +error_propagate(errp, local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
> + error_propagation_pair_cleanup);
> +
> +/*
> + * DEF_AUTO_ERRP
> + *
> + * Define auto_errp variable, which may be used instead of errp, and
> + * *auto_errp may be safely checked to be zero or not, and may be safely
> + * used for error_append_hint(). auto_errp is automatically propagated
> + * to errp at function exit.
> + */
> +#define DEF_AUTO_ERRP(auto_errp, errp) \
> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
> +
> +
> +/*
> + * Another variant:
> + * Pros:
> + * - normal structure instead of cheating with array
> + * - we can directly use errp, if it's not NULL and don't point to
> + * error_abort or error_fatal
> + * Cons:
> + * - we need to define two variables instead of one
> + */
> +typedef struct ErrorPropagationStruct {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagationStruct;
> +
> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
> *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
> + error_propagation_struct_cleanup);
> +
> +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
> +Error **auto_errp = \
> +((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal)
> ? \
> +&__auto_errp_prop.local_err : \
> +(errp);
> +
> +/*
> + * Third variant:
> + * Pros:
> + * - simpler movement for functions which don't have local_err yet
> + * the only thing to do is to call one macro at function start.
> + * This extremely simplifies Greg's series
> + * Cons:
> + * - looks like errp shadowing.. Still seems safe.
> + * - must be after all definitions of local variables and before any
> + * code.
> + * - like v2, several statements in one open macro
> + */
> +#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; \
> +}
Using that idea, what about something like this:
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8bfb6684cb..043ad69f8b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -58,22 +58,42 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
}
+typedef struct ErrorPropagator {
+Error **errp;
+Error *local_err;
+}
Re: [Qemu-devel] [RFC] error: auto propagated local_err
18.09.2019 21:32, Eric Blake wrote:
> On 9/18/19 1:05 PM, Eric Blake wrote:
>
#define MAKE_ERRP_SAFE() \
g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \
errp = &__auto_errp_prop.local_err
>
> I tried to see if this could be done with just a single declaration
> line, as in:
>
> typedef struct ErrorPropagator {
> Error **errp;
> Error *local_err;
> } ErrorPropagator;
>
> #define MAKE_ERRP_SAFE() \
>g_auto(ErrorPropagator) __auto_errp_prop = { \
> errp, ((errp = &__auto_errp_prop.local_err), NULL) }
>
> But sadly, C17 paragraph 6.7.9P23 states:
>
> "The evaluations of the initialization list expressions are
> indeterminately sequenced with respect to one another and thus the order
> in which any side effects occur is unspecified."
>
> which does not bode well for the assignment to __auto_errp_prop.errp.
> All changes to errp would have to be within the same initializer. Maybe:
>
> #define MAKE_ERRP_SAFE() \
>g_auto(ErrorPropagator) __auto_errp_prop = { \
> .local_err = (__auto_errp_prop.err = errp, \
> (errp = &__auto_errp_prop.local_err), NULL) }
Is it guaranteed that .errp will not be initialized to NULL after evaluating of
.local_err initializer?
>
> but by the time you get that complicated, just using a statement is
> easier to read.
>
--
Best regards,
Vladimir
Re: [Qemu-devel] [RFC] error: auto propagated local_err
Patchew URL:
https://patchew.org/QEMU/20190918130244.24257-1-vsement...@virtuozzo.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [RFC] error: auto propagated local_err
Message-id: 20190918130244.24257-1-vsement...@virtuozzo.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
16e8874 error: auto propagated local_err
=== OUTPUT BEGIN ===
WARNING: line over 80 characters
#291: FILE: include/qapi/error.h:391:
+static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
*prop)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#301: FILE: include/qapi/error.h:401:
+#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
+g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
+Error **auto_errp = \
+((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) ?
\
+&__auto_errp_prop.local_err : \
+(errp);
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#320: FILE: include/qapi/error.h:420:
+#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; \
+}
total: 2 errors, 1 warnings, 277 lines checked
Commit 16e8874947e7 (error: auto propagated local_err) has style problems,
please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190918130244.24257-1-vsement...@virtuozzo.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 9/18/19 1:05 PM, Eric Blake wrote:
>>> #define MAKE_ERRP_SAFE() \
>>> g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \
>>> errp = &__auto_errp_prop.local_err
>>>
I tried to see if this could be done with just a single declaration
line, as in:
typedef struct ErrorPropagator {
Error **errp;
Error *local_err;
} ErrorPropagator;
#define MAKE_ERRP_SAFE() \
g_auto(ErrorPropagator) __auto_errp_prop = { \
errp, ((errp = &__auto_errp_prop.local_err), NULL) }
But sadly, C17 paragraph 6.7.9P23 states:
"The evaluations of the initialization list expressions are
indeterminately sequenced with respect to one another and thus the order
in which any side effects occur is unspecified."
which does not bode well for the assignment to __auto_errp_prop.errp.
All changes to errp would have to be within the same initializer. Maybe:
#define MAKE_ERRP_SAFE() \
g_auto(ErrorPropagator) __auto_errp_prop = { \
.local_err = (__auto_errp_prop.err = errp, \
(errp = &__auto_errp_prop.local_err), NULL) }
but by the time you get that complicated, just using a statement is
easier to read.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 9/18/19 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> +/*
>>> + * Third variant:
>>> + * Pros:
>>> + * - simpler movement for functions which don't have local_err yet
>>> + * the only thing to do is to call one macro at function start.
>>> + * This extremely simplifies Greg's series
>>> + * Cons:
>>> + * - looks like errp shadowing.. Still seems safe.
>>> + * - must be after all definitions of local variables and before any
>>> + * code.
>>
>> Why? I see no reason why it can't be hoisted earlier than other
>> declarations, and the only reason to not sink it after earlier code that
>> doesn't touch errp would be our coding standards that frowns on
>> declaration after code.
>
> Hmm, I thought compiler would warn about mixing code and definitions.
> Seems that gcc don't care, so it's OK.
C89 required all definitions before code, but that's historical.
Meanwhile, we require a compiler that supports C99 as well as at least
the __attribute__((cleanup)) extension (gcc and clang qualify, nothing
else really does, but no one has been complaining). And C99 requires
compiler support for intermixing definitions (in part because c++ did it
first, then gcc allowed that as an extension in C89); so it's been
permissible to intermix code and declarations for more than 20 years
now. The only reason we don't do it more is because of habits and
aesthetics, rather than necessity.
>>
>> This is actually quite cool.
>
> I glad to see that you like my favorite variant)
>
>> And if you get rid of your insistence that
>> it must occur after other variable declarations, you could instead
>> easily automate that any function that has a parameter 'Error **errp'
>> then has a MAKE_ERRP_SAFE(errp); as the first line of its function body
>> (that becomes something that you could grep for, rather than having to
>> use the smarts of coccinelle).
>>
>> Or if we want to enforce consistency on the parameter naming, even go with:
>>
>> #define MAKE_ERRP_SAFE() \
>> g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \
>> errp = &__auto_errp_prop.local_err
>>
>
> I am for
So now to wait for other comments.
>>
>>> + */
>>> +MAKE_ERRP_SAFE(errp);
>>> +
>>> if (filename) {
>>> ret = qemu_gluster_parse_uri(gconf, filename);
>>> if (ret < 0) {
>>>
>>
>> This is sweet - you can safely use '*errp' in the rest of the function,
>> without having to remember a second error name - while the caller can
>> still pass NULL or error_abort as desired.
>>
>> And I still think we can probably get Coccinelle to help make the
>> conversions, both of using this macro in any function that has an Error
>> **errp parameter, as well as getting rid of local_err declarations and
>> error_propagate() calls rendered redundant once this macro is used.
>>
>
> Thanks! And sorry for dirty draft.
It was titled RFC, after all :)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC] error: auto propagated local_err
18.09.2019 20:10, Eric Blake wrote:
> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is a proposal (three of them, actually) of auto propagation for
>> local_err, to not call error_propagate on every exit point, when we
>> deal with local_err.
>>
>> It also may help make Greg's series[1] about error_append_hint smaller.
>>
>> See definitions and examples below.
>>
>> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
>> it, the idea will touch same code (and may be more).
>
> It might have been nice to do a patch series of one proposal per patch,
> rather than cramming three in one, if only to see...
>
>>
>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> include/qapi/error.h | 102 +++
>> block.c | 63 --
>> block/backup.c | 8 +++-
>> block/gluster.c | 7 +++
>> 4 files changed, 144 insertions(+), 36 deletions(-)
>
> ...relative diffstat sizing differences between them.
>
> Of course, adding the framework requires growth in error.h. The real
> question, then, is how many lines do we save elsewhere by getting rid of
> boilerplate that is replaced by auto cleanup, and how many lines are
> mechanically churned to change variable names. And how much of that
> churn can itself be automated with Coccinelle.
>
> But I think the idea is worth pursuing!
>
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 3f95141a01..083e061014 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
>> ErrorClass err_class, const char *fmt, ...)
>> GCC_FMT_ATTR(6, 7);
>>
>> +typedef struct ErrorPropagator {
>> +Error **errp;
>> +Error *local_err;
>> +} ErrorPropagator;
>> +
>> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>> +{
>> +if (prop->local_err) {
>> +error_propagate(prop->errp, prop->local_err);
>> +}
>
> Technically, you don't need the 'if' here, since error_propogate() works
> just fine when prop->local_err is NULL.
>
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>> +
>
> Looks reasonable.
>
>> +/*
>> + * ErrorPropagationPair
>> + *
>> + * [Error *local_err, Error **errp]
>> + *
>> + * First element is local_err, second is original errp, which is propagation
>> + * target. Yes, errp has a bit another type, so it should be converted.
>> + *
>> + * ErrorPropagationPair may be used as errp, which points to local_err,
>> + * as it's type is compatible.
>
> s/it's/its/ ("it's" is only appropriate if "it is" can be substituted)
>
>> + */
>> +typedef Error *ErrorPropagationPair[2];
>> +
>> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr)
>> +{
>> +Error *local_err = (*arr)[0];
>> +Error **errp = (Error **)(*arr)[1];
>> +
>> +if (local_err) {
>> +error_propagate(errp, local_err);
>> +}
>> +}
>
> I don't like the type-punning.
>
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
>> + error_propagation_pair_cleanup);
>> +
>> +/*
>> + * DEF_AUTO_ERRP
>> + *
>> + * Define auto_errp variable, which may be used instead of errp, and
>> + * *auto_errp may be safely checked to be zero or not, and may be safely
>> + * used for error_append_hint(). auto_errp is automatically propagated
>> + * to errp at function exit.
>> + */
>> +#define DEF_AUTO_ERRP(auto_errp, errp) \
>> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
>
> Again, more type-punning. Ends up declaring the equivalent of 'Error
> *auto_errp[2]' which devolves to 'Error **auto_errp'.
>
> So, using this macro requires me to pass in the name of my local
> variable (which auto-propagates when it goes out of scope) and the errp
> parameter. Can we shortcut by declaring that the variable it propagates
> to MUST be named 'errp' rather than having that as a parameter name
> (doing so would force us to be consistent at using an Error **errp
> parameter).
>
>> +
>> +
>> +/*
>> + * Another variant:
>> + * Pros:
>> + * - normal structure instead of cheating with array
>> + * - we can directly use errp, if it's not NULL and don't point to
>> + * error_abort or error_fatal
>> + * Cons:
>> + * - we need to define two variables instead of one
>> + */
>> +typedef struct ErrorPropagationStruct {
>> +Error *local_err;
>> +Error **errp;
>> +} ErrorPropagationStruct;
>
> No real difference from ErrorPropagator above.
Sorry, I just forget to remove ErrorPropagator.
>
>> +
>> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
>> *prop)
>> +{
>> +if (prop->local_err) {
>> +error_propagate(prop->errp, prop->local_err);
>> +}
>
>
Re: [Qemu-devel] [RFC] error: auto propagated local_err
On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a proposal (three of them, actually) of auto propagation for
> local_err, to not call error_propagate on every exit point, when we
> deal with local_err.
>
> It also may help make Greg's series[1] about error_append_hint smaller.
>
> See definitions and examples below.
>
> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
> it, the idea will touch same code (and may be more).
It might have been nice to do a patch series of one proposal per patch,
rather than cramming three in one, if only to see...
>
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> include/qapi/error.h | 102 +++
> block.c | 63 --
> block/backup.c | 8 +++-
> block/gluster.c | 7 +++
> 4 files changed, 144 insertions(+), 36 deletions(-)
...relative diffstat sizing differences between them.
Of course, adding the framework requires growth in error.h. The real
question, then, is how many lines do we save elsewhere by getting rid of
boilerplate that is replaced by auto cleanup, and how many lines are
mechanically churned to change variable names. And how much of that
churn can itself be automated with Coccinelle.
But I think the idea is worth pursuing!
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..083e061014 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
> ErrorClass err_class, const char *fmt, ...)
> GCC_FMT_ATTR(6, 7);
>
> +typedef struct ErrorPropagator {
> +Error **errp;
> +Error *local_err;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
> +}
Technically, you don't need the 'if' here, since error_propogate() works
just fine when prop->local_err is NULL.
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
Looks reasonable.
> +/*
> + * ErrorPropagationPair
> + *
> + * [Error *local_err, Error **errp]
> + *
> + * First element is local_err, second is original errp, which is propagation
> + * target. Yes, errp has a bit another type, so it should be converted.
> + *
> + * ErrorPropagationPair may be used as errp, which points to local_err,
> + * as it's type is compatible.
s/it's/its/ ("it's" is only appropriate if "it is" can be substituted)
> + */
> +typedef Error *ErrorPropagationPair[2];
> +
> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr)
> +{
> +Error *local_err = (*arr)[0];
> +Error **errp = (Error **)(*arr)[1];
> +
> +if (local_err) {
> +error_propagate(errp, local_err);
> +}
> +}
I don't like the type-punning.
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
> + error_propagation_pair_cleanup);
> +
> +/*
> + * DEF_AUTO_ERRP
> + *
> + * Define auto_errp variable, which may be used instead of errp, and
> + * *auto_errp may be safely checked to be zero or not, and may be safely
> + * used for error_append_hint(). auto_errp is automatically propagated
> + * to errp at function exit.
> + */
> +#define DEF_AUTO_ERRP(auto_errp, errp) \
> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
Again, more type-punning. Ends up declaring the equivalent of 'Error
*auto_errp[2]' which devolves to 'Error **auto_errp'.
So, using this macro requires me to pass in the name of my local
variable (which auto-propagates when it goes out of scope) and the errp
parameter. Can we shortcut by declaring that the variable it propagates
to MUST be named 'errp' rather than having that as a parameter name
(doing so would force us to be consistent at using an Error **errp
parameter).
> +
> +
> +/*
> + * Another variant:
> + * Pros:
> + * - normal structure instead of cheating with array
> + * - we can directly use errp, if it's not NULL and don't point to
> + * error_abort or error_fatal
> + * Cons:
> + * - we need to define two variables instead of one
> + */
> +typedef struct ErrorPropagationStruct {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagationStruct;
No real difference from ErrorPropagator above.
> +
> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
> *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
> +}
Another useless if.
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
> + error_propagation_struct_cleanup);
> +
> +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp =
[Qemu-devel] [RFC] error: auto propagated local_err
Hi all!
Here is a proposal (three of them, actually) of auto propagation for
local_err, to not call error_propagate on every exit point, when we
deal with local_err.
It also may help make Greg's series[1] about error_append_hint smaller.
See definitions and examples below.
I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
it, the idea will touch same code (and may be more).
[1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/qapi/error.h | 102 +++
block.c | 63 --
block/backup.c | 8 +++-
block/gluster.c | 7 +++
4 files changed, 144 insertions(+), 36 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3f95141a01..083e061014 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
ErrorClass err_class, const char *fmt, ...)
GCC_FMT_ATTR(6, 7);
+typedef struct ErrorPropagator {
+Error **errp;
+Error *local_err;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+if (prop->local_err) {
+error_propagate(prop->errp, prop->local_err);
+}
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
+/*
+ * ErrorPropagationPair
+ *
+ * [Error *local_err, Error **errp]
+ *
+ * First element is local_err, second is original errp, which is propagation
+ * target. Yes, errp has a bit another type, so it should be converted.
+ *
+ * ErrorPropagationPair may be used as errp, which points to local_err,
+ * as it's type is compatible.
+ */
+typedef Error *ErrorPropagationPair[2];
+
+static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr)
+{
+Error *local_err = (*arr)[0];
+Error **errp = (Error **)(*arr)[1];
+
+if (local_err) {
+error_propagate(errp, local_err);
+}
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
+ error_propagation_pair_cleanup);
+
+/*
+ * DEF_AUTO_ERRP
+ *
+ * Define auto_errp variable, which may be used instead of errp, and
+ * *auto_errp may be safely checked to be zero or not, and may be safely
+ * used for error_append_hint(). auto_errp is automatically propagated
+ * to errp at function exit.
+ */
+#define DEF_AUTO_ERRP(auto_errp, errp) \
+g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
+
+
+/*
+ * Another variant:
+ * Pros:
+ * - normal structure instead of cheating with array
+ * - we can directly use errp, if it's not NULL and don't point to
+ * error_abort or error_fatal
+ * Cons:
+ * - we need to define two variables instead of one
+ */
+typedef struct ErrorPropagationStruct {
+Error *local_err;
+Error **errp;
+} ErrorPropagationStruct;
+
+static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
*prop)
+{
+if (prop->local_err) {
+error_propagate(prop->errp, prop->local_err);
+}
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
+ error_propagation_struct_cleanup);
+
+#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
+g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
+Error **auto_errp = \
+((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) ?
\
+&__auto_errp_prop.local_err : \
+(errp);
+
+/*
+ * Third variant:
+ * Pros:
+ * - simpler movement for functions which don't have local_err yet
+ * the only thing to do is to call one macro at function start.
+ * This extremely simplifies Greg's series
+ * Cons:
+ * - looks like errp shadowing.. Still seems safe.
+ * - must be after all definitions of local variables and before any
+ * code.
+ * - like v2, several statements in one open macro
+ */
+#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; \
+}
+
+
/*
* Special error destination to abort on error.
* See error_setg() and error_propagate() for details.
diff --git a/block.c b/block.c
index 5944124845..5253663329 100644
--- a/block.c
+++ b/block.c
@@ -1275,12 +1275,11 @@ static int bdrv_open_driver(BlockDriverState *bs,
BlockDriver *drv,
const char *node_name, QDict *options,
int open_flags, Error **errp)
{
-Error *local_err = NULL;
+DEF_AUTO_ERRP_V2(auto_errp, errp);
int i, ret;
-bdrv_assign_node_name(bs, node_name, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+bdrv_assign_node_name(bs, node_name, auto_errp);
+if (*auto_errp) {
return -EINVAL;
}
@@ -1290,20 +1289,21 @@
