syscall: latch errno deeper in the buffer implementation #3880
Conversation
|
This PR is a WIP and I do plan to add the Thanks, |
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 buffer 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>
venilnoronha
force-pushed
the
gh-3819-fix
branch
from
a9e36ab to
1a00e07
Compare
There was a problem hiding this comment.
This looks great! I think this will really help future-proof us from other errno related bugs.
To your other question, the other places I see we're doing syscalls inside a function are
*the os_syscalls singleton.
- Ipv[]Instance::bind / Ipv[]Instance::connect
- SocketOptionImpl::setSocketOption
Where I think the buffer calls definitely made sense to split out because they did more complicated work, I could go either way on the remaining calls. I'd be happy to see them explicitly return errno so we get into good habits, but as at least the first two are supposed to be syscall wrappers, I think we're less likely to run into problems. Matt, any preference on your end?
include/envoy/buffer/buffer.h
Outdated
| @@ -173,9 +174,9 @@ class Instance { | |||
| /** | |||
| * Write the buffer out to a file descriptor. | |||
| * @param fd supplies the descriptor to write to. | |||
| * @return the number of bytes written or -1 if there was an error. | |||
| * @return a tuple with the number of bytes written or -1 if there was an error, and the errno. | |||
There was a problem hiding this comment.
Here and above, I'd say "the errno, if any error occurred" or some other modification to make it clear that errno is only valid if the bytes_written part is -1
There was a problem hiding this comment.
Addressed in 36d2f52
test/mocks/buffer/mocks.h
Outdated
| errno = EAGAIN; | ||
| return -1; | ||
| return std::make_tuple(-1, errno); |
There was a problem hiding this comment.
Ideally we can skip setting errno now, because no caller should be checking it. If they are, that's a bug we can fix.
There was a problem hiding this comment.
Addressed in 36d2f52
No real preference. Anything we do in this area seems like a big improvement but happy to have it be incremental. Thank you! |
* Update API comments * Remove explicit errno setting in mock Signed-off-by: Venil Noronha <veniln@vmware.com>
|
Completely unrelated, but, what IDE do you generally use for Envoy? |
include/envoy/buffer/buffer.h
Outdated
| @@ -143,7 +143,8 @@ class Instance { | |||
| * Read from a file descriptor directly into the buffer. | |||
| * @param fd supplies the descriptor to read from. | |||
| * @param max_length supplies the maximum length to read. | |||
| * @return a tuple with the number of bytes read or -1 if there was an error, and the errno. | |||
| * @return a tuple with the number of bytes read and the errno. If an error occurs, the number | |||
| * of bytes read would indicate -1 and the errno would be non-zero. | |||
There was a problem hiding this comment.
How about
"a tuple with the number of bytes read and the errno. If an error occurred, the number of bytes read would indicate -1 and the errno would be non-zero. Otherwise if bytes were read, errno should not be used."
?
There was a problem hiding this comment.
Addressed in e5c8ed1.
Signed-off-by: Venil Noronha <veniln@vmware.com>
|
Awesome! I do plan to work on the other APIs you mentioned, but I'll create separate PRs for those. Thanks, |
|
Perfect. As with Matt, I'm a big fan of small and incremental changes :-) |
|
@mattklein123 thanks for merging! |
Description:
The
errnoset by a syscall can be overwritten by code (e.g. logging) as it propagates up through the call stack. This PR refactors thebufferAPI (readandwrite) to allow for returning theerrnofrom deeper down the call stack i.e. as soon as a syscall is performed. See #3819 for more information.Risk Level: Low
Testing:
bazel test //test/... --test_env=ENVOY_IP_TEST_VERSIONS=v4onlyruns without failuresDocs Changes: N/A
Release Notes: N/A