Re: [RFC v5 024/126] error: auto propagated local_err
Vladimir Sementsov-Ogievskiy writes:
> 05.12.2019 17:58, Vladimir Sementsov-Ogievskiy wrote:
>> 05.12.2019 15:36, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy writes:
>>>
04.12.2019 17:59, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy writes:
>
>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>
>> 2. Fix issue with error_abort & error_propagate: when we wrap
>> error_abort by local_err+error_propagate, resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>
> I get what you mean, but I have plenty of context.
>
>> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>> local_err+error_propagate pattern, which will definitely fix the issue)
>
> The parenthesis is not part of the goal.
>
>> [Reported by Kevin Wolf]
>>
>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>> void functions with errp parameter, when caller wants to know resulting
>> status. (Note: actually these functions could be merely updated to
>> return int error code).
>>
>> To achieve these goals, we need to add invocation of the macro at start
>> of functions, which needs error_prepend/error_append_hint (1.); add
>> invocation of the macro at start of functions which do
>> local_err+error_propagate scenario the check errors, drop local errors
>> from them and just use *errp instead (2., 3.).
>
> The paragraph talks about two cases: 1. and 2.+3.
Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
fix achieve 2 and 3 by one action.
> Makes me think we
> want two paragraphs, each illustrated with an example.
>
> What about you provide the examples, and then I try to polish the prose?
1: error_fatal problem
Assume the following code flow:
int f1(errp) {
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
Now, if we call f1 with &error_fatal argument and f2 fails, the program
will exit immediately inside f2, when setting the errp. User will not
see the hint.
So, in this case we should use local_err.
>>>
>>> How does this example look after the transformation?
>>
>> Good point.
>>
>> int f1(errp) {
>> ERRP_AUTO_PROPAGATE();
>> ...
>> ret = f2(errp);
>> if (ret < 0) {
>> error_append_hint(errp, "very useful hint");
>> return ret;
>> }
>> ...
>> }
>>
>> - nothing changed, only add macro at start. But now errp is safe, if it was
>> error_fatal it is wrapped by local error, and will only call exit on
>> automatic
>> propagation on f1 finish.
>>
>>>
2: error_abort problem
Now, consider functions without return value. We normally use local_err
variable to catch failures:
void f1(errp) {
Error *local_err = NULL;
...
f2(&local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
...
}
Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
crash dump will point to error_propagate, not to the failure point in f2,
which complicates debugging.
So, we should never wrap error_abort by local_err.
>>>
>>> Likewise.
>>
>> And here:
>>
>> void f1(errp) {
>> ERRP_AUTO_PROPAGATE();
>> ...
>> f2(errp);
>> if (*errp) {
>> return;
>> }
>> ...
>>
>> - if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
>> local error is automatically propagated to original one.
>
> and if it was error_abort, it is not wrapped, so will crash where failed.
We still need to avoid passing on unwrapped @errp when we want to ignore
some errors, as in fit_load_fdt(), but that should be quite rare.
Doesn't invalidate your approach.
[...]
Re: [RFC v5 024/126] error: auto propagated local_err
On 12/5/19 8:58 AM, Vladimir Sementsov-Ogievskiy wrote:
What about you provide the examples, and then I try to polish the prose?
1: error_fatal problem
Assume the following code flow:
int f1(errp) {
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
Now, if we call f1 with &error_fatal argument and f2 fails, the program
will exit immediately inside f2, when setting the errp. User will not
see the hint.
So, in this case we should use local_err.
How does this example look after the transformation?
Without ERRP_AUTO_PROPAGATE(), the transformation is a lot of boilerplate:
int f1(errp) {
Error *err = NULL;
ret = f2(&err);
if (ret < 0) {
error_append_hint(&err, "very useful hint");
error_propagate(errp, err);
return ret;
}
}
what's worse, that boilerplate to solve problem 1 turns out to be...
Good point.
int f1(errp) {
ERRP_AUTO_PROPAGATE();
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
- nothing changed, only add macro at start. But now errp is safe, if it was
error_fatal it is wrapped by local error, and will only call exit on automatic
propagation on f1 finish.
2: error_abort problem
Now, consider functions without return value. We normally use local_err
variable to catch failures:
void f1(errp) {
Error *local_err = NULL;
...
f2(&local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
...
}
the very same code as the cause of problem 2.
Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
crash dump will point to error_propagate, not to the failure point in f2,
which complicates debugging.
So, we should never wrap error_abort by local_err.
Likewise.
And here:
void f1(errp) {
ERRP_AUTO_PROPAGATE();
...
f2(errp);
if (*errp) {
return;
}
...
- if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
local error is automatically propagated to original one.
So, the use of ERRP_AUTO_PROPAGATE() solves BOTH problems 1 and 2 - we
avoid the boilerplate that trades one problem for another, by
consolidating ALL of the boilerplate into a single-line macro, such that
error_propagate() no longer needs to be called anywhere except inside
the ERRP_AUTO_PROPAGATE macro.
===
Our solution:
- Fixes [1.], adding invocation of new macro into functions with
error_appen_hint/error_prepend,
New macro will wrap error_fatal.
- Fixes [2.], by switching from hand-written local_err to smart macro, which
never
wraps error_abort.
- Handles [3.], by switching to macro, which is less code
- Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
propagations
(in fact, error_propagate is called, but returns immediately on first if
(!local_err))
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [RFC v5 024/126] error: auto propagated local_err
05.12.2019 17:58, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2019 15:36, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy writes:
>>
>>> 04.12.2019 17:59, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy writes:
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
I get what you mean, but I have plenty of context.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
The parenthesis is not part of the goal.
> [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).
The paragraph talks about two cases: 1. and 2.+3.
>>>
>>> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
>>> fix achieve 2 and 3 by one action.
>>>
Makes me think we
want two paragraphs, each illustrated with an example.
What about you provide the examples, and then I try to polish the prose?
>>>
>>> 1: error_fatal problem
>>>
>>> Assume the following code flow:
>>>
>>> int f1(errp) {
>>> ...
>>> ret = f2(errp);
>>> if (ret < 0) {
>>> error_append_hint(errp, "very useful hint");
>>> return ret;
>>> }
>>> ...
>>> }
>>>
>>> Now, if we call f1 with &error_fatal argument and f2 fails, the program
>>> will exit immediately inside f2, when setting the errp. User will not
>>> see the hint.
>>>
>>> So, in this case we should use local_err.
>>
>> How does this example look after the transformation?
>
> Good point.
>
> int f1(errp) {
> ERRP_AUTO_PROPAGATE();
> ...
> ret = f2(errp);
> if (ret < 0) {
> error_append_hint(errp, "very useful hint");
> return ret;
> }
> ...
> }
>
> - nothing changed, only add macro at start. But now errp is safe, if it was
> error_fatal it is wrapped by local error, and will only call exit on automatic
> propagation on f1 finish.
>
>>
>>> 2: error_abort problem
>>>
>>> Now, consider functions without return value. We normally use local_err
>>> variable to catch failures:
>>>
>>> void f1(errp) {
>>> Error *local_err = NULL;
>>> ...
>>> f2(&local_err);
>>> if (local_err) {
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>> ...
>>> }
>>>
>>> Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
>>> crash dump will point to error_propagate, not to the failure point in f2,
>>> which complicates debugging.
>>>
>>> So, we should never wrap error_abort by local_err.
>>
>> Likewise.
>
> And here:
>
> void f1(errp) {
> ERRP_AUTO_PROPAGATE();
> ...
> f2(errp);
> if (*errp) {
> return;
> }
> ...
>
> - if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
> local error is automatically propagated to original one.
and if it was error_abort, it is not wrapped, so will crash where failed.
>
>>
>>>
>>> ===
>>>
>>> Our solution:
>>>
>>> - Fixes [1.], adding invocation of new macro into functions with
>>> error_appen_hint/error_prepend,
>>> New macro will wrap error_fatal.
>>> - Fixes [2.], by switching from hand-written local_err to smart macro,
>>> which never
>>> wraps error_abort.
>>> - Handles [3.], by switching to macro, which is less code
>>> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
>>> propagations
>>> (in fact, error_propagate is called, but returns immediately on first
>>> if (!local_err))
>>
>
>
--
Best regards,
Vladimir
Re: [RFC v5 024/126] error: auto propagated local_err
05.12.2019 15:36, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy writes:
>
>> 04.12.2019 17:59, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy writes:
>>>
Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with errp OUT parameter.
It has three goals:
1. Fix issue with error_fatal & error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]
2. Fix issue with error_abort & error_propagate: when we wrap
error_abort by local_err+error_propagate, resulting coredump will
refer to error_propagate and not to the place where error happened.
>>>
>>> I get what you mean, but I have plenty of context.
>>>
(the macro itself doesn't fix the issue, but it allows to [3.] drop all
local_err+error_propagate pattern, which will definitely fix the issue)
>>>
>>> The parenthesis is not part of the goal.
>>>
[Reported by Kevin Wolf]
3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).
To achieve these goals, we need to add invocation of the macro at start
of functions, which needs error_prepend/error_append_hint (1.); add
invocation of the macro at start of functions which do
local_err+error_propagate scenario the check errors, drop local errors
from them and just use *errp instead (2., 3.).
>>>
>>> The paragraph talks about two cases: 1. and 2.+3.
>>
>> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
>> fix achieve 2 and 3 by one action.
>>
>>> Makes me think we
>>> want two paragraphs, each illustrated with an example.
>>>
>>> What about you provide the examples, and then I try to polish the prose?
>>
>> 1: error_fatal problem
>>
>> Assume the following code flow:
>>
>> int f1(errp) {
>> ...
>> ret = f2(errp);
>> if (ret < 0) {
>> error_append_hint(errp, "very useful hint");
>> return ret;
>> }
>> ...
>> }
>>
>> Now, if we call f1 with &error_fatal argument and f2 fails, the program
>> will exit immediately inside f2, when setting the errp. User will not
>> see the hint.
>>
>> So, in this case we should use local_err.
>
> How does this example look after the transformation?
Good point.
int f1(errp) {
ERRP_AUTO_PROPAGATE();
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
- nothing changed, only add macro at start. But now errp is safe, if it was
error_fatal it is wrapped by local error, and will only call exit on automatic
propagation on f1 finish.
>
>> 2: error_abort problem
>>
>> Now, consider functions without return value. We normally use local_err
>> variable to catch failures:
>>
>> void f1(errp) {
>> Error *local_err = NULL;
>> ...
>> f2(&local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>> }
>> ...
>> }
>>
>> Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
>> crash dump will point to error_propagate, not to the failure point in f2,
>> which complicates debugging.
>>
>> So, we should never wrap error_abort by local_err.
>
> Likewise.
And here:
void f1(errp) {
ERRP_AUTO_PROPAGATE();
...
f2(errp);
if (*errp) {
return;
}
...
- if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
local error is automatically propagated to original one.
>
>>
>> ===
>>
>> Our solution:
>>
>> - Fixes [1.], adding invocation of new macro into functions with
>> error_appen_hint/error_prepend,
>> New macro will wrap error_fatal.
>> - Fixes [2.], by switching from hand-written local_err to smart macro, which
>> never
>> wraps error_abort.
>> - Handles [3.], by switching to macro, which is less code
>> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
>> propagations
>> (in fact, error_propagate is called, but returns immediately on first if
>> (!local_err))
>
--
Best regards,
Vladimir
Re: [RFC v5 024/126] error: auto propagated local_err
Vladimir Sementsov-Ogievskiy writes:
> 04.12.2019 17:59, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy writes:
>>
>>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>>> functions with errp OUT parameter.
>>>
>>> It has three goals:
>>>
>>> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>>> can't see this additional information, because exit() happens in
>>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>>
>>> 2. Fix issue with error_abort & error_propagate: when we wrap
>>> error_abort by local_err+error_propagate, resulting coredump will
>>> refer to error_propagate and not to the place where error happened.
>>
>> I get what you mean, but I have plenty of context.
>>
>>> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>>> local_err+error_propagate pattern, which will definitely fix the issue)
>>
>> The parenthesis is not part of the goal.
>>
>>> [Reported by Kevin Wolf]
>>>
>>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>>> void functions with errp parameter, when caller wants to know resulting
>>> status. (Note: actually these functions could be merely updated to
>>> return int error code).
>>>
>>> To achieve these goals, we need to add invocation of the macro at start
>>> of functions, which needs error_prepend/error_append_hint (1.); add
>>> invocation of the macro at start of functions which do
>>> local_err+error_propagate scenario the check errors, drop local errors
>>> from them and just use *errp instead (2., 3.).
>>
>> The paragraph talks about two cases: 1. and 2.+3.
>
> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
> fix achieve 2 and 3 by one action.
>
>> Makes me think we
>> want two paragraphs, each illustrated with an example.
>>
>> What about you provide the examples, and then I try to polish the prose?
>
> 1: error_fatal problem
>
> Assume the following code flow:
>
> int f1(errp) {
> ...
> ret = f2(errp);
> if (ret < 0) {
>error_append_hint(errp, "very useful hint");
>return ret;
> }
> ...
> }
>
> Now, if we call f1 with &error_fatal argument and f2 fails, the program
> will exit immediately inside f2, when setting the errp. User will not
> see the hint.
>
> So, in this case we should use local_err.
How does this example look after the transformation?
> 2: error_abort problem
>
> Now, consider functions without return value. We normally use local_err
> variable to catch failures:
>
> void f1(errp) {
> Error *local_err = NULL;
> ...
> f2(&local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
> ...
> }
>
> Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
> crash dump will point to error_propagate, not to the failure point in f2,
> which complicates debugging.
>
> So, we should never wrap error_abort by local_err.
Likewise.
>
> ===
>
> Our solution:
>
> - Fixes [1.], adding invocation of new macro into functions with
> error_appen_hint/error_prepend,
>New macro will wrap error_fatal.
> - Fixes [2.], by switching from hand-written local_err to smart macro, which
> never
>wraps error_abort.
> - Handles [3.], by switching to macro, which is less code
> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
> propagations
>(in fact, error_propagate is called, but returns immediately on first if
> (!local_err))
Re: [RFC v5 024/126] error: auto propagated local_err
04.12.2019 17:59, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy writes:
>
>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>
>> 2. Fix issue with error_abort & error_propagate: when we wrap
>> error_abort by local_err+error_propagate, resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>
> I get what you mean, but I have plenty of context.
>
>> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>> local_err+error_propagate pattern, which will definitely fix the issue)
>
> The parenthesis is not part of the goal.
>
>> [Reported by Kevin Wolf]
>>
>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>> void functions with errp parameter, when caller wants to know resulting
>> status. (Note: actually these functions could be merely updated to
>> return int error code).
>>
>> To achieve these goals, we need to add invocation of the macro at start
>> of functions, which needs error_prepend/error_append_hint (1.); add
>> invocation of the macro at start of functions which do
>> local_err+error_propagate scenario the check errors, drop local errors
>> from them and just use *errp instead (2., 3.).
>
> The paragraph talks about two cases: 1. and 2.+3.
Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
fix achieve 2 and 3 by one action.
> Makes me think we
> want two paragraphs, each illustrated with an example.
>
> What about you provide the examples, and then I try to polish the prose?
1: error_fatal problem
Assume the following code flow:
int f1(errp) {
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
Now, if we call f1 with &error_fatal argument and f2 fails, the program
will exit immediately inside f2, when setting the errp. User will not
see the hint.
So, in this case we should use local_err.
2: error_abort problem
Now, consider functions without return value. We normally use local_err
variable to catch failures:
void f1(errp) {
Error *local_err = NULL;
...
f2(&local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
...
}
Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
crash dump will point to error_propagate, not to the failure point in f2,
which complicates debugging.
So, we should never wrap error_abort by local_err.
===
Our solution:
- Fixes [1.], adding invocation of new macro into functions with
error_appen_hint/error_prepend,
New macro will wrap error_fatal.
- Fixes [2.], by switching from hand-written local_err to smart macro, which
never
wraps error_abort.
- Handles [3.], by switching to macro, which is less code
- Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
propagations
(in fact, error_propagate is called, but returns immediately on first if
(!local_err))
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> Reviewed-by: Eric Blake
>> ---
>>
>> CC: Gerd Hoffmann
>> CC: "Gonglei (Arei)"
>> CC: Eduardo Habkost
>> CC: Igor Mammedov
>> CC: Laurent Vivier
>> CC: Amit Shah
>> CC: Kevin Wolf
>> CC: Max Reitz
>> CC: John Snow
>> CC: Ari Sundholm
>> CC: Pavel Dovgalyuk
>> CC: Paolo Bonzini
>> CC: Stefan Hajnoczi
>> CC: Fam Zheng
>> CC: Stefan Weil
>> CC: Ronnie Sahlberg
>> CC: Peter Lieven
>> CC: Eric Blake
>> CC: "Denis V. Lunev"
>> CC: Markus Armbruster
>> CC: Alberto Garcia
>> CC: Jason Dillaman
>> CC: Wen Congyang
>> CC: Xie Changlong
>> CC: Liu Yuan
>> CC: "Richard W.M. Jones"
>> CC: Jeff Cody
>> CC: "Marc-André Lureau"
>> CC: "Daniel P. Berrangé"
>> CC: Richard Henderson
>> CC: Greg Kurz
>> CC: "Michael S. Tsirkin"
>> CC: Marcel Apfelbaum
>> CC: Beniamino Galvani
>> CC: Peter Maydell
>> CC: "Cédric Le Goater"
>> CC: Andrew Jeffery
>> CC: Joel Stanley
>> CC: Andrew Baumann
>> CC: "Philippe Mathieu-Daudé"
>> CC: Antony Pavlov
>> CC: Jean-Christophe Dubois
>> CC: Peter Chubb
>> CC: Subbaraya Sundeep
>> CC: Eric Auger
>> CC: Alistair Francis
>> CC: "Edgar E. Iglesias"
>> CC: Stefano Stabellini
>> CC: Anthony Perard
>> CC: Paul Durrant
>> CC: Paul Burton
>> CC: Aleksandar Rikalo
>> CC: Chris Wulff
>> CC: Marek Vasut
>> CC: David Gibson
>> CC: Cornelia Huck
>> CC: Halil Pasic
>> CC: Christian Borntraeger
>> CC: "Hervé Poussineau"
>> CC: Xiao Guangrong
>> CC: Aurelien Jarno
>> CC: Aleksandar Markovic
>> CC: Mark Cave-Ayland
>> CC: Jason Wang
>> CC: Laszlo Ersek
>> CC: Yuval Shaia
>> CC: Palmer Dabbelt
>> CC: Sagar Karandikar
>> CC: Bastian Koppelmann
>> CC: David Hildenbrand
>> CC: Thomas Huth
>
Re: [RFC v5 024/126] error: auto propagated local_err
Vladimir Sementsov-Ogievskiy writes:
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
I get what you mean, but I have plenty of context.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
The parenthesis is not part of the goal.
> [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).
The paragraph talks about two cases: 1. and 2.+3. Makes me think we
want two paragraphs, each illustrated with an example.
What about you provide the examples, and then I try to polish the prose?
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> Reviewed-by: Eric Blake
> ---
>
> CC: Gerd Hoffmann
> CC: "Gonglei (Arei)"
> CC: Eduardo Habkost
> CC: Igor Mammedov
> CC: Laurent Vivier
> CC: Amit Shah
> CC: Kevin Wolf
> CC: Max Reitz
> CC: John Snow
> CC: Ari Sundholm
> CC: Pavel Dovgalyuk
> CC: Paolo Bonzini
> CC: Stefan Hajnoczi
> CC: Fam Zheng
> CC: Stefan Weil
> CC: Ronnie Sahlberg
> CC: Peter Lieven
> CC: Eric Blake
> CC: "Denis V. Lunev"
> CC: Markus Armbruster
> CC: Alberto Garcia
> CC: Jason Dillaman
> CC: Wen Congyang
> CC: Xie Changlong
> CC: Liu Yuan
> CC: "Richard W.M. Jones"
> CC: Jeff Cody
> CC: "Marc-André Lureau"
> CC: "Daniel P. Berrangé"
> CC: Richard Henderson
> CC: Greg Kurz
> CC: "Michael S. Tsirkin"
> CC: Marcel Apfelbaum
> CC: Beniamino Galvani
> CC: Peter Maydell
> CC: "Cédric Le Goater"
> CC: Andrew Jeffery
> CC: Joel Stanley
> CC: Andrew Baumann
> CC: "Philippe Mathieu-Daudé"
> CC: Antony Pavlov
> CC: Jean-Christophe Dubois
> CC: Peter Chubb
> CC: Subbaraya Sundeep
> CC: Eric Auger
> CC: Alistair Francis
> CC: "Edgar E. Iglesias"
> CC: Stefano Stabellini
> CC: Anthony Perard
> CC: Paul Durrant
> CC: Paul Burton
> CC: Aleksandar Rikalo
> CC: Chris Wulff
> CC: Marek Vasut
> CC: David Gibson
> CC: Cornelia Huck
> CC: Halil Pasic
> CC: Christian Borntraeger
> CC: "Hervé Poussineau"
> CC: Xiao Guangrong
> CC: Aurelien Jarno
> CC: Aleksandar Markovic
> CC: Mark Cave-Ayland
> CC: Jason Wang
> CC: Laszlo Ersek
> CC: Yuval Shaia
> CC: Palmer Dabbelt
> CC: Sagar Karandikar
> CC: Bastian Koppelmann
> CC: David Hildenbrand
> CC: Thomas Huth
> CC: Eric Farman
> CC: Matthew Rosato
> CC: Hannes Reinecke
> CC: Michael Walle
> CC: Artyom Tarasenko
> CC: Stefan Berger
> CC: Samuel Thibault
> CC: Alex Williamson
> CC: Tony Krowiak
> CC: Pierre Morel
> CC: Michael Roth
> CC: Hailiang Zhang
> CC: Juan Quintela
> CC: "Dr. David Alan Gilbert"
> CC: Luigi Rizzo
> CC: Giuseppe Lettieri
> CC: Vincenzo Maffione
> CC: Jan Kiszka
> CC: Anthony Green
> CC: Stafford Horne
> CC: Guan Xuetao
> CC: Max Filippov
> CC: qemu-bl...@nongnu.org
> CC: integrat...@gluster.org
> CC: sheep...@lists.wpkg.org
> CC: qemu-...@nongnu.org
> CC: xen-de...@lists.xenproject.org
> CC: qemu-...@nongnu.org
> CC: qemu-s3...@nongnu.org
> CC: qemu-ri...@nongnu.org
>
> include/qapi/error.h | 38 ++
> 1 file changed, 38 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index d6898d833b..47238d9065 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -345,6 +345,44 @@ void error_set_internal(Error **errp,
> ErrorClass err_class, const char *fmt, ...)
> GCC_FMT_ATTR(6, 7);
>
> +typedef struct ErrorPropagator {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function with Error **errp
> + * OUT parameter. It's needed only in cases where we want to use
> error_prepend,
> + * error_append_hint or dereference *err
Re: [RFC v5 024/126] error: auto propagated local_err
On 11/8/19 3:10 PM, Marc-André Lureau wrote: +/* + * ERRP_AUTO_PROPAGATE + * + * This macro is created to be the first line of a function with Error **errp + * OUT parameter. It's needed only in cases where we want to use error_prepend, + * error_append_hint or dereference *errp. It's still safe (but useless) in + * other cases. + * + * If errp is NULL or points to error_fatal, it is rewritten to point to a + * local Error object, which will be automatically propagated to the original + * errp on function exit (see error_propagator_cleanup). + * + * After invocation of this macro it is always safe to dereference errp + * (as it's not NULL anymore) and to add information (by error_prepend or + * error_append_hint) + * (as, if it was error_fatal, we swapped it with a local_error to be + * propagated on cleanup). Nice improvements. Minor drawback, the abort()/exit() will now take place when going out of scope and running the cleanup instead of error location. Not a big problem I guess. Your assessment is not quite right: Any abort() will happen at the leaf node (because we are no longer wrapping thing into a local err and skipping error_propagate altogether for &error_abort). You are correct that any exit() will now happen during cleanup, but that is an undetectable change (there is no stack trace present for &error_fatal, so calling error_propagate at a later point in time does not affect the observable end behavior). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [RFC v5 024/126] error: auto propagated local_err
On Fri, Oct 11, 2019 at 10:11 PM Vladimir Sementsov-Ogievskiy
wrote:
>
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> Reviewed-by: Eric Blake
> ---
>
> CC: Gerd Hoffmann
> CC: "Gonglei (Arei)"
> CC: Eduardo Habkost
> CC: Igor Mammedov
> CC: Laurent Vivier
> CC: Amit Shah
> CC: Kevin Wolf
> CC: Max Reitz
> CC: John Snow
> CC: Ari Sundholm
> CC: Pavel Dovgalyuk
> CC: Paolo Bonzini
> CC: Stefan Hajnoczi
> CC: Fam Zheng
> CC: Stefan Weil
> CC: Ronnie Sahlberg
> CC: Peter Lieven
> CC: Eric Blake
> CC: "Denis V. Lunev"
> CC: Markus Armbruster
> CC: Alberto Garcia
> CC: Jason Dillaman
> CC: Wen Congyang
> CC: Xie Changlong
> CC: Liu Yuan
> CC: "Richard W.M. Jones"
> CC: Jeff Cody
> CC: "Marc-André Lureau"
> CC: "Daniel P. Berrangé"
> CC: Richard Henderson
> CC: Greg Kurz
> CC: "Michael S. Tsirkin"
> CC: Marcel Apfelbaum
> CC: Beniamino Galvani
> CC: Peter Maydell
> CC: "Cédric Le Goater"
> CC: Andrew Jeffery
> CC: Joel Stanley
> CC: Andrew Baumann
> CC: "Philippe Mathieu-Daudé"
> CC: Antony Pavlov
> CC: Jean-Christophe Dubois
> CC: Peter Chubb
> CC: Subbaraya Sundeep
> CC: Eric Auger
> CC: Alistair Francis
> CC: "Edgar E. Iglesias"
> CC: Stefano Stabellini
> CC: Anthony Perard
> CC: Paul Durrant
> CC: Paul Burton
> CC: Aleksandar Rikalo
> CC: Chris Wulff
> CC: Marek Vasut
> CC: David Gibson
> CC: Cornelia Huck
> CC: Halil Pasic
> CC: Christian Borntraeger
> CC: "Hervé Poussineau"
> CC: Xiao Guangrong
> CC: Aurelien Jarno
> CC: Aleksandar Markovic
> CC: Mark Cave-Ayland
> CC: Jason Wang
> CC: Laszlo Ersek
> CC: Yuval Shaia
> CC: Palmer Dabbelt
> CC: Sagar Karandikar
> CC: Bastian Koppelmann
> CC: David Hildenbrand
> CC: Thomas Huth
> CC: Eric Farman
> CC: Matthew Rosato
> CC: Hannes Reinecke
> CC: Michael Walle
> CC: Artyom Tarasenko
> CC: Stefan Berger
> CC: Samuel Thibault
> CC: Alex Williamson
> CC: Tony Krowiak
> CC: Pierre Morel
> CC: Michael Roth
> CC: Hailiang Zhang
> CC: Juan Quintela
> CC: "Dr. David Alan Gilbert"
> CC: Luigi Rizzo
> CC: Giuseppe Lettieri
> CC: Vincenzo Maffione
> CC: Jan Kiszka
> CC: Anthony Green
> CC: Stafford Horne
> CC: Guan Xuetao
> CC: Max Filippov
> CC: qemu-bl...@nongnu.org
> CC: integrat...@gluster.org
> CC: sheep...@lists.wpkg.org
> CC: qemu-...@nongnu.org
> CC: xen-de...@lists.xenproject.org
> CC: qemu-...@nongnu.org
> CC: qemu-s3...@nongnu.org
> CC: qemu-ri...@nongnu.org
>
> include/qapi/error.h | 38 ++
> 1 file changed, 38 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index d6898d833b..47238d9065 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -345,6 +345,44 @@ void error_set_internal(Error **errp,
> ErrorClass err_class, const char *fmt, ...)
> GCC_FMT_ATTR(6, 7);
>
> +typedef struct ErrorPropagator {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function with Error **errp
> + * OUT parameter. It's needed only in cases where we want to use
> error_prepend,
> + * error_append_hint or dereference *errp. It's still safe (but useless) in
> + * other cases.
> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> + * local Error object, which will be automatically propagated to the original
> + * errp on function exit (see error_prop
