From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47426) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIEYf-0008Uc-9V for qemu-devel@nongnu.org; Wed, 20 Mar 2013 04:39:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UIEYc-0006Wf-9P for qemu-devel@nongnu.org; Wed, 20 Mar 2013 04:39:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60411) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIEYc-0006WQ-1x for qemu-devel@nongnu.org; Wed, 20 Mar 2013 04:39:38 -0400 Date: Wed, 20 Mar 2013 09:39:34 +0100 From: Kevin Wolf Message-ID: <20130320083934.GA3074@dhcp-200-207.str.redhat.com> References: <1362566886-14073-1-git-send-email-kwolf@redhat.com> <513722BD.6010503@redhat.com> <20130306111126.GA2285@dhcp-200-207.str.redhat.com> <513756D5.1020506@redhat.com> <20130319163451.205bc2cd@doriath> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130319163451.205bc2cd@doriath> Subject: Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Paolo Bonzini , aliguori@us.ibm.com, Laszlo Ersek , qemu-devel@nongnu.org Am 19.03.2013 um 21:34 hat Luiz Capitulino geschrieben: > On Wed, 06 Mar 2013 15:46:45 +0100 > Laszlo Ersek 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 > > >>> --- > > >>> 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