From ae42dc18d7ddcc2af2da74d22de0e44cdf4cb3d2 Mon Sep 17 00:00:00 2001 From: Yael Harel Date: Wed, 27 Feb 2019 15:11:48 -0500 Subject: [PATCH 1/3] Move IoError interface from Network to Api namespace Signed-off-by: Yael Harel Signed-off-by: Sam Smith --- include/envoy/api/BUILD | 5 ++ include/envoy/api/io_error.h | 87 +++++++++++++++++++ include/envoy/network/BUILD | 5 +- include/envoy/network/io_handle.h | 74 +--------------- .../common/network/io_socket_handle_impl.cc | 8 +- source/common/network/io_socket_handle_impl.h | 4 +- .../network/io_socket_handle_impl_test.cc | 24 ++--- 7 files changed, 116 insertions(+), 91 deletions(-) create mode 100644 include/envoy/api/io_error.h diff --git a/include/envoy/api/BUILD b/include/envoy/api/BUILD index 9b902d753c035..265e047ec9789 100644 --- a/include/envoy/api/BUILD +++ b/include/envoy/api/BUILD @@ -18,6 +18,11 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "io_error_interface", + hdrs = ["io_error.h"], +) + envoy_cc_library( name = "os_sys_calls_interface", hdrs = ["os_sys_calls.h"], diff --git a/include/envoy/api/io_error.h b/include/envoy/api/io_error.h new file mode 100644 index 0000000000000..f4954b46dd730 --- /dev/null +++ b/include/envoy/api/io_error.h @@ -0,0 +1,87 @@ +#pragma once + +#include +#include + +#include "envoy/common/platform.h" +#include "envoy/common/pure.h" + +namespace Envoy { +namespace Api { + +class IoError; + +// IoErrorCode::Again is used frequently. Define it to be a distinguishable address to avoid +// frequent memory allocation of IoError instance. +// If this is used, IoCallResult has to be instantiated with a deleter that does not +// deallocate memory for this error. +#define ENVOY_ERROR_AGAIN reinterpret_cast(0x01) + +/** + * Base class for any I/O error. + */ +class IoError { +public: + enum class IoErrorCode { + // No data available right now, try again later. + Again, + // Not supported. + NoSupport, + // Address family not supported. + AddressFamilyNoSupport, + // During non-blocking connect, the connection cannot be completed immediately. + InProgress, + // Permission denied. + Permission, + // Bad handle + BadHandle, + // Other error codes cannot be mapped to any one above in getErrorCode(). + UnknownError + }; + virtual ~IoError() {} + + // Map platform specific error into IoErrorCode. + // Needed to hide errorCode() in case of ENVOY_ERROR_AGAIN. + static IoErrorCode getErrorCode(const IoError& err) { + if (&err == ENVOY_ERROR_AGAIN) { + return IoErrorCode::Again; + } + return err.errorCode(); + } + + static std::string getErrorDetails(const IoError& err) { + if (&err == ENVOY_ERROR_AGAIN) { + return "Try again later"; + } + return err.errorDetails(); + } + +protected: + virtual IoErrorCode errorCode() const PURE; + virtual std::string errorDetails() const PURE; +}; + +using IoErrorDeleterType = void (*)(IoError*); +using IoErrorPtr = std::unique_ptr; + +/** + * Basic type for return result which has a return code and error code defined + * according to different implementations. + */ +template struct IoCallResult { + IoCallResult(T rc, IoErrorPtr err) : rc_(rc), err_(std::move(err)) {} + + IoCallResult(IoCallResult&& result) : rc_(result.rc_), err_(std::move(result.err_)) {} + + virtual ~IoCallResult() {} + + T rc_; + IoErrorPtr err_; +}; + +using IoCallBoolResult = IoCallResult; +using IoCallSizeResult = IoCallResult; +using IoCallUintResult = IoCallResult; + +} // namespace Api +} // namespace Envoy diff --git a/include/envoy/network/BUILD b/include/envoy/network/BUILD index a264165b55927..2c1a20f5e90c6 100644 --- a/include/envoy/network/BUILD +++ b/include/envoy/network/BUILD @@ -67,7 +67,10 @@ envoy_cc_library( envoy_cc_library( name = "io_handle_interface", hdrs = ["io_handle.h"], - deps = ["//source/common/common:assert_lib"], + deps = [ + "//include/envoy/api:io_error_interface", + "//source/common/common:assert_lib", + ], ) envoy_cc_library( diff --git a/include/envoy/network/io_handle.h b/include/envoy/network/io_handle.h index d91d6b2c68142..48b0441f48aec 100644 --- a/include/envoy/network/io_handle.h +++ b/include/envoy/network/io_handle.h @@ -2,6 +2,7 @@ #include +#include "envoy/api/io_error.h" #include "envoy/common/pure.h" #include "common/common/assert.h" @@ -9,77 +10,6 @@ namespace Envoy { namespace Network { -class IoError; - -// IoErrorCode::Again is used frequently. Define it to be a distinguishable address to avoid -// frequent memory allocation of IoError instance. -// If this is used, IoHandleCallResult has to be instantiated with a deleter that does not -// deallocate memory for this error. -#define ENVOY_ERROR_AGAIN reinterpret_cast(0x01) - -/** - * Base class for any I/O error. - */ -class IoError { -public: - enum class IoErrorCode { - // No data available right now, try again later. - Again, - // Not supported. - NoSupport, - // Address family not supported. - AddressFamilyNoSupport, - // During non-blocking connect, the connection cannot be completed immediately. - InProgress, - // Permission denied. - Permission, - // Other error codes cannot be mapped to any one above in getErrorCode(). - UnknownError - }; - virtual ~IoError() {} - - // Map platform specific error into IoErrorCode. - // Needed to hide errorCode() in case of ENVOY_ERROR_AGAIN. - static IoErrorCode getErrorCode(const IoError& err) { - if (&err == ENVOY_ERROR_AGAIN) { - return IoErrorCode::Again; - } - return err.errorCode(); - } - - static std::string getErrorDetails(const IoError& err) { - if (&err == ENVOY_ERROR_AGAIN) { - return "Try again later"; - } - return err.errorDetails(); - } - -protected: - virtual IoErrorCode errorCode() const PURE; - virtual std::string errorDetails() const PURE; -}; - -using IoErrorDeleterType = void (*)(IoError*); -using IoErrorPtr = std::unique_ptr; - -/** - * Basic type for return result which has a return code and error code defined - * according to different implementations. - */ -template struct IoHandleCallResult { - IoHandleCallResult(T rc, IoErrorPtr err) : rc_(rc), err_(std::move(err)) {} - - IoHandleCallResult(IoHandleCallResult&& result) - : rc_(result.rc_), err_(std::move(result.err_)) {} - - virtual ~IoHandleCallResult() {} - - T rc_; - IoErrorPtr err_; -}; - -using IoHandleCallUintResult = IoHandleCallResult; - /** * IoHandle: an abstract interface for all I/O operations */ @@ -98,7 +28,7 @@ class IoHandle { /** * Clean up IoHandle resources */ - virtual IoHandleCallUintResult close() PURE; + virtual Api::IoCallUintResult close() PURE; /** * Return true if close() hasn't been called. diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 8e97c10970c52..8a9cdee3c84d9 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -12,7 +12,7 @@ using Envoy::Api::SysCallSizeResult; namespace Envoy { namespace Network { -IoError::IoErrorCode IoSocketError::errorCode() const { +Api::IoError::IoErrorCode IoSocketError::errorCode() const { switch (errno_) { case EAGAIN: // EAGAIN should use specific error ENVOY_ERROR_AGAIN. @@ -39,18 +39,18 @@ IoSocketHandleImpl::~IoSocketHandleImpl() { } // Deallocate memory only if the error is not ENVOY_ERROR_AGAIN. -void deleteIoError(IoError* err) { +void deleteIoError(Api::IoError* err) { ASSERT(err != nullptr); if (err != ENVOY_ERROR_AGAIN) { delete err; } } -IoHandleCallUintResult IoSocketHandleImpl::close() { +Api::IoCallUintResult IoSocketHandleImpl::close() { ASSERT(fd_ != -1); const int rc = ::close(fd_); fd_ = -1; - return IoHandleCallResult(rc, IoErrorPtr(nullptr, deleteIoError)); + return Api::IoCallResult(rc, Api::IoErrorPtr(nullptr, deleteIoError)); } bool IoSocketHandleImpl::isOpen() const { return fd_ != -1; } diff --git a/source/common/network/io_socket_handle_impl.h b/source/common/network/io_socket_handle_impl.h index 8f8f57e3ff7a3..2547e3817b6c6 100644 --- a/source/common/network/io_socket_handle_impl.h +++ b/source/common/network/io_socket_handle_impl.h @@ -6,7 +6,7 @@ namespace Envoy { namespace Network { -class IoSocketError : public IoError { +class IoSocketError : public Api::IoError { public: explicit IoSocketError(int sys_errno) : errno_(sys_errno) {} @@ -33,7 +33,7 @@ class IoSocketHandleImpl : public IoHandle { // TODO(sbelair2) To be removed when the fd is fully abstracted from clients. int fd() const override { return fd_; } - IoHandleCallUintResult close() override; + Api::IoCallUintResult close() override; bool isOpen() const override; diff --git a/test/common/network/io_socket_handle_impl_test.cc b/test/common/network/io_socket_handle_impl_test.cc index 3035550c05211..0b3e1672c457b 100644 --- a/test/common/network/io_socket_handle_impl_test.cc +++ b/test/common/network/io_socket_handle_impl_test.cc @@ -8,30 +8,30 @@ namespace { TEST(IoSocketHandleImplTest, TestIoSocketError) { IoSocketError error1(EAGAIN); - EXPECT_DEATH(IoError::getErrorCode(error1), ""); + EXPECT_DEATH(Api::IoError::getErrorCode(error1), ""); - EXPECT_EQ("Try again later", IoError::getErrorDetails(*ENVOY_ERROR_AGAIN)); + EXPECT_EQ("Try again later", Api::IoError::getErrorDetails(*ENVOY_ERROR_AGAIN)); IoSocketError error3(ENOTSUP); - EXPECT_EQ(IoSocketError::IoErrorCode::NoSupport, IoError::getErrorCode(error3)); - EXPECT_EQ(::strerror(ENOTSUP), IoError::getErrorDetails(error3)); + EXPECT_EQ(IoSocketError::IoErrorCode::NoSupport, Api::IoError::getErrorCode(error3)); + EXPECT_EQ(::strerror(ENOTSUP), Api::IoError::getErrorDetails(error3)); IoSocketError error4(EAFNOSUPPORT); - EXPECT_EQ(IoSocketError::IoErrorCode::AddressFamilyNoSupport, IoError::getErrorCode(error4)); - EXPECT_EQ(::strerror(EAFNOSUPPORT), IoError::getErrorDetails(error4)); + EXPECT_EQ(IoSocketError::IoErrorCode::AddressFamilyNoSupport, Api::IoError::getErrorCode(error4)); + EXPECT_EQ(::strerror(EAFNOSUPPORT), Api::IoError::getErrorDetails(error4)); IoSocketError error5(EINPROGRESS); - EXPECT_EQ(IoSocketError::IoErrorCode::InProgress, IoError::getErrorCode(error5)); - EXPECT_EQ(::strerror(EINPROGRESS), IoError::getErrorDetails(error5)); + EXPECT_EQ(IoSocketError::IoErrorCode::InProgress, Api::IoError::getErrorCode(error5)); + EXPECT_EQ(::strerror(EINPROGRESS), Api::IoError::getErrorDetails(error5)); IoSocketError error6(EPERM); - EXPECT_EQ(IoSocketError::IoErrorCode::Permission, IoError::getErrorCode(error6)); - EXPECT_EQ(::strerror(EPERM), IoError::getErrorDetails(error6)); + EXPECT_EQ(IoSocketError::IoErrorCode::Permission, Api::IoError::getErrorCode(error6)); + EXPECT_EQ(::strerror(EPERM), Api::IoError::getErrorDetails(error6)); // Random unknown error. IoSocketError error7(123); - EXPECT_EQ(IoSocketError::IoErrorCode::UnknownError, IoError::getErrorCode(error7)); - EXPECT_EQ(::strerror(123), IoError::getErrorDetails(error7)); + EXPECT_EQ(IoSocketError::IoErrorCode::UnknownError, Api::IoError::getErrorCode(error7)); + EXPECT_EQ(::strerror(123), Api::IoError::getErrorDetails(error7)); } } // namespace From 0012f056b2a5b585058845496aa24a0e333c96f7 Mon Sep 17 00:00:00 2001 From: Sam Smith Date: Thu, 28 Feb 2019 08:24:23 -0500 Subject: [PATCH 2/3] Remove unused types. Signed-off-by: Sam Smith Signed-off-by: Yael Harel --- include/envoy/api/io_error.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/envoy/api/io_error.h b/include/envoy/api/io_error.h index f4954b46dd730..fd7a4a441d9b4 100644 --- a/include/envoy/api/io_error.h +++ b/include/envoy/api/io_error.h @@ -79,8 +79,6 @@ template struct IoCallResult { IoErrorPtr err_; }; -using IoCallBoolResult = IoCallResult; -using IoCallSizeResult = IoCallResult; using IoCallUintResult = IoCallResult; } // namespace Api From 21205565d11d53e7d39b0473eb33ccb3a052a9a6 Mon Sep 17 00:00:00 2001 From: Yael Harel Date: Thu, 28 Feb 2019 10:31:22 -0500 Subject: [PATCH 3/3] add TODO Signed-off-by: Yael Harel Signed-off-by: Sam Smith --- include/envoy/api/io_error.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/envoy/api/io_error.h b/include/envoy/api/io_error.h index fd7a4a441d9b4..e1f4af3b1df7f 100644 --- a/include/envoy/api/io_error.h +++ b/include/envoy/api/io_error.h @@ -15,6 +15,8 @@ class IoError; // frequent memory allocation of IoError instance. // If this is used, IoCallResult has to be instantiated with a deleter that does not // deallocate memory for this error. +// TODO: This is probably not the best way to avoid allocations in the case of +// EAGAIN. This will be fixed as a part of #6037. #define ENVOY_ERROR_AGAIN reinterpret_cast(0x01) /**