kubkon commented on Issue #1375:
OK, so the only thing missing is storing handles as part of a struct and passing this struct around. Personally, I think I'm leaning towards the first suggestion of always passing handles by ref.
kubkon commented on Issue #1375:
On second thought, this is going to be more tricky than I originally anticipated.
wiggle
currently is built with types that take an explicit lifetime in mind, so ideally we'd have something likeFd<'a>
. In other words, if there was a way for us to hide the ref inside the type, that would be ideal. To make structs hold a ref inwiggle
, I guess we'd need to make handles a special-case indeed, and this might perhaps lead to some overly complicated code? I'm just putting it out there. @pchickey @alexcrichton what do y'all reckon?
alexcrichton commented on Issue #1375:
I think I may be missing the motivation here, it sounds like we have u32 values from wasm,
Fd
, andEntry
. TodayEntry
is already passed by reference, and this PR looks to be switchingFd
, which is currently a thin newtype around u32, to also be passed by reference. Why isFd
being changed to be passed by reference?I feel like a better eventuality may be something where if you take a
handle
type in a function it's automatically looked up, e.g. for(type $fd (handle))
that translates to this in the trait:trait WasiSnapshot { fn lookup_fd(&self, idx: u32) -> Result<&Entry>; }(or something like that)
And that way APIs don't ever actually deal with
Fd
or u32, that all happens in the wiggle layer. Even in APIs that returnhandle
we could change those to returnResult<Entry>
in Rust and that's automatically converted to a file descriptor under the hood.I guess I'm sort of confused why we want more types passed by reference when we already have a type passed by reference,
Entry
?
kubkon commented on Issue #1375:
Hmm, those are some really good points, thanks @alexcrichton! I too don’t want to make this overly complicated and convoluted, and indeed this somewhat starts to look like it. However, one motivation for handles being non-Copy and non-Clone, and passed by ref, would be to signal that they are something more than just an “index” into a capabilities table. I guess you could think of it as
RawFd
vsFile
. Now I’m not saying this is the way to go, but personally I’d welcome it if we managed to come with a wrapper type for handles that could only be passed by reference unless reconstructed using unsafe primitives such asfrom_raw
andas_raw
. So that would be the main motivation the way I understand it. Perhaps @sunfishcode could offer some details or his point of view about all this.
sunfishcode commented on Issue #1375:
@alexcrichton I like that idea. If it's feasible to have wiggle generate the
get_entry
calls, so that the hand-maintained code just gets an&Entry
parameter instead of aFd
, that achieves the same goal as passingFd
by reference, but in a simpler way.
pchickey commented on Issue #1375:
I like the idea of looking up witx handles to get a ref to some other Rust type, and passing that to the trait method. I've been fielding requests for wiggle to be able to convert to and from types defined in the user's code in other contexts. So I wonder if we can come up with a generalized way for wiggle users to specify that a witx type needs to be transformed to/from some externally-defined Rust type before consumption / after production.
The other big use case that has come up is in Fastly's HTTP code, we have a rich (as in
thiserror
) structured error enum that we want to implement the trait method with. If we return that Err from the trait method, we would like to call another Ctx method to log it appropriately and convert it to a "flat" witx error enum (as inerrno
).
pchickey edited a comment on Issue #1375:
I like the idea of looking up witx handles to get a ref to some other Rust type, and passing that to the trait method. I've been fielding requests for wiggle to be able to convert to and from types defined in the user's code in other contexts. So I wonder if we can come up with a generalized way for wiggle users to specify that a witx type needs to be transformed to/from some externally-defined Rust type before consumption / after production.
The other big use case that has come up is in Fastly's HTTP code, we have a rich (as in
thiserror
) structured error enum that we want to implement the trait method with. If we return that Err from the trait method, we would like to call another Ctx method to log it appropriately and convert it to a "flat" witx error enum (as inerrno
). That use case is similar to theGuestErrorType
trait we already require. I would like to figure out some way for that mechanism to work for handles, and external enum definitions like Frank has, as well.This is a pretty complicated design space to figure out so if anyone wants to have a zoom call to chat about it more, lets.
kubkon commented on Issue #1375:
This might be a bit more premature to ask, but I guess in a trait suggested by @alexcrichton we'd expect something like this:
trait WasiSnapshot { type Entry; fn lookup_fd(&self, idx: u32) -> Result<&Self::Entry>; }in the sense that we wouldn't tie some specific type to the trait and the lookup method, or would we actually? I'm just trying to get a better sense of how we'd like to achieve this.
alexcrichton commented on Issue #1375:
That's my rough thinking yeah, although it may not literally work out as such. The idea though would be that each implementation would define what an
Entry
is, and the methods that operate on all the entries would get that object in the trait. That way lucet/wasmtime could have entirely different representations if they really wanted to (but they both use wasi-common so it likely wouldn't matter)This may be a bit pie-in-the-sky though, so don't feel like anything needs to be blocked on it.
github-actions[bot] commented on Issue #1375:
Subscribe to Label Action
This issue or pull request has been labeled: "wasi"
<details> <summary>Users Subscribed to "wasi"</summary>
- @kubkon
</details>
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
sunfishcode commented on Issue #1375:
Closing this PR to pursue other approaches as discussed above.
Last updated: Dec 23 2024 at 12:05 UTC