[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag |
Date: |
Fri, 7 Oct 2016 10:55:55 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 |
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 <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> +++ 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 <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature