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 <otavio.pontes@intel.com>

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();
 

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) {

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.
  */

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");
+}

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

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;

src/scripts.h

@@ -0,0 +1,32 @@
+#ifndef __SCRIPTS__
+#define __SCRIPTS__
+
+#include <stdbool.h>
+#include <stdint.h>
+
+#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

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);

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
 

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);
 	}
 

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
 	)

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:

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
 	)

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
 	)

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 \}
 		\]