From db8d418b2ae7a0d30d513b851077c116239e3eea Mon Sep 17 00:00:00 2001 From: Joel Rosdahl Date: Thu, 27 Jul 2023 15:37:10 +0200 Subject: [PATCH] refactor: Move Util::traverse to util --- src/Util.cpp | 79 -------------------------- src/Util.hpp | 9 --- src/core/mainoptions.cpp | 35 ++++++------ src/storage/local/LocalStorage.cpp | 24 ++++---- src/storage/local/util.cpp | 27 +++++---- src/util/file.cpp | 90 +++++++++++++++++++++++++++++- src/util/file.hpp | 10 ++++ unittest/test_Util.cpp | 68 ---------------------- unittest/test_util_file.cpp | 78 ++++++++++++++++++++++++++ 9 files changed, 224 insertions(+), 196 deletions(-) diff --git a/src/Util.cpp b/src/Util.cpp index d2c8b5cc8..ec0a17a7d 100644 --- a/src/Util.cpp +++ b/src/Util.cpp @@ -37,10 +37,6 @@ #include #include -#ifdef HAVE_DIRENT_H -# include -#endif - #ifdef HAVE_UNISTD_H # include #endif @@ -385,79 +381,4 @@ remove_extension(std::string_view path) return path.substr(0, path.length() - get_extension(path).length()); } -#ifdef HAVE_DIRENT_H - -void -traverse(const std::string& path, const TraverseVisitor& visitor) -{ - DIR* dir = opendir(path.c_str()); - if (dir) { - struct dirent* entry; - while ((entry = readdir(dir))) { - if (strcmp(entry->d_name, "") == 0 || strcmp(entry->d_name, ".") == 0 - || strcmp(entry->d_name, "..") == 0) { - continue; - } - - std::string entry_path = path + "/" + entry->d_name; - bool is_dir; -# ifdef _DIRENT_HAVE_D_TYPE - if (entry->d_type != DT_UNKNOWN) { - is_dir = entry->d_type == DT_DIR; - } else -# endif - { - auto stat = Stat::lstat(entry_path); - if (!stat) { - if (stat.error_number() == ENOENT || stat.error_number() == ESTALE) { - continue; - } - throw core::Error(FMT("failed to lstat {}: {}", - entry_path, - strerror(stat.error_number()))); - } - is_dir = stat.is_directory(); - } - if (is_dir) { - traverse(entry_path, visitor); - } else { - visitor(entry_path, false); - } - } - closedir(dir); - visitor(path, true); - } else if (errno == ENOTDIR) { - visitor(path, false); - } else { - throw core::Error( - FMT("failed to open directory {}: {}", path, strerror(errno))); - } -} - -#else // If not available, use the C++17 std::filesystem implementation. - -void -traverse(const std::string& path, const TraverseVisitor& visitor) -{ - if (fs::is_directory(path)) { - for (auto&& p : fs::directory_iterator(path)) { - std::string entry = p.path().string(); - - if (p.is_directory()) { - traverse(entry, visitor); - } else { - visitor(entry, false); - } - } - visitor(path, true); - } else if (fs::exists(path)) { - visitor(path, false); - } else { - throw core::Error( - FMT("failed to open directory {}: {}", path, strerror(errno))); - } -} - -#endif - } // namespace Util diff --git a/src/Util.hpp b/src/Util.hpp index 717b371b5..6648cb7d6 100644 --- a/src/Util.hpp +++ b/src/Util.hpp @@ -35,9 +35,6 @@ class Context; namespace Util { -using TraverseVisitor = - std::function; - // Get base name of path. std::string_view base_name(std::string_view path); @@ -123,10 +120,4 @@ std::string normalize_concrete_absolute_path(const std::string& path); // extension as determined by `get_extension()`. std::string_view remove_extension(std::string_view path); -// Traverse `path` recursively (postorder, i.e. files are visited before their -// parent directory). -// -// Throws core::Error on error. -void traverse(const std::string& path, const TraverseVisitor& visitor); - } // namespace Util diff --git a/src/core/mainoptions.cpp b/src/core/mainoptions.cpp index 46370a41e..2cfc8e50f 100644 --- a/src/core/mainoptions.cpp +++ b/src/core/mainoptions.cpp @@ -300,23 +300,24 @@ trim_dir(const std::string& dir, std::vector files; uint64_t initial_size = 0; - Util::traverse(dir, [&](const std::string& path, const bool is_dir) { - if (is_dir || TemporaryFile::is_tmp_file(path)) { - return; - } - auto stat = Stat::lstat(path); - if (!stat) { - // Probably some race, ignore. - return; - } - initial_size += stat.size_on_disk(); - const auto name = Util::base_name(path); - if (name == "ccache.conf" || name == "stats") { - throw Fatal( - FMT("this looks like a local cache directory (found {})", path)); - } - files.emplace_back(std::move(stat)); - }); + util::throw_on_error(util::traverse_directory( + dir, [&](const std::string& path, const bool is_dir) { + if (is_dir || TemporaryFile::is_tmp_file(path)) { + return; + } + auto stat = Stat::lstat(path); + if (!stat) { + // Probably some race, ignore. + return; + } + initial_size += stat.size_on_disk(); + const auto name = Util::base_name(path); + if (name == "ccache.conf" || name == "stats") { + throw Fatal( + FMT("this looks like a local cache directory (found {})", path)); + } + files.emplace_back(std::move(stat)); + })); std::sort(files.begin(), files.end(), [&](const auto& f1, const auto& f2) { return trim_lru_mtime ? f1.mtime() < f2.mtime() : f1.atime() < f2.atime(); diff --git a/src/storage/local/LocalStorage.cpp b/src/storage/local/LocalStorage.cpp index d0a904e55..96963c35b 100644 --- a/src/storage/local/LocalStorage.cpp +++ b/src/storage/local/LocalStorage.cpp @@ -1418,16 +1418,20 @@ LocalStorage::clean_internal_tempdir() LOG("Cleaning up {}", m_config.temporary_dir()); core::ensure_dir_exists(m_config.temporary_dir()); - Util::traverse(m_config.temporary_dir(), - [now](const std::string& path, bool is_dir) { - if (is_dir) { - return; - } - const auto st = Stat::lstat(path, Stat::OnError::log); - if (st && st.mtime() + k_tempdir_cleanup_interval < now) { - util::remove(path); - } - }); + util::traverse_directory( + m_config.temporary_dir(), + [now](const std::string& path, bool is_dir) { + if (is_dir) { + return; + } + const auto st = Stat::lstat(path, Stat::OnError::log); + if (st && st.mtime() + k_tempdir_cleanup_interval < now) { + util::remove(path); + } + }) + .or_else([&](const auto& error) { + LOG("Failed to clean up {}: {}", m_config.temporary_dir(), error); + }); util::write_file(cleaned_stamp, ""); } diff --git a/src/storage/local/util.cpp b/src/storage/local/util.cpp index f994daef8..6649b1f11 100644 --- a/src/storage/local/util.cpp +++ b/src/storage/local/util.cpp @@ -1,4 +1,4 @@ -// Copyright (C) 2021-2022 Joel Rosdahl and other contributors +// Copyright (C) 2021-2023 Joel Rosdahl and other contributors // // See doc/AUTHORS.adoc for a complete list of contributors. // @@ -19,7 +19,10 @@ #include "util.hpp" #include +#include #include +#include +#include #include namespace storage::local { @@ -67,18 +70,18 @@ get_cache_dir_files(const std::string& dir) if (!Stat::stat(dir)) { return files; } + util::throw_on_error( + util::traverse_directory(dir, [&](const std::string& path, bool is_dir) { + auto name = Util::base_name(path); + if (name == "CACHEDIR.TAG" || name == "stats" + || util::starts_with(std::string(name), ".nfs")) { + return; + } - Util::traverse(dir, [&](const std::string& path, bool is_dir) { - auto name = Util::base_name(path); - if (name == "CACHEDIR.TAG" || name == "stats" - || util::starts_with(name, ".nfs")) { - return; - } - - if (!is_dir) { - files.emplace_back(Stat::lstat(path)); - } - }); + if (!is_dir) { + files.emplace_back(Stat::lstat(path)); + } + })); return files; } diff --git a/src/util/file.cpp b/src/util/file.cpp index 4ae2c00a6..604ec28a1 100644 --- a/src/util/file.cpp +++ b/src/util/file.cpp @@ -52,9 +52,12 @@ #include #include +#ifdef HAVE_DIRENT_H +# include +#endif + #include #include -#include #include #include #include @@ -467,6 +470,91 @@ set_timestamps(const std::string& path, #endif } +#ifdef HAVE_DIRENT_H + +tl::expected +traverse_directory(const std::string& directory, + const TraverseDirectoryVisitor& visitor) +{ + DIR* dir = opendir(directory.c_str()); + if (!dir) { + return tl::unexpected( + FMT("Failed to traverse {}: {}", directory, strerror(errno))); + } + + Finalizer dir_closer([&] { closedir(dir); }); + + struct dirent* entry; + while ((entry = readdir(dir))) { + if (strcmp(entry->d_name, "") == 0 || strcmp(entry->d_name, ".") == 0 + || strcmp(entry->d_name, "..") == 0) { + continue; + } + + std::string entry_path = directory + "/" + entry->d_name; + bool is_dir; +# ifdef _DIRENT_HAVE_D_TYPE + if (entry->d_type != DT_UNKNOWN) { + is_dir = entry->d_type == DT_DIR; + } else +# endif + { + auto stat = Stat::lstat(entry_path); + if (!stat) { + if (stat.error_number() == ENOENT || stat.error_number() == ESTALE) { + continue; + } + return tl::unexpected(FMT( + "Failed to lstat {}: {}", entry_path, strerror(stat.error_number()))); + } + is_dir = stat.is_directory(); + } + if (is_dir) { + traverse_directory(entry_path, visitor); + } else { + visitor(entry_path, false); + } + } + visitor(directory, true); + + return {}; +} + +#else // If not available, use the C++17 std::filesystem implementation. + +tl::expected +traverse_directory(const std::string& directory, + const TraverseDirectoryVisitor& visitor) +{ + // Note: Intentionally not using std::filesystem::recursive_directory_iterator + // since it visits directories in preorder. + + auto stat = Stat::lstat(directory); + if (!stat.is_directory()) { + return tl::unexpected( + FMT("Failed to traverse {}: {}", + directory, + stat ? "Not a directory" : "No such file or directory")); + } + + try { + for (const auto& entry : std::filesystem::directory_iterator(directory)) { + if (entry.is_directory()) { + traverse_directory(entry.path().string(), visitor); + } else { + visitor(entry.path().string(), entry.is_directory()); + } + } + visitor(directory, true); + } catch (const std::filesystem::filesystem_error& e) { + return tl::unexpected(e.what()); + } + + return {}; +} + +#endif + tl::expected write_fd(int fd, const void* data, size_t size) { diff --git a/src/util/file.hpp b/src/util/file.hpp index 54fd574d5..50e3afb29 100644 --- a/src/util/file.hpp +++ b/src/util/file.hpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -41,6 +42,9 @@ enum class InPlace { yes, no }; enum class LogFailure { yes, no }; enum class ViaTmpFile { yes, no }; +using TraverseDirectoryVisitor = + std::function; + // Copy a file from `src` to `dest`. If `via_tmp_file` is yes, `src` is copied // to a temporary file and then renamed to dest. tl::expected @@ -117,6 +121,12 @@ void set_timestamps(const std::string& path, std::optional mtime = std::nullopt, std::optional atime = std::nullopt); +// Traverse `path` recursively in postorder (directory entries are visited +// before their parent directory). +tl::expected +traverse_directory(const std::string& directory, + const TraverseDirectoryVisitor& visitor); + // Write `size` bytes from binary `data` to `fd`. tl::expected write_fd(int fd, const void* data, size_t size); diff --git a/unittest/test_Util.cpp b/unittest/test_Util.cpp index 43a648700..c7f272655 100644 --- a/unittest/test_Util.cpp +++ b/unittest/test_Util.cpp @@ -128,16 +128,6 @@ TEST_CASE("Util::get_extension") CHECK(Util::get_extension("/foo/bar/f.abc.txt") == ".txt"); } -static inline std::string -os_path(std::string path) -{ -#if defined(_WIN32) && !defined(HAVE_DIRENT_H) - std::replace(path.begin(), path.end(), '/', '\\'); -#endif - - return path; -} - TEST_CASE("Util::get_relative_path") { #ifdef _WIN32 @@ -357,62 +347,4 @@ TEST_CASE("Util::remove_extension") CHECK(Util::remove_extension("/foo/bar/f.abc.txt") == "/foo/bar/f.abc"); } -TEST_CASE("Util::traverse") -{ - TestContext test_context; - - REQUIRE(fs::create_directories("dir-with-subdir-and-file/subdir")); - util::write_file("dir-with-subdir-and-file/subdir/f", ""); - REQUIRE(fs::create_directory("dir-with-files")); - util::write_file("dir-with-files/f1", ""); - util::write_file("dir-with-files/f2", ""); - REQUIRE(fs::create_directory("empty-dir")); - - std::vector visited; - auto visitor = [&visited](const std::string& path, bool is_dir) { - visited.push_back(FMT("[{}] {}", is_dir ? 'd' : 'f', path)); - }; - - SUBCASE("traverse nonexistent path") - { - CHECK_THROWS_WITH( - Util::traverse("nonexistent", visitor), - "failed to open directory nonexistent: No such file or directory"); - } - - SUBCASE("traverse file") - { - CHECK_NOTHROW(Util::traverse("dir-with-subdir-and-file/subdir/f", visitor)); - REQUIRE(visited.size() == 1); - CHECK(visited[0] == "[f] dir-with-subdir-and-file/subdir/f"); - } - - SUBCASE("traverse empty directory") - { - CHECK_NOTHROW(Util::traverse("empty-dir", visitor)); - REQUIRE(visited.size() == 1); - CHECK(visited[0] == "[d] empty-dir"); - } - - SUBCASE("traverse directory with files") - { - CHECK_NOTHROW(Util::traverse("dir-with-files", visitor)); - REQUIRE(visited.size() == 3); - std::string f1 = os_path("[f] dir-with-files/f1"); - std::string f2 = os_path("[f] dir-with-files/f2"); - CHECK(((visited[0] == f1 && visited[1] == f2) - || (visited[0] == f2 && visited[1] == f1))); - CHECK(visited[2] == "[d] dir-with-files"); - } - - SUBCASE("traverse directory hierarchy") - { - CHECK_NOTHROW(Util::traverse("dir-with-subdir-and-file", visitor)); - REQUIRE(visited.size() == 3); - CHECK(visited[0] == os_path("[f] dir-with-subdir-and-file/subdir/f")); - CHECK(visited[1] == os_path("[d] dir-with-subdir-and-file/subdir")); - CHECK(visited[2] == "[d] dir-with-subdir-and-file"); - } -} - TEST_SUITE_END(); diff --git a/unittest/test_util_file.cpp b/unittest/test_util_file.cpp index d864681d8..9e85441c7 100644 --- a/unittest/test_util_file.cpp +++ b/unittest/test_util_file.cpp @@ -20,8 +20,10 @@ #include #include +#include #include #include +#include #include #include @@ -33,8 +35,24 @@ #include #include +namespace fs = util::filesystem; + using TestUtil::TestContext; +namespace { + +std::string +os_path(std::string path) +{ +#if defined(_WIN32) && !defined(HAVE_DIRENT_H) + std::replace(path.begin(), path.end(), '/', '\\'); +#endif + + return path; +} + +} // namespace + TEST_CASE("util::fallocate") { TestContext test_context; @@ -198,3 +216,63 @@ TEST_CASE("util::read_file_part") CHECK(*data == "an"); } } + +TEST_CASE("util::traverse_directory") +{ + TestContext test_context; + + REQUIRE(fs::create_directories("dir-with-subdir-and-file/subdir")); + util::write_file("dir-with-subdir-and-file/subdir/f", ""); + REQUIRE(fs::create_directory("dir-with-files")); + util::write_file("dir-with-files/f1", ""); + util::write_file("dir-with-files/f2", ""); + REQUIRE(fs::create_directory("empty-dir")); + + std::vector visited; + auto visitor = [&visited](const std::string& path, bool is_dir) { + visited.push_back(FMT("[{}] {}", is_dir ? 'd' : 'f', path)); + }; + + SUBCASE("traverse nonexistent path") + { + CHECK(util::traverse_directory("nonexistent", visitor).error() + == "Failed to traverse nonexistent: No such file or directory"); + CHECK(visited.size() == 0); + } + + SUBCASE("traverse file") + { + CHECK(util::traverse_directory("dir-with-subdir-and-file/subdir/f", visitor) + .error() + == "Failed to traverse dir-with-subdir-and-file/subdir/f: Not a directory"); + CHECK(visited.size() == 0); + } + + SUBCASE("traverse empty directory") + { + CHECK_NOTHROW(util::traverse_directory("empty-dir", visitor)); + REQUIRE(visited.size() == 1); + CHECK(visited[0] == "[d] empty-dir"); + } + + SUBCASE("traverse directory with files") + { + CHECK_NOTHROW(util::traverse_directory("dir-with-files", visitor)); + REQUIRE(visited.size() == 3); + std::string f1 = os_path("[f] dir-with-files/f1"); + std::string f2 = os_path("[f] dir-with-files/f2"); + CHECK(((visited[0] == f1 && visited[1] == f2) + || (visited[0] == f2 && visited[1] == f1))); + CHECK(visited[2] == "[d] dir-with-files"); + } + + SUBCASE("traverse directory hierarchy") + { + CHECK_NOTHROW( + util::traverse_directory("dir-with-subdir-and-file", visitor)); + REQUIRE(visited.size() == 3); + CHECK(visited[0] == os_path("[f] dir-with-subdir-and-file/subdir/f")); + CHECK(visited[1] == os_path("[d] dir-with-subdir-and-file/subdir")); + CHECK(visited[2] == "[d] dir-with-subdir-and-file"); + } +} -- 2.47.2