diff --git a/envoy/filesystem/filesystem.h b/envoy/filesystem/filesystem.h index 94b30685eade..245cd936e08f 100644 --- a/envoy/filesystem/filesystem.h +++ b/envoy/filesystem/filesystem.h @@ -10,6 +10,7 @@ #include "envoy/common/pure.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" namespace Envoy { namespace Filesystem { @@ -169,15 +170,19 @@ struct DirectoryEntry { // target. For example, if name_ is a symlink to a directory, its file type will be Directory. FileType type_; + // The file size in bytes for regular files. nullopt for FileType::Directory and FileType::Other, + // and, on Windows, also nullopt for symlinks, and on Linux nullopt for broken symlinks. + absl::optional size_bytes_; + bool operator==(const DirectoryEntry& rhs) const { - return name_ == rhs.name_ && type_ == rhs.type_; + return name_ == rhs.name_ && type_ == rhs.type_ && size_bytes_ == rhs.size_bytes_; } }; class DirectoryIteratorImpl; class DirectoryIterator { public: - DirectoryIterator() : entry_({"", FileType::Other}) {} + DirectoryIterator() : entry_({"", FileType::Other, absl::nullopt}) {} virtual ~DirectoryIterator() = default; const DirectoryEntry& operator*() const { return entry_; } diff --git a/source/common/filesystem/posix/directory_iterator_impl.cc b/source/common/filesystem/posix/directory_iterator_impl.cc index eb760c76cfdf..a50514d991dc 100644 --- a/source/common/filesystem/posix/directory_iterator_impl.cc +++ b/source/common/filesystem/posix/directory_iterator_impl.cc @@ -42,39 +42,38 @@ void DirectoryIteratorImpl::nextEntry() { } if (entry == nullptr) { - entry_ = {"", FileType::Other}; + entry_ = {"", FileType::Other, absl::nullopt}; } else { - const std::string current_path(entry->d_name); - const std::string full_path(directory_path_ + "/" + current_path); - entry_ = {current_path, fileType(full_path, os_sys_calls_)}; + entry_ = makeEntry(entry->d_name); } } -FileType DirectoryIteratorImpl::fileType(const std::string& full_path, - Api::OsSysCallsImpl& os_sys_calls) { +DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) const { + const std::string full_path = absl::StrCat(directory_path_, "/", filename); struct stat stat_buf; - - const Api::SysCallIntResult result = os_sys_calls.stat(full_path.c_str(), &stat_buf); + const Api::SysCallIntResult result = os_sys_calls_.stat(full_path.c_str(), &stat_buf); if (result.return_value_ != 0) { - if (errno == ENOENT) { + if (result.errno_ == ENOENT) { // Special case. This directory entity is likely to be a symlink, // but the reference is broken as the target could not be stat()'ed. // If we confirm this with an lstat, treat this file entity as // a regular file, which may be unlink()'ed. if (::lstat(full_path.c_str(), &stat_buf) == 0 && S_ISLNK(stat_buf.st_mode)) { - return FileType::Regular; + return DirectoryEntry{std::string{filename}, FileType::Regular, absl::nullopt}; } } + // TODO: throwing an exception here makes this dangerous to use in worker threads, + // and in general since it's not clear to the user of Directory that an exception + // may be thrown. Perhaps make this return StatusOr and handle failures gracefully. throw EnvoyException(fmt::format("unable to stat file: '{}' ({})", full_path, errno)); - } - - if (S_ISDIR(stat_buf.st_mode)) { - return FileType::Directory; + } else if (S_ISDIR(stat_buf.st_mode)) { + return DirectoryEntry{std::string{filename}, FileType::Directory, absl::nullopt}; } else if (S_ISREG(stat_buf.st_mode)) { - return FileType::Regular; + return DirectoryEntry{std::string{filename}, FileType::Regular, + static_cast(stat_buf.st_size)}; + } else { + return DirectoryEntry{std::string{filename}, FileType::Other, absl::nullopt}; } - - return FileType::Other; } } // namespace Filesystem diff --git a/source/common/filesystem/posix/directory_iterator_impl.h b/source/common/filesystem/posix/directory_iterator_impl.h index ffb2a6b2d62a..03d45cb7512f 100644 --- a/source/common/filesystem/posix/directory_iterator_impl.h +++ b/source/common/filesystem/posix/directory_iterator_impl.h @@ -25,15 +25,17 @@ class DirectoryIteratorImpl : public DirectoryIterator { DirectoryIteratorImpl(const DirectoryIteratorImpl&) = delete; DirectoryIteratorImpl(DirectoryIteratorImpl&&) = default; - static FileType fileType(const std::string& name, Api::OsSysCallsImpl& os_sys_calls); - private: void nextEntry(); void openDirectory(); + DirectoryEntry makeEntry(absl::string_view filename) const; + std::string directory_path_; DIR* dir_{nullptr}; Api::OsSysCallsImpl& os_sys_calls_; + + friend class DirectoryTest_MakeEntryThrowsOnStatFailure_Test; }; } // namespace Filesystem diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc index 645d5b16e521..3391bce11cb9 100644 --- a/source/common/filesystem/win32/directory_iterator_impl.cc +++ b/source/common/filesystem/win32/directory_iterator_impl.cc @@ -16,7 +16,7 @@ DirectoryIteratorImpl::DirectoryIteratorImpl(const std::string& directory_path) fmt::format("unable to open directory {}: {}", directory_path, ::GetLastError())); } - entry_ = {std::string(find_data.cFileName), fileType(find_data)}; + entry_ = makeEntry(find_data); } DirectoryIteratorImpl::~DirectoryIteratorImpl() { @@ -34,27 +34,31 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() { } if (ret == 0) { - entry_ = {"", FileType::Other}; + entry_ = {"", FileType::Other, absl::nullopt}; } else { - entry_ = {std::string(find_data.cFileName), fileType(find_data)}; + entry_ = makeEntry(find_data); } return *this; } -FileType DirectoryIteratorImpl::fileType(const WIN32_FIND_DATA& find_data) const { +DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data) { if ((find_data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) && !(find_data.dwReserved0 & IO_REPARSE_TAG_SYMLINK)) { // The file is reparse point and not a symlink, so it can't be // a regular file or a directory - return FileType::Other; - } - - if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { - return FileType::Directory; + return {std::string(find_data.cFileName), FileType::Other, absl::nullopt}; + } else if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { + return {std::string(find_data.cFileName), FileType::Directory, absl::nullopt}; + } else if (find_data.dwReserved0 & IO_REPARSE_TAG_SYMLINK) { + return {std::string(find_data.cFileName), FileType::Regular, absl::nullopt}; + } else { + ULARGE_INTEGER file_size; + file_size.LowPart = find_data.nFileSizeLow; + file_size.HighPart = find_data.nFileSizeHigh; + uint64_t size = static_cast(file_size.QuadPart); + return {std::string(find_data.cFileName), FileType::Regular, size}; } - - return FileType::Regular; } } // namespace Filesystem diff --git a/source/common/filesystem/win32/directory_iterator_impl.h b/source/common/filesystem/win32/directory_iterator_impl.h index bfeed6dde6cc..2a0c10aed23e 100644 --- a/source/common/filesystem/win32/directory_iterator_impl.h +++ b/source/common/filesystem/win32/directory_iterator_impl.h @@ -22,7 +22,7 @@ class DirectoryIteratorImpl : public DirectoryIterator { DirectoryIteratorImpl& operator=(DirectoryIteratorImpl&&) = default; private: - FileType fileType(const WIN32_FIND_DATA& find_data) const; + static DirectoryEntry makeEntry(const WIN32_FIND_DATA& find_data); HANDLE find_handle_; }; diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc index ec47699d6a89..75f3715dc793 100644 --- a/test/common/filesystem/directory_test.cc +++ b/test/common/filesystem/directory_test.cc @@ -15,6 +15,17 @@ namespace Envoy { namespace Filesystem { +// NOLINTNEXTLINE(readability-identifier-naming) +void PrintTo(const DirectoryEntry& entry, std::ostream* os) { + *os << "{name=" << entry.name_ << ", type=" << static_cast(entry.type_) << ", size="; + if (entry.size_bytes_ == absl::nullopt) { + *os << "nullopt"; + } else { + *os << entry.size_bytes_.value(); + } + *os << "}"; +} + class DirectoryTest : public testing::Test { public: DirectoryTest() : dir_path_(TestEnvironment::temporaryPath("envoy_test")) { @@ -43,11 +54,26 @@ class DirectoryTest : public testing::Test { void addFiles(std::list files) { for (const std::string& file_name : files) { const std::string full_path = dir_path_ + "/" + file_name; - { const std::ofstream file(full_path); } + { + const std::ofstream file(full_path); + EXPECT_TRUE(file) << "failed to open test file"; + } files_to_remove_.push(full_path); } } + void addFileWithContents(absl::string_view file_name, absl::string_view contents) { + const std::string full_path = absl::StrCat(dir_path_, "/", file_name); + { + std::ofstream file(full_path); + EXPECT_TRUE(file) << "failed to open test file"; + file << contents; + file.close(); + EXPECT_TRUE(file) << "failed to write to test file"; + } + files_to_remove_.push(full_path); + } + void addSymlinks(std::list> symlinks) { for (const auto& link : symlinks) { const std::string target_path = dir_path_ + "/" + link.first; @@ -79,7 +105,7 @@ EntrySet getDirectoryContents(const std::string& dir_path, bool recursive) { std::string subdir_name = entry.name_; EntrySet subdir = getDirectoryContents(dir_path + "/" + subdir_name, recursive); for (const DirectoryEntry& entry : subdir) { - ret.insert({subdir_name + "/" + entry.name_, entry.type_}); + ret.insert({subdir_name + "/" + entry.name_, entry.type_, entry.size_bytes_}); } } } @@ -89,11 +115,22 @@ EntrySet getDirectoryContents(const std::string& dir_path, bool recursive) { // Test that we can list a file in a directory TEST_F(DirectoryTest, DirectoryWithOneFile) { addFiles({"file"}); + const EntrySet expected = { + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"file", FileType::Regular, 0}, + }; + EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); +} + +TEST_F(DirectoryTest, DirectoryWithOneFileIncludesCorrectFileSize) { + absl::string_view contents = "hello!"; + addFileWithContents("file", contents); const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, - {"file", FileType::Regular}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"file", FileType::Regular, contents.size()}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } @@ -103,9 +140,9 @@ TEST_F(DirectoryTest, DirectoryWithOneDirectory) { addSubDirs({"sub_dir"}); const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, - {"sub_dir", FileType::Directory}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"sub_dir", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } @@ -116,9 +153,9 @@ TEST_F(DirectoryTest, DirectoryWithFileInSubDirectory) { addFiles({"sub_dir/sub_file"}); const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, - {"sub_dir", FileType::Directory}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"sub_dir", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } @@ -129,13 +166,13 @@ TEST_F(DirectoryTest, RecursionIntoSubDirectory) { addFiles({"file", "sub_dir/sub_file"}); const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, - {"file", FileType::Regular}, - {"sub_dir", FileType::Directory}, - {"sub_dir/sub_file", FileType::Regular}, - {"sub_dir/.", FileType::Directory}, - {"sub_dir/..", FileType::Directory}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"file", FileType::Regular, 0}, + {"sub_dir", FileType::Directory, absl::nullopt}, + {"sub_dir/sub_file", FileType::Regular, 0}, + {"sub_dir/.", FileType::Directory, absl::nullopt}, + {"sub_dir/..", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, true)); } @@ -146,26 +183,27 @@ TEST_F(DirectoryTest, DirectoryWithFileAndDirectory) { addFiles({"file"}); const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, - {"sub_dir", FileType::Directory}, - {"file", FileType::Regular}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"sub_dir", FileType::Directory, absl::nullopt}, + {"file", FileType::Regular, 0}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } // Test that a symlink to a file has type FileType::Regular TEST_F(DirectoryTest, DirectoryWithSymlinkToFile) { - addFiles({"file"}); + const absl::string_view contents = "hello"; + addFileWithContents("file", contents); addSymlinks({{"file", "link"}}); - const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, - {"file", FileType::Regular}, - {"link", FileType::Regular}, - }; - EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); + const EntrySet result = getDirectoryContents(dir_path_, false); + EXPECT_THAT(result, + testing::Contains(DirectoryEntry{"file", FileType::Regular, contents.size()})); + // Validate without size for link, as it may be nullopt or file-size depending on OS. + EXPECT_THAT(result, testing::Contains(testing::AllOf( + testing::Field(&DirectoryEntry::name_, "link"), + testing::Field(&DirectoryEntry::type_, FileType::Regular)))); } // Test that a symlink to a directory has type FileType::Directory @@ -174,10 +212,10 @@ TEST_F(DirectoryTest, DirectoryWithSymlinkToDirectory) { addSymlinks({{"sub_dir", "link_dir"}}); const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, - {"sub_dir", FileType::Directory}, - {"link_dir", FileType::Directory}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"sub_dir", FileType::Directory, absl::nullopt}, + {"link_dir", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } @@ -189,14 +227,14 @@ TEST_F(DirectoryTest, DirectoryWithBrokenSymlink) { TestEnvironment::removePath(dir_path_ + "/sub_dir"); const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, #ifndef WIN32 // On Linux, a broken directory link is simply a symlink to be rm'ed - {"link_dir", FileType::Regular}, + {"link_dir", FileType::Regular, absl::nullopt}, #else // On Windows, a broken directory link remains a directory link to be rmdir'ed - {"link_dir", FileType::Directory}, + {"link_dir", FileType::Directory, absl::nullopt}, #endif }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); @@ -205,8 +243,8 @@ TEST_F(DirectoryTest, DirectoryWithBrokenSymlink) { // Test that we can list an empty directory TEST_F(DirectoryTest, DirectoryWithEmptyDirectory) { const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } @@ -232,18 +270,22 @@ TEST_F(DirectoryTest, Fifo) { ASSERT_EQ(0, mkfifo(fifo_path.c_str(), 0644)); const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, - {"fifo", FileType::Other}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"fifo", FileType::Other, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); remove(fifo_path.c_str()); } -TEST_F(DirectoryTest, FileTypeTest) { - auto sys_calls = Api::OsSysCallsSingleton::get(); - EXPECT_THROW_WITH_REGEX(DirectoryIteratorImpl::fileType("foo", sys_calls), EnvoyException, - "unable to stat file: 'foo' .*"); +// This test seems like it should be doable by removing a file after directory +// iteration begins, but apparently the behavior of that varies per-filesystem +// (in some cases the absent file is not seen, in others it is). So we test +// instead by directly calling the private function. +TEST_F(DirectoryTest, MakeEntryThrowsOnStatFailure) { + Directory directory(dir_path_); + EXPECT_THROW_WITH_REGEX(directory.begin().makeEntry("foo"), EnvoyException, + "unable to stat file: '.*foo' .*"); } #endif @@ -257,12 +299,37 @@ TEST(Directory, DirectoryHasTrailingPathSeparator) { TestEnvironment::createPath(dir_path); const EntrySet expected = { - {".", FileType::Directory}, - {"..", FileType::Directory}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path, false)); TestEnvironment::removePath(dir_path); } +TEST(DirectoryEntry, EqualityOperator) { + std::vector values{ + DirectoryEntry{"bob", FileType::Directory, absl::nullopt}, + DirectoryEntry{"bob", FileType::Regular, absl::nullopt}, + DirectoryEntry{"bob", FileType::Other, absl::nullopt}, + DirectoryEntry{"bob", FileType::Regular, 0}, + DirectoryEntry{"bob", FileType::Regular, 5}, + DirectoryEntry{"bob", FileType::Regular, 6}, + DirectoryEntry{"alice", FileType::Regular, 6}, + DirectoryEntry{"jim", FileType::Regular, absl::nullopt}, + }; + for (size_t i = 0; i < values.size(); i++) { + DirectoryEntry a = values[i]; + // Two copies of the same value should be ==, in either order. + EXPECT_THAT(a, testing::Eq(values[i])); + EXPECT_THAT(values[i], testing::Eq(a)); + for (size_t j = i + 1; j < values.size(); j++) { + DirectoryEntry b = values[j]; + // No two pairs of the above DirectoryEntries should be ==, in either order. + EXPECT_THAT(a, testing::Not(testing::Eq(b))); + EXPECT_THAT(b, testing::Not(testing::Eq(a))); + } + } +} + } // namespace Filesystem } // namespace Envoy