Re: [PATCH] Assume that accept4 is available

[Florian Weimer <fweimer at redhat dot com>]

On 04/17/2017 05:01 PM, Adhemerval Zanella wrote:
> > +  {
> > +    socklen_t sa_len = sizeof (server_address);
> > +    xgetsockname (server_socket, (struct sockaddr *) &server_address,
> > +                  &sa_len);
> > +    TEST_VERIFY (sa_len == sizeof (server_address));
> > +  }

> Why the brackets? I usually seems this unnecessary.

I prefer to restrict the scope of local variables if possible.

> Now that we are assuming accept4 support regardless, I think
> we can simplify it even more and just have either
> __ASSUME_ACCEPT4_SYSCALL or __ASSUME_SOCKETCALL
> (which would imply __ASSUME_ACCEPT4_SYSCALL_WITH_SOCKETCALL).

We currently need all those because some override headers include 
sysdeps/unix/sysv/linux/kernel-features.h before their own definitions, 
others after.  Rectifying that is a bit involved, and I don't want to do 
it with this change.

I believe I have addressed your other suggestions with the attached patch.

Thanks,
Florian

Assume that accept4 is always available and works

Simplify the Linux accept4 implementation based on the assumption
that it is available in some way.  __ASSUME_ACCEPT4_SOCKETCALL was
previously unused, so remove it.

For ia64, the accept4 system call (and socket call) were backported
in kernel version 3.2.18.  Reflect this in the installation
instructions.

2017-04-18  Florian Weimer  <fweimer@redhat.com>

	* nscd/connections.c (have_accept4): Removed definition.
	(nscd_run_worker, main_loop_poll, main_loop_epolll): Assume that
	accept4 works.
	* manual/install.texi (Linux): Require at least kernel 3.2.18 for
	ia64 because that was the first version with accept4 support.
	* support/Makefile (libsupport-routines): Add xaccept4.
	* support/xsocket.h (xaccept4): Declare.
	* support/xaccept4.c: New file.
	* socket/tst-accept4.c: New file.
	* socket/Makefile (tests): Add tst-accept4.
	* sysdeps/mach/hurd/kernel-features.h (__ASSUME_ACCEPT4): Remove
	definition.
	* sysdeps/nacl/kernel-features.h (__ASSUME_ACCEPT4): Remove
	comment.
	* sysdeps/unix/sysv/linux/accept4.c: Assume that an accept4
	implementation is available.
	(accept4): Use the system call if available, otherwise use the
	socket call.
	* sysdeps/unix/sysv/linux/ia64/configure.ac (arch_minimum_kernel):
	Set to 3.2.18.
	* sysdeps/unix/sysv/linux/ia64/kernel-features.h
	(__ASSUME_ACCEPT4_SYSCALL, __ASSUME_ACCEPT4): Do not undefine.
	accept4 is unconditionally available in later 3.2 stable kernels.
	(__ASSUME_ACCEPT4_SYSCALL): Define.
	* sysdeps/unix/sysv/linux/kernel-features.h
	(__ASSUME_ACCEPT4_SOCKETCALL, __ASSUME_ACCEPT4): Remove
	definitions.
	* sysdeps/unix/sysv/linux/i386/kernel-features.h
	(__ASSUME_ACCEPT4_SYSCALL): Define for Linux 4.3 or later.

diff --git a/INSTALL b/INSTALL
index 60f714e..920c4df 100644
--- a/INSTALL
+++ b/INSTALL
@@ -501,13 +501,15 @@ Specific advice for GNU/Linux systems
 
 If you are installing the GNU C Library on GNU/Linux systems, you need
 to have the header files from a 3.2 or newer kernel around for
-reference.  These headers must be installed using 'make
-headers_install'; the headers present in the kernel source directory are
-not suitable for direct use by the GNU C Library.  You do not need to
-use that kernel, just have its headers installed where the GNU C Library
-can access them, referred to here as INSTALL-DIRECTORY.  The easiest way
-to do this is to unpack it in a directory such as
-'/usr/src/linux-VERSION'.  In that directory, run 'make headers_install
+reference.  (For the ia64 architecture, you need version 3.2.18 or newer
+because this is the first version with support for the 'accept4' system
+call.)  These headers must be installed using 'make headers_install';
+the headers present in the kernel source directory are not suitable for
+direct use by the GNU C Library.  You do not need to use that kernel,
+just have its headers installed where the GNU C Library can access them,
+referred to here as INSTALL-DIRECTORY.  The easiest way to do this is to
+unpack it in a directory such as '/usr/src/linux-VERSION'.  In that
+directory, run 'make headers_install
 INSTALL_HDR_PATH=INSTALL-DIRECTORY'.  Finally, configure the GNU C
 Library with the option '--with-headers=INSTALL-DIRECTORY/include'.  Use
 the most recent kernel you can get your hands on.  (If you are
diff --git a/manual/install.texi b/manual/install.texi
index 99397c2..d39d2da 100644
--- a/manual/install.texi
+++ b/manual/install.texi
@@ -566,6 +566,8 @@ patches, although we try to avoid this.
 
 If you are installing @theglibc{} on @gnulinuxsystems{}, you need to have
 the header files from a 3.2 or newer kernel around for reference.
+(For the ia64 architecture, you need version 3.2.18 or newer because this
+is the first version with support for the @code{accept4} system call.)
 These headers must be installed using @samp{make headers_install}; the
 headers present in the kernel source directory are not suitable for
 direct use by @theglibc{}.  You do not need to use that kernel, just have
diff --git a/nscd/connections.c b/nscd/connections.c
index a5ca57a..cc1ed72 100644
--- a/nscd/connections.c
+++ b/nscd/connections.c
@@ -257,10 +257,6 @@ int inotify_fd = -1;
 static int nl_status_fd = -1;
 #endif
 
-#ifndef __ASSUME_ACCEPT4
-static int have_accept4;
-#endif
-
 /* Number of times clients had to wait.  */
 unsigned long int client_queued;
 
@@ -1650,16 +1646,6 @@ nscd_run_worker (void *p)
       /* We are done with the list.  */
       pthread_mutex_unlock (&readylist_lock);
 
-#ifndef __ASSUME_ACCEPT4
-      if (have_accept4 < 0)
-	{
-	  /* We do not want to block on a short read or so.  */
-	  int fl = fcntl (fd, F_GETFL);
-	  if (fl == -1 || fcntl (fd, F_SETFL, fl | O_NONBLOCK) == -1)
-	    goto close_and_out;
-	}
-#endif
-
       /* Now read the request.  */
       request_header req;
       if (__builtin_expect (TEMP_FAILURE_RETRY (read (fd, &req, sizeof (req)))
@@ -2099,24 +2085,8 @@ main_loop_poll (void)
 	  if (conns[0].revents != 0)
 	    {
 	      /* We have a new incoming connection.  Accept the connection.  */
-	      int fd;
-
-#ifndef __ASSUME_ACCEPT4
-	      fd = -1;
-	      if (have_accept4 >= 0)
-#endif
-		{
-		  fd = TEMP_FAILURE_RETRY (accept4 (sock, NULL, NULL,
+	      int fd = TEMP_FAILURE_RETRY (accept4 (sock, NULL, NULL,
 						    SOCK_NONBLOCK));
-#ifndef __ASSUME_ACCEPT4
-		  if (have_accept4 == 0)
-		    have_accept4 = fd != -1 || errno != ENOSYS ? 1 : -1;
-#endif
-		}
-#ifndef __ASSUME_ACCEPT4
-	      if (have_accept4 < 0)
-		fd = TEMP_FAILURE_RETRY (accept (sock, NULL, NULL));
-#endif
 
 	      /* Use the descriptor if we have not reached the limit.  */
 	      if (fd >= 0)
@@ -2284,24 +2254,8 @@ main_loop_epoll (int efd)
 	if (revs[cnt].data.fd == sock)
 	  {
 	    /* A new connection.  */
-	    int fd;
-
-# ifndef __ASSUME_ACCEPT4
-	    fd = -1;
-	    if (have_accept4 >= 0)
-# endif
-	      {
-		fd = TEMP_FAILURE_RETRY (accept4 (sock, NULL, NULL,
+	    int fd = TEMP_FAILURE_RETRY (accept4 (sock, NULL, NULL,
 						  SOCK_NONBLOCK));
-# ifndef __ASSUME_ACCEPT4
-		if (have_accept4 == 0)
-		  have_accept4 = fd != -1 || errno != ENOSYS ? 1 : -1;
-# endif
-	      }
-# ifndef __ASSUME_ACCEPT4
-	    if (have_accept4 < 0)
-	      fd = TEMP_FAILURE_RETRY (accept (sock, NULL, NULL));
-# endif
 
 	    /* Use the descriptor if we have not reached the limit.  */
 	    if (fd >= 0)
diff --git a/socket/Makefile b/socket/Makefile
index 25d4f68..1e2555d 100644
--- a/socket/Makefile
+++ b/socket/Makefile
@@ -31,6 +31,8 @@ routines := accept bind connect getpeername getsockname getsockopt	\
 	    setsockopt shutdown socket socketpair isfdtype opensock	\
 	    sockatmark accept4 recvmmsg sendmmsg
 
+tests := tst-accept4
+
 aux	 := sa_len
 
 include ../Rules
diff --git a/socket/tst-accept4.c b/socket/tst-accept4.c
new file mode 100644
index 0000000..a5cf3a2
--- /dev/null
+++ b/socket/tst-accept4.c
@@ -0,0 +1,131 @@
+/* Test the accept4 function with differing flags arguments.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <arpa/inet.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <support/check.h>
+#include <support/xsocket.h>
+#include <support/xunistd.h>
+#include <sys/socket.h>
+
+static bool
+is_nonblocking (int fd)
+{
+  int status = fcntl (fd, F_GETFL);
+  if (status < 0)
+    FAIL_EXIT1 ("fcntl (F_GETFL): %m");
+  return status & O_NONBLOCK;
+}
+
+static bool
+is_cloexec (int fd)
+{
+  int status = fcntl (fd, F_GETFD);
+  if (status < 0)
+    FAIL_EXIT1 ("fcntl (F_GETFD): %m");
+  return status & FD_CLOEXEC;
+}
+
+struct client
+{
+  int socket;
+  struct sockaddr_in address;
+};
+
+/* Perform a non-blocking connect to *SERVER_ADDRESS.  */
+static struct client
+client_connect (const struct sockaddr_in *server_address)
+{
+  struct client result;
+  result.socket = xsocket (AF_INET,
+                           SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC, 0);
+  TEST_VERIFY (is_nonblocking (result.socket));
+  TEST_VERIFY (is_cloexec (result.socket));
+  int ret = connect (result.socket, (const struct sockaddr *) server_address,
+                     sizeof (*server_address));
+  if (ret < 0 && errno != EINPROGRESS)
+    FAIL_EXIT1 ("client connect: %m");
+  socklen_t sa_len = sizeof (result.address);
+  xgetsockname (result.socket, (struct sockaddr *) &result.address,
+                &sa_len);
+  TEST_VERIFY (sa_len == sizeof (result.address));
+  return result;
+}
+
+static void
+check_same_address (const struct sockaddr_in *left,
+                    const struct sockaddr_in *right)
+{
+  TEST_VERIFY (left->sin_family == AF_INET);
+  TEST_VERIFY (right->sin_family == AF_INET);
+  TEST_VERIFY (left->sin_addr.s_addr == right->sin_addr.s_addr);
+  TEST_VERIFY (left->sin_port == right->sin_port);
+}
+
+static int
+do_test (void)
+{
+  /* Create server socket.  */
+  int server_socket = xsocket (AF_INET, SOCK_STREAM, 0);
+  TEST_VERIFY (!is_nonblocking (server_socket));
+  TEST_VERIFY (!is_cloexec (server_socket));
+  struct sockaddr_in server_address =
+    {
+      .sin_family = AF_INET,
+      .sin_addr = {.s_addr = htonl (INADDR_LOOPBACK) },
+    };
+  xbind (server_socket,
+         (struct sockaddr *) &server_address, sizeof (server_address));
+  {
+    socklen_t sa_len = sizeof (server_address);
+    xgetsockname (server_socket, (struct sockaddr *) &server_address,
+                  &sa_len);
+    TEST_VERIFY (sa_len == sizeof (server_address));
+  }
+  xlisten (server_socket, 5);
+
+  for (int do_nonblock = 0; do_nonblock < 2; ++do_nonblock)
+    for (int do_cloexec = 0; do_cloexec < 2; ++do_cloexec)
+      {
+        int sockflags = 0;
+        if (do_nonblock)
+          sockflags |= SOCK_NONBLOCK;
+        if (do_cloexec)
+          sockflags |= SOCK_CLOEXEC;
+
+        struct client client = client_connect (&server_address);
+        struct sockaddr_in client_address;
+        socklen_t sa_len = sizeof (client_address);
+        int client_socket = xaccept4 (server_socket,
+                                      (struct sockaddr *) &client_address,
+                                      &sa_len, sockflags);
+        TEST_VERIFY (sa_len == sizeof (client_address));
+        TEST_VERIFY (is_nonblocking (client_socket) == do_nonblock);
+        TEST_VERIFY (is_cloexec (client_socket) == do_cloexec);
+        check_same_address (&client.address, &client_address);
+        xclose (client_socket);
+        xclose (client.socket);
+      }
+
+  xclose (server_socket);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/support/Makefile b/support/Makefile
index 1f33fa0..38dbd83 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -50,6 +50,7 @@ libsupport-routines = \
   temp_file \
   write_message \
   xaccept \
+  xaccept4 \
   xasprintf \
   xbind \
   xcalloc \
diff --git a/support/xaccept4.c b/support/xaccept4.c
new file mode 100644
index 0000000..67dd95e
--- /dev/null
+++ b/support/xaccept4.c
@@ -0,0 +1,32 @@
+/* accept4 with error checking.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <support/xsocket.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <support/check.h>
+
+int
+xaccept4 (int fd, struct sockaddr *sa, socklen_t *salen, int flags)
+{
+  int clientfd = accept4 (fd, sa, salen, flags);
+  if (clientfd < 0)
+    FAIL_EXIT1 ("accept4 (%d, 0x%x): %m", fd, flags);
+  return clientfd;
+}
diff --git a/support/xsocket.h b/support/xsocket.h
index 0dbf13a..d672494 100644
--- a/support/xsocket.h
+++ b/support/xsocket.h
@@ -30,6 +30,7 @@ void xconnect (int, const struct sockaddr *, socklen_t);
 void xbind (int, const struct sockaddr *, socklen_t);
 void xlisten (int, int);
 int xaccept (int, struct sockaddr *, socklen_t *);
+int xaccept4 (int, struct sockaddr *, socklen_t *, int);
 void xsendto (int, const void *, size_t, int,
               const struct sockaddr *, socklen_t);
 size_t xrecvfrom (int, void *, size_t, int, struct sockaddr *, socklen_t *);
diff --git a/sysdeps/mach/hurd/kernel-features.h b/sysdeps/mach/hurd/kernel-features.h
index 20cb3ba..1966388 100644
--- a/sysdeps/mach/hurd/kernel-features.h
+++ b/sysdeps/mach/hurd/kernel-features.h
@@ -19,5 +19,3 @@
 /* This file can define __ASSUME_* macros checked by certain source files.
    Almost none of these are used outside of sysdeps/unix/sysv/linux code.
    But those referring to POSIX-level features like O_* flags can be.  */
-
-#define __ASSUME_ACCEPT4	1
diff --git a/sysdeps/nacl/kernel-features.h b/sysdeps/nacl/kernel-features.h
index 32834fd..ba315b6 100644
--- a/sysdeps/nacl/kernel-features.h
+++ b/sysdeps/nacl/kernel-features.h
@@ -19,7 +19,3 @@
 /* This file can define __ASSUME_* macros checked by certain source files.
    Almost none of these are used outside of sysdeps/unix/sysv/linux code.
    But those referring to POSIX-level features like O_* flags can be.  */
-
-/*
-#define __ASSUME_ACCEPT4	1
-*/
diff --git a/sysdeps/unix/sysv/linux/accept4.c b/sysdeps/unix/sysv/linux/accept4.c
index 53afd7a..0592f43 100644
--- a/sysdeps/unix/sysv/linux/accept4.c
+++ b/sysdeps/unix/sysv/linux/accept4.c
@@ -22,75 +22,20 @@
 
 #include <sysdep-cancel.h>
 #include <sys/syscall.h>
+#include <socketcall.h>
 #include <kernel-features.h>
 
+int
+accept4 (int fd, __SOCKADDR_ARG addr, socklen_t *addr_len, int flags)
+{
 /* Do not use the accept4 syscall on socketcall architectures unless
    it was added at the same time as the socketcall support or can be
    assumed to be present.  */
 #if defined __ASSUME_SOCKETCALL \
     && !defined __ASSUME_ACCEPT4_SYSCALL_WITH_SOCKETCALL \
     && !defined __ASSUME_ACCEPT4_SYSCALL
-# undef __NR_accept4
-#endif
-
-#ifdef __NR_accept4
-int
-accept4 (int fd, __SOCKADDR_ARG addr, socklen_t *addr_len, int flags)
-{
-  return SYSCALL_CANCEL (accept4, fd, addr.__sockaddr__, addr_len, flags);
-}
-#elif defined __NR_socketcall
-# include <socketcall.h>
-# ifdef __ASSUME_ACCEPT4_SOCKETCALL
-int
-accept4 (int fd, __SOCKADDR_ARG addr, socklen_t *addr_len, int flags)
-{
   return SOCKETCALL_CANCEL (accept4, fd, addr.__sockaddr__, addr_len, flags);
-}
-# else
-static int have_accept4;
-
-int
-accept4 (int fd, __SOCKADDR_ARG addr, socklen_t *addr_len, int flags)
-{
-  if (__glibc_likely (have_accept4 >= 0))
-    {
-      int ret = SOCKETCALL_CANCEL (accept4, fd, addr.__sockaddr__, addr_len,
-				   flags);
-      /* The kernel returns -EINVAL for unknown socket operations.
-	 We need to convert that error to an ENOSYS error.  */
-      if (__builtin_expect (ret < 0, 0)
-	  && have_accept4 == 0
-	  && errno == EINVAL)
-	{
-	  /* Try another call, this time with the FLAGS parameter
-	     cleared and an invalid file descriptor.  This call will not
-	     cause any harm and it will return immediately.  */
-	  ret = SOCKETCALL_CANCEL (invalid, -1);
-	  if (errno == EINVAL)
-	    {
-	      have_accept4 = -1;
-	      __set_errno (ENOSYS);
-	    }
-	  else
-	    {
-	      have_accept4 = 1;
-	      __set_errno (EINVAL);
-	    }
-	  return -1;
-	}
-      return ret;
-    }
-  __set_errno (ENOSYS);
-  return -1;
-}
-# endif /* __ASSUME_ACCEPT4_SOCKETCALL  */
-#else /* __NR_socketcall   */
-int
-accept4 (int fd, __SOCKADDR_ARG addr, socklen_t *addr_len, int flags)
-{
-  __set_errno (ENOSYS);
-  return -1;
-}
-stub_warning (accept4)
+#else
+  return SYSCALL_CANCEL (accept4, fd, addr.__sockaddr__, addr_len, flags);
 #endif
+}
diff --git a/sysdeps/unix/sysv/linux/ia64/configure b/sysdeps/unix/sysv/linux/ia64/configure
index 1d4e5d1..3cf7237 100644
--- a/sysdeps/unix/sysv/linux/ia64/configure
+++ b/sysdeps/unix/sysv/linux/ia64/configure
@@ -2,3 +2,7 @@
  # Local configure fragment for sysdeps/unix/sysv/linux/ia64
 
 ldd_rewrite_script=$dir/ldd-rewrite.sed
+
+# First version with support for the accept4 system call.
+# Linux 3.3 includes it as well.
+arch_minimum_kernel=3.2.18
diff --git a/sysdeps/unix/sysv/linux/ia64/configure.ac b/sysdeps/unix/sysv/linux/ia64/configure.ac
index 4fb5647..94a578c 100644
--- a/sysdeps/unix/sysv/linux/ia64/configure.ac
+++ b/sysdeps/unix/sysv/linux/ia64/configure.ac
@@ -2,3 +2,7 @@ GLIBC_PROVIDES dnl See aclocal.m4 in the top level source directory.
 # Local configure fragment for sysdeps/unix/sysv/linux/ia64
 
 ldd_rewrite_script=$dir/ldd-rewrite.sed
+
+# First version with support for the accept4 system call.
+# Linux 3.3 includes it as well.
+arch_minimum_kernel=3.2.18
diff --git a/sysdeps/unix/sysv/linux/ia64/kernel-features.h b/sysdeps/unix/sysv/linux/ia64/kernel-features.h
index ac9403e..cda0ad6 100644
--- a/sysdeps/unix/sysv/linux/ia64/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/ia64/kernel-features.h
@@ -22,13 +22,8 @@
 
 #include_next <kernel-features.h>
 
-/* Support for the accept4 syscall was added in 3.3.  */
-#if __LINUX_KERNEL_VERSION < 0x030300
-# undef __ASSUME_ACCEPT4_SYSCALL
-# undef __ASSUME_ACCEPT4
-#endif
-
 #define __ASSUME_RECV_SYSCALL   	1
 #define __ASSUME_SEND_SYSCALL		1
+#define __ASSUME_ACCEPT4_SYSCALL	1
 
 #endif /* _KERNEL_FEATURES_H */
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index fd936c5..b981466 100644
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -75,18 +75,11 @@
    architectures using a separate syscall rather than socketcall that
    syscall was only added later, and some architectures first had
    socketcall support then a separate syscall.  Define
-   __ASSUME_ACCEPT4_SOCKETCALL if glibc uses socketcall on this
-   architecture and accept4 is available through socketcall,
    __ASSUME_ACCEPT4_SYSCALL if it is available through a separate
-   syscall, __ASSUME_ACCEPT4_SYSCALL_WITH_SOCKETCALL if it became
+   syscall, and __ASSUME_ACCEPT4_SYSCALL_WITH_SOCKETCALL if it became
    available through a separate syscall at the same time as through
-   socketcall, and __ASSUME_ACCEPT4 if the accept4 function is known
-   to work.  */
-#ifdef __ASSUME_SOCKETCALL
-# define __ASSUME_ACCEPT4_SOCKETCALL	1
-#endif
+   socketcall.  */
 #define __ASSUME_ACCEPT4_SYSCALL	1
-#define __ASSUME_ACCEPT4	1
 
 /* Support for the FUTEX_CLOCK_REALTIME flag was added in 2.6.29.  */
 #define __ASSUME_FUTEX_CLOCK_REALTIME	1