Refactors resulting from the issue #83 review

Mostly quality of life improvements.

On the other hand, it looks like the notfatal hash table API was being
used incorrectly. HASH_ADD_KEYPTR can OOM, but `errno` wasn't being
catched.

Fixing this is nontrivial, however, because strange `reqs_error`
functions are in the way, and that's a spaggetti I decided to avoid.
Instead, I converted HASH_ADD_KEYPTR usage to the fatal hash table API.
That's the future according to #40, anyway.

I don't think this has anything to do with #83, though.

diff --git a/src/data_structure/uthash_nonfatal.h b/src/data_structure/uthash_nonfatal.h
index 76fbef6b5353a952b1e41b50323135b012b02fad..3920639661092947e794f2faec366d57d9099db7 100644 (file)
--- a/src/data_structure/uthash_nonfatal.h
+++ b/src/data_structure/uthash_nonfatal.h
@@ -16,6 +16,26 @@
  * this is the caller's responsibility.
  *
  * TODO I think most of the code is not checking this.
+ *
+ * Functions that can OOM:
+ *
+ *     HASH_ADD_TO_TABLE
+ *             HASH_ADD_KEYPTR_BYHASHVALUE_INORDER
+ *                     HASH_REPLACE_BYHASHVALUE_INORDER
+ *                             HASH_REPLACE_INORDER
+ *                     HASH_ADD_KEYPTR_INORDER
+ *                             HASH_ADD_INORDER
+ *                     HASH_ADD_BYHASHVALUE_INORDER
+ *             HASH_ADD_KEYPTR_BYHASHVALUE
+ *                     HASH_REPLACE_BYHASHVALUE
+ *                             HASH_REPLACE (*)
+ *                     HASH_ADD_KEYPTR (**)
+ *                             HASH_ADD
+ *                     HASH_ADD_BYHASHVALUE
+ *     HASH_SELECT
+ *
+ * (*) Used by Fort
+ * (**) Used by Fort, but in its fatal uthash form.
  */
 #define HASH_NONFATAL_OOM 1
 #define uthash_nonfatal_oom(obj)                                       \

diff --git a/src/object/tal.c b/src/object/tal.c
index b63ca3af4d75b6113595d6a19fcbc13256781c71..470e39c43588fb6c6ebc4e26c2b9538edfbfdb27 100644 (file)
--- a/src/object/tal.c
+++ b/src/object/tal.c
@@ -69,7 +69,7 @@ SLIST_HEAD(threads_list, validation_thread);
 struct tal_param {
        struct thread_pool *pool;
        struct db_table *db;
-       struct threads_list *threads;
+       struct threads_list threads;
 };
 
 static int
@@ -497,7 +497,8 @@ tal_get_spki(struct tal *tal, unsigned char const **buffer, size_t *len)
  * have been extracted from a TAL.
  */
 static int
-handle_tal_uri(struct tal *tal, struct rpki_uri *uri, void *arg)
+handle_tal_uri(struct tal *tal, struct rpki_uri *uri,
+    struct validation_thread *thread)
 {
        /*
         * Because of the way the foreach iterates, this function must return
@@ -513,7 +514,6 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, void *arg)
         * A "hard error" is any other error.
         */
 
-       struct validation_thread *thread_arg = arg;
        struct validation_handler validation_handler;
        struct validation *state;
        struct cert_stack *certstack;
@@ -523,13 +523,13 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, void *arg)
        validation_handler.handle_roa_v4 = handle_roa_v4;
        validation_handler.handle_roa_v6 = handle_roa_v6;
        validation_handler.handle_router_key = handle_router_key;
-       validation_handler.arg = thread_arg->arg;
+       validation_handler.arg = thread->arg;
 
        error = validation_prepare(&state, tal, &validation_handler);
        if (error)
                return ENSURE_NEGATIVE(error);
 
-       if (thread_arg->sync_files) {
+       if (thread->sync_files) {
                if (uri_is_rsync(uri)) {
                        if (!config_get_rsync_enabled()) {
                                validation_destroy(state);
@@ -563,8 +563,8 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, void *arg)
        }
 
        /* At least one URI was sync'd */
-       thread_arg->retry_local = false;
-       if (thread_arg->sync_files && working_repo_peek() != NULL)
+       thread->retry_local = false;
+       if (thread->sync_files && working_repo_peek() != NULL)
                reqs_errors_rem_uri(working_repo_peek());
        working_repo_pop();
 
@@ -675,7 +675,7 @@ do_file_validation(void *thread_arg)
        if (error)
                goto destroy_tal;
 
-       error = foreach_uri(tal, __handle_tal_uri_sync, thread_arg);
+       error = foreach_uri(tal, __handle_tal_uri_sync, thread);
        if (error > 0) {
                error = 0;
                goto destroy_tal;
@@ -692,7 +692,7 @@ do_file_validation(void *thread_arg)
        thread->sync_files = false;
        pr_val_warn("Looking for the TA certificate at the local files.");
 
-       error = foreach_uri(tal, __handle_tal_uri_local, thread_arg);
+       error = foreach_uri(tal, __handle_tal_uri_local, thread);
        if (error > 0)
                error = 0;
        else if (error == 0)
@@ -749,7 +749,7 @@ __do_file_validation(char const *tal_file, void *arg)
                goto free_tal_file;
        }
 
-       SLIST_INSERT_HEAD(t_param->threads, thread, next);
+       SLIST_INSERT_HEAD(&t_param->threads, thread, next);
        return 0;
 
 free_tal_file:
@@ -764,64 +764,52 @@ free_db_rrdp:
 int
 perform_standalone_validation(struct thread_pool *pool, struct db_table *table)
 {
-       struct tal_param *param;
-       struct threads_list threads;
+       struct tal_param param;
        struct validation_thread *thread;
-       int error, t_error;
-
-       param = malloc(sizeof(struct tal_param));
-       if (param == NULL)
-               return pr_enomem();
+       int error;
 
        /* Set existent tal RRDP info to non visited */
        db_rrdp_reset_visited_tals();
 
-       SLIST_INIT(&threads);
-
-       param->pool = pool;
-       param->db = table;
-       param->threads = &threads;
+       param.pool = pool;
+       param.db = table;
+       SLIST_INIT(&param.threads);
 
        error = process_file_or_dir(config_get_tal(), TAL_FILE_EXTENSION, true,
-           __do_file_validation, param);
+           __do_file_validation, &param);
        if (error) {
                /* End all thread data */
-               while (!SLIST_EMPTY(&threads)) {
-                       thread = threads.slh_first;
-                       SLIST_REMOVE_HEAD(&threads, next);
+               while (!SLIST_EMPTY(&param.threads)) {
+                       thread = SLIST_FIRST(&param.threads);
+                       SLIST_REMOVE_HEAD(&param.threads, next);
                        thread_destroy(thread);
                }
-               free(param);
                return error;
        }
 
        /* Wait for all */
        thread_pool_wait(pool);
 
-       t_error = 0;
-       while (!SLIST_EMPTY(&threads)) {
-               thread = threads.slh_first;
-               SLIST_REMOVE_HEAD(&threads, next);
+       while (!SLIST_EMPTY(&param.threads)) {
+               thread = SLIST_FIRST(&param.threads);
+               SLIST_REMOVE_HEAD(&param.threads, next);
                if (thread->exit_status) {
-                       t_error = thread->exit_status;
+                       error = thread->exit_status;
                        pr_op_warn("Validation from TAL '%s' yielded error, discarding any other validation results.",
                            thread->tal_file);
                }
                thread_destroy(thread);
        }
 
-       /* The parameter isn't needed anymore */
-       free(param);
-
        /* Log the error'd URIs summary */
        reqs_errors_log_summary();
 
        /* One thread has errors, validation can't keep the resulting table */
-       if (t_error)
-               return t_error;
+       if (error)
+               return error;
 
        /* Remove non-visited rrdps URIS by tal */
        db_rrdp_rem_nonvisited_tals();
 
-       return error;
+       return 0;
 }

diff --git a/src/output_printer.c b/src/output_printer.c
index 34edcf869412e08960925c975a9930647a5b8bcd..570ef2f2e1ea26d030112677062a98ca32fe316d 100644 (file)
--- a/src/output_printer.c
+++ b/src/output_printer.c
@@ -157,7 +157,7 @@ open_file(char const *loc, FILE **out, bool *fopen)
 }
 
 static void
-print_roas(struct db_table *db)
+print_roas(struct db_table const *db)
 {
        FILE *out;
        JSON_OUT json_out;
@@ -188,7 +188,7 @@ print_roas(struct db_table *db)
 }
 
 static void
-print_router_keys(struct db_table *db)
+print_router_keys(struct db_table const *db)
 {
        FILE *out;
        JSON_OUT json_out;
@@ -222,7 +222,7 @@ print_router_keys(struct db_table *db)
 }
 
 void
-output_print_data(struct db_table *db)
+output_print_data(struct db_table const *db)
 {
        print_roas(db);
        print_router_keys(db);

diff --git a/src/output_printer.h b/src/output_printer.h
index 4bb089e2da27697fddbf93b072208ee3eafb686d..92b5b6a15b156c5ed41123f6b8bda1098ca0e65d 100644 (file)
--- a/src/output_printer.h
+++ b/src/output_printer.h
@@ -3,6 +3,6 @@
 
 #include "rtr/db/db_table.h"
 
-void output_print_data(struct db_table *);
+void output_print_data(struct db_table const *);
 
 #endif /* SRC_OUTPUT_PRINTER_H_ */

diff --git a/src/reqs_errors.c b/src/reqs_errors.c
index 951b93e055b644a9282e1e1821ef4c2bf216e9e9..03c3157f5b7c909c99e41ff2e48acc35f90c5e13 100644 (file)
--- a/src/reqs_errors.c
+++ b/src/reqs_errors.c
@@ -4,7 +4,7 @@
 #include <syslog.h>
 #include <time.h>
 
-#include "data_structure/uthash_nonfatal.h"
+#include "data_structure/uthash.h"
 #include "common.h"
 #include "config.h"
 #include "log.h"

diff --git a/src/rrdp/db/db_rrdp_uris.c b/src/rrdp/db/db_rrdp_uris.c
index 106c4b1eb672ca79fa049a79d4657aa0140ce31d..ea9e1533a367ff6f1f4fcdc11178ed004a130303 100644 (file)
--- a/src/rrdp/db/db_rrdp_uris.c
+++ b/src/rrdp/db/db_rrdp_uris.c
@@ -3,7 +3,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
-#include "data_structure/uthash_nonfatal.h"
+#include "data_structure/uthash.h"
 #include "common.h"
 #include "log.h"
 #include "thread_var.h"
@@ -94,6 +94,10 @@ add_rrdp_uri(struct db_rrdp_uri *uris, struct uris_table *new_uri)
 {
        struct uris_table *old_uri;
 
+       /*
+        * TODO (fine) this should use HASH_REPLACE instead of HASH_ADD_KEYPTR
+        */
+
        old_uri = find_rrdp_uri(uris, new_uri->uri);
        if (old_uri != NULL) {
                HASH_DELETE(hh, uris->table, old_uri);

diff --git a/src/rtr/db/db_table.c b/src/rtr/db/db_table.c
index 11eab2cbe41658a5d0419cbf5c1779ebc379e8eb..f18ee15bba6ae0739bfd614eada4b343d8e03539 100644 (file)
--- a/src/rtr/db/db_table.c
+++ b/src/rtr/db/db_table.c
@@ -57,7 +57,7 @@ db_table_destroy(struct db_table *table)
 }
 
 int
-db_table_foreach_roa(struct db_table *table, vrp_foreach_cb cb, void *arg)
+db_table_foreach_roa(struct db_table const *table, vrp_foreach_cb cb, void *arg)
 {
        struct hashable_roa *node, *tmp;
        int error;
@@ -72,8 +72,8 @@ db_table_foreach_roa(struct db_table *table, vrp_foreach_cb cb, void *arg)
 }
 
 int
-db_table_foreach_router_key(struct db_table *table, router_key_foreach_cb cb,
-    void *arg)
+db_table_foreach_router_key(struct db_table const *table,
+    router_key_foreach_cb cb, void *arg)
 {
        struct hashable_key *node, *tmp;
        int error;

diff --git a/src/rtr/db/db_table.h b/src/rtr/db/db_table.h
index 603bd702928c6e156e9fb6f627a86fb855680fc0..8562567ade7c11b019a931f0f0389d7fd9d50975 100644 (file)
--- a/src/rtr/db/db_table.h
+++ b/src/rtr/db/db_table.h
@@ -13,10 +13,10 @@ void db_table_destroy(struct db_table *);
 unsigned int db_table_roa_count(struct db_table *);
 unsigned int db_table_router_key_count(struct db_table *);
 
-int db_table_foreach_roa(struct db_table *, vrp_foreach_cb, void *);
+int db_table_foreach_roa(struct db_table const *, vrp_foreach_cb, void *);
 void db_table_remove_roa(struct db_table *, struct vrp const *);
 
-int db_table_foreach_router_key(struct db_table *, router_key_foreach_cb,
+int db_table_foreach_router_key(struct db_table const *, router_key_foreach_cb,
     void *);
 void db_table_remove_router_key(struct db_table *, struct router_key const *);
 

diff --git a/src/rtr/db/vrps.c b/src/rtr/db/vrps.c
index 40a3468baa4d84dc3fca75f94e8ce2f120427ce6..59a9875d6471e727cc381967fda9750b43615416 100644 (file)
--- a/src/rtr/db/vrps.c
+++ b/src/rtr/db/vrps.c
@@ -168,35 +168,32 @@ vrps_destroy(void)
                db_table_destroy(state.base);
 }
 
-#define WLOCK_HANDLER(lock, cb)                                                \
+#define WLOCK_HANDLER(cb)                                              \
        int error;                                                      \
-       rwlock_write_lock(lock);                                        \
+       rwlock_write_lock(&table_lock);                                 \
        error = cb;                                                     \
-       rwlock_unlock(lock);                                            \
+       rwlock_unlock(&table_lock);                                     \
        return error;
 
 int
 handle_roa_v4(uint32_t as, struct ipv4_prefix const *prefix,
     uint8_t max_length, void *arg)
 {
-       WLOCK_HANDLER(&table_lock,
-           rtrhandler_handle_roa_v4(arg, as, prefix, max_length))
+       WLOCK_HANDLER(rtrhandler_handle_roa_v4(arg, as, prefix, max_length))
 }
 
 int
 handle_roa_v6(uint32_t as, struct ipv6_prefix const * prefix,
     uint8_t max_length, void *arg)
 {
-       WLOCK_HANDLER(&table_lock,
-           rtrhandler_handle_roa_v6(arg, as, prefix, max_length))
+       WLOCK_HANDLER(rtrhandler_handle_roa_v6(arg, as, prefix, max_length))
 }
 
 int
 handle_router_key(unsigned char const *ski, uint32_t as,
     unsigned char const *spk, void *arg)
 {
-       WLOCK_HANDLER(&table_lock,
-           rtrhandler_handle_router_key(arg, ski, as, spk))
+       WLOCK_HANDLER(rtrhandler_handle_router_key(arg, ski, as, spk))
 }
 
 static int

diff --git a/src/validation_handler.c b/src/validation_handler.c
index fd58b72eb2ddfd6259cda369f2c5bfff095cb968..364ee1c5313c27f46e7f6ee999a241d7ef2ffe05 100644 (file)
--- a/src/validation_handler.c
+++ b/src/validation_handler.c
@@ -4,21 +4,20 @@
 #include "log.h"
 #include "thread_var.h"
 
-static int
-get_current_threads_handler(struct validation_handler const **result)
+static struct validation_handler const *
+get_current_threads_handler(void)
 {
        struct validation *state;
        struct validation_handler const *handler;
 
        state = state_retrieve();
        if (state == NULL)
-               return -EINVAL;
+               return NULL;
        handler = validation_get_validation_handler(state);
        if (handler == NULL)
                pr_crit("This thread lacks a validation handler.");
 
-       *result = handler;
-       return 0;
+       return handler;
 }
 
 int
@@ -26,11 +25,10 @@ vhandler_handle_roa_v4(uint32_t as, struct ipv4_prefix const *prefix,
     uint8_t max_length)
 {
        struct validation_handler const *handler;
-       int error;
 
-       error = get_current_threads_handler(&handler);
-       if (error)
-               return error;
+       handler = get_current_threads_handler();
+       if (handler == NULL)
+               return -EINVAL;
 
        return (handler->handle_roa_v4 != NULL)
            ? handler->handle_roa_v4(as, prefix, max_length, handler->arg)
@@ -42,11 +40,10 @@ vhandler_handle_roa_v6(uint32_t as, struct ipv6_prefix const *prefix,
     uint8_t max_length)
 {
        struct validation_handler const *handler;
-       int error;
 
-       error = get_current_threads_handler(&handler);
-       if (error)
-               return error;
+       handler = get_current_threads_handler();
+       if (handler == NULL)
+               return -EINVAL;
 
        return (handler->handle_roa_v6 != NULL)
            ? handler->handle_roa_v6(as, prefix, max_length, handler->arg)
@@ -58,11 +55,10 @@ vhandler_handle_router_key(unsigned char const *ski, uint32_t as,
     unsigned char const *spk)
 {
        struct validation_handler const *handler;
-       int error;
 
-       error = get_current_threads_handler(&handler);
-       if (error)
-               return error;
+       handler = get_current_threads_handler();
+       if (handler == NULL)
+               return -EINVAL;
 
        return (handler->handle_router_key != NULL)
            ? handler->handle_router_key(ski, as, spk, handler->arg)

diff --git a/src/visited_uris.c b/src/visited_uris.c
index 987c73575a2276bf34ed63a5de0cdf1960aaa167..be9fdc8da0bc5995fd8d10d4d3f03bb80f2c9b5a 100644 (file)
--- a/src/visited_uris.c
+++ b/src/visited_uris.c
@@ -6,7 +6,7 @@
 #include "log.h"
 #include "delete_dir_daemon.h"
 #include "data_structure/array_list.h"
-#include "data_structure/uthash_nonfatal.h"
+#include "data_structure/uthash.h"
 
 struct visited_elem {
        /* key */