Allow different Handles to act as stdio #1600

kubkon · 2020-04-25T19:06:38Z

There have been requests to allow more than just raw OS handles to act as stdio in wasi-common (see #939). This PR makes this possible by loosening the requirement of the WasiCtxBuilder to accept any type T: Handle + 'static to act as any of the stdio handles.

A couple words about correctness of this approach. Currently, since we only have a single Handle super-trait to represent all possible WASI handle types (files, dirs, stdio, pipes, virtual, etc.), it is possible to pass in any type to act as stdio which can be wrong. However, I envision this being a problem only in the near(est) future until we work out how to split Handle into several traits (FYI @sunfishcode and I have already started figuring this out), each representing a different type of WASI resource. In this particular case, this would be a resource which would implement the interface required for a handle to act as a stdio (with appropriate rights, etc.).

@jeffcharles @pchickey The minimum this PR still needs is an audit of this approach for the time being (whether it's OK to go with it given the potential correctness problems I've outlined above). Docs around different now-public types are also a must if we decide to go with it. Oh, and I haven't really got to testing this approach, i.e., whether this is actually sufficient of a change to facilitate the requirements of #939. Any comments and feedback is thus most welcome!

Closes #939, #1636, #1735

github-actions · 2020-04-25T19:33:52Z

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

kubkon: wasi

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions · 2020-04-25T21:50:18Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Ekleog · 2020-05-08T14:37:47Z

This looks great! FWIW, I was about to open an issue asking for exactly this, for wasmtime-async, so that it'd be possible to create a WASI context that uses async functions (potentially with blocking tasks, especially for file IO).

However, it looks like preopened_dir is not also hooked to take any Handle, in addition to taking Files. Would it be possible for it to take any Handle, so that it's not necessary to go through the entire directory hierarchy just to copy it into a VirtualDirEntry in memory?

kubkon · 2020-05-21T18:42:40Z

@sunfishcode @pchickey Could you guys take a look and let me know if we're up for merging this change in, or would we rather wait for virtual dispatcher? Given that there a few reports from the users that this is a useful change, I'd vote for merging it in, and then redoing it when virtual dispatcher lands, but if you feel otherwise, I'm OK with it as well.

crates/c-api/src/wasi.rs

peterhuene · 2020-06-05T23:02:43Z

Bump ping @kubkon @sunfishcode @pchickey. This blocks the wasmtime-dotnet CI from being green (see #1735), provided we fix my comment above.

There have been requests to allow more than just raw OS handles to act as stdio in `wasi-common`. This commit makes this possible by loosening the requirement of the `WasiCtxBuilder` to accept any type `T: Handle + 'static` to act as any of the stdio handles. A couple words about correctness of this approach. Currently, since we only have a single `Handle` super-trait to represent all possible WASI handle types (files, dirs, stdio, pipes, virtual, etc.), it is possible to pass in any type to act as stdio which can be wrong. However, I envision this being a problem only in the near(est) future until we work out how to split `Handle` into several traits, each representing a different type of WASI resource. In this particular case, this would be a resource which would implement the interface required for a handle to act as a stdio (with appropriate rights, etc.).

kubkon · 2020-06-07T07:39:16Z

@peterhuene this should be it. If you could double check that this indeed fixes #1735 in dotnet side that’d be great. When you confirm everything is green, I reckon we can go ahead and merge this in.

pchickey · 2020-06-08T18:09:33Z

I'll write some docs for the newly exported types so that we can get this merged today.

kubkon · 2020-06-08T18:17:14Z

Oh nice one, thanks @pchickey, and apologies I’ve not done this earlier myself!

peterhuene · 2020-06-08T18:44:59Z

I'll test this shortly with the .NET API and get back to you.

peterhuene · 2020-06-08T19:29:27Z

I can confirm this fixes #1735.

peterhuene

Just that one (nit) comment and the missing docs and this LGTM! Thanks!

peterhuene · 2020-06-08T19:33:01Z

crates/c-api/src/wasi.rs

    )))
 }

+fn wasi_preview_builder(config: wasi_config_t) -> Result<WasiPreview1CtxBuilder> {


Should this function be merged with create_preview1_instance like create_snapshot0_instance is implemented?

pchickey · 2020-06-08T20:59:40Z

@kubkon It looks like for OsDir, OsFile and several of these types, the type itself has been exposed outside the crate but not any of its constructors. I don't think I understand why you'd need to write down this type but wouldnt be allowed to construct it - was this an oversight, or am I missing something?

pchickey · 2020-06-08T21:27:02Z

The output of #![warn(missing_docs)] on wasi-common means we have some real work to do in both wiggle (which should be possible, since doc comments are in the witx ast!) and on Handle and many other places. There's also lots of snapshot 0 code that needs docs, but as legacy code we shouldn't sweat that as much for now.

Apologies for posting a patch for some docs I added, I don't have any way to push to your branch since its in a different repo. (Do you not have write privs on this one? If not we should fix that)

From 6d0b0aad3da0e9b388f2b1c223c20acc2a3c5e87 Mon Sep 17 00:00:00 2001
From: Pat Hickey <pat@moreproductive.org>
Date: Mon, 8 Jun 2020 14:23:32 -0700
Subject: [PATCH] Add some documention to the types exposed by this PR, and a
 few others

---
 crates/wasi-common/src/sys/osfile.rs           | 3 +++
 crates/wasi-common/src/sys/osother.rs          | 2 ++
 crates/wasi-common/src/sys/unix/bsd/osdir.rs   | 7 ++++++-
 crates/wasi-common/src/sys/unix/linux/osdir.rs | 6 +++++-
 crates/wasi-common/src/virtfs.rs               | 4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/crates/wasi-common/src/sys/osfile.rs b/crates/wasi-common/src/sys/osfile.rs
index e9ced03fc..d1ffd6369 100644
--- a/crates/wasi-common/src/sys/osfile.rs
+++ b/crates/wasi-common/src/sys/osfile.rs
@@ -9,6 +9,9 @@ use std::io::{self, Read, Seek, SeekFrom, Write};
 use std::ops::Deref;
 
 #[derive(Debug)]
+/// A file backed by the operating system's file system. Dereferences to a
+/// `RawOsHandle`.  Its impl of `Handle` uses Rust's `std` to implement all
+/// file descriptor operations.
 pub struct OsFile {
     rights: Cell<HandleRights>,
     handle: RawOsHandle,
diff --git a/crates/wasi-common/src/sys/osother.rs b/crates/wasi-common/src/sys/osother.rs
index 42f15c579..fcfa979ac 100644
--- a/crates/wasi-common/src/sys/osother.rs
+++ b/crates/wasi-common/src/sys/osother.rs
@@ -10,6 +10,8 @@ use std::fs::File;
 use std::io::{self, Read, Write};
 use std::ops::Deref;
 
+/// Extra methods for `OsOther` that are only available when configured for
+/// some operating systems.
 pub trait OsOtherExt {
     /// Create `OsOther` as `dyn Handle` from null device.
     fn from_null() -> io::Result<Box<dyn Handle>>;
diff --git a/crates/wasi-common/src/sys/unix/bsd/osdir.rs b/crates/wasi-common/src/sys/unix/bsd/osdir.rs
index 7baa1939e..19d2e1f1b 100644
--- a/crates/wasi-common/src/sys/unix/bsd/osdir.rs
+++ b/crates/wasi-common/src/sys/unix/bsd/osdir.rs
@@ -6,6 +6,9 @@ use std::io;
 use yanix::dir::Dir;
 
 #[derive(Debug)]
+/// A directory in the operating system's file system. Its impl of `Handle` is
+/// in sys::osdir. This type is exposed to all other modules as
+/// sys::osdir::OsDir when configured.
 pub struct OsDir {
     pub(crate) rights: Cell<HandleRights>,
     pub(crate) handle: RawOsHandle,
@@ -39,7 +42,9 @@ impl OsDir {
             stream_ptr,
         })
     }
-    /// Returns the `Dir` stream pointer associated with this `OsDir`.
+    /// Returns the `Dir` stream pointer associated with this `OsDir`. Duck
+    /// typing: sys::unix::fd::readdir expects the configured OsDir to have
+    /// this method.
     pub(crate) fn stream_ptr(&self) -> Result<RefMut<Dir>> {
         Ok(self.stream_ptr.borrow_mut())
     }
diff --git a/crates/wasi-common/src/sys/unix/linux/osdir.rs b/crates/wasi-common/src/sys/unix/linux/osdir.rs
index f15d89a4c..e0747b72b 100644
--- a/crates/wasi-common/src/sys/unix/linux/osdir.rs
+++ b/crates/wasi-common/src/sys/unix/linux/osdir.rs
@@ -6,6 +6,9 @@ use std::io;
 use yanix::dir::Dir;
 
 #[derive(Debug)]
+/// A directory in the operating system's file system. Its impl of `Handle` is
+/// in sys::osdir. This type is exposed to all other moduleas as
+/// sys::osdir::OsDir when configured.
 pub struct OsDir {
     pub(crate) rights: Cell<HandleRights>,
     pub(crate) handle: RawOsHandle,
@@ -16,7 +19,8 @@ impl OsDir {
         let rights = Cell::new(rights);
         Ok(Self { rights, handle })
     }
-    /// Returns the `Dir` stream pointer associated with this `OsDir`.
+    /// Returns the `Dir` stream pointer associated with this `OsDir`. Duck typing:
+    /// sys::unix::fd::readdir expects the configured OsDir to have this method.
     pub(crate) fn stream_ptr(&self) -> Result<Box<Dir>> {
         // We need to duplicate the handle, because `opendir(3)`:
         //     After a successful call to fdopendir(), fd is used internally by the implementation,
diff --git a/crates/wasi-common/src/virtfs.rs b/crates/wasi-common/src/virtfs.rs
index 3d9b61b19..141a9cc69 100644
--- a/crates/wasi-common/src/virtfs.rs
+++ b/crates/wasi-common/src/virtfs.rs
@@ -11,12 +11,16 @@ use std::io::SeekFrom;
 use std::path::{Path, PathBuf};
 use std::rc::Rc;
 
+/// An entry in a virtual filesystem
 pub enum VirtualDirEntry {
+    /// The contents of a child directory
     Directory(HashMap<String, VirtualDirEntry>),
+    /// A file
     File(Box<dyn FileContents>),
 }
 
 impl VirtualDirEntry {
+    /// Construct an empty directory
     pub fn empty_directory() -> Self {
         Self::Directory(HashMap::new())
     }
-- 
2.17.1

kubkon · 2020-06-09T07:45:06Z

@kubkon It looks like for OsDir, OsFile and several of these types, the type itself has been exposed outside the crate but not any of its constructors. I don't think I understand why you'd need to write down this type but wouldnt be allowed to construct it - was this an oversight, or am I missing something?

Oh right, no, this was intentional. For the time being, the only way to create either of those is using TryFrom/TryInto trait from std::fs::File. This way I wanted to ensure that in case anyone ever tries to construct OsFile from a handle that's incompatible (such as a directory handle), then it would error out early. As an added bonus of this approach, the user doesn't have to worry about adjusting the HandleRights manually -- everything is done automatically when calling TryFrom/TryInto.

Here's an example usage I've had in mind:

let some_file = OpenOptions::new().open("some_file")?;
let os_file = OsFile::try_from(some_file)?;

Having said all that, I'm happy to add an explicit constructor for this to make it more readable and usable, something like OsFile::new(file).

kubkon · 2020-06-09T07:54:56Z

The output of #![warn(missing_docs)] on wasi-common means we have some real work to do in both wiggle (which should be possible, since doc comments are in the witx ast!) and on Handle and many other places. There's also lots of snapshot 0 code that needs docs, but as legacy code we shouldn't sweat that as much for now.

Apologies for posting a patch for some docs I added, I don't have any way to push to your branch since its in a different repo. (Do you not have write privs on this one? If not we should fix that)

From 6d0b0aad3da0e9b388f2b1c223c20acc2a3c5e87 Mon Sep 17 00:00:00 2001
From: Pat Hickey <pat@moreproductive.org>
Date: Mon, 8 Jun 2020 14:23:32 -0700
Subject: [PATCH] Add some documention to the types exposed by this PR, and a
 few others

---
 crates/wasi-common/src/sys/osfile.rs           | 3 +++
 crates/wasi-common/src/sys/osother.rs          | 2 ++
 crates/wasi-common/src/sys/unix/bsd/osdir.rs   | 7 ++++++-
 crates/wasi-common/src/sys/unix/linux/osdir.rs | 6 +++++-
 crates/wasi-common/src/virtfs.rs               | 4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/crates/wasi-common/src/sys/osfile.rs b/crates/wasi-common/src/sys/osfile.rs
index e9ced03fc..d1ffd6369 100644
--- a/crates/wasi-common/src/sys/osfile.rs
+++ b/crates/wasi-common/src/sys/osfile.rs
@@ -9,6 +9,9 @@ use std::io::{self, Read, Seek, SeekFrom, Write};
 use std::ops::Deref;
 
 #[derive(Debug)]
+/// A file backed by the operating system's file system. Dereferences to a
+/// `RawOsHandle`.  Its impl of `Handle` uses Rust's `std` to implement all
+/// file descriptor operations.
 pub struct OsFile {
     rights: Cell<HandleRights>,
     handle: RawOsHandle,
diff --git a/crates/wasi-common/src/sys/osother.rs b/crates/wasi-common/src/sys/osother.rs
index 42f15c579..fcfa979ac 100644
--- a/crates/wasi-common/src/sys/osother.rs
+++ b/crates/wasi-common/src/sys/osother.rs
@@ -10,6 +10,8 @@ use std::fs::File;
 use std::io::{self, Read, Write};
 use std::ops::Deref;
 
+/// Extra methods for `OsOther` that are only available when configured for
+/// some operating systems.
 pub trait OsOtherExt {
     /// Create `OsOther` as `dyn Handle` from null device.
     fn from_null() -> io::Result<Box<dyn Handle>>;
diff --git a/crates/wasi-common/src/sys/unix/bsd/osdir.rs b/crates/wasi-common/src/sys/unix/bsd/osdir.rs
index 7baa1939e..19d2e1f1b 100644
--- a/crates/wasi-common/src/sys/unix/bsd/osdir.rs
+++ b/crates/wasi-common/src/sys/unix/bsd/osdir.rs
@@ -6,6 +6,9 @@ use std::io;
 use yanix::dir::Dir;
 
 #[derive(Debug)]
+/// A directory in the operating system's file system. Its impl of `Handle` is
+/// in sys::osdir. This type is exposed to all other modules as
+/// sys::osdir::OsDir when configured.
 pub struct OsDir {
     pub(crate) rights: Cell<HandleRights>,
     pub(crate) handle: RawOsHandle,
@@ -39,7 +42,9 @@ impl OsDir {
             stream_ptr,
         })
     }
-    /// Returns the `Dir` stream pointer associated with this `OsDir`.
+    /// Returns the `Dir` stream pointer associated with this `OsDir`. Duck
+    /// typing: sys::unix::fd::readdir expects the configured OsDir to have
+    /// this method.
     pub(crate) fn stream_ptr(&self) -> Result<RefMut<Dir>> {
         Ok(self.stream_ptr.borrow_mut())
     }
diff --git a/crates/wasi-common/src/sys/unix/linux/osdir.rs b/crates/wasi-common/src/sys/unix/linux/osdir.rs
index f15d89a4c..e0747b72b 100644
--- a/crates/wasi-common/src/sys/unix/linux/osdir.rs
+++ b/crates/wasi-common/src/sys/unix/linux/osdir.rs
@@ -6,6 +6,9 @@ use std::io;
 use yanix::dir::Dir;
 
 #[derive(Debug)]
+/// A directory in the operating system's file system. Its impl of `Handle` is
+/// in sys::osdir. This type is exposed to all other moduleas as
+/// sys::osdir::OsDir when configured.
 pub struct OsDir {
     pub(crate) rights: Cell<HandleRights>,
     pub(crate) handle: RawOsHandle,
@@ -16,7 +19,8 @@ impl OsDir {
         let rights = Cell::new(rights);
         Ok(Self { rights, handle })
     }
-    /// Returns the `Dir` stream pointer associated with this `OsDir`.
+    /// Returns the `Dir` stream pointer associated with this `OsDir`. Duck typing:
+    /// sys::unix::fd::readdir expects the configured OsDir to have this method.
     pub(crate) fn stream_ptr(&self) -> Result<Box<Dir>> {
         // We need to duplicate the handle, because `opendir(3)`:
         //     After a successful call to fdopendir(), fd is used internally by the implementation,
diff --git a/crates/wasi-common/src/virtfs.rs b/crates/wasi-common/src/virtfs.rs
index 3d9b61b19..141a9cc69 100644
--- a/crates/wasi-common/src/virtfs.rs
+++ b/crates/wasi-common/src/virtfs.rs
@@ -11,12 +11,16 @@ use std::io::SeekFrom;
 use std::path::{Path, PathBuf};
 use std::rc::Rc;
 
+/// An entry in a virtual filesystem
 pub enum VirtualDirEntry {
+    /// The contents of a child directory
     Directory(HashMap<String, VirtualDirEntry>),
+    /// A file
     File(Box<dyn FileContents>),
 }
 
 impl VirtualDirEntry {
+    /// Construct an empty directory
     pub fn empty_directory() -> Self {
         Self::Directory(HashMap::new())
     }
-- 
2.17.1

Hmm, I thought you should be able to push directly to my branch in my fork. And yes, I do have write permissions, however, I always prefer to work out of my fork so as to shield myself from stupid mistakes such as force-pushing into master etc. Anyhow, that's OK, I'll add your patch into my branch myself 👍. Lemme know what you reckon about the constructors of OsFile, etc. (whether you think we should add explicit ones or TryFrom trait is enough).

Signed-off-by: Jakub Konka <kubkon@jakubkonka.com>

kubkon · 2020-06-09T12:18:08Z

@pchickey I've applied your patch (thanks!), and extended the docs with some constructing examples. Have a look and lemme know what you reckon!

pchickey

Thanks for the explanation, I had missed that try_from File was the way to construct these, the updates to the docs are very welcome there. As soon as CI finishes this looks ready to merge

github-actions bot added the wasi Issues pertaining to WASI label Apr 25, 2020

github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Apr 25, 2020

kubkon force-pushed the vfs-stdio branch from d1ebad8 to bc1fcaa Compare April 26, 2020 08:00

This was referenced Apr 26, 2020

Exporting dec_ciovec_slice and ciovec_to_host in wasi-common #939

Closed

Allow using virtual files for stdin/out/err #1636

Closed

kubkon force-pushed the vfs-stdio branch 4 times, most recently from f62a3f6 to 05c278d Compare May 8, 2020 07:49

kubkon requested review from pchickey and sunfishcode May 8, 2020 07:51

kubkon marked this pull request as ready for review May 8, 2020 07:51

Ekleog mentioned this pull request May 8, 2020

re-entrant/cooperative wasm? #642

Closed

kubkon mentioned this pull request May 21, 2020

wasi-common: cannot use file handles for WasiCtxBuilder::std[in|out|err] #1735

Closed

kubkon force-pushed the vfs-stdio branch from 05c278d to 4da10b9 Compare May 21, 2020 18:40

peterhuene reviewed May 21, 2020

View reviewed changes

crates/c-api/src/wasi.rs Outdated Show resolved Hide resolved

peterhuene mentioned this pull request Jun 6, 2020

Added two methods for loading embedded .wasm and .wat bytecodealliance/wasmtime-dotnet#13

Merged

kubkon added 2 commits June 7, 2020 08:56

Use OsFile in c-api

3dcb495

kubkon force-pushed the vfs-stdio branch from 4da10b9 to 3dcb495 Compare June 7, 2020 06:57

pchickey mentioned this pull request Jun 8, 2020

Unable to replace stdin/stdout/stderr with user-provided File #1840

Closed

peterhuene approved these changes Jun 8, 2020

View reviewed changes

peterhuene mentioned this pull request Jun 8, 2020

Update expected trap message for backtrace. bytecodealliance/wasmtime-dotnet#16

Merged

pchickey and others added 2 commits June 9, 2020 13:42

Add some documention to the types exposed by this PR, and a few others

5521106

Signed-off-by: Jakub Konka <kubkon@jakubkonka.com>

Add construction examples and missing docs for Handle trait

4e1fc02

kubkon added 2 commits June 9, 2020 19:02

Fix example on Windows

e2fd57f

Merge wasi_preview_builder into create_preview1_instance

11edb95

pchickey approved these changes Jun 9, 2020

View reviewed changes

kubkon merged commit f47133b into bytecodealliance:master Jun 9, 2020

kubkon deleted the vfs-stdio branch June 9, 2020 18:22

peterhuene mentioned this pull request Jul 9, 2020

Fix kubelet exit on module failure krustlet/krustlet#319

Merged

Ekleog mentioned this pull request Jan 11, 2021

wasi-common: add preopened_handle, to be able to open any handle #2569

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow different Handles to act as stdio #1600

Allow different Handles to act as stdio #1600

kubkon commented Apr 25, 2020 •

edited

Loading

github-actions bot commented Apr 25, 2020

github-actions bot commented Apr 25, 2020

Ekleog commented May 8, 2020

kubkon commented May 21, 2020

peterhuene commented Jun 5, 2020

kubkon commented Jun 7, 2020

pchickey commented Jun 8, 2020

kubkon commented Jun 8, 2020

peterhuene commented Jun 8, 2020

peterhuene commented Jun 8, 2020

peterhuene left a comment

peterhuene Jun 8, 2020

pchickey commented Jun 8, 2020

pchickey commented Jun 8, 2020 •

edited

Loading

kubkon commented Jun 9, 2020 •

edited

Loading

kubkon commented Jun 9, 2020

kubkon commented Jun 9, 2020

pchickey left a comment

Allow different Handles to act as stdio #1600

Allow different Handles to act as stdio #1600

Conversation

kubkon commented Apr 25, 2020 • edited Loading

github-actions bot commented Apr 25, 2020

Subscribe to Label Action

github-actions bot commented Apr 25, 2020

Subscribe to Label Action

Ekleog commented May 8, 2020

kubkon commented May 21, 2020

peterhuene commented Jun 5, 2020

kubkon commented Jun 7, 2020

pchickey commented Jun 8, 2020

kubkon commented Jun 8, 2020

peterhuene commented Jun 8, 2020

peterhuene commented Jun 8, 2020

peterhuene left a comment

Choose a reason for hiding this comment

peterhuene Jun 8, 2020

Choose a reason for hiding this comment

pchickey commented Jun 8, 2020

pchickey commented Jun 8, 2020 • edited Loading

kubkon commented Jun 9, 2020 • edited Loading

kubkon commented Jun 9, 2020

kubkon commented Jun 9, 2020

pchickey left a comment

Choose a reason for hiding this comment

kubkon commented Apr 25, 2020 •

edited

Loading

pchickey commented Jun 8, 2020 •

edited

Loading

kubkon commented Jun 9, 2020 •

edited

Loading