Patchwork [v6,1/5] sockets: change inet_connect() to support nonblock socket

login
register
mail settings
Submitter Amos Kong
Date April 17, 2012, 2:54 p.m.
Message ID <20120417145401.22756.32812.stgit@dhcp-8-167.nay.redhat.com>
Download mbox | patch
Permalink /patch/153220/
State New
Headers show

Comments

Amos Kong - April 17, 2012, 2:54 p.m.
Add a bool argument to inet_connect() to assign if set socket
to block/nonblock, and delete original argument 'socktype'
that is unused.

Retry to connect when following errors are got:
  -EINTR
  -EWOULDBLOCK (win32)
Connect's successful for nonblock socket when following
errors are got, user should wait for connecting by select():
  -EINPROGRESS
  -WSAEALREADY (win32)

Change nbd, vnc to use new interface.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 nbd.c          |    2 +-
 qemu-sockets.c |   58 +++++++++++++++++++++++++++++++++++++++++++-------------
 qemu_socket.h  |    2 +-
 ui/vnc.c       |    2 +-
 4 files changed, 48 insertions(+), 16 deletions(-)
Michael Roth - April 18, 2012, 2:10 a.m.
On Tue, Apr 17, 2012 at 10:54:01PM +0800, Amos Kong wrote:
> Add a bool argument to inet_connect() to assign if set socket
> to block/nonblock, and delete original argument 'socktype'
> that is unused.
> 
> Retry to connect when following errors are got:
>   -EINTR
>   -EWOULDBLOCK (win32)
> Connect's successful for nonblock socket when following
> errors are got, user should wait for connecting by select():
>   -EINPROGRESS
>   -WSAEALREADY (win32)

How does the user access these? Via errno/socket_error()? I'm not sure
how that would work, I don't think we can rely on the value of errno
unless a function indicates an error, so for the case where we're
successful but have an EINPROGRESS, I don't think we have a way of
reliably obtaining that (errno value might be stale) except maybe
something like getsockopt().

I think it would be better to just merge this with your patch #3, which
handles all this nicely via Error.
> 
> Change nbd, vnc to use new interface.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  nbd.c          |    2 +-
>  qemu-sockets.c |   58 +++++++++++++++++++++++++++++++++++++++++++-------------
>  qemu_socket.h  |    2 +-
>  ui/vnc.c       |    2 +-
>  4 files changed, 48 insertions(+), 16 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index 406e555..b4e68a9 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
> 
>  int tcp_socket_outgoing_spec(const char *address_and_port)
>  {
> -    return inet_connect(address_and_port, SOCK_STREAM);
> +    return inet_connect(address_and_port, true);
>  }
> 
>  int tcp_socket_incoming(const char *address, uint16_t port)
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 6bcb8e3..e886195 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = {
>          },{
>              .name = "ipv6",
>              .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "block",
> +            .type = QEMU_OPT_BOOL,
>          },
>          { /* end if list */ }
>      },
> @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts)
>      const char *port;
>      char uaddr[INET6_ADDRSTRLEN+1];
>      char uport[33];
> -    int sock,rc;
> +    int sock, rc, err;
> +    bool block;
> 
>      memset(&ai,0, sizeof(ai));
>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> @@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts)
> 
>      addr = qemu_opt_get(opts, "host");
>      port = qemu_opt_get(opts, "port");
> +    block = qemu_opt_get_bool(opts, "block", 0);
>      if (addr == NULL || port == NULL) {
>          fprintf(stderr, "inet_connect: host and/or port not specified\n");
>          return -1;
> @@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts)
>              continue;
>          }
>          setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> -
> +        if (!block) {
> +            socket_set_nonblock(sock);
> +        }
>          /* connect to peer */
> -        if (connect(sock,e->ai_addr,e->ai_addrlen) < 0) {
> -            if (NULL == e->ai_next)
> -                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> -                        inet_strfamily(e->ai_family),
> -                        e->ai_canonname, uaddr, uport, strerror(errno));
> -            closesocket(sock);
> -            continue;
> +        do {
> +            err = 0;
> +            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
> +                err = -socket_error();
> +            }
> +#ifndef _WIN32
> +        } while (err == -EINTR || err == -EWOULDBLOCK);
> +#else
> +        } while (err == -EINTR);
> +#endif
> +
> +        if (err >= 0) {
> +            goto success;
> +        } else if (!block && err == -EINPROGRESS) {
> +            goto success;
> +#ifdef _WIN32
> +        } else if (!block && err == -WSAEALREADY) {
> +            goto success;
> +#endif
>          }
> -        freeaddrinfo(res);
> -        return sock;
> +
> +        if (NULL == e->ai_next) {
> +            fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> +                    inet_strfamily(e->ai_family),
> +                    e->ai_canonname, uaddr, uport, strerror(errno));
> +        }
> +        closesocket(sock);
>      }
>      freeaddrinfo(res);
>      return -1;
> +
> +success:
> +    freeaddrinfo(res);
> +    return sock;
>  }
> 
>  int inet_dgram_opts(QemuOpts *opts)
> @@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen,
>      return sock;
>  }
> 
> -int inet_connect(const char *str, int socktype)
> +int inet_connect(const char *str, bool block)
>  {
>      QemuOpts *opts;
>      int sock = -1;
> 
>      opts = qemu_opts_create(&dummy_opts, NULL, 0);
> -    if (inet_parse(opts, str) == 0)
> +    if (inet_parse(opts, str) == 0) {
> +        if (block) {
> +            qemu_opt_set(opts, "block", "on");
> +        }
>          sock = inet_connect_opts(opts);
> +    }
>      qemu_opts_del(opts);
>      return sock;
>  }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index a5d0a84..f73e26d 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset);
>  int inet_listen(const char *str, char *ostr, int olen,
>                  int socktype, int port_offset);
>  int inet_connect_opts(QemuOpts *opts);
> -int inet_connect(const char *str, int socktype);
> +int inet_connect(const char *str, bool block);
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index deb9ecd..4a96153 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
>          if (strncmp(display, "unix:", 5) == 0)
>              vs->lsock = unix_connect(display+5);
>          else
> -            vs->lsock = inet_connect(display, SOCK_STREAM);
> +            vs->lsock = inet_connect(display, true);
>          if (-1 == vs->lsock) {
>              g_free(vs->display);
>              vs->display = NULL;
> 
>
Amos Kong - April 18, 2012, 2:49 a.m.
On 18/04/12 10:10, Michael Roth wrote:
> On Tue, Apr 17, 2012 at 10:54:01PM +0800, Amos Kong wrote:
>> Add a bool argument to inet_connect() to assign if set socket
>> to block/nonblock, and delete original argument 'socktype'
>> that is unused.
>>
>> Retry to connect when following errors are got:
>>    -EINTR
>>    -EWOULDBLOCK (win32)
>> Connect's successful for nonblock socket when following
>> errors are got, user should wait for connecting by select():
>>    -EINPROGRESS
>>    -WSAEALREADY (win32)
>
> How does the user access these? Via errno/socket_error()?

Yes, connect error is got by socket_error()

>  I'm not sure
> how that would work, I don't think we can rely on the value of errno
> unless a function indicates an error, so for the case where we're
> successful but have an EINPROGRESS, I don't think we have a way of
> reliably obtaining that (errno value might be stale) except maybe
> something like getsockopt().

> I think it would be better to just merge this with your patch #3, which
> handles all this nicely via Error.

We use Error to pass meaningful error, but we need to process original 
socket error,
which is got by socket_error().

In qemu, we used socket_error() to get socket error in many place. If 
you are confused
with how that would work, we need to fix socket_error. I don't want to 
fix this in the
migration patchset.

What's your opinion?

>> Change nbd, vnc to use new interface.
>>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   nbd.c          |    2 +-
>>   qemu-sockets.c |   58 +++++++++++++++++++++++++++++++++++++++++++-------------
>>   qemu_socket.h  |    2 +-
>>   ui/vnc.c       |    2 +-
>>   4 files changed, 48 insertions(+), 16 deletions(-)
>>
>> diff --git a/nbd.c b/nbd.c
>> index 406e555..b4e68a9 100644
>> --- a/nbd.c
>> +++ b/nbd.c
>> @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
>>
>>   int tcp_socket_outgoing_spec(const char *address_and_port)
>>   {
>> -    return inet_connect(address_and_port, SOCK_STREAM);
>> +    return inet_connect(address_and_port, true);
>>   }
>>
>>   int tcp_socket_incoming(const char *address, uint16_t port)
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 6bcb8e3..e886195 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = {
>>           },{
>>               .name = "ipv6",
>>               .type = QEMU_OPT_BOOL,
>> +        },{
>> +            .name = "block",
>> +            .type = QEMU_OPT_BOOL,
>>           },
>>           { /* end if list */ }
>>       },
>> @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts)
>>       const char *port;
>>       char uaddr[INET6_ADDRSTRLEN+1];
>>       char uport[33];
>> -    int sock,rc;
>> +    int sock, rc, err;
>> +    bool block;
>>
>>       memset(&ai,0, sizeof(ai));
>>       ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>> @@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts)
>>
>>       addr = qemu_opt_get(opts, "host");
>>       port = qemu_opt_get(opts, "port");
>> +    block = qemu_opt_get_bool(opts, "block", 0);
>>       if (addr == NULL || port == NULL) {
>>           fprintf(stderr, "inet_connect: host and/or port not specified\n");
>>           return -1;
>> @@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts)
>>               continue;
>>           }
>>           setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>> -
>> +        if (!block) {
>> +            socket_set_nonblock(sock);
>> +        }
>>           /* connect to peer */
>> -        if (connect(sock,e->ai_addr,e->ai_addrlen)<  0) {
>> -            if (NULL == e->ai_next)
>> -                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
>> -                        inet_strfamily(e->ai_family),
>> -                        e->ai_canonname, uaddr, uport, strerror(errno));
>> -            closesocket(sock);
>> -            continue;
>> +        do {
>> +            err = 0;
>> +            if (connect(sock, e->ai_addr, e->ai_addrlen)<  0) {
>> +                err = -socket_error();


connect error is got here


>> +            }
>> +#ifndef _WIN32
>> +        } while (err == -EINTR || err == -EWOULDBLOCK);
>> +#else
>> +        } while (err == -EINTR);
>> +#endif
>> +
>> +        if (err>= 0) {
>> +            goto success;
>> +        } else if (!block&&  err == -EINPROGRESS) {
>> +            goto success;
>> +#ifdef _WIN32
>> +        } else if (!block&&  err == -WSAEALREADY) {
>> +            goto success;
>> +#endif
>>           }
>> -        freeaddrinfo(res);
>> -        return sock;
>> +
>> +        if (NULL == e->ai_next) {
>> +            fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
>> +                    inet_strfamily(e->ai_family),
>> +                    e->ai_canonname, uaddr, uport, strerror(errno));
>> +        }
>> +        closesocket(sock);
>>       }
>>       freeaddrinfo(res);
>>       return -1;
>> +
>> +success:
>> +    freeaddrinfo(res);
>> +    return sock;
>>   }
>>
>>   int inet_dgram_opts(QemuOpts *opts)
>> @@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen,
>>       return sock;
>>   }
>>
>> -int inet_connect(const char *str, int socktype)
>> +int inet_connect(const char *str, bool block)
>>   {
>>       QemuOpts *opts;
>>       int sock = -1;
>>
>>       opts = qemu_opts_create(&dummy_opts, NULL, 0);
>> -    if (inet_parse(opts, str) == 0)
>> +    if (inet_parse(opts, str) == 0) {
>> +        if (block) {
>> +            qemu_opt_set(opts, "block", "on");
>> +        }
>>           sock = inet_connect_opts(opts);
>> +    }
>>       qemu_opts_del(opts);
>>       return sock;
>>   }
>> diff --git a/qemu_socket.h b/qemu_socket.h
>> index a5d0a84..f73e26d 100644
>> --- a/qemu_socket.h
>> +++ b/qemu_socket.h
>> @@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset);
>>   int inet_listen(const char *str, char *ostr, int olen,
>>                   int socktype, int port_offset);
>>   int inet_connect_opts(QemuOpts *opts);
>> -int inet_connect(const char *str, int socktype);
>> +int inet_connect(const char *str, bool block);
>>   int inet_dgram_opts(QemuOpts *opts);
>>   const char *inet_strfamily(int family);
>>
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index deb9ecd..4a96153 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
>>           if (strncmp(display, "unix:", 5) == 0)
>>               vs->lsock = unix_connect(display+5);
>>           else
>> -            vs->lsock = inet_connect(display, SOCK_STREAM);
>> +            vs->lsock = inet_connect(display, true);
>>           if (-1 == vs->lsock) {
>>               g_free(vs->display);
>>               vs->display = NULL;
>>
>>
>
Orit Wasserman - April 18, 2012, 6:52 a.m.
On 04/17/2012 05:54 PM, Amos Kong wrote:
> Add a bool argument to inet_connect() to assign if set socket
> to block/nonblock, and delete original argument 'socktype'
> that is unused.
> 
> Retry to connect when following errors are got:
>   -EINTR
>   -EWOULDBLOCK (win32)
> Connect's successful for nonblock socket when following
> errors are got, user should wait for connecting by select():
>   -EINPROGRESS
>   -WSAEALREADY (win32)
> 
> Change nbd, vnc to use new interface.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  nbd.c          |    2 +-
>  qemu-sockets.c |   58 +++++++++++++++++++++++++++++++++++++++++++-------------
>  qemu_socket.h  |    2 +-
>  ui/vnc.c       |    2 +-
>  4 files changed, 48 insertions(+), 16 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index 406e555..b4e68a9 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
>  
>  int tcp_socket_outgoing_spec(const char *address_and_port)
>  {
> -    return inet_connect(address_and_port, SOCK_STREAM);
> +    return inet_connect(address_and_port, true);
>  }
>  
>  int tcp_socket_incoming(const char *address, uint16_t port)
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 6bcb8e3..e886195 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = {
>          },{
>              .name = "ipv6",
>              .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "block",
> +            .type = QEMU_OPT_BOOL,
>          },
>          { /* end if list */ }
>      },
> @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts)
>      const char *port;
>      char uaddr[INET6_ADDRSTRLEN+1];
>      char uport[33];
> -    int sock,rc;
> +    int sock, rc, err;
> +    bool block;
>  
>      memset(&ai,0, sizeof(ai));
>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> @@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts)
>  
>      addr = qemu_opt_get(opts, "host");
>      port = qemu_opt_get(opts, "port");
> +    block = qemu_opt_get_bool(opts, "block", 0);
>      if (addr == NULL || port == NULL) {
>          fprintf(stderr, "inet_connect: host and/or port not specified\n");
>          return -1;
> @@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts)
>              continue;
>          }
>          setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> -
> +        if (!block) {
> +            socket_set_nonblock(sock);
> +        }
>          /* connect to peer */
> -        if (connect(sock,e->ai_addr,e->ai_addrlen) < 0) {
> -            if (NULL == e->ai_next)
> -                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> -                        inet_strfamily(e->ai_family),
> -                        e->ai_canonname, uaddr, uport, strerror(errno));
> -            closesocket(sock);
> -            continue;
> +        do {
> +            err = 0;
> +            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
> +                err = -socket_error();
> +            }
> +#ifndef _WIN32
> +        } while (err == -EINTR || err == -EWOULDBLOCK);
> +#else
> +        } while (err == -EINTR);
> +#endif

We shouldn't retry to connect for a blocking socket, please add a check for !block.
According to msn docs in WIN32 if we get EWOULDBLOCK , we should do select 
http://msdn.microsoft.com/en-us/library/windows/desktop/ms737625(v=vs.85).aspx
so I think we only need to retry for -EINTR.

> +
> +        if (err >= 0) {
> +            goto success;
> +        } else if (!block && err == -EINPROGRESS) {
> +            goto success;
> +#ifdef _WIN32
> +        } else if (!block && err == -WSAEALREADY) {

Also EWOULDBLOCK
This is more a style comment as I feel to code doesn't need the go to.

Check for an error path so the rest of the function looks like:

if (err < 0) {
	if ( block ||
#ifndef __WIN32
	 err != -EINPROGRESS ) {
#else 
	(err != -EWOULDBLOCK && err != -WASALREADY) ) {
#endif
	if (NULL == e->ai_next) {
            fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
                    inet_strfamily(e->ai_family),
                    e->ai_canonname, uaddr, uport, strerror(errno));
        }
        closesocket(sock);
	sock = -1;
}

freeaddrinfo(res);
return sock;
}

> +            goto success;
> +#endif
>          }
> -        freeaddrinfo(res);
> -        return sock;
> +
> +        if (NULL == e->ai_next) {
> +            fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> +                    inet_strfamily(e->ai_family),
> +                    e->ai_canonname, uaddr, uport, strerror(errno));
> +        }
> +        closesocket(sock);
>      }
>      freeaddrinfo(res);
>      return -1;
> +
> +success:
> +    freeaddrinfo(res);
> +    return sock;
>  }
>  
>  int inet_dgram_opts(QemuOpts *opts)
> @@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen,
>      return sock;
>  }
>  
> -int inet_connect(const char *str, int socktype)
> +int inet_connect(const char *str, bool block)
>  {
>      QemuOpts *opts;
>      int sock = -1;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0);
> -    if (inet_parse(opts, str) == 0)
> +    if (inet_parse(opts, str) == 0) {
> +        if (block) {
> +            qemu_opt_set(opts, "block", "on");
> +        }
>          sock = inet_connect_opts(opts);
> +    }
>      qemu_opts_del(opts);
>      return sock;
>  }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index a5d0a84..f73e26d 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset);
>  int inet_listen(const char *str, char *ostr, int olen,
>                  int socktype, int port_offset);
>  int inet_connect_opts(QemuOpts *opts);
> -int inet_connect(const char *str, int socktype);
> +int inet_connect(const char *str, bool block);
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);
>  
> diff --git a/ui/vnc.c b/ui/vnc.c
> index deb9ecd..4a96153 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
>          if (strncmp(display, "unix:", 5) == 0)
>              vs->lsock = unix_connect(display+5);
>          else
> -            vs->lsock = inet_connect(display, SOCK_STREAM);
> +            vs->lsock = inet_connect(display, true);
>          if (-1 == vs->lsock) {
>              g_free(vs->display);
>              vs->display = NULL;
>
Amos Kong - April 18, 2012, 10:43 a.m.
On 18/04/12 14:52, Orit Wasserman wrote:
> On 04/17/2012 05:54 PM, Amos Kong wrote:
>> Add a bool argument to inet_connect() to assign if set socket
>> to block/nonblock, and delete original argument 'socktype'
>> that is unused.
>>
>> Retry to connect when following errors are got:
>>    -EINTR
>>    -EWOULDBLOCK (win32)
>> Connect's successful for nonblock socket when following
>> errors are got, user should wait for connecting by select():
>>    -EINPROGRESS
>>    -WSAEALREADY (win32)
>>
>> Change nbd, vnc to use new interface.
>>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   nbd.c          |    2 +-
>>   qemu-sockets.c |   58 +++++++++++++++++++++++++++++++++++++++++++-------------
>>   qemu_socket.h  |    2 +-
>>   ui/vnc.c       |    2 +-
>>   4 files changed, 48 insertions(+), 16 deletions(-)
>>
>> diff --git a/nbd.c b/nbd.c
>> index 406e555..b4e68a9 100644
>> --- a/nbd.c
>> +++ b/nbd.c
>> @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
>>
>>   int tcp_socket_outgoing_spec(const char *address_and_port)
>>   {
>> -    return inet_connect(address_and_port, SOCK_STREAM);
>> +    return inet_connect(address_and_port, true);
>>   }
>>
>>   int tcp_socket_incoming(const char *address, uint16_t port)
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 6bcb8e3..e886195 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = {
>>           },{
>>               .name = "ipv6",
>>               .type = QEMU_OPT_BOOL,
>> +        },{
>> +            .name = "block",
>> +            .type = QEMU_OPT_BOOL,
>>           },
>>           { /* end if list */ }
>>       },
>> @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts)
>>       const char *port;
>>       char uaddr[INET6_ADDRSTRLEN+1];
>>       char uport[33];
>> -    int sock,rc;
>> +    int sock, rc, err;
>> +    bool block;
>>
>>       memset(&ai,0, sizeof(ai));
>>       ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>> @@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts)
>>
>>       addr = qemu_opt_get(opts, "host");
>>       port = qemu_opt_get(opts, "port");
>> +    block = qemu_opt_get_bool(opts, "block", 0);
>>       if (addr == NULL || port == NULL) {
>>           fprintf(stderr, "inet_connect: host and/or port not specified\n");
>>           return -1;
>> @@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts)
>>               continue;
>>           }
>>           setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>> -
>> +        if (!block) {
>> +            socket_set_nonblock(sock);
>> +        }
>>           /* connect to peer */
>> -        if (connect(sock,e->ai_addr,e->ai_addrlen)<  0) {
>> -            if (NULL == e->ai_next)
>> -                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
>> -                        inet_strfamily(e->ai_family),
>> -                        e->ai_canonname, uaddr, uport, strerror(errno));
>> -            closesocket(sock);
>> -            continue;
>> +        do {
>> +            err = 0;
>> +            if (connect(sock, e->ai_addr, e->ai_addrlen)<  0) {
>> +                err = -socket_error();
>> +            }
>> +#ifndef _WIN32
>> +        } while (err == -EINTR || err == -EWOULDBLOCK);
>> +#else
>> +        } while (err == -EINTR);
>> +#endif
>
> We shouldn't retry to connect for a blocking socket, please add a check for !block.
> According to msn docs in WIN32 if we get EWOULDBLOCK , we should do select
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms737625(v=vs.85).aspx
> so I think we only need to retry for -EINTR.

Hi Orit, thanks for your review.

In 
http://msdn.microsoft.com/en-us/library/windows/desktop/ms740668%28v=vs.85%29.aspx,
for 'EWOULDBLOCK': the operation should be retried later."

However, the two methods all works.  I would not re-try for EWOULDBLOCK 
(posix & win32).

>> +
>> +        if (err>= 0) {
>> +            goto success;
>> +        } else if (!block&&  err == -EINPROGRESS) {
>> +            goto success;
>> +#ifdef _WIN32
>> +        } else if (!block&&  err == -WSAEALREADY) {
>
> Also EWOULDBLOCK
> This is more a style comment as I feel to code doesn't need the go to.

Ok.

> Check for an error path so the rest of the function looks like:
>
> if (err<  0) {
> 	if ( block ||
> #ifndef __WIN32
> 	 err != -EINPROGRESS ) {
> #else
> 	(err != -EWOULDBLOCK&&  err != -WASALREADY) ) {
> #endif


EINPROGRESS and EWOULDBLOCK are for posix and win32,

   if (err <  0) {
   	if (block || (err != -EINPROGRESS && err != -EWOULDBLOCK
   #ifndef __WIN32
   	 )) {
   #else
   	&&  err != -WASALREADY)) {
   #endif


> 	if (NULL == e->ai_next) {
>              fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
>                      inet_strfamily(e->ai_family),
>                      e->ai_canonname, uaddr, uport, strerror(errno));
>          }
>          closesocket(sock);
> 	sock = -1;
> }
>
> freeaddrinfo(res);
> return sock;
> }

...
Michael Roth - April 18, 2012, 2:22 p.m.
On Wed, Apr 18, 2012 at 10:49:02AM +0800, Amos Kong wrote:
> On 18/04/12 10:10, Michael Roth wrote:
> >On Tue, Apr 17, 2012 at 10:54:01PM +0800, Amos Kong wrote:
> >>Add a bool argument to inet_connect() to assign if set socket
> >>to block/nonblock, and delete original argument 'socktype'
> >>that is unused.
> >>
> >>Retry to connect when following errors are got:
> >>   -EINTR
> >>   -EWOULDBLOCK (win32)
> >>Connect's successful for nonblock socket when following
> >>errors are got, user should wait for connecting by select():
> >>   -EINPROGRESS
> >>   -WSAEALREADY (win32)
> >
> >How does the user access these? Via errno/socket_error()?
> 
> Yes, connect error is got by socket_error()
> 
> > I'm not sure
> >how that would work, I don't think we can rely on the value of errno
> >unless a function indicates an error, so for the case where we're
> >successful but have an EINPROGRESS, I don't think we have a way of
> >reliably obtaining that (errno value might be stale) except maybe
> >something like getsockopt().
> 
> >I think it would be better to just merge this with your patch #3, which
> >handles all this nicely via Error.
> 
> We use Error to pass meaningful error, but we need to process
> original socket error,
> which is got by socket_error().
> 
> In qemu, we used socket_error() to get socket error in many place.
> If you are confused
> with how that would work, we need to fix socket_error. I don't want
> to fix this in the
> migration patchset.

I don't really think it needs fixing any more than errno needs
fixing, but it should be used only in situations where it makes sense
to use errno, namely:

1) You have a reliable indicator of when errno has been set, such
that you can check it's value and be assured that it's not a stale
value from some unrelated error that occurred in the past.

2) You don't do anything that can clobber the errno value before the
user has a chance to check it.

inet_connect() violates 1) because we rely on the caller to check
errno/socket_error() for EINPROGRESS even though we pass back a valid
fd that has no indication of an error. It may be that we already
successfully connected, and that some unrelated path set EINPROGRESS,
which may cause us to call select()/getsockopt() unecessary.

it also violates 2), because we call close() in the error path, which
can clobber errno/socket_error().

> 
> What's your opinion?

I think you already have the solution in your patches #2/#3: rely on
Error **errp to pass back these errors. So my suggestion is to just
take this patch and merge it with those.

Alternatively, if you'd like to keep the introduction of non-blocking
support in a seperate patch, do it *after* you introduce Error. So
basically move this after your patch #4 so you never have to rely
on socket_error() in your series.

> 
> >>Change nbd, vnc to use new interface.
> >>
> >>Signed-off-by: Amos Kong<akong@redhat.com>
> >>---
> >>  nbd.c          |    2 +-
> >>  qemu-sockets.c |   58 +++++++++++++++++++++++++++++++++++++++++++-------------
> >>  qemu_socket.h  |    2 +-
> >>  ui/vnc.c       |    2 +-
> >>  4 files changed, 48 insertions(+), 16 deletions(-)
> >>
> >>diff --git a/nbd.c b/nbd.c
> >>index 406e555..b4e68a9 100644
> >>--- a/nbd.c
> >>+++ b/nbd.c
> >>@@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
> >>
> >>  int tcp_socket_outgoing_spec(const char *address_and_port)
> >>  {
> >>-    return inet_connect(address_and_port, SOCK_STREAM);
> >>+    return inet_connect(address_and_port, true);
> >>  }
> >>
> >>  int tcp_socket_incoming(const char *address, uint16_t port)
> >>diff --git a/qemu-sockets.c b/qemu-sockets.c
> >>index 6bcb8e3..e886195 100644
> >>--- a/qemu-sockets.c
> >>+++ b/qemu-sockets.c
> >>@@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = {
> >>          },{
> >>              .name = "ipv6",
> >>              .type = QEMU_OPT_BOOL,
> >>+        },{
> >>+            .name = "block",
> >>+            .type = QEMU_OPT_BOOL,
> >>          },
> >>          { /* end if list */ }
> >>      },
> >>@@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts)
> >>      const char *port;
> >>      char uaddr[INET6_ADDRSTRLEN+1];
> >>      char uport[33];
> >>-    int sock,rc;
> >>+    int sock, rc, err;
> >>+    bool block;
> >>
> >>      memset(&ai,0, sizeof(ai));
> >>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> >>@@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts)
> >>
> >>      addr = qemu_opt_get(opts, "host");
> >>      port = qemu_opt_get(opts, "port");
> >>+    block = qemu_opt_get_bool(opts, "block", 0);
> >>      if (addr == NULL || port == NULL) {
> >>          fprintf(stderr, "inet_connect: host and/or port not specified\n");
> >>          return -1;
> >>@@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts)
> >>              continue;
> >>          }
> >>          setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> >>-
> >>+        if (!block) {
> >>+            socket_set_nonblock(sock);
> >>+        }
> >>          /* connect to peer */
> >>-        if (connect(sock,e->ai_addr,e->ai_addrlen)<  0) {
> >>-            if (NULL == e->ai_next)
> >>-                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> >>-                        inet_strfamily(e->ai_family),
> >>-                        e->ai_canonname, uaddr, uport, strerror(errno));
> >>-            closesocket(sock);
> >>-            continue;
> >>+        do {
> >>+            err = 0;
> >>+            if (connect(sock, e->ai_addr, e->ai_addrlen)<  0) {
> >>+                err = -socket_error();
> 
> 
> connect error is got here
> 
> 
> >>+            }
> >>+#ifndef _WIN32
> >>+        } while (err == -EINTR || err == -EWOULDBLOCK);
> >>+#else
> >>+        } while (err == -EINTR);
> >>+#endif
> >>+
> >>+        if (err>= 0) {
> >>+            goto success;
> >>+        } else if (!block&&  err == -EINPROGRESS) {
> >>+            goto success;
> >>+#ifdef _WIN32
> >>+        } else if (!block&&  err == -WSAEALREADY) {
> >>+            goto success;
> >>+#endif
> >>          }
> >>-        freeaddrinfo(res);
> >>-        return sock;
> >>+
> >>+        if (NULL == e->ai_next) {
> >>+            fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> >>+                    inet_strfamily(e->ai_family),
> >>+                    e->ai_canonname, uaddr, uport, strerror(errno));
> >>+        }
> >>+        closesocket(sock);
> >>      }
> >>      freeaddrinfo(res);
> >>      return -1;
> >>+
> >>+success:
> >>+    freeaddrinfo(res);
> >>+    return sock;
> >>  }
> >>
> >>  int inet_dgram_opts(QemuOpts *opts)
> >>@@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen,
> >>      return sock;
> >>  }
> >>
> >>-int inet_connect(const char *str, int socktype)
> >>+int inet_connect(const char *str, bool block)
> >>  {
> >>      QemuOpts *opts;
> >>      int sock = -1;
> >>
> >>      opts = qemu_opts_create(&dummy_opts, NULL, 0);
> >>-    if (inet_parse(opts, str) == 0)
> >>+    if (inet_parse(opts, str) == 0) {
> >>+        if (block) {
> >>+            qemu_opt_set(opts, "block", "on");
> >>+        }
> >>          sock = inet_connect_opts(opts);
> >>+    }
> >>      qemu_opts_del(opts);
> >>      return sock;
> >>  }
> >>diff --git a/qemu_socket.h b/qemu_socket.h
> >>index a5d0a84..f73e26d 100644
> >>--- a/qemu_socket.h
> >>+++ b/qemu_socket.h
> >>@@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset);
> >>  int inet_listen(const char *str, char *ostr, int olen,
> >>                  int socktype, int port_offset);
> >>  int inet_connect_opts(QemuOpts *opts);
> >>-int inet_connect(const char *str, int socktype);
> >>+int inet_connect(const char *str, bool block);
> >>  int inet_dgram_opts(QemuOpts *opts);
> >>  const char *inet_strfamily(int family);
> >>
> >>diff --git a/ui/vnc.c b/ui/vnc.c
> >>index deb9ecd..4a96153 100644
> >>--- a/ui/vnc.c
> >>+++ b/ui/vnc.c
> >>@@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
> >>          if (strncmp(display, "unix:", 5) == 0)
> >>              vs->lsock = unix_connect(display+5);
> >>          else
> >>-            vs->lsock = inet_connect(display, SOCK_STREAM);
> >>+            vs->lsock = inet_connect(display, true);
> >>          if (-1 == vs->lsock) {
> >>              g_free(vs->display);
> >>              vs->display = NULL;
> >>
> >>
> >
> 
> -- 
> 			Amos.
>
Michael Roth - April 18, 2012, 2:38 p.m.
On Wed, Apr 18, 2012 at 10:49:02AM +0800, Amos Kong wrote:
> On 18/04/12 10:10, Michael Roth wrote:
> >On Tue, Apr 17, 2012 at 10:54:01PM +0800, Amos Kong wrote:
> >>Add a bool argument to inet_connect() to assign if set socket
> >>to block/nonblock, and delete original argument 'socktype'
> >>that is unused.
> >>
> >>Retry to connect when following errors are got:
> >>   -EINTR
> >>   -EWOULDBLOCK (win32)
> >>Connect's successful for nonblock socket when following
> >>errors are got, user should wait for connecting by select():
> >>   -EINPROGRESS
> >>   -WSAEALREADY (win32)
> >
> >How does the user access these? Via errno/socket_error()?
> 
> Yes, connect error is got by socket_error()
> 
> > I'm not sure
> >how that would work, I don't think we can rely on the value of errno
> >unless a function indicates an error, so for the case where we're
> >successful but have an EINPROGRESS, I don't think we have a way of
> >reliably obtaining that (errno value might be stale) except maybe
> >something like getsockopt().
> 
> >I think it would be better to just merge this with your patch #3, which
> >handles all this nicely via Error.
> 
> We use Error to pass meaningful error, but we need to process
> original socket error,
> which is got by socket_error().

Don't use Error as an indicator to check socket_error(). If we want to
pass back something akin to errno, it should be done via Error. Paolo
had some suggestions on how to do this, but I think the Errors you
introduced in patch #2 are sufficient for now. qmp_migrate() currently
just sets QERR_UNDEFINED_ERROR and ignores the actual value of
socket_error(), we only use it for DPRINTF(), so if you still want to
dump the output of socket_error()/errno to stderr, do it early in
inet_connect()/inet_listen() so we don't have to propagate anything
back tcp_start_outgoing_migration() other than **errp.

> 
> In qemu, we used socket_error() to get socket error in many place.
> If you are confused
> with how that would work, we need to fix socket_error. I don't want
> to fix this in the
> migration patchset.
> 
> What's your opinion?
> 
> >>Change nbd, vnc to use new interface.
> >>
> >>Signed-off-by: Amos Kong<akong@redhat.com>
> >>---
> >>  nbd.c          |    2 +-
> >>  qemu-sockets.c |   58 +++++++++++++++++++++++++++++++++++++++++++-------------
> >>  qemu_socket.h  |    2 +-
> >>  ui/vnc.c       |    2 +-
> >>  4 files changed, 48 insertions(+), 16 deletions(-)
> >>
> >>diff --git a/nbd.c b/nbd.c
> >>index 406e555..b4e68a9 100644
> >>--- a/nbd.c
> >>+++ b/nbd.c
> >>@@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
> >>
> >>  int tcp_socket_outgoing_spec(const char *address_and_port)
> >>  {
> >>-    return inet_connect(address_and_port, SOCK_STREAM);
> >>+    return inet_connect(address_and_port, true);
> >>  }
> >>
> >>  int tcp_socket_incoming(const char *address, uint16_t port)
> >>diff --git a/qemu-sockets.c b/qemu-sockets.c
> >>index 6bcb8e3..e886195 100644
> >>--- a/qemu-sockets.c
> >>+++ b/qemu-sockets.c
> >>@@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = {
> >>          },{
> >>              .name = "ipv6",
> >>              .type = QEMU_OPT_BOOL,
> >>+        },{
> >>+            .name = "block",
> >>+            .type = QEMU_OPT_BOOL,
> >>          },
> >>          { /* end if list */ }
> >>      },
> >>@@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts)
> >>      const char *port;
> >>      char uaddr[INET6_ADDRSTRLEN+1];
> >>      char uport[33];
> >>-    int sock,rc;
> >>+    int sock, rc, err;
> >>+    bool block;
> >>
> >>      memset(&ai,0, sizeof(ai));
> >>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> >>@@ -210,6 +214,7 @@ int inet_connect_opts(QemuOpts *opts)
> >>
> >>      addr = qemu_opt_get(opts, "host");
> >>      port = qemu_opt_get(opts, "port");
> >>+    block = qemu_opt_get_bool(opts, "block", 0);
> >>      if (addr == NULL || port == NULL) {
> >>          fprintf(stderr, "inet_connect: host and/or port not specified\n");
> >>          return -1;
> >>@@ -241,21 +246,44 @@ int inet_connect_opts(QemuOpts *opts)
> >>              continue;
> >>          }
> >>          setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> >>-
> >>+        if (!block) {
> >>+            socket_set_nonblock(sock);
> >>+        }
> >>          /* connect to peer */
> >>-        if (connect(sock,e->ai_addr,e->ai_addrlen)<  0) {
> >>-            if (NULL == e->ai_next)
> >>-                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> >>-                        inet_strfamily(e->ai_family),
> >>-                        e->ai_canonname, uaddr, uport, strerror(errno));
> >>-            closesocket(sock);
> >>-            continue;
> >>+        do {
> >>+            err = 0;
> >>+            if (connect(sock, e->ai_addr, e->ai_addrlen)<  0) {
> >>+                err = -socket_error();
> 
> 
> connect error is got here
> 
> 
> >>+            }
> >>+#ifndef _WIN32
> >>+        } while (err == -EINTR || err == -EWOULDBLOCK);
> >>+#else
> >>+        } while (err == -EINTR);
> >>+#endif
> >>+
> >>+        if (err>= 0) {
> >>+            goto success;
> >>+        } else if (!block&&  err == -EINPROGRESS) {
> >>+            goto success;
> >>+#ifdef _WIN32
> >>+        } else if (!block&&  err == -WSAEALREADY) {
> >>+            goto success;
> >>+#endif
> >>          }
> >>-        freeaddrinfo(res);
> >>-        return sock;
> >>+
> >>+        if (NULL == e->ai_next) {
> >>+            fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> >>+                    inet_strfamily(e->ai_family),
> >>+                    e->ai_canonname, uaddr, uport, strerror(errno));
> >>+        }
> >>+        closesocket(sock);
> >>      }
> >>      freeaddrinfo(res);
> >>      return -1;
> >>+
> >>+success:
> >>+    freeaddrinfo(res);
> >>+    return sock;
> >>  }
> >>
> >>  int inet_dgram_opts(QemuOpts *opts)
> >>@@ -449,14 +477,18 @@ int inet_listen(const char *str, char *ostr, int olen,
> >>      return sock;
> >>  }
> >>
> >>-int inet_connect(const char *str, int socktype)
> >>+int inet_connect(const char *str, bool block)
> >>  {
> >>      QemuOpts *opts;
> >>      int sock = -1;
> >>
> >>      opts = qemu_opts_create(&dummy_opts, NULL, 0);
> >>-    if (inet_parse(opts, str) == 0)
> >>+    if (inet_parse(opts, str) == 0) {
> >>+        if (block) {
> >>+            qemu_opt_set(opts, "block", "on");
> >>+        }
> >>          sock = inet_connect_opts(opts);
> >>+    }
> >>      qemu_opts_del(opts);
> >>      return sock;
> >>  }
> >>diff --git a/qemu_socket.h b/qemu_socket.h
> >>index a5d0a84..f73e26d 100644
> >>--- a/qemu_socket.h
> >>+++ b/qemu_socket.h
> >>@@ -41,7 +41,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset);
> >>  int inet_listen(const char *str, char *ostr, int olen,
> >>                  int socktype, int port_offset);
> >>  int inet_connect_opts(QemuOpts *opts);
> >>-int inet_connect(const char *str, int socktype);
> >>+int inet_connect(const char *str, bool block);
> >>  int inet_dgram_opts(QemuOpts *opts);
> >>  const char *inet_strfamily(int family);
> >>
> >>diff --git a/ui/vnc.c b/ui/vnc.c
> >>index deb9ecd..4a96153 100644
> >>--- a/ui/vnc.c
> >>+++ b/ui/vnc.c
> >>@@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
> >>          if (strncmp(display, "unix:", 5) == 0)
> >>              vs->lsock = unix_connect(display+5);
> >>          else
> >>-            vs->lsock = inet_connect(display, SOCK_STREAM);
> >>+            vs->lsock = inet_connect(display, true);
> >>          if (-1 == vs->lsock) {
> >>              g_free(vs->display);
> >>              vs->display = NULL;
> >>
> >>
> >
> 
> -- 
> 			Amos.
>

Patch

diff --git a/nbd.c b/nbd.c
index 406e555..b4e68a9 100644
--- a/nbd.c
+++ b/nbd.c
@@ -146,7 +146,7 @@  int tcp_socket_outgoing(const char *address, uint16_t port)
 
 int tcp_socket_outgoing_spec(const char *address_and_port)
 {
-    return inet_connect(address_and_port, SOCK_STREAM);
+    return inet_connect(address_and_port, true);
 }
 
 int tcp_socket_incoming(const char *address, uint16_t port)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 6bcb8e3..e886195 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -51,6 +51,9 @@  static QemuOptsList dummy_opts = {
         },{
             .name = "ipv6",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "block",
+            .type = QEMU_OPT_BOOL,
         },
         { /* end if list */ }
     },
@@ -201,7 +204,8 @@  int inet_connect_opts(QemuOpts *opts)
     const char *port;
     char uaddr[INET6_ADDRSTRLEN+1];
     char uport[33];
-    int sock,rc;
+    int sock, rc, err;
+    bool block;
 
     memset(&ai,0, sizeof(ai));
     ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
@@ -210,6 +214,7 @@  int inet_connect_opts(QemuOpts *opts)
 
     addr = qemu_opt_get(opts, "host");
     port = qemu_opt_get(opts, "port");
+    block = qemu_opt_get_bool(opts, "block", 0);
     if (addr == NULL || port == NULL) {
         fprintf(stderr, "inet_connect: host and/or port not specified\n");
         return -1;
@@ -241,21 +246,44 @@  int inet_connect_opts(QemuOpts *opts)
             continue;
         }
         setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
-
+        if (!block) {
+            socket_set_nonblock(sock);
+        }
         /* connect to peer */
-        if (connect(sock,e->ai_addr,e->ai_addrlen) < 0) {
-            if (NULL == e->ai_next)
-                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
-                        inet_strfamily(e->ai_family),
-                        e->ai_canonname, uaddr, uport, strerror(errno));
-            closesocket(sock);
-            continue;
+        do {
+            err = 0;
+            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
+                err = -socket_error();
+            }
+#ifndef _WIN32
+        } while (err == -EINTR || err == -EWOULDBLOCK);
+#else
+        } while (err == -EINTR);
+#endif
+
+        if (err >= 0) {
+            goto success;
+        } else if (!block && err == -EINPROGRESS) {
+            goto success;
+#ifdef _WIN32
+        } else if (!block && err == -WSAEALREADY) {
+            goto success;
+#endif
         }
-        freeaddrinfo(res);
-        return sock;
+
+        if (NULL == e->ai_next) {
+            fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
+                    inet_strfamily(e->ai_family),
+                    e->ai_canonname, uaddr, uport, strerror(errno));
+        }
+        closesocket(sock);
     }
     freeaddrinfo(res);
     return -1;
+
+success:
+    freeaddrinfo(res);
+    return sock;
 }
 
 int inet_dgram_opts(QemuOpts *opts)
@@ -449,14 +477,18 @@  int inet_listen(const char *str, char *ostr, int olen,
     return sock;
 }
 
-int inet_connect(const char *str, int socktype)
+int inet_connect(const char *str, bool block)
 {
     QemuOpts *opts;
     int sock = -1;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0);
-    if (inet_parse(opts, str) == 0)
+    if (inet_parse(opts, str) == 0) {
+        if (block) {
+            qemu_opt_set(opts, "block", "on");
+        }
         sock = inet_connect_opts(opts);
+    }
     qemu_opts_del(opts);
     return sock;
 }
diff --git a/qemu_socket.h b/qemu_socket.h
index a5d0a84..f73e26d 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -41,7 +41,7 @@  int inet_listen_opts(QemuOpts *opts, int port_offset);
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset);
 int inet_connect_opts(QemuOpts *opts);
-int inet_connect(const char *str, int socktype);
+int inet_connect(const char *str, bool block);
 int inet_dgram_opts(QemuOpts *opts);
 const char *inet_strfamily(int family);
 
diff --git a/ui/vnc.c b/ui/vnc.c
index deb9ecd..4a96153 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3068,7 +3068,7 @@  int vnc_display_open(DisplayState *ds, const char *display)
         if (strncmp(display, "unix:", 5) == 0)
             vs->lsock = unix_connect(display+5);
         else
-            vs->lsock = inet_connect(display, SOCK_STREAM);
+            vs->lsock = inet_connect(display, true);
         if (-1 == vs->lsock) {
             g_free(vs->display);
             vs->display = NULL;