Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix append-ness of new fd affecting old fds #5162

Merged
merged 4 commits into from
Oct 24, 2024

Conversation

yagehu
Copy link
Contributor

@yagehu yagehu commented Oct 20, 2024

This PR fixes a case where opening a new file descriptor with the append flag makes previously opened non-append fds append as well.

The fix is relatively simple, since the append-ness of an fd is already tracked in the virtual fs implementation, there's no need to open the actual file with the append flag.

This behavior is consistent across Linux and other Wasm runtimes.

fixes #5161

This commit fixes a case where opening a new file descriptor with the
append flag makes previously opened non-append fds append as well.

The fix is relatively simple, since the append-ness of an fd is already
tracked in the virtual fs implementation, there's no need to open the
actual file with the append flag.

This behavior is consistent across Linux and other Wasm runtimes.

fixes wasmerio#5161

Signed-off-by: Yage Hu <[email protected]>
Signed-off-by: Yage Hu <[email protected]>
@maminrayej
Copy link
Contributor

This PR seems to have some overlap with #5160. Let's first merge #5160 and then have only new changes in this PR.

@yagehu
Copy link
Contributor Author

yagehu commented Oct 22, 2024

This PR seems to have some overlap with #5160. Let's first merge #5160 and then have only new changes in this PR.

@maminrayej Updated!

Copy link
Contributor

@maminrayej maminrayej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one question though. handle.size() (size of the file from the file's perspective) should be the same as fd_entry.inode.stat.read().unwrap().st_size (size of the file from the perspective of the inode). is there a reason you changed it? did the test fail with handle.size()?

@yagehu
Copy link
Contributor Author

yagehu commented Oct 23, 2024

LGTM, just one question though. handle.size() (size of the file from the file's perspective) should be the same as fd_entry.inode.stat.read().unwrap().st_size (size of the file from the perspective of the inode). is there a reason you changed it? did the test fail with handle.size()?

Right. The test failed with handle.size() because it may be the old valid sometimes. I'm guessing because of async disk operation.

@maminrayej
Copy link
Contributor

Thanks.

@maminrayej maminrayej merged commit 6472633 into wasmerio:main Oct 24, 2024
71 of 72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening a file with append should not change existing fd offset
2 participants