From 8119390abcbb25e849acc50d0af0b37d85adfb04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Thu, 17 Mar 2022 15:28:08 +0100 Subject: [PATCH 01/10] Move some fs-related functions to their own file Unclutter `util.cc` a bit --- src/libutil/filesystem.cc | 44 +++++++++++++++++++++++++++++++++++++++ src/libutil/util.cc | 38 --------------------------------- 2 files changed, 44 insertions(+), 38 deletions(-) create mode 100644 src/libutil/filesystem.cc diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc new file mode 100644 index 000000000..e67383e6f --- /dev/null +++ b/src/libutil/filesystem.cc @@ -0,0 +1,44 @@ +#include + +#include "util.hh" +#include "types.hh" + +namespace nix { + +void createSymlink(const Path & target, const Path & link, + std::optional mtime) +{ + if (symlink(target.c_str(), link.c_str())) + throw SysError("creating symlink from '%1%' to '%2%'", link, target); + if (mtime) { + struct timeval times[2]; + times[0].tv_sec = *mtime; + times[0].tv_usec = 0; + times[1].tv_sec = *mtime; + times[1].tv_usec = 0; + if (lutimes(link.c_str(), times)) + throw SysError("setting time of symlink '%s'", link); + } +} + +void replaceSymlink(const Path & target, const Path & link, + std::optional mtime) +{ + for (unsigned int n = 0; true; n++) { + Path tmp = canonPath(fmt("%s/.%d_%s", dirOf(link), n, baseNameOf(link))); + + try { + createSymlink(target, tmp, mtime); + } catch (SysError & e) { + if (e.errNo == EEXIST) continue; + throw; + } + + if (rename(tmp.c_str(), link.c_str()) != 0) + throw SysError("renaming '%1%' to '%2%'", tmp, link); + + break; + } +} + +} diff --git a/src/libutil/util.cc b/src/libutil/util.cc index be6fe091f..3f3695f0d 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -681,44 +681,6 @@ Paths createDirs(const Path & path) } -void createSymlink(const Path & target, const Path & link, - std::optional mtime) -{ - if (symlink(target.c_str(), link.c_str())) - throw SysError("creating symlink from '%1%' to '%2%'", link, target); - if (mtime) { - struct timeval times[2]; - times[0].tv_sec = *mtime; - times[0].tv_usec = 0; - times[1].tv_sec = *mtime; - times[1].tv_usec = 0; - if (lutimes(link.c_str(), times)) - throw SysError("setting time of symlink '%s'", link); - } -} - - -void replaceSymlink(const Path & target, const Path & link, - std::optional mtime) -{ - for (unsigned int n = 0; true; n++) { - Path tmp = canonPath(fmt("%s/.%d_%s", dirOf(link), n, baseNameOf(link))); - - try { - createSymlink(target, tmp, mtime); - } catch (SysError & e) { - if (e.errNo == EEXIST) continue; - throw; - } - - if (rename(tmp.c_str(), link.c_str()) != 0) - throw SysError("renaming '%1%' to '%2%'", tmp, link); - - break; - } -} - - void readFull(int fd, char * buf, size_t count) { while (count) { From c2de0a232c1cfddb1f1385ffd23dd43a2099458e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Thu, 17 Mar 2022 15:28:46 +0100 Subject: [PATCH 02/10] =?UTF-8?q?Create=20a=20wrapper=20around=20stdlib?= =?UTF-8?q?=E2=80=99s=20`rename`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Directly takes some c++ strings, and gently throws an exception on error (rather than having to inline this logic everywhere) --- src/libstore/build/derivation-goal.cc | 3 +-- src/libstore/build/local-derivation-goal.cc | 8 +++----- src/libstore/builtins/unpack-channel.cc | 3 +-- src/libstore/gc.cc | 4 +--- src/libstore/local-binary-cache-store.cc | 3 +-- src/libstore/local-store.cc | 6 ++---- src/libstore/optimise-store.cc | 6 ++++-- src/libutil/filesystem.cc | 9 +++++++-- src/libutil/util.hh | 2 ++ 9 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 3fff2385f..93f923f18 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -705,8 +705,7 @@ static void movePath(const Path & src, const Path & dst) if (changePerm) chmod_(src, st.st_mode | S_IWUSR); - if (rename(src.c_str(), dst.c_str())) - throw SysError("renaming '%1%' to '%2%'", src, dst); + moveFile(src, dst); if (changePerm) chmod_(dst, st.st_mode); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 79a241ae0..2435377e2 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -223,8 +223,7 @@ static void movePath(const Path & src, const Path & dst) if (changePerm) chmod_(src, st.st_mode | S_IWUSR); - if (rename(src.c_str(), dst.c_str())) - throw SysError("renaming '%1%' to '%2%'", src, dst); + moveFile(src, dst); if (changePerm) chmod_(dst, st.st_mode); @@ -311,7 +310,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() if (buildMode != bmCheck && status.known->isValid()) continue; auto p = worker.store.printStorePath(status.known->path); if (pathExists(chrootRootDir + p)) - rename((chrootRootDir + p).c_str(), p.c_str()); + moveFile((chrootRootDir + p), p); } return diskFull; @@ -2625,8 +2624,7 @@ DrvOutputs LocalDerivationGoal::registerOutputs() Path prev = path + checkSuffix; deletePath(prev); Path dst = path + checkSuffix; - if (rename(path.c_str(), dst.c_str())) - throw SysError("renaming '%s' to '%s'", path, dst); + moveFile(path, dst); } } diff --git a/src/libstore/builtins/unpack-channel.cc b/src/libstore/builtins/unpack-channel.cc index 426d58a53..a8417d7ff 100644 --- a/src/libstore/builtins/unpack-channel.cc +++ b/src/libstore/builtins/unpack-channel.cc @@ -22,8 +22,7 @@ void builtinUnpackChannel(const BasicDerivation & drv) auto entries = readDirectory(out); if (entries.size() != 1) throw Error("channel tarball '%s' contains more than one file", src); - if (rename((out + "/" + entries[0].name).c_str(), (out + "/" + channelName).c_str()) == -1) - throw SysError("renaming channel directory"); + moveFile((out + "/" + entries[0].name), (out + "/" + channelName)); } } diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index d58ed78b1..3682f81cc 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -39,9 +39,7 @@ static void makeSymlink(const Path & link, const Path & target) createSymlink(target, tempLink); /* Atomically replace the old one. */ - if (rename(tempLink.c_str(), link.c_str()) == -1) - throw SysError("cannot rename '%1%' to '%2%'", - tempLink , link); + moveFile(tempLink, link); } diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index ba4416f6d..ff7403f9d 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -57,8 +57,7 @@ protected: AutoDelete del(tmp, false); StreamToSourceAdapter source(istream); writeFile(tmp, source); - if (rename(tmp.c_str(), path2.c_str())) - throw SysError("renaming '%1%' to '%2%'", tmp, path2); + moveFile(tmp, path2); del.cancel(); } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index eba3b0fa5..7f708b243 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1430,8 +1430,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name writeFile(realPath, dumpSource); } else { /* Move the temporary path we restored above. */ - if (rename(tempPath.c_str(), realPath.c_str())) - throw Error("renaming '%s' to '%s'", tempPath, realPath); + moveFile(tempPath, realPath); } /* For computing the nar hash. In recursive SHA-256 mode, this @@ -1942,8 +1941,7 @@ void LocalStore::addBuildLog(const StorePath & drvPath, std::string_view log) writeFile(tmpFile, compress("bzip2", log)); - if (rename(tmpFile.c_str(), logPath.c_str()) != 0) - throw SysError("renaming '%1%' to '%2%'", tmpFile, logPath); + moveFile(tmpFile, logPath); } std::optional LocalStore::getVersion() diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 8af9b1dde..20b9c7611 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -229,7 +229,9 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, } /* Atomically replace the old file with the new hard link. */ - if (rename(tempLink.c_str(), path.c_str()) == -1) { + try { + moveFile(tempLink, path); + } catch (SysError & e) { if (unlink(tempLink.c_str()) == -1) printError("unable to unlink '%1%'", tempLink); if (errno == EMLINK) { @@ -240,7 +242,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, debug("'%s' has reached maximum number of links", linkPath); return; } - throw SysError("cannot rename '%1%' to '%2%'", tempLink, path); + throw; } stats.filesLinked++; diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc index e67383e6f..33a8d81a6 100644 --- a/src/libutil/filesystem.cc +++ b/src/libutil/filesystem.cc @@ -34,11 +34,16 @@ void replaceSymlink(const Path & target, const Path & link, throw; } - if (rename(tmp.c_str(), link.c_str()) != 0) - throw SysError("renaming '%1%' to '%2%'", tmp, link); + moveFile(tmp, link); break; } } +void moveFile(const Path & oldName, const Path & newName) +{ + if (::rename(oldName.c_str(), newName.c_str())) + throw SysError("renaming '%1%' to '%2%'", oldName, newName); +} + } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 29227ecc6..564d36e79 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -168,6 +168,8 @@ void createSymlink(const Path & target, const Path & link, void replaceSymlink(const Path & target, const Path & link, std::optional mtime = {}); +void moveFile(const Path & src, const Path & dst); + /* Wrappers arount read()/write() that read/write exactly the requested number of bytes. */ From 6f89fb60088c4bc1513a005f0350c2bc13068892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Thu, 17 Mar 2022 16:13:29 +0100 Subject: [PATCH 03/10] rename: Fallback to a copy if the filesystems mismatch In `nix::rename`, if the call to `rename` fails with `EXDEV` (failure because the source and the destination are in a different filesystems) switch to copying and removing the source. To avoid having to re-implement the copy manually, I switched the function to use the c++17 `filesystem` library (which has a `copy` function that should do what we want). Fix #6262 --- src/libutil/filesystem.cc | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc index 33a8d81a6..cf99b848a 100644 --- a/src/libutil/filesystem.cc +++ b/src/libutil/filesystem.cc @@ -1,8 +1,11 @@ #include +#include #include "util.hh" #include "types.hh" +namespace fs = std::filesystem; + namespace nix { void createSymlink(const Path & target, const Path & link, @@ -42,8 +45,16 @@ void replaceSymlink(const Path & target, const Path & link, void moveFile(const Path & oldName, const Path & newName) { - if (::rename(oldName.c_str(), newName.c_str())) - throw SysError("renaming '%1%' to '%2%'", oldName, newName); + auto oldPath = fs::path(oldName); + auto newPath = fs::path(newName); + try { + fs::rename(oldPath, newPath); + } catch (fs::filesystem_error & e) { + if (e.code().value() == EXDEV) { + fs::copy(oldName, newName, fs::copy_options::copy_symlinks); + fs::remove_all(oldName); + } + } } } From c5db1821a94406eaff86341220bc301ba1dac82e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Fri, 18 Mar 2022 14:25:56 +0100 Subject: [PATCH 04/10] Re-implement the recursive directory copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recursive copy from the stl doesn’t exactly do what we need because 1. It doesn’t delete things as we go 2. It doesn’t keep the mtime, which change the nars So re-implement it ourselves. A bit dull, but that way we have what we want --- src/libutil/filesystem.cc | 52 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc index cf99b848a..198db2832 100644 --- a/src/libutil/filesystem.cc +++ b/src/libutil/filesystem.cc @@ -43,6 +43,53 @@ void replaceSymlink(const Path & target, const Path & link, } } +void setWriteTime(const fs::path & p, const struct stat & st) +{ + struct timeval times[2]; + times[0] = { + .tv_sec = st.st_atime, + .tv_usec = 0, + }; + times[1] = { + .tv_sec = st.st_mtime, + .tv_usec = 0, + }; + warn("Setting the mtime of %s to %d", p.c_str(), st.st_mtim.tv_sec); + if (lutimes(p.c_str(), times) != 0) + throw SysError("changing modification time of '%s'", p); +} + +void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete) +{ + // TODO: Rewrite the `is_*` to use `symlink_status()` + auto statOfFrom = lstat(from.path().c_str()); + auto fromStatus = from.symlink_status(); + + // Mark the directory as writable so that we can delete its children + if (andDelete && fs::is_directory(fromStatus)) { + fs::permissions(from.path(), fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow); + } + + + if (fs::is_symlink(fromStatus) || fs::is_regular_file(fromStatus)) { + fs::copy(from.path(), 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.path())) { + copy(entry, to / entry.path().filename(), andDelete); + } + } else { + throw Error("file '%s' has an unsupported type", from.path()); + } + + setWriteTime(to, statOfFrom); + if (andDelete) { + if (!fs::is_symlink(fromStatus)) + fs::permissions(from.path(), fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow); + fs::remove(from.path()); + } +} + void moveFile(const Path & oldName, const Path & newName) { auto oldPath = fs::path(oldName); @@ -51,8 +98,9 @@ void moveFile(const Path & oldName, const Path & newName) fs::rename(oldPath, newPath); } catch (fs::filesystem_error & e) { if (e.code().value() == EXDEV) { - fs::copy(oldName, newName, fs::copy_options::copy_symlinks); - fs::remove_all(oldName); + fs::remove(newPath); + warn("Copying %s to %s", oldName, newName); + copy(fs::directory_entry(oldPath), newPath, true); } } } From a4f0fd633c9ec865d385b34bdc51923c204e7ff0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 13 Apr 2022 11:52:39 +0200 Subject: [PATCH 05/10] Link against c++fs on darwin Required by the old clang version --- src/libutil/local.mk | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libutil/local.mk b/src/libutil/local.mk index f880c0fc5..13e8d426a 100644 --- a/src/libutil/local.mk +++ b/src/libutil/local.mk @@ -11,3 +11,7 @@ libutil_LDFLAGS += -pthread $(OPENSSL_LIBS) $(LIBBROTLI_LIBS) $(LIBARCHIVE_LIBS) ifeq ($(HAVE_LIBCPUID), 1) libutil_LDFLAGS += -lcpuid endif + +ifdef HOST_DARWIN + libutil_LDFLAGS += -lc++fs +endif From d71d9e9fbfc972514b0b940181ab7f9e766f41f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 13 Apr 2022 14:10:36 +0200 Subject: [PATCH 06/10] moveFile -> renameFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `move` tends to have this `mv` connotation of “I will copy it for you if needs be” --- src/libstore/build/derivation-goal.cc | 2 +- src/libstore/build/local-derivation-goal.cc | 6 +++--- src/libstore/builtins/unpack-channel.cc | 2 +- src/libstore/gc.cc | 2 +- src/libstore/local-binary-cache-store.cc | 2 +- src/libstore/local-store.cc | 4 ++-- src/libstore/optimise-store.cc | 2 +- src/libutil/filesystem.cc | 4 ++-- src/libutil/util.hh | 2 +- 9 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 93f923f18..459fdae79 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -705,7 +705,7 @@ static void movePath(const Path & src, const Path & dst) if (changePerm) chmod_(src, st.st_mode | S_IWUSR); - moveFile(src, dst); + renameFile(src, dst); if (changePerm) chmod_(dst, st.st_mode); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 2435377e2..6843173a7 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -223,7 +223,7 @@ static void movePath(const Path & src, const Path & dst) if (changePerm) chmod_(src, st.st_mode | S_IWUSR); - moveFile(src, dst); + renameFile(src, dst); if (changePerm) chmod_(dst, st.st_mode); @@ -310,7 +310,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() if (buildMode != bmCheck && status.known->isValid()) continue; auto p = worker.store.printStorePath(status.known->path); if (pathExists(chrootRootDir + p)) - moveFile((chrootRootDir + p), p); + renameFile((chrootRootDir + p), p); } return diskFull; @@ -2624,7 +2624,7 @@ DrvOutputs LocalDerivationGoal::registerOutputs() Path prev = path + checkSuffix; deletePath(prev); Path dst = path + checkSuffix; - moveFile(path, dst); + renameFile(path, dst); } } diff --git a/src/libstore/builtins/unpack-channel.cc b/src/libstore/builtins/unpack-channel.cc index a8417d7ff..ba04bb16c 100644 --- a/src/libstore/builtins/unpack-channel.cc +++ b/src/libstore/builtins/unpack-channel.cc @@ -22,7 +22,7 @@ void builtinUnpackChannel(const BasicDerivation & drv) auto entries = readDirectory(out); if (entries.size() != 1) throw Error("channel tarball '%s' contains more than one file", src); - moveFile((out + "/" + entries[0].name), (out + "/" + channelName)); + renameFile((out + "/" + entries[0].name), (out + "/" + channelName)); } } diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 3682f81cc..4c1a82279 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -39,7 +39,7 @@ static void makeSymlink(const Path & link, const Path & target) createSymlink(target, tempLink); /* Atomically replace the old one. */ - moveFile(tempLink, link); + renameFile(tempLink, link); } diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index ff7403f9d..f20b1fa02 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -57,7 +57,7 @@ protected: AutoDelete del(tmp, false); StreamToSourceAdapter source(istream); writeFile(tmp, source); - moveFile(tmp, path2); + renameFile(tmp, path2); del.cancel(); } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 7f708b243..2d076f404 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1430,7 +1430,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name writeFile(realPath, dumpSource); } else { /* Move the temporary path we restored above. */ - moveFile(tempPath, realPath); + renameFile(tempPath, realPath); } /* For computing the nar hash. In recursive SHA-256 mode, this @@ -1941,7 +1941,7 @@ void LocalStore::addBuildLog(const StorePath & drvPath, std::string_view log) writeFile(tmpFile, compress("bzip2", log)); - moveFile(tmpFile, logPath); + renameFile(tmpFile, logPath); } std::optional LocalStore::getVersion() diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 20b9c7611..4d2781180 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -230,7 +230,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, /* Atomically replace the old file with the new hard link. */ try { - moveFile(tempLink, path); + renameFile(tempLink, path); } catch (SysError & e) { if (unlink(tempLink.c_str()) == -1) printError("unable to unlink '%1%'", tempLink); diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc index 198db2832..fee40d09e 100644 --- a/src/libutil/filesystem.cc +++ b/src/libutil/filesystem.cc @@ -37,7 +37,7 @@ void replaceSymlink(const Path & target, const Path & link, throw; } - moveFile(tmp, link); + renameFile(tmp, link); break; } @@ -90,7 +90,7 @@ void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete) } } -void moveFile(const Path & oldName, const Path & newName) +void renameFile(const Path & oldName, const Path & newName) { auto oldPath = fs::path(oldName); auto newPath = fs::path(newName); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 564d36e79..186513cea 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -168,7 +168,7 @@ void createSymlink(const Path & target, const Path & link, void replaceSymlink(const Path & target, const Path & link, std::optional mtime = {}); -void moveFile(const Path & src, const Path & dst); +void renameFile(const Path & src, const Path & dst); /* Wrappers arount read()/write() that read/write exactly the From 90f968073338d6ba276994bc281fa5efa5306e17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 13 Apr 2022 14:19:42 +0200 Subject: [PATCH 07/10] Only use `renameFile` where needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In most places the fallback to copying isn’t needed and can actually be bad, so we’d rather not transparently fallback --- src/libstore/local-store.cc | 2 +- src/libutil/filesystem.cc | 14 +++++++++----- src/libutil/util.hh | 9 +++++++++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 2d076f404..a272e4301 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1430,7 +1430,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name writeFile(realPath, dumpSource); } else { /* Move the temporary path we restored above. */ - renameFile(tempPath, realPath); + moveFile(tempPath, realPath); } /* For computing the nar hash. In recursive SHA-256 mode, this diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc index fee40d09e..9cc18507b 100644 --- a/src/libutil/filesystem.cc +++ b/src/libutil/filesystem.cc @@ -54,7 +54,6 @@ void setWriteTime(const fs::path & p, const struct stat & st) .tv_sec = st.st_mtime, .tv_usec = 0, }; - warn("Setting the mtime of %s to %d", p.c_str(), st.st_mtim.tv_sec); if (lutimes(p.c_str(), times) != 0) throw SysError("changing modification time of '%s'", p); } @@ -92,14 +91,19 @@ void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete) void renameFile(const Path & oldName, const Path & newName) { - auto oldPath = fs::path(oldName); - auto newPath = fs::path(newName); + fs::rename(oldName, newName); +} + +void moveFile(const Path & oldName, const Path & newName) +{ try { - fs::rename(oldPath, newPath); + renameFile(oldName, newName); } catch (fs::filesystem_error & e) { + auto oldPath = fs::path(oldName); + auto newPath = fs::path(newName); if (e.code().value() == EXDEV) { fs::remove(newPath); - warn("Copying %s to %s", oldName, newName); + warn("Can’t rename %s as %s, copying instead", oldName, newName); copy(fs::directory_entry(oldPath), newPath, true); } } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 186513cea..cd83f250f 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -170,6 +170,15 @@ void replaceSymlink(const Path & target, const Path & link, void renameFile(const Path & src, const Path & dst); +/** + * Similar to 'renameFile', but fallback to a copy+remove if `src` and `dst` + * are on a different filesystem. + * + * Beware that this might not be atomic because of the copy that happens behind + * the scenes + */ +void moveFile(const Path & src, const Path & dst); + /* Wrappers arount read()/write() that read/write exactly the requested number of bytes. */ From 1ba5b3e001c3da3c2e2a4dc7d475da1b20f53638 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 13 Apr 2022 14:48:31 +0200 Subject: [PATCH 08/10] Make `moveFile` more atomic Rather than directly copying the source to its dest, copy it first to a temporary location, and eventually move that temporary. That way, the move is at least atomic from the point-of-view of the destination --- src/libutil/filesystem.cc | 62 ++++++++++++++++++++++++++++++++++++++- src/libutil/util.cc | 55 ---------------------------------- 2 files changed, 61 insertions(+), 56 deletions(-) diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc index 9cc18507b..403389e60 100644 --- a/src/libutil/filesystem.cc +++ b/src/libutil/filesystem.cc @@ -1,6 +1,7 @@ #include #include +#include "finally.hh" #include "util.hh" #include "types.hh" @@ -8,6 +9,59 @@ namespace fs = std::filesystem; namespace nix { +static Path tempName(Path tmpRoot, const Path & prefix, bool includePid, + int & counter) +{ + tmpRoot = canonPath(tmpRoot.empty() ? getEnv("TMPDIR").value_or("/tmp") : tmpRoot, true); + if (includePid) + return (format("%1%/%2%-%3%-%4%") % tmpRoot % prefix % getpid() % counter++).str(); + else + return (format("%1%/%2%-%3%") % tmpRoot % prefix % counter++).str(); +} + +Path createTempDir(const Path & tmpRoot, const Path & prefix, + bool includePid, bool useGlobalCounter, mode_t mode) +{ + static int globalCounter = 0; + int localCounter = 0; + int & counter(useGlobalCounter ? globalCounter : localCounter); + + while (1) { + checkInterrupt(); + Path tmpDir = tempName(tmpRoot, prefix, includePid, counter); + if (mkdir(tmpDir.c_str(), mode) == 0) { +#if __FreeBSD__ + /* Explicitly set the group of the directory. This is to + work around around problems caused by BSD's group + ownership semantics (directories inherit the group of + the parent). For instance, the group of /tmp on + FreeBSD is "wheel", so all directories created in /tmp + will be owned by "wheel"; but if the user is not in + "wheel", then "tar" will fail to unpack archives that + have the setgid bit set on directories. */ + if (chown(tmpDir.c_str(), (uid_t) -1, getegid()) != 0) + throw SysError("setting group of directory '%1%'", tmpDir); +#endif + return tmpDir; + } + if (errno != EEXIST) + throw SysError("creating directory '%1%'", tmpDir); + } +} + + +std::pair createTempFile(const Path & prefix) +{ + Path tmpl(getEnv("TMPDIR").value_or("/tmp") + "/" + prefix + ".XXXXXX"); + // Strictly speaking, this is UB, but who cares... + // FIXME: use O_TMPFILE. + AutoCloseFD fd(mkstemp((char *) tmpl.c_str())); + if (!fd) + throw SysError("creating temporary file '%s'", tmpl); + closeOnExec(fd.get()); + return {std::move(fd), tmpl}; +} + void createSymlink(const Path & target, const Path & link, std::optional mtime) { @@ -101,10 +155,16 @@ void moveFile(const Path & oldName, const Path & newName) } catch (fs::filesystem_error & e) { auto oldPath = fs::path(oldName); auto newPath = fs::path(newName); + // For the move to be as atomic as possible, copy to a temporary + // directory + fs::path temp = createTempDir(newPath.parent_path(), "rename-tmp"); + Finally removeTemp = [&]() { fs::remove(temp); }; + auto tempCopyTarget = temp / "copy-target"; if (e.code().value() == EXDEV) { fs::remove(newPath); warn("Can’t rename %s as %s, copying instead", oldName, newName); - copy(fs::directory_entry(oldPath), newPath, true); + copy(fs::directory_entry(oldPath), tempCopyTarget, true); + renameFile(tempCopyTarget, newPath); } } } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 3f3695f0d..2a1bb770a 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -508,61 +508,6 @@ void deletePath(const Path & path, uint64_t & bytesFreed) } -static Path tempName(Path tmpRoot, const Path & prefix, bool includePid, - int & counter) -{ - tmpRoot = canonPath(tmpRoot.empty() ? getEnv("TMPDIR").value_or("/tmp") : tmpRoot, true); - if (includePid) - return (format("%1%/%2%-%3%-%4%") % tmpRoot % prefix % getpid() % counter++).str(); - else - return (format("%1%/%2%-%3%") % tmpRoot % prefix % counter++).str(); -} - - -Path createTempDir(const Path & tmpRoot, const Path & prefix, - bool includePid, bool useGlobalCounter, mode_t mode) -{ - static int globalCounter = 0; - int localCounter = 0; - int & counter(useGlobalCounter ? globalCounter : localCounter); - - while (1) { - checkInterrupt(); - Path tmpDir = tempName(tmpRoot, prefix, includePid, counter); - if (mkdir(tmpDir.c_str(), mode) == 0) { -#if __FreeBSD__ - /* Explicitly set the group of the directory. This is to - work around around problems caused by BSD's group - ownership semantics (directories inherit the group of - the parent). For instance, the group of /tmp on - FreeBSD is "wheel", so all directories created in /tmp - will be owned by "wheel"; but if the user is not in - "wheel", then "tar" will fail to unpack archives that - have the setgid bit set on directories. */ - if (chown(tmpDir.c_str(), (uid_t) -1, getegid()) != 0) - throw SysError("setting group of directory '%1%'", tmpDir); -#endif - return tmpDir; - } - if (errno != EEXIST) - throw SysError("creating directory '%1%'", tmpDir); - } -} - - -std::pair createTempFile(const Path & prefix) -{ - Path tmpl(getEnv("TMPDIR").value_or("/tmp") + "/" + prefix + ".XXXXXX"); - // Strictly speaking, this is UB, but who cares... - // FIXME: use O_TMPFILE. - AutoCloseFD fd(mkstemp((char *) tmpl.c_str())); - if (!fd) - throw SysError("creating temporary file '%s'", tmpl); - closeOnExec(fd.get()); - return {std::move(fd), tmpl}; -} - - std::string getUserName() { auto pw = getpwuid(geteuid()); From d1cda07ce47064bda2c609a0290c867295ddd0a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Wed, 25 May 2022 11:37:01 +0200 Subject: [PATCH 09/10] Don't use -load_all on darwin That flag breaks `-lc++fs` (introducing a duplicate symbol for some reason). Besides, it was apparently needed for bzip2, but we're not using bzip2 anymore. --- configure.ac | 9 --------- 1 file changed, 9 deletions(-) diff --git a/configure.ac b/configure.ac index f0210ab78..64fa12fc7 100644 --- a/configure.ac +++ b/configure.ac @@ -296,15 +296,6 @@ AC_CHECK_FUNCS([setresuid setreuid lchown]) AC_CHECK_FUNCS([strsignal posix_fallocate sysconf]) -# This is needed if bzip2 is a static library, and the Nix libraries -# are dynamic. -case "${host_os}" in - darwin*) - LDFLAGS="-all_load $LDFLAGS" - ;; -esac - - AC_ARG_WITH(sandbox-shell, AS_HELP_STRING([--with-sandbox-shell=PATH],[path of a statically-linked shell to use as /bin/sh in sandboxes]), sandbox_shell=$withval) AC_SUBST(sandbox_shell) From f4a8426098481245e6fb1388de48366c5c82991f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Mon, 8 Aug 2022 14:34:22 +0200 Subject: [PATCH 10/10] Remove the explicit `c++fs` linkage on darwin Doesn't seem needed on a recent-enough clang anymore (and even seems to break stuff) --- src/libutil/local.mk | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/libutil/local.mk b/src/libutil/local.mk index 13e8d426a..f880c0fc5 100644 --- a/src/libutil/local.mk +++ b/src/libutil/local.mk @@ -11,7 +11,3 @@ libutil_LDFLAGS += -pthread $(OPENSSL_LIBS) $(LIBBROTLI_LIBS) $(LIBARCHIVE_LIBS) ifeq ($(HAVE_LIBCPUID), 1) libutil_LDFLAGS += -lcpuid endif - -ifdef HOST_DARWIN - libutil_LDFLAGS += -lc++fs -endif