From: Alberto Leiva Popper Date: Tue, 27 Jun 2023 17:35:32 +0000 (-0600) Subject: Remove the reqs_errors module X-Git-Tag: 1.6.0~80^2~5 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=566835e8da0ce52b6bded39db72667eeb2e41001;p=thirdparty%2FFORT-validator.git Remove the reqs_errors module It's a collection of failed downloads, and it's two-purposed: 1. To list the failed downloads once the validation is over. (Which is the functionality I deleted in the prevous commit.) 2. To clue the AIA validation code (through an EREQFAILED) not to... uh... redownload... the AIA certificate's... parent. Huh. Yeah, I have a few issues with the implementation: A. Purpose 1 is obsolete. B. Regarding purpose 2: Fort should never redownload a file that was already traversed in the same validation cycle. This purpose is plainly wrong. Oh, you know what? I get it. The original coder was probably concerned that the parent might have been downloaded via RRDP, yet the child's AIA is always an rsync URI... and because RRDP and rsync are cached into separate namespaces, well, Fort wasn't going to find the parent. But the thing is, it's a URI, not a URL. RRDP also refers to these files by way of their "rsync" *URI* in its snapshots and deltas. RRDP might be HTTP, but there is no such thing as http://path.to/certificate.cer. This should be fixed by way of clever local cache resolution, not an awkward redownload. C. The lifescope of this table should be a single validation run, not Fort's lifetime. I think the reason why it's global is so the code could warn if a particular resource had been down for several iterations. But I wouldn't say that's Fort's job, and even if it is, it's probably best to move it to Prometheus somehow. We're relying too much on the logs. --- diff --git a/src/Makefile.am b/src/Makefile.am index 98d5f3d1..d657f4d6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -25,7 +25,6 @@ fort_SOURCES += nid.h nid.c fort_SOURCES += notify.c notify.h fort_SOURCES += output_printer.h output_printer.c fort_SOURCES += random.h random.c -fort_SOURCES += reqs_errors.h reqs_errors.c fort_SOURCES += resource.h resource.c fort_SOURCES += rpp.h rpp.c fort_SOURCES += sorted_array.h sorted_array.c diff --git a/src/main.c b/src/main.c index ebbf2af1..6a03a9e0 100644 --- a/src/main.c +++ b/src/main.c @@ -2,7 +2,6 @@ #include "extension.h" #include "internal_pool.h" #include "nid.h" -#include "reqs_errors.h" #include "thread_var.h" #include "validation_run.h" #include "http/http.h" @@ -108,9 +107,6 @@ main(int argc, char **argv) error = db_rrdp_init(); if (error) goto vrps_cleanup; - error = reqs_errors_init(); - if (error) - goto db_rrdp_cleanup; /* Do stuff */ switch (config_get_mode()) { @@ -124,8 +120,6 @@ main(int argc, char **argv) /* End */ - reqs_errors_cleanup(); -db_rrdp_cleanup: db_rrdp_cleanup(); vrps_cleanup: vrps_destroy(); diff --git a/src/object/certificate.c b/src/object/certificate.c index e83abe22..3c94df8a 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -19,7 +19,6 @@ #include "extension.h" #include "log.h" #include "nid.h" -#include "reqs_errors.h" #include "str_token.h" #include "thread_var.h" #include "asn1/decode.h" @@ -2140,9 +2139,6 @@ verify_mft: /* Reach here on error or when both access methods were utilized */ switch (error) { case 0: - /* Remove the error'd URI, since we got the repo files */ - if (working_repo_peek() != NULL) - reqs_errors_rem_uri(working_repo_peek()); break; case EREQFAILED: /* Log that we'll try to work with a local copy */ diff --git a/src/object/tal.c b/src/object/tal.c index 2c40f4db..8f96a132 100644 --- a/src/object/tal.c +++ b/src/object/tal.c @@ -19,7 +19,6 @@ #include "line_file.h" #include "log.h" #include "random.h" -#include "reqs_errors.h" #include "state.h" #include "thread_var.h" #include "validation_handler.h" @@ -525,8 +524,6 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, /* At least one URI was sync'd */ thread->retry_local = false; - if (thread->sync_files && working_repo_peek() != NULL) - reqs_errors_rem_uri(working_repo_peek()); working_repo_pop(); pr_val_debug("TAL URI '%s' {", uri_val_get_printable(uri)); diff --git a/src/reqs_errors.c b/src/reqs_errors.c deleted file mode 100644 index 09ebde16..00000000 --- a/src/reqs_errors.c +++ /dev/null @@ -1,202 +0,0 @@ -#include "reqs_errors.h" - -#include -#include -#include - -#include "data_structure/uthash.h" -#include "alloc.h" -#include "common.h" -#include "config.h" -#include "log.h" -#include "thread_var.h" - -/* TODO (#78) Is any of this still useful? */ - -struct error_uri { - /* Key */ - char *uri; - /* Date when the first attempt was made */ - time_t first_attempt; - /* Related URI (points to a key of another element) */ - char *uri_related; - /* Refered by (where this.uri == that.uri_related) */ - char *ref_by; - UT_hash_handle hh; -}; - -static struct error_uri *err_uris_db; - -/* Prepare for multithreading. */ -static pthread_rwlock_t db_lock; - -static int -error_uri_create(char const *uri, struct error_uri **err_uri) -{ - struct error_uri *tmp; - int error; - - tmp = pzalloc(sizeof(struct error_uri)); /* Zero needed by uthash */ - - tmp->uri = pstrdup(uri); - error = get_current_time(&tmp->first_attempt); - if (error) - goto release_uri; - tmp->uri_related = NULL; - tmp->ref_by = NULL; - - *err_uri = tmp; - return 0; - -release_uri: - free(tmp->uri); - free(tmp); - return error; -} - -static void -error_uri_destroy(struct error_uri *err_uri) -{ - free(err_uri->uri); - free(err_uri); -} - -int -reqs_errors_init(void) -{ - int error; - - error = pthread_rwlock_init(&db_lock, NULL); - if (error) { - pr_op_err("pthread_rwlock_init() errored: %s", strerror(error)); - return error; - } - - err_uris_db = NULL; - - return 0; -} - -void -reqs_errors_cleanup(void) -{ - /* Remove all the uris */ - struct error_uri *node, *tmp; - - HASH_ITER(hh, err_uris_db, node, tmp) { - HASH_DEL(err_uris_db, node); - error_uri_destroy(node); - } - - pthread_rwlock_destroy(&db_lock); /* Nothing to do with error code */ -} - -static struct error_uri * -find_error_uri(char const *search, bool lock) -{ - struct error_uri *found; - - if (lock) - rwlock_read_lock(&db_lock); - HASH_FIND_STR(err_uris_db, search, found); - if (lock) - rwlock_unlock(&db_lock); - - return found; -} - -static void -set_working_repo(struct error_uri *err_uri) -{ - struct error_uri *ref; - char const *work_uri; - - work_uri = working_repo_peek(); - if (work_uri == NULL) - return; - - ref = find_error_uri(work_uri, true); - if (ref == NULL) - return; - - err_uri->uri_related = ref->uri; - ref->ref_by = err_uri->uri; -} - -int -reqs_errors_add_uri(char const *uri) -{ - struct error_uri *new_uri, *found_uri; - int error; - - /* Don't overwrite if it already exists */ - found_uri = find_error_uri(uri, true); - if (found_uri != NULL) - return 0; - - new_uri = NULL; - error = error_uri_create(uri, &new_uri); - if (error) - return error; - - set_working_repo(new_uri); - - rwlock_write_lock(&db_lock); - HASH_ADD_STR(err_uris_db, uri, new_uri); - rwlock_unlock(&db_lock); - - return 0; -} - -void -reqs_errors_rem_uri(char const *uri) -{ - struct error_uri *found_uri; - struct error_uri *tmp; - char *ref_uri; - - rwlock_write_lock(&db_lock); - found_uri = find_error_uri(uri, false); - if (found_uri == NULL) { - rwlock_unlock(&db_lock); - return; - } - - while (found_uri->uri_related != NULL) { - tmp = find_error_uri(found_uri->uri_related, false); - if (tmp == NULL) - break; - found_uri = tmp; - } - - do { - ref_uri = found_uri->ref_by; - HASH_DELETE(hh, err_uris_db, found_uri); - error_uri_destroy(found_uri); - if (ref_uri == NULL) - break; - HASH_FIND_STR(err_uris_db, ref_uri, found_uri); - if (found_uri == NULL) - break; - } while (true); - rwlock_unlock(&db_lock); -} - -int -reqs_errors_foreach(reqs_errors_cb cb, void *arg) -{ - struct error_uri *node, *tmp; - int error; - - rwlock_read_lock(&db_lock); - HASH_ITER(hh, err_uris_db, node, tmp) { - error = cb(node->uri, arg); - if (error) { - rwlock_unlock(&db_lock); - return error; - } - } - rwlock_unlock(&db_lock); - - return 0; -} diff --git a/src/reqs_errors.h b/src/reqs_errors.h deleted file mode 100644 index 40aa1408..00000000 --- a/src/reqs_errors.h +++ /dev/null @@ -1,17 +0,0 @@ -#ifndef SRC_REQS_ERRORS_H_ -#define SRC_REQS_ERRORS_H_ - -#include - -/* TODO (#78) removable? */ - -int reqs_errors_init(void); -void reqs_errors_cleanup(void); - -int reqs_errors_add_uri(char const *); -void reqs_errors_rem_uri(char const *); - -typedef int (reqs_errors_cb)(char const *, void *); -int reqs_errors_foreach(reqs_errors_cb, void *); - -#endif /* SRC_REQS_ERRORS_H_ */ diff --git a/src/rrdp/rrdp_loader.c b/src/rrdp/rrdp_loader.c index ff9cbd8d..e73a2d5e 100644 --- a/src/rrdp/rrdp_loader.c +++ b/src/rrdp/rrdp_loader.c @@ -7,7 +7,6 @@ #include "common.h" #include "config.h" #include "log.h" -#include "reqs_errors.h" #include "thread_var.h" #include "visited_uris.h" @@ -104,7 +103,7 @@ __rrdp_load(struct rpki_uri *uri, bool force_snapshot, bool *data_updated) struct visited_uris *visited; rrdp_req_status_t requested; rrdp_uri_cmp_result_t res; - int error, upd_error; + int error; (*data_updated) = false; @@ -175,7 +174,7 @@ __rrdp_load(struct rpki_uri *uri, bool force_snapshot, bool *data_updated) if (upd_notification == NULL) { pr_val_debug("The Update Notification has not changed."); - goto upd_end; + return 0; } pr_val_debug("The Update Notification changed."); @@ -246,18 +245,11 @@ upd_destroy: update_notification_destroy(upd_notification); upd_end: /* Just return on success */ - if (!error) { - /* The repository URI is the notification file URI */ - reqs_errors_rem_uri(uri_get_global(uri)); + if (!error) return 0; - } /* Request failed, store the repository URI */ - if (error == EREQFAILED) { - upd_error = reqs_errors_add_uri(uri_get_global(uri)); - if (upd_error) - return upd_error; - } else { + if (error != EREQFAILED) { /* Reset RSYNC visited URIs, this may force the update */ /* TODO um, what? */ reset_downloaded(); diff --git a/src/rsync/rsync.c b/src/rsync/rsync.c index ffbcaf64..1504aabf 100644 --- a/src/rsync/rsync.c +++ b/src/rsync/rsync.c @@ -13,7 +13,6 @@ #include "common.h" #include "config.h" #include "log.h" -#include "reqs_errors.h" #include "str_token.h" #include "thread_var.h" @@ -496,57 +495,6 @@ release_args: return error; } -/* - * Returned values if the ancestor URI of @error_uri: - * 0 - didn't had a previous request error - * EEXIST - had a previous request error - * < 0 - nothing, just something bad happened - */ -static int -ancestor_error(char const *error_uri, void *arg) -{ - struct rpki_uri *search = arg; - struct rpki_uri *req_err_uri; - int error; - - req_err_uri = NULL; - error = uri_create_mixed_str(&req_err_uri, error_uri, - strlen(error_uri)); - switch(error) { - case 0: - break; - default: - return ENSURE_NEGATIVE(error); - } - - /* Ignore non rsync error'd URIs */ - if (!uri_is_rsync(req_err_uri)) { - uri_refput(req_err_uri); - return 0; - } - - error = is_descendant(req_err_uri, search) ? EEXIST : 0; - - uri_refput(req_err_uri); - return error; -} - -/* Validate if the ancestor URI error'd */ -static int -check_ancestor_error(struct rpki_uri *requested_uri) -{ - int error; - - error = reqs_errors_foreach(ancestor_error, requested_uri); - if (error < 0) - return error; - /* Return the requests error'd code */ - if (error == EEXIST) - return EREQFAILED; - - return 0; -} - /** * @is_ta: Are we rsync'ing the TA? * The TA rsync will not be recursive, and will force SYNC_STRICT @@ -577,18 +525,12 @@ rsync_download_files(struct rpki_uri *requested_uri, bool is_ta, bool force) if (!force && is_already_downloaded(requested_uri, visited_uris)) { pr_val_debug("No need to redownload '%s'.", uri_val_get_printable(requested_uri)); - return check_ancestor_error(requested_uri); - } - - if (!force) { - error = get_rsync_uri(requested_uri, is_ta, &rsync_uri); - } else { - error = check_ancestor_error(requested_uri); - if (error) - return error; - error = handle_strict_strategy(requested_uri, &rsync_uri); + return 0; } + error = force + ? handle_strict_strategy(requested_uri, &rsync_uri) + : get_rsync_uri(requested_uri, is_ta, &rsync_uri); if (error) return error; @@ -600,13 +542,8 @@ rsync_download_files(struct rpki_uri *requested_uri, bool is_ta, bool force) /* Don't store when "force" and if its already downloaded */ if (!(force && is_already_downloaded(rsync_uri, visited_uris))) mark_as_downloaded(rsync_uri, visited_uris); - reqs_errors_rem_uri(uri_get_global(rsync_uri)); break; case EREQFAILED: - /* All attempts failed, avoid future requests */ - error = reqs_errors_add_uri(uri_get_global(rsync_uri)); - if (error) - break; mark_as_downloaded(rsync_uri, visited_uris); error = EREQFAILED; /* Return the original error */ break;