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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 06 Mar 2013 15:46:45 +0100
Laszlo Ersek <lersek@redhat.com> 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 <kwolf@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;
> >>>          }
> >>>
> >>
> >> 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) 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.

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.