From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59627) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEeoN-0003vX-TL for qemu-devel@nongnu.org; Thu, 20 Sep 2012 07:20:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEeoJ-0002uV-8f for qemu-devel@nongnu.org; Thu, 20 Sep 2012 07:20:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33599) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEeoI-0002uE-Q8 for qemu-devel@nongnu.org; Thu, 20 Sep 2012 07:20:47 -0400 Message-ID: <505AFC12.6020709@redhat.com> Date: Thu, 20 Sep 2012 14:20:50 +0300 From: Orit Wasserman MIME-Version: 1.0 References: <1347562697-15411-1-git-send-email-owasserm@redhat.com> <1347562697-15411-4-git-send-email-owasserm@redhat.com> <505982FD.3020601@redhat.com> In-Reply-To: <505982FD.3020601@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong Cc: kwolf@redhat.com, aliguori@us.ibm.com, mdroth@linux.vnet.ibm.com, mst@redhat.com, quintela@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, lcapitulino@redhat.com On 09/19/2012 11:31 AM, Amos Kong wrote: > On 14/09/12 02:58, Orit Wasserman wrote: >> getaddrinfo can give us a list of addresses, but we only try to >> connect to the first one. If that fails we never proceed to >> the next one. This is common on desktop setups that often have ipv6 >> configured but not actually working. >> >> To fix this make inet_connect_nonblocking retry connection with a diff= erent >> address. >> callers on inet_nonblocking_connect register a callback function that = will >> be called when connect opertion completes, in case of failure the fd w= ill have >> a negative value >=20 >=20 > Hi Orit, >=20 > We almost get to the destination :) >=20 >> Signed-off-by: Orit Wasserman >> Signed-off-by: Michael S. Tsirkin >> --- >> migration-tcp.c | 29 +++++----------- >> qemu-char.c | 2 +- >> qemu-sockets.c | 95 +++++++++++++++++++++++++++++++++++++++++++++= ++------- >> qemu_socket.h | 13 ++++++-- >> 4 files changed, 102 insertions(+), 37 deletions(-) >> >=20 > ... >=20 >> diff --git a/qemu-sockets.c b/qemu-sockets.c >> index 212075d..d321c58 100644 >> --- a/qemu-sockets.c >> +++ b/qemu-sockets.c >> @@ -24,6 +24,7 @@ >> >> #include "qemu_socket.h" >> #include "qemu-common.h" /* for qemu_isdigit */ >> +#include "main-loop.h" >> >> #ifndef AI_ADDRCONFIG >> # define AI_ADDRCONFIG 0 >> @@ -217,11 +218,69 @@ listen: >> ((rc) =3D=3D -EINPROGRESS) >> #endif >> >> +/* Struct to store connect state for non blocking connect */ >> +typedef struct ConnectState { >> + int fd; >> + struct addrinfo *addr_list; >> + struct addrinfo *current_addr; >> + ConnectHandler *callback; >> + void *opaque; >=20 >> + Error *errp; >=20 > This member is not used. I will remove it. >=20 >> +} ConnectState; >=20 > # make (gcc-4.4.6-4.el6.x86_64) > CC qemu-sockets.o > qemu-sockets.c:229: error: redefinition of typedef =91ConnectState=92 > qemu_socket.h:46: note: previous declaration of =91ConnectState=92 was = here > make: *** [qemu-sockets.o] Error 1 >=20 >=20 > struct ConnectState { > int fd; > struct addrinfo *addr_list; > struct addrinfo *current_addr; > ConnectHandler *callback; > void *opaque; > }; >=20 >=20 >> + >> static int inet_connect_addr(struct addrinfo *addr, bool block, >> - bool *in_progress) >> + bool *in_progress, ConnectState *connect= _state); >> + >> +static void wait_for_connect(void *opaque) >> +{ >> + ConnectState *s =3D opaque; >> + int val =3D 0, rc =3D 0; >> + socklen_t valsize =3D sizeof(val); >> + bool in_progress =3D false; >> + >> + qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL); >> + >> + do { >> + rc =3D getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val,= &valsize); >> + } while (rc =3D=3D -1 && socket_error() =3D=3D EINTR); >=20 > # man getsockopt > RETURN VALUE > On success, zero is returned. On error, -1 is returned, and err= no is set appropriately. >=20 > .. original code: > .. > .. do { > .. ret =3D getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &va= l, &valsize); > .. } while (ret =3D=3D -1 && (socket_error()) =3D=3D EINTR); > .. > .. if (ret < 0) { // ret equals to -1, and socket_error() !=3D EIN= TR >=20 > If ret < 0, just set the error state, but the callback function is not = set to NULL. > Then tcp_wait_for_connect() will be called again, and retry to checking= current > socket by getsockopt(). >=20 > But in this patch, we will abandon current socket, and connect next add= ress on the list. > We should reserve that block, and move qemu_set_fd_handler2(...NULL..) = below it. I'm sorry I don't get you point, it makes no difference where is qemu_set= _fd_handlers(...NULL...). rc < 0 happens in two cases: 1. val !=3D 0 - connect error 2. getsockopt failed for reason different than EINTR . In both cases we can only move to the next addr in the list, inet_connect_addr will call qemu_set_fd_handlers with the new socket fd. >=20 > .. migrate_fd_error(s); > .. return; > .. } > .. > .. qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL); >=20 >> + /* update rc to contain error details */ >> + if (!rc && val) { >> + rc =3D -val; >=20 > another issue: val >=3D 0 all the time? yes, I can had a check anyway. >=20 >> + } >> + >> + /* connect error */ >> + if (rc < 0) { >> + closesocket(s->fd); >> + s->fd =3D rc; >> + } >> + >> + /* try to connect to the next address on the list */ >> + while (s->current_addr->ai_next !=3D NULL && s->fd < 0) { >> + s->current_addr =3D s->current_addr->ai_next; >> + s->fd =3D inet_connect_addr(s->current_addr, false, &in_progr= ess, s); >> + /* connect in progress */ >> + if (in_progress) { >> + return; >> + } >> + } >> + >> + freeaddrinfo(s->addr_list); >> + if (s->callback) { >> + s->callback(s->fd, s->opaque); >> + } >> + g_free(s); >> + return; >> +} >=20 >=20 >=20