[roland/opendir] Refactor opendir.
Commit Message
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
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
> 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.
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
@@ -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)