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

[Windows] Adds a native windows filesystem implementation #13077

Merged
merged 10 commits into from
Sep 21, 2020

Conversation

davinci26
Copy link
Member

Signed-off-by: Sotiris Nanopoulos [email protected]

Commit Message:

Refactor filesystem_impl to use win32 APIs instead of POSIX subsystem. Fixes issue #11655.

Additional Description:

For easier review the major changes are the following:

  1. Adds a Win32 impl
  2. Strips away the file_shared_impl because it relied on errno for error handling. This change cascades to the POSIX implementation
  3. Adds a preliminary error mapping so we don't rely on error strings.

Risk Level: Low
Testing: Already tested in filesystem_impl_test
Docs Changes: N/A
Release Notes: N/A

@davinci26
Copy link
Member Author

cc: @envoyproxy/windows-dev

gimme one CI run to clean up any formatting or cascading build errors

@asraa asraa requested a review from wrowe September 14, 2020 13:18
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

A couple quick comments for discussion, but generally looking really good here, thanks!

include/envoy/common/platform.h Outdated Show resolved Hide resolved
Sotiris Nanopoulos added 5 commits September 16, 2020 11:10
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26
Copy link
Member Author

Attempting to bump coverage above threshold. Technically it shouldn't have been affected by the change since the code is a null-op in linux.

Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26
Copy link
Member Author

clang-tidy fails due to #13162 that is coming from upstream

Signed-off-by: Sotiris Nanopoulos <[email protected]>
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

LGTM!

@davinci26
Copy link
Member Author

@envoyproxy/senior-maintainers can I also get a review from you.

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.

Nice cleanup. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants