[roland/opendir] Refactor opendir.

Message ID 20150513233552.3BAAB2C3A9B@topped-with-meat.com (mailing list archive)
State Committed
Headers

Commit Message

Roland McGrath May 13, 2015, 11:35 p.m. UTC
  I've refactored the opendir implementation, with a few effects.
The main one (and my motivation) is that opendir no longer calls
__opendirat, and so opendir uses only open while only __opendirat
uses openat.  The other things were incidental but they fix
hidden bugs.

1. If O_DIRECTORY is not known to work, then the old code in
   __opendirat called stat (really __xstat64) on the name without
   regard to the fd, so for cases other that AT_FDCWD it was
   stat'ing the wrong thing.  I've changed it to use fstatat
   (really __fxstatat64).  GNU/Linux configurations assume
   O_DIRECTORY works and so don't use this path, while GNU/Hurd
   doesn't use this code at all.  So this did not affect existing
   configurations (other than NaCl, which doesn't have openat anyway).

2. When O_DIRECTORY does work, then the old code bypassed the
   fstat (really __fxstat64) call to do the S_ISDIR check.  But
   this also meant that it didn't sample st_blksize, so ignored
   st_blksize in deciding what buffer size to use.  This was a
   regression from past versions (I didn't look to see when the
   change was made), and is now fixed.

Tested x86_64-linux-gnu (and arm-nacl).

If there are no objections, I'll commit this on Monday.


Thanks,
Roland


	* sysdeps/posix/opendir.c: Include <stdbool.h>.
	(invalid_name): New function, broken out of ...
	(__opendirat): ... here.  Call it.
	(need_isdir_precheck): New function, broken out of ...
	(__opendirat): ... here.  Call it.
	Use __fxstatat64, not __xstatat64.
	(opendir_oflags): New function, broken out of ...
	(__opendirat): ... here.  Call it.
	(opendir_tail): New function, broken out of ...
	(__opendirat): ... here.  Call it.
	(__opendir): Call invalid_name, need_isdir_precheck, __xstat64, and
	opendir_tail, rather than punting to __opendirat.
	(__opendirat): Conditionalize function definition on [IS_IN (libc)].
  

Comments

Mike Frysinger May 14, 2015, 7:58 a.m. UTC | #1
On 13 May 2015 16:35, Roland McGrath wrote:
> I've refactored the opendir implementation, with a few effects.
> The main one (and my motivation) is that opendir no longer calls
> __opendirat, and so opendir uses only open while only __opendirat
> uses openat.

why ?  seems like we generally want to move in the other direction, especially 
as newer kernel ports move to not even include non-at syscall variants.
-mike
  
Roland McGrath May 18, 2015, 9:06 p.m. UTC | #2
> On 13 May 2015 16:35, Roland McGrath wrote:
> > I've refactored the opendir implementation, with a few effects.
> > The main one (and my motivation) is that opendir no longer calls
> > __opendirat, and so opendir uses only open while only __opendirat
> > uses openat.
> 
> why ?  seems like we generally want to move in the other direction, especially 
> as newer kernel ports move to not even include non-at syscall variants.

All the world is Linux and everything we do should be done with Linuxism
tunnel vision, I know.  But I'm just funny that way.
  
Mike Frysinger May 19, 2015, 3:31 a.m. UTC | #3
On 18 May 2015 14:06, Roland McGrath wrote:
> > On 13 May 2015 16:35, Roland McGrath wrote:
> > > I've refactored the opendir implementation, with a few effects.
> > > The main one (and my motivation) is that opendir no longer calls
> > > __opendirat, and so opendir uses only open while only __opendirat
> > > uses openat.
> > 
> > why ?  seems like we generally want to move in the other direction, especially 
> > as newer kernel ports move to not even include non-at syscall variants.
> 
> All the world is Linux and everything we do should be done with Linuxism
> tunnel vision, I know.  But I'm just funny that way.

the *at functions are in POSIX, so having code assume and base itself on that 
assumption makes sense.  especially when it leads to unified & smaller code 
paths.  from your patch, it looks like we're doing the opposite of both.

so i don't see how Linux is related and this patch isn't justified.

also, let's drop the sarcasm as it isn't helping anything.
-mike
  

Patch

diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
index 451c56e..6509f5c 100644
--- a/sysdeps/posix/opendir.c
+++ b/sysdeps/posix/opendir.c
@@ -18,6 +18,7 @@ 
 #include <assert.h>
 #include <errno.h>
 #include <limits.h>
+#include <stdbool.h>
 #include <stddef.h>
 #include <stdlib.h>
 #include <dirent.h>
@@ -81,83 +82,122 @@  tryopen_o_directory (void)
 #endif
 
 
-DIR *
-internal_function
-__opendirat (int dfd, const char *name)
+static bool
+invalid_name (const char *name)
 {
-  struct stat64 statbuf;
-  struct stat64 *statp = NULL;
-
-  if (__builtin_expect (name[0], '\1') == '\0')
+  if (__glibc_unlikely (name[0] == '\0'))
     {
       /* POSIX.1-1990 says an empty name gets ENOENT;
 	 but `open' might like it fine.  */
       __set_errno (ENOENT);
-      return NULL;
+      return true;
     }
+  return false;
+}
 
+
+static bool
+need_isdir_precheck (void)
+{
 #ifdef O_DIRECTORY
   /* Test whether O_DIRECTORY works.  */
   if (o_directory_works == 0)
     tryopen_o_directory ();
 
   /* We can skip the expensive `stat' call if O_DIRECTORY works.  */
-  if (o_directory_works < 0)
+  return o_directory_works > 0;
 #endif
-    {
-      /* We first have to check whether the name is for a directory.  We
-	 cannot do this after the open() call since the open/close operation
-	 performed on, say, a tape device might have undesirable effects.  */
-      if (__builtin_expect (__xstat64 (_STAT_VER, name, &statbuf), 0) < 0)
-	return NULL;
-      if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode)))
-	{
-	  __set_errno (ENOTDIR);
-	  return NULL;
-	 }
-    }
+  return true;
+}
+
 
+static int
+opendir_oflags (void)
+{
   int flags = O_RDONLY|O_NDELAY|EXTRA_FLAGS|O_LARGEFILE;
 #ifdef O_CLOEXEC
   flags |= O_CLOEXEC;
 #endif
-  int fd;
-#if IS_IN (rtld)
-  assert (dfd == AT_FDCWD);
-  fd = open_not_cancel_2 (name, flags);
-#else
-  fd = openat_not_cancel_3 (dfd, name, flags);
-#endif
-  if (__builtin_expect (fd, 0) < 0)
+  return flags;
+}
+
+
+static DIR *
+opendir_tail (int fd)
+{
+  if (__glibc_unlikely (fd < 0))
     return NULL;
 
-#ifdef O_DIRECTORY
-  if (o_directory_works <= 0)
-#endif
+  /* Now make sure this really is a directory and nothing changed since the
+     `stat' call.  The S_ISDIR check is superfluous if O_DIRECTORY works,
+     but it's cheap and we need the stat call for st_blksize anyway.  */
+  struct stat64 statbuf;
+  if (__glibc_unlikely (__fxstat64 (_STAT_VER, fd, &statbuf) < 0))
+    goto lose;
+  if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode)))
     {
-      /* Now make sure this really is a directory and nothing changed since
-	 the `stat' call.  */
-      if (__builtin_expect (__fxstat64 (_STAT_VER, fd, &statbuf), 0) < 0)
-	goto lose;
+      __set_errno (ENOTDIR);
+    lose:
+      close_not_cancel_no_status (fd);
+      return NULL;
+    }
+
+  return __alloc_dir (fd, true, 0, &statbuf);
+}
+
+
+#if IS_IN (libc)
+DIR *
+internal_function
+__opendirat (int dfd, const char *name)
+{
+  if (__glibc_unlikely (invalid_name (name)))
+    return NULL;
+
+  if (need_isdir_precheck ())
+    {
+      /* We first have to check whether the name is for a directory.  We
+	 cannot do this after the open() call since the open/close operation
+	 performed on, say, a tape device might have undesirable effects.  */
+      struct stat64 statbuf;
+      if (__glibc_unlikely (__fxstatat64 (_STAT_VER, dfd, name,
+					  &statbuf, 0) < 0))
+	return NULL;
       if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode)))
 	{
 	  __set_errno (ENOTDIR);
-	lose:
-	  close_not_cancel_no_status (fd);
 	  return NULL;
 	}
-      statp = &statbuf;
     }
 
-  return __alloc_dir (fd, true, 0, statp);
+  return opendir_tail (openat_not_cancel_3 (dfd, name, opendir_oflags ()));
 }
+#endif
 
 
 /* Open a directory stream on NAME.  */
 DIR *
 __opendir (const char *name)
 {
-  return __opendirat (AT_FDCWD, name);
+  if (__glibc_unlikely (invalid_name (name)))
+    return NULL;
+
+  if (need_isdir_precheck ())
+    {
+      /* We first have to check whether the name is for a directory.  We
+	 cannot do this after the open() call since the open/close operation
+	 performed on, say, a tape device might have undesirable effects.  */
+      struct stat64 statbuf;
+      if (__glibc_unlikely (__xstat64 (_STAT_VER, name, &statbuf) < 0))
+	return NULL;
+      if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode)))
+	{
+	  __set_errno (ENOTDIR);
+	  return NULL;
+	}
+    }
+
+  return opendir_tail (open_not_cancel_2 (name, opendir_oflags ()));
 }
 weak_alias (__opendir, opendir)