From: Alberto Leiva Popper Date: Tue, 8 Oct 2024 23:14:56 +0000 (-0600) Subject: Remove mkdir_p() X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8ca22da436e8f21d951e4619d4a3e1ef993e6d5e;p=thirdparty%2FFORT-validator.git Remove mkdir_p() It induced too many redundant mkdir() and stat() system calls. It's now best to use mkdir() directly, since the cache structure no longer involves long and pointless directory chains. Incidentally fixes a missing mkdir("cache/fallback"), which was preventing the cache from backing up any valid RPPs. --- diff --git a/src/cache.c b/src/cache.c index bf89c17e..07757c73 100644 --- a/src/cache.c +++ b/src/cache.c @@ -235,18 +235,22 @@ init_cachedir_tag(void) } static int -init_tmp_dir(void) +init_cache_dirs(void) { int error; - - if (mkdir(CACHE_TMPDIR, CACHE_FILEMODE) < 0) { - error = errno; - if (error != EEXIST) - return pr_op_err("Cannot create '%s': %s", - CACHE_TMPDIR, strerror(error)); - } - - return 0; + error = file_mkdir("rsync", true); + if (error) + return error; + error = file_mkdir("https", true); + if (error) + return error; + error = file_mkdir("rrdp", true); + if (error) + return error; + error = file_mkdir("fallback", true); + if (error) + return error; + return file_mkdir(CACHE_TMPDIR, true); } int @@ -261,7 +265,7 @@ cache_setup(void) // XXX Lock the cache directory init_tables(); init_cache_metafile(); - init_tmp_dir(); + init_cache_dirs(); init_cachedir_tag(); return 0; } @@ -975,6 +979,7 @@ commit_fallbacks(void) struct cache_commit *commit; struct cache_node *fb, *tmp; array_index i; + int error; while (!STAILQ_EMPTY(&commits)) { commit = STAILQ_FIRST(&commits); @@ -983,13 +988,8 @@ commit_fallbacks(void) if (commit->caRepository) { fb = provide_node(&cache.fallback, commit->caRepository); - if (mkdir(fb->map.path, CACHE_FILEMODE) < 0) { - if (errno != EEXIST) { - pr_op_warn("Failed to create %s: %s", - fb->map.path, strerror(errno)); - goto skip; - } - } + if (file_mkdir(fb->map.path, true) != 0) + goto skip; commit_rpp(commit, fb); discard_trash(commit, fb); @@ -1027,9 +1027,10 @@ skip: free(commit->caRepository); * from an expiration threshold. */ pr_op_debug("Removing orphaned fallback: %s", fb->map.path); - if (file_rm_rf(fb->map.path) < 0) - pr_op_warn("Could not remove %s; unknown cause.", - fb->map.path); + error = file_rm_rf(fb->map.path); + if (error) + pr_op_warn("%s removal failed: %s", + fb->map.path, strerror(error)); delete_node(&cache.fallback, fb); } } diff --git a/src/common.c b/src/common.c index e73ddc03..1f7d7e68 100644 --- a/src/common.c +++ b/src/common.c @@ -238,83 +238,6 @@ valid_file_or_dir(char const *location, bool check_file) return result; } -static int -stat_dir(char const *path) -{ - struct stat meta; - - if (stat(path, &meta) != 0) - return errno; - if (!S_ISDIR(meta.st_mode)) - return ENOTDIR; - return 0; -} - -static int -ensure_dir(char const *path) -{ - int error; - - /* - * Perplexingly, mkdir() returns EEXIST instead of ENOTDIR when the - * path actually refers to a file. - * So it looks like this stat() is unavoidable. - */ - if (stat_dir(path) == ENOTDIR && remove(path) < 0) - return errno; - - if (mkdir(path, CACHE_FILEMODE) < 0) { - error = errno; - return (error == EEXIST) ? 0 : error; - } - - return 0; -} - -/* mkdir -p $_path */ -/* XXX Maybe also short-circuit by parent? */ -int -mkdir_p(char const *_path, bool include_basename) -{ - char *path, *last_slash; - int i, result = 0; - - path = pstrdup(_path); /* Remove const */ - - if (!include_basename) { - last_slash = strrchr(path, '/'); - if (last_slash == NULL) - goto end; - *last_slash = '\0'; - } - - result = stat_dir(path); /* short circuit */ - if (result == 0) - goto end; /* Already exists */ - if (result != ENOENT && result != ENOTDIR) { - pr_op_err_st("Error during stat(%s): %s", - path, strerror(result)); - goto end; - } - - for (i = 1; path[i] != '\0'; i++) { - if (path[i] == '/') { - path[i] = '\0'; - result = ensure_dir(path); - path[i] = '/'; - if (result != 0) { - pr_op_err_st("Error during mkdir(%s): %s", - path, strerror(result)); - goto end; - } - } - } - result = ensure_dir(path); - -end: free(path); - return result; -} - time_t time_nonfatal(void) { diff --git a/src/common.h b/src/common.h index 3bc5c1d8..63c37b61 100644 --- a/src/common.h +++ b/src/common.h @@ -47,8 +47,6 @@ int foreach_file(char const *, char const *, bool, foreach_file_cb, void *); // XXX bool valid_file_or_dir(char const *, bool); -int mkdir_p(char const *, bool); - time_t time_nonfatal(void); time_t time_fatal(void); diff --git a/src/config.c b/src/config.c index bcfe64b1..2957c675 100644 --- a/src/config.c +++ b/src/config.c @@ -1211,7 +1211,7 @@ handle_flags_config(int argc, char **argv) } if (rpki_config.daemon) { - pr_op_warn("Executing as daemon, all logs will be sent to syslog."); + pr_op_info("Executing as daemon, all logs will be sent to syslog."); /* Send all logs to syslog */ rpki_config.log.output = SYSLOG; rpki_config.validation_log.output = SYSLOG; diff --git a/src/file.c b/src/file.c index e256f79b..56470539 100644 --- a/src/file.c +++ b/src/file.c @@ -50,8 +50,8 @@ file_write(char const *file_name, char const *mode, FILE **result) file = fopen(file_name, mode); if (file == NULL) { error = errno; - pr_val_err("Could not open file '%s': %s", file_name, - strerror(error)); + pr_val_err("Could not open file '%s' for writing: %s", + file_name, strerror(error)); *result = NULL; return error; } @@ -70,10 +70,6 @@ file_write_full(char const *path, unsigned char const *content, pr_val_debug("Writing file: %s", path); - error = mkdir_p(path, false); - if (error) - return error; - error = file_write(path, "wb", &out); if (error) return error; @@ -167,83 +163,6 @@ file_exists(char const *path) return (stat(path, &meta) == 0) ? 0 : errno; } -/* strlen("cache/tmp/123"), ie. 13 */ -static size_t src_offset; -/* cache/rsync/a.b.c/d/e */ -static char const *merge_dst; - -/* Moves cache/tmp/123/z into cache/rsync/a.b.c/d/e/z. */ -static int -merge_into(const char *src, const struct stat *st, int typeflag, - struct FTW *ftw) -{ - char *dst; - struct timespec times[2]; - - dst = path_join(merge_dst, &src[src_offset]); - - if (S_ISDIR(st->st_mode)) { - pr_op_debug("mkdir -p %s", dst); - if (mkdir_p(dst, true)) { - PR_DEBUG_MSG("Failed: %s", strerror(errno)); - goto end; - } - - times[0] = st->st_atim; - times[1] = st->st_mtim; - if (utimensat(AT_FDCWD, dst, times, AT_SYMLINK_NOFOLLOW) < 0) - PR_DEBUG_MSG("utimensat: %s", strerror(errno)); - } else { - pr_op_debug("rename: %s -> %s", src, dst); - if (rename(src, dst) < 0) { - if (errno == EISDIR) { - /* XXX stacked nftw()s */ - if (file_rm_rf(dst) != 0) - PR_DEBUG_MSG("%s", "AAAAAAAAAAA"); - if (rename(src, dst) < 0) - PR_DEBUG_MSG("rename: %s", strerror(errno)); - } - } - } - -end: free(dst); - return 0; -} - -/* - * Move all the files contained in @src to @dst, overwriting when necessary, - * not touching files that exist in @dst but not in @src. - * - * @src must exist. - * - * @src: cache/tmp/123 - * @dst: cache/rsync/a.b.c/d/e - */ -int -file_merge_into(char const *src, char const *dst) -{ - int error; - - error = mkdir_p(dst, false); - if (error) - return error; - - if (file_exists(dst) == ENOENT) { - if (rename(src, dst) < 0) { - error = errno; - pr_op_err("Could not move %s to %s: %s", - src, dst, strerror(error)); - return error; - } - return 0; - } - - src_offset = strlen(src); - merge_dst = dst; - /* TODO (performance) optimize that 32 */ - return nftw(src, merge_into, 32, FTW_PHYS); -} - /* * Like remove(), but don't care if the file is already deleted. */ @@ -265,15 +184,51 @@ static int rm(const char *fpath, const struct stat *sb, int typeflag, struct FTW *ftwbuf) { pr_op_debug("Deleting %s.", fpath); - return (remove(fpath) < 0) ? errno : 0; + if (remove(fpath) < 0) + pr_op_warn("Cannot delete %s: %s", fpath, strerror(errno)); + return 0; } /* Same as `system("rm -rf ")`, but more portable and maaaaybe faster. */ int file_rm_rf(char const *path) { + int error; + /* TODO (performance) optimize that 32 */ - return nftw(path, rm, 32, FTW_DEPTH | FTW_PHYS); + errno = 0; + switch (nftw(path, rm, 32, FTW_DEPTH | FTW_PHYS)) { + case 0: + return 0; /* Happy path */ + case -1: + /* + * POSIX requires nftw() to set errno, + * but the Linux man page doesn't mention it at all... + */ + error = errno; + return error ? error : -1; + } + + /* This is supposed to be unreachable, but let's not panic. */ + return -1; +} + +/* If @force, don't treat EEXIST as an error. */ +int +file_mkdir(char const *path, bool force) +{ + int error; + + if (mkdir(path, CACHE_FILEMODE) < 0) { + error = errno; + if (!force || error != EEXIST) { + pr_op_err("Cannot create '%s': %s", + path, strerror(error)); + return error; + } + } + + return 0; } void diff --git a/src/file.h b/src/file.h index eca2a318..3e05f1e6 100644 --- a/src/file.h +++ b/src/file.h @@ -36,6 +36,7 @@ int file_exists(char const *); int file_merge_into(char const *, char const *); int file_rm_f(char const *); int file_rm_rf(char const *); +int file_mkdir(char const *, bool); struct cache_sequence { char *prefix; diff --git a/src/http.c b/src/http.c index ef7d3275..9b716929 100644 --- a/src/http.c +++ b/src/http.c @@ -388,11 +388,6 @@ http_download(char const *url, char const *path, curl_off_t ims, bool *changed) pr_val_info("HTTP GET: %s -> %s", url, path); - /* XXX might not be needed anymore */ - error = mkdir_p(path, false); - if (error) - return error; - for (r = 0; true; r++) { pr_val_debug("Download attempt #%u...", r + 1); diff --git a/src/rrdp.c b/src/rrdp.c index 1fcd6fe5..30ff269d 100644 --- a/src/rrdp.c +++ b/src/rrdp.c @@ -1207,6 +1207,12 @@ rrdp_update(struct cache_mapping const *notif, time_t mtim, bool *changed, cseq_init(&old->seq, cage, true); STAILQ_INIT(&old->delta_hashes); + error = file_mkdir(cage, false); + if (error) { + rrdp_state_free(old); + goto clean_notif; + } + error = handle_snapshot(&new, old); if (error) { rrdp_state_free(old); diff --git a/src/rsync.c b/src/rsync.c index 6141f9b8..76e79ec9 100644 --- a/src/rsync.c +++ b/src/rsync.c @@ -295,10 +295,6 @@ rsync_download(char const *url, char const *path) pr_val_debug(" %s", args[i]); } - error = mkdir_p(path, true); - if (error) - return error; - retries = 0; do { child_status = 0; diff --git a/test/cache_test.c b/test/cache_test.c index b2ccd57c..c958ac9f 100644 --- a/test/cache_test.c +++ b/test/cache_test.c @@ -41,7 +41,7 @@ rsync_download(char const *url, char const *path) if (dl_error) return dl_error; - ck_assert_int_eq(0, mkdir_p(path, true)); + ck_assert_int_eq(0, mkdir(path, CACHE_FILEMODE)); touch_file(path); return 0; diff --git a/test/rrdp_update_test.c b/test/rrdp_update_test.c index 902c5dbd..d4637712 100644 --- a/test/rrdp_update_test.c +++ b/test/rrdp_update_test.c @@ -22,7 +22,7 @@ static void setup_test(void) { ck_assert_int_eq(0, system("rm -rf tmp/")); - ck_assert_int_eq(0, system("mkdir -p tmp/rsync tmp/https tmp/tmp")); + ck_assert_int_eq(0, system("mkdir -p tmp/https tmp/rrdp tmp/tmp")); ck_assert_int_eq(0, hash_setup()); ck_assert_int_eq(0, relax_ng_init()); }