diff --git a/envoy/filesystem/filesystem.h b/envoy/filesystem/filesystem.h index 7a7365a1abd0..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,9 +170,9 @@ 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. Should not be relied on for directories, - // symlinks, or FileType::Other. - uint64_t size_bytes_; + // 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_ && size_bytes_ == rhs.size_bytes_; @@ -181,7 +182,7 @@ struct DirectoryEntry { class DirectoryIteratorImpl; class DirectoryIterator { public: - DirectoryIterator() : entry_({"", FileType::Other, 0}) {} + 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 6d313257075a..a50514d991dc 100644 --- a/source/common/filesystem/posix/directory_iterator_impl.cc +++ b/source/common/filesystem/posix/directory_iterator_impl.cc @@ -42,7 +42,7 @@ void DirectoryIteratorImpl::nextEntry() { } if (entry == nullptr) { - entry_ = {"", FileType::Other, 0}; + entry_ = {"", FileType::Other, absl::nullopt}; } else { entry_ = makeEntry(entry->d_name); } @@ -57,9 +57,9 @@ DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) cons // 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, with size 0. + // a regular file, which may be unlink()'ed. if (::lstat(full_path.c_str(), &stat_buf) == 0 && S_ISLNK(stat_buf.st_mode)) { - return DirectoryEntry{std::string{filename}, FileType::Regular, 0}; + return DirectoryEntry{std::string{filename}, FileType::Regular, absl::nullopt}; } } // TODO: throwing an exception here makes this dangerous to use in worker threads, @@ -67,12 +67,12 @@ DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) cons // may be thrown. Perhaps make this return StatusOr and handle failures gracefully. throw EnvoyException(fmt::format("unable to stat file: '{}' ({})", full_path, errno)); } else if (S_ISDIR(stat_buf.st_mode)) { - return DirectoryEntry{std::string{filename}, FileType::Directory, 0}; + return DirectoryEntry{std::string{filename}, FileType::Directory, absl::nullopt}; } else if (S_ISREG(stat_buf.st_mode)) { return DirectoryEntry{std::string{filename}, FileType::Regular, static_cast(stat_buf.st_size)}; } else { - return DirectoryEntry{std::string{filename}, FileType::Other, 0}; + return DirectoryEntry{std::string{filename}, FileType::Other, absl::nullopt}; } } diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc index 7cef9ce8867b..094e548c4bb8 100644 --- a/source/common/filesystem/win32/directory_iterator_impl.cc +++ b/source/common/filesystem/win32/directory_iterator_impl.cc @@ -34,7 +34,7 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() { } if (ret == 0) { - entry_ = {"", FileType::Other, 0}; + entry_ = {"", FileType::Other, absl::nullopt}; } else { entry_ = makeEntry(find_data); } @@ -51,11 +51,12 @@ DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data !(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 {std::string(find_data.cFileName), FileType::Other, 0}; + 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, 0}; + return {std::string(find_data.cFileName), FileType::Directory, absl::nullopt}; } else { - return {std::string(find_data.cFileName), FileType::Regular, size}; + return {std::string(find_data.cFileName), FileType::Regular, + (find_data.dwReserved0 & IO_REPARSE_TAG_SYMLINK) ? absl::nullopt : size}; } } diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc index 3c7962e68d1f..18fb2e43b09b 100644 --- a/test/common/filesystem/directory_test.cc +++ b/test/common/filesystem/directory_test.cc @@ -17,8 +17,13 @@ namespace Filesystem { // NOLINTNEXTLINE(readability-identifier-naming) void PrintTo(const DirectoryEntry& entry, std::ostream* os) { - *os << "{name=" << entry.name_ << ", type=" << static_cast(entry.type_) - << ", size=" << entry.size_bytes_ << "}"; + *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 { @@ -110,10 +115,9 @@ 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, 0}, - {"..", FileType::Directory, 0}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, {"file", FileType::Regular, 0}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); @@ -124,8 +128,8 @@ TEST_F(DirectoryTest, DirectoryWithOneFileIncludesCorrectFileSize) { addFileWithContents("file", contents); const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, {"file", FileType::Regular, contents.size()}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); @@ -136,9 +140,9 @@ TEST_F(DirectoryTest, DirectoryWithOneDirectory) { addSubDirs({"sub_dir"}); const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, - {"sub_dir", FileType::Directory, 0}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"sub_dir", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } @@ -149,9 +153,9 @@ TEST_F(DirectoryTest, DirectoryWithFileInSubDirectory) { addFiles({"sub_dir/sub_file"}); const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, - {"sub_dir", FileType::Directory, 0}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, + {"sub_dir", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } @@ -162,13 +166,13 @@ TEST_F(DirectoryTest, RecursionIntoSubDirectory) { addFiles({"file", "sub_dir/sub_file"}); const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, {"file", FileType::Regular, 0}, - {"sub_dir", FileType::Directory, 0}, + {"sub_dir", FileType::Directory, absl::nullopt}, {"sub_dir/sub_file", FileType::Regular, 0}, - {"sub_dir/.", FileType::Directory, 0}, - {"sub_dir/..", FileType::Directory, 0}, + {"sub_dir/.", FileType::Directory, absl::nullopt}, + {"sub_dir/..", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, true)); } @@ -179,9 +183,9 @@ TEST_F(DirectoryTest, DirectoryWithFileAndDirectory) { addFiles({"file"}); const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, - {"sub_dir", FileType::Directory, 0}, + {".", 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)); @@ -208,10 +212,10 @@ TEST_F(DirectoryTest, DirectoryWithSymlinkToDirectory) { addSymlinks({{"sub_dir", "link_dir"}}); const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, - {"sub_dir", FileType::Directory, 0}, - {"link_dir", FileType::Directory, 0}, + {".", 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)); } @@ -223,14 +227,14 @@ TEST_F(DirectoryTest, DirectoryWithBrokenSymlink) { TestEnvironment::removePath(dir_path_ + "/sub_dir"); const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, + {".", 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, 0}, + {"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, 0}, + {"link_dir", FileType::Directory, absl::nullopt}, #endif }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); @@ -239,8 +243,8 @@ TEST_F(DirectoryTest, DirectoryWithBrokenSymlink) { // Test that we can list an empty directory TEST_F(DirectoryTest, DirectoryWithEmptyDirectory) { const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } @@ -266,9 +270,9 @@ TEST_F(DirectoryTest, Fifo) { ASSERT_EQ(0, mkfifo(fifo_path.c_str(), 0644)); const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, - {"fifo", FileType::Other, 0}, + {".", 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()); @@ -295,8 +299,8 @@ TEST(Directory, DirectoryHasTrailingPathSeparator) { TestEnvironment::createPath(dir_path); const EntrySet expected = { - {".", FileType::Directory, 0}, - {"..", FileType::Directory, 0}, + {".", FileType::Directory, absl::nullopt}, + {"..", FileType::Directory, absl::nullopt}, }; EXPECT_EQ(expected, getDirectoryContents(dir_path, false)); TestEnvironment::removePath(dir_path);