Merge pull request #189 from mingnus/2021-08-30-functional-tests

Functional tests and misc fixes

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),

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");
 		}

functional-tests/cache-functional-tests.scm

@@ -49,146 +49,16 @@
   ;; 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
   ;;;-----------------------------------------------------------
 
-  (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
   ;;;-----------------------------------------------------------
@@ -260,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))))
-
 )

persistent-data/data-structures/btree.tcc

@@ -693,9 +693,15 @@ namespace persistent_data {
 		leaf_node n = spine.template get_node<ValueTraits>();
 		if (need_insert)
 			n.insert_at(index, key[Levels - 1], value);
-		else
+		else {
-			// FIXME: check if we're overwriting with the same value.
+			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<unsigned>(i) < leaf.get_nr_entries() && leaf.key_at(i) == key && inc) {
-			// dec old entry
-		}
 		*index = i;
 
 		return ((static_cast<unsigned>(i) >= leaf.get_nr_entries()) ||

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;

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());
@@ -86,12 +93,12 @@ 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));
-        std::process::exit(1);
+        process::exit(1);
     }
 }
 

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);
     }
 }
 

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) {
+    if let Err(e) = file_utils::is_file_or_blk(input_file) {
-        eprintln!("Couldn't find input file '{:?}'.", &input_file);
+        eprintln!("Invalid input file '{}': {}.", input_file.display(), e);
-        exit(1);
+        process::exit(1);
     }
 
     let report;
@@ -69,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);
     }
 }

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;
@@ -22,13 +21,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.")
@@ -57,9 +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) {
+    if let Err(e) = file_utils::is_file(input_file) {
-        eprintln!("Couldn't find input file '{:?}'.", &input_file);
+        eprintln!("Invalid input file '{}': {}.", input_file.display(), e);
-        exit(1);
+        process::exit(1);
+    }
+
+    if let Err(e) = file_utils::check_output_file_requirements(output_file) {
+        eprintln!("{}", e);
+        process::exit(1);
     }
 
     let report;
@@ -76,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) {
-        println!("{}", reason);
+        report.fatal(&format!("{}", reason));
         process::exit(1);
     }
 }

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::*;
@@ -26,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.")
@@ -55,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")
@@ -80,9 +93,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) {
+    if let Err(e) = file_utils::is_file_or_blk(input_file) {
-        eprintln!("Couldn't find input file '{:?}'.", &input_file);
+        eprintln!("Invalid input file '{}': {}.", input_file.display(), e);
-        exit(1);
+        process::exit(1);
     }
 
     let report;
@@ -96,16 +109,18 @@ fn main() {
     }
 
     let engine: Arc<dyn IoEngine + Send + Sync>;
+    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"),
         );
     }
 
@@ -115,11 +130,12 @@ 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"),
-        report,
+        clear_needs_check: matches.is_present("CLEAR_NEEDS_CHECK"),
+        report: report.clone(),
     };
 
     if let Err(reason) = check(opts) {
-        eprintln!("{}", reason);
+        report.fatal(&format!("{}", reason));
         process::exit(1);
     }
 }

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) {
+    if let Err(e) = file_utils::is_file_or_blk(input_file) {
-        eprintln!("Couldn't find input file '{:?}'.", &input_file);
+        eprintln!("Invalid input file '{}': {}.", input_file.display(), e);
-        exit(1);
+        process::exit(1);
     }
 
     let transaction_id = matches.value_of("TRANSACTION_ID").map(|s| {
         s.parse::<u64>().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::<u32>().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::<u64>().unwrap_or_else(|_| {
             eprintln!("Couldn't parse nr_data_blocks");
-            exit(1);
+            process::exit(1);
         })
     });
 
@@ -129,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,
@@ -139,7 +138,7 @@ fn main() {
     };
 
     if let Err(reason) = dump(opts) {
-        println!("{}", reason);
+        report.fatal(&format!("{}", reason));
         process::exit(1);
     }
 }

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) {
+    if let Err(e) = file_utils::is_file_or_blk(input_file) {
-        eprintln!("Couldn't find input file '{}'.", &input_file.display());
+        eprintln!("Invalid input file '{}': {}.", input_file.display(), e);
         exit(1);
     }
 

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) {
+    if let Err(e) = file_utils::is_file(input_file) {
-        eprintln!("Couldn't find input file '{}'.", &input_file.display());
+        eprintln!("Invalid input file '{}': {}.", input_file.display(), e);
         exit(1);
     }
 

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) {
+    if let Err(e) = file_utils::is_file_or_blk(input_file) {
-        eprintln!("Couldn't find input file '{:?}'.", &input_file);
+        eprintln!("Invalid input file '{}': {}.", input_file.display(), e);
-        exit(1);
+        process::exit(1);
     }
 
     let transaction_id = matches.value_of("TRANSACTION_ID").map(|s| {
         s.parse::<u64>().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::<u32>().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::<u64>().unwrap_or_else(|_| {
             eprintln!("Couldn't parse nr_data_blocks");
-            exit(1);
+            process::exit(1);
         })
     });
 
@@ -109,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,
@@ -118,7 +117,7 @@ fn main() {
     };
 
     if let Err(reason) = repair(opts) {
-        eprintln!("{}", reason);
+        report.fatal(&format!("{}", reason));
         process::exit(1);
     }
 }

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::*;
@@ -44,22 +43,20 @@ 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();
     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) {
+    if let Err(e) = file_utils::is_file(input_file) {
-        eprintln!("Couldn't find input file '{:?}'.", &input_file);
+        eprintln!("Invalid input file '{}': {}.", input_file.display(), e);
-        exit(1);
+        process::exit(1);
+    }
+
+    if let Err(e) = file_utils::check_output_file_requirements(output_file) {
+        eprintln!("{}", e);
+        process::exit(1);
     }
 
     let report;
@@ -76,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) {
-        println!("{}", reason);
+        report.fatal(&format!("{}", reason));
         process::exit(1);
     }
 }

src/bin/thin_shrink.rs

@@ -66,15 +66,15 @@ 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) {
+    if let Err(e) = file_utils::is_file_or_blk(input_file) {
-        eprintln!("Couldn't find input file '{}'.", input_file.display());
+        eprintln!("Invalid input file '{}': {}.", input_file.display(), e);
         exit(1);
     }
 
     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);
     }
 }

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<Context> {
 
 //------------------------------------------
 
+#[derive(PartialEq)]
+enum Section {
+    None,
+    Superblock,
+    Mappings,
+    Hints,
+    Finalized,
+}
+
 pub struct Restorer<'a> {
     write_batcher: &'a mut WriteBatcher,
     sb: Option<ir::Superblock>,
@@ -64,7 +73,8 @@ pub struct Restorer<'a> {
     dirty_root: Option<u64>,
     hint_root: Option<u64>,
     discard_root: Option<u64>,
-    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,
+            data_block_size: src_sb.block_size,
-            cache_blocks: sb.nr_cache_blocks,
+            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<Visit> {
+        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::<u64>::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<Visit> {
+        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<Visit> {
-        let mut mapping_builder = None;
+        if self.in_section != Section::Mappings {
-        std::mem::swap(&mut self.mapping_builder, &mut mapping_builder);
+            return Err(anyhow!("not in mappings"));
-        if let Some(builder) = mapping_builder {
-            self.mapping_root = Some(builder.complete(self.write_batcher)?);
         }
-
+        self.in_section = Section::Superblock;
-        // 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)?);
-        }
-
         Ok(Visit::Continue)
     }
 
@@ -198,15 +237,18 @@ impl<'a> MetadataVisitor for Restorer<'a> {
     }
 
     fn hints_b(&mut self) -> Result<Visit> {
+        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<Visit> {
-        let mut hint_builder = None;
+        if self.in_section != Section::Hints {
-        std::mem::swap(&mut self.hint_builder, &mut hint_builder);
+            return Err(anyhow!("not in hints"));
-        if let Some(builder) = hint_builder {
-            self.hint_root = Some(builder.complete(self.write_batcher)?);
         }
+        self.in_section = Section::Superblock;
         Ok(Visit::Continue)
     }
 
@@ -232,6 +274,9 @@ impl<'a> MetadataVisitor for Restorer<'a> {
     }
 
     fn eof(&mut self) -> Result<Visit> {
+        if self.in_section != Section::Finalized {
+            return Err(anyhow!("incompleted source metadata"));
+        }
         Ok(Visit::Continue)
     }
 }

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!(),
+        Ok(_) => {
-        Err(e) => Err(anyhow!("{:?}", e)),
+            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
+            ))
+        }
     }
 }
 

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 {
+#[inline(always)]
-    (mode & flag.bits()) != 0
+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 {
+#[inline(always)]
-    check_bits(info.st_mode, &stat::SFlag::S_IFBLK)
+pub fn s_isblk(info: &FileStat) -> bool {
-        || check_bits(info.st_mode, &stat::SFlag::S_IFREG)
+    (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<u64> {
 pub fn file_size(path: &Path) -> io::Result<u64> {
     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"),
@@ -98,3 +122,13 @@ pub fn create_sized_file(path: &Path, nr_bytes: u64) -> io::Result<std::fs::File
 }
 
 //---------------------------------------
+
+pub fn check_output_file_requirements(path: &Path) -> io::Result<()> {
+    // minimal thin metadata size is 10 blocks, with one device
+    if file_size(path)? < 40960 {
+        return fail("Output file too small.");
+    }
+    Ok(())
+}
+
+//---------------------------------------

src/io_engine.rs

@@ -131,20 +131,25 @@ impl<'a> Drop for FileGuard<'a> {
 }
 
 impl SyncIoEngine {
-    fn open_file(path: &Path, writeable: bool) -> Result<File> {
+    fn open_file(path: &Path, writable: bool) -> Result<File> {
-        let file = OpenOptions::new().read(true).write(writeable).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, writeable: bool) -> Result<SyncIoEngine> {
+    pub fn new(path: &Path, nr_files: usize, writable: bool) -> Result<SyncIoEngine> {
+        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, writeable)?);
+            files.push(SyncIoEngine::open_file(path, writable)?);
         }
 
         Ok(SyncIoEngine {
-            nr_blocks: get_nr_blocks(path)?,
+            nr_blocks,
             files: Mutex::new(files),
             cvar: Condvar::new(),
         })
@@ -231,18 +236,19 @@ pub struct AsyncIoEngine {
 }
 
 impl AsyncIoEngine {
-    pub fn new(path: &Path, queue_len: u32, writeable: bool) -> Result<AsyncIoEngine> {
+    pub fn new(path: &Path, queue_len: u32, writable: bool) -> Result<AsyncIoEngine> {
+        let nr_blocks = get_nr_blocks(path)?; // check file mode earlier
         let input = OpenOptions::new()
             .read(true)
-            .write(writeable)
+            .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),
             }),

src/pdata/array_builder.rs

@@ -59,8 +59,7 @@ impl<V: Unpack + Pack + Clone + Default> ArrayBlockBuilder<V> {
         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<V: Unpack + Pack + Clone + Default> ArrayBlockBuilder<V> {
             self.emit_block(w)?;
         }
 
-        if i > self.values.len() + 1 {
+        if bi < self.array_blocks.len() as u64 || i < self.values.len() {
-            self.values.resize_with(i - 1, Default::default);
+            return Err(anyhow!("unordered array index"));
+        }
+
+        if i > self.values.len() {
+            self.values.resize_with(i, Default::default);
         }
         self.values.push(v);
 

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));
     }
 
+    if failed {
+        Err(anyhow!("Fatal errors in {} space map", kind))
+    } else {
         Ok(bitmap_leaks)
+    }
 }
 
 fn gather_disk_index_entries(
@@ -309,7 +317,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(())
 }
 

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<Report>,
 }
 
@@ -135,13 +136,14 @@ fn check_mapping_bottom_level(
     metadata_sm: &Arc<Mutex<dyn SpaceMap + Send + Sync>>,
     data_sm: &Arc<Mutex<dyn SpaceMap + Send + Sync>>,
     roots: &BTreeMap<u64, (Vec<u64>, 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
@@ -204,18 +206,6 @@ fn mk_context(engine: Arc<dyn IoEngine + Send + Sync>, report: Arc<Report>) -> 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())?;
 
@@ -276,14 +266,17 @@ 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(());
     }
 
     // mapping bottom level
     let root = unpack::<SMRoot>(&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")?;
 
     //-----------------------------------------
 
@@ -297,7 +290,6 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> {
         metadata_sm.clone(),
         opts.ignore_non_fatal,
     )?;
-    bail_out(&ctx, "data space map")?;
 
     //-----------------------------------------
 
@@ -317,8 +309,6 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> {
         opts.ignore_non_fatal,
     )?;
 
-    bail_out(&ctx, "metadata space map")?;
-
     //-----------------------------------------
 
     if opts.auto_repair {
@@ -331,6 +321,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 +349,15 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> {
     Ok(())
 }
 
+pub fn clear_needs_check_flag(engine: Arc<dyn IoEngine + Send + Sync>) -> Result<bool> {
+    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.
@@ -398,8 +417,7 @@ pub fn check_with_maps(
     // mapping bottom level
     let root = unpack::<SMRoot>(&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")?;
 
     //-----------------------------------------
 
@@ -413,7 +431,6 @@ pub fn check_with_maps(
         metadata_sm.clone(),
         false,
     )?;
-    bail_out(&ctx, "data space map")?;
 
     //-----------------------------------------
 
@@ -427,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")?;
 
     //-----------------------------------------
 

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,

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<Report>,
@@ -71,6 +80,7 @@ pub struct Restorer<'a> {
     sb: Option<ir::Superblock>,
     devices: BTreeMap<u32, (DeviceDetail, u64)>,
     data_sm: Option<Arc<Mutex<dyn SpaceMap>>>,
+    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,45 @@ 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,
+            time: src_sb.time as u32,
-            transaction_id: sb.transaction,
+            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<Visit> {
+        if self.in_section != Section::None {
+            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()?;
         if b.loc != SUPERBLOCK_LOCATION {
             return Err(anyhow!("superblock was occupied"));
         }
+        self.in_section = Section::Superblock;
+
         Ok(Visit::Continue)
     }
 
@@ -198,12 +228,17 @@ impl<'a> MetadataVisitor for Restorer<'a> {
     }
 
     fn def_shared_b(&mut self, name: &str) -> Result<Visit> {
+        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<Visit> {
         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 </def>"))
@@ -211,6 +246,9 @@ impl<'a> MetadataVisitor for Restorer<'a> {
     }
 
     fn device_b(&mut self, d: &ir::Device) -> Result<Visit> {
+        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 +257,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 +266,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 +318,9 @@ impl<'a> MetadataVisitor for Restorer<'a> {
     }
 
     fn eof(&mut self) -> Result<Visit> {
-        // FIXME: build the rest of the device trees
+        if self.in_section != Section::Finalized {
+            return Err(anyhow!("incomplete source metadata"));
+        }
         Ok(Visit::Continue)
     }
 }

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!(),
+        Ok(_) => return Err(anyhow!("Parse error at byte {}", reader.buffer_position())),
-
+        Err(e) => {
-        // FIXME: don't panic!
+            return Err(anyhow!(
-        Err(e) => panic!("error parsing xml {:?}", e),
+                "Parse error at byte {}: {:?}",
+                reader.buffer_position(),
+                e
+            ))
+        }
     }
 }
 

src/xml.rs

@@ -32,8 +32,8 @@ pub fn bool_val(kv: &Attribute) -> anyhow::Result<bool> {
     Ok(n)
 }
 
-pub fn bad_attr<T>(_tag: &str, _attr: &[u8]) -> anyhow::Result<T> {
+pub fn bad_attr<T>(tag: &str, _attr: &[u8]) -> anyhow::Result<T> {
-    todo!();
+    Err(anyhow!("unknown attribute in tag '{}'", tag))
 }
 
 pub fn check_attr<T>(tag: &str, name: &str, maybe_v: Option<T>) -> anyhow::Result<T> {

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<std::path::PathBuf> {
-        common::thin::mk_valid_md(td) // FIXME: create cache metadata
+        mk_valid_md(td)
     }
 
     fn file_not_found() -> &'a str {
@@ -67,7 +68,7 @@ impl<'a> InputProgram<'a> for CacheCheck {
     }
 }
 
-impl<'a> BinaryInputProgram<'_> for CacheCheck {}
+impl<'a> MetadataReader<'a> for CacheCheck {}
 
 //------------------------------------------
 
@@ -106,28 +107,30 @@ fn failing_quiet() -> Result<()> {
     Ok(())
 }
 
-//  (define-scenario (cache-check valid-metadata-passes)
+#[test]
-//    "A valid metadata area passes"
+fn valid_metadata_passes() -> Result<()> {
-//    (with-valid-metadata (md)
+    let mut td = TestDir::new()?;
-//      (run-ok (cache-check md))))
+    let md = mk_valid_md(&mut td)?;
-//
+    run_ok(CACHE_CHECK, args![&md])?;
-//  (define-scenario (cache-check bad-metadata-version)
+    Ok(())
-//    "Invalid metadata version fails"
+}
-//    (with-cache-xml (xml)
+
-//      (with-empty-metadata (md)
+#[test]
-//        (cache-restore "-i" xml "-o" md "--debug-override-metadata-version" "12345")
+fn bad_metadata_version() -> Result<()> {
-//        (run-fail (cache-check md)))))
+    let mut td = TestDir::new()?;
-//
+    let xml = mk_valid_xml(&mut td)?;
-//  (define-scenario (cache-check tiny-metadata)
+    let md = mk_zeroed_md(&mut td)?;
-//    "Prints helpful message in case tiny metadata given"
+    run_ok(
-//    (with-temp-file-sized ((md "cache.bin" 1024))
+        CACHE_RESTORE,
-//      (run-fail-rcv (_ stderr) (cache-check md)
+        args![
-//        (assert-starts-with "Metadata device/file too small.  Is this binary metadata?" stderr))))
+            "-i",
-//
+            &xml,
-//  (define-scenario (cache-check spot-accidental-xml-data)
+            "-o",
-//    "Prints helpful message if XML metadata given"
+            &md,
-//    (with-cache-xml (xml)
+            "--debug-override-metadata-version",
-//      (system (fmt #f "man bash >> " xml))
+            "12345"
-//      (run-fail-rcv (_ stderr) (cache-check xml)
+        ],
-//        (assert-matches ".*This looks like XML.  cache_check only checks the binary metadata format." stderr))))
+    )?;
-//
+    run_fail(CACHE_CHECK, args![&md])?;
+    Ok(())
+}

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::*;
@@ -46,7 +50,7 @@ impl<'a> Program<'a> for CacheDump {
 
 impl<'a> InputProgram<'a> for CacheDump {
     fn mk_valid_input(td: &mut TestDir) -> Result<std::path::PathBuf> {
-        common::thin::mk_valid_md(td) // FIXME: generate cache metadata
+        mk_valid_md(td)
     }
 
     fn file_not_found() -> &'a str {
@@ -75,13 +79,27 @@ test_unreadable_input_file!(CacheDump);
 
 //------------------------------------------
 
-/*
+// TODO: share with thin_dump
-  (define-scenario (cache-dump restore-is-noop)
+#[test]
-    "cache_dump followed by cache_restore is a noop."
+fn dump_restore_cycle() -> Result<()> {
-    (with-valid-metadata (md)
+    let mut td = TestDir::new()?;
-      (run-ok-rcv (d1-stdout _) (cache-dump md)
+    let md = mk_valid_md(&mut td)?;
-        (with-temp-file-containing ((xml "cache.xml" d1-stdout))
+    let output = run_ok_raw(CACHE_DUMP, args![&md])?;
-          (run-ok (cache-restore "-i" xml "-o" md))
+
-          (run-ok-rcv (d2-stdout _) (cache-dump md)
+    let xml = td.mk_path("meta.xml");
-            (assert-equal d1-stdout d2-stdout))))))
+    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(())
+}

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} <input metadata (binary format)>\n  \
+                       {-o|--output} <output metadata (binary format)>\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<std::path::PathBuf> {
+        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);
+
+//-----------------------------------------

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} <input xml file>\n  \
+                       {-o|--output} <output device or file>\n  \
+                       {-q|--quiet}\n  \
+                       {--metadata-version} <1 or 2>\n  \
+                       {-V|--version}\n\
+                     \n  \
+                       {--debug-override-metadata-version} <integer>\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<std::path::PathBuf> {
+        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(())
+}
+
+//-----------------------------------------

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<PathBuf> {
+    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<PathBuf> {
+    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)
+}
+
+//-----------------------------------------------

tests/common/cache_xml_generator.rs

@@ -60,26 +60,26 @@ impl XmlGen for CacheGen {
             hint_width: 4,
         })?;
 
-        let mut cblocks = Vec::new();
+        let nr_resident = (self.nr_cache_blocks * self.percent_resident as u32) / 100u32;
-        for n in 0..self.nr_cache_blocks {
+        let mut cblocks = (0..self.nr_cache_blocks).collect::<Vec<u32>>();
-            cblocks.push(n);
-        }
         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,
                 })?;

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

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;

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(<P as OutputProgram>::file_not_found()));
+    assert!(stderr.contains(<P as MetadataWriter>::file_not_found()));
     Ok(())
 }
 
@@ -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])?;
@@ -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)?;

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> {}
 
 //------------------------------------------

tests/common/target.rs

@@ -34,13 +34,15 @@ 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");
 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
@@ -61,7 +63,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";

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,
     }

tests/thin_check.rs

@@ -70,7 +70,7 @@ impl<'a> InputProgram<'a> for ThinCheck {
     }
 }
 
-impl<'a> BinaryInputProgram<'_> for ThinCheck {}
+impl<'a> MetadataReader<'_> for ThinCheck {}
 
 //------------------------------------------
 

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;
 
@@ -114,6 +113,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()?;
 
@@ -128,27 +128,33 @@ 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)?;
     let output = run_ok_raw(THIN_DUMP, args![&md, flag, value])?;
 
+    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\"")
 }
@@ -158,28 +164,22 @@ 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(
+    let before = run_ok_raw(THIN_DUMP, args![&md])?;
-        THIN_DUMP,
-        args![
-            "--transaction-id=5",
-            "--data-block-size=128",
-            "--nr-data-blocks=4096000",
-            &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
         ],
     )?;
+    if !cfg!(feature = "rust_tests") {
         assert_eq!(after.stderr.len(), 0);
+    }
     assert_eq!(before.stdout, after.stdout);
 
     Ok(())
@@ -190,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)?;
@@ -199,7 +200,7 @@ fn missing_transaction_id() -> Result<()> {
         args![
             "--repair",
             "--data-block-size=128",
-            "--nr-data-blocks=4096000",
+            "--nr-data-blocks=20480",
             &md
         ],
     )?;
@@ -216,8 +217,8 @@ fn missing_data_block_size() -> Result<()> {
         THIN_DUMP,
         args![
             "--repair",
-            "--transaction-id=5",
+            "--transaction-id=1",
-            "--nr-data-blocks=4096000",
+            "--nr-data-blocks=20480",
             &md
         ],
     )?;
@@ -226,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)?;
@@ -234,7 +236,7 @@ fn missing_nr_data_blocks() -> Result<()> {
         THIN_DUMP,
         args![
             "--repair",
-            "--transaction-id=5",
+            "--transaction-id=1",
             "--data-block-size=128",
             &md
         ],

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
     }

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
     }

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,25 +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 file_not_found() -> &'a str {
-        cpp_msg::FILE_NOT_FOUND
-    }
-
     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 {
+        msg::FILE_NOT_FOUND
     }
 }
 
@@ -108,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)?;
@@ -120,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\"")
 }
@@ -139,24 +151,18 @@ 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(
+    let original = run_ok_raw(THIN_DUMP, args![&md1])?;
-        THIN_DUMP,
+    if !cfg!(feature = "rust_tests") {
-        args![
-            "--transaction-id=5",
-            "--data-block-size=128",
-            "--nr-data-blocks=4096000",
-            &md1
-        ],
-    )?;
         assert_eq!(original.stderr.len(), 0);
+    }
     damage_superblock(&md1)?;
     let md2 = mk_zeroed_md(&mut td)?;
     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",
@@ -164,7 +170,9 @@ fn superblock_succeeds() -> Result<()> {
         ],
     )?;
     let repaired = run_ok_raw(THIN_DUMP, args![&md2])?;
+    if !cfg!(feature = "rust_tests") {
         assert_eq!(repaired.stderr.len(), 0);
+    }
     assert_eq!(original.stdout, repaired.stdout);
     Ok(())
 }
@@ -184,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",
     )
 }
@@ -195,16 +204,17 @@ fn missing_transaction_id() -> Result<()> {
 #[test]
 fn missing_data_block_size() -> Result<()> {
     missing_thing(
-        "--transaction-id=5",
+        "--transaction-id=1",
-        "--nr-data-blocks=4096000",
+        "--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",
     )

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<std::path::PathBuf> {
-        mk_valid_md(td)
+        mk_valid_xml(td)
     }
 
     fn file_not_found() -> &'a str {
@@ -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
@@ -122,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)?;
@@ -136,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\"")
 }

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;

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);