On 10/07/2016 08:54 AM, Stefan Hajnoczi wrote: > 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.
Atomic where supported by the OS, racy fallback on older systems, but not the end of the world (and our already-existing fallback is already racy, where the SOCK_CLOEXEC race is more likely to affect a multithreaded forking app, while SOCK_NONBLOCK is just there for convenience). > > 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 /me that looks strangely familiar... :) > > 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 <ebl...@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > --- > +++ 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; Good, since we can't rely on SOCK_NONBLOCK being defined everywhere yet. > --- a/slirp/misc.c > +++ b/slirp/misc.c > @@ -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); Silent bug fix here and elsewhere that we now set CLOEXEC where we previously didn't. Probably worth mentioning in the commit message that you fixed a couple of places that used accept() instead of the proper qemu_accept(). > +++ 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. Indeed. > @@ -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; > + } You're also (implicitly) assuming that CONFIG_ACCEPT4 implies both SOCK_CLOEXEC and SOCK_NONBLOCK; again likely to be true. > @@ -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; Use |= here? ... > @@ -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; > + } ...to match how you did it here? (No current semantic difference, but might lead to future maintenance problems if further additions aren't careful.) I found a minor nit and some suggested commit message improvements, but the patch itself is sane enough that I'm okay if you add: Reviewed-by: Eric Blake <ebl...@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
