From 1dd7b454bbfa2b4edfd86abd7fdcc234ad9551eb Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Tue, 8 Oct 2019 14:34:24 +0100 Subject: [PATCH] [thin_repair, cache_repair] Check input file exists earlier and zero superblock if we fail part way through a repair. bz1499781 --- caching/cache_repair.cc | 8 +++++- functional-tests/cache-functional-tests.scm | 32 +++++++++++++++++++-- functional-tests/era-functional-tests.scm | 4 +-- functional-tests/functional-tests.scm | 4 +-- functional-tests/thin-functional-tests.scm | 27 +++++++++++++++-- thin-provisioning/thin_repair.cc | 8 +++++- 6 files changed, 72 insertions(+), 11 deletions(-) diff --git a/caching/cache_repair.cc b/caching/cache_repair.cc index 9587d5f..bc365c9 100644 --- a/caching/cache_repair.cc +++ b/caching/cache_repair.cc @@ -2,6 +2,7 @@ #include #include +#include "base/file_utils.h" #include "base/output_file_requirements.h" #include "caching/commands.h" #include "caching/metadata.h" @@ -29,12 +30,17 @@ namespace { } int repair(string const &old_path, string const &new_path) { + bool metadata_touched = false; try { + file_utils::check_file_exists(new_path); + metadata_touched = true; metadata_dump(open_metadata_for_read(old_path), output_emitter(new_path), true); } catch (std::exception &e) { + if (metadata_touched) + file_utils::zero_superblock(new_path); cerr << e.what() << endl; return 1; } @@ -110,7 +116,7 @@ cache_repair_cmd::run(int argc, char **argv) check_output_file_requirements(*output_path); else { - cerr << "no output file provided" << endl; + cerr << "No output file provided." << endl; usage(cerr); return 1; } diff --git a/functional-tests/cache-functional-tests.scm b/functional-tests/cache-functional-tests.scm index 0e70b94..9b3a203 100644 --- a/functional-tests/cache-functional-tests.scm +++ b/functional-tests/cache-functional-tests.scm @@ -15,6 +15,7 @@ (define-tool cache-dump) (define-tool cache-restore) (define-tool cache-metadata-size) + (define-tool cache-repair) (define-syntax with-cache-xml (syntax-rules () @@ -35,7 +36,8 @@ (syntax-rules () ((_ (md) b1 b2 ...) (with-temp-file-sized ((md "cache.bin" (to-bytes (meg 4)))) - b1 b2 ...)))) + (system (fmt #f "dd if=/usr/bin/ls of=" md " bs=4096 > /dev/null 2>&1")) + b1 b2 ...)))) (define-syntax with-empty-metadata (syntax-rules () @@ -180,7 +182,7 @@ "the input file can't be found" (with-empty-metadata (md) (run-fail-rcv (_ stderr) (cache-restore "-i no-such-file -o" md) - (assert-superblock-untouched md) + (assert-superblock-all-zeroes md) (assert-starts-with "Couldn't stat file" stderr)))) (define-scenario (cache-restore garbage-input-file) @@ -188,7 +190,7 @@ (with-empty-metadata (md) (with-temp-file-sized ((xml "cache.xml" 4096)) (run-fail-rcv (_ stderr) (cache-restore "-i" xml "-o" md) - (assert-superblock-untouched md))))) + (assert-superblock-all-zeroes md))))) (define-scenario (cache-restore missing-output-file) "the output file can't be found" @@ -354,4 +356,28 @@ (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) + (run-fail-rcv (_ stderr) (cache-repair "-i no-such-file -o" md) + (assert-superblock-all-zeroes md) + (assert-starts-with "Couldn't stat path" stderr)))) + + (define-scenario (cache-repair garbage-input-file) + "the input file is just zeroes" + (with-empty-metadata (md1) + (with-corrupt-metadata (md2) + (run-fail-rcv (_ stderr) (cache-repair "-i" md1 "-o" md2) + (assert-superblock-all-zeroes md2))))) + + (define-scenario (cache-repair missing-output-file) + "the output file can't be found" + (with-cache-xml (xml) + (run-fail-rcv (_ stderr) (cache-repair "-i" xml) + (assert-starts-with "No output file provided." stderr)))) + ) diff --git a/functional-tests/era-functional-tests.scm b/functional-tests/era-functional-tests.scm index 373fe34..890f0ff 100644 --- a/functional-tests/era-functional-tests.scm +++ b/functional-tests/era-functional-tests.scm @@ -153,7 +153,7 @@ "the input file can't be found" (with-empty-metadata (md) (run-fail-rcv (_ stderr) (era-restore "-i no-such-file -o" md) - (assert-superblock-untouched md) + (assert-superblock-all-zeroes md) (assert-starts-with "Couldn't stat file" stderr)))) (define-scenario (era-restore garbage-input-file) @@ -161,7 +161,7 @@ (with-empty-metadata (md) (with-temp-file-sized ((xml "era.xml" 4096)) (run-fail-rcv (_ stderr) (era-restore "-i " xml "-o" md) - (assert-superblock-untouched md))))) + (assert-superblock-all-zeroes md))))) (define-scenario (era-restore output-unspecified) "Fails if no metadata dev specified" diff --git a/functional-tests/functional-tests.scm b/functional-tests/functional-tests.scm index aa5b95c..758498e 100644 --- a/functional-tests/functional-tests.scm +++ b/functional-tests/functional-tests.scm @@ -21,7 +21,7 @@ assert-eof assert-starts-with assert-matches - assert-superblock-untouched + assert-superblock-all-zeroes assert-member? assert-raises-thunk assert-raises @@ -259,7 +259,7 @@ (loop (+ i 1)) #f))))) - (define (assert-superblock-untouched md) + (define (assert-superblock-all-zeroes md) (with-bcache (cache md 1) (with-block (b cache 0 (get-flags)) (unless (all-zeroes? (block-data b) 4096) diff --git a/functional-tests/thin-functional-tests.scm b/functional-tests/thin-functional-tests.scm index d096b78..6bb7d24 100644 --- a/functional-tests/thin-functional-tests.scm +++ b/functional-tests/thin-functional-tests.scm @@ -37,10 +37,12 @@ b1 b2 ...))))) ;;; It would be nice if the metadata was at least similar to valid data. + ;;; Here I'm just using the start of the ls binary as 'random' data. (define-syntax with-corrupt-metadata (syntax-rules () ((_ (md) b1 b2 ...) (with-temp-file-sized ((md "thin.bin" (meg 4))) + (system (fmt #f "dd if=/usr/bin/ls of=" md " bs=4096 > /dev/null 2>&1")) b1 b2 ...)))) (define-syntax with-empty-metadata @@ -167,7 +169,7 @@ "the input file can't be found" (with-empty-metadata (md) (run-fail-rcv (_ stderr) (thin-restore "-i no-such-file -o" md) - (assert-superblock-untouched md) + (assert-superblock-all-zeroes md) (assert-starts-with "Couldn't stat file" stderr)))) (define-scenario (thin-restore garbage-input-file) @@ -175,7 +177,7 @@ (with-empty-metadata (md) (with-temp-file-sized ((xml "thin.xml" 4096)) (run-fail-rcv (_ stderr) (thin-restore "-i " xml "-o" md) - (assert-superblock-untouched md))))) + (assert-superblock-all-zeroes md))))) (define-scenario (thin-restore missing-output-file) "the output file can't be found" @@ -333,4 +335,25 @@ (with-empty-metadata (md) (run-fail-rcv (_ stderr) (thin-repair "-i" xml "-o" md) #t)))) + + (define-scenario (thin-repair missing-input-file) + "the input file can't be found" + (with-empty-metadata (md) + (run-fail-rcv (_ stderr) (thin-repair "-i no-such-file -o" md) + (assert-superblock-all-zeroes md) + (assert-starts-with "Couldn't stat file" stderr)))) + + (define-scenario (thin-repair garbage-input-file) + "the input file is just zeroes" + (with-empty-metadata (md1) + (with-corrupt-metadata (md2) + (run-fail-rcv (_ stderr) (thin-repair "-i " md1 "-o" md2) + (assert-superblock-all-zeroes md2))))) + + (define-scenario (thin-repair missing-output-file) + "the output file can't be found" + (with-thin-xml (xml) + (run-fail-rcv (_ stderr) (thin-repair "-i " xml) + (assert-starts-with "No output file provided." stderr)))) + ) diff --git a/thin-provisioning/thin_repair.cc b/thin-provisioning/thin_repair.cc index e22b193..6f464e2 100644 --- a/thin-provisioning/thin_repair.cc +++ b/thin-provisioning/thin_repair.cc @@ -2,6 +2,7 @@ #include #include +#include "base/file_utils.h" #include "base/output_file_requirements.h" #include "persistent-data/file_utils.h" #include "thin-provisioning/commands.h" @@ -17,15 +18,20 @@ using namespace thin_provisioning; namespace { int repair(string const &old_path, string const &new_path) { + bool metadata_touched = false; try { // block size gets updated by the restorer block_manager<>::ptr new_bm = open_bm(new_path, block_manager<>::READ_WRITE); + file_utils::check_file_exists(old_path); + metadata_touched = true; metadata::ptr new_md(new metadata(new_bm, metadata::CREATE, 128, 0)); emitter::ptr e = create_restore_emitter(new_md); block_manager<>::ptr old_bm = open_bm(old_path, block_manager<>::READ_ONLY); metadata_repair(old_bm, e); } catch (std::exception &e) { + if (metadata_touched) + file_utils::zero_superblock(new_path); cerr << e.what() << endl; return 1; } @@ -101,7 +107,7 @@ thin_repair_cmd::run(int argc, char **argv) check_output_file_requirements(*output_path); else { - cerr << "no output file provided" << endl; + cerr << "No output file provided." << endl; usage(cerr); return 1; }