From 49d11f0d1fae43ae1e5f61204ed051e889062767 Mon Sep 17 00:00:00 2001 From: NRK Date: Wed, 22 Jun 2022 09:28:07 +0600 Subject: [PATCH] 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 --- main.c | 34 +++++++---------- nsxiv.h | 13 +------ util.c | 111 ++++++++++++++++++++++++++++++-------------------------- 3 files changed, 73 insertions(+), 85 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 #include +#include #include #include #include @@ -28,6 +29,7 @@ #include #include +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; } - - if (w && pipe(pfd_write) < 0) { - error(0, errno, "pipe: %s", cmd); - if (r) { - close(pfd_read[0]); - close(pfd_read[1]); - } - return status; + 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]); } - - 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 (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) - close(pfd_write[1]); - } else { /* in parent */ - status.pid = pid; - status.readfd = pfd_read[0]; - status.writefd = pfd_write[1]; - } - - if (r) - close(pfd_read[1]); - if (w) - close(pfd_write[0]); - return status; + return err ? -1 : 0; +} + +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 (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 (writefd != NULL) { + close(pfd_write[0]); + if (pid < 0) + close(pfd_write[1]); + } +err_close_readfd: + if (readfd != NULL) { + if (pid < 0) + close(pfd_read[0]); + close(pfd_read[1]); + } +err_destroy_fa: + posix_spawn_file_actions_destroy(&fa); + return pid; }