rvolosatovs opened PR #6440 from rvolosatovs:feat/preview1-preview2
to bytecodealliance:main
:
Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175
There's a platform-specific bug somewhere, which I suspect originates somewhere in
path_open
flag translation logic
rvolosatovs updated PR #6440.
rvolosatovs updated PR #6440.
rvolosatovs edited PR #6440:
Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175
rvolosatovs edited PR #6440:
Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175
The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.
A follow-up PRs fill be filed for:
fd_readdir
- fixing
windows
support for a few tests (still not sure what's causing it)- making all preview1 implementations consistent
rvolosatovs has marked PR #6440 as ready for review.
rvolosatovs requested itsrainy for a review on PR #6440.
rvolosatovs requested wasmtime-core-reviewers for a review on PR #6440.
rvolosatovs edited PR #6440:
Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175
The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.
A follow-up PRs fill be filed for:
fd_readdir
- fixing
windows
support for a few tests (still not sure what's causing the failures)- making all preview1 implementations consistent
itsrainy requested pchickey for a review on PR #6440.
pchickey submitted PR review:
This is great work! Just a few small issues. Thank you for finding all of these bad behaviors in the legacy implementation!
pchickey submitted PR review:
This is great work! Just a few small issues. Thank you for finding all of these bad behaviors in the legacy implementation!
pchickey created PR review comment:
this is still a
todo!()
- we can leave it to a follow up PR, butunreachable!()
communicates to me that it is a bug in this code if it is reachable, while this is reachable from any guest program.
pchickey created PR review comment:
I wasn't expecting this sort of machinery to be required - we can't get away with implementing the DescriptorsRef methods on WasiPreview1Ctx? I can't intuit where the problem is in a more straightforward implementation, so if this is required, lets leave a comment on the struct definition explaining why.
pchickey created PR review comment:
lets definitely not preserve that bug :). That one has been around for a while! Lets get rid of this check and follow up with a PR to fix the legacy impl, and a test. I expect that fix will only be around for its final release, but its still worth doing.
rvolosatovs created PR review comment:
I took this directly from https://github.com/bytecodealliance/wasmtime/blob/5b93cdb84def5fa7cb3ff823bc80216f19c296c5/crates/wasi-preview1-component-adapter/src/lib.rs#L560-L569 and it was surprising to me as well.
It seems to me that the adapter has to implement rights handling, which the existing one does not do. I'll mark this astodo!
and file an issue for rights handling follow-up
rvolosatovs created PR review comment:
I'll add a comment, but the biggest reasons why this is required are:
- By far the biggest issue is
bindgen
generating host methods with&mut self
receivers, preventing e.g. simultaneous lazy initialization of the descriptor table and calling a preview2 method using any data borrowed from that table, even looking up something liketable().is_file
is problematicWasiPreview1Adapter
(assuming that's what you meant by WasiPreview1Ctx) does not have access to preview2 functionality and/or the underlying table, so it cannot initialize itself using something like https://docs.rs/once_cell/latest/once_cell/sync/struct.Lazy.html . Lazily initializing the descriptor map requires access to both theWasiPreview1Adapter
andwasi::preopens::Host
, so the only place where this functionality could exist is in an extension trait (WasiPreview1ViewExt
). Note that from within this trait, we can never return anything borrowed directly, because we'd first need to calladapter_mut()
to get access to the state (which we may need to lazily initialize). And even if we had a way to return a borrowed descriptor table directly, it would not be possible to call any other method onself
due to 1., since that would cause a double mutable borrow, which is not allowed.In short, an alternative solution would be storing
Option<Arc<Mutex<Descriptors>>>
in the adapter and returningArc<Mutex<Descriptors>>
in theWasiPreview1AdapterExt::descriptors(&mut self)
method, which IMO is way more awkward to work with and simply redundant complexity.
Overall, theDescriptorsRef
is just a workaround forbindgen
design drawbacks. I believe this could be simplified dramatically if host methods would just take&self
rvolosatovs updated PR #6440.
rvolosatovs requested pchickey for a review on PR #6440.
rvolosatovs edited PR #6440:
Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175
The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.
A follow-up PRs fill be filed for:
fd_readdir
- fixing
windows
support for a few tests (still not sure what's causing the failures)- making all preview1 implementations consistent
- add rights handling
rvolosatovs edited PR #6440:
Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175
The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.
A follow-up issues/PRs fill be filed for:
fd_readdir
- fixing
windows
support for a few tests (still not sure what's causing the failures)- making all preview1 implementations consistent
- adding rights handling
rvolosatovs edited PR #6440:
Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175
Refs https://github.com/bytecodealliance/wasmtime/issues/6370
The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.
A follow-up issues/PRs fill be filed for:
fd_readdir
- fixing
windows
support for a few tests (still not sure what's causing the failures)- making all preview1 implementations consistent
- adding rights handling
pchickey created PR review comment:
Oh, that is indeed a bug in the component adapter as well.
unreachabable!()
is just how we spelltodo!()
andpanic!()
and etc over there since we cant use std's macros.In the legacy implementation, set_rights is a no-op, it only validates the fd. That is the correct behavior for both of these spots. Would you mind making this fix to the component adapter as well?
pchickey created PR review comment:
That makes sense. Thank you.
We should look into adding a bindgen option to enable &self receivers in the future, and then we may be able to unwind a bit of this complexity. For now, this is a fine implementation once it has a comment.
rvolosatovs created PR review comment:
I've added a few comments and renamed the struct to
Transaction
so it's a bit more obvious what it actually is
rvolosatovs created PR review comment:
Let's take care of it as part of https://github.com/bytecodealliance/wasmtime/issues/6446 ?
rvolosatovs created PR review comment:
pchickey submitted PR review.
pchickey merged PR #6440.
Last updated: Dec 23 2024 at 12:05 UTC