From dcd6079c9bb5e06716a53b90f8539715bb194eeb Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Fri, 15 Jan 2021 16:12:55 +0100 Subject: [PATCH] i3-dump-log -f: switch from pthreads to UNIX sockets fixes #4117 --- RELEASE-NOTES-next | 2 + i3-dump-log/main.c | 76 +++++++++++++++++-------------- include/i3-atoms_rest.xmacro.h | 1 + include/ipc.h | 7 --- include/log.h | 4 ++ include/shmlog.h | 11 ----- libi3/create_socket.c | 8 ++-- libi3/nonblock.c | 3 +- src/commands.c | 3 ++ src/ipc.c | 9 ---- src/log.c | 82 +++++++++++++++++++++++++++------- src/main.c | 17 ++++++- src/util.c | 1 - src/x.c | 2 + 14 files changed, 143 insertions(+), 83 deletions(-) diff --git a/RELEASE-NOTES-next b/RELEASE-NOTES-next index 8f5abd9d1..cd7a4ddb5 100644 --- a/RELEASE-NOTES-next +++ b/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 │ diff --git a/i3-dump-log/main.c b/i3-dump-log/main.c index e58b0c379..0ce222647 100644 --- a/i3-dump-log/main.c +++ b/i3-dump-log/main.c @@ -25,10 +25,10 @@ #include #include #include +#include +#include +#include -#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); - - /* 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(); + 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"); + } + + 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 diff --git a/include/i3-atoms_rest.xmacro.h b/include/i3-atoms_rest.xmacro.h index 75a5f2300..288a4211b 100644 --- a/include/i3-atoms_rest.xmacro.h +++ b/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) \ diff --git a/include/ipc.h b/include/ipc.h index 0d19daf4d..732fa9b4e 100644 --- a/include/ipc.h +++ b/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. diff --git a/include/log.h b/include/log.h index c2758443a..3a52c64d8 100644 --- a/include/log.h +++ b/include/log.h @@ -10,6 +10,7 @@ #pragma once #include +#include /* 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); diff --git a/include/shmlog.h b/include/shmlog.h index 7b7e133a6..a30852e76 100644 --- a/include/shmlog.h +++ b/include/shmlog.h @@ -12,10 +12,6 @@ #include -#if !defined(__OpenBSD__) -#include -#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; diff --git a/libi3/create_socket.c b/libi3/create_socket.c index 1de7422a3..4b93ff2da 100644 --- a/libi3/create_socket.c +++ b/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; diff --git a/libi3/nonblock.c b/libi3/nonblock.c index 77347e32e..987c274a2 100644 --- a/libi3/nonblock.c +++ b/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"); + } } diff --git a/src/commands.c b/src/commands.c index 8e8aece9e..cd9fb6fd1 100644 --- a/src/commands.c +++ b/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(); diff --git a/src/ipc.c b/src/ipc.c index 862e748e1..d69ecb6ec 100644 --- a/src/ipc.c +++ b/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, ¤t_socketpath); -} - /* * Generates a json workspace event. Returns a dynamically allocated yajl * generator. Free with yajl_gen_free(). diff --git a/src/log.c b/src/log.c index 326f82b8c..183bb8e63 100644 --- a/src/log.c +++ b/src/log.c @@ -12,6 +12,10 @@ #include "all.h" #include "shmlog.h" +#include +#include +#include +#include #include #include #include @@ -23,9 +27,6 @@ #include #include #include -#if !defined(__OpenBSD__) -#include -#endif #if defined(__APPLE__) #include @@ -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); + } + } +} diff --git a/src/main.c b/src/main.c index 220195597..54b38f449 100644 --- a/src/main.c +++ b/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, ¤t_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, ¤t_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. */ diff --git a/src/util.c b/src/util.c index cba7319cf..f2c562228 100644 --- a/src/util.c +++ b/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. diff --git a/src/x.c b/src/x.c index ad9a3ff38..b89d02c2b 100644 --- a/src/x.c +++ b/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(); }