[Tarantool-patches] [PATCH 06/13] popen: decouple logger fd from stderr
Cyrill Gorcunov
gorcunov at gmail.com
Fri Apr 10 12:18:24 MSK 2020
On Fri, Apr 10, 2020 at 05:50:44AM +0300, Alexander Turenko wrote:
> The default logger configuration writes logs to stderr.
>
> Popen implementation holds a logger fd until execve() to be able to
> write debug entries or information about a failure from a child. However
> when popen flags requires to close stderr in the child, the logger fd
> becomes closed: logging will fail.
>
> Another problem appears when a user want to capture stderr and
> tarantool's log level is set to debug (7). Since the logger uses stderr
> and it is fed to the parent using a pipe, the logger output will not
> shown on the 'real' stderr, but will be captured together with child's
> program debugging output.
>
> This commit duplicates a logger file descriptor that allows to close or
> redirect child's stderr without described side effects.
>
> See also 86ec3a5c4792ea1bba9f644da1e8f301314c8d29 ('popen: add logging
> in child process').
>
> Areas for improvements:
>
> * Copy logger fd at module initialization time instead of copying of
> each popen call.
>
> Alternatives:
>
> * Extend logger to allow to accumulate log entries in a buffer. Flush
> the buffer from the parent process. (It is possible since vfork does
> not split a virtual memory space).
>
> Part of #4031
> ---
> src/lib/core/popen.c | 124 +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 115 insertions(+), 9 deletions(-)
>
> diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> index 9d4e6ef3a..62920e0c8 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -74,6 +74,59 @@ popen_unregister(struct popen_handle *handle)
> mh_i32ptr_remove(popen_pids_map, &node, NULL);
> }
>
> +/**
> + * Duplicate a file descriptor, but not to std{in,out,err}.
> + *
> + * Return a new fd at success, otherwise return -1 and set a diag.
> + */
> +static int
> +dup_not_std_streams(int fd)
> +{
> + int res = -1;
> + int save_errno = 0;
> +
> + /*
> + * We will call dup() in a loop until it will return
> + * fd > STDERR_FILENO. The array `discarded_fds` stores
> + * intermediate fds to close them after all dup() calls.
> + */
> + static_assert(STDERR_FILENO + 1 == 3,
> + "Unexpected STDERR_FILENO constant");
We already have (in popen_new)
static_assert(STDIN_FILENO == 0 &&
STDOUT_FILENO == 1 &&
STDERR_FILENO == 2,
"stdin/out/err are not posix compatible");
no need for this again.
> + int discarded_fds[STDERR_FILENO + 1] = {-1, -1, -1};
And here we could
int discarded_fds[POPEN_FLAG_FD_STDEND_BIT]
the POPEN_FLAG_FD_STDEND_BIT constant introduced exactly for that.
> +
> + for (size_t i = 0; i < lengthof(discarded_fds) + 1; ++i) {
> + int new_fd = dup(fd);
> + if (new_fd < 0) {
> + save_errno = errno;
> + break;
> + }
> +
> + /* Found wanted fd. */
> + if (new_fd > STDERR_FILENO) {
> + res = new_fd;
> + break;
> + }
> +
> + /* Save to close then. */
> + assert(i < lengthof(discarded_fds));
> + discarded_fds[i] = new_fd;
> + }
> +
> + /* Close all intermediate fds (if any). */
> + for (size_t i = 0; i < lengthof(discarded_fds); ++i)
> + if (discarded_fds[i] >= 0)
> + close(discarded_fds[i]);
Wrap for() with {} please.
Otherwise looks good.
Acked-by: Cyrill Gorcunov <gorcunov at gmail.com>
More information about the Tarantool-patches
mailing list