Stream: git-wasmtime

Topic: wasmtime / Issue #1375 Pass `Fd` by reference

Wasmtime GitHub notifications bot (Mar 23 2020 at 18:03):

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.

Wasmtime GitHub notifications bot (Mar 23 2020 at 18:46):

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 like Fd<'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 in wiggle, 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?

Wasmtime GitHub notifications bot (Mar 23 2020 at 19:23):

alexcrichton commented on Issue #1375:

I think I may be missing the motivation here, it sounds like we have u32 values from wasm, Fd, and Entry. Today Entry is already passed by reference, and this PR looks to be switching Fd, which is currently a thin newtype around u32, to also be passed by reference. Why is Fd 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 return handle we could change those to return Result<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?

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:58):

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 vs File. 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 as from_raw and as_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.

Wasmtime GitHub notifications bot (Mar 24 2020 at 06:03):

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 a Fd, that achieves the same goal as passing Fd by reference, but in a simpler way.

Wasmtime GitHub notifications bot (Mar 24 2020 at 17:18):

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 in errno).

Wasmtime GitHub notifications bot (Mar 24 2020 at 17:20):

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 in errno). That use case is similar to the GuestErrorType 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.

Wasmtime GitHub notifications bot (Mar 24 2020 at 17:27):

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.

Wasmtime GitHub notifications bot (Mar 24 2020 at 20:57):

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.

Wasmtime GitHub notifications bot (Mar 25 2020 at 13:26):

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.

Learn more.

Wasmtime GitHub notifications bot (May 13 2020 at 23:26):

sunfishcode commented on Issue #1375:

Closing this PR to pursue other approaches as discussed above.

Last updated: Nov 22 2024 at 16:03 UTC