From 7ccc0d591f3eb879af8eae95c994405a1172d210 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 1 May 2025 09:50:53 +0200 Subject: [PATCH 1/2] add DirectoryIterator to re-throw std::filesystem::filesystem_error Co-authored-by: Sergei Zimmerman <145775305+xokdvium@users.noreply.github.com> --- src/libutil-tests/file-system.cc | 21 ++++++++ src/libutil/file-system.cc | 28 ++++++++++ src/libutil/include/nix/util/file-system.hh | 60 +++++++++++++++++++++ 3 files changed, 109 insertions(+) diff --git a/src/libutil-tests/file-system.cc b/src/libutil-tests/file-system.cc index 9e4a27750..2d1058c4f 100644 --- a/src/libutil-tests/file-system.cc +++ b/src/libutil-tests/file-system.cc @@ -297,4 +297,25 @@ TEST(chmodIfNeeded, nonexistent) ASSERT_THROW(chmodIfNeeded("/schnitzel/darmstadt/pommes", 0755), SysError); } +/* ---------------------------------------------------------------------------- + * DirectoryIterator + * --------------------------------------------------------------------------*/ + +TEST(DirectoryIterator, works) +{ + auto tmpDir = nix::createTempDir(); + nix::AutoDelete delTmpDir(tmpDir, true); + + nix::writeFile(tmpDir + "/somefile", ""); + + for (auto path : DirectoryIterator(tmpDir)) { + ASSERT_EQ(path.path().string(), tmpDir + "/somefile"); + } +} + +TEST(DirectoryIterator, nonexistent) +{ + ASSERT_THROW(DirectoryIterator("/schnitzel/darmstadt/pommes"), SysError); +} + } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 071aecc9d..b677c8ccb 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -43,6 +43,34 @@ namespace fs { } } +DirectoryIterator::DirectoryIterator(const std::filesystem::path& p) { + try { + // **Attempt to create the underlying directory_iterator** + it_ = std::filesystem::directory_iterator(p); + } catch (const std::filesystem::filesystem_error& e) { + // **Catch filesystem_error and throw SysError** + // Adapt the error message as needed for SysError + throw SysError("cannot read directory %s", p); + } +} + +DirectoryIterator& DirectoryIterator::operator++() { + // **Attempt to increment the underlying iterator** + std::error_code ec; + it_.increment(ec); + if (ec) { + // Try to get path info if possible, might fail if iterator is bad + try { + if (it_ != std::filesystem::directory_iterator{}) { + throw SysError("cannot read directory past %s: %s", it_->path(), ec.message()); + } + } catch (...) { + throw SysError("cannot read directory"); + } + } + return *this; +} + bool isAbsolute(PathView path) { return fs::path { path }.is_absolute(); diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh index 5c2416750..9afab46ab 100644 --- a/src/libutil/include/nix/util/file-system.hh +++ b/src/libutil/include/nix/util/file-system.hh @@ -376,4 +376,64 @@ extern PathFilter defaultPathFilter; */ bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask = S_IRWXU | S_IRWXG | S_IRWXO); +/** + * @brief A directory iterator that can be used to iterate over the + * contents of a directory. It is similar to std::filesystem::directory_iterator + * but throws NixError on failure instead of std::filesystem::filesystem_error. + */ +class DirectoryIterator { +public: + // --- Iterator Traits --- + using iterator_category = std::input_iterator_tag; + using value_type = std::filesystem::directory_entry; + using difference_type = std::ptrdiff_t; + using pointer = const std::filesystem::directory_entry*; + using reference = const std::filesystem::directory_entry&; + + // Default constructor (represents end iterator) + DirectoryIterator() noexcept = default; + + // Constructor taking a path + explicit DirectoryIterator(const std::filesystem::path& p); + + reference operator*() const { + // Accessing the value itself doesn't typically throw filesystem_error + // after successful construction/increment, but underlying operations might. + // If directory_entry methods called via -> could throw, add try-catch there. + return *it_; + } + + pointer operator->() const { + return &(*it_); + } + + + DirectoryIterator& operator++(); + + // Postfix increment operator + DirectoryIterator operator++(int) { + DirectoryIterator temp = *this; + ++(*this); // Uses the prefix increment's try-catch logic + return temp; + } + + // Equality comparison + friend bool operator==(const DirectoryIterator& a, const DirectoryIterator& b) noexcept { + return a.it_ == b.it_; + } + + // Inequality comparison + friend bool operator!=(const DirectoryIterator& a, const DirectoryIterator& b) noexcept { + return !(a == b); + } + + // Allow direct use in range-based for loops if iterating over an instance + DirectoryIterator begin() const { return *this; } + DirectoryIterator end() const { return DirectoryIterator{}; } + + +private: + std::filesystem::directory_iterator it_; +}; + } From 1c4496f4e5c51b1036087f95889033a71b33edf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 1 May 2025 09:54:14 +0200 Subject: [PATCH 2/2] replace all instances of std::filesystem::directory_iterator with DirectoryIterator --- src/libcmd/repl.cc | 3 +- src/libmain/plugin.cc | 7 ++- src/libstore/builtins/buildenv.cc | 8 +-- src/libstore/builtins/unpack-channel.cc | 10 +--- src/libstore/gc.cc | 4 +- src/libstore/local-binary-cache-store.cc | 2 +- src/libstore/local-store.cc | 4 +- src/libstore/posix-fs-canonicalise.cc | 2 +- src/libstore/profiles.cc | 2 +- src/libutil/file-system.cc | 4 +- src/libutil/linux/cgroup.cc | 2 +- src/libutil/posix-source-accessor.cc | 60 +++++++++---------- src/libutil/unix/file-descriptor.cc | 3 +- .../nix-collect-garbage.cc | 2 +- src/nix/prefetch.cc | 4 +- src/nix/run.cc | 2 +- 16 files changed, 55 insertions(+), 64 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index c5a95268b..8bbda1a4b 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -244,14 +244,13 @@ StringSet NixRepl::completePrefix(const std::string & prefix) try { auto dir = std::string(cur, 0, slash); auto prefix2 = std::string(cur, slash + 1); - for (auto & entry : std::filesystem::directory_iterator{dir == "" ? "/" : dir}) { + for (auto & entry : DirectoryIterator{dir == "" ? "/" : dir}) { checkInterrupt(); auto name = entry.path().filename().string(); if (name[0] != '.' && hasPrefix(name, prefix2)) completions.insert(prev + entry.path().string()); } } catch (Error &) { - } catch (std::filesystem::filesystem_error &) { } } else if ((dot = cur.rfind('.')) == std::string::npos) { /* This is a variable name; look it up in the current scope. */ diff --git a/src/libmain/plugin.cc b/src/libmain/plugin.cc index 63ed650a7..812506a86 100644 --- a/src/libmain/plugin.cc +++ b/src/libmain/plugin.cc @@ -6,6 +6,7 @@ #include "nix/util/config-global.hh" #include "nix/util/signals.hh" +#include "nix/util/file-system.hh" namespace nix { @@ -77,13 +78,13 @@ void initPlugins() for (const auto & pluginFile : pluginSettings.pluginFiles.get()) { std::vector pluginFiles; try { - auto ents = std::filesystem::directory_iterator{pluginFile}; + auto ents = DirectoryIterator{pluginFile}; for (const auto & ent : ents) { checkInterrupt(); pluginFiles.emplace_back(ent.path()); } - } catch (std::filesystem::filesystem_error & e) { - if (e.code() != std::errc::not_a_directory) + } catch (SysError & e) { + if (e.errNo != ENOTDIR) throw; pluginFiles.emplace_back(pluginFile); } diff --git a/src/libstore/builtins/buildenv.cc b/src/libstore/builtins/buildenv.cc index c3b80bb0b..74036d22e 100644 --- a/src/libstore/builtins/buildenv.cc +++ b/src/libstore/builtins/buildenv.cc @@ -18,12 +18,12 @@ struct State /* For each activated package, create symlinks */ static void createLinks(State & state, const Path & srcDir, const Path & dstDir, int priority) { - std::filesystem::directory_iterator srcFiles; + DirectoryIterator srcFiles; try { - srcFiles = std::filesystem::directory_iterator{srcDir}; - } catch (std::filesystem::filesystem_error & e) { - if (e.code() == std::errc::not_a_directory) { + srcFiles = DirectoryIterator{srcDir}; + } catch (SysError & e) { + if (e.errNo == ENOTDIR) { warn("not including '%s' in the user environment because it's not a directory", srcDir); return; } diff --git a/src/libstore/builtins/unpack-channel.cc b/src/libstore/builtins/unpack-channel.cc index f6be21e35..07b1d5773 100644 --- a/src/libstore/builtins/unpack-channel.cc +++ b/src/libstore/builtins/unpack-channel.cc @@ -29,13 +29,9 @@ void builtinUnpackChannel( size_t fileCount; std::string fileName; - try { - auto entries = fs::directory_iterator{out}; - fileName = entries->path().string(); - fileCount = std::distance(fs::begin(entries), fs::end(entries)); - } catch (fs::filesystem_error &) { - throw SysError("failed to read directory %1%", out.string()); - } + auto entries = DirectoryIterator{out}; + fileName = entries->path().string(); + fileCount = std::distance(entries.begin(), entries.end()); if (fileCount != 1) throw Error("channel tarball '%s' contains more than one file", src); diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index db91e5213..f6a4124ff 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -164,7 +164,7 @@ void LocalStore::findTempRoots(Roots & tempRoots, bool censor) { /* Read the `temproots' directory for per-process temporary root files. */ - for (auto & i : std::filesystem::directory_iterator{tempRootsDir}) { + for (auto & i : DirectoryIterator{tempRootsDir}) { checkInterrupt(); auto name = i.path().filename().string(); if (name[0] == '.') { @@ -232,7 +232,7 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R type = std::filesystem::symlink_status(path).type(); if (type == std::filesystem::file_type::directory) { - for (auto & i : std::filesystem::directory_iterator{path}) { + for (auto & i : DirectoryIterator{path}) { checkInterrupt(); findRoots(i.path().string(), i.symlink_status().type(), roots); } diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 212eacc8c..4d1be5785 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -84,7 +84,7 @@ protected: { StorePathSet paths; - for (auto & entry : std::filesystem::directory_iterator{binaryCacheDir}) { + for (auto & entry : DirectoryIterator{binaryCacheDir}) { checkInterrupt(); auto name = entry.path().filename().string(); if (name.size() != 40 || diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 1c6d6bced..ade209b8a 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1382,7 +1382,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) printInfo("checking link hashes..."); - for (auto & link : std::filesystem::directory_iterator{linksDir}) { + for (auto & link : DirectoryIterator{linksDir}) { checkInterrupt(); auto name = link.path().filename(); printMsg(lvlTalkative, "checking contents of '%s'", name); @@ -1475,7 +1475,7 @@ LocalStore::VerificationResult LocalStore::verifyAllValidPaths(RepairFlag repair database and the filesystem) in the loop below, in order to catch invalid states. */ - for (auto & i : std::filesystem::directory_iterator{realStoreDir.to_string()}) { + for (auto & i : DirectoryIterator{realStoreDir.to_string()}) { checkInterrupt(); try { storePathsInStoreDir.insert({i.path().filename().string()}); diff --git a/src/libstore/posix-fs-canonicalise.cc b/src/libstore/posix-fs-canonicalise.cc index aeb35eab5..792fe5c76 100644 --- a/src/libstore/posix-fs-canonicalise.cc +++ b/src/libstore/posix-fs-canonicalise.cc @@ -136,7 +136,7 @@ static void canonicalisePathMetaData_( #endif if (S_ISDIR(st.st_mode)) { - for (auto & i : std::filesystem::directory_iterator{path}) { + for (auto & i : DirectoryIterator{path}) { checkInterrupt(); canonicalisePathMetaData_( i.path().string(), diff --git a/src/libstore/profiles.cc b/src/libstore/profiles.cc index bd24332cb..b5161b79f 100644 --- a/src/libstore/profiles.cc +++ b/src/libstore/profiles.cc @@ -38,7 +38,7 @@ std::pair> findGenerations(Path pro std::filesystem::path profileDir = dirOf(profile); auto profileName = std::string(baseNameOf(profile)); - for (auto & i : std::filesystem::directory_iterator{profileDir}) { + for (auto & i : DirectoryIterator{profileDir}) { checkInterrupt(); if (auto n = parseName(profileName, i.path().filename().string())) { auto path = i.path().string(); diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index b677c8ccb..839e6ff29 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -387,7 +387,7 @@ void recursiveSync(const Path & path) while (!dirsToEnumerate.empty()) { auto currentDir = dirsToEnumerate.back(); dirsToEnumerate.pop_back(); - for (auto & entry : std::filesystem::directory_iterator(currentDir)) { + for (auto & entry : DirectoryIterator(currentDir)) { auto st = entry.symlink_status(); if (fs::is_directory(st)) { dirsToEnumerate.emplace_back(entry.path()); @@ -691,7 +691,7 @@ void copyFile(const fs::path & from, const fs::path & to, bool andDelete) fs::copy(from, to, fs::copy_options::copy_symlinks | fs::copy_options::overwrite_existing); } else if (fs::is_directory(fromStatus)) { fs::create_directory(to); - for (auto & entry : fs::directory_iterator(from)) { + for (auto & entry : DirectoryIterator(from)) { copyFile(entry, to / entry.path().filename(), andDelete); } } else { diff --git a/src/libutil/linux/cgroup.cc b/src/libutil/linux/cgroup.cc index e8e2fdfc7..4acfe82f1 100644 --- a/src/libutil/linux/cgroup.cc +++ b/src/libutil/linux/cgroup.cc @@ -65,7 +65,7 @@ static CgroupStats destroyCgroup(const std::filesystem::path & cgroup, bool retu /* Otherwise, manually kill every process in the subcgroups and this cgroup. */ - for (auto & entry : std::filesystem::directory_iterator{cgroup}) { + for (auto & entry : DirectoryIterator{cgroup}) { checkInterrupt(); if (entry.symlink_status().type() != std::filesystem::file_type::directory) continue; destroyCgroup(cgroup / entry.path().filename(), false); diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc index 5c7b4654b..773540e6a 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -138,42 +138,38 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & { assertNoSymlinks(path); DirEntries res; - try { - for (auto & entry : std::filesystem::directory_iterator{makeAbsPath(path)}) { - checkInterrupt(); - auto type = [&]() -> std::optional { - std::filesystem::file_type nativeType; - try { - nativeType = entry.symlink_status().type(); - } catch (std::filesystem::filesystem_error & e) { - // We cannot always stat the child. (Ideally there is no - // stat because the native directory entry has the type - // already, but this isn't always the case.) - if (e.code() == std::errc::permission_denied || e.code() == std::errc::operation_not_permitted) - return std::nullopt; - else throw; - } + for (auto & entry : DirectoryIterator{makeAbsPath(path)}) { + checkInterrupt(); + auto type = [&]() -> std::optional { + std::filesystem::file_type nativeType; + try { + nativeType = entry.symlink_status().type(); + } catch (std::filesystem::filesystem_error & e) { + // We cannot always stat the child. (Ideally there is no + // stat because the native directory entry has the type + // already, but this isn't always the case.) + if (e.code() == std::errc::permission_denied || e.code() == std::errc::operation_not_permitted) + return std::nullopt; + else throw; + } - // cannot exhaustively enumerate because implementation-specific - // additional file types are allowed. + // cannot exhaustively enumerate because implementation-specific + // additional file types are allowed. #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wswitch-enum" - switch (nativeType) { - case std::filesystem::file_type::regular: return Type::tRegular; break; - case std::filesystem::file_type::symlink: return Type::tSymlink; break; - case std::filesystem::file_type::directory: return Type::tDirectory; break; - case std::filesystem::file_type::character: return Type::tChar; break; - case std::filesystem::file_type::block: return Type::tBlock; break; - case std::filesystem::file_type::fifo: return Type::tFifo; break; - case std::filesystem::file_type::socket: return Type::tSocket; break; - default: return tUnknown; - } -#pragma GCC diagnostic pop - }(); - res.emplace(entry.path().filename().string(), type); + switch (nativeType) { + case std::filesystem::file_type::regular: return Type::tRegular; break; + case std::filesystem::file_type::symlink: return Type::tSymlink; break; + case std::filesystem::file_type::directory: return Type::tDirectory; break; + case std::filesystem::file_type::character: return Type::tChar; break; + case std::filesystem::file_type::block: return Type::tBlock; break; + case std::filesystem::file_type::fifo: return Type::tFifo; break; + case std::filesystem::file_type::socket: return Type::tSocket; break; + default: return tUnknown; } - } catch (std::filesystem::filesystem_error & e) { - throw SysError("reading directory %1%", showPath(path)); +#pragma GCC diagnostic pop + }(); + res.emplace(entry.path().filename().string(), type); } return res; } diff --git a/src/libutil/unix/file-descriptor.cc b/src/libutil/unix/file-descriptor.cc index 73ee49982..e6d0c255d 100644 --- a/src/libutil/unix/file-descriptor.cc +++ b/src/libutil/unix/file-descriptor.cc @@ -191,7 +191,7 @@ void unix::closeExtraFDs() #ifdef __linux__ try { - for (auto & s : std::filesystem::directory_iterator{"/proc/self/fd"}) { + for (auto & s : DirectoryIterator{"/proc/self/fd"}) { checkInterrupt(); auto fd = std::stoi(s.path().filename()); if (fd > MAX_KEPT_FD) { @@ -201,7 +201,6 @@ void unix::closeExtraFDs() } return; } catch (SysError &) { - } catch (std::filesystem::filesystem_error &) { } #endif diff --git a/src/nix-collect-garbage/nix-collect-garbage.cc b/src/nix-collect-garbage/nix-collect-garbage.cc index 3a84d97aa..8e4fc30fb 100644 --- a/src/nix-collect-garbage/nix-collect-garbage.cc +++ b/src/nix-collect-garbage/nix-collect-garbage.cc @@ -30,7 +30,7 @@ void removeOldGenerations(fs::path dir) bool canWrite = access(dir.string().c_str(), W_OK) == 0; - for (auto & i : fs::directory_iterator{dir}) { + for (auto & i : DirectoryIterator{dir}) { checkInterrupt(); auto path = i.path().string(); diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc index 6819e596d..d56b73bf1 100644 --- a/src/nix/prefetch.cc +++ b/src/nix/prefetch.cc @@ -116,11 +116,11 @@ std::tuple prefetchFile( createDirs(unpacked); unpackTarfile(tmpFile.string(), unpacked); - auto entries = std::filesystem::directory_iterator{unpacked}; + auto entries = DirectoryIterator{unpacked}; /* If the archive unpacks to a single file/directory, then use that as the top-level. */ tmpFile = entries->path(); - auto fileCount = std::distance(entries, std::filesystem::directory_iterator{}); + auto fileCount = std::distance(entries, DirectoryIterator{}); if (fileCount != 1) { /* otherwise, use the directory itself */ tmpFile = unpacked; diff --git a/src/nix/run.cc b/src/nix/run.cc index 146ae9ec9..10f5026cd 100644 --- a/src/nix/run.cc +++ b/src/nix/run.cc @@ -179,7 +179,7 @@ void chrootHelper(int argc, char * * argv) if (mount(realStoreDir.c_str(), (tmpDir + storeDir).c_str(), "", MS_BIND, 0) == -1) throw SysError("mounting '%s' on '%s'", realStoreDir, storeDir); - for (const auto & entry : fs::directory_iterator{"/"}) { + for (const auto & entry : DirectoryIterator{"/"}) { checkInterrupt(); const auto & src = entry.path(); fs::path dst = tmpDir / entry.path().filename();