From c371070580afbb7f18972ed52e973f581496f881 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 7 May 2024 00:14:49 -0400 Subject: [PATCH 1/3] Use `std::filesystem` functions in more places This makes for shorter and more portable code. The only tricky part is catching exceptions: I just searched for near by `catch (Error &)` or `catch (SysError &)` and adjusted them to `catch (std::filesystem::filesystem_error &)` according to my human judgement. Good for windows portability; will help @siddhantk232 with his GSOC project. --- configure.ac | 1 - src/libcmd/repl.cc | 1 + src/libstore/builtins/buildenv.cc | 4 +- src/libstore/globals.cc | 4 +- src/libstore/profiles.cc | 2 + src/libstore/unix/gc.cc | 14 ++-- src/libstore/unix/local-store.hh | 2 +- src/libutil/file-system.cc | 83 ++++++------------- src/libutil/file-system.hh | 19 +---- src/libutil/linux/cgroup.cc | 2 +- src/libutil/posix-source-accessor.cc | 14 ++-- src/libutil/unix/file-descriptor.cc | 1 + .../nix-collect-garbage.cc | 10 +-- src/nix/config-check.cc | 3 +- 14 files changed, 61 insertions(+), 99 deletions(-) diff --git a/configure.ac b/configure.ac index 8f60bf4be..b2a5794b5 100644 --- a/configure.ac +++ b/configure.ac @@ -63,7 +63,6 @@ AC_SYS_LARGEFILE # Solaris-specific stuff. -AC_STRUCT_DIRENT_D_TYPE case "$host_os" in solaris*) # Solaris requires -lsocket -lnsl for network functions diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index bade1b538..d2f2a9648 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -264,6 +264,7 @@ StringSet NixRepl::completePrefix(const std::string & prefix) completions.insert(prev + dir + "/" + entry.name); } } 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/libstore/builtins/buildenv.cc b/src/libstore/builtins/buildenv.cc index e009f5b9d..ebd8f1348 100644 --- a/src/libstore/builtins/buildenv.cc +++ b/src/libstore/builtins/buildenv.cc @@ -21,8 +21,8 @@ static void createLinks(State & state, const Path & srcDir, const Path & dstDir, try { srcFiles = readDirectory(srcDir); - } catch (SysError & e) { - if (e.errNo == ENOTDIR) { + } 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); return; } diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index dfe3044ea..f0096b981 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -348,8 +348,8 @@ void initPlugins() auto ents = readDirectory(pluginFile); for (const auto & ent : ents) pluginFiles.emplace_back(pluginFile + "/" + ent.name); - } catch (SysError & e) { - if (e.errNo != ENOTDIR) + } catch (std::filesystem::filesystem_error & e) { + if (e.code() != std::errc::not_a_directory) throw; pluginFiles.emplace_back(pluginFile); } diff --git a/src/libstore/profiles.cc b/src/libstore/profiles.cc index 73d3976f4..a0bb60410 100644 --- a/src/libstore/profiles.cc +++ b/src/libstore/profiles.cc @@ -338,6 +338,8 @@ Path getDefaultProfile() return absPath(readLink(profileLink), dirOf(profileLink)); } catch (Error &) { return profileLink; + } catch (std::filesystem::filesystem_error &) { + return profileLink; } } diff --git a/src/libstore/unix/gc.cc b/src/libstore/unix/gc.cc index be5794395..6677946aa 100644 --- a/src/libstore/unix/gc.cc +++ b/src/libstore/unix/gc.cc @@ -203,7 +203,7 @@ void LocalStore::findTempRoots(Roots & tempRoots, bool censor) } -void LocalStore::findRoots(const Path & path, unsigned char type, Roots & roots) +void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, Roots & roots) { auto foundRoot = [&](const Path & path, const Path & target) { try { @@ -217,15 +217,15 @@ void LocalStore::findRoots(const Path & path, unsigned char type, Roots & roots) try { - if (type == DT_UNKNOWN) + if (type == std::filesystem::file_type::unknown) type = getFileType(path); - if (type == DT_DIR) { + if (type == std::filesystem::file_type::directory) { for (auto & i : readDirectory(path)) findRoots(path + "/" + i.name, i.type, roots); } - else if (type == DT_LNK) { + else if (type == std::filesystem::file_type::symlink) { Path target = readLink(path); if (isInStore(target)) foundRoot(path, target); @@ -247,7 +247,7 @@ void LocalStore::findRoots(const Path & path, unsigned char type, Roots & roots) } } - else if (type == DT_REG) { + else if (type == std::filesystem::file_type::regular) { auto storePath = maybeParseStorePath(storeDir + "/" + std::string(baseNameOf(path))); if (storePath && isValidPath(*storePath)) roots[std::move(*storePath)].emplace(path); @@ -268,8 +268,8 @@ void LocalStore::findRoots(const Path & path, unsigned char type, Roots & roots) void LocalStore::findRootsNoTemp(Roots & roots, bool censor) { /* Process direct roots in {gcroots,profiles}. */ - findRoots(stateDir + "/" + gcRootsDir, DT_UNKNOWN, roots); - findRoots(stateDir + "/profiles", DT_UNKNOWN, roots); + findRoots(stateDir + "/" + gcRootsDir, std::filesystem::file_type::unknown, roots); + findRoots(stateDir + "/profiles", std::filesystem::file_type::unknown, roots); /* Add additional roots returned by different platforms-specific heuristics. This is typically used to add running programs to diff --git a/src/libstore/unix/local-store.hh b/src/libstore/unix/local-store.hh index 47d3c04bc..15bcc826f 100644 --- a/src/libstore/unix/local-store.hh +++ b/src/libstore/unix/local-store.hh @@ -371,7 +371,7 @@ private: PathSet queryValidPathsOld(); ValidPathInfo queryPathInfoOld(const Path & path); - void findRoots(const Path & path, unsigned char type, Roots & roots); + void findRoots(const Path & path, std::filesystem::file_type type, Roots & roots); void findRootsNoTemp(Roots & roots, bool censor); diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index b03bb767b..0e68241fc 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -120,10 +120,10 @@ Path canonPath(PathView path, bool resolveSymlinks) Path dirOf(const PathView path) { - Path::size_type pos = path.rfind('/'); + Path::size_type pos = NativePathTrait::rfindPathSep(path); if (pos == path.npos) return "."; - return pos == 0 ? "/" : Path(path, 0, pos); + return fs::path{path}.parent_path().string(); } @@ -217,72 +217,36 @@ bool pathAccessible(const Path & path) Path readLink(const Path & path) { -#ifndef _WIN32 checkInterrupt(); - std::vector buf; - for (ssize_t bufSize = PATH_MAX/4; true; bufSize += bufSize/2) { - buf.resize(bufSize); - ssize_t rlSize = readlink(path.c_str(), buf.data(), bufSize); - if (rlSize == -1) - if (errno == EINVAL) - throw Error("'%1%' is not a symlink", path); - else - throw SysError("reading symbolic link '%1%'", path); - else if (rlSize < bufSize) - return std::string(buf.data(), rlSize); - } -#else - // TODO modern Windows does in fact support symlinks - throw UnimplementedError("reading symbolic link '%1%'", path); -#endif + return fs::read_symlink(path).string(); } bool isLink(const Path & path) { - return getFileType(path) == DT_LNK; + return getFileType(path) == fs::file_type::symlink; } -DirEntries readDirectory(DIR *dir, const Path & path) +DirEntries readDirectory(const Path & path) { DirEntries entries; entries.reserve(64); - struct dirent * dirent; - while (errno = 0, dirent = readdir(dir)) { /* sic */ + for (auto & entry : fs::directory_iterator{path}) { checkInterrupt(); - std::string name = dirent->d_name; - if (name == "." || name == "..") continue; - entries.emplace_back(name, dirent->d_ino, -#ifdef HAVE_STRUCT_DIRENT_D_TYPE - dirent->d_type -#else - DT_UNKNOWN -#endif - ); + entries.emplace_back( + entry.path().filename().string(), + entry.symlink_status().type()); } - if (errno) throw SysError("reading directory '%1%'", path); return entries; } -DirEntries readDirectory(const Path & path) + +fs::file_type getFileType(const Path & path) { - AutoCloseDir dir(opendir(path.c_str())); - if (!dir) throw SysError("opening directory '%1%'", path); - - return readDirectory(dir.get(), path); -} - - -unsigned char getFileType(const Path & path) -{ - struct stat st = lstat(path); - if (S_ISDIR(st.st_mode)) return DT_DIR; - if (S_ISLNK(st.st_mode)) return DT_LNK; - if (S_ISREG(st.st_mode)) return DT_REG; - return DT_UNKNOWN; + return fs::symlink_status(path).type(); } @@ -432,8 +396,15 @@ static void _deletePath(Descriptor parentfd, const Path & path, uint64_t & bytes AutoCloseDir dir(fdopendir(fd)); if (!dir) throw SysError("opening directory '%1%'", path); - for (auto & i : readDirectory(dir.get(), path)) - _deletePath(dirfd(dir.get()), path + "/" + i.name, bytesFreed); + + struct dirent * dirent; + while (errno = 0, dirent = readdir(dir.get())) { /* sic */ + checkInterrupt(); + std::string childName = dirent->d_name; + if (childName == "." || childName == "..") continue; + _deletePath(dirfd(dir.get()), path + "/" + childName, bytesFreed); + } + if (errno) throw SysError("reading directory '%1%'", path); } int flags = S_ISDIR(st.st_mode) ? AT_REMOVEDIR : 0; @@ -611,13 +582,7 @@ std::pair createTempFile(const Path & prefix) void createSymlink(const Path & target, const Path & link) { -#ifndef _WIN32 - if (symlink(target.c_str(), link.c_str())) - throw SysError("creating symlink from '%1%' to '%2%'", link, target); -#else - // TODO modern Windows does in fact support symlinks - throw UnimplementedError("createSymlink"); -#endif + fs::create_symlink(target, link); } void replaceSymlink(const Path & target, const Path & link) @@ -627,8 +592,8 @@ void replaceSymlink(const Path & target, const Path & link) try { createSymlink(target, tmp); - } catch (SysError & e) { - if (e.errNo == EEXIST) continue; + } catch (fs::filesystem_error & e) { + if (e.code() == std::errc::file_exists) continue; throw; } diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 0c4e7cfdd..2ecf2881c 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -27,13 +27,6 @@ #include #include -#ifndef HAVE_STRUCT_DIRENT_D_TYPE -#define DT_UNKNOWN 0 -#define DT_REG 1 -#define DT_LNK 2 -#define DT_DIR 3 -#endif - /** * Polyfill for MinGW * @@ -132,20 +125,16 @@ bool isLink(const Path & path); struct DirEntry { std::string name; - ino_t ino; - /** - * one of DT_* - */ - unsigned char type; - DirEntry(std::string name, ino_t ino, unsigned char type) - : name(std::move(name)), ino(ino), type(type) { } + std::filesystem::file_type type; + DirEntry(std::string name, std::filesystem::file_type type) + : name(std::move(name)), type(type) { } }; typedef std::vector DirEntries; DirEntries readDirectory(const Path & path); -unsigned char getFileType(const Path & path); +std::filesystem::file_type getFileType(const Path & path); /** * Read the contents of a file into a string. diff --git a/src/libutil/linux/cgroup.cc b/src/libutil/linux/cgroup.cc index 8b8942643..9234a4b2e 100644 --- a/src/libutil/linux/cgroup.cc +++ b/src/libutil/linux/cgroup.cc @@ -65,7 +65,7 @@ static CgroupStats destroyCgroup(const Path & cgroup, bool returnStats) /* Otherwise, manually kill every process in the subcgroups and this cgroup. */ for (auto & entry : readDirectory(cgroup)) { - if (entry.type != DT_DIR) continue; + if (entry.type != std::filesystem::file_type::directory) continue; destroyCgroup(cgroup + "/" + entry.name, false); } diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc index e9c554939..a2559a0fb 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -134,13 +134,17 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & DirEntries res; for (auto & entry : nix::readDirectory(makeAbsPath(path).string())) { std::optional type; + // cannot exhaustively enumerate because implementation-specific + // additional file types are allowed. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wswitch-enum" switch (entry.type) { - case DT_REG: type = Type::tRegular; break; - #ifndef _WIN32 - case DT_LNK: type = Type::tSymlink; break; - #endif - case DT_DIR: type = Type::tDirectory; break; + case std::filesystem::file_type::regular: type = Type::tRegular; break; + case std::filesystem::file_type::symlink: type = Type::tSymlink; break; + case std::filesystem::file_type::directory: type = Type::tDirectory; break; + default: type = tMisc; } +#pragma GCC diagnostic pop res.emplace(entry.name, type); } return res; diff --git a/src/libutil/unix/file-descriptor.cc b/src/libutil/unix/file-descriptor.cc index 27c8d821b..068b37843 100644 --- a/src/libutil/unix/file-descriptor.cc +++ b/src/libutil/unix/file-descriptor.cc @@ -133,6 +133,7 @@ void closeMostFDs(const std::set & exceptions) } 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 bb3f1bc6a..9dfbea3f1 100644 --- a/src/nix-collect-garbage/nix-collect-garbage.cc +++ b/src/nix-collect-garbage/nix-collect-garbage.cc @@ -31,14 +31,14 @@ void removeOldGenerations(std::string dir) checkInterrupt(); auto path = dir + "/" + i.name; - auto type = i.type == DT_UNKNOWN ? getFileType(path) : i.type; + auto type = i.type == std::filesystem::file_type::unknown ? getFileType(path) : i.type; - if (type == DT_LNK && canWrite) { + if (type == std::filesystem::file_type::symlink && canWrite) { std::string link; try { link = readLink(path); - } catch (SysError & e) { - if (e.errNo == ENOENT) continue; + } catch (std::filesystem::filesystem_error & e) { + if (e.code() == std::errc::no_such_file_or_directory) continue; throw; } if (link.find("link") != std::string::npos) { @@ -49,7 +49,7 @@ void removeOldGenerations(std::string dir) } else deleteOldGenerations(path, dryRun); } - } else if (type == DT_DIR) { + } else if (type == std::filesystem::file_type::directory) { removeOldGenerations(path); } } diff --git a/src/nix/config-check.cc b/src/nix/config-check.cc index f7c4cebec..f23e36fb5 100644 --- a/src/nix/config-check.cc +++ b/src/nix/config-check.cc @@ -107,7 +107,8 @@ struct CmdConfigCheck : StoreCommand if (profileDir.find("/profiles/") == std::string::npos) dirs.insert(dir); } - } catch (SystemError &) {} + } catch (SystemError &) { + } catch (std::filesystem::filesystem_error &) {} } if (!dirs.empty()) { From a3c573950bb7f46f2f58592bc7ad4cfcc7b31cef Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 7 May 2024 11:29:33 -0400 Subject: [PATCH 2/3] Replace our `DirEntry` with `std::filesystem`'s --- src/libcmd/repl.cc | 5 +++-- src/libstore/builtins/buildenv.cc | 9 +++++---- src/libstore/globals.cc | 4 ++-- src/libstore/local-binary-cache-store.cc | 7 ++++--- src/libstore/profiles.cc | 8 ++++---- src/libstore/unix/builtins/unpack-channel.cc | 2 +- src/libstore/unix/gc.cc | 9 +++++---- src/libstore/unix/local-store.cc | 11 ++++++----- src/libstore/unix/posix-fs-canonicalise.cc | 4 ++-- src/libutil/file-system.cc | 8 +++----- src/libutil/file-system.hh | 12 +----------- src/libutil/linux/cgroup.cc | 12 ++++++------ src/libutil/posix-source-accessor.cc | 4 ++-- src/libutil/unix/file-descriptor.cc | 2 +- src/nix-collect-garbage/nix-collect-garbage.cc | 4 ++-- src/nix/flake.cc | 4 ++-- src/nix/prefetch.cc | 2 +- src/nix/run.cc | 4 ++-- 18 files changed, 52 insertions(+), 59 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index d2f2a9648..8a9155ab6 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -260,8 +260,9 @@ StringSet NixRepl::completePrefix(const std::string & prefix) auto dir = std::string(cur, 0, slash); auto prefix2 = std::string(cur, slash + 1); for (auto & entry : readDirectory(dir == "" ? "/" : dir)) { - if (entry.name[0] != '.' && hasPrefix(entry.name, prefix2)) - completions.insert(prev + dir + "/" + entry.name); + 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 &) { diff --git a/src/libstore/builtins/buildenv.cc b/src/libstore/builtins/buildenv.cc index ebd8f1348..5fcdf6f15 100644 --- a/src/libstore/builtins/buildenv.cc +++ b/src/libstore/builtins/buildenv.cc @@ -17,7 +17,7 @@ struct State /* For each activated package, create symlinks */ static void createLinks(State & state, const Path & srcDir, const Path & dstDir, int priority) { - DirEntries srcFiles; + std::vector srcFiles; try { srcFiles = readDirectory(srcDir); @@ -30,11 +30,12 @@ static void createLinks(State & state, const Path & srcDir, const Path & dstDir, } for (const auto & ent : srcFiles) { - if (ent.name[0] == '.') + auto name = ent.path().filename(); + if (name.string()[0] == '.') /* not matched by glob */ continue; - auto srcFile = srcDir + "/" + ent.name; - auto dstFile = dstDir + "/" + ent.name; + auto srcFile = (std::filesystem::path{srcDir} / name).string(); + auto dstFile = (std::filesystem::path{dstDir} / name).string(); struct stat srcSt; try { diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index f0096b981..4df2880e6 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -343,11 +343,11 @@ void initPlugins() { assert(!settings.pluginFiles.pluginsLoaded); for (const auto & pluginFile : settings.pluginFiles.get()) { - Paths pluginFiles; + std::vector pluginFiles; try { auto ents = readDirectory(pluginFile); for (const auto & ent : ents) - pluginFiles.emplace_back(pluginFile + "/" + ent.name); + pluginFiles.emplace_back(ent.path()); } catch (std::filesystem::filesystem_error & e) { if (e.code() != std::errc::not_a_directory) throw; diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 5481dd762..3a48f4480 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -84,11 +84,12 @@ protected: StorePathSet paths; for (auto & entry : readDirectory(binaryCacheDir)) { - if (entry.name.size() != 40 || - !hasSuffix(entry.name, ".narinfo")) + auto name = entry.path().filename().string(); + if (name.size() != 40 || + !hasSuffix(name, ".narinfo")) continue; paths.insert(parseStorePath( - storeDir + "/" + entry.name.substr(0, entry.name.size() - 8) + storeDir + "/" + name.substr(0, name.size() - 8) + "-" + MissingName)); } diff --git a/src/libstore/profiles.cc b/src/libstore/profiles.cc index a0bb60410..fa8026703 100644 --- a/src/libstore/profiles.cc +++ b/src/libstore/profiles.cc @@ -34,12 +34,12 @@ std::pair> findGenerations(Path pro { Generations gens; - Path profileDir = dirOf(profile); + std::filesystem::path profileDir = dirOf(profile); auto profileName = std::string(baseNameOf(profile)); - for (auto & i : readDirectory(profileDir)) { - if (auto n = parseName(profileName, i.name)) { - auto path = profileDir + "/" + i.name; + for (auto & i : readDirectory(profileDir.string())) { + if (auto n = parseName(profileName, i.path().filename().string())) { + auto path = i.path().string(); gens.push_back({ .number = *n, .path = path, diff --git a/src/libstore/unix/builtins/unpack-channel.cc b/src/libstore/unix/builtins/unpack-channel.cc index 6f68d4c0b..47bf5d49c 100644 --- a/src/libstore/unix/builtins/unpack-channel.cc +++ b/src/libstore/unix/builtins/unpack-channel.cc @@ -24,7 +24,7 @@ void builtinUnpackChannel( auto entries = readDirectory(out); if (entries.size() != 1) throw Error("channel tarball '%s' contains more than one file", src); - renameFile((out + "/" + entries[0].name), (out + "/" + channelName)); + renameFile(entries[0].path().string(), (out + "/" + channelName)); } } diff --git a/src/libstore/unix/gc.cc b/src/libstore/unix/gc.cc index 6677946aa..c10ed5564 100644 --- a/src/libstore/unix/gc.cc +++ b/src/libstore/unix/gc.cc @@ -160,14 +160,15 @@ void LocalStore::findTempRoots(Roots & tempRoots, bool censor) /* Read the `temproots' directory for per-process temporary root files. */ for (auto & i : readDirectory(tempRootsDir)) { - if (i.name[0] == '.') { + auto name = i.path().filename().string(); + if (name[0] == '.') { // Ignore hidden files. Some package managers (notably portage) create // those to keep the directory alive. continue; } - Path path = tempRootsDir + "/" + i.name; + Path path = i.path(); - pid_t pid = std::stoi(i.name); + pid_t pid = std::stoi(name); debug("reading temporary root file '%1%'", path); AutoCloseFD fd(open(path.c_str(), O_CLOEXEC | O_RDWR, 0666)); @@ -222,7 +223,7 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R if (type == std::filesystem::file_type::directory) { for (auto & i : readDirectory(path)) - findRoots(path + "/" + i.name, i.type, roots); + findRoots(i.path().string(), i.symlink_status().type(), roots); } else if (type == std::filesystem::file_type::symlink) { diff --git a/src/libstore/unix/local-store.cc b/src/libstore/unix/local-store.cc index a3de523a3..7a47d1524 100644 --- a/src/libstore/unix/local-store.cc +++ b/src/libstore/unix/local-store.cc @@ -1388,15 +1388,16 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) printInfo("checking link hashes..."); for (auto & link : readDirectory(linksDir)) { - printMsg(lvlTalkative, "checking contents of '%s'", link.name); - Path linkPath = linksDir + "/" + link.name; + auto name = link.path().filename(); + printMsg(lvlTalkative, "checking contents of '%s'", name); + Path linkPath = linksDir / name; PosixSourceAccessor accessor; std::string hash = hashPath( {getFSSourceAccessor(), CanonPath(linkPath)}, FileIngestionMethod::Recursive, HashAlgorithm::SHA256).to_string(HashFormat::Nix32, false); - if (hash != link.name) { + if (hash != name.string()) { printError("link '%s' was modified! expected hash '%s', got '%s'", - linkPath, link.name, hash); + linkPath, name, hash); if (repair) { if (unlink(linkPath.c_str()) == 0) printInfo("removed link '%s'", linkPath); @@ -1483,7 +1484,7 @@ LocalStore::VerificationResult LocalStore::verifyAllValidPaths(RepairFlag repair */ for (auto & i : readDirectory(realStoreDir)) { try { - storePathsInStoreDir.insert({i.name}); + storePathsInStoreDir.insert({i.path().filename().string()}); } catch (BadStorePath &) { } } diff --git a/src/libstore/unix/posix-fs-canonicalise.cc b/src/libstore/unix/posix-fs-canonicalise.cc index 8b29e90d4..916db49ac 100644 --- a/src/libstore/unix/posix-fs-canonicalise.cc +++ b/src/libstore/unix/posix-fs-canonicalise.cc @@ -136,9 +136,9 @@ static void canonicalisePathMetaData_( } if (S_ISDIR(st.st_mode)) { - DirEntries entries = readDirectory(path); + std::vector entries = readDirectory(path); for (auto & i : entries) - canonicalisePathMetaData_(path + "/" + i.name, uidRange, inodesSeen); + canonicalisePathMetaData_(i.path().string(), uidRange, inodesSeen); } } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 0e68241fc..b9abcade9 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -228,16 +228,14 @@ bool isLink(const Path & path) } -DirEntries readDirectory(const Path & path) +std::vector readDirectory(const Path & path) { - DirEntries entries; + std::vector entries; entries.reserve(64); for (auto & entry : fs::directory_iterator{path}) { checkInterrupt(); - entries.emplace_back( - entry.path().filename().string(), - entry.symlink_status().type()); + entries.push_back(std::move(entry)); } return entries; diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 2ecf2881c..2bf703276 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -122,17 +122,7 @@ bool isLink(const Path & path); * Read the contents of a directory. The entries `.` and `..` are * removed. */ -struct DirEntry -{ - std::string name; - std::filesystem::file_type type; - DirEntry(std::string name, std::filesystem::file_type type) - : name(std::move(name)), type(type) { } -}; - -typedef std::vector DirEntries; - -DirEntries readDirectory(const Path & path); +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 9234a4b2e..619ef7764 100644 --- a/src/libutil/linux/cgroup.cc +++ b/src/libutil/linux/cgroup.cc @@ -47,26 +47,26 @@ std::map getCgroups(const Path & cgroupFile) return cgroups; } -static CgroupStats destroyCgroup(const Path & cgroup, bool returnStats) +static CgroupStats destroyCgroup(const std::filesystem::path & cgroup, bool returnStats) { if (!pathExists(cgroup)) return {}; - auto procsFile = cgroup + "/cgroup.procs"; + auto procsFile = cgroup / "cgroup.procs"; if (!pathExists(procsFile)) throw Error("'%s' is not a cgroup", cgroup); /* Use the fast way to kill every process in a cgroup, if available. */ - auto killFile = cgroup + "/cgroup.kill"; + auto killFile = cgroup / "cgroup.kill"; if (pathExists(killFile)) writeFile(killFile, "1"); /* Otherwise, manually kill every process in the subcgroups and this cgroup. */ for (auto & entry : readDirectory(cgroup)) { - if (entry.type != std::filesystem::file_type::directory) continue; - destroyCgroup(cgroup + "/" + entry.name, false); + if (entry.symlink_status().type() != std::filesystem::file_type::directory) continue; + destroyCgroup(cgroup / entry.path().filename(), false); } int round = 1; @@ -111,7 +111,7 @@ static CgroupStats destroyCgroup(const Path & cgroup, bool returnStats) CgroupStats stats; if (returnStats) { - auto cpustatPath = cgroup + "/cpu.stat"; + auto cpustatPath = cgroup / "cpu.stat"; if (pathExists(cpustatPath)) { for (auto & line : tokenizeString>(readFile(cpustatPath), "\n")) { diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc index a2559a0fb..4ff61f6bb 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -138,14 +138,14 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & // additional file types are allowed. #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wswitch-enum" - switch (entry.type) { + switch (entry.symlink_status().type()) { case std::filesystem::file_type::regular: type = Type::tRegular; break; case std::filesystem::file_type::symlink: type = Type::tSymlink; break; case std::filesystem::file_type::directory: type = Type::tDirectory; break; default: type = tMisc; } #pragma GCC diagnostic pop - res.emplace(entry.name, type); + 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 068b37843..222d077e5 100644 --- a/src/libutil/unix/file-descriptor.cc +++ b/src/libutil/unix/file-descriptor.cc @@ -125,7 +125,7 @@ void closeMostFDs(const std::set & exceptions) #if __linux__ try { for (auto & s : readDirectory("/proc/self/fd")) { - auto fd = std::stoi(s.name); + auto fd = std::stoi(s.path().filename()); if (!exceptions.count(fd)) { debug("closing leaked FD %d", fd); close(fd); diff --git a/src/nix-collect-garbage/nix-collect-garbage.cc b/src/nix-collect-garbage/nix-collect-garbage.cc index 9dfbea3f1..02a3a4b83 100644 --- a/src/nix-collect-garbage/nix-collect-garbage.cc +++ b/src/nix-collect-garbage/nix-collect-garbage.cc @@ -30,8 +30,8 @@ void removeOldGenerations(std::string dir) for (auto & i : readDirectory(dir)) { checkInterrupt(); - auto path = dir + "/" + i.name; - auto type = i.type == std::filesystem::file_type::unknown ? getFileType(path) : i.type; + auto path = i.path().string(); + auto type = i.symlink_status().type(); if (type == std::filesystem::file_type::symlink && canWrite) { std::string link; diff --git a/src/nix/flake.cc b/src/nix/flake.cc index d5987237f..9c1888aa0 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -867,8 +867,8 @@ struct CmdFlakeInitCommon : virtual Args, EvalCommand createDirs(to); for (auto & entry : readDirectory(from)) { - auto from2 = from + "/" + entry.name; - auto to2 = to + "/" + entry.name; + auto from2 = entry.path().string(); + auto to2 = to + "/" + entry.path().filename().string(); auto st = lstat(from2); if (S_ISDIR(st.st_mode)) copyDir(from2, to2); diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc index 6c2eb5aaf..cff1c7988 100644 --- a/src/nix/prefetch.cc +++ b/src/nix/prefetch.cc @@ -117,7 +117,7 @@ std::tuple prefetchFile( that as the top-level. */ auto entries = readDirectory(unpacked); if (entries.size() == 1) - tmpFile = unpacked + "/" + entries[0].name; + tmpFile = entries[0].path().string(); else tmpFile = unpacked; } diff --git a/src/nix/run.cc b/src/nix/run.cc index 88821710d..9c559bdf6 100644 --- a/src/nix/run.cc +++ b/src/nix/run.cc @@ -249,8 +249,8 @@ void chrootHelper(int argc, char * * argv) throw SysError("mounting '%s' on '%s'", realStoreDir, storeDir); for (auto entry : readDirectory("/")) { - auto src = "/" + entry.name; - Path dst = tmpDir + "/" + entry.name; + auto src = entry.path().string(); + Path dst = tmpDir + "/" + entry.path().filename().string(); if (pathExists(dst)) continue; auto st = lstat(src); if (S_ISDIR(st.st_mode)) { From 72a0d4b022b72a7f48a7784df04d50ec8bd874f5 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 7 May 2024 13:57:39 -0400 Subject: [PATCH 3/3] Try to fix macOS Nixpkgs lib test failure Sometimes we read a directory with children we cannot stat. It's a pitty we even try to stat at all (wasteful) in the `DT_UNKNOWN` case, but at least this should get rid of the failure. --- src/libstore/unix/gc.cc | 8 +++++++ src/libutil/posix-source-accessor.cc | 31 ++++++++++++++++++++-------- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/libstore/unix/gc.cc b/src/libstore/unix/gc.cc index c10ed5564..38cbf12b2 100644 --- a/src/libstore/unix/gc.cc +++ b/src/libstore/unix/gc.cc @@ -256,6 +256,14 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R } + catch (std::filesystem::filesystem_error & e) { + /* We only ignore permanent failures. */ + if (e.code() == std::errc::permission_denied || e.code() == std::errc::no_such_file_or_directory || e.code() == std::errc::not_a_directory) + printInfo("cannot read potential root '%1%'", path); + else + throw; + } + catch (SysError & e) { /* We only ignore permanent failures. */ if (e.errNo == EACCES || e.errNo == ENOENT || e.errNo == ENOTDIR) diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc index 4ff61f6bb..aa13f4c56 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -133,18 +133,31 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & assertNoSymlinks(path); DirEntries res; for (auto & entry : nix::readDirectory(makeAbsPath(path).string())) { - std::optional type; - // cannot exhaustively enumerate because implementation-specific - // additional file types are allowed. + 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. #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wswitch-enum" - switch (entry.symlink_status().type()) { - case std::filesystem::file_type::regular: type = Type::tRegular; break; - case std::filesystem::file_type::symlink: type = Type::tSymlink; break; - case std::filesystem::file_type::directory: type = Type::tDirectory; break; - default: type = tMisc; - } + 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; + default: return tMisc; + } #pragma GCC diagnostic pop + }(); res.emplace(entry.path().filename().string(), type); } return res;