From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f68.google.com (mail-lf1-f68.google.com [209.85.167.68]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E2D224696C3 for ; Fri, 10 Apr 2020 12:18:27 +0300 (MSK) Received: by mail-lf1-f68.google.com with SMTP id m19so896953lfq.13 for ; Fri, 10 Apr 2020 02:18:27 -0700 (PDT) Date: Fri, 10 Apr 2020 12:18:24 +0300 From: Cyrill Gorcunov Message-ID: <20200410091824.GR3072@uranus> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 06/13] popen: decouple logger fd from stderr List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org 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