Merge pull request #4106 from orestisfl/nagbar

Improve handling of nagbar processes

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

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

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);
+
+    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 */

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

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();
 
@@ -336,30 +336,19 @@ 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;
-}
-
-/*
- * 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);
+    pid_t *nagbar_pid = watcher->data;
+    if (*nagbar_pid == watcher->pid) {
+        /* Only reset if the watched nagbar is the active nagbar */
+        *nagbar_pid = -1;
     }
 }
 
@@ -395,27 +384,20 @@ 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);
 }
 
 /*
- * 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 +407,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 <defunct> process. Therefore we
      * waitpid() here. */
-    waitpid(*nagbar_pid, NULL, 0);
+    waitpid(nagbar_pid, NULL, 0);
 }
 
 /*