filesystem: add Windows implementation by yaelharel · Pull Request #6072 · envoyproxy/envoy

yaelharel · 2019-02-26T21:49:58Z

Description:
Adds Windows implementation of Filesystem::Instance and Filesystem::File interfaces.
Inject the platform-specific implementations of Filesystem::Instance& through MainCommon.
Moves IoError class from Network namespace to Api namespace.

This is step 3/3 in adding Windows filesystem support as outlined here: #5470 (comment).

Risk Level:
Low

Testing:
bazel build //source/... && bazel test //test/...

Docs Changes:
N/A

Release Notes:
N/A

Relates to issue #129

cc: @achasveachas
cc: @sesmith177

Adds Windows implementation of Filesystem::Instance and Filesystem::File interfaces. Inject the platform-specific implementations of Filesystem::Instance& through MainCommon. Moves IoError class from Network namespace to Api namespace. Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz · 2019-02-27T18:57:45Z

I have some context on this and will make some comments :)

dnoe · 2019-02-27T19:16:27Z

Thanks, context much appreciated. I am also wondering if it makes sense to split out the mechanical refactoring moves (I see there are a few in the diff) from the logic changes and new stuff? It may make this much easier to review.

jmarantz · 2019-02-27T19:18:30Z

+1 for separate PRs for refactors vs new code. Please split this in two.

danzh2010 · 2019-02-27T19:25:52Z

include/envoy/api/io_error.h

+// frequent memory allocation of IoError instance.
+// If this is used, IoCallResult has to be instantiated with a deleter that does not
+// deallocate memory for this error.
+#define ENVOY_ERROR_AGAIN reinterpret_cast<Api::IoError*>(0x01)


This might be changed in #6037 to address an memory alignment error.

Oh good :) Can I suggest addressing this with a singleton? I'm generally against singletons but this seems like a good place to use one.

I'll comment also in #6037 .

We'll let that be handled in #6037 .

yaelharel · 2019-02-27T20:18:08Z

As requested by @dnoe and @jmarantz, submitted #6104 (handle refactors).

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

yaelharel · 2019-02-28T19:21:54Z

Hi @dnoe and @jmarantz, I think the PR is ready for another look.
Thanks!

yaelharel · 2019-02-28T21:35:37Z

/retest

repokitteh-read-only · 2019-02-28T21:35:40Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #6072 (comment) was created by @yaelharel.

see: more, trace.

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

dnoe

I still need to look at the test but flushing comments now to unblock you. Thanks!

include/envoy/filesystem/filesystem.h

source/common/api/api_impl.cc

source/common/api/api_impl.h

source/common/filesystem/io_file_error.h

source/common/filesystem/win32/filesystem_impl.cc

dnoe · 2019-03-01T15:49:30Z

source/common/filesystem/win32/filesystem_impl.cc

+    throw EnvoyException(fmt::format("Invalid path: {}", path));
+  }
+
+  std::ios::sync_with_stdio(false);


Does this change some global state?

This was copied from the existing Linux implementation:

envoy/source/common/filesystem/filesystem_impl.cc

Line 104 in 0c0bb54

std::ios::sync_with_stdio(false);

Looks like this has been there for a long long time! I'm actually not sure what it's meant to be doing here, since the documentation implies it only affects the standard streams like stdin, stdout, and stderr. But nothing in this function seems to be dealing with these streams.

@mattklein123 do you remember why this was added?

No clue. Can we remove this in a follow up on both sides?

source/common/filesystem/win32/filesystem_impl.cc

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io> Signed-off-by: Yael Harel <yharel@pivotal.io>

dnoe · 2019-03-06T20:52:18Z

Let me know whenever this is ready for another pass and I will take a look. Thanks!

yaelharel · 2019-03-07T15:21:03Z

Hi @dnoe, I think that we're ready for another review. Thanks!

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Natalie Arellano <narellano@pivotal.io>

Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Yael Harel <yharel@pivotal.io>

yaelharel · 2019-03-12T16:32:17Z

/retest

repokitteh-read-only · 2019-03-12T16:32:22Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #6072 (comment) was created by @yaelharel.

see: more, trace.

Envoy::Filesystem::InstanceImpl was splitted to Envoy::Filesystem::InstanceImplWin32 and Envoy::Filesystem::InstanceImplPosix Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Natalie Arellano <narellano@pivotal.io>

Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Yael Harel <yharel@pivotal.io>

yaelharel · 2019-03-12T21:37:04Z

Hi @dnoe, did you have a chance to review this PR again?
Thanks!

mhoran · 2019-03-19T13:35:46Z

Hey @alyssawilk @dnoe any thoughts on getting this merged? This is blocking the next track of work for Windows support. Thanks!

alyssawilk · 2019-03-19T16:03:38Z

I think this one is jmarantz@ and dpn@?

I'll ping them internally - sorry for the lag.

dnoe

LGTM - Thanks for making these changes.

mattklein123

LGTM with 2 small comments. Sorry for the delay.

/wait

mattklein123 · 2019-03-19T17:56:44Z

include/envoy/api/io_error.h

    InProgress,
    // Permission denied.
    Permission,
+    // Bad handle


This was covered in a different @danzh2010 PR, but can we remove this? This should never happen and should be covered with ASSERT/RELEASE_ASSERT if necessary depending on the situation..

mattklein123 · 2019-03-19T17:59:25Z

source/common/filesystem/win32/filesystem_impl.cc

+    throw EnvoyException(fmt::format("Invalid path: {}", path));
+  }
+
+  std::ios::sync_with_stdio(false);


No clue. Can we remove this in a follow up on both sides?

repokitteh-read-only · 2019-03-19T18:00:12Z

🙀 Error while processing event:

evaluation error
error: cannot load github.com/repokitteh/modules/lib/utils.star: load("github.com/repokitteh/modules/lib/utils.star") error: module load error

🐱

Caused by: a #6072 (review) was submitted by @mattklein123.

see: more, trace.

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io>

mattklein123

Thank you!

alyssawilk assigned dnoe Feb 27, 2019

jmarantz self-assigned this Feb 27, 2019

danzh2010 reviewed Feb 27, 2019

View reviewed changes

yaelharel mentioned this pull request Feb 27, 2019

Move IoError interface from Network to Api namespace #6104

Merged

Yael Harel added 2 commits February 28, 2019 12:05

Fix includes

5071a3c

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

Merge branch 'master' into filesystem-part-3

a8bfaaa

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

Merge branch 'master' into filesystem-part-3

d2f321e

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

dnoe reviewed Mar 1, 2019

View reviewed changes

Yael Harel and others added 4 commits March 1, 2019 12:28

Fix nits

08f5e47

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

add FileSharedImpl class, clean interface

7a4cd3a

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

Fix tsan

f69fe09

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

Merge branch 'master' into filesystem-part-3

4edccb5

Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io> Signed-off-by: Yael Harel <yharel@pivotal.io>

Yael Harel and others added 2 commits March 11, 2019 11:52

Merge branch 'master' into filesystem-part-3

5a4f87f

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Natalie Arellano <narellano@pivotal.io>

Merge master - continue

21c24e4

Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Yael Harel <yharel@pivotal.io>

Fix merge conflict:

aa19af7

Envoy::Filesystem::InstanceImpl was splitted to Envoy::Filesystem::InstanceImplWin32 and Envoy::Filesystem::InstanceImplPosix Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Natalie Arellano <narellano@pivotal.io>

yaelharel requested review from alyssawilk and mattklein123 as code owners March 12, 2019 18:18

Fix merge conflict

9c0dbc1

Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Yael Harel <yharel@pivotal.io>

mhoran mentioned this pull request Mar 19, 2019

NT support #129

Closed

dnoe previously approved these changes Mar 19, 2019

View reviewed changes

mattklein123 self-assigned this Mar 19, 2019

mattklein123 requested changes Mar 19, 2019

View reviewed changes

mattklein123 added the waiting label Mar 19, 2019

Remove BadHandle

17fe009

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io>

achasveachas dismissed dnoe’s stale review via 17fe009 March 19, 2019 19:26

repokitteh-read-only bot removed the waiting label Mar 19, 2019

mattklein123 approved these changes Mar 19, 2019

View reviewed changes

mattklein123 merged commit dc23262 into envoyproxy:master Mar 19, 2019

yangminzhu mentioned this pull request Apr 18, 2019

Update envoy sha and fix bulid break istio/proxy#2179

Merged

wrowe deleted the filesystem-part-3 branch December 24, 2019 17:25

Conversation

yaelharel commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Feb 27, 2019

Uh oh!

dnoe commented Feb 27, 2019

Uh oh!

jmarantz commented Feb 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaelharel commented Feb 27, 2019

Uh oh!

yaelharel commented Feb 28, 2019

Uh oh!

yaelharel commented Feb 28, 2019

Uh oh!

repokitteh-read-only bot commented Feb 28, 2019

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dnoe commented Mar 6, 2019

Uh oh!

yaelharel commented Mar 7, 2019

Uh oh!

yaelharel commented Mar 12, 2019

Uh oh!

repokitteh-read-only bot commented Mar 12, 2019

Uh oh!

yaelharel commented Mar 12, 2019

Uh oh!

mhoran commented Mar 19, 2019

Uh oh!

alyssawilk commented Mar 19, 2019

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Mar 19, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

yaelharel commented Feb 26, 2019 •

edited

Loading