From 29473be8154df390ffde9d2ca1c5d02b25306151 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 20 May 2022 23:17:21 +0800 Subject: [PATCH] [thin_dump] Prompt for repair only on input errors thin_dump should display the reparing hint only if there's any error in the input metadata rather than the output process. A context therefore is added to the returned error for callers to determine the error type. Note that a broken pipe error (EPIPE) is treated as an output error since the Rust std library ignores SIGPIPE by default [1][2]. [1] https://github.com/rust-lang/rust/pull/13158 [2] https://github.com/rust-lang/rust/issues/62569 --- src/commands/thin_dump.rs | 22 ++++++++------- src/thin/dump.rs | 56 ++++++++++++++++++++++++--------------- 2 files changed, 48 insertions(+), 30 deletions(-) diff --git a/src/commands/thin_dump.rs b/src/commands/thin_dump.rs index 8380535f..742c7353 100644 --- a/src/commands/thin_dump.rs +++ b/src/commands/thin_dump.rs @@ -9,7 +9,7 @@ use std::sync::Arc; use crate::commands::utils::*; use crate::commands::Command; use crate::report::*; -use crate::thin::dump::{dump, ThinDumpOptions}; +use crate::thin::dump::{dump, OutputError, ThinDumpOptions}; use crate::thin::metadata_repair::SuperblockOverrides; pub struct ThinDumpCommand; @@ -128,13 +128,17 @@ impl<'a> Command<'a> for ThinDumpCommand { }, }; - dump(opts).map_err(|reason| { - report.fatal(&format!("{}", reason)); - report.fatal( - "metadata contains errors (run thin_check for details).\n\ - perhaps you wanted to run with --repair ?", - ); - std::io::Error::from_raw_os_error(libc::EPERM) - }) + if let Err(e) = dump(opts) { + if !e.is::() { + report.fatal(&format!("{:?}", e)); + report.fatal( + "metadata contains errors (run thin_check for details).\n\ + perhaps you wanted to run with --repair ?", + ); + } + return Err(std::io::Error::from_raw_os_error(libc::EPERM)); + } + + Ok(()) } } diff --git a/src/thin/dump.rs b/src/thin/dump.rs index 6a573d7d..a1a5a068 100644 --- a/src/thin/dump.rs +++ b/src/thin/dump.rs @@ -1,4 +1,4 @@ -use anyhow::{anyhow, Result}; +use anyhow::{anyhow, Context, Result}; use std::fs::File; use std::io::BufWriter; use std::io::Write; @@ -153,12 +153,12 @@ pub struct ThinDumpOptions<'a> { pub overrides: SuperblockOverrides, } -struct Context { +struct ThinDumpContext { report: Arc, engine: Arc, } -fn mk_context(opts: &ThinDumpOptions) -> Result { +fn mk_context(opts: &ThinDumpOptions) -> Result { let engine: Arc = if opts.async_io { Arc::new(AsyncIoEngine::new(opts.input, MAX_CONCURRENT_IO, false)?) } else { @@ -166,7 +166,7 @@ fn mk_context(opts: &ThinDumpOptions) -> Result { Arc::new(SyncIoEngine::new(opts.input, nr_threads, false)?) }; - Ok(Context { + Ok(ThinDumpContext { report: opts.report.clone(), engine, }) @@ -174,6 +174,16 @@ fn mk_context(opts: &ThinDumpOptions) -> Result { //------------------------------------------ +// a wrapper for callers to indicate the error type +#[derive(Debug)] +pub struct OutputError; + +impl std::fmt::Display for OutputError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "output error") + } +} + fn emit_leaf(v: &mut MappingVisitor, b: &Block) -> Result<()> { use Node::*; let path = Vec::new(); @@ -181,24 +191,22 @@ fn emit_leaf(v: &mut MappingVisitor, b: &Block) -> Result<()> { let bt = checksum::metadata_block_type(b.get_data()); if bt != checksum::BT::NODE { - return Err(anyhow!(format!( - "checksum failed for node {}, {:?}", - b.loc, bt - ))); + return Err(anyhow!("checksum failed for node {}, {:?}", b.loc, bt)); } let node = unpack_node::(&path, b.get_data(), true, true)?; match node { Internal { .. } => { - return Err(anyhow!("not a leaf")); + return Err(anyhow!("block {} is not a leaf", b.loc)); } Leaf { header, keys, values, } => { - v.visit(&path, &kr, &header, &keys, &values)?; + v.visit(&path, &kr, &header, &keys, &values) + .map_err(|e| anyhow::Error::from(e).context(OutputError))?; } } @@ -214,7 +222,8 @@ where .read_many(cs) .map_err(|_e| anyhow!("read_many failed"))? { - t(b.map_err(|_e| anyhow!("read of individual block failed"))?)?; + let blk = b.map_err(|_e| anyhow!("read of individual block failed"))?; + t(blk)?; } } @@ -234,7 +243,7 @@ fn emit_leaves( read_for(engine, leaves, proc)?; v.end_walk() - .map_err(|e| anyhow!("failed to emit leaves: {}", e)) + .map_err(|e| anyhow::Error::from(e).context(OutputError)) } fn emit_entries( @@ -255,7 +264,7 @@ fn emit_entries( leaves.clear(); } let str = format!("{}", id); - out.ref_shared(&str)?; + out.ref_shared(&str).context(OutputError)?; } } } @@ -284,12 +293,13 @@ pub fn dump_metadata( nr_data_blocks: data_root.nr_blocks, metadata_snap: None, }; - out.superblock_b(&out_sb)?; + out.superblock_b(&out_sb).context(OutputError)?; for d in &md.defs { - out.def_shared_b(&format!("{}", d.def_id))?; + out.def_shared_b(&format!("{}", d.def_id)) + .context(OutputError)?; emit_entries(engine.clone(), out, &d.map.entries)?; - out.def_shared_e()?; + out.def_shared_e().context(OutputError)?; } for dev in &md.devs { @@ -300,12 +310,12 @@ pub fn dump_metadata( creation_time: dev.detail.creation_time, snap_time: dev.detail.snapshotted_time, }; - out.device_b(&device)?; + out.device_b(&device).context(OutputError)?; emit_entries(engine.clone(), out, &dev.map.entries)?; - out.device_e()?; + out.device_e().context(OutputError)?; } - out.superblock_e()?; - out.eof()?; + out.superblock_e().context(OutputError)?; + out.eof().context(OutputError)?; Ok(()) } @@ -327,11 +337,15 @@ pub fn dump(opts: ThinDumpOptions) -> Result<()> { read_superblock(ctx.engine.as_ref(), SUPERBLOCK_LOCATION) .and_then(|sb| sb.overrides(&opts.overrides))? }; + let md = build_metadata(ctx.engine.clone(), &sb)?; let md = optimise_metadata(md)?; let writer: Box = if opts.output.is_some() { - Box::new(BufWriter::new(File::create(opts.output.unwrap())?)) + Box::new(BufWriter::new( + File::create(opts.output.unwrap()) + .map_err(|e| anyhow::Error::from(e).context(OutputError))?, + )) } else { Box::new(BufWriter::new(std::io::stdout())) };