From f5cab385c9ac5b8845ca490f8442607f5b6faa05 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Fri, 27 Jul 2018 15:05:39 -0700 Subject: [PATCH] syscall: use Api::SysCallResult in buffer impl This commit replaces std::tuple with Api::SysCallResult in the buffer API and implementations. Signed-off-by: Venil Noronha --- include/envoy/buffer/BUILD | 1 + include/envoy/buffer/buffer.h | 16 +++++----- source/common/buffer/buffer_impl.cc | 14 ++++----- source/common/buffer/buffer_impl.h | 4 +-- source/common/buffer/watermark_buffer.cc | 8 ++--- source/common/buffer/watermark_buffer.h | 4 +-- source/common/network/raw_buffer_socket.cc | 35 +++++++++------------ test/common/buffer/owned_impl_test.cc | 22 ++++++------- test/common/buffer/watermark_buffer_test.cc | 13 ++++---- test/common/network/connection_impl_test.cc | 4 +-- test/mocks/buffer/mocks.h | 13 ++++---- 11 files changed, 63 insertions(+), 71 deletions(-) diff --git a/include/envoy/buffer/BUILD b/include/envoy/buffer/BUILD index 084ed91ebf24f..01dcb26234196 100644 --- a/include/envoy/buffer/BUILD +++ b/include/envoy/buffer/BUILD @@ -11,4 +11,5 @@ envoy_package() envoy_cc_library( name = "buffer_interface", hdrs = ["buffer.h"], + deps = ["//include/envoy/api:os_sys_calls_interface"], ) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index eb4d3fc0f400a..cfe8611e0848a 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -4,8 +4,8 @@ #include #include #include -#include +#include "envoy/api/os_sys_calls.h" #include "envoy/common/pure.h" namespace Envoy { @@ -143,11 +143,10 @@ class Instance { * Read from a file descriptor directly into the buffer. * @param fd supplies the descriptor to read from. * @param max_length supplies the maximum length to read. - * @return a tuple with the number of bytes read and the errno. If an error occurred, the - * number of bytes read would indicate -1 and the errno would be non-zero. Otherwise, if - * bytes were read, errno shouldn't be used. + * @return a Api::SysCallResult with rc_ = the number of bytes read if successful, or rc_ = -1 + * for failure. If the call is successful, errno_ shouldn't be used. */ - virtual std::tuple read(int fd, uint64_t max_length) PURE; + virtual Api::SysCallResult read(int fd, uint64_t max_length) PURE; /** * Reserve space in the buffer. @@ -176,11 +175,10 @@ class Instance { /** * Write the buffer out to a file descriptor. * @param fd supplies the descriptor to write to. - * @return a tuple with the number of bytes written and the errno. If an error occurred, the - * number of bytes written would indicate -1 and the errno would be non-zero. Otherwise, if - * bytes were written, errno shouldn't be used. + * @return a Api::SysCallResult with rc_ = the number of bytes written if successful, or rc_ = -1 + * for failure. If the call is successful, errno_ shouldn't be used. */ - virtual std::tuple write(int fd) PURE; + virtual Api::SysCallResult write(int fd) PURE; }; typedef std::unique_ptr InstancePtr; diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 3e70b1d70b889..888d87077376f 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -94,9 +94,9 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { static_cast(rhs).postProcess(); } -std::tuple OwnedImpl::read(int fd, uint64_t max_length) { +Api::SysCallResult OwnedImpl::read(int fd, uint64_t max_length) { if (max_length == 0) { - return std::make_tuple(0, 0); + return {0, 0}; } constexpr uint64_t MaxSlices = 2; RawSlice slices[MaxSlices]; @@ -117,7 +117,7 @@ std::tuple OwnedImpl::read(int fd, uint64_t max_length) { const ssize_t rc = os_syscalls.readv(fd, iov, static_cast(num_slices_to_read)); const int error = errno; if (rc < 0) { - return std::make_tuple(rc, error); + return {static_cast(rc), error}; } uint64_t num_slices_to_commit = 0; uint64_t bytes_to_commit = rc; @@ -131,7 +131,7 @@ std::tuple OwnedImpl::read(int fd, uint64_t max_length) { } ASSERT(num_slices_to_commit <= num_slices); commit(slices, num_slices_to_commit); - return std::make_tuple(rc, error); + return {static_cast(rc), error}; } uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { @@ -152,7 +152,7 @@ ssize_t OwnedImpl::search(const void* data, uint64_t size, size_t start) const { return result_ptr.pos; } -std::tuple OwnedImpl::write(int fd) { +Api::SysCallResult OwnedImpl::write(int fd) { constexpr uint64_t MaxSlices = 16; RawSlice slices[MaxSlices]; const uint64_t num_slices = std::min(getRawSlices(slices, MaxSlices), MaxSlices); @@ -166,7 +166,7 @@ std::tuple OwnedImpl::write(int fd) { } } if (num_slices_to_write == 0) { - return std::make_tuple(0, 0); + return {0, 0}; } auto& os_syscalls = Api::OsSysCallsSingleton::get(); const ssize_t rc = os_syscalls.writev(fd, iov, num_slices_to_write); @@ -174,7 +174,7 @@ std::tuple OwnedImpl::write(int fd) { if (rc > 0) { drain(static_cast(rc)); } - return std::make_tuple(static_cast(rc), error); + return {static_cast(rc), error}; } OwnedImpl::OwnedImpl() : buffer_(evbuffer_new()) {} diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index e4adc74d0f66d..993f4990405d8 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -80,10 +80,10 @@ class OwnedImpl : public LibEventInstance { void* linearize(uint32_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; - std::tuple read(int fd, uint64_t max_length) override; + Api::SysCallResult read(int fd, uint64_t max_length) override; uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; ssize_t search(const void* data, uint64_t size, size_t start) const override; - std::tuple write(int fd) override; + Api::SysCallResult write(int fd) override; void postProcess() override {} std::string toString() const override; diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index f09f9fb3cc944..fe2c1981e54f0 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -40,8 +40,8 @@ void WatermarkBuffer::move(Instance& rhs, uint64_t length) { checkHighWatermark(); } -std::tuple WatermarkBuffer::read(int fd, uint64_t max_length) { - std::tuple result = OwnedImpl::read(fd, max_length); +Api::SysCallResult WatermarkBuffer::read(int fd, uint64_t max_length) { + Api::SysCallResult result = OwnedImpl::read(fd, max_length); checkHighWatermark(); return result; } @@ -52,8 +52,8 @@ uint64_t WatermarkBuffer::reserve(uint64_t length, RawSlice* iovecs, uint64_t nu return bytes_reserved; } -std::tuple WatermarkBuffer::write(int fd) { - std::tuple result = OwnedImpl::write(fd); +Api::SysCallResult WatermarkBuffer::write(int fd) { + Api::SysCallResult result = OwnedImpl::write(fd); checkLowWatermark(); return result; } diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index 9524530c23b97..fb74ccde04f4c 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -28,9 +28,9 @@ class WatermarkBuffer : public OwnedImpl { void drain(uint64_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; - std::tuple read(int fd, uint64_t max_length) override; + Api::SysCallResult read(int fd, uint64_t max_length) override; uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; - std::tuple write(int fd) override; + Api::SysCallResult write(int fd) override; void postProcess() override { checkLowWatermark(); } void setWatermarks(uint32_t watermark) { setWatermarks(watermark / 2, watermark); } diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc index 0ab70d60ea6c5..699313042386e 100644 --- a/source/common/network/raw_buffer_socket.cc +++ b/source/common/network/raw_buffer_socket.cc @@ -17,25 +17,22 @@ IoResult RawBufferSocket::doRead(Buffer::Instance& buffer) { bool end_stream = false; do { // 16K read is arbitrary. TODO(mattklein123) PERF: Tune the read size. - std::tuple result = buffer.read(callbacks_->fd(), 16384); - const int rc = std::get<0>(result); - const int error = std::get<1>(result); - ENVOY_CONN_LOG(trace, "read returns: {}", callbacks_->connection(), rc); + Api::SysCallResult result = buffer.read(callbacks_->fd(), 16384); + ENVOY_CONN_LOG(trace, "read returns: {}", callbacks_->connection(), result.rc_); - if (rc == 0) { + if (result.rc_ == 0) { // Remote close. end_stream = true; break; - } else if (rc == -1) { + } else if (result.rc_ == -1) { // Remote error (might be no data). - ENVOY_CONN_LOG(trace, "read error: {}", callbacks_->connection(), error); - if (error != EAGAIN) { + ENVOY_CONN_LOG(trace, "read error: {}", callbacks_->connection(), result.errno_); + if (result.errno_ != EAGAIN) { action = PostIoAction::Close; } - break; } else { - bytes_read += rc; + bytes_read += result.rc_; if (callbacks_->shouldDrainReadBuffer()) { callbacks_->setReadBufferReady(); break; @@ -61,22 +58,20 @@ IoResult RawBufferSocket::doWrite(Buffer::Instance& buffer, bool end_stream) { action = PostIoAction::KeepOpen; break; } - std::tuple result = buffer.write(callbacks_->fd()); - const int rc = std::get<0>(result); - const int error = std::get<1>(result); - ENVOY_CONN_LOG(trace, "write returns: {}", callbacks_->connection(), rc); - if (rc == -1) { - ENVOY_CONN_LOG(trace, "write error: {} ({})", callbacks_->connection(), error, - strerror(error)); - if (error == EAGAIN) { + Api::SysCallResult result = buffer.write(callbacks_->fd()); + ENVOY_CONN_LOG(trace, "write returns: {}", callbacks_->connection(), result.rc_); + + if (result.rc_ == -1) { + ENVOY_CONN_LOG(trace, "write error: {} ({})", callbacks_->connection(), result.errno_, + strerror(result.errno_)); + if (result.errno_ == EAGAIN) { action = PostIoAction::KeepOpen; } else { action = PostIoAction::Close; } - break; } else { - bytes_written += rc; + bytes_written += result.rc_; } } while (true); diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index b48dc4bb01a10..9d02fc1c1c099 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -83,34 +83,34 @@ TEST_F(OwnedImplTest, Write) { Buffer::OwnedImpl buffer; buffer.add("example"); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(7)); - std::tuple result = buffer.write(-1); - EXPECT_EQ(7, std::get<0>(result)); + Api::SysCallResult result = buffer.write(-1); + EXPECT_EQ(7, result.rc_); EXPECT_EQ(0, buffer.length()); buffer.add("example"); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(6)); result = buffer.write(-1); - EXPECT_EQ(6, std::get<0>(result)); + EXPECT_EQ(6, result.rc_); EXPECT_EQ(1, buffer.length()); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(0)); result = buffer.write(-1); - EXPECT_EQ(0, std::get<0>(result)); + EXPECT_EQ(0, result.rc_); EXPECT_EQ(1, buffer.length()); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(-1)); result = buffer.write(-1); - EXPECT_EQ(-1, std::get<0>(result)); + EXPECT_EQ(-1, result.rc_); EXPECT_EQ(1, buffer.length()); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(1)); result = buffer.write(-1); - EXPECT_EQ(1, std::get<0>(result)); + EXPECT_EQ(1, result.rc_); EXPECT_EQ(0, buffer.length()); EXPECT_CALL(os_sys_calls, writev(_, _, _)).Times(0); result = buffer.write(-1); - EXPECT_EQ(0, std::get<0>(result)); + EXPECT_EQ(0, result.rc_); EXPECT_EQ(0, buffer.length()); } @@ -120,18 +120,18 @@ TEST_F(OwnedImplTest, Read) { Buffer::OwnedImpl buffer; EXPECT_CALL(os_sys_calls, readv(_, _, _)).WillOnce(Return(0)); - std::tuple result = buffer.read(-1, 100); - EXPECT_EQ(0, std::get<0>(result)); + Api::SysCallResult result = buffer.read(-1, 100); + EXPECT_EQ(0, result.rc_); EXPECT_EQ(0, buffer.length()); EXPECT_CALL(os_sys_calls, readv(_, _, _)).WillOnce(Return(-1)); result = buffer.read(-1, 100); - EXPECT_EQ(-1, std::get<0>(result)); + EXPECT_EQ(-1, result.rc_); EXPECT_EQ(0, buffer.length()); EXPECT_CALL(os_sys_calls, readv(_, _, _)).Times(0); result = buffer.read(-1, 0); - EXPECT_EQ(0, std::get<0>(result)); + EXPECT_EQ(0, result.rc_); EXPECT_EQ(0, buffer.length()); } diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc index 4fe667ee5e3de..b70f423889fae 100644 --- a/test/common/buffer/watermark_buffer_test.cc +++ b/test/common/buffer/watermark_buffer_test.cc @@ -131,12 +131,11 @@ TEST_F(WatermarkBufferTest, WatermarkFdFunctions) { int bytes_written_total = 0; while (bytes_written_total < 20) { - std::tuple result = buffer_.write(pipe_fds[1]); - int bytes_written = std::get<0>(result); - if (bytes_written < 0) { - ASSERT_EQ(EAGAIN, errno); + Api::SysCallResult result = buffer_.write(pipe_fds[1]); + if (result.rc_ < 0) { + ASSERT_EQ(EAGAIN, result.errno_); } else { - bytes_written_total += bytes_written; + bytes_written_total += result.rc_; } } EXPECT_EQ(1, times_high_watermark_called_); @@ -145,8 +144,8 @@ TEST_F(WatermarkBufferTest, WatermarkFdFunctions) { int bytes_read_total = 0; while (bytes_read_total < 20) { - std::tuple result = buffer_.read(pipe_fds[0], 20); - bytes_read_total += std::get<0>(result); + Api::SysCallResult result = buffer_.read(pipe_fds[0], 20); + bytes_read_total += result.rc_; } EXPECT_EQ(2, times_high_watermark_called_); EXPECT_EQ(20, buffer_.length()); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 1258534038c28..a7c4f022a3ea2 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -678,7 +678,7 @@ TEST_P(ConnectionImplTest, WriteWithWatermarks) { EXPECT_CALL(*client_write_buffer_, move(_)) .WillRepeatedly(DoAll(AddBufferToStringWithoutDraining(&data_written), Invoke(client_write_buffer_, &MockWatermarkBuffer::baseMove))); - EXPECT_CALL(*client_write_buffer_, write(_)).WillOnce(Invoke([&](int fd) -> std::tuple { + EXPECT_CALL(*client_write_buffer_, write(_)).WillOnce(Invoke([&](int fd) -> Api::SysCallResult { dispatcher_->exit(); return client_write_buffer_->failWrite(fd); })); @@ -764,7 +764,7 @@ TEST_P(ConnectionImplTest, WatermarkFuzzing) { .WillOnce(Invoke(client_write_buffer_, &MockWatermarkBuffer::baseMove)); EXPECT_CALL(*client_write_buffer_, write(_)) .WillOnce(DoAll(Invoke([&](int) -> void { client_write_buffer_->drain(bytes_to_flush); }), - Return(std::make_tuple(bytes_to_flush, 0)))) + Return(Api::SysCallResult{bytes_to_flush, 0}))) .WillRepeatedly(testing::Invoke(client_write_buffer_, &MockWatermarkBuffer::failWrite)); client_connection_->write(buffer_to_write, false); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); diff --git a/test/mocks/buffer/mocks.h b/test/mocks/buffer/mocks.h index 2be1903c621ca..59cdae2902ec9 100644 --- a/test/mocks/buffer/mocks.h +++ b/test/mocks/buffer/mocks.h @@ -16,7 +16,7 @@ template class MockBufferBase : public BaseClass { MockBufferBase(); MockBufferBase(std::function below_low, std::function above_high); - MOCK_METHOD1(write, std::tuple(int fd)); + MOCK_METHOD1(write, Api::SysCallResult(int fd)); MOCK_METHOD1(move, void(Buffer::Instance& rhs)); MOCK_METHOD2(move, void(Buffer::Instance& rhs, uint64_t length)); MOCK_METHOD1(drain, void(uint64_t size)); @@ -24,11 +24,10 @@ template class MockBufferBase : public BaseClass { void baseMove(Buffer::Instance& rhs) { BaseClass::move(rhs); } void baseDrain(uint64_t size) { BaseClass::drain(size); } - std::tuple trackWrites(int fd) { - std::tuple result = BaseClass::write(fd); - int bytes_written = std::get<0>(result); - if (bytes_written > 0) { - bytes_written_ += bytes_written; + Api::SysCallResult trackWrites(int fd) { + Api::SysCallResult result = BaseClass::write(fd); + if (result.rc_ > 0) { + bytes_written_ += result.rc_; } return result; } @@ -39,7 +38,7 @@ template class MockBufferBase : public BaseClass { } // A convenience function to invoke on write() which fails the write with EAGAIN. - std::tuple failWrite(int) { return std::make_tuple(-1, EAGAIN); } + Api::SysCallResult failWrite(int) { return {-1, EAGAIN}; } int bytes_written() const { return bytes_written_; } uint64_t bytes_drained() const { return bytes_drained_; }