Detect XML in *_check tools (#86)

* [*_check] Detect XML in cache_check and era_check

This is based on previous commit b10d8d4440.

* [*_check] Fix typo in check_superblock

caching/cache_check.cc

@@ -14,6 +14,7 @@
 
 #include "base/error_state.h"
 #include "base/error_string.h"
+#include "base/file_utils.h"
 #include "base/nested_output.h"
 #include "caching/commands.h"
 #include "caching/metadata.h"
@@ -212,12 +213,18 @@ namespace {
 
 	error_state metadata_check(string const &path, flags const &fs,
 				   bool &needs_check_set) {
-		block_manager<>::ptr bm = open_bm(path, block_manager<>::READ_ONLY);
-
 		nested_output out(cerr, 2);
 		if (fs.quiet_)
 			out.disable();
 
+		if (file_utils::get_file_length(path) < persistent_data::MD_BLOCK_SIZE) {
+			out << "Metadata device/file too small.  Is this binary metadata?"
+			    << end_message();
+			return FATAL;
+		}
+
+		block_manager<>::ptr bm = open_bm(path, block_manager<>::READ_ONLY);
+
 		superblock_reporter sb_rep(out);
 		mapping_reporter mapping_rep(out);
 		hint_reporter hint_rep(out);
@@ -229,8 +236,11 @@ namespace {
 			check_superblock(bm, bm->get_nr_blocks(), sb_rep);
 		}
 
-		if (sb_rep.get_error() == FATAL)
+		if (sb_rep.get_error() == FATAL) {
+			if (check_for_xml(bm))
+				out << "This looks like XML.  cache_check only checks the binary metadata format." << end_message();
 			return FATAL;
+		}
 
 		superblock sb = read_superblock(bm);
 		transaction_manager::ptr tm = open_tm(bm, SUPERBLOCK_LOCATION);

caching/superblock.cc

@@ -365,7 +365,7 @@ caching::check_superblock(superblock const &sb,
 
 	if (sb.magic != SUPERBLOCK_MAGIC) {
 		ostringstream msg;
-		msg << "magic in incorrect: " << sb.magic;
+		msg << "magic is incorrect: " << sb.magic;
 		visitor.visit(superblock_invalid(msg.str()));
 	}
 

era/era_check.cc

@@ -14,6 +14,7 @@
 
 #include "base/error_state.h"
 #include "base/error_string.h"
+#include "base/file_utils.h"
 #include "base/nested_output.h"
 #include "era/commands.h"
 #include "era/writeset_tree.h"
@@ -185,11 +186,19 @@ namespace {
 		return info;
 	}
 
-	error_state metadata_check(block_manager<>::ptr bm, flags const &fs) {
+	error_state metadata_check(string const &path, flags const &fs) {
 		nested_output out(cerr, 2);
 		if (fs.quiet_)
 			out.disable();
 
+		if (file_utils::get_file_length(path) < persistent_data::MD_BLOCK_SIZE) {
+			out << "Metadata device/file too small.  Is this binary metadata?"
+			    << end_message();
+			return FATAL;
+		}
+
+		block_manager<>::ptr bm = open_bm(path, block_manager<>::READ_ONLY);
+
 		superblock_reporter sb_rep(out);
 
 		out << "examining superblock" << end_message();
@@ -198,8 +207,11 @@ namespace {
 			check_superblock(bm, bm->get_nr_blocks(), sb_rep);
 		}
 
-		if (sb_rep.get_error() == FATAL)
+		if (sb_rep.get_error() == FATAL) {
+			if (check_for_xml(bm))
+				out << "This looks like XML.  era_check only checks the binary metadata format." << end_message();
 			return FATAL;
+		}
 
 		superblock sb = read_superblock(bm);
 		transaction_manager::ptr tm = open_tm(bm, SUPERBLOCK_LOCATION);
@@ -233,8 +245,7 @@ namespace {
 			throw runtime_error(msg.str());
 		}
 
-		block_manager<>::ptr bm = open_bm(path, block_manager<>::READ_ONLY);
-		err = metadata_check(bm, fs);
+		err = metadata_check(path, fs);
 
 		return err == NO_ERROR ? 0 : 1;
 	}

era/superblock.cc

@@ -283,7 +283,7 @@ era::check_superblock(superblock const &sb,
 
 	if (sb.magic != SUPERBLOCK_MAGIC) {
 		ostringstream msg;
-		msg << "magic in incorrect: " << sb.magic;
+		msg << "magic is incorrect: " << sb.magic;
 		visitor.visit(superblock_invalid(msg.str()));
 	}
 

functional-tests/cache-functional-tests.scm

@@ -133,12 +133,18 @@
         (cache-restore "-i" xml "-o" md "--debug-override-metadata-version" "12345")
         (run-fail "cache_check" md))))
 
-  (define-scenario (cache-check dont-repair-xml)
-    "Fails gracefully if run on XML rather than metadata"
+  (define-scenario (cache-check tiny-metadata)
+    "Prints helpful message in case XML metadata given"
     (with-cache-xml (xml)
-      (with-empty-metadata (md)
-        (receive (_ stderr) (run-fail "cache_check " xml)
-          #t))))
+      (receive (_ stderr) (run-fail "cache_check" xml)
+        (assert-starts-with "Metadata device/file too small.  Is this binary metadata?" stderr))))
+
+  (define-scenario (cache-check spot-accidental-xml-data)
+    "Prints helpful message if XML metadata given"
+    (with-cache-xml (xml)
+      (system (fmt #f "man bash >> " xml))
+      (receive (_ stderr) (run-fail "cache_check" xml)
+        (assert-matches ".*This looks like XML.  cache_check only checks the binary metadata format." stderr))))
 
   ;;;-----------------------------------------------------------
   ;;; cache_restore scenarios

functional-tests/era-functional-tests.scm

@@ -107,6 +107,19 @@
         (assert-eof stdout)
         (assert-eof stderr))))
 
+  (define-scenario (era-check tiny-metadata)
+    "Prints helpful message in case XML metadata given"
+    (with-era-xml (xml)
+      (receive (_ stderr) (run-fail "era_check" xml)
+        (assert-starts-with "Metadata device/file too small.  Is this binary metadata?" stderr))))
+
+  (define-scenario (era-check spot-accidental-xml-data)
+    "Prints helpful message if XML metadata given"
+    (with-era-xml (xml)
+      (system (fmt #f "man bash >> " xml))
+      (receive (_ stderr) (run-fail "era_check" xml)
+        (assert-matches ".*This looks like XML.  era_check only checks the binary metadata format." stderr))))
+
   ;;;-----------------------------------------------------------
   ;;; era_restore scenarios
   ;;;-----------------------------------------------------------

persistent-data/file_utils.cc

@@ -7,6 +7,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#include <string.h>
 #include <sstream>
 #include <unistd.h>
 
@@ -17,6 +18,14 @@ using namespace std;
 
 //----------------------------------------------------------------
 
+bool
+persistent_data::check_for_xml(block_manager<>::ptr bm) {
+	block_manager<>::read_ref b = bm->read_lock(0);
+	const char *data = reinterpret_cast<const char *>(b.data());
+	return (!strncmp(data, "<superblock", 11) || !strncmp(data, "<?xml", 5)
+	                       || !strncmp(data, "<!DOCTYPE", 9));
+}
+
 persistent_data::block_address
 persistent_data::get_nr_blocks(std::string const &path, sector_t block_size)
 {

persistent-data/file_utils.h

@@ -10,6 +10,7 @@
 
 // FIXME: move to a different unit
 namespace persistent_data {
+	bool check_for_xml(block_manager<>::ptr bm);
 	persistent_data::block_address get_nr_blocks(std::string const &path, sector_t block_size = MD_BLOCK_SIZE);
 	block_address get_nr_metadata_blocks(std::string const &path);
 

thin-provisioning/thin_check.cc

@@ -190,12 +190,6 @@ namespace {
 		return err;
 	}
 
-	void check_for_xml(block_manager<>::ptr bm, nested_output &out) {
-		block_manager<>::read_ref b = bm->read_lock(superblock_detail::SUPERBLOCK_LOCATION);
-		if (!strncmp(reinterpret_cast<const char *>(b.data()), "<superblock", 10))
-			out << "This looks like XML.  thin_check only checks the binary metadata format." << end_message();
-	}
-
 	block_address mapping_root(superblock_detail::superblock const &sb, flags const &fs)
 	{
 		return fs.override_mapping_root ? *fs.override_mapping_root : sb.data_mapping_root_;
@@ -225,7 +219,8 @@ namespace {
 		}
 
 		if (sb_rep.get_error() == FATAL) {
-			check_for_xml(bm, out);
+			if (check_for_xml(bm))
+				out << "This looks like XML.  thin_check only checks the binary metadata format." << end_message();
 			return FATAL;
 		}