Re: [Qemu-devel] [PATCH v6 1/5] sockets: change inet_connect() to support nonblock socket

[Amos Kong]

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<ak...@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;
}

...

--
                        Amos.