Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inline the usage of nix::renameFile, nix::getFileType and nix::copyFile #10685

Merged
merged 4 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libstore/gc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R
try {

if (type == std::filesystem::file_type::unknown)
type = getFileType(path);
type = std::filesystem::symlink_status(path).type();

if (type == std::filesystem::file_type::directory) {
for (auto & i : readDirectory(path))
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/indirect-root-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ void IndirectRootStore::makeSymlink(const Path & link, const Path & target)
createSymlink(target, tempLink);

/* Atomically replace the old one. */
renameFile(tempLink, link);
std::filesystem::rename(tempLink, link);
}

Path IndirectRootStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot)
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/local-binary-cache-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class LocalBinaryCacheStore : public virtual LocalBinaryCacheStoreConfig, public
AutoDelete del(tmp, false);
StreamToSourceAdapter source(istream);
writeFile(tmp, source);
renameFile(tmp, path2);
std::filesystem::rename(tmp, path2);
del.cancel();
}

Expand Down
2 changes: 1 addition & 1 deletion src/libstore/local-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1779,7 +1779,7 @@ void LocalStore::addBuildLog(const StorePath & drvPath, std::string_view log)

writeFile(tmpFile, compress("bzip2", log));

renameFile(tmpFile, logPath);
std::filesystem::rename(tmpFile, logPath);
}

std::optional<std::string> LocalStore::getVersion()
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/unix/build/derivation-goal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -783,7 +783,7 @@ static void movePath(const Path & src, const Path & dst)
if (changePerm)
chmod_(src, st.st_mode | S_IWUSR);

renameFile(src, dst);
std::filesystem::rename(src, dst);

if (changePerm)
chmod_(dst, st.st_mode);
Expand Down
15 changes: 10 additions & 5 deletions src/libstore/unix/build/local-derivation-goal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ static void movePath(const Path & src, const Path & dst)
if (changePerm)
chmod_(src, st.st_mode | S_IWUSR);

renameFile(src, dst);
std::filesystem::rename(src, dst);

if (changePerm)
chmod_(dst, st.st_mode);
Expand Down Expand Up @@ -372,7 +372,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull()
if (buildMode != bmCheck && status.known->isValid()) continue;
auto p = worker.store.toRealPath(status.known->path);
if (pathExists(chrootRootDir + p))
renameFile((chrootRootDir + p), p);
std::filesystem::rename((chrootRootDir + p), p);
}

return diskFull;
Expand Down Expand Up @@ -421,7 +421,9 @@ static void doBind(const Path & source, const Path & target, bool optional = fal
} else if (S_ISLNK(st.st_mode)) {
// Symlinks can (apparently) not be bind-mounted, so just copy it
createDirs(dirOf(target));
copyFile(source, target, /* andDelete */ false);
copyFile(
std::filesystem::path(source),
std::filesystem::path(target), false);
} else {
createDirs(dirOf(target));
writeFile(target, "");
Expand Down Expand Up @@ -2568,8 +2570,11 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
// Replace the output by a fresh copy of itself to make sure
// that there's no stale file descriptor pointing to it
Path tmpOutput = actualPath + ".tmp";
copyFile(actualPath, tmpOutput, true);
renameFile(tmpOutput, actualPath);
copyFile(
std::filesystem::path(actualPath),
std::filesystem::path(tmpOutput), true);

std::filesystem::rename(tmpOutput, actualPath);

auto newInfo0 = newInfoFromCA(DerivationOutput::CAFloating {
.method = dof.ca.method,
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/unix/builtins/unpack-channel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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(entries[0].path().string(), (out + "/" + channelName));
std::filesystem::rename(entries[0].path().string(), (out + "/" + channelName));
}

}
44 changes: 14 additions & 30 deletions src/libutil/file-system.cc
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,6 @@ std::vector<fs::directory_entry> readDirectory(const Path & path)
}


fs::file_type getFileType(const Path & path)
{
return fs::symlink_status(path).type();
}


std::string readFile(const Path & path)
{
AutoCloseFD fd = toDescriptor(open(path.c_str(), O_RDONLY
Expand Down Expand Up @@ -588,7 +582,7 @@ void replaceSymlink(const Path & target, const Path & link)
throw;
}

renameFile(tmp, link);
std::filesystem::rename(tmp, link);

break;
}
Expand All @@ -611,55 +605,45 @@ static void setWriteTime(const fs::path & p, const struct stat & st)
}
#endif

void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might still want a copyEntry function as an implementation detail because it can avoid another stat in many cases (because the directory listing already has the info).

void copyFile(const fs::path & from, const fs::path & to, bool andDelete)
{
#ifndef _WIN32
// TODO: Rewrite the `is_*` to use `symlink_status()`
auto statOfFrom = lstat(from.path().c_str());
auto statOfFrom = lstat(from.c_str());
#endif
auto fromStatus = from.symlink_status();
auto fromStatus = fs::symlink_status(from);

// 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);
fs::permissions(from, 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);
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.path())) {
copy(entry, to / entry.path().filename(), andDelete);
for (auto & entry : fs::directory_iterator(from)) {
copyFile(entry, to / entry.path().filename(), andDelete);
}
} else {
throw Error("file '%s' has an unsupported type", from.path());
throw Error("file '%s' has an unsupported type", from);
}

#ifndef _WIN32
setWriteTime(to, statOfFrom);
#endif
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());
fs::permissions(from, fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow);
fs::remove(from);
}
}

void copyFile(const Path & oldPath, const Path & newPath, bool andDelete)
{
return copy(fs::directory_entry(fs::path(oldPath)), fs::path(newPath), andDelete);
}

void renameFile(const Path & oldName, const Path & newName)
{
fs::rename(oldName, newName);
}

void moveFile(const Path & oldName, const Path & newName)
{
try {
renameFile(oldName, newName);
std::filesystem::rename(oldName, newName);
} catch (fs::filesystem_error & e) {
auto oldPath = fs::path(oldName);
auto newPath = fs::path(newName);
Expand All @@ -673,8 +657,8 @@ void moveFile(const Path & oldName, const Path & newName)
if (e.code().value() == EXDEV) {
fs::remove(newPath);
warn("Can’t rename %s as %s, copying instead", oldName, newName);
copy(fs::directory_entry(oldPath), tempCopyTarget, true);
renameFile(
copyFile(oldPath, tempCopyTarget, true);
std::filesystem::rename(
os_string_to_string(PathViewNG { tempCopyTarget }),
os_string_to_string(PathViewNG { newPath }));
}
Expand Down
6 changes: 1 addition & 5 deletions src/libutil/file-system.hh
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,6 @@ Descriptor openDirectory(const std::filesystem::path & path);
*/
std::vector<std::filesystem::directory_entry> readDirectory(const Path & path);

std::filesystem::file_type getFileType(const Path & path);

/**
* Read the contents of a file into a string.
*/
Expand Down Expand Up @@ -177,8 +175,6 @@ void createSymlink(const Path & target, const Path & link);
*/
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.
Expand All @@ -194,7 +190,7 @@ void moveFile(const Path & src, const Path & dst);
* with the guaranty that the destination will be “fresh”, with no stale inode
* or file descriptor pointing to it).
*/
void copyFile(const Path & oldPath, const Path & newPath, bool andDelete);
void copyFile(const std::filesystem::path & from, const std::filesystem::path & to, bool andDelete);

/**
* Automatic cleanup of resources.
Expand Down
Loading