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

[wasi-common] Clean up the use of mutable Entry (#1395)

Browse Source 
* Clean up the use of mutable Entry

Until now, several syscalls including `fd_pwrite` etc. were relying on
mutating `&mut Entry` by mutating its inner file handle. This is
unnecessary in almost all cases since all methods mutating `std::fs::File`
in Rust's libstd are also implemented for `&std::fs::File`.

While here, I've also modified `OsHandle` in BSD to include `RefCell<Option<Dir>>`
rather than `Option<Mutex<Dir>>` as was until now. While `RefCell`
could easily be replaced with `RefCell`, since going multithreading
will require a lot of (probably even) conceptual changes to `wasi-common`,
I thought it'd be best not to mix single- with multithreading contexts
and swap all places at once when it comes to it.

I've also had to make some modifications to virtual FS which mainly
swapped mutability for interior mutability in places.

* Use one-liners wherever convenient
This commit is contained in:
Jakub Konka
2020-03-25 14:00:52 +01:00
committed by GitHub
parent 336d9d1ab6
commit 1f6890e070
9 changed files with 81 additions and 110 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
crates/wasi-common/src/entry.rs
View File

@@ -5,7 +5,7 @@ use crate::wasi::types::{Filetype, Rights};
use crate::wasi::{Errno, Result};
use std::marker::PhantomData;
use std::mem::ManuallyDrop;
use std::ops::{Deref, DerefMut};
use std::ops::Deref;
use std::path::PathBuf;
use std::{fmt, fs, io};
@@ -63,15 +63,6 @@ impl Descriptor {
}
}
/// Like `as_file`, but return a mutable reference.
pub(crate) fn as_file_mut<'descriptor>(&'descriptor mut self) -> Result<&'descriptor mut Self> {
match self {
Self::OsHandle(_) => Ok(self),
Self::VirtualFile(_) => Ok(self),
_ => Err(Errno::Badf),
}
}
/// Return an `OsHandle`, which may be a stream or socket file descriptor.
pub(crate) fn as_os_handle<'descriptor>(&'descriptor self) -> OsHandleRef<'descriptor> {
descriptor_as_oshandle(self)
@@ -263,9 +254,3 @@ impl<'descriptor> Deref for OsHandleRef<'descriptor> {
&self.handle
}
}
impl<'descriptor> DerefMut for OsHandleRef<'descriptor> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.handle
}
}

58
crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs
View File

@@ -7,8 +7,8 @@ use crate::{clock, fd, path, poll};
use log::{debug, error, trace};
use std::cell::Ref;
use std::convert::TryInto;
use std::fs::File;
use std::io::{self, Read, Seek, SeekFrom, Write};
use std::ops::DerefMut;
use wiggle_runtime::{GuestBorrows, GuestPtr};
impl<'a> WasiSnapshotPreview1 for WasiCtx {
@@ -124,10 +124,10 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
len,
advice
);
let mut entry = self.get_entry_mut(fd)?;
let entry = self.get_entry(fd)?;
let file = entry
.as_descriptor_mut(types::Rights::FD_ADVISE, types::Rights::empty())?
.as_file_mut()?;
.as_descriptor(types::Rights::FD_ADVISE, types::Rights::empty())?
.as_file()?;
match file {
Descriptor::OsHandle(fd) => fd::advise(&fd, advice, offset, len),
Descriptor::VirtualFile(virt) => virt.advise(advice, offset, len),
@@ -361,13 +361,13 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
buf.push(io::IoSliceMut::new(slice));
}
let mut entry = self.get_entry_mut(fd)?;
let entry = self.get_entry(fd)?;
let file = entry
.as_descriptor_mut(
.as_descriptor(
types::Rights::FD_READ | types::Rights::FD_SEEK,
types::Rights::empty(),
)?
.as_file_mut()?;
.as_file()?;
if offset > i64::max_value() as u64 {
return Err(Errno::Io);
@@ -375,6 +375,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
let host_nread = match file {
Descriptor::OsHandle(fd) => {
let mut fd: &File = fd;
let cur_pos = fd.seek(SeekFrom::Current(0))?;
fd.seek(SeekFrom::Start(offset))?;
let nread = fd.read_vectored(&mut buf)?;
@@ -474,13 +475,13 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
buf.push(io::IoSlice::new(slice));
}
let mut entry = self.get_entry_mut(fd)?;
let entry = self.get_entry(fd)?;
let file = entry
.as_descriptor_mut(
.as_descriptor(
types::Rights::FD_WRITE | types::Rights::FD_SEEK,
types::Rights::empty(),
)?
.as_file_mut()?;
.as_file()?;
if offset > i64::max_value() as u64 {
return Err(Errno::Io);
@@ -488,6 +489,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
let host_nwritten = match file {
Descriptor::OsHandle(fd) => {
let mut fd: &File = fd;
let cur_pos = fd.seek(SeekFrom::Current(0))?;
fd.seek(SeekFrom::Start(offset))?;
let nwritten = fd.write_vectored(&buf)?;
@@ -524,10 +526,10 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
slices.push(io::IoSliceMut::new(slice));
}
let mut entry = self.get_entry_mut(fd)?;
let entry = self.get_entry(fd)?;
let host_nread =
match entry.as_descriptor_mut(types::Rights::FD_READ, types::Rights::empty())? {
Descriptor::OsHandle(file) => file.read_vectored(&mut slices)?,
match entry.as_descriptor(types::Rights::FD_READ, types::Rights::empty())? {
Descriptor::OsHandle(file) => (file as &File).read_vectored(&mut slices)?,
Descriptor::VirtualFile(virt) => virt.read_vectored(&mut slices)?,
Descriptor::Stdin => io::stdin().read_vectored(&mut slices)?,
_ => return Err(Errno::Badf),
@@ -554,10 +556,10 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
cookie,
);
let mut entry = self.get_entry_mut(fd)?;
let entry = self.get_entry(fd)?;
let file = entry
.as_descriptor_mut(types::Rights::FD_READDIR, types::Rights::empty())?
.as_file_mut()?;
.as_descriptor(types::Rights::FD_READDIR, types::Rights::empty())?
.as_file()?;
fn copy_entities<T: Iterator<Item = Result<(types::Dirent, String)>>>(
iter: T,
@@ -643,17 +645,17 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
} else {
types::Rights::FD_SEEK | types::Rights::FD_TELL
};
let mut entry = self.get_entry_mut(fd)?;
let entry = self.get_entry(fd)?;
let file = entry
.as_descriptor_mut(rights, types::Rights::empty())?
.as_file_mut()?;
.as_descriptor(rights, types::Rights::empty())?
.as_file()?;
let pos = match whence {
types::Whence::Cur => SeekFrom::Current(offset),
types::Whence::End => SeekFrom::End(offset),
types::Whence::Set => SeekFrom::Start(offset as u64),
};
let host_newoffset = match file {
Descriptor::OsHandle(fd) => fd.seek(pos)?,
Descriptor::OsHandle(fd) => (fd as &File).seek(pos)?,
Descriptor::VirtualFile(virt) => virt.seek(pos)?,
_ => {
unreachable!(
@@ -689,12 +691,12 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
fn fd_tell(&self, fd: types::Fd) -> Result<types::Filesize> {
trace!("fd_tell(fd={:?})", fd);
let mut entry = self.get_entry_mut(fd)?;
let entry = self.get_entry(fd)?;
let file = entry
.as_descriptor_mut(types::Rights::FD_TELL, types::Rights::empty())?
.as_file_mut()?;
.as_descriptor(types::Rights::FD_TELL, types::Rights::empty())?
.as_file()?;
let host_offset = match file {
Descriptor::OsHandle(fd) => fd.seek(SeekFrom::Current(0))?,
Descriptor::OsHandle(fd) => (fd as &File).seek(SeekFrom::Current(0))?,
Descriptor::VirtualFile(virt) => virt.seek(SeekFrom::Current(0))?,
_ => {
unreachable!(
@@ -726,15 +728,15 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
}
// perform unbuffered writes
let mut entry = self.get_entry_mut(fd)?;
let entry = self.get_entry(fd)?;
let isatty = entry.isatty();
let desc = entry.as_descriptor_mut(types::Rights::FD_WRITE, types::Rights::empty())?;
let desc = entry.as_descriptor(types::Rights::FD_WRITE, types::Rights::empty())?;
let host_nwritten = match desc {
Descriptor::OsHandle(file) => {
if isatty {
SandboxedTTYWriter::new(file.deref_mut()).write_vectored(&slices)?
SandboxedTTYWriter::new(&mut (file as &File)).write_vectored(&slices)?
} else {
file.write_vectored(&slices)?
(file as &File).write_vectored(&slices)?
}
}
Descriptor::VirtualFile(virt) => {

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

@@ -1,26 +1,21 @@
use crate::sys::entry::OsHandle;
use crate::wasi::Result;
use std::sync::{Mutex, MutexGuard};
use std::cell::RefMut;
use yanix::dir::Dir;
pub(crate) fn get_dir_from_os_handle<'a>(
os_handle: &'a mut OsHandle,
) -> Result<MutexGuard<'a, Dir>> {
let dir = match os_handle.dir {
Some(ref mut dir) => dir,
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 dir = Dir::from(fd)?;
os_handle.dir.get_or_insert(Mutex::new(dir))
}
};
// Note that from this point on, until the end of the parent scope (i.e., enclosing this
// function), we're locking the `Dir` member of this `OsHandle`.
Ok(dir.lock().unwrap())
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()
}))
}

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

@@ -1,7 +1,7 @@
use std::cell::RefCell;
use std::fs;
use std::ops::{Deref, DerefMut};
use std::ops::Deref;
use std::os::unix::prelude::{AsRawFd, RawFd};
use std::sync::Mutex;
use yanix::dir::Dir;
#[derive(Debug)]
@@ -18,12 +18,15 @@ 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: Option<Mutex<Dir>>,
pub(crate) dir: RefCell<Option<Dir>>,
}
impl From<fs::File> for OsHandle {
fn from(file: fs::File) -> Self {
Self { file, dir: None }
Self {
file,
dir: RefCell::new(None),
}
}
}
@@ -40,9 +43,3 @@ impl Deref for OsHandle {
&self.file
}
}
impl DerefMut for OsHandle {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.file
}
}

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

@@ -44,7 +44,7 @@ pub(crate) fn filestat_get(file: &std::fs::File) -> Result<types::Filestat> {
}
pub(crate) fn readdir<'a>(
os_handle: &'a mut OsHandle,
os_handle: &'a OsHandle,
cookie: types::Dircookie,
) -> Result<impl Iterator<Item = Result<(types::Dirent, String)>> + 'a> {
use yanix::dir::{DirIter, Entry, EntryExt, SeekLoc};

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

@@ -2,7 +2,7 @@ use crate::sys::entry::OsHandle;
use crate::wasi::Result;
use yanix::dir::Dir;
pub(crate) fn get_dir_from_os_handle(os_handle: &mut OsHandle) -> Result<Box<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.

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

@@ -1,5 +1,5 @@
use std::fs;
use std::ops::{Deref, DerefMut};
use std::ops::Deref;
use std::os::unix::prelude::{AsRawFd, RawFd};
#[derive(Debug)]
@@ -24,9 +24,3 @@ impl Deref for OsHandle {
&self.0
}
}
impl DerefMut for OsHandle {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}

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

@@ -3,7 +3,7 @@ use crate::wasi::{types, RightsExt};
use std::fs::File;
use std::io;
use std::mem::ManuallyDrop;
use std::ops::{Deref, DerefMut};
use std::ops::Deref;
use std::os::windows::prelude::{AsRawHandle, FromRawHandle, RawHandle};
#[derive(Debug)]
@@ -29,12 +29,6 @@ impl Deref for OsHandle {
}
}
impl DerefMut for OsHandle {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
impl AsRawHandle for Descriptor {
fn as_raw_handle(&self) -> RawHandle {
match self {

42
crates/wasi-common/src/virtfs.rs
View File

@@ -86,7 +86,7 @@ pub(crate) trait VirtualFile: MovableFile {
Err(Errno::Badf)
}
fn write_vectored(&mut self, _iovs: &[io::IoSlice]) -> Result<usize> {
fn write_vectored(&self, _iovs: &[io::IoSlice]) -> Result<usize> {
Err(Errno::Badf)
}
@@ -98,7 +98,7 @@ pub(crate) trait VirtualFile: MovableFile {
Err(Errno::Badf)
}
fn seek(&mut self, _offset: SeekFrom) -> Result<u64> {
fn seek(&self, _offset: SeekFrom) -> Result<u64> {
Err(Errno::Badf)
}
@@ -131,7 +131,7 @@ pub(crate) trait VirtualFile: MovableFile {
Err(Errno::Badf)
}
fn read_vectored(&mut self, _iovs: &mut [io::IoSliceMut]) -> Result<usize> {
fn read_vectored(&self, _iovs: &mut [io::IoSliceMut]) -> Result<usize> {
Err(Errno::Badf)
}
@@ -254,7 +254,7 @@ impl VecFileContents {
/// a filesystem wherein a file descriptor is one view into a possibly-shared underlying collection
/// of data and permissions on a filesystem.
pub struct InMemoryFile {
cursor: types::Filesize,
cursor: RefCell<types::Filesize>,
parent: Rc<RefCell<Option<Box<dyn VirtualFile>>>>,
fd_flags: types::Fdflags,
data: Rc<RefCell<Box<dyn FileContents>>>,
@@ -263,7 +263,7 @@ pub struct InMemoryFile {
impl InMemoryFile {
pub fn memory_backed() -> Self {
Self {
cursor: 0,
cursor: RefCell::new(0),
parent: Rc::new(RefCell::new(None)),
fd_flags: types::Fdflags::empty(),
data: Rc::new(RefCell::new(Box::new(VecFileContents::new()))),
@@ -272,7 +272,7 @@ impl InMemoryFile {
pub fn new(contents: Box<dyn FileContents>) -> Self {
Self {
cursor: 0,
cursor: RefCell::new(0),
fd_flags: types::Fdflags::empty(),
parent: Rc::new(RefCell::new(None)),
data: Rc::new(RefCell::new(contents)),
@@ -293,7 +293,7 @@ impl VirtualFile for InMemoryFile {
fn try_clone(&self) -> io::Result<Box<dyn VirtualFile>> {
Ok(Box::new(Self {
cursor: 0,
cursor: RefCell::new(0),
fd_flags: self.fd_flags,
parent: Rc::clone(&self.parent),
data: Rc::clone(&self.data),
@@ -356,7 +356,7 @@ impl VirtualFile for InMemoryFile {
Ok(())
}
fn write_vectored(&mut self, iovs: &[io::IoSlice]) -> Result<usize> {
fn write_vectored(&self, iovs: &[io::IoSlice]) -> Result<usize> {
trace!("write_vectored(iovs={:?})", iovs);
let mut data = self.data.borrow_mut();
@@ -367,7 +367,7 @@ impl VirtualFile for InMemoryFile {
let write_start = if append_mode {
data.size()
} else {
self.cursor
*self.cursor.borrow()
};
let max_size = iovs
@@ -396,16 +396,16 @@ impl VirtualFile for InMemoryFile {
// If we are not appending, adjust the cursor appropriately for the write, too. This can't
// overflow, as we checked against that before writing any data.
if !append_mode {
self.cursor += written as u64;
*self.cursor.borrow_mut() += written as u64;
}
Ok(written)
}
fn read_vectored(&mut self, iovs: &mut [io::IoSliceMut]) -> Result<usize> {
fn read_vectored(&self, iovs: &mut [io::IoSliceMut]) -> Result<usize> {
trace!("read_vectored(iovs={:?})", iovs);
trace!(" | *read_start={:?}", self.cursor);
self.data.borrow_mut().preadv(iovs, self.cursor)
self.data.borrow_mut().preadv(iovs, *self.cursor.borrow())
}
fn preadv(&self, buf: &mut [io::IoSliceMut], offset: types::Filesize) -> Result<usize> {
@@ -416,32 +416,36 @@ impl VirtualFile for InMemoryFile {
self.data.borrow_mut().pwritev(buf, offset)
}
fn seek(&mut self, offset: SeekFrom) -> Result<types::Filesize> {
fn seek(&self, offset: SeekFrom) -> Result<types::Filesize> {
let content_len = self.data.borrow().size();
match offset {
SeekFrom::Current(offset) => {
let new_cursor = if offset < 0 {
self.cursor
.borrow()
.checked_sub(offset.wrapping_neg() as u64)
.ok_or(Errno::Inval)?
} else {
self.cursor.checked_add(offset as u64).ok_or(Errno::Inval)?
self.cursor
.borrow()
.checked_add(offset as u64)
.ok_or(Errno::Inval)?
};
self.cursor = std::cmp::min(content_len, new_cursor);
*self.cursor.borrow_mut() = std::cmp::min(content_len, new_cursor);
}
SeekFrom::End(offset) => {
// A negative offset from the end would be past the end of the file,
let offset: u64 = offset.try_into().map_err(|_| Errno::Inval)?;
self.cursor = content_len.saturating_sub(offset);
*self.cursor.borrow_mut() = content_len.saturating_sub(offset);
}
SeekFrom::Start(offset) => {
// A negative offset from the end would be before the start of the file.
let offset: u64 = offset.try_into().map_err(|_| Errno::Inval)?;
self.cursor = std::cmp::min(content_len, offset);
*self.cursor.borrow_mut() = std::cmp::min(content_len, offset);
}
}
Ok(self.cursor)
Ok(*self.cursor.borrow())
}
fn advise(
@@ -749,7 +753,7 @@ impl VirtualFile for VirtualDir {
}
}
fn write_vectored(&mut self, _iovs: &[io::IoSlice]) -> Result<usize> {
fn write_vectored(&self, _iovs: &[io::IoSlice]) -> Result<usize> {
Err(Errno::Badf)
}