Make `restart` IPC command send a reply once restart completed (!) (#3743)

This is achieved by retaining the IPC connection which is sending the restart
command across the restart.

This is the cleaner fix for https://github.com/i3/go-i3/issues/3

fixes #3565

docs/ipc

@@ -111,6 +111,12 @@ The following reply types are implemented:
 
 COMMAND (0)::
 	Confirmation/Error code for the RUN_COMMAND message.
++
+Note that when sending the `restart` command, you will get a reply once
+the restart completed. All IPC connection state (e.g. subscriptions)
+will reset, and libraries must be able to cope with it. One way of
+achieving that is to close the connection, if the library already
+supports transparent reconnects.
 WORKSPACES (1)::
 	Reply to the GET_WORKSPACES message.
 SUBSCRIBE (2)::

include/commands_parser.h

@@ -22,6 +22,10 @@ struct CommandResultIR {
     /* The JSON generator to append a reply to (may be NULL). */
     yajl_gen json_gen;
 
+    /* The IPC client connection which sent this command (may be NULL, e.g. for
+       key bindings). */
+    ipc_client *client;
+
     /* The next state to transition to. Passed to the function so that we can
      * determine the next state as a result of a function call, like
      * cfg_criteria_pop_state() does. */
@@ -61,7 +65,7 @@ char *parse_string(const char **walk, bool as_word);
  *
  * Free the returned CommandResult with command_result_free().
  */
-CommandResult *parse_command(const char *input, yajl_gen gen);
+CommandResult *parse_command(const char *input, yajl_gen gen, ipc_client *client);
 
 /**
  * Frees a CommandResult

include/ipc.h

@@ -72,6 +72,16 @@ typedef void (*handler_t)(ipc_client *, uint8_t *, int, uint32_t, uint32_t);
  */
 void ipc_new_client(EV_P_ struct ev_io *w, int revents);
 
+/**
+ * ipc_new_client_on_fd() only sets up the event handler
+ * for activity on the new connection and inserts the file descriptor into
+ * the list of clients.
+ *
+ * This variant is useful for the inherited IPC connection when restarting.
+ *
+ */
+ipc_client *ipc_new_client_on_fd(EV_P_ int fd);
+
 /**
  * Creates the UNIX domain socket at the given path, sets it to non-blocking
  * mode, bind()s and listen()s on it.
@@ -95,10 +105,13 @@ typedef enum {
 } shutdown_reason_t;
 
 /**
- * Calls shutdown() on each socket and closes it.
+ * Calls shutdown() on each socket and closes it. This function is to be called
+ * when exiting or restarting only!
+ *
+ * exempt_fd is never closed. Set to -1 to close all fds.
  *
  */
-void ipc_shutdown(shutdown_reason_t reason);
+void ipc_shutdown(shutdown_reason_t reason, int exempt_fd);
 
 void dump_node(yajl_gen gen, Con *con, bool inplace_restart);
 
@@ -136,3 +149,8 @@ void ipc_send_binding_event(const char *event_type, Binding *bind);
   * socket.
   */
 void ipc_set_kill_timeout(ev_tstamp new);
+
+/**
+ * Sends a restart reply to the IPC client on the specified fd.
+ */
+void ipc_confirm_restart(ipc_client *client);

src/assignments.c

@@ -48,7 +48,7 @@ void run_assignments(i3Window *window) {
         DLOG("matching assignment, execute command %s\n", current->dest.command);
         char *full_command;
         sasprintf(&full_command, "[id=\"%d\"] %s", window->id, current->dest.command);
-        CommandResult *result = parse_command(full_command, NULL);
+        CommandResult *result = parse_command(full_command, NULL, NULL);
         free(full_command);
 
         if (result->needs_tree_render)

src/bindings.c

@@ -824,7 +824,7 @@ CommandResult *run_binding(Binding *bind, Con *con) {
         sasprintf(&command, "[con_id=\"%p\"] %s", con, bind->command);
 
     Binding *bind_cp = binding_copy(bind);
-    CommandResult *result = parse_command(command, NULL);
+    CommandResult *result = parse_command(command, NULL, NULL);
     free(command);
 
     if (result->needs_tree_render)

src/commands.c

@@ -12,6 +12,8 @@
 #include <stdint.h>
 #include <float.h>
 #include <stdarg.h>
+#include <unistd.h>
+#include <fcntl.h>
 
 #include "shmlog.h"
 
@@ -1590,15 +1592,27 @@ void cmd_reload(I3_CMD) {
  */
 void cmd_restart(I3_CMD) {
     LOG("restarting i3\n");
-    ipc_shutdown(SHUTDOWN_REASON_RESTART);
+    int exempt_fd = -1;
+    if (cmd_output->client != NULL) {
+        exempt_fd = cmd_output->client->fd;
+        LOG("Carrying file descriptor %d across restart\n", exempt_fd);
+        int flags;
+        if ((flags = fcntl(exempt_fd, F_GETFD)) < 0 ||
+            fcntl(exempt_fd, F_SETFD, flags & ~FD_CLOEXEC) < 0) {
+            ELOG("Could not disable FD_CLOEXEC on fd %d\n", exempt_fd);
+        }
+        char *fdstr = NULL;
+        sasprintf(&fdstr, "%d", exempt_fd);
+        setenv("_I3_RESTART_FD", fdstr, 1);
+    }
+    ipc_shutdown(SHUTDOWN_REASON_RESTART, exempt_fd);
     unlink(config.ipc_socket_path);
     /* We need to call this manually since atexit handlers don’t get called
      * when exec()ing */
     purge_zerobyte_logfile();
     i3_restart(false);
-
+    /* unreached */
-    // XXX: default reply for now, make this a better reply
+    assert(false);
-    ysuccess(true);
 }
 
 /*

src/commands_parser.c

@@ -181,6 +181,7 @@ static struct CommandResultIR command_output;
 static void next_state(const cmdp_token *token) {
     if (token->next_state == __CALL) {
         subcommand_output.json_gen = command_output.json_gen;
+        subcommand_output.client = command_output.client;
         subcommand_output.needs_tree_render = false;
         GENERATED_call(token->extra.call_identifier, &subcommand_output);
         state = subcommand_output.next_state;
@@ -261,11 +262,13 @@ char *parse_string(const char **walk, bool as_word) {
  *
  * Free the returned CommandResult with command_result_free().
  */
-CommandResult *parse_command(const char *input, yajl_gen gen) {
+CommandResult *parse_command(const char *input, yajl_gen gen, ipc_client *client) {
     DLOG("COMMAND: *%s*\n", input);
     state = INITIAL;
     CommandResult *result = scalloc(1, sizeof(CommandResult));
 
+    command_output.client = client;
+
     /* A YAJL JSON generator used for formatting replies. */
     command_output.json_gen = gen;
 
@@ -499,7 +502,7 @@ int main(int argc, char *argv[]) {
     }
     yajl_gen gen = yajl_gen_alloc(NULL);
 
-    CommandResult *result = parse_command(argv[1], gen);
+    CommandResult *result = parse_command(argv[1], gen, NULL);
 
     command_result_free(result);
 

src/ipc.c

@@ -33,6 +33,9 @@ all_clients = TAILQ_HEAD_INITIALIZER(all_clients);
  */
 static void set_nonblock(int sockfd) {
     int flags = fcntl(sockfd, F_GETFL, 0);
+    if (flags & O_NONBLOCK) {
+        return;
+    }
     flags |= O_NONBLOCK;
     if (fcntl(sockfd, F_SETFL, flags) < 0)
         err(-1, "Could not set O_NONBLOCK");
@@ -125,9 +128,11 @@ static void ipc_send_client_message(ipc_client *client, size_t size, const uint3
     }
 }
 
-static void free_ipc_client(ipc_client *client) {
+static void free_ipc_client(ipc_client *client, int exempt_fd) {
+    if (client->fd != exempt_fd) {
         DLOG("Disconnecting client on fd %d\n", client->fd);
         close(client->fd);
+    }
 
     ev_io_stop(main_loop, client->read_callback);
     FREE(client->read_callback);
@@ -195,15 +200,19 @@ static void ipc_send_shutdown_event(shutdown_reason_t reason) {
  * Calls shutdown() on each socket and closes it. This function is to be called
  * when exiting or restarting only!
  *
+ * exempt_fd is never closed. Set to -1 to close all fds.
+ *
  */
-void ipc_shutdown(shutdown_reason_t reason) {
+void ipc_shutdown(shutdown_reason_t reason, int exempt_fd) {
     ipc_send_shutdown_event(reason);
 
     ipc_client *current;
     while (!TAILQ_EMPTY(&all_clients)) {
         current = TAILQ_FIRST(&all_clients);
+        if (current->fd != exempt_fd) {
             shutdown(current->fd, SHUT_RDWR);
-        free_ipc_client(current);
+        }
+        free_ipc_client(current, exempt_fd);
     }
 }
 
@@ -219,7 +228,7 @@ IPC_HANDLER(run_command) {
     LOG("IPC: received: *%s*\n", command);
     yajl_gen gen = yajl_gen_alloc(NULL);
 
-    CommandResult *result = parse_command(command, gen);
+    CommandResult *result = parse_command(command, gen, client);
     free(command);
 
     if (result->needs_tree_render)
@@ -1339,7 +1348,7 @@ static void ipc_receive_message(EV_P_ struct ev_io *w, int revents) {
 
         /* If not, there was some kind of error. We don’t bother and close the
          * connection. Delete the client from the list of clients. */
-        free_ipc_client(client);
+        free_ipc_client(client, -1);
         FREE(message);
         return;
     }
@@ -1397,7 +1406,7 @@ end:
         ELOG("client %p on fd %d timed out, killing\n", client, client->fd);
     }
 
-    free_ipc_client(client);
+    free_ipc_client(client, -1);
 }
 
 static void ipc_socket_writeable_cb(EV_P_ ev_io *w, int revents) {
@@ -1431,6 +1440,18 @@ void ipc_new_client(EV_P_ struct ev_io *w, int revents) {
     /* Close this file descriptor on exec() */
     (void)fcntl(fd, F_SETFD, FD_CLOEXEC);
 
+    ipc_new_client_on_fd(EV_A_ fd);
+}
+
+/*
+ * ipc_new_client_on_fd() only sets up the event handler
+ * for activity on the new connection and inserts the file descriptor into
+ * the list of clients.
+ *
+ * This variant is useful for the inherited IPC connection when restarting.
+ *
+ */
+ipc_client *ipc_new_client_on_fd(EV_P_ int fd) {
     set_nonblock(fd);
 
     ipc_client *client = scalloc(1, sizeof(ipc_client));
@@ -1445,8 +1466,9 @@ void ipc_new_client(EV_P_ struct ev_io *w, int revents) {
     client->write_callback->data = client;
     ev_io_init(client->write_callback, ipc_socket_writeable_cb, fd, EV_WRITE);
 
-    DLOG("IPC: new client connected on fd %d\n", w->fd);
+    DLOG("IPC: new client connected on fd %d\n", fd);
     TAILQ_INSERT_TAIL(&all_clients, client, clients);
+    return client;
 }
 
 /*
@@ -1627,3 +1649,15 @@ void ipc_send_binding_event(const char *event_type, Binding *bind) {
     y(free);
     setlocale(LC_NUMERIC, "");
 }
+
+/*
+ * Sends a restart reply to the IPC client on the specified fd.
+ */
+void ipc_confirm_restart(ipc_client *client) {
+    DLOG("ipc_confirm_restart(fd %d)\n", client->fd);
+    static const char *reply = "{\"success\":true}";
+    ipc_send_client_message(
+        client, strlen(reply), I3_IPC_REPLY_TYPE_COMMAND,
+        (const uint8_t *)reply);
+    ipc_push_pending(client);
+}

src/main.c

@@ -166,7 +166,7 @@ static void i3_exit(void) {
         fflush(stderr);
         shm_unlink(shmlogname);
     }
-    ipc_shutdown(SHUTDOWN_REASON_EXIT);
+    ipc_shutdown(SHUTDOWN_REASON_EXIT, -1);
     unlink(config.ipc_socket_path);
     xcb_disconnect(conn);
 
@@ -236,6 +236,20 @@ static void setup_term_handlers(void) {
     }
 }
 
+static int parse_restart_fd(void) {
+    const char *restart_fd = getenv("_I3_RESTART_FD");
+    if (restart_fd == NULL) {
+        return -1;
+    }
+
+    long int fd = -1;
+    if (!parse_long(restart_fd, &fd, 10)) {
+        ELOG("Malformed _I3_RESTART_FD \"%s\"\n", restart_fd);
+        return -1;
+    }
+    return fd;
+}
+
 int main(int argc, char *argv[]) {
     /* Keep a symbol pointing to the I3_VERSION string constant so that we have
      * it in gdb backtraces. */
@@ -847,6 +861,15 @@ int main(int argc, char *argv[]) {
         }
     }
 
+    {
+        const int restart_fd = parse_restart_fd();
+        if (restart_fd != -1) {
+            DLOG("serving restart fd %d", restart_fd);
+            ipc_client *client = ipc_new_client_on_fd(main_loop, restart_fd);
+            ipc_confirm_restart(client);
+        }
+    }
+
     /* Set up i3 specific atoms like I3_SOCKET_PATH and I3_CONFIG_PATH */
     x_set_i3_atoms();
     ewmh_update_workarea();

src/util.c

@@ -287,7 +287,7 @@ void i3_restart(bool forget_layout) {
 
     restore_geometry();
 
-    ipc_shutdown(SHUTDOWN_REASON_RESTART);
+    ipc_shutdown(SHUTDOWN_REASON_RESTART, -1);
 
     LOG("restarting \"%s\"...\n", start_argv[0]);
     /* make sure -a is in the argument list or add it */
@@ -465,7 +465,7 @@ void kill_nagbar(pid_t *nagbar_pid, bool wait_for_it) {
  * if the number could be parsed.
  */
 bool parse_long(const char *str, long *out, int base) {
-    char *end;
+    char *end = NULL;
     long result = strtol(str, &end, base);
     if (result == LONG_MIN || result == LONG_MAX || result < 0 || (end != NULL && *end != '\0')) {
         *out = result;