diff mbox

[v2] Remove __ASSUME_O_CLOEXEC / O_CLOEXEC conditionals in sysdeps/unix/sysv/linux/

Message ID Pine.LNX.4.64.1406251821230.12113@digraph.polyomino.org.uk
State New
Headers show

Commit Message

Joseph Myers June 25, 2014, 6:22 p.m. UTC 
(Differences from the previous version
<https://sourceware.org/ml/libc-alpha/2014-06/msg00520.html>: this one
includes the removal of a <kernel-features.h> include from shm_open.c,
while I've committed all the rest of
<https://sourceware.org/ml/libc-alpha/2014-06/msg00636.html> that
didn't depend on this patch.)

This patch removes conditionals on __ASSUME_O_CLOEXEC, and on
O_CLOEXEC being defined, in sysdeps/unix/sysv/linux/, now that
O_CLOEXEC support can be unconditionally assumed.

The patch is conservative in what it changes and further followup
cleanups may be possible.  It may be possible to remove dl-opendir.c,
but the patch does not do so, just removing a redundant undefine and
redefine of __ASSUME_O_CLOEXEC.  Also, __ASSUME_O_CLOEXEC is defined
unconditionally for Hurd as well as Linux.  Thus, if we decide that
O_CLOEXEC support is a required feature of any glibc port, we could
remove __ASSUME_O_CLOEXEC and all conditionals on it throughout glibc,
rather than just cleaning up sysdeps/unix/sysv/linux/.

Tested x86_64 that the disassembly of installed shared libraries is
unchanged by this patch.

2014-06-25  Joseph Myers  <joseph@codesourcery.com>

	* sysdeps/unix/sysv/linux/dl-opendir.c (__ASSUME_O_CLOEXEC): Do
	not undefine and redefine.
	* sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs)
	[O_CLOEXEC]: Make code unconditional.
	(__get_nprocs) [!O_CLOEXEC]: Remove conditional code.
	* sysdeps/unix/sysv/linux/shm_open.c: Do not include
	<kernel-features.h>.
	[O_CLOEXEC && !__ASSUME_O_CLOEXEC] (have_o_cloexec): Remove
	conditional variable definition.
	(shm_open) [O_CLOEXEC]: Make code unconditional.
	(shm_open) [!O_CLOEXEC || !__ASSUME_O_CLOEXEC]: Remove conditional
	code.

Comments

Roland McGrath June 25, 2014, 6:46 p.m. UTC | #1 
Currently I'm inclined against requiring O_CLOEXEC for all configurations.

> @@ -164,9 +157,7 @@ shm_open (const char *name, int oflag, mode_t mode)
>    __mempcpy (__mempcpy (fname, mountpoint.dir, mountpoint.dirlen),
>  	     name, namelen + 1);
>  
> -#ifdef O_CLOEXEC
>    oflag |= O_CLOEXEC;
> -#endif

Just move | O_CLOEXEC into the open call along with | O_NOFOLLOW.

Otherwise looks fine.


Thanks,
Roland
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/dl-opendir.c b/sysdeps/unix/sysv/linux/dl-opendir.c
index 72d2c06..c1cdc05 100644
--- a/sysdeps/unix/sysv/linux/dl-opendir.c
+++ b/sysdeps/unix/sysv/linux/dl-opendir.c
@@ -1,6 +1 @@ 
-/* In this implementation we do not really care whether the opened
-   file descriptor has the CLOEXEC bit set.  The only call happens
-   long before there is a call to fork or exec.  */
-#undef __ASSUME_O_CLOEXEC
-#define __ASSUME_O_CLOEXEC 1
 #include <opendir.c>
diff --git a/sysdeps/unix/sysv/linux/getsysstats.c b/sysdeps/unix/sysv/linux/getsysstats.c
index 2a81184..1746827 100644
--- a/sysdeps/unix/sysv/linux/getsysstats.c
+++ b/sysdeps/unix/sysv/linux/getsysstats.c
@@ -142,11 +142,7 @@  __get_nprocs (void)
   char *cp = buffer_end;
   char *re = buffer_end;
 
-#ifdef O_CLOEXEC
   const int flags = O_RDONLY | O_CLOEXEC;
-#else
-  const int flags = O_RDONLY;
-#endif
   int fd = open_not_cancel_2 ("/sys/devices/system/cpu/online", flags);
   char *l;
   int result = 0;
diff --git a/sysdeps/unix/sysv/linux/shm_open.c b/sysdeps/unix/sysv/linux/shm_open.c
index cec6fdb..94952ec 100644
--- a/sysdeps/unix/sysv/linux/shm_open.c
+++ b/sysdeps/unix/sysv/linux/shm_open.c
@@ -28,8 +28,6 @@ 
 #include <bits/libc-lock.h>
 #include "linux_fsinfo.h"
 
-#include <kernel-features.h>
-
 
 /* Mount point of the shared memory filesystem.  */
 static struct
@@ -45,11 +43,6 @@  static const char defaultdir[] = "/dev/shm/";
 __libc_once_define (static, once);
 
 
-#if defined O_CLOEXEC && !defined __ASSUME_O_CLOEXEC
-static bool have_o_cloexec;
-#endif
-
-
 /* Determine where the shmfs is mounted (if at all).  */
 static void
 where_is_shmfs (void)
@@ -164,9 +157,7 @@  shm_open (const char *name, int oflag, mode_t mode)
   __mempcpy (__mempcpy (fname, mountpoint.dir, mountpoint.dirlen),
 	     name, namelen + 1);
 
-#ifdef O_CLOEXEC
   oflag |= O_CLOEXEC;
-#endif
 
   /* And get the file descriptor.
      XXX Maybe we should test each descriptor whether it really is for a
@@ -174,41 +165,7 @@  shm_open (const char *name, int oflag, mode_t mode)
      should be revamped since we can determine whether shmfs is available
      while trying to open the file, all in one turn.  */
   fd = open (fname, oflag | O_NOFOLLOW, mode);
-  if (fd != -1)
-    {
-#if !defined O_CLOEXEC || !defined __ASSUME_O_CLOEXEC
-# ifdef O_CLOEXEC
-      if (have_o_cloexec <= 0)
-# endif
-	{
-	  /* We got a descriptor.  Now set the FD_CLOEXEC bit.  */
-	  int flags = fcntl (fd, F_GETFD, 0);
-
-	  if (__builtin_expect (flags, 0) >= 0)
-	    {
-# ifdef O_CLOEXEC
-	      if (have_o_cloexec == 0)
-		have_o_cloexec = (flags & FD_CLOEXEC) == 0 ? -1 : 1;
-	      if (have_o_cloexec < 0)
-# endif
-		{
-		  flags |= FD_CLOEXEC;
-		  flags = fcntl (fd, F_SETFD, flags);
-		}
-	    }
-
-	  if (flags == -1)
-	    {
-	      /* Something went wrong.  We cannot return the descriptor.  */
-	      int save_errno = errno;
-	      close (fd);
-	      fd = -1;
-	      __set_errno (save_errno);
-	    }
-	}
-#endif
-    }
-  else if (__glibc_unlikely (errno == EISDIR))
+  if (fd == -1 && __glibc_unlikely (errno == EISDIR))
     /* It might be better to fold this error with EINVAL since
        directory names are just another example for unsuitable shared
        object names and the standard does not mention EISDIR.  */