[thin_check] Change the policy of --clear-needs-check-flag to prevent error recurrence

- Disallow clearing the needs_check flag if there's any error,
  i.e., the metadata must be fully examined, and the result must
  be NO_ERROR.
- Disallow combining --clear-needs-check with -m, --super-blocks-only,
  --skip-mappings, --override-mapping-root, or --ignore-non-fatal-errors.

thin-provisioning/metadata_checker.cc

@@ -16,6 +16,7 @@
 // with thin-provisioning-tools.  If not, see
 // <http://www.gnu.org/licenses/>.
 
+#include "base/error_state.h"
 #include "base/nested_output.h"
 #include "persistent-data/file_utils.h"
 #include "persistent-data/space-maps/core.h"
@@ -437,8 +438,30 @@ namespace {
 			return true;
 		}
 
-		error_state get_error() const {
-			return err_;
+		bool clear_needs_check_flag() {
+			if (!verify_preconditions_before_fixing()) {
+				out_ << "metadata has not been fully examined" << end_message();
+				return false;
+			}
+
+			if (err_ != NO_ERROR)
+				return false;
+
+			block_manager::ptr bm = open_bm(path_, block_manager::READ_WRITE);
+			superblock_detail::superblock sb = read_superblock(bm);
+			sb.set_needs_check_flag(false);
+			write_superblock(bm, sb);
+
+			out_ << "cleared needs_check flag" << end_message();
+
+			return true;
+		}
+
+		bool get_status() const {
+			if (options_.ignore_non_fatal_)
+				return (err_ == FATAL) ? false : true;
+
+			return (err_ == NO_ERROR) ? true : false;
 		}
 
 	private:
@@ -529,7 +552,8 @@ check_options::check_options()
 	  data_mapping_opts_(DATA_MAPPING_LEVEL2),
 	  sm_opts_(SPACE_MAP_FULL),
 	  ignore_non_fatal_(false),
-	  fix_metadata_leaks_(false) {
+	  fix_metadata_leaks_(false),
+	  clear_needs_check_(false) {
 }
 
 void check_options::set_superblock_only() {
@@ -559,8 +583,12 @@ void check_options::set_fix_metadata_leaks() {
 	fix_metadata_leaks_ = true;
 }
 
+void check_options::set_clear_needs_check() {
+	clear_needs_check_ = true;
+}
+
 bool check_options::check_conformance() {
-	if (fix_metadata_leaks_) {
+	if (fix_metadata_leaks_ || clear_needs_check_) {
 		if (ignore_non_fatal_) {
 			cerr << "cannot perform fix by ignoring non-fatal errors" << endl;
 			return false;
@@ -588,7 +616,7 @@ bool check_options::check_conformance() {
 
 //----------------------------------------------------------------
 
-base::error_state
+bool
 thin_provisioning::check_metadata(std::string const &path,
 				  check_options const &check_opts,
 				  output_options output_opts)
@@ -598,8 +626,11 @@ thin_provisioning::check_metadata(std::string const &path,
 	checker.check();
 	if (check_opts.fix_metadata_leaks_)
 		checker.fix_metadata_leaks();
+	if (check_opts.fix_metadata_leaks_ ||
+	    check_opts.clear_needs_check_)
+		checker.clear_needs_check_flag();
 
-	return checker.get_error();
+	return checker.get_status();
 }
 
 //----------------------------------------------------------------

thin-provisioning/metadata_checker.h

@@ -19,7 +19,6 @@
 #ifndef METADATA_CHECKER_H
 #define METADATA_CHECKER_H
 
-#include "base/error_state.h"
 #include "block-cache/block_cache.h"
 #include "persistent-data/block.h"
 
@@ -47,6 +46,7 @@ namespace thin_provisioning {
 		void set_metadata_snap();
 		void set_ignore_non_fatal();
 		void set_fix_metadata_leaks();
+		void set_clear_needs_check();
 
 		bool use_metadata_snap_;
 		data_mapping_options data_mapping_opts_;
@@ -54,6 +54,7 @@ namespace thin_provisioning {
 		boost::optional<bcache::block_address> override_mapping_root_;
 		bool ignore_non_fatal_;
 		bool fix_metadata_leaks_;
+		bool clear_needs_check_;
 	};
 
 	enum output_options {
@@ -61,7 +62,7 @@ namespace thin_provisioning {
 		OUTPUT_QUIET,
 	};
 
-	base::error_state
+	bool
 	check_metadata(std::string const &path,
 		       check_options const &check_opts,
 		       output_options output_opts);

thin-provisioning/thin_check.cc

@@ -43,27 +43,13 @@ using namespace thin_provisioning;
 namespace {
 	struct flags {
 		flags()
-			: ignore_non_fatal_errors(false),
-			  quiet(false),
-			  clear_needs_check_flag_on_success(false) {
+			: quiet(false) {
 		}
 
 		check_options check_opts;
-
-		bool ignore_non_fatal_errors;
-
 		bool quiet;
-		bool clear_needs_check_flag_on_success;
 	};
 
-	void clear_needs_check(string const &path) {
-		block_manager::ptr bm = open_bm(path, block_manager::READ_WRITE);
-
-		superblock_detail::superblock sb = read_superblock(bm);
-		sb.set_needs_check_flag(false);
-		write_superblock(bm, sb);
-	}
-
 	// Returns 0 on success, 1 on failure (this gets returned directly
 	// by main).
 	int check(string const &path, flags fs) {
@@ -77,15 +63,7 @@ namespace {
 			}
 
 			output_options output_opts = !fs.quiet ? OUTPUT_NORMAL : OUTPUT_QUIET;
-			error_state err = check_metadata(path, fs.check_opts, output_opts);
-
-			if (fs.ignore_non_fatal_errors)
-				success = (err == FATAL) ? false : true;
-			else
-				success = (err == NO_ERROR) ? true : false;
-
-			if (success && fs.clear_needs_check_flag_on_success)
-				clear_needs_check(path);
+			success = check_metadata(path, fs.check_opts, output_opts);
 
 		} catch (std::exception &e) {
 			if (!fs.quiet)
@@ -173,13 +151,12 @@ thin_check_cmd::run(int argc, char **argv)
 
 		case 3:
 			// ignore-non-fatal-errors
-			fs.ignore_non_fatal_errors = true;
 			fs.check_opts.set_ignore_non_fatal();
 			break;
 
 		case 4:
 			// clear needs-check flag
-			fs.clear_needs_check_flag_on_success = true;
+			fs.check_opts.set_clear_needs_check();
 			break;
 
 		case 5:
@@ -198,12 +175,6 @@ thin_check_cmd::run(int argc, char **argv)
 		}
 	}
 
-	if (fs.clear_needs_check_flag_on_success && fs.check_opts.use_metadata_snap_) {
-		cerr << "--metadata-snap cannot be combined with --clear-needs-check-flag." << endl;
-		usage(cerr);
-		exit(1);
-	}
-
 	if (!fs.check_opts.check_conformance()) {
 		usage(cerr);
 		exit(1);