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 f21cb0b6aafeb7bf5fae6a872b27d81a8cffbaf5..b6c352d597e9604dda61c3db7e93b0cc108bbc70 100644 (file)
--- 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 b3084e53ffed2d38825bfb88ccfe360064381394..59775f44c4f06ae0d842799dafefe9bb08a18b22 100644 (file)
--- 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 <path>")`, 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 2084551111c4f774bcfb299a3c37670f176988fc..e96012dc5f0a135fbfab415d3fbe180fb1700147 100644 (file)
--- 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 893960d3123133ee9d79db183890be7cdc70ca31..6c37fefe91aaa66416a7ac8461791c7cfebc8509 100644 (file)
--- 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 <path>/*`
-       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 (file)
index 0000000..29cc0b1
--- /dev/null
+++ b/test/file_test.c
@@ -0,0 +1,85 @@
+#include "file.c"
+
+#include <check.h>
+
+#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;
+}