Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(vfs) Try to unify file descriptor #2528

Merged
merged 5 commits into from
Aug 17, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Aug 16, 2021

Unfortunately, this patch contains multiple features.

First off, it renames the types from host_fs and mem_fs so that
they use the same names. Apart from the module name, the types owned
by the module are the same, which simplifies the usage greatly.

Second, the code has been cleaned up a little bit. The more important
clean up is probably the renaming of MemKind to Node. It's OK to
do that here, because the code is not released yet.

Third, a new FileDescriptor type is introduced. It represents a
“generic” file descriptor, that a VirtualFile can hold. It contains
a value with the size of the larger size a file descriptor on any
platform, namely on Windows where a file descriptor is the size of a
pointer (compared to Unix where it's a i32). Then a bunch of
TryInto implementations are written to convert it to RawFd (on
Unix) or RawHandle (on Windows). I'm not confident it's the best
design for the moment, but it's better than before where Windows file
descriptors were impossible to represent. Note that we cannot make
FileDescriptor generic over the system for 2 reasons

  1. We can activate multiple file systems at the same time, so the
    representation of a file descriptor can change between file system,

  2. If we make FileDescriptor generic, then the size of dyn VirtualFile is unknown at compile-time, which makes impossible to
    use in wasmer-wasi, hence this design decision of taking the
    larger possible size for a file descriptor so that it fits all
    scenarios.

@syrusakbary
Copy link
Member

syrusakbary commented Aug 16, 2021

I believe a strategy using unsafe-io will be better for the cross-platform abstraction.
https://docs.rs/unsafe-io/0.7.1/unsafe_io/

UnsafeFile, UnsafeSocket or UnsafeHandle.

Resource Posix-ish type Windows type   Platform-independent types
File/Pipe RawFd RawHandle   UnsafeHandle or UnsafeFile
Socket RawFd RawSocket   UnsafeHandle or UnsafeSocket
Any RawFd RawHandleOrSocket   UnsafeHandle

@Hywan
Copy link
Contributor Author

Hywan commented Aug 17, 2021

Actually we cannot do that.

In our case, we cannot have multiple file descriptor implementations. Why? Because the entry point is VirtualFile, which has a get_(raw)_fd method. It must return a statically known value, otherwise the size of VirtualFile cannot be known at compile-time. So we cannot use a FileDescriptor implementation from host_fs or mem_fs, or even both depending on how wasmer-vfs is compiled.

This patch contains a little bit more clean up, but everything compiles gently, even with wasmer-wasi.

Calling about wasmer-wasi, from a VirtualFile, to get a RawFd, we now just need to do: virtual_file.get_fd().try_into().unwrap() and that's it. The addition compared to the initial code is try_into. We could use an Into implementation but I prefer to provide safe abstraction and not assuming that the file descriptor has been constructed correctly.

I've also tried but cancelled, to “connect”/“link” a FileDescriptor to a filesystem instance, but it actually makes VirtualFile really complex. The gain doesn't worth it I suppose for the moment.

Unfortunately, this patch contains multiple features.

First off, it renames the types from `host_fs` and `mem_fs` so that
they use the same names. Apart from the module name, the types owned
by the module are the same, which simplifies the usage greatly.

Second, the code has been cleaned up a little bit. The more important
clean up is probably the renaming of `MemKind` to `Node`. It's OK to
do that here, because the code is not released yet.

Third, a new `FileDescriptor` type is introduced. It represents a
“generic” file descriptor, that a `VirtualFile` can hold. It contains
a value with the size of the larger size a file descriptor on any
platform, namely on Windows where a file descriptor is the size of a
pointer (compared to Unix where it's a `i32`). Then a bunch of
`TryInto` implementations are written to convert it to `RawFd` (on
Unix) or `RawHandle` (on Windows). I'm not confident it's the best
design for the moment, but it's better than before where Windows file
descriptors were impossible to represent. Note that we cannot make
`FileDescriptor` generic over the system for 2 reasons

1. We can activate multiple file systems at the same time, so the
   representation of a file descriptor can change between file system,

2. If we make `FileDescriptor` generic, then the size of `dyn
   VirtualFile` is unknown at compile-time, which makes impossible to
   use in `wasmer-wasi`, hence this design decision of taking the
   larger possible size for a file descriptor so that it fits all
   scenarios.
When rebasing, Git losts some of the commit. Here is a bunch of them
in a single commit… To learn more, see
wasmerio@f211130.
@Hywan Hywan changed the title feat(vfs) Try to unified file descriptor feat(vfs) Try to unify file descriptor Aug 17, 2021
@Hywan Hywan merged commit cbc20c1 into wasmerio:js-api-wasi Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants