From 2b8ea938eaadd9a7e295c2f38efca035901f7041 Mon Sep 17 00:00:00 2001 From: Josh Megnauth Date: Sat, 20 Dec 2025 16:08:58 -0500 Subject: [PATCH] Fix test lints and add rename test This fixes lints for tests/tests.rs and adds a test for rename. The new rename test is based on the rename_no_replace tests. There are two discrepancies between Linux and Redox but I'm not sure if they matter. I added a note to those tests. There are two fixed lints of note. * assert_cmd deprecated Command::cargo_bin in favor of a compile time checked macro. * The mode literals were wrong: 0666 instead of 0o666. 0o is the prefix for octal literals. --- src/tests.rs | 35 ++++++------- src/unmount.rs | 5 +- tests/tests.rs | 137 +++++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 137 insertions(+), 40 deletions(-) diff --git a/src/tests.rs b/src/tests.rs index a4ad10c939..0187a62b88 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -1,5 +1,5 @@ -use crate::htree::{HTreeHash, HTreeNode, HTreePtr, HTREE_IDX_ENTRIES}; use crate::{ + htree::{HTreeHash, HTreeNode, HTreePtr, HTREE_IDX_ENTRIES}, transaction::{level_data, level_data_mut, FsCtx}, BlockAddr, BlockData, BlockMeta, BlockPtr, DirEntry, DirList, DiskMemory, DiskSparse, FileSystem, Node, TreePtr, ALLOC_GC_THRESHOLD, BLOCK_SIZE, @@ -49,7 +49,7 @@ fn many_create_remove_should_not_increase_size() { tx.create_node( tree_ptr, &format!("{}{}", name, i), - Node::MODE_FILE | 0644, + Node::MODE_FILE | 0o644, 1, 0, )?; @@ -78,7 +78,7 @@ fn many_create_then_many_remove_should_not_increase_size() { tx.create_node( tree_ptr, &format!("test{}", i), - Node::MODE_FILE | 0644, + Node::MODE_FILE | 0o644, 1, 0, ) @@ -115,8 +115,7 @@ fn empty_dir() { // List let mut children = Vec::::new(); - let _ = fs - .tx(|tx| tx.child_nodes(empty_dir.ptr(), &mut children)) + fs.tx(|tx| tx.child_nodes(empty_dir.ptr(), &mut children)) .unwrap(); assert_eq!(children.len(), 0); @@ -151,7 +150,7 @@ fn many_create_write_list_find_read_delete() { tx.create_node( tree_ptr, &format!("file{i:05}"), - Node::MODE_FILE | 0644, + Node::MODE_FILE | 0o644, 1, 0, ) @@ -179,7 +178,7 @@ fn many_create_write_list_find_read_delete() { // Confirm that they can be listed { let mut children = Vec::::with_capacity(total_count); - let _ = fs.tx(|tx| tx.child_nodes(tree_ptr, &mut children)).unwrap(); + fs.tx(|tx| tx.child_nodes(tree_ptr, &mut children)).unwrap(); assert_eq!( children.len(), total_count, @@ -228,15 +227,14 @@ fn many_create_write_list_find_read_delete() { // Delete all the files for i in 0..total_count { let file_name = format!("file{i:05}"); - let result = fs.tx(|tx| tx.remove_node(tree_ptr, &file_name, Node::MODE_FILE)); - if result.is_err() { + if let Err(e) = fs.tx(|tx| tx.remove_node(tree_ptr, &file_name, Node::MODE_FILE)) { println!("Failure on delete iteration {i}"); - result.unwrap(); + panic!("{e}"); } let result = fs.tx(|tx| tx.find_node(tree_ptr, &file_name)); - if !result.is_err() || result.err().unwrap().errno != syscall::error::ENOENT { + if result.is_ok() || result.unwrap_err().errno != syscall::error::ENOENT { println!("Failure on delete verification iteration {i}"); - assert!(false, "Deletion appears to ahve failred"); + panic!("Deletion appears to have failed"); } } } @@ -678,14 +676,13 @@ fn split_htree_level0_to_level1() { for i in 8..17 { let name = format!("child__{i:0243}"); let result = tx.remove_node(parent_ptr, name.as_str(), Node::MODE_FILE); - if result.is_err() { - assert!( - false, + result.unwrap_or_else(|e| { + panic!( "Failed to remove file {name} with hash {:?} error {:?}", HTreeHash::from_name(&name), - result.err() - ); - } + e + ) + }); } Ok(()) }) @@ -929,7 +926,7 @@ fn split_htree_with_multiple_levels() { }); } -/// Test a pathalogical case of many HTreeHash collisions. This should never happen in reality, +/// Test a pathological case of many HTreeHash collisions. This should never happen in reality, /// but the system can support it. #[test] fn split_htree_with_multiple_levels_using_duplicates() { diff --git a/src/unmount.rs b/src/unmount.rs index 368fd5910f..2f781e271b 100644 --- a/src/unmount.rs +++ b/src/unmount.rs @@ -43,10 +43,7 @@ pub fn unmount_path(mount_path: &str) -> Result<(), io::Error> { let status = status_res?; if !status.success() { - return Err(io::Error::new( - io::ErrorKind::Other, - "redoxfs umount failed", - )); + return Err(io::Error::other("redoxfs umount failed")); } } diff --git a/tests/tests.rs b/tests/tests.rs index 011cfaf6a2..39d1b09f22 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -1,5 +1,3 @@ -use assert_cmd::cargo::CommandCargoExt; - use core::panic::AssertUnwindSafe; use redoxfs::{unmount_path, DirEntry, DiskMemory, DiskSparse, FileSystem, Node, TreePtr}; @@ -54,7 +52,7 @@ where "/root/target/x86_64-unknown-redox/debug/redoxfs", ); } - let mut mount_cmd = Command::cargo_bin("redoxfs").expect("unable to find mount bin"); + let mut mount_cmd = Command::new(assert_cmd::cargo_bin!("redoxfs")); mount_cmd.arg("-d").arg(dbg!(&fs)).arg(dbg!(&mount_path)); let mut child = mount_cmd.spawn().expect("mount failed to run"); @@ -84,6 +82,7 @@ where sleep(Duration::from_millis(200)); child.kill().expect("Can't kill"); + let _ = child.wait(); if cfg!(target_os = "redox") { unmount_path(&mount_path).unwrap(); @@ -92,12 +91,12 @@ where panic!("sync failed"); } - if !unmount_path(&mount_path).is_ok() { + if unmount_path(&mount_path).is_err() { // There seems to be a race condition where the device can be busy when trying to unmount. // So, we pause for a moment and retry. There will still be an error output to the logs // for the first failed attempt. sleep(Duration::from_millis(200)); - if !unmount_path(&mount_path).is_ok() { + if unmount_path(&mount_path).is_err() { panic!("umount failed"); } } @@ -116,7 +115,7 @@ where #[test] fn simple() { with_mounted(|path| { - dbg!(fs::create_dir(&path.join("test"))).unwrap(); + dbg!(fs::create_dir(path.join("test"))).unwrap(); }) } @@ -147,7 +146,8 @@ fn create_and_remove_directory() { let dir_path = path.join(dir_name); // Create the directory - fs::create_dir(&dir_path).expect(&format!("cannot create dir {}", &dir_path.display())); + fs::create_dir(&dir_path) + .unwrap_or_else(|_| panic!("cannot create dir {}", &dir_path.display())); assert!(fs::exists(&dir_path).unwrap()); // Check that the directory is empty @@ -192,7 +192,7 @@ fn create_and_remove_symlink() { // Create the symmlink according to the platform #[cfg(unix)] - std::os::unix::fs::symlink(&real_file, &symlink_path).unwrap(); + std::os::unix::fs::symlink(real_file, &symlink_path).unwrap(); #[cfg(windows)] std::os::windows::fs::symlink_file(&real_file, &symlink_path).unwrap(); @@ -296,7 +296,7 @@ fn many_create_write_list_find_read_delete() { tx.create_node( tree_ptr, &format!("file{i:05}"), - Node::MODE_FILE | 0644, + Node::MODE_FILE | 0o644, 1, 0, ) @@ -324,7 +324,7 @@ fn many_create_write_list_find_read_delete() { // Confirm that they can be listed { let mut children = Vec::::with_capacity(total_count); - let _ = fs.tx(|tx| tx.child_nodes(tree_ptr, &mut children)).unwrap(); + fs.tx(|tx| tx.child_nodes(tree_ptr, &mut children)).unwrap(); assert_eq!( children.len(), total_count, @@ -373,15 +373,15 @@ fn many_create_write_list_find_read_delete() { // Delete all the files for i in 0..total_count { let file_name = format!("file{i:05}"); - let result = fs.tx(|tx| tx.remove_node(tree_ptr, &file_name, Node::MODE_FILE)); - if result.is_err() { + if let Err(e) = fs.tx(|tx| tx.remove_node(tree_ptr, &file_name, Node::MODE_FILE)) { println!("Failure on delete iteration {i}"); - result.unwrap(); + panic!("{e}"); } + let result = fs.tx(|tx| tx.find_node(tree_ptr, &file_name)); - if !result.is_err() || result.err().unwrap().errno != syscall::error::ENOENT { + if result.is_ok() || result.unwrap_err().errno != syscall::error::ENOENT { println!("Failure on delete verification iteration {i}"); - assert!(false, "Deletion appears to ahve failred"); + panic!("Deletion appears to have failed"); } } } @@ -393,7 +393,7 @@ fn many_write_read_delete_mounted() { for i in 0..total_count { fs::write( - &path.join(format!("file{}", i)), + path.join(format!("file{}", i)), format!("Hello, number {i}!"), ) .unwrap(); @@ -401,7 +401,7 @@ fn many_write_read_delete_mounted() { // Confirm each of the created files can be found and read for i in 0..total_count { - let contents = fs::read_to_string(&path.join(format!("file{}", i))).unwrap(); + let contents = fs::read_to_string(path.join(format!("file{}", i))).unwrap(); assert_eq!(contents, format!("Hello, number {i}!")); } @@ -511,3 +511,106 @@ fn rename_no_replace() { fs.tx(|tx| tx.rename_node_no_replace(root, "dir", root, "newdir")) .expect("Renaming 'dir' to 'newdir' should succeed"); } + +#[test] +fn rename_works() { + let disk = DiskMemory::new(1024 * 1024 * 1024); + let mut fs = FileSystem::create(disk, None, 0, 0) + .expect("Creating in memory file system should succeed"); + + let root = TreePtr::root(); + let dir = fs + .tx(|tx| tx.create_node(root, "dir", Node::MODE_DIR, 0, 0)) + .expect("Creating a directory should succeed"); + let source_file = fs + .tx(|tx| tx.create_node(root, "source", Node::MODE_FILE, 0, 0)) + .expect("Creating source file should succeed"); + let target_file_orig = fs + .tx(|tx| tx.create_node(root, "target", Node::MODE_FILE, 0, 0)) + .expect("Creating target file should succeed"); + + // Rename /source to /source2 + fs.tx(|tx| tx.rename_node(root, "source", root, "source2")) + .expect("Renaming existing 'source' to non-existing 'source2' should succeed"); + let source2_file = fs + .tx(|tx| tx.find_node(root, "source2")) + .expect("'source2' should exist because we just renamed 'source' to 'source2'"); + assert_eq!(source_file.id(), source2_file.id()); + let err = fs + .tx(|tx| tx.find_node(root, "source")) + .expect_err("'source' should not exist because it was moved"); + assert_eq!(syscall::ENOENT, err.errno); + + // Rename /source2 to /target + fs.tx(|tx| tx.rename_node(root, "source2", root, "target")) + .expect("Renaming existing 'source2' to existing 'target' should succeed"); + let target_file_mv = fs + .tx(|tx| tx.find_node(root, "target")) + .expect("'target' should exist because the rename succeeded"); + assert_ne!( + target_file_orig.id(), + target_file_mv.id(), + "Move failed because 'target' is still the same" + ); + assert_eq!( + source2_file.id(), + target_file_mv.id(), + "Move failed because 'source2' != 'target'" + ); + + // Don't rename /target to /dir + // XXX: A similar test fails on Linux using rename(). Not sure if the discrepancy matters. + // let err = fs + // .tx(|tx| tx.rename_node(root, "target", root, "dir")) + // .expect_err("Renaming 'target' to existing directory 'dir' should fail"); + // assert_eq!( + // syscall::EEXIST, + // err.errno, + // "Renaming to existing file should fail with EEXIST" + // ); + // assert_ne!( + // dir.id(), + // target_file_mv.id(), + // "'target' and 'dir' should be distinct nodes" + // ); + + // Don't rename /dir to /target + // XXX: A similar test fails on Linux using rename(). + // let err = fs + // .tx(|tx| tx.rename_node(root, "dir", root, "target")) + // .expect_err("Renaming 'dir' to existing file 'target' should fail"); + // assert_eq!( + // syscall::EEXIST, + // err.errno, + // "Renaming to existing file should fail with EEXIST" + // ); + // assert_ne!( + // target_file_mv.id(), + // dir.id(), + // "'dir' and 'target' should be distinct nodes" + // ); + + // Rename /target to /target + fs.tx(|tx| tx.rename_node(root, "target", root, "target")) + .expect("Renaming 'target' to itself should succeed"); + let target_self_mv = fs + .tx(|tx| tx.find_node(root, "target")) + .expect("'target' should exist because rename succeeded"); + assert_eq!( + target_file_mv.id(), + target_self_mv.id(), + "'target' shouldn't have changed during a move to self" + ); + + // Rename /target to /dir/target + fs.tx(|tx| tx.rename_node(root, "target", dir.ptr(), "target")) + .expect("Renaming /target to /dir/target should succeed"); + let moved_target = fs + .tx(|tx| tx.find_node(dir.ptr(), "target")) + .expect("'target' should have moved to /dir/target"); + assert_eq!(target_file_mv.id(), moved_target.id()); + + // Rename /dir to /newdir + fs.tx(|tx| tx.rename_node(root, "dir", root, "newdir")) + .expect("Renaming 'dir' to 'newdir' should succeed"); +}