Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions envoy/api/os_sys_calls.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,11 @@ class OsSysCalls {
*/
virtual SysCallIntResult stat(const char* pathname, struct stat* buf) PURE;

/**
* @see man 2 fstat
*/
virtual SysCallIntResult fstat(os_fd_t fd, struct stat* buf) PURE;

/**
* @see man 2 setsockopt
*/
Expand Down
5 changes: 5 additions & 0 deletions source/common/api/posix/os_sys_calls_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ SysCallIntResult OsSysCallsImpl::stat(const char* pathname, struct stat* buf) {
return {rc, rc != -1 ? 0 : errno};
}

SysCallIntResult OsSysCallsImpl::fstat(os_fd_t fd, struct stat* buf) {
const int rc = ::fstat(fd, buf);
return {rc, rc != -1 ? 0 : errno};
}

SysCallIntResult OsSysCallsImpl::setsockopt(os_fd_t sockfd, int level, int optname,
const void* optval, socklen_t optlen) {
const int rc = ::setsockopt(sockfd, level, optname, optval, optlen);
Expand Down
1 change: 1 addition & 0 deletions source/common/api/posix/os_sys_calls_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class OsSysCallsImpl : public OsSysCalls {
SysCallPtrResult mmap(void* addr, size_t length, int prot, int flags, int fd,
off_t offset) override;
SysCallIntResult stat(const char* pathname, struct stat* buf) override;
SysCallIntResult fstat(os_fd_t fd, struct stat* buf) override;
SysCallIntResult setsockopt(os_fd_t sockfd, int level, int optname, const void* optval,
socklen_t optlen) override;
SysCallIntResult getsockopt(os_fd_t sockfd, int level, int optname, void* optval,
Expand Down
15 changes: 13 additions & 2 deletions source/common/api/win32/os_sys_calls_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,14 @@ SysCallIntResult OsSysCallsImpl::ioctl(os_fd_t sockfd, unsigned long control_cod
}

SysCallIntResult OsSysCallsImpl::close(os_fd_t fd) {
const int rc = ::closesocket(fd);
return {rc, rc != -1 ? 0 : ::WSAGetLastError()};
int rc = ::closesocket(fd);
const int socket_err = ::WSAGetLastError();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't appear to be called out in the release note. is it tested in the new test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in Slack; in existing code close is only called where closesocket behavior is fine. My hope is to later change this back and make it explicitly closesocket (for all platforms), and remove everything I've done here after implementing [fstat, pread, pwrite, tmpfiles] in Envoy::Filesystem::File and migrating AsyncFileManager to using that. But getting it in here allows me to make progress elsewhere while that chain of changes is in flight.

if (rc == -1) {
// If `closesocket` failed, maybe the descriptor was a file.
rc = ::close(fd);
}
// Assume the descriptor was a socket, and return the error from that one, if both failed.
return {rc, rc != -1 ? 0 : socket_err};
}

SysCallSizeResult OsSysCallsImpl::writev(os_fd_t fd, const iovec* iov, int num_iov) {
Expand Down Expand Up @@ -226,6 +232,11 @@ SysCallIntResult OsSysCallsImpl::stat(const char* pathname, struct stat* buf) {
return {rc, rc != -1 ? 0 : errno};
}

SysCallIntResult OsSysCallsImpl::fstat(os_fd_t fd, struct stat* buf) {
const int rc = ::fstat(fd, buf);
return {rc, rc != -1 ? 0 : errno};
}

SysCallIntResult OsSysCallsImpl::setsockopt(os_fd_t sockfd, int level, int optname,
const void* optval, socklen_t optlen) {
const int rc = ::setsockopt(sockfd, level, optname, static_cast<const char*>(optval), optlen);
Expand Down
1 change: 1 addition & 0 deletions source/common/api/win32/os_sys_calls_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class OsSysCallsImpl : public OsSysCalls {
SysCallPtrResult mmap(void* addr, size_t length, int prot, int flags, int fd,
off_t offset) override;
SysCallIntResult stat(const char* pathname, struct stat* buf) override;
SysCallIntResult fstat(os_fd_t fd, struct stat* buf) override;
SysCallIntResult setsockopt(os_fd_t sockfd, int level, int optname, const void* optval,
socklen_t optlen) override;
SysCallIntResult getsockopt(os_fd_t sockfd, int level, int optname, void* optval,
Expand Down
64 changes: 64 additions & 0 deletions test/common/api/os_sys_calls_test.cc
Original file line number Diff line number Diff line change
@@ -1,9 +1,73 @@
#ifdef WIN32
#include <fcntl.h> // Needed for `O_CREAT` and `O_RDWR` on Win32.

#endif

#include "source/common/api/os_sys_calls_impl.h"

#include "test/test_common/environment.h"

#include "gtest/gtest.h"

namespace Envoy {

// Test happy path for `open`, `pwrite`, `pread`, `fstat`, `close`, `stat` and `unlink`.
TEST(OsSyscallsTest, OpenPwritePreadFstatCloseStatUnlink) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh I really want to break these out but you can't really fully test read without write/open, ditto fstat. so yeah can leave as is but if fully testing close requires tweaking anything let's at least add line breaks between the different sections. if close is tested we can merge as-is

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close is [kind of] tested, in that this new test tests the new path, and existing tests implicitly cover the old path. But it turns out Windows doesn't like to unlink a file that was created without explicit windows-specific permissions flags, so that shot the auto-merge. :(

auto& os_syscalls = Api::OsSysCallsSingleton::get();
std::string path{TestEnvironment::temporaryPath("envoy_test")};
TestEnvironment::createPath(path);
std::string file_path = path + "/file";
absl::string_view file_contents = "12345";
// Test `open`
#ifdef WIN32
int pmode = _S_IWRITE | _S_IREAD;
#else
int pmode = S_IRUSR | S_IWUSR;
#endif
Api::SysCallIntResult open_result = os_syscalls.open(file_path.c_str(), O_CREAT | O_RDWR, pmode);
EXPECT_NE(open_result.return_value_, -1);
EXPECT_EQ(open_result.errno_, 0);
os_fd_t fd = open_result.return_value_;
#ifdef WIN32
// `pwrite` and `pread` are not supported. Just write some bytes so we can still test stat.
EXPECT_EQ(file_contents.size(), ::_write(fd, file_contents.begin(), file_contents.size()));
#else
// Test `pwrite`
Api::SysCallSizeResult write_result =
os_syscalls.pwrite(fd, file_contents.begin(), file_contents.size(), 0);
EXPECT_EQ(write_result.return_value_, file_contents.size());
EXPECT_EQ(write_result.errno_, 0);
// Test `pread`
char read_buffer[5];
Api::SysCallSizeResult read_result = os_syscalls.pread(fd, read_buffer, sizeof(read_buffer), 0);
EXPECT_EQ(read_result.return_value_, sizeof(read_buffer));
EXPECT_EQ(read_result.errno_, 0);
absl::string_view read_buffer_view{read_buffer, sizeof(read_buffer)};
EXPECT_EQ(file_contents, read_buffer_view);
#endif
// Test `fstat`
struct stat fstat_value;
Api::SysCallIntResult fstat_result = os_syscalls.fstat(fd, &fstat_value);
EXPECT_EQ(fstat_result.return_value_, 0);
EXPECT_EQ(fstat_result.errno_, 0);
EXPECT_EQ(fstat_value.st_size, file_contents.size());
// Test `close`
Api::SysCallIntResult close_result = os_syscalls.close(fd);
EXPECT_EQ(close_result.return_value_, 0);
EXPECT_EQ(close_result.errno_, 0);
// Test `stat`
struct stat stat_value;
Api::SysCallIntResult stat_result = os_syscalls.stat(file_path.c_str(), &stat_value);
EXPECT_EQ(stat_result.return_value_, 0);
EXPECT_EQ(stat_result.errno_, 0);
EXPECT_EQ(stat_value.st_size, file_contents.size());
// Test `unlink`
Api::SysCallIntResult unlink_result = os_syscalls.unlink(file_path.c_str());
EXPECT_EQ(unlink_result.return_value_, 0);
EXPECT_EQ(unlink_result.errno_, 0);
TestEnvironment::removePath(path);
}

TEST(OsSyscallsTest, SetAlternateGetifaddrs) {
auto& os_syscalls = Api::OsSysCallsSingleton::get();
const bool pre_alternate_support = os_syscalls.supportsGetifaddrs();
Expand Down
1 change: 1 addition & 0 deletions test/mocks/api/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ class MockOsSysCalls : public OsSysCallsImpl {
MOCK_METHOD(SysCallPtrResult, mmap,
(void* addr, size_t length, int prot, int flags, int fd, off_t offset));
MOCK_METHOD(SysCallIntResult, stat, (const char* name, struct stat* stat));
MOCK_METHOD(SysCallIntResult, fstat, (os_fd_t fd, struct stat* stat));
MOCK_METHOD(SysCallIntResult, chmod, (const std::string& name, mode_t mode));
MOCK_METHOD(int, setsockopt_,
(os_fd_t sockfd, int level, int optname, const void* optval, socklen_t optlen));
Expand Down