From 99725b909b8ea82ba59ab589325efe5389713ace Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Thu, 27 May 2021 17:04:36 -0500 Subject: [PATCH] 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. --- src/common.c | 18 ++---------------- src/config.c | 10 ++-------- src/config.h | 1 - src/delete_dir_daemon.c | 3 +-- src/http/http.c | 2 +- src/object/tal.c | 7 +++---- src/rrdp/rrdp_loader.c | 6 +++--- src/rtr/rtr.c | 12 ++++-------- src/thread/thread_pool.h | 2 +- test/thread_pool_test.c | 3 +-- 10 files changed, 18 insertions(+), 46 deletions(-) diff --git a/src/common.c b/src/common.c index de96f8e0..83b6fe67 100644 --- 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 2d4c4668..5194e288 100644 --- 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 0994515e..9e4c1dcf 100644 --- 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 2c3ef20f..0aebaa41 100644 --- 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 dd497d80..30bf491d 100644 --- 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 870cdc12..2bc6ba04 100644 --- 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 14dd65f3..e0aeb439 100644 --- 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 9678896a..4d39a861 100644 --- 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 c294f7df..b714cf41 100644 --- 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 c9c526bd..0c984138 100644 --- 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 -- 2.47.2