From 098967cee129b9116b468224257234c8ef1c9707 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Axel=20D=C3=B6rfler?= Date: Tue, 4 Sep 2012 00:54:25 +0200 Subject: [PATCH] Fixed the new issue in #8910 from r44585. * The ASSERT() I introduced in r44585 was incorrect: when the sub transaction used block_cache_get_empty() to get the block, there is no original_data for a reason. * Added a test case that reproduces this situation. * The block must be moved to the unused list in this situation, though, or else it might contain invalid data. Since the block can only be allocated in the current transaction, this should not be a problem, though, AFAICT. --- src/system/kernel/cache/block_cache.cpp | 8 ++-- .../system/kernel/cache/block_cache_test.cpp | 41 ++++++++++++++++++- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/system/kernel/cache/block_cache.cpp b/src/system/kernel/cache/block_cache.cpp index 7cd01571e0..22e20ccb8b 100644 --- a/src/system/kernel/cache/block_cache.cpp +++ b/src/system/kernel/cache/block_cache.cpp @@ -3046,9 +3046,11 @@ cache_abort_sub_transaction(void* _cache, int32 id) // The parent transaction didn't change the block, but the sub // transaction did - we need to revert to the original data. // The block is no longer part of the transaction - ASSERT(block->original_data != NULL); - memcpy(block->current_data, block->original_data, - cache->block_size); + if (block->original_data != NULL) { + // The block might not have original data if was empty + memcpy(block->current_data, block->original_data, + cache->block_size); + } if (last != NULL) last->transaction_next = next; diff --git a/src/tests/system/kernel/cache/block_cache_test.cpp b/src/tests/system/kernel/cache/block_cache_test.cpp index f03f37b4eb..d0b88f171b 100644 --- a/src/tests/system/kernel/cache/block_cache_test.cpp +++ b/src/tests/system/kernel/cache/block_cache_test.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2008, Axel Dörfler, axeld@pinc-software.de. + * Copyright 2008-2012, Axel Dörfler, axeld@pinc-software.de. * Distributed under the terms of the MIT License. */ @@ -400,6 +400,45 @@ test_abort_sub_transaction() cache_end_transaction(gCache, id, NULL, NULL); cache_sync_transaction(gCache, id); + + start_test("Abort sub with empty block"); + id = cache_start_transaction(gCache); + + gBlocks[1].present = true; + gBlocks[1].read = true; + + block = block_cache_get_writable(gCache, 1, id); + or_block(block, BLOCK_CHANGED_IN_PREVIOUS); + gBlocks[1].original = gBlocks[1].current; + gBlocks[1].current |= BLOCK_CHANGED_IN_PREVIOUS; + + block_cache_put(gCache, 1); + + gBlocks[1].is_dirty = true; + TEST_BLOCKS(1, 1); + + cache_start_sub_transaction(gCache, id); + + gBlocks[0].present = true; + + block = block_cache_get_empty(gCache, 0, id); + or_block(block, BLOCK_CHANGED_IN_SUB); + gBlocks[0].current = BLOCK_CHANGED_IN_SUB; + + block_cache_put(gCache, 0); + + cache_abort_sub_transaction(gCache, id); + + gBlocks[0].write = false; + gBlocks[0].is_dirty = false; + gBlocks[0].parent = 0; + gBlocks[0].original = 0; + TEST_BLOCKS(0, 1); + + gBlocks[1].write = true; + gBlocks[1].is_dirty = false; + cache_end_transaction(gCache, id, NULL, NULL); + cache_sync_transaction(gCache, id); }