From e44c445262c4b1e00a342e0f5c8bd289c10aa207 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 18:07:56 +0000 Subject: [PATCH 01/12] Use the actual dbus error when failing in the client This helps greatly when identifying what went wrong --- lib/client_impl.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/client_impl.c b/lib/client_impl.c index 4c23278..1627596 100644 --- a/lib/client_impl.c +++ b/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 From 9df1dd857c37225a9c86fe868156959043a53d5c Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 18:18:04 +0000 Subject: [PATCH 02/12] Try waiting for the reaper thread at the start of tests if needed --- daemon/gamemode-tests.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 08fdd04..5ebe394 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -46,22 +46,39 @@ POSSIBILITY OF SUCH DAMAGE. #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 */ @@ -668,7 +685,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 */ From 024acddf90dc51686edb77582c0551482185372e Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 18:28:54 +0000 Subject: [PATCH 03/12] Run the custom start scripts to after the other optimisations This ensures the other featues are applied first, and the scripts can react if needed to those settings --- daemon/gamemode.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 6f206a2..c45b025 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -170,21 +170,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) { @@ -217,6 +202,23 @@ 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); + + 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++; + } } /** From 5facf2bba57177ece28e9ca1005b3a5bff2a5702 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 18:33:00 +0000 Subject: [PATCH 04/12] Refactor out script execution --- daemon/gamemode.c | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index c45b025..076388b 100644 --- a/daemon/gamemode.c +++ b/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]); void game_mode_context_init(GameModeContext *self) { @@ -208,17 +209,7 @@ static void game_mode_context_enter(GameModeContext *self) 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++; - } + game_mode_execute_scripts(scripts); } /** @@ -261,17 +252,7 @@ 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++; - } + game_mode_execute_scripts(scripts); } /** @@ -703,3 +684,18 @@ 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]) +{ + 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++; + } +} From 1665447350b36c026fffca250fe0298eac23518f Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 18:37:53 +0000 Subject: [PATCH 05/12] Use run_external_process for the script execution This protects the main process against script exection and allows more detailed error handling --- daemon/gamemode-tests.c | 7 +++++-- daemon/gamemode.c | 3 ++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 5ebe394..2df541e 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -41,6 +41,7 @@ POSSIBILITY OF SUCH DAMAGE. #include #include "daemon_config.h" +#include "external-helper.h" #include "gamemode_client.h" #include "governors-query.h" #include "gpu-control.h" @@ -370,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); if (ret == 0) LOG_MSG(":::: Passed\n"); @@ -392,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); if (ret == 0) LOG_MSG(":::: Passed\n"); diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 076388b..f445374 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -692,7 +692,8 @@ static void game_mode_execute_scripts(char scripts[CONFIG_LIST_MAX][CONFIG_VALUE while (*scripts[i] != '\0' && i < CONFIG_LIST_MAX) { LOG_MSG("Executing script [%s]\n", scripts[i]); int err; - if ((err = system(scripts[i])) != 0) { + const char *args[] = { "/bin/sh", "-c", scripts[i], NULL }; + if ((err = run_external_process(args)) != 0) { /* Log the failure, but this is not fatal */ LOG_ERROR("Script [%s] failed with error %d\n", scripts[i], err); } From e9ff2cbb100523b3ca2d83174b13d91cb4895465 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 18:53:51 +0000 Subject: [PATCH 06/12] Implement a timeout in run_external_process --- daemon/external-helper.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/daemon/external-helper.c b/daemon/external-helper.c index ca19e96..18335d8 100644 --- a/daemon/external-helper.c +++ b/daemon/external-helper.c @@ -37,6 +37,7 @@ POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include #include /** @@ -47,6 +48,16 @@ int run_external_process(const char *const *exec_args) pid_t p; int status = 0; + /* 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; + } + if ((p = fork()) < 0) { LOG_ERROR("Failed to fork(): %s\n", strerror(errno)); return false; @@ -65,6 +76,27 @@ int run_external_process(const char *const *exec_args) _exit(EXIT_SUCCESS); } + /* Set up the timout */ + struct timespec timeout; + timeout.tv_sec = 5; /* 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; + } + if (waitpid(p, &status, 0) < 0) { LOG_ERROR("Failed to waitpid(%d): %s\n", (int)p, strerror(errno)); return -1; From 4578af47baa7ebdc9de06896288b209fd63427e2 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 20:03:59 +0000 Subject: [PATCH 07/12] Combine the two run_external_process functions so they both have the same timeout protection --- daemon/external-helper.c | 60 ++++++++-------------------------------- daemon/external-helper.h | 5 +--- daemon/gamemode-gpu.c | 4 +-- daemon/gamemode-tests.c | 4 +-- daemon/gamemode.c | 6 ++-- daemon/gpuclockctl.c | 8 +++--- 6 files changed, 24 insertions(+), 63 deletions(-) diff --git a/daemon/external-helper.c b/daemon/external-helper.c index 18335d8..fcdcaa4 100644 --- a/daemon/external-helper.c +++ b/daemon/external-helper.c @@ -43,10 +43,16 @@ POSSIBILITY OF SUCH DAMAGE. /** * 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]) { pid_t p; int status = 0; + int pipes[2]; + + if (pipe(pipes) == -1) { + LOG_ERROR("Could not create pipe: %s!\n", strerror(errno)); + return -1; + } /* set up our signaling for the child and the timout */ sigset_t mask; @@ -62,6 +68,10 @@ int run_external_process(const char *const *exec_args) LOG_ERROR("Failed to fork(): %s\n", strerror(errno)); return false; } else if (p == 0) { + /* Send STDOUT to the pipe */ + dup2(pipes[1], STDOUT_FILENO); + close(pipes[0]); + close(pipes[1]); /* Execute the command */ /* Note about cast: * The statement about argv[] and envp[] being constants is @@ -97,54 +107,8 @@ int run_external_process(const char *const *exec_args) break; } - if (waitpid(p, &status, 0) < 0) { - LOG_ERROR("Failed to waitpid(%d): %s\n", (int)p, 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; - } - - 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)); - return -1; - } - - if ((p = fork()) < 0) { - LOG_ERROR("Failed to fork(): %s\n", strerror(errno)); - return false; - } else if (p == 0) { - /* Send STDOUT to the pipe */ - dup2(pipes[1], STDOUT_FILENO); - close(pipes[0]); - close(pipes[1]); - /* Launch the process */ - 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); - } - close(pipes[1]); - if (read(pipes[0], buffer, EXTERNAL_BUFFER_MAX) < 0) { + if (buffer && read(pipes[0], buffer, EXTERNAL_BUFFER_MAX) < 0) { LOG_ERROR("Failed to read from process %s: %s\n", exec_args[0], strerror(errno)); return -1; } diff --git a/daemon/external-helper.h b/daemon/external-helper.h index 0febe20..73d0a01 100644 --- a/daemon/external-helper.h +++ b/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]); diff --git a/daemon/gamemode-gpu.c b/daemon/gamemode-gpu.c index 2ae3374..9c5de33 100644 --- a/daemon/gamemode-gpu.c +++ b/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) != 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) != 0) { LOG_ERROR("Failed to call gpuclockctl, could get values!\n"); return -1; } diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 2df541e..a65cbb2 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -372,7 +372,7 @@ static int run_custom_scripts_tests(struct GameModeConfig *config) LOG_MSG(":::: Running start script [%s]\n", startscripts[i]); const char *args[] = { "/bin/sh", "-c", startscripts[i], NULL }; - int ret = run_external_process(args); + int ret = run_external_process(args, NULL); if (ret == 0) LOG_MSG(":::: Passed\n"); @@ -395,7 +395,7 @@ static int run_custom_scripts_tests(struct GameModeConfig *config) LOG_MSG(":::: Running end script [%s]\n", endscripts[i]); const char *args[] = { "/bin/sh", "-c", endscripts[i], NULL }; - int ret = run_external_process(args); + int ret = run_external_process(args, NULL); if (ret == 0) LOG_MSG(":::: Passed\n"); diff --git a/daemon/gamemode.c b/daemon/gamemode.c index f445374..78fee5d 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -189,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) != 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 */ @@ -242,7 +242,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) != 0) { LOG_ERROR("Failed to update cpu governor policy\n"); } @@ -693,7 +693,7 @@ static void game_mode_execute_scripts(char scripts[CONFIG_LIST_MAX][CONFIG_VALUE 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)) != 0) { + if ((err = run_external_process(args, NULL)) != 0) { /* Log the failure, but this is not fatal */ LOG_ERROR("Script [%s] failed with error %d\n", scripts[i], err); } diff --git a/daemon/gpuclockctl.c b/daemon/gpuclockctl.c index 28963f4..26e1d0a 100644 --- a/daemon/gpuclockctl.c +++ b/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) != 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) != 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) != 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) != 0) { LOG_ERROR("Failed to set %s!\n", mem_arg); return -1; } From 53d1700a680a02576ada3b3f7d3cfd913d5a98d0 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 20:27:41 +0000 Subject: [PATCH 08/12] Add the timout to the call signature of run_external_process --- daemon/external-helper.c | 12 ++++++++++-- daemon/external-helper.h | 2 +- daemon/gamemode-gpu.c | 4 ++-- daemon/gamemode-tests.c | 4 ++-- daemon/gamemode.c | 6 +++--- daemon/gpuclockctl.c | 8 ++++---- 6 files changed, 22 insertions(+), 14 deletions(-) diff --git a/daemon/external-helper.c b/daemon/external-helper.c index fcdcaa4..5bbc990 100644 --- a/daemon/external-helper.c +++ b/daemon/external-helper.c @@ -40,10 +40,12 @@ POSSIBILITY OF SUCH DAMAGE. #include #include +static const int default_timout = 5; + /** * Call an external process */ -int run_external_process(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) { pid_t p; int status = 0; @@ -54,6 +56,11 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF return -1; } + /* Set the default timeout */ + if (tsec == -1) { + tsec = default_timout; + } + /* set up our signaling for the child and the timout */ sigset_t mask; sigset_t omask; @@ -88,7 +95,8 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF /* Set up the timout */ struct timespec timeout; - timeout.tv_sec = 5; /* Magic timeout value of 5s for now - should be sane for most commands */ + 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 */ diff --git a/daemon/external-helper.h b/daemon/external-helper.h index 73d0a01..9d4e2e4 100644 --- a/daemon/external-helper.h +++ b/daemon/external-helper.h @@ -34,4 +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, char buffer[EXTERNAL_BUFFER_MAX]); +int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFFER_MAX], int tsec); diff --git a/daemon/gamemode-gpu.c b/daemon/gamemode-gpu.c index 9c5de33..0040b9c 100644 --- a/daemon/gamemode-gpu.c +++ b/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, NULL) != 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(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; } diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index a65cbb2..9761f78 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -372,7 +372,7 @@ static int run_custom_scripts_tests(struct GameModeConfig *config) LOG_MSG(":::: Running start script [%s]\n", startscripts[i]); const char *args[] = { "/bin/sh", "-c", startscripts[i], NULL }; - int ret = run_external_process(args, NULL); + int ret = run_external_process(args, NULL, 10); if (ret == 0) LOG_MSG(":::: Passed\n"); @@ -395,7 +395,7 @@ static int run_custom_scripts_tests(struct GameModeConfig *config) LOG_MSG(":::: Running end script [%s]\n", endscripts[i]); const char *args[] = { "/bin/sh", "-c", endscripts[i], NULL }; - int ret = run_external_process(args, NULL); + int ret = run_external_process(args, NULL, 10); if (ret == 0) LOG_MSG(":::: Passed\n"); diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 78fee5d..575a4c0 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -189,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, NULL) != 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 */ @@ -242,7 +242,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, NULL) != 0) { + if (run_external_process(exec_args, NULL, -1) != 0) { LOG_ERROR("Failed to update cpu governor policy\n"); } @@ -693,7 +693,7 @@ static void game_mode_execute_scripts(char scripts[CONFIG_LIST_MAX][CONFIG_VALUE 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)) != 0) { + if ((err = run_external_process(args, NULL, 10)) != 0) { /* Log the failure, but this is not fatal */ LOG_ERROR("Script [%s] failed with error %d\n", scripts[i], err); } diff --git a/daemon/gpuclockctl.c b/daemon/gpuclockctl.c index 26e1d0a..001c07d 100644 --- a/daemon/gpuclockctl.c +++ b/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(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(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, NULL) != 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, NULL) != 0) { + if (run_external_process(exec_args_mem, NULL, -1) != 0) { LOG_ERROR("Failed to set %s!\n", mem_arg); return -1; } From c215626ccd75096c1e4314d094e9bafa0f939cfc Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 20:47:31 +0000 Subject: [PATCH 09/12] Add "script_timeout" config value to control if a user wants to extend the script timeout before kill value --- daemon/daemon_config.c | 9 +++++++++ daemon/daemon_config.h | 5 +++++ daemon/gamemode-tests.c | 5 +++-- daemon/gamemode.c | 12 +++++++----- 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/daemon/daemon_config.c b/daemon/daemon_config.c index 1183433..96db7a7 100644 --- a/daemon/daemon_config.c +++ b/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 */ diff --git a/daemon/daemon_config.h b/daemon/daemon_config.h index 2981e8d..d52da9b 100644 --- a/daemon/daemon_config.h +++ b/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 */ diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 9761f78..4603585 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -360,6 +360,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]; @@ -372,7 +373,7 @@ static int run_custom_scripts_tests(struct GameModeConfig *config) LOG_MSG(":::: Running start script [%s]\n", startscripts[i]); const char *args[] = { "/bin/sh", "-c", startscripts[i], NULL }; - int ret = run_external_process(args, NULL, 10); + int ret = run_external_process(args, NULL, (int)timeout); if (ret == 0) LOG_MSG(":::: Passed\n"); @@ -395,7 +396,7 @@ static int run_custom_scripts_tests(struct GameModeConfig *config) LOG_MSG(":::: Running end script [%s]\n", endscripts[i]); const char *args[] = { "/bin/sh", "-c", endscripts[i], NULL }; - int ret = run_external_process(args, NULL, 10); + int ret = run_external_process(args, NULL, (int)timeout); if (ret == 0) LOG_MSG(":::: Passed\n"); diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 575a4c0..1bce846 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -93,7 +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]); +static void game_mode_execute_scripts(char scripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX], int timeout); void game_mode_context_init(GameModeContext *self) { @@ -209,7 +209,8 @@ static void game_mode_context_enter(GameModeContext *self) char scripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX]; memset(scripts, 0, sizeof(scripts)); config_get_gamemode_start_scripts(self->config, scripts); - game_mode_execute_scripts(scripts); + long timeout = config_get_script_timeout(self->config); + game_mode_execute_scripts(scripts, (int)timeout); } /** @@ -252,7 +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); - game_mode_execute_scripts(scripts); + long timeout = config_get_script_timeout(self->config); + game_mode_execute_scripts(scripts, (int)timeout); } /** @@ -686,14 +688,14 @@ fail: } /* Executes a set of scripts */ -static void game_mode_execute_scripts(char scripts[CONFIG_LIST_MAX][CONFIG_VALUE_MAX]) +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, 10)) != 0) { + 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); } From b6da948ca2d1dcc999bae7fb042266058b282fc8 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 21 Feb 2019 20:50:04 +0000 Subject: [PATCH 10/12] Add the output to the log for external processes that have failed --- daemon/external-helper.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/daemon/external-helper.c b/daemon/external-helper.c index 5bbc990..6cb01fd 100644 --- a/daemon/external-helper.c +++ b/daemon/external-helper.c @@ -50,6 +50,7 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF pid_t p; int status = 0; int pipes[2]; + char internal[EXTERNAL_BUFFER_MAX] = { 0 }; if (pipe(pipes) == -1) { LOG_ERROR("Could not create pipe: %s!\n", strerror(errno)); @@ -116,7 +117,7 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF } close(pipes[1]); - if (buffer && 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; } @@ -130,9 +131,13 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF 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; } From 0efd65fc788f78d15e933fca13c2355990c261d8 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Fri, 8 Mar 2019 11:55:41 +0000 Subject: [PATCH 11/12] set the default gov not the desired gov in tests --- daemon/gamemode-tests.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 4603585..2c2541a 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -319,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 " From 6a240d550e308a549b2ebc4be4e813429fc98aa6 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Thu, 7 Mar 2019 20:09:14 +0000 Subject: [PATCH 12/12] Extend the sleep for the child gamemode in tests --- daemon/gamemode-tests.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 2c2541a..2dd6c10 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -201,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)