From d9dedbac91840e8ea6d2ddd98ff4afa549760580 Mon Sep 17 00:00:00 2001 From: Otavio Pontes Date: Tue, 9 Apr 2019 16:10:45 -0700 Subject: [PATCH] scripts: Use run_command() instead of system() for all scripts Also reorganize, change some functions name and create a .h for scripts module. Signed-off-by: Otavio Pontes --- src/bundle.c | 2 +- src/lib/strings.c | 14 ++ src/lib/strings.h | 8 ++ src/lib/sys.c | 91 ++++++++----- src/lib/sys.h | 18 +++ src/scripts.c | 128 +++++++++---------- src/scripts.h | 32 +++++ src/swupd.h | 4 +- src/update.c | 4 +- src/verify.c | 2 +- test/functional/bundleadd/add-boot-file.bats | 2 +- test/functional/ignore-list | 2 +- test/functional/update/update-boot-file.bats | 2 +- test/functional/verify/verify-boot-file.bats | 2 +- test/functional/verify/verify-json.bats | 4 +- 15 files changed, 203 insertions(+), 112 deletions(-) create mode 100644 src/scripts.h diff --git a/src/bundle.c b/src/bundle.c index 505f6d66..4cb8e334 100644 --- a/src/bundle.c +++ b/src/bundle.c @@ -1022,7 +1022,7 @@ static enum swupd_code install_bundles(struct list *bundles, struct list **subs, /* step 7: Run any scripts that are needed to complete update */ timelist_timer_start(global_times, "Run Scripts"); progress_set_step(7, "run_scripts"); - run_scripts(false); + scripts_run_post_update(false); timelist_timer_stop(global_times); // closing: Run Scripts progress_complete_step(); diff --git a/src/lib/strings.c b/src/lib/strings.c index c1a84987..5a4bc0c3 100644 --- a/src/lib/strings.c +++ b/src/lib/strings.c @@ -52,6 +52,20 @@ void string_or_die(char **strp, const char *fmt, ...) va_end(ap); } +char *str_or_die(const char *fmt, ...) +{ + char *str; + va_list ap; + + va_start(ap, fmt); + if (vasprintf(&str, fmt, ap) < 0) { + abort(); + } + va_end(ap); + + return str; +} + void free_string(char **s) { if (s) { diff --git a/src/lib/strings.h b/src/lib/strings.h index 1d1570a2..4bd94ad2 100644 --- a/src/lib/strings.h +++ b/src/lib/strings.h @@ -12,6 +12,14 @@ extern "C" { */ void string_or_die(char **strp, const char *fmt, ...); +/* + * Return a new allocated string with the content printed from fmt and + * parameters using vasprintf. Abort on memory allocation errors. + * + * Similar to string_or_die(), but returns the string pointer. + */ +char *str_or_die(const char *fmt, ...); + /* Return a duplicated copy of the string using strdup(). * Abort if there's no memory to allocate the new string. */ diff --git a/src/lib/sys.c b/src/lib/sys.c index d2bcd02b..466fe5a9 100644 --- a/src/lib/sys.c +++ b/src/lib/sys.c @@ -20,6 +20,7 @@ #include "sys.h" #include "list.h" #include "log.h" +#include "macros.h" #include "memory.h" #include "strings.h" @@ -53,9 +54,11 @@ static int replace_fd(int fd, const char *fd_file) return replaced_fd; } -int run_command_full(const char *stdout_file, const char *stderr_file, const char *cmd, ...) +int run_command_full_params(const char *stdout_file, const char *stderr_file, char **params) { int pid, ret, child_ret; + const char *cmd = params[0]; + ; pid = fork(); if (pid < 0) { @@ -77,34 +80,6 @@ int run_command_full(const char *stdout_file, const char *stderr_file, const cha } // Child - va_list ap; - char **params; - int args_count = 0, i = 0; - char *arg; - - va_start(ap, cmd); - while ((arg = va_arg(ap, char *)) != NULL) { - args_count++; - } - va_end(ap); - - // Create params array with space for all parameters, - // command basename and NULL terminator - params = malloc(sizeof(char *) * (args_count + 2)); - if (!params) { - goto error_child; - } - - params[i++] = (char *)cmd; - - va_start(ap, cmd); - while ((arg = va_arg(ap, char *)) != NULL) { - params[i++] = arg; - } - va_end(ap); - - params[i] = NULL; - if (replace_fd(STDOUT_FILENO, stdout_file) < 0) { goto error_child; } @@ -117,7 +92,6 @@ int run_command_full(const char *stdout_file, const char *stderr_file, const cha if (ret < 0) { error("run_command %s failed: %i (%s)\n", cmd, errno, strerror(errno)); } - free(params); exit(EXIT_FAILURE); // execve nevers return on success return 0; @@ -127,6 +101,41 @@ error_child: return 0; } +int run_command_full(const char *stdout_file, const char *stderr_file, const char *cmd, ...) +{ + char **params; + int args_count = 0, i = 0; + va_list ap; + char *arg; + int ret = 0; + + va_start(ap, cmd); + while ((arg = va_arg(ap, char *)) != NULL) { + args_count++; + } + va_end(ap); + + // Create params array with space for all parameters, + // command basename and NULL terminator + params = malloc(sizeof(char *) * (args_count + 2)); + ON_NULL_ABORT(params); + + params[i++] = (char *)cmd; + + va_start(ap, cmd); + while ((arg = va_arg(ap, char *)) != NULL) { + params[i++] = arg; + } + va_end(ap); + + params[i] = NULL; + + ret = run_command_full_params(stdout_file, stderr_file, params); + + free(params); + return ret; +} + long get_available_space(const char *path) { struct statvfs stat; @@ -236,6 +245,11 @@ int systemctl_restart(const char *service) return systemctl_cmd("restart", service, NULL); } +int systemctl_restart_noblock(const char *service) +{ + return systemctl_cmd("restart", service, NULL); +} + bool systemctl_active(void) { /* In a container, "/usr/bin/systemctl" will return 1 with @@ -246,3 +260,20 @@ bool systemctl_active(void) return systemctl_cmd(NULL) == 0; } + +int systemctl_daemon_reexec(void) +{ + return systemctl_cmd("daemon-reexec", NULL); +} + +int systemctl_daemon_reload(void) +{ + return systemctl_cmd("daemon-reload", NULL); +} + +bool systemd_in_container(void) +{ + /* systemd-detect-virt -c does container detection only * + * The return code is zero if the system is in a container */ + return !run_command("/usr/bin/systemd-detect-virt", "-c"); +} diff --git a/src/lib/sys.h b/src/lib/sys.h index b78c459a..c0a7de3c 100644 --- a/src/lib/sys.h +++ b/src/lib/sys.h @@ -20,6 +20,9 @@ long get_available_space(const char *path); /* run_command: runs a command with standard and error output (stdout and stderr) set */ #define run_command(...) run_command_full(NULL, NULL, __VA_ARGS__) +/* run_command_params: runs a command from params with standard and error output (stdout and stderr) set */ +#define run_command_params(_params) run_command_full_params(NULL, NULL, _params) + /* run_command_full: Run command cmd with parameters informed in a NULL * terminated list of strings. * @@ -38,6 +41,12 @@ long get_available_space(const char *path); */ int run_command_full(const char *stdout_file, const char *stderr_file, const char *cmd, ...); +/* run_command_full_params: Run command from a NULL terminated string array of + * parameters. First parameter of the list should be the full path to the + * command to be executed. + */ +int run_command_full_params(const char *stdout_file, const char *stderr_file, char **params); + /* copy_all: Runs cp -a [src] [dst] using run_command_quiet */ int copy_all(const char *src, const char *dst); @@ -73,6 +82,11 @@ void journal_log_error(const char *message); */ int systemctl_restart(const char *service); +/* + * Restart a systemd service without blocking + */ +int systemctl_restart_noblock(const char *service); + /* * Check if systemd is active and running in the system. * Necessary because not all commands returns correct error codes when running @@ -80,6 +94,10 @@ int systemctl_restart(const char *service); */ bool systemctl_active(void); +int systemctl_daemon_reexec(void); +int systemctl_daemon_reload(void); +bool systemd_in_container(void); + #define systemctl_cmd(...) run_command_quiet(SYSTEMCTL, __VA_ARGS__) #ifdef __cplusplus diff --git a/src/scripts.c b/src/scripts.c index fb83a8db..59d1a3cb 100644 --- a/src/scripts.c +++ b/src/scripts.c @@ -33,64 +33,69 @@ #include "config.h" #include "swupd.h" -static bool in_container(void) -{ - /* systemd-detect-virt -c does container detection only * - * The return code is zero if the system is in a container */ - return !run_command("/usr/bin/systemd-detect-virt", "-c"); -} +#define CLEAR_SERVICE_RESTART_SCRIPT "/usr/bin/clr-service-restart" -static void run_script(char *scriptname, char *cmd) -{ - struct stat s; - __attribute__((unused)) int ret = 0; - - /* make sure the script exists before attempting to execute it */ - if (stat(scriptname, &s) == 0 && (S_ISREG(s.st_mode))) { - ret = system(cmd); - } else { - warn("post-update helper script (%s) not found, it will be skipped\n", scriptname); - } -} +// Run script if it exists. It's a macro instead of a functions to be able +// to call another function with variable number of parameters +#define run_script_if_exists(_scriptname, ...) \ + do { \ + if (!file_is_executable(_scriptname)) { \ + warn("helper script (%s) not found, it will be skipped\n", _scriptname); \ + break; \ + } \ + run_command_full(NULL, NULL, _scriptname, __VA_ARGS__); \ + } while (0) static void update_boot(void) { - char *boot_update_cmd = NULL; char *scriptname; /* Don't run clr-boot-manager update in a container on the rootfs */ - if (strcmp("/", path_prefix) == 0 && in_container()) { + if (strcmp("/", path_prefix) == 0 && systemd_in_container()) { return; } if (strcmp("/", path_prefix) == 0) { - string_or_die(&scriptname, "/usr/bin/clr-boot-manager"); - string_or_die(&boot_update_cmd, "/usr/bin/clr-boot-manager update"); + run_script_if_exists("/usr/bin/clr-boot-manager", "update", NULL); } else { string_or_die(&scriptname, "%s/usr/bin/clr-boot-manager", path_prefix); - string_or_die(&boot_update_cmd, "%s/usr/bin/clr-boot-manager update --path %s", path_prefix, path_prefix); + run_script_if_exists(scriptname, "update", "--path", path_prefix, NULL); + free_string(&scriptname); + } +} + +void exec_post_update_script(bool reexec, bool block) +{ + + char *params[5]; + int i = 0; + bool has_path_prefix; + + has_path_prefix = strcmp("/", path_prefix) != 0; + + params[i++] = str_or_die("%s%s", has_path_prefix ? path_prefix : "", + POST_UPDATE); + + if (has_path_prefix) { + params[i++] = path_prefix; } - run_script(scriptname, boot_update_cmd); + if (block) { + params[i++] = "--no-block"; + } - free_string(&boot_update_cmd); - free_string(&scriptname); + if (reexec) { + params[i++] = "--reexec"; + } + + params[i++] = NULL; + + run_command_params(params); + free(params[0]); } static void update_triggers(bool block) { - char *cmd = NULL; - char *scriptname; - char const *block_flag = NULL; - char const *reexec_flag = NULL; - __attribute__((unused)) int ret = 0; - - if (!block) { - block_flag = "--no-block"; - } else { - block_flag = ""; - } - if (strlen(POST_UPDATE) == 0) { /* fall back to systemd if path prefix is not the rootfs * and the POST_UPDATE trigger wasn't specified */ @@ -98,49 +103,37 @@ static void update_triggers(bool block) return; } - ret = system("/usr/bin/systemctl > /dev/null 2>&1"); - if (ret != 0) { + if (!systemctl_active()) { warn("systemctl not operable, " "unable to run systemd update triggers\n"); return; } + /* These must block so that new update triggers are executed after */ if (need_systemd_reexec) { - ret = system("/usr/bin/systemctl daemon-reexec"); + systemctl_daemon_reexec(); } else { - ret = system("/usr/bin/systemctl daemon-reload"); + systemctl_daemon_reload(); } /* Check for daemons that need to be restarted */ - if (access("/usr/bin/clr-service-restart", F_OK | X_OK) == 0) { - ret = system("/usr/bin/clr-service-restart"); + if (file_is_executable(CLEAR_SERVICE_RESTART_SCRIPT)) { + run_command(CLEAR_SERVICE_RESTART_SCRIPT, NULL); } - string_or_die(&scriptname, "/usr/bin/systemctl"); - string_or_die(&cmd, "/usr/bin/systemctl %s restart update-triggers.target", block_flag); + if (block) { + systemctl_restart("update-triggers.target"); + } else { + systemctl_restart_noblock("update-triggers.target"); + } } else { /* These must block so that new update triggers are executed after */ - if (need_systemd_reexec) { - reexec_flag = "--reexec"; - } else { - reexec_flag = ""; - } - if (strcmp("/", path_prefix) == 0) { - string_or_die(&scriptname, "%s", POST_UPDATE); - string_or_die(&cmd, "%s %s %s", POST_UPDATE, reexec_flag, block_flag); - } else { - string_or_die(&scriptname, "%s%s", path_prefix, POST_UPDATE); - string_or_die(&cmd, "%s/%s %s %s %s", path_prefix, POST_UPDATE, path_prefix, reexec_flag, block_flag); - } + exec_post_update_script(need_systemd_reexec, block); } - run_script(scriptname, cmd); - - free_string(&scriptname); - free_string(&cmd); } -void run_scripts(bool block) +void scripts_run_post_update(bool block) { if (no_scripts) { warn("post-update helper scripts skipped due to " @@ -165,16 +158,13 @@ void run_scripts(bool block) static void exec_pre_update_script(const char *script) { if (strlen(PRE_UPDATE) == 0 || strcmp("/", path_prefix) == 0) { - run_command(script, NULL); + run_script_if_exists(script, NULL); } else { - run_command(script, path_prefix, NULL); + run_script_if_exists(script, path_prefix, NULL); } } -/* Run any "mandatory" pre-update scripts needed. In this case, mandatory - * means the script must run, but it is not yet fatal if the script does not - * return success */ -void run_preupdate_scripts(struct manifest *manifest) +void scripts_run_pre_update(struct manifest *manifest) { struct list *iter = list_tail(manifest->files); struct file *file; diff --git a/src/scripts.h b/src/scripts.h new file mode 100644 index 00000000..5d634f17 --- /dev/null +++ b/src/scripts.h @@ -0,0 +1,32 @@ +#ifndef __SCRIPTS__ +#define __SCRIPTS__ + +#include +#include + +#ifdef __cplusplus +extern "C" { +#endif + +/* + * Run post-update scripts. + */ +void scripts_run_post_update(bool block); + +/* + * Run pre-update scripts. + * + * 'manifests' should point the MoM that includes the pre-update script to be + * run. + * + * Pre update scripts are executed only if they are found and if the hash + * matches. In the case of failures, update will continue. + * + */ +void scripts_run_pre_update(struct manifest *manifest); + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/src/swupd.h b/src/swupd.h index e91ce47f..f1010f01 100644 --- a/src/swupd.h +++ b/src/swupd.h @@ -19,6 +19,7 @@ #include "lib/strings.h" #include "lib/sys.h" #include "manifest.h" +#include "scripts.h" #include "swupd_curl.h" #include "swupd_exit_codes.h" #include "timelist.h" @@ -314,9 +315,6 @@ extern bool is_directory_mounted(const char *filename); extern bool is_under_mounted_directory(const char *filename); extern bool is_populated_dir(char *dirname); -extern void run_scripts(bool block); -extern void run_preupdate_scripts(struct manifest *manifest); - /* filedesc.c */ extern void dump_file_descriptor_leaks(void); extern void record_fds(void); diff --git a/src/update.c b/src/update.c index 28d638d5..5eb74666 100644 --- a/src/update.c +++ b/src/update.c @@ -411,7 +411,7 @@ version_check: /* Step 5: check disk state before attempting update */ timelist_timer_start(global_times, "Run pre-update scripts"); progress_set_step(5, "run_preupdate_scripts"); - run_preupdate_scripts(server_manifest); + scripts_run_pre_update(server_manifest); progress_complete_step(); timelist_timer_stop(global_times); // closing: Run pre-update scripts @@ -472,7 +472,7 @@ version_check: if (on_new_format() && (requested_version == -1 || (requested_version > new_current_version))) { re_update = true; } - run_scripts(re_update); + scripts_run_post_update(re_update); progress_complete_step(); timelist_timer_stop(global_times); // closing: Run post-update scripts diff --git a/src/verify.c b/src/verify.c index 5d3afe9c..83c65c4f 100644 --- a/src/verify.c +++ b/src/verify.c @@ -941,7 +941,7 @@ brick_the_system_and_clean_curl: need_update_boot = true; need_update_bootloader = true; timelist_timer_start(global_times, "Run Scripts"); - run_scripts(false); + scripts_run_post_update(false); timelist_timer_stop(global_times); } diff --git a/test/functional/bundleadd/add-boot-file.bats b/test/functional/bundleadd/add-boot-file.bats index b658f416..fc0fab91 100755 --- a/test/functional/bundleadd/add-boot-file.bats +++ b/test/functional/bundleadd/add-boot-file.bats @@ -32,7 +32,7 @@ test_setup() { .*...100% Calling post-update helper scripts. - Warning: post-update helper script \($TEST_DIRNAME/testfs/target-dir//usr/bin/clr-boot-manager\) not found, it will be skipped + Warning: helper script \($TEST_DIRNAME/testfs/target-dir//usr/bin/clr-boot-manager\) not found, it will be skipped Successfully installed 1 bundle EOM ) diff --git a/test/functional/ignore-list b/test/functional/ignore-list index da094329..db79b6bb 100644 --- a/test/functional/ignore-list +++ b/test/functional/ignore-list @@ -1,4 +1,4 @@ -Warning: post-update helper script .* not found, it will be skipped +Warning: helper script .* not found, it will be skipped Update took .* Compile-time options:.* Compile-time configuration: diff --git a/test/functional/update/update-boot-file.bats b/test/functional/update/update-boot-file.bats index 2fcbe340..8ddcf8a5 100755 --- a/test/functional/update/update-boot-file.bats +++ b/test/functional/update/update-boot-file.bats @@ -32,7 +32,7 @@ test_setup() { Applying update Update was applied. Calling post-update helper scripts. - Warning: post-update helper script ($TEST_DIRNAME/testfs/target-dir//usr/bin/clr-boot-manager) not found, it will be skipped + Warning: helper script ($TEST_DIRNAME/testfs/target-dir//usr/bin/clr-boot-manager) not found, it will be skipped Update successful. System updated from version 10 to version 100 EOM ) diff --git a/test/functional/verify/verify-boot-file.bats b/test/functional/verify/verify-boot-file.bats index e4b143cf..f7cca478 100755 --- a/test/functional/verify/verify-boot-file.bats +++ b/test/functional/verify/verify-boot-file.bats @@ -47,7 +47,7 @@ test_setup() { 1 of 1 files were fixed 0 of 1 files were not fixed Calling post-update helper scripts. - Warning: post-update helper script \\($TEST_DIRNAME/testfs/target-dir//usr/bin/clr-boot-manager\\) not found, it will be skipped + Warning: helper script \\($TEST_DIRNAME/testfs/target-dir//usr/bin/clr-boot-manager\\) not found, it will be skipped Fix successful EOM ) diff --git a/test/functional/verify/verify-json.bats b/test/functional/verify/verify-json.bats index 5ab2a795..64a6906b 100755 --- a/test/functional/verify/verify-json.bats +++ b/test/functional/verify/verify-json.bats @@ -72,7 +72,7 @@ test_setup() { \{ "type" : "info", "msg" : " 2 of 2 missing files were replaced " \}, \{ "type" : "info", "msg" : " 0 of 2 missing files were not replaced " \}, \{ "type" : "info", "msg" : "Calling post-update helper scripts. " \}, - \{ "type" : "warning", "msg" : "post-update helper script \\(.*/usr/bin/clr-boot-manager\\) not found, it will be skipped " \}, + \{ "type" : "warning", "msg" : "helper script \\(.*/usr/bin/clr-boot-manager\\) not found, it will be skipped " \}, \{ "type" : "info", "msg" : "Fix successful " \}, \{ "type" : "end", "section" : "verify", "status" : 0 \} \] @@ -159,7 +159,7 @@ test_setup() { \{ "type" : "info", "msg" : " 3 of 3 files were deleted " \}, \{ "type" : "info", "msg" : " 0 of 3 files were not deleted " \}, \{ "type" : "info", "msg" : "Calling post-update helper scripts. " \}, - \{ "type" : "warning", "msg" : "post-update helper script \\(.*/usr/bin/clr-boot-manager\\) not found, it will be skipped " \}, + \{ "type" : "warning", "msg" : "helper script \\(.*/usr/bin/clr-boot-manager\\) not found, it will be skipped " \}, \{ "type" : "info", "msg" : "Fix successful " \}, \{ "type" : "end", "section" : "verify", "status" : 0 \} \]