Re: [Tarantool-patches] [PATCH 06/13] popen: decouple logger fd from stderr

[Cyrill Gorcunov <gorcunov@gmail.com>]

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@gmail.com>