From 1eb8a8a7fe8e120bdbfceddf6c472326f693af03 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Tue, 26 Jan 2021 12:23:52 -0800 Subject: [PATCH] integrate GetSetFdFlags! change reopen_with_fdflags(&self, fdflags) -> Result> to set_fdflags(&mut self, fdflags) -> Result<()>. this makes way more sense than my prior hare-brained schemes. --- crates/test-programs/TEST_FAILURES | 30 ++++--------- crates/wasi-c2/cap-std-sync/src/file.rs | 53 ++++++++++++++++++++--- crates/wasi-c2/cap-std-sync/src/stdio.rs | 11 ++--- crates/wasi-c2/src/file.rs | 32 +++++++------- crates/wasi-c2/src/pipe.rs | 4 +- crates/wasi-c2/src/snapshots/preview_1.rs | 16 +++---- 6 files changed, 83 insertions(+), 63 deletions(-) diff --git a/crates/test-programs/TEST_FAILURES b/crates/test-programs/TEST_FAILURES index 2c226a686d..f4cc6b9f84 100644 --- a/crates/test-programs/TEST_FAILURES +++ b/crates/test-programs/TEST_FAILURES @@ -1,8 +1,4 @@ -TODOs: -* File::reopen_with_fdflags is unimplemented, File::get_fdflags is lying - these are - fcntl on unix, reopenfile / require reopening on windows. - # Linux @@ -12,12 +8,8 @@ TODOs: * remove_directory_trailing_slashes - cap-std Dir::remove_dir gives EINVAL when trying to remove dir with trailing slash. otherwise, everything passes. - -* fd_flags_set - - set_fdflags is not implemented. test wanted to clear O_APPEND mode * path_filestat - - fdstat.fs_flags is not populated correctly - APPEND | SYNC aren't - present because File::get_fdflags isnt implemented correctly + - symlink mtim doesnt match expectations # Windows @@ -27,6 +19,7 @@ TODOs: * file_allocate - call to fd_allocate(10,10) reduces size from 100 to 20 - fix upstream: fd_allocate is impossible on windows, so dont even try + - the test should be modified to accept this fd_allocate always failing on windows. * nofollow_errors - I loosened up some errno acceptance but windows requires rmdir to delete a symlink to a directory, rather than unlink_file @@ -34,14 +27,15 @@ TODOs: - narrowed down the symlink delete issue that only shows up on linux * path_rename - permission denied on windows to rename a dir to an existing empty dir - - This should fail - - We should disable this test on windows - - is there a windows api that makes this possible? - - if not, and we drew the line at "no racy emulation" in fallocate, then - is returning the error windows gives us OK here? - + - we're not going to try to emulate this functionality. + - the test should be modified accept this operation failing completely on windows. * path_link - fails on the first dangling symlink +* fd_flags_set + - same metadata panic as fd_readdir +* path_filestat + - same metadata panic as fd_readdir + ## "Trailing slashes are a bonified boondoggle" - Dan @@ -51,9 +45,3 @@ TODOs: * path_symlink_trailing_slashes * remove_directory_trailing_slashes * unlink_file_trailing_slashes - -## Same missing functionality as over in Linux: - -* fd_flags_set -* path_filestat - diff --git a/crates/wasi-c2/cap-std-sync/src/file.rs b/crates/wasi-c2/cap-std-sync/src/file.rs index 0089af7779..66d55c2043 100644 --- a/crates/wasi-c2/cap-std-sync/src/file.rs +++ b/crates/wasi-c2/cap-std-sync/src/file.rs @@ -3,8 +3,10 @@ use fs_set_times::{SetTimes, SystemTimeSpec}; use std::any::Any; use std::convert::TryInto; use std::io; -use system_interface::fs::{Advice, FileIoExt}; -use system_interface::io::ReadReady; +use system_interface::{ + fs::{Advice, FileIoExt, GetSetFdFlags}, + io::ReadReady, +}; use wasi_c2::{ file::{FdFlags, FileType, Filestat, WasiFile}, Error, @@ -35,11 +37,11 @@ impl WasiFile for File { Ok(filetype_from(&meta.file_type())) } fn get_fdflags(&self) -> Result { - // XXX get_fdflags is not implemented but lets lie rather than panic: - Ok(FdFlags::empty()) + let fdflags = self.0.get_fd_flags()?; + Ok(from_sysif_fdflags(fdflags)) } - unsafe fn reopen_with_fdflags(&self, _fdflags: FdFlags) -> Result, Error> { - todo!("reopen_with_fdflags is not implemented") + fn set_fdflags(&mut self, fdflags: FdFlags) -> Result<(), Error> { + Ok(self.0.set_fd_flags(to_sysif_fdflags(fdflags))?) } fn get_filestat(&self) -> Result { let meta = self.0.metadata()?; @@ -150,3 +152,42 @@ pub fn convert_systimespec(t: Option) -> Option None, } } + +pub fn to_sysif_fdflags(f: wasi_c2::file::FdFlags) -> system_interface::fs::FdFlags { + let mut out = system_interface::fs::FdFlags::empty(); + if f.contains(wasi_c2::file::FdFlags::APPEND) { + out |= system_interface::fs::FdFlags::APPEND; + } + if f.contains(wasi_c2::file::FdFlags::DSYNC) { + out |= system_interface::fs::FdFlags::DSYNC; + } + if f.contains(wasi_c2::file::FdFlags::NONBLOCK) { + out |= system_interface::fs::FdFlags::NONBLOCK; + } + if f.contains(wasi_c2::file::FdFlags::RSYNC) { + out |= system_interface::fs::FdFlags::RSYNC; + } + if f.contains(wasi_c2::file::FdFlags::SYNC) { + out |= system_interface::fs::FdFlags::SYNC; + } + out +} +pub fn from_sysif_fdflags(f: system_interface::fs::FdFlags) -> wasi_c2::file::FdFlags { + let mut out = wasi_c2::file::FdFlags::empty(); + if f.contains(system_interface::fs::FdFlags::APPEND) { + out |= wasi_c2::file::FdFlags::APPEND; + } + if f.contains(system_interface::fs::FdFlags::DSYNC) { + out |= wasi_c2::file::FdFlags::DSYNC; + } + if f.contains(system_interface::fs::FdFlags::NONBLOCK) { + out |= wasi_c2::file::FdFlags::NONBLOCK; + } + if f.contains(system_interface::fs::FdFlags::RSYNC) { + out |= wasi_c2::file::FdFlags::RSYNC; + } + if f.contains(system_interface::fs::FdFlags::SYNC) { + out |= wasi_c2::file::FdFlags::SYNC; + } + out +} diff --git a/crates/wasi-c2/cap-std-sync/src/stdio.rs b/crates/wasi-c2/cap-std-sync/src/stdio.rs index 935f37a4da..9cc1badf76 100644 --- a/crates/wasi-c2/cap-std-sync/src/stdio.rs +++ b/crates/wasi-c2/cap-std-sync/src/stdio.rs @@ -36,10 +36,9 @@ impl WasiFile for Stdin { Ok(FileType::Unknown) } fn get_fdflags(&self) -> Result { - // XXX get_fdflags is not implemented but lets lie rather than panic: Ok(FdFlags::empty()) } - unsafe fn reopen_with_fdflags(&self, _fdflags: FdFlags) -> Result, Error> { + fn set_fdflags(&mut self, _fdflags: FdFlags) -> Result<(), Error> { Err(Error::badf()) } fn get_filestat(&self) -> Result { @@ -125,13 +124,9 @@ macro_rules! wasi_file_write_impl { Ok(FileType::Unknown) } fn get_fdflags(&self) -> Result { - // XXX get_fdflags is not implemented but lets lie rather than panic: - Ok(FdFlags::empty()) + Ok(FdFlags::APPEND) } - unsafe fn reopen_with_fdflags( - &self, - _fdflags: FdFlags, - ) -> Result, Error> { + fn set_fdflags(&mut self, _fdflags: FdFlags) -> Result<(), Error> { Err(Error::badf()) } fn get_filestat(&self) -> Result { diff --git a/crates/wasi-c2/src/file.rs b/crates/wasi-c2/src/file.rs index c69c795b80..38e94d3bc5 100644 --- a/crates/wasi-c2/src/file.rs +++ b/crates/wasi-c2/src/file.rs @@ -1,8 +1,8 @@ use crate::{Error, ErrorExt, SystemTimeSpec}; use bitflags::bitflags; use std::any::Any; -use std::cell::Ref; -use std::ops::Deref; +use std::cell::{Ref, RefMut}; +use std::ops::{Deref, DerefMut}; pub trait WasiFile { fn as_any(&self) -> &dyn Any; @@ -10,9 +10,7 @@ pub trait WasiFile { fn sync(&self) -> Result<(), Error>; // file op fn get_filetype(&self) -> Result; // file op fn get_fdflags(&self) -> Result; // file op - /// This method takes a `&self` so that it can be called on a `&dyn WasiFile`. However, - /// the caller makes the additional guarantee to drop `self` after the call is successful. - unsafe fn reopen_with_fdflags(&self, flags: FdFlags) -> Result, Error>; // file op + fn set_fdflags(&mut self, flags: FdFlags) -> Result<(), Error>; // file op fn get_filestat(&self) -> Result; // split out get_length as a read & write op, rest is a file op fn set_filestat_size(&self, _size: u64) -> Result<(), Error>; // write op fn advise( @@ -83,22 +81,14 @@ pub struct Filestat { pub(crate) trait TableFileExt { fn get_file(&self, fd: u32) -> Result, Error>; - fn update_file_in_place(&mut self, fd: u32, f: F) -> Result<(), Error> - where - F: FnOnce(&dyn WasiFile) -> Result, Error>; + fn get_file_mut(&self, fd: u32) -> Result, Error>; } impl TableFileExt for crate::table::Table { fn get_file(&self, fd: u32) -> Result, Error> { self.get(fd) } - fn update_file_in_place(&mut self, fd: u32, f: F) -> Result<(), Error> - where - F: FnOnce(&dyn WasiFile) -> Result, Error>, - { - self.update_in_place(fd, |FileEntry { caps, file }| { - let file = f(file.deref())?; - Ok(FileEntry { caps, file }) - }) + fn get_file_mut(&self, fd: u32) -> Result, Error> { + self.get_mut(fd) } } @@ -145,6 +135,16 @@ impl<'a> FileEntryExt<'a> for Ref<'a, FileEntry> { Ok(Ref::map(self, |r| r.file.deref())) } } +pub trait FileEntryMutExt<'a> { + fn get_cap(self, caps: FileCaps) -> Result, Error>; +} + +impl<'a> FileEntryMutExt<'a> for RefMut<'a, FileEntry> { + fn get_cap(self, caps: FileCaps) -> Result, Error> { + self.capable_of(caps)?; + Ok(RefMut::map(self, |r| r.file.deref_mut())) + } +} bitflags! { pub struct FileCaps : u32 { diff --git a/crates/wasi-c2/src/pipe.rs b/crates/wasi-c2/src/pipe.rs index 4705c8bdba..0652f2093f 100644 --- a/crates/wasi-c2/src/pipe.rs +++ b/crates/wasi-c2/src/pipe.rs @@ -114,7 +114,7 @@ impl WasiFile for ReadPipe { fn get_fdflags(&self) -> Result { Ok(FdFlags::empty()) } - unsafe fn reopen_with_fdflags(&self, _fdflags: FdFlags) -> Result, Error> { + fn set_fdflags(&mut self, _fdflags: FdFlags) -> Result<(), Error> { Err(Error::badf()) } fn get_filestat(&self) -> Result { @@ -250,7 +250,7 @@ impl WasiFile for WritePipe { fn get_fdflags(&self) -> Result { Ok(FdFlags::APPEND) } - unsafe fn reopen_with_fdflags(&self, _fdflags: FdFlags) -> Result, Error> { + fn set_fdflags(&mut self, _fdflags: FdFlags) -> Result<(), Error> { Err(Error::badf()) } fn get_filestat(&self) -> Result { diff --git a/crates/wasi-c2/src/snapshots/preview_1.rs b/crates/wasi-c2/src/snapshots/preview_1.rs index 28640d23a3..d948d51321 100644 --- a/crates/wasi-c2/src/snapshots/preview_1.rs +++ b/crates/wasi-c2/src/snapshots/preview_1.rs @@ -2,8 +2,8 @@ use crate::{ dir::{DirCaps, DirEntry, DirEntryExt, DirFdStat, ReaddirCursor, ReaddirEntity, TableDirExt}, file::{ - FdFlags, FdStat, FileCaps, FileEntry, FileEntryExt, FileType, Filestat, OFlags, - TableFileExt, + FdFlags, FdStat, FileCaps, FileEntry, FileEntryExt, FileEntryMutExt, FileType, Filestat, + OFlags, TableFileExt, }, sched::{ subscription::{RwEventFlags, SubscriptionResult}, @@ -332,14 +332,10 @@ impl<'a> wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { } fn fd_fdstat_set_flags(&self, fd: types::Fd, flags: types::Fdflags) -> Result<(), Error> { - let mut table = self.table(); - let fd = u32::from(fd); - let table_check = table.get_file(fd)?.get_cap(FileCaps::FDSTAT_SET_FLAGS)?; - drop(table_check); - table.update_file_in_place(fd, |f| unsafe { - // Safety: update_file_in_place will drop `f` after this call. - f.reopen_with_fdflags(FdFlags::from(&flags)) - }) + self.table() + .get_file_mut(u32::from(fd))? + .get_cap(FileCaps::FDSTAT_SET_FLAGS)? + .set_fdflags(FdFlags::from(&flags)) } fn fd_fdstat_set_rights(