acfoltzer opened PR #1949 from acf/virtual-pipes
to main
:
This introduces
Handle
implementations for readable and writable pipes, backed by arbitraryRead
andWrite
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 likeseek
orallocate
.Note that these types are not 1:1 replacements for real pipes, because they do not support
poll_oneoff
.
iximeow submitted PR Review.
iximeow submitted PR Review.
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?
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 anyErrno
" (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.
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?
pchickey submitted PR Review.
pchickey submitted PR Review.
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?
pchickey created PR Review Comment:
use
HandleRights::from_base
to construct a HandleRights that has an empty inheriting set
pchickey created PR Review Comment:
I'd rather leave traces out of these components
pchickey created PR Review Comment:
HandleRights::from_base here
pchickey created PR Review Comment:
This would be nicer if wiggle had derived Default on Filestat - just a note for future work in wiggle.
pchickey created PR Review Comment:
would rather not trace
iximeow submitted PR Review.
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.
pchickey created PR Review Comment:
Yes, I would prefer that are removed as well. Low priority, but nice to clean up.
acfoltzer submitted PR Review.
acfoltzer created PR Review Comment:
Sadly
&'a [u8]
is notAny
due to the non-static lifetime, butAny
is required to implementHandle
:sob:
sunfishcode submitted PR Review.
sunfishcode submitted PR Review.
sunfishcode submitted PR Review.
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.
sunfishcode submitted PR Review.
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.
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.
sunfishcode submitted PR Review.
pchickey submitted PR Review.
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 thetracing_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.
pchickey edited PR Review Comment.
pchickey edited PR Review Comment.
pchickey edited PR Review Comment.
acfoltzer submitted PR Review.
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?
acfoltzer submitted PR Review.
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?
acfoltzer updated PR #1949 from acf/virtual-pipes
to main
:
This introduces
Handle
implementations for readable and writable pipes, backed by arbitraryRead
andWrite
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 likeseek
orallocate
.Note that these types are not 1:1 replacements for real pipes, because they do not support
poll_oneoff
.
acfoltzer requested iximeow for a review on PR #1949.
acfoltzer requested pchickey and iximeow for a review on PR #1949.
iximeow submitted PR Review.
iximeow created PR Review Comment:
Ahh drat.
pchickey submitted PR Review.
iximeow submitted PR Review.
acfoltzer submitted PR Review.
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.
acfoltzer merged PR #1949.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Yeah, I should have clarified; I'm talking about cleanups we can do in the future here.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Yes, as long as you're ok making subtle behavior changes later :-).
Last updated: Dec 23 2024 at 13:07 UTC