Re: [PATCH] Assume that accept4 is available

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

This patch shows no regression on sparc64, armhf, and aarch64 as well.

On 14/04/2017 07:45, Florian Weimer wrote:
> diff --git a/INSTALL b/INSTALL
> index 60f714e..51989b2 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -500,21 +500,21 @@ 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
> -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
> -cross-compiling the GNU C Library, you need to specify
> -'ARCH=ARCHITECTURE' in the 'make headers_install' command, where
> -ARCHITECTURE is the architecture name used by the Linux kernel, such as
> -'x86' or 'powerpc'.)
> +to have the header files from a 3.2 or newer kernel (a 3.2.18 or newer
> +kernel for the ia64 architecture) around for reference.  These headers

I think since you are referring a specific version for ia64, it's worth to
add a reference why we are requiring it.

Also, and more important, I think we need to reflect it on alpha 
'arch_minimum_kernel' minimum version on configure.ac (so glibc configure
fails to build with old installed kernel version).

> diff --git a/socket/tst-accept4.c b/socket/tst-accept4.c
> new file mode 100644
> index 0000000..0e02bc0
> --- /dev/null
> +++ b/socket/tst-accept4.c
> @@ -0,0 +1,130 @@

Missing line with file description.

> +/* 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));
> +  }

Why the brackets? I usually seems this unnecessary.

> +  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..b676b58
> --- /dev/null
> +++ b/support/xaccept4.c
> @@ -0,0 +1,32 @@
> +/* accept with error checking.
> +   Copyright (C) 2016-2017 Free Software Foundation, Inc.

I think it should be only 2017, shouldn't it?

> +   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

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


> -# 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/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
>