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

[wasi-common] add custom FdPool container for managing fd allocs/deallocs (#1329)

Browse Source 
* Rename FdEntry to Entry

* Add custom FdSet container for managing fd allocs/deallocs

This commit adds a custom `FdSet` container which is intended
for use in `wasi-common` to track WASI fd allocs/deallocs. The
main aim for this container is to abstract away the current
approach of spawning new handles

```rust
fd = fd.checked_add(1).ok_or(...)?;
```

and to make it possible to reuse unused/reclaimed handles
which currently is not done.

The struct offers 3 methods to manage its functionality:
* `FdSet::new` initialises the internal data structures,
  and most notably, it preallocates an `FdSet::BATCH_SIZE`
  worth of handles in such a way that we always start popping
  from the "smallest" handle (think of it as of reversed stack,
  I guess; it's not a binary heap since we don't really care
  whether internally the handles are sorted in some way, just that
  the "largets" handle is at the bottom. Why will become clear
  when describing `allocate` method.)
* `FdSet::allocate` pops the next available handle if one is available.
  The tricky bit here is that, if we run out of handles, we preallocate
  the next `FdSet::BATCH_SIZE` worth of handles starting from the
  latest popped handle (i.e., the "largest" handle). This
  works only because we make sure to only ever pop and push already
  existing handles from the back, and push _new_ handles (from the
  preallocation step) from the front. When we ultimately run out
  of _all_ available handles, we then return `None` for the client
  to handle in some way (e.g., throwing an error such as `WasiError::EMFILE`
  or whatnot).
* `FdSet::deallocate` returns the already allocated handle back to
  the pool for further reuse.

When figuring out the internals, I've tried to optimise for both
alloc and dealloc performance, and I believe we've got an amortised
`O(1)~*` performance for both (if my maths is right, and it may very
well not be, so please verify!).

In order to keep `FdSet` fairly generic, I've made sure not to hard-code
it for the current type system generated by `wig` (i.e., `wasi::__wasi_fd_t`
representing WASI handle), but rather, any type which wants to be managed
by `FdSet` needs to conform to `Fd` trait. This trait is quite simple as
it only requires a couple of rudimentary traits (although `std:#️⃣:Hash`
is quite a powerful assumption here!), and a custom method

```rust
Fd::next(&self) -> Option<Self>;
```

which is there to encapsulate creating another handle from the given one.
In the current state of the code, that'd be simply `u32::checked_add(1)`.
When `wiggle` makes it way into the `wasi-common`, I'd imagine it being
similar to

```rust
fn next(&self) -> Option<Self> {
    self.0.checked_add(1).map(Self::from)
}
```

Anyhow, I'd be happy to learn your thoughts about this design!

* Fix compilation on other targets

* Rename FdSet to FdPool

* Fix FdPool unit tests

* Skip preallocation step in FdPool

* Replace 'replace' calls with direct assignment

* Reuse FdPool from snapshot1 in snapshot0

* Refactor FdPool::allocate

* Remove entry before deallocating the fd

* Refactor the design to accommodate `u32` as underlying type

This commit refactors the design by ensuring that the underlying
type in `FdPool` which we use to track and represent raw file
descriptors is `u32`. As a result, the structure of `FdPool` is
simplified massively as we no longer need to track the claimed
descriptors; in a way, we trust the caller to return the handle
after it's done with it. In case the caller decides to be clever
and return a handle which was not yet legally allocated, we panic.
This should never be a problem in `wasi-common` unless we hit a
bug.

To make all of this work, `Fd` trait is modified to require two
methods: `as_raw(&self) -> u32` and `from_raw(raw_fd: u32) -> Self`
both of which are used to convert to and from the `FdPool`'s underlying
type `u32`.
This commit is contained in:
Jakub Konka
2020-03-17 22:58:49 +01:00
committed by GitHub
parent ba0dc40b2b
commit 7228a248c1
33 changed files with 419 additions and 319 deletions
Download Patch File Download Diff File Expand all files Collapse all files

84
crates/wasi-common/src/hostcalls_impl/fs.rs
View File

@@ -1,7 +1,7 @@
#![allow(non_camel_case_types)]
use super::fs_helpers::path_get;
use crate::ctx::WasiCtx;
use crate::fdentry::{Descriptor, FdEntry};
use crate::entry::{Descriptor, Entry};
use crate::helpers::*;
use crate::host::Dirent;
use crate::memory::*;
@@ -24,14 +24,14 @@ pub(crate) unsafe fn fd_close(
) -> WasiResult<()> {
trace!("fd_close(fd={:?})", fd);
if let Ok(fe) = wasi_ctx.get_fd_entry(fd) {
if let Ok(fe) = wasi_ctx.get_entry(fd) {
// can't close preopened files
if fe.preopen_path.is_some() {
return Err(WasiError::ENOTSUP);
}
}
wasi_ctx.remove_fd_entry(fd)?;
wasi_ctx.remove_entry(fd)?;
Ok(())
}
@@ -43,7 +43,7 @@ pub(crate) unsafe fn fd_datasync(
trace!("fd_datasync(fd={:?})", fd);
let file = wasi_ctx
.get_fd_entry(fd)?
.get_entry(fd)?
.as_descriptor(wasi::__WASI_RIGHTS_FD_DATASYNC, 0)?;
match file {
@@ -72,7 +72,7 @@ pub(crate) unsafe fn fd_pread(
);
let file = wasi_ctx
.get_fd_entry(fd)?
.get_entry(fd)?
.as_descriptor(wasi::__WASI_RIGHTS_FD_READ | wasi::__WASI_RIGHTS_FD_SEEK, 0)?
.as_file()?;
@@ -140,7 +140,7 @@ pub(crate) unsafe fn fd_pwrite(
);
let file = wasi_ctx
.get_fd_entry(fd)?
.get_entry(fd)?
.as_descriptor(
wasi::__WASI_RIGHTS_FD_WRITE | wasi::__WASI_RIGHTS_FD_SEEK,
0,
@@ -207,7 +207,7 @@ pub(crate) unsafe fn fd_read(
.collect();
let maybe_host_nread = match wasi_ctx
.get_fd_entry_mut(fd)?
.get_entry_mut(fd)?
.as_descriptor_mut(wasi::__WASI_RIGHTS_FD_READ, 0)?
{
Descriptor::OsHandle(file) => file.read_vectored(&mut iovs).map_err(Into::into),
@@ -231,25 +231,25 @@ pub(crate) unsafe fn fd_renumber(
) -> WasiResult<()> {
trace!("fd_renumber(from={:?}, to={:?})", from, to);
if !wasi_ctx.contains_fd_entry(from) {
if !wasi_ctx.contains_entry(from) {
return Err(WasiError::EBADF);
}
// Don't allow renumbering over a pre-opened resource.
// TODO: Eventually, we do want to permit this, once libpreopen in
// userspace is capable of removing entries from its tables as well.
let from_fe = wasi_ctx.get_fd_entry(from)?;
let from_fe = wasi_ctx.get_entry(from)?;
if from_fe.preopen_path.is_some() {
return Err(WasiError::ENOTSUP);
}
if let Ok(to_fe) = wasi_ctx.get_fd_entry(to) {
if let Ok(to_fe) = wasi_ctx.get_entry(to) {
if to_fe.preopen_path.is_some() {
return Err(WasiError::ENOTSUP);
}
}
let fe = wasi_ctx.remove_fd_entry(from)?;
wasi_ctx.insert_fd_entry_at(to, fe);
let fe = wasi_ctx.remove_entry(from)?;
wasi_ctx.insert_entry_at(to, fe);
Ok(())
}
@@ -276,7 +276,7 @@ pub(crate) unsafe fn fd_seek(
wasi::__WASI_RIGHTS_FD_SEEK | wasi::__WASI_RIGHTS_FD_TELL
};
let file = wasi_ctx
.get_fd_entry_mut(fd)?
.get_entry_mut(fd)?
.as_descriptor_mut(rights, 0)?
.as_file_mut()?;
@@ -310,7 +310,7 @@ pub(crate) unsafe fn fd_tell(
trace!("fd_tell(fd={:?}, newoffset={:#x?})", fd, newoffset);
let file = wasi_ctx
.get_fd_entry_mut(fd)?
.get_entry_mut(fd)?
.as_descriptor_mut(wasi::__WASI_RIGHTS_FD_TELL, 0)?
.as_file_mut()?;
@@ -338,7 +338,7 @@ pub(crate) unsafe fn fd_fdstat_get(
trace!("fd_fdstat_get(fd={:?}, fdstat_ptr={:#x?})", fd, fdstat_ptr);
let mut fdstat = dec_fdstat_byref(memory, fdstat_ptr)?;
let wasi_file = wasi_ctx.get_fd_entry(fd)?.as_descriptor(0, 0)?;
let wasi_file = wasi_ctx.get_entry(fd)?.as_descriptor(0, 0)?;
let fs_flags = match wasi_file {
Descriptor::OsHandle(wasi_fd) => hostcalls_impl::fd_fdstat_get(&wasi_fd)?,
@@ -346,7 +346,7 @@ pub(crate) unsafe fn fd_fdstat_get(
other => hostcalls_impl::fd_fdstat_get(&other.as_os_handle())?,
};
let fe = wasi_ctx.get_fd_entry(fd)?;
let fe = wasi_ctx.get_entry(fd)?;
fdstat.fs_filetype = fe.file_type;
fdstat.fs_rights_base = fe.rights_base;
fdstat.fs_rights_inheriting = fe.rights_inheriting;
@@ -366,7 +366,7 @@ pub(crate) unsafe fn fd_fdstat_set_flags(
trace!("fd_fdstat_set_flags(fd={:?}, fdflags={:#x?})", fd, fdflags);
let descriptor = wasi_ctx
.get_fd_entry_mut(fd)?
.get_entry_mut(fd)?
.as_descriptor_mut(wasi::__WASI_RIGHTS_FD_FDSTAT_SET_FLAGS, 0)?;
match descriptor {
@@ -409,7 +409,7 @@ pub(crate) unsafe fn fd_fdstat_set_rights(
fs_rights_inheriting
);
let fe = wasi_ctx.get_fd_entry_mut(fd)?;
let fe = wasi_ctx.get_entry_mut(fd)?;
if fe.rights_base & fs_rights_base != fs_rights_base
|| fe.rights_inheriting & fs_rights_inheriting != fs_rights_inheriting
{
@@ -429,7 +429,7 @@ pub(crate) unsafe fn fd_sync(
trace!("fd_sync(fd={:?})", fd);
let file = wasi_ctx
.get_fd_entry(fd)?
.get_entry(fd)?
.as_descriptor(wasi::__WASI_RIGHTS_FD_SYNC, 0)?
.as_file()?;
match file {
@@ -463,7 +463,7 @@ pub(crate) unsafe fn fd_write(
let iovs: Vec<io::IoSlice> = iovs.iter().map(|vec| host::ciovec_to_host(vec)).collect();
// perform unbuffered writes
let entry = wasi_ctx.get_fd_entry_mut(fd)?;
let entry = wasi_ctx.get_entry_mut(fd)?;
let isatty = entry.isatty();
let desc = entry.as_descriptor_mut(wasi::__WASI_RIGHTS_FD_WRITE, 0)?;
let host_nwritten = match desc {
@@ -523,7 +523,7 @@ pub(crate) unsafe fn fd_advise(
);
let file = wasi_ctx
.get_fd_entry_mut(fd)?
.get_entry_mut(fd)?
.as_descriptor_mut(wasi::__WASI_RIGHTS_FD_ADVISE, 0)?
.as_file_mut()?;
@@ -548,7 +548,7 @@ pub(crate) unsafe fn fd_allocate(
trace!("fd_allocate(fd={:?}, offset={}, len={})", fd, offset, len);
let file = wasi_ctx
.get_fd_entry(fd)?
.get_entry(fd)?
.as_descriptor(wasi::__WASI_RIGHTS_FD_ALLOCATE, 0)?
.as_file()?;
@@ -597,7 +597,7 @@ pub(crate) unsafe fn path_create_directory(
trace!(" | (path_ptr,path_len)='{}'", path);
let rights = wasi::__WASI_RIGHTS_PATH_OPEN | wasi::__WASI_RIGHTS_PATH_CREATE_DIRECTORY;
let fe = wasi_ctx.get_fd_entry(dirfd)?;
let fe = wasi_ctx.get_entry(dirfd)?;
let resolved = path_get(fe, rights, 0, 0, path, false)?;
resolved.path_create_directory()
@@ -631,8 +631,8 @@ pub(crate) unsafe fn path_link(
trace!(" | (old_path_ptr,old_path_len)='{}'", old_path);
trace!(" | (new_path_ptr,new_path_len)='{}'", new_path);
let old_fe = wasi_ctx.get_fd_entry(old_dirfd)?;
let new_fe = wasi_ctx.get_fd_entry(new_dirfd)?;
let old_fe = wasi_ctx.get_entry(old_dirfd)?;
let new_fe = wasi_ctx.get_entry(new_dirfd)?;
let resolved_old = path_get(
old_fe,
wasi::__WASI_RIGHTS_PATH_LINK_SOURCE,
@@ -693,7 +693,7 @@ pub(crate) unsafe fn path_open(
needed_base,
needed_inheriting
);
let fe = wasi_ctx.get_fd_entry(dirfd)?;
let fe = wasi_ctx.get_entry(dirfd)?;
let resolved = path_get(
fe,
needed_base,
@@ -719,12 +719,12 @@ pub(crate) unsafe fn path_open(
);
let fd = resolved.open_with(read, write, oflags, fs_flags)?;
let mut fe = FdEntry::from(fd)?;
let mut fe = Entry::from(fd)?;
// We need to manually deny the rights which are not explicitly requested
// because FdEntry::from will assign maximal consistent rights.
fe.rights_base &= fs_rights_base;
fe.rights_inheriting &= fs_rights_inheriting;
let guest_fd = wasi_ctx.insert_fd_entry(fe)?;
let guest_fd = wasi_ctx.insert_entry(fe)?;
trace!(" | *fd={:?}", guest_fd);
@@ -757,7 +757,7 @@ pub(crate) unsafe fn path_readlink(
trace!(" | (path_ptr,path_len)='{}'", &path);
let fe = wasi_ctx.get_fd_entry(dirfd)?;
let fe = wasi_ctx.get_entry(dirfd)?;
let resolved = path_get(fe, wasi::__WASI_RIGHTS_PATH_READLINK, 0, 0, &path, false)?;
let mut buf = dec_slice_of_mut_u8(memory, buf_ptr, buf_len)?;
@@ -801,8 +801,8 @@ pub(crate) unsafe fn path_rename(
trace!(" | (old_path_ptr,old_path_len)='{}'", old_path);
trace!(" | (new_path_ptr,new_path_len)='{}'", new_path);
let old_fe = wasi_ctx.get_fd_entry(old_dirfd)?;
let new_fe = wasi_ctx.get_fd_entry(new_dirfd)?;
let old_fe = wasi_ctx.get_entry(old_dirfd)?;
let new_fe = wasi_ctx.get_entry(new_dirfd)?;
let resolved_old = path_get(
old_fe,
wasi::__WASI_RIGHTS_PATH_RENAME_SOURCE,
@@ -847,7 +847,7 @@ pub(crate) unsafe fn fd_filestat_get(
);
let fd = wasi_ctx
.get_fd_entry(fd)?
.get_entry(fd)?
.as_descriptor(wasi::__WASI_RIGHTS_FD_FILESTAT_GET, 0)?
.as_file()?;
let host_filestat = match fd {
@@ -882,7 +882,7 @@ pub(crate) unsafe fn fd_filestat_set_times(
);
let fd = wasi_ctx
.get_fd_entry(fd)?
.get_entry(fd)?
.as_descriptor(wasi::__WASI_RIGHTS_FD_FILESTAT_SET_TIMES, 0)?
.as_file()?;
@@ -942,7 +942,7 @@ pub(crate) unsafe fn fd_filestat_set_size(
trace!("fd_filestat_set_size(fd={:?}, st_size={})", fd, st_size);
let file = wasi_ctx
.get_fd_entry(fd)?
.get_entry(fd)?
.as_descriptor(wasi::__WASI_RIGHTS_FD_FILESTAT_SET_SIZE, 0)?
.as_file()?;
@@ -983,7 +983,7 @@ pub(crate) unsafe fn path_filestat_get(
trace!(" | (path_ptr,path_len)='{}'", path);
let fe = wasi_ctx.get_fd_entry(dirfd)?;
let fe = wasi_ctx.get_entry(dirfd)?;
let resolved = path_get(
fe,
wasi::__WASI_RIGHTS_PATH_FILESTAT_GET,
@@ -1029,7 +1029,7 @@ pub(crate) unsafe fn path_filestat_set_times(
trace!(" | (path_ptr,path_len)='{}'", path);
let fe = wasi_ctx.get_fd_entry(dirfd)?;
let fe = wasi_ctx.get_entry(dirfd)?;
let resolved = path_get(
fe,
wasi::__WASI_RIGHTS_PATH_FILESTAT_SET_TIMES,
@@ -1073,7 +1073,7 @@ pub(crate) unsafe fn path_symlink(
trace!(" | (old_path_ptr,old_path_len)='{}'", old_path);
trace!(" | (new_path_ptr,new_path_len)='{}'", new_path);
let fe = wasi_ctx.get_fd_entry(dirfd)?;
let fe = wasi_ctx.get_entry(dirfd)?;
let resolved_new = path_get(fe, wasi::__WASI_RIGHTS_PATH_SYMLINK, 0, 0, new_path, true)?;
match resolved_new.dirfd() {
@@ -1102,7 +1102,7 @@ pub(crate) unsafe fn path_unlink_file(
trace!(" | (path_ptr,path_len)='{}'", path);
let fe = wasi_ctx.get_fd_entry(dirfd)?;
let fe = wasi_ctx.get_entry(dirfd)?;
let resolved = path_get(fe, wasi::__WASI_RIGHTS_PATH_UNLINK_FILE, 0, 0, path, false)?;
match resolved.dirfd() {
@@ -1129,7 +1129,7 @@ pub(crate) unsafe fn path_remove_directory(
trace!(" | (path_ptr,path_len)='{}'", path);
let fe = wasi_ctx.get_fd_entry(dirfd)?;
let fe = wasi_ctx.get_entry(dirfd)?;
let resolved = path_get(
fe,
wasi::__WASI_RIGHTS_PATH_REMOVE_DIRECTORY,
@@ -1160,7 +1160,7 @@ pub(crate) unsafe fn fd_prestat_get(
);
// TODO: should we validate any rights here?
let fe = wasi_ctx.get_fd_entry(fd)?;
let fe = wasi_ctx.get_entry(fd)?;
let po_path = fe.preopen_path.as_ref().ok_or(WasiError::ENOTSUP)?;
if fe.file_type != wasi::__WASI_FILETYPE_DIRECTORY {
return Err(WasiError::ENOTDIR);
@@ -1197,7 +1197,7 @@ pub(crate) unsafe fn fd_prestat_dir_name(
);
// TODO: should we validate any rights here?
let fe = wasi_ctx.get_fd_entry(fd)?;
let fe = wasi_ctx.get_entry(fd)?;
let po_path = fe.preopen_path.as_ref().ok_or(WasiError::ENOTSUP)?;
if fe.file_type != wasi::__WASI_FILETYPE_DIRECTORY {
return Err(WasiError::ENOTDIR);
@@ -1235,7 +1235,7 @@ pub(crate) unsafe fn fd_readdir(
enc_usize_byref(memory, buf_used, 0)?;
let file = wasi_ctx
.get_fd_entry_mut(fd)?
.get_entry_mut(fd)?
.as_descriptor_mut(wasi::__WASI_RIGHTS_FD_READDIR, 0)?
.as_file_mut()?;
let host_buf = dec_slice_of_mut_u8(memory, buf, buf_len)?;

6
crates/wasi-common/src/hostcalls_impl/fs_helpers.rs
View File

@@ -1,9 +1,9 @@
#![allow(non_camel_case_types)]
use crate::sys::fdentry_impl::OsHandle;
use crate::sys::entry_impl::OsHandle;
use crate::sys::host_impl;
use crate::sys::hostcalls_impl::fs_helpers::*;
use crate::wasi::{self, WasiError, WasiResult};
use crate::{fdentry::Descriptor, fdentry::FdEntry};
use crate::{entry::Descriptor, entry::Entry};
use std::path::{Component, Path};
#[derive(Debug)]
@@ -102,7 +102,7 @@ impl<'a, 'b> PathRef<'a, 'b> {
///
/// This is a workaround for not having Capsicum support in the OS.
pub(crate) fn path_get(
fe: &FdEntry,
fe: &Entry,
rights_base: wasi::__wasi_rights_t,
rights_inheriting: wasi::__wasi_rights_t,
dirflags: wasi::__wasi_lookupflags_t,

6
crates/wasi-common/src/hostcalls_impl/misc.rs
View File

@@ -1,6 +1,6 @@
#![allow(non_camel_case_types)]
use crate::ctx::WasiCtx;
use crate::fdentry::Descriptor;
use crate::entry::Descriptor;
use crate::memory::*;
use crate::sys::hostcalls_impl;
use crate::wasi::{self, WasiError, WasiResult};
@@ -250,7 +250,7 @@ pub(crate) fn poll_oneoff(
let rights = wasi::__WASI_RIGHTS_FD_READ | wasi::__WASI_RIGHTS_POLL_FD_READWRITE;
match unsafe {
wasi_ctx
.get_fd_entry(wasi_fd)
.get_entry(wasi_fd)
.and_then(|fe| fe.as_descriptor(rights, 0))
} {
Ok(descriptor) => fd_events.push(FdEventData {
@@ -278,7 +278,7 @@ pub(crate) fn poll_oneoff(
let rights = wasi::__WASI_RIGHTS_FD_WRITE | wasi::__WASI_RIGHTS_POLL_FD_READWRITE;
match unsafe {
wasi_ctx
.get_fd_entry(wasi_fd)
.get_entry(wasi_fd)
.and_then(|fe| fe.as_descriptor(rights, 0))
} {
Ok(descriptor) => fd_events.push(FdEventData {