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.

Sign up for GitHub

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

Jump to bottom

Portably handle fd_advise on directory fd #2875

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Portably handle fd_advise on directory fd #2875

merged 3 commits into from
Dec 7, 2023

Conversation

yagehu
Copy link
Contributor

@yagehu yagehu commented Dec 6, 2023

This commit adds a check to fd_advise. If the fd is a directory, return ebadf. This brings iwasm in line with Wasmtime's behavior. WASI folks have stated that fd_advise should not work on directories as this is a Linux-specific behavior.

yagehu and others added 3 commits December 5, 2023 20:31
@yagehu
Portably handle fd_advise on directory fd
aa928ee 
This commit adds a check to `fd_advise`.  If the fd is a directory,
return `ebadf`.  This brings iwasm in line with Wasmtime's behavior.
WASI folks have stated that fd_advise should not work on directories
as this is a Linux-specific behavior.
@yagehu
Merge branch 'bytecodealliance:main' into yagehu/fd-advise-dir
2fd8b45
@yagehu
Fix test failure
305aa5e
wenyongh
wenyongh reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh
Copy link
Contributor

wenyongh commented Dec 7, 2023

@yagehu thanks for the fixing! Let's merge the PR.

@wenyongh wenyongh merged commit 0b332d8 into bytecodealliance:main Dec 7, 2023
383 checks passed
@yamt
Copy link
Collaborator

yamt commented Dec 7, 2023

This commit adds a check to fd_advise. If the fd is a directory, return ebadf. This brings iwasm in line with Wasmtime's behavior. WASI folks have stated that fd_advise should not work on directories as this is a Linux-specific behavior.

where in WASI or POSIX is it stated?

@yagehu
Copy link
Contributor Author

yagehu commented Dec 7, 2023

This commit adds a check to fd_advise. If the fd is a directory, return ebadf. This brings iwasm in line with Wasmtime's behavior. WASI folks have stated that fd_advise should not work on directories as this is a Linux-specific behavior.

where in WASI or POSIX is it stated?

The dirfd behavior is not specified by POSIX. I submitted this PR observing that using directory fds with fd_* WASI calls fail (with different errnos) in other runtimes (Wasmtime and WasmEdge). Wasmtime has a conformance test asserting this behavior.

@yamt
Copy link
Collaborator

yamt commented Dec 7, 2023

This commit adds a check to fd_advise. If the fd is a directory, return ebadf. This brings iwasm in line with Wasmtime's behavior. WASI folks have stated that fd_advise should not work on directories as this is a Linux-specific behavior.

where in WASI or POSIX is it stated?

The dirfd behavior is not specified by POSIX. I submitted this PR observing that using directory fds with fd_* WASI calls fail (with different errnos) in other runtimes (Wasmtime and WasmEdge). Wasmtime has a conformance test asserting this behavior.

my impression is that it should be fixed in wasmtime instead.

@yagehu yagehu deleted the yagehu/fd-advise-dir branch December 9, 2023 06:42
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
@yagehu
Portably handle fd_advise on directory fd (bytecodealliance#2875)
20c6f74 
This commit adds a check to `fd_advise`.  If the fd is a directory,
return `ebadf`.  This brings iwasm in line with Wasmtime's behavior.
WASI folks have stated that fd_advise should not work on directories
as this is a Linux-specific behavior:
bytecodealliance/wasmtime#6505 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wenyongh wenyongh wenyongh left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yagehu @wenyongh @yamt