Re: [Qemu-devel] [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5577 bytes --]

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 <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@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 <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]