[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Error ** parameter conventions (was: [PATCH] qemu-sockets:
From: |
Markus Armbruster |
Subject: |
[Qemu-devel] Error ** parameter conventions (was: [PATCH] qemu-sockets: Fix assertion failure) |
Date: |
Wed, 06 Mar 2013 16:05:13 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
[Note cc: Luiz]
Kevin Wolf <address@hidden> writes:
> inet_connect_opts() tries all possible addrinfos returned by
> getaddrinfo(). If one fails with an error, the next one is tried. In
> this case, the Error should be discarded because the whole operation is
> successful if another addrinfo from the list succeeds; and if it
> doesn't, setting an already set Error will trigger an assertion failure.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> util/qemu-sockets.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 1350ccc..32e609a 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
> }
>
> for (e = res; e != NULL; e = e->ai_next) {
> +
> + /* Overwriting errors isn't allowed, so clear any error that may have
> + * occured in the previous iteration */
> + if (error_is_set(errp)) {
> + error_free(*errp);
> + *errp = NULL;
> + }
> +
> if (connect_state != NULL) {
> connect_state->current_addr = e;
> }
I'm not sure this is the proper solution, but that could be just my
uncertainty on proper Error usage.
The convention as I understand it is that a function stores whatever
error it wants to return through its errp parameter, with
error_set(errp, ...). error_set() does nothing when passed a null errp.
This lets callers ignore errors without fuss.
I guess we don't want callers to pass a non-null errp with *errp != NULL
ever, but I don't think that's spelled out anywhere. I suspect code is
generally unprepared for such usage.
Your patch *clears* errors through its errp parameter. I'm not saying
it's wrong, I just spooks poor ignorant me.
My gut feeling: as soon as we go beyond utterly trivial with errors,
it's best to put them in local variables, and error_propagate() to the
parameter only at the end.
Let's review some usage patterns, in the hope of learning more.
/*
* Do something on @arg.
* On error, set address@hidden if @errp isn't null.
*/
void do_something(Argument *arg, Error **errp);
// ignoring errors:
do_something(arg, NULL);
// checking and handling errors locally:
Error *local_err = NULL;
do_something(arg, &local_err);
if (local_err) { // some prefer error_is_set(local_err) here,
// but I think that only muddies the waters
// error handling goes here
error_free(local_err);
}
// propagating to caller via parameter Error *errp:
Error *local_err = NULL;
do_something(arg, &local_err);
if (local_err) {
// local error handling goes here
error_propagate(errp, local_err);
}
// same, but "simpler" (except it doesn't work):
do_something(arg, errp);
if (error_is_set(errp)) { // BUG: doesn't work when !errp
// local error handling goes here
}
// same, when no local error handling is wanted:
do_something(arg, errp);
// but it works only once:
do_something(arg1, errp);
do_something(arg2, errp); // BUG: trips assert() when both fail
// pity
Let me stress: beware of error_is_set()! If the argument may or may not
be null, the result is *worthless*. If it can't be null, you could just
as well test the argument directly. If it must be null, the result is
always false.
More patterns:
// accumulating errors, first error wins
Error *err = NULL;
while (...) {
Error *local_err = NULL:
do_something(arg, &local_err);
if (local_err) {
// local error handling goes here
error_propagate(&err, local_err);
}
}
// err contains the first error
// need to free or propagate it
// same, but "simpler" (except it doesn't work)
Error *err = NULL;
while (...) {
do_something(arg, &err);// BUG: trips assert() on second error
if (err) { // BUG: always true after first error
// local error handling goes here
}
}
// can omit err when propagating, just replace it by errp:
// (works because error_propagate() is designed for this)
while (...) {
Error *local_err = NULL:
do_something(arg, &local_err);
if (local_err) {
// local error handling goes here
error_propagate(errp, local_err);
}
}
// errp contains the first error
// accumulating errors, last error wins
Error *err = NULL;
while (...) {
Error *local_err = NULL:
do_something(arg, &local_err);
if (local_err) {
// local error handling goes here
// if we do the following often, we should perhaps provide a
// function for it, just like error_propagate(), only "last
// one wins" instead of "first one wins"
if (err) {
error_free(err);
}
err = local_err;
}
}
// err contains the last error
// need to free or propagate it
// can omit err when propagating, but need to be careful, because
// errp may be null:
while (...) {
Error *local_err = NULL:
do_something(arg, &local_err);
if (local_err) {
// local error handling goes here
if (error_is_set(errp)) {
// exploits that error_is_set(errp) implies errp != NULL
error_free(*errp);
*errp = NULL;
}
error_propagate(errp, local_err);
}
}
Just found a tolerable use of error_is_set(). I'd prefer a function
error_clear(errp) to clear through a possibly null errp.
Still more patterns:
// try a number of args, until one works
Error *local_err = NULL;
for (arg = first_one(); arg; arg = next_one()) {
if (local_err) {
error_free(local_err);
local_err = NULL;
}
do_something(arg, &local_err);
if (!local_err) {
break; // arg worked
}
// local error handling goes here
}
if (local_err) {
// local error handling goes here
// need to free or propagate local_err
} else {
// arg worked
}
We can't omit local_err when propagating, because with just errp, we
can't test whether do_something() succeeded.
With a function that return a value that can be tested:
/*
* Get @arg's foo.
* On success, return foo.
* On failure, return null, and set address@hidden if @errp isn't null.
*/
Foo *get_foo(Argument *arg, Error *errp)
we can omit local_err:
// try a number of args, until one works
for (arg = first_one(); arg; arg = next_one()) {
if (error_is_set(errp)) {
// exploits that error_is_set(errp) implies errp != NULL
error_free(*errp);
*errp = NULL;
}
foo = get_foo(arg, errp);
if (foo) {
break; // arg worked
}
// local error handling goes here
}
if (foo) {
// arg worked
} else {
// local error handling goes here
// need to free or propagate local_err
}
This is roughly how your patch works.
Functions like get_foo() spook me, because when I do
local_err = NULL;
foo = get_foo(arg, &local_err);
if (!foo) {
// fail
return;
}
// success
// use foo (surely not null)
I worry about local_err being null at // fail, or non-null (and leaked)
at // success. When I do
local_err = NULL;
foo = get_foo(arg, &local_err);
if (local_err) {
// fail
return;
}
// success
// use foo
I worry about foo being null at // success, or non-null (and leaked) at
// fail.
Perhaps I'm too easily worried.
- Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable, (continued)
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, Luiz Capitulino, 2013/03/19
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, Kevin Wolf, 2013/03/20
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, Luiz Capitulino, 2013/03/20
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, Kevin Wolf, 2013/03/20
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, Luiz Capitulino, 2013/03/20
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, Markus Armbruster, 2013/03/06
[Qemu-devel] Error ** parameter conventions (was: [PATCH] qemu-sockets: Fix assertion failure),
Markus Armbruster <=