From d64cfc7eaad0fdfb082cac4644c777509b91f72a Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 12 Jun 2024 01:40:07 -0500 Subject: [PATCH] Don't propagate grm cleanup errors While it may be useful to know when/why lfsr_fs_fixgrm fails, at this point in lfsr_rename/lfsr_remove the operation has already succeeded as far as the filesystem is concerned. It's counterintuitive, but ignoring these errors actually tells the user _more_ information, specifically whether or not the operation completed on disk. At least we can log the error via LFS_WARN, and such errors will likely come up again in a future operation, such as the call to lfsr_fs_fixgrm on the next filesystem mutation. This was noticed in test_grow, which tests error code-paths quite a bit more than any other test. Code changes: code stack before: 33934 2592 after: 33942 (+0.0%) 2592 (+0.0%) --- lfs.c | 18 ++++++++++++++++-- tests/test_grow.toml | 30 ++++++++++++++---------------- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/lfs.c b/lfs.c index 70f8c524..796a2cda 100644 --- a/lfs.c +++ b/lfs.c @@ -9536,7 +9536,14 @@ int lfsr_remove(lfs_t *lfs, const char *path) { // if we were a directory, we need to clean up, fortunately we can leave // this up to lfsr_fs_fixgrm - return lfsr_fs_fixgrm(lfs); + err = lfsr_fs_fixgrm(lfs); + if (err) { + // we did complete the remove, so we shouldn't error here, best + // we can do is log this + LFS_WARN("Failed to clean up grm (%d)", err); + } + + return 0; } int lfsr_rename(lfs_t *lfs, const char *old_path, const char *new_path) { @@ -9705,7 +9712,14 @@ int lfsr_rename(lfs_t *lfs, const char *old_path, const char *new_path) { // we need to clean up any pending grms, fortunately we can leave // this up to lfsr_fs_fixgrm - return lfsr_fs_fixgrm(lfs); + err = lfsr_fs_fixgrm(lfs); + if (err) { + // we did complete the remove, so we shouldn't error here, best + // we can do is log this + LFS_WARN("Failed to clean up grm (%d)", err); + } + + return 0; } // this just populates the info struct based on what we found diff --git a/tests/test_grow.toml b/tests/test_grow.toml index a2cee7ec..31966bc0 100644 --- a/tests/test_grow.toml +++ b/tests/test_grow.toml @@ -658,7 +658,7 @@ code = ''' char name[256]; sprintf(name, "dir%03x", x); int err = lfsr_remove(&lfs, name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -681,7 +681,7 @@ code = ''' char new_name[256]; sprintf(new_name, "dir%03x", y); int err = lfsr_rename(&lfs, old_name, new_name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -1070,7 +1070,7 @@ code = ''' char name[256]; sprintf(name, "amethyst%03x", x); int err = lfsr_remove(&lfs, name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -1097,7 +1097,7 @@ code = ''' char new_name[256]; sprintf(new_name, "amethyst%03x", y); int err = lfsr_rename(&lfs, old_name, new_name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -1469,7 +1469,7 @@ code = ''' char name[256]; sprintf(name, "batman%03x", x); int err = lfsr_remove(&lfs, name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -1506,7 +1506,7 @@ code = ''' char new_name[256]; sprintf(new_name, "batman%03x", y); int err = lfsr_rename(&lfs, old_name, new_name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -1902,7 +1902,7 @@ code = ''' char name[256]; sprintf(name, "batman%03x", x); int err = lfsr_remove(&lfs, name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -1955,7 +1955,7 @@ code = ''' char new_name[256]; sprintf(new_name, "batman%03x", y); int err = lfsr_rename(&lfs, old_name, new_name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -2363,7 +2363,7 @@ code = ''' assert(strlen(info.name) == strlen("amethyst...")); sprintf(name, "test/%s", info.name); err = lfsr_remove(&lfs, name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -2390,7 +2390,7 @@ code = ''' char new_name[256]; sprintf(new_name, "test/amethyst%03x", y); err = lfsr_rename(&lfs, old_name, new_name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -2657,8 +2657,7 @@ code = ''' err = lfsr_remove(&lfs, name); assert(!err || err == LFS_ERR_NOTEMPTY - || err == LFS_ERR_NOSPC - || err == LFS_ERR_NOENT); + || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -2687,8 +2686,7 @@ code = ''' err = lfsr_rename(&lfs, old_name, new_name); assert(!err || err == LFS_ERR_NOTEMPTY - || err == LFS_ERR_NOSPC - || err == LFS_ERR_NOENT); + || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -2793,7 +2791,7 @@ code = ''' assert(strlen(info.name) == strlen("amethyst...")); sprintf(name, "%s/%s", dir_path, info.name); err = lfsr_remove(&lfs, name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; } @@ -2833,7 +2831,7 @@ code = ''' char new_name[256]; sprintf(new_name, "test/%s/amethyst%03x", info_.name, y); err = lfsr_rename(&lfs, old_name, new_name); - assert(!err || err == LFS_ERR_NOSPC || err == LFS_ERR_NOENT); + assert(!err || err == LFS_ERR_NOSPC); if (err == LFS_ERR_NOSPC) { goto grow; }