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

[Amos Kong <akong@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<akong@redhat.com>
>> ---
>>   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.