Extract file open mode from OpenFileOp #3259
Conversation
kislaykishore
requested review from
a team and
abhishek10004
and removed request for
a team
anushka567
force-pushed
the
file-open-method
branch
2 times, most recently
from
94c1fa1 to
2c2f09f
Compare
anushka567
force-pushed
the
file-open-method
branch
from
2c2f09f to
040acc6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3259 +/- ##
==========================================
+ Coverage 78.60% 78.84% +0.23%
==========================================
Files 128 130 +2
Lines 17657 17781 +124
==========================================
+ Hits 13880 14019 +139
+ Misses 3317 3296 -21
- Partials 460 466 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
anushka567
changed the title
internal/fs/util/file_open_modes.go
Outdated
| switch { | ||
| case op.OpenFlags.IsAppend(): | ||
| return Append | ||
| case op.OpenFlags.IsWriteOnly(): |
There was a problem hiding this comment.
IsWriteOnly is true only for w mode. what about w+ and r+ modes?
Have you tested all the file open modes manually. is this function returning data correctly?
There was a problem hiding this comment.
There was confusion earlier, Updated the code so that,
- a/a+ fall under append.
- r under read
- w/w+/r+ fall under write mode.
I have checked manually to validate that correct mode is being propagated.
internal/fs/util/file_open_modes.go
Outdated
| // Append mode is returned for both 'a' and 'a+' flags as the kernel handles the distinction. | ||
| // Similarly, read/write ('r+') mode is implicitly handled by the kernel, so this function | ||
| // focuses on read-only, write-only, and append modes. | ||
| func DetermineFileOpenMode(op *fuseops.OpenFileOp) OpenMode { |
There was a problem hiding this comment.
Please add unit tests to this file.
There was a problem hiding this comment.
Adding unit tests is infeasible as the function takes the OpenFileOp argument, and the method's functionality is tested based on the openflags which is of the type fusekernel.OpenFlags which is private and cannot be accessed or initialized directly in GCSFuse.
internal/fs/util/file_open_modes.go
Outdated
| // It returns Read, Write, or Append. | ||
| // | ||
| // Append mode is returned for both 'a' and 'a+' flags as the kernel handles the distinction. | ||
| // Similarly, read/write ('r+') mode is implicitly handled by the kernel, so this function |
There was a problem hiding this comment.
This comment is not clear.
r+/w/w+ will return as write mode. Wherther reads should be supported or not is implicitly handled by the kernel. So we need not differentiate.
There was a problem hiding this comment.
Noted. I have updated the comment, was confused earlier.
internal/fs/util/file_open_modes.go
Outdated
| ) | ||
|
|
||
| // OpenMode represents the file access mode. | ||
| type OpenMode int |
There was a problem hiding this comment.
Should we call it FileOpenMode so that aliasing the import with different name is not required.
There was a problem hiding this comment.
I have moved the util file to internal/util so we do not have to care about aliasing here.
anushka567
force-pushed
the
file-open-method
branch
from
bf5c763 to
1d4106f
Compare
Description
This PR introduces the method for determining the access mode for the file being opened. This was necessary to support appends to existing files by extending the streaming writes support for files opened via a/a+ mode.
Note: Required changes merged.
Blocked on this PR in jacobsa/fuseLink to the issue in case of a bug fix.
b/414135351
Testing details
Any backward incompatible change? If so, please explain.