Add file size to DirectoryEntry #24176

ravenblackx · 2022-11-23T16:35:54Z

Commit Message: Add file size to DirectoryEntry
Additional Description: Directory [find] iterator is potentially useful for file system cache and, in future, serving content directly from file system. The iterator already has access to file size from stat() or its Windows equivalent, but it is not accessible to the caller. Using the current available APIs, the caller would have to make an additional call to stat for each file they wish to know the size of. This change exposes the file size through the existing API, sparing those additional OS calls in the case that the size is useful, at the cost of a small value-copy.
In passing, fixes a mistaken use of errno that should have been the error number from the os-call's returned structure, and updates a throw-test to not require a nominally private function to be public.
Risk Level: Low. Simple change, existing fields should be unchanged.
Testing: Tests updated to match new values, one test added to validate that file size is returned.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: Updated both win32 and posix versions.

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

repokitteh-read-only · 2022-11-23T16:35:59Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24176 was opened by ravenblackx.

see: more, trace.

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

ravenblackx · 2022-11-24T00:09:01Z

/retest

repokitteh-read-only · 2022-11-24T00:09:08Z

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

🐱

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

see: more, trace.

jmarantz

lgtm with a few nits.

envoy/filesystem/filesystem.h

source/common/filesystem/posix/directory_iterator_impl.cc

test/common/filesystem/directory_test.cc

jmarantz · 2022-11-28T03:23:52Z

/wait

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

jmarantz

lgtm modulo minor remaining nits.

envoy/filesystem/filesystem.h

test/common/filesystem/directory_test.cc

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

test/common/filesystem/directory_test.cc

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

ravenblackx · 2022-11-29T22:12:38Z

/retest

repokitteh-read-only · 2022-11-29T22:12:45Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

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

see: more, trace.

jmarantz

/assign-from envoyproxy/senior-maintainers

repokitteh-read-only · 2022-11-30T14:39:02Z

envoyproxy/senior-maintainers assignee is @snowp

🐱

Caused by: a #24176 (review) was submitted by @jmarantz.

see: more, trace.

…mlinks, Linux has actual file size symlinks. Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx · 2022-12-01T15:22:25Z

/retest

repokitteh-read-only · 2022-12-01T15:22:34Z

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

🐱

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

see: more, trace.

(Maybe fix unrelated test failures.) Signed-off-by: Raven Black <ravenblack@dropbox.com>

envoy/filesystem/filesystem.h

ravenblackx · 2022-12-02T15:54:33Z

/retest

repokitteh-read-only · 2022-12-02T15:54:44Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

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

see: more, trace.

snowp

Overall lgtm, just one suggestion

snowp · 2022-12-05T13:53:57Z

envoy/filesystem/filesystem.h

+  // The file size in bytes for regular files. Should not be relied on for directories,
+  // symlinks, or FileType::Other.
+  uint64_t size_bytes_;


Any way we can make this less of a footgun? One thing that comes to mind is wrapping it in a call that ASSERTs that type_ is Regular? Or perhaps making it a absl::optional?

Asserting that type_ is Regular doesn't actually solve the problem, because on Windows with a symlink the type is regular and the size is unknown (or maybe it's the size of the linked path, I forget - either way, not the size of the file).

Using absl::optional is a good idea though, to make it a hard failure instead of a quiet failure if size is used when it shouldn't be.

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

alyssawilk

Looks good overall!
/wait

alyssawilk · 2022-12-06T14:05:07Z

source/common/filesystem/win32/directory_iterator_impl.cc

  }

  return *this;
 }

-FileType DirectoryIteratorImpl::fileType(const WIN32_FIND_DATA& find_data) const {
+DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data) {
+  ULARGE_INTEGER file_size;


should this all be in the final else?

Good call, now that it's unset everywhere else!

alyssawilk · 2022-12-06T14:05:42Z

test/common/filesystem/directory_test.cc

@@ -15,6 +15,17 @@
 namespace Envoy {
 namespace Filesystem {

+// NOLINTNEXTLINE(readability-identifier-naming)
+void PrintTo(const DirectoryEntry& entry, std::ostream* os) {


sorry if I'm missing something obvious but why NOLINTNEXTLINE? Can you not just have Envoy standard naming?

PrintTo is a weird template override for test output of objects, so it's stuck using Google conventions. (If it was printTo then it wouldn't override.)

alyssawilk · 2022-12-06T14:07:34Z

test/common/filesystem/directory_test.cc

+      EXPECT_TRUE(file) << "failed to open test file";
+      file << contents;
+      file.close();
+      EXPECT_TRUE(file) << "failed to write to test file";


sorry ignorance of ostream but if you fail to write will file become null/false?

It becomes "falsy", because it overrides the bool-cast operator. Can't be null or false, because it's not a pointer.

alyssawilk · 2022-12-06T14:08:25Z

envoy/filesystem/filesystem.h

  bool operator==(const DirectoryEntry& rhs) const {
-    return name_ == rhs.name_ && type_ == rhs.type_;
+    return name_ == rhs.name_ && type_ == rhs.type_ && size_bytes_ == rhs.size_bytes_;


sorry if I'm missing but did we test this new equivalence?

We did now.

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

addressed

….h-into-multiple-header-files * origin/main: Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) Signed-off-by: JP Simard <jp@jpsim.com>

…-cpp-to-latest-version * origin/main: (23 commits) Reduce Route memory utilization by avoiding RuntimeData instances when not needed (#24327) build: fix compile error for mac (#24429) postgres: support for upstream SSL (#23990) iOS: split `EnvoyEngine.h` into multiple header files (#24397) mobile: check for pending exceptions after JNI call (#24361) Remove uneccessary `this->` from mobile engine builder (#24389) Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) ... Signed-off-by: JP Simard <jp@jpsim.com>

Add file size to DirectoryEntry

60306a9

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

ravenblackx added 4 commits November 23, 2022 17:17

Add test to validate size

2a19b95

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

Fix tests

19c8a58

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

Fix mistaken const

9cc86ca

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

makeEntry test needs to be filesystem-independent

331c5af

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

ravenblackx marked this pull request as ready for review November 24, 2022 03:18

jmarantz self-assigned this Nov 25, 2022

jmarantz reviewed Nov 28, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 28, 2022

ravenblackx added 3 commits November 28, 2022 15:23

Drop unused temps, clarify the final FileType case.

47323f2

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

TODO for not throwing exception.

78d3c79

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

Assert success of test file operations

2a6b3ab

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

repokitteh-read-only bot removed the waiting label Nov 28, 2022

jmarantz reviewed Nov 29, 2022

View reviewed changes

envoy/filesystem/filesystem.h Outdated Show resolved Hide resolved

test/common/filesystem/directory_test.cc Show resolved Hide resolved

ravenblackx added 4 commits November 29, 2022 15:40

Fix operator==, improve test output, simplify flow

6037bb9

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

ASSERT->EXPECT

99a7632

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

Windows style large integers without byte shuffling

2ce171d

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

*Unsigned* large integer

2eecbfc

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

jmarantz reviewed Nov 29, 2022

View reviewed changes

test/common/filesystem/directory_test.cc Show resolved Hide resolved

Turn off linter for PrintTo

4e840cd

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

jmarantz previously approved these changes Nov 30, 2022

View reviewed changes

repokitteh-read-only bot assigned snowp Nov 30, 2022

Allow indeterminate size for non-regular files; Windows has size-0 sy…

5acaaf7

…mlinks, Linux has actual file size symlinks. Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx dismissed jmarantz’s stale review via 5acaaf7 November 30, 2022 18:44

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

b582141

(Maybe fix unrelated test failures.) Signed-off-by: Raven Black <ravenblack@dropbox.com>

jmarantz previously approved these changes Dec 2, 2022

View reviewed changes

envoy/filesystem/filesystem.h Outdated Show resolved Hide resolved

jmarantz assigned alyssawilk and unassigned snowp Dec 5, 2022

snowp previously requested changes Dec 5, 2022

View reviewed changes

Use absl::nullopt to indicate size unknown

ae4ae1f

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

ravenblackx dismissed jmarantz’s stale review via ae4ae1f December 5, 2022 16:21

Correct test comment that still said 0

0257b05

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

ravenblackx requested a review from snowp December 5, 2022 16:23

alyssawilk reviewed Dec 6, 2022

View reviewed changes

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

ravenblackx added 2 commits December 6, 2022 20:47

Only get size when used, and fix grouping, for Win32

de66291

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

Add testing of DirectoryEntry == operator

f11bd48

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

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

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

6c3d3b4

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

alyssawilk approved these changes Dec 6, 2022

View reviewed changes

alyssawilk enabled auto-merge (squash) December 6, 2022 22:00

alyssawilk merged commit 75d06fb into envoyproxy:main Dec 7, 2022

ravenblackx mentioned this pull request Dec 9, 2022

Deflake directory iterator for windows #24469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file size to DirectoryEntry #24176

Add file size to DirectoryEntry #24176

ravenblackx commented Nov 23, 2022 •

edited

Loading

repokitteh-read-only bot commented Nov 23, 2022

ravenblackx commented Nov 24, 2022

repokitteh-read-only bot commented Nov 24, 2022

jmarantz left a comment

jmarantz commented Nov 28, 2022

jmarantz left a comment

ravenblackx commented Nov 29, 2022

repokitteh-read-only bot commented Nov 29, 2022

jmarantz left a comment

repokitteh-read-only bot commented Nov 30, 2022

ravenblackx commented Dec 1, 2022

repokitteh-read-only bot commented Dec 1, 2022

ravenblackx commented Dec 2, 2022

repokitteh-read-only bot commented Dec 2, 2022

snowp left a comment

snowp Dec 5, 2022

ravenblackx Dec 5, 2022

alyssawilk left a comment

alyssawilk Dec 6, 2022

ravenblackx Dec 6, 2022

alyssawilk Dec 6, 2022

ravenblackx Dec 6, 2022

alyssawilk Dec 6, 2022

ravenblackx Dec 6, 2022

alyssawilk Dec 6, 2022

ravenblackx Dec 6, 2022

Add file size to DirectoryEntry #24176

Add file size to DirectoryEntry #24176

Conversation

ravenblackx commented Nov 23, 2022 • edited Loading

repokitteh-read-only bot commented Nov 23, 2022

ravenblackx commented Nov 24, 2022

repokitteh-read-only bot commented Nov 24, 2022

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz commented Nov 28, 2022

jmarantz left a comment

Choose a reason for hiding this comment

ravenblackx commented Nov 29, 2022

repokitteh-read-only bot commented Nov 29, 2022

jmarantz left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Nov 30, 2022

ravenblackx commented Dec 1, 2022

repokitteh-read-only bot commented Dec 1, 2022

ravenblackx commented Dec 2, 2022

repokitteh-read-only bot commented Dec 2, 2022

snowp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravenblackx commented Nov 23, 2022 •

edited

Loading