From 955c3ef39c9641d3c12115dbb1a07bca293cd386 Mon Sep 17 00:00:00 2001 From: "M.Amin Rayej" Date: Wed, 15 May 2024 14:53:03 +0330 Subject: [PATCH 1/4] rename when redirect is to the same fs --- lib/virtual-fs/src/mem_fs/filesystem.rs | 158 ++++++++++++++---------- tests/wasi-fyi/test.sh | 2 +- 2 files changed, 92 insertions(+), 68 deletions(-) diff --git a/lib/virtual-fs/src/mem_fs/filesystem.rs b/lib/virtual-fs/src/mem_fs/filesystem.rs index 17ce1bf31df..82180c6fe3a 100644 --- a/lib/virtual-fs/src/mem_fs/filesystem.rs +++ b/lib/virtual-fs/src/mem_fs/filesystem.rs @@ -4,7 +4,7 @@ use self::offloaded_file::OffloadBackingStore; use super::*; use crate::{DirEntry, FileSystem as _, FileType, FsError, Metadata, OpenOptions, ReadDir, Result}; -use futures::future::BoxFuture; +use futures::future::{BoxFuture, Either}; use slab::Slab; use std::collections::VecDeque; use std::convert::identity; @@ -438,12 +438,8 @@ impl crate::FileSystem for FileSystem { Box::pin(async { let name_of_to; - let ( - (position_of_from, inode, inode_of_from_parent), - (inode_of_to_parent, name_of_to), - inode_dest, - ) = { - // Read lock. + // Read lock. + let (name_of_from, inode_of_from_parent, name_of_to, inode_of_to_parent) = { let fs = self.inner.read().map_err(|_| FsError::Lock)?; let from = fs.canonicalize_without_inode(from)?; @@ -462,85 +458,113 @@ impl crate::FileSystem for FileSystem { // Find the parent inodes. let inode_of_from_parent = match fs.inode_of_parent(parent_of_from)? { - InodeResolution::Found(a) => a, - InodeResolution::Redirect(..) => { - return Err(FsError::InvalidInput); - } + InodeResolution::Found(a) => Either::Left(a), + InodeResolution::Redirect(fs, path) => Either::Right((fs, path)), }; let inode_of_to_parent = match fs.inode_of_parent(parent_of_to)? { - InodeResolution::Found(a) => a, - InodeResolution::Redirect(..) => { - return Err(FsError::InvalidInput); - } + InodeResolution::Found(a) => Either::Left(a), + InodeResolution::Redirect(fs, path) => Either::Right((fs, path)), }; - // Find the inode of the dest file if it exists - let maybe_position_and_inode_of_file = - fs.as_parent_get_position_and_inode_of_file(inode_of_to_parent, &name_of_to)?; - - // Get the child indexes to update in the parent nodes, in - // addition to the inode of the directory to update. - let (position_of_from, inode) = fs - .as_parent_get_position_and_inode(inode_of_from_parent, &name_of_from)? - .ok_or(FsError::EntryNotFound)?; - ( - (position_of_from, inode, inode_of_from_parent), - (inode_of_to_parent, name_of_to), - maybe_position_and_inode_of_file, + name_of_from, + inode_of_from_parent, + name_of_to, + inode_of_to_parent, ) }; - let inode = match inode { - InodeResolution::Found(a) => a, - InodeResolution::Redirect(..) => { - return Err(FsError::InvalidInput); - } - }; - - { - // Write lock. - let mut fs = self.inner.write().map_err(|_| FsError::Lock)?; - - if let Some((position, inode_of_file)) = inode_dest { - // Remove the file from the storage. - match inode_of_file { - InodeResolution::Found(inode_of_file) => { - fs.storage.remove(inode_of_file); - } + match (inode_of_from_parent, inode_of_to_parent) { + (Either::Left(inode_of_from_parent), Either::Left(inode_of_to_parent)) => { + let fs = self.inner.read().map_err(|_| FsError::Lock)?; + + // Find the inode of the dest file if it exists + let maybe_position_and_inode_of_file = fs + .as_parent_get_position_and_inode_of_file( + inode_of_to_parent, + &name_of_to, + )?; + + // Get the child indexes to update in the parent nodes, in + // addition to the inode of the directory to update. + let (position_of_from, inode) = fs + .as_parent_get_position_and_inode(inode_of_from_parent, &name_of_from)? + .ok_or(FsError::EntryNotFound)?; + + let ( + (position_of_from, inode, inode_of_from_parent), + (inode_of_to_parent, name_of_to), + inode_dest, + ) = ( + (position_of_from, inode, inode_of_from_parent), + (inode_of_to_parent, name_of_to), + maybe_position_and_inode_of_file, + ); + + let inode = match inode { + InodeResolution::Found(a) => a, InodeResolution::Redirect(..) => { return Err(FsError::InvalidInput); } - } + }; - fs.remove_child_from_node(inode_of_to_parent, position)?; - } + drop(fs); - // Update the file name, and update the modified time. - fs.update_node_name(inode, name_of_to)?; + { + // Write lock. + let mut fs = self.inner.write().map_err(|_| FsError::Lock)?; + + if let Some((position, inode_of_file)) = inode_dest { + // Remove the file from the storage. + match inode_of_file { + InodeResolution::Found(inode_of_file) => { + fs.storage.remove(inode_of_file); + } + InodeResolution::Redirect(..) => { + return Err(FsError::InvalidInput); + } + } + + fs.remove_child_from_node(inode_of_to_parent, position)?; + } + + // Update the file name, and update the modified time. + fs.update_node_name(inode, name_of_to)?; - // The parents are different. Let's update them. - if inode_of_from_parent != inode_of_to_parent { - // Remove the file from its parent, and update the - // modified time. - fs.remove_child_from_node(inode_of_from_parent, position_of_from)?; + // The parents are different. Let's update them. + if inode_of_from_parent != inode_of_to_parent { + // Remove the file from its parent, and update the + // modified time. + fs.remove_child_from_node(inode_of_from_parent, position_of_from)?; - // Add the file to its new parent, and update the modified - // time. - fs.add_child_to_node(inode_of_to_parent, inode)?; + // Add the file to its new parent, and update the modified + // time. + fs.add_child_to_node(inode_of_to_parent, inode)?; + } + // Otherwise, we need to at least update the modified time of the parent. + else { + let mut inode = fs.storage.get_mut(inode_of_from_parent); + match inode.as_mut() { + Some(Node::Directory(node)) => node.metadata.modified = time(), + Some(Node::ArcDirectory(node)) => node.metadata.modified = time(), + _ => return Err(FsError::UnknownError), + } + } + } + + Ok(()) } - // Otherwise, we need to at least update the modified time of the parent. - else { - let mut inode = fs.storage.get_mut(inode_of_from_parent); - match inode.as_mut() { - Some(Node::Directory(node)) => node.metadata.modified = time(), - Some(Node::ArcDirectory(node)) => node.metadata.modified = time(), - _ => return Err(FsError::UnknownError), + (Either::Right((from_fs, from_path)), Either::Right((to_fs, to_path))) => { + if Arc::ptr_eq(&from_fs, &to_fs) { + let same_fs = from_fs; + + same_fs.rename(&from_path, &to_path).await + } else { + Err(FsError::InvalidInput) } } + _ => Err(FsError::InvalidInput), } - - Ok(()) }) } diff --git a/tests/wasi-fyi/test.sh b/tests/wasi-fyi/test.sh index 664d605033e..4085c2c024e 100755 --- a/tests/wasi-fyi/test.sh +++ b/tests/wasi-fyi/test.sh @@ -6,7 +6,7 @@ bash build.sh status=0 # Define skip list as an array -SKIP_LIST=("ported_readlink.wasm" "fs_create_dir-existing-directory.wasm" "fs_rename-directory.wasm" "fs_rename-file.wasm") +SKIP_LIST=("ported_readlink.wasm" "fs_create_dir-existing-directory.wasm") # List and process .foo files for file in *.wasm; do From 95c77d7dc54506e3fa7d764a679e52624b8cc382 Mon Sep 17 00:00:00 2001 From: Arshia Ghafoori Date: Wed, 15 May 2024 13:21:02 +0330 Subject: [PATCH 2/4] Verify the file was renamed successfully in the fs_rename-file test --- tests/wasi-fyi/fs_rename-file.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/wasi-fyi/fs_rename-file.rs b/tests/wasi-fyi/fs_rename-file.rs index 2da55f0a496..e598ce2dbb3 100644 --- a/tests/wasi-fyi/fs_rename-file.rs +++ b/tests/wasi-fyi/fs_rename-file.rs @@ -5,4 +5,10 @@ fn main() { let new_path = "/fyi/fs_rename-file.dir/new_file"; assert!(fs::rename(old_path, new_path).is_ok()); + + let error = fs::metadata(old_path).unwrap_err(); + assert_eq!(error.kind(), std::io::ErrorKind::NotFound); + + let metadata = fs::metadata(new_path).unwrap(); + assert!(metadata.is_file()); } From e7af7d74bf47f8b740b80cf9d1d5a2c9767ac2a4 Mon Sep 17 00:00:00 2001 From: Arshia Ghafoori Date: Wed, 15 May 2024 13:39:44 +0330 Subject: [PATCH 3/4] Add unit test for renaming in inner FS --- lib/virtual-fs/src/mem_fs/filesystem.rs | 54 ++++++++++++++++--------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/lib/virtual-fs/src/mem_fs/filesystem.rs b/lib/virtual-fs/src/mem_fs/filesystem.rs index 82180c6fe3a..c47573de304 100644 --- a/lib/virtual-fs/src/mem_fs/filesystem.rs +++ b/lib/virtual-fs/src/mem_fs/filesystem.rs @@ -1281,6 +1281,10 @@ mod test_filesystem { async fn test_rename() { let fs = FileSystem::default(); + let fs2 = FileSystem::default(); + fs2.create_dir(path!("/boz")).unwrap(); + let arc_fs2: Arc = Arc::new(fs2); + assert_eq!( fs.rename(path!("/"), path!("/bar")).await, Err(FsError::BaseNotDirectory), @@ -1304,30 +1308,29 @@ mod test_filesystem { assert_eq!(fs.create_dir(path!("/bar")), Ok(())); assert!( - matches!( - fs.new_open_options() - .write(true) - .create_new(true) - .open(path!("/bar/hello1.txt")), - Ok(_), - ), + fs.new_open_options() + .write(true) + .create_new(true) + .open(path!("/bar/hello1.txt")) + .is_ok(), "creating a new file (`hello1.txt`)", ); assert!( - matches!( - fs.new_open_options() - .write(true) - .create_new(true) - .open(path!("/bar/hello2.txt")), - Ok(_), - ), + fs.new_open_options() + .write(true) + .create_new(true) + .open(path!("/bar/hello2.txt")) + .is_ok(), "creating a new file (`hello2.txt`)", ); + fs.mount(path!(buf "/mnt"), &arc_fs2, path!(buf "/")) + .unwrap(); + { let fs_inner = fs.inner.read().unwrap(); - assert_eq!(fs_inner.storage.len(), 6, "storage has all files"); + assert_eq!(fs_inner.storage.len(), 7, "storage has all files"); assert!( matches!( fs_inner.storage.get(ROOT_INODE), @@ -1336,9 +1339,9 @@ mod test_filesystem { name, children, .. - })) if name == "/" && children == &[1, 3] + })) if name == "/" && children == &[1, 3, 6] ), - "`/` contains `foo` and `bar`", + "`/` contains `foo` and `bar` and `mnt`", ); assert!( matches!( @@ -1413,6 +1416,17 @@ mod test_filesystem { "renaming a directory", ); + assert_eq!( + fs.rename(path!("/mnt/boz"), path!("/mnt/cat")).await, + Ok(()), + "renaming a directory within a mounted FS" + ); + + assert!( + arc_fs2.read_dir(path!("/cat")).is_ok(), + "The directory was renamed in the inner FS" + ); + assert_eq!( fs.rename(path!("/bar/hello1.txt"), path!("/bar/world1.txt")) .await, @@ -1427,7 +1441,7 @@ mod test_filesystem { assert_eq!( fs_inner.storage.len(), - 6, + 7, "storage has still all directories" ); assert!( @@ -1438,9 +1452,9 @@ mod test_filesystem { name, children, .. - })) if name == "/" && children == &[3] + })) if name == "/" && children == &[3, 6] ), - "`/` contains `bar`", + "`/` contains `bar` and `mnt`", ); assert!( matches!( From e3b8c3617dd307a4722003a7eb28e07ba4873a7c Mon Sep 17 00:00:00 2001 From: "M.Amin Rayej" Date: Wed, 15 May 2024 15:47:28 +0330 Subject: [PATCH 4/4] append file name after redirect --- lib/virtual-fs/src/mem_fs/filesystem.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/virtual-fs/src/mem_fs/filesystem.rs b/lib/virtual-fs/src/mem_fs/filesystem.rs index c47573de304..3c1258048c4 100644 --- a/lib/virtual-fs/src/mem_fs/filesystem.rs +++ b/lib/virtual-fs/src/mem_fs/filesystem.rs @@ -459,11 +459,17 @@ impl crate::FileSystem for FileSystem { // Find the parent inodes. let inode_of_from_parent = match fs.inode_of_parent(parent_of_from)? { InodeResolution::Found(a) => Either::Left(a), - InodeResolution::Redirect(fs, path) => Either::Right((fs, path)), + InodeResolution::Redirect(fs, mut path) => { + path.push(&name_of_from); + Either::Right((fs, path)) + } }; let inode_of_to_parent = match fs.inode_of_parent(parent_of_to)? { InodeResolution::Found(a) => Either::Left(a), - InodeResolution::Redirect(fs, path) => Either::Right((fs, path)), + InodeResolution::Redirect(fs, mut path) => { + path.push(&name_of_to); + Either::Right((fs, path)) + } }; (