Stream: git-wasmtime

Topic: wasmtime / PR #1561 Introduce strongly-typed system primi...

Wasmtime GitHub notifications bot (Apr 22 2020 at 08:45):

kubkon edited PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (Apr 22 2020 at 09:52):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (Apr 26 2020 at 07:50):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (Apr 27 2020 at 21:17):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Apr 27 2020 at 21:17):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Apr 27 2020 at 21:17):

pchickey created PR Review Comment:

This is maybe a comment for the PR that introduced Handle, but if instead of having an as_any(&self) -> &dyn Any, the Handle trait instead depended on Any, then you could drop that trivial method, and also this 'static' constraint, because Any implies 'static`.

Wasmtime GitHub notifications bot (Apr 27 2020 at 21:17):

pchickey created PR Review Comment:

I think its pretty dangerous to panic here - instead, can we return a Result, so we can bubble this error outwards without crashing?

Wasmtime GitHub notifications bot (Apr 27 2020 at 21:17):

pchickey created PR Review Comment:

Can you leave a comment specifically on why we have to handle the tty and non-tty cases separately? Whether io::stdout() gives us a tty or not depends on how the embedding process is invoked, right? Is there any way to avoid exposing that to the guest?

Wasmtime GitHub notifications bot (Apr 28 2020 at 13:48):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 13:48):

kubkon created PR Review Comment:

Agreed! I'll redo it returning a Result instead as suggested :-)

Wasmtime GitHub notifications bot (Apr 28 2020 at 13:49):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 13:49):

kubkon created PR Review Comment:

Oh nice, I didn't think of that! Lemme give it a spin and report back. Oh, and thanks for the suggestion!

Wasmtime GitHub notifications bot (Apr 28 2020 at 13:53):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 13:53):

kubkon created PR Review Comment:

Yeah, that's correct. I actually need to revisit at some point, but the reason we ended up with custom Stdio type which is a proxy for Rust's stdio handles, is because Windows was doing some crazy stdio redirects on the CI which were nontrivial to handle. I'll add some comments about for posterity. Thanks for spotting that one!

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:10):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:11):

iximeow created PR Review Comment:

drops in This is just a &File -> ManuallyDrop<File> conversion, right? You ought to be able to remove this line (and the AsFile impl above, it seems?

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:11):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:14):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:15):

kubkon created PR Review Comment:

Ah, well spotted, thanks!

As far as the AsFile impl goes, this blanket impl is needed so that any type that wraps OsHandle (which itself implements AsRawFd) to be able to convert automatically to std::fs::File.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:15):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:15):

kubkon created PR Review Comment:

That's why, if you look closely, the impl goes for any T: AsRawFd.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:16):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:18):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:18):

kubkon created PR Review Comment:

Done in 26c894a.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:18):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:18):

kubkon created PR Review Comment:

The line removed in 5b7e54e BTW.

Wasmtime GitHub notifications bot (Apr 29 2020 at 09:21):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 09:21):

kubkon created PR Review Comment:

OK, so after some careful analysis, I believe we'll still require as_any(&self) -> &dyn Any method defined in each implementor of the Handle trait. Here's the breakout of the reasons I discovered:

1. vtables in Rust currently do not permit for trait upcasting (see here and here). This then implies that, even if Handle: Any, we can't go from dyn Handle to dyn Any.
2. Even if we added the constraint Handle: Any and made as_any(&self) -> &dyn Any a provided method, we'd still need to require that, at the very least, for as_any(...) Self: Sized. But then, as_any(...) is not available for dyn Handle trait object.

All in all, requiring Any on Handle seems to only get rid of the 'static requirement in the referenced impl AsFile which I don't think is worth it? Lemme know what you think! Maybe there's something obvious that I missed and this will actually work, or perhaps I misunderstood what you meant! :-)

Wasmtime GitHub notifications bot (Apr 29 2020 at 09:37):

kubkon created PR Review Comment:

Just clarify further, as far as the guest is concerned, Stdio should be transparent to them. As I mentioned above, the only reason we have a separate Stdio type in wasi-common is to correctly facilitate redirects on Windows. To elaborate further, in POSIX, we can get a stdio handle by opening a specific fd {0,1,2}. On Windows however, we need to issue a syscall that's separate from standard Windows "open" to get a console handle, and this is GetStdHandle. This is exactly what Rust does and what is wrapped inside their Stdio object in the libstd. We wrap it here as well because of this nuance on Windows:

The standard handles of a process may be redirected by a call to SetStdHandle, in which case GetStdHandle returns the redirected handle.

The MSDN also says this however:

If the standard handles have been redirected, you can specify the CONIN$ value in a call to the CreateFile function to get a handle to a console's input buffer. Similarly, you can specify the CONOUT$ value to get a handle to a console's active screen buffer.

But I'm not that well versed in Windows programming to know how to do this properly, which is not to say we shouldn't (re-)investigate at some point.

Anyway, do you think this explanation would do here? Oh, and as far as the check for TTY goes, I'm actually not sure what to do here. I'm not sure whether the handle returned by GetStdHandle will be treated like a TTY. I guess we should investigate further, but I'd vouch for leaving a TODO comment in and do it in a subsequent PR?

Wasmtime GitHub notifications bot (Apr 29 2020 at 09:37):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 09:57):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (Apr 29 2020 at 09:57):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 09:57):

kubkon created PR Review Comment:

OK, after careful analysis, I think we can remove the check and _always_ santise the output to stdout since is_tty will always resolve to true for Stdio handle type. However, I think we need to keep the check in case of OsOther handle type which can encapsulate user-specified handle to redirected stdio. I'm thinking here of piping and mainly on Unix, but perhaps it might also be possible on Windows.

I've done that now in f76e7c4.

Wasmtime GitHub notifications bot (Apr 29 2020 at 21:13):

kubkon requested alexcrichton, pchickey, peterhuene and sunfishcode for a review on PR #1561.

Wasmtime GitHub notifications bot (Apr 29 2020 at 23:01):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 23:01):

pchickey created PR Review Comment:

I had no idea that trait upcasting was not allowed. That is a bummer and a sufficient reason for this design.

Wasmtime GitHub notifications bot (Apr 29 2020 at 23:07):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 23:07):

pchickey created PR Review Comment:

Thanks, I understand this a lot better now. Thanks for the explanation and adding it to the docs!

Wasmtime GitHub notifications bot (Apr 29 2020 at 23:08):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Apr 30 2020 at 08:41):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Apr 30 2020 at 08:41):

kubkon created PR Review Comment:

I didn't either, so thanks for prodding me to have a look! :-)

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode created PR Review Comment:

And stderr here?

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode created PR Review Comment:

How difficult would it be to split this enum out into three separate structs that all implement the Handle trait independently? Besides the additional dynamic dispatch inside the read and write methods, I think that'd really illustrate how this Handle trait works -- every different kind of stream has its own impl. There'd be some duplication in the other methods, but it seems like that could be factored out with helper methods.

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode created PR Review Comment:

Should this be stdout?

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode created PR Review Comment:

This can be written as Self::In { rights } | Self::Out { rights } | Self::Err { rights } => rights.get(),, and similar in set_rights.

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode created PR Review Comment:

Stdio won't always be a character device, such as when it's redirected from a file ("grep foo < file.txt") or a pipe ("cat file.txt | grep foo"). We still need to do fstat on it to determine its type.

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode created PR Review Comment:

Should OsHandle have a name with Raw in it, since it wraps a RawFd/RawHandle? Maybe RawOSHandle?

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode created PR Review Comment:

Can you add a comment here briefly explaining what OsOther is?

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode created PR Review Comment:

Why does this use OsOther here? Looking at the code, that seems to be for things that aren't files.

Wasmtime GitHub notifications bot (May 02 2020 at 00:24):

sunfishcode created PR Review Comment:

If I understand correctly, the main use for update_from is for the fdstat_set_flags, which on Windows opens a new handle. If that's the only thing that needs it, could we make update_from on Unix just panic?

And, if we don't need to implement update_from, can OsHandle on Unix hold a plain RawFd instead of a Cell<RawFd>?

Wasmtime GitHub notifications bot (May 02 2020 at 07:03):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 07:03):

kubkon created PR Review Comment:

Oh shoot, you're absolutely correct, thanks for spotting it!

Wasmtime GitHub notifications bot (May 02 2020 at 07:04):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 07:04):

kubkon created PR Review Comment:

Likewise here! Makes me start questioning our test suite. Then again, I'm not sure if there will be an obvious way of verifying stdio handles in the CI.

Wasmtime GitHub notifications bot (May 02 2020 at 07:05):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (May 02 2020 at 07:09):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 07:09):

kubkon created PR Review Comment:

Right, however, here, I think it's safe to assume that our internal Stdio type will always be a character device since it is meant to be a wrapper for Rust's stdio primitives. I envisioned that redirections should be handled by a separate entity in the codebase, namely, OsOther which is something like a catch-all for everything not covered with the specific handle types. Does that make sense? In fact, that's why in ctx.rs we map std::fs::File to OsOther as currently we only ever permit specifying custom fds/handles for stdio when builing the context.

Wasmtime GitHub notifications bot (May 02 2020 at 07:10):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (May 02 2020 at 07:10):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 07:10):

kubkon created PR Review Comment:

Ah excellent, thanks!

Wasmtime GitHub notifications bot (May 02 2020 at 07:12):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 07:12):

kubkon created PR Review Comment:

Oh right, sure thing!

Wasmtime GitHub notifications bot (May 02 2020 at 07:15):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (May 02 2020 at 07:17):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 07:17):

kubkon created PR Review Comment:

Hmm, I think this should be fine. It will require some additional tweaks to WasiCtxBuilder but nothing major I think.

Wasmtime GitHub notifications bot (May 02 2020 at 07:18):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 07:18):

kubkon created PR Review Comment:

Makes sense!

Wasmtime GitHub notifications bot (May 02 2020 at 07:18):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 07:18):

kubkon created PR Review Comment:

Oh, see my reply to your comment about Stdio :-)

Wasmtime GitHub notifications bot (May 02 2020 at 21:33):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (May 02 2020 at 21:34):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 21:34):

kubkon created PR Review Comment:

OK, I've separated it out into 3 standalone structs: Stdin, Stdout and Stderr.

Wasmtime GitHub notifications bot (May 02 2020 at 21:34):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (May 02 2020 at 21:36):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 02 2020 at 21:36):

kubkon created PR Review Comment:

In order to clear it up a little bit, I've renamed PendingEntry::File to PendingEntry::OsHandle. This naming was somewhat confusing since the File in this case represented for instance a pipe which we then used in place of a stdio handle. Oh and this is precisely why we use OsOther here instead of an OsFile, since the former is meant to represent pipes as well, whereas the latter only regular files.

Wasmtime GitHub notifications bot (May 04 2020 at 06:51):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (May 04 2020 at 06:56):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 04 2020 at 06:56):

kubkon created PR Review Comment:

OK, it's done now. Since, as you rightly pointed out in one of the comments above, we don't really need interior mutability on *nix wrt the inner RawFd instance, I've also replaced RawFd with File instance which takes care of a couple of ownership related things for us. I believe this is what @iximeow was suggesting all along as well. Now that @sunfishcode pointed out that issuing RawOsHandle::update_from call on *nix is a bug, using File was now possible.

Wasmtime GitHub notifications bot (May 04 2020 at 07:15):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (May 04 2020 at 07:16):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 04 2020 at 07:16):

kubkon created PR Review Comment:

Done!

Wasmtime GitHub notifications bot (May 04 2020 at 20:30):

kubkon requested alexcrichton, peterhuene and sunfishcode for a review on PR #1561.

Wasmtime GitHub notifications bot (May 04 2020 at 21:00):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 04 2020 at 21:00):

sunfishcode created PR Review Comment:

The logic for flushing Rust's stdout buffer is in the Stdout implementation here, and we need that for files too. I expect we're always going to want to use Stdout for stdout, even when it's attached to a file, in order to ensure that we always stay in sync with Rust's stdout.

Wasmtime GitHub notifications bot (May 04 2020 at 21:07):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 04 2020 at 21:07):

kubkon created PR Review Comment:

Hmm, I'm a little bit confused here. We cannot create a std::io::Stdout instance from a raw file descriptor, so I'm not sure we can ever enforce that we always use sys::stdio::Stdout for stdout. I think we could probably think of extracting PipedStdout or something from OsOther that would have RawFd/RawHandle attached to it.

Wasmtime GitHub notifications bot (May 04 2020 at 21:13):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (May 04 2020 at 21:15):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 04 2020 at 21:15):

kubkon created PR Review Comment:

Oh, wait, I think I get it now. If the OS redirects the stdio handles, Rust's Stdout will use the redirected RawFd underneath, and this one doesn't necessarily have to be a character device. Is that what you meant?

Wasmtime GitHub notifications bot (May 04 2020 at 21:16):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (May 04 2020 at 21:42):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (May 04 2020 at 21:45):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 04 2020 at 21:45):

kubkon created PR Review Comment:

Fixed in 0945267. @sunfishcode if you could have a look and lemme know if that's what you had in mind!

Wasmtime GitHub notifications bot (May 04 2020 at 23:18):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 04 2020 at 23:18):

sunfishcode created PR Review Comment:

Yes, that's right.

Wasmtime GitHub notifications bot (May 06 2020 at 17:46):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (May 07 2020 at 14:13):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 14:13):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 14:13):

sunfishcode created PR Review 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.

Wasmtime GitHub notifications bot (May 07 2020 at 14:13):

sunfishcode created PR Review Comment:

Can you comment on why this as_file method isn't part of the Handle trait? Looking at the implementation below, it's doing a lot of as_any().downcast_ref<Stuff>(), which seems like it would be avoided if you could just call as_file on a Handle directly.

Wasmtime GitHub notifications bot (May 07 2020 at 14:16):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 14:16):

kubkon created PR Review Comment:

Ah, right! I was meant to do this and somehow forgot. Thanks for pointing this one out!

Wasmtime GitHub notifications bot (May 07 2020 at 14:18):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 14:18):

kubkon created PR Review Comment:

Hmm, that's an interesting suggestion. So previously, AsFile wasn't part of Handle trait since it wasn't fallible, and hence, it wouldn't make much sense for VirtualFile, etc. But now, given that we return Result I guess we could in principle return Errno::Badf when called by a virtual handle? Or should be outright panic in such a case?

Wasmtime GitHub notifications bot (May 07 2020 at 15:36):

sunfishcode created PR Review Comment:

Oh, right. I wasn't thinking about cases like VirtualHandle. I think it's fine as is then.

Wasmtime GitHub notifications bot (May 07 2020 at 15:36):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 21:41):

kubkon updated PR #1561 from handle-rights to master:

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.

Wasmtime GitHub notifications bot (May 07 2020 at 21:46):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 21:46):

kubkon created PR Review Comment:

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.

Wasmtime GitHub notifications bot (May 07 2020 at 21:48):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (May 07 2020 at 23:00):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 23:00):

sunfishcode merged PR #1561.

Last updated: Dec 23 2024 at 12:05 UTC