From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47609) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsXVB-0004Cg-TO for qemu-devel@nongnu.org; Fri, 07 Oct 2016 11:56:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsXV8-0000h7-L4 for qemu-devel@nongnu.org; Fri, 07 Oct 2016 11:56:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49646) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsXV8-0000h1-B1 for qemu-devel@nongnu.org; Fri, 07 Oct 2016 11:55:58 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6DCDFC0467F1 for ; Fri, 7 Oct 2016 15:55:57 +0000 (UTC) References: <1475848458-1285-1-git-send-email-stefanha@redhat.com> From: Eric Blake Message-ID: Date: Fri, 7 Oct 2016 10:55:55 -0500 MIME-Version: 1.0 In-Reply-To: <1475848458-1285-1-git-send-email-stefanha@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="U5LR0OPp27fAsTjbxubEcX8cpcsRqbCmO" Subject: Re: [Qemu-devel] [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , qemu-devel@nongnu.org Cc: Paolo Bonzini , Gerd Hoffmann , Daniel Berrange This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --U5LR0OPp27fAsTjbxubEcX8cpcsRqbCmO From: Eric Blake To: Stefan Hajnoczi , qemu-devel@nongnu.org Cc: Paolo Bonzini , Gerd Hoffmann , Daniel Berrange Message-ID: Subject: Re: [PATCH] socket: add atomic QEMU_SOCK_NONBLOCK flag References: <1475848458-1285-1-git-send-email-stefanha@redhat.com> In-Reply-To: <1475848458-1285-1-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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). >=20 > 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: >=20 > fd =3D qemu_socket(...) or qemu_accept(...); > ... > qemu_set_nonblock(fd); >=20 > Becomes: >=20 > fd =3D qemu_socket(..., QEMU_SOCK_NONBLOCK) or > qemu_accept(..., QEMU_SOCK_NONBLOCK); >=20 > For full details on SOCK_NONBLOCK in the POSIX spec see: > http://austingroupbugs.net/view.php?id=3D411 /me that looks strangely familiar... :) >=20 > 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. >=20 > Suggested-by: Eric Blake > Signed-off-by: Stefan Hajnoczi > --- > +++ b/include/qemu/sockets.h > @@ -11,9 +11,14 @@ int inet_aton(const char *cp, struct in_addr *ia); > =20 > #include "qapi-types.h" > =20 > +typedef enum { > + QEMU_SOCK_NONBLOCK =3D 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 =3D accept(s, (struct sockaddr *)&addr, &add= rlen); > + so->s =3D 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 flag= s) > { > int ret; > =20 > -#ifdef SOCK_CLOEXEC > - ret =3D 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 Grou= p #411 so > + * chances are good they come in a pair. Indeed. > @@ -288,12 +300,17 @@ int qemu_socket(int domain, int type, int protoco= l) > /* > * 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; > =20 > #ifdef CONFIG_ACCEPT4 > - ret =3D accept4(s, addr, addrlen, SOCK_CLOEXEC); > + int accept_flags =3D SOCK_CLOEXEC; > + if (flags & QEMU_SOCK_NONBLOCK) { > + accept_flags |=3D 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 *add= r, bool *in_progress, > ConnectState *connect_state, Error **errp= ) > { > int sock, rc; > + QemuSockFlags flags =3D 0; > =20 > *in_progress =3D false; > =20 > - sock =3D 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 !=3D NULL) { > - qemu_set_nonblock(sock); > + flags =3D QEMU_SOCK_NONBLOCK; Use |=3D here? ... > @@ -732,16 +737,16 @@ static int unix_connect_saddr(UnixSocketAddress *= saddr, Error **errp, > return -1; > } > =20 > - sock =3D qemu_socket(PF_UNIX, SOCK_STREAM, 0); > - if (sock < 0) { > - error_setg_errno(errp, errno, "Failed to create socket"); > - return -1; > - } > if (callback !=3D NULL) { > connect_state =3D g_malloc0(sizeof(*connect_state)); > connect_state->callback =3D callback; > connect_state->opaque =3D opaque; > - qemu_set_nonblock(sock); > + flags |=3D QEMU_SOCK_NONBLOCK; > + } =2E..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 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --U5LR0OPp27fAsTjbxubEcX8cpcsRqbCmO Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJX98WLAAoJEKeha0olJ0NqYYQH/A09/TiWenM4TOupBatUKn76 OEEeRt1hBBe37NPLWjVXjuT+mwpItAHLmVUc1i5M53wYdqeWLGVjhuJz0cAKm7aL bTxpT8cSex+qaIL3pH9o4e9Y5eBzxIaHnrEYZCAQNByeK2PTnFDEp4bm6zgyi9rV D+MhgfHhNA1su8S5GRCORiblI/gXwpHNtnKc5WTE1cLozdzQS8du0f63pm2S1p8a wcXOmr1JMEmimXUKYsNWSM3c097aOwa5Kyx0JwRo9cIT1qsqsBOK1BneLSOXHmtQ Jn07cfVimlmOUx/CSklvwAN4HkuUIvl4wjmNFU66xWmtQck/LY2boYVutkeqUb4= =IiN0 -----END PGP SIGNATURE----- --U5LR0OPp27fAsTjbxubEcX8cpcsRqbCmO--