T0b1/wasmtime
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Refactor use of Refs and RefMuts in wasi-common (#1412)

Browse Source 
* Refactor use of Refs and RefMuts in wasi-common

This commit refactors the use of `Ref`s and `RefMut`s in `wasi-common`.
Now, `Entry` is stored behind an `Rc` inside the `EntryTable`. The `Entry`
itself on the other hand now stores rights behind a `RefCell` and the
descriptor as `Rc<RefCell<..>>` combo to enable easy reference tracking
and interior mutability which is required down the line in a couple of
syscalls. In essence, this implies that we no longer have need for
mutable accessor to `Entry` from `WasiCtx`, and so all related methods
go away (`get_entry_mut`, etc.).

While here, I've also simplified handling and aggregating of rights on
the `Entry` object. Instead of storing base and inheriting rights as
separate fields, they are now aggregated into one struct `EntryRights`
which features convenient constructors for each possible combination; i.e.,
when only base rights are set, or both base and inheriting are set, or
both are left as empty. Since we do need to be able to mutate those
rights down the line in `fdstat_set_rights` syscall, this object
is kept behind a `RefCell` (note no `Rc` since we don't need to pass it
around anywhere).

The descriptor field in `Entry` is now kept behind `Rc<RefCell<..>>` combo
since we not only need to mutate it down the line, but we also need to
be able to pass it around (as part of the machinery making `poll_oneoff`
work).

I've also removed `as_file` and `try_clone` methods on `Descriptor` struct
since they were adding more noise than necessary, and making them work
with `Rc` was unnecessarily complicated.

Finally, I've converted the `get_dir_from_os_handle` function into a
method attached to the `OsHandle` itself, called `dir_stream`. IMHO,
it makes more sense to have it there directly as a method than as a separate
function.

* Use Cell for types that are Copy
This commit is contained in:
Jakub Konka
2020-03-27 09:34:52 +01:00
committed by GitHub
parent 092538cc54
commit 5c51940100
20 changed files with 446 additions and 550 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
crates/wasi-common/src/sys/unix/bsd/fd.rs
View File

@@ -1,21 +0,0 @@
use crate::sys::entry::OsHandle;
use crate::wasi::Result;
use std::cell::RefMut;
use yanix::dir::Dir;
pub(crate) fn get_dir_from_os_handle(os_handle: &OsHandle) -> Result<RefMut<Dir>> {
if os_handle.dir.borrow().is_none() {
// We need to duplicate the fd, 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 fd = (*os_handle).try_clone()?;
let d = Dir::from(fd)?;
*os_handle.dir.borrow_mut() = Some(d);
}
Ok(RefMut::map(os_handle.dir.borrow_mut(), |dir| {
dir.as_mut().unwrap()
}))
}

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

@@ -1,4 +1,3 @@
pub(crate) mod fd;
pub(crate) mod oshandle;
pub(crate) mod path;

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

@@ -1,4 +1,5 @@
use std::cell::RefCell;
use crate::wasi::Result;
use std::cell::{RefCell, RefMut};
use std::fs;
use std::ops::Deref;
use std::os::unix::prelude::{AsRawFd, RawFd};
@@ -6,7 +7,7 @@ use yanix::dir::Dir;
#[derive(Debug)]
pub(crate) struct OsHandle {
pub(crate) file: fs::File,
file: fs::File,
// In case that this `OsHandle` actually refers to a directory,
// when the client makes a `fd_readdir` syscall on this descriptor,
// we will need to cache the `libc::DIR` pointer manually in order
@@ -18,7 +19,26 @@ pub(crate) struct OsHandle {
// > 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.
pub(crate) dir: RefCell<Option<Dir>>,
dir: RefCell<Option<Dir>>,
}
impl OsHandle {
pub(crate) fn dir_stream(&self) -> Result<RefMut<Dir>> {
if self.dir.borrow().is_none() {
// We need to duplicate the fd, 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 fd = self.file.try_clone()?;
let d = Dir::from(fd)?;
*self.dir.borrow_mut() = Some(d);
}
Ok(RefMut::map(self.dir.borrow_mut(), |dir| {
dir.as_mut().unwrap()
}))
}
}
impl From<fs::File> for OsHandle {

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

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

23
crates/wasi-common/src/sys/unix/entry.rs
View File

@@ -1,4 +1,4 @@
use crate::entry::{Descriptor, OsHandleRef};
use crate::entry::{Descriptor, EntryRights, OsHandleRef};
use crate::wasi::{types, RightsExt};
use std::fs::File;
use std::io;
@@ -33,19 +33,19 @@ pub(crate) fn descriptor_as_oshandle<'lifetime>(
/// This function is unsafe because it operates on a raw file descriptor.
pub(crate) unsafe fn determine_type_and_access_rights<Fd: AsRawFd>(
fd: &Fd,
) -> io::Result<(types::Filetype, types::Rights, types::Rights)> {
let (file_type, mut rights_base, rights_inheriting) = determine_type_rights(fd)?;
) -> io::Result<(types::Filetype, EntryRights)> {
let (file_type, mut rights) = determine_type_rights(fd)?;
use yanix::{fcntl, file::OFlag};
let flags = fcntl::get_status_flags(fd.as_raw_fd())?;
let accmode = flags & OFlag::ACCMODE;
if accmode == OFlag::RDONLY {
rights_base &= !types::Rights::FD_WRITE;
rights.base &= !types::Rights::FD_WRITE;
} else if accmode == OFlag::WRONLY {
rights_base &= !types::Rights::FD_READ;
rights.base &= !types::Rights::FD_READ;
}
Ok((file_type, rights_base, rights_inheriting))
Ok((file_type, rights))
}
/// Returns the set of all possible rights that are relevant for file type.
@@ -53,12 +53,12 @@ pub(crate) unsafe fn determine_type_and_access_rights<Fd: AsRawFd>(
/// This function is unsafe because it operates on a raw file descriptor.
pub(crate) unsafe fn determine_type_rights<Fd: AsRawFd>(
fd: &Fd,
) -> io::Result<(types::Filetype, types::Rights, types::Rights)> {
let (file_type, rights_base, rights_inheriting) = {
) -> io::Result<(types::Filetype, EntryRights)> {
let (file_type, rights) = {
// we just make a `File` here for convenience; we don't want it to close when it drops
let file = std::mem::ManuallyDrop::new(std::fs::File::from_raw_fd(fd.as_raw_fd()));
let ft = file.metadata()?.file_type();
if ft.is_block_device() {
let (filetype, base, inheriting) = if ft.is_block_device() {
log::debug!("Host fd {:?} is a block device", fd.as_raw_fd());
(
types::Filetype::BlockDevice,
@@ -121,8 +121,9 @@ pub(crate) unsafe fn determine_type_rights<Fd: AsRawFd>(
} else {
log::debug!("Host fd {:?} is unknown", fd.as_raw_fd());
return Err(io::Error::from_raw_os_error(libc::EINVAL));
}
};
(filetype, EntryRights::new(base, inheriting))
};
Ok((file_type, rights_base, rights_inheriting))
Ok((file_type, rights))
}

7
crates/wasi-common/src/sys/unix/fd.rs
View File

@@ -1,4 +1,3 @@
use super::sys_impl::fd::get_dir_from_os_handle;
use crate::sys::entry::OsHandle;
use crate::wasi::{self, types, Result};
use std::convert::TryInto;
@@ -12,7 +11,9 @@ pub(crate) fn fdstat_get(fd: &File) -> Result<types::Fdflags> {
pub(crate) fn fdstat_set_flags(fd: &File, fdflags: types::Fdflags) -> Result<Option<OsHandle>> {
unsafe { yanix::fcntl::set_status_flags(fd.as_raw_fd(), fdflags.into())? };
// TODO why are we returning Ok(None) here?
// We return None here to signal that the operation succeeded on the original
// file descriptor and mutating the original WASI Descriptor is thus unnecessary.
// This is needed as on Windows this operation required reopening a file.
Ok(None)
}
@@ -51,7 +52,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 = get_dir_from_os_handle(os_handle)?;
let mut dir = os_handle.dir_stream()?;
// Seek if needed. Unless cookie is wasi::__WASI_DIRCOOKIE_START,
// new items may not be returned to the caller.

19
crates/wasi-common/src/sys/unix/linux/fd.rs
View File

@@ -1,19 +0,0 @@
use crate::sys::entry::OsHandle;
use crate::wasi::Result;
use yanix::dir::Dir;
pub(crate) fn get_dir_from_os_handle(os_handle: &OsHandle) -> Result<Box<Dir>> {
// We need to duplicate the fd, 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 fd = os_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(fd)?))
}

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

@@ -1,4 +1,3 @@
pub(crate) mod fd;
pub(crate) mod oshandle;
pub(crate) mod path;

20
crates/wasi-common/src/sys/unix/linux/oshandle.rs
View File

@@ -1,10 +1,30 @@
use crate::wasi::Result;
use std::fs;
use std::ops::Deref;
use std::os::unix::prelude::{AsRawFd, RawFd};
use yanix::dir::Dir;
#[derive(Debug)]
pub(crate) struct OsHandle(fs::File);
impl OsHandle {
pub(crate) fn dir_stream(&self) -> Result<Box<Dir>> {
// We need to duplicate the fd, 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 fd = self.0.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(fd)?))
}
}
impl From<fs::File> for OsHandle {
fn from(file: fs::File) -> Self {
Self(file)

11
crates/wasi-common/src/sys/unix/path.rs
View File

@@ -1,4 +1,4 @@
use crate::entry::Descriptor;
use crate::entry::{Descriptor, EntryRights};
use crate::path::PathGet;
use crate::sys::entry::OsHandle;
use crate::sys::unix::sys_impl;
@@ -22,14 +22,13 @@ pub(crate) fn from_host<S: AsRef<OsStr>>(s: S) -> Result<String> {
}
pub(crate) fn open_rights(
rights_base: types::Rights,
rights_inheriting: types::Rights,
input_rights: &EntryRights,
oflags: types::Oflags,
fs_flags: types::Fdflags,
) -> (types::Rights, types::Rights) {
) -> EntryRights {
// which rights are needed on the dirfd?
let mut needed_base = types::Rights::PATH_OPEN;
let mut needed_inheriting = rights_base | rights_inheriting;
let mut needed_inheriting = input_rights.base | input_rights.inheriting;
// convert open flags
let oflags: OFlag = oflags.into();
@@ -49,7 +48,7 @@ pub(crate) fn open_rights(
needed_inheriting |= types::Rights::FD_SYNC;
}
(needed_base, needed_inheriting)
EntryRights::new(needed_base, needed_inheriting)
}
pub(crate) fn openat(dirfd: &File, path: &str) -> Result<File> {

8
crates/wasi-common/src/sys/unix/poll.rs
View File

@@ -26,7 +26,7 @@ pub(crate) fn oneoff(
// events we filtered before. If we get something else here, the code has a serious bug.
_ => unreachable!(),
};
unsafe { PollFd::new(event.descriptor.as_raw_fd(), flags) }
unsafe { PollFd::new(event.descriptor.borrow().as_raw_fd(), flags) }
})
.collect();
@@ -69,8 +69,8 @@ fn handle_timeout_event(timeout: ClockEventData, events: &mut Vec<types::Event>)
});
}
fn handle_fd_event<'a>(
ready_events: impl Iterator<Item = (FdEventData<'a>, yanix::poll::PollFd)>,
fn handle_fd_event(
ready_events: impl Iterator<Item = (FdEventData, yanix::poll::PollFd)>,
events: &mut Vec<types::Event>,
) -> Result<()> {
use crate::entry::Descriptor;
@@ -103,7 +103,7 @@ fn handle_fd_event<'a>(
log::debug!("poll_oneoff_handle_fd_event revents = {:?}", revents);
let nbytes = if fd_event.r#type == types::Eventtype::FdRead {
query_nbytes(&fd_event.descriptor)?
query_nbytes(&fd_event.descriptor.borrow())?
} else {
0
};

23
crates/wasi-common/src/sys/windows/entry.rs
View File

@@ -1,4 +1,4 @@
use crate::entry::{Descriptor, OsHandleRef};
use crate::entry::{Descriptor, EntryRights, OsHandleRef};
use crate::wasi::{types, RightsExt};
use std::fs::File;
use std::io;
@@ -57,19 +57,19 @@ pub(crate) fn descriptor_as_oshandle<'lifetime>(
/// This function is unsafe because it operates on a raw file descriptor.
pub(crate) unsafe fn determine_type_and_access_rights<Handle: AsRawHandle>(
handle: &Handle,
) -> io::Result<(types::Filetype, types::Rights, types::Rights)> {
) -> io::Result<(types::Filetype, EntryRights)> {
use winx::file::{query_access_information, AccessMode};
let (file_type, mut rights_base, rights_inheriting) = determine_type_rights(handle)?;
let (file_type, mut rights) = determine_type_rights(handle)?;
match file_type {
types::Filetype::Directory | types::Filetype::RegularFile => {
let mode = query_access_information(handle.as_raw_handle())?;
if mode.contains(AccessMode::FILE_GENERIC_READ) {
rights_base |= types::Rights::FD_READ;
rights.base |= types::Rights::FD_READ;
}
if mode.contains(AccessMode::FILE_GENERIC_WRITE) {
rights_base |= types::Rights::FD_WRITE;
rights.base |= types::Rights::FD_WRITE;
}
}
_ => {
@@ -78,7 +78,7 @@ pub(crate) unsafe fn determine_type_and_access_rights<Handle: AsRawHandle>(
}
}
Ok((file_type, rights_base, rights_inheriting))
Ok((file_type, rights))
}
/// Returns the set of all possible rights that are relevant for file type.
@@ -86,10 +86,10 @@ pub(crate) unsafe fn determine_type_and_access_rights<Handle: AsRawHandle>(
/// This function is unsafe because it operates on a raw file descriptor.
pub(crate) unsafe fn determine_type_rights<Handle: AsRawHandle>(
handle: &Handle,
) -> io::Result<(types::Filetype, types::Rights, types::Rights)> {
let (file_type, rights_base, rights_inheriting) = {
) -> io::Result<(types::Filetype, EntryRights)> {
let (file_type, rights) = {
let file_type = winx::file::get_file_type(handle.as_raw_handle())?;
if file_type.is_char() {
let (file_type, base, inheriting) = if file_type.is_char() {
// character file: LPT device or console
// TODO: rule out LPT device
(
@@ -126,7 +126,8 @@ pub(crate) unsafe fn determine_type_rights<Handle: AsRawHandle>(
)
} else {
return Err(io::Error::from_raw_os_error(libc::EINVAL));
}
};
(file_type, EntryRights::new(base, inheriting))
};
Ok((file_type, rights_base, rights_inheriting))
Ok((file_type, rights))
}

11
crates/wasi-common/src/sys/windows/path.rs
View File

@@ -1,4 +1,4 @@
use crate::entry::Descriptor;
use crate::entry::{Descriptor, EntryRights};
use crate::fd;
use crate::path::PathGet;
use crate::sys::entry::OsHandle;
@@ -41,14 +41,13 @@ impl PathGetExt for PathGet {
}
pub(crate) fn open_rights(
rights_base: types::Rights,
rights_inheriting: types::Rights,
input_rights: &EntryRights,
oflags: types::Oflags,
fdflags: types::Fdflags,
) -> (types::Rights, types::Rights) {
) -> EntryRights {
// which rights are needed on the dirfd?
let mut needed_base = types::Rights::PATH_OPEN;
let mut needed_inheriting = rights_base | rights_inheriting;
let mut needed_inheriting = input_rights.base | input_rights.inheriting;
// convert open flags
if oflags.contains(&types::Oflags::CREAT) {
@@ -66,7 +65,7 @@ pub(crate) fn open_rights(
needed_inheriting |= types::Rights::FD_SYNC;
}
(needed_base, needed_inheriting)
EntryRights::new(needed_base, needed_inheriting)
}
pub(crate) fn openat(dirfd: &File, path: &str) -> Result<File> {

10
crates/wasi-common/src/sys/windows/poll.rs
View File

@@ -5,6 +5,7 @@ use lazy_static::lazy_static;
use log::{debug, error, trace, warn};
use std::convert::TryInto;
use std::os::windows::io::AsRawHandle;
use std::rc::Rc;
use std::sync::mpsc::{self, Receiver, RecvTimeoutError, Sender, TryRecvError};
use std::sync::Mutex;
use std::thread;
@@ -140,8 +141,7 @@ fn handle_timeout_event(timeout_event: ClockEventData, events: &mut Vec<types::E
}
fn handle_rw_event(event: FdEventData, out_events: &mut Vec<types::Event>) {
let descriptor: &Descriptor = &event.descriptor;
let size = match descriptor {
let size = match &*event.descriptor.borrow() {
Descriptor::OsHandle(os_handle) => {
if event.r#type == types::Eventtype::FdRead {
os_handle.metadata().map(|m| m.len()).map_err(Into::into)
@@ -201,8 +201,8 @@ pub(crate) fn oneoff(
let mut pipe_events = vec![];
for event in fd_events {
let descriptor: &Descriptor = &event.descriptor;
match descriptor {
let descriptor = Rc::clone(&event.descriptor);
match &*descriptor.borrow() {
Descriptor::Stdin if event.r#type == types::Eventtype::FdRead => {
stdin_events.push(event)
}
@@ -230,7 +230,7 @@ pub(crate) fn oneoff(
Descriptor::VirtualFile(_) => {
panic!("virtual files do not get rw events");
}
}
};
}
let immediate = !immediate_events.is_empty();