Stream: git-wasmtime

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

Wasmtime GitHub notifications bot (Mar 31 2020 at 15:36):

kubkon opened PR #1443 from yacc to master:

OK, so this PR is a bit of an experiment that came about somewhat itself
when I was looking at refactoring use of Rc<RefCell<Descriptor>> inside
Entry struct. I've noticed that since we've placed VirtualFile on the
same level as OsHandle and Stdin etc., we've ended up necessiitating
checks for different combinations such as "is a real OS resource being mixed
up with a virtual resource?", and if that was the case, we'd panic since
this was clearly not allowed (e.g., symlinking, or worse renaming).
Therefore, it seemed natural for virtual file to be on the same level
as _any_ OS handle (regardless of whether it's an actual file, socket,
or stdio handle). In other words, we should ideally envision the following
hierarchy:

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

This way, we can deal with the mix up at a level above which cleans up
our logic significantly.

On the other hand, when looking through the virtfs, the trait approach
to some type that's a valid Handle grew on me, and I think this
is the way to go. And this is what this PR is proposing, a trait
Handle which features enough functionality to make both virtual and
OS ops to work. Now, inside Entry we can safely store something like
Rc<dyn Handle> where Handle can downcast to either VirtualFile or
VirtualDir, or OsHandle if its an actual OS resource. Note that
I've left Handle as one massive trait, but I reckon we could split
it up into several smaller traits, each dealing with some bit of WASI
functionality. I'm hoping this would perhaps make it easier to figure
out polyfilling between snapshots and the new upcoming ephemeral
snapshot since a lot of boilerplate functionality is now done as part
of the Handle trait implementation.

Next, I've redone the original OsHandle to be an OsFile which
now stores a raw descriptor/handle (RawFd/RawHandle) inside a
Cell so that we can handle interior mutability in an easy (read,
non-panicky) way. In order not to lose the perks of derefercing to
std::fs::File, I've added a convenience trait AsFile which
will take OsFile by reference (or the stdio handles) and create
a non-owned ManuallyDrop<File> resource which can be passed around
and acted upon the way we'd normally do on &File. This change of
course implies that we now have to worry about properly closing all
OS resources stored as part of OsFile, thus this type now implements
Drop trait which essentially speaking moves the raw descriptor/handle
into a File and drops it.

Finally, I've redone setting time info on relative paths on *nix using
the same approach as advocated in the virtual fs. Namely, we do an
openat followed by filestat_set_times on the obtained descriptor.
This effectively removes the need for custom filetime module in
yanix. However, this does probably incur additional cost of at least
one additional syscall, and I haven't checked whether this approach
performs as expected on platforms such as NixOS which as far as I remember
had some weirdness todo with linking utimensat symbols, etc. Still,
this change is worth considering given that the implementation of
path_filestat_set_times cleans up a lot, albeit with some additional
cost.

Anyhow, lemme y'all know what you think!

PS, if anybody is wondering why the incoming branch is called yacc,
it stands for "yet another cleanup cleanup" simply because I was out
of ideas...

Wasmtime GitHub notifications bot (Mar 31 2020 at 15:36):

kubkon requested iximeow for a review on PR #1443.

Wasmtime GitHub notifications bot (Mar 31 2020 at 15:37):

kubkon requested sunfishcode and iximeow for a review on PR #1443.

Wasmtime GitHub notifications bot (Mar 31 2020 at 15:37):

kubkon assigned PR #1443 to alexcrichton.

Wasmtime GitHub notifications bot (Mar 31 2020 at 15:37):

kubkon unassigned PR #1443 to alexcrichton.

Wasmtime GitHub notifications bot (Mar 31 2020 at 15:37):

kubkon requested alexcrichton, sunfishcode and iximeow for a review on PR #1443.

Wasmtime GitHub notifications bot (Mar 31 2020 at 17:01):

kubkon updated PR #1443 from yacc to master:

OK, so this PR is a bit of an experiment that came about somewhat itself
when I was looking at refactoring use of Rc<RefCell<Descriptor>> inside
Entry struct. I've noticed that since we've placed VirtualFile on the
same level as OsHandle and Stdin etc., we've ended up necessiitating
checks for different combinations such as "is a real OS resource being mixed
up with a virtual resource?", and if that was the case, we'd panic since
this was clearly not allowed (e.g., symlinking, or worse renaming).
Therefore, it seemed natural for virtual file to be on the same level
as _any_ OS handle (regardless of whether it's an actual file, socket,
or stdio handle). In other words, we should ideally envision the following
hierarchy:

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

This way, we can deal with the mix up at a level above which cleans up
our logic significantly.

On the other hand, when looking through the virtfs, the trait approach
to some type that's a valid Handle grew on me, and I think this
is the way to go. And this is what this PR is proposing, a trait
Handle which features enough functionality to make both virtual and
OS ops to work. Now, inside Entry we can safely store something like
Rc<dyn Handle> where Handle can downcast to either VirtualFile or
VirtualDir, or OsHandle if its an actual OS resource. Note that
I've left Handle as one massive trait, but I reckon we could split
it up into several smaller traits, each dealing with some bit of WASI
functionality. I'm hoping this would perhaps make it easier to figure
out polyfilling between snapshots and the new upcoming ephemeral
snapshot since a lot of boilerplate functionality is now done as part
of the Handle trait implementation.

Next, I've redone the original OsHandle to be an OsFile which
now stores a raw descriptor/handle (RawFd/RawHandle) inside a
Cell so that we can handle interior mutability in an easy (read,
non-panicky) way. In order not to lose the perks of derefercing to
std::fs::File, I've added a convenience trait AsFile which
will take OsFile by reference (or the stdio handles) and create
a non-owned ManuallyDrop<File> resource which can be passed around
and acted upon the way we'd normally do on &File. This change of
course implies that we now have to worry about properly closing all
OS resources stored as part of OsFile, thus this type now implements
Drop trait which essentially speaking moves the raw descriptor/handle
into a File and drops it.

Finally, I've redone setting time info on relative paths on *nix using
the same approach as advocated in the virtual fs. Namely, we do an
openat followed by filestat_set_times on the obtained descriptor.
This effectively removes the need for custom filetime module in
yanix. However, this does probably incur additional cost of at least
one additional syscall, and I haven't checked whether this approach
performs as expected on platforms such as NixOS which as far as I remember
had some weirdness todo with linking utimensat symbols, etc. Still,
this change is worth considering given that the implementation of
path_filestat_set_times cleans up a lot, albeit with some additional
cost.

Anyhow, lemme y'all know what you think!

PS, if anybody is wondering why the incoming branch is called yacc,
it stands for "yet another cleanup cleanup" simply because I was out
of ideas...

Wasmtime GitHub notifications bot (Apr 01 2020 at 07:06):

kubkon updated PR #1443 from yacc to master:

OK, so this PR is a bit of an experiment that came about somewhat itself
when I was looking at refactoring use of Rc<RefCell<Descriptor>> inside
Entry struct. I've noticed that since we've placed VirtualFile on the
same level as OsHandle and Stdin etc., we've ended up necessiitating
checks for different combinations such as "is a real OS resource being mixed
up with a virtual resource?", and if that was the case, we'd panic since
this was clearly not allowed (e.g., symlinking, or worse renaming).
Therefore, it seemed natural for virtual file to be on the same level
as _any_ OS handle (regardless of whether it's an actual file, socket,
or stdio handle). In other words, we should ideally envision the following
hierarchy:

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

This way, we can deal with the mix up at a level above which cleans up
our logic significantly.

On the other hand, when looking through the virtfs, the trait approach
to some type that's a valid Handle grew on me, and I think this
is the way to go. And this is what this PR is proposing, a trait
Handle which features enough functionality to make both virtual and
OS ops to work. Now, inside Entry we can safely store something like
Rc<dyn Handle> where Handle can downcast to either VirtualFile or
VirtualDir, or OsHandle if its an actual OS resource. Note that
I've left Handle as one massive trait, but I reckon we could split
it up into several smaller traits, each dealing with some bit of WASI
functionality. I'm hoping this would perhaps make it easier to figure
out polyfilling between snapshots and the new upcoming ephemeral
snapshot since a lot of boilerplate functionality is now done as part
of the Handle trait implementation.

Next, I've redone the original OsHandle to be an OsFile which
now stores a raw descriptor/handle (RawFd/RawHandle) inside a
Cell so that we can handle interior mutability in an easy (read,
non-panicky) way. In order not to lose the perks of derefercing to
std::fs::File, I've added a convenience trait AsFile which
will take OsFile by reference (or the stdio handles) and create
a non-owned ManuallyDrop<File> resource which can be passed around
and acted upon the way we'd normally do on &File. This change of
course implies that we now have to worry about properly closing all
OS resources stored as part of OsFile, thus this type now implements
Drop trait which essentially speaking moves the raw descriptor/handle
into a File and drops it.

Finally, I've redone setting time info on relative paths on *nix using
the same approach as advocated in the virtual fs. Namely, we do an
openat followed by filestat_set_times on the obtained descriptor.
This effectively removes the need for custom filetime module in
yanix. However, this does probably incur additional cost of at least
one additional syscall, and I haven't checked whether this approach
performs as expected on platforms such as NixOS which as far as I remember
had some weirdness todo with linking utimensat symbols, etc. Still,
this change is worth considering given that the implementation of
path_filestat_set_times cleans up a lot, albeit with some additional
cost.

Anyhow, lemme y'all know what you think!

PS, if anybody is wondering why the incoming branch is called yacc,
it stands for "yet another cleanup cleanup" simply because I was out
of ideas...

Wasmtime GitHub notifications bot (Apr 01 2020 at 18:02):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Apr 01 2020 at 18:02):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Apr 01 2020 at 18:02):

pchickey created PR Review Comment:

left in some commented out debugs here and just below

Wasmtime GitHub notifications bot (Apr 01 2020 at 21:37):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 06 2020 at 16:43):

kubkon updated PR #1443 from yacc to master:

OK, so this PR is a bit of an experiment that came about somewhat itself
when I was looking at refactoring use of Rc<RefCell<Descriptor>> inside
Entry struct. I've noticed that since we've placed VirtualFile on the
same level as OsHandle and Stdin etc., we've ended up necessiitating
checks for different combinations such as "is a real OS resource being mixed
up with a virtual resource?", and if that was the case, we'd panic since
this was clearly not allowed (e.g., symlinking, or worse renaming).
Therefore, it seemed natural for virtual file to be on the same level
as _any_ OS handle (regardless of whether it's an actual file, socket,
or stdio handle). In other words, we should ideally envision the following
hierarchy:

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

This way, we can deal with the mix up at a level above which cleans up
our logic significantly.

On the other hand, when looking through the virtfs, the trait approach
to some type that's a valid Handle grew on me, and I think this
is the way to go. And this is what this PR is proposing, a trait
Handle which features enough functionality to make both virtual and
OS ops to work. Now, inside Entry we can safely store something like
Rc<dyn Handle> where Handle can downcast to either VirtualFile or
VirtualDir, or OsHandle if its an actual OS resource. Note that
I've left Handle as one massive trait, but I reckon we could split
it up into several smaller traits, each dealing with some bit of WASI
functionality. I'm hoping this would perhaps make it easier to figure
out polyfilling between snapshots and the new upcoming ephemeral
snapshot since a lot of boilerplate functionality is now done as part
of the Handle trait implementation.

Next, I've redone the original OsHandle to be an OsFile which
now stores a raw descriptor/handle (RawFd/RawHandle) inside a
Cell so that we can handle interior mutability in an easy (read,
non-panicky) way. In order not to lose the perks of derefercing to
std::fs::File, I've added a convenience trait AsFile which
will take OsFile by reference (or the stdio handles) and create
a non-owned ManuallyDrop<File> resource which can be passed around
and acted upon the way we'd normally do on &File. This change of
course implies that we now have to worry about properly closing all
OS resources stored as part of OsFile, thus this type now implements
Drop trait which essentially speaking moves the raw descriptor/handle
into a File and drops it.

Finally, I've redone setting time info on relative paths on *nix using
the same approach as advocated in the virtual fs. Namely, we do an
openat followed by filestat_set_times on the obtained descriptor.
This effectively removes the need for custom filetime module in
yanix. However, this does probably incur additional cost of at least
one additional syscall, and I haven't checked whether this approach
performs as expected on platforms such as NixOS which as far as I remember
had some weirdness todo with linking utimensat symbols, etc. Still,
this change is worth considering given that the implementation of
path_filestat_set_times cleans up a lot, albeit with some additional
cost.

Anyhow, lemme y'all know what you think!

PS, if anybody is wondering why the incoming branch is called yacc,
it stands for "yet another cleanup cleanup" simply because I was out
of ideas...

Wasmtime GitHub notifications bot (Apr 06 2020 at 21:42):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Apr 06 2020 at 21:42):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Apr 06 2020 at 21:42):

iximeow created PR Review Comment:

Why the + 'static here? My understanding of the interaction between lifetimes/trait objects is kind of fuzzy, but isn't 'static the implied default? (I'm not sure why we'd want to forbid non-'static, either)

Wasmtime GitHub notifications bot (Apr 06 2020 at 21:42):

iximeow created PR Review Comment:

What _is_ the TODO here? The get_file_type() == Err case?

Wasmtime GitHub notifications bot (Apr 06 2020 at 21:42):

iximeow created PR Review Comment:

update_from on an OsFile that has a dir with Some would then have a stale dir. I'm not sure how much an issue this actually is, since we only update_from in fdstat_set_flags. Flags are on the referenced filesystem entry, not on the fd, right? So inconsistent flags aren't actually possible? Otherwise we might end up in a state where dir was populated can iterate when it shouldn't, or can't iterate when it should.

At the very least, if this is sound (and I suspect it is), I think it'd be good to have a comment describing why alongside this impl.

Wasmtime GitHub notifications bot (Apr 06 2020 at 21:42):

iximeow created PR Review Comment:

ReOpenFile requires the file handle came from CreateFile, but the standard library will use GetStdHandle rather than CreateFile("CONIN$",... or CONOUT$. If it works I'd be a little suspect of it mysteriously breaking one day. Judging by https://github.com/rust-lang/rust/issues/40490 (found when looking to confirm how stdlib deals with Windows stdio) it seems pretty suspect to retain a stdio handle in the first place?

Nothing specific to add, just topical thought meandering, sorry :sweat_smile:

Wasmtime GitHub notifications bot (Apr 06 2020 at 21:54):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Apr 06 2020 at 21:54):

pchickey created PR Review Comment:

That syntax threw me off when I learned it a week or two ago, it just means that the Handle concrete type cant have a lifetime.

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

kubkon submitted PR Review.

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

kubkon created PR Review Comment:

Yep, exactly as @pchickey said. I can't foresee a situation where type that implements Handle wouldn't last for the entire duration of the program.

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

kubkon submitted PR Review.

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

kubkon created PR Review Comment:

Oh right, I forgot to remove it. Thanks for spotting this one!

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

kubkon created PR Review Comment:

That's an excellent observation, thanks! I think adding a comment to remember about this potential issue is the best thing to do here.

On a related albeit somewhat different note, I'd still like to refactor how we store the DIR stream pointer on BSDes in OsFile. Having an embedded Option seems like a less than ideal solution to use here. I was actually thinking of splitting the type into two or possibly an enum, so that if we are dealing with a dir on BSD, we'd immediately store a valid DIR stream pointer. But this will be a subsequent PR :-)

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

kubkon submitted PR Review.

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

kubkon submitted PR Review.

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

kubkon created PR Review Comment:

Oh good find, thanks! So if I understood it right, modifying stdio should error out on Windows since the handle was obtained from GetStdHandle and not CreateFile. I'm keen to leave some sort of TODO here anyway to investigate this further on Windows.

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

kubkon updated PR #1443 from yacc to master:

OK, so this PR is a bit of an experiment that came about somewhat itself
when I was looking at refactoring use of Rc<RefCell<Descriptor>> inside
Entry struct. I've noticed that since we've placed VirtualFile on the
same level as OsHandle and Stdin etc., we've ended up necessiitating
checks for different combinations such as "is a real OS resource being mixed
up with a virtual resource?", and if that was the case, we'd panic since
this was clearly not allowed (e.g., symlinking, or worse renaming).
Therefore, it seemed natural for virtual file to be on the same level
as _any_ OS handle (regardless of whether it's an actual file, socket,
or stdio handle). In other words, we should ideally envision the following
hierarchy:

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

This way, we can deal with the mix up at a level above which cleans up
our logic significantly.

On the other hand, when looking through the virtfs, the trait approach
to some type that's a valid Handle grew on me, and I think this
is the way to go. And this is what this PR is proposing, a trait
Handle which features enough functionality to make both virtual and
OS ops to work. Now, inside Entry we can safely store something like
Rc<dyn Handle> where Handle can downcast to either VirtualFile or
VirtualDir, or OsHandle if its an actual OS resource. Note that
I've left Handle as one massive trait, but I reckon we could split
it up into several smaller traits, each dealing with some bit of WASI
functionality. I'm hoping this would perhaps make it easier to figure
out polyfilling between snapshots and the new upcoming ephemeral
snapshot since a lot of boilerplate functionality is now done as part
of the Handle trait implementation.

Next, I've redone the original OsHandle to be an OsFile which
now stores a raw descriptor/handle (RawFd/RawHandle) inside a
Cell so that we can handle interior mutability in an easy (read,
non-panicky) way. In order not to lose the perks of derefercing to
std::fs::File, I've added a convenience trait AsFile which
will take OsFile by reference (or the stdio handles) and create
a non-owned ManuallyDrop<File> resource which can be passed around
and acted upon the way we'd normally do on &File. This change of
course implies that we now have to worry about properly closing all
OS resources stored as part of OsFile, thus this type now implements
Drop trait which essentially speaking moves the raw descriptor/handle
into a File and drops it.

Finally, I've redone setting time info on relative paths on *nix using
the same approach as advocated in the virtual fs. Namely, we do an
openat followed by filestat_set_times on the obtained descriptor.
This effectively removes the need for custom filetime module in
yanix. However, this does probably incur additional cost of at least
one additional syscall, and I haven't checked whether this approach
performs as expected on platforms such as NixOS which as far as I remember
had some weirdness todo with linking utimensat symbols, etc. Still,
this change is worth considering given that the implementation of
path_filestat_set_times cleans up a lot, albeit with some additional
cost.

Anyhow, lemme y'all know what you think!

PS, if anybody is wondering why the incoming branch is called yacc,
it stands for "yet another cleanup cleanup" simply because I was out
of ideas...

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

kubkon updated PR #1443 from yacc to master:

OK, so this PR is a bit of an experiment that came about somewhat itself
when I was looking at refactoring use of Rc<RefCell<Descriptor>> inside
Entry struct. I've noticed that since we've placed VirtualFile on the
same level as OsHandle and Stdin etc., we've ended up necessiitating
checks for different combinations such as "is a real OS resource being mixed
up with a virtual resource?", and if that was the case, we'd panic since
this was clearly not allowed (e.g., symlinking, or worse renaming).
Therefore, it seemed natural for virtual file to be on the same level
as _any_ OS handle (regardless of whether it's an actual file, socket,
or stdio handle). In other words, we should ideally envision the following
hierarchy:

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

This way, we can deal with the mix up at a level above which cleans up
our logic significantly.

On the other hand, when looking through the virtfs, the trait approach
to some type that's a valid Handle grew on me, and I think this
is the way to go. And this is what this PR is proposing, a trait
Handle which features enough functionality to make both virtual and
OS ops to work. Now, inside Entry we can safely store something like
Rc<dyn Handle> where Handle can downcast to either VirtualFile or
VirtualDir, or OsHandle if its an actual OS resource. Note that
I've left Handle as one massive trait, but I reckon we could split
it up into several smaller traits, each dealing with some bit of WASI
functionality. I'm hoping this would perhaps make it easier to figure
out polyfilling between snapshots and the new upcoming ephemeral
snapshot since a lot of boilerplate functionality is now done as part
of the Handle trait implementation.

Next, I've redone the original OsHandle to be an OsFile which
now stores a raw descriptor/handle (RawFd/RawHandle) inside a
Cell so that we can handle interior mutability in an easy (read,
non-panicky) way. In order not to lose the perks of derefercing to
std::fs::File, I've added a convenience trait AsFile which
will take OsFile by reference (or the stdio handles) and create
a non-owned ManuallyDrop<File> resource which can be passed around
and acted upon the way we'd normally do on &File. This change of
course implies that we now have to worry about properly closing all
OS resources stored as part of OsFile, thus this type now implements
Drop trait which essentially speaking moves the raw descriptor/handle
into a File and drops it.

Finally, I've redone setting time info on relative paths on *nix using
the same approach as advocated in the virtual fs. Namely, we do an
openat followed by filestat_set_times on the obtained descriptor.
This effectively removes the need for custom filetime module in
yanix. However, this does probably incur additional cost of at least
one additional syscall, and I haven't checked whether this approach
performs as expected on platforms such as NixOS which as far as I remember
had some weirdness todo with linking utimensat symbols, etc. Still,
this change is worth considering given that the implementation of
path_filestat_set_times cleans up a lot, albeit with some additional
cost.

Anyhow, lemme y'all know what you think!

PS, if anybody is wondering why the incoming branch is called yacc,
it stands for "yet another cleanup cleanup" simply because I was out
of ideas...

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

kubkon merged PR #1443.

Last updated: Nov 22 2024 at 16:03 UTC