Add file size to DirectoryEntry

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

source/common/filesystem/posix/directory_iterator_impl.cc

@@ -42,39 +42,40 @@ 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) {
+      // 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;
   } else if (S_ISREG(stat_buf.st_mode)) {
-    return FileType::Regular;
-  }
-
-  return FileType::Other;
+    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)};
 }
 
 } // 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,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) {
+  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

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

@@ -43,11 +43,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);
+        ASSERT_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);
+      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);
+  }
+
   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 +94,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 +106,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 +130,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 +143,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 +156,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 +173,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 +187,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 +201,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 +216,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 +232,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 +259,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 +288,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);