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 <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().
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
