From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41940) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7lJo-0006dU-3f for qemu-devel@nongnu.org; Wed, 14 Mar 2012 06:21:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S7lJD-0002iF-KU for qemu-devel@nongnu.org; Wed, 14 Mar 2012 06:20:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64907) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7lJD-0002ht-C3 for qemu-devel@nongnu.org; Wed, 14 Mar 2012 06:19:55 -0400 Message-ID: <4F6070C4.2080006@redhat.com> Date: Wed, 14 Mar 2012 18:19:48 +0800 From: Amos Kong MIME-Version: 1.0 References: <20120306224330.24264.9494.stgit@dhcp-8-167.nay.redhat.com> <20120306224803.24264.44273.stgit@dhcp-8-167.nay.redhat.com> <20120313183534.GC3699@illuin> In-Reply-To: <20120313183534.GC3699@illuin> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: aliguori@us.ibm.com, kvm@vger.kernel.org, quintela@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, owasserm@redhat.com, laine@redhat.com On 14/03/12 02:35, Michael Roth wrote: > On Wed, Mar 07, 2012 at 06:48:03AM +0800, Amos Kong wrote: >> Introduce tcp_client_start() by moving original code in >> tcp_start_outgoing_migration(). >> >> Signed-off-by: Amos Kong >> --- >> net.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> qemu_socket.h | 1 + >> 2 files changed, 42 insertions(+), 0 deletions(-) >> >> diff --git a/net.c b/net.c >> index e90ff23..9afb0d1 100644 >> --- a/net.c >> +++ b/net.c >> @@ -127,6 +127,47 @@ int tcp_server_start(const char *str, int *fd) >> return ret; >> } >> >> +int tcp_client_start(const char *str, int *fd) >> +{ ... Hi Michael, >> + *fd = qemu_socket(PF_INET, SOCK_STREAM, 0); >> + if (fd< 0) { >> + perror("socket"); >> + return -1; >> + } >> + socket_set_nonblock(*fd); >> + >> + for (;;) { >> + ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr)); >> + if (ret< 0) { >> + ret = -socket_error(); >> + if (ret == -EINPROGRESS) { >> + break; > > The previous implementation and your next patch seem to be expecting a break on > -EWOULDBLOCK/-EAGAIN as well. Was the behavior changed on purpose? In original tcp_start_outgoing_migration(): break: -EINPROGRES cont : -EINTR or -EWOULDBLOCK In original net_socket_connect_init(): break: -EINPROGRES or -EWOULDBLOCK cont : -EINTR http://www.cs.utah.edu/dept/old/texinfo/glibc-manual-0.02/library_15.html EWOULDBLOCK socket has nonblocking mode set, and there are no pending connections immediately available. So continue to re-connect if EWOULDBLOCK or EINTR returned by socket_error() in tcp_client_start() > I'm not > sure what the proper handling is for -EAGAIN: whether a non-blocking connect() > can eventually succeed or not. I suspect that it's not, but that previously we > treated it synonymously with -EINPROGRESS, then eventually got an error via > getsockopt() before failing the migration. If so, we're now changing the > behavior to retry until successful, but given the man page entry I don't > think that's a good idea since you might block indefinitely: > > EAGAIN No more free local ports or insufficient > entries in the routing cache. For AF_INET > see the description of > /proc/sys/net/ipv4/ip_local_port_range > ip(7) for information on how to increase > the number of local ports. We didn't process EAGAIN specially, you mean EINTR ? > >> +#ifdef _WIN32 >> + } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) { >> + break; >> +#endif >> + } else if (ret != -EINTR&& ret != -EWOULDBLOCK) { >> + perror("connect"); >> + closesocket(*fd); >> + return ret; -EAGAIN would go this path. >> + } >> + } else { >> + break; >> + } >> + } >> + >> + return ret; >> +} >> + -- Amos.