Re: [PATCH] Assume that pipe2 is always available

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

On 13/04/2017 12:37, Florian Weimer wrote:
> The Debian patches (which are already required to build glibc before
> this commit) contain an implementation of pipe2.

I did not follow, which 'Debian patches' are you referring here?

> 
> 2017-04-13  Florian Weimer  <fweimer@redhat.com>
> 
> 	* include/unistd.h (__have_pipe2): Remove declaration.
> 	* socket/have_sock_cloexec.c (__have_pipe2): Remove definition.
> 	* libio/iopopen.c (_IO_new_proc_open): Assume that pipe2 is
> 	available.
> 	* posix/wordexp.c (exec_comm_child, exec_comm): Likewise.
> 	* sysdeps/unix/sysv/linux/kernel-features.h (__ASSUME_PIPE2):
> 	Remove definition.
> 
> diff --git a/include/unistd.h b/include/unistd.h
> index 16d68a1..e15fa0e 100644
> --- a/include/unistd.h
> +++ b/include/unistd.h
> @@ -171,7 +171,6 @@ extern int __libc_pause (void);
>  /* Not cancelable variant.  */
>  extern int __pause_nocancel (void) attribute_hidden;
>  
> -extern int __have_pipe2 attribute_hidden;
>  extern int __have_dup3 attribute_hidden;
>  
>  extern int __getlogin_r_loginuid (char *name, size_t namesize)
> diff --git a/libio/iopopen.c b/libio/iopopen.c
> index 08e29b4..5887bd1 100644
> --- a/libio/iopopen.c
> +++ b/libio/iopopen.c
> @@ -141,28 +141,12 @@ _IO_new_proc_open (_IO_FILE *fp, const char *command, const char *mode)
>      return NULL;
>  
>  #ifdef O_CLOEXEC
> -# ifndef __ASSUME_PIPE2
> -  if (__have_pipe2 >= 0)
> -# endif
>      {
>        int r = __pipe2 (pipe_fds, O_CLOEXEC);
> -# ifndef __ASSUME_PIPE2
> -      if (__have_pipe2 == 0)
> -	__have_pipe2 = r != -1 || errno != ENOSYS ? 1 : -1;
> -
> -      if (__have_pipe2 > 0)
> -# endif
>  	if (r < 0)
>  	  return NULL;
>      }
>  #endif
> -#ifndef __ASSUME_PIPE2
> -# ifdef O_CLOEXEC
> -  if (__have_pipe2 < 0)
> -# endif
> -    if (__pipe (pipe_fds) < 0)
> -      return NULL;
> -#endif

Why not remove the bracket and just use something like:

#ifdef O_CLOCEXEC
  if (__pipe2 (pipe_fds, O_CLOCEXEC) != 0)
    return NULL
#endif

>  
>    if (do_read)
>      {
> @@ -183,27 +167,13 @@ _IO_new_proc_open (_IO_FILE *fp, const char *command, const char *mode)
>        int child_std_end = do_read ? 1 : 0;
>        struct _IO_proc_file *p;
>  
> -#ifndef __ASSUME_PIPE2
> -      /* If we have pipe2 the descriptor is marked for close-on-exec.  */
> -      _IO_close (parent_end);
> -#endif
>        if (child_end != child_std_end)
> -	{
> -	  _IO_dup2 (child_end, child_std_end);
> -#ifndef __ASSUME_PIPE2
> -	  _IO_close (child_end);
> -#endif
> -	}
> +	_IO_dup2 (child_end, child_std_end);
>  #ifdef O_CLOEXEC
>        else
> -	{
> -	  /* The descriptor is already the one we will use.  But it must
> -	     not be marked close-on-exec.  Undo the effects.  */
> -# ifndef __ASSUME_PIPE2
> -	  if (__have_pipe2 > 0)
> -# endif
> -	    __fcntl (child_end, F_SETFD, 0);
> -	}
> +	/* The descriptor is already the one we will use.  But it must
> +	   not be marked close-on-exec.  Undo the effects.  */
> +	__fcntl (child_end, F_SETFD, 0);
>  #endif
>        /* POSIX.2:  "popen() shall ensure that any streams from previous
>           popen() calls that remain open in the parent process are closed
> @@ -229,26 +199,12 @@ _IO_new_proc_open (_IO_FILE *fp, const char *command, const char *mode)
>        return NULL;
>      }
>  
> -  if (do_cloexec)
> -    {
> -#ifndef __ASSUME_PIPE2
> -# ifdef O_CLOEXEC
> -      if (__have_pipe2 < 0)
> -# endif
> -	__fcntl (parent_end, F_SETFD, FD_CLOEXEC);
> -#endif
> -    }
> -  else
> -    {
> +  if (!do_cloexec)
>  #ifdef O_CLOEXEC
> -      /* Undo the effects of the pipe2 call which set the
> -	 close-on-exec flag.  */
> -# ifndef __ASSUME_PIPE2
> -      if (__have_pipe2 > 0)
> -# endif
> -	__fcntl (parent_end, F_SETFD, 0);
> +    /* Undo the effects of the pipe2 call which set the
> +       close-on-exec flag.  */
> +    __fcntl (parent_end, F_SETFD, 0);
>  #endif
> -    }
>  
>    _IO_fileno (fp) = parent_end;
>  
> diff --git a/posix/wordexp.c b/posix/wordexp.c
> index ba3f3ed..639d73e 100644
> --- a/posix/wordexp.c
> +++ b/posix/wordexp.c
> @@ -836,10 +836,7 @@ exec_comm_child (char *comm, int *fildes, int showerr, int noexec)
>      {
>  #ifdef O_CLOEXEC
>        /* Reset the close-on-exec flag (if necessary).  */
> -# ifndef __ASSUME_PIPE2
> -      if (__have_pipe2 > 0)
> -# endif
> -	__fcntl (fildes[1], F_SETFD, 0);
> +      __fcntl (fildes[1], F_SETFD, 0);
>  #endif
>      }
>  
> @@ -906,29 +903,12 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length,
>      return 0;
>  
>  #ifdef O_CLOEXEC
> -# ifndef __ASSUME_PIPE2
> -  if (__have_pipe2 >= 0)
> -# endif
> -    {
> -      int r = __pipe2 (fildes, O_CLOEXEC);
> -# ifndef __ASSUME_PIPE2
> -      if (__have_pipe2 == 0)
> -	__have_pipe2 = r != -1 || errno != ENOSYS ? 1 : -1;
> -
> -      if (__have_pipe2 > 0)
> -# endif
> -	if (r < 0)
> -	  /* Bad */
> -	  return WRDE_NOSPACE;
> -    }
> -#endif
> -#ifndef __ASSUME_PIPE2
> -# ifdef O_CLOEXEC
> -  if (__have_pipe2 < 0)
> -# endif
> -    if (__pipe (fildes) < 0)
> +  {
> +    int r = __pipe2 (fildes, O_CLOEXEC);
> +    if (r < 0)
>        /* Bad */
>        return WRDE_NOSPACE;
> +  }
>  #endif

Same as before, I think it is simpler to just remove the brackets.

>  
>   again:
> diff --git a/socket/have_sock_cloexec.c b/socket/have_sock_cloexec.c
> index 70c730e..579577d 100644
> --- a/socket/have_sock_cloexec.c
> +++ b/socket/have_sock_cloexec.c
> @@ -19,10 +19,6 @@
>  #include <sys/socket.h>
>  #include <kernel-features.h>
>  
> -#if defined O_CLOEXEC && !defined __ASSUME_PIPE2
> -int __have_pipe2;
> -#endif
> -
>  #if defined O_CLOEXEC && !defined __ASSUME_DUP3
>  int __have_dup3;
>  #endif
> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
> index e6a2720..233e302 100644
> --- a/sysdeps/unix/sysv/linux/kernel-features.h
> +++ b/sysdeps/unix/sysv/linux/kernel-features.h
> @@ -74,7 +74,6 @@
>  /* Support for various CLOEXEC and NONBLOCK flags was added in
>     2.6.27.  */
>  #define __ASSUME_IN_NONBLOCK	1
> -#define __ASSUME_PIPE2		1
>  #define __ASSUME_DUP3		1
>  
>  /* Support for accept4 functionality was added in 2.6.28, but for some
>