From c7ffc2dcb680eb95fef58766e7b4c44f7c309282 Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Tue, 2 Mar 2021 19:01:29 -0600 Subject: [PATCH] Issue #46: Code review - Mandatory cgcc cleanup. - Change the VRPS read-write lock into a mutex. (Because there are no readers.) - Remove `static` modifier from `rtrhandler_handle_roa_v4()`'s helper functions (in the hopes of getting a slightly more revealing stack trace). Still haven't found the problem. It behaves like a concurrency error, but doesn't look possible. --- src/common.c | 26 ++++++++++++++++++++++++++ src/common.h | 4 +++- src/config/rrdp_conf.c | 16 ++++++++-------- src/log.c | 4 ++-- src/object/tal.c | 2 +- src/rtr/db/db_table.c | 4 ++-- src/rtr/db/vrps.c | 41 ++++++++++++++++++----------------------- 7 files changed, 60 insertions(+), 37 deletions(-) diff --git a/src/common.c b/src/common.c index 05e386e0..b3a5496b 100644 --- a/src/common.c +++ b/src/common.c @@ -13,6 +13,32 @@ #include "config.h" #include "log.h" +void +mutex_lock(pthread_mutex_t *lock) +{ + int error; + + error = pthread_mutex_lock(lock); + if (error) { + pr_op_err("pthread_mutex_lock() returned error code %d. This is too critical for a graceful recovery; I must die now.", + error); + exit(error); + } +} + +void +mutex_unlock(pthread_mutex_t *lock) +{ + int error; + + error = pthread_mutex_unlock(lock); + if (error) { + pr_op_err("pthread_mutex_unlock() returned error code %d. This is too critical for a graceful recovery; I must die now.", + error); + exit(error); + } +} + int rwlock_read_lock(pthread_rwlock_t *lock) { diff --git a/src/common.h b/src/common.h index 24fd976a..d74a4ee5 100644 --- a/src/common.h +++ b/src/common.h @@ -40,9 +40,11 @@ #define ARRAY_LEN(array) (sizeof(array) / sizeof((array)[0])) /* - * rwlock wrappers. They are just a bunch of boilerplate, and removal of + * mutex wrappers. They are just a bunch of boilerplate, and removal of * unrecoverable resulting error codes. */ +void mutex_lock(pthread_mutex_t *); +void mutex_unlock(pthread_mutex_t *); int rwlock_read_lock(pthread_rwlock_t *); void rwlock_write_lock(pthread_rwlock_t *); void rwlock_unlock(pthread_rwlock_t *); diff --git a/src/config/rrdp_conf.c b/src/config/rrdp_conf.c index 4ec952f2..bdc4d83e 100644 --- a/src/config/rrdp_conf.c +++ b/src/config/rrdp_conf.c @@ -69,7 +69,7 @@ set_retry_interval(char const *name, unsigned int value) return 0; } -int +static int parse_argv_enabled(struct option_field const *field, char const *str, void *result) { @@ -82,7 +82,7 @@ parse_argv_enabled(struct option_field const *field, char const *str, return set_rrdp_enabled(field->name, DEREFERENCE_BOOL(result)); } -int +static int parse_json_enabled(struct option_field const *opt, struct json_t *json, void *result) { @@ -95,7 +95,7 @@ parse_json_enabled(struct option_field const *opt, struct json_t *json, return set_rrdp_enabled(opt->name, DEREFERENCE_BOOL(result)); } -int +static int parse_argv_priority(struct option_field const *field, char const *str, void *result) { @@ -108,7 +108,7 @@ parse_argv_priority(struct option_field const *field, char const *str, return set_priority(field->name, DEREFERENCE_UINT32(result)); } -int +static int parse_json_priority(struct option_field const *opt, json_t *json, void *result) { int error; @@ -120,7 +120,7 @@ parse_json_priority(struct option_field const *opt, json_t *json, void *result) return set_priority(opt->name, DEREFERENCE_UINT32(result)); } -int +static int parse_argv_retry_count(struct option_field const *field, char const *str, void *result) { @@ -133,7 +133,7 @@ parse_argv_retry_count(struct option_field const *field, char const *str, return set_retry_count(field->name, DEREFERENCE_UINT(result)); } -int +static int parse_json_retry_count(struct option_field const *opt, json_t *json, void *result) { @@ -146,7 +146,7 @@ parse_json_retry_count(struct option_field const *opt, json_t *json, return set_retry_count(opt->name, DEREFERENCE_UINT(result)); } -int +static int parse_argv_retry_interval(struct option_field const *field, char const *str, void *result) { @@ -159,7 +159,7 @@ parse_argv_retry_interval(struct option_field const *field, char const *str, return set_retry_interval(field->name, DEREFERENCE_UINT(result)); } -int +static int parse_json_retry_interval(struct option_field const *opt, json_t *json, void *result) { diff --git a/src/log.c b/src/log.c index 5f56159c..5791cd49 100644 --- a/src/log.c +++ b/src/log.c @@ -183,7 +183,7 @@ __fprintf(int level, char const *prefix, bool color_output, if (color_output) fprintf(lvl->stream, "%s", lvl->color); - now = time(0); + now = time(NULL); if (now != ((time_t) -1)) { localtime_r(&now, &stm_buff); strftime(time_buff, sizeof(time_buff), "%b %e %T", &stm_buff); @@ -255,7 +255,7 @@ pr_stream(int level, char const *prefix, const char *format, bool color_output, if (color_output) fprintf(lvl->stream, "%s", lvl->color); - now = time(0); + now = time(NULL); if (now != ((time_t) -1)) { localtime_r(&now, &stm_buff); strftime(time_buff, sizeof(time_buff), "%b %e %T", &stm_buff); diff --git a/src/object/tal.c b/src/object/tal.c index f50bfe89..02a4901f 100644 --- a/src/object/tal.c +++ b/src/object/tal.c @@ -404,7 +404,7 @@ void tal_destroy(struct tal *tal) free(tal); } -int +static int foreach_uri(struct tal *tal, foreach_uri_cb cb, void *arg) { unsigned int i; diff --git a/src/rtr/db/db_table.c b/src/rtr/db/db_table.c index 58a095e3..9e86db20 100644 --- a/src/rtr/db/db_table.c +++ b/src/rtr/db/db_table.c @@ -85,7 +85,7 @@ db_table_foreach_router_key(struct db_table *table, router_key_foreach_cb cb, return 0; } -static struct hashable_roa * +struct hashable_roa * create_roa(uint32_t asn, uint8_t max_length) { struct hashable_roa *roa; @@ -102,7 +102,7 @@ create_roa(uint32_t asn, uint8_t max_length) return roa; } -static int +int add_roa(struct db_table *table, struct hashable_roa *new) { struct hashable_roa *old; diff --git a/src/rtr/db/vrps.c b/src/rtr/db/vrps.c index ec6901c1..9f21cd03 100644 --- a/src/rtr/db/vrps.c +++ b/src/rtr/db/vrps.c @@ -68,7 +68,7 @@ static struct state state; static pthread_rwlock_t state_lock; /** Lock to protect ROA table during construction. */ -static pthread_rwlock_t table_lock; +static pthread_mutex_t table_lock; void deltagroup_cleanup(struct delta_group *group) @@ -113,7 +113,7 @@ vrps_init(void) goto release_deltas; } - error = pthread_rwlock_init(&table_lock, NULL); + error = pthread_mutex_init(&table_lock, NULL); if (error) { error = pr_op_errno(error, "table pthread_rwlock_init() errored"); goto release_state_lock; @@ -137,45 +137,40 @@ vrps_destroy(void) deltas_db_cleanup(&state.deltas, deltagroup_cleanup); /* Nothing to do with error codes from now on */ pthread_rwlock_destroy(&state_lock); - pthread_rwlock_destroy(&table_lock); + pthread_mutex_destroy(&table_lock); } -#define WLOCK_HANDLER(lock, cb) \ - int error; \ - rwlock_write_lock(lock); \ - error = cb; \ - rwlock_unlock(lock); \ - return error; - -#define RLOCK_HANDLER(lock, cb) \ - int error; \ - rwlock_read_lock(lock); \ - error = cb; \ - rwlock_unlock(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)) + int error; + mutex_lock(&table_lock); + error = rtrhandler_handle_roa_v4(arg, as, prefix, max_length); + mutex_unlock(&table_lock); + return error; } 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)) + int error; + mutex_lock(&table_lock); + error = rtrhandler_handle_roa_v6(arg, as, prefix, max_length); + mutex_unlock(&table_lock); + return error; } 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)) + int error; + mutex_lock(&table_lock); + error = rtrhandler_handle_router_key(arg, ski, as, spk); + mutex_unlock(&table_lock); + return error; } static int -- 2.47.3