syscall: refactor address APIs for deeper errno latching #3897

venilnoronha · 2018-07-19T05:03:08Z

Description:
The errno set by a syscall can be overwritten by code (e.g. logging) as it propagates up through the call stack. This PR refactors the bind, connect and socket 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. See #3819 for more information.

Signed-off-by: Venil Noronha veniln@vmware.com

Risk Level: Low
Testing: Existing tests suffice
Docs Changes: N/A
Release Notes: N/A

/cc @alyssawilk @mattklein123

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 <veniln@vmware.com>

alyssawilk

Cool - safer and safer :-)

alyssawilk · 2018-07-19T14:03:36Z

test/test_common/network_utility.cc

  if (fd < 0) {
+    err = errno;


I think it'd be nice to have socket return a tuple for consistency.

Addressed in 6c65b13.

This commit ensures that address implementations return the errno along with the file descriptor in socket method calls. Signed-off-by: Venil Noronha <veniln@vmware.com>

lizan · 2018-07-20T20:26:23Z

include/envoy/network/address.h

   */
-  virtual int bind(int fd) const PURE;
+  virtual std::tuple<int, int> bind(int fd) const PURE;


would absl::optional be a better return type? (ditto for connect)

@alyssawilk thoughts?

Would it be better to have a custom type with rc (or fd) and the errno instead? Or, maybe, a generic for the first value?

I would prefer a struct for socket rather than a tuple, it is clearer which field is fd or errno.

+1 for a struct instead of tuple, for all of these.

Single struct or multiple? As, the returned values could be rc or fd depending on the call.

I think a single struct is fine, with members rc and errno. It's always up to the caller to know what the function is returning and how to interpret it.

Addressed in 36c81e5.

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<int, int>. Signed-off-by: Venil Noronha <veniln@vmware.com>

lizan · 2018-07-24T06:50:16Z

include/envoy/network/address.h

+  /**
+   * The return code from the system call.
+   */
+  int rc;


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

Addressed in bb2841e.

mattklein123

Thanks for switching this over to a struct. I prefer this also.

mattklein123 · 2018-07-24T15:16:28Z

include/envoy/network/address.h

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


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.

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

Addressed in bb2841e.

mattklein123 · 2018-07-24T15:17:46Z

source/common/network/address_impl.cc

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


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?

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

The order of evaluation of function arguments is unspecified.

I'll fix this.

@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.

@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.

Addressed in 7e05d55.

* Fixes struct styling * Moves address/Result to os_sys_calls/SysCallResult Signed-off-by: Venil Noronha <veniln@vmware.com>

Signed-off-by: Venil Noronha <veniln@vmware.com>

ggreenway · 2018-07-24T20:36:05Z

source/common/network/address_impl.cc

@@ -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};


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?

@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?

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

Addressed in dc0c613.

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 <veniln@vmware.com>

mattklein123

LGTM, nice. Will defer to @ggreenway for final review.

mattklein123 · 2018-07-26T13:57:41Z

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) {
+    Api::SysCallResult result = source_address->bind(fd());


nit: result here and below can be const.

Addressed in 42d0771.

ggreenway

This is looking good. After fixing the one small comment from me, and the one from Matt, I think this is ready.

ggreenway · 2018-07-26T17:01:50Z

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 = address->connect(fd_).rc_;


I'd prefer to keep the SysCallResult instead of an int, so that if the assert fires in a test it's easier in a debugger to see the value of errno.

Addressed in 42d0771.

* Use const Api::SysCallResult in connection_impl.cc * Capture result value in place of rc_ for better debugging Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha · 2018-07-27T18:27:55Z

@mattklein123 @ggreenway @alyssawilk @lizan thanks for reviewing/merging!

venilnoronha changed the title ~~Refactor address APIs for deeper errno latching~~ syscall: refactor address APIs for deeper errno latching Jul 19, 2018

alyssawilk reviewed Jul 19, 2018

View reviewed changes

alyssawilk self-assigned this Jul 19, 2018

Update address's socket method signature

6c65b13

This commit ensures that address implementations return the errno along with the file descriptor in socket method calls. Signed-off-by: Venil Noronha <veniln@vmware.com>

lizan reviewed Jul 20, 2018

View reviewed changes

lizan reviewed Jul 24, 2018

View reviewed changes

mattklein123 reviewed Jul 24, 2018

View reviewed changes

Address review comments

bb2841e

* Fixes struct styling * Moves address/Result to os_sys_calls/SysCallResult Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha force-pushed the gh-3819-fix-address branch from 737eabf to bb2841e Compare July 24, 2018 17:08

Make syscalls explicit prior to SysCallResult init

7e05d55

Signed-off-by: Venil Noronha <veniln@vmware.com>

ggreenway requested changes Jul 24, 2018

View reviewed changes

alyssawilk assigned mattklein123 and ggreenway and unassigned alyssawilk Jul 26, 2018

mattklein123 reviewed Jul 26, 2018

View reviewed changes

ggreenway requested changes Jul 26, 2018

View reviewed changes

Address review comments

42d0771

* Use const Api::SysCallResult in connection_impl.cc * Capture result value in place of rc_ for better debugging Signed-off-by: Venil Noronha <veniln@vmware.com>

mattklein123 approved these changes Jul 26, 2018

View reviewed changes

ggreenway approved these changes Jul 27, 2018

View reviewed changes

ggreenway merged commit 324e628 into envoyproxy:master Jul 27, 2018

syscall: refactor address APIs for deeper errno latching #3897

syscall: refactor address APIs for deeper errno latching #3897

Conversation

venilnoronha commented Jul 19, 2018 • edited Loading

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha commented Jul 27, 2018

venilnoronha commented Jul 19, 2018 •

edited

Loading

venilnoronha Jul 21, 2018 •

edited

Loading

venilnoronha Jul 24, 2018 •

edited

Loading