Merge pull request #108 from mdiluz/bug-and-feature-bash

Assorted small bug fixes

daemon/daemon_config.c

@@ -71,6 +71,7 @@ struct GameModeConfig {
 		char whitelist[CONFIG_LIST_MAX][CONFIG_VALUE_MAX];
 		char blacklist[CONFIG_LIST_MAX][CONFIG_VALUE_MAX];
 
+		long script_timeout;
 		char startscripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX];
 		char endscripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX];
 
@@ -278,6 +279,8 @@ static int inih_handler(void *user, const char *section, const char *name, const
 			valid = append_value_to_list(name, value, self->values.startscripts);
 		} else if (strcmp(name, "end") == 0) {
 			valid = append_value_to_list(name, value, self->values.endscripts);
+		} else if (strcmp(name, "script_timeout") == 0) {
+			valid = get_long_value(name, value, &self->values.script_timeout);
 		}
 	}
 
@@ -330,6 +333,7 @@ static void load_config_files(GameModeConfig *self)
 	self->values.reaper_frequency = DEFAULT_REAPER_FREQ;
 	self->values.gpu_device = -1; /* 0 is a valid device ID so use -1 to indicate no value */
 	self->values.nv_perf_level = -1;
+	self->values.script_timeout = 10; /* Default to 10 seconds for scripts */
 
 	/*
 	 * Locations to load, in order
@@ -506,6 +510,11 @@ void config_get_gamemode_end_scripts(GameModeConfig *self,
 	memcpy_locked_config(self, scripts, self->values.endscripts, sizeof(self->values.startscripts));
 }
 
+/*
+ * Get the script timemout value
+ */
+DEFINE_CONFIG_GET(script_timeout)
+
 /*
  * Get the chosen default governor
  */

daemon/daemon_config.h

@@ -105,6 +105,11 @@ void config_get_gamemode_start_scripts(GameModeConfig *self,
 void config_get_gamemode_end_scripts(GameModeConfig *self,
                                      char scripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX]);
 
+/*
+ * Get the script timout value
+ */
+long config_get_script_timeout(GameModeConfig *self);
+
 /*
  * Get the chosen default governor
  */

daemon/external-helper.c

@@ -37,61 +37,38 @@ POSSIBILITY OF SUCH DAMAGE.
 #include <linux/limits.h>
 #include <stdio.h>
 #include <sys/wait.h>
+#include <time.h>
 #include <unistd.h>
 
+static const int default_timout = 5;
+
 /**
  * Call an external process
  */
-int run_external_process(const char *const *exec_args)
+int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFFER_MAX], int tsec)
 {
 	pid_t p;
 	int status = 0;
+	int pipes[2];
+	char internal[EXTERNAL_BUFFER_MAX] = { 0 };
 
-	if ((p = fork()) < 0) {
-		LOG_ERROR("Failed to fork(): %s\n", strerror(errno));
-		return false;
-	} else if (p == 0) {
-		/* Execute the command */
-		/* Note about cast:
-		 *   The statement about argv[] and envp[] being constants is
-		 *   included to make explicit to future writers of language
-		 *   bindings that these objects are completely constant.
-		 * http://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
-		 */
-		if (execv(exec_args[0], (char *const *)exec_args) != 0) {
-			LOG_ERROR("Failed to execute external process: %s %s\n", exec_args[0], strerror(errno));
-			exit(EXIT_FAILURE);
-		}
-		_exit(EXIT_SUCCESS);
-	}
-
-	if (waitpid(p, &status, 0) < 0) {
-		LOG_ERROR("Failed to waitpid(%d): %s\n", (int)p, strerror(errno));
+	if (pipe(pipes) == -1) {
+		LOG_ERROR("Could not create pipe: %s!\n", strerror(errno));
 		return -1;
 	}
 
-	/* i.e. sigsev */
-	if (!WIFEXITED(status)) {
-		LOG_ERROR("Child process '%s' exited abnormally\n", exec_args[0]);
-	} else if (WEXITSTATUS(status) != 0) {
-		LOG_ERROR("External process failed\n");
-		return -1;
+	/* Set the default timeout */
+	if (tsec == -1) {
+		tsec = default_timout;
 	}
 
-	return 0;
-}
-
-/**
- * Call an external process and get output
- */
-int run_external_process_get_output(const char *const *exec_args, char buffer[EXTERNAL_BUFFER_MAX])
-{
-	pid_t p;
-	int status = 0;
-	int pipes[2];
-
-	if (pipe(pipes) == -1) {
-		LOG_ERROR("Could not create pipe: %s!\n", strerror(errno));
+	/* set up our signaling for the child and the timout */
+	sigset_t mask;
+	sigset_t omask;
+	sigemptyset(&mask);
+	sigaddset(&mask, SIGCHLD);
+	if (sigprocmask(SIG_BLOCK, &mask, &omask) < 0) {
+		LOG_ERROR("sigprocmask failed: %s\n", strerror(errno));
 		return -1;
 	}
 
@@ -103,16 +80,44 @@ int run_external_process_get_output(const char *const *exec_args, char buffer[EX
 		dup2(pipes[1], STDOUT_FILENO);
 		close(pipes[0]);
 		close(pipes[1]);
-		/* Launch the process */
+		/* Execute the command */
+		/* Note about cast:
+		 *   The statement about argv[] and envp[] being constants is
+		 *   included to make explicit to future writers of language
+		 *   bindings that these objects are completely constant.
+		 * http://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
+		 */
 		if (execv(exec_args[0], (char *const *)exec_args) != 0) {
-			LOG_ERROR("Failed to execute external process %s: %s\n", exec_args[0], strerror(errno));
+			LOG_ERROR("Failed to execute external process: %s %s\n", exec_args[0], strerror(errno));
 			exit(EXIT_FAILURE);
 		}
 		_exit(EXIT_SUCCESS);
 	}
 
+	/* Set up the timout */
+	struct timespec timeout;
+	timeout.tv_sec = tsec; /* Magic timeout value of 5s for now - should be sane for most commands
+	                        */
+	timeout.tv_nsec = 0;
+
+	/* Wait for the child to finish up with a timout */
+	while (true) {
+		if (sigtimedwait(&mask, NULL, &timeout) < 0) {
+			if (errno == EINTR) {
+				continue;
+			} else if (errno == EAGAIN) {
+				LOG_ERROR("Child process timed out for %s, killing and returning\n", exec_args[0]);
+				kill(p, SIGKILL);
+			} else {
+				LOG_ERROR("sigtimedwait failed: %s\n", strerror(errno));
+				return -1;
+			}
+		}
+		break;
+	}
+
 	close(pipes[1]);
-	if (read(pipes[0], buffer, EXTERNAL_BUFFER_MAX) < 0) {
+	if (read(pipes[0], internal, EXTERNAL_BUFFER_MAX) < 0) {
 		LOG_ERROR("Failed to read from process %s: %s\n", exec_args[0], strerror(errno));
 		return -1;
 	}
@@ -126,9 +131,13 @@ int run_external_process_get_output(const char *const *exec_args, char buffer[EX
 	if (!WIFEXITED(status)) {
 		LOG_ERROR("Child process '%s' exited abnormally\n", exec_args[0]);
 	} else if (WEXITSTATUS(status) != 0) {
-		LOG_ERROR("External process failed\n");
+		LOG_ERROR("External process failed with exit code %u\n", WEXITSTATUS(status));
+		LOG_ERROR("Output was: %s\n", buffer ? buffer : internal);
 		return -1;
 	}
 
+	if (buffer)
+		memcpy(buffer, internal, EXTERNAL_BUFFER_MAX);
+
 	return 0;
 }

daemon/external-helper.h

@@ -34,7 +34,4 @@ POSSIBILITY OF SUCH DAMAGE.
 #define EXTERNAL_BUFFER_MAX 1024
 
 /* Run an external process and capture the return value */
-int run_external_process(const char *const *exec_args);
-
-/* Run an external process and capture the return value as well as output */
-int run_external_process_get_output(const char *const *exec_args, char buffer[EXTERNAL_BUFFER_MAX]);
+int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFFER_MAX], int tsec);

daemon/gamemode-gpu.c

@@ -229,7 +229,7 @@ int game_mode_apply_gpu(const GameModeGPUInfo *info, bool apply)
 		NULL,
 	};
 
-	if (run_external_process(exec_args) != 0) {
+	if (run_external_process(exec_args, NULL, -1) != 0) {
 		LOG_ERROR("Failed to call gpuclockctl, could not apply optimisations!\n");
 		return -1;
 	}
@@ -262,7 +262,7 @@ int game_mode_get_gpu(GameModeGPUInfo *info)
 	};
 
 	char buffer[EXTERNAL_BUFFER_MAX] = { 0 };
-	if (run_external_process_get_output(exec_args, buffer) != 0) {
+	if (run_external_process(exec_args, buffer, -1) != 0) {
 		LOG_ERROR("Failed to call gpuclockctl, could get values!\n");
 		return -1;
 	}

daemon/gamemode-tests.c

@@ -41,27 +41,45 @@ POSSIBILITY OF SUCH DAMAGE.
 #include <unistd.h>
 
 #include "daemon_config.h"
+#include "external-helper.h"
 #include "gamemode_client.h"
 #include "governors-query.h"
 #include "gpu-control.h"
 
 /* Initial verify step to ensure gamemode isn't already active */
-static int verify_gamemode_initial(void)
+static int verify_gamemode_initial(struct GameModeConfig *config)
 {
 	int status = 0;
 
 	if ((status = gamemode_query_status()) != 0 && status != -1) {
-		LOG_ERROR("gamemode is currently active, tests require gamemode to start deactivated!\n");
-		status = -1;
+		long reaper = config_get_reaper_frequency(config);
+		LOG_MSG("GameMode was active, waiting for the reaper thread (%ld seconds)!\n", reaper);
+		sleep(1);
+
+		/* Try again after waiting */
+		for (int i = 0; i < reaper; i++) {
+			if ((status = gamemode_query_status()) == 0) {
+				status = 0;
+				break;
+			} else if (status == -1) {
+				goto status_error;
+			}
+			LOG_MSG("Waiting...\n");
+			sleep(1);
+		}
+		if (status == 1)
+			LOG_ERROR("GameMode still active, cannot run tests!\n");
 	} else if (status == -1) {
-		LOG_ERROR("gamemode_query_status failed: %s!\n", gamemode_error_string());
-		LOG_ERROR("is gamemode installed correctly?\n");
-		status = -1;
+		goto status_error;
 	} else {
 		status = 0;
 	}
 
 	return status;
+status_error:
+	LOG_ERROR("gamemode_query_status failed: %s!\n", gamemode_error_string());
+	LOG_ERROR("is gamemode installed correctly?\n");
+	return -1;
 }
 
 /* Check if gamemode is active and this client is registered */
@@ -183,8 +201,8 @@ static int run_dual_client_tests(void)
 	/* Parent process */
 	/* None of these should early-out as we need to clean up the child */
 
-	/* Give the child a chance to reqeust gamemode */
-	usleep(1000);
+	/* Give the child a chance to request gamemode */
+	usleep(10000);
 
 	/* Check that when we request gamemode, it replies that the other client is connected */
 	if (verify_other_client_connected() != 0)
@@ -301,12 +319,11 @@ static int run_cpu_governor_tests(struct GameModeConfig *config)
 		strcpy(desiredgov, "performance");
 
 	char defaultgov[CONFIG_VALUE_MAX] = { 0 };
-	config_get_default_governor(config, defaultgov);
 
-	if (desiredgov[0] == '\0') {
+	if (defaultgov[0] == '\0') {
 		const char *currentgov = get_gov_state();
 		if (currentgov) {
-			strncpy(desiredgov, currentgov, CONFIG_VALUE_MAX);
+			strncpy(defaultgov, currentgov, CONFIG_VALUE_MAX);
 		} else {
 			LOG_ERROR(
 			    "Could not get current CPU governor state, this indicates an error! See rest "
@@ -342,6 +359,7 @@ static int run_cpu_governor_tests(struct GameModeConfig *config)
 static int run_custom_scripts_tests(struct GameModeConfig *config)
 {
 	int scriptstatus = 0;
+	long timeout = config_get_script_timeout(config);
 
 	/* Grab and test the start scripts */
 	char startscripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX];
@@ -353,7 +371,8 @@ static int run_custom_scripts_tests(struct GameModeConfig *config)
 		while (*startscripts[i] != '\0' && i < CONFIG_LIST_MAX) {
 			LOG_MSG(":::: Running start script [%s]\n", startscripts[i]);
 
-			int ret = system(startscripts[i]);
+			const char *args[] = { "/bin/sh", "-c", startscripts[i], NULL };
+			int ret = run_external_process(args, NULL, (int)timeout);
 
 			if (ret == 0)
 				LOG_MSG(":::: Passed\n");
@@ -375,7 +394,8 @@ static int run_custom_scripts_tests(struct GameModeConfig *config)
 		while (*endscripts[i] != '\0' && i < CONFIG_LIST_MAX) {
 			LOG_MSG(":::: Running end script [%s]\n", endscripts[i]);
 
-			int ret = system(endscripts[i]);
+			const char *args[] = { "/bin/sh", "-c", endscripts[i], NULL };
+			int ret = run_external_process(args, NULL, (int)timeout);
 
 			if (ret == 0)
 				LOG_MSG(":::: Passed\n");
@@ -668,7 +688,7 @@ int game_mode_run_client_tests()
 	/* First verify that gamemode is not currently active on the system
 	 * As well as it being currently installed and queryable
 	 */
-	if (verify_gamemode_initial() != 0)
+	if (verify_gamemode_initial(config) != 0)
 		return -1;
 
 	/* Controls whether we require a supervisor to actually make requests */

daemon/gamemode.c

@@ -93,6 +93,7 @@ static void *game_mode_context_reaper(void *userdata);
 static void game_mode_context_enter(GameModeContext *self);
 static void game_mode_context_leave(GameModeContext *self);
 static char *game_mode_context_find_exe(pid_t pid);
+static void game_mode_execute_scripts(char scripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX], int timeout);
 
 void game_mode_context_init(GameModeContext *self)
 {
@@ -170,21 +171,6 @@ static void game_mode_context_enter(GameModeContext *self)
 	LOG_MSG("Entering Game Mode...\n");
 	sd_notifyf(0, "STATUS=%sGameMode is now active.%s\n", "\x1B[1;32m", "\x1B[0m");
 
-	char scripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX];
-	memset(scripts, 0, sizeof(scripts));
-	config_get_gamemode_start_scripts(self->config, scripts);
-
-	unsigned int i = 0;
-	while (*scripts[i] != '\0' && i < CONFIG_LIST_MAX) {
-		LOG_MSG("Executing script [%s]\n", scripts[i]);
-		int err;
-		if ((err = system(scripts[i])) != 0) {
-			/* Log the failure, but this is not fatal */
-			LOG_ERROR("Script [%s] failed with error %d\n", scripts[i], err);
-		}
-		i++;
-	}
-
 	/* Read the initial governor state so we can revert it correctly */
 	const char *initial_state = get_gov_state();
 	if (initial_state) {
@@ -203,7 +189,7 @@ static void game_mode_context_enter(GameModeContext *self)
 		};
 
 		LOG_MSG("Requesting update of governor policy to %s\n", desiredGov);
-		if (run_external_process(exec_args) != 0) {
+		if (run_external_process(exec_args, NULL, -1) != 0) {
 			LOG_ERROR("Failed to update cpu governor policy\n");
 			/* if the set fails, clear the initial mode so we don't try and reset it back and fail
 			 * again, presumably */
@@ -217,6 +203,14 @@ static void game_mode_context_enter(GameModeContext *self)
 
 	/* Apply GPU optimisations */
 	game_mode_apply_gpu(self->gpu_info, true);
+
+	/* Run custom scripts last - ensures the above are applied first and these scripts can react to
+	 * them if needed */
+	char scripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX];
+	memset(scripts, 0, sizeof(scripts));
+	config_get_gamemode_start_scripts(self->config, scripts);
+	long timeout = config_get_script_timeout(self->config);
+	game_mode_execute_scripts(scripts, (int)timeout);
 }
 
 /**
@@ -249,7 +243,7 @@ static void game_mode_context_leave(GameModeContext *self)
 		};
 
 		LOG_MSG("Requesting update of governor policy to %s\n", gov_mode);
-		if (run_external_process(exec_args) != 0) {
+		if (run_external_process(exec_args, NULL, -1) != 0) {
 			LOG_ERROR("Failed to update cpu governor policy\n");
 		}
 
@@ -259,17 +253,8 @@ static void game_mode_context_leave(GameModeContext *self)
 	char scripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX];
 	memset(scripts, 0, sizeof(scripts));
 	config_get_gamemode_end_scripts(self->config, scripts);
-
-	unsigned int i = 0;
-	while (*scripts[i] != '\0' && i < CONFIG_LIST_MAX) {
-		LOG_MSG("Executing script [%s]\n", scripts[i]);
-		int err;
-		if ((err = system(scripts[i])) != 0) {
-			/* Log the failure, but this is not fatal */
-			LOG_ERROR("Script [%s] failed with error %d\n", scripts[i], err);
-		}
-		i++;
-	}
+	long timeout = config_get_script_timeout(self->config);
+	game_mode_execute_scripts(scripts, (int)timeout);
 }
 
 /**
@@ -701,3 +686,19 @@ fail:
 		LOG_ERROR("Unable to find executable for PID %d: %s\n", pid, strerror(errno));
 	return NULL;
 }
+
+/* Executes a set of scripts */
+static void game_mode_execute_scripts(char scripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX], int timeout)
+{
+	unsigned int i = 0;
+	while (*scripts[i] != '\0' && i < CONFIG_LIST_MAX) {
+		LOG_MSG("Executing script [%s]\n", scripts[i]);
+		int err;
+		const char *args[] = { "/bin/sh", "-c", scripts[i], NULL };
+		if ((err = run_external_process(args, NULL, timeout)) != 0) {
+			/* Log the failure, but this is not fatal */
+			LOG_ERROR("Script [%s] failed with error %d\n", scripts[i], err);
+		}
+		i++;
+	}
+}

daemon/gpuclockctl.c

@@ -79,7 +79,7 @@ static int get_gpu_state_nv(struct GameModeGPUInfo *info)
 	         NV_CORE_OFFSET_ATTRIBUTE,
 	         info->nv_perf_level);
 	const char *exec_args_core[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL };
-	if (run_external_process_get_output(exec_args_core, buf) != 0) {
+	if (run_external_process(exec_args_core, buf, -1) != 0) {
 		LOG_ERROR("Failed to set %s!\n", arg);
 		return -1;
 	}
@@ -98,7 +98,7 @@ static int get_gpu_state_nv(struct GameModeGPUInfo *info)
 	         NV_MEM_OFFSET_ATTRIBUTE,
 	         info->nv_perf_level);
 	const char *exec_args_mem[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL };
-	if (run_external_process_get_output(exec_args_mem, buf) != 0) {
+	if (run_external_process(exec_args_mem, buf, -1) != 0) {
 		LOG_ERROR("Failed to set %s!\n", arg);
 		return -1;
 	}
@@ -145,7 +145,7 @@ static int set_gpu_state_nv(struct GameModeGPUInfo *info)
 	         info->nv_perf_level,
 	         info->core);
 	const char *exec_args_core[] = { "/usr/bin/nvidia-settings", "-a", core_arg, NULL };
-	if (run_external_process(exec_args_core) != 0) {
+	if (run_external_process(exec_args_core, NULL, -1) != 0) {
 		LOG_ERROR("Failed to set %s!\n", core_arg);
 		return -1;
 	}
@@ -160,7 +160,7 @@ static int set_gpu_state_nv(struct GameModeGPUInfo *info)
 	         info->nv_perf_level,
 	         info->mem);
 	const char *exec_args_mem[] = { "/usr/bin/nvidia-settings", "-a", mem_arg, NULL };
-	if (run_external_process(exec_args_mem) != 0) {
+	if (run_external_process(exec_args_mem, NULL, -1) != 0) {
 		LOG_ERROR("Failed to set %s!\n", mem_arg);
 		return -1;
 	}

lib/client_impl.c

@@ -47,6 +47,8 @@ static int gamemode_request(const char *function, int arg)
 {
 	sd_bus_message *msg = NULL;
 	sd_bus *bus = NULL;
+	sd_bus_error err;
+	memset(&err, 0, sizeof(err));
 
 	int result = -1;
 
@@ -64,7 +66,7 @@ static int gamemode_request(const char *function, int arg)
 		                         "/com/feralinteractive/GameMode",
 		                         "com.feralinteractive.GameMode",
 		                         function,
-		                         NULL,
+		                         &err,
 		                         &msg,
 		                         arg ? "ii" : "i",
 		                         getpid(),
@@ -72,7 +74,13 @@ static int gamemode_request(const char *function, int arg)
 		if (ret < 0) {
 			snprintf(error_string,
 			         sizeof(error_string),
-			         "Could not call method on bus: %s",
+			         "Could not call method %s on com.feralinteractive.GameMode\n"
+			         "\t%s\n"
+			         "\t%s\n"
+			         "\t%s\n",
+			         function,
+			         err.name,
+			         err.message,
 			         strerror(-ret));
 		} else {
 			// Read the reply