From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 75500469719 for ; Thu, 12 Mar 2020 14:58:38 +0300 (MSK) Date: Thu, 12 Mar 2020 14:58:42 +0300 From: Alexander Turenko Message-ID: <20200312115842.q2gwcdkvolghalvu@tkn_work_nb> References: <20200302201227.31785-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200302201227.31785-1-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH 0/7] popen: various fixes and a test List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml I don't have objections anymore. LGTM. The actual version is on the branch gorcunov/gh-4031-popen-fixup-5. I'm a bit afraid that we don't test POPEN_FLAG_SETSID. However the code itself looks right and we maybe will return to this later. NB: We'll need to test two things to verify setsid(): * The situation when a controlling terminal died. Maybe use screen / tmux here? * The ability to kill a spawned process group at whole (but w/o tarantool itself). Say, "sh -c 'foo | bar'". Pushed also gorcunov/gh-4031-popen-fixup-5-full-ci to verify on all targets (just in case). See results here: https://gitlab.com/tarantool/tarantool/pipelines/125648893 (they are now in fly). WBR, Alexander Turenko. On Mon, Mar 02, 2020 at 11:12:20PM +0300, Cyrill Gorcunov wrote: > Here are a few fixes for popen engine I discovered during > testing on macos. Now the unit test passes. > > branch gorcunov/gh-4031-popen-fixup > > Cyrill Gorcunov (7): > core/say: Export logger fd > popen: allow accessing environ variable > popen: close_inherited_fds - add support for macos/freebsd > popen: log errors if popen creation failed > popen: add logging in child process > popen: handle setsid os specifics > test/unit: add popen test > > src/lib/core/popen.c | 92 ++++++++------ > src/lib/core/say.c | 6 + > src/lib/core/say.h | 7 ++ > test/unit/CMakeLists.txt | 3 + > test/unit/popen.c | 250 +++++++++++++++++++++++++++++++++++++++ > test/unit/popen.result | 25 ++++ > 6 files changed, 348 insertions(+), 35 deletions(-) > create mode 100644 test/unit/popen.c > create mode 100644 test/unit/popen.result > > > base-commit: 5e5d5a4a7a567891de2082e7f6c173497bb4e84b > -- > 2.20.1 >