From 55f0872dbc9e7fdb7cf700c12a6251841111fe16 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 9 Jun 2024 14:59:05 -0500 Subject: [PATCH] Change lfsr_mount to return CORRUPT or NOTSUP Before, lfsr_mount would return LFS_ERR_INVAL if it could not mount the filesystem for any reason. This matches POSIX's mount behavior, but is, in my humble opinion, unhelpful... A corrupted filesystem image is an "invalid parameter"? This splits lfsr_mount's failed-to-mount behavior into two error codes: - LFS_ERR_CORRUPT - Failed to mount because something was corrupted. Unlikely disk contains a littlefs image. - LFS_ERR_NOTSUP - Failed to mount because on-disk filesystem is incompatible. Reconfiguring your driver may successfully mount. This offers a bit more of a hint to users on why mount failed. Though relevant error logs will probably have more useful information. Worst case users can always treat CORRUPT/NOTSUP the same after calling lfsr_mount. Code changes: code stack before: 33674 2592 after: 33686 (+0.0%) 2592 (+0.0%) --- lfs.c | 26 +++++------ tests/test_incompat.toml | 99 ++++++++++++++++++++++++++++++++++------ 2 files changed, 97 insertions(+), 28 deletions(-) diff --git a/lfs.c b/lfs.c index cbc95bc8..caefbc68 100644 --- a/lfs.c +++ b/lfs.c @@ -5543,7 +5543,7 @@ static int lfsr_data_readgrm(lfs_t *lfs, lfsr_data_t *data, // unknown mode? return an error, we may be able to mount read-only if (mode > 2) { - return LFS_ERR_INVAL; + return LFS_ERR_CORRUPT; } for (uint8_t i = 0; i < mode; i++) { @@ -8338,7 +8338,7 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { if (err) { if (err == LFS_ERR_NOENT) { LFS_ERROR("No littlefs version found"); - return LFS_ERR_INVAL; + return LFS_ERR_CORRUPT; } return err; } @@ -8355,7 +8355,7 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { version[1], LFS_DISK_VERSION_MAJOR, LFS_DISK_VERSION_MINOR); - return LFS_ERR_INVAL; + return LFS_ERR_NOTSUP; } // check for any rcompatflags, we must understand these to read @@ -8378,7 +8378,7 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { " (!= 0x%0"PRIx16")", rcompat, LFSR_RCOMPAT_COMPAT); - return LFS_ERR_INVAL; + return LFS_ERR_NOTSUP; } // check for any wcompatflags, we must understand these to write @@ -8402,7 +8402,7 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { " (!= 0x%0"PRIx16")", wcompat, LFSR_WCOMPAT_COMPAT); - return LFS_ERR_INVAL; + return LFS_ERR_NOTSUP; } // we don't bother to check for any ocompatflags, we would just @@ -8428,14 +8428,14 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { LFS_ERROR("Incompatible block size %"PRId32" (!= %"PRId32")", geometry.block_size, lfs->cfg->block_size); - return LFS_ERR_INVAL; + return LFS_ERR_NOTSUP; } if (geometry.block_count != lfs->cfg->block_count) { LFS_ERROR("Incompatible block count %"PRId32" (!= %"PRId32")", geometry.block_count, lfs->cfg->block_count); - return LFS_ERR_INVAL; + return LFS_ERR_NOTSUP; } // read the name limit @@ -8459,7 +8459,7 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { LFS_ERROR("Incompatible name limit (%"PRId32" > %"PRId32")", name_limit, lfs->name_limit); - return LFS_ERR_INVAL; + return LFS_ERR_NOTSUP; } lfs->name_limit = name_limit; @@ -8485,7 +8485,7 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { LFS_ERROR("Incompatible file limit (%"PRId32" > %"PRId32")", file_limit, lfs->file_limit); - return LFS_ERR_INVAL; + return LFS_ERR_NOTSUP; } lfs->file_limit = file_limit; @@ -8502,7 +8502,7 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { && lfsr_tag_suptype(tag) == LFSR_TAG_CONFIG) { LFS_ERROR("Unknown config 0x%04"PRIx16, tag); - return LFS_ERR_INVAL; + return LFS_ERR_NOTSUP; } return 0; @@ -8543,7 +8543,7 @@ static int lfsr_mountinited(lfs_t *lfs) { if (err) { if (err == LFS_ERR_NOENT) { LFS_ERROR("No littlefs magic found"); - return LFS_ERR_INVAL; + return LFS_ERR_CORRUPT; } return err; } @@ -8555,7 +8555,7 @@ static int lfsr_mountinited(lfs_t *lfs) { } if (cmp != LFS_CMP_EQ) { LFS_ERROR("No littlefs magic found"); - return LFS_ERR_INVAL; + return LFS_ERR_CORRUPT; } // are we the last mroot? @@ -8625,7 +8625,7 @@ static int lfsr_mountinited(lfs_t *lfs) { >> lfs->mdir_bits, rid, lfsr_tag_subtype(tag)); - return LFS_ERR_INVAL; + return LFS_ERR_NOTSUP; } } diff --git a/tests/test_incompat.toml b/tests/test_incompat.toml index 313cfc80..6d215011 100644 --- a/tests/test_incompat.toml +++ b/tests/test_incompat.toml @@ -7,6 +7,52 @@ after = ['test_mtree', 'test_dirs', 'test_files'] ## incompatiblity tests ## +# test that we fail if we find no magic +[cases.test_incompat_no_magic] +in = 'lfs.c' +code = ''' + // create a superblock + lfs_t lfs; + lfsr_format(&lfs, CFG) => 0; + + // delete the magic string + // + // note we're messing around with internals to do this! this + // is not a user API + lfsr_mount(&lfs, CFG) => 0; + lfsr_mdir_commit(&lfs, &lfs.mroot, LFSR_ATTRS( + LFSR_ATTR( + LFSR_TAG_RM | LFSR_TAG_MAGIC, 0, + LFSR_DATA_NULL()))) => 0; + lfsr_unmount(&lfs) => 0; + + // mount should now fail + lfsr_mount(&lfs, CFG) => LFS_ERR_CORRUPT; +''' + +# test that we fail if we find bad magic +[cases.test_incompat_bad_magic] +in = 'lfs.c' +code = ''' + // create a superblock + lfs_t lfs; + lfsr_format(&lfs, CFG) => 0; + + // tweak the magic string + // + // note we're messing around with internals to do this! this + // is not a user API + lfsr_mount(&lfs, CFG) => 0; + lfsr_mdir_commit(&lfs, &lfs.mroot, LFSR_ATTRS( + LFSR_ATTR( + LFSR_TAG_MAGIC, 0, + LFSR_DATA_BUF("lottlefs", 8)))) => 0; + lfsr_unmount(&lfs) => 0; + + // mount should now fail + lfsr_mount(&lfs, CFG) => LFS_ERR_CORRUPT; +''' + # test that we fail to mount after a major version bump [cases.test_incompat_major] in = 'lfs.c' @@ -29,7 +75,7 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' # test that we fail to mount after a minor version bump @@ -54,7 +100,7 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' # test that we fail to mount incompatible rcompat flags @@ -80,7 +126,7 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' # test that we fail to mount incompatible wcompat flags @@ -106,7 +152,7 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' # test that an incompatible ocompat flag is a noop @@ -157,7 +203,7 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' [cases.test_incompat_wcompat_overflow] @@ -180,7 +226,7 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' [cases.test_incompat_ocompat_overflow] @@ -230,7 +276,7 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' # test that we fail to mount after incompatible block counts @@ -256,7 +302,7 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' # test that we fail to mount after incompatible name limit @@ -280,7 +326,7 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' # test that we fail to mount after incompatible file limit @@ -308,11 +354,31 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; +''' + +# test what happens if we find an unknown config +[cases.test_incompat_unknown_config] +in = 'lfs.c' +code = ''' + // create a superblock + lfs_t lfs; + lfsr_format(&lfs, CFG) => 0; + + // create an unknown config + lfsr_mount(&lfs, CFG) => 0; + lfsr_mdir_commit(&lfs, &lfs.mroot, LFSR_ATTRS( + LFSR_ATTR( + LFSR_TAG_CONFIG + 0x13, 0, + LFSR_DATA_BUF("oh no!", strlen("oh no!"))))) => 0; + lfsr_unmount(&lfs) => 0; + + // mount should now fail + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; ''' # test what happens if we find an unknown file type -[cases.test_incompat_unknown] +[cases.test_incompat_unknown_type] in = 'lfs.c' code = ''' // create a superblock @@ -324,15 +390,18 @@ code = ''' lfsr_file_t file; lfsr_file_open(&lfs, &file, "a", LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; - lfsr_file_write(&lfs, &file, "hi a!", strlen("hi a!")) => strlen("hi a!"); + lfsr_file_write(&lfs, &file, + "hi a!", strlen("hi a!")) => strlen("hi a!"); lfsr_file_close(&lfs, &file) => 0; lfsr_file_open(&lfs, &file, "b", LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; - lfsr_file_write(&lfs, &file, "hi b!", strlen("hi b!")) => strlen("hi b!"); + lfsr_file_write(&lfs, &file, + "oh no!", strlen("oh no!")) => strlen("oh no!"); lfsr_file_close(&lfs, &file) => 0; lfsr_file_open(&lfs, &file, "c", LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; - lfsr_file_write(&lfs, &file, "hi c!", strlen("hi c!")) => strlen("hi c!"); + lfsr_file_write(&lfs, &file, + "hi c!", strlen("hi c!")) => strlen("hi c!"); lfsr_file_close(&lfs, &file) => 0; lfsr_unmount(&lfs) => 0; @@ -352,5 +421,5 @@ code = ''' lfsr_unmount(&lfs) => 0; // mount should now fail - lfsr_mount(&lfs, CFG) => LFS_ERR_INVAL; + lfsr_mount(&lfs, CFG) => LFS_ERR_NOTSUP; '''