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

[Michael Roth]

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<address@hidden>
> >>---
> >>  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.
>