From 1db7d1b8404978c4ae00d6ec5d17eca6821d94f6 Mon Sep 17 00:00:00 2001 From: siddhantCodes Date: Sun, 12 May 2024 17:42:18 +0530 Subject: [PATCH 1/2] inline the usage of `nix::readDirectory` `nix::readDirectory` is removed. `std::filesystem::directory_iterator` is used directly in places that used this util. --- src/libcmd/repl.cc | 2 +- src/libstore/builtins/buildenv.cc | 4 ++-- src/libstore/gc.cc | 4 ++-- src/libstore/globals.cc | 2 +- src/libstore/local-binary-cache-store.cc | 2 +- src/libstore/local-store.cc | 4 ++-- src/libstore/posix-fs-canonicalise.cc | 3 +-- src/libstore/profiles.cc | 2 +- src/libstore/unix/builtins/unpack-channel.cc | 8 +++++--- src/libutil/file-system.cc | 14 -------------- src/libutil/file-system.hh | 6 ------ src/libutil/linux/cgroup.cc | 2 +- src/libutil/posix-source-accessor.cc | 2 +- src/libutil/unix/file-descriptor.cc | 2 +- src/nix-collect-garbage/nix-collect-garbage.cc | 2 +- src/nix/flake.cc | 2 +- src/nix/prefetch.cc | 7 ++++--- src/nix/run.cc | 2 +- 18 files changed, 26 insertions(+), 44 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 8a9155ab6d7..a069dd52cb8 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -259,7 +259,7 @@ 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 : readDirectory(dir == "" ? "/" : dir)) { + for (auto & entry : std::filesystem::directory_iterator{dir == "" ? "/" : dir}) { auto name = entry.path().filename().string(); if (name[0] != '.' && hasPrefix(name, prefix2)) completions.insert(prev + entry.path().string()); diff --git a/src/libstore/builtins/buildenv.cc b/src/libstore/builtins/buildenv.cc index 5fcdf6f157f..ab35c861d34 100644 --- a/src/libstore/builtins/buildenv.cc +++ b/src/libstore/builtins/buildenv.cc @@ -17,10 +17,10 @@ struct State /* For each activated package, create symlinks */ static void createLinks(State & state, const Path & srcDir, const Path & dstDir, int priority) { - std::vector srcFiles; + std::filesystem::directory_iterator srcFiles; try { - srcFiles = readDirectory(srcDir); + srcFiles = std::filesystem::directory_iterator{srcDir}; } catch (std::filesystem::filesystem_error & e) { if (e.code() == std::errc::not_a_directory) { warn("not including '%s' in the user environment because it's not a directory", srcDir); diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 3cd4fb839db..59566d01033 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -161,7 +161,7 @@ void LocalStore::findTempRoots(Roots & tempRoots, bool censor) { /* Read the `temproots' directory for per-process temporary root files. */ - for (auto & i : readDirectory(tempRootsDir)) { + for (auto & i : std::filesystem::directory_iterator{tempRootsDir}) { auto name = i.path().filename().string(); if (name[0] == '.') { // Ignore hidden files. Some package managers (notably portage) create @@ -228,7 +228,7 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R type = getFileType(path); if (type == std::filesystem::file_type::directory) { - for (auto & i : readDirectory(path)) + for (auto & i : std::filesystem::directory_iterator{path}) findRoots(i.path().string(), i.symlink_status().type(), roots); } diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 4df2880e6d4..d9cab2fb8bf 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -345,7 +345,7 @@ void initPlugins() for (const auto & pluginFile : settings.pluginFiles.get()) { std::vector pluginFiles; try { - auto ents = readDirectory(pluginFile); + auto ents = std::filesystem::directory_iterator{pluginFile}; for (const auto & ent : ents) pluginFiles.emplace_back(ent.path()); } catch (std::filesystem::filesystem_error & e) { diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 3a48f448095..5cd6af41232 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -83,7 +83,7 @@ class LocalBinaryCacheStore : public virtual LocalBinaryCacheStoreConfig, public { StorePathSet paths; - for (auto & entry : readDirectory(binaryCacheDir)) { + for (auto & entry : std::filesystem::directory_iterator{binaryCacheDir}) { auto name = entry.path().filename().string(); if (name.size() != 40 || !hasSuffix(name, ".narinfo")) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 800e6930971..106bd9fa15a 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1406,7 +1406,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) printInfo("checking link hashes..."); - for (auto & link : readDirectory(linksDir)) { + for (auto & link : std::filesystem::directory_iterator{linksDir}) { auto name = link.path().filename(); printMsg(lvlTalkative, "checking contents of '%s'", name); PosixSourceAccessor accessor; @@ -1498,7 +1498,7 @@ LocalStore::VerificationResult LocalStore::verifyAllValidPaths(RepairFlag repair database and the filesystem) in the loop below, in order to catch invalid states. */ - for (auto & i : readDirectory(realStoreDir)) { + for (auto & i : std::filesystem::directory_iterator{realStoreDir.to_string()}) { try { storePathsInStoreDir.insert({i.path().filename().string()}); } catch (BadStorePath &) { } diff --git a/src/libstore/posix-fs-canonicalise.cc b/src/libstore/posix-fs-canonicalise.cc index d70ef6faefe..d8bae13f5f9 100644 --- a/src/libstore/posix-fs-canonicalise.cc +++ b/src/libstore/posix-fs-canonicalise.cc @@ -144,8 +144,7 @@ static void canonicalisePathMetaData_( #endif if (S_ISDIR(st.st_mode)) { - std::vector entries = readDirectory(path); - for (auto & i : entries) + for (auto & i : std::filesystem::directory_iterator{path}) canonicalisePathMetaData_( i.path().string(), #ifndef _WIN32 diff --git a/src/libstore/profiles.cc b/src/libstore/profiles.cc index fa80267039b..d0da9626213 100644 --- a/src/libstore/profiles.cc +++ b/src/libstore/profiles.cc @@ -37,7 +37,7 @@ std::pair> findGenerations(Path pro std::filesystem::path profileDir = dirOf(profile); auto profileName = std::string(baseNameOf(profile)); - for (auto & i : readDirectory(profileDir.string())) { + for (auto & i : std::filesystem::directory_iterator{profileDir}) { if (auto n = parseName(profileName, i.path().filename().string())) { auto path = i.path().string(); gens.push_back({ diff --git a/src/libstore/unix/builtins/unpack-channel.cc b/src/libstore/unix/builtins/unpack-channel.cc index 47bf5d49c0d..c5360414562 100644 --- a/src/libstore/unix/builtins/unpack-channel.cc +++ b/src/libstore/unix/builtins/unpack-channel.cc @@ -21,10 +21,12 @@ void builtinUnpackChannel( unpackTarfile(src, out); - auto entries = readDirectory(out); - if (entries.size() != 1) + auto entries = std::filesystem::directory_iterator{out}; + auto file_count = std::distance(entries, std::filesystem::directory_iterator{}); + + if (file_count != 1) throw Error("channel tarball '%s' contains more than one file", src); - renameFile(entries[0].path().string(), (out + "/" + channelName)); + renameFile(entries->path().string(), (out + "/" + channelName)); } } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 39efa19fe40..7032ecdf7f1 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -222,20 +222,6 @@ Path readLink(const Path & path) } -std::vector readDirectory(const Path & path) -{ - std::vector entries; - entries.reserve(64); - - for (auto & entry : fs::directory_iterator{path}) { - checkInterrupt(); - entries.push_back(std::move(entry)); - } - - return entries; -} - - fs::file_type getFileType(const Path & path) { return fs::symlink_status(path).type(); diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 4536accc379..73150239a42 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -122,12 +122,6 @@ Path readLink(const Path & path); */ Descriptor openDirectory(const std::filesystem::path & path); -/** - * Read the contents of a directory. The entries `.` and `..` are - * removed. - */ -std::vector readDirectory(const Path & path); - std::filesystem::file_type getFileType(const Path & path); /** diff --git a/src/libutil/linux/cgroup.cc b/src/libutil/linux/cgroup.cc index 619ef7764ae..ec407747813 100644 --- a/src/libutil/linux/cgroup.cc +++ b/src/libutil/linux/cgroup.cc @@ -64,7 +64,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 : readDirectory(cgroup)) { + for (auto & entry : std::filesystem::directory_iterator{cgroup}) { 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 aa13f4c56b8..225fc852caf 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -132,7 +132,7 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & { assertNoSymlinks(path); DirEntries res; - for (auto & entry : nix::readDirectory(makeAbsPath(path).string())) { + for (auto & entry : std::filesystem::directory_iterator{makeAbsPath(path)}) { auto type = [&]() -> std::optional { std::filesystem::file_type nativeType; try { diff --git a/src/libutil/unix/file-descriptor.cc b/src/libutil/unix/file-descriptor.cc index 222d077e511..84a33af8181 100644 --- a/src/libutil/unix/file-descriptor.cc +++ b/src/libutil/unix/file-descriptor.cc @@ -124,7 +124,7 @@ void closeMostFDs(const std::set & exceptions) { #if __linux__ try { - for (auto & s : readDirectory("/proc/self/fd")) { + for (auto & s : std::filesystem::directory_iterator{"/proc/self/fd"}) { auto fd = std::stoi(s.path().filename()); if (!exceptions.count(fd)) { debug("closing leaked FD %d", fd); diff --git a/src/nix-collect-garbage/nix-collect-garbage.cc b/src/nix-collect-garbage/nix-collect-garbage.cc index 02a3a4b8343..91209c97898 100644 --- a/src/nix-collect-garbage/nix-collect-garbage.cc +++ b/src/nix-collect-garbage/nix-collect-garbage.cc @@ -27,7 +27,7 @@ void removeOldGenerations(std::string dir) bool canWrite = access(dir.c_str(), W_OK) == 0; - for (auto & i : readDirectory(dir)) { + for (auto & i : std::filesystem::directory_iterator{dir}) { checkInterrupt(); auto path = i.path().string(); diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 9c1888aa05f..78a8a55c33c 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -866,7 +866,7 @@ struct CmdFlakeInitCommon : virtual Args, EvalCommand { createDirs(to); - for (auto & entry : readDirectory(from)) { + for (auto & entry : std::filesystem::directory_iterator{from}) { auto from2 = entry.path().string(); auto to2 = to + "/" + entry.path().filename().string(); auto st = lstat(from2); diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc index e932170cf73..3ce52acc563 100644 --- a/src/nix/prefetch.cc +++ b/src/nix/prefetch.cc @@ -115,9 +115,10 @@ std::tuple prefetchFile( /* If the archive unpacks to a single file/directory, then use that as the top-level. */ - auto entries = readDirectory(unpacked); - if (entries.size() == 1) - tmpFile = entries[0].path(); + auto entries = std::filesystem::directory_iterator{unpacked}; + auto file_count = std::distance(entries, std::filesystem::directory_iterator{}); + if (file_count == 1) + tmpFile = entries->path(); else tmpFile = unpacked; } diff --git a/src/nix/run.cc b/src/nix/run.cc index 9c559bdf695..cc999ddf4f4 100644 --- a/src/nix/run.cc +++ b/src/nix/run.cc @@ -248,7 +248,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 (auto entry : readDirectory("/")) { + for (auto entry : std::filesystem::directory_iterator{"/"}) { auto src = entry.path().string(); Path dst = tmpDir + "/" + entry.path().filename().string(); if (pathExists(dst)) continue; From 4d0777ca69463cbaba3501c10d16012101bb0933 Mon Sep 17 00:00:00 2001 From: siddhantCodes Date: Mon, 13 May 2024 15:36:00 +0530 Subject: [PATCH 2/2] fix: copy fileName before calling `std::distance` --- src/libstore/unix/builtins/unpack-channel.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libstore/unix/builtins/unpack-channel.cc b/src/libstore/unix/builtins/unpack-channel.cc index c5360414562..bb60688ea16 100644 --- a/src/libstore/unix/builtins/unpack-channel.cc +++ b/src/libstore/unix/builtins/unpack-channel.cc @@ -22,11 +22,12 @@ void builtinUnpackChannel( unpackTarfile(src, out); auto entries = std::filesystem::directory_iterator{out}; - auto file_count = std::distance(entries, std::filesystem::directory_iterator{}); + auto fileName = entries->path().string(); + auto fileCount = std::distance(std::filesystem::begin(entries), std::filesystem::end(entries)); - if (file_count != 1) + if (fileCount != 1) throw Error("channel tarball '%s' contains more than one file", src); - renameFile(entries->path().string(), (out + "/" + channelName)); + renameFile(fileName, (out + "/" + channelName)); } }