From 60306a93c8fa6f68f7ba8fe8a566362d1d0520fa Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Wed, 23 Nov 2022 16:34:30 +0000
Subject: [PATCH 01/18] Add file size to DirectoryEntry

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 envoy/filesystem/filesystem.h                 |  5 +-
 .../posix/directory_iterator_impl.cc          | 30 +++---
 .../posix/directory_iterator_impl.h           |  4 +-
 .../win32/directory_iterator_impl.cc          | 23 ++---
 .../win32/directory_iterator_impl.h           |  2 +-
 test/common/filesystem/directory_test.cc      | 95 ++++++++++---------
 6 files changed, 84 insertions(+), 75 deletions(-)

diff --git a/envoy/filesystem/filesystem.h b/envoy/filesystem/filesystem.h
index 94b30685eade..569be672ea1d 100644
--- a/envoy/filesystem/filesystem.h
+++ b/envoy/filesystem/filesystem.h
@@ -169,6 +169,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.
+  uint64_t size_bytes_;
+
   bool operator==(const DirectoryEntry& rhs) const {
     return name_ == rhs.name_ && type_ == rhs.type_;
   }
@@ -177,7 +180,7 @@ struct DirectoryEntry {
 class DirectoryIteratorImpl;
 class DirectoryIterator {
 public:
-  DirectoryIterator() : entry_({"", FileType::Other}) {}
+  DirectoryIterator() : entry_({"", FileType::Other, 0}) {}
   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..a3b63bf7c9be 100644
--- a/source/common/filesystem/posix/directory_iterator_impl.cc
+++ b/source/common/filesystem/posix/directory_iterator_impl.cc
@@ -42,39 +42,39 @@ void DirectoryIteratorImpl::nextEntry() {
   }
 
   if (entry == nullptr) {
-    entry_ = {"", FileType::Other};
+    entry_ = {"", FileType::Other, 0};
   } 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;
+  FileType file_type = FileType::Other;
 
-  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;
+        file_type = FileType::Regular;
       }
     }
-    throw EnvoyException(fmt::format("unable to stat file: '{}' ({})", full_path, errno));
-  }
-
-  if (S_ISDIR(stat_buf.st_mode)) {
-    return FileType::Directory;
+    if (file_type == FileType::Other) {
+      throw EnvoyException(fmt::format("unable to stat file: '{}' ({})", full_path, errno));
+    }
+  } else if (S_ISDIR(stat_buf.st_mode)) {
+    file_type = FileType::Directory;
   } else if (S_ISREG(stat_buf.st_mode)) {
-    return FileType::Regular;
+    file_type = FileType::Regular;
   }
-
-  return FileType::Other;
+  return DirectoryEntry{std::string{filename}, file_type, static_cast<uint64_t>(stat_buf.st_size)};
 }
 
 } // namespace Filesystem
diff --git a/source/common/filesystem/posix/directory_iterator_impl.h b/source/common/filesystem/posix/directory_iterator_impl.h
index ffb2a6b2d62a..005a840324f9 100644
--- a/source/common/filesystem/posix/directory_iterator_impl.h
+++ b/source/common/filesystem/posix/directory_iterator_impl.h
@@ -25,12 +25,12 @@ 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_;
diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc
index 645d5b16e521..507d32c6d25f 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,28 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() {
   }
 
   if (ret == 0) {
-    entry_ = {"", FileType::Other};
+    entry_ = {"", FileType::Other, 0};
   } 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) const {
+  FileType file_type;
   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;
+    file_type = FileType::Other;
+  } else if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
+    file_type = FileType::Directory;
+  } else {
+    file_type = FileType::Regular;
   }
-
-  return FileType::Regular;
+  return {std::string(find_data.cFileName), file_type,
+          static_cast<uint64_t>(find_data.nFileSizeHigh) << 32 + find_data.nFileSizeLow};
 }
 
 } // namespace Filesystem
diff --git a/source/common/filesystem/win32/directory_iterator_impl.h b/source/common/filesystem/win32/directory_iterator_impl.h
index bfeed6dde6cc..b02a07b189e7 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) const;
 
   HANDLE find_handle_;
 };
diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index ec47699d6a89..0328a0367f8a 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -24,7 +24,7 @@ class DirectoryTest : public testing::Test {
 protected:
   void SetUp() override { TestEnvironment::createPath(dir_path_); }
 
-  void TearDown() override {
+  void removeAllAddedFiles() {
     while (!files_to_remove_.empty()) {
       const std::string& f = files_to_remove_.top();
       TestEnvironment::removePath(f);
@@ -32,6 +32,8 @@ class DirectoryTest : public testing::Test {
     }
   }
 
+  void TearDown() override { removeAllAddedFiles(); }
+
   void addSubDirs(std::list<std::string> sub_dirs) {
     for (const std::string& dir_name : sub_dirs) {
       const std::string full_path = dir_path_ + "/" + dir_name;
@@ -79,7 +81,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_});
       }
     }
   }
@@ -91,9 +93,9 @@ TEST_F(DirectoryTest, DirectoryWithOneFile) {
   addFiles({"file"});
 
   const EntrySet expected = {
-      {".", FileType::Directory},
-      {"..", FileType::Directory},
-      {"file", FileType::Regular},
+      {".", FileType::Directory, 0},
+      {"..", FileType::Directory, 0},
+      {"file", FileType::Regular, 0},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
@@ -103,9 +105,9 @@ TEST_F(DirectoryTest, DirectoryWithOneDirectory) {
   addSubDirs({"sub_dir"});
 
   const EntrySet expected = {
-      {".", FileType::Directory},
-      {"..", FileType::Directory},
-      {"sub_dir", FileType::Directory},
+      {".", FileType::Directory, 0},
+      {"..", FileType::Directory, 0},
+      {"sub_dir", FileType::Directory, 0},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
@@ -116,9 +118,9 @@ TEST_F(DirectoryTest, DirectoryWithFileInSubDirectory) {
   addFiles({"sub_dir/sub_file"});
 
   const EntrySet expected = {
-      {".", FileType::Directory},
-      {"..", FileType::Directory},
-      {"sub_dir", FileType::Directory},
+      {".", FileType::Directory, 0},
+      {"..", FileType::Directory, 0},
+      {"sub_dir", FileType::Directory, 0},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
@@ -129,13 +131,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, 0},
+      {"..", FileType::Directory, 0},
+      {"file", FileType::Regular, 0},
+      {"sub_dir", FileType::Directory, 0},
+      {"sub_dir/sub_file", FileType::Regular, 0},
+      {"sub_dir/.", FileType::Directory, 0},
+      {"sub_dir/..", FileType::Directory, 0},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, true));
 }
@@ -146,10 +148,10 @@ TEST_F(DirectoryTest, DirectoryWithFileAndDirectory) {
   addFiles({"file"});
 
   const EntrySet expected = {
-      {".", FileType::Directory},
-      {"..", FileType::Directory},
-      {"sub_dir", FileType::Directory},
-      {"file", FileType::Regular},
+      {".", FileType::Directory, 0},
+      {"..", FileType::Directory, 0},
+      {"sub_dir", FileType::Directory, 0},
+      {"file", FileType::Regular, 0},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
@@ -160,10 +162,10 @@ TEST_F(DirectoryTest, DirectoryWithSymlinkToFile) {
   addSymlinks({{"file", "link"}});
 
   const EntrySet expected = {
-      {".", FileType::Directory},
-      {"..", FileType::Directory},
-      {"file", FileType::Regular},
-      {"link", FileType::Regular},
+      {".", FileType::Directory, 0},
+      {"..", FileType::Directory, 0},
+      {"file", FileType::Regular, 0},
+      {"link", FileType::Regular, 0},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
@@ -174,10 +176,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, 0},
+      {"..", FileType::Directory, 0},
+      {"sub_dir", FileType::Directory, 0},
+      {"link_dir", FileType::Directory, 0},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
@@ -189,14 +191,14 @@ TEST_F(DirectoryTest, DirectoryWithBrokenSymlink) {
   TestEnvironment::removePath(dir_path_ + "/sub_dir");
 
   const EntrySet expected = {
-      {".", FileType::Directory},
-      {"..", FileType::Directory},
+      {".", FileType::Directory, 0},
+      {"..", FileType::Directory, 0},
 #ifndef WIN32
       // On Linux, a broken directory link is simply a symlink to be rm'ed
-      {"link_dir", FileType::Regular},
+      {"link_dir", FileType::Regular, 0},
 #else
       // On Windows, a broken directory link remains a directory link to be rmdir'ed
-      {"link_dir", FileType::Directory},
+      {"link_dir", FileType::Directory, 0},
 #endif
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
@@ -205,8 +207,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, 0},
+      {"..", FileType::Directory, 0},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
@@ -232,18 +234,21 @@ 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, 0},
+      {"..", FileType::Directory, 0},
+      {"fifo", FileType::Other, 0},
   };
   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' .*");
+TEST_F(DirectoryTest, MakeEntryThrowsOnStatFailure) {
+  addFiles({"foo"});
+  Directory directory(dir_path_);
+  DirectoryIteratorImpl it = directory.begin();
+  ++it;
+  removeAllAddedFiles();
+  EXPECT_THROW_WITH_REGEX(++it, EnvoyException, "unable to stat file: '.*foo' .*");
 }
 #endif
 
@@ -257,8 +262,8 @@ TEST(Directory, DirectoryHasTrailingPathSeparator) {
   TestEnvironment::createPath(dir_path);
 
   const EntrySet expected = {
-      {".", FileType::Directory},
-      {"..", FileType::Directory},
+      {".", FileType::Directory, 0},
+      {"..", FileType::Directory, 0},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path, false));
   TestEnvironment::removePath(dir_path);

From 2a19b95af113840cbf95e2a9cef3249a53b80e30 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Wed, 23 Nov 2022 17:17:53 +0000
Subject: [PATCH 02/18] Add test to validate size

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 test/common/filesystem/directory_test.cc | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index 0328a0367f8a..f964b3c9b42f 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -50,6 +50,15 @@ class DirectoryTest : public testing::Test {
     }
   }
 
+  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);
+      file << contents;
+    }
+    files_to_remove_.push(full_path);
+  }
+
   void addSymlinks(std::list<std::pair<std::string, std::string>> symlinks) {
     for (const auto& link : symlinks) {
       const std::string target_path = dir_path_ + "/" + link.first;
@@ -100,6 +109,18 @@ TEST_F(DirectoryTest, DirectoryWithOneFile) {
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
 
+TEST_F(DirectoryTest, DirectoryWithOneFileIncludesCorrectFileSize) {
+  absl::string_view contents = "hello!";
+  addFileWithContents("file", contents);
+
+  const EntrySet expected = {
+      {".", FileType::Directory, 0},
+      {"..", FileType::Directory, 0},
+      {"file", FileType::Regular, contents.size()},
+  };
+  EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
+}
+
 // Test that we can list a sub directory in a directory
 TEST_F(DirectoryTest, DirectoryWithOneDirectory) {
   addSubDirs({"sub_dir"});

From 19c8a5835a7327c14b69c01c116f9cb4ee102bfb Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Wed, 23 Nov 2022 19:44:11 +0000
Subject: [PATCH 03/18] Fix tests

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 test/common/filesystem/directory_test.cc | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index f964b3c9b42f..350cc4694a29 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -24,7 +24,7 @@ class DirectoryTest : public testing::Test {
 protected:
   void SetUp() override { TestEnvironment::createPath(dir_path_); }
 
-  void removeAllAddedFiles() {
+  void TearDown() override {
     while (!files_to_remove_.empty()) {
       const std::string& f = files_to_remove_.top();
       TestEnvironment::removePath(f);
@@ -32,8 +32,6 @@ class DirectoryTest : public testing::Test {
     }
   }
 
-  void TearDown() override { removeAllAddedFiles(); }
-
   void addSubDirs(std::list<std::string> sub_dirs) {
     for (const std::string& dir_name : sub_dirs) {
       const std::string full_path = dir_path_ + "/" + dir_name;
@@ -264,12 +262,13 @@ TEST_F(DirectoryTest, Fifo) {
 }
 
 TEST_F(DirectoryTest, MakeEntryThrowsOnStatFailure) {
-  addFiles({"foo"});
+  const std::string file_path = dir_path_ + "/foo";
+  { const std::ofstream file(file_path); }
   Directory directory(dir_path_);
   DirectoryIteratorImpl it = directory.begin();
-  ++it;
-  removeAllAddedFiles();
-  EXPECT_THROW_WITH_REGEX(++it, EnvoyException, "unable to stat file: '.*foo' .*");
+  TestEnvironment::removePath(file_path);
+  EXPECT_THROW_WITH_REGEX(while (++it != directory.end()){}, EnvoyException,
+                          "unable to stat file: '.*foo' .*");
 }
 #endif
 

From 9cc86ca05e53e015e95bacc2c9f2b57c88e185e8 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Wed, 23 Nov 2022 20:23:03 +0000
Subject: [PATCH 04/18] Fix mistaken const

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 source/common/filesystem/win32/directory_iterator_impl.cc | 2 +-
 source/common/filesystem/win32/directory_iterator_impl.h  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc
index 507d32c6d25f..ecbea3810dfc 100644
--- a/source/common/filesystem/win32/directory_iterator_impl.cc
+++ b/source/common/filesystem/win32/directory_iterator_impl.cc
@@ -42,7 +42,7 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() {
   return *this;
 }
 
-DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data) const {
+DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data) {
   FileType file_type;
   if ((find_data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
       !(find_data.dwReserved0 & IO_REPARSE_TAG_SYMLINK)) {
diff --git a/source/common/filesystem/win32/directory_iterator_impl.h b/source/common/filesystem/win32/directory_iterator_impl.h
index b02a07b189e7..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:
-  static DirectoryEntry makeEntry(const WIN32_FIND_DATA& find_data) const;
+  static DirectoryEntry makeEntry(const WIN32_FIND_DATA& find_data);
 
   HANDLE find_handle_;
 };

From 331c5af6759f612d4d124317be8a0e9acc523141 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Wed, 23 Nov 2022 21:23:40 +0000
Subject: [PATCH 05/18] makeEntry test needs to be filesystem-independent

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 .../common/filesystem/posix/directory_iterator_impl.h  |  2 ++
 test/common/filesystem/directory_test.cc               | 10 +++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/source/common/filesystem/posix/directory_iterator_impl.h b/source/common/filesystem/posix/directory_iterator_impl.h
index 005a840324f9..03d45cb7512f 100644
--- a/source/common/filesystem/posix/directory_iterator_impl.h
+++ b/source/common/filesystem/posix/directory_iterator_impl.h
@@ -34,6 +34,8 @@ class DirectoryIteratorImpl : public DirectoryIterator {
   std::string directory_path_;
   DIR* dir_{nullptr};
   Api::OsSysCallsImpl& os_sys_calls_;
+
+  friend class DirectoryTest_MakeEntryThrowsOnStatFailure_Test;
 };
 
 } // namespace Filesystem
diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index 350cc4694a29..ff236276c9ea 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -261,13 +261,13 @@ TEST_F(DirectoryTest, Fifo) {
   remove(fifo_path.c_str());
 }
 
+// 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) {
-  const std::string file_path = dir_path_ + "/foo";
-  { const std::ofstream file(file_path); }
   Directory directory(dir_path_);
-  DirectoryIteratorImpl it = directory.begin();
-  TestEnvironment::removePath(file_path);
-  EXPECT_THROW_WITH_REGEX(while (++it != directory.end()){}, EnvoyException,
+  EXPECT_THROW_WITH_REGEX(directory.begin().makeEntry("foo"), EnvoyException,
                           "unable to stat file: '.*foo' .*");
 }
 #endif

From 47323f29e1cbb349674835d1894f3527ad103887 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Mon, 28 Nov 2022 15:23:23 +0000
Subject: [PATCH 06/18] Drop unused temps, clarify the final FileType case.

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 source/common/filesystem/posix/directory_iterator_impl.cc | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/source/common/filesystem/posix/directory_iterator_impl.cc b/source/common/filesystem/posix/directory_iterator_impl.cc
index a3b63bf7c9be..87465202bf1f 100644
--- a/source/common/filesystem/posix/directory_iterator_impl.cc
+++ b/source/common/filesystem/posix/directory_iterator_impl.cc
@@ -44,8 +44,6 @@ void DirectoryIteratorImpl::nextEntry() {
   if (entry == nullptr) {
     entry_ = {"", FileType::Other, 0};
   } else {
-    const std::string current_path(entry->d_name);
-    const std::string full_path(directory_path_ + "/" + current_path);
     entry_ = makeEntry(entry->d_name);
   }
 }
@@ -73,7 +71,7 @@ DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) cons
     file_type = FileType::Directory;
   } else if (S_ISREG(stat_buf.st_mode)) {
     file_type = FileType::Regular;
-  }
+  } // else use the already-assigned FileType::Other.
   return DirectoryEntry{std::string{filename}, file_type, static_cast<uint64_t>(stat_buf.st_size)};
 }
 

From 78d3c799a62c4de4df370af01942e2e8c16aab91 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Mon, 28 Nov 2022 15:28:03 +0000
Subject: [PATCH 07/18] TODO for not throwing exception.

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 source/common/filesystem/posix/directory_iterator_impl.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/source/common/filesystem/posix/directory_iterator_impl.cc b/source/common/filesystem/posix/directory_iterator_impl.cc
index 87465202bf1f..4d0b2f96b660 100644
--- a/source/common/filesystem/posix/directory_iterator_impl.cc
+++ b/source/common/filesystem/posix/directory_iterator_impl.cc
@@ -65,6 +65,9 @@ DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) cons
       }
     }
     if (file_type == FileType::Other) {
+      // 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));
     }
   } else if (S_ISDIR(stat_buf.st_mode)) {

From 2a6b3ab7fd5fca0d6e78647eb60ccbcd30e7dbed Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Mon, 28 Nov 2022 15:34:23 +0000
Subject: [PATCH 08/18] Assert success of test file operations

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 test/common/filesystem/directory_test.cc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index ff236276c9ea..932559d5e10b 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -43,7 +43,10 @@ class DirectoryTest : public testing::Test {
   void addFiles(std::list<std::string> 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);
+        ASSERT_TRUE(file) << "failed to open test file";
+      }
       files_to_remove_.push(full_path);
     }
   }
@@ -52,7 +55,10 @@ class DirectoryTest : public testing::Test {
     const std::string full_path = absl::StrCat(dir_path_, "/", file_name);
     {
       std::ofstream file(full_path);
+      ASSERT_TRUE(file) << "failed to open test file";
       file << contents;
+      file.close();
+      ASSERT_TRUE(file) << "failed to write to test file";
     }
     files_to_remove_.push(full_path);
   }

From 6037bb9348c356bc072dd2a545477caae2d39a52 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Tue, 29 Nov 2022 15:40:30 +0000
Subject: [PATCH 09/18] Fix operator==, improve test output, simplify flow

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 envoy/filesystem/filesystem.h                 |  4 +--
 .../posix/directory_iterator_impl.cc          | 27 +++++++++----------
 .../win32/directory_iterator_impl.cc          | 10 +++----
 test/common/filesystem/directory_test.cc      | 12 ++++++---
 4 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/envoy/filesystem/filesystem.h b/envoy/filesystem/filesystem.h
index 569be672ea1d..7d28c6d6b949 100644
--- a/envoy/filesystem/filesystem.h
+++ b/envoy/filesystem/filesystem.h
@@ -169,11 +169,11 @@ 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.
+  // The file size in bytes. 0 for directories or broken symlinks.
   uint64_t 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_;
   }
 };
 
diff --git a/source/common/filesystem/posix/directory_iterator_impl.cc b/source/common/filesystem/posix/directory_iterator_impl.cc
index 4d0b2f96b660..7766d1728093 100644
--- a/source/common/filesystem/posix/directory_iterator_impl.cc
+++ b/source/common/filesystem/posix/directory_iterator_impl.cc
@@ -51,31 +51,30 @@ void DirectoryIteratorImpl::nextEntry() {
 DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) const {
   const std::string full_path = absl::StrCat(directory_path_, "/", filename);
   struct stat stat_buf;
-  FileType file_type = FileType::Other;
-
   const Api::SysCallIntResult result = os_sys_calls_.stat(full_path.c_str(), &stat_buf);
   if (result.return_value_ != 0) {
     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.
+      // a regular file, which may be unlink()'ed, with size 0.
       if (::lstat(full_path.c_str(), &stat_buf) == 0 && S_ISLNK(stat_buf.st_mode)) {
-        file_type = FileType::Regular;
+        return DirectoryEntry{std::string{filename}, FileType::Regular, 0};
       }
     }
-    if (file_type == FileType::Other) {
-      // 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));
-    }
+    // 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));
   } else if (S_ISDIR(stat_buf.st_mode)) {
-    file_type = FileType::Directory;
+    return DirectoryEntry{std::string{filename}, FileType::Directory, 0};
   } else if (S_ISREG(stat_buf.st_mode)) {
-    file_type = FileType::Regular;
-  } // else use the already-assigned FileType::Other.
-  return DirectoryEntry{std::string{filename}, file_type, static_cast<uint64_t>(stat_buf.st_size)};
+    return DirectoryEntry{std::string{filename}, FileType::Regular,
+                          static_cast<uint64_t>(stat_buf.st_size)};
+  } else {
+    return DirectoryEntry{std::string{filename}, FileType::Other,
+                          static_cast<uint64_t>(stat_buf.st_size)};
+  }
 }
 
 } // namespace Filesystem
diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc
index ecbea3810dfc..35a5aae44c8c 100644
--- a/source/common/filesystem/win32/directory_iterator_impl.cc
+++ b/source/common/filesystem/win32/directory_iterator_impl.cc
@@ -43,19 +43,17 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() {
 }
 
 DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data) {
-  FileType file_type;
+  uint64_t size = static_cast<uint64_t>(find_data.nFileSizeHigh) << 32 + find_data.nFileSizeLow;
   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
-    file_type = FileType::Other;
+    return {std::string(find_data.cFileName), FileType::Other, size};
   } else if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
-    file_type = FileType::Directory;
+    return {std::string(find_data.cFileName), FileType::Directory, 0};
   } else {
-    file_type = FileType::Regular;
+    return {std::string(find_data.cFileName), FileType::Regular, size};
   }
-  return {std::string(find_data.cFileName), file_type,
-          static_cast<uint64_t>(find_data.nFileSizeHigh) << 32 + find_data.nFileSizeLow};
 }
 
 } // namespace Filesystem
diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index 932559d5e10b..d1176b4925b1 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -15,6 +15,11 @@
 namespace Envoy {
 namespace Filesystem {
 
+void PrintTo(const DirectoryEntry& entry, std::ostream* os) {
+  *os << "{name=" << entry.name_ << ", type=" << static_cast<int>(entry.type_)
+      << ", size=" << entry.size_bytes_ << "}";
+}
+
 class DirectoryTest : public testing::Test {
 public:
   DirectoryTest() : dir_path_(TestEnvironment::temporaryPath("envoy_test")) {
@@ -183,14 +188,15 @@ TEST_F(DirectoryTest, DirectoryWithFileAndDirectory) {
 
 // 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, 0},
       {"..", FileType::Directory, 0},
-      {"file", FileType::Regular, 0},
-      {"link", FileType::Regular, 0},
+      {"file", FileType::Regular, contents.size()},
+      {"link", FileType::Regular, contents.size()},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }

From 99a7632199fcd33121bc4b7a69a8883790c544c8 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Tue, 29 Nov 2022 17:24:23 +0000
Subject: [PATCH 10/18] ASSERT->EXPECT

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 test/common/filesystem/directory_test.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index d1176b4925b1..faff8964282b 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -50,7 +50,7 @@ class DirectoryTest : public testing::Test {
       const std::string full_path = dir_path_ + "/" + file_name;
       {
         const std::ofstream file(full_path);
-        ASSERT_TRUE(file) << "failed to open test file";
+        EXPECT_TRUE(file) << "failed to open test file";
       }
       files_to_remove_.push(full_path);
     }
@@ -60,10 +60,10 @@ class DirectoryTest : public testing::Test {
     const std::string full_path = absl::StrCat(dir_path_, "/", file_name);
     {
       std::ofstream file(full_path);
-      ASSERT_TRUE(file) << "failed to open test file";
+      EXPECT_TRUE(file) << "failed to open test file";
       file << contents;
       file.close();
-      ASSERT_TRUE(file) << "failed to write to test file";
+      EXPECT_TRUE(file) << "failed to write to test file";
     }
     files_to_remove_.push(full_path);
   }

From 2ce171da51c8c1d09fa6e27798462290912d4bcc Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Tue, 29 Nov 2022 17:52:04 +0000
Subject: [PATCH 11/18] Windows style large integers without byte shuffling

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 source/common/filesystem/win32/directory_iterator_impl.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc
index 35a5aae44c8c..c9aa72b4cbdf 100644
--- a/source/common/filesystem/win32/directory_iterator_impl.cc
+++ b/source/common/filesystem/win32/directory_iterator_impl.cc
@@ -43,7 +43,10 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() {
 }
 
 DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data) {
-  uint64_t size = static_cast<uint64_t>(find_data.nFileSizeHigh) << 32 + find_data.nFileSizeLow;
+  LARGE_INTEGER file_size;
+  file_size.LowPart = find_data.nFileSizeLow;
+  file_size.HighPart = find_data.nFileSizeHigh;
+  uint64_t size = static_cast<uint64_t>(file_size.QuadPart);
   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

From 2eecbfcc5a918b9c6187e67fdb3741adab5679ae Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Tue, 29 Nov 2022 17:54:37 +0000
Subject: [PATCH 12/18] *Unsigned* large integer

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 source/common/filesystem/win32/directory_iterator_impl.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc
index c9aa72b4cbdf..bfe82eff4d04 100644
--- a/source/common/filesystem/win32/directory_iterator_impl.cc
+++ b/source/common/filesystem/win32/directory_iterator_impl.cc
@@ -43,7 +43,7 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() {
 }
 
 DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data) {
-  LARGE_INTEGER file_size;
+  ULARGE_INTEGER file_size;
   file_size.LowPart = find_data.nFileSizeLow;
   file_size.HighPart = find_data.nFileSizeHigh;
   uint64_t size = static_cast<uint64_t>(file_size.QuadPart);

From 4e840cd5140f6649a09d390a0d6eb32bdadcd43f Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Tue, 29 Nov 2022 20:02:30 +0000
Subject: [PATCH 13/18] Turn off linter for PrintTo

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 test/common/filesystem/directory_test.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index faff8964282b..4f9d07dfee36 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -15,6 +15,7 @@
 namespace Envoy {
 namespace Filesystem {
 
+// NOLINTNEXTLINE(readability-identifier-naming)
 void PrintTo(const DirectoryEntry& entry, std::ostream* os) {
   *os << "{name=" << entry.name_ << ", type=" << static_cast<int>(entry.type_)
       << ", size=" << entry.size_bytes_ << "}";

From 5acaaf70d92c6fb6d21aa83224742ef2c09db585 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Wed, 30 Nov 2022 18:43:40 +0000
Subject: [PATCH 14/18] Allow indeterminate size for non-regular files; Windows
 has size-0 symlinks, Linux has actual file size symlinks.

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 envoy/filesystem/filesystem.h                      |  3 ++-
 .../filesystem/posix/directory_iterator_impl.cc    |  3 +--
 .../filesystem/win32/directory_iterator_impl.cc    |  2 +-
 test/common/filesystem/directory_test.cc           | 14 +++++++-------
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/envoy/filesystem/filesystem.h b/envoy/filesystem/filesystem.h
index 7d28c6d6b949..7a7365a1abd0 100644
--- a/envoy/filesystem/filesystem.h
+++ b/envoy/filesystem/filesystem.h
@@ -169,7 +169,8 @@ 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. 0 for directories or broken symlinks.
+  // The file size in bytes for regular files. Should not be relied on for directories,
+  // symlinks, or FileType::Other.
   uint64_t size_bytes_;
 
   bool operator==(const DirectoryEntry& rhs) const {
diff --git a/source/common/filesystem/posix/directory_iterator_impl.cc b/source/common/filesystem/posix/directory_iterator_impl.cc
index 7766d1728093..6d313257075a 100644
--- a/source/common/filesystem/posix/directory_iterator_impl.cc
+++ b/source/common/filesystem/posix/directory_iterator_impl.cc
@@ -72,8 +72,7 @@ DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) cons
     return DirectoryEntry{std::string{filename}, FileType::Regular,
                           static_cast<uint64_t>(stat_buf.st_size)};
   } else {
-    return DirectoryEntry{std::string{filename}, FileType::Other,
-                          static_cast<uint64_t>(stat_buf.st_size)};
+    return DirectoryEntry{std::string{filename}, FileType::Other, 0};
   }
 }
 
diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc
index bfe82eff4d04..7cef9ce8867b 100644
--- a/source/common/filesystem/win32/directory_iterator_impl.cc
+++ b/source/common/filesystem/win32/directory_iterator_impl.cc
@@ -51,7 +51,7 @@ 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, size};
+    return {std::string(find_data.cFileName), FileType::Other, 0};
   } else if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
     return {std::string(find_data.cFileName), FileType::Directory, 0};
   } else {
diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index 4f9d07dfee36..3c7962e68d1f 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -193,13 +193,13 @@ TEST_F(DirectoryTest, DirectoryWithSymlinkToFile) {
   addFileWithContents("file", contents);
   addSymlinks({{"file", "link"}});
 
-  const EntrySet expected = {
-      {".", FileType::Directory, 0},
-      {"..", FileType::Directory, 0},
-      {"file", FileType::Regular, contents.size()},
-      {"link", FileType::Regular, contents.size()},
-  };
-  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 0 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

From ae4ae1feae6c0d1162ec0bc30aa804c9e6628eaf Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Mon, 5 Dec 2022 16:20:22 +0000
Subject: [PATCH 15/18] Use `absl::nullopt` to indicate size unknown

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 envoy/filesystem/filesystem.h                 |  9 ++-
 .../posix/directory_iterator_impl.cc          | 10 +--
 .../win32/directory_iterator_impl.cc          |  9 ++-
 test/common/filesystem/directory_test.cc      | 76 ++++++++++---------
 4 files changed, 55 insertions(+), 49 deletions(-)

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<uint64_t> 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<uint64_t>(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<int>(entry.type_)
-      << ", size=" << entry.size_bytes_ << "}";
+  *os << "{name=" << entry.name_ << ", type=" << static_cast<int>(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);

From 0257b0579aef29d6ff910efce756f050a841db14 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Mon, 5 Dec 2022 16:22:04 +0000
Subject: [PATCH 16/18] Correct test comment that still said 0

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 test/common/filesystem/directory_test.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index 18fb2e43b09b..90ca5376bb6e 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -200,7 +200,7 @@ TEST_F(DirectoryTest, DirectoryWithSymlinkToFile) {
   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 0 or file-size depending on OS.
+  // 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))));

From de66291932572603a047a3848d4440e8cfb54dda Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Tue, 6 Dec 2022 20:47:32 +0000
Subject: [PATCH 17/18] Only get size when used, and fix grouping, for Win32

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 .../filesystem/win32/directory_iterator_impl.cc     | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc
index 094e548c4bb8..3391bce11cb9 100644
--- a/source/common/filesystem/win32/directory_iterator_impl.cc
+++ b/source/common/filesystem/win32/directory_iterator_impl.cc
@@ -43,10 +43,6 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() {
 }
 
 DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data) {
-  ULARGE_INTEGER file_size;
-  file_size.LowPart = find_data.nFileSizeLow;
-  file_size.HighPart = find_data.nFileSizeHigh;
-  uint64_t size = static_cast<uint64_t>(file_size.QuadPart);
   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
@@ -54,9 +50,14 @@ DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data
     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 {
-    return {std::string(find_data.cFileName), FileType::Regular,
-            (find_data.dwReserved0 & IO_REPARSE_TAG_SYMLINK) ? absl::nullopt : size};
+    ULARGE_INTEGER file_size;
+    file_size.LowPart = find_data.nFileSizeLow;
+    file_size.HighPart = find_data.nFileSizeHigh;
+    uint64_t size = static_cast<uint64_t>(file_size.QuadPart);
+    return {std::string(find_data.cFileName), FileType::Regular, size};
   }
 }
 

From f11bd48c41d7ce4b5c444df50edc9c6c1d6700b7 Mon Sep 17 00:00:00 2001
From: Raven Black <ravenblack@dropbox.com>
Date: Tue, 6 Dec 2022 21:10:11 +0000
Subject: [PATCH 18/18] Add testing of DirectoryEntry == operator

Signed-off-by: Raven Black <ravenblack@dropbox.com>
---
 test/common/filesystem/directory_test.cc | 25 ++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
index 90ca5376bb6e..75f3715dc793 100644
--- a/test/common/filesystem/directory_test.cc
+++ b/test/common/filesystem/directory_test.cc
@@ -306,5 +306,30 @@ TEST(Directory, DirectoryHasTrailingPathSeparator) {
   TestEnvironment::removePath(dir_path);
 }
 
+TEST(DirectoryEntry, EqualityOperator) {
+  std::vector<DirectoryEntry> 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