Assume that pipe2 is always available

Submitted by Florian Weimer on April 13, 2017, 3:37 p.m.

Details

Message ID 20170413153744.A69B2400F200A@oldenburg.str.redhat.com
State New
Headers show

Commit Message

Florian Weimer April 13, 2017, 3:37 p.m.
The Debian patches (which are already required to build glibc before
this commit) contain an implementation of pipe2.

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.

Comments

Adhemerval Zanella April 13, 2017, 5:39 p.m.
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
>
Florian Weimer April 13, 2017, 5:54 p.m.
On 04/13/2017 07:39 PM, Adhemerval Zanella wrote:
> 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?

Upstream master contains an incomplete implementation of O_CLOEXEC 
support for Hurd.  For example, the file sysdeps/mach/hurd/accept4.c 
refers to the sock_to_o_flags identifier, but neither glibc, gnumach, 
nor hurd contain a definition.  The definition is found in a patch in 
the Debian package.  There is another patch which contains an 
implementation of pipe2:

https://sources.debian.net/src/glibc/2.24-8/debian/patches/hurd-i386/tg-pipe2.diff/

Anyone who wants to build glibc for Hurd needs those Debian patches, so 
I see no problem with applying this cleanup to upstream master.

(From the Hurd perspective.  NaCl does not support pipe2, either.)

Thanks,
Florian
Florian Weimer April 13, 2017, 6:18 p.m.
I have looked at the NaCl port.

It does not support fork or the exec* family of functions.  This means 
that the O_CLOEXEC flag is quite meaningless.  O_NONBLOCK is still 
useful.  The races can largely be obscured due to the lack of fork 
support.  The exception is dup3, where the new descriptor will 
temporarily lack the O_CLOEXEC flag, and this flag is actually 
observable by a racing thread (because the descriptor is specified by 
the caller).  But this looks like a minor issue.

In any case, I think it is possible to implement a reasonable emulation 
of pipe2 and dup3 in glibc for NaCl (unlike Linux, where kernel support 
would be needed).  So I would like to proceed with these cleanups even 
if we decide not to remove the NaCl port.

Thanks,
Florian
Adhemerval Zanella April 17, 2017, 6:17 p.m.
On 13/04/2017 14:54, Florian Weimer wrote:
> On 04/13/2017 07:39 PM, Adhemerval Zanella wrote:
>> 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?
> 
> Upstream master contains an incomplete implementation of O_CLOEXEC support for Hurd.  For example, the file sysdeps/mach/hurd/accept4.c refers to the sock_to_o_flags identifier, but neither glibc, gnumach, nor hurd contain a definition.  The definition is found in a patch in the Debian package.  There is another patch which contains an implementation of pipe2:
> 
> https://sources.debian.net/src/glibc/2.24-8/debian/patches/hurd-i386/tg-pipe2.diff/
> 
> Anyone who wants to build glibc for Hurd needs those Debian patches, so I see no problem with applying this cleanup to upstream master.
> 
> (From the Hurd perspective.  NaCl does not support pipe2, either.)

Right, I think we need to sync on master since it seems required to actually
build hurd.
Florian Weimer April 17, 2017, 6:43 p.m.
On 04/17/2017 08:17 PM, Adhemerval Zanella wrote:
> 
> 
> On 13/04/2017 14:54, Florian Weimer wrote:
>> On 04/13/2017 07:39 PM, Adhemerval Zanella wrote:
>>> 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?
>>
>> Upstream master contains an incomplete implementation of O_CLOEXEC support for Hurd.  For example, the file sysdeps/mach/hurd/accept4.c refers to the sock_to_o_flags identifier, but neither glibc, gnumach, nor hurd contain a definition.  The definition is found in a patch in the Debian package.  There is another patch which contains an implementation of pipe2:
>>
>> https://sources.debian.net/src/glibc/2.24-8/debian/patches/hurd-i386/tg-pipe2.diff/
>>
>> Anyone who wants to build glibc for Hurd needs those Debian patches, so I see no problem with applying this cleanup to upstream master.
>>
>> (From the Hurd perspective.  NaCl does not support pipe2, either.)
> 
> Right, I think we need to sync on master since it seems required to actually
> build hurd.

Would you please clarify what you mean?  Do you suggest we have to pull 
in the Debian patches before we should apply this pipe2 cleanup?

Thanks,
Florian
Adhemerval Zanella April 17, 2017, 7:14 p.m.
On 17/04/2017 15:43, Florian Weimer wrote:
> On 04/17/2017 08:17 PM, Adhemerval Zanella wrote:
>>
>>
>> On 13/04/2017 14:54, Florian Weimer wrote:
>>> On 04/13/2017 07:39 PM, Adhemerval Zanella wrote:
>>>> 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?
>>>
>>> Upstream master contains an incomplete implementation of O_CLOEXEC support for Hurd.  For example, the file sysdeps/mach/hurd/accept4.c refers to the sock_to_o_flags identifier, but neither glibc, gnumach, nor hurd contain a definition.  The definition is found in a patch in the Debian package.  There is another patch which contains an implementation of pipe2:
>>>
>>> https://sources.debian.net/src/glibc/2.24-8/debian/patches/hurd-i386/tg-pipe2.diff/
>>>
>>> Anyone who wants to build glibc for Hurd needs those Debian patches, so I see no problem with applying this cleanup to upstream master.
>>>
>>> (From the Hurd perspective.  NaCl does not support pipe2, either.)
>>
>> Right, I think we need to sync on master since it seems required to actually
>> build hurd.
> 
> Would you please clarify what you mean?  Do you suggest we have to pull in the Debian patches before we should apply this pipe2 cleanup?

I think we can push pipe2 cleanup first and then if you have time sync hurd
patches with debian. It would be good to have some hurd developer to ack
about these patches as well...

Patch hide | download patch | download mbox

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