Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syscall: refactor address APIs for deeper errno latching #3897

Merged
merged 7 commits into from
Jul 27, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 26 additions & 10 deletions include/envoy/network/address.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,22 @@ namespace Envoy {
namespace Network {
namespace Address {

/**
* Result holds the rc and errno values resulting from a system call.
*/
struct Result {
Copy link
Member

Choose a reason for hiding this comment

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

Q: Is this really address specific? Should we potentially put this in https://github.com/envoyproxy/envoy/blob/master/include/envoy/api/os_sys_calls.h as SyscalResult or something like that? I don't feel that strongly but throwing it out there.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes perfect sense. I wanted to use this struct in other places (buffer, etc.) anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in bb2841e.


/**
* The return code from the system call.
*/
int rc;
Copy link
Member

Choose a reason for hiding this comment

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

Struct/Class member variables have a _ postfix (e.g., int foo_;).
https://github.com/envoyproxy/envoy/blob/master/STYLE.md

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in bb2841e.


/**
* The errno value as captured after the system call.
*/
int error;
};

/**
* Interface for an Ipv4 address.
*/
Expand Down Expand Up @@ -128,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 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 Result with rc = 0 for success and rc = -1 for failure. If the call is successful,
* the error value shouldn't be used.
*/
virtual int 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 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 Result with rc = 0 for success and rc = -1 for failure. If the call is successful,
* the error value shouldn't be used.
*/
virtual int connect(int fd) const PURE;
virtual Result connect(int fd) const PURE;

/**
* @return the IP address information IFF type() == Type::Ip, otherwise nullptr.
Expand All @@ -150,11 +166,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 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 int socket(SocketType type) const PURE;
virtual Result socket(SocketType type) const PURE;

/**
* @return the type of address.
Expand Down
64 changes: 35 additions & 29 deletions source/common/network/address_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ InstanceConstSharedPtr peerAddressFromFd(int fd) {
return addressFromSockAddr(ss, ss_len);
}

int InstanceBase::socketFromSocketType(SocketType socketType) const {
Result InstanceBase::socketFromSocketType(SocketType socketType) const {
#if defined(__APPLE__)
int flags = 0;
#else
Expand Down Expand Up @@ -168,7 +168,7 @@ int InstanceBase::socketFromSocketType(SocketType socketType) const {
RELEASE_ASSERT(fcntl(fd, F_SETFL, O_NONBLOCK) != -1, "");
#endif

return fd;
return {fd, errno};
Copy link
Contributor

Choose a reason for hiding this comment

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

In the __APPLE__ case, I think that fcntl could be resetting errno, so you need to latch it earlier.

On the other hand, there's already a RELEASE_ASSERT that an error did not occur (fd != -1). So should this function even return errno?

Copy link
Member Author

@venilnoronha venilnoronha Jul 24, 2018

Choose a reason for hiding this comment

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

@ggreenway good catch! What do you suggest? Should we still keep the function return type as Api::SysCallResult for consistency, and to provide a way for future error handling?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say leave this function as returning an int, and make sure the docs mention that this will never fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in dc0c613.

}

Ipv4Instance::Ipv4Instance(const sockaddr_in* address) : InstanceBase(Type::Ip) {
Expand Down Expand Up @@ -212,17 +212,19 @@ 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_));
Result Ipv4Instance::bind(int fd) const {
return {::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
sizeof(ip_.ipv4_.address_)),
errno};
Copy link
Member

Choose a reason for hiding this comment

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

It's quite possible that C++ guarantees that the ::bind call happens first, and then errno is captured, but TBH it's not very obvious to me that is the case. I would prefer that we do the call into a local variable and then return the result and errno. Same elsewhere. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. The c++14 documentation states:

The order of evaluation of function arguments is unspecified.

I'll fix this.

Copy link
Member

Choose a reason for hiding this comment

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

@venilnoronha You're referring to function arguments, but this is list-initialization, and C++14 guarantees the order:
https://timsong-cpp.github.io/cppwp/n4140/dcl.init.list#4

Within the initializer-list of a braced-init-list, the initializer-clauses, including any that result from pack expansions ([temp.variadic]), are evaluated in the order in which they appear.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lizan my bad, I just realized that we switched over to a struct. Thanks for sharing!

I think I'd still have it explicit, at least for readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 7e05d55.

}

int Ipv4Instance::connect(int fd) const {
return ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
sizeof(ip_.ipv4_.address_));
Result Ipv4Instance::connect(int fd) const {
return {::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
sizeof(ip_.ipv4_.address_)),
errno};
}

int 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};
Expand Down Expand Up @@ -275,23 +277,25 @@ 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_));
Result Ipv6Instance::bind(int fd) const {
return {::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
sizeof(ip_.ipv6_.address_)),
errno};
}

int Ipv6Instance::connect(int fd) const {
return ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
sizeof(ip_.ipv6_.address_));
Result Ipv6Instance::connect(int fd) const {
return {::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_),
sizeof(ip_.ipv6_.address_)),
errno};
}

int Ipv6Instance::socket(SocketType type) const {
const int fd = socketFromSocketType(type);

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, "");
return fd;
RELEASE_ASSERT(::setsockopt(result.rc, IPPROTO_IPV6, IPV6_V6ONLY, &v6only, sizeof(v6only)) != -1,
"");
return result;
}

PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len)
Expand Down Expand Up @@ -333,27 +337,29 @@ 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 {
Result PipeInstance::bind(int fd) const {
if (abstract_namespace_) {
return ::bind(fd, reinterpret_cast<const sockaddr*>(&address_),
offsetof(struct sockaddr_un, sun_path) + address_length_);
return {::bind(fd, reinterpret_cast<const sockaddr*>(&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<const sockaddr*>(&address_), sizeof(address_));
return {::bind(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_)), errno};
}

int PipeInstance::connect(int fd) const {
Result PipeInstance::connect(int fd) const {
if (abstract_namespace_) {
return ::connect(fd, reinterpret_cast<const sockaddr*>(&address_),
offsetof(struct sockaddr_un, sun_path) + address_length_);
return {::connect(fd, reinterpret_cast<const sockaddr*>(&address_),
offsetof(struct sockaddr_un, sun_path) + address_length_),
errno};
}
return ::connect(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_));
return {::connect(fd, reinterpret_cast<const sockaddr*>(&address_), sizeof(address_)), errno};
}

int PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); }
Result PipeInstance::socket(SocketType type) const { return socketFromSocketType(type); }

} // namespace Address
} // namespace Network
Expand Down
20 changes: 10 additions & 10 deletions source/common/network/address_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class InstanceBase : public Instance {

protected:
InstanceBase(Type type) : type_(type) {}
int socketFromSocketType(SocketType type) const;
Result socketFromSocketType(SocketType type) const;

std::string friendly_name_;

Expand Down Expand Up @@ -91,10 +91,10 @@ 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;
Result bind(int fd) const override;
Result connect(int fd) const override;
const Ip* ip() const override { return &ip_; }
int socket(SocketType type) const override;
Result socket(SocketType type) const override;

private:
struct Ipv4Helper : public Ipv4 {
Expand Down Expand Up @@ -151,10 +151,10 @@ 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;
Result bind(int fd) const override;
Result connect(int fd) const override;
const Ip* ip() const override { return &ip_; }
int socket(SocketType type) const override;
Result socket(SocketType type) const override;

private:
struct Ipv6Helper : public Ipv6 {
Expand Down Expand Up @@ -208,10 +208,10 @@ 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;
Result bind(int fd) const override;
Result connect(int fd) const override;
const Ip* ip() const override { return nullptr; }
int socket(SocketType type) const override;
Result socket(SocketType type) const override;

private:
sockaddr_un address_;
Expand Down
16 changes: 8 additions & 8 deletions source/common/network/connection_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -551,10 +551,10 @@ ClientConnectionImpl::ClientConnectionImpl(
}

if (source_address != nullptr) {
const int rc = source_address->bind(fd());
if (rc < 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(errno));
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".
Expand All @@ -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) {
Address::Result 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.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, result.error);

// Trigger a write event. This is needed on OSX and seems harmless on Linux.
file_event_->activate(Event::FileReadyType::Write);
Expand Down
10 changes: 5 additions & 5 deletions source/common/network/listen_socket_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ namespace Envoy {
namespace Network {

void ListenSocketImpl::doBind() {
int rc = local_address_->bind(fd_);
if (rc == -1) {
const Address::Result 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.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
Expand All @@ -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(address->socket(Address::SocketType::Stream).rc, address) {
RELEASE_ASSERT(fd_ != -1, "");

// TODO(htuch): This might benefit from moving to SocketOptionImpl.
Expand All @@ -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(address->socket(Address::SocketType::Stream).rc, address) {
RELEASE_ASSERT(fd_ != -1, "");
doBind();
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/network/listen_socket_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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), nullptr,
: ConnectionSocketImpl(remote_address->socket(Address::SocketType::Stream).rc, nullptr,
remote_address) {}
};

Expand Down
4 changes: 2 additions & 2 deletions source/extensions/stat_sinks/common/statsd/statsd.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ namespace Common {
namespace Statsd {

Writer::Writer(Network::Address::InstanceConstSharedPtr address) {
fd_ = address->socket(Network::Address::SocketType::Datagram);
fd_ = address->socket(Network::Address::SocketType::Datagram).rc;
ASSERT(fd_ != -1);

int rc = address->connect(fd_);
const int rc = address->connect(fd_).rc;
ASSERT(rc != -1);
}

Expand Down
14 changes: 7 additions & 7 deletions test/common/network/addr_family_aware_socket_option_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 = address.socket(Address::SocketType::Stream).rc;
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));

AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND,
Expand All @@ -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 = address.socket(Address::SocketType::Stream).rc;
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));
AddrFamilyAwareSocketOptionImpl socket_option{
envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1};
Expand All @@ -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 = address.socket(Address::SocketType::Stream).rc;
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));
AddrFamilyAwareSocketOptionImpl socket_option{
envoy::api::v2::core::SocketOption::STATE_PREBIND, {}, {}, 1};
Expand All @@ -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 = address.socket(Address::SocketType::Stream).rc;
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));

AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND,
Expand All @@ -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 = address.socket(Address::SocketType::Stream).rc;
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));

AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND,
Expand All @@ -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 = address.socket(Address::SocketType::Stream).rc;
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));

AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND,
Expand All @@ -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 = address.socket(Address::SocketType::Stream).rc;
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));

AddrFamilyAwareSocketOptionImpl socket_option{envoy::api::v2::core::SocketOption::STATE_PREBIND,
Expand Down
Loading