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

Reconsider MUST on "." and ".." entries in directory listings #52

Closed
codefromthecrypt opened this issue Jan 31, 2023 · 7 comments · Fixed by WebAssembly/WASI#516
Closed

Comments

@codefromthecrypt
Copy link

The recently added rust tests include tests that summarize as directory listings MUST return "." and ".." entires https://github.com/WebAssembly/wasi-testsuite/blob/main/tests/rust/src/bin/fd_readdir.rs#L150-L153

I think this should be backed by specification about why and also edge cases. For example, should a pre-open return ".." at any time? Should it return ".." if its pre-open name is "/"? What about if the pre-open name is ".."?

This is particularly of interest with virtual filesystems, which may not have a ".." notion at all. Meanwhile at least zig and go guests filter out "." and ".." from listings, but to synthetically produce ".." based on the above cases is not only extra state but also futile when they are discarded.

My opinion is that the testsuite should not introduce new rules that are not documented in the spec itself, and so if this should be a MUST, it should be specified first, including edge cases. In other words, this test should be reverted or switched to a MAY. If folks believe this should be a MUST then we need to move this issue to the proper forum to debate it and the implications diversely, inclusive of people who represent Go and Ziglang communities.

@sunfishcode
Copy link
Member

Rust also filters out . and .., and in general I encourage you to consider a level of patience. We have finite resources, and filesystems have an immense surface area of innumerable details, so it's inevitable that wasi-testsuite is going to be the place where some questions like this are going to arise.

Filtering out . and .. makes sense to me. The current filesystem API includes . and .. because it's what POSIX does. However, indeed, it's not especially useful behavior. I suspect it doesn't make sense to change this for preview1 because existing applications may be depending on the existing behavior. But for preview2, we can certainly consider this change; would you be interested in filing an issue in the wasi-filesystem repo, which is the proper forum to file such issues in?

@codefromthecrypt
Copy link
Author

I think that saying users are depending on this behavior maybe applies to wasmtime, but not others who filter it out. It feels like lifting behaviors of wasmtime immutable into this repo isn't the worst choice, but also not the best. So, basically I'll ask again if we can change these tests to MAY if it is required for wasmtime to continue that behavior. Would that be acceptable?

@codefromthecrypt
Copy link
Author

WebAssembly/wasi-filesystem#90 for forward, and hoping after reflection you can reconsider the opinion here. End users and runtime owners also have time pressures made worse both by ambiguity and the arbitrary.

@codefromthecrypt
Copy link
Author

@johanbrandhorst I know you are helping on the WASI proposal in Go, which is starting with snapshot-01 for pragmatic reasons. Since you are tighter in Go than me, copying you on this. I'm curious your opinion and my current thought is to fork the tests and edit them. If this repo allows test behavior contributions from other projects, we can raise that as a PR once things get more open.

the long part

In wazero, you know we use Go as a runtime, which is unlike most, what's also unlike most is we don't use LLVM. We tend to find implicit bias in implementation. This repo is an attempt to specify what wasi snapshot-01 was supposed to be, though there's no effort to document it. It seems mainly a tool to bridge to the next, but the point remains that this is literally a w3c repo named testsuite. We'd like to pass things here, but also not ignore reality on conflict.

Effectively nothing is documented in specification, yet tests are being written. They seem to favor wasmtime as for example the tests were lifted from there with so far no interest in modification based on reality of other languages/runtimes.

For example, in Go directory entries of "." and ".." are filtered out before we can read them in os.Dir, yet the main test here for reading a directory requires them to be present. This is literally the code in os/dir_darwin.go. We can't know if they existed or not.

		// Check for useless names before allocating a string.
		if string(name) == "." || string(name) == ".." {
			continue
		}

I think that possibly if there were multiple spec owners someone would agree with me that it is dubious to require this at the WASI abstraction, and in any case the role of a spec steward includes making those specs relevant, even specs that are inconvenient when one wishes to rally folks towards a new incomplete alternative. I think these pressures will result in issues like this, IOTW this may not be the only time this pops up.

To think sustainably about it, give wazero's first priority is its users, not trying to reverse engineer directories to match this, we have a few options.

  • ignore all tests here
    • go back to our existing tinygo/zig, etc behavioral tests which prove end user code is likely to work and still exist
  • disable this test with a comment
    • here and anywhere else, we basically edit the test locally and make sure the normal parts work, but disable it. This allows us to re-use pre-built binaries
  • edit and commit this test to a fork until it changes
    • this repo is fast changing, but a fork implies maintenance. We have maintenance anyway as we are not in the list of runners here (hence this issue really). We would have to rebuild the wasm and not able to re-use theirs anymore.

WDYT?

@codefromthecrypt
Copy link
Author

As I'm working on a test filter meanwhile, I wanted to cite that I can't find a place in POSIX that is at odds with my suggestion to make this a MAY vs a MUST. If we are saying POSIX documents this in lieu of WASI documenting it, I think the spec agrees they may be returned, but aren't required to.

The readdir() function shall not return directory entries containing empty names. If entries for dot or dot-dot exist, one entry shall be returned for dot and one entry shall be returned for dot-dot; otherwise, they shall not be returned.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html

I'm happy to modify the tests here accordingly if given the OK from @sunfishcode it would be accepted

codefromthecrypt pushed a commit to tetratelabs/wazero that referenced this issue Feb 5, 2023
The current fd_readdir test is invalid as it requires dot and dot-dot
entries, which both aren't required by POSIX nor returned by go. Once
this is addressed upstream, we can remove the skip.

See WebAssembly/wasi-testsuite#52
See #1036

Signed-off-by: Adrian Cole <[email protected]>
codefromthecrypt added a commit to tetratelabs/wazero that referenced this issue Feb 5, 2023
The current fd_readdir test is invalid as it requires dot and dot-dot
entries, which both aren't required by POSIX nor returned by go. Once
this is addressed upstream, we can remove the skip.

See WebAssembly/wasi-testsuite#52
See #1036

Signed-off-by: Adrian Cole <[email protected]>
@sunfishcode
Copy link
Member

I think that wording refers to the fact that POSIX technically permits implementations to avoid assigning any special meaning to . and .., but when even POSIX calls something "historical", you know it's truly ancient 😉.

As I said above, I agree with you about changing this going forward. And as I said above, you're not the only one here implementing these features in a language whose standard library filters out . and ...

And as I said above, the main consideration that I see for Preview1 is compatibility. I said POSIX, but it appears strictly speaking I should have said all popular POSIX-like platforms for probably decades at this point, and also Windows, appear to include the . and ... And wasi-libc has assumed that it's WASI's job to provide these for as long as it has existed.

I've now submitted a meeting agenda item and will leave it to the Subgroup as a whole to decide.

sunfishcode added a commit to sunfishcode/WASI that referenced this issue Feb 15, 2023
As discussed in the the 02-09 meeting, explicitly document that
Preview1's `fd_readdir` includes the `.` and `..`.

This resolves WebAssembly/wasi-testsuite#52.
@codefromthecrypt
Copy link
Author

closing this issue, here's a summary

linclark pushed a commit to WebAssembly/WASI that referenced this issue Feb 17, 2023
…`. (#516)

As discussed in the the 02-09 meeting, explicitly document that
Preview1's `fd_readdir` includes the `.` and `..`.

This resolves WebAssembly/wasi-testsuite#52.
codefromthecrypt pushed a commit to tetratelabs/wazero that referenced this issue Feb 22, 2023
This manually adds dot and dot-dot directory entries discarded by
compilers like Go. Doing so complies with the latest interpretation of
wasi preview1 without forcing us to do the same in preview2 or in
GOOS=js. There's a cost penalty of one stat per directory list, which
will be only measurable when using real file I/O.

This should result in us being able to remove the exclusion here
https://github.com/WebAssembly/wasi-testsuite/pull/55/files#diff-8a3ffd323d75a12f8deb01b053f062876d83dda0b89c1fa24b293cee4195bcfd

See WebAssembly/wasi-testsuite#52

Signed-off-by: Adrian Cole <[email protected]>
codefromthecrypt added a commit to tetratelabs/wazero that referenced this issue Feb 22, 2023
This manually adds dot and dot-dot directory entries discarded by
compilers like Go. Doing so complies with the latest interpretation of
wasi preview1 without forcing us to do the same in preview2 or in
GOOS=js. There's a cost penalty of one stat per directory list, which
will be only measurable when using real file I/O.

This should result in us being able to remove the exclusion here
https://github.com/WebAssembly/wasi-testsuite/pull/55/files#diff-8a3ffd323d75a12f8deb01b053f062876d83dda0b89c1fa24b293cee4195bcfd

See WebAssembly/wasi-testsuite#52

Signed-off-by: Adrian Cole <[email protected]>
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 a pull request may close this issue.

2 participants