[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure |
Date: |
Wed, 20 Mar 2013 09:39:34 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
> 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.
> 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.
Kevin
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, (continued)
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, Laszlo Ersek, 2013/03/06
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, Markus Armbruster, 2013/03/06
- Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure, Paolo Bonzini, 2013/03/06
- [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable, Kevin Wolf, 2013/03/14
- Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable, Laszlo Ersek, 2013/03/14
- Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable, Kevin Wolf, 2013/03/15
- Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable, Laszlo Ersek, 2013/03/15
- Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable, Kevin Wolf, 2013/03/15
- Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable, Laszlo Ersek, 2013/03/15
- 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 <=
- 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, 2013/03/06