qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag
@ 2016-10-07 13:54 Stefan Hajnoczi
  2016-10-07 15:55 ` Eric Blake
  2016-10-10 19:05 ` no-reply
  0 siblings, 2 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2016-10-07 13:54 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, eblake, Gerd Hoffmann, Daniel Berrange,
	Stefan Hajnoczi

The socket(2) and accept(2) syscalls have been extended to take flags
that affect the socket atomically at creation time.  This not only
avoids the overhead of additional system calls but also closes the race
condition with forking threads.

This patch adds support for the SOCK_NONBLOCK flag.  QEMU already
implements the SOCK_CLOEXEC flag.

All qemu_socket() and qemu_accept() callers are updated to pass the new
flags argument.  Callers that later use qemu_set_nonblock() are
refactored as follows:

  fd = qemu_socket(...) or qemu_accept(...);
  ...
  qemu_set_nonblock(fd);

Becomes:

  fd = qemu_socket(..., QEMU_SOCK_NONBLOCK) or
       qemu_accept(..., QEMU_SOCK_NONBLOCK);

For full details on SOCK_NONBLOCK in the POSIX spec see:
http://austingroupbugs.net/view.php?id=411

Note that slirp code violates the coding style with a mix of tabs and
space indentation.  This patch passes checkpatch.pl but the diff may
appear odd due to the mixed indentation in slirp code.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/ivshmem-server/ivshmem-server.c |  4 ++--
 include/qemu/sockets.h                  |  9 ++++++--
 io/channel-socket.c                     |  2 +-
 net/socket.c                            |  8 +++----
 qga/channel-posix.c                     |  4 ++--
 slirp/ip_icmp.c                         |  2 +-
 slirp/misc.c                            |  9 ++++----
 slirp/socket.c                          |  3 ++-
 slirp/tcp_subr.c                        |  7 +++---
 slirp/udp.c                             |  4 ++--
 util/osdep.c                            | 30 +++++++++++++++++++++-----
 util/qemu-sockets.c                     | 38 +++++++++++++++++++--------------
 12 files changed, 75 insertions(+), 45 deletions(-)

diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c
index e2f295b..be96841 100644
--- a/contrib/ivshmem-server/ivshmem-server.c
+++ b/contrib/ivshmem-server/ivshmem-server.c
@@ -140,14 +140,14 @@ ivshmem_server_handle_new_conn(IvshmemServer *server)
     /* accept the incoming connection */
     unaddr_len = sizeof(unaddr);
     newfd = qemu_accept(server->sock_fd,
-                        (struct sockaddr *)&unaddr, &unaddr_len);
+                        (struct sockaddr *)&unaddr, &unaddr_len,
+                        QEMU_SOCK_NONBLOCK);
 
     if (newfd < 0) {
         IVSHMEM_SERVER_DEBUG(server, "cannot accept() %s\n", strerror(errno));
         return -1;
     }
 
-    qemu_set_nonblock(newfd);
     IVSHMEM_SERVER_DEBUG(server, "accept()=%d\n", newfd);
 
     /* allocate new structure for this peer */
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 9eb2470..d0e543f 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -11,9 +11,14 @@ int inet_aton(const char *cp, struct in_addr *ia);
 
 #include "qapi-types.h"
 
+typedef enum {
+    QEMU_SOCK_NONBLOCK = 1,
+} QemuSockFlags;
+
 /* misc helpers */
-int qemu_socket(int domain, int type, int protocol);
-int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
+int qemu_socket(int domain, int type, int protocol, QemuSockFlags flags);
+int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen,
+                QemuSockFlags flags);
 int socket_set_cork(int fd, int v);
 int socket_set_nodelay(int fd);
 void qemu_set_block(int fd);
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 196a4f1..428cea5 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -362,7 +362,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
  retry:
     trace_qio_channel_socket_accept(ioc);
     cioc->fd = qemu_accept(ioc->fd, (struct sockaddr *)&cioc->remoteAddr,
-                           &cioc->remoteAddrLen);
+                           &cioc->remoteAddrLen, 0);
     if (cioc->fd < 0) {
         trace_qio_channel_socket_accept_fail(ioc);
         if (errno == EINTR) {
diff --git a/net/socket.c b/net/socket.c
index 982c8de..f9379e3 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -228,7 +228,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
         return -1;
 
     }
-    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
+    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0, QEMU_SOCK_NONBLOCK);
     if (fd < 0) {
         perror("socket(PF_INET, SOCK_DGRAM)");
         return -1;
@@ -286,7 +286,6 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
         }
     }
 
-    qemu_set_nonblock(fd);
     return fd;
 fail:
     if (fd >= 0)
@@ -465,7 +464,7 @@ static void net_socket_accept(void *opaque)
 
     for(;;) {
         len = sizeof(saddr);
-        fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len);
+        fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len, 0);
         if (fd < 0 && errno != EINTR) {
             return;
         } else if (fd >= 0) {
@@ -647,7 +646,7 @@ static int net_socket_udp_init(NetClientState *peer,
         return -1;
     }
 
-    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
+    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0, QEMU_SOCK_NONBLOCK);
     if (fd < 0) {
         perror("socket(PF_INET, SOCK_DGRAM)");
         return -1;
@@ -664,7 +663,6 @@ static int net_socket_udp_init(NetClientState *peer,
         closesocket(fd);
         return -1;
     }
-    qemu_set_nonblock(fd);
 
     s = net_socket_fd_init(peer, model, name, fd, 0);
     if (!s) {
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index bb65d8b..0379b5d 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -32,12 +32,12 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
     g_assert(channel != NULL);
 
     client_fd = qemu_accept(g_io_channel_unix_get_fd(channel),
-                            (struct sockaddr *)&addr, &addrlen);
+                            (struct sockaddr *)&addr, &addrlen,
+                            QEMU_SOCK_NONBLOCK);
     if (client_fd == -1) {
         g_warning("error converting fd to gsocket: %s", strerror(errno));
         goto out;
     }
-    qemu_set_nonblock(client_fd);
     ret = ga_channel_client_add(c, client_fd);
     if (ret) {
         g_warning("error setting up connection");
diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index 5ffc7a6..99654ed 100644
--- a/slirp/ip_icmp.c
+++ b/slirp/ip_icmp.c
@@ -79,7 +79,7 @@ static int icmp_send(struct socket *so, struct mbuf *m, int hlen)
     struct ip *ip = mtod(m, struct ip *);
     struct sockaddr_in addr;
 
-    so->s = qemu_socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP);
+    so->s = qemu_socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP, 0);
     if (so->s == -1) {
         return -1;
     }
diff --git a/slirp/misc.c b/slirp/misc.c
index 88e9d94..fc0282c 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -108,7 +108,8 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 		addr.sin_port = 0;
 		addr.sin_addr.s_addr = INADDR_ANY;
 
-		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
+        s = qemu_socket(AF_INET, SOCK_STREAM, 0, 0);
+        if (s < 0 ||
 		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
 		    listen(s, 1) < 0) {
 			error_report("Error: inet socket: %s", strerror(errno));
@@ -135,7 +136,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                  * Connect to the socket
                  * XXX If any of these fail, we're in trouble!
                  */
-                s = qemu_socket(AF_INET, SOCK_STREAM, 0);
+                s = qemu_socket(AF_INET, SOCK_STREAM, 0, 0);
                 addr.sin_addr = loopback_addr;
                 do {
                     ret = connect(s, (struct sockaddr *)&addr, addrlen);
@@ -184,13 +185,13 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                  * of connect() fail in the child process
                  */
                 do {
-                    so->s = accept(s, (struct sockaddr *)&addr, &addrlen);
+                    so->s = qemu_accept(s, (struct sockaddr *)&addr, &addrlen,
+                                        QEMU_SOCK_NONBLOCK);
                 } while (so->s < 0 && errno == EINTR);
                 closesocket(s);
                 socket_set_fast_reuse(so->s);
                 opt = 1;
                 qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
-		qemu_set_nonblock(so->s);
 
 		/* Append the telnet options now */
                 if (so->so_m != NULL && do_pty == 1)  {
diff --git a/slirp/socket.c b/slirp/socket.c
index 280050a..ffe1d70 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -690,7 +690,8 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	addr.sin_addr.s_addr = haddr;
 	addr.sin_port = hport;
 
-	if (((s = qemu_socket(AF_INET,SOCK_STREAM,0)) < 0) ||
+    s = qemu_socket(AF_INET, SOCK_STREAM, 0, 0);
+    if ((s < 0) ||
 	    (socket_set_fast_reuse(s) < 0) ||
 	    (bind(s,(struct sockaddr *)&addr, sizeof(addr)) < 0) ||
 	    (listen(s,1) < 0)) {
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index ed16e18..026368b 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -398,12 +398,11 @@ int tcp_fconnect(struct socket *so, unsigned short af)
   DEBUG_CALL("tcp_fconnect");
   DEBUG_ARG("so = %p", so);
 
-  ret = so->s = qemu_socket(af, SOCK_STREAM, 0);
+  ret = so->s = qemu_socket(af, SOCK_STREAM, 0, QEMU_SOCK_NONBLOCK);
   if (ret >= 0) {
     int opt, s=so->s;
     struct sockaddr_storage addr;
 
-    qemu_set_nonblock(s);
     socket_set_fast_reuse(s);
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(opt));
@@ -473,12 +472,12 @@ void tcp_connect(struct socket *inso)
 
     tcp_mss(sototcpcb(so), 0);
 
-    s = accept(inso->s, (struct sockaddr *)&addr, &addrlen);
+    s = qemu_accept(inso->s, (struct sockaddr *)&addr, &addrlen,
+                    QEMU_SOCK_NONBLOCK);
     if (s < 0) {
         tcp_close(sototcpcb(so)); /* This will sofree() as well */
         return;
     }
-    qemu_set_nonblock(s);
     socket_set_fast_reuse(s);
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
diff --git a/slirp/udp.c b/slirp/udp.c
index 93d7224..e377c16 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -285,7 +285,7 @@ int udp_output(struct socket *so, struct mbuf *m,
 int
 udp_attach(struct socket *so, unsigned short af)
 {
-  so->s = qemu_socket(af, SOCK_DGRAM, 0);
+  so->s = qemu_socket(af, SOCK_DGRAM, 0, 0);
   if (so->s != -1) {
     so->so_expire = curtime + SO_EXPIRE;
     insque(so, &so->slirp->udb);
@@ -334,7 +334,7 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	if (!so) {
 	    return NULL;
 	}
-	so->s = qemu_socket(AF_INET,SOCK_DGRAM,0);
+    so->s = qemu_socket(AF_INET, SOCK_DGRAM, 0, 0);
 	so->so_expire = curtime + SO_EXPIRE;
 	insque(so, &slirp->udb);
 
diff --git a/util/osdep.c b/util/osdep.c
index 06fb1cf..38019e2 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -267,12 +267,21 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count)
 /*
  * Opens a socket with FD_CLOEXEC set
  */
-int qemu_socket(int domain, int type, int protocol)
+int qemu_socket(int domain, int type, int protocol, QemuSockFlags flags)
 {
     int ret;
 
-#ifdef SOCK_CLOEXEC
-    ret = socket(domain, type | SOCK_CLOEXEC, protocol);
+    /* Require both SOCK_CLOEXEC and SOCK_NONBLOCK to avoid additional cases
+     * with only one defined.  Both were added to POSIX in Austin Group #411 so
+     * chances are good they come in a pair.
+     */
+#if defined(SOCK_CLOEXEC) && defined(SOCK_NONBLOCK)
+    int new_type = type | SOCK_CLOEXEC;
+    if (flags & QEMU_SOCK_NONBLOCK) {
+        new_type |= SOCK_NONBLOCK;
+    }
+
+    ret = socket(domain, new_type, protocol);
     if (ret != -1 || errno != EINVAL) {
         return ret;
     }
@@ -280,6 +289,9 @@ int qemu_socket(int domain, int type, int protocol)
     ret = socket(domain, type, protocol);
     if (ret >= 0) {
         qemu_set_cloexec(ret);
+        if (flags & QEMU_SOCK_NONBLOCK) {
+            qemu_set_nonblock(ret);
+        }
     }
 
     return ret;
@@ -288,12 +300,17 @@ int qemu_socket(int domain, int type, int protocol)
 /*
  * Accept a connection and set FD_CLOEXEC
  */
-int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen)
+int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen,
+                QemuSockFlags flags)
 {
     int ret;
 
 #ifdef CONFIG_ACCEPT4
-    ret = accept4(s, addr, addrlen, SOCK_CLOEXEC);
+    int accept_flags = SOCK_CLOEXEC;
+    if (flags & QEMU_SOCK_NONBLOCK) {
+        accept_flags |= SOCK_NONBLOCK;
+    }
+    ret = accept4(s, addr, addrlen, accept_flags);
     if (ret != -1 || errno != ENOSYS) {
         return ret;
     }
@@ -301,6 +318,9 @@ int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen)
     ret = accept(s, addr, addrlen);
     if (ret >= 0) {
         qemu_set_cloexec(ret);
+        if (flags & QEMU_SOCK_NONBLOCK) {
+            qemu_set_nonblock(ret);
+        }
     }
 
     return ret;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 6db48b3..1e0d4af 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -183,7 +183,8 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
 		        uaddr,INET6_ADDRSTRLEN,uport,32,
 		        NI_NUMERICHOST | NI_NUMERICSERV);
-        slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
+        slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol,
+                              0);
         if (slisten < 0) {
             if (!e->ai_next) {
                 error_setg_errno(errp, errno, "Failed to create socket");
@@ -317,18 +318,20 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
                              ConnectState *connect_state, Error **errp)
 {
     int sock, rc;
+    QemuSockFlags flags = 0;
 
     *in_progress = false;
 
-    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
-    if (sock < 0) {
-        error_setg_errno(errp, errno, "Failed to create socket");
-        return -1;
-    }
-    socket_set_fast_reuse(sock);
     if (connect_state != NULL) {
-        qemu_set_nonblock(sock);
+        flags = QEMU_SOCK_NONBLOCK;
     }
+    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol,
+                       flags);
+    if (sock < 0) {
+        error_setg_errno(errp, errno, "Failed to create socket");
+        return -1;
+    }
+    socket_set_fast_reuse(sock);
     /* connect to peer */
     do {
         rc = 0;
@@ -524,7 +527,8 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
     }
 
     /* create socket */
-    sock = qemu_socket(peer->ai_family, peer->ai_socktype, peer->ai_protocol);
+    sock = qemu_socket(peer->ai_family, peer->ai_socktype, peer->ai_protocol,
+                       0);
     if (sock < 0) {
         error_setg_errno(errp, errno, "Failed to create socket");
         goto err;
@@ -659,7 +663,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     struct sockaddr_un un;
     int sock, fd;
 
-    sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
+    sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0, 0);
     if (sock < 0) {
         error_setg_errno(errp, errno, "Failed to create Unix socket");
         return -1;
@@ -725,6 +729,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp,
 {
     struct sockaddr_un un;
     ConnectState *connect_state = NULL;
+    QemuSockFlags flags = 0;
     int sock, rc;
 
     if (saddr->path == NULL) {
@@ -732,16 +737,16 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp,
         return -1;
     }
 
-    sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
-    if (sock < 0) {
-        error_setg_errno(errp, errno, "Failed to create socket");
-        return -1;
-    }
     if (callback != NULL) {
         connect_state = g_malloc0(sizeof(*connect_state));
         connect_state->callback = callback;
         connect_state->opaque = opaque;
-        qemu_set_nonblock(sock);
+        flags |= QEMU_SOCK_NONBLOCK;
+    }
+    sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0, flags);
+    if (sock < 0) {
+        error_setg_errno(errp, errno, "Failed to create socket");
+        goto out;
     }
 
     memset(&un, 0, sizeof(un));
@@ -773,6 +778,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp,
         sock = -1;
     }
 
+out:
     g_free(connect_state);
     return sock;
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-10-12 15:11 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-07 13:54 [Qemu-devel] [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag Stefan Hajnoczi
2016-10-07 15:55 ` Eric Blake
2016-10-12 15:11   ` Stefan Hajnoczi
2016-10-10 19:05 ` no-reply

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).