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>
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>
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 anyHandle
, in addition to takingFile
s. Would it be possible for it to take anyHandle
, so that it's not necessary to go through the entire directory hierarchy just to copy it into aVirtualDirEntry
in memory?
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.
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.
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.
pchickey commented on Issue #1600:
I'll write some docs for the newly exported types so that we can get this merged today.
kubkon commented on Issue #1600:
Oh nice one, thanks @pchickey, and apologies I’ve not done this earlier myself!
peterhuene commented on Issue #1600:
I'll test this shortly with the .NET API and get back to you.
peterhuene commented on Issue #1600:
I can confirm this fixes #1735.
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?
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 onHandle
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
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 onHandle
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 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 fromstd::fs::File
. This way I wanted to ensure that in case anyone ever tries to constructOsFile
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 theHandleRights
manually -- everything is done automatically when callingTryFrom
/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)?;
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 fromstd::fs::File
. This way I wanted to ensure that in case anyone ever tries to constructOsFile
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 theHandleRights
manually -- everything is done automatically when callingTryFrom
/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 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 onHandle
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 ofHandle
uses Rust'sstd
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 {
/// CreateOsOther
asdyn 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 ofHandle
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 theDir
stream pointer associated with thisOsDir
.
+ /// Returns theDir
stream pointer associated with thisOsDir
. 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 ofHandle
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 theDir
stream pointer associated with thisOsDir
.
+ /// Returns theDir
stream pointer associated with thisOsDir
. 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, becauseopendir(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 ofOsFile
, etc. (whether you think we should add explicit ones orTryFrom
trait is enough).
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: Dec 23 2024 at 12:05 UTC