Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure

[Luiz Capitulino]

On Wed, 20 Mar 2013 09:39:34 +0100
Kevin Wolf <address@hidden> wrote:

> Am 19.03.2013 um 21:34 hat Luiz Capitulino geschrieben:
> > On Wed, 06 Mar 2013 15:46:45 +0100
> > Laszlo Ersek <address@hidden> wrote:
> > 
> > > On 03/06/13 12:11, Kevin Wolf wrote:
> > > > Am 06.03.2013 um 12:04 hat Paolo Bonzini geschrieben:
> > > >> Il 06/03/2013 11:48, Kevin Wolf ha scritto:
> > > >>> 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;
> > > >>>          }
> > > >>>
> > > >>
> > > >> Should we also do nothing if errp is not NULL on entry?
> > > > 
> > > > We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
> > > > an Error, you must return instead of calling more functions with the
> > > > same error pointer.
> > > 
> > > I think Luiz would suggest (*) to receive any error into a
> > > NULL-initialized local_err pointer; do the logic above on local_err, and
> > > just before returning, error_propagate() it to errp.
> > 
> > Yes, I'd suggest that but it turns out that inet_connect_addr() error
> > reporting was and still is confusing, which causes callers to use it
> > incorrectly.
> > 
> > This patch (which has been applied by Anthony)
> 
> No, Anthony applied a different, but similar patch of his own. This is
> why I don't feel particularly responsible for the specific problem any
> more.
> 
> How to do error handling with Error right is the only reason for me to
> continue the discussion.

I think we need a kind of "Error best practices" doc.

> > solves the problem at
> > hand but it also introduces a new issue: errors from inet_connect_addr()
> > are only reported if they happen in the last loop interaction. Note that
> > a few other errors other than 'couldn't connect' can happen.
> 
> > Laszlo's comment seemed to have triggered a discussion around Error **,
> > but this really has very little to do with it: the real problem is that
> > inet_connect_addr() is too confusing.
> 
> Maybe we need to discuss first what the intended behaviour even is. My
> interpretation was this: We may have several addresses to try. If one of
> them works, the function as a whole has succeeded and must not return an
> error, neither in errp nor as -errno. If none of them succeeds, the
> function has to return an error, and returning the error of the last
> attempt is as good as the error of any other attempt.

I agree. When I looked at the code yesterday I had the impression that
several other errors where possible, which made me wonder if we shouldn't
stop short the loop on non-"can't connect" type of errors.

But looking at it again we have only socket() and connect() calls, and
I'd expect that most (all?) non can't connect errors will happen in all
loop iterations, which will cause the error to be reported.

> > inet_connect_addr() has two users: inet_connect_opts() and 
> > wait_for_connect(),
> > with this patch both of them are now ignoring errors from 
> > inet_connect_addr().
> > 
> > Suggested solution: refactor inet_connect_addr() to return an errno value.
> > Callers use error_set() when they want to report an error upward.
> 
> Doesn't change the problem that you need to know when to set a return
> value != 0. So it doesn't help, but you'd lose some error information.

My real point is that it's easier to check against errno to find out
the error cause (compared to using Error for that).