On Thu, 23 Jun 2022 at 15:05, Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Jun 23, 2022, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> > On Thu, 23 Jun 2022 at 12:08, Alexandre Oliva <ol...@adacore.com> wrote:
> >>
> >> On Jun 22, 2022, Jonathan Wakely <jwak...@redhat.com> wrote:
> >>
> >> > There are other interactions between AT_CDCWD and ::openat not covered
> >> > by this patch. I this this also needs to check HAVE_OPENAT:
> >>
> >> Here's an updated version, tested with this additional change.
>
> > Did this improve your test results for directory iterators?
>
> 'fraid the bad results I posted earlier today had this patch in. I
> can't tell whether it improved anything because I didn't save earlier
> results to compare.
>
> > In the unlikely even that the target has ::unlinkat but not ::openat
>
> c++config.h on the target says:
>
> /* #undef _GLIBCXX_HAVE_UNLINKAT */
>
> Thanks for the concern,
Here's what I'm thinking of so that the derived _Dir class doesn't
need to know whether the base _Dir_base::openat function will actually
use ::openat or not.
It defines a new _At_path type which contains a {fd, path} pair (to be
used together by openat and unlinkat) and a separate path to be used
on its own. This allows identifying a file within a directory
unambiguously, without being concerned with whether HAVE_OPENAT and
HAVE_UNLINKAT are defined.
With this change I don't think your patch to make dir_and_pathname()
check HAVE_OPENAT is needed.
This passes tests on x86_64-linux.
diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc
index c67fe76bc14..fb752239e1f 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
if (filename_only)
@@ -120,15 +120,15 @@ struct fs::_Dir : _Dir_base
// 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*>
+ _At_path
dir_and_pathname() const noexcept
{
const fs::path& p = entry.path();
#if _GLIBCXX_HAVE_DIRFD
if (!p.empty())
- return {::dirfd(this->dirp), std::prev(p.end())->c_str()};
+ return {::dirfd(this->dirp), std::prev(p.end())->c_str(), p.c_str()};
#endif
- return {this->fdcwd(), p.c_str()};
+ return p.c_str();
}
// Create a new _Dir for the directory this->entry.path().
@@ -136,8 +136,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(dir_and_pathname(), 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,7 +146,7 @@ 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();
+ auto [dirfd, pathname, _] = dir_and_pathname();
if (::unlinkat(dirfd, pathname, is_directory ? AT_REMOVEDIR : 0) == -1)
{
ec.assign(errno, std::generic_category());
diff --git a/libstdc++-v3/src/filesystem/dir-common.h
b/libstdc++-v3/src/filesystem/dir-common.h
index 365fd527f4d..5f7f4909ad3 100644
--- a/libstdc++-v3/src/filesystem/dir-common.h
+++ b/libstdc++-v3/src/filesystem/dir-common.h
@@ -91,12 +91,37 @@ is_permission_denied_error(int e)
struct _Dir_base
{
+ struct _At_path
+ {
+ _At_path(const char* p) noexcept
+ : dir_fd(fdcwd()), path_from_dir(p), pathname(p)
+ { }
+
+ _At_path(int fd, const char* p1, const char* p2)
+ : dir_fd(fd), path_from_dir(p1), pathname(p2)
+ { }
+
+ int dir_fd; // A directory descriptor (either the parent dir, or AT_FDCWD).
+ const posix::char_type* path_from_dir; // Path relative to dir_fd.
+ const posix::char_type* pathname; // Full path relative to CWD.
+
+ 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 +168,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 +189,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 +213,17 @@ struct _Dir_base
nofollow = false;
#endif
+ int fd;
#ifdef AT_FDCWD
- fd = ::openat(fd, pathname, flags);
+ fd = ::openat(atp.dir_fd, atp.path_from_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.pathname);
- fd = ::open(pathname, flags);
+ fd = ::open(atp.pathname, flags);
#endif
if (fd == -1)
@@ -220,7 +236,7 @@ struct _Dir_base
errno = err;
return nullptr;
#else
- return posix::opendir(pathname);
+ return posix::opendir(atp.pathname);
#endif
}
diff --git a/libstdc++-v3/src/filesystem/dir.cc
b/libstdc++-v3/src/filesystem/dir.cc
index b451902c4a1..c0202849bc4 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;
@@ -116,14 +116,14 @@ struct fs::_Dir : std::filesystem::_Dir_base
// 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*>
+ _At_path
dir_and_pathname() const noexcept
{
const fs::path& p = entry.path();
#if _GLIBCXX_HAVE_DIRFD
- return {::dirfd(this->dirp), std::prev(p.end())->c_str()};
+ return {::dirfd(this->dirp), std::prev(p.end())->c_str(), p.c_str()};
#endif
- return {this->fdcwd(), p.c_str()};
+ return 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(dir_and_pathname(), skip_permission_denied, nofollow, ec);
return _Dir(std::move(d), entry.path());
}
