diff mbox series

[committed] libstdc++: Improve directory iterator abstractions for openat

Message ID 20220628111212.206509-1-jwakely@redhat.com
State New
Headers show
Series [committed] libstdc++: Improve directory iterator abstractions for openat | expand

Commit Message

Jonathan Wakely June 28, 2022, 11:12 a.m. UTC
Tested powerpc64le-linux, pushed to trunk.

-- >8 --

Currently the _Dir::open_subdir function decides whether to construct a
_Dir_base with just a pathname, or a file descriptor and pathname. But
that means it is tiughtly coupled to the implementation of
_Dir_base::openat, which is what actually decides whether to use a file
descriptor or not. If the derived class passes a file descriptor and
filename, but the base class expects a full path and ignores the file
descriptor, then recursive_directory_iterator cannot recurse.

This change introduces a new type that provides the union of all the
information available to the derived class (the full pathname, as well
as a file descriptor for a directory and another pathname relative to
that directory). This allows the derived class to be agnostic to how the
base class will use that information.

libstdc++-v3/ChangeLog:

	* src/c++17/fs_dir.cc (_Dir::dir_and_pathname):: Replace with
	current() returning _At_path.
	(_Dir::_Dir, _Dir::open_subdir, _Dir::do_unlink): Adjust.
	* src/filesystem/dir-common.h (_Dir_base::_At_path): New class.
	(_Dir_base::_Dir_Base, _Dir_base::openat): Use _At_path.
	* src/filesystem/dir.cc (_Dir::dir_and_pathname): Replace with
	current() returning _At_path.
	(_Dir::_Dir, _Dir::open_subdir): Adjust.
---
 libstdc++-v3/src/c++17/fs_dir.cc         | 29 ++++++-----
 libstdc++-v3/src/filesystem/dir-common.h | 66 +++++++++++++++++-------
 libstdc++-v3/src/filesystem/dir.cc       | 21 ++++----
 3 files changed, 73 insertions(+), 43 deletions(-)

Comments

Jonathan Wakely June 28, 2022, 2:55 p.m. UTC | #1
On Tue, 28 Jun 2022 at 15:24, Jonathan Wakely via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
> --- a/libstdc++-v3/src/filesystem/dir-common.h
> +++ b/libstdc++-v3/src/filesystem/dir-common.h
> @@ -25,6 +25,7 @@
>  #ifndef _GLIBCXX_DIR_COMMON_H
>  #define _GLIBCXX_DIR_COMMON_H 1
>
> +#include <stdint.h>  // uint32_t
>  #include <string.h>  // strcmp
>  #include <errno.h>
>  #if _GLIBCXX_FILESYSTEM_IS_WINDOWS
> @@ -91,12 +92,50 @@ is_permission_denied_error(int e)
>
>  struct _Dir_base
>  {
> +  // As well as the full pathname (including the directory iterator's path)
> +  // this type contains a file descriptor for a directory and a second pathname
> +  // relative to that directory. The file descriptor and relative pathname
> +  // can be used with POSIX openat and unlinkat.
> +  struct _At_path
> +  {
> +    // No file descriptor given, so interpret the pathname relative to the CWD.
> +    _At_path(const char* p) noexcept

I forgot to consistently use the char_type alias to support wchar_t
paths on Windows. Fixed by the attached patch.

Tested x86_64-linux and x86_64-mingw32, pushed to trunk.
commit bb1f266a7d602ffee4a070f586351bdfafcb6150
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jun 28 12:56:19 2022

    libstdc++: Fix filesystem build for Windows
    
    I only half remembered to use char_type instead of char for filesystem
    paths, so that it works with wchar_t on Windows. This fixes the
    bootstrap failure.
    
    libstdc++-v3/ChangeLog:
    
            * src/filesystem/dir-common.h (_Dir_base::_At_path):
            Use char_type consistently for paths.

diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h
index 4844b1ac453..228fab55afb 100644
--- a/libstdc++-v3/src/filesystem/dir-common.h
+++ b/libstdc++-v3/src/filesystem/dir-common.h
@@ -99,18 +99,22 @@ struct _Dir_base
   struct _At_path
   {
     // No file descriptor given, so interpret the pathname relative to the CWD.
-    _At_path(const char* p) noexcept
+    _At_path(const posix::char_type* p) noexcept
     : pathname(p), dir_fd(fdcwd()), offset(0)
     { }
 
-    _At_path(int fd, const char* p, size_t offset) noexcept
+    _At_path(int fd, const posix::char_type* p, size_t offset) noexcept
     : pathname(p), dir_fd(fd), offset(offset)
     { }
 
-    const char* path() const noexcept { return pathname; }
+    const posix::char_type*
+    path() const noexcept { return pathname; }
 
-    int dir() const noexcept { return dir_fd; }
-    const char* path_at_dir() const noexcept { return pathname + offset; }
+    int
+    dir() const noexcept { return dir_fd; }
+
+    const posix::char_type*
+    path_at_dir() const noexcept { return pathname + offset; }
 
   private:
     const posix::char_type* pathname; // Full path relative to CWD.
diff mbox series

Patch

diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc
index 025317b0a08..2258399da25 100644
--- a/libstdc++-v3/src/c++17/fs_dir.cc
+++ b/libstdc++-v3/src/c++17/fs_dir.cc
@@ -46,7 +46,7 @@  struct fs::_Dir : _Dir_base
 {
   _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow,
        [[maybe_unused]] bool filename_only, error_code& ec)
-  : _Dir_base(fdcwd(), p.c_str(), skip_permission_denied, nofollow, ec)
+  : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec)
   {
 #if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT && _GLIBCXX_HAVE_UNLINKAT
     if (filename_only)
@@ -117,18 +117,19 @@  struct fs::_Dir : _Dir_base
     return false;
   }
 
-  // Return a file descriptor for the directory and current entry's path.
-  // If dirfd is available, use it and return only the filename.
-  // Otherwise, return AT_FDCWD and return the full path.
-  pair<int, const posix::char_type*>
-  dir_and_pathname() const noexcept
+  // Return a pathname for the current directory entry, as an _At_path.
+  _Dir_base::_At_path
+  current() const noexcept
   {
     const fs::path& p = entry.path();
-#if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT
-    if (!p.empty())
-      return {::dirfd(this->dirp), std::prev(p.end())->c_str()};
+#if _GLIBCXX_HAVE_DIRFD
+    if (!p.empty()) [[__likely__]]
+      {
+	auto len = std::prev(p.end())->native().size();
+	return {::dirfd(this->dirp), p.c_str(), p.native().size() - len};
+      }
 #endif
-    return {this->fdcwd(), p.c_str()};
+    return p.c_str();
   }
 
   // Create a new _Dir for the directory this->entry.path().
@@ -136,8 +137,7 @@  struct fs::_Dir : _Dir_base
   open_subdir(bool skip_permission_denied, bool nofollow,
 	      error_code& ec) const noexcept
   {
-    auto [dirfd, pathname] = dir_and_pathname();
-    _Dir_base d(dirfd, pathname, skip_permission_denied, nofollow, ec);
+    _Dir_base d(current(), skip_permission_denied, nofollow, ec);
     // If this->path is empty, the new _Dir should have an empty path too.
     const fs::path& p = this->path.empty() ? this->path : this->entry.path();
     return _Dir(std::move(d), p);
@@ -147,8 +147,9 @@  struct fs::_Dir : _Dir_base
   do_unlink(bool is_directory, error_code& ec) const noexcept
   {
 #if _GLIBCXX_HAVE_UNLINKAT
-    auto [dirfd, pathname] = dir_and_pathname();
-    if (::unlinkat(dirfd, pathname, is_directory ? AT_REMOVEDIR : 0) == -1)
+    const auto atp = current();
+    if (::unlinkat(atp.dir(), atp.path_at_dir(),
+		   is_directory ? AT_REMOVEDIR : 0) == -1)
       {
 	ec.assign(errno, std::generic_category());
 	return false;
diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h
index 669780ea23f..4844b1ac453 100644
--- a/libstdc++-v3/src/filesystem/dir-common.h
+++ b/libstdc++-v3/src/filesystem/dir-common.h
@@ -25,6 +25,7 @@ 
 #ifndef _GLIBCXX_DIR_COMMON_H
 #define _GLIBCXX_DIR_COMMON_H 1
 
+#include <stdint.h>  // uint32_t
 #include <string.h>  // strcmp
 #include <errno.h>
 #if _GLIBCXX_FILESYSTEM_IS_WINDOWS
@@ -91,12 +92,50 @@  is_permission_denied_error(int e)
 
 struct _Dir_base
 {
+  // As well as the full pathname (including the directory iterator's path)
+  // this type contains a file descriptor for a directory and a second pathname
+  // relative to that directory. The file descriptor and relative pathname
+  // can be used with POSIX openat and unlinkat.
+  struct _At_path
+  {
+    // No file descriptor given, so interpret the pathname relative to the CWD.
+    _At_path(const char* p) noexcept
+    : pathname(p), dir_fd(fdcwd()), offset(0)
+    { }
+
+    _At_path(int fd, const char* p, size_t offset) noexcept
+    : pathname(p), dir_fd(fd), offset(offset)
+    { }
+
+    const char* path() const noexcept { return pathname; }
+
+    int dir() const noexcept { return dir_fd; }
+    const char* path_at_dir() const noexcept { return pathname + offset; }
+
+  private:
+    const posix::char_type* pathname; // Full path relative to CWD.
+    int dir_fd; // A directory descriptor (either the parent dir, or AT_FDCWD).
+    uint32_t offset; // Offset into pathname for the part relative to dir_fd.
+
+    // Special value representing the current working directory.
+    // Not a valid file descriptor for an open directory stream.
+    static constexpr int
+    fdcwd() noexcept
+    {
+#ifdef AT_FDCWD
+      return AT_FDCWD;
+#else
+      return -1; // Use invalid fd if AT_FDCWD isn't supported.
+#endif
+    }
+  };
+
   // If no error occurs then dirp is non-null,
   // otherwise null (even if a permission denied error is ignored).
-  _Dir_base(int fd, const posix::char_type* pathname,
+  _Dir_base(const _At_path& atp,
 	    bool skip_permission_denied, bool nofollow,
 	    error_code& ec) noexcept
-  : dirp(_Dir_base::openat(fd, pathname, nofollow))
+  : dirp(_Dir_base::openat(atp, nofollow))
   {
     if (dirp)
       ec.clear();
@@ -143,16 +182,6 @@  struct _Dir_base
       }
   }
 
-  static constexpr int
-  fdcwd() noexcept
-  {
-#ifdef AT_FDCWD
-    return AT_FDCWD;
-#else
-    return -1; // Use invalid fd if AT_FDCWD isn't supported.
-#endif
-  }
-
   static bool is_dot_or_dotdot(const char* s) noexcept
   { return !strcmp(s, ".") || !strcmp(s, ".."); }
 
@@ -174,7 +203,7 @@  struct _Dir_base
   }
 
   static posix::DIR*
-  openat(int fd, const posix::char_type* pathname, bool nofollow)
+  openat(const _At_path& atp, bool nofollow)
   {
 #if _GLIBCXX_HAVE_FDOPENDIR && defined O_RDONLY && defined O_DIRECTORY \
     && ! _GLIBCXX_FILESYSTEM_IS_WINDOWS
@@ -198,16 +227,17 @@  struct _Dir_base
     nofollow = false;
 #endif
 
+    int fd;
 
-#if _GLIBCXX_HAVE_OPENAT && defined AT_FDCWD
-    fd = ::openat(fd, pathname, flags);
+#if _GLIBCXX_HAVE_OPENAT
+    fd = ::openat(atp.dir(), atp.path_at_dir(), flags);
 #else
     // If we cannot use openat, there's no benefit to using posix::open unless
     // we will use O_NOFOLLOW, so just use the simpler posix::opendir.
     if (!nofollow)
-      return posix::opendir(pathname);
+      return posix::opendir(atp.path());
 
-    fd = ::open(pathname, flags);
+    fd = ::open(atp.path(), flags);
 #endif
 
     if (fd == -1)
@@ -220,7 +250,7 @@  struct _Dir_base
     errno = err;
     return nullptr;
 #else
-    return posix::opendir(pathname);
+    return posix::opendir(atp.path());
 #endif
   }
 
diff --git a/libstdc++-v3/src/filesystem/dir.cc b/libstdc++-v3/src/filesystem/dir.cc
index e64489162e5..a66a6773580 100644
--- a/libstdc++-v3/src/filesystem/dir.cc
+++ b/libstdc++-v3/src/filesystem/dir.cc
@@ -53,7 +53,7 @@  struct fs::_Dir : std::filesystem::_Dir_base
 {
   _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow,
        error_code& ec)
-  : _Dir_base(this->fdcwd(), p.c_str(), skip_permission_denied, nofollow, ec)
+  : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec)
   {
     if (!ec)
       path = p;
@@ -113,17 +113,17 @@  struct fs::_Dir : std::filesystem::_Dir_base
     return false;
   }
 
-  // Return a file descriptor for the directory and current entry's path.
-  // If dirfd is available, use it and return only the filename.
-  // Otherwise, return AT_FDCWD and return the full path.
-  pair<int, const posix::char_type*>
-  dir_and_pathname() const noexcept
+  // Return a pathname for the current directory entry, as an _At_path.
+  _Dir_base::_At_path
+  current() const noexcept
   {
     const fs::path& p = entry.path();
-#if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT
-    return {::dirfd(this->dirp), std::prev(p.end())->c_str()};
+#if _GLIBCXX_HAVE_DIRFD
+    auto len = std::prev(p.end())->native().size();
+    return {::dirfd(this->dirp), p.c_str(), p.native().size() - len};
+#else
+    return p.c_str();
 #endif
-    return {this->fdcwd(), p.c_str()};
   }
 
   // Create a new _Dir for the directory this->entry.path().
@@ -131,8 +131,7 @@  struct fs::_Dir : std::filesystem::_Dir_base
   open_subdir(bool skip_permission_denied, bool nofollow,
 	      error_code& ec) noexcept
   {
-    auto [dirfd, pathname] = dir_and_pathname();
-    _Dir_base d(dirfd, pathname, skip_permission_denied, nofollow, ec);
+    _Dir_base d(current(), skip_permission_denied, nofollow, ec);
     return _Dir(std::move(d), entry.path());
   }