From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48937) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEiUa-00011B-EA for qemu-devel@nongnu.org; Thu, 20 Sep 2012 11:16:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEiUR-00034C-Rd for qemu-devel@nongnu.org; Thu, 20 Sep 2012 11:16:40 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:34873) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEiUR-000342-JN for qemu-devel@nongnu.org; Thu, 20 Sep 2012 11:16:31 -0400 Date: Thu, 20 Sep 2012 11:16:27 -0400 (EDT) From: Amos Kong Message-ID: <170225001.2762316.1348154187593.JavaMail.root@redhat.com> In-Reply-To: <505AFC12.6020709@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 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: Orit Wasserman Cc: kwolf@redhat.com, aliguori@us.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, mst@redhat.com, armbru@redhat.com, mdroth@linux.vnet.ibm.com, pbonzini@redhat.com, lcapitulino@redhat.com ----- Original Message ----- > 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 > >> different > >> address. > >> callers on inet_nonblocking_connect register a callback function > >> that will > >> be called when connect opertion completes, in case of failure the > >> fd will 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 =E2=80=98ConnectStat= e=E2=80=99 > > qemu_socket.h:46: note: previous declaration of =E2=80=98ConnectState= =E2=80=99 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 > > errno is set appropriately. > >=20 > > .. original code: > > .. > > .. do { > > .. ret =3D getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) > > &val, &valsize); > > .. } while (ret =3D=3D -1 && (socket_error()) =3D=3D EINTR); > > .. > > .. if (ret < 0) { // ret equals to -1, and socket_error() !=3D > > EINTR > >=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 > > address 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 It seems rc is 0 in this case. > 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. In the past, if rc < 0, we will return without setting fd handler to NULL, tcp_wait_for_connect() will be re-called. the original socket (current addr= ) will be re-checked by getsockopt() But in your patch, it will retry next addr in the list. > >=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_progress, 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 >=20 >=20 >=20