From: Alexander Turenko <alexander.turenko@tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com> Cc: tarantool-patches@dev.tarantool.org Subject: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds Date: Tue, 14 Apr 2020 14:38:21 +0300 [thread overview] Message-ID: <6873a6070a181c7c47f2e94295539c483fd6e7b7.1586862436.git.alexander.turenko@tarantool.org> (raw) In-Reply-To: <cover.1586862436.git.alexander.turenko@tarantool.org> The function popen_shutdown() checks whether std{in,out,err} was piped and closes the parent's end. A user should have ability to send EOF for child's stdin for stream programs like `grep`. It is better when there is a function that encapsulates proper checks, error messages and the actual actions. This commit in particular reverts 1ef95b99f6553b246729e7bb5bdc19038043db74 ('popen: remove redundant fd check before perform IO'), because now the check is meaningful: an fd may become closed before the whole popen handle will be deleted. Part of #4031 --- src/lib/core/popen.c | 170 ++++++++++++++++++++++++++++++++----------- src/lib/core/popen.h | 3 + 2 files changed, 130 insertions(+), 43 deletions(-) diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c index 640dffc2b..8760429c2 100644 --- a/src/lib/core/popen.c +++ b/src/lib/core/popen.c @@ -34,6 +34,43 @@ static RLIST_HEAD(popen_head); static int dev_null_fd_ro = -1; static int dev_null_fd_wr = -1; +static const struct { + unsigned int mask; + unsigned int mask_devnull; + unsigned int mask_close; + int fileno; + int *dev_null_fd; + int parent_idx; + int child_idx; + bool nonblock; +} pfd_map[POPEN_FLAG_FD_STDEND_BIT] = { + { + .mask = POPEN_FLAG_FD_STDIN, + .mask_devnull = POPEN_FLAG_FD_STDIN_DEVNULL, + .mask_close = POPEN_FLAG_FD_STDIN_CLOSE, + .fileno = STDIN_FILENO, + .dev_null_fd = &dev_null_fd_ro, + .parent_idx = 1, + .child_idx = 0, + }, { + .mask = POPEN_FLAG_FD_STDOUT, + .mask_devnull = POPEN_FLAG_FD_STDOUT_DEVNULL, + .mask_close = POPEN_FLAG_FD_STDOUT_CLOSE, + .fileno = STDOUT_FILENO, + .dev_null_fd = &dev_null_fd_wr, + .parent_idx = 0, + .child_idx = 1, + }, { + .mask = POPEN_FLAG_FD_STDERR, + .mask_devnull = POPEN_FLAG_FD_STDERR_DEVNULL, + .mask_close = POPEN_FLAG_FD_STDERR_CLOSE, + .fileno = STDERR_FILENO, + .dev_null_fd = &dev_null_fd_wr, + .parent_idx = 0, + .child_idx = 1, + }, +}; + /** * Register popen handle in a pids map. */ @@ -213,7 +250,8 @@ handle_free(struct popen_handle *handle) * Returns 0 if so and -1 otherwise (and set a diag). */ static inline int -popen_may_io(struct popen_handle *handle, unsigned int io_flags) +popen_may_io(struct popen_handle *handle, unsigned int idx, + unsigned int io_flags, bool allow_closed) { if (!(io_flags & handle->flags)) { diag_set(IllegalParams, "popen: handle does not support the " @@ -221,6 +259,12 @@ popen_may_io(struct popen_handle *handle, unsigned int io_flags) return -1; } + if (! allow_closed && handle->ios[idx].fd < 0) { + diag_set(IllegalParams, "popen: attempt to operate on a closed " + "file descriptor"); + return -1; + } + return 0; } @@ -299,6 +343,7 @@ stdX_str(unsigned int index) * - count: data is too big. * - flags: POPEN_FLAG_FD_STDIN bit is unset. * - handle: handle does not support the requested IO operation. + * - handle: attempt to operate on a closed fd. * - SocketError: an IO error occurs at write(). * - TimedOut: @a timeout quota is exceeded. * - FiberIsCancelled: cancelled by an outside code. @@ -327,11 +372,11 @@ popen_write_timeout(struct popen_handle *handle, const void *buf, return -1; } - if (popen_may_io(handle, flags) != 0) - return -1; - int idx = STDIN_FILENO; + if (popen_may_io(handle, idx, flags, false) != 0) + return -1; + say_debug("popen: %d: write idx [%s:%d] buf %p count %zu " "fds %d timeout %.9g", handle->pid, stdX_str(idx), idx, buf, count, @@ -362,6 +407,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf, * - count: buffer is too big. * - flags: stdout and stdrr are both choosen or both missed * - handle: handle does not support the requested IO operation. + * - handle: attempt to operate on a closed fd. * - SocketError: an IO error occurs at read(). * - TimedOut: @a timeout quota is exceeded. * - FiberIsCancelled: cancelled by an outside code. @@ -390,12 +436,12 @@ popen_read_timeout(struct popen_handle *handle, void *buf, return -1; } - if (popen_may_io(handle, flags) != 0) - return -1; - int idx = flags & POPEN_FLAG_FD_STDOUT ? STDOUT_FILENO : STDERR_FILENO; + if (popen_may_io(handle, idx, flags, false) != 0) + return -1; + say_debug("popen: %d: read idx [%s:%d] buf %p count %zu " "fds %d timeout %.9g", handle->pid, stdX_str(idx), idx, buf, count, @@ -405,6 +451,80 @@ popen_read_timeout(struct popen_handle *handle, void *buf, timeout); } +/** + * Close parent's ends of std* fds. + * + * The following @a flags controls which fds should be closed: + * + * POPEN_FLAG_FD_STDIN close parent's end of child's stdin + * POPEN_FLAG_FD_STDOUT close parent's end of child's stdout + * POPEN_FLAG_FD_STDERR close parent's end of child's stderr + * + * The main reason to use this function is to send EOF to + * child's stdin. However parent's end of stdout / stderr + * may be closed too. + * + * The function does not fail on already closed fds (idempotence). + * However it fails on attempt to close the end of a pipe that was + * never exist. In other words, a subset of ..._FD_STD{IN,OUT,ERR} + * flags used at a handle creation may be used here. + * + * The function does not close any fds on a failure: either all + * requested fds are closed or neither of them. + * + * Returns 0 at success, otherwise -1 and set a diag. + * + * Possible errors: + * + * - IllegalParams: a parameter check fails: + * - flags: neither stdid, stdout nor stderr is choosen. + * - handle: handle does not support the requested IO operation + * (one of fds is not piped). + */ +int +popen_shutdown(struct popen_handle *handle, unsigned int flags) +{ + assert(handle != NULL); + + if ((flags & (POPEN_FLAG_FD_STDIN | + POPEN_FLAG_FD_STDOUT | + POPEN_FLAG_FD_STDERR)) == 0) { + diag_set(IllegalParams, + "popen: neither stdin, stdout nor stderr is choosen"); + return -1; + } + + /* Verify the operation. */ + for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) { + /* Operate only on asked fds. */ + unsigned int op_mask = pfd_map[idx].mask; + if ((flags & op_mask) == 0) + continue; + + if (popen_may_io(handle, idx, op_mask, true) != 0) + return -1; + } + + /* Perform the operation. */ + for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) { + /* Operate only on asked fds. */ + unsigned int op_mask = pfd_map[idx].mask; + if ((flags & op_mask) == 0) + continue; + + /* Skip already closed fds. */ + if (handle->ios[idx].fd < 0) + continue; + + say_debug("popen: %d: shutdown idx [%s:%d] fd %s", + handle->pid, stdX_str(idx), idx, + handle->ios[idx].fd); + coio_close_io(loop(), &handle->ios[idx]); + } + + return 0; +} + /** * Encode signal status into a human readable form. * @@ -865,42 +985,6 @@ popen_new(struct popen_opts *opts) int saved_errno; size_t i; - static const struct { - unsigned int mask; - unsigned int mask_devnull; - unsigned int mask_close; - int fileno; - int *dev_null_fd; - int parent_idx; - int child_idx; - bool nonblock; - } pfd_map[POPEN_FLAG_FD_STDEND_BIT] = { - { - .mask = POPEN_FLAG_FD_STDIN, - .mask_devnull = POPEN_FLAG_FD_STDIN_DEVNULL, - .mask_close = POPEN_FLAG_FD_STDIN_CLOSE, - .fileno = STDIN_FILENO, - .dev_null_fd = &dev_null_fd_ro, - .parent_idx = 1, - .child_idx = 0, - }, { - .mask = POPEN_FLAG_FD_STDOUT, - .mask_devnull = POPEN_FLAG_FD_STDOUT_DEVNULL, - .mask_close = POPEN_FLAG_FD_STDOUT_CLOSE, - .fileno = STDOUT_FILENO, - .dev_null_fd = &dev_null_fd_wr, - .parent_idx = 0, - .child_idx = 1, - }, { - .mask = POPEN_FLAG_FD_STDERR, - .mask_devnull = POPEN_FLAG_FD_STDERR_DEVNULL, - .mask_close = POPEN_FLAG_FD_STDERR_CLOSE, - .fileno = STDERR_FILENO, - .dev_null_fd = &dev_null_fd_wr, - .parent_idx = 0, - .child_idx = 1, - }, - }; /* * At max we could be skipping each pipe end * plus dev/null variants and logfd diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h index 4cdd95175..c068d5028 100644 --- a/src/lib/core/popen.h +++ b/src/lib/core/popen.h @@ -175,6 +175,9 @@ popen_read_timeout(struct popen_handle *handle, void *buf, size_t count, unsigned int flags, ev_tstamp timeout); +extern int +popen_shutdown(struct popen_handle *handle, unsigned int flags); + extern void popen_state(struct popen_handle *handle, int *state, int *exit_code); -- 2.25.0
next prev parent reply other threads:[~2020-04-14 11:39 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 01/12] popen: allow to kill process group Alexander Turenko 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 02/12] popen: add ability to keep child on deletion Alexander Turenko 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 03/12] popen: log a reason of close inherited fds failure Alexander Turenko 2020-04-14 11:52 ` Cyrill Gorcunov 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 04/12] popen: add missed diag_set() in popen_new() Alexander Turenko 2020-04-14 11:54 ` Cyrill Gorcunov 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 05/12] popen: remove retval from popen_stat() Alexander Turenko 2020-04-14 11:54 ` Cyrill Gorcunov 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 06/12] popen: quote multiword command arguments Alexander Turenko 2020-04-14 11:58 ` Cyrill Gorcunov 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 07/12] popen: add logging of duplicated logger fd Alexander Turenko 2020-04-14 11:58 ` Cyrill Gorcunov 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 08/12] popen: fix close-on-exec flag setting Alexander Turenko 2020-04-14 12:03 ` Cyrill Gorcunov 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 09/12] popen: clarify popen_{signal, delete} contract Alexander Turenko 2020-04-14 12:29 ` Cyrill Gorcunov 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw Alexander Turenko 2020-04-14 13:19 ` Cyrill Gorcunov 2020-04-15 4:21 ` Alexander Turenko 2020-04-15 7:27 ` Cyrill Gorcunov 2020-04-14 11:38 ` [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message Alexander Turenko 2020-04-14 12:32 ` Cyrill Gorcunov 2020-04-15 4:21 ` Alexander Turenko 2020-04-15 7:39 ` Cyrill Gorcunov 2020-04-15 21:45 ` Alexander Turenko 2020-04-14 11:38 ` Alexander Turenko [this message] 2020-04-14 13:05 ` [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds Cyrill Gorcunov 2020-04-15 4:21 ` Alexander Turenko 2020-04-15 7:43 ` Cyrill Gorcunov 2020-04-15 21:45 ` Alexander Turenko 2020-04-15 4:25 ` [Tarantool-patches] [PATCH 13/13] popen: add caution comment for popen_may_io() Alexander Turenko 2020-04-15 7:44 ` Cyrill Gorcunov 2020-04-15 4:52 ` [Tarantool-patches] [PATCH 14/14] popen: fix popen_write_timeout retval type Alexander Turenko 2020-04-15 23:57 ` [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko 2020-04-16 0:00 ` Alexander Turenko 2020-04-16 11:52 ` Cyrill Gorcunov 2020-04-17 6:58 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6873a6070a181c7c47f2e94295539c483fd6e7b7.1586862436.git.alexander.turenko@tarantool.org \ --to=alexander.turenko@tarantool.org \ --cc=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent'\''s end of std* fds' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox