syscall: refactor address APIs for deeper errno latching

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() {}

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(

include/envoy/network/address.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include "envoy/api/os_sys_calls.h"
 #include "envoy/common/pure.h"
 
 #include "absl/numeric/int128.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 Api::SysCallResult with rc_ = 0 for success and rc_ = -1 for failure. If the call
+   *   is successful, errno_ shouldn't be used.
    */
-  virtual int 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 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 Api::SysCallResult with rc_ = 0 for success and rc_ = -1 for failure. If the call
+   *   is successful, errno_ shouldn't be used.
    */
-  virtual int connect(int fd) const PURE;
+  virtual Api::SysCallResult connect(int fd) const PURE;
 
   /**
    * @return the IP address information IFF type() == Type::Ip, otherwise nullptr.
@@ -150,9 +151,8 @@ 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 the file descriptor naming the socket. In case of a failure, the program would be
+   *   aborted.
    */
   virtual int socket(SocketType type) const PURE;
 

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<const sockaddr*>(&ip_.ipv4_.address_),
-                sizeof(ip_.ipv4_.address_));
+Api::SysCallResult Ipv4Instance::bind(int fd) const {
+  const int rc = ::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
+                        sizeof(ip_.ipv4_.address_));
+  return {rc, errno};
 }
 
-int Ipv4Instance::connect(int fd) const {
-  return ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
-                   sizeof(ip_.ipv4_.address_));
+Api::SysCallResult Ipv4Instance::connect(int fd) const {
+  const int rc = ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
+                           sizeof(ip_.ipv4_.address_));
+  return {rc, errno};
 }
 
 int Ipv4Instance::socket(SocketType type) const { return socketFromSocketType(type); }
@@ -275,19 +277,20 @@ bool Ipv6Instance::operator==(const Instance& rhs) const {
           (ip_.port() == rhs_casted->ip_.port()));
 }
 
-int Ipv6Instance::bind(int fd) const {
-  return ::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
-                sizeof(ip_.ipv6_.address_));
+Api::SysCallResult Ipv6Instance::bind(int fd) const {
+  const int rc = ::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
+                        sizeof(ip_.ipv6_.address_));
+  return {rc, errno};
 }
 
-int Ipv6Instance::connect(int fd) const {
-  return ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
-                   sizeof(ip_.ipv6_.address_));
+Api::SysCallResult Ipv6Instance::connect(int fd) const {
+  const int rc = ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
+                           sizeof(ip_.ipv6_.address_));
+  return {rc, errno};
 }
 
 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(fd, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1, "");
@@ -333,24 +336,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 {
+Api::SysCallResult PipeInstance::bind(int fd) const {
   if (abstract_namespace_) {
-    return ::bind(fd, reinterpret_cast<const sockaddr*>(&address_),
-                  offsetof(struct sockaddr_un, sun_path) + address_length_);
+    const int rc = ::bind(fd, reinterpret_cast<const sockaddr*>(&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<const sockaddr*>(&address_), sizeof(address_));
+  const int rc = ::bind(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_));
+  return {rc, errno};
 }
 
-int PipeInstance::connect(int fd) const {
+Api::SysCallResult PipeInstance::connect(int fd) const {
   if (abstract_namespace_) {
-    return ::connect(fd, reinterpret_cast<const sockaddr*>(&address_),
-                     offsetof(struct sockaddr_un, sun_path) + address_length_);
+    const int rc = ::connect(fd, reinterpret_cast<const sockaddr*>(&address_),
+                             offsetof(struct sockaddr_un, sun_path) + address_length_);
+    return {rc, errno};
   }
-  return ::connect(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_));
+  const int rc = ::connect(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_));
+  return {rc, errno};
 }
 
 int PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); }

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;
+  Api::SysCallResult bind(int fd) const override;
+  Api::SysCallResult 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;
+  Api::SysCallResult bind(int fd) const override;
+  Api::SysCallResult 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;
+  Api::SysCallResult bind(int fd) const override;
+  Api::SysCallResult connect(int fd) const override;
   const Ip* ip() const override { return nullptr; }
   int socket(SocketType type) const override;
 

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 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(errno));
+                     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());
-  const int rc = socket_->remoteAddress()->connect(fd());
-  if (rc == 0) {
+  const Api::SysCallResult result = socket_->remoteAddress()->connect(fd());
+  if (result.rc_ == 0) {
     // write will become ready.
     ASSERT(connecting_);
   } else {
-    ASSERT(rc == -1);
-    if (errno == 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, errno);
+      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);

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 Api::SysCallResult result = local_address_->bind(fd_);
+  if (result.rc_ == -1) {
     close();
     throw EnvoyException(
-        fmt::format("cannot bind '{}': {}", local_address_->asString(), strerror(errno)));
+        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

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);
 
-  int rc = address->connect(fd_);
-  ASSERT(rc != -1);
+  const Api::SysCallResult result = address->connect(fd_);
+  ASSERT(result.rc_ != -1);
 }
 
 Writer::~Writer() {

test/common/network/address_impl_test.cc

@@ -64,9 +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;
-  ASSERT_EQ(rc, 0) << addr_port->asString() << "\nerror: " << strerror(err) << "\nerrno: " << err;
+  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.
@@ -85,9 +85,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;
-    ASSERT_EQ(rc, 0) << addr_port->asString() << "\nerror: " << strerror(err) << "\nerrno: " << err;
+    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);
@@ -314,9 +314,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;
-    ASSERT_EQ(rc, 0) << address.asString() << "\nerror: " << strerror(err) << "\nerrno: " << err;
+    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");

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); }
+  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(); }
   int socket(Address::SocketType type) const override { return instance_.socket(type); }
   Address::Type type() const override { return instance_.type(); }

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, Api::SysCallResult(int));
+  MOCK_CONST_METHOD1(connect, Api::SysCallResult(int));
   MOCK_CONST_METHOD0(ip, Address::Ip*());
   MOCK_CONST_METHOD1(socket, int(Address::SocketType));
   MOCK_CONST_METHOD0(type, Address::Type());