From d9dd677448771a3a14666595e7c50c5658e28f46 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 25 Feb 2026 19:19:16 -0500 Subject: [PATCH] Create two wrappers for `unlink` See b9ef088e80fb9c138b94b208341b6a652a488e88 for why `std::filesystem::remove` was no good. --- src/libstore/builtins/buildenv.cc | 6 ++---- src/libstore/gc.cc | 9 ++++----- src/libstore/local-store.cc | 2 +- src/libstore/unix/pathlocks.cc | 4 ++-- src/libutil/file-system.cc | 21 ++++++++++++++++++++- src/libutil/include/nix/util/file-system.hh | 14 ++++++++++++++ src/libutil/unix-domain-socket.cc | 7 +------ src/nix/nix-channel/nix-channel.cc | 3 +-- 8 files changed, 45 insertions(+), 21 deletions(-) diff --git a/src/libstore/builtins/buildenv.cc b/src/libstore/builtins/buildenv.cc index 6061fd4d6603..f84820812337 100644 --- a/src/libstore/builtins/buildenv.cc +++ b/src/libstore/builtins/buildenv.cc @@ -80,8 +80,7 @@ createLinks(State & state, const std::filesystem::path & srcDir, const std::file auto target = canonPath(dstFile, true); if (!S_ISDIR(lstat(target).st_mode)) throw Error("collision between %1% and non-directory %2%", PathFmt(srcFile), PathFmt(target)); - if (unlink(dstFile.c_str()) == -1) - throw SysError("unlinking %1%", PathFmt(dstFile)); + unlink(dstFile); if (mkdir( dstFile.c_str() #ifndef _WIN32 // TODO abstract mkdir perms for Windows @@ -108,8 +107,7 @@ createLinks(State & state, const std::filesystem::path & srcDir, const std::file throw BuildEnvFileConflictError(readLink(dstFile), srcFile, priority); if (prevPriority < priority) continue; - if (unlink(dstFile.c_str()) == -1) - throw SysError("unlinking %1%", PathFmt(dstFile)); + unlink(dstFile); } else if (S_ISDIR(dstSt.st_mode)) throw Error("collision between non-directory '%1%' and directory '%2%'", srcFile, dstFile); } diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 923fffbb3720..e82d9dcf168f 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -57,7 +57,7 @@ void LocalStore::createTempRootsFile() if (pathExists(fnTempRoots)) /* It *must* be stale, since there can be no two processes with the same pid. */ - unlink(fnTempRoots.string().c_str()); + tryUnlink(fnTempRoots); *fdTempRoots = openLockFile(fnTempRoots, true); @@ -190,7 +190,7 @@ void LocalStore::findTempRoots(Roots & tempRoots, bool censor) we don't care about its temporary roots. */ if (lockFile(fd.get(), ltWrite, false)) { printInfo("removing stale temporary roots file %1%", PathFmt(path)); - unlink(path.string().c_str()); + tryUnlink(path); writeFull(fd.get(), "d"); continue; } @@ -247,7 +247,7 @@ void LocalStore::findRoots(const std::filesystem::path & path, std::filesystem:: if (!pathExists(target)) { if (isInDir(path, config->stateDir.get() / gcRootsDir / "auto")) { printInfo("removing stale link from %1% to %2%", PathFmt(path), PathFmt(target)); - unlink(path.string().c_str()); + tryUnlink(path); } } else { if (!std::filesystem::is_symlink(target)) @@ -782,8 +782,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) printMsg(lvlTalkative, "deleting unused link %1%", PathFmt(path)); - if (unlink(path.string().c_str()) == -1) - throw SysError("deleting %1%", PathFmt(path)); + unlink(path); /* Do not account for deleted file here. Rely on deletePath() accounting. */ diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 528615e683e6..784c5770316b 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -450,7 +450,7 @@ LocalStore::~LocalStore() auto fdTempRoots(_fdTempRoots.lock()); if (*fdTempRoots) { fdTempRoots->close(); - unlink(fnTempRoots.string().c_str()); + tryUnlink(fnTempRoots); } } catch (...) { ignoreExceptionInDestructor(); diff --git a/src/libstore/unix/pathlocks.cc b/src/libstore/unix/pathlocks.cc index b4592d9f0d8d..ce2adf74debc 100644 --- a/src/libstore/unix/pathlocks.cc +++ b/src/libstore/unix/pathlocks.cc @@ -31,9 +31,9 @@ void deleteLockFile(const std::filesystem::path & path, Descriptor desc) races. Write a (meaningless) token to the file to indicate to other processes waiting on this lock that the lock is stale (deleted). */ - unlink(path.c_str()); + tryUnlink(path); writeFull(desc, "d"); - /* Note that the result of unlink() is ignored; removing the lock + /* We just try to unlink don't care if it fails; removing the lock file is an optimisation, not a necessity. */ } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 5fe42f1377a8..f62ab03216b1 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -587,7 +587,7 @@ AutoCloseFD createAnonymousTempFile() if (!fd2) throw SysError("creating temporary file %s", PathFmt(path)); fd = std::move(fd2); - unlink(requireCString(path)); /* We only care about the file descriptor. */ + tryUnlink(path); /* We only care about the file descriptor. */ #endif return fd; @@ -762,6 +762,25 @@ void chmod(const std::filesystem::path & path, mode_t mode) throw SysError("setting permissions on %s", PathFmt(path)); } +#ifdef _WIN32 +# define UNLINK_PROC ::_wunlink +#else +# define UNLINK_PROC ::unlink +#endif + +void unlink(const std::filesystem::path & path) +{ + if (UNLINK_PROC(path.c_str()) == -1) + throw SysError("removing %s", PathFmt(path)); +} + +void tryUnlink(const std::filesystem::path & path) +{ + UNLINK_PROC(path.c_str()); +} + +#undef UNLINK_PROC + bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask) { auto prevMode = lstat(path).st_mode; diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh index da496a30359f..6fd4f3253919 100644 --- a/src/libutil/include/nix/util/file-system.hh +++ b/src/libutil/include/nix/util/file-system.hh @@ -457,6 +457,20 @@ bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask */ void chmod(const std::filesystem::path & path, mode_t mode); +/** + * Remove a file, throwing an exception on error. + * + * @param path Path to the file to remove. + */ +void unlink(const std::filesystem::path & path); + +/** + * Try to remove a file, ignoring errors. + * + * @param path Path to the file to try to remove. + */ +void tryUnlink(const std::filesystem::path & path); + /** * @brief A directory iterator that can be used to iterate over the * contents of a directory. It is similar to std::filesystem::directory_iterator diff --git a/src/libutil/unix-domain-socket.cc b/src/libutil/unix-domain-socket.cc index d1ee23df0796..b13bf028d873 100644 --- a/src/libutil/unix-domain-socket.cc +++ b/src/libutil/unix-domain-socket.cc @@ -105,12 +105,7 @@ bindConnectProcHelper(std::string_view operationName, auto && operation, Socket void bind(Socket fd, const std::filesystem::path & path) { -#ifdef _WIN32 - _wunlink -#else - unlink -#endif - (path.c_str()); + tryUnlink(path); bindConnectProcHelper("bind", ::bind, fd, path.string()); } diff --git a/src/nix/nix-channel/nix-channel.cc b/src/nix/nix-channel/nix-channel.cc index 45b8e802cfb1..05adc3d2cda8 100644 --- a/src/nix/nix-channel/nix-channel.cc +++ b/src/nix/nix-channel/nix-channel.cc @@ -192,8 +192,7 @@ static void update(const StringSet & channelNames) if (lstat(nixDefExpr.c_str(), &st) == 0) { if (S_ISLNK(st.st_mode)) // old-skool ~/.nix-defexpr - if (unlink(nixDefExpr.c_str()) == -1) - throw SysError("unlinking %1%", PathFmt(nixDefExpr)); + unlink(nixDefExpr); } else if (errno != ENOENT) { throw SysError("getting status of %1%", PathFmt(nixDefExpr)); }