[Note cc: Luiz] Kevin Wolf <kw...@redhat.com> 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 <kw...@redhat.com> > --- > 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 *@errp 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 *@errp 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.