Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Virtual file support #701

Merged
merged 29 commits into from
Mar 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
0af1b9c
Add support for virtual files (eg, not backed by an OS file).
iximeow Dec 5, 2019
8d03646
bet this fixes the macos test failure
iximeow Feb 1, 2020
c9b2b8e
fix merge conflict with windows FdEvent handling
iximeow Feb 1, 2020
ceb1fb3
adjust tests to also run for virtfs-backed preopens
iximeow Feb 7, 2020
b15d01a
fix several typos
iximeow Feb 7, 2020
d0b0eac
implement most of the remaining virtfs functionality
iximeow Feb 7, 2020
e0d943b
forgot that FileType needs to be PartialEq now
iximeow Feb 7, 2020
1c729b8
fix remaining tests, add some logging, and ignore yet-unimplemented e…
iximeow Feb 8, 2020
910a0e7
un-trace wasi-common in CI
iximeow Feb 8, 2020
222f0b3
oops broke bsd
iximeow Feb 8, 2020
10ee1c5
and fix some windows impl details
iximeow Feb 8, 2020
b96f58f
rustfmt
iximeow Feb 8, 2020
34cc7b1
fix a few cases where Windows fs hostcalls did not dispatch to virtfs…
iximeow Feb 10, 2020
2ec462e
keep flags and data together so they're both shared by open InMemoryFile
iximeow Feb 10, 2020
29ef153
missed cargo-fmting test-programs/build.rs
iximeow Feb 10, 2020
9fe0f7c
move VirtualFile delegation out of any sys/ hostcall impls
iximeow Feb 18, 2020
f4ace0c
add From impls for Descriptor and clean up some transforms
iximeow Feb 18, 2020
fbb6d91
avoid unnecessary style changes
iximeow Feb 19, 2020
6af98fd
how did i miss rustfmting this
iximeow Feb 19, 2020
9fd7fcd
check and fix non-linux impls
iximeow Feb 19, 2020
14d763c
see what this looks like without Handle or HandleMut
iximeow Feb 20, 2020
cc8ee2a
public part of VirtualFile to just read/write and vectored forms
iximeow Feb 28, 2020
18e9946
rustfmt
iximeow Feb 28, 2020
b43b4bf
fix wonky rebase conflict
iximeow Feb 28, 2020
17bd739
forgot to commit and push test fixes
iximeow Feb 28, 2020
1892a44
move iovec total size check to all fd_{read,write}, not just virtfs
iximeow Feb 28, 2020
253061a
PR debris
iximeow Feb 28, 2020
70d1219
Merge branch 'master' into virtfs
sunfishcode Mar 6, 2020
f468b6c
one last bit of cleanup!
iximeow Mar 6, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 92 additions & 19 deletions crates/test-programs/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,19 @@ fn main() {
#[cfg(feature = "test_programs")]
mod wasi_tests {
use std::env;
use std::fs::{read_dir, DirEntry, File};
use std::fs::{read_dir, File};
use std::io::{self, Write};
use std::path::{Path, PathBuf};
use std::process::{Command, Stdio};

#[derive(Clone, Copy, Debug)]
enum PreopenType {
/// Preopens should be satisfied with real OS files.
OS,
/// Preopens should be satisfied with virtual files.
Virtual,
}

pub(super) fn build_and_generate_tests() {
// Validate if any of test sources are present and if they changed
// This should always work since there is no submodule to init anymore
Expand Down Expand Up @@ -103,34 +111,52 @@ mod wasi_tests {
.replace("-", "_")
)?;
writeln!(out, " use super::{{runtime, utils, setup_log}};")?;
writeln!(out, " use runtime::PreopenType;")?;
for dir_entry in dir_entries {
write_testsuite_tests(out, dir_entry, testsuite)?;
let test_path = dir_entry.path();
let stemstr = test_path
.file_stem()
.expect("file_stem")
.to_str()
.expect("to_str");

if no_preopens(testsuite, stemstr) {
write_testsuite_tests(out, &test_path, testsuite, PreopenType::OS)?;
} else {
write_testsuite_tests(out, &test_path, testsuite, PreopenType::OS)?;
write_testsuite_tests(out, &test_path, testsuite, PreopenType::Virtual)?;
}
}
writeln!(out, "}}")?;
Ok(())
}

fn write_testsuite_tests(
out: &mut File,
dir_entry: DirEntry,
path: &Path,
testsuite: &str,
preopen_type: PreopenType,
) -> io::Result<()> {
let path = dir_entry.path();
let stemstr = path
.file_stem()
.expect("file_stem")
.to_str()
.expect("to_str");

writeln!(out, " #[test]")?;
if ignore(testsuite, stemstr) {
let test_fn_name = format!(
"{}{}",
&stemstr.replace("-", "_"),
if let PreopenType::Virtual = preopen_type {
"_virtualfs"
} else {
""
}
);
if ignore(testsuite, &test_fn_name) {
writeln!(out, " #[ignore]")?;
}
writeln!(
out,
" fn r#{}() -> anyhow::Result<()> {{",
&stemstr.replace("-", "_")
)?;
writeln!(out, " fn r#{}() -> anyhow::Result<()> {{", test_fn_name,)?;
writeln!(out, " setup_log();")?;
writeln!(
out,
Expand All @@ -145,16 +171,25 @@ mod wasi_tests {
let workspace = if no_preopens(testsuite, stemstr) {
"None"
} else {
writeln!(
out,
" let workspace = utils::prepare_workspace(&bin_name)?;"
)?;
"Some(workspace.path())"
match preopen_type {
PreopenType::OS => {
writeln!(
out,
" let workspace = utils::prepare_workspace(&bin_name)?;"
)?;
"Some(workspace.path())"
}
PreopenType::Virtual => "Some(std::path::Path::new(&bin_name))",
}
};
writeln!(
out,
" runtime::instantiate(&data, &bin_name, {})",
workspace
" runtime::instantiate(&data, &bin_name, {}, {})",
workspace,
match preopen_type {
PreopenType::OS => "PreopenType::OS",
PreopenType::Virtual => "PreopenType::Virtual",
}
)?;
writeln!(out, " }}")?;
writeln!(out)?;
Expand All @@ -164,8 +199,30 @@ mod wasi_tests {
cfg_if::cfg_if! {
if #[cfg(not(windows))] {
/// Ignore tests that aren't supported yet.
fn ignore(_testsuite: &str, _name: &str) -> bool {
false
fn ignore(testsuite: &str, name: &str) -> bool {
if testsuite == "wasi-tests" {
match name {
sunfishcode marked this conversation as resolved.
Show resolved Hide resolved
// TODO: virtfs files cannot be poll_oneoff'd yet
"poll_oneoff_virtualfs" => true,
// TODO: virtfs does not support filetimes yet.
"path_filestat_virtualfs" |
"fd_filestat_set_virtualfs" => true,
// TODO: virtfs does not support symlinks yet.
"nofollow_errors_virtualfs" |
"path_link_virtualfs" |
"readlink_virtualfs" |
"readlink_no_buffer_virtualfs" |
"dangling_symlink_virtualfs" |
"symlink_loop_virtualfs" |
"path_symlink_trailing_slashes_virtualfs" => true,
// TODO: virtfs does not support rename yet.
"path_rename_trailing_slashes_virtualfs" |
"path_rename_virtualfs" => true,
_ => false,
}
} else {
unreachable!()
}
}
} else {
/// Ignore tests that aren't supported yet.
Expand All @@ -178,6 +235,22 @@ mod wasi_tests {
"truncation_rights" => true,
"path_link" => true,
"dangling_fd" => true,
// TODO: virtfs files cannot be poll_oneoff'd yet
"poll_oneoff_virtualfs" => true,
// TODO: virtfs does not support filetimes yet.
"path_filestat_virtualfs" |
"fd_filestat_set_virtualfs" => true,
// TODO: virtfs does not support symlinks yet.
"nofollow_errors_virtualfs" |
"path_link_virtualfs" |
"readlink_virtualfs" |
"readlink_no_buffer_virtualfs" |
"dangling_symlink_virtualfs" |
"symlink_loop_virtualfs" |
"path_symlink_trailing_slashes_virtualfs" => true,
// TODO: virtfs does not support rename yet.
"path_rename_trailing_slashes_virtualfs" |
"path_rename_virtualfs" => true,
Comment on lines +238 to +253
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this looks like we're redoing every VirtualFs test on every host. Am I correct in assuming so, and do we actually need to do that given that our de facto host is "virtual"? How much effort do you think it would take to have VirtualFs tests behind a feature flag (treating it as a separate "host" altogether), and do you reckon it would actually make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So.. my thinking here is that because Descriptor involves non-virtual variants, in the strictest sense we ought to run on all hosts. Slightly less interesting, we'd need to pick some host to run *_virtualfs tests on, unless we codify the idea of a "virtual" host to the point of having it define "OsHandle" for tests too.

Realistically, your recommendation to adjust dispatch to VirtualFs to consistently be before getting reaching host-specific impls would help here. At that point I think running these on only one host (linux, because I'm biased?) is less risky .. when doing full CI runs, anyway.

I think the last reason to run VirtualFs tests for each host is local development; Running on every host is helpful for local platform-specific development, since you'll only run one host's view. Do you think it makes sense to make it conditional on tests are running in CI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see what you mean by having a mix of the two: a host plus some virtual fs on top of it. The reason I mentioned it here is since there seems to be a lot of code duplication so I was trying to figure out if we could somehow reduce it especially since it seems to be very much copy-paste reuse. 🤔

I think the last reason to run VirtualFs tests for each host is local development; Running on every host is helpful for local platform-specific development, since you'll only run one host's view. Do you think it makes sense to make it conditional on tests are running in CI?

I think I'd actually be more comfortable always having it on so that we make sure we don't break it with any changes to wasi-common in the future. In fact, I think that full test coverage that's always on trumps some code duplication here and there. But it would be nice to have the first without the second ;-)

_ => false,
}
} else {
Expand Down
44 changes: 29 additions & 15 deletions crates/test-programs/tests/wasm_tests/runtime.rs
Original file line number Diff line number Diff line change
@@ -1,30 +1,44 @@
use anyhow::{bail, Context};
use std::fs::File;
use std::path::Path;
use wasi_common::VirtualDirEntry;
use wasmtime::{Instance, Module, Store};

pub fn instantiate(data: &[u8], bin_name: &str, workspace: Option<&Path>) -> anyhow::Result<()> {
let store = Store::default();

let get_preopens = |workspace: Option<&Path>| -> anyhow::Result<Vec<_>> {
if let Some(workspace) = workspace {
let preopen_dir = wasi_common::preopen_dir(workspace)
.context(format!("error while preopening {:?}", workspace))?;
#[derive(Clone, Copy, Debug)]
pub enum PreopenType {
/// Preopens should be satisfied with real OS files.
OS,
/// Preopens should be satisfied with virtual files.
Virtual,
}

Ok(vec![(".".to_owned(), preopen_dir)])
} else {
Ok(vec![])
}
};
pub fn instantiate(
data: &[u8],
bin_name: &str,
workspace: Option<&Path>,
preopen_type: PreopenType,
) -> anyhow::Result<()> {
let store = Store::default();

// Create our wasi context with pretty standard arguments/inheritance/etc.
// Additionally register andy preopened directories if we have them.
// Additionally register any preopened directories if we have them.
let mut builder = wasi_common::WasiCtxBuilder::new();

builder.arg(bin_name).arg(".").inherit_stdio();

for (dir, file) in get_preopens(workspace)? {
builder.preopened_dir(file, dir);
if let Some(workspace) = workspace {
match preopen_type {
PreopenType::OS => {
let preopen_dir = wasi_common::preopen_dir(workspace)
.context(format!("error while preopening {:?}", workspace))?;
builder.preopened_dir(preopen_dir, ".");
}
PreopenType::Virtual => {
// we can ignore the workspace path for virtual preopens because virtual preopens
// don't exist in the filesystem anyway - no name conflict concerns.
builder.preopened_virt(VirtualDirEntry::empty_directory(), ".");
}
}
}

// The nonstandard thing we do with `WasiCtxBuilder` is to ensure that
Expand Down
6 changes: 3 additions & 3 deletions crates/test-programs/wasi-tests/src/bin/fd_flags_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ unsafe fn test_fd_fdstat_set_flags(dir_fd: wasi::Fd) {
)
.expect("reading file"),
buffer.len(),
"shoudl read {} bytes",
"should read {} bytes",
buffer.len()
);

Expand Down Expand Up @@ -87,7 +87,7 @@ unsafe fn test_fd_fdstat_set_flags(dir_fd: wasi::Fd) {
)
.expect("reading file"),
buffer.len(),
"shoudl read {} bytes",
"should read {} bytes",
buffer.len()
);

Expand Down Expand Up @@ -126,7 +126,7 @@ unsafe fn test_fd_fdstat_set_flags(dir_fd: wasi::Fd) {
)
.expect("reading file"),
buffer.len(),
"shoudl read {} bytes",
"should read {} bytes",
buffer.len()
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ unsafe fn test_remove_directory_trailing_slashes(dir_fd: wasi::Fd) {

wasi::path_create_directory(dir_fd, "dir").expect("creating a directory");

// Test that removing it with a trailing flash succeeds.
// Test that removing it with a trailing slash succeeds.
wasi::path_remove_directory(dir_fd, "dir/")
.expect("remove_directory with a trailing slash on a directory should succeed");

// Create a temporary file.
create_file(dir_fd, "file");

// Test that removing it with no trailing flash fails.
// Test that removing it with no trailing slash fails.
assert_eq!(
wasi::path_remove_directory(dir_fd, "file")
.expect_err("remove_directory without a trailing slash on a file should fail")
Expand All @@ -27,7 +27,7 @@ unsafe fn test_remove_directory_trailing_slashes(dir_fd: wasi::Fd) {
"errno should be ERRNO_NOTDIR"
);

// Test that removing it with a trailing flash fails.
// Test that removing it with a trailing slash fails.
assert_eq!(
wasi::path_remove_directory(dir_fd, "file/")
.expect_err("remove_directory with a trailing slash on a file should fail")
Expand Down
62 changes: 56 additions & 6 deletions crates/wasi-common/src/ctx.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::fdentry::FdEntry;
use crate::fdentry::{Descriptor, FdEntry};
use crate::sys::fdentry_impl::OsHandle;
use crate::virtfs::{VirtualDir, VirtualDirEntry};
use crate::{wasi, Error, Result};
use std::borrow::Borrow;
use std::collections::HashMap;
Expand Down Expand Up @@ -61,7 +63,7 @@ impl PendingCString {
/// A builder allowing customizable construction of `WasiCtx` instances.
pub struct WasiCtxBuilder {
fds: Option<HashMap<wasi::__wasi_fd_t, PendingFdEntry>>,
preopens: Option<Vec<(PathBuf, File)>>,
preopens: Option<Vec<(PathBuf, Descriptor)>>,
args: Option<Vec<PendingCString>>,
env: Option<HashMap<PendingCString, PendingCString>>,
}
Expand Down Expand Up @@ -222,10 +224,46 @@ impl WasiCtxBuilder {

/// Add a preopened directory.
pub fn preopened_dir<P: AsRef<Path>>(&mut self, dir: File, guest_path: P) -> &mut Self {
self.preopens.as_mut().unwrap().push((
guest_path.as_ref().to_owned(),
Descriptor::OsHandle(OsHandle::from(dir)),
));
self
}

/// Add a preopened virtual directory.
pub fn preopened_virt<P: AsRef<Path>>(
&mut self,
dir: VirtualDirEntry,
guest_path: P,
) -> &mut Self {
fn populate_directory(virtentry: HashMap<String, VirtualDirEntry>, dir: &mut VirtualDir) {
for (path, entry) in virtentry.into_iter() {
match entry {
VirtualDirEntry::Directory(dir_entries) => {
let mut subdir = VirtualDir::new(true);
populate_directory(dir_entries, &mut subdir);
dir.add_dir(subdir, path);
}
VirtualDirEntry::File(content) => {
dir.add_file(content, path);
}
}
}
}

let dir = if let VirtualDirEntry::Directory(entries) = dir {
let mut dir = VirtualDir::new(true);
populate_directory(entries, &mut dir);
Box::new(dir)
} else {
panic!("the root of a VirtualDirEntry tree must be a VirtualDirEntry::Directory");
};

self.preopens
.as_mut()
.unwrap()
.push((guest_path.as_ref().to_owned(), dir));
.push((guest_path.as_ref().to_owned(), Descriptor::VirtualFile(dir)));
self
}

Expand Down Expand Up @@ -271,7 +309,7 @@ impl WasiCtxBuilder {
fds.insert(fd, f()?);
}
PendingFdEntry::File(f) => {
fds.insert(fd, FdEntry::from(f)?);
fds.insert(fd, FdEntry::from(Descriptor::OsHandle(OsHandle::from(f)))?);
}
}
}
Expand All @@ -284,8 +322,20 @@ impl WasiCtxBuilder {
// unnecessarily if we have exactly the maximum number of file descriptors.
preopen_fd = preopen_fd.checked_add(1).ok_or(Error::ENFILE)?;

if !dir.metadata()?.is_dir() {
return Err(Error::EBADF);
match &dir {
Descriptor::OsHandle(handle) => {
if !handle.metadata()?.is_dir() {
return Err(Error::EBADF);
}
}
Descriptor::VirtualFile(virt) => {
if virt.get_file_type() != wasi::__WASI_FILETYPE_DIRECTORY {
return Err(Error::EBADF);
}
}
Descriptor::Stdin | Descriptor::Stdout | Descriptor::Stderr => {
panic!("implementation error, stdin/stdout/stderr shouldn't be in the list of preopens");
}
}

// We don't currently allow setting file descriptors other than 0-2, but this will avoid
Expand Down
Loading