From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94])
 (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 CD6124696C3
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 10 Apr 2020 19:41:26 +0300 (MSK)
Date: Fri, 10 Apr 2020 19:41:24 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20200410164124.5ynh3ltaixcg66kv@tkn_work_nb>
References: <20200410144021.5704-1-gorcunov@gmail.com>
 <20200410144021.5704-2-gorcunov@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200410144021.5704-2-gorcunov@gmail.com>
Subject: Re: [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process
	group
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: tml <tarantool-patches@dev.tarantool.org>

On Fri, Apr 10, 2020 at 05:40:20PM +0300, Cyrill Gorcunov wrote:
> As Alexander pointed out this might be useful
> for running a pipe of programs inside shell
> (i.e. popen.shell('foo | bar | baz', 'r')).
> 
> Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

Acked-by: Alexander Turenko <alexander.turenko@tarantool.org>

> @@ -443,10 +456,17 @@ popen_send_signal(struct popen_handle *handle, int signo)
>  	if (!popen_may_pidop(handle))
>  		return -1;
>  
> -	say_debug("popen: kill %d signo %d", handle->pid, signo);
> -	ret = kill(handle->pid, signo);
> +	say_debug("popen: %s %d signo %d",
> +		  handle->flags & POPEN_FLAG_GROUP_SIGNAL ?
> +		  "killpg" : "kill", handle->pid, signo);
> +	if (handle->flags & POPEN_FLAG_GROUP_SIGNAL)
> +		ret = killpg(handle->pid, signo);
> +	else
> +		ret = kill(handle->pid, signo);
>  	if (ret < 0) {
> -		diag_set(SystemError, "Unable to kill %d signo %d",
> +		diag_set(SystemError, "Unable to %s %d signo %d",
> +			 handle->flags & POPEN_FLAG_GROUP_SIGNAL ?
> +			 "killpg" : "kill",
>  			 handle->pid, signo);
>  	}

Nit: I would define a string constant, it will be easier to read I
guess.

 | const char *kill_func = handle->flags & POPEN_FLAG_GROUP_SIGNAL ?
 | 		"killpg" : "kill";

This function is changed in the 'Popen Lua API: preliminary patches'
patchset, which is pushed to master now. Need to be rebased so.