Introduce strongly-typed system primitives #1561
Conversation
Subscribe to Label Actioncc @kubkon
This issue or pull request has been labeled: "wasi"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
Hmm, so I reckon this PR is at a stage that's ready for review so I'll mark it as such. Besides the major changes to the way we treat OS handles, there's a couple more things I'm somewhat unclear on:
Anyhow, all thoughts, suggestions are more than welcome! |
|
@alexcrichton even though you might not know all the internals very well, I'd appreciate your opinion on this even if high level! |
kubkon
changed the title
|
OK, so I've gone ahead and done a little bit of refactoring that IMHO should aid readability. Namely, I've delegated defining |
|
Sorry for the delay, but I think I won't have a ton of time to review this. It sounds reasonable enough but I don't have time to dig into the details right now. |
There was a problem hiding this comment.
We just had a chat on zulip about this PR. I wasn't clear on why Stdio and OsOther were distinct types and what exactly was the reasons for their sticking around, versus some unified TTY abstraction.
There's a lot of complexity in src/sys that I haven't fully internalized. Can you add some doc comments about what each of these types are for, and in the cases like OsOther where it exists until we can refactor further, this can just be an explanation of what we need to consider when refactoring further.
|
@sunfishcode do you think we're go green for merging or would you like to take another look at the PR? |
This commit does a lot of reshuffling and even some more. It introduces strongly-typed system primitives which are: `OsFile`, `OsDir`, `Stdio`, and `OsOther`. Those primitives are separate structs now, each implementing a subset of `Handle` methods, rather than all being an enumeration of some supertype such as `OsHandle`. To summarise the structs: * `OsFile` represents a regular file, and implements fd-ops of `Handle` trait * `OsDir` represents a directory, and primarily implements path-ops, plus `readdir` and some common fd-ops such as `fdstat`, etc. * `Stdio` represents a stdio handle, and implements a subset of fd-ops such as `fdstat` _and_ `read_` and `write_vectored` calls * `OsOther` currently represents anything else and implements a set similar to that implemented by `Stdio` This commit is effectively an experiment and an excercise into better understanding what's going on for each OS resource/type under-the-hood. It's meant to give us some intuition in order to move on with the idea of having strongly-typed handles in WASI both in the syscall impl as well as at the libc level. Some more minor changes include making `OsHandle` represent an OS-specific wrapper for a raw OS handle (Unix fd or Windows handle). Also, since `OsDir` is tricky across OSes, we also have a supertype of `OsHandle` called `OsDirHandle` which may store a `DIR*` stream pointer (mainly BSD). Last but not least, the `Filetype` and `Rights` are now computed when the resource is created, rather than every time we call `Handle::get_file_type` and `Handle::get_rights`. Finally, in order to facilitate the latter, I've converted `EntryRights` into `HandleRights` and pushed them into each `Handle` implementor.
This cleans up a lot of repeating boilerplate code todo with dynamic dispatch.
Delegates defining `OsDir` struct to OS-specific modules (BSD, Linux, Emscripten, Windows). This way, `OsDir` can safely re-use `OsHandle` for raw OS handle storage, and can store some aux data such as an initialized stream ptr in case of BSD. As a result, we can safely get rid of `OsDirHandle` which IMHO was causing unnecessary noise and overcomplicating the design. On the other hand, delegating definition of `OsDir` to OS-specific modules isn't super clean in and of itself either. Perhaps there's a better way of handling this?
It seems prudent to check if the passed in `File` instance is of type matching that of the requested WASI filetype. In other words, we'd like to avoid situations where `OsFile` is created from a pipe.
Return `EBADF` in `AsFile` in case a `Handle` cannot be made into a `std::fs::File`.
Also, since `RawOsHandle` on *nix doesn't need interior mutability wrt the inner raw file descriptor, we can safely swap the `RawFd` for `File` instance.
crates/wasi-common/src/ctx.rs
Outdated
| self | ||
| } | ||
|
|
||
| /// Add a preopened directory. | ||
| pub fn preopened_dir<P: AsRef<Path>>(&mut self, dir: File, guest_path: P) -> &mut Self { | ||
| self.preopens.as_mut().unwrap().push(( | ||
| guest_path.as_ref().to_owned(), | ||
| Box::new(OsHandle::from(dir)), | ||
| Box::new(OsDir::try_from(dir).expect("valid OsDir handle")), |
There was a problem hiding this comment.
This expect could fail if the user passes in the wrong kind of handle? That feels like something where should return a Result and forward the error on, rather than panicking.
There was a problem hiding this comment.
Ah, right! I was meant to do this and somehow forgot. Thanks for pointing this one out!
There was a problem hiding this comment.
@sunfishcode Fixed in 36f07cf. So in order not to force public API changes, instead of throwing an error directly at the preopen callsite (either preopened_dir or preopened_virt), the creation of the actual instance is now postponed until we build the context object, much like creation of the stdio handles.
A regression in bytecodealliance#1561 caused "invalid argument" errors when `stdin`, `stdout`, and `stderr` are given a file handle. This was missed in Wasmtime CI because previously only a pipe handle was being used. It was caught in the .NET implementation CI because it uses file handles for its WASI tests. Fixes bytecodealliance#1735.
A regression in bytecodealliance#1561 caused "invalid argument" errors when `stdin`, `stdout`, and `stderr` are given a file handle. This was missed in Wasmtime CI because previously only a pipe handle was being used. It was caught in the .NET implementation CI because it uses file handles for its WASI tests. Fixes bytecodealliance#1735.
This commit does a lot of reshuffling and even some more. It introduces
strongly-typed system primitives which are:
OsFile,OsDir,Stdio,and
OsOther. Those primitives are separate structs now, each implementinga subset of
Handlemethods, rather than all being an enumeration of somesupertype such as
OsHandle. To summarise the structs:OsFilerepresents a regular file, and implements fd-opsof
HandletraitOsDirrepresents a directory, and primarily implements path-ops, plusreaddirand some common fd-ops such asfdstat, etc.Stdiorepresents a stdio handle, and implements a subset of fd-opssuch as
fdstatandread_andwrite_vectoredcallsOsOthercurrently represents anything else and implements a set similarto that implemented by
StdioThis commit is effectively an experiment and an excercise into better
understanding what's going on for each OS resource/type under-the-hood.
It's meant to give us some intuition in order to move on with the idea
of having strongly-typed handles in WASI both in the syscall impl as well
as at the libc level.
Some more minor changes include making
OsHandlerepresent an OS-specificwrapper for a raw OS handle (Unix fd or Windows handle). Also, since
OsDiris tricky across OSes, we also have a supertype of
OsHandlecalledOsDirHandlewhich may store aDIR*stream pointer (mainly BSD). Last but notleast, the
FiletypeandRightsare now computed when the resource is created,rather than every time we call
Handle::get_file_typeandHandle::get_rights.Finally, in order to facilitate the latter, I've converted
EntryRightsintoHandleRightsand pushed them into eachHandleimplementor.