i3-dump-log -f: switch from pthreads to UNIX sockets

fixes #4117

RELEASE-NOTES-next

@@ -15,6 +15,8 @@ strongly encouraged to upgrade.
   • i3-nagbar: add option to position on primary monitor
   • alternate focusing tab/stack children-parent containers by clicking on their titlebars
   • i3bar: use first bar config by default
+  • i3-dump-log -f now uses UNIX sockets instead of pthreads. The UNIX socket approach
+    should be more reliable and also more portable.
 
  ┌────────────────────────────┐
  │ Bugfixes                   │

i3-dump-log/main.c

@@ -25,10 +25,10 @@
 #include <sys/mman.h>
 #include <sys/stat.h>
 #include <unistd.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/un.h>
 
-#if !defined(__OpenBSD__)
-static uint32_t offset_next_write;
-#endif
 static uint32_t wrap_count;
 
 static i3_shmlog_header *header;
@@ -36,12 +36,6 @@ static char *logbuffer,
     *walk;
 static int ipcfd = -1;
 
-static volatile bool interrupted = false;
-
-static void sighandler(int signal) {
-    interrupted = true;
-}
-
 static void disable_shmlog(void) {
     const char *disablecmd = "debuglog off; shmlog off";
     if (ipc_send_message(ipcfd, strlen(disablecmd),
@@ -188,22 +182,25 @@ int main(int argc, char *argv[]) {
 
     /* NB: While we must never write, we need O_RDWR for the pthread condvar. */
     int logbuffer_shm = shm_open(shmname, O_RDWR, 0);
-    if (logbuffer_shm == -1)
+    if (logbuffer_shm == -1) {
         err(EXIT_FAILURE, "Could not shm_open SHM segment for the i3 log (%s)", shmname);
+    }
 
-    if (fstat(logbuffer_shm, &statbuf) != 0)
+    if (fstat(logbuffer_shm, &statbuf) != 0) {
         err(EXIT_FAILURE, "stat(%s)", shmname);
+    }
 
-    /* NB: While we must never write, we need PROT_WRITE for the pthread condvar. */
-    logbuffer = mmap(NULL, statbuf.st_size, PROT_READ | PROT_WRITE, MAP_SHARED, logbuffer_shm, 0);
-    if (logbuffer == MAP_FAILED)
+    logbuffer = mmap(NULL, statbuf.st_size, PROT_READ, MAP_SHARED, logbuffer_shm, 0);
+    if (logbuffer == MAP_FAILED) {
         err(EXIT_FAILURE, "Could not mmap SHM segment for the i3 log");
+    }
 
     header = (i3_shmlog_header *)logbuffer;
 
-    if (verbose)
+    if (verbose) {
         printf("next_write = %d, last_wrap = %d, logbuffer_size = %d, shmname = %s\n",
                header->offset_next_write, header->offset_last_wrap, header->size, shmname);
+    }
     free(shmname);
     walk = logbuffer + header->offset_next_write;
 
@@ -233,25 +230,38 @@ int main(int argc, char *argv[]) {
         return 0;
     }
 
-    /* Handle SIGINT gracefully to invoke atexit handlers, if any. */
-    struct sigaction action;
-    action.sa_handler = sighandler;
-    sigemptyset(&action.sa_mask);
-    action.sa_flags = 0;
-    sigaction(SIGINT, &action, NULL);
+    char *log_stream_socket_path = root_atom_contents("I3_LOG_STREAM_SOCKET_PATH", NULL, 0);
+    if (log_stream_socket_path == NULL) {
+        errx(EXIT_FAILURE, "could not determine i3 log stream socket path: possible i3-dump-log and i3 version mismatch");
+    }
 
-    /* Since pthread_cond_wait() expects a mutex, we need to provide one.
-     * To not lock i3 (that’s bad, mhkay?) we just define one outside of
-     * the shared memory. */
-    pthread_mutex_t dummy_mutex = PTHREAD_MUTEX_INITIALIZER;
-    pthread_mutex_lock(&dummy_mutex);
-    while (!interrupted) {
-        pthread_cond_wait(&(header->condvar), &dummy_mutex);
-        /* If this was not a spurious wakeup, print the new lines. */
-        if (header->offset_next_write != offset_next_write) {
-            offset_next_write = header->offset_next_write;
-            print_till_end();
+    int sockfd = socket(AF_LOCAL, SOCK_STREAM, 0);
+    if (sockfd == -1) {
+        err(EXIT_FAILURE, "Could not create socket");
+    }
+
+    (void)fcntl(sockfd, F_SETFD, FD_CLOEXEC);
+
+    struct sockaddr_un addr;
+    memset(&addr, 0, sizeof(struct sockaddr_un));
+    addr.sun_family = AF_LOCAL;
+    strncpy(addr.sun_path, log_stream_socket_path, sizeof(addr.sun_path) - 1);
+    if (connect(sockfd, (const struct sockaddr *)&addr, sizeof(struct sockaddr_un)) < 0) {
+        err(EXIT_FAILURE, "Could not connect to i3 on socket %s", log_stream_socket_path);
+    }
+
+    /* Same size as the buffer used in log.c vlog(): */
+    char buf[4096];
+    for (;;) {
+        const int n = read(sockfd, buf, sizeof(buf));
+        if (n == -1) {
+            err(EXIT_FAILURE, "read(log-stream-socket):");
         }
+        if (n == 0) {
+            exit(0); /* i3 closed the socket */
+        }
+        buf[n] = '\0';
+        swrite(STDOUT_FILENO, buf, n);
     }
 
 #endif

include/i3-atoms_rest.xmacro.h

@@ -15,6 +15,7 @@ xmacro(I3_CONFIG_PATH) \
 xmacro(I3_SYNC) \
 xmacro(I3_SHMLOG_PATH) \
 xmacro(I3_PID) \
+xmacro(I3_LOG_STREAM_SOCKET_PATH) \
 xmacro(I3_FLOATING_WINDOW) \
 xmacro(_NET_REQUEST_FRAME_EXTENTS) \
 xmacro(_NET_FRAME_EXTENTS) \

include/ipc.h

@@ -80,13 +80,6 @@ void ipc_new_client(EV_P_ struct ev_io *w, int revents);
  */
 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.
- *
- */
-int ipc_create_socket(const char *filename);
-
 /**
  * Sends the specified event to all IPC clients which are currently connected
  * and subscribed to this kind of event.

include/log.h

@@ -10,6 +10,7 @@
 #pragma once
 
 #include <config.h>
+#include <ev.h>
 
 /* We will include libi3.h which define its own version of LOG, ELOG.
  * We want *our* version, so we undef the libi3 one. */
@@ -31,6 +32,7 @@
 extern char *errorfilename;
 extern char *shmlogname;
 extern int shmlog_size;
+extern char *current_log_stream_socket_path;
 
 /**
  * Initializes logging by creating an error logfile in /tmp (or
@@ -100,3 +102,5 @@ void verboselog(char *fmt, ...)
  * failures. This function is invoked automatically when exiting.
  */
 void purge_zerobyte_logfile(void);
+
+void log_new_client(EV_P_ struct ev_io *w, int revents);

include/shmlog.h

@@ -12,10 +12,6 @@
 
 #include <config.h>
 
-#if !defined(__OpenBSD__)
-#include <pthread.h>
-#endif
-
 /* Default shmlog size if not set by user. */
 extern const int default_shmlog_size;
 
@@ -39,11 +35,4 @@ typedef struct i3_shmlog_header {
      * coincidentally be exactly the same as previously). Overflows can happen
      * and don’t matter — clients use an equality check (==). */
     uint32_t wrap_count;
-
-#if !defined(__OpenBSD__)
-    /* pthread condvar which will be broadcasted whenever there is a new
-     * message in the log. i3-dump-log uses this to implement -f (follow, like
-     * tail -f) in an efficient way. */
-    pthread_cond_t condvar;
-#endif
 } i3_shmlog_header;

libi3/create_socket.c

@@ -25,20 +25,20 @@
  *
  */
 int create_socket(const char *filename, char **out_socketpath) {
-    int sockfd;
-
     char *resolved = resolve_tilde(filename);
     DLOG("Creating UNIX socket at %s\n", resolved);
     char *copy = sstrdup(resolved);
     const char *dir = dirname(copy);
-    if (!path_exists(dir))
+    if (!path_exists(dir)) {
         mkdirp(dir, DEFAULT_DIR_MODE);
+    }
     free(copy);
 
     /* Unlink the unix domain socket before */
     unlink(resolved);
 
-    if ((sockfd = socket(AF_LOCAL, SOCK_STREAM, 0)) < 0) {
+    int sockfd = socket(AF_LOCAL, SOCK_STREAM, 0);
+    if (sockfd < 0) {
         perror("socket()");
         free(resolved);
         return -1;

libi3/nonblock.c

@@ -15,6 +15,7 @@ void set_nonblock(int sockfd) {
         return;
     }
     flags |= O_NONBLOCK;
-    if (fcntl(sockfd, F_SETFL, flags) < 0)
+    if (fcntl(sockfd, F_SETFL, flags) < 0) {
         err(-1, "Could not set O_NONBLOCK");
+    }
 }

src/commands.c

@@ -1661,6 +1661,9 @@ void cmd_restart(I3_CMD) {
     }
     ipc_shutdown(SHUTDOWN_REASON_RESTART, exempt_fd);
     unlink(config.ipc_socket_path);
+    if (current_log_stream_socket_path != NULL) {
+        unlink(current_log_stream_socket_path);
+    }
     /* We need to call this manually since atexit handlers don’t get called
      * when exec()ing */
     purge_zerobyte_logfile();

src/ipc.c

@@ -1507,15 +1507,6 @@ ipc_client *ipc_new_client_on_fd(EV_P_ int fd) {
     return client;
 }
 
-/*
- * Creates the UNIX domain socket at the given path, sets it to non-blocking
- * mode, bind()s and listen()s on it.
- *
- */
-int ipc_create_socket(const char *filename) {
-    return create_socket(filename, &current_socketpath);
-}
-
 /*
  * Generates a json workspace event. Returns a dynamically allocated yajl
  * generator. Free with yajl_gen_free().

src/log.c

@@ -12,6 +12,10 @@
 #include "all.h"
 #include "shmlog.h"
 
+#include <ev.h>
+#include <libgen.h>
+#include <sys/socket.h>
+#include <sys/un.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <stdarg.h>
@@ -23,9 +27,6 @@
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <unistd.h>
-#if !defined(__OpenBSD__)
-#include <pthread.h>
-#endif
 
 #if defined(__APPLE__)
 #include <sys/sysctl.h>
@@ -60,6 +61,18 @@ static int logbuffer_shm;
 /* Size (in bytes) of physical memory */
 static long long physical_mem_bytes;
 
+typedef struct log_client {
+    int fd;
+
+    TAILQ_ENTRY(log_client)
+    clients;
+} log_client;
+
+TAILQ_HEAD(log_client_head, log_client)
+log_clients = TAILQ_HEAD_INITIALIZER(log_clients);
+
+void log_broadcast_to_clients(const char *message, size_t len);
+
 /*
  * Writes the offsets for the next write and for the last wrap to the
  * shmlog_header.
@@ -161,14 +174,6 @@ void open_logbuffer(void) {
 
     header = (i3_shmlog_header *)logbuffer;
 
-#if !defined(__OpenBSD__)
-    pthread_condattr_t cond_attr;
-    pthread_condattr_init(&cond_attr);
-    if (pthread_condattr_setpshared(&cond_attr, PTHREAD_PROCESS_SHARED) != 0)
-        fprintf(stderr, "pthread_condattr_setpshared() failed, i3-dump-log -f will not work!\n");
-    pthread_cond_init(&(header->condvar), &cond_attr);
-#endif
-
     logwalk = logbuffer + sizeof(i3_shmlog_header);
     loglastwrap = logbuffer + logbuffer_size;
     store_log_markers();
@@ -283,13 +288,10 @@ static void vlog(const bool print, const char *fmt, va_list args) {
 
         store_log_markers();
 
-#if !defined(__OpenBSD__)
-        /* Wake up all (i3-dump-log) processes waiting for condvar. */
-        pthread_cond_broadcast(&(header->condvar));
-#endif
-
         if (print)
             fwrite(message, len, 1, stdout);
+
+        log_broadcast_to_clients(message, len);
     }
 }
 
@@ -370,3 +372,51 @@ void purge_zerobyte_logfile(void) {
         rmdir(errorfilename);
     }
 }
+
+char *current_log_stream_socket_path = NULL;
+
+/*
+ * Handler for activity on the listening socket, meaning that a new client
+ * has just connected and we should accept() them. Sets up the event handler
+ * for activity on the new connection and inserts the file descriptor into
+ * the list of log clients.
+ *
+ */
+void log_new_client(EV_P_ struct ev_io *w, int revents) {
+    struct sockaddr_un peer;
+    socklen_t len = sizeof(struct sockaddr_un);
+    int fd;
+    if ((fd = accept(w->fd, (struct sockaddr *)&peer, &len)) < 0) {
+        if (errno != EINTR) {
+            perror("accept()");
+        }
+        return;
+    }
+
+    /* Close this file descriptor on exec() */
+    (void)fcntl(fd, F_SETFD, FD_CLOEXEC);
+
+    set_nonblock(fd);
+
+    log_client *client = scalloc(1, sizeof(log_client));
+    client->fd = fd;
+    TAILQ_INSERT_TAIL(&log_clients, client, clients);
+
+    DLOG("log: new client connected on fd %d\n", fd);
+}
+
+void log_broadcast_to_clients(const char *message, size_t len) {
+    log_client *current = TAILQ_FIRST(&log_clients);
+    while (current != TAILQ_END(&log_clients)) {
+        /* XXX: In case slow connections turn out to be a problem here
+         * (unlikely as long as i3-dump-log is the only consumer), introduce
+         * buffering, similar to the IPC interface. */
+        ssize_t n = writeall(current->fd, message, len);
+        log_client *previous = current;
+        current = TAILQ_NEXT(current, clients);
+        if (n < 0) {
+            TAILQ_REMOVE(&log_clients, previous, clients);
+            free(previous);
+        }
+    }
+}

src/main.c

@@ -181,6 +181,9 @@ static void i3_exit(void) {
     }
     ipc_shutdown(SHUTDOWN_REASON_EXIT, -1);
     unlink(config.ipc_socket_path);
+    if (current_log_stream_socket_path != NULL) {
+        unlink(current_log_stream_socket_path);
+    }
     xcb_disconnect(conn);
 
     /* If a nagbar is active, kill it */
@@ -845,7 +848,7 @@ int main(int argc, char *argv[]) {
     tree_render();
 
     /* Create the UNIX domain socket for IPC */
-    int ipc_socket = ipc_create_socket(config.ipc_socket_path);
+    int ipc_socket = create_socket(config.ipc_socket_path, &current_socketpath);
     if (ipc_socket == -1) {
         ELOG("Could not create the IPC socket, IPC disabled\n");
     } else {
@@ -854,6 +857,18 @@ int main(int argc, char *argv[]) {
         ev_io_start(main_loop, ipc_io);
     }
 
+    /* Chose a file name in /tmp/ based on the PID */
+    char *log_stream_socket_path = get_process_filename("log-stream-socket");
+    int log_socket = create_socket(log_stream_socket_path, &current_log_stream_socket_path);
+    free(log_stream_socket_path);
+    if (log_socket == -1) {
+        ELOG("Could not create the log socket, i3-dump-log -f will not work\n");
+    } else {
+        struct ev_io *log_io = scalloc(1, sizeof(struct ev_io));
+        ev_io_init(log_io, log_new_client, log_socket, EV_READ);
+        ev_io_start(main_loop, log_io);
+    }
+
     /* Also handle the UNIX domain sockets passed via socket activation. The
      * parameter 1 means "remove the environment variables", we don’t want to
      * pass these to child processes. */

src/util.c

@@ -176,7 +176,6 @@ void exec_i3_utility(char *name, char *argv[]) {
     _exit(2);
 }
 
-
 /*
  * Goes through the list of arguments (for exec()) and add/replace the given option,
  * including the option name, its argument, and the option character.

src/x.c

@@ -1422,6 +1422,8 @@ void x_set_i3_atoms(void) {
     xcb_change_property(conn, XCB_PROP_MODE_REPLACE, root, A_I3_PID, XCB_ATOM_CARDINAL, 32, 1, &pid);
     xcb_change_property(conn, XCB_PROP_MODE_REPLACE, root, A_I3_CONFIG_PATH, A_UTF8_STRING, 8,
                         strlen(current_configpath), current_configpath);
+    xcb_change_property(conn, XCB_PROP_MODE_REPLACE, root, A_I3_LOG_STREAM_SOCKET_PATH, A_UTF8_STRING, 8,
+                        strlen(current_log_stream_socket_path), current_log_stream_socket_path);
     update_shmlog_atom();
 }