Add fstat to OsSysCalls by ravenblackx · Pull Request #24298 · envoyproxy/envoy

ravenblackx · 2022-12-01T18:57:58Z

Commit Message: Add fstat to OsSysCalls
Additional Description: Just another os call wrapper. This functionality will be useful in file system cache implementation.
Risk Level: Very low, not changing any production code paths.
Testing: Added a unit test covering the new function and several others.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: Identical implementation for Posix and Windows.

Signed-off-by: Raven Black <ravenblack@dropbox.com>

(.circleci yaml CI failure) Signed-off-by: Raven Black <ravenblack@dropbox.com>

Signed-off-by: Raven Black <ravenblack@dropbox.com>

jmarantz

nice. lgtm with one nit.

jmarantz · 2022-12-05T13:58:17Z

test/common/api/os_sys_calls_test.cc

@@ -1,9 +1,65 @@
+#include <fcntl.h>


do you need this? Aren't we working entirely with the OSSysCalls abstraction?

Yeah, it won't build on Windows without it because O_CREAT and O_READWR aren't defined without that header.

that makes sense. The completist in me makes me want to contemplate having an enum layer in OSSysCalls, but I don't want to burden you with that one here and maybe it's overkill; not sure.

In the meantime can you just add a line-comment like // needed for O_CREAT and O_READRW or similar to this include-line?

Done. (And I agree - also a real cleanup would do something with unifying this and Alyssa's file abstraction, which has an enum mapping but only for some of the options and in a way that precludes reusing it here. Probably the right solution would be to add all the file operation functionality to the file abstraction, and migrate AsyncFileManager to using that abstraction - that way the Windows side could use HANDLE objects and the functions that work with them, which would mean supporting pwrite and pread style operations would become possible like it currently is not - Windows doesn't have thread-safe read-at-position functions that work on file descriptors.)

jmarantz · 2022-12-05T13:59:40Z

format issues also

/wait

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx · 2022-12-06T19:18:08Z

/retest

repokitteh-read-only · 2022-12-06T19:18:17Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #24298 (comment) was created by @ravenblackx.

see: more, trace.

Signed-off-by: Raven Black <ravenblack@dropbox.com>

jmarantz

thanks!

alyssawilk · 2022-12-07T17:02:37Z

source/common/api/win32/os_sys_calls_impl.cc

-  const int rc = ::closesocket(fd);
-  return {rc, rc != -1 ? 0 : ::WSAGetLastError()};
+  int rc = ::closesocket(fd);
+  const int socket_err = ::WSAGetLastError();


this doesn't appear to be called out in the release note. is it tested in the new test?

Discussed in Slack; in existing code close is only called where closesocket behavior is fine. My hope is to later change this back and make it explicitly closesocket (for all platforms), and remove everything I've done here after implementing [fstat, pread, pwrite, tmpfiles] in Envoy::Filesystem::File and migrating AsyncFileManager to using that. But getting it in here allows me to make progress elsewhere while that chain of changes is in flight.

alyssawilk · 2022-12-07T17:06:12Z

test/common/api/os_sys_calls_test.cc

 namespace Envoy {

+// Test happy path for `open`, `pwrite`, `pread`, `fstat`, `close`, `stat` and `unlink`.
+TEST(OsSyscallsTest, OpenPwritePreadFstatCloseStatUnlink) {


ugh I really want to break these out but you can't really fully test read without write/open, ditto fstat. so yeah can leave as is but if fully testing close requires tweaking anything let's at least add line breaks between the different sections. if close is tested we can merge as-is

Close is [kind of] tested, in that this new test tests the new path, and existing tests implicitly cover the old path. But it turns out Windows doesn't like to unlink a file that was created without explicit windows-specific permissions flags, so that shot the auto-merge. :(

Mostly just to trigger CI to retry Signed-off-by: Raven Black <ravenblack@dropbox.com>

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx · 2022-12-09T15:36:31Z

/retest
(unrelated flakes)

repokitteh-read-only · 2022-12-09T15:36:40Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #24298 (comment) was created by @ravenblackx.

see: more, trace.

ravenblackx · 2022-12-09T19:33:37Z

/retest
(Different unrelated flakes.)

repokitteh-read-only · 2022-12-09T19:33:47Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #24298 (comment) was created by @ravenblackx.

see: more, trace.

ravenblackx · 2022-12-09T22:14:54Z

/retest
Hoping to get luckier on that first flake. #24469 should land eventually if not!

repokitteh-read-only · 2022-12-09T22:15:05Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #24298 (comment) was created by @ravenblackx.

see: more, trace.

ravenblackx added 6 commits December 1, 2022 18:53

Add fstat to OsSysCalls

017797b

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Add test comments for readability

9e8d1b4

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Merge remote-tracking branch 'upstream/main' into fstat

f29cf99

(.circleci yaml CI failure) Signed-off-by: Raven Black <ravenblack@dropbox.com>

Satisfy the spelling demon

ec3358f

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Include fcntl.h for O_CREATE etc on windows

9bd5d0a

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Implement for windows, and test-cover pread and pwrite

13d2137

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx changed the title ~~Add fstat to OsSysCalls~~ Add fstat to OsSysCalls, and pread and pwrite for Windows Dec 2, 2022

ravenblackx changed the title ~~Add fstat to OsSysCalls, and pread and pwrite for Windows~~ Add fstat to OsSysCalls Dec 2, 2022

ravenblackx added 2 commits December 2, 2022 22:33

no pread and pwrite for Windows after all; make the test not use them

660c8db

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Catch failure of write in the windows test path

46afa09

Signed-off-by: Raven Black <ravenblack@dropbox.com>

alyssawilk assigned jmarantz Dec 5, 2022

jmarantz reviewed Dec 5, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 5, 2022

spell function names with backticks

64a6ab8

Signed-off-by: Raven Black <ravenblack@dropbox.com>

repokitteh-read-only bot removed the waiting label Dec 5, 2022

ravenblackx added 5 commits December 5, 2022 16:31

Explain fcntl.h, and make it only included on Win32

4ece430

Signed-off-by: Raven Black <ravenblack@dropbox.com>

check_format insists on adding this newline

5458415

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Make Win32 OsSysCalls::close work on file descriptors, not just sockets

6dfddbb

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Backtick the spelling demon again

8a8b5c9

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Remove accidental ifndef

37d6ca0

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx added 2 commits December 6, 2022 21:18

Return code no longer const

bdbf2ea

Signed-off-by: Raven Black <ravenblack@dropbox.com>

Merge remote-tracking branch 'upstream/main' into fstat

3efd132

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx requested a review from jmarantz December 7, 2022 15:26

jmarantz previously approved these changes Dec 7, 2022

View reviewed changes

jmarantz assigned alyssawilk Dec 7, 2022

alyssawilk reviewed Dec 7, 2022

View reviewed changes

alyssawilk enabled auto-merge (squash) December 7, 2022 17:14

ravenblackx added 2 commits December 8, 2022 16:41

Merge remote-tracking branch 'upstream/main' into fstat

4b0ef0d

Mostly just to trigger CI to retry Signed-off-by: Raven Black <ravenblack@dropbox.com>

Add permission mode to open in test for Windows

7e9d582

Signed-off-by: Raven Black <ravenblack@dropbox.com>

auto-merge was automatically disabled December 8, 2022 20:39
Head branch was pushed to by a user without write access

ravenblackx dismissed jmarantz’s stale review via 7e9d582 December 8, 2022 20:39

alyssawilk approved these changes Dec 8, 2022

View reviewed changes

alyssawilk enabled auto-merge (squash) December 8, 2022 21:00

alyssawilk merged commit ceca213 into envoyproxy:main Dec 9, 2022

ravenblackx deleted the fstat branch December 12, 2022 16:07

Conversation

ravenblackx commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz 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!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Dec 5, 2022

Uh oh!

ravenblackx commented Dec 6, 2022

Uh oh!

repokitteh-read-only bot commented Dec 6, 2022

Uh oh!

jmarantz 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!

Choose a reason for hiding this comment

Uh oh!

ravenblackx commented Dec 9, 2022

Uh oh!

repokitteh-read-only bot commented Dec 9, 2022

Uh oh!

ravenblackx commented Dec 9, 2022

Uh oh!

repokitteh-read-only bot commented Dec 9, 2022

Uh oh!

ravenblackx commented Dec 9, 2022

Uh oh!

repokitteh-read-only bot commented Dec 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ravenblackx commented Dec 1, 2022 •

edited

Loading