Add file size to DirectoryEntry

envoy/filesystem/filesystem.h

@@ -169,15 +169,18 @@ 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.
+  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_;
   }
 };
 
 class DirectoryIteratorImpl;
 class DirectoryIterator {
 public:
-  DirectoryIterator() : entry_({"", FileType::Other}) {}
+  DirectoryIterator() : entry_({"", FileType::Other, 0}) {}
   virtual ~DirectoryIterator() = default;
 
   const DirectoryEntry& operator*() const { return entry_; }

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;
-
-  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.
+      // 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)) {
-        return FileType::Regular;
+        return DirectoryEntry{std::string{filename}, FileType::Regular, 0};
       }
     }
+    // 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, 0};
   } else if (S_ISREG(stat_buf.st_mode)) {
-    return FileType::Regular;
+    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 FileType::Other;
 }
 
 } // namespace Filesystem

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

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,29 @@ 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) {
+  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
     // 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, size};
+  } else if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
+    return {std::string(find_data.cFileName), FileType::Directory, 0};
+  } else {
+    return {std::string(find_data.cFileName), FileType::Regular, size};
   }
-
-  return FileType::Regular;
 }
 
 } // namespace Filesystem

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_;
 };

test/common/filesystem/directory_test.cc

@@ -15,6 +15,12 @@
 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_ << "}";
+}
+
 class DirectoryTest : public testing::Test {
 public:
   DirectoryTest() : dir_path_(TestEnvironment::temporaryPath("envoy_test")) {
@@ -43,11 +49,26 @@ 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);
+        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<std::pair<std::string, std::string>> symlinks) {
     for (const auto& link : symlinks) {
       const std::string target_path = dir_path_ + "/" + link.first;
@@ -79,7 +100,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 +112,21 @@ 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));
+}
+
+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));
 }
@@ -103,9 +136,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 +149,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 +162,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,24 +179,25 @@ 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));
 }
 
 // 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},
+      {".", FileType::Directory, 0},
+      {"..", FileType::Directory, 0},
+      {"file", FileType::Regular, contents.size()},
+      {"link", FileType::Regular, contents.size()},
   };
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
@@ -174,10 +208,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 +223,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 +239,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 +266,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, 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' .*");
+// 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,8 +295,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);