[Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2

[Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2

[Alexander Turenko]

It is the second patchset of preparation patches that are needed to
implement Lua API for popen.

See the previous series here (already pushed to master):
https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015609.html

https://github.com/tarantool/tarantool/issues/4031
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2

I also pushed the same commit to *-full-ci branch to verify it on all
platforms:

https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2-full-ci

Alexander Turenko (10):
  popen: log a reason of close inherited fds failure
  popen: add missed diag_set() in popen_new()
  popen: remove retval from popen_stat()
  popen: quote multiword command arguments
  popen: add logging of duplicated logger fd
  popen: fix close-on-exec flag setting
  popen: clarify popen_{signal,delete} contract
  popen: add FIXME re group signal flaw
  popen: clarify popen_read_timeout error message
  popen: allow to close parent's end of std* fds

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

 src/lib/core/popen.c | 305 +++++++++++++++++++++++++++++++++----------
 src/lib/core/popen.h |  17 ++-
 2 files changed, 253 insertions(+), 69 deletions(-)

-- 
2.25.0

[Tarantool-patches] [PATCH 01/12] popen: allow to kill process group

[Alexander Turenko]

From: Cyrill Gorcunov <gorcunov@gmail.com>

As Alexander pointed out this might be useful
for running a pipe of programs inside shell
(i.e. popen.shell('foo | bar | baz', 'r')).

Part of #4031

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

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index df7f797b9..a5a305013 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -138,6 +138,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;
@@ -526,6 +539,9 @@ popen_state_str(unsigned int state)
 int
 popen_send_signal(struct popen_handle *handle, int signo)
 {
+	static const char *killops[] = { "kill", "killpg" };
+	const char *killop = handle->flags & POPEN_FLAG_GROUP_SIGNAL ?
+		killops[1] : killops[0];
 	int ret;
 
 	assert(handle != NULL);
@@ -535,17 +551,21 @@ popen_send_signal(struct popen_handle *handle, int signo)
 	 */
 	ret = popen_may_pidop(handle);
 	if (ret == 0) {
-		say_debug("popen: kill %d signo %d", handle->pid, signo);
+		say_debug("popen: %s %d signo %d", killop,
+			  handle->pid, signo);
 		assert(handle->pid != -1);
-		ret = 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 && errno == ESRCH) {
 		diag_set(SystemError, "Attempt to send a signal %d to a "
 			 "process that does not exist anymore", signo);
 		return -1;
 	} else if (ret < 0) {
-		diag_set(SystemError, "Unable to kill %d signo %d",
-			 handle->pid, signo);
+		diag_set(SystemError, "Unable to %s %d signo %d",
+			 killop, handle->pid, signo);
 		return -1;
 	}
 	return 0;
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 97c581c13..623d826b9 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.25.0

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

[Alexander Turenko]

From: Cyrill Gorcunov <gorcunov@gmail.com>

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.

Part of #4031

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

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index a5a305013..30be74a5f 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -585,12 +585,15 @@ popen_delete(struct popen_handle *handle)
 
 	assert(handle != NULL);
 
-	/*
-	 * Unable to kill the process -- give an error.
-	 * The process is not exist already -- pass over.
-	 */
-	if (popen_send_signal(handle, SIGKILL) != 0 && errno != ESRCH)
-		return -1;
+	if ((handle->flags & POPEN_FLAG_KEEP_CHILD) == 0) {
+		/*
+		 * Unable to kill the process -- give an error.
+		 * The process is not exist already -- pass over.
+		 */
+		if (popen_send_signal(handle, SIGKILL) != 0 &&
+		    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 623d826b9..570376d33 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 running on delete.
+	 */
+	POPEN_FLAG_KEEP_CHILD_BIT	= 17,
+	POPEN_FLAG_KEEP_CHILD		= (1 << POPEN_FLAG_KEEP_CHILD_BIT),
 };
 
 /**
-- 
2.25.0

[Tarantool-patches] [PATCH 03/12] popen: log a reason of close inherited fds failure

[Alexander Turenko]

This information may be useful for debuggging.

Part of #4031
---
 src/lib/core/popen.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 30be74a5f..fddcbae8f 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -652,6 +652,8 @@ make_pipe(int pfd[2])
  *
  * @a skip_fds is an array of @a nr_skip_fds elements
  * with descriptors which should be kept opened.
+ *
+ * Returns 0 at success, otherwise -1 and set a diag.
  */
 static int
 close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
@@ -1030,7 +1032,8 @@ popen_new(struct popen_opts *opts)
 		}
 
 		if (opts->flags & POPEN_FLAG_CLOSE_FDS) {
-			if (close_inherited_fds(skip_fds, nr_skip_fds)) {
+			if (close_inherited_fds(skip_fds, nr_skip_fds) != 0) {
+				diag_log();
 				say_syserror("child: close inherited fds");
 				goto exit_child;
 			}
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 03/12] popen: log a reason of close inherited fds failure

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:12PM +0300, Alexander Turenko wrote:
> This information may be useful for debuggging.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 04/12] popen: add missed diag_set() in popen_new()

[Alexander Turenko]

See the previous similar commits:

* popen: add missed diag_set() in popen IO functions
* popen: add missed diag_set in popen_signal/delete

Part of #4031
---
 src/lib/core/popen.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index fddcbae8f..c74ac17c7 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -803,7 +803,18 @@ signal_reset(void)
  * process will run with all files inherited from a parent.
  *
  * Returns pointer to a new popen handle on success,
- * otherwise NULL returned setting @a errno.
+ * otherwise NULL returned and an error is set to the
+ * diagnostics area.
+ *
+ * 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}
+ *   is closed in the parent process.
+ * - OutOfMemory: unable to allocate handle.
  */
 struct popen_handle *
 popen_new(struct popen_opts *opts)
@@ -972,6 +983,7 @@ popen_new(struct popen_opts *opts)
 	 */
 	handle->pid = vfork();
 	if (handle->pid < 0) {
+		diag_set(SystemError, "vfork() fails");
 		goto out_err;
 	} else if (handle->pid == 0) {
 		/*
@@ -1180,6 +1192,16 @@ exit_child:
 out_err:
 	diag_log();
 	saved_errno = errno;
+
+	/*
+	 * Save a reason of failure, because popen_delete() may
+	 * clobber the diagnostics area.
+	 */
+	struct diag *diag = diag_get();
+	struct error *e = diag_last_error(diag);
+	assert(e != NULL);
+	error_ref(e);
+
 	popen_delete(handle);
 	for (i = 0; i < lengthof(pfd); i++) {
 		if (pfd[i][0] != -1)
@@ -1189,6 +1211,11 @@ out_err:
 	}
 	if (log_fd >= 0)
 		close(log_fd);
+
+	/* Restore the diagnostics area entry. */
+	diag_set_error(diag, e);
+	error_unref(e);
+
 	errno = saved_errno;
 	return NULL;
 }
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 04/12] popen: add missed diag_set() in popen_new()

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:13PM +0300, Alexander Turenko wrote:
> See the previous similar commits:
> 
> * popen: add missed diag_set() in popen IO functions
> * popen: add missed diag_set in popen_signal/delete
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 05/12] popen: remove retval from popen_stat()

[Alexander Turenko]

The change 'popen: require popen handle to be non-NULL' makes
popen_stat() function always successful. There is no reason to return a
success / failure result.

See the previous similar patch: 'popen: remove retval from
popen_state()'.

Part of #4031
---
 src/lib/core/popen.c | 3 +--
 src/lib/core/popen.h | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index c74ac17c7..3ba1e2a48 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -239,7 +239,7 @@ popen_may_pidop(struct popen_handle *handle)
 /**
  * Fill popen object statistics.
  */
-int
+void
 popen_stat(struct popen_handle *handle, struct popen_stat *st)
 {
 	assert(handle != NULL);
@@ -252,7 +252,6 @@ popen_stat(struct popen_handle *handle, struct popen_stat *st)
 
 	for (size_t i = 0; i < lengthof(handle->ios); i++)
 		st->fds[i] = handle->ios[i].fd;
-	return 0;
 }
 
 /**
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 570376d33..4cdd95175 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -159,7 +159,7 @@ struct popen_stat {
 	int			fds[POPEN_FLAG_FD_STDEND_BIT];
 };
 
-extern int
+extern void
 popen_stat(struct popen_handle *handle, struct popen_stat *st);
 
 extern const char *
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 05/12] popen: remove retval from popen_stat()

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:14PM +0300, Alexander Turenko wrote:
> The change 'popen: require popen handle to be non-NULL' makes
> popen_stat() function always successful. There is no reason to return a
> success / failure result.
> 
> See the previous similar patch: 'popen: remove retval from
> popen_state()'.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 06/12] popen: quote multiword command arguments

[Alexander Turenko]

Of course it is still not fair shell-style quoting: at least we should
also escape quotes inside arguments. But it gives correct output for
most of typical commands and has straightforward implementation.

Part of #4031
---
 src/lib/core/popen.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 3ba1e2a48..089c84830 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -154,7 +154,7 @@ handle_new(struct popen_opts *opts)
 	for (i = 0; i < opts->nr_argv; i++) {
 		if (opts->argv[i] == NULL)
 			continue;
-		size += strlen(opts->argv[i]) + 1;
+		size += strlen(opts->argv[i]) + 3;
 	}
 
 	handle = malloc(sizeof(*handle) + size);
@@ -168,8 +168,13 @@ handle_new(struct popen_opts *opts)
 	for (i = 0; i < opts->nr_argv-1; i++) {
 		if (opts->argv[i] == NULL)
 			continue;
+		bool is_multiword = strchr(opts->argv[i], ' ') != NULL;
+		if (is_multiword)
+			*pos++ = '\'';
 		strcpy(pos, opts->argv[i]);
 		pos += strlen(opts->argv[i]);
+		if (is_multiword)
+			*pos++ = '\'';
 		*pos++ = ' ';
 	}
 	pos[-1] = '\0';
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 06/12] popen: quote multiword command arguments

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:15PM +0300, Alexander Turenko wrote:
> Of course it is still not fair shell-style quoting: at least we should
> also escape quotes inside arguments. But it gives correct output for
> most of typical commands and has straightforward implementation.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 07/12] popen: add logging of duplicated logger fd

[Alexander Turenko]

For debugging purposes.

Part of #4031
---
 src/lib/core/popen.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 089c84830..4d57cd41e 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -896,6 +896,7 @@ popen_new(struct popen_opts *opts)
 	int old_log_fd = log_get_fd();
 	if (old_log_fd >= 0) {
 		log_fd = dup_not_std_streams(old_log_fd);
+		say_debug("popen: duplicate logfd: %d", log_fd);
 		if (log_fd < 0)
 			return NULL;
 		if (fcntl(log_fd, F_SETFL, O_CLOEXEC) != 0) {
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 07/12] popen: add logging of duplicated logger fd

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:16PM +0300, Alexander Turenko wrote:
> For debugging purposes.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 08/12] popen: fix close-on-exec flag setting

[Alexander Turenko]

fcntl(2) lists flags that can be set using F_SETFL: O_CLOEXEC is not
included there. F_SETFD should be used to set close-on-exec.

Parent's end of pipes are closed explicitly in a child process anyway.
However this change fixes closing of the copy of a logger fd. See commit
07a07b3cc7b85375d20b3fc6ca1e5060304f337b ('popen: decouple logger fd
from stderr') for more information why this file descriptor was
introduced.

Part of #4031.
---
 src/lib/core/popen.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 4d57cd41e..1733e5131 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -638,8 +638,8 @@ make_pipe(int pfd[2])
 		diag_set(SystemError, "Can't create pipe");
 		return -1;
 	}
-	if (fcntl(pfd[0], F_SETFL, O_CLOEXEC) ||
-	    fcntl(pfd[1], F_SETFL, O_CLOEXEC)) {
+	if (fcntl(pfd[0], F_SETFD, FD_CLOEXEC) ||
+	    fcntl(pfd[1], F_SETFD, FD_CLOEXEC)) {
 		int saved_errno = errno;
 		diag_set(SystemError, "Can't unblock pipe");
 		close(pfd[0]), pfd[0] = -1;
@@ -899,9 +899,9 @@ popen_new(struct popen_opts *opts)
 		say_debug("popen: duplicate logfd: %d", log_fd);
 		if (log_fd < 0)
 			return NULL;
-		if (fcntl(log_fd, F_SETFL, O_CLOEXEC) != 0) {
+		if (fcntl(log_fd, F_SETFD, FD_CLOEXEC) != 0) {
 			diag_set(SystemError,
-				 "Unable to set O_CLOEXEC on temporary logfd");
+				 "Unable to set FD_CLOEXEC on temporary logfd");
 			close(log_fd);
 			return NULL;
 		}
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 08/12] popen: fix close-on-exec flag setting

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:17PM +0300, Alexander Turenko wrote:
> fcntl(2) lists flags that can be set using F_SETFL: O_CLOEXEC is not
> included there. F_SETFD should be used to set close-on-exec.
> 
> Parent's end of pipes are closed explicitly in a child process anyway.
> However this change fixes closing of the copy of a logger fd. See commit
> 07a07b3cc7b85375d20b3fc6ca1e5060304f337b ('popen: decouple logger fd
> from stderr') for more information why this file descriptor was
> introduced.
> 
> Part of #4031.

Good catch!
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 09/12] popen: clarify popen_{signal, delete} contract

[Alexander Turenko]

It is convenient to have a formal description of an API during
development and when writing a documentation. I plan to use those
contracts when I will write an API description for the future popen Lua
module.

Part of #4031
---
 src/lib/core/popen.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 1733e5131..411aad03b 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -360,7 +360,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
  *
  * - IllegalParams: a parameter check fails:
  *   - count: buffer is too big.
- *   - flags: POPEN_FLAG_FD_STD{OUT,ERR} are unset both.
+ *   - flags: POPEN_FLAG_FD_STD{OUT,ERR} are set or unset both.
  *   - handle: handle does not support the requested IO operation.
  * - SocketError: an IO error occurs at read().
  * - TimedOut: @a timeout quota is exceeded.
@@ -536,8 +536,18 @@ popen_state_str(unsigned int state)
 /**
  * Send a signal to a child process.
  *
+ * When POPEN_FLAG_GROUP_SIGNAL is set the function sends
+ * a signal to a process group rather than a process.
+ *
  * Return 0 at success or -1 at failure (and set a diag).
  *
+ * Possible errors:
+ *
+ * - SystemError: a process does not exists anymore
+ * - SystemError: invalid signal number
+ * - SystemError: no permission to send a signal to
+ *                a process or a process group
+ *
  * Set errno to ESRCH when a process does not exist.
  */
 int
@@ -578,9 +588,21 @@ popen_send_signal(struct popen_handle *handle, int signo)
 /**
  * Delete a popen handle.
  *
- * The function kills a child process and
- * close all fds and remove the handle from
- * a living list and finally frees the handle.
+ * The function performs the following actions:
+ *
+ * - Kill a child process or a process group depending of
+ *   POPEN_FLAG_GROUP_SIGNAL (if the process is alive and
+ *   POPEN_FLAG_KEEP_CHILD flag is not set).
+ * - Close all fds occupied by the handle.
+ * - Remove the handle from a living list.
+ * - Free all occupied memory.
+ *
+ * Return 0 at success and -1 at failure (and set a diag).
+ *
+ * Possible errors:
+ *
+ * - SystemError: no permission to send a signal to
+ *                a process or a process group
  */
 int
 popen_delete(struct popen_handle *handle)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 09/12] popen: clarify popen_{signal, delete} contract

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:18PM +0300, Alexander Turenko wrote:
> It is convenient to have a formal description of an API during
> development and when writing a documentation. I plan to use those
> contracts when I will write an API description for the future popen Lua
> module.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw

[Alexander Turenko]

It is convenient to have such anchors for known problems.

Part of #4031
---
 src/lib/core/popen.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 411aad03b..36cd7b07d 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -562,6 +562,11 @@ popen_send_signal(struct popen_handle *handle, int signo)
 
 	/*
 	 * A child may be killed or exited already.
+	 *
+	 * FIXME: A process may be died at the time and
+	 * the function will not send a signal to other
+	 * processes in the group even when
+	 * POPEN_FLAG_GROUP_SIGNAL is set.
 	 */
 	ret = popen_may_pidop(handle);
 	if (ret == 0) {
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:19PM +0300, Alexander Turenko wrote:
> It is convenient to have such anchors for known problems.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Actually I need to think about all this more. But I think
it is safe to merge now.

Re: [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw

[Alexander Turenko]

On Tue, Apr 14, 2020 at 04:19:10PM +0300, Cyrill Gorcunov wrote:
> On Tue, Apr 14, 2020 at 02:38:19PM +0300, Alexander Turenko wrote:
> > It is convenient to have such anchors for known problems.
> > 
> > Part of #4031
> Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> Actually I need to think about all this more. But I think
> it is safe to merge now.

I think about it more and also briefly discussed with Cyrill.

It seems this problem is a kind of fundamental and we should explain it
in the documentation comments rather than mark to fix later.

So I rewrote the commit in this way:

commit 234b184ffdd9dce234520643f12fec12bbc9fa1a
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Mon Apr 13 13:37:55 2020 +0300

    popen: clarify group signaling details
    
    Even when ..._SETSID and ..._GROUP_SIGNAL are set, we unable to safely
    kill a process group after the child process we spawned becomes died. So
    we don't do that.
    
    The behaviour seems to be indefeasible part of Unix process group
    design. The best that we can do here is describe those details in the
    documentation comment.
    
    NB: It seems that pid namespaces allow to overcome this problem, however
    it is the Linux specific feature, so we unlikely will use them.
    
    Part of #4031

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 411aad03b..5cd7926d1 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -539,6 +539,13 @@ popen_state_str(unsigned int state)
  * When POPEN_FLAG_GROUP_SIGNAL is set the function sends
  * a signal to a process group rather than a process.
  *
+ * A signal will not be sent if the child process is already
+ * dead: otherwise we might kill another process that occupies
+ * the same PID later. This means that if the child process
+ * dies before its own childs, the function will not send a
+ * signal to the process group even when ..._SETSID and
+ * ..._GROUP_SIGNAL are set.
+ *
  * Return 0 at success or -1 at failure (and set a diag).
  *
  * Possible errors:
@@ -597,6 +604,8 @@ popen_send_signal(struct popen_handle *handle, int signo)
  * - Remove the handle from a living list.
  * - Free all occupied memory.
  *
+ * @see popen_send_signal() for note about ..._GROUP_SIGNAL.
+ *
  * Return 0 at success and -1 at failure (and set a diag).
  *
  * Possible errors:
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 4cdd95175..8cb71e28d 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -99,6 +99,8 @@ enum popen_flag_bits {
 
 	/*
 	 * Send signal to a process group.
+	 *
+	 * @see popen_send_signal() for details.
 	 */
 	POPEN_FLAG_GROUP_SIGNAL_BIT	= 16,
 	POPEN_FLAG_GROUP_SIGNAL		= (1 << POPEN_FLAG_GROUP_SIGNAL_BIT),

Re: [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw

[Cyrill Gorcunov]

On Wed, Apr 15, 2020 at 07:21:23AM +0300, Alexander Turenko wrote:
...
> @@ -597,6 +604,8 @@ popen_send_signal(struct popen_handle *handle, int signo)
>   * - Remove the handle from a living list.
>   * - Free all occupied memory.
>   *
> + * @see popen_send_signal() for note about ..._GROUP_SIGNAL.
> + *
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message

[Alexander Turenko]

Popen backend errors should be meaningful for a user of the popen Lua
API, because otherwise we'll need to map backend errors into Lua API
errors. This particular failure can't appear when the function is called
from the Lua API, but it is good to keep all error messages in one
style.

Part of #4031
---
 src/lib/core/popen.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 36cd7b07d..640dffc2b 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -360,7 +360,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
  *
  * - IllegalParams: a parameter check fails:
  *   - count: buffer is too big.
- *   - flags: POPEN_FLAG_FD_STD{OUT,ERR} are set or unset both.
+ *   - flags: stdout and stdrr are both choosen or both missed
  *   - handle: handle does not support the requested IO operation.
  * - SocketError: an IO error occurs at read().
  * - TimedOut: @a timeout quota is exceeded.
@@ -379,8 +379,8 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 	}
 
 	if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
-		diag_set(IllegalParams, "popen: POPEN_FLAG_FD_STD{OUT,ERR} are "
-			 "unset both");
+		diag_set(IllegalParams,
+			 "popen: neither stdout nor stderr is choosen");
 		return -1;
 	}
 
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:20PM +0300, Alexander Turenko wrote:
> Popen backend errors should be meaningful for a user of the popen Lua
> API, because otherwise we'll need to map backend errors into Lua API
> errors. This particular failure can't appear when the function is called
> from the Lua API, but it is good to keep all error messages in one
> style.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message

[Alexander Turenko]

On Tue, Apr 14, 2020 at 02:38:20PM +0300, Alexander Turenko wrote:
> Popen backend errors should be meaningful for a user of the popen Lua
> API, because otherwise we'll need to map backend errors into Lua API
> errors. This particular failure can't appear when the function is called
> from the Lua API, but it is good to keep all error messages in one
> style.
> 
> Part of #4031
> ---
>  src/lib/core/popen.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> index 36cd7b07d..640dffc2b 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -360,7 +360,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
>   *
>   * - IllegalParams: a parameter check fails:
>   *   - count: buffer is too big.
> - *   - flags: POPEN_FLAG_FD_STD{OUT,ERR} are set or unset both.
> + *   - flags: stdout and stdrr are both choosen or both missed
>   *   - handle: handle does not support the requested IO operation.
>   * - SocketError: an IO error occurs at read().
>   * - TimedOut: @a timeout quota is exceeded.
> @@ -379,8 +379,8 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
>  	}
>  
>  	if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
> -		diag_set(IllegalParams, "popen: POPEN_FLAG_FD_STD{OUT,ERR} are "
> -			 "unset both");
> +		diag_set(IllegalParams,
> +			 "popen: neither stdout nor stderr is choosen");
>  		return -1;
>  	}

I was inattentive: there is an error message of the same style in
popen_read_timeout(). Those functions should be changed both together.

Incremental diff:

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 640dffc2b..64711d737 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -297,7 +297,7 @@ stdX_str(unsigned int index)
  *
  * - IllegalParams: a parameter check fails:
  *   - count: data is too big.
- *   - flags: POPEN_FLAG_FD_STDIN bit is unset.
+ *   - flags: stdin is not choosen.
  *   - handle: handle does not support the requested IO operation.
  * - SocketError: an IO error occurs at write().
  * - TimedOut: @a timeout quota is exceeded.
@@ -322,8 +322,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
 	}
 
 	if (!(flags & POPEN_FLAG_FD_STDIN)) {
-		diag_set(IllegalParams,
-			 "popen: POPEN_FLAG_FD_STDIN bit is unset");
+		diag_set(IllegalParams, "popen: stdin is not choosen");
 		return -1;
 	}

The new commit message:

 | popen: refine popen_{read,write}_timeout errors
 |
 | Popen backend errors should be meaningful for a user of the popen Lua
 | API, because otherwise we'll need to map backend errors into Lua API
 | errors. Those particular failures can't appear when the functions are
 | called from the Lua API, but it is good to keep all error messages in
 | one style.
 |
 | Part of #4031

Don't sure, however, that 'choosen' is the right word in this context.

Re: [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message

[Cyrill Gorcunov]

On Wed, Apr 15, 2020 at 07:21:36AM +0300, Alexander Turenko wrote:
> >  
> >  	if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
> > -		diag_set(IllegalParams, "popen: POPEN_FLAG_FD_STD{OUT,ERR} are "
> > -			 "unset both");
> > +		diag_set(IllegalParams,
> > +			 "popen: neither stdout nor stderr is choosen");
> >  		return -1;
> >  	}
> 
> I was inattentive: there is an error message of the same style in
> popen_read_timeout(). Those functions should be changed both together.

I think better "popen: neither stdout nor stderr is set", but no
strong preferences here (except there is no such word as 'choosen')

Re: [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message

[Alexander Turenko]

On Wed, Apr 15, 2020 at 10:39:30AM +0300, Cyrill Gorcunov wrote:
> On Wed, Apr 15, 2020 at 07:21:36AM +0300, Alexander Turenko wrote:
> > >  
> > >  	if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
> > > -		diag_set(IllegalParams, "popen: POPEN_FLAG_FD_STD{OUT,ERR} are "
> > > -			 "unset both");
> > > +		diag_set(IllegalParams,
> > > +			 "popen: neither stdout nor stderr is choosen");
> > >  		return -1;
> > >  	}
> > 
> > I was inattentive: there is an error message of the same style in
> > popen_read_timeout(). Those functions should be changed both together.
> 
> I think better "popen: neither stdout nor stderr is set", but no
> strong preferences here (except there is no such word as 'choosen')

Strictly speaking ...FD_STD{OUT,ERR} can be set, they are flags. But
stdout / stderr are not flags, it is a stream on which a user want to do
an operaton. It is requested, askec, chosen.

However options in the Lua API names exactly as 'stdin', 'stdout',
'stderr': so 'set' is meaningful for the Lua API.

Anyway, I guess 'stdout is set' is handy shortcut for formal 'asked to
operate on stdout'. So I'll just use 'set' here.

Thanks for the advice!

----

The incremental diff:

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index d33efbb18..ee16da28b 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -297,7 +297,7 @@ stdX_str(unsigned int index)
  *
  * - IllegalParams: a parameter check fails:
  *   - count: data is too big.
- *   - flags: stdin is not choosen.
+ *   - flags: stdin is not set.
  *   - handle: handle does not support the requested IO operation.
  * - SocketError: an IO error occurs at write().
  * - TimedOut: @a timeout quota is exceeded.
@@ -322,7 +322,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
 	}
 
 	if (!(flags & POPEN_FLAG_FD_STDIN)) {
-		diag_set(IllegalParams, "popen: stdin is not choosen");
+		diag_set(IllegalParams, "popen: stdin is not set");
 		return -1;
 	}
 
@@ -359,7 +359,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
  *
  * - IllegalParams: a parameter check fails:
  *   - count: buffer is too big.
- *   - flags: stdout and stdrr are both choosen or both missed
+ *   - flags: stdout and stdrr are both set or both missed.
  *   - handle: handle does not support the requested IO operation.
  * - SocketError: an IO error occurs at read().
  * - TimedOut: @a timeout quota is exceeded.
@@ -379,7 +379,7 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 
 	if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
 		diag_set(IllegalParams,
-			 "popen: neither stdout nor stderr is choosen");
+			 "popen: neither stdout nor stderr is set");
 		return -1;
 	}
 

[Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds

[Alexander Turenko]

The function popen_shutdown() checks whether std{in,out,err} was piped
and closes the parent's end. A user should have ability to send EOF for
child's stdin for stream programs like `grep`. It is better when there
is a function that encapsulates proper checks, error messages and the
actual actions.

This commit in particular reverts
1ef95b99f6553b246729e7bb5bdc19038043db74 ('popen: remove redundant fd
check before perform IO'), because now the check is meaningful: an fd
may become closed before the whole popen handle will be deleted.

Part of #4031
---
 src/lib/core/popen.c | 170 ++++++++++++++++++++++++++++++++-----------
 src/lib/core/popen.h |   3 +
 2 files changed, 130 insertions(+), 43 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 640dffc2b..8760429c2 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -34,6 +34,43 @@ static RLIST_HEAD(popen_head);
 static int dev_null_fd_ro = -1;
 static int dev_null_fd_wr = -1;
 
+static const struct {
+	unsigned int	mask;
+	unsigned int	mask_devnull;
+	unsigned int	mask_close;
+	int		fileno;
+	int		*dev_null_fd;
+	int		parent_idx;
+	int		child_idx;
+	bool		nonblock;
+} pfd_map[POPEN_FLAG_FD_STDEND_BIT] = {
+	{
+		.mask		= POPEN_FLAG_FD_STDIN,
+		.mask_devnull	= POPEN_FLAG_FD_STDIN_DEVNULL,
+		.mask_close	= POPEN_FLAG_FD_STDIN_CLOSE,
+		.fileno		= STDIN_FILENO,
+		.dev_null_fd	= &dev_null_fd_ro,
+		.parent_idx	= 1,
+		.child_idx	= 0,
+	}, {
+		.mask		= POPEN_FLAG_FD_STDOUT,
+		.mask_devnull	= POPEN_FLAG_FD_STDOUT_DEVNULL,
+		.mask_close	= POPEN_FLAG_FD_STDOUT_CLOSE,
+		.fileno		= STDOUT_FILENO,
+		.dev_null_fd	= &dev_null_fd_wr,
+		.parent_idx	= 0,
+		.child_idx	= 1,
+	}, {
+		.mask		= POPEN_FLAG_FD_STDERR,
+		.mask_devnull	= POPEN_FLAG_FD_STDERR_DEVNULL,
+		.mask_close	= POPEN_FLAG_FD_STDERR_CLOSE,
+		.fileno		= STDERR_FILENO,
+		.dev_null_fd	= &dev_null_fd_wr,
+		.parent_idx	= 0,
+		.child_idx	= 1,
+	},
+};
+
 /**
  * Register popen handle in a pids map.
  */
@@ -213,7 +250,8 @@ handle_free(struct popen_handle *handle)
  * Returns 0 if so and -1 otherwise (and set a diag).
  */
 static inline int
-popen_may_io(struct popen_handle *handle, unsigned int io_flags)
+popen_may_io(struct popen_handle *handle, unsigned int idx,
+	     unsigned int io_flags, bool allow_closed)
 {
 	if (!(io_flags & handle->flags)) {
 		diag_set(IllegalParams, "popen: handle does not support the "
@@ -221,6 +259,12 @@ popen_may_io(struct popen_handle *handle, unsigned int io_flags)
 		return -1;
 	}
 
+       if (! allow_closed && handle->ios[idx].fd < 0) {
+	       diag_set(IllegalParams, "popen: attempt to operate on a closed "
+			"file descriptor");
+               return -1;
+       }
+
 	return 0;
 }
 
@@ -299,6 +343,7 @@ stdX_str(unsigned int index)
  *   - count: data is too big.
  *   - flags: POPEN_FLAG_FD_STDIN bit is unset.
  *   - handle: handle does not support the requested IO operation.
+ *   - handle: attempt to operate on a closed fd.
  * - SocketError: an IO error occurs at write().
  * - TimedOut: @a timeout quota is exceeded.
  * - FiberIsCancelled: cancelled by an outside code.
@@ -327,11 +372,11 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
 		return -1;
 	}
 
-	if (popen_may_io(handle, flags) != 0)
-		return -1;
-
 	int idx = STDIN_FILENO;
 
+	if (popen_may_io(handle, idx, flags, false) != 0)
+		return -1;
+
 	say_debug("popen: %d: write idx [%s:%d] buf %p count %zu "
 		  "fds %d timeout %.9g",
 		  handle->pid, stdX_str(idx), idx, buf, count,
@@ -362,6 +407,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
  *   - count: buffer is too big.
  *   - flags: stdout and stdrr are both choosen or both missed
  *   - handle: handle does not support the requested IO operation.
+ *   - handle: attempt to operate on a closed fd.
  * - SocketError: an IO error occurs at read().
  * - TimedOut: @a timeout quota is exceeded.
  * - FiberIsCancelled: cancelled by an outside code.
@@ -390,12 +436,12 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 		return -1;
 	}
 
-	if (popen_may_io(handle, flags) != 0)
-		return -1;
-
 	int idx = flags & POPEN_FLAG_FD_STDOUT ?
 		STDOUT_FILENO : STDERR_FILENO;
 
+	if (popen_may_io(handle, idx, flags, false) != 0)
+		return -1;
+
 	say_debug("popen: %d: read idx [%s:%d] buf %p count %zu "
 		  "fds %d timeout %.9g",
 		  handle->pid, stdX_str(idx), idx, buf, count,
@@ -405,6 +451,80 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 					    timeout);
 }
 
+/**
+ * Close parent's ends of std* fds.
+ *
+ * The following @a flags controls which fds should be closed:
+ *
+ *  POPEN_FLAG_FD_STDIN   close parent's end of child's stdin
+ *  POPEN_FLAG_FD_STDOUT  close parent's end of child's stdout
+ *  POPEN_FLAG_FD_STDERR  close parent's end of child's stderr
+ *
+ * The main reason to use this function is to send EOF to
+ * child's stdin. However parent's end of stdout / stderr
+ * may be closed too.
+ *
+ * The function does not fail on already closed fds (idempotence).
+ * However it fails on attempt to close the end of a pipe that was
+ * never exist. In other words, a subset of ..._FD_STD{IN,OUT,ERR}
+ * flags used at a handle creation may be used here.
+ *
+ * The function does not close any fds on a failure: either all
+ * requested fds are closed or neither of them.
+ *
+ * Returns 0 at success, otherwise -1 and set a diag.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: a parameter check fails:
+ *   - flags: neither stdid, stdout nor stderr is choosen.
+ *   - handle: handle does not support the requested IO operation
+ *             (one of fds is not piped).
+ */
+int
+popen_shutdown(struct popen_handle *handle, unsigned int flags)
+{
+	assert(handle != NULL);
+
+	if ((flags & (POPEN_FLAG_FD_STDIN |
+		      POPEN_FLAG_FD_STDOUT |
+		      POPEN_FLAG_FD_STDERR)) == 0) {
+		diag_set(IllegalParams,
+			 "popen: neither stdin, stdout nor stderr is choosen");
+		return -1;
+	}
+
+	/* Verify the operation. */
+	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
+		/* Operate only on asked fds. */
+		unsigned int op_mask = pfd_map[idx].mask;
+		if ((flags & op_mask) == 0)
+			continue;
+
+		if (popen_may_io(handle, idx, op_mask, true) != 0)
+			return -1;
+	}
+
+	/* Perform the operation. */
+	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
+		/* Operate only on asked fds. */
+		unsigned int op_mask = pfd_map[idx].mask;
+		if ((flags & op_mask) == 0)
+			continue;
+
+		/* Skip already closed fds. */
+		if (handle->ios[idx].fd < 0)
+			continue;
+
+		say_debug("popen: %d: shutdown idx [%s:%d] fd %s",
+			  handle->pid, stdX_str(idx), idx,
+			  handle->ios[idx].fd);
+		coio_close_io(loop(), &handle->ios[idx]);
+	}
+
+	return 0;
+}
+
 /**
  * Encode signal status into a human readable form.
  *
@@ -865,42 +985,6 @@ popen_new(struct popen_opts *opts)
 	int saved_errno;
 	size_t i;
 
-	static const struct {
-		unsigned int	mask;
-		unsigned int	mask_devnull;
-		unsigned int	mask_close;
-		int		fileno;
-		int		*dev_null_fd;
-		int		parent_idx;
-		int		child_idx;
-		bool		nonblock;
-	} pfd_map[POPEN_FLAG_FD_STDEND_BIT] = {
-		{
-			.mask		= POPEN_FLAG_FD_STDIN,
-			.mask_devnull	= POPEN_FLAG_FD_STDIN_DEVNULL,
-			.mask_close	= POPEN_FLAG_FD_STDIN_CLOSE,
-			.fileno		= STDIN_FILENO,
-			.dev_null_fd	= &dev_null_fd_ro,
-			.parent_idx	= 1,
-			.child_idx	= 0,
-		}, {
-			.mask		= POPEN_FLAG_FD_STDOUT,
-			.mask_devnull	= POPEN_FLAG_FD_STDOUT_DEVNULL,
-			.mask_close	= POPEN_FLAG_FD_STDOUT_CLOSE,
-			.fileno		= STDOUT_FILENO,
-			.dev_null_fd	= &dev_null_fd_wr,
-			.parent_idx	= 0,
-			.child_idx	= 1,
-		}, {
-			.mask		= POPEN_FLAG_FD_STDERR,
-			.mask_devnull	= POPEN_FLAG_FD_STDERR_DEVNULL,
-			.mask_close	= POPEN_FLAG_FD_STDERR_CLOSE,
-			.fileno		= STDERR_FILENO,
-			.dev_null_fd	= &dev_null_fd_wr,
-			.parent_idx	= 0,
-			.child_idx	= 1,
-		},
-	};
 	/*
 	 * At max we could be skipping each pipe end
 	 * plus dev/null variants and logfd
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 4cdd95175..c068d5028 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -175,6 +175,9 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 		   size_t count, unsigned int flags,
 		   ev_tstamp timeout);
 
+extern int
+popen_shutdown(struct popen_handle *handle, unsigned int flags);
+
 extern void
 popen_state(struct popen_handle *handle, int *state, int *exit_code);
 
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds

[Cyrill Gorcunov]

On Tue, Apr 14, 2020 at 02:38:21PM +0300, Alexander Turenko wrote:
> The function popen_shutdown() checks whether std{in,out,err} was piped
> and closes the parent's end. A user should have ability to send EOF for
> child's stdin for stream programs like `grep`. It is better when there
> is a function that encapsulates proper checks, error messages and the
> actual actions.
> 
> This commit in particular reverts
> 1ef95b99f6553b246729e7bb5bdc19038043db74 ('popen: remove redundant fd
> check before perform IO'), because now the check is meaningful: an fd
> may become closed before the whole popen handle will be deleted.
> 
> Part of #4031
> ---
>  src/lib/core/popen.c | 170 ++++++++++++++++++++++++++++++++-----------
>  src/lib/core/popen.h |   3 +
>  2 files changed, 130 insertions(+), 43 deletions(-)
> 
> diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> index 640dffc2b..8760429c2 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -34,6 +34,43 @@ static RLIST_HEAD(popen_head);
>  static int dev_null_fd_ro = -1;
>  static int dev_null_fd_wr = -1;
>  
> +static const struct {
> +	unsigned int	mask;
> +	unsigned int	mask_devnull;
> +	unsigned int	mask_close;
> +	int		fileno;
> +	int		*dev_null_fd;
> +	int		parent_idx;
> +	int		child_idx;
> +	bool		nonblock;
> +} pfd_map[POPEN_FLAG_FD_STDEND_BIT] = {
> +	{
> +		.mask		= POPEN_FLAG_FD_STDIN,
> +		.mask_devnull	= POPEN_FLAG_FD_STDIN_DEVNULL,
> +		.mask_close	= POPEN_FLAG_FD_STDIN_CLOSE,
> +		.fileno		= STDIN_FILENO,
> +		.dev_null_fd	= &dev_null_fd_ro,
> +		.parent_idx	= 1,
> +		.child_idx	= 0,
> +	}, {
> +		.mask		= POPEN_FLAG_FD_STDOUT,
> +		.mask_devnull	= POPEN_FLAG_FD_STDOUT_DEVNULL,
> +		.mask_close	= POPEN_FLAG_FD_STDOUT_CLOSE,
> +		.fileno		= STDOUT_FILENO,
> +		.dev_null_fd	= &dev_null_fd_wr,
> +		.parent_idx	= 0,
> +		.child_idx	= 1,
> +	}, {
> +		.mask		= POPEN_FLAG_FD_STDERR,
> +		.mask_devnull	= POPEN_FLAG_FD_STDERR_DEVNULL,
> +		.mask_close	= POPEN_FLAG_FD_STDERR_CLOSE,
> +		.fileno		= STDERR_FILENO,
> +		.dev_null_fd	= &dev_null_fd_wr,
> +		.parent_idx	= 0,
> +		.child_idx	= 1,
> +	},
> +};
> +
>  /**
>   * Register popen handle in a pids map.
>   */
> @@ -213,7 +250,8 @@ handle_free(struct popen_handle *handle)
>   * Returns 0 if so and -1 otherwise (and set a diag).
>   */
>  static inline int
> -popen_may_io(struct popen_handle *handle, unsigned int io_flags)
> +popen_may_io(struct popen_handle *handle, unsigned int idx,
> +	     unsigned int io_flags, bool allow_closed)
>  {
>  	if (!(io_flags & handle->flags)) {
>  		diag_set(IllegalParams, "popen: handle does not support the "
> @@ -221,6 +259,12 @@ popen_may_io(struct popen_handle *handle, unsigned int io_flags)
>  		return -1;
>  	}
>  
> +       if (! allow_closed && handle->ios[idx].fd < 0) {
> +	       diag_set(IllegalParams, "popen: attempt to operate on a closed "
> +			"file descriptor");
> +               return -1;
> +       }
> +
>  	return 0;
>  }
>  
> @@ -299,6 +343,7 @@ stdX_str(unsigned int index)
>   *   - count: data is too big.
>   *   - flags: POPEN_FLAG_FD_STDIN bit is unset.
>   *   - handle: handle does not support the requested IO operation.
> + *   - handle: attempt to operate on a closed fd.
>   * - SocketError: an IO error occurs at write().
>   * - TimedOut: @a timeout quota is exceeded.
>   * - FiberIsCancelled: cancelled by an outside code.
> @@ -327,11 +372,11 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
>  		return -1;
>  	}
>  
> -	if (popen_may_io(handle, flags) != 0)
> -		return -1;
> -
>  	int idx = STDIN_FILENO;
>  
> +	if (popen_may_io(handle, idx, flags, false) != 0)
> +		return -1;
> +
>  	say_debug("popen: %d: write idx [%s:%d] buf %p count %zu "
>  		  "fds %d timeout %.9g",
>  		  handle->pid, stdX_str(idx), idx, buf, count,
> @@ -362,6 +407,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
>   *   - count: buffer is too big.
>   *   - flags: stdout and stdrr are both choosen or both missed
>   *   - handle: handle does not support the requested IO operation.
> + *   - handle: attempt to operate on a closed fd.
>   * - SocketError: an IO error occurs at read().
>   * - TimedOut: @a timeout quota is exceeded.
>   * - FiberIsCancelled: cancelled by an outside code.
> @@ -390,12 +436,12 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
>  		return -1;
>  	}
>  
> -	if (popen_may_io(handle, flags) != 0)
> -		return -1;
> -
>  	int idx = flags & POPEN_FLAG_FD_STDOUT ?
>  		STDOUT_FILENO : STDERR_FILENO;
>  
> +	if (popen_may_io(handle, idx, flags, false) != 0)
> +		return -1;
> +
>  	say_debug("popen: %d: read idx [%s:%d] buf %p count %zu "
>  		  "fds %d timeout %.9g",
>  		  handle->pid, stdX_str(idx), idx, buf, count,
> @@ -405,6 +451,80 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
>  					    timeout);
>  }
...
> +int
> +popen_shutdown(struct popen_handle *handle, unsigned int flags)
> +{
> +	assert(handle != NULL);
> +
> +	if ((flags & (POPEN_FLAG_FD_STDIN |
> +		      POPEN_FLAG_FD_STDOUT |
> +		      POPEN_FLAG_FD_STDERR)) == 0) {
> +		diag_set(IllegalParams,
> +			 "popen: neither stdin, stdout nor stderr is choosen");
> +		return -1;
> +	}
> +
> +	/* Verify the operation. */
> +	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {

	for (size_t i = 0; i < lengthof(pfd_map); i++)

We already do a build time check for STDIN_x proper mapping to numbers,
lets make it shorter.


> +		/* Operate only on asked fds. */
> +		unsigned int op_mask = pfd_map[idx].mask;
> +		if ((flags & op_mask) == 0)
> +			continue;
> +
> +		if (popen_may_io(handle, idx, op_mask, true) != 0)
> +			return -1;
> +	}
> +
> +	/* Perform the operation. */
> +	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
> +		/* Operate only on asked fds. */
> +		unsigned int op_mask = pfd_map[idx].mask;
> +		if ((flags & op_mask) == 0)
> +			continue;
> +
> +		/* Skip already closed fds. */
> +		if (handle->ios[idx].fd < 0)
> +			continue;
> +
> +		say_debug("popen: %d: shutdown idx [%s:%d] fd %s",
> +			  handle->pid, stdX_str(idx), idx,
> +			  handle->ios[idx].fd);
> +		coio_close_io(loop(), &handle->ios[idx]);
> +	}

I don't get why we need two for() cycles? Also, I don't like that we
mangle popen_may_io(). The shutdown is special. Why not do something like

for (size_t idx = 0; i < lengthof(pfd_map); i++) {
	unsigned int op_mask = pfd_map[idx].mask;
	if ((flags & op_mask) == 0)
		continue;

	if (handle->ios[idx].fd < 0)
		continue;

	...
}

Can't we do something like that?

	Cyrill

Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds

[Alexander Turenko]

> > +int
> > +popen_shutdown(struct popen_handle *handle, unsigned int flags)
> > +{
> > +	assert(handle != NULL);
> > +
> > +	if ((flags & (POPEN_FLAG_FD_STDIN |
> > +		      POPEN_FLAG_FD_STDOUT |
> > +		      POPEN_FLAG_FD_STDERR)) == 0) {
> > +		diag_set(IllegalParams,
> > +			 "popen: neither stdin, stdout nor stderr is choosen");
> > +		return -1;
> > +	}
> > +
> > +	/* Verify the operation. */
> > +	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
> 
> 	for (size_t i = 0; i < lengthof(pfd_map); i++)
> 
> We already do a build time check for STDIN_x proper mapping to numbers,
> lets make it shorter.

Sure, changed.

The diff at bottom of the email.

> 
> 
> > +		/* Operate only on asked fds. */
> > +		unsigned int op_mask = pfd_map[idx].mask;
> > +		if ((flags & op_mask) == 0)
> > +			continue;
> > +
> > +		if (popen_may_io(handle, idx, op_mask, true) != 0)
> > +			return -1;
> > +	}
> > +
> > +	/* Perform the operation. */
> > +	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
> > +		/* Operate only on asked fds. */
> > +		unsigned int op_mask = pfd_map[idx].mask;
> > +		if ((flags & op_mask) == 0)
> > +			continue;
> > +
> > +		/* Skip already closed fds. */
> > +		if (handle->ios[idx].fd < 0)
> > +			continue;
> > +
> > +		say_debug("popen: %d: shutdown idx [%s:%d] fd %s",
> > +			  handle->pid, stdX_str(idx), idx,
> > +			  handle->ios[idx].fd);
> > +		coio_close_io(loop(), &handle->ios[idx]);
> > +	}
> 
> I don't get why we need two for() cycles?

The first loop is to check that we able to close all fds: all checks are
made first, then all close()'s are called. But the first loop actually
may be replaced with the following check:

 | flags &= POPEN_FLAG_FD_STDIN  |
 |          POPEN_FLAG_FD_STDOUT |
 |          POPEN_FLAG_FD_STDERR;
 | if ((handle->flags & flags) != flags)
 |         /* Give an error here. */

It looks more clean, so I'll use this way. Thanks for catching this!

> Also, I don't like that we
> mangle popen_may_io(). The shutdown is special. Why not do something like

We should check for closed fd in popen_may_io(), because read / write
should give proper error in the case. I guess that here we agreed.

But I made this check conditional in popen_may_io() and I guess it is
what you complain about.

I agree that it looks a bit artificial and so I extracted setting of the
'not supported IO operation' error into a helper function and use it in
popen_shutdown() directly. This really looks better.

See the diff below.

> 
> for (size_t idx = 0; i < lengthof(pfd_map); i++) {
> 	unsigned int op_mask = pfd_map[idx].mask;
> 	if ((flags & op_mask) == 0)
> 		continue;
> 
> 	if (handle->ios[idx].fd < 0)
> 		continue;
> 
> 	...
> }
> 
> Can't we do something like that?

Not exactly, but a kind of this. See below.

WBR, Alexander Turenko.

----

Incremental diff:

--- a/src/lib/core/popen.c	2020-04-15 01:44:51.387489580 +0300
+++ b/src/lib/core/popen.c	2020-04-15 01:44:35.118490605 +0300
@@ -245,21 +245,30 @@
 }
 
 /**
+ * Generate an error about IO operation that is not supported by
+ * a popen handle.
+ */
+static inline int
+popen_set_unsupported_io_error(void)
+{
+	diag_set(IllegalParams, "popen: handle does not support the "
+		 "requested IO operation");
+	return -1;
+}
+
+/**
  * Test if the handle can run a requested IO operation.
  *
  * Returns 0 if so and -1 otherwise (and set a diag).
  */
 static inline int
 popen_may_io(struct popen_handle *handle, unsigned int idx,
-	     unsigned int io_flags, bool allow_closed)
+	     unsigned int io_flags)
 {
-	if (!(io_flags & handle->flags)) {
-		diag_set(IllegalParams, "popen: handle does not support the "
-			 "requested IO operation");
-		return -1;
-	}
+	if (!(io_flags & handle->flags))
+		return popen_set_unsupported_io_error();
 
-       if (! allow_closed && handle->ios[idx].fd < 0) {
+	if (handle->ios[idx].fd < 0) {
 		diag_set(IllegalParams, "popen: attempt to operate "
 			 "on a closed file descriptor");
 		return -1;
@@ -374,7 +383,7 @@
 
 	int idx = STDIN_FILENO;
 
-	if (popen_may_io(handle, idx, flags, false) != 0)
+	if (popen_may_io(handle, idx, flags) != 0)
 		return -1;
 
 	say_debug("popen: %d: write idx [%s:%d] buf %p count %zu "
@@ -439,7 +448,7 @@
 	int idx = flags & POPEN_FLAG_FD_STDOUT ?
 		STDOUT_FILENO : STDERR_FILENO;
 
-	if (popen_may_io(handle, idx, flags, false) != 0)
+	if (popen_may_io(handle, idx, flags) != 0)
 		return -1;
 
 	say_debug("popen: %d: read idx [%s:%d] buf %p count %zu "
@@ -486,27 +495,29 @@
 {
 	assert(handle != NULL);
 
-	if ((flags & (POPEN_FLAG_FD_STDIN |
-		      POPEN_FLAG_FD_STDOUT |
-		      POPEN_FLAG_FD_STDERR)) == 0) {
+	/* Ignore irrelevant flags. */
+	flags &= POPEN_FLAG_FD_STDIN	|
+		 POPEN_FLAG_FD_STDOUT	|
+		 POPEN_FLAG_FD_STDERR;
+
+	/* At least one ..._FD_STDx flag should be set. */
+	if (flags == 0) {
 		diag_set(IllegalParams,
 			 "popen: neither stdin, stdout nor stderr is choosen");
 		return -1;
 	}
 
-	/* Verify the operation. */
-	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
-		/* Operate only on asked fds. */
-		unsigned int op_mask = pfd_map[idx].mask;
-		if ((flags & op_mask) == 0)
-			continue;
-
-		if (popen_may_io(handle, idx, op_mask, true) != 0)
-			return -1;
-	}
+	/*
+	 * The handle should have all std*, which are asked to
+	 * close, be piped.
+	 *
+	 * Otherwise the operation has no sense: we should close
+	 * parent's end of a pipe, but it was not created at all.
+	 */
+	if ((handle->flags & flags) != flags)
+		return popen_set_unsupported_io_error();
 
-	/* Perform the operation. */
-	for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
+	for (size_t idx = 0; idx < lengthof(pfd_map); ++idx) {
 		/* Operate only on asked fds. */
 		unsigned int op_mask = pfd_map[idx].mask;
 		if ((flags & op_mask) == 0)

Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds

[Cyrill Gorcunov]

On Wed, Apr 15, 2020 at 07:21:12AM +0300, Alexander Turenko wrote:
> 
> The diff at bottom of the email.

Ack

Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds

[Alexander Turenko]

On Wed, Apr 15, 2020 at 10:43:27AM +0300, Cyrill Gorcunov wrote:
> On Wed, Apr 15, 2020 at 07:21:12AM +0300, Alexander Turenko wrote:
> > 
> > The diff at bottom of the email.
> 
> Ack

Changed after last wording fixes in 'refine popen_{read,write}_timeout
errors' (incremental diff):

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 638826fb9..3a12a439b 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -492,7 +492,7 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
  * Possible errors:
  *
  * - IllegalParams: a parameter check fails:
- *   - flags: neither stdin, stdout nor stderr is choosen.
+ *   - flags: neither stdin, stdout nor stderr is set.
  *   - handle: handle does not support the requested IO operation
  *             (one of fds is not piped).
  */
@@ -509,7 +509,7 @@ popen_shutdown(struct popen_handle *handle, unsigned int flags)
 	/* At least one ..._FD_STDx flag should be set. */
 	if (flags == 0) {
 		diag_set(IllegalParams,
-			 "popen: neither stdin, stdout nor stderr is choosen");
+			 "popen: neither stdin, stdout nor stderr is set");
 		return -1;
 	}

[Tarantool-patches] [PATCH 13/13] popen: add caution comment for popen_may_io()

[Alexander Turenko]

It was easy to misinterpret popen_may_io() contract. In fact, I made
this mistake recently and want to clarify how the function should be
called.

Part of #4031
---
 src/lib/core/popen.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 1b564d0c2..cbf2fd690 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -259,12 +259,19 @@ popen_set_unsupported_io_error(void)
 /**
  * Test if the handle can run a requested IO operation.
  *
+ * NB: Expects @a io_flags to be a ..._FD_STDx flag rather
+ * then a mask with several flags: otherwise it'll check
+ * that one (any) of @a io_flags is set.
+ *
  * Returns 0 if so and -1 otherwise (and set a diag).
  */
 static inline int
 popen_may_io(struct popen_handle *handle, unsigned int idx,
 	     unsigned int io_flags)
 {
+	assert(io_flags == POPEN_FLAG_FD_STDIN	||
+	       io_flags == POPEN_FLAG_FD_STDOUT	||
+	       io_flags == POPEN_FLAG_FD_STDERR);
 	if (!(io_flags & handle->flags))
 		return popen_set_unsupported_io_error();
 
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 13/13] popen: add caution comment for popen_may_io()

[Cyrill Gorcunov]

On Wed, Apr 15, 2020 at 07:25:50AM +0300, Alexander Turenko wrote:
> It was easy to misinterpret popen_may_io() contract. In fact, I made
> this mistake recently and want to clarify how the function should be
> called.
> 
> Part of #4031

Ack

[Tarantool-patches] [PATCH 14/14] popen: fix popen_write_timeout retval type

[Alexander Turenko]

On Linux x86_64 `ssize_t` is 64 bit, while `int` is 32 bit wide (at
least typically). Let's return `ssize_t` from popen_write_timeout() to
prevent data loss.

Part of #4031

Reported-by: Cyrill Gorcunov <gorcunov@gmail.com>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/popen.c | 6 +++---
 src/lib/core/popen.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index cbf2fd690..bb55a80e7 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -370,7 +370,7 @@ stdX_str(unsigned int index)
  * FIXME: Provide an info re amount written bytes in the case.
  *        Say, return -(written) in the case.
  */
-int
+ssize_t
 popen_write_timeout(struct popen_handle *handle, const void *buf,
 		    size_t count, unsigned int flags,
 		    ev_tstamp timeout)
@@ -397,8 +397,8 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
 		  handle->pid, stdX_str(idx), idx, buf, count,
 		  handle->ios[idx].fd, timeout);
 
-	int rc = coio_write_timeout_noxc(&handle->ios[idx], buf, count,
-					 timeout);
+	ssize_t rc = coio_write_timeout_noxc(&handle->ios[idx], buf,
+					     count, timeout);
 	assert(rc < 0 || rc == (ssize_t)count);
 	return rc;
 }
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index cffb53933..4f4a6d99d 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -167,7 +167,7 @@ popen_stat(struct popen_handle *handle, struct popen_stat *st);
 extern const char *
 popen_command(struct popen_handle *handle);
 
-extern int
+extern ssize_t
 popen_write_timeout(struct popen_handle *handle, const void *buf,
 		    size_t count, unsigned int flags,
 		    ev_tstamp timeout);
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2

[Alexander Turenko]

On Tue, Apr 14, 2020 at 02:38:09PM +0300, Alexander Turenko wrote:
> It is the second patchset of preparation patches that are needed to
> implement Lua API for popen.
> 
> See the previous series here (already pushed to master):
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015609.html
> 
> https://github.com/tarantool/tarantool/issues/4031
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2
> 
> I also pushed the same commit to *-full-ci branch to verify it on all
> platforms:
> 
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2-full-ci

CCed Kirill.

Added two patches upward (now the patchset contains 14 patches). Got
ACKs for all patches from Cyrill. Updated the both branches (see above).
GitLab CI looks okay (Travis CI jobs are pending).

Kirill, the patchset seems to be ready to final review and push.

> Alexander Turenko (10):
>   popen: log a reason of close inherited fds failure
>   popen: add missed diag_set() in popen_new()
>   popen: remove retval from popen_stat()
>   popen: quote multiword command arguments
>   popen: add logging of duplicated logger fd
>   popen: fix close-on-exec flag setting
>   popen: clarify popen_{signal,delete} contract
>   popen: add FIXME re group signal flaw
>   popen: clarify popen_read_timeout error message
>   popen: allow to close parent's end of std* fds

Added:

Alexander Turenko (2):
  popen: fix popen_write_timeout retval type
  popen: add caution comment for popen_may_io()

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

Re: [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2

[Alexander Turenko]

Misclicked. Resend it to add Kirill into the recipients list.

----

On Tue, Apr 14, 2020 at 02:38:09PM +0300, Alexander Turenko wrote:
> It is the second patchset of preparation patches that are needed to
> implement Lua API for popen.
> 
> See the previous series here (already pushed to master):
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015609.html
> 
> https://github.com/tarantool/tarantool/issues/4031
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2
> 
> I also pushed the same commit to *-full-ci branch to verify it on all
> platforms:
> 
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2-full-ci

CCed Kirill.

Added two patches upward (now the patchset contains 14 patches). Got
ACKs for all patches from Cyrill. Updated the both branches (see above).
GitLab CI looks okay (Travis CI jobs are pending).

Kirill, the patchset seems to be ready to final review and push.

> Alexander Turenko (10):
>   popen: log a reason of close inherited fds failure
>   popen: add missed diag_set() in popen_new()
>   popen: remove retval from popen_stat()
>   popen: quote multiword command arguments
>   popen: add logging of duplicated logger fd
>   popen: fix close-on-exec flag setting
>   popen: clarify popen_{signal,delete} contract
>   popen: add FIXME re group signal flaw
>   popen: clarify popen_read_timeout error message
>   popen: allow to close parent's end of std* fds

Added:

Alexander Turenko (2):
  popen: fix popen_write_timeout retval type
  popen: add caution comment for popen_may_io()

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

Re: [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2

[Cyrill Gorcunov]

On Thu, Apr 16, 2020 at 02:57:13AM +0300, Alexander Turenko wrote:
> CCed Kirill.
> 
> Added two patches upward (now the patchset contains 14 patches). Got
> ACKs for all patches from Cyrill. Updated the both branches (see above).
> GitLab CI looks okay (Travis CI jobs are pending).
> 
> Kirill, the patchset seems to be ready to final review and push.

Yes, I think it is about a time to merge it up.

Re: [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2

[Kirill Yukhin]

Hello,

On 14 апр 14:38, Alexander Turenko wrote:
> It is the second patchset of preparation patches that are needed to
> implement Lua API for popen.
> 
> See the previous series here (already pushed to master):
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015609.html
> 
> https://github.com/tarantool/tarantool/issues/4031
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2
> 
> I also pushed the same commit to *-full-ci branch to verify it on all
> platforms:
> 
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2-full-ci
> 
> Alexander Turenko (10):
>   popen: log a reason of close inherited fds failure
>   popen: add missed diag_set() in popen_new()
>   popen: remove retval from popen_stat()
>   popen: quote multiword command arguments
>   popen: add logging of duplicated logger fd
>   popen: fix close-on-exec flag setting
>   popen: clarify popen_{signal,delete} contract
>   popen: add FIXME re group signal flaw
>   popen: clarify popen_read_timeout error message
>   popen: allow to close parent's end of std* fds
> 
> Cyrill Gorcunov (2):
>   popen: allow to kill process group
>   popen: add ability to keep child on deletion
> 
>  src/lib/core/popen.c | 305 +++++++++++++++++++++++++++++++++----------
>  src/lib/core/popen.h |  17 ++-
>  2 files changed, 253 insertions(+), 69 deletions(-)

I've checked your patch into master.

--
Regards, Kirill Yukhin