Delegate definition of OsDir to OS-specific modules

Delegates defining `OsDir` struct to OS-specific modules (BSD, Linux,
Emscripten, Windows). This way, `OsDir` can safely re-use `OsHandle`
for raw OS handle storage, and can store some aux data such as an
initialized stream ptr in case of BSD. As a result, we can safely
get rid of `OsDirHandle` which IMHO was causing unnecessary noise and
overcomplicating the design. On the other hand, delegating definition
of `OsDir` to OS-specific modules isn't super clean in and of itself
either. Perhaps there's a better way of handling this?

Author: Jakub Konka

crates/wasi-common/src/sys/osdir.rs

@@ -1,28 +1,19 @@
-use super::sys_impl::oshandle::OsDirHandle;
+use super::sys_impl::oshandle::OsHandle;
 use super::{fd, path, AsFile};
 use crate::handle::{Handle, HandleRights};
 use crate::wasi::{types, Errno, Result};
 use log::{debug, error};
 use std::any::Any;
-use std::cell::Cell;
 use std::io;
 use std::ops::Deref;
 
-#[derive(Debug)]
-pub(crate) struct OsDir {
-    rights: Cell<HandleRights>,
-    handle: OsDirHandle,
-}
-
-impl OsDir {
-    pub(super) fn new(rights: HandleRights, handle: OsDirHandle) -> Self {
-        let rights = Cell::new(rights);
-        Self { rights, handle }
-    }
-}
+// TODO could this be cleaned up?
+// The actual `OsDir` struct is OS-dependent, therefore we delegate
+// its definition to OS-specific modules.
+pub(crate) use super::sys_impl::osdir::OsDir;
 
 impl Deref for OsDir {
-    type Target = OsDirHandle;
+    type Target = OsHandle;
 
     fn deref(&self) -> &Self::Target {
         &self.handle
@@ -35,8 +26,8 @@ impl Handle for OsDir {
     }
     fn try_clone(&self) -> io::Result<Box<dyn Handle>> {
         let handle = self.handle.try_clone()?;
-        let rights = self.rights.clone();
-        Ok(Box::new(Self { rights, handle }))
+        let new = Self::new(self.rights.get(), handle)?;
+        Ok(Box::new(new))
     }
     fn get_file_type(&self) -> types::Filetype {
         types::Filetype::Directory

crates/wasi-common/src/sys/osfile.rs

@@ -9,6 +9,7 @@ use std::io::{self, Read, Seek, SeekFrom, Write};
 use std::ops::Deref;
 
 pub(crate) trait OsFileExt {
+    /// Create `OsFile` as `dyn Handle` from null device.
     fn from_null() -> io::Result<Box<dyn Handle>>;
 }
 

crates/wasi-common/src/sys/stdio.rs

@@ -8,8 +8,11 @@ use std::cell::Cell;
 use std::io::{self, Read, Write};
 
 pub(crate) trait StdioExt: Sized {
+    /// Create `Stdio` from `io::stdin`.
     fn stdin() -> io::Result<Box<dyn Handle>>;
+    /// Create `Stdio` from `io::stdout`.
     fn stdout() -> io::Result<Box<dyn Handle>>;
+    /// Create `Stdio` from `io::stderr`.
     fn stderr() -> io::Result<Box<dyn Handle>>;
 }
 

crates/wasi-common/src/sys/unix/bsd/mod.rs

@@ -1,4 +1,4 @@
-pub(crate) mod oshandle;
+pub(crate) mod osdir;
 pub(crate) mod path;
 
 pub(crate) const O_RSYNC: yanix::file::OFlag = yanix::file::OFlag::SYNC;

crates/wasi-common/src/sys/unix/bsd/osdir.rs

@@ -0,0 +1,46 @@
+use crate::handle::HandleRights;
+use crate::sys::sys_impl::oshandle::OsHandle;
+use crate::wasi::Result;
+use std::cell::{Cell, RefCell, RefMut};
+use std::io;
+use yanix::dir::Dir;
+
+#[derive(Debug)]
+pub(crate) struct OsDir {
+    pub(crate) rights: Cell<HandleRights>,
+    pub(crate) handle: OsHandle,
+    // When the client makes a `fd_readdir` syscall on this descriptor,
+    // we will need to cache the `libc::DIR` pointer manually in order
+    // to be able to seek on it later. While on Linux, this is handled
+    // by the OS, BSD Unixes require the client to do this caching.
+    //
+    // This comes directly from the BSD man pages on `readdir`:
+    //   > Values returned by telldir() are good only for the lifetime
+    //   > of the DIR pointer, dirp, from which they are derived.
+    //   > If the directory is closed and then reopened, prior values
+    //   > returned by telldir() will no longer be valid.
+    stream_ptr: RefCell<Dir>,
+}
+
+impl OsDir {
+    pub(crate) fn new(rights: HandleRights, handle: OsHandle) -> io::Result<Self> {
+        let rights = Cell::new(rights);
+        // We need to duplicate the handle, because `opendir(3)`:
+        //     Upon successful return from fdopendir(), the file descriptor is under
+        //     control of the system, and if any attempt is made to close the file
+        //     descriptor, or to modify the state of the associated description other
+        //     than by means of closedir(), readdir(), readdir_r(), or rewinddir(),
+        //     the behaviour is undefined.
+        let stream_ptr = Dir::from(handle.try_clone()?)?;
+        let stream_ptr = RefCell::new(stream_ptr);
+        Ok(Self {
+            rights,
+            handle,
+            stream_ptr,
+        })
+    }
+    /// Returns the `Dir` stream pointer associated with this `OsDir`.
+    pub(crate) fn stream_ptr(&self) -> Result<RefMut<Dir>> {
+        Ok(self.stream_ptr.borrow_mut())
+    }
+}

crates/wasi-common/src/sys/unix/bsd/oshandle.rs

@@ -1,5 +1,5 @@
-#[path = "../linux/oshandle.rs"]
-pub(crate) mod oshandle;
+#[path = "../linux/osdir.rs"]
+pub(crate) mod osdir;
 #[path = "../linux/path.rs"]
 pub(crate) mod path;
 

crates/wasi-common/src/sys/unix/emscripten/mod.rs

@@ -54,7 +54,7 @@ pub(crate) fn readdir<'a>(
 
     // Get an instance of `Dir`; this is host-specific due to intricasies
     // of managing a dir stream between Linux and BSD *nixes
-    let mut dir = dirfd.dir_stream()?;
+    let mut dir = dirfd.stream_ptr()?;
 
     // Seek if needed. Unless cookie is wasi::__WASI_DIRCOOKIE_START,
     // new items may not be returned to the caller.

crates/wasi-common/src/sys/unix/fd.rs

@@ -1,4 +1,4 @@
-pub(crate) mod oshandle;
+pub(crate) mod osdir;
 pub(crate) mod path;
 
 pub(crate) const O_RSYNC: yanix::file::OFlag = yanix::file::OFlag::RSYNC;

crates/wasi-common/src/sys/unix/linux/mod.rs

@@ -1,44 +1,35 @@
+use crate::handle::HandleRights;
 use crate::sys::sys_impl::oshandle::OsHandle;
 use crate::wasi::Result;
+use std::cell::Cell;
 use std::io;
-use std::ops::Deref;
 use yanix::dir::Dir;
 
 #[derive(Debug)]
-pub(crate) struct OsDirHandle(OsHandle);
+pub(crate) struct OsDir {
+    pub(crate) rights: Cell<HandleRights>,
+    pub(crate) handle: OsHandle,
+}
 
-impl OsDirHandle {
-    /// Consumes the spcified `OsHandle`.
-    pub(crate) fn new(handle: OsHandle) -> io::Result<Self> {
-        Ok(Self(handle))
-    }
-    /// Tries clone `self`.
-    pub(crate) fn try_clone(&self) -> io::Result<Self> {
-        let handle = self.0.try_clone()?;
-        Self::new(handle)
+impl OsDir {
+    pub(crate) fn new(rights: HandleRights, handle: OsHandle) -> io::Result<Self> {
+        let rights = Cell::new(rights);
+        Ok(Self { rights, handle })
     }
-    /// Gets current dir stream pointer.
-    pub(crate) fn dir_stream(&self) -> Result<Box<Dir>> {
-        // We need to duplicate the fd, because `opendir(3)`:
+    /// Returns the `Dir` stream pointer associated with this `OsDir`.
+    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,
         //     and should not otherwise be used by the application.
         // `opendir(3p)` also says that it's undefined behavior to
         // modify the state of the fd in a different way than by accessing DIR*.
         //
         // Still, rewinddir will be needed because the two file descriptors
         // share progress. But we can safely execute closedir now.
-        let file = self.0.try_clone()?;
+        let file = self.handle.try_clone()?;
         // TODO This doesn't look very clean. Can we do something about it?
         // Boxing is needed here in order to satisfy `yanix`'s trait requirement for the `DirIter`
         // where `T: Deref<Target = Dir>`.
         Ok(Box::new(Dir::from(file)?))
     }
 }
-
-impl Deref for OsDirHandle {
-    type Target = OsHandle;
-
-    fn deref(&self) -> &Self::Target {
-        &self.0
-    }
-}