[block] make sure we can change validators

persistent-data/block.h

@@ -119,7 +119,8 @@ namespace persistent_data {
 
 			void flush();
 
-			void change_validator(typename block_manager<BlockSize>::validator::ptr v);
+			void change_validator(typename block_manager<BlockSize>::validator::ptr v,
+					      bool check = true);
 
 			typename block_io<BlockSize>::ptr io_;
 			block_address location_;

persistent-data/block.tcc

@@ -210,8 +210,9 @@ block_manager<BlockSize>::block::block(typename block_io<BlockSize>::ptr io,
 	  dirty_(false)
 {
 	if (zero) {
+		// FIXME: duplicate memset
 		memset(data_->raw(), 0, BlockSize);
-		dirty_ = true;
+		dirty_ = true;	// redundant?
 	} else {
 		io_->read_buffer(location_, *data_);
 		validator_->check(*data_, location_);
@@ -237,14 +238,19 @@ block_manager<BlockSize>::block::flush()
 
 template <uint32_t BlockSize>
 void
-block_manager<BlockSize>::block::change_validator(typename block_manager<BlockSize>::validator::ptr v)
+block_manager<BlockSize>::block::change_validator(typename block_manager<BlockSize>::validator::ptr v,
+						  bool check)
 {
 	if (v.get() != validator_.get()) {
 		if (dirty_)
+			// It may have already happened, by calling
+			// this we ensure we're consistent.
 			validator_->prepare(*data_, location_);
 
 		validator_ = v;
-		validator_->check(*data_, location_);
+
+		if (check)
+			validator_->check(*data_, location_);
 	}
 }
 
@@ -383,7 +389,10 @@ block_manager<BlockSize>::write_lock(block_address location,
 		boost::optional<typename block::ptr> cached_block = cache_.get(location);
 
 		if (cached_block) {
-			(*cached_block)->check_write_lockable();
+			typename block::ptr cb = *cached_block;
+			cb->check_write_lockable();
+			cb->change_validator(v);
+
 			return write_ref(*this, *cached_block);
 		}
 
@@ -409,8 +418,11 @@ block_manager<BlockSize>::write_lock_zero(block_address location,
 
 		boost::optional<typename block::ptr> cached_block = cache_.get(location);
 		if (cached_block) {
-			(*cached_block)->check_write_lockable();
+			typename block::ptr cb = *cached_block;
+			cb->check_write_lockable();
+			cb->change_validator(v, false);
 			memset((*cached_block)->data_->raw(), 0, BlockSize);
+
 			return write_ref(*this, *cached_block);
 		}
 
@@ -436,9 +448,11 @@ block_manager<BlockSize>::superblock(block_address location,
 		boost::optional<typename block::ptr> cached_block = cache_.get(location);
 
 		if (cached_block) {
-			(*cached_block)->check_write_lockable();
+			typename block::ptr cb = *cached_block;
-			(*cached_block)->bt_ = BT_SUPERBLOCK;
+			cb->check_write_lockable();
-			(*cached_block)->validator_ = v;
+			cb->bt_ = BT_SUPERBLOCK;
+			cb->change_validator(v);
+
 			return write_ref(*this, *cached_block);
 		}
 
@@ -464,15 +478,16 @@ block_manager<BlockSize>::superblock_zero(block_address location,
 		boost::optional<typename block::ptr> cached_block = cache_.get(location);
 
 		if (cached_block) {
-			(*cached_block)->check_write_lockable();
+			typename block::ptr cb = *cached_block;
-			memset((*cached_block)->data_->raw(), 0, BlockSize); // FIXME: add a zero method to buffer
+			cb->check_write_lockable();
-			(*cached_block)->validator_ = v;
+			cb->bt_ = BT_SUPERBLOCK;
+			cb->change_validator(v, false);
+			memset(cb->data_->raw(), 0, BlockSize); // FIXME: add a zero method to buffer
+
 			return write_ref(*this, *cached_block);
 		}
 
-		typename block::ptr b(new block(io_, location, BT_SUPERBLOCK,
+		typename block::ptr b(new block(io_, location, BT_SUPERBLOCK, v, true));
-				      mk_validator(new noop_validator), true));
-		b->validator_ = v;
 		cache_.insert(b);
 		return write_ref(*this, b);
 

unit-tests/block_t.cc

@@ -49,6 +49,14 @@ namespace {
 		}
 	};
 
+	class validator_mock : public block_manager<4096>::validator {
+	public:
+		typedef boost::shared_ptr<validator_mock> ptr;
+
+		MOCK_CONST_METHOD2(check, void(buffer<4096> const &, block_address));
+		MOCK_CONST_METHOD2(prepare, void(buffer<4096> &, block_address));
+	};
+
 	typedef block_manager<4096> bm4096;
 }
 
@@ -208,3 +216,170 @@ TEST(BlockTests, write_then_read)
 }
 
 //----------------------------------------------------------------
+
+namespace {
+	class ValidatorTests : public Test {
+	public:
+		ValidatorTests()
+			: bm(create_bm<4096>()),
+			  vmock(new validator_mock),
+			  vmock2(new validator_mock) {
+		}
+
+		void expect_check(validator_mock::ptr v) {
+			EXPECT_CALL(*v, check(_, Eq(0ull))).Times(1);
+		}
+
+		void expect_prepare(validator_mock::ptr v) {
+			EXPECT_CALL(*v, prepare(_, Eq(0ull))).Times(1);
+		}
+
+		bm4096::ptr bm;
+		validator_mock::ptr vmock;
+		validator_mock::ptr vmock2;
+	};
+}
+
+//--------------------------------
+
+TEST_F(ValidatorTests, check_on_read_lock)
+{
+	expect_check(vmock);
+	bm4096::read_ref rr = bm->read_lock(0, vmock);
+}
+
+TEST_F(ValidatorTests, check_only_called_once_on_read_lock)
+{
+	{
+		expect_check(vmock);
+		bm4096::read_ref rr = bm->read_lock(0, vmock);
+	}
+
+	bm4096::read_ref rr = bm->read_lock(0, vmock);
+}
+
+TEST_F(ValidatorTests, validator_can_be_changed_by_read_lock)
+{
+	{
+		expect_check(vmock);
+		bm4096::read_ref rr = bm->read_lock(0, vmock);
+	}
+
+	{
+		expect_check(vmock2);
+		bm4096::read_ref rr = bm->read_lock(0, vmock2);
+	}
+}
+
+//--------------------------------
+
+TEST_F(ValidatorTests, check_and_prepare_on_write_lock)
+{
+	expect_check(vmock);
+	expect_prepare(vmock);
+	bm4096::write_ref wr = bm->write_lock(0, vmock);
+}
+
+TEST_F(ValidatorTests, check_only_called_once_on_write_lock)
+{
+	{
+		expect_check(vmock);
+		bm4096::write_ref wr = bm->write_lock(0, vmock);
+	}
+
+	bm4096::write_ref wr = bm->write_lock(0, vmock);
+	expect_prepare(vmock);
+}
+
+TEST_F(ValidatorTests, validator_can_be_changed_by_write_lock)
+{
+	{
+		expect_check(vmock);
+		expect_prepare(vmock);
+		bm4096::write_ref wr = bm->write_lock(0, vmock);
+	}
+
+	{
+		expect_check(vmock2);
+		expect_prepare(vmock2);
+		bm4096::write_ref wr = bm->write_lock(0, vmock2);
+	}
+}
+
+//--------------------------------
+
+TEST_F(ValidatorTests, no_check_but_prepare_on_write_lock_zero)
+{
+	expect_prepare(vmock);
+	bm4096::write_ref wr = bm->write_lock_zero(0, vmock);
+}
+
+TEST_F(ValidatorTests, validator_can_be_changed_by_write_lock_zero)
+{
+	expect_prepare(vmock);
+	expect_prepare(vmock2);
+
+	{
+		bm4096::write_ref wr = bm->write_lock_zero(0, vmock);
+	}
+
+	bm4096::write_ref wr = bm->write_lock_zero(0, vmock2);
+}
+
+//--------------------------------
+
+TEST_F(ValidatorTests, check_and_prepare_on_superblock_lock)
+{
+	expect_check(vmock);
+	expect_prepare(vmock);
+	bm4096::write_ref wr = bm->superblock(0, vmock);
+}
+
+TEST_F(ValidatorTests, check_only_called_once_on_superblock_lock)
+{
+	{
+		expect_check(vmock);
+		expect_prepare(vmock);
+		bm4096::write_ref wr = bm->superblock(0, vmock);
+	}
+
+	bm4096::write_ref wr = bm->superblock(0, vmock);
+	expect_prepare(vmock);
+}
+
+TEST_F(ValidatorTests, validator_can_be_changed_by_superblock_lock)
+{
+	{
+		expect_check(vmock);
+		expect_prepare(vmock);
+		bm4096::write_ref wr = bm->write_lock(0, vmock);
+	}
+
+	{
+		expect_check(vmock2);
+		expect_prepare(vmock2);
+		bm4096::write_ref wr = bm->write_lock(0, vmock2);
+	}
+}
+
+//--------------------------------
+
+TEST_F(ValidatorTests, no_check_but_prepare_on_superblock_lock_zero)
+{
+	expect_prepare(vmock);
+	bm4096::write_ref wr = bm->superblock_zero(0, vmock);
+}
+
+TEST_F(ValidatorTests, validator_can_be_changed_by_superblock_zero)
+{
+	expect_prepare(vmock);
+	expect_prepare(vmock2);
+
+	{
+		bm4096::write_ref wr = bm->write_lock(0, vmock);
+	}
+
+	bm4096::write_ref wr = bm->write_lock(0, vmock2);
+}
+
+//----------------------------------------------------------------