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

[Amos Kong]

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()


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