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

Inherit Linux semantics for fd_pwrite with O_APPEND #8823

Merged

Conversation

alexcrichton
Copy link
Member

This commit updates the implementation of fd_pwrite in WASI to match Linux semantics for an under-specified corner of WASI. Specifically if fd_pwrite is used the offset specified is ignored if the file is opened in append mode and the bytes are instead appended.

This commit additionally refactors fd_write and fd_pwrite to have basically the same code with only a minor branch internally when the final write is being performed to help deduplicate more logic.

Closes #8817

@alexcrichton alexcrichton requested a review from a team as a code owner June 17, 2024 19:39
@alexcrichton alexcrichton requested review from fitzgen and removed request for a team June 17, 2024 19:39
@fitzgen fitzgen requested review from pchickey and removed request for fitzgen June 17, 2024 19:52
@fitzgen
Copy link
Member

fitzgen commented Jun 17, 2024

Redirecting to @pchickey

@alexcrichton alexcrichton added this pull request to the merge queue Jun 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 17, 2024
This commit updates the implementation of `fd_pwrite` in WASI to match
Linux semantics for an under-specified corner of WASI. Specifically if
`fd_pwrite` is used the offset specified is ignored if the file is
opened in append mode and the bytes are instead appended.

This commit additionally refactors `fd_write` and `fd_pwrite` to have
basically the same code with only a minor branch internally when the
final write is being performed to help deduplicate more logic.

Closes bytecodealliance#8817
@alexcrichton
Copy link
Member Author

CI turned up an interesting phenomena which makes sense in retrospect. With wasi-common the behavior is subject to whatever the platform OS provides which makes Linux the odd-one-out. Papering over OS differences is easy enough in wasmtime-wasi but is not trivial in wasi-common.

@pchickey how do you feel about sticking with these semantics and just ignoring the whole test in wasi-common? Alternatives could include not landing this in Wasmtime and taking it up in an issue with WASI and/or adding more stuff to wasi-common.

@pchickey
Copy link
Contributor

I think it is fine to just leave this bug in wasi-common and ignore the test - I don't think there is much value in bringing bug fixes for corner cases like this, if users need the fix the real question is why they havent upgraded to wasmtime-wasi.

@alexcrichton alexcrichton added this pull request to the merge queue Jun 17, 2024
Merged via the queue into bytecodealliance:main with commit aff28bf Jun 17, 2024
119 checks passed
@alexcrichton alexcrichton deleted the match-linux-for-fd-pwrite branch June 17, 2024 21:39
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.

File fd_pwrite bug
3 participants