🕳 Add virtual pipes to wasi-common #1949
Conversation
This introduces `Handle` implementations for readable and writable pipes, backed by arbitrary `Read` and `Write` types, respectively. In particular, this allows for easily providing, capturing, or redirecting WASI stdio without having to resort to OS-provided file descriptors. The implementation is based heavily on `wasi_common::virtfs::InMemoryFile`, but without inapplicable operations like `seek` or `allocate`. Note that these types are not 1:1 replacements for real pipes, because they do not support `poll_oneoff`.
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 |
| } | ||
| } | ||
|
|
||
| impl From<&[u8]> for ReadPipe<io::Cursor<Vec<u8>>> { |
There was a problem hiding this comment.
| impl From<&[u8]> for ReadPipe<io::Cursor<Vec<u8>>> { | |
| impl<'a> From<&'a [u8]> for ReadPipe<io::Cursor<&'a [u8]>> { |
I think this is a correct way to spell an impl entirely on a ref of the underlying data, and I'm pretty sure we can Cursor<&'_ [u8]> for reading here, so this ought to let is avoid forcing a copy of the underlying slice?
There was a problem hiding this comment.
Sadly &'a [u8] is not Any due to the non-static lifetime, but Any is required to implement Handle 😭
| } | ||
| } | ||
|
|
||
| impl From<&str> for ReadPipe<io::Cursor<String>> { |
There was a problem hiding this comment.
Same idea about a Cursor<&'a str> here too, but I'm not as sure that this'll Just Work. Might be worth trying?
| } | ||
|
|
||
| fn fdstat_set_flags(&self, _fdflags: types::Fdflags) -> Result<()> { | ||
| // do nothing for now |
There was a problem hiding this comment.
One thing I punted on when first doing virtfs work was figuring out what kind of errors are actually acceptable to return from what function. I broadly worked off of In the parts of WASI which do correspond to POSIX functionality, WASI follows POSIX when it doesn't conflict with WASI's other goals. And, we consult POSIX even when we aren't strictly following it. from DesignPrinciples.md but we should probably consider documenting the full set of errors that WASI functions may return - even if that's simply "all functions may return any Errno" (I think we can and should do better than that, since it would be good to be accurate in communicating kinds of errors to downstream users)
I bring this up mostly because I'm wondering if it's right to accept and do nothing here, or should this Errno::Perm? Silently doing nothing is probably the right answer, but there ought to be more implementation guidance for virtfs users.
There was a problem hiding this comment.
What does POSIX do if you try to set O_APPEND on a pipe? Do real-world implementations support O_NONBLOCK on pipes? I don't have a clear sense of the right thing to do here yet.
I agree that we eventually want to move toward documenting the possible error codes for each function. I expect that'll be easier to do once we can start splitting up "file descriptors" into separate types, and we can have separate errno enums rather than one big POSIX-style errno space shared by filesystem and networking functions of all kinds.
There was a problem hiding this comment.
Agreed that there should be more implementation guidance, but since I'm new to this corner of the codebase I'm not sure what to provide. Would you be okay with merging this for now with the expectation of doing that work later?
There was a problem hiding this comment.
What does POSIX do if you try to set O_APPEND on a pipe? Do real-world implementations support O_NONBLOCK on pipes? I don't have a clear sense of the right thing to do here yet.
I was actually really curious about this and how I'd find the answer: it turns out Linux source is a pretty clear answer - looks like O_APPEND and O_NONBLOCK would be accepted without issue. I didn't see anything in particular about fcntl's interaction with pipes in POSIX either, so I assume the flags should do the things they say they do? Where O_APPEND is functionally ignored on pipes, of course :)
Personally merging this as-is and providing better implementation guidance independently is exactly the approach I think we ought to take.
There was a problem hiding this comment.
Sorry @sunfishcode, Github wasn't showing your comment in this thread when I left my reply. In my testing, O_APPEND seems to be ignored.
There was a problem hiding this comment.
Yes, as long as you're ok making subtle behavior changes later :-).
acfoltzer
added
the
wasi:impl
|
|
||
| fn unlink_file(&self, _path: &str) -> Result<()> { | ||
| Err(Errno::Notdir) | ||
| } |
There was a problem hiding this comment.
Along the lines above, splitting up file descriptors into different types should also reduce the one-big-trait factor here, which should clean a lot of this up.
There was a problem hiding this comment.
Yeah, I was actually thinking about breaking up Handle, but since this part of the code is new to me I didn't want to stir things up too much. Are you alright with saving this for a future PR?
There was a problem hiding this comment.
Yeah, I should have clarified; I'm talking about cleanups we can do in the future here.
|
I'm looking forward to the point where we can say that features like this should be implemented through link-time virtualization mechanisms, rather than traits in Wasmtime, but we're not there yet :-), so this overall looks good for now. |
This introduces
Handleimplementations for readable and writable pipes, backed by arbitraryReadandWritetypes, respectively. In particular, this allows for easily providing, capturing, or redirecting WASI stdio without having to resort to OS-provided file descriptors.The implementation is based heavily on
wasi_common::virtfs::InMemoryFile, but without inapplicable operations likeseekorallocate.Note that these types are not 1:1 replacements for real pipes, because they do not support
poll_oneoff.