From 75196701376825cf8c0a28bbba09746b7d0d9aea Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Sun, 13 Sep 2020 20:58:00 -0700 Subject: [PATCH 01/10] fixes issue #11655. Adds a native windows filesystem implementation Signed-off-by: Sotiris Nanopoulos --- include/envoy/common/platform.h | 11 +++ source/common/filesystem/file_shared_impl.cc | 43 +++------- source/common/filesystem/file_shared_impl.h | 12 +-- .../filesystem/posix/filesystem_impl.cc | 21 +++-- .../common/filesystem/posix/filesystem_impl.h | 8 +- .../filesystem/win32/filesystem_impl.cc | 79 +++++++++++++------ .../common/filesystem/win32/filesystem_impl.h | 12 +-- .../common/filesystem/filesystem_impl_test.cc | 15 ++-- 8 files changed, 118 insertions(+), 83 deletions(-) diff --git a/include/envoy/common/platform.h b/include/envoy/common/platform.h index 3bc541286362..1b04d578de05 100644 --- a/include/envoy/common/platform.h +++ b/include/envoy/common/platform.h @@ -61,6 +61,7 @@ typedef ptrdiff_t ssize_t; typedef uint32_t mode_t; typedef SOCKET os_fd_t; +typedef HANDLE os_h_t; typedef unsigned int sa_family_t; @@ -116,6 +117,7 @@ struct msghdr { #define IPV6_RECVPKTINFO IPV6_PKTINFO #endif +#define INVALID_HANDLE INVALID_HANDLE_VALUE #define SOCKET_VALID(sock) ((sock) != INVALID_SOCKET) #define SOCKET_INVALID(sock) ((sock) == INVALID_SOCKET) #define SOCKET_FAILURE(rc) ((rc) == SOCKET_ERROR) @@ -143,6 +145,9 @@ struct msghdr { #define SOCKET_ERROR_INVAL WSAEINVAL #define SOCKET_ERROR_ADDR_IN_USE WSAEADDRINUSE +#define HANDLE_ERROR_PERM ERROR_ACCESS_DENIED +#define HANDLE_ERROR_INVALID ERROR_INVALID_HANDLE + namespace Platform { constexpr absl::string_view null_device_path{"NUL"}; } @@ -206,7 +211,9 @@ constexpr absl::string_view null_device_path{"NUL"}; #endif typedef int os_fd_t; +typedef int os_h_t; +#define INVALID_HANDLE -1 #define INVALID_SOCKET -1 #define SOCKET_VALID(sock) ((sock) >= 0) #define SOCKET_INVALID(sock) ((sock) == -1) @@ -231,6 +238,10 @@ typedef int os_fd_t; #define SOCKET_ERROR_INVAL EINVAL #define SOCKET_ERROR_ADDR_IN_USE EADDRINUSE +// Mapping POSIX file errors to common error names +#define HANDLE_ERROR_PERM EACCES +#define HANDLE_ERROR_INVALID EBADF + namespace Platform { constexpr absl::string_view null_device_path{"/dev/null"}; } diff --git a/source/common/filesystem/file_shared_impl.cc b/source/common/filesystem/file_shared_impl.cc index 56601badb01c..e7dc1775f60b 100644 --- a/source/common/filesystem/file_shared_impl.cc +++ b/source/common/filesystem/file_shared_impl.cc @@ -1,43 +1,26 @@ #include "common/filesystem/file_shared_impl.h" - -#include +#include "common/common/utility.h" namespace Envoy { namespace Filesystem { -Api::IoError::IoErrorCode IoFileError::getErrorCode() const { return IoErrorCode::UnknownError; } - -std::string IoFileError::getErrorDetails() const { - // TODO(sunjayBhatia, wrowe): Disable clang-format until win32 implementation no longer uses POSIX - // subsystem, see https://github.com/envoyproxy/envoy/issues/11655 - // clang-format off - return ::strerror(errno_); - // clang-format on -} - -Api::IoCallBoolResult FileSharedImpl::open(FlagSet in) { - if (isOpen()) { - return resultSuccess(true); +Api::IoError::IoErrorCode IoFileError::getErrorCode() const { + switch (errno_) { + case HANDLE_ERROR_PERM: + return IoErrorCode::Permission; + case HANDLE_ERROR_INVALID: + return IoErrorCode::BadFd; + default: + ENVOY_LOG_MISC(debug, "Unknown error code {} details {}", errno_, getErrorDetails()); + return IoErrorCode::UnknownError; } - - openFile(in); - return fd_ != -1 ? resultSuccess(true) : resultFailure(false, errno); } -Api::IoCallSizeResult FileSharedImpl::write(absl::string_view buffer) { - const ssize_t rc = writeFile(buffer); - return rc != -1 ? resultSuccess(rc) : resultFailure(rc, errno); -}; - -Api::IoCallBoolResult FileSharedImpl::close() { - ASSERT(isOpen()); - - bool success = closeFile(); - fd_ = -1; - return success ? resultSuccess(true) : resultFailure(false, errno); +std::string IoFileError::getErrorDetails() const { + return errorDetails(errno_); } -bool FileSharedImpl::isOpen() const { return fd_ != -1; }; +bool FileSharedImpl::isOpen() const { return fd_ != INVALID_HANDLE; }; std::string FileSharedImpl::path() const { return path_; }; diff --git a/source/common/filesystem/file_shared_impl.h b/source/common/filesystem/file_shared_impl.h index 06e166661287..1af0d1372296 100644 --- a/source/common/filesystem/file_shared_impl.h +++ b/source/common/filesystem/file_shared_impl.h @@ -37,23 +37,15 @@ template Api::IoCallResult resultSuccess(T result) { class FileSharedImpl : public File { public: - FileSharedImpl(std::string path) : fd_(-1), path_(std::move(path)) {} + FileSharedImpl(std::string path) : fd_(INVALID_HANDLE), path_(std::move(path)) {} ~FileSharedImpl() override = default; - // Filesystem::File - Api::IoCallBoolResult open(FlagSet flag) override; - Api::IoCallSizeResult write(absl::string_view buffer) override; - Api::IoCallBoolResult close() override; bool isOpen() const override; std::string path() const override; protected: - virtual void openFile(FlagSet in) PURE; - virtual ssize_t writeFile(absl::string_view buffer) PURE; - virtual bool closeFile() PURE; - - int fd_; + os_h_t fd_; const std::string path_; }; diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc index e24814d0ca70..5f69e98e764b 100644 --- a/source/common/filesystem/posix/filesystem_impl.cc +++ b/source/common/filesystem/posix/filesystem_impl.cc @@ -30,13 +30,26 @@ FileImplPosix::~FileImplPosix() { } } -void FileImplPosix::openFile(FlagSet in) { +Api::IoCallBoolResult FileImplPosix::open(FlagSet in) { + if (isOpen()) { + return resultSuccess(true); + } + const auto flags_and_mode = translateFlag(in); fd_ = ::open(path_.c_str(), flags_and_mode.flags_, flags_and_mode.mode_); + return fd_ != -1 ? resultSuccess(true) : resultFailure(false, errno); } -ssize_t FileImplPosix::writeFile(absl::string_view buffer) { - return ::write(fd_, buffer.data(), buffer.size()); +Api::IoCallSizeResult FileImplPosix::write(absl::string_view buffer) { + const ssize_t rc = ::write(fd_, buffer.data(), buffer.size()); + return rc != -1 ? resultSuccess(rc) : resultFailure(rc, errno); +}; + +Api::IoCallBoolResult FileImplPosix::close() { + ASSERT(isOpen()); + int rc = ::close(fd_); + fd_ = -1; + return (rc != -1) ? resultSuccess(true) : resultFailure(false, errno); } FileImplPosix::FlagsAndMode FileImplPosix::translateFlag(FlagSet in) { @@ -62,8 +75,6 @@ FileImplPosix::FlagsAndMode FileImplPosix::translateFlag(FlagSet in) { return {out, mode}; } -bool FileImplPosix::closeFile() { return ::close(fd_) != -1; } - FilePtr InstanceImplPosix::createFile(const std::string& path) { return std::make_unique(path); } diff --git a/source/common/filesystem/posix/filesystem_impl.h b/source/common/filesystem/posix/filesystem_impl.h index 173be8918d33..310653e21030 100644 --- a/source/common/filesystem/posix/filesystem_impl.h +++ b/source/common/filesystem/posix/filesystem_impl.h @@ -21,11 +21,11 @@ class FileImplPosix : public FileSharedImpl { mode_t mode_ = 0; }; - // Filesystem::FileSharedImpl + Api::IoCallBoolResult open(FlagSet flag) override; + Api::IoCallSizeResult write(absl::string_view buffer) override; + Api::IoCallBoolResult close() override; + FlagsAndMode translateFlag(FlagSet in); - void openFile(FlagSet flags) override; - ssize_t writeFile(absl::string_view buffer) override; - bool closeFile() override; private: friend class FileSystemImplTest; diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc index 268408e2dd67..67fc36f53d1c 100644 --- a/source/common/filesystem/win32/filesystem_impl.cc +++ b/source/common/filesystem/win32/filesystem_impl.cc @@ -26,40 +26,67 @@ FileImplWin32::~FileImplWin32() { } } -void FileImplWin32::openFile(FlagSet in) { - const auto flags_and_mode = translateFlag(in); - fd_ = ::open(path_.c_str(), flags_and_mode.flags_, flags_and_mode.pmode_); +Api::IoCallBoolResult FileImplWin32::open(FlagSet in) { + if (isOpen()) { + return resultSuccess(true); + } + + auto flags = translateFlag(in); + fd_ = CreateFileA(path_.c_str(), flags.access_, NULL, NULL, flags.creation_, NULL, NULL); + if (fd_ == INVALID_HANDLE) { + return resultFailure(false, ::GetLastError()); + } + return resultSuccess(true); } -ssize_t FileImplWin32::writeFile(absl::string_view buffer) { - return ::_write(fd_, buffer.data(), buffer.size()); +Api::IoCallSizeResult FileImplWin32::write(absl::string_view buffer) { + DWORD bytes_written; + BOOL result = WriteFile(fd_, buffer.data(), buffer.length(), &bytes_written, NULL); + if (result == 0) { + return resultFailure(-1, ::GetLastError()); + } + return resultSuccess(bytes_written); +}; + +Api::IoCallBoolResult FileImplWin32::close() { + ASSERT(isOpen()); + + BOOL result = CloseHandle(fd_); + fd_ = INVALID_HANDLE; + if (result == 0) { + return resultFailure(false, ::GetLastError()); + } + return resultSuccess(true); } + FileImplWin32::FlagsAndMode FileImplWin32::translateFlag(FlagSet in) { - int out = 0; - int pmode = 0; + DWORD access = 0; + DWORD creation = OPEN_EXISTING; + if (in.test(File::Operation::Create)) { - out |= _O_CREAT; - pmode |= _S_IREAD | _S_IWRITE; + creation = OPEN_ALWAYS; } + if (in.test(File::Operation::Write)) { + access = GENERIC_WRITE; + } + + // Order of tests matter here. There reason for that + // is that `FILE_APPEND_DATA` should not be used together + // with `GENERIC_WRITE`. If both of them are used the file + // is not opened in append mode. if (in.test(File::Operation::Append)) { - out |= _O_APPEND; + access = FILE_APPEND_DATA; } - if (in.test(File::Operation::Read) && in.test(File::Operation::Write)) { - out |= _O_RDWR; - } else if (in.test(File::Operation::Read)) { - out |= _O_RDONLY; - } else if (in.test(File::Operation::Write)) { - out |= _O_WRONLY; + if (in.test(File::Operation::Read)) { + access |= GENERIC_READ; } - return {out, pmode}; + return {access, creation}; } -bool FileImplWin32::closeFile() { return ::_close(fd_) != -1; } - FilePtr InstanceImplWin32::createFile(const std::string& path) { return std::make_unique(path); } @@ -78,11 +105,19 @@ bool InstanceImplWin32::directoryExists(const std::string& path) { } ssize_t InstanceImplWin32::fileSize(const std::string& path) { - struct _stat info; - if (::_stat(path.c_str(), &info) != 0) { + auto fd = CreateFileA(path.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, NULL, NULL); + if (fd == INVALID_HANDLE) { + return -1; + } + ssize_t result = 0; + LARGE_INTEGER lFileSize; + BOOL bGetSize = GetFileSizeEx(fd, &lFileSize); + CloseHandle(fd); + if(!bGetSize) { return -1; } - return info.st_size; + result += lFileSize.QuadPart; + return result; } std::string InstanceImplWin32::fileReadToEnd(const std::string& path) { diff --git a/source/common/filesystem/win32/filesystem_impl.h b/source/common/filesystem/win32/filesystem_impl.h index f39b40378d64..4fbfa6eaee96 100644 --- a/source/common/filesystem/win32/filesystem_impl.h +++ b/source/common/filesystem/win32/filesystem_impl.h @@ -15,15 +15,15 @@ class FileImplWin32 : public FileSharedImpl { protected: struct FlagsAndMode { - int flags_ = 0; - int pmode_ = 0; + DWORD access_ = 0; + DWORD creation_ = 0; }; - // Filesystem::FileSharedImpl + Api::IoCallBoolResult open(FlagSet flag) override; + Api::IoCallSizeResult write(absl::string_view buffer) override; + Api::IoCallBoolResult close() override; + FlagsAndMode translateFlag(FlagSet in); - void openFile(FlagSet in) override; - ssize_t writeFile(absl::string_view buffer) override; - bool closeFile() override; private: friend class FileSystemImplTest; diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index 7870c285e19d..1c00f39eec24 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -19,7 +19,7 @@ static constexpr FlagSet DefaultFlags{ class FileSystemImplTest : public testing::Test { protected: - int getFd(File* file) { + os_h_t getFd(File* file) { #ifdef WIN32 auto file_impl = dynamic_cast(file); #else @@ -240,10 +240,10 @@ TEST_F(FileSystemImplTest, OpenTwice) { ::unlink(new_file_path.c_str()); FilePtr file = file_system_.createFile(new_file_path); - EXPECT_EQ(getFd(file.get()), -1); + EXPECT_EQ(getFd(file.get()), INVALID_HANDLE); const Api::IoCallBoolResult result1 = file->open(DefaultFlags); - const int initial_fd = getFd(file.get()); + const os_h_t initial_fd = getFd(file.get()); EXPECT_TRUE(result1.rc_); EXPECT_TRUE(file->isOpen()); @@ -319,8 +319,7 @@ TEST_F(FileSystemImplTest, WriteAfterClose) { EXPECT_TRUE(bool_result2.rc_); const Api::IoCallSizeResult size_result = file->write(" new data"); EXPECT_EQ(-1, size_result.rc_); - EXPECT_EQ(IoFileError::IoErrorCode::UnknownError, size_result.err_->getErrorCode()); - EXPECT_EQ("Bad file descriptor", size_result.err_->getErrorDetails()); + EXPECT_EQ(IoFileError::IoErrorCode::BadFd, size_result.err_->getErrorCode()); } TEST_F(FileSystemImplTest, NonExistingFileAndReadOnly) { @@ -345,7 +344,11 @@ TEST_F(FileSystemImplTest, ExistingReadOnlyFileAndWrite) { std::string data(" new data"); const Api::IoCallSizeResult result = file->write(data); EXPECT_TRUE(result.rc_ < 0); - EXPECT_EQ(result.err_->getErrorDetails(), "Bad file descriptor"); +#ifdef WIN32 + EXPECT_EQ(IoFileError::IoErrorCode::Permission, result.err_->getErrorCode()); +#else + EXPECT_EQ(IoFileError::IoErrorCode::BadFd, result.err_->getErrorCode()); +#endif } auto contents = TestEnvironment::readFileToStringForTest(file_path); From 31e408346fed133a72aa1ccd053f731badcd7966 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Mon, 14 Sep 2020 09:34:58 -0700 Subject: [PATCH 02/10] format Signed-off-by: Sotiris Nanopoulos --- include/envoy/common/platform.h | 2 +- source/common/filesystem/file_shared_impl.cc | 21 +++++++++---------- .../filesystem/win32/filesystem_impl.cc | 8 +++---- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/include/envoy/common/platform.h b/include/envoy/common/platform.h index 1b04d578de05..47ef701216c1 100644 --- a/include/envoy/common/platform.h +++ b/include/envoy/common/platform.h @@ -238,7 +238,7 @@ typedef int os_h_t; #define SOCKET_ERROR_INVAL EINVAL #define SOCKET_ERROR_ADDR_IN_USE EADDRINUSE -// Mapping POSIX file errors to common error names +// Mapping POSIX file errors to common error names #define HANDLE_ERROR_PERM EACCES #define HANDLE_ERROR_INVALID EBADF diff --git a/source/common/filesystem/file_shared_impl.cc b/source/common/filesystem/file_shared_impl.cc index e7dc1775f60b..5c76ea6c29de 100644 --- a/source/common/filesystem/file_shared_impl.cc +++ b/source/common/filesystem/file_shared_impl.cc @@ -1,24 +1,23 @@ #include "common/filesystem/file_shared_impl.h" + #include "common/common/utility.h" namespace Envoy { namespace Filesystem { -Api::IoError::IoErrorCode IoFileError::getErrorCode() const { +Api::IoError::IoErrorCode IoFileError::getErrorCode() const { switch (errno_) { - case HANDLE_ERROR_PERM: - return IoErrorCode::Permission; - case HANDLE_ERROR_INVALID: - return IoErrorCode::BadFd; - default: - ENVOY_LOG_MISC(debug, "Unknown error code {} details {}", errno_, getErrorDetails()); - return IoErrorCode::UnknownError; + case HANDLE_ERROR_PERM: + return IoErrorCode::Permission; + case HANDLE_ERROR_INVALID: + return IoErrorCode::BadFd; + default: + ENVOY_LOG_MISC(debug, "Unknown error code {} details {}", errno_, getErrorDetails()); + return IoErrorCode::UnknownError; } } -std::string IoFileError::getErrorDetails() const { - return errorDetails(errno_); -} +std::string IoFileError::getErrorDetails() const { return errorDetails(errno_); } bool FileSharedImpl::isOpen() const { return fd_ != INVALID_HANDLE; }; diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc index 67fc36f53d1c..0da4efc237d4 100644 --- a/source/common/filesystem/win32/filesystem_impl.cc +++ b/source/common/filesystem/win32/filesystem_impl.cc @@ -59,7 +59,6 @@ Api::IoCallBoolResult FileImplWin32::close() { return resultSuccess(true); } - FileImplWin32::FlagsAndMode FileImplWin32::translateFlag(FlagSet in) { DWORD access = 0; DWORD creation = OPEN_EXISTING; @@ -71,7 +70,7 @@ FileImplWin32::FlagsAndMode FileImplWin32::translateFlag(FlagSet in) { if (in.test(File::Operation::Write)) { access = GENERIC_WRITE; } - + // Order of tests matter here. There reason for that // is that `FILE_APPEND_DATA` should not be used together // with `GENERIC_WRITE`. If both of them are used the file @@ -105,7 +104,8 @@ bool InstanceImplWin32::directoryExists(const std::string& path) { } ssize_t InstanceImplWin32::fileSize(const std::string& path) { - auto fd = CreateFileA(path.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, NULL, NULL); + auto fd = + CreateFileA(path.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, NULL, NULL); if (fd == INVALID_HANDLE) { return -1; } @@ -113,7 +113,7 @@ ssize_t InstanceImplWin32::fileSize(const std::string& path) { LARGE_INTEGER lFileSize; BOOL bGetSize = GetFileSizeEx(fd, &lFileSize); CloseHandle(fd); - if(!bGetSize) { + if (!bGetSize) { return -1; } result += lFileSize.QuadPart; From e22ac3eaa32da79d7ce14dc1a0045ceb28fd4891 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Mon, 14 Sep 2020 16:57:42 -0700 Subject: [PATCH 03/10] PR comments and fix ci Signed-off-by: Sotiris Nanopoulos --- include/envoy/common/platform.h | 4 ++-- source/common/filesystem/win32/filesystem_impl.cc | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/envoy/common/platform.h b/include/envoy/common/platform.h index 47ef701216c1..012b73983362 100644 --- a/include/envoy/common/platform.h +++ b/include/envoy/common/platform.h @@ -61,7 +61,7 @@ typedef ptrdiff_t ssize_t; typedef uint32_t mode_t; typedef SOCKET os_fd_t; -typedef HANDLE os_h_t; +using os_h_t = HANDLE; typedef unsigned int sa_family_t; @@ -211,7 +211,7 @@ constexpr absl::string_view null_device_path{"NUL"}; #endif typedef int os_fd_t; -typedef int os_h_t; +using os_h_t = int; #define INVALID_HANDLE -1 #define INVALID_SOCKET -1 diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc index 0da4efc237d4..cfdb3098fe1e 100644 --- a/source/common/filesystem/win32/filesystem_impl.cc +++ b/source/common/filesystem/win32/filesystem_impl.cc @@ -32,7 +32,8 @@ Api::IoCallBoolResult FileImplWin32::open(FlagSet in) { } auto flags = translateFlag(in); - fd_ = CreateFileA(path_.c_str(), flags.access_, NULL, NULL, flags.creation_, NULL, NULL); + fd_ = CreateFileA(path_.c_str(), flags.access_, FILE_SHARE_READ | FILE_SHARE_WRITE, 0, + flags.creation_, 0, NULL); if (fd_ == INVALID_HANDLE) { return resultFailure(false, ::GetLastError()); } @@ -104,8 +105,7 @@ bool InstanceImplWin32::directoryExists(const std::string& path) { } ssize_t InstanceImplWin32::fileSize(const std::string& path) { - auto fd = - CreateFileA(path.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, NULL, NULL); + auto fd = CreateFileA(path.c_str(), GENERIC_READ, FILE_SHARE_READ, 0, OPEN_EXISTING, 0, NULL); if (fd == INVALID_HANDLE) { return -1; } From 6c7a120dc28a3a98c84f5e3e896690fc115242cf Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Wed, 16 Sep 2020 11:10:58 -0700 Subject: [PATCH 04/10] adds tests for iofileerror Signed-off-by: Sotiris Nanopoulos --- .../common/filesystem/filesystem_impl_test.cc | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index 1c00f39eec24..b0aed59a27fb 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -63,11 +63,7 @@ TEST_F(FileSystemImplTest, DirectoryExists) { } TEST_F(FileSystemImplTest, FileSize) { -#ifdef WIN32 - EXPECT_EQ(0, file_system_.fileSize("NUL")); -#else - EXPECT_EQ(0, file_system_.fileSize("/dev/null")); -#endif + EXPECT_EQ(0, file_system_.fileSize(std::string(Platform::null_device_path))); EXPECT_EQ(-1, file_system_.fileSize("/dev/blahblahblah")); const std::string data = "test string\ntest"; const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", data); @@ -355,5 +351,20 @@ TEST_F(FileSystemImplTest, ExistingReadOnlyFileAndWrite) { EXPECT_EQ("existing file", contents); } +TEST(FileSystemImplTest, TestIoFileError) { + IoFileError error1(HANDLE_ERROR_PERM); + EXPECT_EQ(IoFileError::IoErrorCode::Permission, error1.getErrorCode()); + EXPECT_EQ(errorDetails(HANDLE_ERROR_PERM), error1.getErrorDetails()); + + + IoFileError error2(HANDLE_ERROR_INVALID); + EXPECT_EQ(IoFileError::IoErrorCode::BadFd, error1.getErrorCode()); + EXPECT_EQ(errorDetails(HANDLE_ERROR_PERM), error1.getErrorDetails()); + + int not_known_error = 42; + IoFileError error3(not_known_error); + EXPECT_EQ(IoFileError::IoErrorCode::UnknownError, error1.getErrorCode()); +} + } // namespace Filesystem } // namespace Envoy From f27c3229fb12a85c508ff0113c352dc871ae7339 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Wed, 16 Sep 2020 11:21:49 -0700 Subject: [PATCH 05/10] format + cp errors Signed-off-by: Sotiris Nanopoulos --- include/envoy/common/platform.h | 4 ++-- test/common/filesystem/filesystem_impl_test.cc | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/include/envoy/common/platform.h b/include/envoy/common/platform.h index 012b73983362..47ef701216c1 100644 --- a/include/envoy/common/platform.h +++ b/include/envoy/common/platform.h @@ -61,7 +61,7 @@ typedef ptrdiff_t ssize_t; typedef uint32_t mode_t; typedef SOCKET os_fd_t; -using os_h_t = HANDLE; +typedef HANDLE os_h_t; typedef unsigned int sa_family_t; @@ -211,7 +211,7 @@ constexpr absl::string_view null_device_path{"NUL"}; #endif typedef int os_fd_t; -using os_h_t = int; +typedef int os_h_t; #define INVALID_HANDLE -1 #define INVALID_SOCKET -1 diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index b0aed59a27fb..0ae611d5a0a7 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -351,19 +351,18 @@ TEST_F(FileSystemImplTest, ExistingReadOnlyFileAndWrite) { EXPECT_EQ("existing file", contents); } -TEST(FileSystemImplTest, TestIoFileError) { +TEST_F(FileSystemImplTest, TestIoFileError) { IoFileError error1(HANDLE_ERROR_PERM); EXPECT_EQ(IoFileError::IoErrorCode::Permission, error1.getErrorCode()); EXPECT_EQ(errorDetails(HANDLE_ERROR_PERM), error1.getErrorDetails()); - IoFileError error2(HANDLE_ERROR_INVALID); - EXPECT_EQ(IoFileError::IoErrorCode::BadFd, error1.getErrorCode()); - EXPECT_EQ(errorDetails(HANDLE_ERROR_PERM), error1.getErrorDetails()); + EXPECT_EQ(IoFileError::IoErrorCode::BadFd, error2.getErrorCode()); + EXPECT_EQ(errorDetails(HANDLE_ERROR_INVALID), error2.getErrorDetails()); int not_known_error = 42; IoFileError error3(not_known_error); - EXPECT_EQ(IoFileError::IoErrorCode::UnknownError, error1.getErrorCode()); + EXPECT_EQ(IoFileError::IoErrorCode::UnknownError, error3.getErrorCode()); } } // namespace Filesystem From 7711884002f966a336b9854e2daaeb734a2decc9 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Wed, 16 Sep 2020 18:28:12 -0700 Subject: [PATCH 06/10] moves translate into private members Signed-off-by: Sotiris Nanopoulos --- source/common/filesystem/posix/filesystem_impl.h | 3 +-- source/common/filesystem/win32/filesystem_impl.h | 13 +++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/source/common/filesystem/posix/filesystem_impl.h b/source/common/filesystem/posix/filesystem_impl.h index 310653e21030..4f81255be222 100644 --- a/source/common/filesystem/posix/filesystem_impl.h +++ b/source/common/filesystem/posix/filesystem_impl.h @@ -25,9 +25,8 @@ class FileImplPosix : public FileSharedImpl { Api::IoCallSizeResult write(absl::string_view buffer) override; Api::IoCallBoolResult close() override; - FlagsAndMode translateFlag(FlagSet in); - private: + FlagsAndMode translateFlag(FlagSet in); friend class FileSystemImplTest; }; diff --git a/source/common/filesystem/win32/filesystem_impl.h b/source/common/filesystem/win32/filesystem_impl.h index 4fbfa6eaee96..2b603a2cca02 100644 --- a/source/common/filesystem/win32/filesystem_impl.h +++ b/source/common/filesystem/win32/filesystem_impl.h @@ -14,18 +14,19 @@ class FileImplWin32 : public FileSharedImpl { ~FileImplWin32(); protected: - struct FlagsAndMode { - DWORD access_ = 0; - DWORD creation_ = 0; - }; Api::IoCallBoolResult open(FlagSet flag) override; Api::IoCallSizeResult write(absl::string_view buffer) override; Api::IoCallBoolResult close() override; - FlagsAndMode translateFlag(FlagSet in); - private: + + struct FlagsAndMode { + DWORD access_ = 0; + DWORD creation_ = 0; + }; + + FlagsAndMode translateFlag(FlagSet in); friend class FileSystemImplTest; }; From 47371422958b974e7589af88e931c08f9822c8e1 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Thu, 17 Sep 2020 11:42:39 -0700 Subject: [PATCH 07/10] clang tidy + variable name Signed-off-by: Sotiris Nanopoulos --- include/envoy/common/platform.h | 4 ++-- source/common/filesystem/file_shared_impl.h | 2 +- test/common/filesystem/filesystem_impl_test.cc | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/envoy/common/platform.h b/include/envoy/common/platform.h index 47ef701216c1..d654026e4126 100644 --- a/include/envoy/common/platform.h +++ b/include/envoy/common/platform.h @@ -61,7 +61,7 @@ typedef ptrdiff_t ssize_t; typedef uint32_t mode_t; typedef SOCKET os_fd_t; -typedef HANDLE os_h_t; +typedef HANDLE filesystem_os_id_t; // NOLINT(modernize-use-using) typedef unsigned int sa_family_t; @@ -211,7 +211,7 @@ constexpr absl::string_view null_device_path{"NUL"}; #endif typedef int os_fd_t; -typedef int os_h_t; +typedef int filesystem_os_id_t; // NOLINT(modernize-use-using) #define INVALID_HANDLE -1 #define INVALID_SOCKET -1 diff --git a/source/common/filesystem/file_shared_impl.h b/source/common/filesystem/file_shared_impl.h index 1af0d1372296..4b8b623f7fdb 100644 --- a/source/common/filesystem/file_shared_impl.h +++ b/source/common/filesystem/file_shared_impl.h @@ -45,7 +45,7 @@ class FileSharedImpl : public File { std::string path() const override; protected: - os_h_t fd_; + filesystem_os_id_t fd_; const std::string path_; }; diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index 0ae611d5a0a7..b747b20018c1 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -19,7 +19,7 @@ static constexpr FlagSet DefaultFlags{ class FileSystemImplTest : public testing::Test { protected: - os_h_t getFd(File* file) { + filesystem_os_id_t getFd(File* file) { #ifdef WIN32 auto file_impl = dynamic_cast(file); #else @@ -239,7 +239,7 @@ TEST_F(FileSystemImplTest, OpenTwice) { EXPECT_EQ(getFd(file.get()), INVALID_HANDLE); const Api::IoCallBoolResult result1 = file->open(DefaultFlags); - const os_h_t initial_fd = getFd(file.get()); + const filesystem_os_id_t initial_fd = getFd(file.get()); EXPECT_TRUE(result1.rc_); EXPECT_TRUE(file->isOpen()); From 52a3cda83709ffb4cedb2eea995c7e2fd21e7de1 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Thu, 17 Sep 2020 12:47:30 -0700 Subject: [PATCH 08/10] attempt to increase coverage Signed-off-by: Sotiris Nanopoulos --- test/common/filesystem/filesystem_impl_test.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index b747b20018c1..e78cf91f739f 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -231,6 +231,18 @@ TEST_F(FileSystemImplTest, Open) { EXPECT_TRUE(file->isOpen()); } +TEST_F(FileSystemImplTest, OpenReadOnly) { + const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); + ::unlink(new_file_path.c_str()); + static constexpr FlagSet ReadOnlyFlags{1 << Filesystem::File::Operation::Read | + 1 << Filesystem::File::Operation::Create | + 1 << Filesystem::File::Operation::Append}; + FilePtr file = file_system_.createFile(new_file_path); + const Api::IoCallBoolResult result = file->open(ReadOnlyFlags); + EXPECT_TRUE(result.rc_); + EXPECT_TRUE(file->isOpen()); +} + TEST_F(FileSystemImplTest, OpenTwice) { const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); ::unlink(new_file_path.c_str()); From bcf28624a19e622cf1220c9215d966aed04d0725 Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Thu, 17 Sep 2020 13:41:35 -0700 Subject: [PATCH 09/10] fix format Signed-off-by: Sotiris Nanopoulos --- source/common/filesystem/win32/filesystem_impl.h | 2 -- test/common/filesystem/filesystem_impl_test.cc | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/source/common/filesystem/win32/filesystem_impl.h b/source/common/filesystem/win32/filesystem_impl.h index 2b603a2cca02..95989ceb9e0f 100644 --- a/source/common/filesystem/win32/filesystem_impl.h +++ b/source/common/filesystem/win32/filesystem_impl.h @@ -14,13 +14,11 @@ class FileImplWin32 : public FileSharedImpl { ~FileImplWin32(); protected: - Api::IoCallBoolResult open(FlagSet flag) override; Api::IoCallSizeResult write(absl::string_view buffer) override; Api::IoCallBoolResult close() override; private: - struct FlagsAndMode { DWORD access_ = 0; DWORD creation_ = 0; diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index e78cf91f739f..127451d3929e 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -235,8 +235,8 @@ TEST_F(FileSystemImplTest, OpenReadOnly) { const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); ::unlink(new_file_path.c_str()); static constexpr FlagSet ReadOnlyFlags{1 << Filesystem::File::Operation::Read | - 1 << Filesystem::File::Operation::Create | - 1 << Filesystem::File::Operation::Append}; + 1 << Filesystem::File::Operation::Create | + 1 << Filesystem::File::Operation::Append}; FilePtr file = file_system_.createFile(new_file_path); const Api::IoCallBoolResult result = file->open(ReadOnlyFlags); EXPECT_TRUE(result.rc_); From a933346033f21510599509c6210c333bf83ad8cd Mon Sep 17 00:00:00 2001 From: Sotiris Nanopoulos Date: Thu, 17 Sep 2020 17:54:25 -0700 Subject: [PATCH 10/10] trigger ci Signed-off-by: Sotiris Nanopoulos