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.

Sign up for GitHub

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

Rename when redirect is to the same fs #4702

Merged
merged 5 commits into from
May 15, 2024
Merged

Rename when redirect is to the same fs #4702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
955c3ef
rename when redirect is to the same fs
maminrayej May 15, 2024
95c77d7
Verify the file was renamed successfully in the fs_rename-file test
Arshia001 May 15, 2024
e7af7d7
Add unit test for renaming in inner FS
Arshia001 May 15, 2024
e3b8c36
append file name after redirect
maminrayej May 15, 2024
b90536f
Merge branch 'main' into redirect-when-renaming
maminrayej May 15, 2024
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
214 changes: 129 additions & 85 deletions lib/virtual-fs/src/mem_fs/filesystem.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)?;
Expand All @@ -462,85 +458,119 @@ 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, 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) => a,
InodeResolution::Redirect(..) => {
return Err(FsError::InvalidInput);
InodeResolution::Found(a) => Either::Left(a),
InodeResolution::Redirect(fs, mut path) => {
path.push(&name_of_to);
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);

{
// 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)?;
// 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(())
})
}

Expand Down Expand Up @@ -1257,6 +1287,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<dyn crate::FileSystem + Send + Sync> = Arc::new(fs2);

assert_eq!(
fs.rename(path!("/"), path!("/bar")).await,
Err(FsError::BaseNotDirectory),
Expand All @@ -1280,30 +1314,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),
Expand All @@ -1312,9 +1345,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!(
Expand Down Expand Up @@ -1389,6 +1422,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,
Expand All @@ -1403,7 +1447,7 @@ mod test_filesystem {

assert_eq!(
fs_inner.storage.len(),
6,
7,
"storage has still all directories"
);
assert!(
Expand All @@ -1414,9 +1458,9 @@ mod test_filesystem {
name,
children,
..
})) if name == "/" && children == &[3]
})) if name == "/" && children == &[3, 6]
),
"`/` contains `bar`",
"`/` contains `bar` and `mnt`",
);
assert!(
matches!(
Expand Down
6 changes: 6 additions & 0 deletions tests/wasi-fyi/fs_rename-file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
2 changes: 1 addition & 1 deletion tests/wasi-fyi/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading