From 8d01895b325f0ca70577fe01ef013875770d3a09 Mon Sep 17 00:00:00 2001 From: Tim Nordell Date: Mon, 13 Jan 2025 16:52:57 -0600 Subject: [PATCH 01/10] scripts: Fixed several SyntaxWarning for python test helpers Many of these require a r'' string context to avoid errors like: scripts/test.py:105: SyntaxWarning: invalid escape sequence '\s' --- scripts/prettyasserts.py | 6 +++--- scripts/test.py | 38 +++++++++++++++++++------------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/scripts/prettyasserts.py b/scripts/prettyasserts.py index 925c14f0..ff33e27a 100755 --- a/scripts/prettyasserts.py +++ b/scripts/prettyasserts.py @@ -35,10 +35,10 @@ LEXEMES = { 'assert': ['assert'], 'arrow': ['=>'], 'string': [r'"(?:\\.|[^"])*"', r"'(?:\\.|[^'])\'"], - 'paren': ['\(', '\)'], + 'paren': [r'\(', r'\)'], 'cmp': CMP.keys(), - 'logic': ['\&\&', '\|\|'], - 'sep': [':', ';', '\{', '\}', ','], + 'logic': [r'\&\&', r'\|\|'], + 'sep': [':', ';', r'\{', r'\}', ','], 'op': ['->'], # specifically ops that conflict with cmp } diff --git a/scripts/test.py b/scripts/test.py index 6e8a201c..e7f78e4c 100755 --- a/scripts/test.py +++ b/scripts/test.py @@ -102,9 +102,9 @@ class TestCase: # the runner itself. for v_ in csplit(v): m = re.search(r'\brange\b\s*\(' - '(?P[^,\s]*)' - '\s*(?:,\s*(?P[^,\s]*)' - '\s*(?:,\s*(?P[^,\s]*)\s*)?)?\)', + r'(?P[^,\s]*)' + r'\s*(?:,\s*(?P[^,\s]*)' + r'\s*(?:,\s*(?P[^,\s]*)\s*)?)?\)', v_) if m: start = (int(m.group('start'), 0) @@ -163,8 +163,8 @@ class TestSuite: code_linenos = [] for i, line in enumerate(f): match = re.match( - '(?P\[\s*cases\s*\.\s*(?P\w+)\s*\])' - '|' '(?Pcode\s*=)', + r'(?P\[\s*cases\s*\.\s*(?P\w+)\s*\])' + r'|' r'(?Pcode\s*=)', line) if match and match.group('case'): case_linenos.append((i+1, match.group('name'))) @@ -602,9 +602,9 @@ def find_perms(runner_, ids=[], **args): errors='replace', close_fds=False) pattern = re.compile( - '^(?P[^\s]+)' - '\s+(?P[^\s]+)' - '\s+(?P\d+)/(?P\d+)') + r'^(?P[^\s]+)' + r'\s+(?P[^\s]+)' + r'\s+(?P\d+)/(?P\d+)') # skip the first line for line in it.islice(proc.stdout, 1, None): m = pattern.match(line) @@ -632,8 +632,8 @@ def find_perms(runner_, ids=[], **args): errors='replace', close_fds=False) pattern = re.compile( - '^(?P[^\s]+)' - '\s+(?P[^:]+):(?P\d+)') + r'^(?P[^\s]+)' + r'\s+(?P[^:]+):(?P\d+)') # skip the first line for line in it.islice(proc.stdout, 1, None): m = pattern.match(line) @@ -676,8 +676,8 @@ def find_path(runner_, id, **args): errors='replace', close_fds=False) pattern = re.compile( - '^(?P[^\s]+)' - '\s+(?P[^:]+):(?P\d+)') + r'^(?P[^\s]+)' + r'\s+(?P[^:]+):(?P\d+)') # skip the first line for line in it.islice(proc.stdout, 1, None): m = pattern.match(line) @@ -706,7 +706,7 @@ def find_defines(runner_, id, **args): errors='replace', close_fds=False) defines = co.OrderedDict() - pattern = re.compile('^(?P\w+)=(?P.+)') + pattern = re.compile(r'^(?P\w+)=(?P.+)') for line in proc.stdout: m = pattern.match(line) if m: @@ -781,12 +781,12 @@ def run_stage(name, runner_, ids, stdout_, trace_, output_, **args): failures = [] killed = False - pattern = re.compile('^(?:' - '(?Prunning|finished|skipped|powerloss) ' - '(?P(?P[^:]+)[^\s]*)' - '|' '(?P[^:]+):(?P\d+):(?Passert):' - ' *(?P.*)' - ')$') + pattern = re.compile(r'^(?:' + r'(?Prunning|finished|skipped|powerloss) ' + r'(?P(?P[^:]+)[^\s]*)' + r'|' r'(?P[^:]+):(?P\d+):(?Passert):' + r' *(?P.*)' + r')$') locals = th.local() children = set() From 152d03043ccab2ca6c454dd6cef43dc072d5810a Mon Sep 17 00:00:00 2001 From: Amilcar Ubiera Date: Wed, 15 Jan 2025 14:22:12 -0500 Subject: [PATCH 02/10] Fix for "unsafe use of type bool" warning when compiling with MSVC. --- lfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lfs.c b/lfs.c index 7520f2ea..00a0a545 100644 --- a/lfs.c +++ b/lfs.c @@ -2558,7 +2558,7 @@ static int lfs_dir_orphaningcommit(lfs_t *lfs, lfs_mdir_t *dir, if (err != LFS_ERR_NOENT) { if (lfs_gstate_hasorphans(&lfs->gstate)) { // next step, clean up orphans - err = lfs_fs_preporphans(lfs, -hasparent); + err = lfs_fs_preporphans(lfs, -(int8_t)hasparent); if (err) { return err; } From caba4f31df7a93fca8a04030a5d79f6819552308 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 3 Feb 2025 18:05:15 -0600 Subject: [PATCH 03/10] Fixed dir iteration being broken by concurrent removes When removing a file, we mark all open handles as "removed" ( pair={-1,-1}) to avoid trying to later read metadata that no longer exists. Unfortunately, this also includes open dir handles that happen to be pointing at the removed file, causing them to return LFS_ERR_CORRUPT on the next read. The good news is this is _not_ actual filesystem corruption, only a logic error in lfs_dir_read. We actually already have logic in place to nudge the dir to the next id, but it was unreachable with the existing logic. I suspect this worked at one point but was broken during a refactor due to lack of testing. --- Fortunately, all we need to do is _not_ clobber the handle if the internal type is a dir. Then the dir-nudging logic can correctly take over. I've also added test_dirs_remove_read to test this and prevent another regression, adapted from tests provided by tpwrules that identified the original bug. Found by tpwrules --- lfs.c | 3 +- tests/test_dirs.toml | 76 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/lfs.c b/lfs.c index 7520f2ea..458aae03 100644 --- a/lfs.c +++ b/lfs.c @@ -2369,7 +2369,8 @@ fixmlist:; if (d->m.pair != pair) { for (int i = 0; i < attrcount; i++) { if (lfs_tag_type3(attrs[i].tag) == LFS_TYPE_DELETE && - d->id == lfs_tag_id(attrs[i].tag)) { + d->id == lfs_tag_id(attrs[i].tag) && + d->type != LFS_TYPE_DIR) { d->m.pair[0] = LFS_BLOCK_NULL; d->m.pair[1] = LFS_BLOCK_NULL; } else if (lfs_tag_type3(attrs[i].tag) == LFS_TYPE_DELETE && diff --git a/tests/test_dirs.toml b/tests/test_dirs.toml index cb1f2e94..3b28a302 100644 --- a/tests/test_dirs.toml +++ b/tests/test_dirs.toml @@ -725,6 +725,82 @@ code = ''' lfs_unmount(&lfs) => 0; ''' +[cases.test_dirs_remove_read] +defines.N = 10 +if = 'N < BLOCK_COUNT/2' +code = ''' + lfs_t lfs; + lfs_format(&lfs, cfg) => 0; + lfs_mount(&lfs, cfg) => 0; + lfs_mkdir(&lfs, "prickly-pear") => 0; + for (int i = 0; i < N; i++) { + char path[1024]; + sprintf(path, "prickly-pear/cactus%03d", i); + lfs_mkdir(&lfs, path) => 0; + } + lfs_dir_t dir; + lfs_dir_open(&lfs, &dir, "prickly-pear") => 0; + struct lfs_info info; + lfs_dir_read(&lfs, &dir, &info) => 1; + assert(info.type == LFS_TYPE_DIR); + assert(strcmp(info.name, ".") == 0); + lfs_dir_read(&lfs, &dir, &info) => 1; + assert(info.type == LFS_TYPE_DIR); + assert(strcmp(info.name, "..") == 0); + for (int i = 0; i < N; i++) { + char path[1024]; + sprintf(path, "cactus%03d", i); + lfs_dir_read(&lfs, &dir, &info) => 1; + assert(info.type == LFS_TYPE_DIR); + assert(strcmp(info.name, path) == 0); + } + lfs_dir_read(&lfs, &dir, &info) => 0; + lfs_dir_close(&lfs, &dir) => 0; + lfs_unmount(&lfs); + + for (lfs_size_t k = 0; k < N; k++) { + for (lfs_size_t j = 0; j < N; j++) { + lfs_mount(&lfs, cfg) => 0; + lfs_dir_open(&lfs, &dir, "prickly-pear") => 0; + lfs_dir_read(&lfs, &dir, &info) => 1; + assert(info.type == LFS_TYPE_DIR); + assert(strcmp(info.name, ".") == 0); + lfs_dir_read(&lfs, &dir, &info) => 1; + assert(info.type == LFS_TYPE_DIR); + assert(strcmp(info.name, "..") == 0); + // iterate over dirs < j + for (unsigned i = 0; i < j; i++) { + char path[1024]; + sprintf(path, "cactus%03d", i); + lfs_dir_read(&lfs, &dir, &info) => 1; + assert(info.type == LFS_TYPE_DIR); + assert(strcmp(info.name, path) == 0); + } + + // remove k while iterating + char path[1024]; + sprintf(path, "prickly-pear/cactus%03d", k); + lfs_remove(&lfs, path) => 0; + + // iterate over dirs >= j + for (unsigned i = j; i < ((k >= j) ? N-1 : N); i++) { + char path[1024]; + sprintf(path, "cactus%03d", (k >= j && i >= k) ? i+1 : i); + lfs_dir_read(&lfs, &dir, &info) => 1; + assert(info.type == LFS_TYPE_DIR); + assert(strcmp(info.name, path) == 0); + } + lfs_dir_read(&lfs, &dir, &info) => 0; + lfs_dir_close(&lfs, &dir) => 0; + + // recreate k + sprintf(path, "prickly-pear/cactus%03d", k); + lfs_mkdir(&lfs, path) => 0; + lfs_unmount(&lfs) => 0; + } + } +''' + [cases.test_dirs_other_errors] code = ''' lfs_t lfs; From 0320e7db0e7465c3c2578feb261b52853507bfa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonathan=20S=C3=B6nnerup?= Date: Wed, 5 Feb 2025 16:16:53 +0100 Subject: [PATCH 04/10] Changed cast to correct type when trace is enabled for filebd --- bd/lfs_filebd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bd/lfs_filebd.c b/bd/lfs_filebd.c index 4ff25d44..ca2fa05e 100644 --- a/bd/lfs_filebd.c +++ b/bd/lfs_filebd.c @@ -133,7 +133,7 @@ int lfs_filebd_prog(const struct lfs_config *cfg, lfs_block_t block, int lfs_filebd_erase(const struct lfs_config *cfg, lfs_block_t block) { LFS_FILEBD_TRACE("lfs_filebd_erase(%p, 0x%"PRIx32" (%"PRIu32"))", - (void*)cfg, block, ((lfs_file_t*)cfg->context)->cfg->erase_size); + (void*)cfg, block, ((lfs_filebd_t*)cfg->context)->cfg->erase_size); lfs_filebd_t *bd = cfg->context; // check if erase is valid From d2c3a476277b9f9ebf1291c65653d2c565b66dfd Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Thu, 6 Feb 2025 01:20:29 -0600 Subject: [PATCH 05/10] gha: Added test-yes-trace build/test job to CI To hopefully catch typos like the one found by Noxet in the future. Nothing is actually testing that these trace statements compile otherwise. --- .github/workflows/test.yml | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 73e5aa6d..f3100bee 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -374,6 +374,29 @@ jobs: run: | CFLAGS="$CFLAGS -DLFS_NO_INTRINSICS" make test + # run with all trace options enabled to at least make sure these + # all compile + test-yes-trace: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: install + run: | + # need a few things + sudo apt-get update -qq + sudo apt-get install -qq gcc python3 python3-pip + pip3 install toml + gcc --version + python3 --version + - name: test-yes-trace + run: | + CFLAGS="$CFLAGS \ + -DLFS_YES_TRACE \ + -DLFS_RAMBD_YES_TRACE \ + -DLFS_FILEBD_YES_TRACE \ + -DLFS_RAMBD_YES_TRACE" \ + make test + # run LFS_MULTIVERSION tests test-multiversion: runs-on: ubuntu-latest From 936919d13488f307731fa203981ffb62d9e43479 Mon Sep 17 00:00:00 2001 From: Rocloong <63897185+RocLoong@users.noreply.github.com> Date: Sat, 8 Feb 2025 15:12:16 +0800 Subject: [PATCH 06/10] LFS_TRACE: Fixed sign mismatch in lfs_file_size --- lfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lfs.c b/lfs.c index 7520f2ea..416ec729 100644 --- a/lfs.c +++ b/lfs.c @@ -6288,7 +6288,7 @@ lfs_soff_t lfs_file_size(lfs_t *lfs, lfs_file_t *file) { lfs_soff_t res = lfs_file_size_(lfs, file); - LFS_TRACE("lfs_file_size -> %"PRId32, res); + LFS_TRACE("lfs_file_size -> %"PRIu32, res); LFS_UNLOCK(lfs->cfg); return res; } From f55520380da00985015180be4a5da554c461645b Mon Sep 17 00:00:00 2001 From: Brian Pugh Date: Thu, 27 Feb 2025 17:41:29 -0800 Subject: [PATCH 07/10] Add a little bit of documentation on how to run tests. --- README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/README.md b/README.md index a1dc7437..4c79c5df 100644 --- a/README.md +++ b/README.md @@ -199,6 +199,18 @@ The tests assume a Linux environment and can be started with make: make test ``` +Tests are defined as C code in toml files in the `tests/` directory. +When developing a feature or fixing a bug, it is frequently useful to run a single suite of tests. +To run individual tests or suite of tests: + +``` bash +make test-runner +python scripts/test.py --help # See all available options +python scripts/test.py --list-suites # prints out all available test suites. +python scripts/test.py --list-cases # prints out all available individual test cases. +python scripts/test.py runners/test_runner test_attrs # runs the test_attrs suite. +``` + ## License The littlefs is provided under the [BSD-3-Clause] license. See From 5281a20f6c3e2af453591eeb62a8f5d5a86dd063 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Thu, 13 Mar 2025 13:18:48 -0500 Subject: [PATCH 08/10] README.md: Tweaked testing documentation - Showing some of the more useful flags. - Showing the usual flow of bug -> reproduce -> gdb. - Being a bit pedantic since this is the README.md. --- README.md | 45 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 4c79c5df..5ae6aa88 100644 --- a/README.md +++ b/README.md @@ -199,16 +199,45 @@ The tests assume a Linux environment and can be started with make: make test ``` -Tests are defined as C code in toml files in the `tests/` directory. -When developing a feature or fixing a bug, it is frequently useful to run a single suite of tests. -To run individual tests or suite of tests: +Tests are implemented in C in the .toml files found in the `tests` directory. +When developing a feature or fixing a bug, it is frequently useful to run a +single test case or suite of tests: ``` bash -make test-runner -python scripts/test.py --help # See all available options -python scripts/test.py --list-suites # prints out all available test suites. -python scripts/test.py --list-cases # prints out all available individual test cases. -python scripts/test.py runners/test_runner test_attrs # runs the test_attrs suite. +./scripts/test.py -l runners/test_runner # list available test suites +./scripts/test.py -L runners/test_runner test_dirs # list available test cases +./scripts/test.py runners/test_runner test_dirs # run a specific test suite +``` + +If an assert fails in a test, test.py will try to print information about the +failure: + +``` bash +tests/test_dirs.toml:1:failure: test_dirs_root:1g12gg2 (PROG_SIZE=16, ERASE_SIZE=512) failed +tests/test_dirs.toml:5:assert: assert failed with 0, expected eq 42 + lfs_mount(&lfs, cfg) => 42; +``` + +This includes the test id, which can be passed to test.py to run only that +specific test permutation: + +``` bash +./scripts/test.py runners/test_runner test_dirs_root:1g12gg2 # run a specific test permutation +./scripts/test.py runners/test_runner test_dirs_root:1g12gg2 --gdb # drop into gdb on failure +``` + +Some other flags that may be useful: + +```bash +./scripts/test.py runners/test_runner -b -j # run tests in parallel +./scripts/test.py runners/test_runner -v -O- # redirect stdout to stdout +./scripts/test.py runners/test_runner -ddisk # capture resulting disk image +``` + +See `-h/--help` for a full list of available flags: + +``` bash +./scripts/test.py --help ``` ## License From ffafb9cbb106c8e890a9949b49033e6deee44863 Mon Sep 17 00:00:00 2001 From: Joakim Plate Date: Tue, 11 Mar 2025 16:19:58 +0100 Subject: [PATCH 09/10] fix: avoid assuming struct packing lfs_gstate_t was assumed to be a packed array of uint32_t, but this is not always guaranteed. Access the fields directly instead of attempting to loop over an array of uint32_t Fixes clang tidy warnings about use of uninitialized memory accessed. --- lfs.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lfs.c b/lfs.c index 7520f2ea..901d712e 100644 --- a/lfs.c +++ b/lfs.c @@ -404,16 +404,20 @@ struct lfs_diskoff { // operations on global state static inline void lfs_gstate_xor(lfs_gstate_t *a, const lfs_gstate_t *b) { - for (int i = 0; i < 3; i++) { - ((uint32_t*)a)[i] ^= ((const uint32_t*)b)[i]; - } + a->tag ^= b->tag; + a->pair[0] ^= b->pair[0]; + a->pair[1] ^= b->pair[1]; } static inline bool lfs_gstate_iszero(const lfs_gstate_t *a) { - for (int i = 0; i < 3; i++) { - if (((uint32_t*)a)[i] != 0) { - return false; - } + if (a->tag != 0) { + return false; + } + if (a->pair[0] != 0) { + return false; + } + if (a->pair[1] != 0) { + return false; } return true; } From 61a1b0b496958de3ba1849626296bac8a5a97f02 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 18 Mar 2025 02:39:28 -0500 Subject: [PATCH 10/10] Tweaked lfs_gstate_iszero for terseness --- lfs.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/lfs.c b/lfs.c index 901d712e..d0965a30 100644 --- a/lfs.c +++ b/lfs.c @@ -410,16 +410,9 @@ static inline void lfs_gstate_xor(lfs_gstate_t *a, const lfs_gstate_t *b) { } static inline bool lfs_gstate_iszero(const lfs_gstate_t *a) { - if (a->tag != 0) { - return false; - } - if (a->pair[0] != 0) { - return false; - } - if (a->pair[1] != 0) { - return false; - } - return true; + return a->tag == 0 + && a->pair[0] == 0 + && a->pair[1] == 0; } #ifndef LFS_READONLY