From 6e087d4a205ff4a28655d25823a9c1a8bf9aa8d7 Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Sun, 24 May 2020 12:43:56 +0200 Subject: [PATCH 1/3] kill_nagbar: No need for pointer to pid_t --- include/util.h | 4 ++-- src/commands.c | 4 ++-- src/util.c | 14 +++++++------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/include/util.h b/include/util.h index f0db03a5..09ad941f 100644 --- a/include/util.h +++ b/include/util.h @@ -143,13 +143,13 @@ char *pango_escape_markup(char *input); void start_nagbar(pid_t *nagbar_pid, char *argv[]); /** - * Kills the i3-nagbar process, if *nagbar_pid != -1. + * Kills the i3-nagbar process, if nagbar_pid != -1. * * If wait_for_it is set (restarting i3), this function will waitpid(), * otherwise, ev is assumed to handle it (reloading). * */ -void kill_nagbar(pid_t *nagbar_pid, bool wait_for_it); +void kill_nagbar(pid_t nagbar_pid, bool wait_for_it); /** * Converts a string into a long using strtol(). diff --git a/src/commands.c b/src/commands.c index e6f6ef6a..8310c811 100644 --- a/src/commands.c +++ b/src/commands.c @@ -1615,8 +1615,8 @@ void cmd_exit(I3_CMD) { */ void cmd_reload(I3_CMD) { LOG("reloading\n"); - kill_nagbar(&config_error_nagbar_pid, false); - kill_nagbar(&command_error_nagbar_pid, false); + kill_nagbar(config_error_nagbar_pid, false); + kill_nagbar(command_error_nagbar_pid, false); load_configuration(NULL, C_RELOAD); x_set_i3_atoms(); /* Send an IPC event just in case the ws names have changed */ diff --git a/src/util.c b/src/util.c index 6d8d2ee7..2ca5b330 100644 --- a/src/util.c +++ b/src/util.c @@ -287,8 +287,8 @@ static char *store_restart_layout(void) { void i3_restart(bool forget_layout) { char *restart_filename = forget_layout ? NULL : store_restart_layout(); - kill_nagbar(&config_error_nagbar_pid, true); - kill_nagbar(&command_error_nagbar_pid, true); + kill_nagbar(config_error_nagbar_pid, true); + kill_nagbar(command_error_nagbar_pid, true); restore_geometry(); @@ -405,17 +405,17 @@ void start_nagbar(pid_t *nagbar_pid, char *argv[]) { } /* - * Kills the i3-nagbar process, if *nagbar_pid != -1. + * Kills the i3-nagbar process, if nagbar_pid != -1. * * If wait_for_it is set (restarting i3), this function will waitpid(), * otherwise, ev is assumed to handle it (reloading). * */ -void kill_nagbar(pid_t *nagbar_pid, bool wait_for_it) { - if (*nagbar_pid == -1) +void kill_nagbar(pid_t nagbar_pid, bool wait_for_it) { + if (nagbar_pid == -1) return; - if (kill(*nagbar_pid, SIGTERM) == -1) + if (kill(nagbar_pid, SIGTERM) == -1) warn("kill(configerror_nagbar) failed"); if (!wait_for_it) @@ -425,7 +425,7 @@ void kill_nagbar(pid_t *nagbar_pid, bool wait_for_it) { * exec(), our old pid is no longer watched. So, ev won’t handle SIGCHLD * for us and we would end up with a process. Therefore we * waitpid() here. */ - waitpid(*nagbar_pid, NULL, 0); + waitpid(nagbar_pid, NULL, 0); } /* From 78595f1f68a49e49a1ac3a427e040ab9b35fd598 Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Sun, 24 May 2020 10:33:02 +0200 Subject: [PATCH 2/3] Improve handling of nagbar processes Other changes in nagbar_exited: - Remove ERROR from ELOG as it shows up as "ERROR: ERROR:" - Make sure to reset the PID even when the process did not exit normally. - Use ELOG when exitcode != 0 - Remove the not found error: if nagbar is not in $PATH, exec_i3_utility prints an error as well. For the record, I also implemented a more complicated approach with a new watcher data structure: https://github.com/orestisfl/i3/commit/bd3aaf3a333a677433ac046901205086ab58a15c While writing the commit message, it occurred to me to compare watcher->pid with *watcher->data, which fixes the problems mentioned in that patch. Fixes #4104 --- RELEASE-NOTES-next | 1 + src/commands.c | 8 ++++++++ src/util.c | 20 +++++++++++--------- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/RELEASE-NOTES-next b/RELEASE-NOTES-next index d5db5322..78984c8a 100644 --- a/RELEASE-NOTES-next +++ b/RELEASE-NOTES-next @@ -42,3 +42,4 @@ working. Please reach out to us in that case! • build: correctly provide auxiliary functions when needed • build: fix issues with parallel build • set _NET_DESKTOP_VIEWPORT after randr changes + • fix a bug with i3-nagbar not starting after it has already started once diff --git a/src/commands.c b/src/commands.c index 8310c811..3f8c2695 100644 --- a/src/commands.c +++ b/src/commands.c @@ -1615,8 +1615,16 @@ void cmd_exit(I3_CMD) { */ void cmd_reload(I3_CMD) { LOG("reloading\n"); + kill_nagbar(config_error_nagbar_pid, false); kill_nagbar(command_error_nagbar_pid, false); + /* start_nagbar() will refuse to start a new process if the passed pid is + * set. This will happen when our child watcher is triggered by libev when + * the loop re-starts. However, config errors might be detected before + * that since we will read the config right now with load_configuration. + * See #4104. */ + config_error_nagbar_pid = command_error_nagbar_pid = -1; + load_configuration(NULL, C_RELOAD); x_set_i3_atoms(); /* Send an IPC event just in case the ws names have changed */ diff --git a/src/util.c b/src/util.c index 2ca5b330..641ce3ce 100644 --- a/src/util.c +++ b/src/util.c @@ -336,18 +336,20 @@ char *pango_escape_markup(char *input) { static void nagbar_exited(EV_P_ ev_child *watcher, int revents) { ev_child_stop(EV_A_ watcher); - if (!WIFEXITED(watcher->rstatus)) { - ELOG("ERROR: i3-nagbar did not exit normally.\n"); - return; - } - int exitcode = WEXITSTATUS(watcher->rstatus); - DLOG("i3-nagbar process exited with status %d\n", exitcode); - if (exitcode == 2) { - ELOG("ERROR: i3-nagbar could not be found. Is it correctly installed on your system?\n"); + if (!WIFEXITED(watcher->rstatus)) { + ELOG("i3-nagbar (%d) did not exit normally. This is not an error if the config was reloaded while a nagbar was active.\n", watcher->pid); + } else if (exitcode != 0) { + ELOG("i3-nagbar (%d) process exited with status %d\n", watcher->pid, exitcode); + } else { + DLOG("i3-nagbar (%d) process exited with status %d\n", watcher->pid, exitcode); } - *((pid_t *)watcher->data) = -1; + pid_t *nagbar_pid = watcher->data; + if (*nagbar_pid == watcher->pid) { + /* Only reset if the watched nagbar is the active nagbar */ + *nagbar_pid = -1; + } } /* From 00ffa68a6c9c6849714f7387f75a63de02ce9311 Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Sun, 24 May 2020 12:57:35 +0200 Subject: [PATCH 3/3] Move nagbar cleanup to i3_exit Otherwise, each time we start a nagbar, a cleanup handler is created. However, each of these handlers tries to kill the same process (->data is a pointer to config_error_nagbar_pid / command_error_nagbar_pid). With this commit, both potential nagbar processes are killed once. --- src/main.c | 4 ++++ src/util.c | 20 -------------------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/main.c b/src/main.c index d9b10ecb..83b3e181 100644 --- a/src/main.c +++ b/src/main.c @@ -182,6 +182,10 @@ static void i3_exit(void) { unlink(config.ipc_socket_path); xcb_disconnect(conn); + /* If a nagbar is active, kill it */ + kill_nagbar(config_error_nagbar_pid, false); + kill_nagbar(command_error_nagbar_pid, false); + /* We need ev >= 4 for the following code. Since it is not *that* important (it * only makes sure that there are no i3-nagbar instances left behind) we still * support old systems with libev 3. */ diff --git a/src/util.c b/src/util.c index 641ce3ce..cf7f41c8 100644 --- a/src/util.c +++ b/src/util.c @@ -352,19 +352,6 @@ static void nagbar_exited(EV_P_ ev_child *watcher, int revents) { } } -/* - * Cleanup handler. Will be called when i3 exits. Kills i3-nagbar with signal - * SIGKILL (9) to make sure there are no left-over i3-nagbar processes. - * - */ -static void nagbar_cleanup(EV_P_ ev_cleanup *watcher, int revent) { - pid_t *nagbar_pid = (pid_t *)watcher->data; - if (*nagbar_pid != -1) { - LOG("Sending SIGKILL (%d) to i3-nagbar with PID %d\n", SIGKILL, *nagbar_pid); - kill(*nagbar_pid, SIGKILL); - } -} - /* * Starts an i3-nagbar instance with the given parameters. Takes care of * handling SIGCHLD and killing i3-nagbar when i3 exits. @@ -397,13 +384,6 @@ void start_nagbar(pid_t *nagbar_pid, char *argv[]) { ev_child_init(child, &nagbar_exited, *nagbar_pid, 0); child->data = nagbar_pid; ev_child_start(main_loop, child); - - /* install a cleanup watcher (will be called when i3 exits and i3-nagbar is - * still running) */ - ev_cleanup *cleanup = smalloc(sizeof(ev_cleanup)); - ev_cleanup_init(cleanup, nagbar_cleanup); - cleanup->data = nagbar_pid; - ev_cleanup_start(main_loop, cleanup); } /*