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

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

tealsnow
Copy link
Contributor

@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

@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 added a commit to tealsnow/zig that referenced this pull request May 2, 2024
@tealsnow
Copy link
Contributor 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
andrewrk pushed a commit to tealsnow/zig that referenced this pull request Jul 21, 2024
@andrewrk andrewrk enabled auto-merge July 21, 2024 09:15
@andrewrk
Copy link
Member

Thanks!

tealsnow added 2 commits July 28, 2024 19:01
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.
@andrewrk andrewrk merged commit 390c7d8 into ziglang:master Jul 29, 2024
10 checks passed
@squeek502
Copy link
Collaborator

Just to clarify what happened here: this PR was rebased and auto-merge was turned on, but master was failing CI at the time. I just rebased again and CI passed so it was auto-merged.

SammyJames pushed a commit to SammyJames/zig that referenced this pull request Aug 7, 2024
igor84 pushed a commit to igor84/zig that referenced this pull request Aug 11, 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.

3 participants