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

std.posix: handle INVAL in openZ, openatZ and openatWasi #19833

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tealsnow
Copy link

@tealsnow tealsnow commented May 1, 2024

Contributes to #15607

Although the case is not handled in openatWasi (as I could not get a
working wasi environment to test the change) I have added a FIXME
addressing it and linking to the issue.

Edit: Handled case in openatWasi too

Contributes to ziglang#15607

Although the case is not handled in `openatWasi` (as I could not get a
working wasi environment to test the change) I have added a FIXME
addressing it and linking to the issue.
@squeek502
Copy link
Collaborator

squeek502 commented May 1, 2024

Using the createfile.zig example from #15607 on a FAT filesystem, compiled with:

zig build-exe createfile.zig -target wasm32-wasi

Ran with:

wasmtime --dir . createfile.wasm

(note: openatWasi is used for Dir.createFile)

  • When the host is Windows, NOENT is returned from wasi.path_open (on both NTFS and FAT filesystems)
  • When the host is Linux, INVAL is returned from wasi.path_open

So, .INVAL => return error.BadPathName, seems like the way to go for now in openatWasi


Side note: INVAL can be returned for more reasons than just characters that are unsupported by the underlying filesystem. We might eventually want to take that into account, but I think mapping it to BadPathName for now is fine.

@tealsnow
Copy link
Author

tealsnow commented May 2, 2024

Side note: INVAL can be returned for more reasons than just characters that are unsupported by the underlying filesystem. We might eventually want to take that into account, but I think mapping it to BadPathName for now is fine.

I'm interested in investigating these other cases and handling them properly. I found some documentation on such cases here. Are you aware of any other cases?

@squeek502
Copy link
Collaborator

squeek502 commented May 2, 2024

Those are the cases I was referring to. However, each operating system may also behave differently; that link is just for Linux.

For example, it looks like FreeBSD doesn't document what happens in this case (the only documented EINVAL return is about invalid flag combinations). Would likely need to test mounting a FAT32 drive and seeing what happens when you try to create a file with an illegal character.

So, the idea I had in mind would be something like:

  • Check the documented EINVAL cases for as many operating systems as possible
  • Check the actual behavior of attempting to create a file with an illegal character on a mounted FAT32 drive on as many operating systems as possible

Then, we'd try figure out if there are any commonalities, and potentially whether or not it would make sense to translate EINVAL to a broader error than BadPathName.

It also might make sense to handle EINVAL with a switch, something like:

.INVAL => switch (native_os) {
    .linux => return error.BadPathName,
    .freebsd => unreachable, // just a hypothetical, if it turns out that FreeBSD doesn't use INVAL for the illegal character case
    // etc
},

but, again, I feel like we would need more information about how different OSes handle the situation to determine what the switch cases should actually do. In the meantime, I think the changes in this PR are a step in the right direction.


These are just my initial thoughts on this, though. Addressing #15607 has been on my todo list for a while but I haven't actually put any work into it yet.

@tealsnow tealsnow changed the title std.posix: handle INVAL in openZ and openatZ std.posix: handle INVAL in openZ, openatZ and openatWasi May 2, 2024
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.

None yet

2 participants