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.

Sign up for GitHub

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

Jump to bottom

Use os_sys_calls in all code paths to replace raw syscalls #10107

Merged
merged 10 commits into from
Feb 28, 2020
Merged

Use os_sys_calls in all code paths to replace raw syscalls #10107

merged 10 commits into from
Feb 28, 2020

Conversation

sunjayBhatia
Copy link
Member

Description:

  • This PR seeks to address remaining changes for cross-platform code (ultimately for Windows support) that apply to the linux and os/x code paths in order to consolidate the necessary patch review requirements.
  • The subsequent PR(s) should largely avoid changes impacting the current working linux and os/x code paths and should be targeted mainly at Windows code.
  • Primarily adds usage of os_sys_calls instead of directly using syscalls for various network and test sources.
  • Addresses a reoccurring issue that only linux/posix allows for connection to an any-address. On Windows and in general, this makes no sense so several tests are adjusted to explicitly connect to the loopback address instead of the "any" address.
  • Includes changes to simplify platform specific includes/macros
  • Coalesces between platforms (posix/Windows) and hopefully simplifies the implementation of io_socket_handle_impl
    Risk Level: Low
    Testing: Local Linux gcc and Windows MSVC
    Docs Changes: N/A
    Release Notes: N/A

@sunjayBhatia @wrowe
Use os_sys_calls in all code paths to replace raw syscalls
b402f78 
This PR seeks to address all remaining changes to the linux
and os/x code paths, in order to consolidate the necessary
patch review requirements.

The subsequent PR(s) should not include changes impacting the
current working linux and os/x code paths.

Primarilly adds usage of os_sys_calls for various network and
test sources.

Addresses a reoccuring issue that only linux/posix allows for
connection -to- an any-address, on Windows and in general,
this makes no sense. So several tests are adjusted to explicitly
connect to the loopback interface of the "any" if listener.

Signed-off-by: Sunjay Bhatia <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
@wrowe @sunjayBhatia
Fix formatting
eaaea33 
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia @wrowe
proxy protocol test should not use raw syscalls
04f2985 
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
zuercher
zuercher reviewed Feb 20, 2020
View reviewed changes
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks. I had a few questions, but looks pretty good.

source/common/network/address_impl.cc Outdated Show resolved Hide resolved
source/common/network/address_impl.cc Outdated Show resolved Hide resolved
test/common/network/connection_impl_test.cc Outdated Show resolved Hide resolved
test/common/network/listen_socket_impl_test.cc Show resolved Hide resolved
test/common/network/listener_impl_test.cc Outdated Show resolved Hide resolved
@wrowe @sunjayBhatia
Corrections per PR feedback
1b73915 
- Relies on localAddress() to return an explicit local if:port
  as proposed by zuercher
- Correct test regressions by introducing appropriate mocks
- Drop posix ::dup call by closing and reusing the same if:port
  for socket in ListenSocketImplTest/testBindSpecificPort

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Feb 26, 2020 •
edited
Loading

Circle CI failing due to #10012

Only //test/extensions/quic_listeners/quiche/integration:quic_http_integration_test fails locally for us disregard this

@sunjayBhatia @wrowe
Correct asan leak detected
925480c 
Need to keep this apparently duplicate logic due to the scope of the cmsg buffer

Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
zuercher
zuercher reviewed Feb 27, 2020
View reviewed changes
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

I think there's one more location we can use sockAddr/sockAddrLen(), but otherwise looks good!

source/common/network/address_impl.cc Outdated Show resolved Hide resolved
@sunjayBhatia
Copy link
Member Author

@zuercher any idea how to get CI for this unstuck? looks like the coverage tests failed again on the same flake but not even the sign-off check has run

wrowe and others added 2 commits February 27, 2020 13:58
@wrowe @sunjayBhatia
Kick CI
88bc628 
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia @wrowe
Kick CI
0ffe192 
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
@wrowe
Copy link
Contributor

wrowe commented Feb 27, 2020

Confirming CircleCI coverage is failing on the usual flake (same as issue #10012), but this PR appears good to go;

[ RUN ] Protocols/BufferIntegrationTest.RouterRequestPopulateContentLengthOnTrailers/IPv4_HttpDownstream_HttpUpstream
external/bazel_tools/tools/test/collect_coverage.sh: line 128: 1002 Segmentation fault (core dumped) "$@"

  • TEST_STATUS=139
  • touch /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/4629/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/coverage/coverage_tests/shard_27_of_50/coverage.dat
  • [[ 139 -ne 0 ]]
  • echo --
    --
  • echo Coverage runner: Not collecting coverage for failed test.

zuercher
zuercher previously approved these changes Feb 28, 2020
View reviewed changes
@zuercher
Copy link
Member

@mattklein123 (or maybe @lizan) I think if this is to be merged without coverage passing one of you is going to have to push the button.

mattklein123
mattklein123 requested changes Feb 28, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

One small question/comment. Thank you!

/wait

source/common/network/connection_impl.cc Outdated Show resolved Hide resolved
@lizan
Copy link
Member

lizan commented Feb 28, 2020

Can you merge master to fix coverage?

@sunjayBhatia
Copy link
Member Author

Can you merge master to fix coverage?

will do!

wrowe and others added 2 commits February 28, 2020 14:38
@wrowe @sunjayBhatia
Remove socket shutdown for now
c2316ef 
- so we can re-discuss as it changes linux functionality
- we can re-evaluate for Windows as well

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia @wrowe
Merge remote-tracking branch 'origin/master' into test-crossplat-fixup
8d09285 
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
@mattklein123 mattklein123 self-assigned this Feb 28, 2020
mattklein123
mattklein123 approved these changes Feb 28, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@sunjayBhatia sunjayBhatia mentioned this pull request Feb 28, 2020
Closed
@mattklein123 mattklein123 merged commit d173319 into envoyproxy:master Feb 28, 2020
@wrowe wrowe deleted the test-crossplat-fixup branch February 29, 2020 03:33
@asraa asraa mentioned this pull request Feb 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wrowe wrowe wrowe left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

@zuercher zuercher zuercher left review comments

@htuch htuch Awaiting requested review from htuch

@PiotrSikora PiotrSikora Awaiting requested review from PiotrSikora

Assignees

@zuercher zuercher

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sunjayBhatia @wrowe @zuercher @lizan @mattklein123