Code review

1. (error) Fix the --work-offline flag.
   It has been unused since commit
   85478ff30ebc029abb0ded48de5b557f52a758e0.
2. (performance) Remove redundant fopen() and fclose() during
   valid_file_or_dir().
   If stat() is used instead of fstat(), there's no need to open and
   close the file.
   (Technically, it's no longer validating readabilty, but since the
   validator downloads the files, read permission errors should be
   extremely rare, and can be catched later.)
3. (fine) Remove return value from thread_pool_task_cb.
   This wasn't a problem, but the return value was meaningless, and
   no callers were using it.

diff --git a/src/common.c b/src/common.c
index de96f8e0bfaeac10aee926f2ef170d4f08e6c128..83b6fe67efb640d412e3e9c8839f32422d9733b9 100644 (file)
--- a/src/common.c
+++ b/src/common.c
@@ -189,7 +189,6 @@ bool
 valid_file_or_dir(char const *location, bool check_file, bool check_dir,
     int (*error_fn)(int error, const char *format, ...))
 {
-       FILE *file;
        struct stat attr;
        bool is_file, is_dir;
        bool result;
@@ -197,21 +196,12 @@ valid_file_or_dir(char const *location, bool check_file, bool check_dir,
        if (!check_file && !check_dir)
                pr_crit("Wrong usage, at least one check must be 'true'.");
 
-       result = false;
-       file = fopen(location, "rb");
-       if (file == NULL) {
+       if (stat(location, &attr) == -1) {
                if (error_fn != NULL)
-                       error_fn(errno, "Could not open location '%s'",
-                           location);
+                       error_fn(errno, "stat(%s) failed", location);
                return false;
        }
 
-       if (fstat(fileno(file), &attr) == -1) {
-               if (error_fn != NULL)
-                       error_fn(errno, "fstat(%s) failed", location);
-               goto end;
-       }
-
        is_file = check_file && S_ISREG(attr.st_mode);
        is_dir = check_dir && S_ISDIR(attr.st_mode);
 
@@ -221,10 +211,6 @@ valid_file_or_dir(char const *location, bool check_file, bool check_dir,
                    (check_file && check_dir) ? "file or directory" :
                    (check_file) ? "file" : "directory");
 
-end:
-       if (fclose(file) == -1)
-               if (error_fn != NULL)
-                       error_fn(errno, "fclose() failed");
        return result;
 }
 

diff --git a/src/config.c b/src/config.c
index 2d4c4668a4f6fe233b78df3c475286bdf980b08c..5194e288c12625839d526bd750968bfff2ce031b 100644 (file)
--- a/src/config.c
+++ b/src/config.c
@@ -1308,12 +1308,6 @@ config_get_server_queue(void)
        return rpki_config.server.backlog;
 }
 
-bool
-config_get_work_offline(void)
-{
-       return rpki_config.work_offline;
-}
-
 unsigned int
 config_get_validation_interval(void)
 {
@@ -1455,7 +1449,7 @@ config_get_val_log_facility(void)
 bool
 config_get_rsync_enabled(void)
 {
-       return rpki_config.rsync.enabled;
+       return !rpki_config.work_offline && rpki_config.rsync.enabled;
 }
 
 unsigned int
@@ -1510,7 +1504,7 @@ config_get_rsync_args(bool is_ta)
 bool
 config_get_http_enabled(void)
 {
-       return rpki_config.http.enabled;
+       return !rpki_config.work_offline && rpki_config.http.enabled;
 }
 
 unsigned int

diff --git a/src/config.h b/src/config.h
index 0994515e7a7a7a5bcb0eadc67b5ada605d063bb3..9e4c1dcfb46ddbba91b2493a79d6b88930f9f646 100644 (file)
--- a/src/config.h
+++ b/src/config.h
@@ -31,7 +31,6 @@ char const *config_get_local_repository(void);
 bool config_get_shuffle_tal_uris(void);
 unsigned int config_get_max_cert_depth(void);
 enum mode config_get_mode(void);
-bool config_get_work_offline(void);
 char const *config_get_http_user_agent(void);
 unsigned int config_get_http_connect_timeout(void);
 unsigned int config_get_http_transfer_timeout(void);

diff --git a/src/delete_dir_daemon.c b/src/delete_dir_daemon.c
index 2c3ef20f931ba924f1e455c694e18873279d968b..0aebaa41b93b90e0f87f836dc69c0ddfb64d8ff4 100644 (file)
--- a/src/delete_dir_daemon.c
+++ b/src/delete_dir_daemon.c
@@ -69,7 +69,7 @@ traverse(char const *path, struct stat const *sb, int flag, struct FTW *ftwbuf)
        }
 }
 
-static void *
+static void
 remove_from_root(void *arg)
 {
        struct rem_dirs *root_arg = arg;
@@ -100,7 +100,6 @@ remove_from_root(void *arg)
 
        pr_op_debug("Done removing dirs.");
        free(dirs_arr);
-       return NULL;
 }
 
 /*

diff --git a/src/http/http.c b/src/http/http.c
index dd497d80cbad1272a27bbeb313f9611604b63c94..30bf491d680c6a2373a76c936a639a148d796161 100644 (file)
--- a/src/http/http.c
+++ b/src/http/http.c
@@ -158,7 +158,7 @@ http_fetch(struct http_handler *handler, char const *uri, long *response_code,
        setopt_str(handler->curl, CURLOPT_URL, uri);
        setopt_writefunction(handler->curl, cb, arg);
 
-       pr_val_debug("Doing HTTP GET to '%s'.", uri);
+       pr_val_debug("HTTP GET: %s", uri);
        res = curl_easy_perform(handler->curl);
 
        res2 = curl_easy_getinfo(handler->curl, CURLINFO_RESPONSE_CODE,

diff --git a/src/object/tal.c b/src/object/tal.c
index 870cdc12777c94bd0d29fcfec7339d2dff4f98b6..2bc6ba049220a57c62391299b8042b36fc89c110 100644 (file)
--- a/src/object/tal.c
+++ b/src/object/tal.c
@@ -532,13 +532,13 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, void *arg)
                if (uri_is_rsync(uri)) {
                        if (!config_get_rsync_enabled()) {
                                validation_destroy(state);
-                               return 0; /* Soft error */
+                               return 0; /* Try some other TAL URI */
                        }
                        error = rsync_download_files(uri, true, false);
                } else /* HTTPS */ {
                        if (!config_get_http_enabled()) {
                                validation_destroy(state);
-                               return 0; /* Soft error */
+                               return 0; /* Try some other TAL URI */
                        }
                        error = http_download_file(uri,
                            reqs_errors_log_uri(uri_get_global(uri)));
@@ -654,7 +654,7 @@ __handle_tal_uri_local(struct tal *tal, struct rpki_uri *uri, void *arg)
        return handle_tal_uri(tal, uri, arg);
 }
 
-static void *
+static void
 do_file_validation(void *thread_arg)
 {
        struct validation_thread *thread = thread_arg;
@@ -703,7 +703,6 @@ end:
        working_repo_cleanup();
        fnstack_cleanup();
        thread->exit_status = error;
-       return NULL;
 }
 
 static void

diff --git a/src/rrdp/rrdp_loader.c b/src/rrdp/rrdp_loader.c
index 14dd65f3346d99333a1cac3e99404596a790334d..e0aeb439d359f5266e63b44bde2a4e07a75f68b1 100644 (file)
--- a/src/rrdp/rrdp_loader.c
+++ b/src/rrdp/rrdp_loader.c
@@ -274,9 +274,9 @@ upd_end:
  * were no errors, just return success; otherwise, return error code -EPERM.
  *
  * @data_updated will be true if:
- * - Delta files were processed
- * - Snapshot file was processed
- * - @uri was already visited at this cycle
+ * - Delta files were processed,
+ * - Snapshot file was processed,
+ * - or @uri was already visited at this cycle
  */
 int
 rrdp_load(struct rpki_uri *uri, bool *data_updated)

diff --git a/src/rtr/rtr.c b/src/rtr/rtr.c
index 9678896a9c69c11f143c8368d335af8566781880..4d39a8618a26a1b3e65bb38d3c24acbcf960fe07 100644 (file)
--- a/src/rtr/rtr.c
+++ b/src/rtr/rtr.c
@@ -437,7 +437,7 @@ reject_client(int fd, struct sockaddr_storage *addr)
  * The client socket threads' entry routine.
  * @arg must be released.
  */
-static void *
+static void
 client_thread_cb(void *arg)
 {
        struct pdu_metadata const *meta;
@@ -451,7 +451,7 @@ client_thread_cb(void *arg)
        error = clients_add(param.fd, param.addr);
        if (error) {
                close(param.fd);
-               return NULL;
+               return;
        }
 
        while (true) { /* For each PDU... */
@@ -466,8 +466,6 @@ client_thread_cb(void *arg)
        }
 
        clients_forget(param.fd, end_client, CL_CLOSED);
-
-       return NULL;
 }
 
 static void
@@ -483,7 +481,7 @@ init_fdset(struct server_fds *fds, fd_set *fdset)
 /*
  * Waits for client connections and spawns threads to handle them.
  */
-static void *
+static void
 handle_client_connections(void *arg)
 {
        struct sockaddr_storage client_addr;
@@ -533,7 +531,7 @@ handle_client_connections(void *arg)
                        case VERDICT_RETRY:
                                continue;
                        case VERDICT_EXIT:
-                               return NULL;
+                               return;
                        }
 
                        /*
@@ -578,8 +576,6 @@ handle_client_connections(void *arg)
                        }
                }
        } while (true);
-
-       return NULL;
 }
 
 static int

diff --git a/src/thread/thread_pool.h b/src/thread/thread_pool.h
index c294f7df98212e4366ccca73c327addb862e8d99..b714cf41faa00d28766d03ec997fc11f4b8bd995 100644 (file)
--- a/src/thread/thread_pool.h
+++ b/src/thread/thread_pool.h
@@ -9,7 +9,7 @@ struct thread_pool;
 int thread_pool_create(char const *, unsigned int, struct thread_pool **);
 void thread_pool_destroy(struct thread_pool *);
 
-typedef void *(*thread_pool_task_cb)(void *);
+typedef void (*thread_pool_task_cb)(void *);
 int thread_pool_push(struct thread_pool *, char const *, thread_pool_task_cb,
     void *);
 

diff --git a/test/thread_pool_test.c b/test/thread_pool_test.c
index c9c526bdd8305f3f76eefe89848c9c94dab6b844..0c9841384d2c2d4eb0aac1fd424a3deeaf84f64d 100644 (file)
--- a/test/thread_pool_test.c
+++ b/test/thread_pool_test.c
@@ -6,12 +6,11 @@
 #include "impersonator.c"
 #include "thread/thread_pool.c"
 
-static void *
+static void
 thread_work(void *arg)
 {
        int *value = arg;
        (*value) += 2;
-       return NULL;
 }
 
 static void