Stream: git-wasmtime

Topic: wasmtime / Issue #1443 Make Handle a trait required for a...

Wasmtime GitHub notifications bot (Mar 31 2020 at 16:16):

github-actions[bot] commented on Issue #1443:

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 (Apr 06 2020 at 17:22):

kubkon commented on Issue #1443:

@sunfishcode Did you perhaps have a look at the proposed changes in this PR? I'd like to merge this in so that we can move on to other things such as the polyfill, etc.

Wasmtime GitHub notifications bot (Apr 08 2020 at 18:47):

kubkon commented on Issue #1443:

This looks good! Handle growing into a proper item rather than "weird thing for virtfs" is nice to see :)

The hierarchy you mentioned,

\-- OsHandle \-- OsFile -- Stdio \-- Virtual

is slightly different from what I'd had in mind, something like:

\-- File \-- Os -- Virtual \-- Stream \-- Os -- Virtual

where delineating on a File/not-File line seemed to match with "can I even take an action on this handle", like seek on a Stream, where "can this be fulfilled with the handle I have" is possibly an implementation constraint from "is virtualfs implemented in all the places people want to use it".

To that end, I think it's about the same as you've outlined here, where there's a trait impl that either form takes, so we don't have to worry about nonsense calls like asking VirtualFile for its raw fd. I just mention this here because I'm not sure if this is still the same direction you're thinking, or maybe I'd missed an important conversation in the last month or so.

I'm also curious about the RawFd shenanigans we do in wasi-common - is there a particular reason we _need_ to touch RawFd or ManuallyDrop? I see there are cases we have to update a referenced file descriptor (fdstat_set_flags, to support Windows' impl), but even the we could retain the returned File rather than swapping out fd numbers under the hood. The implementation here looks good, but it'd be nice to be sure we won't accidentally pick up a bug in the future from some change that forgets to drop an fd :D

Approving as this all looks good, and even if there are points to keep from my comment they can be changes in a subsequent PR or here - your choice.

So actually, your model is exactly what I had in mind here. Perhaps naming is somewhat misleading here. When refactoring, I noticed that VirtualFs is like std::fs::File, hence why I pushed it out from OsHandle since OsHandle is also meant to act like std::fs::File. And when streams come up, they will be a type separate from OsHandle. Perhaps renaming will be in order then to avoid this sort of confusion!

Wasmtime GitHub notifications bot (Apr 09 2020 at 20:18):

kubkon commented on Issue #1443:

So after some offline discussion with @sunfishcode, we agreed to land this PR with the intention to take this refactoring further however by splitting the "one trait" for WASI handle into multiple traits, each responsible for different type of functionality, essentially disambiguating between files, dirs, streams, etc. This will take some considerable amount of effort to pull off, but anyway, we do believe this PR is a good first step in that direction, and landing it will give us some decent base to iterate from.

Last updated: Nov 22 2024 at 16:03 UTC