regular_file.c: don't treat 0 as a bad file descriptor#2616
Merged
sporksmith merged 2 commits intoshadow:mainfrom Dec 16, 2022
Merged
regular_file.c: don't treat 0 as a bad file descriptor#2616sporksmith merged 2 commits intoshadow:mainfrom
sporksmith merged 2 commits intoshadow:mainfrom
Conversation
I wanted to avoid comparing with equality to OSFILE_INVALID everywhere, in case we ever want to add other negative error values. Added an inline helper `_fd_isValid` instead. Most of these checks could probably be omitted altogether in favor of letting the native file operation fail and return EBADF for us or us later failing to find the descriptor, but probably not worth changing it right now. Fixes shadow#2604
fe02cf0 to
27a5b70
Compare
Codecov ReportBase: 67.62% // Head: 66.99% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2616 +/- ##
==========================================
- Coverage 67.62% 66.99% -0.63%
==========================================
Files 200 200
Lines 29279 29279
Branches 5759 5759
==========================================
- Hits 19800 19616 -184
- Misses 4887 5092 +205
+ Partials 4592 4571 -21
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
d5f55ff to
b67bf58
Compare
robgjansen
approved these changes
Dec 16, 2022
In particular having them before the FILE_TYPE_IN_MEMORY processing caused those operations to always fail, since those files don't have a valid descriptor. We could drop most of these checks altogether and just let the native calls fail with EBADFD themselves, but there'd be some risk of a performance regression since we'd no longer be skipping a native syscall in those cases.
b67bf58 to
31e538d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I wanted to avoid comparing with equality to OSFILE_INVALID everywhere, in case we ever want to add other negative error values. Added an inline helper
_fd_isValidinstead.Most of these checks could probably be omitted altogether in favor of letting the native file operation fail and return EBADF for us or us later failing to find the fd in our descriptor table, but probably not worth changing it right now.
Fixes #2604