From 47d39d1efa212fb1fc1da91b8393348c4726f187 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Mon, 30 Aug 2021 16:19:54 +0800 Subject: [PATCH 01/21] [tests] Do not assume no stderr with thin_dump --- tests/thin_dump.rs | 9 +++++++-- tests/thin_repair.rs | 8 ++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/tests/thin_dump.rs b/tests/thin_dump.rs index 8e17e19..c1bd3c4 100644 --- a/tests/thin_dump.rs +++ b/tests/thin_dump.rs @@ -114,6 +114,7 @@ fn dump_restore_cycle() -> Result<()> { // test no stderr with a normal dump #[test] +#[cfg(not(feature = "rust_tests"))] fn no_stderr() -> Result<()> { let mut td = TestDir::new()?; @@ -133,7 +134,9 @@ fn override_something(flag: &str, value: &str, pattern: &str) -> Result<()> { let md = mk_valid_md(&mut td)?; let output = run_ok_raw(THIN_DUMP, args![&md, flag, value])?; - assert_eq!(output.stderr.len(), 0); + if !cfg!(feature = "rust_tests") { + assert_eq!(output.stderr.len(), 0); + } assert!(from_utf8(&output.stdout[0..])?.contains(pattern)); Ok(()) } @@ -179,7 +182,9 @@ fn repair_superblock() -> Result<()> { &md ], )?; - assert_eq!(after.stderr.len(), 0); + if !cfg!(feature = "rust_tests") { + assert_eq!(after.stderr.len(), 0); + } assert_eq!(before.stdout, after.stdout); Ok(()) diff --git a/tests/thin_repair.rs b/tests/thin_repair.rs index 19e4865..3a37846 100644 --- a/tests/thin_repair.rs +++ b/tests/thin_repair.rs @@ -148,7 +148,9 @@ fn superblock_succeeds() -> Result<()> { &md1 ], )?; - assert_eq!(original.stderr.len(), 0); + if !cfg!(feature = "rust_tests") { + assert_eq!(original.stderr.len(), 0); + } damage_superblock(&md1)?; let md2 = mk_zeroed_md(&mut td)?; run_ok( @@ -164,7 +166,9 @@ fn superblock_succeeds() -> Result<()> { ], )?; let repaired = run_ok_raw(THIN_DUMP, args![&md2])?; - assert_eq!(repaired.stderr.len(), 0); + if !cfg!(feature = "rust_tests") { + assert_eq!(repaired.stderr.len(), 0); + } assert_eq!(original.stdout, repaired.stdout); Ok(()) } From 737cf2f67dcd5a51c3b194babcb5cc0050d047b3 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 31 Aug 2021 00:19:44 +0800 Subject: [PATCH 02/21] [tests] Refine trait names --- tests/cache_check.rs | 2 +- tests/common/input_arg.rs | 4 ++-- tests/common/output_option.rs | 6 +++--- tests/common/program.rs | 10 ++++++++-- tests/thin_check.rs | 2 +- tests/thin_metadata_pack.rs | 4 ---- tests/thin_metadata_unpack.rs | 4 ---- tests/thin_repair.rs | 10 ++++++---- tests/thin_restore.rs | 12 +++++++----- 9 files changed, 28 insertions(+), 26 deletions(-) diff --git a/tests/cache_check.rs b/tests/cache_check.rs index d729542..3aa724f 100644 --- a/tests/cache_check.rs +++ b/tests/cache_check.rs @@ -67,7 +67,7 @@ impl<'a> InputProgram<'a> for CacheCheck { } } -impl<'a> BinaryInputProgram<'_> for CacheCheck {} +impl<'a> MetadataReader<'a> for CacheCheck {} //------------------------------------------ diff --git a/tests/common/input_arg.rs b/tests/common/input_arg.rs index 3453f24..7ef7e69 100644 --- a/tests/common/input_arg.rs +++ b/tests/common/input_arg.rs @@ -182,7 +182,7 @@ macro_rules! test_unreadable_input_file { pub fn test_help_message_for_tiny_input_file<'a, P>() -> Result<()> where - P: BinaryInputProgram<'a>, + P: MetadataReader<'a>, { let mut td = TestDir::new()?; @@ -209,7 +209,7 @@ macro_rules! test_help_message_for_tiny_input_file { pub fn test_spot_xml_data<'a, P>() -> Result<()> where - P: BinaryInputProgram<'a>, + P: MetadataReader<'a>, { let mut td = TestDir::new()?; diff --git a/tests/common/output_option.rs b/tests/common/output_option.rs index e4e7695..57028f1 100644 --- a/tests/common/output_option.rs +++ b/tests/common/output_option.rs @@ -33,12 +33,12 @@ macro_rules! test_missing_output_option { pub fn test_output_file_not_found<'a, P>() -> Result<()> where - P: OutputProgram<'a>, + P: MetadataWriter<'a>, { let mut td = TestDir::new()?; let input = P::mk_valid_input(&mut td)?; let stderr = run_fail(P::path(), args!["-i", &input, "-o", "no-such-file"])?; - assert!(stderr.contains(

::file_not_found())); + assert!(stderr.contains(

::file_not_found())); Ok(()) } @@ -105,7 +105,7 @@ macro_rules! test_unwritable_output_file { // currently thin/cache_restore only pub fn test_tiny_output_file<'a, P>() -> Result<()> where - P: BinaryOutputProgram<'a>, + P: MetadataWriter<'a>, { let mut td = TestDir::new()?; let input = P::mk_valid_input(&mut td)?; diff --git a/tests/common/program.rs b/tests/common/program.rs index 51b63e9..0414b3d 100644 --- a/tests/common/program.rs +++ b/tests/common/program.rs @@ -30,14 +30,20 @@ pub trait InputProgram<'a>: Program<'a> { fn corrupted_input() -> &'a str; } -pub trait BinaryInputProgram<'a>: InputProgram<'a> {} +pub trait MetadataReader<'a>: InputProgram<'a> {} pub trait OutputProgram<'a>: InputProgram<'a> { // error messages fn missing_output_arg() -> &'a str; +} + +// programs that write existed files +pub trait MetadataWriter<'a>: OutputProgram<'a> { + // error messages fn file_not_found() -> &'a str; } -pub trait BinaryOutputProgram<'a>: OutputProgram<'a> {} +// programs that create output files (O_CREAT) +pub trait MetadataCreator<'a>: OutputProgram<'a> {} //------------------------------------------ diff --git a/tests/thin_check.rs b/tests/thin_check.rs index 1f1db46..608a5cb 100644 --- a/tests/thin_check.rs +++ b/tests/thin_check.rs @@ -70,7 +70,7 @@ impl<'a> InputProgram<'a> for ThinCheck { } } -impl<'a> BinaryInputProgram<'_> for ThinCheck {} +impl<'a> MetadataReader<'_> for ThinCheck {} //------------------------------------------ diff --git a/tests/thin_metadata_pack.rs b/tests/thin_metadata_pack.rs index dea1eff..b45c1bc 100644 --- a/tests/thin_metadata_pack.rs +++ b/tests/thin_metadata_pack.rs @@ -74,10 +74,6 @@ impl<'a> InputProgram<'a> for ThinMetadataPack { } impl<'a> OutputProgram<'a> for ThinMetadataPack { - fn file_not_found() -> &'a str { - rust_msg::FILE_NOT_FOUND - } - fn missing_output_arg() -> &'a str { rust_msg::MISSING_OUTPUT_ARG } diff --git a/tests/thin_metadata_unpack.rs b/tests/thin_metadata_unpack.rs index 1292965..fe77f22 100644 --- a/tests/thin_metadata_unpack.rs +++ b/tests/thin_metadata_unpack.rs @@ -76,10 +76,6 @@ impl<'a> InputProgram<'a> for ThinMetadataUnpack { } impl<'a> OutputProgram<'a> for ThinMetadataUnpack { - fn file_not_found() -> &'a str { - rust_msg::FILE_NOT_FOUND - } - fn missing_output_arg() -> &'a str { rust_msg::MISSING_OUTPUT_ARG } diff --git a/tests/thin_repair.rs b/tests/thin_repair.rs index 3a37846..eb3ac2d 100644 --- a/tests/thin_repair.rs +++ b/tests/thin_repair.rs @@ -69,15 +69,17 @@ impl<'a> InputProgram<'a> for ThinRepair { } impl<'a> OutputProgram<'a> for ThinRepair { - fn file_not_found() -> &'a str { - cpp_msg::FILE_NOT_FOUND - } - fn missing_output_arg() -> &'a str { cpp_msg::MISSING_OUTPUT_ARG } } +impl<'a> MetadataWriter<'a> for ThinRepair { + fn file_not_found() -> &'a str { + cpp_msg::FILE_NOT_FOUND + } +} + //----------------------------------------- test_accepts_help!(ThinRepair); diff --git a/tests/thin_restore.rs b/tests/thin_restore.rs index 65ef87a..b908566 100644 --- a/tests/thin_restore.rs +++ b/tests/thin_restore.rs @@ -70,16 +70,16 @@ impl<'a> InputProgram<'a> for ThinRestore { } impl<'a> OutputProgram<'a> for ThinRestore { - fn file_not_found() -> &'a str { - msg::FILE_NOT_FOUND - } - fn missing_output_arg() -> &'a str { msg::MISSING_OUTPUT_ARG } } -impl<'a> BinaryOutputProgram<'_> for ThinRestore {} +impl<'a> MetadataWriter<'a> for ThinRestore { + fn file_not_found() -> &'a str { + msg::FILE_NOT_FOUND + } +} //----------------------------------------- @@ -93,6 +93,8 @@ test_corrupted_input_data!(ThinRestore); test_missing_output_option!(ThinRestore); test_tiny_output_file!(ThinRestore); +test_unwritable_output_file!(ThinRestore); + //----------------------------------------- // TODO: share with cache_restore, era_restore From ea8dcb54d0424b672ff35fc3e5e0494bd4c2787e Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 31 Aug 2021 00:35:48 +0800 Subject: [PATCH 03/21] [tests] Fix the file size for testing --- tests/common/output_option.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/common/output_option.rs b/tests/common/output_option.rs index 57028f1..5a627c4 100644 --- a/tests/common/output_option.rs +++ b/tests/common/output_option.rs @@ -81,7 +81,7 @@ where let input = P::mk_valid_input(&mut td)?; let output = td.mk_path("meta.bin"); - let _file = file_utils::create_sized_file(&output, 4096); + let _file = file_utils::create_sized_file(&output, 4_194_304); duct::cmd!("chmod", "-w", &output).run()?; let stderr = run_fail(P::path(), args!["-i", &input, "-o", &output])?; From aaa84aa35adbd80cdfd3d63e037af5f6e2a1fe7f Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 31 Aug 2021 10:04:48 +0800 Subject: [PATCH 04/21] [tests] Enable tests for rust thin_repair --- tests/common/target.rs | 2 +- tests/thin_repair.rs | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/common/target.rs b/tests/common/target.rs index 65885bc..79b742b 100644 --- a/tests/common/target.rs +++ b/tests/common/target.rs @@ -40,7 +40,7 @@ pub const THIN_DELTA: &str = path_to_cpp!("thin_delta"); // TODO: rust version pub const THIN_DUMP: &str = path_to!("thin_dump"); pub const THIN_METADATA_PACK: &str = path_to_rust!("thin_metadata_pack"); // rust-only pub const THIN_METADATA_UNPACK: &str = path_to_rust!("thin_metadata_unpack"); // rust-only -pub const THIN_REPAIR: &str = path_to_cpp!("thin_repair"); // TODO: rust version +pub const THIN_REPAIR: &str = path_to!("thin_repair"); pub const THIN_RESTORE: &str = path_to!("thin_restore"); pub const THIN_RMAP: &str = path_to_cpp!("thin_rmap"); // TODO: rust version pub const THIN_GENERATE_METADATA: &str = path_to_cpp!("thin_generate_metadata"); // cpp-only diff --git a/tests/thin_repair.rs b/tests/thin_repair.rs index eb3ac2d..da54a7a 100644 --- a/tests/thin_repair.rs +++ b/tests/thin_repair.rs @@ -46,7 +46,7 @@ impl<'a> Program<'a> for ThinRepair { } fn bad_option_hint(option: &str) -> String { - cpp_msg::bad_option_hint(option) + msg::bad_option_hint(option) } } @@ -56,27 +56,33 @@ impl<'a> InputProgram<'a> for ThinRepair { } fn file_not_found() -> &'a str { - cpp_msg::FILE_NOT_FOUND + msg::FILE_NOT_FOUND } fn missing_input_arg() -> &'a str { - cpp_msg::MISSING_INPUT_ARG + msg::MISSING_INPUT_ARG } + #[cfg(not(feature = "rust_tests"))] fn corrupted_input() -> &'a str { "The following field needs to be provided on the command line due to corruption in the superblock" } + + #[cfg(feature = "rust_tests")] + fn corrupted_input() -> &'a str { + "data block size needs to be provided due to corruption in the superblock" + } } impl<'a> OutputProgram<'a> for ThinRepair { fn missing_output_arg() -> &'a str { - cpp_msg::MISSING_OUTPUT_ARG + msg::MISSING_OUTPUT_ARG } } impl<'a> MetadataWriter<'a> for ThinRepair { fn file_not_found() -> &'a str { - cpp_msg::FILE_NOT_FOUND + msg::FILE_NOT_FOUND } } From 0acc57d17f4e00f55d8c1941648bc73e2b438c7a Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 31 Aug 2021 12:33:12 +0800 Subject: [PATCH 05/21] [thin_restore (rust)] Fix errors in tests - Move error messages to stderr (fixes unwritable_output_file, input_file_not_found, and missing_input_option) - Validate XML structures implicitly (fixes corrupted_input_data) - Check output file size (fixes tiny_output_file) - Provide XML error context - Fix mk_valid_input() --- src/bin/thin_restore.rs | 7 +++++- src/file_utils.rs | 10 +++++++++ src/thin/restore.rs | 50 ++++++++++++++++++++++++++++++++++++----- src/thin/xml.rs | 20 ++++++++++------- src/xml.rs | 4 ++-- tests/thin_restore.rs | 2 +- 6 files changed, 75 insertions(+), 18 deletions(-) diff --git a/src/bin/thin_restore.rs b/src/bin/thin_restore.rs index c221326..2201a83 100644 --- a/src/bin/thin_restore.rs +++ b/src/bin/thin_restore.rs @@ -62,6 +62,11 @@ fn main() { exit(1); } + if let Err(e) = file_utils::check_output_file_requirements(output_file) { + eprintln!("{}", e); + exit(1); + } + let report; if matches.is_present("QUIET") { @@ -80,7 +85,7 @@ fn main() { }; if let Err(reason) = restore(opts) { - println!("{}", reason); + eprintln!("{}", reason); process::exit(1); } } diff --git a/src/file_utils.rs b/src/file_utils.rs index 85f0714..a76aa4a 100644 --- a/src/file_utils.rs +++ b/src/file_utils.rs @@ -98,3 +98,13 @@ pub fn create_sized_file(path: &Path, nr_bytes: u64) -> io::Result io::Result<()> { + // minimal thin metadata size is 10 blocks, with one device + if file_size(path)? < 40960 { + return fail("Output file too small."); + } + Ok(()) +} + +//--------------------------------------- diff --git a/src/thin/restore.rs b/src/thin/restore.rs index 078a3e3..38380fa 100644 --- a/src/thin/restore.rs +++ b/src/thin/restore.rs @@ -57,6 +57,15 @@ impl std::fmt::Display for MappedSection { //------------------------------------------ +#[derive(PartialEq)] +enum Section { + None, + Superblock, + Device, + Def, + Finalized, +} + pub struct Restorer<'a> { w: &'a mut WriteBatcher, report: Arc, @@ -71,6 +80,7 @@ pub struct Restorer<'a> { sb: Option, devices: BTreeMap, data_sm: Option>>, + in_section: Section, } impl<'a> Restorer<'a> { @@ -84,6 +94,7 @@ impl<'a> Restorer<'a> { sb: None, devices: BTreeMap::new(), data_sm: None, + in_section: Section::None, } } @@ -149,6 +160,13 @@ impl<'a> Restorer<'a> { } fn finalize(&mut self) -> Result<()> { + let src_sb; + if let Some(sb) = self.sb.take() { + src_sb = sb; + } else { + return Err(anyhow!("missing superblock")); + } + let (details_root, mapping_root) = self.build_device_details()?; self.release_subtrees()?; @@ -162,33 +180,41 @@ impl<'a> Restorer<'a> { let metadata_sm_root = pack_root(&metadata_sm, SPACE_MAP_ROOT_SIZE)?; // Write the superblock - let sb = self.sb.as_ref().unwrap(); let sb = superblock::Superblock { flags: SuperblockFlags { needs_check: false }, block: SUPERBLOCK_LOCATION, version: 2, - time: sb.time as u32, - transaction_id: sb.transaction, + time: src_sb.time as u32, + transaction_id: src_sb.transaction, metadata_snap: 0, data_sm_root, metadata_sm_root, mapping_root, details_root, - data_block_size: sb.data_block_size, + data_block_size: src_sb.data_block_size, nr_metadata_blocks: metadata_sm.nr_blocks, }; - write_superblock(self.w.engine.as_ref(), SUPERBLOCK_LOCATION, &sb) + write_superblock(self.w.engine.as_ref(), SUPERBLOCK_LOCATION, &sb)?; + self.in_section = Section::Finalized; + + Ok(()) } } impl<'a> MetadataVisitor for Restorer<'a> { fn superblock_b(&mut self, sb: &ir::Superblock) -> Result { + if self.in_section != Section::None { + return Err(anyhow!("duplicated superblock")); + } + self.sb = Some(sb.clone()); self.data_sm = Some(core_sm(sb.nr_data_blocks, u32::MAX)); let b = self.w.alloc()?; if b.loc != SUPERBLOCK_LOCATION { return Err(anyhow!("superblock was occupied")); } + self.in_section = Section::Superblock; + Ok(Visit::Continue) } @@ -198,12 +224,17 @@ impl<'a> MetadataVisitor for Restorer<'a> { } fn def_shared_b(&mut self, name: &str) -> Result { + if self.in_section != Section::Superblock { + return Err(anyhow!("missing superblock")); + } + self.in_section = Section::Def; self.begin_section(MappedSection::Def(name.to_string())) } fn def_shared_e(&mut self) -> Result { if let (MappedSection::Def(name), nodes) = self.end_section()? { self.sub_trees.insert(name, nodes); + self.in_section = Section::Superblock; Ok(Visit::Continue) } else { Err(anyhow!("unexpected ")) @@ -211,6 +242,9 @@ impl<'a> MetadataVisitor for Restorer<'a> { } fn device_b(&mut self, d: &ir::Device) -> Result { + if self.in_section != Section::Superblock { + return Err(anyhow!("missing superblock")); + } self.report .info(&format!("building btree for device {}", d.dev_id)); self.current_dev = Some(DeviceDetail { @@ -219,6 +253,7 @@ impl<'a> MetadataVisitor for Restorer<'a> { creation_time: d.creation_time as u32, snapshotted_time: d.snap_time as u32, }); + self.in_section = Section::Device; self.begin_section(MappedSection::Dev(d.dev_id)) } @@ -227,6 +262,7 @@ impl<'a> MetadataVisitor for Restorer<'a> { if let (MappedSection::Dev(thin_id), nodes) = self.end_section()? { let root = build_btree(self.w, nodes)?; self.devices.insert(thin_id, (detail, root)); + self.in_section = Section::Superblock; Ok(Visit::Continue) } else { Err(anyhow!("internal error, couldn't find device details")) @@ -278,7 +314,9 @@ impl<'a> MetadataVisitor for Restorer<'a> { } fn eof(&mut self) -> Result { - // FIXME: build the rest of the device trees + if self.in_section != Section::Finalized { + return Err(anyhow!("incomplete source metadata")); + } Ok(Visit::Continue) } } diff --git a/src/thin/xml.rs b/src/thin/xml.rs index 42bfe0d..5063b73 100644 --- a/src/thin/xml.rs +++ b/src/thin/xml.rs @@ -1,4 +1,4 @@ -use anyhow::Result; +use anyhow::{anyhow, Result}; use std::{io::prelude::*, io::BufReader, io::Write}; use quick_xml::events::{BytesEnd, BytesStart, Event}; @@ -271,19 +271,19 @@ where b"superblock" => visitor.superblock_b(&parse_superblock(e)?), b"device" => visitor.device_b(&parse_device(e)?), b"def" => visitor.def_shared_b(&parse_def(e, "def")?), - _ => todo!(), + _ => return Err(anyhow!("Parse error at byte {}", reader.buffer_position())), }, Ok(Event::End(ref e)) => match e.name() { b"superblock" => visitor.superblock_e(), b"device" => visitor.device_e(), b"def" => visitor.def_shared_e(), - _ => todo!(), + _ => return Err(anyhow!("Parse error at byte {}", reader.buffer_position())), }, Ok(Event::Empty(ref e)) => match e.name() { b"single_mapping" => visitor.map(&parse_single_map(e)?), b"range_mapping" => visitor.map(&parse_range_map(e)?), b"ref" => visitor.ref_shared(&parse_def(e, "ref")?), - _ => todo!(), + _ => return Err(anyhow!("Parse error at byte {}", reader.buffer_position())), }, Ok(Event::Text(_)) => Ok(Visit::Continue), Ok(Event::Comment(_)) => Ok(Visit::Continue), @@ -291,10 +291,14 @@ where visitor.eof()?; Ok(Visit::Stop) } - Ok(_) => todo!(), - - // FIXME: don't panic! - Err(e) => panic!("error parsing xml {:?}", e), + Ok(_) => return Err(anyhow!("Parse error at byte {}", reader.buffer_position())), + Err(e) => { + return Err(anyhow!( + "Parse error at byte {}: {:?}", + reader.buffer_position(), + e + )) + } } } diff --git a/src/xml.rs b/src/xml.rs index 577c0e3..2b617bf 100644 --- a/src/xml.rs +++ b/src/xml.rs @@ -32,8 +32,8 @@ pub fn bool_val(kv: &Attribute) -> anyhow::Result { Ok(n) } -pub fn bad_attr(_tag: &str, _attr: &[u8]) -> anyhow::Result { - todo!(); +pub fn bad_attr(tag: &str, _attr: &[u8]) -> anyhow::Result { + Err(anyhow!("unknown attribute in tag '{}'", tag)) } pub fn check_attr(tag: &str, name: &str, maybe_v: Option) -> anyhow::Result { diff --git a/tests/thin_restore.rs b/tests/thin_restore.rs index b908566..e1ec504 100644 --- a/tests/thin_restore.rs +++ b/tests/thin_restore.rs @@ -53,7 +53,7 @@ impl<'a> Program<'a> for ThinRestore { impl<'a> InputProgram<'a> for ThinRestore { fn mk_valid_input(td: &mut TestDir) -> Result { - mk_valid_md(td) + mk_valid_xml(td) } fn file_not_found() -> &'a str { From 9253117132ac07d6f84b6b47ea746a083639a348 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 31 Aug 2021 22:28:12 +0800 Subject: [PATCH 06/21] [tests] Add cache fixtures - Generate temp xml and metadata files - Correct existing tests to use cache fixtures - Fix cache xml generation --- tests/cache_check.rs | 3 ++- tests/cache_dump.rs | 4 ++-- tests/common/cache.rs | 35 +++++++++++++++++++++++++++++ tests/common/cache_xml_generator.rs | 16 ++++++------- tests/common/mod.rs | 1 + 5 files changed, 48 insertions(+), 11 deletions(-) create mode 100644 tests/common/cache.rs diff --git a/tests/cache_check.rs b/tests/cache_check.rs index 3aa724f..b676742 100644 --- a/tests/cache_check.rs +++ b/tests/cache_check.rs @@ -2,6 +2,7 @@ use anyhow::Result; mod common; +use common::cache::*; use common::common_args::*; use common::fixture::*; use common::input_arg::*; @@ -51,7 +52,7 @@ impl<'a> Program<'a> for CacheCheck { impl<'a> InputProgram<'a> for CacheCheck { fn mk_valid_input(td: &mut TestDir) -> Result { - common::thin::mk_valid_md(td) // FIXME: create cache metadata + mk_valid_md(td) } fn file_not_found() -> &'a str { diff --git a/tests/cache_dump.rs b/tests/cache_dump.rs index f1b0f25..da85b5c 100644 --- a/tests/cache_dump.rs +++ b/tests/cache_dump.rs @@ -2,9 +2,9 @@ use anyhow::Result; mod common; +use common::cache::*; use common::common_args::*; use common::input_arg::*; - use common::program::*; use common::target::*; use common::test_dir::*; @@ -46,7 +46,7 @@ impl<'a> Program<'a> for CacheDump { impl<'a> InputProgram<'a> for CacheDump { fn mk_valid_input(td: &mut TestDir) -> Result { - common::thin::mk_valid_md(td) // FIXME: generate cache metadata + mk_valid_md(td) } fn file_not_found() -> &'a str { diff --git a/tests/common/cache.rs b/tests/common/cache.rs new file mode 100644 index 0000000..449f00f --- /dev/null +++ b/tests/common/cache.rs @@ -0,0 +1,35 @@ +use anyhow::Result; +use std::path::PathBuf; + +use thinp::file_utils; +//use thinp::io_engine::*; + +use crate::args; +use crate::common::cache_xml_generator::{write_xml, CacheGen}; +use crate::common::process::*; +use crate::common::target::*; +use crate::common::test_dir::TestDir; + +//----------------------------------------------- + +pub fn mk_valid_xml(td: &mut TestDir) -> Result { + let xml = td.mk_path("meta.xml"); + let mut gen = CacheGen::new(512, 128, 1024, 80, 50); // bs, cblocks, oblocks, res, dirty + write_xml(&xml, &mut gen)?; + Ok(xml) +} + +pub fn mk_valid_md(td: &mut TestDir) -> Result { + let xml = td.mk_path("meta.xml"); + let md = td.mk_path("meta.bin"); + + let mut gen = CacheGen::new(512, 4096, 32768, 80, 50); + write_xml(&xml, &mut gen)?; + + let _file = file_utils::create_sized_file(&md, 4096 * 4096); + run_ok(CACHE_RESTORE, args!["-i", &xml, "-o", &md])?; + + Ok(md) +} + +//----------------------------------------------- diff --git a/tests/common/cache_xml_generator.rs b/tests/common/cache_xml_generator.rs index e7df242..27c9c6f 100644 --- a/tests/common/cache_xml_generator.rs +++ b/tests/common/cache_xml_generator.rs @@ -60,26 +60,26 @@ impl XmlGen for CacheGen { hint_width: 4, })?; - let mut cblocks = Vec::new(); - for n in 0..self.nr_cache_blocks { - cblocks.push(n); - } + let nr_resident = (self.nr_cache_blocks * self.percent_resident as u32) / 100u32; + let mut cblocks = (0..self.nr_cache_blocks).collect::>(); cblocks.shuffle(&mut rand::thread_rng()); + cblocks.truncate(nr_resident as usize); + cblocks.sort(); v.mappings_b()?; { - let nr_resident = (self.nr_cache_blocks * 100u32) / (self.percent_resident as u32); let mut used = HashSet::new(); - for n in 0..nr_resident { + let mut rng = rand::thread_rng(); + for cblock in cblocks { let mut oblock = 0u64; while used.contains(&oblock) { - oblock = rand::thread_rng().gen(); + oblock = rng.gen_range(0..self.nr_origin_blocks); } used.insert(oblock); // FIXME: dirty should vary v.mapping(&ir::Map { - cblock: cblocks[n as usize], + cblock, oblock, dirty: false, })?; diff --git a/tests/common/mod.rs b/tests/common/mod.rs index a8e1717..93f6db4 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -2,6 +2,7 @@ // https://github.com/rust-lang/rust/issues/46379 #![allow(dead_code)] +pub mod cache; pub mod cache_xml_generator; pub mod common_args; pub mod fixture; From 59e44667a9e3335ba73e471f73c537c79c3b36cf Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Wed, 1 Sep 2021 11:39:11 +0800 Subject: [PATCH 07/21] [tests] Port cache_restore tests --- functional-tests/cache-functional-tests.scm | 96 ----------- tests/cache_restore.rs | 167 ++++++++++++++++++++ tests/common/target.rs | 1 + 3 files changed, 168 insertions(+), 96 deletions(-) create mode 100644 tests/cache_restore.rs diff --git a/functional-tests/cache-functional-tests.scm b/functional-tests/cache-functional-tests.scm index b8e2f41..42d3713 100644 --- a/functional-tests/cache-functional-tests.scm +++ b/functional-tests/cache-functional-tests.scm @@ -49,102 +49,6 @@ ;; to run. (define (register-cache-tests) #t) - ;;;----------------------------------------------------------- - ;;; cache_restore scenarios - ;;;----------------------------------------------------------- - - (define-scenario (cache-restore v) - "print version (-V flag)" - (run-ok-rcv (stdout _) (cache-restore "-V") - (assert-equal tools-version stdout))) - - (define-scenario (cache-restore version) - "print version (--version flags)" - (run-ok-rcv (stdout _) (cache-restore "--version") - (assert-equal tools-version stdout))) - - (define-scenario (cache-restore h) - "cache_restore -h" - (run-ok-rcv (stdout _) (cache-restore "-h") - (assert-equal cache-restore-help stdout))) - - (define-scenario (cache-restore help) - "cache_restore --help" - (run-ok-rcv (stdout _) (cache-restore "--help") - (assert-equal cache-restore-help stdout))) - - (define-scenario (cache-restore no-input-file) - "forget to specify an input file" - (with-empty-metadata (md) - (run-fail-rcv (_ stderr) (cache-restore "-o" md) - (assert-starts-with "No input file provided." stderr)))) - - (define-scenario (cache-restore missing-input-file) - "the input file can't be found" - (with-empty-metadata (md) - (let ((bad-path "no-such-file")) - (run-fail-rcv (_ stderr) (cache-restore "-i" bad-path "-o" md) - (assert-superblock-all-zeroes md) - (assert-starts-with - (string-append bad-path ": No such file or directory") - stderr))))) - - (define-scenario (cache-restore garbage-input-file) - "the input file is just zeroes" - (with-empty-metadata (md) - (with-temp-file-sized ((xml "cache.xml" 4096)) - (run-fail-rcv (_ stderr) (cache-restore "-i" xml "-o" md) - (assert-superblock-all-zeroes md))))) - - (define-scenario (cache-restore missing-output-file) - "the output file can't be found" - (with-cache-xml (xml) - (run-fail-rcv (_ stderr) (cache-restore "-i" xml) - (assert-starts-with "No output file provided." stderr)))) - - (define-scenario (cache-restore tiny-output-file) - "Fails if the output file is too small." - (with-temp-file-sized ((md "cache.bin" (* 1024 4))) - (with-cache-xml (xml) - (run-fail-rcv (_ stderr) (cache-restore "-i" xml "-o" md) - (assert-starts-with cache-restore-outfile-too-small-text stderr))))) - - (define-scenario (cache-restore successfully-restores) - "Restore succeeds." - (with-empty-metadata (md) - (with-cache-xml (xml) - (run-ok (cache-restore "-i" xml "-o" md))))) - - (define-scenario (cache-restore q) - "cache_restore accepts -q" - (with-empty-metadata (md) - (with-cache-xml (xml) - (run-ok-rcv (stdout stderr) (cache-restore "-i" xml "-o" md "-q") - (assert-eof stdout) - (assert-eof stderr))))) - - (define-scenario (cache-restore quiet) - "cache_restore accepts --quiet" - (with-empty-metadata (md) - (with-cache-xml (xml) - (run-ok-rcv (stdout stderr) (cache-restore "-i" xml "-o" md "--quiet") - (assert-eof stdout) - (assert-eof stderr))))) - - (define-scenario (cache-restore override-metadata-version) - "we can set any metadata version" - (with-empty-metadata (md) - (with-cache-xml (xml) - (run-ok - (cache-restore "-i" xml "-o" md "--debug-override-metadata-version 10298"))))) - - (define-scenario (cache-restore omit-clean-shutdown) - "accepts --omit-clean-shutdown" - (with-empty-metadata (md) - (with-cache-xml (xml) - (run-ok - (cache-restore "-i" xml "-o" md "--omit-clean-shutdown"))))) - ;;;----------------------------------------------------------- ;;; cache_dump scenarios ;;;----------------------------------------------------------- diff --git a/tests/cache_restore.rs b/tests/cache_restore.rs new file mode 100644 index 0000000..41c133a --- /dev/null +++ b/tests/cache_restore.rs @@ -0,0 +1,167 @@ +use anyhow::Result; + +mod common; + +use common::cache::*; +use common::common_args::*; +use common::fixture::*; +use common::input_arg::*; +use common::output_option::*; +use common::process::*; +use common::program::*; +use common::target::*; +use common::test_dir::*; + +//------------------------------------------ + +const USAGE: &str = "Usage: cache_restore [options]\n\ + Options:\n \ + {-h|--help}\n \ + {-i|--input} \n \ + {-o|--output} \n \ + {-q|--quiet}\n \ + {--metadata-version} <1 or 2>\n \ + {-V|--version}\n\ + \n \ + {--debug-override-metadata-version} \n \ + {--omit-clean-shutdown}"; + +//------------------------------------------ + +struct CacheRestore; + +impl<'a> Program<'a> for CacheRestore { + fn name() -> &'a str { + "thin_restore" + } + + fn path() -> &'a std::ffi::OsStr { + CACHE_RESTORE.as_ref() + } + + fn usage() -> &'a str { + USAGE + } + + fn arg_type() -> ArgType { + ArgType::IoOptions + } + + fn bad_option_hint(option: &str) -> String { + msg::bad_option_hint(option) + } +} + +impl<'a> InputProgram<'a> for CacheRestore { + fn mk_valid_input(td: &mut TestDir) -> Result { + mk_valid_xml(td) + } + + fn file_not_found() -> &'a str { + msg::FILE_NOT_FOUND + } + + fn missing_input_arg() -> &'a str { + msg::MISSING_INPUT_ARG + } + + fn corrupted_input() -> &'a str { + "" // we don't intent to verify error messages of XML parsing + } +} + +impl<'a> OutputProgram<'a> for CacheRestore { + fn missing_output_arg() -> &'a str { + msg::MISSING_OUTPUT_ARG + } +} + +impl<'a> MetadataWriter<'a> for CacheRestore { + fn file_not_found() -> &'a str { + msg::FILE_NOT_FOUND + } +} + +//----------------------------------------- + +test_accepts_help!(CacheRestore); +test_accepts_version!(CacheRestore); + +test_missing_input_option!(CacheRestore); +test_input_file_not_found!(CacheRestore); +test_corrupted_input_data!(CacheRestore); + +test_missing_output_option!(CacheRestore); +test_tiny_output_file!(CacheRestore); + +test_unwritable_output_file!(CacheRestore); + +//----------------------------------------- + +// TODO: share with thin_restore, era_restore + +fn quiet_flag(flag: &str) -> Result<()> { + let mut td = TestDir::new()?; + let xml = mk_valid_xml(&mut td)?; + let md = mk_zeroed_md(&mut td)?; + + let output = run_ok_raw(CACHE_RESTORE, args!["-i", &xml, "-o", &md, flag])?; + + assert_eq!(output.stdout.len(), 0); + assert_eq!(output.stderr.len(), 0); + Ok(()) +} + +#[test] +fn accepts_q() -> Result<()> { + quiet_flag("-q") +} + +#[test] +fn accepts_quiet() -> Result<()> { + quiet_flag("--quiet") +} + +//----------------------------------------- + +#[test] +fn successfully_restores() -> Result<()> { + let mut td = TestDir::new()?; + let xml = mk_valid_xml(&mut td)?; + let md = mk_zeroed_md(&mut td)?; + run_ok(CACHE_RESTORE, args!["-i", &xml, "-o", &md])?; + Ok(()) +} + +#[test] +fn override_metadata_version() -> Result<()> { + let mut td = TestDir::new()?; + let xml = mk_valid_xml(&mut td)?; + let md = mk_zeroed_md(&mut td)?; + run_ok( + CACHE_RESTORE, + args![ + "-i", + &xml, + "-o", + &md, + "--debug-override-metadata-version", + "10298" + ], + )?; + Ok(()) +} + +#[test] +fn accepts_omit_clean_shutdown() -> Result<()> { + let mut td = TestDir::new()?; + let xml = mk_valid_xml(&mut td)?; + let md = mk_zeroed_md(&mut td)?; + run_ok( + CACHE_RESTORE, + args!["-i", &xml, "-o", &md, "--omit-clean-shutdown"], + )?; + Ok(()) +} + +//----------------------------------------- diff --git a/tests/common/target.rs b/tests/common/target.rs index 79b742b..53fd8af 100644 --- a/tests/common/target.rs +++ b/tests/common/target.rs @@ -34,6 +34,7 @@ macro_rules! path_to { pub const CACHE_CHECK: &str = path_to!("cache_check"); pub const CACHE_DUMP: &str = path_to!("cache_dump"); +pub const CACHE_RESTORE: &str = path_to!("cache_restore"); pub const THIN_CHECK: &str = path_to!("thin_check"); pub const THIN_DELTA: &str = path_to_cpp!("thin_delta"); // TODO: rust version From 4ed2348b3664c8eacabcf1a76a8c8eb4bebc11c0 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Wed, 1 Sep 2021 15:20:13 +0800 Subject: [PATCH 08/21] [cache_restore (rust)] Fix errors in tests - Move error messages to stderr (fixes unwritable_output_file, input_file_not_found, and missing_input_option) - Validate XML structures implicitly (fixes corrupted_input_data) - Check output file size (fixes tiny_output_file) - Allow restoring XML without the hints (for creating test fixtures) - Provide XML error context - Remove unused option --- src/bin/cache_restore.rs | 14 +++-- src/cache/restore.rs | 109 +++++++++++++++++++++++++++------------ src/cache/xml.rs | 36 +++++++++++-- 3 files changed, 114 insertions(+), 45 deletions(-) diff --git a/src/bin/cache_restore.rs b/src/bin/cache_restore.rs index 714d2d4..1cbd3ae 100644 --- a/src/bin/cache_restore.rs +++ b/src/bin/cache_restore.rs @@ -22,13 +22,6 @@ fn main() { .long("async-io") .hidden(true), ) - .arg( - Arg::with_name("OVERRIDE_MAPPING_ROOT") - .help("Specify a mapping root to use") - .long("override-mapping-root") - .value_name("OVERRIDE_MAPPING_ROOT") - .takes_value(true), - ) .arg( Arg::with_name("QUIET") .help("Suppress output messages, return only exit code.") @@ -62,6 +55,11 @@ fn main() { exit(1); } + if let Err(e) = file_utils::check_output_file_requirements(output_file) { + eprintln!("{}", e); + exit(1); + } + let report; if matches.is_present("QUIET") { @@ -80,7 +78,7 @@ fn main() { }; if let Err(reason) = restore(opts) { - println!("{}", reason); + eprintln!("{}", reason); process::exit(1); } } diff --git a/src/cache/restore.rs b/src/cache/restore.rs index 45000a1..07970c7 100644 --- a/src/cache/restore.rs +++ b/src/cache/restore.rs @@ -1,4 +1,4 @@ -use anyhow::Result; +use anyhow::{anyhow, Result}; use std::convert::TryInto; use std::fs::OpenOptions; @@ -54,6 +54,15 @@ fn mk_context(opts: &CacheRestoreOptions) -> anyhow::Result { //------------------------------------------ +#[derive(PartialEq)] +enum Section { + None, + Superblock, + Mappings, + Hints, + Finalized, +} + pub struct Restorer<'a> { write_batcher: &'a mut WriteBatcher, sb: Option, @@ -64,7 +73,8 @@ pub struct Restorer<'a> { dirty_root: Option, hint_root: Option, discard_root: Option, - dirty_bits: (u32, u64), + dirty_bits: (u32, u64), // (index in u64 array, value) + in_section: Section, } impl<'a> Restorer<'a> { @@ -80,14 +90,43 @@ impl<'a> Restorer<'a> { hint_root: None, discard_root: None, dirty_bits: (0, 0), + in_section: Section::None, } } fn finalize(&mut self) -> Result<()> { + let src_sb; + if let Some(sb) = self.sb.take() { + src_sb = sb; + } else { + return Err(anyhow!("not in superblock")); + } + + // complete the mapping array + if let Some(builder) = self.mapping_builder.take() { + self.mapping_root = Some(builder.complete(self.write_batcher)?); + } + + // complete the dirty array + if let Some(mut builder) = self.dirty_builder.take() { + // push the bufferred trailing bits + builder.push_value( + self.write_batcher, + self.dirty_bits.0 as u64, + self.dirty_bits.1, + )?; + + self.dirty_root = Some(builder.complete(self.write_batcher)?); + } + + // complete the hint array + if let Some(builder) = self.hint_builder.take() { + self.hint_root = Some(builder.complete(self.write_batcher)?); + } + // build metadata space map let metadata_sm_root = build_metadata_sm(self.write_batcher)?; - let sb = self.sb.as_ref().unwrap(); let mapping_root = self.mapping_root.as_ref().unwrap(); let hint_root = self.hint_root.as_ref().unwrap(); let discard_root = self.discard_root.as_ref().unwrap(); @@ -98,9 +137,9 @@ impl<'a> Restorer<'a> { }, block: SUPERBLOCK_LOCATION, version: 2, - policy_name: sb.policy.as_bytes().to_vec(), + policy_name: src_sb.policy.as_bytes().to_vec(), policy_version: vec![2, 0, 0], - policy_hint_size: sb.hint_width, + policy_hint_size: src_sb.hint_width, metadata_sm_root, mapping_root: *mapping_root, dirty_root: self.dirty_root, // dirty_root is optional @@ -108,8 +147,8 @@ impl<'a> Restorer<'a> { discard_root: *discard_root, discard_block_size: 0, discard_nr_blocks: 0, - data_block_size: sb.block_size, - cache_blocks: sb.nr_cache_blocks, + data_block_size: src_sb.block_size, + cache_blocks: src_sb.nr_cache_blocks, compat_flags: 0, compat_ro_flags: 0, incompat_flags: 0, @@ -118,20 +157,32 @@ impl<'a> Restorer<'a> { write_hits: 0, write_misses: 0, }; - write_superblock(self.write_batcher.engine.as_ref(), SUPERBLOCK_LOCATION, &sb) + write_superblock(self.write_batcher.engine.as_ref(), SUPERBLOCK_LOCATION, &sb)?; + + self.in_section = Section::Finalized; + Ok(()) } } impl<'a> MetadataVisitor for Restorer<'a> { fn superblock_b(&mut self, sb: &ir::Superblock) -> Result { + if self.in_section != Section::None { + return Err(anyhow!("duplicated superblock")); + } + self.sb = Some(sb.clone()); - self.write_batcher.alloc()?; + let b = self.write_batcher.alloc()?; + if b.loc != SUPERBLOCK_LOCATION { + return Err(anyhow!("superblock was occupied")); + } + self.mapping_builder = Some(ArrayBuilder::new(sb.nr_cache_blocks as u64)); self.dirty_builder = Some(ArrayBuilder::new(div_up(sb.nr_cache_blocks as u64, 64))); self.hint_builder = Some(ArrayBuilder::new(sb.nr_cache_blocks as u64)); let discard_builder = ArrayBuilder::::new(0); // discard bitset is optional self.discard_root = Some(discard_builder.complete(self.write_batcher)?); + self.in_section = Section::Superblock; Ok(Visit::Continue) } @@ -142,30 +193,18 @@ impl<'a> MetadataVisitor for Restorer<'a> { } fn mappings_b(&mut self) -> Result { + if self.in_section != Section::Superblock { + return Err(anyhow!("not in superblock")); + } + self.in_section = Section::Mappings; Ok(Visit::Continue) } fn mappings_e(&mut self) -> Result { - let mut mapping_builder = None; - std::mem::swap(&mut self.mapping_builder, &mut mapping_builder); - if let Some(builder) = mapping_builder { - self.mapping_root = Some(builder.complete(self.write_batcher)?); + if self.in_section != Section::Mappings { + return Err(anyhow!("not in mappings")); } - - // push the bufferred trailing bits - let b = self.dirty_builder.as_mut().unwrap(); - b.push_value( - self.write_batcher, - self.dirty_bits.0 as u64, - self.dirty_bits.1, - )?; - - let mut dirty_builder = None; - std::mem::swap(&mut self.dirty_builder, &mut dirty_builder); - if let Some(builder) = dirty_builder { - self.dirty_root = Some(builder.complete(self.write_batcher)?); - } - + self.in_section = Section::Superblock; Ok(Visit::Continue) } @@ -198,15 +237,18 @@ impl<'a> MetadataVisitor for Restorer<'a> { } fn hints_b(&mut self) -> Result { + if self.in_section != Section::Superblock { + return Err(anyhow!("not in superblock")); + } + self.in_section = Section::Hints; Ok(Visit::Continue) } fn hints_e(&mut self) -> Result { - let mut hint_builder = None; - std::mem::swap(&mut self.hint_builder, &mut hint_builder); - if let Some(builder) = hint_builder { - self.hint_root = Some(builder.complete(self.write_batcher)?); + if self.in_section != Section::Hints { + return Err(anyhow!("not in hints")); } + self.in_section = Section::Superblock; Ok(Visit::Continue) } @@ -232,6 +274,9 @@ impl<'a> MetadataVisitor for Restorer<'a> { } fn eof(&mut self) -> Result { + if self.in_section != Section::Finalized { + return Err(anyhow!("incompleted source metadata")); + } Ok(Visit::Continue) } } diff --git a/src/cache/xml.rs b/src/cache/xml.rs index f171151..b323c91 100644 --- a/src/cache/xml.rs +++ b/src/cache/xml.rs @@ -202,18 +202,33 @@ where b"superblock" => visitor.superblock_b(&parse_superblock(e)?), b"mappings" => visitor.mappings_b(), b"hints" => visitor.hints_b(), - _ => todo!(), + _ => { + return Err(anyhow!( + "Parse error 1 at byte {}", + reader.buffer_position() + )) + } }, Ok(Event::End(ref e)) => match e.name() { b"superblock" => visitor.superblock_e(), b"mappings" => visitor.mappings_e(), b"hints" => visitor.hints_e(), - _ => todo!(), + _ => { + return Err(anyhow!( + "Parse error 2 at byte {}", + reader.buffer_position() + )) + } }, Ok(Event::Empty(ref e)) => match e.name() { b"mapping" => visitor.mapping(&parse_mapping(e)?), b"hint" => visitor.hint(&parse_hint(e)?), - _ => todo!(), + _ => { + return Err(anyhow!( + "Parse error 3 at byte {}", + reader.buffer_position() + )) + } }, Ok(Event::Text(_)) => Ok(Visit::Continue), Ok(Event::Comment(_)) => Ok(Visit::Continue), @@ -221,8 +236,19 @@ where visitor.eof()?; Ok(Visit::Stop) } - Ok(_) => todo!(), - Err(e) => Err(anyhow!("{:?}", e)), + Ok(_) => { + return Err(anyhow!( + "Parse error 4 at byte {}", + reader.buffer_position() + )) + } + Err(e) => { + return Err(anyhow!( + "Parse error 5 at byte {}: {:?}", + reader.buffer_position(), + e + )) + } } } From 2f22a8c55d1e8798a2b0321f5b1b103546e7e104 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 7 Sep 2021 00:01:37 +0800 Subject: [PATCH 09/21] [all (rust)] Fix errors in testing input option - Fix file mode bits checking - Return error reason from stat --- src/bin/cache_check.rs | 9 ++++++- src/bin/cache_dump.rs | 9 ++++++- src/bin/cache_repair.rs | 7 +++-- src/bin/cache_restore.rs | 9 +++---- src/bin/thin_check.rs | 7 +++-- src/bin/thin_dump.rs | 13 +++++---- src/bin/thin_metadata_pack.rs | 4 +-- src/bin/thin_metadata_unpack.rs | 4 +-- src/bin/thin_repair.rs | 13 +++++---- src/bin/thin_restore.rs | 9 +++---- src/bin/thin_shrink.rs | 4 +-- src/file_utils.rs | 48 ++++++++++++++++++++++++--------- tests/common/target.rs | 2 +- 13 files changed, 85 insertions(+), 53 deletions(-) diff --git a/src/bin/cache_check.rs b/src/bin/cache_check.rs index 34fe874..e85057b 100644 --- a/src/bin/cache_check.rs +++ b/src/bin/cache_check.rs @@ -4,9 +4,11 @@ extern crate thinp; use atty::Stream; use clap::{App, Arg}; use std::path::Path; +use std::process; use std::sync::Arc; use thinp::cache::check::{check, CacheCheckOptions}; +use thinp::file_utils; use thinp::report::*; //------------------------------------------ @@ -68,6 +70,11 @@ fn main() { let matches = parser.get_matches(); let input_file = Path::new(matches.value_of("INPUT").unwrap()); + if let Err(e) = file_utils::is_file_or_blk(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); + process::exit(1); + } + let report; if matches.is_present("QUIET") { report = std::sync::Arc::new(mk_quiet_report()); @@ -91,7 +98,7 @@ fn main() { if let Err(reason) = check(opts) { eprintln!("{}", reason); - std::process::exit(1); + process::exit(1); } } diff --git a/src/bin/cache_dump.rs b/src/bin/cache_dump.rs index 5440725..b6875c9 100644 --- a/src/bin/cache_dump.rs +++ b/src/bin/cache_dump.rs @@ -3,7 +3,9 @@ extern crate thinp; use clap::{App, Arg}; use std::path::Path; +use std::process; use thinp::cache::dump::{dump, CacheDumpOptions}; +use thinp::file_utils; //------------------------------------------ @@ -48,6 +50,11 @@ fn main() { None }; + if let Err(e) = file_utils::is_file_or_blk(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); + process::exit(1); + } + let opts = CacheDumpOptions { input: input_file, output: output_file, @@ -57,7 +64,7 @@ fn main() { if let Err(reason) = dump(opts) { eprintln!("{}", reason); - std::process::exit(1); + process::exit(1); } } diff --git a/src/bin/cache_repair.rs b/src/bin/cache_repair.rs index f2d4f0c..8252b6b 100644 --- a/src/bin/cache_repair.rs +++ b/src/bin/cache_repair.rs @@ -5,7 +5,6 @@ use atty::Stream; use clap::{App, Arg}; use std::path::Path; use std::process; -use std::process::exit; use std::sync::Arc; use thinp::cache::repair::{repair, CacheRepairOptions}; use thinp::file_utils; @@ -50,9 +49,9 @@ fn main() { let input_file = Path::new(matches.value_of("INPUT").unwrap()); let output_file = Path::new(matches.value_of("OUTPUT").unwrap()); - if !file_utils::file_exists(input_file) { - eprintln!("Couldn't find input file '{:?}'.", &input_file); - exit(1); + if let Err(e) = file_utils::is_file_or_blk(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); + process::exit(1); } let report; diff --git a/src/bin/cache_restore.rs b/src/bin/cache_restore.rs index 1cbd3ae..c209176 100644 --- a/src/bin/cache_restore.rs +++ b/src/bin/cache_restore.rs @@ -5,7 +5,6 @@ use atty::Stream; use clap::{App, Arg}; use std::path::Path; use std::process; -use std::process::exit; use std::sync::Arc; use thinp::cache::restore::{restore, CacheRestoreOptions}; use thinp::file_utils; @@ -50,14 +49,14 @@ fn main() { let input_file = Path::new(matches.value_of("INPUT").unwrap()); let output_file = Path::new(matches.value_of("OUTPUT").unwrap()); - if !file_utils::file_exists(input_file) { - eprintln!("Couldn't find input file '{:?}'.", &input_file); - exit(1); + if let Err(e) = file_utils::is_file(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); + process::exit(1); } if let Err(e) = file_utils::check_output_file_requirements(output_file) { eprintln!("{}", e); - exit(1); + process::exit(1); } let report; diff --git a/src/bin/thin_check.rs b/src/bin/thin_check.rs index 7887be3..0913c44 100644 --- a/src/bin/thin_check.rs +++ b/src/bin/thin_check.rs @@ -5,7 +5,6 @@ use atty::Stream; use clap::{App, Arg}; use std::path::Path; use std::process; -use std::process::exit; use std::sync::Arc; use thinp::file_utils; use thinp::io_engine::*; @@ -80,9 +79,9 @@ fn main() { let matches = parser.get_matches(); let input_file = Path::new(matches.value_of("INPUT").unwrap()); - if !file_utils::file_exists(input_file) { - eprintln!("Couldn't find input file '{:?}'.", &input_file); - exit(1); + if let Err(e) = file_utils::is_file_or_blk(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); + process::exit(1); } let report; diff --git a/src/bin/thin_dump.rs b/src/bin/thin_dump.rs index 4f6c07d..45ab904 100644 --- a/src/bin/thin_dump.rs +++ b/src/bin/thin_dump.rs @@ -5,7 +5,6 @@ use atty::Stream; use clap::{App, Arg}; use std::path::Path; use std::process; -use std::process::exit; use std::sync::Arc; use thinp::file_utils; use thinp::report::*; @@ -89,29 +88,29 @@ fn main() { None }; - if !file_utils::file_exists(input_file) { - eprintln!("Couldn't find input file '{:?}'.", &input_file); - exit(1); + if let Err(e) = file_utils::is_file_or_blk(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); + process::exit(1); } let transaction_id = matches.value_of("TRANSACTION_ID").map(|s| { s.parse::().unwrap_or_else(|_| { eprintln!("Couldn't parse transaction_id"); - exit(1); + process::exit(1); }) }); let data_block_size = matches.value_of("DATA_BLOCK_SIZE").map(|s| { s.parse::().unwrap_or_else(|_| { eprintln!("Couldn't parse data_block_size"); - exit(1); + process::exit(1); }) }); let nr_data_blocks = matches.value_of("NR_DATA_BLOCKS").map(|s| { s.parse::().unwrap_or_else(|_| { eprintln!("Couldn't parse nr_data_blocks"); - exit(1); + process::exit(1); }) }); diff --git a/src/bin/thin_metadata_pack.rs b/src/bin/thin_metadata_pack.rs index e29678a..bc72af0 100644 --- a/src/bin/thin_metadata_pack.rs +++ b/src/bin/thin_metadata_pack.rs @@ -27,8 +27,8 @@ fn main() { let input_file = Path::new(matches.value_of("INPUT").unwrap()); let output_file = Path::new(matches.value_of("OUTPUT").unwrap()); - if !file_utils::file_exists(&input_file) { - eprintln!("Couldn't find input file '{}'.", &input_file.display()); + if let Err(e) = file_utils::is_file_or_blk(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); exit(1); } diff --git a/src/bin/thin_metadata_unpack.rs b/src/bin/thin_metadata_unpack.rs index 94874a3..b2463df 100644 --- a/src/bin/thin_metadata_unpack.rs +++ b/src/bin/thin_metadata_unpack.rs @@ -33,8 +33,8 @@ fn main() { let input_file = Path::new(matches.value_of("INPUT").unwrap()); let output_file = Path::new(matches.value_of("OUTPUT").unwrap()); - if !file_utils::file_exists(input_file) { - eprintln!("Couldn't find input file '{}'.", &input_file.display()); + if let Err(e) = file_utils::is_file(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); exit(1); } diff --git a/src/bin/thin_repair.rs b/src/bin/thin_repair.rs index 781cb31..188f874 100644 --- a/src/bin/thin_repair.rs +++ b/src/bin/thin_repair.rs @@ -5,7 +5,6 @@ use atty::Stream; use clap::{App, Arg}; use std::path::Path; use std::process; -use std::process::exit; use std::sync::Arc; use thinp::file_utils; use thinp::report::*; @@ -69,29 +68,29 @@ fn main() { let input_file = Path::new(matches.value_of("INPUT").unwrap()); let output_file = Path::new(matches.value_of("OUTPUT").unwrap()); - if !file_utils::file_exists(input_file) { - eprintln!("Couldn't find input file '{:?}'.", &input_file); - exit(1); + if let Err(e) = file_utils::is_file_or_blk(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); + process::exit(1); } let transaction_id = matches.value_of("TRANSACTION_ID").map(|s| { s.parse::().unwrap_or_else(|_| { eprintln!("Couldn't parse transaction_id"); - exit(1); + process::exit(1); }) }); let data_block_size = matches.value_of("DATA_BLOCK_SIZE").map(|s| { s.parse::().unwrap_or_else(|_| { eprintln!("Couldn't parse data_block_size"); - exit(1); + process::exit(1); }) }); let nr_data_blocks = matches.value_of("NR_DATA_BLOCKS").map(|s| { s.parse::().unwrap_or_else(|_| { eprintln!("Couldn't parse nr_data_blocks"); - exit(1); + process::exit(1); }) }); diff --git a/src/bin/thin_restore.rs b/src/bin/thin_restore.rs index 2201a83..f064594 100644 --- a/src/bin/thin_restore.rs +++ b/src/bin/thin_restore.rs @@ -5,7 +5,6 @@ use atty::Stream; use clap::{App, Arg}; use std::path::Path; use std::process; -use std::process::exit; use std::sync::Arc; use thinp::file_utils; use thinp::report::*; @@ -57,14 +56,14 @@ fn main() { let input_file = Path::new(matches.value_of("INPUT").unwrap()); let output_file = Path::new(matches.value_of("OUTPUT").unwrap()); - if !file_utils::file_exists(input_file) { - eprintln!("Couldn't find input file '{:?}'.", &input_file); - exit(1); + if let Err(e) = file_utils::is_file(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); + process::exit(1); } if let Err(e) = file_utils::check_output_file_requirements(output_file) { eprintln!("{}", e); - exit(1); + process::exit(1); } let report; diff --git a/src/bin/thin_shrink.rs b/src/bin/thin_shrink.rs index 722ef2b..fc452da 100644 --- a/src/bin/thin_shrink.rs +++ b/src/bin/thin_shrink.rs @@ -66,8 +66,8 @@ fn main() { let data_file = Path::new(matches.value_of("DATA").unwrap()); let do_copy = !matches.is_present("NOCOPY"); - if !file_utils::file_exists(input_file) { - eprintln!("Couldn't find input file '{}'.", input_file.display()); + if let Err(e) = file_utils::is_file_or_blk(input_file) { + eprintln!("Invalid input file '{}': {}.", input_file.display(), e); exit(1); } diff --git a/src/file_utils.rs b/src/file_utils.rs index a76aa4a..fffc014 100644 --- a/src/file_utils.rs +++ b/src/file_utils.rs @@ -1,5 +1,5 @@ use nix::sys::stat; -use nix::sys::stat::{FileStat, SFlag}; +use nix::sys::stat::FileStat; use std::fs::{File, OpenOptions}; use std::io; use std::io::{Seek, Write}; @@ -9,22 +9,46 @@ use tempfile::tempfile; //--------------------------------------- -fn check_bits(mode: u32, flag: &SFlag) -> bool { - (mode & flag.bits()) != 0 +#[inline(always)] +pub fn s_isreg(info: &FileStat) -> bool { + (info.st_mode & stat::SFlag::S_IFMT.bits()) == stat::SFlag::S_IFREG.bits() } -pub fn is_file_or_blk(info: FileStat) -> bool { - check_bits(info.st_mode, &stat::SFlag::S_IFBLK) - || check_bits(info.st_mode, &stat::SFlag::S_IFREG) +#[inline(always)] +pub fn s_isblk(info: &FileStat) -> bool { + (info.st_mode & stat::SFlag::S_IFMT.bits()) == stat::SFlag::S_IFBLK.bits() } -pub fn file_exists(path: &Path) -> bool { +pub fn is_file(path: &Path) -> io::Result<()> { match stat::stat(path) { - Ok(info) => is_file_or_blk(info), + Ok(info) => { + if s_isreg(&info) { + Ok(()) + } else { + fail("Not a regular file") + } + } _ => { // FIXME: assuming all errors indicate the file doesn't // exist. - false + fail("No such file or directory") + } + } +} + +pub fn is_file_or_blk(path: &Path) -> io::Result<()> { + match stat::stat(path) { + Ok(info) => { + if s_isreg(&info) || s_isblk(&info) { + Ok(()) + } else { + fail("Not a block device or regular file") + } + } + _ => { + // FIXME: assuming all errors indicate the file doesn't + // exist. + fail("No such file or directory") } } } @@ -55,12 +79,12 @@ fn get_device_size(path: &Path) -> io::Result { pub fn file_size(path: &Path) -> io::Result { match stat::stat(path) { Ok(info) => { - if check_bits(info.st_mode, &SFlag::S_IFREG) { + if s_isreg(&info) { Ok(info.st_size as u64) - } else if check_bits(info.st_mode, &SFlag::S_IFBLK) { + } else if s_isblk(&info) { get_device_size(path) } else { - fail("not a regular file or block device") + fail("Not a block device or regular file") } } _ => fail("stat failed"), diff --git a/tests/common/target.rs b/tests/common/target.rs index 53fd8af..ab0f89d 100644 --- a/tests/common/target.rs +++ b/tests/common/target.rs @@ -62,7 +62,7 @@ pub mod cpp_msg { } pub mod rust_msg { - pub const FILE_NOT_FOUND: &str = "Couldn't find input file"; + pub const FILE_NOT_FOUND: &str = "No such file or directory"; pub const MISSING_INPUT_ARG: &str = "The following required arguments were not provided"; // TODO: be specific pub const MISSING_OUTPUT_ARG: &str = "The following required arguments were not provided"; // TODO: be specific pub const BAD_SUPERBLOCK: &str = "bad checksum in superblock"; From bb5308327173bb48d77a464c879f64c5518fe162 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 7 Sep 2021 16:13:41 +0800 Subject: [PATCH 10/21] [array (rust)] Fix building uncontiguous array --- src/pdata/array_builder.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/pdata/array_builder.rs b/src/pdata/array_builder.rs index d19a6bd..bc89f01 100644 --- a/src/pdata/array_builder.rs +++ b/src/pdata/array_builder.rs @@ -59,8 +59,7 @@ impl ArrayBlockBuilder { let bi = index / self.entries_per_block as u64; let i = (index % self.entries_per_block as u64) as usize; - if bi < self.array_blocks.len() as u64 || i < self.values.len() || index >= self.nr_entries - { + if index >= self.nr_entries { return Err(anyhow!("array index out of bounds")); } @@ -68,8 +67,12 @@ impl ArrayBlockBuilder { self.emit_block(w)?; } - if i > self.values.len() + 1 { - self.values.resize_with(i - 1, Default::default); + if bi < self.array_blocks.len() as u64 || i < self.values.len() { + return Err(anyhow!("unordered array index")); + } + + if i > self.values.len() { + self.values.resize_with(i, Default::default); } self.values.push(v); From f8c40a1fda4b91553813b306f791a2c02f45e198 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 7 Sep 2021 18:19:55 +0800 Subject: [PATCH 11/21] [thin_check (rust)] Set compatibility between options --- src/bin/thin_check.rs | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/bin/thin_check.rs b/src/bin/thin_check.rs index 0913c44..cf74f4a 100644 --- a/src/bin/thin_check.rs +++ b/src/bin/thin_check.rs @@ -25,18 +25,39 @@ fn main() { .arg( Arg::with_name("AUTO_REPAIR") .help("Auto repair trivial issues.") - .long("auto-repair"), + .long("auto-repair") + .conflicts_with_all(&[ + "IGNORE_NON_FATAL", + "METADATA_SNAPSHOT", + "OVERRIDE_MAPPING_ROOT", + "SB_ONLY", + "SKIP_MAPPINGS", + ]), ) .arg( + // Using --clear-needs-check along with --skip-mappings is allowed + // (but not recommended) for backward compatibility (commit 1fe8a0d) Arg::with_name("CLEAR_NEEDS_CHECK") .help("Clears the 'needs_check' flag in the superblock") - .long("clear-needs-check-flag"), + .long("clear-needs-check-flag") + .conflicts_with_all(&[ + "IGNORE_NON_FATAL", + "METADATA_SNAPSHOT", + "OVERRIDE_MAPPING_ROOT", + "SB_ONLY", + ]), ) .arg( Arg::with_name("IGNORE_NON_FATAL") .help("Only return a non-zero exit code if a fatal error is found.") .long("ignore-non-fatal-errors"), ) + .arg( + Arg::with_name("METADATA_SNAPSHOT") + .help("Check the metadata snapshot on a live pool") + .short("m") + .long("metadata-snapshot"), + ) .arg( Arg::with_name("QUIET") .help("Suppress output messages, return only exit code.") @@ -54,13 +75,6 @@ fn main() { .long("skip-mappings"), ) // options - .arg( - Arg::with_name("METADATA_SNAPSHOT") - .help("Check the metadata snapshot on a live pool") - .short("m") - .long("metadata-snapshot") - .value_name("METADATA_SNAPSHOT"), - ) .arg( Arg::with_name("OVERRIDE_MAPPING_ROOT") .help("Specify a mapping root to use") From e7fa012701fb018b92c76183a37ee672a944d205 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 9 Sep 2021 09:58:45 +0800 Subject: [PATCH 12/21] [btree] Fix ref-counting on overwritten values --- era/era_detail.h | 10 +++++++++- persistent-data/data-structures/btree.tcc | 17 +++++++++-------- persistent-data/space-maps/disk_structures.h | 10 ++++++++++ thin-provisioning/device_tree.h | 8 ++++++++ thin-provisioning/mapping_tree.h | 8 ++++++++ 5 files changed, 44 insertions(+), 9 deletions(-) diff --git a/era/era_detail.h b/era/era_detail.h index 80961d5..a2c1851 100644 --- a/era/era_detail.h +++ b/era/era_detail.h @@ -22,6 +22,14 @@ namespace era { uint64_t writeset_root; }; + inline bool operator==(era_detail const& lhs, era_detail const& rhs) { + return lhs.nr_bits == rhs.nr_bits && lhs.writeset_root == rhs.writeset_root; + } + + inline bool operator!=(era_detail const& lhs, era_detail const& rhs) { + return !(lhs == rhs); + } + struct era_detail_ref_counter { era_detail_ref_counter(persistent_data::transaction_manager::ptr tm) : tm_(tm) { @@ -31,7 +39,7 @@ namespace era { tm_->get_sm()->inc(d.writeset_root); } - void dec(persistent_data::block_address b) { + void dec(era_detail const &d) { // I don't think we ever do this in the tools throw std::runtime_error("not implemented"); } diff --git a/persistent-data/data-structures/btree.tcc b/persistent-data/data-structures/btree.tcc index 27c9adc..3dc0704 100644 --- a/persistent-data/data-structures/btree.tcc +++ b/persistent-data/data-structures/btree.tcc @@ -693,9 +693,15 @@ namespace persistent_data { leaf_node n = spine.template get_node(); if (need_insert) n.insert_at(index, key[Levels - 1], value); - else - // FIXME: check if we're overwriting with the same value. - n.set_value(index, value); + else { + typename ValueTraits::value_type old_value = n.value_at(index); + if (value != old_value) { + // do decrement the old value if it already exists + rc_.dec(old_value); + + n.set_value(index, value); + } + } root_ = spine.get_root(); @@ -981,11 +987,6 @@ namespace persistent_data { if (i < 0 || leaf.key_at(i) != key) i++; - // do decrement the old value if it already exists - // FIXME: I'm not sure about this, I don't understand the |inc| reference - if (static_cast(i) < leaf.get_nr_entries() && leaf.key_at(i) == key && inc) { - // dec old entry - } *index = i; return ((static_cast(i) >= leaf.get_nr_entries()) || diff --git a/persistent-data/space-maps/disk_structures.h b/persistent-data/space-maps/disk_structures.h index aa8fbe5..50249d8 100644 --- a/persistent-data/space-maps/disk_structures.h +++ b/persistent-data/space-maps/disk_structures.h @@ -42,6 +42,16 @@ namespace persistent_data { uint32_t none_free_before_; }; + inline bool operator==(index_entry const& lhs, index_entry const& rhs) { + // The return value doesn't matter, since the ref-counts of bitmap blocks + // are managed by shadow operations. + return false; + } + + inline bool operator!=(index_entry const& lhs, index_entry const& rhs) { + return !(lhs == rhs); + } + struct index_entry_traits { typedef index_entry_disk disk_type; typedef index_entry value_type; diff --git a/thin-provisioning/device_tree.h b/thin-provisioning/device_tree.h index ee241a5..ffa98ff 100644 --- a/thin-provisioning/device_tree.h +++ b/thin-provisioning/device_tree.h @@ -25,6 +25,14 @@ namespace thin_provisioning { uint32_t snapshotted_time_; }; + inline bool operator==(device_details const& lhs, device_details const& rhs) { + return false; // device_details are not compariable + } + + inline bool operator!=(device_details const& lhs, device_details const& rhs) { + return !(lhs == rhs); + } + struct device_details_traits { typedef device_details_disk disk_type; typedef device_details value_type; diff --git a/thin-provisioning/mapping_tree.h b/thin-provisioning/mapping_tree.h index 24207ed..6281f2a 100644 --- a/thin-provisioning/mapping_tree.h +++ b/thin-provisioning/mapping_tree.h @@ -24,6 +24,14 @@ namespace thin_provisioning { uint32_t time_; }; + inline bool operator==(block_time const& lhs, block_time const& rhs) { + return lhs.block_ == rhs.block_; + } + + inline bool operator!=(block_time const& lhs, block_time const& rhs) { + return !(lhs == rhs); + } + class block_time_ref_counter { public: block_time_ref_counter(space_map::ptr sm); From a18fd60f3f27fbfbd868a716356edcf8d8af23ad Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 9 Sep 2021 16:46:39 +0800 Subject: [PATCH 13/21] [thin_check (rust)] Fix auto-repair related errors - Returns error on metadata leaks - Clear needs_check flag on success - Check auto-repair write errors - Fix file open flags, and correct spelling --- src/bin/thin_check.rs | 7 +++++-- src/io_engine.rs | 12 ++++++------ src/pdata/space_map_checker.rs | 7 ++++++- src/thin/check.rs | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/bin/thin_check.rs b/src/bin/thin_check.rs index cf74f4a..371dd21 100644 --- a/src/bin/thin_check.rs +++ b/src/bin/thin_check.rs @@ -109,16 +109,18 @@ fn main() { } let engine: Arc; + let writable = matches.is_present("AUTO_REPAIR") || matches.is_present("CLEAR_NEEDS_CHECK"); if matches.is_present("ASYNC_IO") { engine = Arc::new( - AsyncIoEngine::new(&input_file, MAX_CONCURRENT_IO, false) + AsyncIoEngine::new(&input_file, MAX_CONCURRENT_IO, writable) .expect("unable to open input file"), ); } else { let nr_threads = std::cmp::max(8, num_cpus::get() * 2); engine = Arc::new( - SyncIoEngine::new(&input_file, nr_threads, false).expect("unable to open input file"), + SyncIoEngine::new(&input_file, nr_threads, writable) + .expect("unable to open input file"), ); } @@ -128,6 +130,7 @@ fn main() { skip_mappings: matches.is_present("SKIP_MAPPINGS"), ignore_non_fatal: matches.is_present("IGNORE_NON_FATAL"), auto_repair: matches.is_present("AUTO_REPAIR"), + clear_needs_check: matches.is_present("CLEAR_NEEDS_CHECK"), report, }; diff --git a/src/io_engine.rs b/src/io_engine.rs index 2bd7687..37f0d49 100644 --- a/src/io_engine.rs +++ b/src/io_engine.rs @@ -131,16 +131,16 @@ impl<'a> Drop for FileGuard<'a> { } impl SyncIoEngine { - fn open_file(path: &Path, writeable: bool) -> Result { - let file = OpenOptions::new().read(true).write(writeable).open(path)?; + fn open_file(path: &Path, writable: bool) -> Result { + let file = OpenOptions::new().read(true).write(writable).open(path)?; Ok(file) } - pub fn new(path: &Path, nr_files: usize, writeable: bool) -> Result { + pub fn new(path: &Path, nr_files: usize, writable: bool) -> Result { let mut files = Vec::with_capacity(nr_files); for _n in 0..nr_files { - files.push(SyncIoEngine::open_file(path, writeable)?); + files.push(SyncIoEngine::open_file(path, writable)?); } Ok(SyncIoEngine { @@ -231,10 +231,10 @@ pub struct AsyncIoEngine { } impl AsyncIoEngine { - pub fn new(path: &Path, queue_len: u32, writeable: bool) -> Result { + pub fn new(path: &Path, queue_len: u32, writable: bool) -> Result { let input = OpenOptions::new() .read(true) - .write(writeable) + .write(writable) .custom_flags(libc::O_DIRECT) .open(path)?; diff --git a/src/pdata/space_map_checker.rs b/src/pdata/space_map_checker.rs index 3e01561..1ae7f6b 100644 --- a/src/pdata/space_map_checker.rs +++ b/src/pdata/space_map_checker.rs @@ -309,7 +309,12 @@ pub fn repair_space_map( } } - engine.write_many(&write_blocks[0..])?; + let results = engine.write_many(&write_blocks[0..])?; + for ret in results { + if ret.is_err() { + return Err(anyhow!("Unable to repair space map: {:?}", ret)); + } + } Ok(()) } diff --git a/src/thin/check.rs b/src/thin/check.rs index 1c5836e..7fdf9d4 100644 --- a/src/thin/check.rs +++ b/src/thin/check.rs @@ -87,6 +87,7 @@ pub struct ThinCheckOptions { pub skip_mappings: bool, pub ignore_non_fatal: bool, pub auto_repair: bool, + pub clear_needs_check: bool, pub report: Arc, } @@ -276,6 +277,10 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> { )?; if opts.skip_mappings { + let cleared = clear_needs_check_flag(ctx.engine.clone())?; + if cleared { + ctx.report.info("Cleared needs_check flag"); + } return Ok(()); } @@ -331,6 +336,26 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> { ctx.report.info("Repairing metadata leaks."); repair_space_map(ctx.engine.clone(), metadata_leaks, metadata_sm.clone())?; } + + let cleared = clear_needs_check_flag(ctx.engine.clone())?; + if cleared { + ctx.report.info("Cleared needs_check flag"); + } + } else if !opts.ignore_non_fatal { + if !data_leaks.is_empty() { + return Err(anyhow!("data space map contains leaks")); + } + + if !metadata_leaks.is_empty() { + return Err(anyhow!("metadata space map contains leaks")); + } + + if opts.clear_needs_check { + let cleared = clear_needs_check_flag(ctx.engine.clone())?; + if cleared { + ctx.report.info("Cleared needs_check flag"); + } + } } stop_progress.store(true, Ordering::Relaxed); @@ -339,6 +364,15 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> { Ok(()) } +pub fn clear_needs_check_flag(engine: Arc) -> Result { + let mut sb = read_superblock(engine.as_ref(), SUPERBLOCK_LOCATION)?; + if !sb.flags.needs_check { + return Ok(false); + } + sb.flags.needs_check = false; + write_superblock(engine.as_ref(), SUPERBLOCK_LOCATION, &sb).map(|_| true) +} + //------------------------------------------ // Some callers wish to know which blocks are allocated. From 438730951efe1c4342fab1fbd42e2ca0fb65ec69 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 9 Sep 2021 16:53:33 +0800 Subject: [PATCH 14/21] [thin_check (rust)] Allow ignoring non-fatal errors in mapping tree --- src/thin/check.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/thin/check.rs b/src/thin/check.rs index 7fdf9d4..cf61296 100644 --- a/src/thin/check.rs +++ b/src/thin/check.rs @@ -136,13 +136,14 @@ fn check_mapping_bottom_level( metadata_sm: &Arc>, data_sm: &Arc>, roots: &BTreeMap, u64)>, + ignore_non_fatal: bool, ) -> Result<()> { ctx.report.set_sub_title("mapping tree"); let w = Arc::new(BTreeWalker::new_with_sm( ctx.engine.clone(), metadata_sm.clone(), - false, + ignore_non_fatal, )?); // We want to print out errors as we progress, so we aggregate for each thin and print @@ -287,7 +288,7 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> { // mapping bottom level let root = unpack::(&sb.data_sm_root[0..])?; let data_sm = core_sm(root.nr_blocks, nr_devs as u32); - check_mapping_bottom_level(&ctx, &metadata_sm, &data_sm, &roots)?; + check_mapping_bottom_level(&ctx, &metadata_sm, &data_sm, &roots, opts.ignore_non_fatal)?; bail_out(&ctx, "mapping tree")?; //----------------------------------------- @@ -432,7 +433,7 @@ pub fn check_with_maps( // mapping bottom level let root = unpack::(&sb.data_sm_root[0..])?; let data_sm = core_sm(root.nr_blocks, nr_devs as u32); - check_mapping_bottom_level(&ctx, &metadata_sm, &data_sm, &roots)?; + check_mapping_bottom_level(&ctx, &metadata_sm, &data_sm, &roots, false)?; bail_out(&ctx, "mapping tree")?; //----------------------------------------- From 34f927d9896b320fb0040fd461ecce05b9370334 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 9 Sep 2021 17:42:36 +0800 Subject: [PATCH 15/21] [thin_check (rust)] Make better use of Rust's Result type Replace the bail_out checking with the returned Result, which helps decoupling the internal state of Report from application logic. --- src/pdata/space_map_checker.rs | 10 +++++++++- src/thin/check.rs | 19 ------------------- 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/src/pdata/space_map_checker.rs b/src/pdata/space_map_checker.rs index 1ae7f6b..ad5998b 100644 --- a/src/pdata/space_map_checker.rs +++ b/src/pdata/space_map_checker.rs @@ -98,6 +98,7 @@ fn check_low_ref_counts( // compare ref-counts in bitmap blocks let mut leaks = 0; + let mut failed = false; let mut blocknr = 0; let mut bitmap_leaks = Vec::new(); let sm = sm.lock().unwrap(); @@ -113,6 +114,7 @@ fn check_low_ref_counts( "Index entry points to block ({}) that isn't a bitmap", b.loc )); + failed = true; // FIXME: revert the ref-count at b.loc? } @@ -134,6 +136,7 @@ fn check_low_ref_counts( } else if *actual != expected as u8 { report.fatal(&format!("Bad reference count for {} block {}. Expected {}, but space map contains {}.", kind, blocknr, expected, actual)); + failed = true; } } BitmapEntry::Overflow => { @@ -141,6 +144,7 @@ fn check_low_ref_counts( if expected < 3 { report.fatal(&format!("Bad reference count for {} block {}. Expected {}, but space map says it's >= 3.", kind, blocknr, expected)); + failed = true; } } } @@ -160,7 +164,11 @@ fn check_low_ref_counts( report.non_fatal(&format!("{} {} blocks have leaked.", leaks, kind)); } - Ok(bitmap_leaks) + if failed { + Err(anyhow!("Fatal errors in {} space map", kind)) + } else { + Ok(bitmap_leaks) + } } fn gather_disk_index_entries( diff --git a/src/thin/check.rs b/src/thin/check.rs index cf61296..e3e8a93 100644 --- a/src/thin/check.rs +++ b/src/thin/check.rs @@ -206,18 +206,6 @@ fn mk_context(engine: Arc, report: Arc) -> R }) } -fn bail_out(ctx: &Context, task: &str) -> Result<()> { - use ReportOutcome::*; - - match ctx.report.get_outcome() { - Fatal => Err(anyhow!(format!( - "Check of {} failed, ending check early.", - task - ))), - _ => Ok(()), - } -} - pub fn check(opts: ThinCheckOptions) -> Result<()> { let ctx = mk_context(opts.engine.clone(), opts.report.clone())?; @@ -289,7 +277,6 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> { let root = unpack::(&sb.data_sm_root[0..])?; let data_sm = core_sm(root.nr_blocks, nr_devs as u32); check_mapping_bottom_level(&ctx, &metadata_sm, &data_sm, &roots, opts.ignore_non_fatal)?; - bail_out(&ctx, "mapping tree")?; //----------------------------------------- @@ -303,7 +290,6 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> { metadata_sm.clone(), opts.ignore_non_fatal, )?; - bail_out(&ctx, "data space map")?; //----------------------------------------- @@ -323,8 +309,6 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> { opts.ignore_non_fatal, )?; - bail_out(&ctx, "metadata space map")?; - //----------------------------------------- if opts.auto_repair { @@ -434,7 +418,6 @@ pub fn check_with_maps( let root = unpack::(&sb.data_sm_root[0..])?; let data_sm = core_sm(root.nr_blocks, nr_devs as u32); check_mapping_bottom_level(&ctx, &metadata_sm, &data_sm, &roots, false)?; - bail_out(&ctx, "mapping tree")?; //----------------------------------------- @@ -448,7 +431,6 @@ pub fn check_with_maps( metadata_sm.clone(), false, )?; - bail_out(&ctx, "data space map")?; //----------------------------------------- @@ -462,7 +444,6 @@ pub fn check_with_maps( // Now the counts should be correct and we can check it. let _metadata_leaks = check_metadata_space_map(engine.clone(), report, root, metadata_sm.clone(), false)?; - bail_out(&ctx, "metadata space map")?; //----------------------------------------- From c133e62353663a233207902fc698f1b184d766be Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 10 Sep 2021 01:11:20 +0800 Subject: [PATCH 16/21] [thin_dump] Emit superblock flags --- src/thin/dump.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/thin/dump.rs b/src/thin/dump.rs index 2cbb019..3371996 100644 --- a/src/thin/dump.rs +++ b/src/thin/dump.rs @@ -276,7 +276,7 @@ pub fn dump_metadata( uuid: "".to_string(), time: sb.time, transaction: sb.transaction_id, - flags: None, + flags: if sb.flags.needs_check { Some(1) } else { None }, version: Some(2), data_block_size: sb.data_block_size, nr_data_blocks: data_root.nr_blocks, From d436f35ed3d0c707f0c401c84013f05ac4b9b0a4 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 10 Sep 2021 10:33:53 +0800 Subject: [PATCH 17/21] [thin_dump/thin_repair/thin_restore (rust)] Fix errors in tests - Move error messages to stderr - Fix the transaction_id and data_block_size in test fixture, and check the data_block_size in thin_restore. - Disable the missing_something tests for Rust. The transaction_id and nr_data_blocks now could be recovered automatically. - Limit the use of override options in test cases. Only broken superblocks could be overridden, and the provided values should be compatible to the original metadata. - Remove unused option --- src/bin/thin_dump.rs | 2 +- src/bin/thin_restore.rs | 7 ------- src/thin/restore.rs | 4 ++++ tests/common/thin_xml_generator.rs | 4 ++-- tests/thin_dump.rs | 31 ++++++++++++++---------------- tests/thin_repair.rs | 28 +++++++++++++-------------- tests/thin_restore.rs | 5 ++++- 7 files changed, 38 insertions(+), 43 deletions(-) diff --git a/src/bin/thin_dump.rs b/src/bin/thin_dump.rs index 45ab904..59ac543 100644 --- a/src/bin/thin_dump.rs +++ b/src/bin/thin_dump.rs @@ -138,7 +138,7 @@ fn main() { }; if let Err(reason) = dump(opts) { - println!("{}", reason); + eprintln!("{}", reason); process::exit(1); } } diff --git a/src/bin/thin_restore.rs b/src/bin/thin_restore.rs index f064594..125e148 100644 --- a/src/bin/thin_restore.rs +++ b/src/bin/thin_restore.rs @@ -43,13 +43,6 @@ fn main() { .long("output") .value_name("FILE") .required(true), - ) - .arg( - Arg::with_name("OVERRIDE_MAPPING_ROOT") - .help("Specify a mapping root to use") - .long("override-mapping-root") - .value_name("OVERRIDE_MAPPING_ROOT") - .takes_value(true), ); let matches = parser.get_matches(); diff --git a/src/thin/restore.rs b/src/thin/restore.rs index 38380fa..ecaa7a0 100644 --- a/src/thin/restore.rs +++ b/src/thin/restore.rs @@ -207,6 +207,10 @@ impl<'a> MetadataVisitor for Restorer<'a> { return Err(anyhow!("duplicated superblock")); } + if !(128..=2097152).contains(&sb.data_block_size) || (sb.data_block_size & 0x7F != 0) { + return Err(anyhow!("invalid data block size")); + } + self.sb = Some(sb.clone()); self.data_sm = Some(core_sm(sb.nr_data_blocks, u32::MAX)); let b = self.w.alloc()?; diff --git a/tests/common/thin_xml_generator.rs b/tests/common/thin_xml_generator.rs index 9d56856..a0cc66c 100644 --- a/tests/common/thin_xml_generator.rs +++ b/tests/common/thin_xml_generator.rs @@ -29,10 +29,10 @@ fn common_sb(nr_blocks: u64) -> ir::Superblock { ir::Superblock { uuid: "".to_string(), time: 0, - transaction: 0, + transaction: 1, flags: None, version: None, - data_block_size: 32, + data_block_size: 128, nr_data_blocks: nr_blocks, metadata_snap: None, } diff --git a/tests/thin_dump.rs b/tests/thin_dump.rs index c1bd3c4..206acfb 100644 --- a/tests/thin_dump.rs +++ b/tests/thin_dump.rs @@ -1,7 +1,6 @@ use anyhow::Result; use std::fs::OpenOptions; use std::io::Write; -use std::str::from_utf8; mod common; @@ -129,6 +128,7 @@ fn no_stderr() -> Result<()> { // test superblock overriding & repair // TODO: share with thin_repair +#[cfg(not(feature = "rust_tests"))] fn override_something(flag: &str, value: &str, pattern: &str) -> Result<()> { let mut td = TestDir::new()?; let md = mk_valid_md(&mut td)?; @@ -137,21 +137,24 @@ fn override_something(flag: &str, value: &str, pattern: &str) -> Result<()> { if !cfg!(feature = "rust_tests") { assert_eq!(output.stderr.len(), 0); } - assert!(from_utf8(&output.stdout[0..])?.contains(pattern)); + assert!(std::str::from_utf8(&output.stdout[0..])?.contains(pattern)); Ok(()) } #[test] +#[cfg(not(feature = "rust_tests"))] fn override_transaction_id() -> Result<()> { override_something("--transaction-id", "2345", "transaction=\"2345\"") } #[test] +#[cfg(not(feature = "rust_tests"))] fn override_data_block_size() -> Result<()> { override_something("--data-block-size", "8192", "data_block_size=\"8192\"") } #[test] +#[cfg(not(feature = "rust_tests"))] fn override_nr_data_blocks() -> Result<()> { override_something("--nr-data-blocks", "234500", "nr_data_blocks=\"234500\"") } @@ -161,24 +164,16 @@ fn override_nr_data_blocks() -> Result<()> { fn repair_superblock() -> Result<()> { let mut td = TestDir::new()?; let md = mk_valid_md(&mut td)?; - let before = run_ok_raw( - THIN_DUMP, - args![ - "--transaction-id=5", - "--data-block-size=128", - "--nr-data-blocks=4096000", - &md - ], - )?; + let before = run_ok_raw(THIN_DUMP, args![&md])?; damage_superblock(&md)?; let after = run_ok_raw( THIN_DUMP, args![ "--repair", - "--transaction-id=5", + "--transaction-id=1", "--data-block-size=128", - "--nr-data-blocks=4096000", + "--nr-data-blocks=20480", &md ], )?; @@ -195,6 +190,7 @@ fn repair_superblock() -> Result<()> { // TODO: share with thin_repair #[test] +#[cfg(not(feature = "rust_tests"))] fn missing_transaction_id() -> Result<()> { let mut td = TestDir::new()?; let md = mk_valid_md(&mut td)?; @@ -204,7 +200,7 @@ fn missing_transaction_id() -> Result<()> { args![ "--repair", "--data-block-size=128", - "--nr-data-blocks=4096000", + "--nr-data-blocks=20480", &md ], )?; @@ -221,8 +217,8 @@ fn missing_data_block_size() -> Result<()> { THIN_DUMP, args![ "--repair", - "--transaction-id=5", - "--nr-data-blocks=4096000", + "--transaction-id=1", + "--nr-data-blocks=20480", &md ], )?; @@ -231,6 +227,7 @@ fn missing_data_block_size() -> Result<()> { } #[test] +#[cfg(not(feature = "rust_tests"))] fn missing_nr_data_blocks() -> Result<()> { let mut td = TestDir::new()?; let md = mk_valid_md(&mut td)?; @@ -239,7 +236,7 @@ fn missing_nr_data_blocks() -> Result<()> { THIN_DUMP, args![ "--repair", - "--transaction-id=5", + "--transaction-id=1", "--data-block-size=128", &md ], diff --git a/tests/thin_repair.rs b/tests/thin_repair.rs index da54a7a..a5acf31 100644 --- a/tests/thin_repair.rs +++ b/tests/thin_repair.rs @@ -116,6 +116,7 @@ fn dont_repair_xml() -> Result<()> { // TODO: share with thin_dump +#[cfg(not(feature = "rust_tests"))] fn override_thing(flag: &str, val: &str, pattern: &str) -> Result<()> { let mut td = TestDir::new()?; let md1 = mk_valid_md(&mut td)?; @@ -128,16 +129,19 @@ fn override_thing(flag: &str, val: &str, pattern: &str) -> Result<()> { } #[test] +#[cfg(not(feature = "rust_tests"))] fn override_transaction_id() -> Result<()> { override_thing("--transaction-id", "2345", "transaction=\"2345\"") } #[test] +#[cfg(not(feature = "rust_tests"))] fn override_data_block_size() -> Result<()> { override_thing("--data-block-size", "8192", "data_block_size=\"8192\"") } #[test] +#[cfg(not(feature = "rust_tests"))] fn override_nr_data_blocks() -> Result<()> { override_thing("--nr-data-blocks", "234500", "nr_data_blocks=\"234500\"") } @@ -147,15 +151,7 @@ fn override_nr_data_blocks() -> Result<()> { fn superblock_succeeds() -> Result<()> { let mut td = TestDir::new()?; let md1 = mk_valid_md(&mut td)?; - let original = run_ok_raw( - THIN_DUMP, - args![ - "--transaction-id=5", - "--data-block-size=128", - "--nr-data-blocks=4096000", - &md1 - ], - )?; + let original = run_ok_raw(THIN_DUMP, args![&md1])?; if !cfg!(feature = "rust_tests") { assert_eq!(original.stderr.len(), 0); } @@ -164,9 +160,9 @@ fn superblock_succeeds() -> Result<()> { run_ok( THIN_REPAIR, args![ - "--transaction-id=5", + "--transaction-id=1", "--data-block-size=128", - "--nr-data-blocks=4096000", + "--nr-data-blocks=20480", "-i", &md1, "-o", @@ -196,10 +192,11 @@ fn missing_thing(flag1: &str, flag2: &str, pattern: &str) -> Result<()> { } #[test] +#[cfg(not(feature = "rust_tests"))] fn missing_transaction_id() -> Result<()> { missing_thing( "--data-block-size=128", - "--nr-data-blocks=4096000", + "--nr-data-blocks=20480", "transaction id", ) } @@ -207,16 +204,17 @@ fn missing_transaction_id() -> Result<()> { #[test] fn missing_data_block_size() -> Result<()> { missing_thing( - "--transaction-id=5", - "--nr-data-blocks=4096000", + "--transaction-id=1", + "--nr-data-blocks=20480", "data block size", ) } #[test] +#[cfg(not(feature = "rust_tests"))] fn missing_nr_data_blocks() -> Result<()> { missing_thing( - "--transaction-id=5", + "--transaction-id=1", "--data-block-size=128", "nr data blocks", ) diff --git a/tests/thin_restore.rs b/tests/thin_restore.rs index e1ec504..da2d90a 100644 --- a/tests/thin_restore.rs +++ b/tests/thin_restore.rs @@ -124,7 +124,7 @@ fn accepts_quiet() -> Result<()> { //----------------------------------------- // TODO: share with thin_dump - +#[cfg(not(feature = "rust_tests"))] fn override_something(flag: &str, value: &str, pattern: &str) -> Result<()> { let mut td = TestDir::new()?; let xml = mk_valid_xml(&mut td)?; @@ -138,16 +138,19 @@ fn override_something(flag: &str, value: &str, pattern: &str) -> Result<()> { } #[test] +#[cfg(not(feature = "rust_tests"))] fn override_transaction_id() -> Result<()> { override_something("--transaction-id", "2345", "transaction=\"2345\"") } #[test] +#[cfg(not(feature = "rust_tests"))] fn override_data_block_size() -> Result<()> { override_something("--data-block-size", "8192", "data_block_size=\"8192\"") } #[test] +#[cfg(not(feature = "rust_tests"))] fn override_nr_data_blocks() -> Result<()> { override_something("--nr-data-blocks", "234500", "nr_data_blocks=\"234500\"") } From 66c1d629a42dae167f54a9ee82a73927da93fbc0 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 10 Sep 2021 15:45:50 +0800 Subject: [PATCH 18/21] [all (rust)] Keep silent if --quiet was applied --- src/bin/cache_check.rs | 4 ++-- src/bin/cache_repair.rs | 4 ++-- src/bin/cache_restore.rs | 4 ++-- src/bin/thin_check.rs | 4 ++-- src/bin/thin_dump.rs | 4 ++-- src/bin/thin_repair.rs | 4 ++-- src/bin/thin_restore.rs | 4 ++-- src/bin/thin_shrink.rs | 2 +- 8 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/bin/cache_check.rs b/src/bin/cache_check.rs index e85057b..d37c07a 100644 --- a/src/bin/cache_check.rs +++ b/src/bin/cache_check.rs @@ -93,11 +93,11 @@ fn main() { skip_discards: matches.is_present("SKIP_DISCARDS"), ignore_non_fatal: matches.is_present("IGNORE_NON_FATAL"), auto_repair: matches.is_present("AUTO_REPAIR"), - report, + report: report.clone(), }; if let Err(reason) = check(opts) { - eprintln!("{}", reason); + report.fatal(&format!("{}", reason)); process::exit(1); } } diff --git a/src/bin/cache_repair.rs b/src/bin/cache_repair.rs index 8252b6b..646516b 100644 --- a/src/bin/cache_repair.rs +++ b/src/bin/cache_repair.rs @@ -68,11 +68,11 @@ fn main() { input: &input_file, output: &output_file, async_io: matches.is_present("ASYNC_IO"), - report, + report: report.clone(), }; if let Err(reason) = repair(opts) { - eprintln!("{}", reason); + report.fatal(&format!("{}", reason)); process::exit(1); } } diff --git a/src/bin/cache_restore.rs b/src/bin/cache_restore.rs index c209176..66b8359 100644 --- a/src/bin/cache_restore.rs +++ b/src/bin/cache_restore.rs @@ -73,11 +73,11 @@ fn main() { input: &input_file, output: &output_file, async_io: matches.is_present("ASYNC_IO"), - report, + report: report.clone(), }; if let Err(reason) = restore(opts) { - eprintln!("{}", reason); + report.fatal(&format!("{}", reason)); process::exit(1); } } diff --git a/src/bin/thin_check.rs b/src/bin/thin_check.rs index 371dd21..d04cc62 100644 --- a/src/bin/thin_check.rs +++ b/src/bin/thin_check.rs @@ -131,11 +131,11 @@ fn main() { ignore_non_fatal: matches.is_present("IGNORE_NON_FATAL"), auto_repair: matches.is_present("AUTO_REPAIR"), clear_needs_check: matches.is_present("CLEAR_NEEDS_CHECK"), - report, + report: report.clone(), }; if let Err(reason) = check(opts) { - eprintln!("{}", reason); + report.fatal(&format!("{}", reason)); process::exit(1); } } diff --git a/src/bin/thin_dump.rs b/src/bin/thin_dump.rs index 59ac543..0a6a8b8 100644 --- a/src/bin/thin_dump.rs +++ b/src/bin/thin_dump.rs @@ -128,7 +128,7 @@ fn main() { input: input_file, output: output_file, async_io: matches.is_present("ASYNC_IO"), - report, + report: report.clone(), repair: matches.is_present("REPAIR"), overrides: SuperblockOverrides { transaction_id, @@ -138,7 +138,7 @@ fn main() { }; if let Err(reason) = dump(opts) { - eprintln!("{}", reason); + report.fatal(&format!("{}", reason)); process::exit(1); } } diff --git a/src/bin/thin_repair.rs b/src/bin/thin_repair.rs index 188f874..d2de7a3 100644 --- a/src/bin/thin_repair.rs +++ b/src/bin/thin_repair.rs @@ -108,7 +108,7 @@ fn main() { input: &input_file, output: &output_file, async_io: matches.is_present("ASYNC_IO"), - report, + report: report.clone(), overrides: SuperblockOverrides { transaction_id, data_block_size, @@ -117,7 +117,7 @@ fn main() { }; if let Err(reason) = repair(opts) { - eprintln!("{}", reason); + report.fatal(&format!("{}", reason)); process::exit(1); } } diff --git a/src/bin/thin_restore.rs b/src/bin/thin_restore.rs index 125e148..be471d8 100644 --- a/src/bin/thin_restore.rs +++ b/src/bin/thin_restore.rs @@ -73,11 +73,11 @@ fn main() { input: &input_file, output: &output_file, async_io: matches.is_present("ASYNC_IO"), - report, + report: report.clone(), }; if let Err(reason) = restore(opts) { - eprintln!("{}", reason); + report.fatal(&format!("{}", reason)); process::exit(1); } } diff --git a/src/bin/thin_shrink.rs b/src/bin/thin_shrink.rs index fc452da..e61ed5b 100644 --- a/src/bin/thin_shrink.rs +++ b/src/bin/thin_shrink.rs @@ -74,7 +74,7 @@ fn main() { if let Err(reason) = thinp::shrink::toplevel::shrink(&input_file, &output_file, &data_file, size, do_copy) { - println!("Application error: {}\n", reason); + eprintln!("Application error: {}\n", reason); exit(1); } } From 5abb92838cbfae32caf2fb50a697459312af82b8 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 10 Sep 2021 16:23:53 +0800 Subject: [PATCH 19/21] [tests] Port the remaining cache tests --- functional-tests/cache-functional-tests.scm | 61 -------------- tests/cache_check.rs | 52 ++++++------ tests/cache_dump.rs | 38 ++++++--- tests/cache_repair.rs | 90 +++++++++++++++++++++ tests/common/target.rs | 1 + 5 files changed, 146 insertions(+), 96 deletions(-) create mode 100644 tests/cache_repair.rs diff --git a/functional-tests/cache-functional-tests.scm b/functional-tests/cache-functional-tests.scm index 42d3713..f99606f 100644 --- a/functional-tests/cache-functional-tests.scm +++ b/functional-tests/cache-functional-tests.scm @@ -53,46 +53,12 @@ ;;; cache_dump scenarios ;;;----------------------------------------------------------- - (define-scenario (cache-dump v) - "print version (-V flag)" - (run-ok-rcv (stdout _) (cache-dump "-V") - (assert-equal tools-version stdout))) - - (define-scenario (cache-dump version) - "print version (--version flags)" - (run-ok-rcv (stdout _) (cache-dump "--version") - (assert-equal tools-version stdout))) - - (define-scenario (cache-dump h) - "cache_dump -h" - (run-ok-rcv (stdout _) (cache-dump "-h") - (assert-equal cache-dump-help stdout))) - - (define-scenario (cache-dump help) - "cache_dump --help" - (run-ok-rcv (stdout _) (cache-dump "--help") - (assert-equal cache-dump-help stdout))) - - (define-scenario (cache-dump missing-input-file) - "Fails with missing input file." - (run-fail-rcv (stdout stderr) (cache-dump) - (assert-starts-with "No input file provided." stderr))) - (define-scenario (cache-dump small-input-file) "Fails with small input file" (with-temp-file-sized ((md "cache.bin" 512)) (run-fail (cache-dump md)))) - (define-scenario (cache-dump restore-is-noop) - "cache_dump followed by cache_restore is a noop." - (with-valid-metadata (md) - (run-ok-rcv (d1-stdout _) (cache-dump md) - (with-temp-file-containing ((xml "cache.xml" d1-stdout)) - (run-ok (cache-restore "-i" xml "-o" md)) - (run-ok-rcv (d2-stdout _) (cache-dump md) - (assert-equal d1-stdout d2-stdout)))))) - ;;;----------------------------------------------------------- ;;; cache_metadata_size scenarios ;;;----------------------------------------------------------- @@ -164,31 +130,4 @@ (run-ok-rcv (stdout stderr) (cache-metadata-size "--nr-blocks 67108864") (assert-equal "3678208 sectors" stdout) (assert-eof stderr))) - - ;;;----------------------------------------------------------- - ;;; cache_repair scenarios - ;;;----------------------------------------------------------- - (define-scenario (cache-repair missing-input-file) - "the input file can't be found" - (with-empty-metadata (md) - (let ((bad-path "no-such-file")) - (run-fail-rcv (_ stderr) (cache-repair "-i no-such-file -o" md) - (assert-superblock-all-zeroes md) - (assert-starts-with - (string-append bad-path ": No such file or directory") - stderr))))) - - (define-scenario (cache-repair garbage-input-file) - "the input file is just zeroes" - (with-empty-metadata (md1) - (with-corrupt-metadata (md2) - (run-fail-rcv (_ stderr) (cache-repair "-i" md1 "-o" md2) - (assert-superblock-all-zeroes md2))))) - - (define-scenario (cache-repair missing-output-file) - "the output file can't be found" - (with-cache-xml (xml) - (run-fail-rcv (_ stderr) (cache-repair "-i" xml) - (assert-starts-with "No output file provided." stderr)))) - ) diff --git a/tests/cache_check.rs b/tests/cache_check.rs index b676742..bdc2a72 100644 --- a/tests/cache_check.rs +++ b/tests/cache_check.rs @@ -107,28 +107,30 @@ fn failing_quiet() -> Result<()> { Ok(()) } -// (define-scenario (cache-check valid-metadata-passes) -// "A valid metadata area passes" -// (with-valid-metadata (md) -// (run-ok (cache-check md)))) -// -// (define-scenario (cache-check bad-metadata-version) -// "Invalid metadata version fails" -// (with-cache-xml (xml) -// (with-empty-metadata (md) -// (cache-restore "-i" xml "-o" md "--debug-override-metadata-version" "12345") -// (run-fail (cache-check md))))) -// -// (define-scenario (cache-check tiny-metadata) -// "Prints helpful message in case tiny metadata given" -// (with-temp-file-sized ((md "cache.bin" 1024)) -// (run-fail-rcv (_ stderr) (cache-check md) -// (assert-starts-with "Metadata device/file too small. Is this binary metadata?" stderr)))) -// -// (define-scenario (cache-check spot-accidental-xml-data) -// "Prints helpful message if XML metadata given" -// (with-cache-xml (xml) -// (system (fmt #f "man bash >> " xml)) -// (run-fail-rcv (_ stderr) (cache-check xml) -// (assert-matches ".*This looks like XML. cache_check only checks the binary metadata format." stderr)))) -// +#[test] +fn valid_metadata_passes() -> Result<()> { + let mut td = TestDir::new()?; + let md = mk_valid_md(&mut td)?; + run_ok(CACHE_CHECK, args![&md])?; + Ok(()) +} + +#[test] +fn bad_metadata_version() -> Result<()> { + let mut td = TestDir::new()?; + let xml = mk_valid_xml(&mut td)?; + let md = mk_zeroed_md(&mut td)?; + run_ok( + CACHE_RESTORE, + args![ + "-i", + &xml, + "-o", + &md, + "--debug-override-metadata-version", + "12345" + ], + )?; + run_fail(CACHE_CHECK, args![&md])?; + Ok(()) +} diff --git a/tests/cache_dump.rs b/tests/cache_dump.rs index da85b5c..4d44dcd 100644 --- a/tests/cache_dump.rs +++ b/tests/cache_dump.rs @@ -1,10 +1,14 @@ use anyhow::Result; +use std::fs::OpenOptions; +use std::io::Write; mod common; use common::cache::*; use common::common_args::*; +use common::fixture::*; use common::input_arg::*; +use common::process::*; use common::program::*; use common::target::*; use common::test_dir::*; @@ -75,13 +79,27 @@ test_unreadable_input_file!(CacheDump); //------------------------------------------ -/* - (define-scenario (cache-dump restore-is-noop) - "cache_dump followed by cache_restore is a noop." - (with-valid-metadata (md) - (run-ok-rcv (d1-stdout _) (cache-dump md) - (with-temp-file-containing ((xml "cache.xml" d1-stdout)) - (run-ok (cache-restore "-i" xml "-o" md)) - (run-ok-rcv (d2-stdout _) (cache-dump md) - (assert-equal d1-stdout d2-stdout)))))) -*/ +// TODO: share with thin_dump +#[test] +fn dump_restore_cycle() -> Result<()> { + let mut td = TestDir::new()?; + let md = mk_valid_md(&mut td)?; + let output = run_ok_raw(CACHE_DUMP, args![&md])?; + + let xml = td.mk_path("meta.xml"); + let mut file = OpenOptions::new() + .read(false) + .write(true) + .create(true) + .open(&xml)?; + file.write_all(&output.stdout[0..])?; + drop(file); + + let md2 = mk_zeroed_md(&mut td)?; + run_ok(CACHE_RESTORE, args!["-i", &xml, "-o", &md2])?; + + let output2 = run_ok_raw(CACHE_DUMP, args![&md2])?; + assert_eq!(output.stdout, output2.stdout); + + Ok(()) +} diff --git a/tests/cache_repair.rs b/tests/cache_repair.rs new file mode 100644 index 0000000..c7701dd --- /dev/null +++ b/tests/cache_repair.rs @@ -0,0 +1,90 @@ +use anyhow::Result; + +mod common; + +use common::cache::*; +use common::common_args::*; +use common::input_arg::*; +use common::output_option::*; +use common::program::*; +use common::target::*; +use common::test_dir::*; + +//------------------------------------------ + +const USAGE: &str = "Usage: cache_repair [options] {device|file}\n\ + Options:\n \ + {-h|--help}\n \ + {-i|--input} \n \ + {-o|--output} \n \ + {-V|--version}"; + +//----------------------------------------- + +struct CacheRepair; + +impl<'a> Program<'a> for CacheRepair { + fn name() -> &'a str { + "cache_repair" + } + + fn path() -> &'a std::ffi::OsStr { + CACHE_REPAIR.as_ref() + } + + fn usage() -> &'a str { + USAGE + } + + fn arg_type() -> ArgType { + ArgType::IoOptions + } + + fn bad_option_hint(option: &str) -> String { + msg::bad_option_hint(option) + } +} + +impl<'a> InputProgram<'a> for CacheRepair { + fn mk_valid_input(td: &mut TestDir) -> Result { + mk_valid_md(td) + } + + fn file_not_found() -> &'a str { + msg::FILE_NOT_FOUND + } + + fn missing_input_arg() -> &'a str { + msg::MISSING_INPUT_ARG + } + + fn corrupted_input() -> &'a str { + "bad checksum in superblock" + } +} + +impl<'a> OutputProgram<'a> for CacheRepair { + fn missing_output_arg() -> &'a str { + msg::MISSING_OUTPUT_ARG + } +} + +impl<'a> MetadataWriter<'a> for CacheRepair { + fn file_not_found() -> &'a str { + msg::FILE_NOT_FOUND + } +} + +//----------------------------------------- + +test_accepts_help!(CacheRepair); +test_accepts_version!(CacheRepair); +test_rejects_bad_option!(CacheRepair); + +test_input_file_not_found!(CacheRepair); +test_input_cannot_be_a_directory!(CacheRepair); +test_corrupted_input_data!(CacheRepair); + +test_missing_output_option!(CacheRepair); + +//----------------------------------------- diff --git a/tests/common/target.rs b/tests/common/target.rs index ab0f89d..d30eb28 100644 --- a/tests/common/target.rs +++ b/tests/common/target.rs @@ -34,6 +34,7 @@ macro_rules! path_to { pub const CACHE_CHECK: &str = path_to!("cache_check"); pub const CACHE_DUMP: &str = path_to!("cache_dump"); +pub const CACHE_REPAIR: &str = path_to!("cache_repair"); pub const CACHE_RESTORE: &str = path_to!("cache_restore"); pub const THIN_CHECK: &str = path_to!("thin_check"); From 6bd7741dfaa01c42d7bf5aa4cf1b786f03996591 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 10 Sep 2021 23:35:56 +0800 Subject: [PATCH 20/21] [cache_repair] Avoid touching the output file by checking input file earlier The output file has been checked by the caller, so there's no need to check the output file again. --- caching/cache_repair.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/caching/cache_repair.cc b/caching/cache_repair.cc index 720047f..9e28b26 100644 --- a/caching/cache_repair.cc +++ b/caching/cache_repair.cc @@ -32,7 +32,7 @@ namespace { int repair(string const &old_path, string const &new_path) { bool metadata_touched = false; try { - file_utils::check_file_exists(new_path, false); + file_utils::check_file_exists(old_path, false); metadata_touched = true; metadata_dump(open_metadata_for_read(old_path), output_emitter(new_path), From d712190db45df8c5b810fa5d01ce87680d6b10cb Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sat, 11 Sep 2021 22:38:00 +0800 Subject: [PATCH 21/21] [io_engine (rust)] Open file exclusively --- src/io_engine.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/io_engine.rs b/src/io_engine.rs index 37f0d49..4d68d75 100644 --- a/src/io_engine.rs +++ b/src/io_engine.rs @@ -132,19 +132,24 @@ impl<'a> Drop for FileGuard<'a> { impl SyncIoEngine { fn open_file(path: &Path, writable: bool) -> Result { - let file = OpenOptions::new().read(true).write(writable).open(path)?; + let file = OpenOptions::new() + .read(true) + .write(writable) + .custom_flags(libc::O_EXCL) + .open(path)?; Ok(file) } pub fn new(path: &Path, nr_files: usize, writable: bool) -> Result { + let nr_blocks = get_nr_blocks(path)?; // check file mode eariler let mut files = Vec::with_capacity(nr_files); for _n in 0..nr_files { files.push(SyncIoEngine::open_file(path, writable)?); } Ok(SyncIoEngine { - nr_blocks: get_nr_blocks(path)?, + nr_blocks, files: Mutex::new(files), cvar: Condvar::new(), }) @@ -232,17 +237,18 @@ pub struct AsyncIoEngine { impl AsyncIoEngine { pub fn new(path: &Path, queue_len: u32, writable: bool) -> Result { + let nr_blocks = get_nr_blocks(path)?; // check file mode earlier let input = OpenOptions::new() .read(true) .write(writable) - .custom_flags(libc::O_DIRECT) + .custom_flags(libc::O_DIRECT | libc::O_EXCL) .open(path)?; Ok(AsyncIoEngine { inner: Mutex::new(AsyncIoEngine_ { queue_len, ring: IoUring::new(queue_len)?, - nr_blocks: get_nr_blocks(path)?, + nr_blocks, fd: input.as_raw_fd(), input: Arc::new(input), }),