[thin_check] Start refactoring metadata_checker.

Makefile.in

@@ -48,12 +48,14 @@ SOURCE=\
 	\
 	cache/metadata_disk_structures.cc \
 	\
+	thin-provisioning/file_utils.cc \
 	thin-provisioning/human_readable_format.cc \
 	thin-provisioning/metadata.cc \
 	thin-provisioning/metadata_checker.cc \
 	thin-provisioning/metadata_disk_structures.cc \
 	thin-provisioning/metadata_dumper.cc \
 	thin-provisioning/restore_emitter.cc \
+	thin-provisioning/superblock_validator.cc \
 	thin-provisioning/thin_pool.cc \
 	thin-provisioning/xml_format.cc
 PDATA_OBJECTS=$(subst .cc,.o,$(SOURCE))
@@ -130,9 +132,11 @@ THIN_CHECK_SOURCE=\
 	persistent-data/space-maps/recursive.cc \
 	persistent-data/space-maps/careful_alloc.cc \
 	persistent-data/transaction_manager.cc \
+	thin-provisioning/file_utils.cc \
 	thin-provisioning/metadata.cc \
 	thin-provisioning/metadata_checker.cc \
-	thin-provisioning/metadata_disk_structures.cc
+	thin-provisioning/metadata_disk_structures.cc \
+	thin-provisioning/superblock_validator.cc
 
 THIN_DEBUG_OBJECTS=$(subst .cc,.o,$(THIN_DEBUG_SOURCE))
 THIN_DUMP_OBJECTS=$(subst .cc,.o,$(THIN_DUMP_SOURCE))

features/step_definitions/thin_steps.rb

@@ -7,7 +7,20 @@ Given(/^valid metadata$/) do
   run_simple("thin_restore -i #{xml_file} -o #{dev_file}")
 end
 
-When(/^I run thin_check with (.*?)$/) do |opts|
+Given(/^a corrupt superblock$/) do
-  run "thin_check #{opts} #{dev_file}"
+  in_current_dir do
+    write_valid_xml(xml_file)
+  end
+
+  run_simple("dd if=/dev/zero of=#{dev_file} bs=4k count=1024")
+  run_simple("thin_restore -i #{xml_file} -o #{dev_file}")
+
+  in_current_dir do
+    corrupt_block(0)
+  end
+end
+
+When(/^I run thin_check with (.*?)$/) do |opts|
+  run_simple("thin_check #{opts} #{dev_file}", false)
 end
 

features/support/world_extensions.rb

@@ -11,6 +11,10 @@ module ThinpWorld
     'metadata.bin'
   end
 
+  def corrupt_block(n)
+    write_block(n, change_random_byte(read_block(n)))
+  end
+
   # FIXME: we should really break out the xml stuff from
   # thinp-test-suite and put in a gem in this repo.
   def write_valid_xml(path)
@@ -24,6 +28,32 @@ module ThinpWorld
 EOF
     end
   end
+
+  private
+  def read_block(n)
+    b = nil
+
+    File.open(dev_file, "r") do |f|
+      f.seek(n * 4096)
+      b = f.read(4096)
+    end
+
+    b
+  end
+
+  def write_block(n, b)
+    File.open(dev_file, "w") do |f|
+      f.seek(n * 4096)
+      f.write(b)
+    end
+  end
+
+  def change_random_byte(b)
+    i = rand(b.size)
+    puts "changing byte #{i}"
+    b.setbyte(i, (b.getbyte(i) + 1) % 256)
+    b
+  end
 end
 
 World(ThinpWorld)

features/thin_check.feature

@@ -40,8 +40,17 @@ Feature: thin_check
       {--super-block-only}
     """
 
-  @announce
+  Scenario: Unrecognised option should cause failure
+    When I run `thin_check --hedeghogs-only`
+    Then it should fail
+
   Scenario: --super-block-only check passes on valid metadata
     Given valid metadata
     When I run thin_check with --super-block-only
-    Then it should pass
+    Then it should pass
+
+  @announce
+  Scenario: --super-block-only check fails with corrupt superblock
+    Given a corrupt superblock
+    When I run thin_check with --super-block-only
+    Then it should fail

thin-provisioning/file_utils.cc

@@ -0,0 +1,48 @@
+#include "persistent-data/math_utils.h"
+#include "thin-provisioning/file_utils.h"
+
+#include <linux/fs.h>
+#include <sys/ioctl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+using namespace base;
+
+//----------------------------------------------------------------
+
+block_address
+thin_provisioning::get_nr_blocks(string const &path)
+{
+	struct stat info;
+	block_address nr_blocks;
+
+	int r = ::stat(path.c_str(), &info);
+	if (r)
+		throw runtime_error("Couldn't stat dev path");
+
+	if (S_ISREG(info.st_mode) && info.st_size)
+		nr_blocks = div_up<block_address>(info.st_size, MD_BLOCK_SIZE);
+
+	else if (S_ISBLK(info.st_mode)) {
+		// To get the size of a block device we need to
+		// open it, and then make an ioctl call.
+		int fd = ::open(path.c_str(), O_RDONLY);
+		if (fd < 0)
+			throw runtime_error("couldn't open block device to ascertain size");
+
+		r = ::ioctl(fd, BLKGETSIZE64, &nr_blocks);
+		if (r) {
+			::close(fd);
+			throw runtime_error("ioctl BLKGETSIZE64 failed");
+		}
+		::close(fd);
+		nr_blocks = div_down<block_address>(nr_blocks, MD_BLOCK_SIZE);
+	} else
+		// FIXME: needs a better message
+		throw runtime_error("bad path");
+
+	return nr_blocks;
+}
+
+//----------------------------------------------------------------

thin-provisioning/file_utils.h

@@ -0,0 +1,14 @@
+#ifndef THIN_FILE_UTILS_H
+#define THIN_FILE_UTILS_H
+
+#include "persistent-data/block.h"
+
+//----------------------------------------------------------------
+
+namespace thin_provisioning {
+	block_address get_nr_blocks(string const &path);
+}
+
+//----------------------------------------------------------------
+
+#endif

thin-provisioning/metadata.cc

@@ -16,7 +16,9 @@
 // with thin-provisioning-tools.  If not, see
 // <http://www.gnu.org/licenses/>.
 
+#include "thin-provisioning/file_utils.h"
 #include "thin-provisioning/metadata.h"
+#include "thin-provisioning/superblock_validator.h"
 
 #include "persistent-data/math_utils.h"
 #include "persistent-data/space-maps/core.h"
@@ -28,66 +30,13 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+using namespace base;
 using namespace thin_provisioning;
 
 //----------------------------------------------------------------
 
 namespace {
-	uint32_t const SUPERBLOCK_MAGIC = 27022010;
-        uint32_t const VERSION = 1;
 	unsigned const METADATA_CACHE_SIZE = 1024;
-        unsigned const SECTOR_TO_BLOCK_SHIFT = 3;
-	uint32_t const SUPERBLOCK_CSUM_SEED = 160774;
-
-	struct superblock_validator : public block_manager<>::validator {
-		virtual void check(buffer<> const &b, block_address location) const {
-			superblock_disk const *sbd = reinterpret_cast<superblock_disk const *>(&b);
-			crc32c sum(SUPERBLOCK_CSUM_SEED);
-			sum.append(&sbd->flags_, MD_BLOCK_SIZE - sizeof(uint32_t));
-			if (sum.get_sum() != to_cpu<uint32_t>(sbd->csum_))
-				throw checksum_error("bad checksum in superblock");
-		}
-
-		virtual void prepare(buffer<> &b, block_address location) const {
-			superblock_disk *sbd = reinterpret_cast<superblock_disk *>(&b);
-			crc32c sum(SUPERBLOCK_CSUM_SEED);
-			sum.append(&sbd->flags_, MD_BLOCK_SIZE - sizeof(uint32_t));
-			sbd->csum_ = to_disk<base::le32>(sum.get_sum());
-		}
-	};
-
-	block_address
-	get_nr_blocks(string const &path) {
-		struct stat info;
-		block_address nr_blocks;
-
-		int r = ::stat(path.c_str(), &info);
-		if (r)
-			throw runtime_error("Couldn't stat dev path");
-
-		if (S_ISREG(info.st_mode) && info.st_size)
-			nr_blocks = div_up<block_address>(info.st_size, MD_BLOCK_SIZE);
-
-		else if (S_ISBLK(info.st_mode)) {
-			// To get the size of a block device we need to
-			// open it, and then make an ioctl call.
-			int fd = ::open(path.c_str(), O_RDONLY);
-			if (fd < 0)
-				throw runtime_error("couldn't open block device to ascertain size");
-
-			r = ::ioctl(fd, BLKGETSIZE64, &nr_blocks);
-			if (r) {
-				::close(fd);
-				throw runtime_error("ioctl BLKGETSIZE64 failed");
-			}
-			::close(fd);
-			nr_blocks = div_down<block_address>(nr_blocks, MD_BLOCK_SIZE);
-		} else
-			// FIXME: needs a better message
-			throw runtime_error("bad path");
-
-		return nr_blocks;
-	}
 
 	transaction_manager::ptr
 	open_tm(string const &dev_path, bool writeable) {
@@ -106,8 +55,7 @@ namespace {
 	superblock
 	read_superblock(block_manager<>::ptr bm, block_address location = SUPERBLOCK_LOCATION) {
 		superblock sb;
-		block_manager<>::read_ref r = bm->read_lock(location,
+		block_manager<>::read_ref r = bm->read_lock(location, superblock_validator());
-							    mk_validator(new superblock_validator));
 		superblock_disk const *sbd = reinterpret_cast<superblock_disk const *>(&r.data());
 		superblock_traits::unpack(*sbd, sb);
 		return sb;
@@ -214,9 +162,7 @@ metadata::commit()
 	metadata_sm_->copy_root(&sb_.metadata_space_map_root_, sizeof(sb_.metadata_space_map_root_));
 	print_superblock(sb_);
 
-	superblock_validator v;
+	write_ref superblock = tm_->get_bm()->superblock_zero(SUPERBLOCK_LOCATION, superblock_validator());
-	write_ref superblock = tm_->get_bm()->superblock_zero(SUPERBLOCK_LOCATION,
-							      mk_validator(new superblock_validator()));
         superblock_disk *disk = reinterpret_cast<superblock_disk *>(superblock.data().raw());
 	superblock_traits::pack(sb_, *disk);
 }

thin-provisioning/metadata_checker.cc

@@ -16,7 +16,10 @@
 // with thin-provisioning-tools.  If not, see
 // <http://www.gnu.org/licenses/>.
 
+#include "thin-provisioning/file_utils.h"
+#include "thin-provisioning/metadata.h"
 #include "thin-provisioning/metadata_checker.h"
+#include "thin-provisioning/superblock_validator.h"
 
 using namespace thin_provisioning;
 
@@ -159,65 +162,116 @@ namespace {
 		sm->iterate(checker);
 		return checker.bad_ ? optional<error_set::ptr>(checker.errors_) : optional<error_set::ptr>();
 	}
+
+	class metadata_checker {
+	public:
+		metadata_checker(string const &dev_path)
+		: bm_(open_bm(dev_path)) {
+		}
+
+		boost::optional<error_set::ptr> check() {
+#if 1
+			// FIXME: finish
+			error_set::ptr errors(new error_set("Errors in metadata"));
+			superblock sb = read_superblock();
+			return errors;
+#else
+			error_set::ptr errors(new error_set("Errors in metadata"));
+
+			block_counter metadata_counter, data_counter;
+
+			if (md->sb_.metadata_snap_) {
+				block_manager<>::ptr bm = md->tm_->get_bm();
+
+
+				block_address root = md->sb_.metadata_snap_;
+
+				metadata_counter.inc(root);
+
+				superblock sb;
+				block_manager<>::read_ref r = bm->read_lock(root);
+				superblock_disk const *sbd = reinterpret_cast<superblock_disk const *>(&r.data());
+				superblock_traits::unpack(*sbd, sb);
+
+				metadata_counter.inc(sb.data_mapping_root_);
+				metadata_counter.inc(sb.device_details_root_);
+			}
+
+			mapping_validator::ptr mv(new mapping_validator(metadata_counter,
+									data_counter));
+			md->mappings_->visit(mv);
+
+			set<uint64_t> const &mapped_devs = mv->get_devices();
+			details_validator::ptr dv(new details_validator(metadata_counter));
+			md->details_->visit(dv);
+
+			set<uint64_t> const &details_devs = dv->get_devices();
+
+			for (set<uint64_t>::const_iterator it = mapped_devs.begin(); it != mapped_devs.end(); ++it)
+				if (details_devs.count(*it) == 0) {
+					ostringstream out;
+					out << "mapping exists for device " << *it
+					    << ", yet there is no entry in the details tree.";
+					throw runtime_error(out.str());
+				}
+
+			metadata_counter.inc(SUPERBLOCK_LOCATION);
+			md->metadata_sm_->check(metadata_counter);
+
+			md->data_sm_->check(metadata_counter);
+			errors->add_child(check_ref_counts("Errors in metadata block reference counts",
+							   metadata_counter, md->metadata_sm_));
+			errors->add_child(check_ref_counts("Errors in data block reference counts",
+							   data_counter, md->data_sm_));
+
+			return (errors->get_children().size() > 0) ?
+				optional<error_set::ptr>(errors) :
+				optional<error_set::ptr>();
+#endif
+		}
+
+	private:
+		static block_manager<>::ptr
+		open_bm(string const &dev_path) {
+			block_address nr_blocks = thin_provisioning::get_nr_blocks(dev_path);
+			return block_manager<>::ptr(new block_manager<>(dev_path, nr_blocks, 1, block_io<>::READ_ONLY));
+		}
+
+		// FIXME: common code with metadata.cc
+		superblock read_superblock() {
+			superblock sb;
+			block_manager<>::read_ref r = bm_->read_lock(SUPERBLOCK_LOCATION, superblock_validator());
+			superblock_disk const *sbd = reinterpret_cast<superblock_disk const *>(&r.data());
+			superblock_traits::unpack(*sbd, sb);
+			return sb;
+		}
+
+		typedef block_manager<>::read_ref read_ref;
+		typedef block_manager<>::write_ref write_ref;
+		typedef boost::shared_ptr<metadata> ptr;
+
+		block_manager<>::ptr bm_;
+#if 0
+		tm_ptr tm_;
+		superblock sb_;
+
+		checked_space_map::ptr metadata_sm_;
+		checked_space_map::ptr data_sm_;
+		detail_tree::ptr details_;
+		dev_tree::ptr mappings_top_level_;
+		mapping_tree::ptr mappings_;
+#endif
+	};
 }
 
 
 //----------------------------------------------------------------
 
 boost::optional<error_set::ptr>
-thin_provisioning::metadata_check(metadata::ptr md)
+thin_provisioning::metadata_check(std::string const &dev_path)
 {
-	error_set::ptr errors(new error_set("Errors in metadata"));
+	metadata_checker checker(dev_path);
-
+	return checker.check();
-	block_counter metadata_counter, data_counter;
-
-	if (md->sb_.metadata_snap_) {
-		block_manager<>::ptr bm = md->tm_->get_bm();
-
-
-		block_address root = md->sb_.metadata_snap_;
-
-		metadata_counter.inc(root);
-
-		superblock sb;
-		block_manager<>::read_ref r = bm->read_lock(root);
-		superblock_disk const *sbd = reinterpret_cast<superblock_disk const *>(&r.data());
-		superblock_traits::unpack(*sbd, sb);
-
-		metadata_counter.inc(sb.data_mapping_root_);
-		metadata_counter.inc(sb.device_details_root_);
-	}
-
-	mapping_validator::ptr mv(new mapping_validator(metadata_counter,
-							data_counter));
-	md->mappings_->visit(mv);
-
-	set<uint64_t> const &mapped_devs = mv->get_devices();
-	details_validator::ptr dv(new details_validator(metadata_counter));
-	md->details_->visit(dv);
-
-	set<uint64_t> const &details_devs = dv->get_devices();
-
-	for (set<uint64_t>::const_iterator it = mapped_devs.begin(); it != mapped_devs.end(); ++it)
-		if (details_devs.count(*it) == 0) {
-			ostringstream out;
-			out << "mapping exists for device " << *it
-			    << ", yet there is no entry in the details tree.";
-			throw runtime_error(out.str());
-		}
-
-	metadata_counter.inc(SUPERBLOCK_LOCATION);
-	md->metadata_sm_->check(metadata_counter);
-
-	md->data_sm_->check(metadata_counter);
-	errors->add_child(check_ref_counts("Errors in metadata block reference counts",
-					   metadata_counter, md->metadata_sm_));
-	errors->add_child(check_ref_counts("Errors in data block reference counts",
-					   data_counter, md->data_sm_));
-
-	return (errors->get_children().size() > 0) ?
-		optional<error_set::ptr>(errors) :
-		optional<error_set::ptr>();
 }
 
 //----------------------------------------------------------------

thin-provisioning/metadata_checker.h

@@ -20,12 +20,13 @@
 #define METADATA_CHECKER_H
 
 #include "persistent-data/error_set.h"
-#include "thin-provisioning/metadata.h"
 
 //----------------------------------------------------------------
 
 namespace thin_provisioning {
-	boost::optional<persistent_data::error_set::ptr> metadata_check(metadata::ptr md);
+	// FIXME: pass in flags like --super-block-only
+	boost::optional<persistent_data::error_set::ptr>
+	metadata_check(std::string const &dev_path);
 }
 
 //----------------------------------------------------------------

thin-provisioning/superblock_validator.cc

@@ -0,0 +1,40 @@
+#include "thin-provisioning/superblock_validator.h"
+
+#include "thin-provisioning/metadata_disk_structures.h"
+
+using namespace thin_provisioning;
+
+//----------------------------------------------------------------
+
+namespace {
+        uint32_t const VERSION = 1;
+        unsigned const SECTOR_TO_BLOCK_SHIFT = 3;
+	uint32_t const SUPERBLOCK_CSUM_SEED = 160774;
+
+	struct validator : public block_manager<>::validator {
+		virtual void check(buffer<> const &b, block_address location) const {
+			superblock_disk const *sbd = reinterpret_cast<superblock_disk const *>(&b);
+			crc32c sum(SUPERBLOCK_CSUM_SEED);
+			sum.append(&sbd->flags_, MD_BLOCK_SIZE - sizeof(uint32_t));
+			if (sum.get_sum() != to_cpu<uint32_t>(sbd->csum_))
+				throw checksum_error("bad checksum in superblock");
+		}
+
+		virtual void prepare(buffer<> &b, block_address location) const {
+			superblock_disk *sbd = reinterpret_cast<superblock_disk *>(&b);
+			crc32c sum(SUPERBLOCK_CSUM_SEED);
+			sum.append(&sbd->flags_, MD_BLOCK_SIZE - sizeof(uint32_t));
+			sbd->csum_ = to_disk<base::le32>(sum.get_sum());
+		}
+	};
+}
+
+//----------------------------------------------------------------
+
+block_manager<>::validator::ptr
+thin_provisioning::superblock_validator()
+{
+	return block_manager<>::validator::ptr(new validator);
+}
+
+//----------------------------------------------------------------

thin-provisioning/superblock_validator.h

@@ -0,0 +1,17 @@
+#ifndef THIN_SUPERBLOCK_VALIDATOR_H
+#define THIN_SUPERBLOCK_VALIDATOR_H
+
+#include "persistent-data/block.h"
+
+//----------------------------------------------------------------
+
+namespace thin_provisioning {
+	// FIXME: put with metadata disk structures?
+	uint32_t const SUPERBLOCK_MAGIC = 27022010;
+
+	block_manager<>::validator::ptr superblock_validator();
+}
+
+//----------------------------------------------------------------
+
+#endif

thin-provisioning/thin_check.cc

@@ -31,9 +31,7 @@ using namespace thin_provisioning;
 namespace {
 	int check(string const &path, bool quiet) {
 		try {
-			metadata::ptr md(new metadata(path, metadata::OPEN));
+			optional<error_set::ptr> maybe_errors = metadata_check(path);
-
-			optional<error_set::ptr> maybe_errors = metadata_check(md);
 			if (maybe_errors) {
 				if (!quiet)
 					cerr << error_selector(*maybe_errors, 3);
@@ -61,13 +59,13 @@ namespace {
 int main(int argc, char **argv)
 {
 	int c;
-	bool quiet = false;
+	bool quiet = false, superblock_only = false;
 	const char shortopts[] = "qhV";
 	const struct option longopts[] = {
 		{ "quiet", no_argument, NULL, 'q'},
 		{ "help", no_argument, NULL, 'h'},
 		{ "version", no_argument, NULL, 'V'},
-		{ "super-block-only", no_argument, NULL, 0},
+		{ "super-block-only", no_argument, NULL, 1},
 		{ NULL, no_argument, NULL, 0 }
 	};
 
@@ -85,6 +83,10 @@ int main(int argc, char **argv)
 			cout << THIN_PROVISIONING_TOOLS_VERSION << endl;
 			return 0;
 
+		case 1:
+			superblock_only = true;
+			break;
+
 		default:
 			usage(cerr, basename(argv[0]));
 			return 1;
@@ -97,5 +99,6 @@ int main(int argc, char **argv)
 		exit(1);
 	}
 
+
 	return check(argv[optind], quiet);
 }