From 52ef0c1c9efac025b71f2bf3d33e8785538e88c7 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 26 Jan 2020 19:16:57 -0600 Subject: [PATCH] Fixed a crazy consistency issue in test.py The root of the problem was the notorious Python quirk with mutable default parameters. The default defines for the TestSuite class ended up being mutated as the class determined the permutations to test, corrupting other test's defines. However, the only define that was mutated this way was the CACHE_SIZE config in test_entries. The crazy thing was how this small innocuous change would cause "./scripts/test.py -nr test_relocations" and "./scripts/test.py -nr" to drift out of sync only after a commit spanning the different cache sizes would be written out with a different number of prog calls. This offset the power-cycle counter enough to cause one case to make it to an erase, and the other to not. Normally, the difference between a successful/unsuccessful erase wouldn't change the result of a test, but in this case it offset the revision count used for wear-leveling, causing one run run expand the superblock and the other to not. This change to the filesystem would then propogate through the rest of the test, making it difficult to reproduce test failures. Fortunately the fix was to just make a copy of the default define dictionary. This should also prevent accidently mutating of dicts belonging to our caller. Oh, also fixed a buffer overflow in test_files. --- scripts/test_.py | 6 +++--- testbd/lfs_testbd.c | 4 ++-- tests_/test_files.toml | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/scripts/test_.py b/scripts/test_.py index 1a13c5b..4593b40 100755 --- a/scripts/test_.py +++ b/scripts/test_.py @@ -304,8 +304,8 @@ class ReentrantTestCase(TestCase): # exact cycle we should drop into debugger? if gdb and failure and failure.cycleno == cycles: - return super().test(gdb=gdb, - persist=persist, failure=failure, **args) + return super().test(gdb=gdb, persist=persist, cycles=cycles, + failure=failure, **args) # run tests, but kill the program after prog/erase has # been hit n cycles. We exit with a special return code if the @@ -327,7 +327,7 @@ class TestSuite: self.name = self.name[:-len('.toml')] self.path = path self.classes = classes - self.defines = defines + self.defines = defines.copy() self.filter = filter with open(path) as f: diff --git a/testbd/lfs_testbd.c b/testbd/lfs_testbd.c index f13140e..0fae6bf 100644 --- a/testbd/lfs_testbd.c +++ b/testbd/lfs_testbd.c @@ -199,7 +199,7 @@ int lfs_testbd_prog(const struct lfs_config *cfg, lfs_block_t block, bd->power_cycles -= 1; if (bd->power_cycles == 0) { // sync to make sure we persist the last changes - lfs_testbd_rawsync(cfg); + assert(lfs_testbd_rawsync(cfg) == 0); // simulate power loss exit(33); } @@ -241,7 +241,7 @@ int lfs_testbd_erase(const struct lfs_config *cfg, lfs_block_t block) { bd->power_cycles -= 1; if (bd->power_cycles == 0) { // sync to make sure we persist the last changes - lfs_testbd_rawsync(cfg); + assert(lfs_testbd_rawsync(cfg) == 0); // simulate power loss exit(33); } diff --git a/tests_/test_files.toml b/tests_/test_files.toml index da132f2..565e665 100644 --- a/tests_/test_files.toml +++ b/tests_/test_files.toml @@ -60,7 +60,7 @@ code = ''' [[case]] # rewriting files define.SIZE1 = [32, 8192, 131072, 0, 7, 8193] define.SIZE2 = [32, 8192, 131072, 0, 7, 8193] -define.CHUNKSIZE = [31, 16, 1, 1025] +define.CHUNKSIZE = [31, 16, 1] code = ''' lfs_format(&lfs, &cfg) => 0; @@ -142,7 +142,7 @@ code = ''' [[case]] # appending files define.SIZE1 = [32, 8192, 131072, 0, 7, 8193] define.SIZE2 = [32, 8192, 131072, 0, 7, 8193] -define.CHUNKSIZE = [31, 16, 1, 1025] +define.CHUNKSIZE = [31, 16, 1] code = ''' lfs_format(&lfs, &cfg) => 0; @@ -219,7 +219,7 @@ code = ''' [[case]] # truncating files define.SIZE1 = [32, 8192, 131072, 0, 7, 8193] define.SIZE2 = [32, 8192, 131072, 0, 7, 8193] -define.CHUNKSIZE = [31, 16, 1, 1025] +define.CHUNKSIZE = [31, 16, 1] code = ''' lfs_format(&lfs, &cfg) => 0;