Stream: git-wasmtime

Topic: wasmtime / PR #1949 🕳 Add virtual pipes to wasi-common

Wasmtime GitHub notifications bot (Jun 30 2020 at 17:46):

acfoltzer opened PR #1949 from acf/virtual-pipes to main:

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.

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:50):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:50):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:50):

iximeow created PR Review Comment:

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?

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:50):

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

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:50):

iximeow created PR Review 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?

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:55):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:55):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:55):

pchickey created PR Review Comment:

Not an issue we can solve here, but I guess we could add a Filetype for pipes to the standard? @sunfishcode is an existing filetype more appropriate for a pipe?

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:55):

pchickey created PR Review Comment:

use HandleRights::from_base to construct a HandleRights that has an empty inheriting set

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:55):

pchickey created PR Review Comment:

I'd rather leave traces out of these components

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:55):

pchickey created PR Review Comment:

HandleRights::from_base here

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:55):

pchickey created PR Review Comment:

This would be nicer if wiggle had derived Default on Filestat - just a note for future work in wiggle.

Wasmtime GitHub notifications bot (Jun 30 2020 at 19:55):

pchickey created PR Review Comment:

would rather not trace

Wasmtime GitHub notifications bot (Jun 30 2020 at 20:00):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 20:00):

iximeow created PR Review Comment:

just noting: this is probably for consistency with the other virtfs write_vectored methods: https://github.com/bytecodealliance/wasmtime/blob/main/crates/wasi-common/src/virtfs.rs#L290 - I was pretty trace-y in all that and it might make sense to remove some of those too.

Wasmtime GitHub notifications bot (Jun 30 2020 at 20:17):

pchickey created PR Review Comment:

Yes, I would prefer that are removed as well. Low priority, but nice to clean up.

Wasmtime GitHub notifications bot (Jun 30 2020 at 20:35):

acfoltzer submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 20:35):

acfoltzer created PR Review Comment:

Sadly &'a [u8] is not Any due to the non-static lifetime, but Any is required to implement Handle :sob:

Wasmtime GitHub notifications bot (Jun 30 2020 at 23:24):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 23:26):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 23:31):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 23:31):

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

Wasmtime GitHub notifications bot (Jun 30 2020 at 23:32):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jun 30 2020 at 23:32):

sunfishcode created PR Review Comment:

tracing should be (optionally) inserted by wiggle, right? In which case, we don't need the implementation code to manually do it.

Wasmtime GitHub notifications bot (Jun 30 2020 at 23:32):

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

Wasmtime GitHub notifications bot (Jun 30 2020 at 23:32):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 00:30):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 00:30):

pchickey created PR Review Comment:

tracing is always inserted by wiggle at the layer below this, and will trace just the guest pointer. however, without a tracing_subscriber enabled in some manner, or the tracing_log feature enabled, the traces will act as no-ops. This tracing does show more detail, because the guest pointers have been translated into host slices, which wiggle doesn't know about.

Wasmtime GitHub notifications bot (Jul 01 2020 at 00:31):

pchickey edited PR Review Comment.

Wasmtime GitHub notifications bot (Jul 01 2020 at 00:32):

pchickey edited PR Review Comment.

Wasmtime GitHub notifications bot (Jul 01 2020 at 00:32):

pchickey edited PR Review Comment.

Wasmtime GitHub notifications bot (Jul 01 2020 at 16:44):

acfoltzer submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 16:44):

acfoltzer created PR Review 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?

Wasmtime GitHub notifications bot (Jul 01 2020 at 16:46):

acfoltzer submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 16:46):

acfoltzer created PR Review 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?

Wasmtime GitHub notifications bot (Jul 01 2020 at 16:46):

acfoltzer updated PR #1949 from acf/virtual-pipes to main:

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.

Wasmtime GitHub notifications bot (Jul 01 2020 at 16:47):

acfoltzer requested iximeow for a review on PR #1949.

Wasmtime GitHub notifications bot (Jul 01 2020 at 16:47):

acfoltzer requested pchickey and iximeow for a review on PR #1949.

Wasmtime GitHub notifications bot (Jul 01 2020 at 17:29):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 17:29):

iximeow created PR Review Comment:

Ahh drat.

Wasmtime GitHub notifications bot (Jul 01 2020 at 17:54):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 17:55):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 18:25):

acfoltzer submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 18:25):

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

Wasmtime GitHub notifications bot (Jul 01 2020 at 18:26):

acfoltzer merged PR #1949.

Wasmtime GitHub notifications bot (Jul 01 2020 at 20:35):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 20:35):

sunfishcode created PR Review Comment:

Yeah, I should have clarified; I'm talking about cleanups we can do in the future here.

Wasmtime GitHub notifications bot (Jul 01 2020 at 20:36):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Jul 01 2020 at 20:36):

sunfishcode created PR Review Comment:

Yes, as long as you're ok making subtle behavior changes later :-).

Last updated: Nov 22 2024 at 16:03 UTC