Skip to content

Commit

Permalink
Use absl::nullopt to indicate size unknown
Browse files Browse the repository at this point in the history 
Signed-off-by: Raven Black <ravenblack@dropbox.com>
  • Loading branch information
@ravenblackx
ravenblackx committed Dec 5, 2022
1 parent b582141 commit ae4ae1f
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 49 deletions.
9 changes: 5 additions & 4 deletions envoy/filesystem/filesystem.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "envoy/common/pure.h"

#include "absl/strings/string_view.h"
#include "absl/types/optional.h"

namespace Envoy {
namespace Filesystem {
Expand Down Expand Up @@ -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_;
Expand All @@ -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_; }
Expand Down
10 changes: 5 additions & 5 deletions source/common/filesystem/posix/directory_iterator_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -57,22 +57,22 @@ 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,
// 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)) {
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};
}
}

Expand Down
9 changes: 5 additions & 4 deletions source/common/filesystem/win32/directory_iterator_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() {
}

if (ret == 0) {
entry_ = {"", FileType::Other, 0};
entry_ = {"", FileType::Other, absl::nullopt};
} else {
entry_ = makeEntry(find_data);
}
Expand All @@ -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};
}
}

Expand Down
76 changes: 40 additions & 36 deletions test/common/filesystem/directory_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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));
}
Expand All @@ -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));
}
Expand All @@ -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));
}
Expand All @@ -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));
Expand All @@ -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));
}
Expand All @@ -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));
Expand All @@ -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));
}
Expand All @@ -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());
Expand All @@ -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);
Expand Down

0 comments on commit ae4ae1f

Please sign in to comment.