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.

Attachment: signature.asc
Description: PGP signature

Reply via email to