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.

diff --git a/src/common.c b/src/common.c
index 05e386e0b8838150c4abe334ee5a1b1f55014a54..b3a5496ba273afb7ae2a8bda72f485860bf05699 100644 (file)
--- 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 24fd976a0933b7d1cf79fbd5dfe2b99d8e3b65a3..d74a4ee590db990fd515b276c05451f819d11dd2 100644 (file)
--- 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 4ec952f2471619ed3b6cfb36051c3e9c52a018b4..bdc4d83eb9067b3a6fb765b3d62886efdec0b02b 100644 (file)
--- 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 5f56159c80a354bef20f20c869858481b6160dab..5791cd49d8efc269e9d49d1010d0984064b82106 100644 (file)
--- 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 f50bfe891ed10a8c419539fd40d023b3614dd7c2..02a4901ff01d5c8a99013898050bbdd9a54318cb 100644 (file)
--- 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 58a095e3112cd3df511f3e4d52dd91849a57a513..9e86db20a99a4df535b15df7d19e20a98315a539 100644 (file)
--- 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 ec6901c10972c4aeefd19da9d825e8b24f8da09b..9f21cd037299b12d92f0d7b18cd446968982485b 100644 (file)
--- 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