[Tarantool-patches] [PATCH 0/2] popen: provide more flags

[Tarantool-patches] [PATCH 0/2] popen: provide more flags

[Cyrill Gorcunov]

To be able to kill the group and keep child process running
even if tarantool has exited.

branch gorcunov/gh-4031-popen-tok-2

Cyrill Gorcunov (2):
  popen: Allow to kill process group
  popen: add ability to keep child on deletion

 src/lib/core/popen.c | 31 +++++++++++++++++++++++++++----
 src/lib/core/popen.h | 12 ++++++++++++
 2 files changed, 39 insertions(+), 4 deletions(-)

-- 
2.20.1

[Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group

[Cyrill Gorcunov]

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>
---
 src/lib/core/popen.c | 26 +++++++++++++++++++++++---
 src/lib/core/popen.h |  6 ++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 6b6062215..200b31b21 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -86,6 +86,19 @@ handle_new(struct popen_opts *opts)
 
 	assert(opts->argv != NULL && opts->nr_argv > 0);
 
+	/*
+	 * Killing group of signals allowed for a new
+	 * session only where it makes sense, otherwise
+	 * child gonna inherit group and we will be killing
+	 * ourself.
+	 */
+	if (opts->flags & POPEN_FLAG_GROUP_SIGNAL &&
+	    (opts->flags & POPEN_FLAG_SETSID) == 0) {
+		diag_set(IllegalParams,
+			 "popen: group signal without setting sid");
+		return NULL;
+	}
+
 	for (i = 0; i < opts->nr_argv; i++) {
 		if (opts->argv[i] == NULL)
 			continue;
@@ -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);
 	}
 	return ret;
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 9cbfed60c..f9dd0ff45 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -96,6 +96,12 @@ enum popen_flag_bits {
 	 */
 	POPEN_FLAG_RESTORE_SIGNALS_BIT	= 15,
 	POPEN_FLAG_RESTORE_SIGNALS	= (1 << POPEN_FLAG_RESTORE_SIGNALS_BIT),
+
+	/*
+	 * Send signal to a process group.
+	 */
+	POPEN_FLAG_GROUP_SIGNAL_BIT	= 16,
+	POPEN_FLAG_GROUP_SIGNAL		= (1 << POPEN_FLAG_GROUP_SIGNAL_BIT),
 };
 
 /**
-- 
2.20.1

[Tarantool-patches] [PATCH 2/2] popen: add ability to keep child on deletion

[Cyrill Gorcunov]

Currently popen_delete kills all children process.
Moreover we use popen_delete on tarantool exit.

Alexander pointed out that keep children running
even if tarantool is exited is still needed.

Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/popen.c | 5 ++++-
 src/lib/core/popen.h | 6 ++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 200b31b21..50b40b3a4 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -489,8 +489,11 @@ popen_delete(struct popen_handle *handle)
 		return -1;
 	}
 
-	if (popen_send_signal(handle, SIGKILL) && errno != ESRCH)
+	if ((handle->flags & POPEN_FLAG_KEEP_CHILD) == 0) {
+		if (popen_send_signal(handle, SIGKILL) &&
+		    errno != ESRCH)
 		return -1;
+	}
 
 	for (i = 0; i < lengthof(handle->ios); i++) {
 		if (handle->ios[i].fd != -1)
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index f9dd0ff45..2e87e11ff 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -102,6 +102,12 @@ enum popen_flag_bits {
 	 */
 	POPEN_FLAG_GROUP_SIGNAL_BIT	= 16,
 	POPEN_FLAG_GROUP_SIGNAL		= (1 << POPEN_FLAG_GROUP_SIGNAL_BIT),
+
+	/*
+	 * Keep child rinning on delete.
+	 */
+	POPEN_FLAG_KEEP_CHILD_BIT	= 17,
+	POPEN_FLAG_KEEP_CHILD		= (1 << POPEN_FLAG_KEEP_CHILD_BIT),
 };
 
 /**
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group

[Alexander Turenko]

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.

Re: [Tarantool-patches] [PATCH 2/2] popen: add ability to keep child on deletion

[Alexander Turenko]

On Fri, Apr 10, 2020 at 05:40:21PM +0300, Cyrill Gorcunov wrote:
> Currently popen_delete kills all children process.
> Moreover we use popen_delete on tarantool exit.
> 
> Alexander pointed out that keep children running
> even if tarantool is exited is still needed.
> 
> Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

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

> @@ -489,8 +489,11 @@ popen_delete(struct popen_handle *handle)
>  		return -1;
>  	}
>  
> -	if (popen_send_signal(handle, SIGKILL) && errno != ESRCH)
> +	if ((handle->flags & POPEN_FLAG_KEEP_CHILD) == 0) {
> +		if (popen_send_signal(handle, SIGKILL) &&
> +		    errno != ESRCH)
>  		return -1;
> +	}

Please, rebase after the patchset 'Popen Lua API: preliminary patches',
which was pushed to master.

> +	/*
> +	 * Keep child rinning on delete.
> +	 */

Typo: rinning.

> +	POPEN_FLAG_KEEP_CHILD_BIT	= 17,
> +	POPEN_FLAG_KEEP_CHILD		= (1 << POPEN_FLAG_KEEP_CHILD_BIT),
>  };

Re: [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group

[Alexander Turenko]

> +	/*
> +	 * Killing group of signals allowed for a new
> +	 * session only where it makes sense, otherwise
> +	 * child gonna inherit group and we will be killing
> +	 * ourself.
> +	 */
> +	if (opts->flags & POPEN_FLAG_GROUP_SIGNAL &&
> +	    (opts->flags & POPEN_FLAG_SETSID) == 0) {
> +		diag_set(IllegalParams,
> +			 "popen: group signal without setting sid");
> +		return NULL;
> +	}
> +

Since the patchset with the preliminary patches is land to master and it
contains formal list of possible errors for popen_new(), I propose the
following addition:

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 593e72d4d..fcc562734 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -812,6 +812,8 @@ signal_reset(void)
  *
  * Possible errors:
  *
+ * - IllegalParams: a parameter check fails:
+ *   - group signal is set, while setsid is not.
  * - SystemError: dup(), fcntl(), pipe(), vfork() or close() fails
  *   in the parent process.
  * - SystemError: (temporary restriction) one of std{in,out,err}

Re: [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group

[Alexander Turenko]

On Sun, Apr 12, 2020 at 04:27:59AM +0300, Alexander Turenko wrote:
> > +	/*
> > +	 * Killing group of signals allowed for a new
> > +	 * session only where it makes sense, otherwise
> > +	 * child gonna inherit group and we will be killing
> > +	 * ourself.
> > +	 */
> > +	if (opts->flags & POPEN_FLAG_GROUP_SIGNAL &&
> > +	    (opts->flags & POPEN_FLAG_SETSID) == 0) {
> > +		diag_set(IllegalParams,
> > +			 "popen: group signal without setting sid");
> > +		return NULL;
> > +	}
> > +
> 
> Since the patchset with the preliminary patches is land to master and it
> contains formal list of possible errors for popen_new(), I propose the
> following addition:
> 
> diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> index 593e72d4d..fcc562734 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -812,6 +812,8 @@ signal_reset(void)
>   *
>   * Possible errors:
>   *
> + * - IllegalParams: a parameter check fails:
> + *   - group signal is set, while setsid is not.
>   * - SystemError: dup(), fcntl(), pipe(), vfork() or close() fails
>   *   in the parent process.
>   * - SystemError: (temporary restriction) one of std{in,out,err}

Ouch, no, ignore it.

I added the list in the another patchset, which is under development for
now.

So I'll add the IllegalParams error to the list together with other
errors.

Sorry for noise.

WBR, Alexander Turenko.