Stream: git-wasmtime

Topic: wasmtime / Issue #1600 Allow different Handles to act as ...

Wasmtime GitHub notifications bot (Apr 25 2020 at 19:33):

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

Subscribe to Label Action

cc @kubkon

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Apr 25 2020 at 21:50):

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

Subscribe to Label Action

cc @peterhuene

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (May 08 2020 at 14:37):

Ekleog commented on Issue #1600:

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?

Wasmtime GitHub notifications bot (May 21 2020 at 18:42):

kubkon commented on Issue #1600:

@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.

Wasmtime GitHub notifications bot (Jun 05 2020 at 23:02):

peterhuene commented on Issue #1600:

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

Wasmtime GitHub notifications bot (Jun 07 2020 at 07:39):

kubkon commented on Issue #1600:

@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.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:09):

pchickey commented on Issue #1600:

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

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:17):

kubkon commented on Issue #1600:

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

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:44):

peterhuene commented on Issue #1600:

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

Wasmtime GitHub notifications bot (Jun 08 2020 at 19:29):

peterhuene commented on Issue #1600:

I can confirm this fixes #1735.

Wasmtime GitHub notifications bot (Jun 08 2020 at 20:59):

pchickey commented on Issue #1600:

@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?

Wasmtime GitHub notifications bot (Jun 08 2020 at 21:27):

pchickey commented on Issue #1600:

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.

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

Wasmtime GitHub notifications bot (Jun 08 2020 at 21:27):

pchickey edited a comment on Issue #1600:

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

Wasmtime GitHub notifications bot (Jun 09 2020 at 07:45):

kubkon commented on Issue #1600:

@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)?;

Wasmtime GitHub notifications bot (Jun 09 2020 at 07:52):

kubkon edited a comment on Issue #1600:

@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).

Wasmtime GitHub notifications bot (Jun 09 2020 at 07:54):

kubkon commented on Issue #1600:

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 :+1:. Lemme know what you reckon about the constructors of OsFile, etc. (whether you think we should add explicit ones or TryFrom trait is enough).

Wasmtime GitHub notifications bot (Jun 09 2020 at 12:18):

kubkon commented on Issue #1600:

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

Last updated: Nov 22 2024 at 16:03 UTC