[Tarantool-patches] [PATCH 0/7] popen: various fixes and a test

[Tarantool-patches] [PATCH 0/7] popen: various fixes and a test

[Cyrill Gorcunov]

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

[Tarantool-patches] [PATCH 1/7] core/say: Export logger fd

[Cyrill Gorcunov]

We need it inside popen engine to be able to print
errors from inside of a child process.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/say.c | 6 ++++++
 src/lib/core/say.h | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index 64a637c58..dd05285a6 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -168,6 +168,12 @@ log_type()
 	return log_default->type;
 }
 
+int
+log_get_fd(void)
+{
+	return log_default->fd;
+}
+
 void
 log_set_level(struct log *log, enum say_level level)
 {
diff --git a/src/lib/core/say.h b/src/lib/core/say.h
index d26c3ddef..e17de659c 100644
--- a/src/lib/core/say.h
+++ b/src/lib/core/say.h
@@ -144,6 +144,7 @@ typedef int (*log_format_func_t)(struct log *log, char *buf, int len, int level,
  * A log object. There is a singleton for the default log.
  */
 struct log {
+	/** The current file descriptor. */
 	int fd;
 	/** The current log level. */
 	int level;
@@ -202,6 +203,12 @@ log_say(struct log *log, int level, const char *filename,
 enum say_logger_type
 log_type();
 
+/**
+ * Default logger file descriptor.
+ */
+int
+log_get_fd(void);
+
 /**
  * Set log level. Can be used dynamically.
  *
-- 
2.20.1

[Tarantool-patches] [PATCH 2/7] popen: allow accessing environ variable

[Cyrill Gorcunov]

This is part of posix standart.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/popen.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 1cfe58ee0..0e2d9dd00 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -591,9 +591,7 @@ close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
 	return 0;
 }
 
-#ifdef TARGET_OS_LINUX
 extern char **environ;
-#endif
 
 /**
  * Get pointer to environment variables to use in
@@ -603,17 +601,8 @@ static inline char **
 get_envp(struct popen_opts *opts)
 {
 	if (!opts->env) {
-#ifdef TARGET_OS_LINUX
 		/* Inherit existing ones if not specified */
 		return environ;
-#else
-		static const char **empty_envp[] = { NULL };
-		static bool said = false;
-		if (!said)
-			say_warn("popen: Environment inheritance "
-				 "unsupported, passing empty");
-		return (char **)empty_envp;
-#endif
 	}
 	return opts->env;
 }
-- 
2.20.1

[Tarantool-patches] [PATCH 3/7] popen: close_inherited_fds - add support for macos/freebsd

[Cyrill Gorcunov]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/popen.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 0e2d9dd00..ce7d7fff7 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -529,8 +529,11 @@ make_pipe(int pfd[2])
 static int
 close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
 {
-#ifdef TARGET_OS_LINUX
+# if defined(TARGET_OS_LINUX)
 	static const char path[] = "/proc/self/fd";
+# else
+	static const char path[] = "/dev/fd";
+# endif
 	struct dirent *de;
 	int fd_no, fd_dir;
 	DIR *dir;
@@ -577,17 +580,6 @@ close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
 		diag_set(SystemError, "fdin: Can't close %s", path);
 		return -1;
 	}
-#else
-	/* FIXME: What about FreeBSD/MachO? */
-	(void)skip_fds;
-	(void)nr_skip_fds;
-
-	static bool said = false;
-	if (!said) {
-		say_warn("popen: fdin: Skip closing inherited");
-		said = true;
-	}
-#endif
 	return 0;
 }
 
-- 
2.20.1

[Tarantool-patches] [PATCH 4/7] popen: log errors if popen creation failed

[Cyrill Gorcunov]

On error path we arm diag with error but strictly
speaking some users (such as unit tests) do not have
to access diag for logging.

Thus log it explicitly for debug sake.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 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 ce7d7fff7..806d004bf 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -948,6 +948,7 @@ popen_new(struct popen_opts *opts)
 	return handle;
 
 out_err:
+	diag_log();
 	saved_errno = errno;
 	popen_delete(handle);
 	for (i = 0; i < lengthof(pfd); i++) {
-- 
2.20.1

[Tarantool-patches] [PATCH 5/7] popen: add logging in child process

[Cyrill Gorcunov]

This helps to identify if something gone
wrong inside a child process.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/popen.c | 57 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 12 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 806d004bf..6e5ca21bd 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -623,13 +623,17 @@ signal_reset(void)
 	    sigaction(SIGHUP, &sa, NULL) == -1 ||
 	    sigaction(SIGWINCH, &sa, NULL) == -1 ||
 	    sigaction(SIGSEGV, &sa, NULL) == -1 ||
-	    sigaction(SIGFPE, &sa, NULL) == -1)
+	    sigaction(SIGFPE, &sa, NULL) == -1) {
+		say_error("child: sigaction failed");
 		_exit(errno);
+	}
 
 	/* Unblock any signals blocked by libev */
 	sigfillset(&sigset);
-	if (sigprocmask(SIG_UNBLOCK, &sigset, NULL) == -1)
+	if (sigprocmask(SIG_UNBLOCK, &sigset, NULL) == -1) {
+		say_error("child: SIG_UNBLOCK failed");
 		_exit(errno);
+	}
 }
 
 /**
@@ -725,9 +729,10 @@ popen_new(struct popen_opts *opts)
 	};
 	/*
 	 * At max we could be skipping each pipe end
-	 * plus dev/null variants.
+	 * plus dev/null variants and logfd
 	 */
-	int skip_fds[POPEN_FLAG_FD_STDEND_BIT * 2 + 2];
+	int skip_fds[POPEN_FLAG_FD_STDEND_BIT * 2 + 2 + 1];
+	int log_fd = log_get_fd();
 	size_t nr_skip_fds = 0;
 
 	/*
@@ -755,6 +760,8 @@ popen_new(struct popen_opts *opts)
 	if (!handle)
 		return NULL;
 
+	if (log_fd >= 0)
+		skip_fds[nr_skip_fds++] = log_fd;
 	skip_fds[nr_skip_fds++] = dev_null_fd_ro;
 	skip_fds[nr_skip_fds++] = dev_null_fd_wr;
 	assert(nr_skip_fds <= lengthof(skip_fds));
@@ -831,13 +838,17 @@ popen_new(struct popen_opts *opts)
 		 * won't be able to kill a group of children.
 		 */
 		if (opts->flags & POPEN_FLAG_SETSID) {
-			if (setsid() == -1)
+			if (setsid() == -1) {
+				say_syserror("child: setsid failed");
 				goto exit_child;
+			}
 		}
 
 		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)) {
+				say_syserror("child: close inherited fds");
 				goto exit_child;
+			}
 		}
 
 		for (i = 0; i < lengthof(pfd_map); i++) {
@@ -849,13 +860,19 @@ popen_new(struct popen_opts *opts)
 				int child_idx = pfd_map[i].child_idx;
 
 				/* put child peer end at known place */
-				if (dup2(pfd[i][child_idx], fileno) < 0)
+				if (dup2(pfd[i][child_idx], fileno) < 0) {
+					say_syserror("child: dup %d -> %d",
+						     pfd[i][child_idx], fileno);
 					goto exit_child;
+				}
 
 				/* parent's pipe no longer needed */
 				if (close(pfd[i][0]) ||
-				    close(pfd[i][1]))
+				    close(pfd[i][1])) {
+					say_syserror("child: close %d %d",
+						     pfd[i][0], pfd[i][1]);
 					goto exit_child;
+				}
 				continue;
 			}
 
@@ -863,8 +880,10 @@ popen_new(struct popen_opts *opts)
 			 * Use /dev/null if requested.
 			 */
 			if (opts->flags & pfd_map[i].mask_devnull) {
-				if (dup2(*pfd_map[i].dev_null_fd,
-					 fileno) < 0) {
+				if (dup2(*pfd_map[i].dev_null_fd, fileno) < 0) {
+					say_syserror("child: dup2 %d -> %d",
+						     *pfd_map[i].dev_null_fd,
+						     fileno);
 					goto exit_child;
 				}
 				continue;
@@ -877,8 +896,11 @@ popen_new(struct popen_opts *opts)
 			 * EBADF happens.
 			 */
 			if (opts->flags & pfd_map[i].mask_close) {
-				if (close(fileno) && errno != EBADF)
+				if (close(fileno) && errno != EBADF) {
+					say_syserror("child: can't close %d",
+						     fileno);
 					goto exit_child;
+				}
 				continue;
 			}
 
@@ -888,8 +910,19 @@ popen_new(struct popen_opts *opts)
 			 */
 		}
 
-		if (close(dev_null_fd_ro) || close(dev_null_fd_wr))
+		if (close(dev_null_fd_ro) || close(dev_null_fd_wr)) {
+			say_error("child: can't close %d or %d",
+				  dev_null_fd_ro, dev_null_fd_wr);
 			goto exit_child;
+		}
+
+		if (log_fd >= 0) {
+			if (close(log_fd)) {
+				say_error("child: can't close logfd %d",
+					  log_fd);
+				goto exit_child;
+			}
+		}
 
 		if (opts->flags & POPEN_FLAG_SHELL)
 			execve(_PATH_BSHELL, opts->argv, envp);
-- 
2.20.1

[Tarantool-patches] [PATCH 6/7] popen: handle setsid os specifics

[Cyrill Gorcunov]

On linux it is fine to call setsid right after
the vfork, in turn on bsd pgrp should be used.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 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 6e5ca21bd..d41e9cfce 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -839,8 +839,15 @@ popen_new(struct popen_opts *opts)
 		 */
 		if (opts->flags & POPEN_FLAG_SETSID) {
 			if (setsid() == -1) {
+#ifndef TARGET_OS_DARWIN
 				say_syserror("child: setsid failed");
 				goto exit_child;
+#else
+				if (setpgrp() == -1) {
+					say_syserror("child: setpgrp failed");
+					goto exit_child;
+				}
+#endif
 			}
 		}
 
-- 
2.20.1

[Tarantool-patches] [PATCH 7/7] test/unit: add popen test

[Cyrill Gorcunov]

Basic test for popen engine

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/unit/CMakeLists.txt |   3 +
 test/unit/popen.c        | 250 +++++++++++++++++++++++++++++++++++++++
 test/unit/popen.result   |  25 ++++
 3 files changed, 278 insertions(+)
 create mode 100644 test/unit/popen.c
 create mode 100644 test/unit/popen.result

diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index c037ac539..4ac08de8d 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -240,3 +240,6 @@ target_link_libraries(swim_errinj.test unit swim)
 
 add_executable(merger.test merger.test.c)
 target_link_libraries(merger.test unit core box)
+
+add_executable(popen.test popen.c)
+target_link_libraries(popen.test misc unit core)
diff --git a/test/unit/popen.c b/test/unit/popen.c
new file mode 100644
index 000000000..7656ebe73
--- /dev/null
+++ b/test/unit/popen.c
@@ -0,0 +1,250 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "trivia/util.h"
+#include "unit.h"
+
+#include "coio.h"
+#include "coio_task.h"
+#include "memory.h"
+#include "fiber.h"
+#include "popen.h"
+#include "say.h"
+
+#define TEST_POPEN_COMMON_FLAGS			\
+	(POPEN_FLAG_SETSID		|	\
+	POPEN_FLAG_SHELL		|	\
+	POPEN_FLAG_RESTORE_SIGNALS)
+
+static int
+wait_exit(struct popen_handle *handle, int *state, int *exit_code)
+{
+	for (;;) {
+		if (popen_state(handle, state, exit_code))
+			return -1;
+		if (*state == POPEN_STATE_EXITED ||
+		    *state == POPEN_STATE_SIGNALED)
+			break;
+		fiber_sleep(0.1);
+	}
+	return 0;
+}
+
+static int
+popen_write_exit(void)
+{
+	struct popen_handle *handle;
+	char *child_argv[] = {
+		"/bin/sh", "-c",
+		"prompt=''; read -n 5 prompt; echo $prompt",
+		NULL,
+	};
+
+	const char data[] = "12345";
+	int state, exit_code;
+
+	struct popen_opts opts = {
+		.argv		= child_argv,
+		.nr_argv	= lengthof(child_argv),
+		.env		= NULL,
+		.flags		=
+			POPEN_FLAG_FD_STDIN		|
+			POPEN_FLAG_FD_STDOUT		|
+			POPEN_FLAG_FD_STDERR		|
+			TEST_POPEN_COMMON_FLAGS,
+	};
+	int rc;
+
+	plan(7);
+	header();
+
+	handle = popen_new(&opts);
+	ok(handle != NULL, "popen_new");
+	if (handle == NULL)
+		goto out;
+
+	rc = popen_state(handle, &state, &exit_code);
+	ok(rc == 0, "popen_state");
+
+	ok(state == POPEN_STATE_ALIVE, "state %s",
+	   popen_state_str(state));
+
+	rc = popen_write_timeout(handle, (void *)data,
+				 (int)strlen(data),
+				 POPEN_FLAG_FD_STDOUT, 180);
+	ok(rc == -1, "write flag check");
+
+	rc = popen_write_timeout(handle, (void *)data,
+				 (int)strlen(data),
+				 POPEN_FLAG_FD_STDIN, 180);
+	ok(rc == (int)strlen(data), "write to pipe");
+	if (rc != (int)strlen(data))
+		goto out_kill;
+
+	rc = wait_exit(handle, &state, &exit_code);
+	if (rc) {
+		ok(false, "child wait");
+		goto out_kill;
+	}
+
+	ok(state == POPEN_STATE_EXITED, "child exited");
+
+out_kill:
+	rc = popen_delete(handle);
+	ok(rc == 0, "popen_delete");
+
+out:
+	footer();
+	return check_plan();
+}
+
+static int
+popen_read_exit(void)
+{
+	struct popen_handle *handle;
+	char *child_argv[] = {
+		"/bin/sh", "-c",
+		"echo 1 2 3 4 5",
+		NULL,
+	};
+
+	int state, exit_code;
+	char data[32] = { };
+
+	struct popen_opts opts = {
+		.argv		= child_argv,
+		.nr_argv	= lengthof(child_argv),
+		.env		= NULL,
+		.flags		=
+			POPEN_FLAG_FD_STDIN		|
+			POPEN_FLAG_FD_STDOUT		|
+			POPEN_FLAG_FD_STDERR		|
+			TEST_POPEN_COMMON_FLAGS,
+	};
+	int rc;
+
+	plan(5);
+	header();
+
+	handle = popen_new(&opts);
+	ok(handle != NULL, "popen_new");
+	if (handle == NULL)
+		goto out;
+
+	rc = wait_exit(handle, &state, &exit_code);
+	if (rc) {
+		ok(false, "child wait");
+		goto out_kill;
+	}
+	ok(state == POPEN_STATE_EXITED, "child exited");
+
+	rc = popen_read_timeout(handle, data, sizeof(data),
+				POPEN_FLAG_FD_STDIN, 180);
+	ok(rc == -1, "read flag check");
+
+	rc = popen_read_timeout(handle, data, sizeof(data),
+				POPEN_FLAG_FD_STDOUT, 180);
+	ok(rc >= 9 && !strncmp(data, "1 2 3 4 5", 9), "read from pipe");
+
+out_kill:
+	rc = popen_delete(handle);
+	ok(rc == 0, "popen_delete");
+
+out:
+	footer();
+	return check_plan();
+}
+
+static int
+popen_kill(void)
+{
+	struct popen_handle *handle;
+	char *child_argv[] = {
+		"/bin/sh", "-c",
+		"while [ 1 ]; do sleep 10; done",
+		NULL,
+	};
+
+	int state, exit_code;
+
+	struct popen_opts opts = {
+		.argv		= child_argv,
+		.nr_argv	= lengthof(child_argv),
+		.env		= NULL,
+		.flags		=
+			POPEN_FLAG_FD_STDIN		|
+			POPEN_FLAG_FD_STDOUT		|
+			POPEN_FLAG_FD_STDERR		|
+			TEST_POPEN_COMMON_FLAGS,
+	};
+	int rc;
+
+	plan(4);
+	header();
+
+	handle = popen_new(&opts);
+	ok(handle != NULL, "popen_new");
+	if (handle == NULL)
+		goto out;
+
+	rc = popen_send_signal(handle, SIGTERM);
+	ok(rc == 0, "popen_send_signal");
+	if (rc != 0)
+		goto out_kill;
+
+	rc = wait_exit(handle, &state, &exit_code);
+	if (rc) {
+		ok(false, "child wait");
+		goto out_kill;
+	}
+	ok(state == POPEN_STATE_SIGNALED, "child terminated");
+
+out_kill:
+	rc = popen_delete(handle);
+	ok(rc == 0, "popen_delete");
+
+out:
+	footer();
+	return check_plan();
+}
+
+static int
+main_f(va_list ap)
+{
+	int rc = 0;
+
+	rc = popen_write_exit();
+	if (rc == 0)
+		rc = popen_read_exit();
+	if (rc == 0)
+		rc = popen_kill();
+
+	ev_break(loop(), EVBREAK_ALL);
+	return 0;
+}
+
+int
+main(int argc, char *argv[])
+{
+	//say_logger_init(NULL, S_DEBUG, 0, "plain", 0);
+	memory_init();
+
+	fiber_init(fiber_c_invoke);
+	popen_init();
+	coio_init();
+	coio_enable();
+
+	if (!loop())
+		panic("%s", "can't init event loop");
+
+	struct fiber *test = fiber_new("coio_stat", main_f);
+	fiber_wakeup(test);
+
+	ev_now_update(loop());
+	ev_run(loop(), 0);
+	popen_free();
+	fiber_free();
+	memory_free();
+
+	return 0;
+}
diff --git a/test/unit/popen.result b/test/unit/popen.result
new file mode 100644
index 000000000..14fd3e43e
--- /dev/null
+++ b/test/unit/popen.result
@@ -0,0 +1,25 @@
+1..7
+	*** popen_write_exit ***
+ok 1 - popen_new
+ok 2 - popen_state
+ok 3 - state alive
+ok 4 - write flag check
+ok 5 - write to pipe
+ok 6 - child exited
+ok 7 - popen_delete
+	*** popen_write_exit: done ***
+1..5
+	*** popen_read_exit ***
+ok 1 - popen_new
+ok 2 - child exited
+ok 3 - read flag check
+ok 4 - read from pipe
+ok 5 - popen_delete
+	*** popen_read_exit: done ***
+1..4
+	*** popen_kill ***
+ok 1 - popen_new
+ok 2 - popen_send_signal
+ok 3 - child terminated
+ok 4 - popen_delete
+	*** popen_kill: done ***
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 6/7] popen: handle setsid os specifics

[Alexander Turenko]

On Mon, Mar 02, 2020 at 11:12:26PM +0300, Cyrill Gorcunov wrote:
> On linux it is fine to call setsid right after
> the vfork, in turn on bsd pgrp should be used.

Can you refer a source of this information?

I tried the following snippet on Mac OS and FreeBSD and it at least does
not report an error:

 | #include <unistd.h>
 | #include <stdio.h>
 | 
 | int
 | main()
 | {
 | 	if (setsid() == -1) {
 | 		perror("setsid");
 | 		return 1;
 | 	}
 | 	return 0;
 | }

$ cc setsid.c -o setsid
$ echo 1 | ./setsid

(Pipeline is to not be a session leader already, this will lead to EPERM.)

> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  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 6e5ca21bd..d41e9cfce 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -839,8 +839,15 @@ popen_new(struct popen_opts *opts)
>  		 */
>  		if (opts->flags & POPEN_FLAG_SETSID) {
>  			if (setsid() == -1) {
> +#ifndef TARGET_OS_DARWIN
>  				say_syserror("child: setsid failed");
>  				goto exit_child;
> +#else
> +				if (setpgrp() == -1) {
> +					say_syserror("child: setpgrp failed");
> +					goto exit_child;
> +				}
> +#endif
>  			}
>  		}
>  
> -- 
> 2.20.1
> 

Re: [Tarantool-patches] [PATCH 6/7] popen: handle setsid os specifics

[Cyrill Gorcunov]

On Tue, Mar 03, 2020 at 02:38:53PM +0300, Alexander Turenko wrote:
> On Mon, Mar 02, 2020 at 11:12:26PM +0300, Cyrill Gorcunov wrote:
> > On linux it is fine to call setsid right after
> > the vfork, in turn on bsd pgrp should be used.
> 
> Can you refer a source of this information?
> 
> I tried the following snippet on Mac OS and FreeBSD and it at least does
> not report an error:
> 
>  | #include <unistd.h>
>  | #include <stdio.h>
>  | 
>  | int
>  | main()
>  | {
>  | 	if (setsid() == -1) {
>  | 		perror("setsid");
>  | 		return 1;
>  | 	}
>  | 	return 0;
>  | }
> 
> $ cc setsid.c -o setsid
> $ echo 1 | ./setsid
> 
> (Pipeline is to not be a session leader already, this will lead to EPERM.)

I found that people are hitting the same problem.
https://stackoverflow.com/questions/15179361/setpgrp-setpgid-fails-works-on-mac-osx-not-on-linux

The main problem is that I don't have a clue about macos internals
but it looks like we have to setup group instead. To me it is
vague area. I suspect it is due to vfork macos specifics.

[Tarantool-patches] [PATCH v2 6/7] popen: handle setsid os specifics

[Cyrill Gorcunov]

Due to os specifics we can't call setsid after
vfork on macos (vfork is not longer a part of
posix btw). Thus lets simply ignore it.

If there gonna be a strong need to drop
controlling terminal (we could use ioctl)
or setup new group we will enhance it.
But only if really needed.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

I pushed the new series into
gorcunov/gh-4031-popen-fixup-2

 src/lib/core/popen.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 6e5ca21bd..40da05b52 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -833,9 +833,11 @@ popen_new(struct popen_opts *opts)
 		if (opts->flags & POPEN_FLAG_RESTORE_SIGNALS)
 			signal_reset();
 
+#ifndef TARGET_OS_DARWIN
 		/*
-		 * We have to be a session leader otherwise
-		 * won't be able to kill a group of children.
+		 * Note that on MacOS we're not allowed to
+		 * set sid after vfork (it is OS specific)
+		 * thus simply ignore this flag.
 		 */
 		if (opts->flags & POPEN_FLAG_SETSID) {
 			if (setsid() == -1) {
@@ -843,6 +845,7 @@ popen_new(struct popen_opts *opts)
 				goto exit_child;
 			}
 		}
+#endif
 
 		if (opts->flags & POPEN_FLAG_CLOSE_FDS) {
 			if (close_inherited_fds(skip_fds, nr_skip_fds)) {
-- 
2.20.1

[Tarantool-patches] [PATCH v3 6/7] popen: use ioctl on macos

[Cyrill Gorcunov]

Due to os specifics we can't call setsid after
vfork on macos (vfork is not longer a part of
posix btw). Instead we can use ioctl to clear
the session.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
branch gorcunov/gh-4031-popen-fixup-3

Sasha, I do not send the whole series since the only
this particular patch is changed. Thus if needed we
can merge from the branch above.

 src/lib/core/popen.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 6e5ca21bd..cc3d2d946 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -16,6 +16,10 @@
 #include "coio.h"
 #include "say.h"
 
+#ifdef TARGET_OS_DARWIN
+# include <sys/ioctl.h>
+#endif
+
 /* A mapping to find popens by their pids in a signal handler */
 static struct mh_i32ptr_t *popen_pids_map = NULL;
 
@@ -833,15 +837,24 @@ popen_new(struct popen_opts *opts)
 		if (opts->flags & POPEN_FLAG_RESTORE_SIGNALS)
 			signal_reset();
 
-		/*
-		 * We have to be a session leader otherwise
-		 * won't be able to kill a group of children.
-		 */
 		if (opts->flags & POPEN_FLAG_SETSID) {
+#ifndef TARGET_OS_DARWIN
 			if (setsid() == -1) {
 				say_syserror("child: setsid failed");
 				goto exit_child;
 			}
+#else
+			/*
+			 * Note that on MacOS we're not allowed to
+			 * set sid after vfork (it is OS specific)
+			 * thus use ioctl instead.
+			 */
+			int ttyfd = open("/dev/tty", O_RDWR, 0);
+			if (ttyfd >= 0) {
+				ioctl(ttyfd, TIOCNOTTY, 0);
+				close(ttyfd);
+			}
+#endif
 		}
 
 		if (opts->flags & POPEN_FLAG_CLOSE_FDS) {
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 6/7] popen: handle setsid os specifics

[Alexander Turenko]

On Tue, Mar 03, 2020 at 02:45:37PM +0300, Cyrill Gorcunov wrote:
> On Tue, Mar 03, 2020 at 02:38:53PM +0300, Alexander Turenko wrote:
> > On Mon, Mar 02, 2020 at 11:12:26PM +0300, Cyrill Gorcunov wrote:
> > > On linux it is fine to call setsid right after
> > > the vfork, in turn on bsd pgrp should be used.
> > 
> > Can you refer a source of this information?
> > 
> > I tried the following snippet on Mac OS and FreeBSD and it at least does
> > not report an error:
> > 
> >  | #include <unistd.h>
> >  | #include <stdio.h>
> >  | 
> >  | int
> >  | main()
> >  | {
> >  | 	if (setsid() == -1) {
> >  | 		perror("setsid");
> >  | 		return 1;
> >  | 	}
> >  | 	return 0;
> >  | }
> > 
> > $ cc setsid.c -o setsid
> > $ echo 1 | ./setsid
> > 
> > (Pipeline is to not be a session leader already, this will lead to EPERM.)
> 
> I found that people are hitting the same problem.
> https://stackoverflow.com/questions/15179361/setpgrp-setpgid-fails-works-on-mac-osx-not-on-linux
> 
> The main problem is that I don't have a clue about macos internals
> but it looks like we have to setup group instead. To me it is
> vague area. I suspect it is due to vfork macos specifics.

We discussed this with Cyrill and the result is the following.

setsid() actually does not work on Mac OS after vfork() (however works
after fork()):

 | #include <sys/types.h>
 | #include <sys/wait.h>
 | #include <unistd.h>
 | #include <stdio.h>
 | 
 | int
 | main()
 | {
 | 	pid_t pid;
 | 	if ((pid = vfork()) == 0) {
 | 		/* Child. */
 | 		if (setsid() == -1) {
 | 			perror("setsid");
 | 			_exit(1);
 | 		}
 | 		_exit(0);
 | 	}
 | 	/* Parent. */
 | 	int status;
 | 	waitpid(pid, &status, 0);
 | 	return 0;
 | }

Compile and run it on Mac OS:

 | $ cc vfork_setsid.c -o vfork_setsid
 | $ ./vfork_setsid
 | setsid: Operation not permitted

It seems that setsid() is used mainly to disassociate from a controlling
terminal (to don't be hit by SIGHUP if it'll die). In this context
setpgrp() would not be sufficient.

I found that it is possible to use ioctl(<tty fd>, TIOCNOTTY, 0) to
disassociate from a controlling terminal. It will not move a child
process to the new session, but it seems that just don't being hit by
SIGHUP is everything that a user want.

So I would pick up this way as the workaround.

My sources:

[1]: https://github.com/emacs-mirror/emacs/commit/a13eaddce2ddbe3ba0b7f4c81715bc0fcdba99f6
[2]: https://github.com/emacs-mirror/emacs/commit/9cd23a29147acb86c860ce11febe24cf837f3f8a
[3]: http://man7.org/linux/man-pages/man4/tty.4.html
[4]: https://stackoverflow.com/a/8777697/1598057
[5]: https://reviews.freebsd.org/D22572

Cyrill already sent the fix, but I decided to summarize our discussion
anyway.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH 6/7] popen: handle setsid os specifics

[Alexander Turenko]

> It seems that setsid() is used mainly to disassociate from a controlling
> terminal (to don't be hit by SIGHUP if it'll die). In this context
> setpgrp() would not be sufficient.

I just realized that there is another reason to use setsid(), where
setpgrp() is applicable too: move the child into its own process group
and kill the whole group (child and its childs if any) then. I mean, use
the corresponding flag (which I proposed to add in [1]), which will
change :kill() behaviour.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/014608.html

So, it seems, we should do ioctl() + setpgrp() on Mac OS?

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH 6/7] popen: handle setsid os specifics

[Cyrill Gorcunov]

On Tue, Mar 10, 2020 at 07:36:46PM +0300, Alexander Turenko wrote:
> > It seems that setsid() is used mainly to disassociate from a controlling
> > terminal (to don't be hit by SIGHUP if it'll die). In this context
> > setpgrp() would not be sufficient.
> 
> I just realized that there is another reason to use setsid(), where
> setpgrp() is applicable too: move the child into its own process group
> and kill the whole group (child and its childs if any) then. I mean, use
> the corresponding flag (which I proposed to add in [1]), which will
> change :kill() behaviour.
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/014608.html
> 
> So, it seems, we should do ioctl() + setpgrp() on Mac OS?

The child may generate subchildren with own groups. If I'm not missing
something obvious we should provide only basic functionality whic would
be enough to spawn new processes. The child may generate subchildren
with own group, serisouly without pid namespace we simply do not control
much. Thus I propose to leave it in the state it is right now.

Re: [Tarantool-patches] [PATCH 6/7] popen: handle setsid os specifics

[Alexander Turenko]

On Tue, Mar 10, 2020 at 07:41:00PM +0300, Cyrill Gorcunov wrote:
> On Tue, Mar 10, 2020 at 07:36:46PM +0300, Alexander Turenko wrote:
> > > It seems that setsid() is used mainly to disassociate from a controlling
> > > terminal (to don't be hit by SIGHUP if it'll die). In this context
> > > setpgrp() would not be sufficient.
> > 
> > I just realized that there is another reason to use setsid(), where
> > setpgrp() is applicable too: move the child into its own process group
> > and kill the whole group (child and its childs if any) then. I mean, use
> > the corresponding flag (which I proposed to add in [1]), which will
> > change :kill() behaviour.
> > 
> > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/014608.html
> > 
> > So, it seems, we should do ioctl() + setpgrp() on Mac OS?
> 
> The child may generate subchildren with own groups. If I'm not missing
> something obvious we should provide only basic functionality whic would
> be enough to spawn new processes. The child may generate subchildren
> with own group, serisouly without pid namespace we simply do not control
> much. Thus I propose to leave it in the state it is right now.

We don't intend to offer some kind of guaranteed isolation. A child may
spawn a daemon, yep. But we usually know what we're run and whether it
will do something like this.

We should provide a tool to do the following:

* Allow to spawn a background process, which will not die even if a
  controlling terminal died (setsid() or ioctl() solve it).
* Allow to set a new process group for a process and its childs (say,
  for a non-inveractive shell) in order to be able to kill the entire
  group if we're going to free resources (setsid() or setpgrp() solve
  it).

Usual case for the latter bullet: spawn a pipeline using "sh -c 'foo |
bar'" and be able to kill it entirely. A non-interactive shell does not
perform job control, so even "bash -c 'foo & bar'" will be killed
entirely in the case.

After looking around discussions re other popen implementations, I guess
that we'll be asked for those abilities sooner or later. It seems
logical to implement it, since we know it is expected from a popen
implementation.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH 6/7] popen: handle setsid os specifics

[Cyrill Gorcunov]

On Tue, Mar 10, 2020 at 08:12:52PM +0300, Alexander Turenko wrote:
> 
> After looking around discussions re other popen implementations, I guess
> that we'll be asked for those abilities sooner or later. It seems
> logical to implement it, since we know it is expected from a popen
> implementation.

OK, convinced. Will do.

[Tarantool-patches] [PATCH v5 6/7] popen: handle sid on macos

[Cyrill Gorcunov]

Due to os specifics we can't call setsid after vfork on macos
(vfork is not longer a part of posix btw). Instead we can use
ioctl to clear the session, then initiate a new process group.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
branch gorcunov/gh-4031-popen-fixup-4

 src/lib/core/popen.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 6e5ca21bd..a0630e3d9 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -16,6 +16,10 @@
 #include "coio.h"
 #include "say.h"
 
+#ifdef TARGET_OS_DARWIN
+# include <sys/ioctl.h>
+#endif
+
 /* A mapping to find popens by their pids in a signal handler */
 static struct mh_i32ptr_t *popen_pids_map = NULL;
 
@@ -833,15 +837,29 @@ popen_new(struct popen_opts *opts)
 		if (opts->flags & POPEN_FLAG_RESTORE_SIGNALS)
 			signal_reset();
 
-		/*
-		 * We have to be a session leader otherwise
-		 * won't be able to kill a group of children.
-		 */
 		if (opts->flags & POPEN_FLAG_SETSID) {
+#ifndef TARGET_OS_DARWIN
 			if (setsid() == -1) {
 				say_syserror("child: setsid failed");
 				goto exit_child;
 			}
+#else
+			/*
+			 * Note that on MacOS we're not allowed to
+			 * set sid after vfork (it is OS specific)
+			 * thus use ioctl instead.
+			 */
+			int ttyfd = open("/dev/tty", O_RDWR, 0);
+			if (ttyfd >= 0) {
+				ioctl(ttyfd, TIOCNOTTY, 0);
+				close(ttyfd);
+			}
+
+			if (setpgrp() == -1) {
+				say_syserror("child: setpgrp failed");
+				goto exit_child;
+			}
+#endif
 		}
 
 		if (opts->flags & POPEN_FLAG_CLOSE_FDS) {
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 7/7] test/unit: add popen test

[Alexander Turenko]

There are two good properties, which can be easily achieved for the
test:

* better fit TAP13 format;
* set a non-zero exit code at fail.

See below (comments 2 and 4) if you're interested or skip if not.

There also two comments around (1 and 3), see them below too.

WBR, Alexander Turenko.

> +static int
> +main_f(va_list ap)
> +{
> +	int rc = 0;
> +
> +	rc = popen_write_exit();
> +	if (rc == 0)
> +		rc = popen_read_exit();
> +	if (rc == 0)
> +		rc = popen_kill();

1. I would run all tests even if some of them fails: this way we can get
   more info what is healthy and what is broken. And it is usual for
   TAP13 tests as I see.

> +
> +	ev_break(loop(), EVBREAK_ALL);
> +	return 0;
> +}

2. Since our unit.[ch] module supports test nesting I would start the
   function from plan() + header() prologue and end it with footer() +
   check_plan() epilogue. See swim.c or uri.c unit tests for example.

   This will make the output almost TAP13 compliant (now it contains
   several plans on one nesting level) and I hope we'll make it fully
   compliant by a change in unit.[ch] and remove most of *.result files
   in test/unit directory.

   I propose also save the check_plan() return value in a global static
   variable like in swim.c unit test to report it in the exit code of
   the process.

> +int
> +main(int argc, char *argv[])
> +{
> +	//say_logger_init(NULL, S_DEBUG, 0, "plain", 0);
> +	memory_init();
> +
> +	fiber_init(fiber_c_invoke);
> +	popen_init();
> +	coio_init();
> +	coio_enable();
> +
> +	if (!loop())
> +		panic("%s", "can't init event loop");
> +
> +	struct fiber *test = fiber_new("coio_stat", main_f);

3. It seems the name was copied from coio.cc and forgotten to change.
   AFAIS, it is usually "main" for such tests.

> +	fiber_wakeup(test);
> +
> +	ev_now_update(loop());
> +	ev_run(loop(), 0);
> +	popen_free();
> +	fiber_free();
> +	memory_free();
> +
> +	return 0;
> +}

4. I would return a return value from check_plan() in main_f() here,
   like in swim.c unit test.

   I think that it is good property to report a non-zero exit code from
   a test if it fails in some way, because this way we may relax
   requirements for a testing harness if we'll need it for some reason.

----

In fact I experimented around this and made the proposed changes. So
I'll share it with you in order to better show what I propose.

I also verified that after small changes in test/unit/unit.[ch] and the
changes below the result file for popen test can be deleted at all.

diff --git a/test/unit/popen.c b/test/unit/popen.c
index 7656ebe73..a40ca514c 100644
--- a/test/unit/popen.c
+++ b/test/unit/popen.c
@@ -16,6 +16,11 @@
 	POPEN_FLAG_SHELL		|	\
 	POPEN_FLAG_RESTORE_SIGNALS)
 
+/**
+ * A real return value of main_f(), see a comment in swim.c.
+ */
+static int test_result = 1;
+
 static int
 wait_exit(struct popen_handle *handle, int *state, int *exit_code)
 {
@@ -30,7 +35,7 @@ wait_exit(struct popen_handle *handle, int *state, int *exit_code)
 	return 0;
 }
 
-static int
+static void
 popen_write_exit(void)
 {
 	struct popen_handle *handle;
@@ -95,10 +100,10 @@ out_kill:
 
 out:
 	footer();
-	return check_plan();
+	check_plan();
 }
 
-static int
+static void
 popen_read_exit(void)
 {
 	struct popen_handle *handle;
@@ -152,10 +157,10 @@ out_kill:
 
 out:
 	footer();
-	return check_plan();
+	check_plan();
 }
 
-static int
+static void
 popen_kill(void)
 {
 	struct popen_handle *handle;
@@ -205,21 +210,24 @@ out_kill:
 
 out:
 	footer();
-	return check_plan();
+	check_plan();
 }
 
 static int
 main_f(va_list ap)
 {
-	int rc = 0;
+	plan(3);
+	header();
 
-	rc = popen_write_exit();
-	if (rc == 0)
-		rc = popen_read_exit();
-	if (rc == 0)
-		rc = popen_kill();
+	popen_write_exit();
+	popen_read_exit();
+	popen_kill();
 
 	ev_break(loop(), EVBREAK_ALL);
+
+	footer();
+	test_result = check_plan();
+
 	return 0;
 }
 
@@ -237,7 +245,7 @@ main(int argc, char *argv[])
 	if (!loop())
 		panic("%s", "can't init event loop");
 
-	struct fiber *test = fiber_new("coio_stat", main_f);
+	struct fiber *test = fiber_new("main", main_f);
 	fiber_wakeup(test);
 
 	ev_now_update(loop());
@@ -246,5 +254,5 @@ main(int argc, char *argv[])
 	fiber_free();
 	memory_free();
 
-	return 0;
+	return test_result;
 }

[Tarantool-patches] [PATCH v5 7/7] test/unit: add popen test

[Cyrill Gorcunov]

Basic test for popen engine

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
Sasha, thanks a huge for all your comments! I've merged your
changes into the series.

branch gorcunov/gh-4031-popen-fixup-5

 test/unit/CMakeLists.txt |   3 +
 test/unit/popen.c        | 258 +++++++++++++++++++++++++++++++++++++++
 test/unit/popen.result   |  31 +++++
 3 files changed, 292 insertions(+)
 create mode 100644 test/unit/popen.c
 create mode 100644 test/unit/popen.result

diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index c037ac539..4ac08de8d 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -240,3 +240,6 @@ target_link_libraries(swim_errinj.test unit swim)
 
 add_executable(merger.test merger.test.c)
 target_link_libraries(merger.test unit core box)
+
+add_executable(popen.test popen.c)
+target_link_libraries(popen.test misc unit core)
diff --git a/test/unit/popen.c b/test/unit/popen.c
new file mode 100644
index 000000000..a40ca514c
--- /dev/null
+++ b/test/unit/popen.c
@@ -0,0 +1,258 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "trivia/util.h"
+#include "unit.h"
+
+#include "coio.h"
+#include "coio_task.h"
+#include "memory.h"
+#include "fiber.h"
+#include "popen.h"
+#include "say.h"
+
+#define TEST_POPEN_COMMON_FLAGS			\
+	(POPEN_FLAG_SETSID		|	\
+	POPEN_FLAG_SHELL		|	\
+	POPEN_FLAG_RESTORE_SIGNALS)
+
+/**
+ * A real return value of main_f(), see a comment in swim.c.
+ */
+static int test_result = 1;
+
+static int
+wait_exit(struct popen_handle *handle, int *state, int *exit_code)
+{
+	for (;;) {
+		if (popen_state(handle, state, exit_code))
+			return -1;
+		if (*state == POPEN_STATE_EXITED ||
+		    *state == POPEN_STATE_SIGNALED)
+			break;
+		fiber_sleep(0.1);
+	}
+	return 0;
+}
+
+static void
+popen_write_exit(void)
+{
+	struct popen_handle *handle;
+	char *child_argv[] = {
+		"/bin/sh", "-c",
+		"prompt=''; read -n 5 prompt; echo $prompt",
+		NULL,
+	};
+
+	const char data[] = "12345";
+	int state, exit_code;
+
+	struct popen_opts opts = {
+		.argv		= child_argv,
+		.nr_argv	= lengthof(child_argv),
+		.env		= NULL,
+		.flags		=
+			POPEN_FLAG_FD_STDIN		|
+			POPEN_FLAG_FD_STDOUT		|
+			POPEN_FLAG_FD_STDERR		|
+			TEST_POPEN_COMMON_FLAGS,
+	};
+	int rc;
+
+	plan(7);
+	header();
+
+	handle = popen_new(&opts);
+	ok(handle != NULL, "popen_new");
+	if (handle == NULL)
+		goto out;
+
+	rc = popen_state(handle, &state, &exit_code);
+	ok(rc == 0, "popen_state");
+
+	ok(state == POPEN_STATE_ALIVE, "state %s",
+	   popen_state_str(state));
+
+	rc = popen_write_timeout(handle, (void *)data,
+				 (int)strlen(data),
+				 POPEN_FLAG_FD_STDOUT, 180);
+	ok(rc == -1, "write flag check");
+
+	rc = popen_write_timeout(handle, (void *)data,
+				 (int)strlen(data),
+				 POPEN_FLAG_FD_STDIN, 180);
+	ok(rc == (int)strlen(data), "write to pipe");
+	if (rc != (int)strlen(data))
+		goto out_kill;
+
+	rc = wait_exit(handle, &state, &exit_code);
+	if (rc) {
+		ok(false, "child wait");
+		goto out_kill;
+	}
+
+	ok(state == POPEN_STATE_EXITED, "child exited");
+
+out_kill:
+	rc = popen_delete(handle);
+	ok(rc == 0, "popen_delete");
+
+out:
+	footer();
+	check_plan();
+}
+
+static void
+popen_read_exit(void)
+{
+	struct popen_handle *handle;
+	char *child_argv[] = {
+		"/bin/sh", "-c",
+		"echo 1 2 3 4 5",
+		NULL,
+	};
+
+	int state, exit_code;
+	char data[32] = { };
+
+	struct popen_opts opts = {
+		.argv		= child_argv,
+		.nr_argv	= lengthof(child_argv),
+		.env		= NULL,
+		.flags		=
+			POPEN_FLAG_FD_STDIN		|
+			POPEN_FLAG_FD_STDOUT		|
+			POPEN_FLAG_FD_STDERR		|
+			TEST_POPEN_COMMON_FLAGS,
+	};
+	int rc;
+
+	plan(5);
+	header();
+
+	handle = popen_new(&opts);
+	ok(handle != NULL, "popen_new");
+	if (handle == NULL)
+		goto out;
+
+	rc = wait_exit(handle, &state, &exit_code);
+	if (rc) {
+		ok(false, "child wait");
+		goto out_kill;
+	}
+	ok(state == POPEN_STATE_EXITED, "child exited");
+
+	rc = popen_read_timeout(handle, data, sizeof(data),
+				POPEN_FLAG_FD_STDIN, 180);
+	ok(rc == -1, "read flag check");
+
+	rc = popen_read_timeout(handle, data, sizeof(data),
+				POPEN_FLAG_FD_STDOUT, 180);
+	ok(rc >= 9 && !strncmp(data, "1 2 3 4 5", 9), "read from pipe");
+
+out_kill:
+	rc = popen_delete(handle);
+	ok(rc == 0, "popen_delete");
+
+out:
+	footer();
+	check_plan();
+}
+
+static void
+popen_kill(void)
+{
+	struct popen_handle *handle;
+	char *child_argv[] = {
+		"/bin/sh", "-c",
+		"while [ 1 ]; do sleep 10; done",
+		NULL,
+	};
+
+	int state, exit_code;
+
+	struct popen_opts opts = {
+		.argv		= child_argv,
+		.nr_argv	= lengthof(child_argv),
+		.env		= NULL,
+		.flags		=
+			POPEN_FLAG_FD_STDIN		|
+			POPEN_FLAG_FD_STDOUT		|
+			POPEN_FLAG_FD_STDERR		|
+			TEST_POPEN_COMMON_FLAGS,
+	};
+	int rc;
+
+	plan(4);
+	header();
+
+	handle = popen_new(&opts);
+	ok(handle != NULL, "popen_new");
+	if (handle == NULL)
+		goto out;
+
+	rc = popen_send_signal(handle, SIGTERM);
+	ok(rc == 0, "popen_send_signal");
+	if (rc != 0)
+		goto out_kill;
+
+	rc = wait_exit(handle, &state, &exit_code);
+	if (rc) {
+		ok(false, "child wait");
+		goto out_kill;
+	}
+	ok(state == POPEN_STATE_SIGNALED, "child terminated");
+
+out_kill:
+	rc = popen_delete(handle);
+	ok(rc == 0, "popen_delete");
+
+out:
+	footer();
+	check_plan();
+}
+
+static int
+main_f(va_list ap)
+{
+	plan(3);
+	header();
+
+	popen_write_exit();
+	popen_read_exit();
+	popen_kill();
+
+	ev_break(loop(), EVBREAK_ALL);
+
+	footer();
+	test_result = check_plan();
+
+	return 0;
+}
+
+int
+main(int argc, char *argv[])
+{
+	//say_logger_init(NULL, S_DEBUG, 0, "plain", 0);
+	memory_init();
+
+	fiber_init(fiber_c_invoke);
+	popen_init();
+	coio_init();
+	coio_enable();
+
+	if (!loop())
+		panic("%s", "can't init event loop");
+
+	struct fiber *test = fiber_new("main", main_f);
+	fiber_wakeup(test);
+
+	ev_now_update(loop());
+	ev_run(loop(), 0);
+	popen_free();
+	fiber_free();
+	memory_free();
+
+	return test_result;
+}
diff --git a/test/unit/popen.result b/test/unit/popen.result
new file mode 100644
index 000000000..d7894d1db
--- /dev/null
+++ b/test/unit/popen.result
@@ -0,0 +1,31 @@
+1..3
+	*** main_f ***
+    1..7
+	*** popen_write_exit ***
+    ok 1 - popen_new
+    ok 2 - popen_state
+    ok 3 - state alive
+    ok 4 - write flag check
+    ok 5 - write to pipe
+    ok 6 - child exited
+    ok 7 - popen_delete
+	*** popen_write_exit: done ***
+ok 1 - subtests
+    1..5
+	*** popen_read_exit ***
+    ok 1 - popen_new
+    ok 2 - child exited
+    ok 3 - read flag check
+    ok 4 - read from pipe
+    ok 5 - popen_delete
+	*** popen_read_exit: done ***
+ok 2 - subtests
+    1..4
+	*** popen_kill ***
+    ok 1 - popen_new
+    ok 2 - popen_send_signal
+    ok 3 - child terminated
+    ok 4 - popen_delete
+	*** popen_kill: done ***
+ok 3 - subtests
+	*** main_f: done ***
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 0/7] popen: various fixes and a test

[Alexander Turenko]

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
> 

Re: [Tarantool-patches] [PATCH 0/7] popen: various fixes and a test

[Cyrill Gorcunov]

On Thu, Mar 12, 2020 at 02:58:42PM +0300, Alexander Turenko wrote:
> 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).

Thanks a huge, Sasha! I think we continue fixing/extending popen engine.

Re: [Tarantool-patches] [PATCH 0/7] popen: various fixes and a test

[Kirill Yukhin]

Hello,

On 02 мар 23:12, 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

I've checked your patchset into master.

--
Regards, Kirill Yukhin