spawn(): improve performance and simplify API

posix_spawn() is designed especially for this purpose, and thus it's
much more lightweight and efficient than manually fork/dup/exec-ing.

on my system, it improves the performance of spawn() by about 10x. given
that we make frequent calls to potentially multiple scripts, the
increased efficiency will add up overtime.

using posix_spawn() also simplifies the logic quite a bit, despite the
very verbose function names. however it does make cleanup a bit more
complicated.

this patch uses the linux kernel style cleanup strategy [0] (which I'm
personally not a huge fan of, but it fits this situation quite nicely)
with a "stack-like" unwinding via `goto`-s.

additionally simplify the spawn() API by taking in {read,write}fd
pointers and returning the pid instead of using some custom struct.

this coincidently also fixes #299

[0]: https://www.kernel.org/doc/html/v4.10/process/coding-style.html?highlight=goto#centralized-exiting-of-functions

3 files changed, 67 insertions, 79 deletions

diff --git a/main.c b/main.c
index 8ee5b9d..3312503 100644
--- a/main.c
+++ b/main.c
@@ -277,9 +277,9 @@ static bool check_timeouts(int *t)
 static size_t get_win_title(char *buf, size_t len)
 {
 	char *argv[8];
-	spawn_t pfd;
 	char w[12] = "", h[12] = "", z[12] = "", fidx[12], fcnt[12];
 	ssize_t n = -1;
+	int readfd;
 
 	if (wintitle.f.err || buf == NULL || len == 0)
 		return 0;
@@ -293,11 +293,10 @@ static size_t get_win_title(char *buf, size_t len)
 	snprintf(fcnt, ARRLEN(fcnt), "%d", filecnt);
 	construct_argv(argv, ARRLEN(argv), wintitle.f.cmd, files[fileidx].path,
 	               fidx, fcnt, w, h, z, NULL);
-	pfd = spawn(wintitle.f.cmd, argv, X_READ);
-	if (pfd.readfd >= 0) {
-		if ((n = read(pfd.readfd, buf, len-1)) > 0)
+	if (spawn(&readfd, NULL, argv) > 0) {
+		if ((n = read(readfd, buf, len-1)) > 0)
 			buf[n] = '\0';
-		close(pfd.readfd);
+		close(readfd);
 	}
 
 	return MAX(0, n);
@@ -314,9 +313,7 @@ void close_info(void)
 
 void open_info(void)
 {
-	spawn_t pfd;
-	char w[12] = "", h[12] = "";
-	char *argv[6];
+	char *argv[6], w[12] = "", h[12] = "";
 	char *cmd = mode == MODE_IMAGE ? info.f.cmd : info.ft.cmd;
 	bool ferr = mode == MODE_IMAGE ? info.f.err : info.ft.err;
 
@@ -329,12 +326,8 @@ void open_info(void)
 	}
 	construct_argv(argv, ARRLEN(argv), cmd, files[fileidx].name, w, h,
 	               files[fileidx].path, NULL);
-	pfd = spawn(cmd, argv, X_READ);
-	if (pfd.readfd >= 0) {
-		fcntl(pfd.readfd, F_SETFL, O_NONBLOCK);
-		info.fd = pfd.readfd;
-		info.pid = pfd.pid;
-	}
+	if ((info.pid = spawn(&info.fd, NULL, argv)) > 0)
+		fcntl(info.fd, F_SETFL, O_NONBLOCK);
 }
 
 static void read_info(void)
@@ -579,13 +572,13 @@ static bool run_key_handler(const char *key, unsigned int mask)
 	FILE *pfs;
 	bool marked = mode == MODE_THUMB && markcnt > 0;
 	bool changed = false;
-	int f, i;
+	pid_t pid;
+	int writefd, f, i;
 	int fcnt = marked ? markcnt : 1;
 	char kstr[32];
 	struct stat *oldst, st;
 	XEvent dump;
 	char *argv[3];
-	spawn_t pfd;
 
 	if (keyhandler.f.err) {
 		if (!keyhandler.warned) {
@@ -608,12 +601,11 @@ static bool run_key_handler(const char *key, unsigned int mask)
 	         mask & Mod1Mask    ? "M-" : "",
 	         mask & ShiftMask   ? "S-" : "", key);
 	construct_argv(argv, ARRLEN(argv), keyhandler.f.cmd, kstr, NULL);
-	pfd = spawn(keyhandler.f.cmd, argv, X_WRITE);
-	if (pfd.writefd < 0)
+	if ((pid = spawn(NULL, &writefd, argv)) < 0)
 		return false;
-	if ((pfs = fdopen(pfd.writefd, "w")) == NULL) {
+	if ((pfs = fdopen(writefd, "w")) == NULL) {
 		error(0, errno, "open pipe");
-		close(pfd.writefd);
+		close(writefd);
 		return false;
 	}
 
@@ -626,7 +618,7 @@ static bool run_key_handler(const char *key, unsigned int mask)
 		}
 	}
 	fclose(pfs);
-	while (waitpid(pfd.pid, NULL, 0) == -1 && errno == EINTR);
+	while (waitpid(pid, NULL, 0) == -1 && errno == EINTR);
 
 	for (f = i = 0; f < fcnt; i++) {
 		if ((marked && (files[i].flags & FF_MARK)) || (!marked && i == fileidx)) {
diff --git a/nsxiv.h b/nsxiv.h
index 33a2bde..19e5399 100644
--- a/nsxiv.h
+++ b/nsxiv.h
@@ -326,17 +326,6 @@ typedef struct {
 	int stlen;
 } r_dir_t;
 
-typedef struct {
-	int readfd;
-	int writefd;
-	pid_t pid;
-} spawn_t;
-
-enum {
-	X_READ  = (1 << 0),
-	X_WRITE = (1 << 1)
-};
-
 extern const char *progname;
 
 void* emalloc(size_t);
@@ -349,7 +338,7 @@ int r_closedir(r_dir_t*);
 char* r_readdir(r_dir_t*, bool);
 int r_mkdir(char*);
 void construct_argv(char**, unsigned int, ...);
-spawn_t spawn(const char*, char *const [], unsigned int);
+pid_t spawn(int*, int*, char *const []);
 
 
 /* window.c */
diff --git a/util.c b/util.c
index 9172e85..043c5bd 100644
--- a/util.c
+++ b/util.c
@@ -21,6 +21,7 @@
 
 #include <assert.h>
 #include <errno.h>
+#include <spawn.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -28,6 +29,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+extern char **environ;
 const char *progname = "nsxiv";
 
 void* emalloc(size_t size)
@@ -228,60 +230,65 @@ void construct_argv(char **argv, unsigned int len, ...)
 		error(EXIT_FAILURE, 0, "argv not NULL terminated");
 }
 
-spawn_t spawn(const char *cmd, char *const argv[], unsigned int flags)
+static int mkspawn_pipe(posix_spawn_file_actions_t *fa, const char *cmd, int *pfd, int dupidx)
 {
-	pid_t pid;
-	spawn_t status = { -1, -1, -1 };
-	int pfd_read[2] = { -1, -1 }, pfd_write[2] = { -1, -1 };
-	const bool r = flags & X_READ;
-	const bool w = flags & X_WRITE;
-
-	if (cmd == NULL || argv == NULL || flags == 0)
-		return status;
-
-	if (r && pipe(pfd_read) < 0) {
+	int err;
+	if (pipe(pfd) < 0) {
 		error(0, errno, "pipe: %s", cmd);
-		return status;
+		return -1;
 	}
+	err = posix_spawn_file_actions_adddup2(fa, pfd[dupidx], dupidx);
+	err = err ? err : posix_spawn_file_actions_addclose(fa, pfd[0]);
+	err = err ? err : posix_spawn_file_actions_addclose(fa, pfd[1]);
+	if (err) {
+		error(0, err, "posix_spawn_file_actions: %s", cmd);
+		close(pfd[0]);
+		close(pfd[1]);
+	}
+	return err ? -1 : 0;
+}
 
-	if (w && pipe(pfd_write) < 0) {
-		error(0, errno, "pipe: %s", cmd);
-		if (r) {
-			close(pfd_read[0]);
-			close(pfd_read[1]);
-		}
-		return status;
+pid_t spawn(int *readfd, int *writefd, char *const argv[])
+{
+	pid_t pid = -1;
+	const char *cmd;
+	int err, pfd_read[2], pfd_write[2];
+	posix_spawn_file_actions_t fa;
+
+	assert(argv != NULL && argv[0] != NULL);
+	cmd = argv[0];
+
+	if ((err = posix_spawn_file_actions_init(&fa)) != 0) {
+		error(0, err, "spawn: %s", cmd);
+		return pid;
 	}
 
-	if ((pid = fork()) == 0) { /* in child */
-		if ((r && dup2(pfd_read[1], 1) < 0) || (w && dup2(pfd_write[0], 0) < 0))
-			error(EXIT_FAILURE, errno, "dup2: %s", cmd);
+	if (readfd != NULL && mkspawn_pipe(&fa, cmd, pfd_read, 1) < 0)
+		goto err_destroy_fa;
+	if (writefd != NULL && mkspawn_pipe(&fa, cmd, pfd_write, 0) < 0)
+		goto err_close_readfd;
+
+	if ((err = posix_spawn(&pid, cmd, &fa, NULL, argv, environ)) != 0) {
+		error(0, err, "spawn: %s", cmd);
+	} else {
+		if (readfd != NULL)
+			*readfd = pfd_read[0];
+		if (writefd != NULL)
+			*writefd = pfd_write[1];
+	}
 
-		if (r) {
-			close(pfd_read[0]);
-			close(pfd_read[1]);
-		}
-		if (w) {
-			close(pfd_write[0]);
-			close(pfd_write[1]);
-		}
-		execv(cmd, argv);
-		error(EXIT_FAILURE, errno, "exec: %s", cmd);
-	} else if (pid < 0) { /* fork failed */
-		error(0, errno, "fork: %s", cmd);
-		if (r)
-			close(pfd_read[0]);
-		if (w)
+	if (writefd != NULL) {
+		close(pfd_write[0]);
+		if (pid < 0)
 			close(pfd_write[1]);
-	} else { /* in parent */
-		status.pid = pid;
-		status.readfd = pfd_read[0];
-		status.writefd = pfd_write[1];
 	}
-
-	if (r)
+err_close_readfd:
+	if (readfd != NULL) {
+		if (pid < 0)
+			close(pfd_read[0]);
 		close(pfd_read[1]);
-	if (w)
-		close(pfd_write[0]);
-	return status;
+	}
+err_destroy_fa:
+	posix_spawn_file_actions_destroy(&fa);
+	return pid;
 }