[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag |
Date: |
Wed, 12 Oct 2016 17:11:37 +0200 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
On Fri, Oct 07, 2016 at 10:55:55AM -0500, Eric Blake wrote:
> 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().
Ok
> > @@ -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.
The code already assumed CONFIG_ACCEPT4 implies SOCK_CLOEXEC so I
checked linux.git and found it is true in mainline Linux. Of course
distros could do a crazy backport where only some of the feature is
backported...
> > @@ -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? ...
Sure.
signature.asc
Description: PGP signature