From 4b581b380460c663b4f39d04c2481681b2678ab7 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Wed, 18 Jul 2018 20:58:55 -0700 Subject: [PATCH 1/7] Refactor address APIs for deeper errno latching The errno set by a syscall can be overwritten by code (e.g. logging) as it propagates up through the call stack. This commit refactors the bind and connect methods in the address API to allow for returning the errno from deeper down the call stack i.e. as soon as a syscall is performed. Signed-off-by: Venil Noronha --- include/envoy/network/address.h | 13 ++--- source/common/network/address_impl.cc | 48 +++++++++++-------- source/common/network/address_impl.h | 12 ++--- source/common/network/connection_impl.cc | 14 +++--- source/common/network/listen_socket_impl.cc | 8 ++-- .../stat_sinks/common/statsd/statsd.cc | 2 +- test/common/network/address_impl_test.cc | 15 +++--- test/common/network/dns_impl_test.cc | 4 +- test/mocks/network/mocks.h | 4 +- test/test_common/network_utility.cc | 22 ++++++--- 10 files changed, 82 insertions(+), 60 deletions(-) diff --git a/include/envoy/network/address.h b/include/envoy/network/address.h index 949747484018f..1d52e2afe2e44 100644 --- a/include/envoy/network/address.h +++ b/include/envoy/network/address.h @@ -7,6 +7,7 @@ #include #include #include +#include #include "envoy/common/pure.h" @@ -128,19 +129,19 @@ class Instance { * Bind a socket to this address. The socket should have been created with a call to socket() on * an Instance of the same address family. * @param fd supplies the platform socket handle. - * @return 0 for success and -1 for failure. The error code associated with a failure will - * be accessible in a plaform dependent fashion (e.g. errno for Unix platforms). + * @return a tuple of <0, errno> for success and <-1, errno> for failure. If the call is + * successful, errno shouldn't be used. */ - virtual int bind(int fd) const PURE; + virtual std::tuple bind(int fd) const PURE; /** * Connect a socket to this address. The socket should have been created with a call to socket() * on this object. * @param fd supplies the platform socket handle. - * @return 0 for success and -1 for failure. The error code associated with a failure will - * be accessible in a plaform dependent fashion (e.g. errno for Unix platforms). + * @return a tuple of <0, errno> for success and <-1, errno> for failure. If the call is + * successful, errno shouldn't be used. */ - virtual int connect(int fd) const PURE; + virtual std::tuple connect(int fd) const PURE; /** * @return the IP address information IFF type() == Type::Ip, otherwise nullptr. diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index e7d1e6d0a0cd2..e4b72dee3a560 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -212,14 +212,16 @@ bool Ipv4Instance::operator==(const Instance& rhs) const { (ip_.port() == rhs_casted->ip_.port())); } -int Ipv4Instance::bind(int fd) const { - return ::bind(fd, reinterpret_cast(&ip_.ipv4_.address_), - sizeof(ip_.ipv4_.address_)); +std::tuple Ipv4Instance::bind(int fd) const { + return std::make_tuple(::bind(fd, reinterpret_cast(&ip_.ipv4_.address_), + sizeof(ip_.ipv4_.address_)), + errno); } -int Ipv4Instance::connect(int fd) const { - return ::connect(fd, reinterpret_cast(&ip_.ipv4_.address_), - sizeof(ip_.ipv4_.address_)); +std::tuple Ipv4Instance::connect(int fd) const { + return std::make_tuple(::connect(fd, reinterpret_cast(&ip_.ipv4_.address_), + sizeof(ip_.ipv4_.address_)), + errno); } int Ipv4Instance::socket(SocketType type) const { return socketFromSocketType(type); } @@ -275,14 +277,16 @@ bool Ipv6Instance::operator==(const Instance& rhs) const { (ip_.port() == rhs_casted->ip_.port())); } -int Ipv6Instance::bind(int fd) const { - return ::bind(fd, reinterpret_cast(&ip_.ipv6_.address_), - sizeof(ip_.ipv6_.address_)); +std::tuple Ipv6Instance::bind(int fd) const { + return std::make_tuple(::bind(fd, reinterpret_cast(&ip_.ipv6_.address_), + sizeof(ip_.ipv6_.address_)), + errno); } -int Ipv6Instance::connect(int fd) const { - return ::connect(fd, reinterpret_cast(&ip_.ipv6_.address_), - sizeof(ip_.ipv6_.address_)); +std::tuple Ipv6Instance::connect(int fd) const { + return std::make_tuple(::connect(fd, reinterpret_cast(&ip_.ipv6_.address_), + sizeof(ip_.ipv6_.address_)), + errno); } int Ipv6Instance::socket(SocketType type) const { @@ -333,24 +337,28 @@ PipeInstance::PipeInstance(const std::string& pipe_path) : InstanceBase(Type::Pi bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); } -int PipeInstance::bind(int fd) const { +std::tuple PipeInstance::bind(int fd) const { if (abstract_namespace_) { - return ::bind(fd, reinterpret_cast(&address_), - offsetof(struct sockaddr_un, sun_path) + address_length_); + return std::make_tuple(::bind(fd, reinterpret_cast(&address_), + offsetof(struct sockaddr_un, sun_path) + address_length_), + errno); } // Try to unlink an existing filesystem object at the requested path. Ignore // errors -- it's fine if the path doesn't exist, and if it exists but can't // be unlinked then `::bind()` will generate a reasonable errno. unlink(address_.sun_path); - return ::bind(fd, reinterpret_cast(&address_), sizeof(address_)); + return std::make_tuple(::bind(fd, reinterpret_cast(&address_), sizeof(address_)), + errno); } -int PipeInstance::connect(int fd) const { +std::tuple PipeInstance::connect(int fd) const { if (abstract_namespace_) { - return ::connect(fd, reinterpret_cast(&address_), - offsetof(struct sockaddr_un, sun_path) + address_length_); + return std::make_tuple(::connect(fd, reinterpret_cast(&address_), + offsetof(struct sockaddr_un, sun_path) + address_length_), + errno); } - return ::connect(fd, reinterpret_cast(&address_), sizeof(address_)); + return std::make_tuple( + ::connect(fd, reinterpret_cast(&address_), sizeof(address_)), errno); } int PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); } diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index c48003624c137..d58b26ad91c86 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -91,8 +91,8 @@ class Ipv4Instance : public InstanceBase { // Network::Address::Instance bool operator==(const Instance& rhs) const override; - int bind(int fd) const override; - int connect(int fd) const override; + std::tuple bind(int fd) const override; + std::tuple connect(int fd) const override; const Ip* ip() const override { return &ip_; } int socket(SocketType type) const override; @@ -151,8 +151,8 @@ class Ipv6Instance : public InstanceBase { // Network::Address::Instance bool operator==(const Instance& rhs) const override; - int bind(int fd) const override; - int connect(int fd) const override; + std::tuple bind(int fd) const override; + std::tuple connect(int fd) const override; const Ip* ip() const override { return &ip_; } int socket(SocketType type) const override; @@ -208,8 +208,8 @@ class PipeInstance : public InstanceBase { // Network::Address::Instance bool operator==(const Instance& rhs) const override; - int bind(int fd) const override; - int connect(int fd) const override; + std::tuple bind(int fd) const override; + std::tuple connect(int fd) const override; const Ip* ip() const override { return nullptr; } int socket(SocketType type) const override; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index b08c483ca8a7b..423baa5487478 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -551,10 +551,10 @@ ClientConnectionImpl::ClientConnectionImpl( } if (source_address != nullptr) { - const int rc = source_address->bind(fd()); - if (rc < 0) { + const std::tuple result = source_address->bind(fd()); + if (std::get<0>(result) < 0) { ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(), - strerror(errno)); + strerror(std::get<1>(result))); bind_error_ = true; // Set a special error state to ensure asynchronous close to give the owner of the // ConnectionImpl a chance to add callbacks and detect the "disconnect". @@ -568,19 +568,21 @@ ClientConnectionImpl::ClientConnectionImpl( void ClientConnectionImpl::connect() { ENVOY_CONN_LOG(debug, "connecting to {}", *this, socket_->remoteAddress()->asString()); - const int rc = socket_->remoteAddress()->connect(fd()); + const std::tuple result = socket_->remoteAddress()->connect(fd()); + const int rc = std::get<0>(result); + const int error = std::get<1>(result); if (rc == 0) { // write will become ready. ASSERT(connecting_); } else { ASSERT(rc == -1); - if (errno == EINPROGRESS) { + if (error == EINPROGRESS) { ASSERT(connecting_); ENVOY_CONN_LOG(debug, "connection in progress", *this); } else { immediate_error_event_ = ConnectionEvent::RemoteClose; connecting_ = false; - ENVOY_CONN_LOG(debug, "immediate connection error: {}", *this, errno); + ENVOY_CONN_LOG(debug, "immediate connection error: {}", *this, error); // Trigger a write event. This is needed on OSX and seems harmless on Linux. file_event_->activate(Event::FileReadyType::Write); diff --git a/source/common/network/listen_socket_impl.cc b/source/common/network/listen_socket_impl.cc index 3e79e13edc80b..3db0424159ce2 100644 --- a/source/common/network/listen_socket_impl.cc +++ b/source/common/network/listen_socket_impl.cc @@ -16,11 +16,11 @@ namespace Envoy { namespace Network { void ListenSocketImpl::doBind() { - int rc = local_address_->bind(fd_); - if (rc == -1) { + const std::tuple result = local_address_->bind(fd_); + if (std::get<0>(result) == -1) { close(); - throw EnvoyException( - fmt::format("cannot bind '{}': {}", local_address_->asString(), strerror(errno))); + throw EnvoyException(fmt::format("cannot bind '{}': {}", local_address_->asString(), + strerror(std::get<1>(result)))); } if (local_address_->type() == Address::Type::Ip && local_address_->ip()->port() == 0) { // If the port we bind is zero, then the OS will pick a free port for us (assuming there are diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 3326dcaedfc01..a945157d9233e 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -23,7 +23,7 @@ Writer::Writer(Network::Address::InstanceConstSharedPtr address) { fd_ = address->socket(Network::Address::SocketType::Datagram); ASSERT(fd_ != -1); - int rc = address->connect(fd_); + const int rc = std::get<0>(address->connect(fd_)); ASSERT(rc != -1); } diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index 338ad1fe3e185..500a2391cdf2e 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -64,8 +64,9 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl } // Bind the socket to the desired address and port. - const int rc = addr_port->bind(listen_fd); - const int err = errno; + const std::tuple result = addr_port->bind(listen_fd); + const int rc = std::get<0>(result); + const int err = std::get<1>(result); ASSERT_EQ(rc, 0) << addr_port->asString() << "\nerror: " << strerror(err) << "\nerrno: " << err; // Do a bare listen syscall. Not bothering to accept connections as that would @@ -85,8 +86,9 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl makeFdBlocking(client_fd); // Connect to the server. - const int rc = addr_port->connect(client_fd); - const int err = errno; + const std::tuple result = addr_port->connect(client_fd); + const int rc = std::get<0>(result); + const int err = std::get<1>(result); ASSERT_EQ(rc, 0) << addr_port->asString() << "\nerror: " << strerror(err) << "\nerrno: " << err; }; @@ -314,8 +316,9 @@ TEST(PipeInstanceTest, UnlinksExistingFile) { ASSERT_GE(listen_fd, 0) << address.asString(); ScopedFdCloser closer(listen_fd); - const int rc = address.bind(listen_fd); - const int err = errno; + const std::tuple result = address.bind(listen_fd); + const int rc = std::get<0>(result); + const int err = std::get<1>(result); ASSERT_EQ(rc, 0) << address.asString() << "\nerror: " << strerror(err) << "\nerrno: " << err; }; diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index f275a86fa733c..cf8a8177f5196 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -359,8 +359,8 @@ class CustomInstance : public Address::Instance { } const std::string& asString() const override { return antagonistic_name_; } const std::string& logicalName() const override { return antagonistic_name_; } - int bind(int fd) const override { return instance_.bind(fd); } - int connect(int fd) const override { return instance_.connect(fd); } + std::tuple bind(int fd) const override { return instance_.bind(fd); } + std::tuple connect(int fd) const override { return instance_.connect(fd); } const Address::Ip* ip() const override { return instance_.ip(); } int socket(Address::SocketType type) const override { return instance_.socket(type); } Address::Type type() const override { return instance_.type(); } diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 8b21c1bc3c479..f2a314fc000ca 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -409,8 +409,8 @@ class MockResolvedAddress : public Address::Instance { return asString() == other.asString(); } - MOCK_CONST_METHOD1(bind, int(int)); - MOCK_CONST_METHOD1(connect, int(int)); + MOCK_CONST_METHOD1(bind, std::tuple(int)); + MOCK_CONST_METHOD1(connect, std::tuple(int)); MOCK_CONST_METHOD0(ip, Address::Ip*()); MOCK_CONST_METHOD1(socket, int(Address::SocketType)); MOCK_CONST_METHOD0(type, Address::Type()); diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index ae42b3b4cbe2c..e8d2d22015770 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -37,19 +37,22 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(Address::InstanceConstShared // Not setting REUSEADDR, therefore if the address has been recently used we won't reuse it here. // However, because we're going to use the address while checking if it is available, we'll need // to set REUSEADDR on listener sockets created by tests using an address validated by this means. - int rc = addr_port->bind(fd); + std::tuple result = addr_port->bind(fd); + int rc = std::get<0>(result); + int err; const char* failing_fn = nullptr; if (rc != 0) { + err = std::get<1>(result); failing_fn = "bind"; } else if (type == Address::SocketType::Stream) { // Try listening on the port also, if the type is TCP. rc = ::listen(fd, 1); if (rc != 0) { + err = errno; failing_fn = "listen"; } } if (failing_fn != nullptr) { - const int err = errno; if (err == EADDRINUSE) { // The port is already in use. Perfectly normal. return nullptr; @@ -152,7 +155,7 @@ bool supportsIpVersion(const Address::IpVersion version) { // Socket creation failed. return false; } - if (0 != addr->bind(fd)) { + if (0 != std::get<0>(addr->bind(fd))) { // Socket bind failed. RELEASE_ASSERT(::close(fd) == 0, ""); return false; @@ -166,14 +169,19 @@ std::pair bindFreeLoopbackPort(Address::Ip Address::InstanceConstSharedPtr addr = getCanonicalLoopbackAddress(version); const char* failing_fn = nullptr; const int fd = addr->socket(type); + int err; if (fd < 0) { + err = errno; failing_fn = "socket"; - } else if (0 != addr->bind(fd)) { - failing_fn = "bind"; } else { - return std::make_pair(Address::addressFromFd(fd), fd); + std::tuple result = addr->bind(fd); + if (0 != std::get<0>(result)) { + err = std::get<1>(result); + failing_fn = "bind"; + } else { + return std::make_pair(Address::addressFromFd(fd), fd); + } } - const int err = errno; if (fd >= 0) { close(fd); } From 6c65b13850583447b5ae5fe69194b5be5e08f07b Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Thu, 19 Jul 2018 11:08:10 -0700 Subject: [PATCH 2/7] Update address's socket method signature This commit ensures that address implementations return the errno along with the file descriptor in socket method calls. Signed-off-by: Venil Noronha --- include/envoy/network/address.h | 8 ++++---- source/common/network/address_impl.cc | 19 ++++++++++++------- source/common/network/address_impl.h | 8 ++++---- source/common/network/listen_socket_impl.cc | 4 ++-- source/common/network/listen_socket_impl.h | 4 ++-- .../stat_sinks/common/statsd/statsd.cc | 2 +- ...dr_family_aware_socket_option_impl_test.cc | 14 +++++++------- test/common/network/address_impl_test.cc | 6 +++--- test/common/network/dns_impl_test.cc | 4 +++- test/mocks/network/mocks.h | 2 +- test/test_common/network_utility.cc | 18 ++++++++++-------- 11 files changed, 49 insertions(+), 40 deletions(-) diff --git a/include/envoy/network/address.h b/include/envoy/network/address.h index 1d52e2afe2e44..e96a10e02c897 100644 --- a/include/envoy/network/address.h +++ b/include/envoy/network/address.h @@ -151,11 +151,11 @@ class Instance { /** * Create a socket for this address. * @param type supplies the socket type to create. - * @return the file descriptor naming the socket for success and -1 for failure. The error - * code associated with a failure will be accessible in a plaform dependent fashion (e.g. - * errno for Unix platforms). + * @return a tuple of for success and <-1, errno> for failure, where fd represents + * the file descriptor naming the socket. If a valid file descriptor is returned, errno + * shouldn't be used. */ - virtual int socket(SocketType type) const PURE; + virtual std::tuple socket(SocketType type) const PURE; /** * @return the type of address. diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index e4b72dee3a560..787437474940e 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -133,7 +133,7 @@ InstanceConstSharedPtr peerAddressFromFd(int fd) { return addressFromSockAddr(ss, ss_len); } -int InstanceBase::socketFromSocketType(SocketType socketType) const { +std::tuple InstanceBase::socketFromSocketType(SocketType socketType) const { #if defined(__APPLE__) int flags = 0; #else @@ -168,7 +168,7 @@ int InstanceBase::socketFromSocketType(SocketType socketType) const { RELEASE_ASSERT(fcntl(fd, F_SETFL, O_NONBLOCK) != -1, ""); #endif - return fd; + return std::make_tuple(fd, errno); } Ipv4Instance::Ipv4Instance(const sockaddr_in* address) : InstanceBase(Type::Ip) { @@ -224,7 +224,9 @@ std::tuple Ipv4Instance::connect(int fd) const { errno); } -int Ipv4Instance::socket(SocketType type) const { return socketFromSocketType(type); } +std::tuple Ipv4Instance::socket(SocketType type) const { + return socketFromSocketType(type); +} absl::uint128 Ipv6Instance::Ipv6Helper::address() const { absl::uint128 result{0}; @@ -289,13 +291,14 @@ std::tuple Ipv6Instance::connect(int fd) const { errno); } -int Ipv6Instance::socket(SocketType type) const { - const int fd = socketFromSocketType(type); +std::tuple Ipv6Instance::socket(SocketType type) const { + const std::tuple result = socketFromSocketType(type); + const int fd = std::get<0>(result); // Setting IPV6_V6ONLY resticts the IPv6 socket to IPv6 connections only. const int v6only = ip_.v6only_; RELEASE_ASSERT(::setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, ""); - return fd; + return result; } PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len) @@ -361,7 +364,9 @@ std::tuple PipeInstance::connect(int fd) const { ::connect(fd, reinterpret_cast(&address_), sizeof(address_)), errno); } -int PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); } +std::tuple PipeInstance::socket(SocketType type) const { + return socketFromSocketType(type); +} } // namespace Address } // namespace Network diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index d58b26ad91c86..876bdf19cecbd 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -55,7 +55,7 @@ class InstanceBase : public Instance { protected: InstanceBase(Type type) : type_(type) {} - int socketFromSocketType(SocketType type) const; + std::tuple socketFromSocketType(SocketType type) const; std::string friendly_name_; @@ -94,7 +94,7 @@ class Ipv4Instance : public InstanceBase { std::tuple bind(int fd) const override; std::tuple connect(int fd) const override; const Ip* ip() const override { return &ip_; } - int socket(SocketType type) const override; + std::tuple socket(SocketType type) const override; private: struct Ipv4Helper : public Ipv4 { @@ -154,7 +154,7 @@ class Ipv6Instance : public InstanceBase { std::tuple bind(int fd) const override; std::tuple connect(int fd) const override; const Ip* ip() const override { return &ip_; } - int socket(SocketType type) const override; + std::tuple socket(SocketType type) const override; private: struct Ipv6Helper : public Ipv6 { @@ -211,7 +211,7 @@ class PipeInstance : public InstanceBase { std::tuple bind(int fd) const override; std::tuple connect(int fd) const override; const Ip* ip() const override { return nullptr; } - int socket(SocketType type) const override; + std::tuple socket(SocketType type) const override; private: sockaddr_un address_; diff --git a/source/common/network/listen_socket_impl.cc b/source/common/network/listen_socket_impl.cc index 3db0424159ce2..8b1e9c3588f9a 100644 --- a/source/common/network/listen_socket_impl.cc +++ b/source/common/network/listen_socket_impl.cc @@ -39,7 +39,7 @@ void ListenSocketImpl::setListenSocketOptions(const Network::Socket::OptionsShar TcpListenSocket::TcpListenSocket(const Address::InstanceConstSharedPtr& address, const Network::Socket::OptionsSharedPtr& options, bool bind_to_port) - : ListenSocketImpl(address->socket(Address::SocketType::Stream), address) { + : ListenSocketImpl(std::get<0>(address->socket(Address::SocketType::Stream)), address) { RELEASE_ASSERT(fd_ != -1, ""); // TODO(htuch): This might benefit from moving to SocketOptionImpl. @@ -61,7 +61,7 @@ TcpListenSocket::TcpListenSocket(int fd, const Address::InstanceConstSharedPtr& } UdsListenSocket::UdsListenSocket(const Address::InstanceConstSharedPtr& address) - : ListenSocketImpl(address->socket(Address::SocketType::Stream), address) { + : ListenSocketImpl(std::get<0>(address->socket(Address::SocketType::Stream)), address) { RELEASE_ASSERT(fd_ != -1, ""); doBind(); } diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index ef512226d0514..06a630f6c7acb 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -137,8 +137,8 @@ class AcceptedSocketImpl : public ConnectionSocketImpl { class ClientSocketImpl : public ConnectionSocketImpl { public: ClientSocketImpl(const Address::InstanceConstSharedPtr& remote_address) - : ConnectionSocketImpl(remote_address->socket(Address::SocketType::Stream), nullptr, - remote_address) {} + : ConnectionSocketImpl(std::get<0>(remote_address->socket(Address::SocketType::Stream)), + nullptr, remote_address) {} }; } // namespace Network diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index a945157d9233e..7acd80deffd5c 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -20,7 +20,7 @@ namespace Common { namespace Statsd { Writer::Writer(Network::Address::InstanceConstSharedPtr address) { - fd_ = address->socket(Network::Address::SocketType::Datagram); + fd_ = std::get<0>(address->socket(Network::Address::SocketType::Datagram)); ASSERT(fd_ != -1); const int rc = std::get<0>(address->connect(fd_)); diff --git a/test/common/network/addr_family_aware_socket_option_impl_test.cc b/test/common/network/addr_family_aware_socket_option_impl_test.cc index 3c6c16a0d9c9f..a0cbb0bf4c339 100644 --- a/test/common/network/addr_family_aware_socket_option_impl_test.cc +++ b/test/common/network/addr_family_aware_socket_option_impl_test.cc @@ -23,7 +23,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionFailure) { // If a platform supports IPv4 socket option variant for an IPv4 address, it works TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionSuccess) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream); + const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -37,7 +37,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionSuccess) { // If a platform doesn't support IPv4 socket option variant for an IPv4 address we fail TEST_F(AddrFamilyAwareSocketOptionImplTest, V4EmptyOptionNames) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream); + const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1}; @@ -50,7 +50,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V4EmptyOptionNames) { // If a platform doesn't support IPv4 and IPv6 socket option variants for an IPv4 address, we fail TEST_F(AddrFamilyAwareSocketOptionImplTest, V6EmptyOptionNames) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream); + const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1}; @@ -64,7 +64,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6EmptyOptionNames) { // IPv4 varient TEST_F(AddrFamilyAwareSocketOptionImplTest, V4IgnoreV6) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream); + const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -78,7 +78,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V4IgnoreV6) { // If a platform suppports IPv6 socket option variant for an IPv6 address it works TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Only) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream); + const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -93,7 +93,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Only) { // we apply the IPv4 variant. TEST_F(AddrFamilyAwareSocketOptionImplTest, V6OnlyV4Fallback) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream); + const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -108,7 +108,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6OnlyV4Fallback) { // AddrFamilyAwareSocketOptionImpl::setIpSocketOption() works with the IPv6 variant. TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Precedence) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream); + const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index 500a2391cdf2e..bc5ffdfd41e98 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -51,7 +51,7 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl ASSERT_NE(addr_port->ip(), nullptr); // Create a socket on which we'll listen for connections from clients. - const int listen_fd = addr_port->socket(SocketType::Stream); + const int listen_fd = std::get<0>(addr_port->socket(SocketType::Stream)); ASSERT_GE(listen_fd, 0) << addr_port->asString(); ScopedFdCloser closer1(listen_fd); @@ -75,7 +75,7 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl auto client_connect = [](Address::InstanceConstSharedPtr addr_port) { // Create a client socket and connect to the server. - const int client_fd = addr_port->socket(SocketType::Stream); + const int client_fd = std::get<0>(addr_port->socket(SocketType::Stream)); ASSERT_GE(client_fd, 0) << addr_port->asString(); ScopedFdCloser closer2(client_fd); @@ -312,7 +312,7 @@ TEST(PipeInstanceTest, BadAddress) { TEST(PipeInstanceTest, UnlinksExistingFile) { const auto bind_uds_socket = [](const std::string& path) { PipeInstance address(path); - const int listen_fd = address.socket(SocketType::Stream); + const int listen_fd = std::get<0>(address.socket(SocketType::Stream)); ASSERT_GE(listen_fd, 0) << address.asString(); ScopedFdCloser closer(listen_fd); diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index cf8a8177f5196..73e22ab0ee35c 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -362,7 +362,9 @@ class CustomInstance : public Address::Instance { std::tuple bind(int fd) const override { return instance_.bind(fd); } std::tuple connect(int fd) const override { return instance_.connect(fd); } const Address::Ip* ip() const override { return instance_.ip(); } - int socket(Address::SocketType type) const override { return instance_.socket(type); } + std::tuple socket(Address::SocketType type) const override { + return instance_.socket(type); + } Address::Type type() const override { return instance_.type(); } private: diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index f2a314fc000ca..9d1fc72de2733 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -412,7 +412,7 @@ class MockResolvedAddress : public Address::Instance { MOCK_CONST_METHOD1(bind, std::tuple(int)); MOCK_CONST_METHOD1(connect, std::tuple(int)); MOCK_CONST_METHOD0(ip, Address::Ip*()); - MOCK_CONST_METHOD1(socket, int(Address::SocketType)); + MOCK_CONST_METHOD1(socket, std::tuple(Address::SocketType)); MOCK_CONST_METHOD0(type, Address::Type()); const std::string& asString() const override { return physical_; } diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index e8d2d22015770..d420c54136ac0 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -26,9 +26,11 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(Address::InstanceConstShared << (addr_port == nullptr ? "nullptr" : addr_port->asString()); return nullptr; } - const int fd = addr_port->socket(type); + std::tuple result = addr_port->socket(type); + const int fd = std::get<0>(result); + int err; if (fd < 0) { - const int err = errno; + err = std::get<1>(result); ADD_FAILURE() << "socket failed for '" << addr_port->asString() << "' with error: " << strerror(err) << " (" << err << ")"; return nullptr; @@ -37,9 +39,8 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(Address::InstanceConstShared // Not setting REUSEADDR, therefore if the address has been recently used we won't reuse it here. // However, because we're going to use the address while checking if it is available, we'll need // to set REUSEADDR on listener sockets created by tests using an address validated by this means. - std::tuple result = addr_port->bind(fd); + result = addr_port->bind(fd); int rc = std::get<0>(result); - int err; const char* failing_fn = nullptr; if (rc != 0) { err = std::get<1>(result); @@ -150,7 +151,7 @@ Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, bool supportsIpVersion(const Address::IpVersion version) { Address::InstanceConstSharedPtr addr = getCanonicalLoopbackAddress(version); - const int fd = addr->socket(Address::SocketType::Stream); + const int fd = std::get<0>(addr->socket(Address::SocketType::Stream)); if (fd < 0) { // Socket creation failed. return false; @@ -168,13 +169,14 @@ std::pair bindFreeLoopbackPort(Address::Ip Address::SocketType type) { Address::InstanceConstSharedPtr addr = getCanonicalLoopbackAddress(version); const char* failing_fn = nullptr; - const int fd = addr->socket(type); + std::tuple result = addr->socket(type); + const int fd = std::get<0>(result); int err; if (fd < 0) { - err = errno; + err = std::get<1>(result); failing_fn = "socket"; } else { - std::tuple result = addr->bind(fd); + result = addr->bind(fd); if (0 != std::get<0>(result)) { err = std::get<1>(result); failing_fn = "bind"; From 36c81e51fceb0a31e100d2ac9a2d12e19d2d2bb5 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Mon, 23 Jul 2018 17:49:26 -0700 Subject: [PATCH 3/7] Replace std::tuple with Result This commit adds a struct named Result which holds the rc and errno values resulting from a syscall. Also, the address API is refactored to use the new Result struct instead of std::tuple. Signed-off-by: Venil Noronha --- include/envoy/network/address.h | 35 ++++++--- source/common/network/address_impl.cc | 75 +++++++++---------- source/common/network/address_impl.h | 20 ++--- source/common/network/connection_impl.cc | 18 ++--- source/common/network/listen_socket_impl.cc | 12 +-- source/common/network/listen_socket_impl.h | 4 +- .../stat_sinks/common/statsd/statsd.cc | 4 +- ...dr_family_aware_socket_option_impl_test.cc | 14 ++-- test/common/network/address_impl_test.cc | 27 +++---- test/common/network/dns_impl_test.cc | 8 +- test/mocks/network/mocks.h | 6 +- test/test_common/network_utility.cc | 31 ++++---- 12 files changed, 126 insertions(+), 128 deletions(-) diff --git a/include/envoy/network/address.h b/include/envoy/network/address.h index e96a10e02c897..3692da5bd9866 100644 --- a/include/envoy/network/address.h +++ b/include/envoy/network/address.h @@ -7,7 +7,6 @@ #include #include #include -#include #include "envoy/common/pure.h" @@ -17,6 +16,22 @@ namespace Envoy { namespace Network { namespace Address { +/** + * Result holds the rc and errno values resulting from a system call. + */ +struct Result { + + /** + * The return code from the system call. + */ + int rc; + + /** + * The errno value as captured after the system call. + */ + int error; +}; + /** * Interface for an Ipv4 address. */ @@ -129,19 +144,19 @@ class Instance { * Bind a socket to this address. The socket should have been created with a call to socket() on * an Instance of the same address family. * @param fd supplies the platform socket handle. - * @return a tuple of <0, errno> for success and <-1, errno> for failure. If the call is - * successful, errno shouldn't be used. + * @return a Result with rc = 0 for success and rc = -1 for failure. If the call is successful, + * the error value shouldn't be used. */ - virtual std::tuple bind(int fd) const PURE; + virtual Result bind(int fd) const PURE; /** * Connect a socket to this address. The socket should have been created with a call to socket() * on this object. * @param fd supplies the platform socket handle. - * @return a tuple of <0, errno> for success and <-1, errno> for failure. If the call is - * successful, errno shouldn't be used. + * @return a Result with rc = 0 for success and rc = -1 for failure. If the call is successful, + * the error value shouldn't be used. */ - virtual std::tuple connect(int fd) const PURE; + virtual Result connect(int fd) const PURE; /** * @return the IP address information IFF type() == Type::Ip, otherwise nullptr. @@ -151,11 +166,11 @@ class Instance { /** * Create a socket for this address. * @param type supplies the socket type to create. - * @return a tuple of for success and <-1, errno> for failure, where fd represents - * the file descriptor naming the socket. If a valid file descriptor is returned, errno + * @return a Result with rc = fd for success and rc = -1 for failure, where fd represents the + * file descriptor naming the socket. If a valid file descriptor is returned, the error value * shouldn't be used. */ - virtual std::tuple socket(SocketType type) const PURE; + virtual Result socket(SocketType type) const PURE; /** * @return the type of address. diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 787437474940e..232fbfbc6f7a8 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -133,7 +133,7 @@ InstanceConstSharedPtr peerAddressFromFd(int fd) { return addressFromSockAddr(ss, ss_len); } -std::tuple InstanceBase::socketFromSocketType(SocketType socketType) const { +Result InstanceBase::socketFromSocketType(SocketType socketType) const { #if defined(__APPLE__) int flags = 0; #else @@ -168,7 +168,7 @@ std::tuple InstanceBase::socketFromSocketType(SocketType socketType) c RELEASE_ASSERT(fcntl(fd, F_SETFL, O_NONBLOCK) != -1, ""); #endif - return std::make_tuple(fd, errno); + return {fd, errno}; } Ipv4Instance::Ipv4Instance(const sockaddr_in* address) : InstanceBase(Type::Ip) { @@ -212,21 +212,19 @@ bool Ipv4Instance::operator==(const Instance& rhs) const { (ip_.port() == rhs_casted->ip_.port())); } -std::tuple Ipv4Instance::bind(int fd) const { - return std::make_tuple(::bind(fd, reinterpret_cast(&ip_.ipv4_.address_), - sizeof(ip_.ipv4_.address_)), - errno); +Result Ipv4Instance::bind(int fd) const { + return {::bind(fd, reinterpret_cast(&ip_.ipv4_.address_), + sizeof(ip_.ipv4_.address_)), + errno}; } -std::tuple Ipv4Instance::connect(int fd) const { - return std::make_tuple(::connect(fd, reinterpret_cast(&ip_.ipv4_.address_), - sizeof(ip_.ipv4_.address_)), - errno); +Result Ipv4Instance::connect(int fd) const { + return {::connect(fd, reinterpret_cast(&ip_.ipv4_.address_), + sizeof(ip_.ipv4_.address_)), + errno}; } -std::tuple Ipv4Instance::socket(SocketType type) const { - return socketFromSocketType(type); -} +Result Ipv4Instance::socket(SocketType type) const { return socketFromSocketType(type); } absl::uint128 Ipv6Instance::Ipv6Helper::address() const { absl::uint128 result{0}; @@ -279,25 +277,24 @@ bool Ipv6Instance::operator==(const Instance& rhs) const { (ip_.port() == rhs_casted->ip_.port())); } -std::tuple Ipv6Instance::bind(int fd) const { - return std::make_tuple(::bind(fd, reinterpret_cast(&ip_.ipv6_.address_), - sizeof(ip_.ipv6_.address_)), - errno); +Result Ipv6Instance::bind(int fd) const { + return {::bind(fd, reinterpret_cast(&ip_.ipv6_.address_), + sizeof(ip_.ipv6_.address_)), + errno}; } -std::tuple Ipv6Instance::connect(int fd) const { - return std::make_tuple(::connect(fd, reinterpret_cast(&ip_.ipv6_.address_), - sizeof(ip_.ipv6_.address_)), - errno); +Result Ipv6Instance::connect(int fd) const { + return {::connect(fd, reinterpret_cast(&ip_.ipv6_.address_), + sizeof(ip_.ipv6_.address_)), + errno}; } -std::tuple Ipv6Instance::socket(SocketType type) const { - const std::tuple result = socketFromSocketType(type); - const int fd = std::get<0>(result); - +Result Ipv6Instance::socket(SocketType type) const { + const Result result = socketFromSocketType(type); // Setting IPV6_V6ONLY resticts the IPv6 socket to IPv6 connections only. const int v6only = ip_.v6only_; - RELEASE_ASSERT(::setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, ""); + RELEASE_ASSERT(::setsockopt(result.rc, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, + ""); return result; } @@ -340,33 +337,29 @@ PipeInstance::PipeInstance(const std::string& pipe_path) : InstanceBase(Type::Pi bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); } -std::tuple PipeInstance::bind(int fd) const { +Result PipeInstance::bind(int fd) const { if (abstract_namespace_) { - return std::make_tuple(::bind(fd, reinterpret_cast(&address_), - offsetof(struct sockaddr_un, sun_path) + address_length_), - errno); + return {::bind(fd, reinterpret_cast(&address_), + offsetof(struct sockaddr_un, sun_path) + address_length_), + errno}; } // Try to unlink an existing filesystem object at the requested path. Ignore // errors -- it's fine if the path doesn't exist, and if it exists but can't // be unlinked then `::bind()` will generate a reasonable errno. unlink(address_.sun_path); - return std::make_tuple(::bind(fd, reinterpret_cast(&address_), sizeof(address_)), - errno); + return {::bind(fd, reinterpret_cast(&address_), sizeof(address_)), errno}; } -std::tuple PipeInstance::connect(int fd) const { +Result PipeInstance::connect(int fd) const { if (abstract_namespace_) { - return std::make_tuple(::connect(fd, reinterpret_cast(&address_), - offsetof(struct sockaddr_un, sun_path) + address_length_), - errno); + return {::connect(fd, reinterpret_cast(&address_), + offsetof(struct sockaddr_un, sun_path) + address_length_), + errno}; } - return std::make_tuple( - ::connect(fd, reinterpret_cast(&address_), sizeof(address_)), errno); + return {::connect(fd, reinterpret_cast(&address_), sizeof(address_)), errno}; } -std::tuple PipeInstance::socket(SocketType type) const { - return socketFromSocketType(type); -} +Result PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); } } // namespace Address } // namespace Network diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 876bdf19cecbd..337bf9c5a22a5 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -55,7 +55,7 @@ class InstanceBase : public Instance { protected: InstanceBase(Type type) : type_(type) {} - std::tuple socketFromSocketType(SocketType type) const; + Result socketFromSocketType(SocketType type) const; std::string friendly_name_; @@ -91,10 +91,10 @@ class Ipv4Instance : public InstanceBase { // Network::Address::Instance bool operator==(const Instance& rhs) const override; - std::tuple bind(int fd) const override; - std::tuple connect(int fd) const override; + Result bind(int fd) const override; + Result connect(int fd) const override; const Ip* ip() const override { return &ip_; } - std::tuple socket(SocketType type) const override; + Result socket(SocketType type) const override; private: struct Ipv4Helper : public Ipv4 { @@ -151,10 +151,10 @@ class Ipv6Instance : public InstanceBase { // Network::Address::Instance bool operator==(const Instance& rhs) const override; - std::tuple bind(int fd) const override; - std::tuple connect(int fd) const override; + Result bind(int fd) const override; + Result connect(int fd) const override; const Ip* ip() const override { return &ip_; } - std::tuple socket(SocketType type) const override; + Result socket(SocketType type) const override; private: struct Ipv6Helper : public Ipv6 { @@ -208,10 +208,10 @@ class PipeInstance : public InstanceBase { // Network::Address::Instance bool operator==(const Instance& rhs) const override; - std::tuple bind(int fd) const override; - std::tuple connect(int fd) const override; + Result bind(int fd) const override; + Result connect(int fd) const override; const Ip* ip() const override { return nullptr; } - std::tuple socket(SocketType type) const override; + Result socket(SocketType type) const override; private: sockaddr_un address_; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 423baa5487478..c775e98e8477e 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -551,10 +551,10 @@ ClientConnectionImpl::ClientConnectionImpl( } if (source_address != nullptr) { - const std::tuple result = source_address->bind(fd()); - if (std::get<0>(result) < 0) { + Address::Result result = source_address->bind(fd()); + if (result.rc < 0) { ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(), - strerror(std::get<1>(result))); + strerror(result.error)); bind_error_ = true; // Set a special error state to ensure asynchronous close to give the owner of the // ConnectionImpl a chance to add callbacks and detect the "disconnect". @@ -568,21 +568,19 @@ ClientConnectionImpl::ClientConnectionImpl( void ClientConnectionImpl::connect() { ENVOY_CONN_LOG(debug, "connecting to {}", *this, socket_->remoteAddress()->asString()); - const std::tuple result = socket_->remoteAddress()->connect(fd()); - const int rc = std::get<0>(result); - const int error = std::get<1>(result); - if (rc == 0) { + Address::Result result = socket_->remoteAddress()->connect(fd()); + if (result.rc == 0) { // write will become ready. ASSERT(connecting_); } else { - ASSERT(rc == -1); - if (error == EINPROGRESS) { + ASSERT(result.rc == -1); + if (result.error == EINPROGRESS) { ASSERT(connecting_); ENVOY_CONN_LOG(debug, "connection in progress", *this); } else { immediate_error_event_ = ConnectionEvent::RemoteClose; connecting_ = false; - ENVOY_CONN_LOG(debug, "immediate connection error: {}", *this, error); + ENVOY_CONN_LOG(debug, "immediate connection error: {}", *this, result.error); // Trigger a write event. This is needed on OSX and seems harmless on Linux. file_event_->activate(Event::FileReadyType::Write); diff --git a/source/common/network/listen_socket_impl.cc b/source/common/network/listen_socket_impl.cc index 8b1e9c3588f9a..e465972edc7fb 100644 --- a/source/common/network/listen_socket_impl.cc +++ b/source/common/network/listen_socket_impl.cc @@ -16,11 +16,11 @@ namespace Envoy { namespace Network { void ListenSocketImpl::doBind() { - const std::tuple result = local_address_->bind(fd_); - if (std::get<0>(result) == -1) { + const Address::Result result = local_address_->bind(fd_); + if (result.rc == -1) { close(); - throw EnvoyException(fmt::format("cannot bind '{}': {}", local_address_->asString(), - strerror(std::get<1>(result)))); + throw EnvoyException( + fmt::format("cannot bind '{}': {}", local_address_->asString(), strerror(result.error))); } if (local_address_->type() == Address::Type::Ip && local_address_->ip()->port() == 0) { // If the port we bind is zero, then the OS will pick a free port for us (assuming there are @@ -39,7 +39,7 @@ void ListenSocketImpl::setListenSocketOptions(const Network::Socket::OptionsShar TcpListenSocket::TcpListenSocket(const Address::InstanceConstSharedPtr& address, const Network::Socket::OptionsSharedPtr& options, bool bind_to_port) - : ListenSocketImpl(std::get<0>(address->socket(Address::SocketType::Stream)), address) { + : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc, address) { RELEASE_ASSERT(fd_ != -1, ""); // TODO(htuch): This might benefit from moving to SocketOptionImpl. @@ -61,7 +61,7 @@ TcpListenSocket::TcpListenSocket(int fd, const Address::InstanceConstSharedPtr& } UdsListenSocket::UdsListenSocket(const Address::InstanceConstSharedPtr& address) - : ListenSocketImpl(std::get<0>(address->socket(Address::SocketType::Stream)), address) { + : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc, address) { RELEASE_ASSERT(fd_ != -1, ""); doBind(); } diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 06a630f6c7acb..3860b8793d275 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -137,8 +137,8 @@ class AcceptedSocketImpl : public ConnectionSocketImpl { class ClientSocketImpl : public ConnectionSocketImpl { public: ClientSocketImpl(const Address::InstanceConstSharedPtr& remote_address) - : ConnectionSocketImpl(std::get<0>(remote_address->socket(Address::SocketType::Stream)), - nullptr, remote_address) {} + : ConnectionSocketImpl(remote_address->socket(Address::SocketType::Stream).rc, nullptr, + remote_address) {} }; } // namespace Network diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 7acd80deffd5c..4290a2ea13eaf 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -20,10 +20,10 @@ namespace Common { namespace Statsd { Writer::Writer(Network::Address::InstanceConstSharedPtr address) { - fd_ = std::get<0>(address->socket(Network::Address::SocketType::Datagram)); + fd_ = address->socket(Network::Address::SocketType::Datagram).rc; ASSERT(fd_ != -1); - const int rc = std::get<0>(address->connect(fd_)); + const int rc = address->connect(fd_).rc; ASSERT(rc != -1); } diff --git a/test/common/network/addr_family_aware_socket_option_impl_test.cc b/test/common/network/addr_family_aware_socket_option_impl_test.cc index a0cbb0bf4c339..cd44dfbd6b845 100644 --- a/test/common/network/addr_family_aware_socket_option_impl_test.cc +++ b/test/common/network/addr_family_aware_socket_option_impl_test.cc @@ -23,7 +23,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionFailure) { // If a platform supports IPv4 socket option variant for an IPv4 address, it works TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionSuccess) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); + const int fd = address.socket(Address::SocketType::Stream).rc; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -37,7 +37,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionSuccess) { // If a platform doesn't support IPv4 socket option variant for an IPv4 address we fail TEST_F(AddrFamilyAwareSocketOptionImplTest, V4EmptyOptionNames) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); + const int fd = address.socket(Address::SocketType::Stream).rc; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1}; @@ -50,7 +50,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V4EmptyOptionNames) { // If a platform doesn't support IPv4 and IPv6 socket option variants for an IPv4 address, we fail TEST_F(AddrFamilyAwareSocketOptionImplTest, V6EmptyOptionNames) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); + const int fd = address.socket(Address::SocketType::Stream).rc; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1}; @@ -64,7 +64,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6EmptyOptionNames) { // IPv4 varient TEST_F(AddrFamilyAwareSocketOptionImplTest, V4IgnoreV6) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); + const int fd = address.socket(Address::SocketType::Stream).rc; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -78,7 +78,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V4IgnoreV6) { // If a platform suppports IPv6 socket option variant for an IPv6 address it works TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Only) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); + const int fd = address.socket(Address::SocketType::Stream).rc; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -93,7 +93,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Only) { // we apply the IPv4 variant. TEST_F(AddrFamilyAwareSocketOptionImplTest, V6OnlyV4Fallback) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); + const int fd = address.socket(Address::SocketType::Stream).rc; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -108,7 +108,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6OnlyV4Fallback) { // AddrFamilyAwareSocketOptionImpl::setIpSocketOption() works with the IPv6 variant. TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Precedence) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = std::get<0>(address.socket(Address::SocketType::Stream)); + const int fd = address.socket(Address::SocketType::Stream).rc; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index bc5ffdfd41e98..153fdee3e6cfb 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -51,7 +51,7 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl ASSERT_NE(addr_port->ip(), nullptr); // Create a socket on which we'll listen for connections from clients. - const int listen_fd = std::get<0>(addr_port->socket(SocketType::Stream)); + const int listen_fd = addr_port->socket(SocketType::Stream).rc; ASSERT_GE(listen_fd, 0) << addr_port->asString(); ScopedFdCloser closer1(listen_fd); @@ -64,10 +64,9 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl } // Bind the socket to the desired address and port. - const std::tuple result = addr_port->bind(listen_fd); - const int rc = std::get<0>(result); - const int err = std::get<1>(result); - ASSERT_EQ(rc, 0) << addr_port->asString() << "\nerror: " << strerror(err) << "\nerrno: " << err; + const Result result = addr_port->bind(listen_fd); + ASSERT_EQ(result.rc, 0) << addr_port->asString() << "\nerror: " << strerror(result.error) + << "\nerrno: " << result.error; // Do a bare listen syscall. Not bothering to accept connections as that would // require another thread. @@ -75,7 +74,7 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl auto client_connect = [](Address::InstanceConstSharedPtr addr_port) { // Create a client socket and connect to the server. - const int client_fd = std::get<0>(addr_port->socket(SocketType::Stream)); + const int client_fd = addr_port->socket(SocketType::Stream).rc; ASSERT_GE(client_fd, 0) << addr_port->asString(); ScopedFdCloser closer2(client_fd); @@ -86,10 +85,9 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl makeFdBlocking(client_fd); // Connect to the server. - const std::tuple result = addr_port->connect(client_fd); - const int rc = std::get<0>(result); - const int err = std::get<1>(result); - ASSERT_EQ(rc, 0) << addr_port->asString() << "\nerror: " << strerror(err) << "\nerrno: " << err; + const Result result = addr_port->connect(client_fd); + ASSERT_EQ(result.rc, 0) << addr_port->asString() << "\nerror: " << strerror(result.error) + << "\nerrno: " << result.error; }; client_connect(addr_port); @@ -312,14 +310,13 @@ TEST(PipeInstanceTest, BadAddress) { TEST(PipeInstanceTest, UnlinksExistingFile) { const auto bind_uds_socket = [](const std::string& path) { PipeInstance address(path); - const int listen_fd = std::get<0>(address.socket(SocketType::Stream)); + const int listen_fd = address.socket(SocketType::Stream).rc; ASSERT_GE(listen_fd, 0) << address.asString(); ScopedFdCloser closer(listen_fd); - const std::tuple result = address.bind(listen_fd); - const int rc = std::get<0>(result); - const int err = std::get<1>(result); - ASSERT_EQ(rc, 0) << address.asString() << "\nerror: " << strerror(err) << "\nerrno: " << err; + const Result result = address.bind(listen_fd); + ASSERT_EQ(result.rc, 0) << address.asString() << "\nerror: " << strerror(result.error) + << "\nerrno: " << result.error; }; const std::string path = TestEnvironment::unixDomainSocketPath("UnlinksExistingFile.sock"); diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 73e22ab0ee35c..1f02d275b6a00 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -359,12 +359,10 @@ class CustomInstance : public Address::Instance { } const std::string& asString() const override { return antagonistic_name_; } const std::string& logicalName() const override { return antagonistic_name_; } - std::tuple bind(int fd) const override { return instance_.bind(fd); } - std::tuple connect(int fd) const override { return instance_.connect(fd); } + Address::Result bind(int fd) const override { return instance_.bind(fd); } + Address::Result connect(int fd) const override { return instance_.connect(fd); } const Address::Ip* ip() const override { return instance_.ip(); } - std::tuple socket(Address::SocketType type) const override { - return instance_.socket(type); - } + Address::Result socket(Address::SocketType type) const override { return instance_.socket(type); } Address::Type type() const override { return instance_.type(); } private: diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 9d1fc72de2733..4cefe316ef711 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -409,10 +409,10 @@ class MockResolvedAddress : public Address::Instance { return asString() == other.asString(); } - MOCK_CONST_METHOD1(bind, std::tuple(int)); - MOCK_CONST_METHOD1(connect, std::tuple(int)); + MOCK_CONST_METHOD1(bind, Address::Result(int)); + MOCK_CONST_METHOD1(connect, Address::Result(int)); MOCK_CONST_METHOD0(ip, Address::Ip*()); - MOCK_CONST_METHOD1(socket, std::tuple(Address::SocketType)); + MOCK_CONST_METHOD1(socket, Address::Result(Address::SocketType)); MOCK_CONST_METHOD0(type, Address::Type()); const std::string& asString() const override { return physical_; } diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index d420c54136ac0..7c1f751ff7558 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -26,13 +26,11 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(Address::InstanceConstShared << (addr_port == nullptr ? "nullptr" : addr_port->asString()); return nullptr; } - std::tuple result = addr_port->socket(type); - const int fd = std::get<0>(result); - int err; + Network::Address::Result result = addr_port->socket(type); + const int fd = result.rc; if (fd < 0) { - err = std::get<1>(result); ADD_FAILURE() << "socket failed for '" << addr_port->asString() - << "' with error: " << strerror(err) << " (" << err << ")"; + << "' with error: " << strerror(result.error) << " (" << result.error << ")"; return nullptr; } ScopedFdCloser closer(fd); @@ -40,15 +38,14 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(Address::InstanceConstShared // However, because we're going to use the address while checking if it is available, we'll need // to set REUSEADDR on listener sockets created by tests using an address validated by this means. result = addr_port->bind(fd); - int rc = std::get<0>(result); + int err; const char* failing_fn = nullptr; - if (rc != 0) { - err = std::get<1>(result); + if (result.rc != 0) { + err = result.error; failing_fn = "bind"; } else if (type == Address::SocketType::Stream) { // Try listening on the port also, if the type is TCP. - rc = ::listen(fd, 1); - if (rc != 0) { + if (::listen(fd, 1) != 0) { err = errno; failing_fn = "listen"; } @@ -151,12 +148,12 @@ Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, bool supportsIpVersion(const Address::IpVersion version) { Address::InstanceConstSharedPtr addr = getCanonicalLoopbackAddress(version); - const int fd = std::get<0>(addr->socket(Address::SocketType::Stream)); + const int fd = addr->socket(Address::SocketType::Stream).rc; if (fd < 0) { // Socket creation failed. return false; } - if (0 != std::get<0>(addr->bind(fd))) { + if (0 != addr->bind(fd).rc) { // Socket bind failed. RELEASE_ASSERT(::close(fd) == 0, ""); return false; @@ -169,16 +166,16 @@ std::pair bindFreeLoopbackPort(Address::Ip Address::SocketType type) { Address::InstanceConstSharedPtr addr = getCanonicalLoopbackAddress(version); const char* failing_fn = nullptr; - std::tuple result = addr->socket(type); - const int fd = std::get<0>(result); + Network::Address::Result result = addr->socket(type); + const int fd = result.rc; int err; if (fd < 0) { - err = std::get<1>(result); + err = result.error; failing_fn = "socket"; } else { result = addr->bind(fd); - if (0 != std::get<0>(result)) { - err = std::get<1>(result); + if (0 != result.rc) { + err = result.error; failing_fn = "bind"; } else { return std::make_pair(Address::addressFromFd(fd), fd); From bb2841e38fab71fdfb897f94a23ce2b8896874ab Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Tue, 24 Jul 2018 09:12:57 -0700 Subject: [PATCH 4/7] Address review comments * Fixes struct styling * Moves address/Result to os_sys_calls/SysCallResult Signed-off-by: Venil Noronha --- include/envoy/api/os_sys_calls.h | 16 ++++++++ include/envoy/network/BUILD | 1 + include/envoy/network/address.h | 37 ++++++------------- source/common/network/address_impl.cc | 28 ++++++++------ source/common/network/address_impl.h | 20 +++++----- source/common/network/connection_impl.cc | 16 ++++---- source/common/network/listen_socket_impl.cc | 10 ++--- source/common/network/listen_socket_impl.h | 2 +- .../stat_sinks/common/statsd/statsd.cc | 4 +- ...dr_family_aware_socket_option_impl_test.cc | 14 +++---- test/common/network/address_impl_test.cc | 24 ++++++------ test/common/network/dns_impl_test.cc | 8 ++-- test/mocks/network/mocks.h | 6 +-- test/test_common/network_utility.cc | 24 ++++++------ 14 files changed, 109 insertions(+), 101 deletions(-) diff --git a/include/envoy/api/os_sys_calls.h b/include/envoy/api/os_sys_calls.h index 648816a76b061..2d13094b4e7d6 100644 --- a/include/envoy/api/os_sys_calls.h +++ b/include/envoy/api/os_sys_calls.h @@ -14,6 +14,22 @@ namespace Envoy { namespace Api { +/** + * SysCallResult holds the rc and errno values resulting from a system call. + */ +struct SysCallResult { + + /** + * The return code from the system call. + */ + int rc_; + + /** + * The errno value as captured after the system call. + */ + int errno_; +}; + class OsSysCalls { public: virtual ~OsSysCalls() {} diff --git a/include/envoy/network/BUILD b/include/envoy/network/BUILD index 31781824fa098..5f7e0ebd06acc 100644 --- a/include/envoy/network/BUILD +++ b/include/envoy/network/BUILD @@ -11,6 +11,7 @@ envoy_package() envoy_cc_library( name = "address_interface", hdrs = ["address.h"], + deps = ["//include/envoy/api:os_sys_calls_interface"], ) envoy_cc_library( diff --git a/include/envoy/network/address.h b/include/envoy/network/address.h index 3692da5bd9866..c04491ac0ec9e 100644 --- a/include/envoy/network/address.h +++ b/include/envoy/network/address.h @@ -8,6 +8,7 @@ #include #include +#include "envoy/api/os_sys_calls.h" #include "envoy/common/pure.h" #include "absl/numeric/int128.h" @@ -16,22 +17,6 @@ namespace Envoy { namespace Network { namespace Address { -/** - * Result holds the rc and errno values resulting from a system call. - */ -struct Result { - - /** - * The return code from the system call. - */ - int rc; - - /** - * The errno value as captured after the system call. - */ - int error; -}; - /** * Interface for an Ipv4 address. */ @@ -144,19 +129,19 @@ class Instance { * Bind a socket to this address. The socket should have been created with a call to socket() on * an Instance of the same address family. * @param fd supplies the platform socket handle. - * @return a Result with rc = 0 for success and rc = -1 for failure. If the call is successful, - * the error value shouldn't be used. + * @return a Api::SysCallResult with rc_ = 0 for success and rc_ = -1 for failure. If the call + * is successful, errno_ shouldn't be used. */ - virtual Result bind(int fd) const PURE; + virtual Api::SysCallResult bind(int fd) const PURE; /** * Connect a socket to this address. The socket should have been created with a call to socket() * on this object. * @param fd supplies the platform socket handle. - * @return a Result with rc = 0 for success and rc = -1 for failure. If the call is successful, - * the error value shouldn't be used. + * @return a Api::SysCallResult with rc_ = 0 for success and rc_ = -1 for failure. If the call + * is successful, errno_ shouldn't be used. */ - virtual Result connect(int fd) const PURE; + virtual Api::SysCallResult connect(int fd) const PURE; /** * @return the IP address information IFF type() == Type::Ip, otherwise nullptr. @@ -166,11 +151,11 @@ class Instance { /** * Create a socket for this address. * @param type supplies the socket type to create. - * @return a Result with rc = fd for success and rc = -1 for failure, where fd represents the - * file descriptor naming the socket. If a valid file descriptor is returned, the error value - * shouldn't be used. + * @return a Api::SysCallResult with rc_ = fd for success and rc_ = -1 for failure, where fd + * represents the file descriptor naming the socket. If a valid file descriptor is returned, + * errno_ shouldn't be used. */ - virtual Result socket(SocketType type) const PURE; + virtual Api::SysCallResult socket(SocketType type) const PURE; /** * @return the type of address. diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 232fbfbc6f7a8..a69ad644936a7 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -133,7 +133,7 @@ InstanceConstSharedPtr peerAddressFromFd(int fd) { return addressFromSockAddr(ss, ss_len); } -Result InstanceBase::socketFromSocketType(SocketType socketType) const { +Api::SysCallResult InstanceBase::socketFromSocketType(SocketType socketType) const { #if defined(__APPLE__) int flags = 0; #else @@ -212,19 +212,21 @@ bool Ipv4Instance::operator==(const Instance& rhs) const { (ip_.port() == rhs_casted->ip_.port())); } -Result Ipv4Instance::bind(int fd) const { +Api::SysCallResult Ipv4Instance::bind(int fd) const { return {::bind(fd, reinterpret_cast(&ip_.ipv4_.address_), sizeof(ip_.ipv4_.address_)), errno}; } -Result Ipv4Instance::connect(int fd) const { +Api::SysCallResult Ipv4Instance::connect(int fd) const { return {::connect(fd, reinterpret_cast(&ip_.ipv4_.address_), sizeof(ip_.ipv4_.address_)), errno}; } -Result Ipv4Instance::socket(SocketType type) const { return socketFromSocketType(type); } +Api::SysCallResult Ipv4Instance::socket(SocketType type) const { + return socketFromSocketType(type); +} absl::uint128 Ipv6Instance::Ipv6Helper::address() const { absl::uint128 result{0}; @@ -277,23 +279,23 @@ bool Ipv6Instance::operator==(const Instance& rhs) const { (ip_.port() == rhs_casted->ip_.port())); } -Result Ipv6Instance::bind(int fd) const { +Api::SysCallResult Ipv6Instance::bind(int fd) const { return {::bind(fd, reinterpret_cast(&ip_.ipv6_.address_), sizeof(ip_.ipv6_.address_)), errno}; } -Result Ipv6Instance::connect(int fd) const { +Api::SysCallResult Ipv6Instance::connect(int fd) const { return {::connect(fd, reinterpret_cast(&ip_.ipv6_.address_), sizeof(ip_.ipv6_.address_)), errno}; } -Result Ipv6Instance::socket(SocketType type) const { - const Result result = socketFromSocketType(type); +Api::SysCallResult Ipv6Instance::socket(SocketType type) const { + const Api::SysCallResult result = socketFromSocketType(type); // Setting IPV6_V6ONLY resticts the IPv6 socket to IPv6 connections only. const int v6only = ip_.v6only_; - RELEASE_ASSERT(::setsockopt(result.rc, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, + RELEASE_ASSERT(::setsockopt(result.rc_, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, ""); return result; } @@ -337,7 +339,7 @@ PipeInstance::PipeInstance(const std::string& pipe_path) : InstanceBase(Type::Pi bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); } -Result PipeInstance::bind(int fd) const { +Api::SysCallResult PipeInstance::bind(int fd) const { if (abstract_namespace_) { return {::bind(fd, reinterpret_cast(&address_), offsetof(struct sockaddr_un, sun_path) + address_length_), @@ -350,7 +352,7 @@ Result PipeInstance::bind(int fd) const { return {::bind(fd, reinterpret_cast(&address_), sizeof(address_)), errno}; } -Result PipeInstance::connect(int fd) const { +Api::SysCallResult PipeInstance::connect(int fd) const { if (abstract_namespace_) { return {::connect(fd, reinterpret_cast(&address_), offsetof(struct sockaddr_un, sun_path) + address_length_), @@ -359,7 +361,9 @@ Result PipeInstance::connect(int fd) const { return {::connect(fd, reinterpret_cast(&address_), sizeof(address_)), errno}; } -Result PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); } +Api::SysCallResult PipeInstance::socket(SocketType type) const { + return socketFromSocketType(type); +} } // namespace Address } // namespace Network diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 337bf9c5a22a5..3d363fe2d89b0 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -55,7 +55,7 @@ class InstanceBase : public Instance { protected: InstanceBase(Type type) : type_(type) {} - Result socketFromSocketType(SocketType type) const; + Api::SysCallResult socketFromSocketType(SocketType type) const; std::string friendly_name_; @@ -91,10 +91,10 @@ class Ipv4Instance : public InstanceBase { // Network::Address::Instance bool operator==(const Instance& rhs) const override; - Result bind(int fd) const override; - Result connect(int fd) const override; + Api::SysCallResult bind(int fd) const override; + Api::SysCallResult connect(int fd) const override; const Ip* ip() const override { return &ip_; } - Result socket(SocketType type) const override; + Api::SysCallResult socket(SocketType type) const override; private: struct Ipv4Helper : public Ipv4 { @@ -151,10 +151,10 @@ class Ipv6Instance : public InstanceBase { // Network::Address::Instance bool operator==(const Instance& rhs) const override; - Result bind(int fd) const override; - Result connect(int fd) const override; + Api::SysCallResult bind(int fd) const override; + Api::SysCallResult connect(int fd) const override; const Ip* ip() const override { return &ip_; } - Result socket(SocketType type) const override; + Api::SysCallResult socket(SocketType type) const override; private: struct Ipv6Helper : public Ipv6 { @@ -208,10 +208,10 @@ class PipeInstance : public InstanceBase { // Network::Address::Instance bool operator==(const Instance& rhs) const override; - Result bind(int fd) const override; - Result connect(int fd) const override; + Api::SysCallResult bind(int fd) const override; + Api::SysCallResult connect(int fd) const override; const Ip* ip() const override { return nullptr; } - Result socket(SocketType type) const override; + Api::SysCallResult socket(SocketType type) const override; private: sockaddr_un address_; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index c775e98e8477e..ea319fe005746 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -551,10 +551,10 @@ ClientConnectionImpl::ClientConnectionImpl( } if (source_address != nullptr) { - Address::Result result = source_address->bind(fd()); - if (result.rc < 0) { + Api::SysCallResult result = source_address->bind(fd()); + if (result.rc_ < 0) { ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(), - strerror(result.error)); + strerror(result.errno_)); bind_error_ = true; // Set a special error state to ensure asynchronous close to give the owner of the // ConnectionImpl a chance to add callbacks and detect the "disconnect". @@ -568,19 +568,19 @@ ClientConnectionImpl::ClientConnectionImpl( void ClientConnectionImpl::connect() { ENVOY_CONN_LOG(debug, "connecting to {}", *this, socket_->remoteAddress()->asString()); - Address::Result result = socket_->remoteAddress()->connect(fd()); - if (result.rc == 0) { + Api::SysCallResult result = socket_->remoteAddress()->connect(fd()); + if (result.rc_ == 0) { // write will become ready. ASSERT(connecting_); } else { - ASSERT(result.rc == -1); - if (result.error == EINPROGRESS) { + ASSERT(result.rc_ == -1); + if (result.errno_ == EINPROGRESS) { ASSERT(connecting_); ENVOY_CONN_LOG(debug, "connection in progress", *this); } else { immediate_error_event_ = ConnectionEvent::RemoteClose; connecting_ = false; - ENVOY_CONN_LOG(debug, "immediate connection error: {}", *this, result.error); + ENVOY_CONN_LOG(debug, "immediate connection error: {}", *this, result.errno_); // Trigger a write event. This is needed on OSX and seems harmless on Linux. file_event_->activate(Event::FileReadyType::Write); diff --git a/source/common/network/listen_socket_impl.cc b/source/common/network/listen_socket_impl.cc index e465972edc7fb..7649f02e16a13 100644 --- a/source/common/network/listen_socket_impl.cc +++ b/source/common/network/listen_socket_impl.cc @@ -16,11 +16,11 @@ namespace Envoy { namespace Network { void ListenSocketImpl::doBind() { - const Address::Result result = local_address_->bind(fd_); - if (result.rc == -1) { + const Api::SysCallResult result = local_address_->bind(fd_); + if (result.rc_ == -1) { close(); throw EnvoyException( - fmt::format("cannot bind '{}': {}", local_address_->asString(), strerror(result.error))); + fmt::format("cannot bind '{}': {}", local_address_->asString(), strerror(result.errno_))); } if (local_address_->type() == Address::Type::Ip && local_address_->ip()->port() == 0) { // If the port we bind is zero, then the OS will pick a free port for us (assuming there are @@ -39,7 +39,7 @@ void ListenSocketImpl::setListenSocketOptions(const Network::Socket::OptionsShar TcpListenSocket::TcpListenSocket(const Address::InstanceConstSharedPtr& address, const Network::Socket::OptionsSharedPtr& options, bool bind_to_port) - : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc, address) { + : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc_, address) { RELEASE_ASSERT(fd_ != -1, ""); // TODO(htuch): This might benefit from moving to SocketOptionImpl. @@ -61,7 +61,7 @@ TcpListenSocket::TcpListenSocket(int fd, const Address::InstanceConstSharedPtr& } UdsListenSocket::UdsListenSocket(const Address::InstanceConstSharedPtr& address) - : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc, address) { + : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc_, address) { RELEASE_ASSERT(fd_ != -1, ""); doBind(); } diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 3860b8793d275..4124b7a92dbfb 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -137,7 +137,7 @@ class AcceptedSocketImpl : public ConnectionSocketImpl { class ClientSocketImpl : public ConnectionSocketImpl { public: ClientSocketImpl(const Address::InstanceConstSharedPtr& remote_address) - : ConnectionSocketImpl(remote_address->socket(Address::SocketType::Stream).rc, nullptr, + : ConnectionSocketImpl(remote_address->socket(Address::SocketType::Stream).rc_, nullptr, remote_address) {} }; diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 4290a2ea13eaf..bf7976efd1c90 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -20,10 +20,10 @@ namespace Common { namespace Statsd { Writer::Writer(Network::Address::InstanceConstSharedPtr address) { - fd_ = address->socket(Network::Address::SocketType::Datagram).rc; + fd_ = address->socket(Network::Address::SocketType::Datagram).rc_; ASSERT(fd_ != -1); - const int rc = address->connect(fd_).rc; + const int rc = address->connect(fd_).rc_; ASSERT(rc != -1); } diff --git a/test/common/network/addr_family_aware_socket_option_impl_test.cc b/test/common/network/addr_family_aware_socket_option_impl_test.cc index cd44dfbd6b845..c8028c49f0040 100644 --- a/test/common/network/addr_family_aware_socket_option_impl_test.cc +++ b/test/common/network/addr_family_aware_socket_option_impl_test.cc @@ -23,7 +23,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionFailure) { // If a platform supports IPv4 socket option variant for an IPv4 address, it works TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionSuccess) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc; + const int fd = address.socket(Address::SocketType::Stream).rc_; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -37,7 +37,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionSuccess) { // If a platform doesn't support IPv4 socket option variant for an IPv4 address we fail TEST_F(AddrFamilyAwareSocketOptionImplTest, V4EmptyOptionNames) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc; + const int fd = address.socket(Address::SocketType::Stream).rc_; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1}; @@ -50,7 +50,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V4EmptyOptionNames) { // If a platform doesn't support IPv4 and IPv6 socket option variants for an IPv4 address, we fail TEST_F(AddrFamilyAwareSocketOptionImplTest, V6EmptyOptionNames) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc; + const int fd = address.socket(Address::SocketType::Stream).rc_; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1}; @@ -64,7 +64,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6EmptyOptionNames) { // IPv4 varient TEST_F(AddrFamilyAwareSocketOptionImplTest, V4IgnoreV6) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc; + const int fd = address.socket(Address::SocketType::Stream).rc_; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -78,7 +78,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V4IgnoreV6) { // If a platform suppports IPv6 socket option variant for an IPv6 address it works TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Only) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc; + const int fd = address.socket(Address::SocketType::Stream).rc_; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -93,7 +93,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Only) { // we apply the IPv4 variant. TEST_F(AddrFamilyAwareSocketOptionImplTest, V6OnlyV4Fallback) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc; + const int fd = address.socket(Address::SocketType::Stream).rc_; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -108,7 +108,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6OnlyV4Fallback) { // AddrFamilyAwareSocketOptionImpl::setIpSocketOption() works with the IPv6 variant. TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Precedence) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc; + const int fd = address.socket(Address::SocketType::Stream).rc_; EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index 153fdee3e6cfb..9ebd2e9b84732 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -51,7 +51,7 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl ASSERT_NE(addr_port->ip(), nullptr); // Create a socket on which we'll listen for connections from clients. - const int listen_fd = addr_port->socket(SocketType::Stream).rc; + const int listen_fd = addr_port->socket(SocketType::Stream).rc_; ASSERT_GE(listen_fd, 0) << addr_port->asString(); ScopedFdCloser closer1(listen_fd); @@ -64,9 +64,9 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl } // Bind the socket to the desired address and port. - const Result result = addr_port->bind(listen_fd); - ASSERT_EQ(result.rc, 0) << addr_port->asString() << "\nerror: " << strerror(result.error) - << "\nerrno: " << result.error; + const Api::SysCallResult result = addr_port->bind(listen_fd); + ASSERT_EQ(result.rc_, 0) << addr_port->asString() << "\nerror: " << strerror(result.errno_) + << "\nerrno: " << result.errno_; // Do a bare listen syscall. Not bothering to accept connections as that would // require another thread. @@ -74,7 +74,7 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl auto client_connect = [](Address::InstanceConstSharedPtr addr_port) { // Create a client socket and connect to the server. - const int client_fd = addr_port->socket(SocketType::Stream).rc; + const int client_fd = addr_port->socket(SocketType::Stream).rc_; ASSERT_GE(client_fd, 0) << addr_port->asString(); ScopedFdCloser closer2(client_fd); @@ -85,9 +85,9 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl makeFdBlocking(client_fd); // Connect to the server. - const Result result = addr_port->connect(client_fd); - ASSERT_EQ(result.rc, 0) << addr_port->asString() << "\nerror: " << strerror(result.error) - << "\nerrno: " << result.error; + const Api::SysCallResult result = addr_port->connect(client_fd); + ASSERT_EQ(result.rc_, 0) << addr_port->asString() << "\nerror: " << strerror(result.errno_) + << "\nerrno: " << result.errno_; }; client_connect(addr_port); @@ -310,13 +310,13 @@ TEST(PipeInstanceTest, BadAddress) { TEST(PipeInstanceTest, UnlinksExistingFile) { const auto bind_uds_socket = [](const std::string& path) { PipeInstance address(path); - const int listen_fd = address.socket(SocketType::Stream).rc; + const int listen_fd = address.socket(SocketType::Stream).rc_; ASSERT_GE(listen_fd, 0) << address.asString(); ScopedFdCloser closer(listen_fd); - const Result result = address.bind(listen_fd); - ASSERT_EQ(result.rc, 0) << address.asString() << "\nerror: " << strerror(result.error) - << "\nerrno: " << result.error; + const Api::SysCallResult result = address.bind(listen_fd); + ASSERT_EQ(result.rc_, 0) << address.asString() << "\nerror: " << strerror(result.errno_) + << "\nerrno: " << result.errno_; }; const std::string path = TestEnvironment::unixDomainSocketPath("UnlinksExistingFile.sock"); diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 1f02d275b6a00..522f02a5723a5 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -359,10 +359,12 @@ class CustomInstance : public Address::Instance { } const std::string& asString() const override { return antagonistic_name_; } const std::string& logicalName() const override { return antagonistic_name_; } - Address::Result bind(int fd) const override { return instance_.bind(fd); } - Address::Result connect(int fd) const override { return instance_.connect(fd); } + Api::SysCallResult bind(int fd) const override { return instance_.bind(fd); } + Api::SysCallResult connect(int fd) const override { return instance_.connect(fd); } const Address::Ip* ip() const override { return instance_.ip(); } - Address::Result socket(Address::SocketType type) const override { return instance_.socket(type); } + Api::SysCallResult socket(Address::SocketType type) const override { + return instance_.socket(type); + } Address::Type type() const override { return instance_.type(); } private: diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 4cefe316ef711..7c4065d23e95f 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -409,10 +409,10 @@ class MockResolvedAddress : public Address::Instance { return asString() == other.asString(); } - MOCK_CONST_METHOD1(bind, Address::Result(int)); - MOCK_CONST_METHOD1(connect, Address::Result(int)); + MOCK_CONST_METHOD1(bind, Api::SysCallResult(int)); + MOCK_CONST_METHOD1(connect, Api::SysCallResult(int)); MOCK_CONST_METHOD0(ip, Address::Ip*()); - MOCK_CONST_METHOD1(socket, Address::Result(Address::SocketType)); + MOCK_CONST_METHOD1(socket, Api::SysCallResult(Address::SocketType)); MOCK_CONST_METHOD0(type, Address::Type()); const std::string& asString() const override { return physical_; } diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index 7c1f751ff7558..9b9476f77ade1 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -26,11 +26,11 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(Address::InstanceConstShared << (addr_port == nullptr ? "nullptr" : addr_port->asString()); return nullptr; } - Network::Address::Result result = addr_port->socket(type); - const int fd = result.rc; + Api::SysCallResult result = addr_port->socket(type); + const int fd = result.rc_; if (fd < 0) { ADD_FAILURE() << "socket failed for '" << addr_port->asString() - << "' with error: " << strerror(result.error) << " (" << result.error << ")"; + << "' with error: " << strerror(result.errno_) << " (" << result.errno_ << ")"; return nullptr; } ScopedFdCloser closer(fd); @@ -40,8 +40,8 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(Address::InstanceConstShared result = addr_port->bind(fd); int err; const char* failing_fn = nullptr; - if (result.rc != 0) { - err = result.error; + if (result.rc_ != 0) { + err = result.errno_; failing_fn = "bind"; } else if (type == Address::SocketType::Stream) { // Try listening on the port also, if the type is TCP. @@ -148,12 +148,12 @@ Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, bool supportsIpVersion(const Address::IpVersion version) { Address::InstanceConstSharedPtr addr = getCanonicalLoopbackAddress(version); - const int fd = addr->socket(Address::SocketType::Stream).rc; + const int fd = addr->socket(Address::SocketType::Stream).rc_; if (fd < 0) { // Socket creation failed. return false; } - if (0 != addr->bind(fd).rc) { + if (0 != addr->bind(fd).rc_) { // Socket bind failed. RELEASE_ASSERT(::close(fd) == 0, ""); return false; @@ -166,16 +166,16 @@ std::pair bindFreeLoopbackPort(Address::Ip Address::SocketType type) { Address::InstanceConstSharedPtr addr = getCanonicalLoopbackAddress(version); const char* failing_fn = nullptr; - Network::Address::Result result = addr->socket(type); - const int fd = result.rc; + Api::SysCallResult result = addr->socket(type); + const int fd = result.rc_; int err; if (fd < 0) { - err = result.error; + err = result.errno_; failing_fn = "socket"; } else { result = addr->bind(fd); - if (0 != result.rc) { - err = result.error; + if (0 != result.rc_) { + err = result.errno_; failing_fn = "bind"; } else { return std::make_pair(Address::addressFromFd(fd), fd); From 7e05d552abd2fc629137b9e371ef1038a1380e36 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Tue, 24 Jul 2018 11:02:45 -0700 Subject: [PATCH 5/7] Make syscalls explicit prior to SysCallResult init Signed-off-by: Venil Noronha --- source/common/network/address_impl.cc | 42 ++++++++++++++------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index a69ad644936a7..2154a4e72f709 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -213,15 +213,15 @@ bool Ipv4Instance::operator==(const Instance& rhs) const { } Api::SysCallResult Ipv4Instance::bind(int fd) const { - return {::bind(fd, reinterpret_cast(&ip_.ipv4_.address_), - sizeof(ip_.ipv4_.address_)), - errno}; + const int rc = ::bind(fd, reinterpret_cast(&ip_.ipv4_.address_), + sizeof(ip_.ipv4_.address_)); + return {rc, errno}; } Api::SysCallResult Ipv4Instance::connect(int fd) const { - return {::connect(fd, reinterpret_cast(&ip_.ipv4_.address_), - sizeof(ip_.ipv4_.address_)), - errno}; + const int rc = ::connect(fd, reinterpret_cast(&ip_.ipv4_.address_), + sizeof(ip_.ipv4_.address_)); + return {rc, errno}; } Api::SysCallResult Ipv4Instance::socket(SocketType type) const { @@ -280,15 +280,15 @@ bool Ipv6Instance::operator==(const Instance& rhs) const { } Api::SysCallResult Ipv6Instance::bind(int fd) const { - return {::bind(fd, reinterpret_cast(&ip_.ipv6_.address_), - sizeof(ip_.ipv6_.address_)), - errno}; + const int rc = ::bind(fd, reinterpret_cast(&ip_.ipv6_.address_), + sizeof(ip_.ipv6_.address_)); + return {rc, errno}; } Api::SysCallResult Ipv6Instance::connect(int fd) const { - return {::connect(fd, reinterpret_cast(&ip_.ipv6_.address_), - sizeof(ip_.ipv6_.address_)), - errno}; + const int rc = ::connect(fd, reinterpret_cast(&ip_.ipv6_.address_), + sizeof(ip_.ipv6_.address_)); + return {rc, errno}; } Api::SysCallResult Ipv6Instance::socket(SocketType type) const { @@ -341,24 +341,26 @@ bool PipeInstance::operator==(const Instance& rhs) const { return asString() == Api::SysCallResult PipeInstance::bind(int fd) const { if (abstract_namespace_) { - return {::bind(fd, reinterpret_cast(&address_), - offsetof(struct sockaddr_un, sun_path) + address_length_), - errno}; + const int rc = ::bind(fd, reinterpret_cast(&address_), + offsetof(struct sockaddr_un, sun_path) + address_length_); + return {rc, errno}; } // Try to unlink an existing filesystem object at the requested path. Ignore // errors -- it's fine if the path doesn't exist, and if it exists but can't // be unlinked then `::bind()` will generate a reasonable errno. unlink(address_.sun_path); - return {::bind(fd, reinterpret_cast(&address_), sizeof(address_)), errno}; + const int rc = ::bind(fd, reinterpret_cast(&address_), sizeof(address_)); + return {rc, errno}; } Api::SysCallResult PipeInstance::connect(int fd) const { if (abstract_namespace_) { - return {::connect(fd, reinterpret_cast(&address_), - offsetof(struct sockaddr_un, sun_path) + address_length_), - errno}; + const int rc = ::connect(fd, reinterpret_cast(&address_), + offsetof(struct sockaddr_un, sun_path) + address_length_); + return {rc, errno}; } - return {::connect(fd, reinterpret_cast(&address_), sizeof(address_)), errno}; + const int rc = ::connect(fd, reinterpret_cast(&address_), sizeof(address_)); + return {rc, errno}; } Api::SysCallResult PipeInstance::socket(SocketType type) const { From dc0c613e7de0cde6a9ac54fc15bfbdd1afe44dc0 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Tue, 24 Jul 2018 14:00:56 -0700 Subject: [PATCH 6/7] Revert address's socket routine to return an int RELEASE_ASSERT statements in the socket implementation ensure that the routine returns only on success. This commit does the following: * Revert function return type to int * Update socket documentation * Remove redundant error handling Signed-off-by: Venil Noronha --- include/envoy/network/address.h | 7 ++- source/common/network/address_impl.cc | 21 ++++----- source/common/network/address_impl.h | 8 ++-- source/common/network/listen_socket_impl.cc | 4 +- source/common/network/listen_socket_impl.h | 2 +- .../stat_sinks/common/statsd/statsd.cc | 2 +- ...dr_family_aware_socket_option_impl_test.cc | 14 +++--- test/common/network/address_impl_test.cc | 6 +-- test/common/network/dns_impl_test.cc | 4 +- test/mocks/network/mocks.h | 2 +- test/test_common/network_utility.cc | 45 +++++-------------- 11 files changed, 42 insertions(+), 73 deletions(-) diff --git a/include/envoy/network/address.h b/include/envoy/network/address.h index c04491ac0ec9e..13004a1f19b03 100644 --- a/include/envoy/network/address.h +++ b/include/envoy/network/address.h @@ -151,11 +151,10 @@ class Instance { /** * Create a socket for this address. * @param type supplies the socket type to create. - * @return a Api::SysCallResult with rc_ = fd for success and rc_ = -1 for failure, where fd - * represents the file descriptor naming the socket. If a valid file descriptor is returned, - * errno_ shouldn't be used. + * @return the file descriptor naming the socket. In case of a failure, the program would be + * aborted. */ - virtual Api::SysCallResult socket(SocketType type) const PURE; + virtual int socket(SocketType type) const PURE; /** * @return the type of address. diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index 2154a4e72f709..25a83d8583345 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -133,7 +133,7 @@ InstanceConstSharedPtr peerAddressFromFd(int fd) { return addressFromSockAddr(ss, ss_len); } -Api::SysCallResult InstanceBase::socketFromSocketType(SocketType socketType) const { +int InstanceBase::socketFromSocketType(SocketType socketType) const { #if defined(__APPLE__) int flags = 0; #else @@ -168,7 +168,7 @@ Api::SysCallResult InstanceBase::socketFromSocketType(SocketType socketType) con RELEASE_ASSERT(fcntl(fd, F_SETFL, O_NONBLOCK) != -1, ""); #endif - return {fd, errno}; + return fd; } Ipv4Instance::Ipv4Instance(const sockaddr_in* address) : InstanceBase(Type::Ip) { @@ -224,9 +224,7 @@ Api::SysCallResult Ipv4Instance::connect(int fd) const { return {rc, errno}; } -Api::SysCallResult Ipv4Instance::socket(SocketType type) const { - return socketFromSocketType(type); -} +int Ipv4Instance::socket(SocketType type) const { return socketFromSocketType(type); } absl::uint128 Ipv6Instance::Ipv6Helper::address() const { absl::uint128 result{0}; @@ -291,13 +289,12 @@ Api::SysCallResult Ipv6Instance::connect(int fd) const { return {rc, errno}; } -Api::SysCallResult Ipv6Instance::socket(SocketType type) const { - const Api::SysCallResult result = socketFromSocketType(type); +int Ipv6Instance::socket(SocketType type) const { + const int fd = socketFromSocketType(type); // Setting IPV6_V6ONLY resticts the IPv6 socket to IPv6 connections only. const int v6only = ip_.v6only_; - RELEASE_ASSERT(::setsockopt(result.rc_, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, - ""); - return result; + RELEASE_ASSERT(::setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, ""); + return fd; } PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len) @@ -363,9 +360,7 @@ Api::SysCallResult PipeInstance::connect(int fd) const { return {rc, errno}; } -Api::SysCallResult PipeInstance::socket(SocketType type) const { - return socketFromSocketType(type); -} +int PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); } } // namespace Address } // namespace Network diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 3d363fe2d89b0..ffb6e0261ff26 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -55,7 +55,7 @@ class InstanceBase : public Instance { protected: InstanceBase(Type type) : type_(type) {} - Api::SysCallResult socketFromSocketType(SocketType type) const; + int socketFromSocketType(SocketType type) const; std::string friendly_name_; @@ -94,7 +94,7 @@ class Ipv4Instance : public InstanceBase { Api::SysCallResult bind(int fd) const override; Api::SysCallResult connect(int fd) const override; const Ip* ip() const override { return &ip_; } - Api::SysCallResult socket(SocketType type) const override; + int socket(SocketType type) const override; private: struct Ipv4Helper : public Ipv4 { @@ -154,7 +154,7 @@ class Ipv6Instance : public InstanceBase { Api::SysCallResult bind(int fd) const override; Api::SysCallResult connect(int fd) const override; const Ip* ip() const override { return &ip_; } - Api::SysCallResult socket(SocketType type) const override; + int socket(SocketType type) const override; private: struct Ipv6Helper : public Ipv6 { @@ -211,7 +211,7 @@ class PipeInstance : public InstanceBase { Api::SysCallResult bind(int fd) const override; Api::SysCallResult connect(int fd) const override; const Ip* ip() const override { return nullptr; } - Api::SysCallResult socket(SocketType type) const override; + int socket(SocketType type) const override; private: sockaddr_un address_; diff --git a/source/common/network/listen_socket_impl.cc b/source/common/network/listen_socket_impl.cc index 7649f02e16a13..bc7e8a7858d97 100644 --- a/source/common/network/listen_socket_impl.cc +++ b/source/common/network/listen_socket_impl.cc @@ -39,7 +39,7 @@ void ListenSocketImpl::setListenSocketOptions(const Network::Socket::OptionsShar TcpListenSocket::TcpListenSocket(const Address::InstanceConstSharedPtr& address, const Network::Socket::OptionsSharedPtr& options, bool bind_to_port) - : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc_, address) { + : ListenSocketImpl(address->socket(Address::SocketType::Stream), address) { RELEASE_ASSERT(fd_ != -1, ""); // TODO(htuch): This might benefit from moving to SocketOptionImpl. @@ -61,7 +61,7 @@ TcpListenSocket::TcpListenSocket(int fd, const Address::InstanceConstSharedPtr& } UdsListenSocket::UdsListenSocket(const Address::InstanceConstSharedPtr& address) - : ListenSocketImpl(address->socket(Address::SocketType::Stream).rc_, address) { + : ListenSocketImpl(address->socket(Address::SocketType::Stream), address) { RELEASE_ASSERT(fd_ != -1, ""); doBind(); } diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 4124b7a92dbfb..ef512226d0514 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -137,7 +137,7 @@ class AcceptedSocketImpl : public ConnectionSocketImpl { class ClientSocketImpl : public ConnectionSocketImpl { public: ClientSocketImpl(const Address::InstanceConstSharedPtr& remote_address) - : ConnectionSocketImpl(remote_address->socket(Address::SocketType::Stream).rc_, nullptr, + : ConnectionSocketImpl(remote_address->socket(Address::SocketType::Stream), nullptr, remote_address) {} }; diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index bf7976efd1c90..e033366a7e3e2 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -20,7 +20,7 @@ namespace Common { namespace Statsd { Writer::Writer(Network::Address::InstanceConstSharedPtr address) { - fd_ = address->socket(Network::Address::SocketType::Datagram).rc_; + fd_ = address->socket(Network::Address::SocketType::Datagram); ASSERT(fd_ != -1); const int rc = address->connect(fd_).rc_; diff --git a/test/common/network/addr_family_aware_socket_option_impl_test.cc b/test/common/network/addr_family_aware_socket_option_impl_test.cc index c8028c49f0040..3c6c16a0d9c9f 100644 --- a/test/common/network/addr_family_aware_socket_option_impl_test.cc +++ b/test/common/network/addr_family_aware_socket_option_impl_test.cc @@ -23,7 +23,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionFailure) { // If a platform supports IPv4 socket option variant for an IPv4 address, it works TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionSuccess) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc_; + const int fd = address.socket(Address::SocketType::Stream); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -37,7 +37,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, SetOptionSuccess) { // If a platform doesn't support IPv4 socket option variant for an IPv4 address we fail TEST_F(AddrFamilyAwareSocketOptionImplTest, V4EmptyOptionNames) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc_; + const int fd = address.socket(Address::SocketType::Stream); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1}; @@ -50,7 +50,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V4EmptyOptionNames) { // If a platform doesn't support IPv4 and IPv6 socket option variants for an IPv4 address, we fail TEST_F(AddrFamilyAwareSocketOptionImplTest, V6EmptyOptionNames) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc_; + const int fd = address.socket(Address::SocketType::Stream); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{ envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1}; @@ -64,7 +64,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6EmptyOptionNames) { // IPv4 varient TEST_F(AddrFamilyAwareSocketOptionImplTest, V4IgnoreV6) { Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc_; + const int fd = address.socket(Address::SocketType::Stream); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -78,7 +78,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V4IgnoreV6) { // If a platform suppports IPv6 socket option variant for an IPv6 address it works TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Only) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc_; + const int fd = address.socket(Address::SocketType::Stream); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -93,7 +93,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Only) { // we apply the IPv4 variant. TEST_F(AddrFamilyAwareSocketOptionImplTest, V6OnlyV4Fallback) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc_; + const int fd = address.socket(Address::SocketType::Stream); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, @@ -108,7 +108,7 @@ TEST_F(AddrFamilyAwareSocketOptionImplTest, V6OnlyV4Fallback) { // AddrFamilyAwareSocketOptionImpl::setIpSocketOption() works with the IPv6 variant. TEST_F(AddrFamilyAwareSocketOptionImplTest, V6Precedence) { Address::Ipv6Instance address("::1:2:3:4", 5678); - const int fd = address.socket(Address::SocketType::Stream).rc_; + const int fd = address.socket(Address::SocketType::Stream); EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND, diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index 9ebd2e9b84732..839f4437699cd 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -51,7 +51,7 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl ASSERT_NE(addr_port->ip(), nullptr); // Create a socket on which we'll listen for connections from clients. - const int listen_fd = addr_port->socket(SocketType::Stream).rc_; + const int listen_fd = addr_port->socket(SocketType::Stream); ASSERT_GE(listen_fd, 0) << addr_port->asString(); ScopedFdCloser closer1(listen_fd); @@ -74,7 +74,7 @@ void testSocketBindAndConnect(Network::Address::IpVersion ip_version, bool v6onl auto client_connect = [](Address::InstanceConstSharedPtr addr_port) { // Create a client socket and connect to the server. - const int client_fd = addr_port->socket(SocketType::Stream).rc_; + const int client_fd = addr_port->socket(SocketType::Stream); ASSERT_GE(client_fd, 0) << addr_port->asString(); ScopedFdCloser closer2(client_fd); @@ -310,7 +310,7 @@ TEST(PipeInstanceTest, BadAddress) { TEST(PipeInstanceTest, UnlinksExistingFile) { const auto bind_uds_socket = [](const std::string& path) { PipeInstance address(path); - const int listen_fd = address.socket(SocketType::Stream).rc_; + const int listen_fd = address.socket(SocketType::Stream); ASSERT_GE(listen_fd, 0) << address.asString(); ScopedFdCloser closer(listen_fd); diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 522f02a5723a5..d8de2de5b253d 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -362,9 +362,7 @@ class CustomInstance : public Address::Instance { Api::SysCallResult bind(int fd) const override { return instance_.bind(fd); } Api::SysCallResult connect(int fd) const override { return instance_.connect(fd); } const Address::Ip* ip() const override { return instance_.ip(); } - Api::SysCallResult socket(Address::SocketType type) const override { - return instance_.socket(type); - } + int socket(Address::SocketType type) const override { return instance_.socket(type); } Address::Type type() const override { return instance_.type(); } private: diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 7c4065d23e95f..e5e820d4e0368 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -412,7 +412,7 @@ class MockResolvedAddress : public Address::Instance { MOCK_CONST_METHOD1(bind, Api::SysCallResult(int)); MOCK_CONST_METHOD1(connect, Api::SysCallResult(int)); MOCK_CONST_METHOD0(ip, Address::Ip*()); - MOCK_CONST_METHOD1(socket, Api::SysCallResult(Address::SocketType)); + MOCK_CONST_METHOD1(socket, int(Address::SocketType)); MOCK_CONST_METHOD0(type, Address::Type()); const std::string& asString() const override { return physical_; } diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index 9b9476f77ade1..76e190170ddb1 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -26,18 +26,12 @@ Address::InstanceConstSharedPtr findOrCheckFreePort(Address::InstanceConstShared << (addr_port == nullptr ? "nullptr" : addr_port->asString()); return nullptr; } - Api::SysCallResult result = addr_port->socket(type); - const int fd = result.rc_; - if (fd < 0) { - ADD_FAILURE() << "socket failed for '" << addr_port->asString() - << "' with error: " << strerror(result.errno_) << " (" << result.errno_ << ")"; - return nullptr; - } + const int fd = addr_port->socket(type); ScopedFdCloser closer(fd); // Not setting REUSEADDR, therefore if the address has been recently used we won't reuse it here. // However, because we're going to use the address while checking if it is available, we'll need // to set REUSEADDR on listener sockets created by tests using an address validated by this means. - result = addr_port->bind(fd); + Api::SysCallResult result = addr_port->bind(fd); int err; const char* failing_fn = nullptr; if (result.rc_ != 0) { @@ -148,11 +142,7 @@ Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, bool supportsIpVersion(const Address::IpVersion version) { Address::InstanceConstSharedPtr addr = getCanonicalLoopbackAddress(version); - const int fd = addr->socket(Address::SocketType::Stream).rc_; - if (fd < 0) { - // Socket creation failed. - return false; - } + const int fd = addr->socket(Address::SocketType::Stream); if (0 != addr->bind(fd).rc_) { // Socket bind failed. RELEASE_ASSERT(::close(fd) == 0, ""); @@ -165,29 +155,16 @@ bool supportsIpVersion(const Address::IpVersion version) { std::pair bindFreeLoopbackPort(Address::IpVersion version, Address::SocketType type) { Address::InstanceConstSharedPtr addr = getCanonicalLoopbackAddress(version); - const char* failing_fn = nullptr; - Api::SysCallResult result = addr->socket(type); - const int fd = result.rc_; - int err; - if (fd < 0) { - err = result.errno_; - failing_fn = "socket"; - } else { - result = addr->bind(fd); - if (0 != result.rc_) { - err = result.errno_; - failing_fn = "bind"; - } else { - return std::make_pair(Address::addressFromFd(fd), fd); - } - } - if (fd >= 0) { + const int fd = addr->socket(type); + Api::SysCallResult result = addr->bind(fd); + if (0 != result.rc_) { close(fd); + std::string msg = fmt::format("bind failed for address {} with error: {} ({})", + addr->asString(), strerror(result.errno_), result.errno_); + ADD_FAILURE() << msg; + throw EnvoyException(msg); } - std::string msg = fmt::format("{} failed for address {} with error: {} ({})", failing_fn, - addr->asString(), strerror(err), err); - ADD_FAILURE() << msg; - throw EnvoyException(msg); + return std::make_pair(Address::addressFromFd(fd), fd); } TransportSocketPtr createRawBufferSocket() { return std::make_unique(); } From 42d07710361156395acd73e23dbcf2c82220bca7 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Thu, 26 Jul 2018 13:02:52 -0700 Subject: [PATCH 7/7] Address review comments * Use const Api::SysCallResult in connection_impl.cc * Capture result value in place of rc_ for better debugging Signed-off-by: Venil Noronha --- source/common/network/connection_impl.cc | 4 ++-- source/extensions/stat_sinks/common/statsd/statsd.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index ea319fe005746..073cead4dd664 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -551,7 +551,7 @@ ClientConnectionImpl::ClientConnectionImpl( } if (source_address != nullptr) { - Api::SysCallResult result = source_address->bind(fd()); + const Api::SysCallResult result = source_address->bind(fd()); if (result.rc_ < 0) { ENVOY_LOG_MISC(debug, "Bind failure. Failed to bind to {}: {}", source_address->asString(), strerror(result.errno_)); @@ -568,7 +568,7 @@ ClientConnectionImpl::ClientConnectionImpl( void ClientConnectionImpl::connect() { ENVOY_CONN_LOG(debug, "connecting to {}", *this, socket_->remoteAddress()->asString()); - Api::SysCallResult result = socket_->remoteAddress()->connect(fd()); + const Api::SysCallResult result = socket_->remoteAddress()->connect(fd()); if (result.rc_ == 0) { // write will become ready. ASSERT(connecting_); diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index e033366a7e3e2..a026f8ab90c06 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -23,8 +23,8 @@ Writer::Writer(Network::Address::InstanceConstSharedPtr address) { fd_ = address->socket(Network::Address::SocketType::Datagram); ASSERT(fd_ != -1); - const int rc = address->connect(fd_).rc_; - ASSERT(rc != -1); + const Api::SysCallResult result = address->connect(fd_); + ASSERT(result.rc_ != -1); } Writer::~Writer() {