Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()

[Michael Roth]

On Wed, Mar 14, 2012 at 06:19:48PM +0800, Amos Kong wrote:
> 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<address@hidden>
> >>---
> >>  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()
> 

That seems to be for accept(), not connect(). And in the accept()/incoming
case I don't think it's an issue to keep retrying.

On the connect()/outgoing case I think we need to be careful because we can
hang both the monitor and the guest indefinitely if there's an issue affecting
outgoing connection attempts on the source-side. It's much safer to fail in
this situation rather than loop indefinitely, and originally that's what the
code did, albeit somewhat indirectly. That behavior is changed with your
implementation:

tcp_start_outgoing_migration() originally:
    ...
    socket_set_nonblock(s->fd);

    do {
        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
        if (ret == -1) {
            ret = -socket_error();
        }
        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
            return 0;
        }
    } while (ret == -EINTR);
    ...

tcp_start_output_migration() with your changes:
    ...
    ret = tcp_client_start(host_port, &s->fd);
    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
        DPRINTF("connect in progress");
        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);

tcp_client_start():

    static int tcp_client_connect(int fd, struct sockaddr_in *saddr)
    {
        int ret;
    
        do {
            ret = connect(fd, (struct sockaddr *)saddr, sizeof(*saddr));
            if (ret == -1) {
                ret = -socket_error();
            }
        } while (ret == -EINTR || ret == -EWOULDBLOCK);

> 
> >  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 ?

I was referring to the EWOULDBLOCK handling, but on linux at least,
EAGAIN == EWOULDBLOCK.

> 
> 
> >
> >>+#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.

When EAGAIN == EWOULDBLOCK, it would loop, and I'm not aware of any hosts where
this won't be the case. BSD maybe?

> 
> 
> >>+            }
> >>+        } else {
> >>+            break;
> >>+        }
> >>+    }
> >>+
> >>+    return ret;
> >>+}
> >>+
> 
> -- 
>                       Amos.
>