[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] rust-lang/rust#13158
[2] rust-lang/rust#62569

Author: mingnus

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::<OutputError>() {
+                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(())
     }
 }

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,56 +153,59 @@ pub struct ThinDumpOptions<'a> {
     pub overrides: SuperblockOverrides,
 }
 
-struct Context {
+struct ThinDumpContext {
     report: Arc<Report>,
     engine: Arc<dyn IoEngine + Send + Sync>,
 }
 
-fn mk_context(opts: &ThinDumpOptions) -> Result<Context> {
+fn mk_context(opts: &ThinDumpOptions) -> Result<ThinDumpContext> {
     let engine: Arc<dyn IoEngine + Send + Sync> = if opts.async_io {
         Arc::new(AsyncIoEngine::new(opts.input, MAX_CONCURRENT_IO, false)?)
     } else {
         let nr_threads = std::cmp::max(8, num_cpus::get() * 2);
         Arc::new(SyncIoEngine::new(opts.input, nr_threads, false)?)
     };
 
-    Ok(Context {
+    Ok(ThinDumpContext {
         report: opts.report.clone(),
         engine,
     })
 }
 
 //------------------------------------------
 
-fn emit_leaf(v: &mut MappingVisitor, b: &Block) -> 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: &MappingVisitor, b: &Block) -> Result<()> {
     use Node::*;
     let path = Vec::new();
     let kr = KeyRange::new();
 
     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::<BlockTime>(&path, b.get_data(), true, true)?;
 
     match node {
-        Internal { .. } => {
-            return Err(anyhow!("not a leaf"));
-        }
+        Internal { .. } => 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)
+            .context(OutputError),
     }
-
-    Ok(())
 }
 
 fn read_for<T>(engine: Arc<dyn IoEngine>, blocks: &[u64], mut t: T) -> Result<()>
@@ -214,7 +217,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)?;
         }
     }
 
@@ -226,15 +230,14 @@ fn emit_leaves(
     out: &mut dyn MetadataVisitor,
     leaves: &[u64],
 ) -> Result<()> {
-    let mut v = MappingVisitor::new(out);
+    let v = MappingVisitor::new(out);
     let proc = |b| {
-        emit_leaf(&mut v, &b)?;
+        emit_leaf(&v, &b)?;
         Ok(())
     };
 
     read_for(engine, leaves, proc)?;
-    v.end_walk()
-        .map_err(|e| anyhow!("failed to emit leaves: {}", e))
+    v.end_walk().context(OutputError)
 }
 
 fn emit_entries(
@@ -255,7 +258,7 @@ fn emit_entries(
                     leaves.clear();
                 }
                 let str = format!("{}", id);
-                out.ref_shared(&str)?;
+                out.ref_shared(&str).context(OutputError)?;
             }
         }
     }
@@ -284,12 +287,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 +304,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 +331,13 @@ 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<dyn Write> = if opts.output.is_some() {
-        Box::new(BufWriter::new(File::create(opts.output.unwrap())?))
+        let f = File::create(opts.output.unwrap()).context(OutputError)?;
+        Box::new(BufWriter::new(f))
     } else {
         Box::new(BufWriter::new(std::io::stdout()))
     };