Assume that dup3 is available

Submitted by Florian Weimer on April 13, 2017, 7:25 p.m.

Details

Message ID 20170413192548.BF5A040EBCF62@oldenburg.str.redhat.com
State New
Headers show

Commit Message

Florian Weimer April 13, 2017, 7:25 p.m.
2017-04-13  Florian Weimer  <fweimer@redhat.com>

	* include/unistd.h (__have_dup3): Remove declaration.
	* libio/freopen.c (freopen): Assume that O_CLOEXEC is defined and
	dup3 is available.
	* libio/freopen64.c (freopen64): Likewise.
	* socket/Makefile (aux): Remove have_sock_cloexec.
	* socket/have_sock_cloexec.c: Remove file.
	* sysdeps/mach/hurd/kernel-features.h (__ASSUME_DUP3): Remove
	definition.
	* sysdeps/unix/sysv/linux/kernel-features.h (__ASSUME_DUP3):
	Likewise.
	* sysdeps/nacl/kernel-features.h: Update comment.

Comments

Adhemerval Zanella April 17, 2017, 6:16 p.m.
LGTM with with one remark below.

On 13/04/2017 16:25, Florian Weimer wrote:
> 2017-04-13  Florian Weimer  <fweimer@redhat.com>
> 
> 	* include/unistd.h (__have_dup3): Remove declaration.
> 	* libio/freopen.c (freopen): Assume that O_CLOEXEC is defined and
> 	dup3 is available.
> 	* libio/freopen64.c (freopen64): Likewise.
> 	* socket/Makefile (aux): Remove have_sock_cloexec.
> 	* socket/have_sock_cloexec.c: Remove file.
> 	* sysdeps/mach/hurd/kernel-features.h (__ASSUME_DUP3): Remove
> 	definition.
> 	* sysdeps/unix/sysv/linux/kernel-features.h (__ASSUME_DUP3):
> 	Likewise.
> 	* sysdeps/nacl/kernel-features.h: Update comment.
> 
> diff --git a/include/unistd.h b/include/unistd.h
> index e15fa0e..16a8815 100644
> --- a/include/unistd.h
> +++ b/include/unistd.h
> @@ -171,8 +171,6 @@ extern int __libc_pause (void);
>  /* Not cancelable variant.  */
>  extern int __pause_nocancel (void) attribute_hidden;
>  
> -extern int __have_dup3 attribute_hidden;
> -
>  extern int __getlogin_r_loginuid (char *name, size_t namesize)
>       attribute_hidden;
>  
> diff --git a/libio/freopen.c b/libio/freopen.c
> index 03e3ae7..ad1c848 100644
> --- a/libio/freopen.c
> +++ b/libio/freopen.c
> @@ -78,32 +78,9 @@ freopen (const char *filename, const char *mode, FILE *fp)
>  
>        if (fd != -1)
>  	{
> -#ifdef O_CLOEXEC
> -# ifndef __ASSUME_DUP3
> -	  int newfd;
> -	  if (__have_dup3 < 0)
> -	    newfd = -1;
> -	  else
> -	    newfd =
> -# endif
> -	      __dup3 (_IO_fileno (result), fd,
> -                      (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
> -                      ? O_CLOEXEC : 0);
> -#else
> -# define newfd 1
> -#endif
> -
> -#ifndef __ASSUME_DUP3
> -	  if (newfd < 0)
> -	    {
> -	      if (errno == ENOSYS)
> -		__have_dup3 = -1;
> -
> -	      __dup2 (_IO_fileno (result), fd);
> -	      if ((result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0)
> -		__fcntl (fd, F_SETFD, FD_CLOEXEC);
> -	    }
> -#endif
> +	  __dup3 (_IO_fileno (result), fd,
> +		  (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
> +		  ? O_CLOEXEC : 0);

Shouldn't we check the result of __dup3 and act accordingly in this situation
(basically EINTR and EMFILE)?

>  	  __close (_IO_fileno (result));
>  	  _IO_fileno (result) = fd;
>  	}
> diff --git a/libio/freopen64.c b/libio/freopen64.c
> index 1f0d8ab..adf749a 100644
> --- a/libio/freopen64.c
> +++ b/libio/freopen64.c
> @@ -61,32 +61,9 @@ freopen64 (const char *filename, const char *mode, FILE *fp)
>  
>        if (fd != -1)
>  	{
> -#ifdef O_CLOEXEC
> -# ifndef __ASSUME_DUP3
> -	  int newfd;
> -	  if (__have_dup3 < 0)
> -	    newfd = -1;
> -	  else
> -	    newfd =
> -# endif
> -	      __dup3 (_IO_fileno (result), fd,
> -                      (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
> -                      ? O_CLOEXEC : 0);
> -#else
> -# define newfd 1
> -#endif
> -
> -#ifndef __ASSUME_DUP3
> -	  if (newfd < 0)
> -	    {
> -	      if (errno == ENOSYS)
> -		__have_dup3 = -1;
> -
> -	      __dup2 (_IO_fileno (result), fd);
> -	      if ((result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0)
> -		__fcntl (fd, F_SETFD, FD_CLOEXEC);
> -	    }
> -#endif
> +	  __dup3 (_IO_fileno (result), fd,
> +		  (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
> +		  ? O_CLOEXEC : 0);

Same as before.

>  	  __close (_IO_fileno (result));
>  	  _IO_fileno (result) = fd;
>  	}
> diff --git a/socket/Makefile b/socket/Makefile
> index c299d34..25d4f68 100644
> --- a/socket/Makefile
> +++ b/socket/Makefile
> @@ -31,6 +31,6 @@ routines := accept bind connect getpeername getsockname getsockopt	\
>  	    setsockopt shutdown socket socketpair isfdtype opensock	\
>  	    sockatmark accept4 recvmmsg sendmmsg
>  
> -aux	 := have_sock_cloexec sa_len
> +aux	 := sa_len
>  
>  include ../Rules
> diff --git a/socket/have_sock_cloexec.c b/socket/have_sock_cloexec.c
> deleted file mode 100644
> index 579577d..0000000
> --- a/socket/have_sock_cloexec.c
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/* Copyright (C) 2008-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 <fcntl.h>
> -#include <sys/socket.h>
> -#include <kernel-features.h>
> -
> -#if defined O_CLOEXEC && !defined __ASSUME_DUP3
> -int __have_dup3;
> -#endif
> diff --git a/sysdeps/mach/hurd/kernel-features.h b/sysdeps/mach/hurd/kernel-features.h
> index 687c7f0..60c5bca 100644
> --- a/sysdeps/mach/hurd/kernel-features.h
> +++ b/sysdeps/mach/hurd/kernel-features.h
> @@ -21,5 +21,4 @@
>     But those referring to POSIX-level features like O_* flags can be.  */
>  
>  #define __ASSUME_O_CLOEXEC	1
> -#define __ASSUME_DUP3		1
>  #define __ASSUME_ACCEPT4	1
> diff --git a/sysdeps/nacl/kernel-features.h b/sysdeps/nacl/kernel-features.h
> index 38a47f1..4a5808b 100644
> --- a/sysdeps/nacl/kernel-features.h
> +++ b/sysdeps/nacl/kernel-features.h
> @@ -23,6 +23,5 @@
>  #define __ASSUME_O_CLOEXEC	1
>  
>  /*
> -#define __ASSUME_DUP3		1
>  #define __ASSUME_ACCEPT4	1
>  */
> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
> index 233e302..e0eb4e2 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_DUP3		1
>  
>  /* Support for accept4 functionality was added in 2.6.28, but for some
>     architectures using a separate syscall rather than socketcall that
>
Florian Weimer April 17, 2017, 8:11 p.m.
On 04/17/2017 08:16 PM, Adhemerval Zanella wrote:
> Shouldn't we check the result of __dup3 and act accordingly in this situation
> (basically EINTR and EMFILE)?

Yes, but that's an existing bug in the code.  I would like to keep this 
change the bug fix separate.  I filed bug 21393 for the missing error check.

Thanks,
Florian
Andreas Schwab April 17, 2017, 10:01 p.m.
On Apr 17 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

>> +	  __dup3 (_IO_fileno (result), fd,
>> +		  (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
>> +		  ? O_CLOEXEC : 0);
>
> Shouldn't we check the result of __dup3 and act accordingly in this situation
> (basically EINTR and EMFILE)?

I don't think either is possible, since both descriptors are already
allocated.

Andreas.

Patch hide | download patch | download mbox

diff --git a/include/unistd.h b/include/unistd.h
index e15fa0e..16a8815 100644
--- a/include/unistd.h
+++ b/include/unistd.h
@@ -171,8 +171,6 @@  extern int __libc_pause (void);
 /* Not cancelable variant.  */
 extern int __pause_nocancel (void) attribute_hidden;
 
-extern int __have_dup3 attribute_hidden;
-
 extern int __getlogin_r_loginuid (char *name, size_t namesize)
      attribute_hidden;
 
diff --git a/libio/freopen.c b/libio/freopen.c
index 03e3ae7..ad1c848 100644
--- a/libio/freopen.c
+++ b/libio/freopen.c
@@ -78,32 +78,9 @@  freopen (const char *filename, const char *mode, FILE *fp)
 
       if (fd != -1)
 	{
-#ifdef O_CLOEXEC
-# ifndef __ASSUME_DUP3
-	  int newfd;
-	  if (__have_dup3 < 0)
-	    newfd = -1;
-	  else
-	    newfd =
-# endif
-	      __dup3 (_IO_fileno (result), fd,
-                      (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
-                      ? O_CLOEXEC : 0);
-#else
-# define newfd 1
-#endif
-
-#ifndef __ASSUME_DUP3
-	  if (newfd < 0)
-	    {
-	      if (errno == ENOSYS)
-		__have_dup3 = -1;
-
-	      __dup2 (_IO_fileno (result), fd);
-	      if ((result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0)
-		__fcntl (fd, F_SETFD, FD_CLOEXEC);
-	    }
-#endif
+	  __dup3 (_IO_fileno (result), fd,
+		  (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
+		  ? O_CLOEXEC : 0);
 	  __close (_IO_fileno (result));
 	  _IO_fileno (result) = fd;
 	}
diff --git a/libio/freopen64.c b/libio/freopen64.c
index 1f0d8ab..adf749a 100644
--- a/libio/freopen64.c
+++ b/libio/freopen64.c
@@ -61,32 +61,9 @@  freopen64 (const char *filename, const char *mode, FILE *fp)
 
       if (fd != -1)
 	{
-#ifdef O_CLOEXEC
-# ifndef __ASSUME_DUP3
-	  int newfd;
-	  if (__have_dup3 < 0)
-	    newfd = -1;
-	  else
-	    newfd =
-# endif
-	      __dup3 (_IO_fileno (result), fd,
-                      (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
-                      ? O_CLOEXEC : 0);
-#else
-# define newfd 1
-#endif
-
-#ifndef __ASSUME_DUP3
-	  if (newfd < 0)
-	    {
-	      if (errno == ENOSYS)
-		__have_dup3 = -1;
-
-	      __dup2 (_IO_fileno (result), fd);
-	      if ((result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0)
-		__fcntl (fd, F_SETFD, FD_CLOEXEC);
-	    }
-#endif
+	  __dup3 (_IO_fileno (result), fd,
+		  (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
+		  ? O_CLOEXEC : 0);
 	  __close (_IO_fileno (result));
 	  _IO_fileno (result) = fd;
 	}
diff --git a/socket/Makefile b/socket/Makefile
index c299d34..25d4f68 100644
--- a/socket/Makefile
+++ b/socket/Makefile
@@ -31,6 +31,6 @@  routines := accept bind connect getpeername getsockname getsockopt	\
 	    setsockopt shutdown socket socketpair isfdtype opensock	\
 	    sockatmark accept4 recvmmsg sendmmsg
 
-aux	 := have_sock_cloexec sa_len
+aux	 := sa_len
 
 include ../Rules
diff --git a/socket/have_sock_cloexec.c b/socket/have_sock_cloexec.c
deleted file mode 100644
index 579577d..0000000
--- a/socket/have_sock_cloexec.c
+++ /dev/null
@@ -1,24 +0,0 @@ 
-/* Copyright (C) 2008-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 <fcntl.h>
-#include <sys/socket.h>
-#include <kernel-features.h>
-
-#if defined O_CLOEXEC && !defined __ASSUME_DUP3
-int __have_dup3;
-#endif
diff --git a/sysdeps/mach/hurd/kernel-features.h b/sysdeps/mach/hurd/kernel-features.h
index 687c7f0..60c5bca 100644
--- a/sysdeps/mach/hurd/kernel-features.h
+++ b/sysdeps/mach/hurd/kernel-features.h
@@ -21,5 +21,4 @@ 
    But those referring to POSIX-level features like O_* flags can be.  */
 
 #define __ASSUME_O_CLOEXEC	1
-#define __ASSUME_DUP3		1
 #define __ASSUME_ACCEPT4	1
diff --git a/sysdeps/nacl/kernel-features.h b/sysdeps/nacl/kernel-features.h
index 38a47f1..4a5808b 100644
--- a/sysdeps/nacl/kernel-features.h
+++ b/sysdeps/nacl/kernel-features.h
@@ -23,6 +23,5 @@ 
 #define __ASSUME_O_CLOEXEC	1
 
 /*
-#define __ASSUME_DUP3		1
 #define __ASSUME_ACCEPT4	1
 */
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index 233e302..e0eb4e2 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_DUP3		1
 
 /* Support for accept4 functionality was added in 2.6.28, but for some
    architectures using a separate syscall rather than socketcall that