diff mbox

Remove __ASSUME_O_CLOEXEC / O_CLOEXEC conditionals in sysdeps/unix/sysv/linux/

Message ID Pine.LNX.4.64.1406201639240.16806@digraph.polyomino.org.uk
State Superseded
Headers show

Commit Message

Joseph Myers June 20, 2014, 4:39 p.m. UTC
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-20  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
	[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

Joseph Myers June 25, 2014, 11:11 a.m. UTC | #1
Ping.  This patch 
<https://sourceware.org/ml/libc-alpha/2014-06/msg00520.html> is pending 
review.
Andreas Schwab June 25, 2014, 12:45 p.m. UTC | #2
"Joseph S. Myers" <joseph@codesourcery.com> writes:

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

That file is no longer needed.

Andreas.
Joseph Myers June 25, 2014, 1 p.m. UTC | #3
On Wed, 25 Jun 2014, Andreas Schwab wrote:

> "Joseph S. Myers" <joseph@codesourcery.com> writes:
> 
> > 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>
> 
> That file is no longer needed.

Followup cleanups in that regard for both this and the __ASSUME_ATFCTS 
patch are welcome.  This patch is deliberately conservative to make sure 
that it doesn't change the disassembly of installed shared libraries at 
all (not even e.g. through reordering of the objects linked into them).

Comments are also welcome on the question of whether - again as a followup 
- we should make O_CLOEXEC a required feature for glibc and so remove all 
conditionals on __ASSUME_O_CLOEXEC throughout the source tree.
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 755c259..3e93548 100644
--- a/sysdeps/unix/sysv/linux/getsysstats.c
+++ b/sysdeps/unix/sysv/linux/getsysstats.c
@@ -143,11 +143,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..84b208a 100644
--- a/sysdeps/unix/sysv/linux/shm_open.c
+++ b/sysdeps/unix/sysv/linux/shm_open.c
@@ -45,11 +45,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 +159,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 +167,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.  */