From 92fee4ebc4852f023527dc64b28e5b10e4507053 Mon Sep 17 00:00:00 2001 From: wm4 Date: Sun, 16 Feb 2020 21:27:34 +0100 Subject: subprocess: change to a fancier API Introduce mp_subprocess() and related definitions. This is a bit more flexible than the old stuff. This may or may not be used for a more complicated feature that involves starting processes, and which would require more control. Only port subprocess-posix.c to this API. The player still uses the "old" API, so for win32 and dummy implementations, the new API is simply not available, while for POSIX, the old APIs are emulated on top of the new one. I'm hoping the win32 code can be ported as well, so the ifdefs in subprocess.c can be dropped, and the player can (if convenient or needed) use the new API. --- osdep/subprocess-posix.c | 161 ++++++++++++++++++++++++----------------------- osdep/subprocess.c | 59 +++++++++++++++-- osdep/subprocess.h | 48 +++++++++++++- 3 files changed, 185 insertions(+), 83 deletions(-) diff --git a/osdep/subprocess-posix.c b/osdep/subprocess-posix.c index ff78eb42f8..8165f7f641 100644 --- a/osdep/subprocess-posix.c +++ b/osdep/subprocess-posix.c @@ -35,45 +35,34 @@ extern char **environ; #define SAFE_CLOSE(fd) do { if ((fd) >= 0) close((fd)); (fd) = -1; } while (0) -// A silly helper: automatically skips entries with negative FDs -static int sparse_poll(struct pollfd *fds, int num_fds, int timeout) -{ - struct pollfd p_fds[10]; - int map[10]; - if (num_fds > MP_ARRAY_SIZE(p_fds)) - return -1; - int p_num_fds = 0; - for (int n = 0; n < num_fds; n++) { - map[n] = -1; - if (fds[n].fd < 0) - continue; - map[n] = p_num_fds; - p_fds[p_num_fds++] = fds[n]; - } - int r = poll(p_fds, p_num_fds, timeout); - for (int n = 0; n < num_fds; n++) - fds[n].revents = (map[n] < 0 && r >= 0) ? 0 : p_fds[map[n]].revents; - return r; -} - -int mp_subprocess(char **args, struct mp_cancel *cancel, void *ctx, - subprocess_read_cb on_stdout, subprocess_read_cb on_stderr, - char **error) +void mp_subprocess2(struct mp_subprocess_opts *opts, + struct mp_subprocess_result *res) { posix_spawn_file_actions_t fa; bool fa_destroy = false; int status = -1; - int p_stdout[2] = {-1, -1}; - int p_stderr[2] = {-1, -1}; + int comm_pipe[MP_SUBPROCESS_MAX_FDS][2]; int devnull = -1; pid_t pid = -1; bool spawned = false; bool killed_by_us = false; + int cancel_fd = -1; - if (on_stdout && mp_make_cloexec_pipe(p_stdout) < 0) - goto done; - if (on_stderr && mp_make_cloexec_pipe(p_stderr) < 0) - goto done; + *res = (struct mp_subprocess_result){0}; + + for (int n = 0; n < opts->num_fds; n++) + comm_pipe[n][0] = comm_pipe[n][1] = -1; + + if (opts->cancel) { + cancel_fd = mp_cancel_get_fd(opts->cancel); + if (cancel_fd < 0) + goto done; + } + + for (int n = 0; n < opts->num_fds; n++) { + if (opts->fds[n].on_read && mp_make_cloexec_pipe(comm_pipe[n]) < 0) + goto done; + } devnull = open("/dev/null", O_RDONLY | O_CLOEXEC); if (devnull < 0) @@ -82,79 +71,97 @@ int mp_subprocess(char **args, struct mp_cancel *cancel, void *ctx, if (posix_spawn_file_actions_init(&fa)) goto done; fa_destroy = true; - // redirect stdin/stdout/stderr - if (posix_spawn_file_actions_adddup2(&fa, devnull, 0)) - goto done; - if (p_stdout[1] >= 0 && posix_spawn_file_actions_adddup2(&fa, p_stdout[1], 1)) - goto done; - if (p_stderr[1] >= 0 && posix_spawn_file_actions_adddup2(&fa, p_stderr[1], 2)) - goto done; - if (posix_spawnp(&pid, args[0], &fa, NULL, args, environ)) { + // redirect FDs + for (int n = 0; n < opts->num_fds; n++) { + int src_fd = devnull; + if (comm_pipe[n][1] >= 0) + src_fd = comm_pipe[n][1]; + if (opts->fds[n].src_fd >= 0) + src_fd = opts->fds[n].src_fd; + if (posix_spawn_file_actions_adddup2(&fa, src_fd, opts->fds[n].fd)) + goto done; + } + + char **env = opts->env ? opts->env : environ; + if (posix_spawnp(&pid, opts->exe, &fa, NULL, opts->args, env)) { pid = -1; goto done; } spawned = true; - SAFE_CLOSE(p_stdout[1]); - SAFE_CLOSE(p_stderr[1]); + for (int n = 0; n < opts->num_fds; n++) + SAFE_CLOSE(comm_pipe[n][1]); SAFE_CLOSE(devnull); - int *read_fds[2] = {&p_stdout[0], &p_stderr[0]}; - subprocess_read_cb read_cbs[2] = {on_stdout, on_stderr}; - - while (p_stdout[0] >= 0 || p_stderr[0] >= 0) { - struct pollfd fds[] = { - {.events = POLLIN, .fd = *read_fds[0]}, - {.events = POLLIN, .fd = *read_fds[1]}, - {.events = POLLIN, .fd = cancel ? mp_cancel_get_fd(cancel) : -1}, - }; - if (sparse_poll(fds, MP_ARRAY_SIZE(fds), -1) < 0 && errno != EINTR) - break; - for (int n = 0; n < 2; n++) { - if (fds[n].revents) { - char buf[4096]; - ssize_t r = read(*read_fds[n], buf, sizeof(buf)); - if (r < 0 && errno == EINTR) - continue; - if (r > 0 && read_cbs[n]) - read_cbs[n](ctx, buf, r); - if (r <= 0) - SAFE_CLOSE(*read_fds[n]); + while (1) { + struct pollfd fds[MP_SUBPROCESS_MAX_FDS + 1]; + int map_fds[MP_SUBPROCESS_MAX_FDS + 1]; + int num_fds = 0; + for (int n = 0; n < opts->num_fds; n++) { + if (comm_pipe[n][0] >= 0) { + map_fds[num_fds] = n; + fds[num_fds++] = (struct pollfd){ + .events = POLLIN, + .fd = comm_pipe[n][0], + }; } } - if (fds[2].revents) { - kill(pid, SIGKILL); - killed_by_us = true; + if (!num_fds) + break; + if (cancel_fd >= 0) { + map_fds[num_fds] = -1; + fds[num_fds++] = (struct pollfd){.events = POLLIN, .fd = cancel_fd}; + } + + if (poll(fds, num_fds, -1) < 0 && errno != EINTR) break; + + for (int idx = 0; idx < num_fds; idx++) { + if (fds[idx].revents) { + int n = map_fds[idx]; + if (n < 0) { + // cancel_fd + kill(pid, SIGKILL); + killed_by_us = true; + break; + } else { + char buf[4096]; + ssize_t r = read(comm_pipe[n][0], buf, sizeof(buf)); + if (r < 0 && errno == EINTR) + continue; + if (r > 0 && opts->fds[n].on_read) + opts->fds[n].on_read(opts->fds[n].on_read_ctx, buf, r); + if (r <= 0) + SAFE_CLOSE(comm_pipe[n][0]); + } + } } } // Note: it can happen that a child process closes the pipe, but does not // terminate yet. In this case, we would have to run waitpid() in // a separate thread and use pthread_cancel(), or use other weird - // and laborious tricks. So this isn't handled yet. + // and laborious tricks in order to react to mp_cancel. + // So this isn't handled yet. while (waitpid(pid, &status, 0) < 0 && errno == EINTR) {} done: if (fa_destroy) posix_spawn_file_actions_destroy(&fa); - SAFE_CLOSE(p_stdout[0]); - SAFE_CLOSE(p_stdout[1]); - SAFE_CLOSE(p_stderr[0]); - SAFE_CLOSE(p_stderr[1]); + for (int n = 0; n < opts->num_fds; n++) { + SAFE_CLOSE(comm_pipe[n][0]); + SAFE_CLOSE(comm_pipe[n][1]); + } SAFE_CLOSE(devnull); if (!spawned || (WIFEXITED(status) && WEXITSTATUS(status) == 127)) { - *error = "init"; - status = -1; + res->error = MP_SUBPROCESS_EINIT; } else if (WIFEXITED(status)) { - *error = NULL; - status = WEXITSTATUS(status); + res->exit_status = WEXITSTATUS(status); + } else if (killed_by_us) { + res->error = MP_SUBPROCESS_EKILLED_BY_US; } else { - *error = "killed"; - status = killed_by_us ? MP_SUBPROCESS_EKILLED_BY_US : -1; + res->error = MP_SUBPROCESS_EGENERIC; } - - return status; } diff --git a/osdep/subprocess.c b/osdep/subprocess.c index 1e94d23b67..9da5c10c1c 100644 --- a/osdep/subprocess.c +++ b/osdep/subprocess.c @@ -25,6 +25,48 @@ #include "subprocess.h" +void mp_devnull(void *ctx, char *data, size_t size) +{ +} + +#if HAVE_POSIX_SPAWN + +int mp_subprocess(char **args, struct mp_cancel *cancel, void *ctx, + subprocess_read_cb on_stdout, subprocess_read_cb on_stderr, + char **error) +{ + struct mp_subprocess_opts opts = { + .exe = args[0], + .args = args, + .cancel = cancel, + }; + opts.fds[opts.num_fds++] = (struct mp_subprocess_fd){ + .fd = 0, // stdin + .src_fd = 0, + }; + opts.fds[opts.num_fds++] = (struct mp_subprocess_fd){ + .fd = 1, // stdout + .on_read = on_stdout, + .on_read_ctx = ctx, + .src_fd = on_stdout ? -1 : 1, + }; + opts.fds[opts.num_fds++] = (struct mp_subprocess_fd){ + .fd = 2, // stderr + .on_read = on_stderr, + .on_read_ctx = ctx, + .src_fd = on_stderr ? -1 : 2, + }; + struct mp_subprocess_result res; + mp_subprocess2(&opts, &res); + if (res.error < 0) { + *error = (char *)mp_subprocess_err_str(res.error); + return res.error; + } + return res.exit_status; +} + +#endif + struct subprocess_args { struct mp_log *log; char **args; @@ -45,10 +87,6 @@ static void *run_subprocess(void *ptr) return NULL; } -void mp_devnull(void *ctx, char *data, size_t size) -{ -} - void mp_subprocess_detached(struct mp_log *log, char **args) { struct subprocess_args *p = talloc_zero(NULL, struct subprocess_args); @@ -61,3 +99,16 @@ void mp_subprocess_detached(struct mp_log *log, char **args) if (pthread_create(&thread, NULL, run_subprocess, p)) talloc_free(p); } + +const char *mp_subprocess_err_str(int num) +{ + // Note: these are visible to the public client API + switch (num) { + case MP_SUBPROCESS_OK: return "success"; + case MP_SUBPROCESS_EKILLED_BY_US: return "killed"; + case MP_SUBPROCESS_EINIT: return "init"; + case MP_SUBPROCESS_EUNSUPPORTED: return "unsupported"; + case MP_SUBPROCESS_EGENERIC: // fall through + default: return "unknown"; + } +} diff --git a/osdep/subprocess.h b/osdep/subprocess.h index f272e1ad42..6aa2981f1d 100644 --- a/osdep/subprocess.h +++ b/osdep/subprocess.h @@ -18,7 +18,9 @@ #ifndef MP_SUBPROCESS_H_ #define MP_SUBPROCESS_H_ +#include #include +#include struct mp_cancel; @@ -26,12 +28,54 @@ typedef void (*subprocess_read_cb)(void *ctx, char *data, size_t size); void mp_devnull(void *ctx, char *data, size_t size); +#define MP_SUBPROCESS_MAX_FDS 10 + +struct mp_subprocess_fd { + int fd; // target FD + + // Only one of on_read or src_fd can be set. If none are set, use /dev/null. + // Note: "neutral" initialization requires setting src_fd=-1. + subprocess_read_cb on_read; // if not NULL, serve reads + void *on_read_ctx; // for on_read(on_read_ctx, ...) + int src_fd; // if >=0, dup this FD to target FD +}; + +struct mp_subprocess_opts { + char *exe; // binary to execute (never non-NULL) + char **args; // argument list (NULL for none, otherwise NULL-terminated) + char **env; // if !NULL, set this as environment variable block + // Complete set of FDs passed down. All others are supposed to be closed. + struct mp_subprocess_fd fds[MP_SUBPROCESS_MAX_FDS]; + int num_fds; + struct mp_cancel *cancel; // if !NULL, asynchronous process abort (kills it) +}; + +struct mp_subprocess_result { + int error; // one of MP_SUBPROCESS_* (>0 on error) + // NB: if WIFEXITED applies, error==0, and this is WEXITSTATUS + // on win32, this can use the full 32 bit + uint32_t exit_status; // if error==0==MP_SUBPROCESS_OK, 0 otherwise +}; + +// Subprocess error values. +#define MP_SUBPROCESS_OK 0 // no error +#define MP_SUBPROCESS_EGENERIC -1 // unknown error +#define MP_SUBPROCESS_EKILLED_BY_US -2 // mp_cancel was triggered +#define MP_SUBPROCESS_EINIT -3 // error during initialization +#define MP_SUBPROCESS_EUNSUPPORTED -4 // API not supported + +// Turn MP_SUBPROCESS_* values into a static string. Never returns NULL. +const char *mp_subprocess_err_str(int num); + +// Caller must set *opts. +void mp_subprocess2(struct mp_subprocess_opts *opts, + struct mp_subprocess_result *res); + // Start a subprocess. Uses callbacks to read from stdout and stderr. +// Returns any of MP_SUBPROCESS_*, or a value >=0 for the process exir int mp_subprocess(char **args, struct mp_cancel *cancel, void *ctx, subprocess_read_cb on_stdout, subprocess_read_cb on_stderr, char **error); -// mp_subprocess return values. -1 is a generic error code. -#define MP_SUBPROCESS_EKILLED_BY_US -2 struct mp_log; void mp_subprocess_detached(struct mp_log *log, char **args); -- cgit v1.2.3