From: Alberto Leiva Popper Date: Thu, 15 May 2025 20:38:18 +0000 (-0600) Subject: Resolve directory pruning critical TODOs X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=45a286c092fe35e80b82c084c3a0ee0148578dcb;p=thirdparty%2FFORT-validator.git Resolve directory pruning critical TODOs - Return error code when remove() fails, but keep deleting files until tree traversal complete. - At least one platform thinks `nftw(a, b, c, d)` is an error when `a` is not a directory, so fall back to unlinking when that happens. --- diff --git a/src/cache.c b/src/cache.c index f21cb0b6..b6c352d5 100644 --- a/src/cache.c +++ b/src/cache.c @@ -322,34 +322,35 @@ reset_cache_dir(void) { DIR *dir; struct dirent *file; - int error; + int tmperr, abserr; unsigned int deleted; pr_op_debug("Resetting cache..."); + abserr = 0; + deleted = 0; + dir = opendir("."); if (dir == NULL) - goto fail; + goto end; - deleted = 0; FOREACH_DIR_FILE(dir, file) if (!S_ISDOTS(file) && strcmp(file->d_name, LOCKFILE) != 0) { - error = file_rm_rf(file->d_name); - if (error) - goto end; + tmperr = file_rm_rf(file->d_name); + if (tmperr) + abserr = tmperr; deleted++; } - if (errno) - goto fail; - - pr_op_debug(deleted > 0 ? "Cache cleared." : "Cache was empty."); - error = 0; - goto end; -fail: error = errno; - pr_op_err("Cannot traverse the cache: %s", strerror(error)); -end: closedir(dir); - return error; +end: tmperr = errno; + if (tmperr) + abserr = tmperr; + if (abserr) + pr_op_warn("Cannot reset cache: %s", strerror(abserr)); + else + pr_op_debug(deleted > 0 ? "Cache reset." : "Cache was empty."); + closedir(dir); + return abserr; } static void diff --git a/src/file.c b/src/file.c index b3084e53..59775f44 100644 --- a/src/file.c +++ b/src/file.c @@ -192,15 +192,24 @@ file_rm_f(char const *path) return 0; } +static int rm_error; + static int rm(const char *fpath, const struct stat *sb, int typeflag, struct FTW *ftwbuf) { - if (remove(fpath) < 0) - pr_op_warn("Cannot delete %s: %s", fpath, strerror(errno)); - return 0; + if (remove(fpath) < 0) { + rm_error = errno; + pr_op_warn("Cannot remove '%s': %s", fpath, strerror(rm_error)); + } + + return 0; /* Remove as much as possible, regardless of error */ } -/* Same as `system("rm -rf ")`, but more portable and maaaaybe faster. */ +/* + * rm -rf $path + * + * NOT THREAD-SAFE! + */ int file_rm_rf(char const *path) { @@ -209,15 +218,20 @@ file_rm_rf(char const *path) pr_op_debug("rm -rf %s", path); /* TODO (performance) optimize that 32 */ - // XXX In MacOS, this breaks if path is a file. - if (nftw(path, rm, 32, FTW_DEPTH | FTW_PHYS) < 0) { + rm_error = 0; + if (nftw(path, rm, 32, FTW_DEPTH | FTW_PHYS) >= 0) + return rm_error; /* Happy path */ + + error = errno; + if (error == ENOTDIR) { + /* This can happen in MacOS; try file removal. */ + if (unlink(path) == 0) + return 0; /* Rare happy path */ error = errno; - // XXX This msg is sometimes annoying; maybe defer it - pr_op_warn("Cannot remove %s: %s", path, strerror(error)); - return error ? error : -1; } - return 0; + pr_op_warn("Cannot remove '%s': %s", path, strerror(error)); + return error; } /* If @force, don't treat EEXIST as an error. */ diff --git a/src/file.h b/src/file.h index 20845511..e96012dc 100644 --- a/src/file.h +++ b/src/file.h @@ -36,6 +36,7 @@ int file_exists(char const *); int file_rm_f(char const *); int file_rm_rf(char const *); + int file_mkdir(char const *, bool); void file_ln(char const *, char const *); diff --git a/test/cache_test.c b/test/cache_test.c index 893960d3..6c37fefe 100644 --- a/test/cache_test.c +++ b/test/cache_test.c @@ -93,16 +93,17 @@ MOCK_ABORT_INT(asn_generic_no_constraint, static void setup_test(void) { + char const *dirs[] = { "rsync", "https", "rrdp", "fallback", "tmp", NULL }; + char const **d; + dl_error = 0; init_tables(); - // XXX consider changing this function to `rm -rf /*` - ck_assert_int_eq(0, file_rm_rf(".")); - ck_assert_int_eq(0, mkdir("rsync", CACHE_FILEMODE)); - ck_assert_int_eq(0, mkdir("https", CACHE_FILEMODE)); - ck_assert_int_eq(0, mkdir("rrdp", CACHE_FILEMODE)); - ck_assert_int_eq(0, mkdir("fallback", CACHE_FILEMODE)); - ck_assert_int_eq(0, mkdir("tmp", CACHE_FILEMODE)); + for (d = dirs; *d; d++) { + if (file_exists(*d) == 0) + ck_assert_int_eq(0, file_rm_rf(*d)); + ck_assert_int_eq(0, mkdir(*d, CACHE_FILEMODE)); + } } static struct cache_cage * diff --git a/test/file_test.c b/test/file_test.c new file mode 100644 index 00000000..29cc0b1e --- /dev/null +++ b/test/file_test.c @@ -0,0 +1,85 @@ +#include "file.c" + +#include + +#include "mock.c" + +static void +touch_dir(char const *dir) +{ + ck_assert_int_eq(0, file_mkdir(dir, true)); +} + +static void +touch_file(char const *file) +{ + int fd; + int error; + + pr_op_debug("touch %s", file); + + fd = open(file, O_WRONLY | O_CREAT, CACHE_FILEMODE); + if (fd < 0) { + error = errno; + if (error == EEXIST) + return; + ck_abort_msg("open(%s): %s", file, strerror(error)); + } + + close(fd); +} + +static void +create_test_sandbox(void) +{ + touch_dir ("tmp"); + touch_dir ("tmp/file"); + touch_dir ("tmp/file/abc"); + + touch_file("tmp/file/abc/a"); + + touch_dir ("tmp/file/abc/b"); + touch_file("tmp/file/abc/b/d"); + touch_file("tmp/file/abc/b/e"); + + touch_file("tmp/file/abc/c"); +} + +START_TEST(test_rm) +{ + create_test_sandbox(); + + ck_assert_int_eq(0, file_exists("tmp/file/abc")); + ck_assert_int_eq(0, file_rm_rf("tmp/file/abc")); + ck_assert_int_eq(ENOENT, file_exists("tmp/file/abc")); +} +END_TEST + +static Suite *create_suite(void) +{ + Suite *suite; + TCase *rm; + + rm = tcase_create("rm"); + tcase_add_test(rm, test_rm); + + suite = suite_create("File"); + suite_add_tcase(suite, rm); + return suite; +} + +int main(void) +{ + Suite *suite; + SRunner *runner; + int tests_failed; + + suite = create_suite(); + + runner = srunner_create(suite); + srunner_run_all(runner, CK_NORMAL); + tests_failed = srunner_ntests_failed(runner); + srunner_free(runner); + + return (tests_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE; +}