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

Introduce os_fd_t and windows port of os_sys_calls_impl #10036

Merged
merged 11 commits into from
Feb 18, 2020
Merged

Introduce os_fd_t and windows port of os_sys_calls_impl #10036

merged 11 commits into from
Feb 18, 2020

Conversation

wrowe
Copy link
Contributor

@wrowe wrowe commented Feb 12, 2020

Description:

  • Add os_fd_t as the new data type for os_sys_calls socket methods
    necessary to port to windows, where SOCKET is 2x width of an int
  • Add SysCallSocketResult of that data type
  • Normalize and "const-ify" all usage of iovec and msghdr types
  • Add additional os_sys_calls methods all required for porting
    the various test/ sources to Windows in subsequent PRs
  • Introduce the win32 port of os_sys_calls_impl

This patch defers the actual work of employing the os_sys_calls API
where needed for Windows, which is a much more involved PR review.

Risk Level: Moderate
Testing: local on linux gcc, windows msvc
Docs Changes: n/a
Release Notes: n/a

wrowe and others added 2 commits February 12, 2020 16:29
@wrowe @sunjayBhatia
Most of the simple os_fd_t reorg
61af245 
- Add os_fd_t as the new data type for os_sys_calls socket methods
- Add SysCallSocketResult of that data type
- Normalize and "const-ify" all usage of iovec and msghdr types
- Add additional os_sys_calls methods all required for porting
  the various test/ sources to Windows in subsequent PRs

Co-authored-by: Sunjay Bhatia <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
@wrowe @sunjayBhatia
Add implementation of os_sys_calls for Windows
260a55b 
Co-authored-by: Sunjay Bhatia <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
zuercher
zuercher reviewed Feb 13, 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.

Nicely done. I have some mostly-style related comments below.

One question I have is whether our existing tests are sufficient to exercise all the glue code in win32/os_sys_calls_impl.cc? Specifically, I worry there are some network errors that aren't exercised by our tests or subtle behavioral differences when converting between iovec and WSABUF.

@sunjayBhatia @wrowe
Move win32 os_sys_calls helper functions to anonymous namespace
466a899 
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@wrowe
Copy link
Contributor Author

wrowe commented Feb 13, 2020

One question I have is whether our existing tests are sufficient to exercise all the glue code in win32/os_sys_calls_impl.cc

It wasn't, nor was the existing posix implementation. That patch is coming in the next PR.

@sunjayBhatia
Copy link
Member

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10036 (comment) was created by @sunjayBhatia.

see: more, trace.

@sunjayBhatia
Copy link
Member

/retest

test passes locally but failed in CI, just retesting to make sure

wrowe and others added 6 commits February 13, 2020 11:56
@wrowe @sunjayBhatia
Adjust accessor for WSARecvMsg function
a8611a3 
- Collapse panics
- rename to with a more envoy-style name

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia
Rename local variable iMode -> io_mode
cc6a7d0 
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>

Signed-off-by: William A Rowe Jr <[email protected]>
@wrowe @sunjayBhatia
Correct use of errno in posix os_sys_calls
19281f7 
- errno is only reset if the previous syscall failed, in the case where
  the return code is 0, we may still be returning non-zero errno
- callers should be checking the rc before looking at errno, but this is
  a cleanup to ensure they can never see an erroneous errno on success
  of a syscall

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@wrowe @sunjayBhatia
Correct return type of socket
0d03127 
Should be SysCallSocketResult

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@wrowe @sunjayBhatia
Simplify usage of os_sys_calls in listener_manager_impl_test
6b5d982 
- The test class has an os_sys_calls mock member that is injected as a
singleton, use that where needed
- Use new macros for socket validity

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia @wrowe
Don't add os_sys_calls as a class member
5f78ad3 
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
@sunjayBhatia
Copy link
Member

In our efforts to try to resolve the circle CI failure, we noticed we could reduce the relevant test file and did so in: 6b5d982

zuercher
zuercher previously approved these changes Feb 14, 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! This looks good to me.

lizan
lizan reviewed Feb 14, 2020
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

LGTM modulo some style nits.

@sunjayBhatia @wrowe
Formatting/style fixes
4ded275 
- snake case all variables
- no static in anonymous namespace
- get rid of redundant std::move

Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
@wrowe @sunjayBhatia
Merge branch 'master' into test-os_fd_t
a61026c 
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia
Copy link
Member

retesting due to coverage tests segfault (same as #10012)

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10036 (comment) was created by @sunjayBhatia.

see: more, trace.

@sunjayBhatia
Copy link
Member

retesting (again) due to coverage tests segfault (same as #10012)

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10036 (comment) was created by @sunjayBhatia.

see: more, trace.

@sunjayBhatia
Copy link
Member

@zuercher looks like we just keep failing on #10012, should we continue to retest until it passes? a previous commit before some windows-only changes had this pass (see: https://github.com/envoyproxy/envoy/pull/10036/commits, specifically commit 5f78ad3)

@mattklein123
Copy link
Member

We can merge w/o coverage passing until we fix it. cc @lizan

@mattklein123 mattklein123 merged commit 6c3c2b3 into envoyproxy:master Feb 18, 2020
@wrowe wrowe deleted the test-os_fd_t branch February 19, 2020 23:07
@sunjayBhatia sunjayBhatia mentioned this pull request Feb 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizan lizan lizan left review comments

@sunjayBhatia sunjayBhatia sunjayBhatia left review comments

@zuercher zuercher zuercher approved these changes

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@htuch htuch Awaiting requested review from htuch

@mattklein123 mattklein123 Awaiting requested review from mattklein123

@PiotrSikora PiotrSikora Awaiting requested review from PiotrSikora

Assignees

@zuercher zuercher

@lizan lizan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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