[PATCH v2] core: Non-blocking io.popen

[PATCH v2] core: Non-blocking io.popen

[Stanislav Zudin]

Adds nonblocking implementation of popen.
The method is available in namespace fio.
fio.popen() returns an object providing facilities
for dealing with standard input/output, getting
status of the child process.

Closes #4031

@TarantoolBot document
Title: Nonblocking fio.popen

handle, err = fio.popen(parameters)

fio.popen starts a process and redirects its input/output.

parameters - a table containing arguments to run a process.
The following arguments are expected:

argv - [mandatory] is a table of argument strings passed to
the new program. By convention, the first of these strings should
contain the filename associated with the file being executed.

environment - [optional] is a table of strings, conventionally of the
form key=value, which are passed as environment to the new program.

By default stdin, stdout,stderr of the associated process are
available for writing/reading using object's methods
handle:write() and handle:read().
One can override default behavior to redirect streams to/from file
or to input/output of another process or to the default input/output
of the parent process.

stdin - [optional] overrides the child process's standard input.
stdout - [optional] overrides the child process's standard output.
stderr - [optional] overrides the the child process's standard
error output.
May accept one of the following values:
Handle of the file open with fio.open()
Handle of the standard input/output of another process,
open by fio.popen
A constant defining the parent's STDIN, STDOUT or STDERR.
A constants:
fio.PIPE - Opens a file descriptor available for reading/writing
or redirection to/from another process.
fio.DEVNULL - Makes fio.popen redirect the output to /dev/null.

On success returns an object providing methods for
communication with the running process.
Returns nil if underlying functions calls fail;
in this case the second return value, err, contains a error message.

The object created by fio.popen provides the following methods:
read()
read2()
write()
kill()
wait()
get_status()
get_stdin()
get_stdout()
get_stderr()

number handle:get_stdin()

Returns handle of the child process's standard input.
The handle is available only if it wasn't redirected earlier.
Use this handle to setup a redirection
from file or other process to the input of the associated process.
If handle is unavailable the method returns nil.

number handle:get_stdout()
number handle:get_stderr()

Return STDOUT and STDIN of the associated process accordingly.
See handle:get_stdin() for details.

rc,err = handle:wait(timeout)

The wait() waits for the associated process to terminate
and returns the exit status of the command.

timeout - an integer specifies number of seconds to wait.
If the requested time has elapsed the method returns nil,
the second return value, err, contains a error message.
To distinguish timeout from the the other errors use errno.
If timeout is nil, the method waits infinitely until
the associated process is terminated.
On success function returns an exit code as a positive number
or signal id as a negative number.
If failed, rc is nul and err contains a error message.

If the associated process is terminated, one can use the following
methods get the exit status:

rc = handle:get_status()

returns nil if process is still running
 >= 0 if process exited normally
 < 0 if process was terminated by a signal

rc, err = handle:kill(sig)

The kill() sends a specified signal to the associated process.
On success the method returns true, if failed - nil and error message.
If the sig is nil the default "SIGTERM" is being sent to the process.
If the signal is unknown then the method fails (with error EINVAL).
The following signals are acceptable:
"SIGINT"
"SIGILL"
"SIGABRT"
"SIGFPE"
"SIGSEGV"
"SIGTERM"

"SIGHUP"
"SIGQUIT"
"SIGTRAP"
"SIGKILL"
"SIGBUS"
"SIGSYS"
"SIGPIPE"
"SIGALRM"

"SIGURG"
"SIGSTOP"
"SIGTSTP"
"SIGCONT"
"SIGCHLD"
"SIGTTIN"
"SIGTTOU"
"SIGPOLL"
"SIGXCPU"
"SIGXFSZ"
"SIGVTALRM"
"SIGPROF"
"SIGUSR1"
"SIGUSR2"

rc,src,err = handle:read(buffer,size)
rc,src,err = handle:read2(buffer,size,seconds)

read stdout & stderr of the process started by fio.popen
read() -> str, source
read(buf) -> len, source
read(size) -> str, source
read(buf, size) -> len, source
read2(seconds) -> str, source
read2(buf,seconds) -> len, source
read2(size,seconds) -> str, source
read2(buf, size,seconds) -> len, source

seconds - an integer specifies number of seconds to wait.
If the requested time has elapsed the method returns nil.

src contains id of the stream: fio.STDOUT or fio.STDERR.
If method failed the rc and src are nil and err contains error message.

rc, err = handle:write(data, length)
Writes specified number of bytes
On success returns number of written bytes.
If failed the rc is nil and err contains an error message.
---
Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-4031-nonblocking-popen
Issue: https://github.com/tarantool/tarantool/issues/4031

 src/CMakeLists.txt                |   1 +
 src/lib/core/CMakeLists.txt       |   1 +
 src/lib/core/coio_file.c          | 243 +++++++++++
 src/lib/core/coio_file.h          |  30 ++
 src/lib/core/coio_popen.c         | 661 ++++++++++++++++++++++++++++++
 src/lib/core/coio_popen.h         | 243 +++++++++++
 src/lib/core/coio_task.c          |   2 +
 src/lib/core/fiber.h              |   4 +-
 src/lua/fio.c                     | 289 +++++++++++++
 src/lua/fio.lua                   | 262 ++++++++++++
 src/lua/init.c                    |   2 +
 src/lua/lua_signal.c              |  99 +++++
 src/lua/lua_signal.h              |  45 ++
 src/main.cc                       |  21 +-
 test/box-tap/fio_popen.sample.txt |   5 +
 test/box-tap/fio_popen.test.lua   | 189 +++++++++
 test/box-tap/fio_popen_test1.sh   |   7 +
 third_party/libeio/eio.c          |  19 +-
 third_party/libeio/etp.c          |  28 ++
 third_party/libev/ev.c            |   2 +
 20 files changed, 2149 insertions(+), 4 deletions(-)
 create mode 100644 src/lib/core/coio_popen.c
 create mode 100644 src/lib/core/coio_popen.h
 create mode 100644 src/lua/lua_signal.c
 create mode 100644 src/lua/lua_signal.h
 create mode 100644 test/box-tap/fio_popen.sample.txt
 create mode 100755 test/box-tap/fio_popen.test.lua
 create mode 100755 test/box-tap/fio_popen_test1.sh

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 68674d06a..cfe46dfe3 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -117,6 +117,7 @@ set (server_sources
      lua/info.c
      lua/string.c
      lua/buffer.c
+     lua/lua_signal.c
      ${lua_sources}
      ${PROJECT_SOURCE_DIR}/third_party/lua-yaml/lyaml.cc
      ${PROJECT_SOURCE_DIR}/third_party/lua-yaml/b64.c
diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
index eb10b11c3..8b1f8d32e 100644
--- a/src/lib/core/CMakeLists.txt
+++ b/src/lib/core/CMakeLists.txt
@@ -26,6 +26,7 @@ set(core_sources
     trigger.cc
     mpstream.c
     port.c
+    coio_popen.c
 )
 
 if (TARGET_OS_NETBSD)
diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index 3359f42bc..93956e533 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -34,6 +34,7 @@
 #include "say.h"
 #include "fio.h"
 #include "errinj.h"
+#include "coio_popen.h"
 #include <stdio.h>
 #include <stdlib.h>
 #include <dirent.h>
@@ -103,6 +104,36 @@ struct coio_file_task {
 			const char *source;
 			const char *dest;
 		} copyfile;
+
+		struct {
+			char** argv;
+			int argc;
+			char** environment;
+			int environment_size;
+			int stdin_fd;
+			int stdout_fd;
+			int stderr_fd;
+			void *handle;
+		} popen_params;
+
+		struct {
+			void *handle;
+		} pclose_params;
+
+		struct {
+			void *handle;
+			const void* buf;
+			size_t count;
+			size_t *written;
+		} popen_write;
+
+		struct {
+			void *handle;
+			void *buf;
+			size_t count;
+			size_t *read_bytes;
+			int *output_number;
+		} popen_read;
 	};
 };
 
@@ -631,3 +662,215 @@ coio_copyfile(const char *source, const char *dest)
 	eio_req *req = eio_custom(coio_do_copyfile, 0, coio_complete, &eio);
 	return coio_wait_done(req, &eio);
 }
+
+static void
+coio_do_popen(eio_req *req)
+{
+	struct coio_file_task *eio = (struct coio_file_task *)req->data;
+	eio->popen_params.handle = coio_popen_impl(eio->popen_params.argv,
+						   eio->popen_params.argc,
+						   eio->popen_params.environment,
+						   eio->popen_params.environment_size,
+						   eio->popen_params.stdin_fd,
+						   eio->popen_params.stdout_fd,
+						   eio->popen_params.stderr_fd);
+
+	eio->result = 0;
+	eio->errorno = errno;
+}
+
+void *
+coio_popen(char** argv, int argc,
+	   char** environment, int environment_size,
+	   int stdin_fd, int stdout_fd, int stderr_fd)
+{
+	INIT_COEIO_FILE(eio);
+	eio.popen_params.argv = argv;
+	eio.popen_params.argc = argc;
+	eio.popen_params.environment = environment;
+	eio.popen_params.environment_size = environment_size;
+	eio.popen_params.stdin_fd = stdin_fd;
+	eio.popen_params.stdout_fd = stdout_fd;
+	eio.popen_params.stderr_fd = stderr_fd;
+
+	eio_req *req = eio_custom(coio_do_popen, 0,
+				  coio_complete, &eio);
+	coio_wait_done(req, &eio);
+	return eio.popen_params.handle;
+}
+
+static void
+coio_do_popen_read(eio_req *req)
+{
+	struct coio_file_task *eio = (struct coio_file_task *)req->data;
+
+	int rc = coio_popen_try_to_read(eio->popen_read.handle,
+					eio->popen_read.buf,
+					eio->popen_read.count,
+					eio->popen_read.read_bytes,
+					eio->popen_read.output_number);
+
+	req->result = rc;
+	req->errorno = errno;
+}
+
+static int
+coio_do_nonblock_popen_read(void *fh, void *buf, size_t count,
+	size_t *read_bytes, int *source_id)
+{
+	INIT_COEIO_FILE(eio);
+	eio.popen_read.buf = buf;
+	eio.popen_read.count = count;
+	eio.popen_read.handle = fh;
+	eio.popen_read.read_bytes = read_bytes;
+	eio.popen_read.output_number = source_id;
+	eio_req *req = eio_custom(coio_do_popen_read, 0,
+				  coio_complete, &eio);
+	return coio_wait_done(req, &eio);
+}
+
+ssize_t
+coio_popen_read(void *fh, void *buf, size_t count,
+		int *output_number, int timeout)
+{
+	size_t received = 0;
+	int rc = coio_popen_try_to_read(fh, buf, count,
+		&received, output_number);
+	if (rc == 0)		/* The reading's succeeded */
+		return (ssize_t)received;
+	else if (rc == -1 && errno != EAGAIN)	/* Failed */
+		return -1;
+
+	/* A blocking operation is expected */
+
+	time_t start_tt;
+	time(&start_tt);
+
+	bool in_progress;
+	do {
+		if (fiber_is_cancelled()) {
+			errno = EINTR;
+			return -1;
+		}
+
+		if (timeout > 0) {
+			time_t tt;
+			time(&tt);
+			if ((tt - start_tt) > timeout) {
+				errno = ETIMEDOUT;
+				return -1;
+			}
+		}
+
+		rc = coio_do_nonblock_popen_read(fh, buf, count,
+			&received, output_number);
+		in_progress = (rc == -1 && errno == EAGAIN);
+	} while (in_progress);
+
+	return (rc == 0) ? (ssize_t)received
+			 : -1;
+}
+
+static void
+coio_do_popen_write(eio_req *req)
+{
+	struct coio_file_task *eio = (struct coio_file_task *)req->data;
+
+	int rc = coio_popen_try_to_write(eio->popen_write.handle,
+					eio->popen_write.buf,
+					eio->popen_write.count,
+					eio->popen_write.written);
+
+	req->result = rc;
+	req->errorno = errno;
+}
+
+static int
+coio_do_nonblock_popen_write(void *fh, const void *buf, size_t count,
+	size_t *written)
+{
+	INIT_COEIO_FILE(eio);
+	eio.popen_write.buf = buf;
+	eio.popen_write.count = count;
+	eio.popen_write.handle = fh;
+	eio.popen_write.written = written;
+	eio_req *req = eio_custom(coio_do_popen_write, 0,
+				  coio_complete, &eio);
+	return coio_wait_done(req, &eio);
+}
+
+ssize_t
+coio_popen_write(void *fh, const void *buf, size_t count)
+{
+	ssize_t  total = 0;
+	size_t written = 0;
+	int rc = coio_popen_try_to_write(fh, buf, count,
+					&written);
+	if (rc == 0 && written == count) /* The writing's succeeded */
+		return (ssize_t)written;
+	else if (rc == -1 && errno != EAGAIN) /* Failed */
+		return -1;
+
+	/* A blocking operation is expected */
+	bool in_progress;
+
+	do {
+		if (fiber_is_cancelled()) {
+			errno = EINTR;
+			return -1;
+		}
+
+		buf += written;		/* advance writing position */
+		total += (ssize_t)written;
+		count -= written;
+
+		if (count == 0)
+			return total;
+
+		written = 0;
+		rc = coio_do_nonblock_popen_write(fh, buf, count,
+						  &written);
+		in_progress = 	(rc == 0 && written < count) ||
+				(rc == -1 && errno == EAGAIN);
+	} while (in_progress);
+
+	return (rc == 0) ? total
+			 : -1;
+}
+
+int
+coio_popen_wait(void *fh, int timeout, int *exit_code)
+{
+	time_t start_tt;
+	time(&start_tt);
+
+	do {
+		/* Wait for SIGCHLD */
+		int sig = 0;
+		int code = 0;
+
+		int rc = coio_popen_get_status(fh, &sig, &code);
+		if (rc != POPEN_RUNNING) {
+			*exit_code = (rc == POPEN_EXITED) ? code
+							  : -sig;
+			return 0;
+		}
+
+		/* Check for timeout */
+		if (timeout > 0) {
+			time_t tt;
+			time(&tt);
+			if ((tt - start_tt) > timeout) {
+				errno = ETIMEDOUT;
+				return -1;
+			}
+		}
+
+		fiber_yield_timeout(0);
+
+	} while(!fiber_is_cancelled());
+
+	errno = EINTR;
+	return -1;
+}
+
diff --git a/src/lib/core/coio_file.h b/src/lib/core/coio_file.h
index f2112ceed..4181796eb 100644
--- a/src/lib/core/coio_file.h
+++ b/src/lib/core/coio_file.h
@@ -84,6 +84,36 @@ int	coio_tempdir(char *path, size_t path_len);
 
 int	coio_readdir(const char *path, char **buf);
 int	coio_copyfile(const char *source, const char *dest);
+
+void *
+coio_popen(char** argv, int argc,
+	   char** environment, int environment_size,
+	   int stdin_fd, int stdout_fd, int stderr_fd);
+
+ssize_t
+coio_popen_read(void *fh, void *buf, size_t count,
+		int *output_number, int timeout);
+
+ssize_t
+coio_popen_write(void *fh, const void *buf, size_t count);
+
+/**
+ * Wait for the associated process to terminate.
+ * The function doesn't release the allocated resources.
+ *
+ * @param fd handle returned by fio.popen.
+ *
+ * @param timeout number of second to wait before function exit with error.
+ * If function exited due to timeout the errno equals to ETIMEDOUT.
+ *
+ * @exit_code On success contains the exit code as a positive number
+ * or signal id as a negative number.
+
+ * @return On success function returns 0, and -1 on error.
+ */
+int
+coio_popen_wait(void *fh, int timeout, int *exit_code);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/src/lib/core/coio_popen.c b/src/lib/core/coio_popen.c
new file mode 100644
index 000000000..b44cbf60b
--- /dev/null
+++ b/src/lib/core/coio_popen.c
@@ -0,0 +1,661 @@
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <stddef.h>
+#include <signal.h>
+#include "coio_popen.h"
+#include "coio_task.h"
+#include "fiber.h"
+#include "say.h"
+#include "fio.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <dirent.h>
+#include <sys/wait.h>
+#include <paths.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <bits/types/siginfo_t.h>
+#include <pthread.h>
+
+/*
+ * On OSX this global variable is not declared
+ * in <unistd.h>
+ */
+extern char **environ;
+
+
+struct popen_data {
+	/* process id */
+	pid_t pid;
+	int fh[3];
+	/*
+	 * Three handles:
+	 * [0] write to stdin of the child process
+	 * [1] read from stdout of the child process
+	 * [2] read from stderr of the child process
+	 */
+
+	/* Handle to /dev/null */
+	int devnull_fd;
+
+	/* The ID of socket was read recently
+	 * (STDERR_FILENO or STDOUT_FILENO */
+	int prev_source;
+
+	/*
+	 * Current process status.
+	 * The SIGCHLD handler changes this status.
+	 */
+	enum popen_status status;
+
+	/* exit status of the associated process. */
+	int exit_code;
+
+	/*
+	 * Number of the signal that caused the
+	 * assosiated process to terminate
+	 */
+	int signal_id;
+
+	/*
+	 * The next entry in the global list
+	 */
+	struct popen_data* next;
+};
+
+static pthread_mutex_t mutex;
+static struct popen_data* popen_data_list = NULL;
+
+void
+popen_initialize()
+{
+	pthread_mutexattr_t errorcheck;
+	pthread_mutexattr_init(&errorcheck);
+	pthread_mutexattr_settype(&errorcheck,
+		PTHREAD_MUTEX_ERRORCHECK);
+	pthread_mutex_init(&mutex, &errorcheck);
+	pthread_mutexattr_destroy(&errorcheck);
+}
+
+static void
+popen_lock_data_list()
+{
+	pthread_mutex_lock(&mutex);
+}
+
+static void
+popen_unlock_data_list()
+{
+	pthread_mutex_unlock(&mutex);
+}
+
+static void
+popen_append_to_list(struct popen_data *data)
+{
+	/* Insert into head of the list */
+	data->next = popen_data_list;
+	popen_data_list = data;
+}
+
+static struct popen_data *
+popen_lookup_data_by_pid(pid_t pid)
+{
+	struct popen_data *cur = popen_data_list;
+	for(; cur; cur = cur->next) {
+		if (cur->pid == pid)
+			return cur;
+	}
+
+	return NULL;
+}
+
+static void
+popen_exclude_from_list(struct popen_data *data)
+{
+	if (popen_data_list == data) {
+		popen_data_list = data->next;
+		return;
+	}
+
+	/* Find the previous entry */
+	struct popen_data *prev = popen_data_list;
+	for( ; prev && prev->next != data; prev = prev->next) {
+		/* empty */;
+	}
+
+	if (!prev)
+		return ;
+	assert(prev->next == data);
+	prev->next = data->next;
+}
+
+/*
+ * Returns next socket to read.
+ * Use this function when both STDOUT and STDERR outputs
+ * are ready for reading.
+ * */
+static inline int
+get_handle_in_order(struct popen_data *data)
+{
+	/*
+	 * Invert the order of handles to be read
+	 */
+	const int mask = STDERR_FILENO | STDOUT_FILENO;
+	data->prev_source ^= mask;
+
+	/*
+	 * If handle is not available, invert it back
+	 */
+	if (data->fh[data->prev_source] < 0)
+		data->prev_source ^= mask;
+	/*
+	 * if both reading handles are invalid return -1
+	 */
+	return data->fh[data->prev_source];
+}
+
+static struct popen_data *
+popen_data_new()
+{
+	struct popen_data *data =
+		(struct popen_data *)calloc(1, sizeof(*data));
+	data->fh[0] = -1;
+	data->fh[1] = -1;
+	data->fh[2] = -1;
+	data->devnull_fd = -1;
+
+	data->prev_source = STDERR_FILENO;
+	/*
+	 * if both streams are ready then
+	 * start reading from STDOUT
+	 */
+
+	data->status = POPEN_RUNNING;
+	return data;
+}
+
+void *
+coio_popen_impl(char** argv, int argc,
+	char** environment, int environment_size,
+	int stdin_fd, int stdout_fd, int stderr_fd)
+{
+	pid_t pid;
+	int socket_rd[2] = {-1,-1};
+	int socket_wr[2] = {-1,-1};
+	int socket_er[2] = {-1,-1};
+	errno = 0;
+
+	struct popen_data *data = popen_data_new();
+	if (data == NULL)
+		return NULL;
+
+	/* Setup a /dev/null if necessary */
+	bool read_devnull = (stdin_fd == FIO_DEVNULL);
+	bool write_devnull = (stdout_fd == FIO_DEVNULL) ||
+			     (stderr_fd == FIO_DEVNULL);
+	int devnull_flags = O_RDWR | O_CREAT;
+	if (!read_devnull)
+		devnull_flags = O_WRONLY | O_CREAT;
+	else if (!write_devnull)
+		devnull_flags = O_RDONLY | O_CREAT;
+
+	if (read_devnull || write_devnull) {
+		data->devnull_fd = open("/dev/null", devnull_flags, 0666);
+		if (data->devnull_fd < 0)
+			goto on_error;
+		else {
+			if (stdin_fd == FIO_DEVNULL)
+				stdin_fd = data->devnull_fd;
+			if (stdout_fd == FIO_DEVNULL)
+				stdout_fd = data->devnull_fd;
+			if (stderr_fd == FIO_DEVNULL)
+				stderr_fd = data->devnull_fd;
+		}
+	}
+
+	if (stdin_fd == FIO_PIPE) {
+		/*
+		 * Enable non-blocking for the parent side
+		 * and close-on-exec on the child's side.
+		 * The socketpair on OSX doesn't support
+		 * SOCK_NONBLOCK & SOCK_CLOEXEC flags.
+		 */
+		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_rd) < 0 ||
+		    fcntl(socket_rd[0], F_SETFL, O_NONBLOCK) < 0 ||
+		    fcntl(socket_rd[1], F_SETFD, FD_CLOEXEC) < 0) {
+			goto on_error;
+		}
+	}
+
+	if (stdout_fd == FIO_PIPE) {
+		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_wr) < 0 ||
+		    fcntl(socket_wr[0], F_SETFL, O_NONBLOCK) < 0 ||
+		    fcntl(socket_wr[1], F_SETFD, FD_CLOEXEC) < 0) {
+			goto on_error;
+		}
+	}
+
+	if (stderr_fd == FIO_PIPE) {
+		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_er) < 0 ||
+		    fcntl(socket_er[0], F_SETFL, O_NONBLOCK) < 0 ||
+		    fcntl(socket_er[1], F_SETFD, FD_CLOEXEC) < 0) {
+			goto on_error;
+		}
+	}
+
+	pid = fork();
+
+	if (pid < 0)
+		goto on_error;
+	else if (pid == 0) /* child */ {
+		/* Setup stdin/stdout */
+		if (stdin_fd == FIO_PIPE) {
+			close(socket_rd[0]); /* close parent's side */
+			stdin_fd = socket_rd[1];
+		}
+		if (stdin_fd != STDIN_FILENO) {
+			dup2(stdin_fd, STDIN_FILENO);
+			close(stdin_fd);
+		}
+
+		if (stdout_fd == FIO_PIPE) {
+			close(socket_wr[0]); /* close parent's side */
+			stdout_fd = socket_wr[1];
+		}
+		if (stdout_fd != STDOUT_FILENO) {
+			dup2(stdout_fd, STDOUT_FILENO);
+			if (stdout_fd != STDERR_FILENO)
+				close(stdout_fd);
+		}
+
+		if (stderr_fd == FIO_PIPE) {
+			close(socket_er[0]); /* close parent's side */
+			stderr_fd = socket_er[1];
+		}
+		if (stderr_fd != STDERR_FILENO) {
+			dup2(stderr_fd, STDERR_FILENO);
+			if (stderr_fd != STDOUT_FILENO)
+				close(stderr_fd);
+		}
+
+		execve( argv[0], argv,
+			environment ? environment : environ);
+		_exit(127);
+		unreachable();
+	}
+
+	/* parent process */
+	if (stdin_fd == FIO_PIPE) {
+		close(socket_rd[1]); /* close child's side */
+		data->fh[STDIN_FILENO] = socket_rd[0];
+	}
+
+	if (stdout_fd == FIO_PIPE) {
+		close(socket_wr[1]); /* close child's side */
+		data->fh[STDOUT_FILENO] = socket_wr[0];
+	}
+
+	if (stderr_fd == FIO_PIPE) {
+		close(socket_er[1]); /* close child's side */
+		data->fh[STDERR_FILENO] = socket_er[0];
+	}
+
+	data->pid = pid;
+
+on_cleanup:
+	if (argv){
+		for(int i = 0; i < argc; ++i)
+			free(argv[i]);
+		free(argv);
+	}
+	if (environment) {
+		for(int i = 0; i < environment_size; ++i)
+			free(environment[i]);
+		free(environment);
+	}
+
+	if (data){
+		popen_lock_data_list();
+		popen_append_to_list(data);
+		popen_unlock_data_list();
+	}
+
+	return data;
+
+on_error:
+	if (socket_rd[0] >= 0) {
+		close(socket_rd[0]);
+		close(socket_rd[1]);
+	}
+	if (socket_wr[0] >= 0) {
+		close(socket_wr[0]);
+		close(socket_wr[1]);
+	}
+	if (socket_er[0] >= 0) {
+		close(socket_er[0]);
+		close(socket_er[1]);
+	}
+
+	if (data) {
+		if (data->devnull_fd >= 0)
+			close(data->devnull_fd);
+		free(data);
+	}
+	data = NULL;
+
+	goto on_cleanup;
+	unreachable();
+}
+
+static void
+popen_close_handles(struct popen_data *data)
+{
+	for(int i = 0; i < 3; ++i) {
+		if (data->fh[i] >= 0) {
+			close(data->fh[i]);
+			data->fh[i] = -1;
+		}
+	}
+	if (data->devnull_fd >= 0) {
+		close(data->devnull_fd);
+		data->devnull_fd = -1;
+	}
+}
+
+int
+coio_popen_destroy(void *fh)
+{
+	struct popen_data *data = (struct popen_data *)fh;
+
+	if (data == NULL){
+		errno = EBADF;
+		return -1;
+	}
+
+	popen_lock_data_list();
+	popen_close_handles(data);
+	popen_exclude_from_list(data);
+	popen_unlock_data_list();
+
+	free(data);
+	return 0;
+}
+
+int
+coio_popen_try_to_read(void *fh, void *buf, size_t count,
+	size_t *read_bytes, int *source_id)
+{
+	struct popen_data *data = (struct popen_data *)fh;
+
+	if (data == NULL){
+		errno = EBADF;
+		return -1;
+	}
+
+	fd_set rfds;
+	FD_ZERO(&rfds);
+	ssize_t received = 0;
+	int num = 0;
+
+	if (data->fh[STDOUT_FILENO] >= 0) {
+		FD_SET(data->fh[STDOUT_FILENO], &rfds);
+		++num;
+	}
+	if (data->fh[STDERR_FILENO] >= 0) {
+		FD_SET(data->fh[STDERR_FILENO], &rfds);
+		++num;
+	}
+
+	if (num == 0) {
+		/*
+		 * There are no open handles for reading
+		 */
+		errno = EBADF;
+		return -1;
+	}
+
+	struct timeval tv = {0,0};
+	int max_h = MAX(data->fh[STDOUT_FILENO],
+			data->fh[STDERR_FILENO]);
+
+	errno = 0;
+	int retv = select(max_h + 1, &rfds, NULL, NULL, &tv);
+	switch (retv) {
+	case -1:	/* Error */
+		return -1;
+	case 0:		/* Not ready yet */
+		errno = EAGAIN;
+		return -1;
+	case 1: {        /* One socket is ready */
+
+		/* Choose the socket */
+		int fno = STDOUT_FILENO;
+		if (!FD_ISSET(data->fh[fno], &rfds))
+			fno = STDERR_FILENO;
+		if (!FD_ISSET(data->fh[fno], &rfds)) {
+			unreachable();
+			return -1;
+		}
+
+		received = read(data->fh[fno], buf, count);
+		if (received < 0)
+			goto on_error;
+		data->prev_source = fno;
+		*read_bytes = received;
+		*source_id = fno;
+		return 0;
+		}
+	case 2: {        /* Both sockets are ready */
+		int attempt = 2;
+second_attempt:
+		--attempt;
+		received = read(get_handle_in_order(data), buf, count);
+
+		if (received < 0)
+			goto on_error;
+		if (received == 0 && attempt > 0)
+			goto second_attempt;
+
+		*read_bytes = received;
+		*source_id = (int)data->prev_source;
+		return 0;
+		}
+	}
+
+	unreachable();
+
+on_error:
+	if (errno == EINTR) {
+		*read_bytes = 0;
+		errno = EAGAIN;	/* Repeat */
+	}
+
+	return -1;
+}
+
+int
+coio_popen_try_to_write(void *fh, const void *buf, size_t count,
+	size_t *written)
+{
+	if (count == 0) {
+		*written = 0;
+		return  0;
+	}
+
+	struct popen_data *data = (struct popen_data *)fh;
+
+	if (data == NULL){
+		errno = EBADF;
+		return -1;
+	}
+
+	if (data->fh[STDIN_FILENO] < 0) {
+		/*
+		 * There are no open handles for writing
+		 */
+		errno = EBADF;
+		return -1;
+	}
+
+	fd_set wfds;
+	FD_ZERO(&wfds);
+
+	int wh = data->fh[STDIN_FILENO];
+	FD_SET(wh, &wfds);
+
+	struct timeval tv = {0,0};
+
+	int retv = select(wh + 1, NULL, &wfds, NULL, &tv);
+	if (retv < 0)
+		goto on_error;
+	else if (retv == 0) {
+		errno = EAGAIN;
+		return -1;	/* Not ready yet */
+	}
+
+	assert(retv == 1);	/* The socket is ready */
+
+	if (FD_ISSET(wh, &wfds)) {
+		ssize_t rc = write(wh, buf, count);
+		if (rc < 0)
+			goto on_error;
+		*written = rc;
+		return 0;
+	}
+
+	unreachable();
+	return -1;
+
+on_error:
+	if (errno == EINTR) {
+		*written = 0;
+		errno = EAGAIN;	/* Repeat */
+	}
+
+	return -1;
+}
+
+int
+coio_popen_kill(void *fh, int signal_id)
+{
+	struct popen_data *data = (struct popen_data *)fh;
+
+	if (data == NULL){
+		errno = EBADF;
+		return -1;
+	}
+
+	return kill(data->pid, signal_id);
+}
+
+int
+coio_popen_get_std_file_handle(void *fh, int file_no)
+{
+	if (file_no < STDIN_FILENO || STDERR_FILENO < file_no){
+		errno = EINVAL;
+		return -1;
+	}
+
+	struct popen_data *data = (struct popen_data *)fh;
+
+	if (data == NULL){
+		errno = EBADF;
+		return -1;
+	}
+
+	errno = 0;
+	return data->fh[file_no];
+}
+
+int
+coio_popen_get_status(void *fh, int *signal_id, int *exit_code)
+{
+	struct popen_data *data = (struct popen_data *)fh;
+
+	if (data == NULL){
+		errno = EBADF;
+		return -1;
+	}
+
+	errno = 0;
+
+	if (signal_id)
+		*signal_id = data->signal_id;
+	if (exit_code)
+		*exit_code = data->exit_code;
+
+	return data->status;
+}
+
+void
+coio_popen_child_is_dead(int sig, siginfo_t *si, void *context)
+{
+	(void)context;	/* UNUSED */
+
+	if (sig != SIGCHLD)
+		return;
+
+	/*
+	 * The sigaction is called with SA_NOCLDWAIT,
+	 * so no need to call waitpid()
+	 */
+
+	popen_lock_data_list();
+
+	struct popen_data *data = popen_lookup_data_by_pid(si->si_pid);
+	if (data) {
+		switch (si->si_code) {
+		case CLD_EXITED:
+			data->exit_code = si->si_status;
+			data->status = POPEN_EXITED;
+			break;
+		case CLD_KILLED:
+			data->signal_id = si->si_status;
+			data->status = POPEN_KILLED;
+			break;
+		case CLD_DUMPED:
+			/* exit_status makes no sense */
+			data->status = POPEN_DUMPED;
+			break;
+		}
+
+		/*
+		 * We shouldn't close file descriptors here.
+		 * The child process may exit earlier than
+		 * the parent process finishes reading data.
+		 * In this case the reading fails.
+		 */
+	}
+
+	popen_unlock_data_list();
+}
diff --git a/src/lib/core/coio_popen.h b/src/lib/core/coio_popen.h
new file mode 100644
index 000000000..e90ce9b25
--- /dev/null
+++ b/src/lib/core/coio_popen.h
@@ -0,0 +1,243 @@
+#ifndef TARANTOOL_LIB_CORE_COIO_POPEN_H_INCLUDED
+#define TARANTOOL_LIB_CORE_COIO_POPEN_H_INCLUDED
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+/**
+ * Special values of the file descriptors passed to fio.popen
+ * */
+enum {
+	FIO_PIPE = -2,
+	/*
+	 * Tells fio.popen to open a handle for
+	 * direct reading/writing
+	 */
+
+	FIO_DEVNULL = -3
+	/*
+	 * Tells fio.popen to redirect the given standard
+	 * stream into /dev/null
+	 */
+};
+
+/**
+ * Possible status of the process started via fio.popen
+ **/
+enum popen_status {
+	POPEN_UNKNOWN = -1,
+
+	POPEN_RUNNING = 1,
+	/*the process is alive and well*/
+
+	POPEN_EXITED = 2,
+	/*the process exited*/
+
+	POPEN_KILLED = 3,
+	/*the process terminated by a signal*/
+
+	POPEN_DUMPED = 4
+	/* the process terminated abnormally */
+};
+
+/**
+ * Initializes inner data of fio.popen
+ * */
+void
+popen_initialize();
+
+/**
+ * Implementation of fio.popen.
+ * The function opens a process by creating a pipe
+ * forking.
+ *
+ * @param argv - is an array of character pointers
+ * to the arguments terminated by a null pointer.
+ * The null pointer terminating the argv array is
+ * not counted in argc.
+ *
+ * @param argc - is the argument count.
+ *
+ * @param environment - is the pointer to an array
+ * of character pointers to the environment strings.
+ *
+ * @param environment_size - the environment strings count.
+ * The null pointer terminating the environment array is
+ * not counted in environment_size.
+ *
+ * @param stdin_fd - the file handle to be redirected to the
+ * child process's STDIN.
+ *
+ * @param stdout_fd - the file handle receiving the STDOUT
+ * output of the child process.
+ *
+ * @param stderr_fd - the file handle receiving the STDERR
+ * output of the child process.
+ *
+ * The stdin_fd, stdout_fd & stderr_fd accept file descriptors
+ * from open() or the following values:
+ *
+ * FIO_PIPE - opens a pipe, binds it with child's
+ * input/output. The pipe is available for reading/writing.
+ *
+ * FIO_DEVNULL - redirects output from process to /dev/null.
+ *
+ * @return handle of the pipe for reading or writing
+ * (depends on value of type).
+ * In a case of error returns NULL.
+ */
+void *
+coio_popen_impl(char** argv, int argc,
+	char** environment, int environment_size,
+	int stdin_fd, int stdout_fd, int stderr_fd);
+
+/**
+ * The function releases allocated resources.
+ * The function doesn't wait for the associated process
+ * to terminate.
+ *
+ * @param fh handle returned by fio.popen.
+ *
+ * @return 0 if the process is terminated
+ * @return -1 for an error
+ */
+int
+coio_popen_destroy(void *fh);
+
+/**
+ * The function reads up to count bytes from the handle
+ * associated with the child process.
+ * Returns immediately
+ *
+ * @param fd handle returned by fio.popen.
+ * @param buf a buffer to be read into
+ * @param count size of buffer in bytes
+ * @param read_bytes A pointer to the
+ * variable that receives the number of bytes read.
+ * @param source_id A pointer to the variable that receives a
+ * source stream id, 1 - for STDOUT, 2 - for STDERR.
+ *
+ * @return 0 data were successfully read
+ * @return -1 an error occurred, see errno for error code
+ *
+ * If there is nothing to read yet function returns -1
+ * and errno set no EAGAIN.
+ */
+int
+coio_popen_try_to_read(void *fh, void *buf, size_t count,
+	size_t *read_bytes, int *source_id);
+
+/**
+ * The function writes up to count bytes to the handle
+ * associated with the child process.
+ * Tries to write as much as possible without blocking
+ * and immediately returns.
+ *
+ * @param fd handle returned by fio.popen.
+ * @param buf a buffer to be written from
+ * @param count size of buffer in bytes
+ * @param written A pointer to the
+ * variable that receives the number of bytes actually written.
+ * If function fails the number of written bytes is undefined.
+ *
+ * @return 0 data were successfully written
+ * Compare values of <count> and <written> to check
+ * whether all data were written or not.
+ * @return -1 an error occurred, see errno for error code
+ *
+ * If the writing can block, function returns -1
+ * and errno set no EAGAIN.
+ */
+int
+coio_popen_try_to_write(void *fh, const void *buf, size_t count,
+	size_t *written);
+
+
+/**
+ * The function send the specified signal
+ * to the associated process.
+ *
+ * @param fd - handle returned by fio.popen.
+ *
+ * @return 0 on success
+ * @return -1 an error occurred, see errno for error code
+ */
+int
+coio_popen_kill(void *fh, int signal_id);
+
+/**
+ * Returns descriptor of the specified file.
+ *
+ * @param fd - handle returned by fio.popen.
+ * @param file_no accepts one of the
+ * following values:
+ * STDIN_FILENO,
+ * STDOUT_FILENO,
+ * STDERR_FILENO
+ *
+ * @return file descriptor or -1 if not available
+ */
+int
+coio_popen_get_std_file_handle(void *fh, int file_no);
+
+
+/**
+ * Returns status of the associated process.
+ *
+ * @param fd - handle returned by fio.popen.
+ * @param signal_id - if not NULL accepts the signal
+ * sent to terminate the process
+ * @param exit_code - if not NULL accepts the exit code
+ * if the process terminated normally.
+ *
+ * @return one of the following values:
+ * POPEN_RUNNING if the process is alive
+ * POPEN_EXITED if the process was terminated normally
+ * POPEN_KILLED if the process was terminated by a signal
+ * POPEN_UNKNOWN an error's occurred
+ */
+int
+coio_popen_get_status(void *fh, int *signal_id, int *exit_code);
+
+/**
+ * Handle SIGCHLD signal
+ */
+void
+coio_popen_child_is_dead(int sig, siginfo_t *si, void *);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_LIB_CORE_COIO_POPEN_H_INCLUDED */
diff --git a/src/lib/core/coio_task.c b/src/lib/core/coio_task.c
index 908b336ed..e6a1b327f 100644
--- a/src/lib/core/coio_task.c
+++ b/src/lib/core/coio_task.c
@@ -40,6 +40,7 @@
 
 #include "fiber.h"
 #include "third_party/tarantool_ev.h"
+#include "coio_popen.h"
 
 /*
  * Asynchronous IO Tasks (libeio wrapper).
@@ -129,6 +130,7 @@ coio_on_stop(void *data)
 void
 coio_init(void)
 {
+	popen_initialize();
 	eio_set_thread_on_start(coio_on_start, NULL);
 	eio_set_thread_on_stop(coio_on_stop, NULL);
 }
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index fb168e25e..614b8fefe 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -494,8 +494,8 @@ struct cord {
 extern __thread struct cord *cord_ptr;
 
 #define cord() cord_ptr
-#define fiber() cord()->fiber
-#define loop() (cord()->loop)
+#define fiber() (cord() ? cord()->fiber : NULL)
+#define loop() (cord() ? cord()->loop : NULL)
 
 void
 cord_create(struct cord *cord, const char *name);
diff --git a/src/lua/fio.c b/src/lua/fio.c
index 806f4256b..519be90d7 100644
--- a/src/lua/fio.c
+++ b/src/lua/fio.c
@@ -46,6 +46,7 @@
 
 #include "lua/utils.h"
 #include "coio_file.h"
+#include "coio_popen.h"
 
 static inline void
 lbox_fio_pushsyserror(struct lua_State *L)
@@ -703,11 +704,292 @@ lbox_fio_copyfile(struct lua_State *L)
 	return lbox_fio_pushbool(L, coio_copyfile(source, dest) == 0);
 }
 
+static bool
+popen_verify_argv(struct lua_State *L)
+{
+	if (!lua_istable(L, 1))
+		return false;
+	int num = (int)lua_objlen(L, 1);  /*luaL_getn(L,1);*/
+	return num >= 1;
+}
+
+static char**
+popen_extract_strarray(struct lua_State *L, int index, int* array_size)
+{
+	if (lua_type(L, index) != LUA_TTABLE) {
+		if (array_size)
+			*array_size = 0;
+		return NULL;
+	}
+
+	size_t num = lua_objlen(L, index);  /*luaL_getn(L,index);*/
+
+	char** array = calloc(num+1, sizeof(char*));
+	/*
+	 * The last item in the array must be NULL
+	 */
+
+	if (array == NULL)
+		return NULL;
+
+	for(size_t i = 0; i < num; ++i) {
+		lua_rawgeti(L, index, i+1);
+		size_t slen = 0;
+		const char* str = lua_tolstring(L, -1, &slen);
+		if (!str)
+			str = "";
+		array[i] = strdup(str);
+		lua_pop(L, 1);
+	}
+
+	if (array_size)
+		*array_size = num;
+	/*
+	 * The number of elements doesn't include
+	 * the trailing NULL pointer
+	 */
+	return array;
+}
+
+/**
+ * A wrapper applicable for using with lua's GC.
+ * */
+struct fio_popen_wrap {
+	void* handle;
+};
+static const char* fio_popen_typename = "fio.popen.data";
+
+static struct fio_popen_wrap *
+fio_popen_get_handle_wrap(lua_State *L, int index)
+{
+	luaL_checktype(L, index, LUA_TUSERDATA);
+	struct fio_popen_wrap *wrap = (struct fio_popen_wrap *)
+		luaL_checkudata(L, index, fio_popen_typename);
+	if (wrap == NULL)
+		luaL_typerror(L, index, fio_popen_typename);
+	return wrap;
+}
+
+static void *
+fio_popen_get_handle(lua_State *L, int index)
+{
+	struct fio_popen_wrap *wrap = fio_popen_get_handle_wrap(L, index);
+	void *fh = wrap->handle;
+	if (!fh)
+		luaL_error(L, "fio.popen failed");
+	return fh;
+}
+
+static int
+lbox_fio_popen_gc(lua_State *L)
+{
+	struct fio_popen_wrap *wrap = (struct fio_popen_wrap *)
+		luaL_checkudata(L, -1, fio_popen_typename);
+	if (wrap->handle)
+		coio_popen_destroy(wrap->handle);
+	wrap->handle = NULL;
+	return 0;
+}
+
+static int
+lbox_fio_popen(struct lua_State *L)
+{
+	if (lua_gettop(L) < 1) {
+		usage:
+		luaL_error(L, "fio.popen: Invalid arguments");
+	}
+
+	/*
+	 * handle* popen(argv, env, hstdin, hstdout, hstderr)
+	 */
+	if (!popen_verify_argv(L))
+		goto usage;
+
+	int stdin_fd = FIO_PIPE;
+	int stdout_fd = FIO_PIPE;
+	int stderr_fd = FIO_PIPE;
+
+	int argc = 0;
+	char** argv = popen_extract_strarray(L, 1, &argc);
+	int env_num = 0;
+	char** env = popen_extract_strarray(L, 2, &env_num);
+
+	if (lua_isnumber(L, 3))
+		stdin_fd = lua_tonumber(L, 3);
+	if (lua_isnumber(L, 4))
+		stdout_fd = lua_tonumber(L, 4);
+	if (lua_isnumber(L, 5))
+		stderr_fd = lua_tonumber(L, 5);
+
+	struct fio_popen_wrap *wrap =
+		(struct fio_popen_wrap*)lua_newuserdata(L,
+			sizeof(struct fio_popen_wrap));
+
+	luaL_getmetatable(L, fio_popen_typename);
+	lua_setmetatable(L, -2);
+
+	wrap->handle = coio_popen(argv, argc, env, env_num,
+		stdin_fd, stdout_fd, stderr_fd);
+
+	if (wrap->handle == NULL) {
+		lua_pop(L, 1);  /* remove wrap from the stack */
+		lua_pushnil(L);
+		lbox_fio_pushsyserror(L);
+		return 2;
+	}
+
+	return 1;
+}
+
+static int
+lbox_fio_popen_read(struct lua_State *L)
+{
+	/* popen_read(self.fh, buf, size, seconds) */
+
+	void* fh = fio_popen_get_handle(L, 1);
+	uint32_t ctypeid;
+	char *buf = *(char **)luaL_checkcdata(L, 2, &ctypeid);
+	size_t len = lua_tonumber(L, 3);
+	int seconds = lua_tonumber(L, 4);
+
+	if (!len) {
+		lua_pushinteger(L, 0);
+		lua_pushinteger(L, STDOUT_FILENO);
+		return 2;
+	}
+
+	int output_number = 0;
+	int res = coio_popen_read(fh, buf, len, &output_number, seconds);
+
+	if (res < 0) {
+		lua_pushnil(L);
+		lua_pushnil(L);
+		lbox_fio_pushsyserror(L);
+		return 3;
+	}
+
+	lua_pushinteger(L, res);
+	lua_pushinteger(L, output_number);
+	return 2;
+}
+
+static int
+lbox_fio_popen_write(struct lua_State *L)
+{
+	void* fh = fio_popen_get_handle(L, 1);
+	const char *buf = lua_tostring(L, 2);
+	uint32_t ctypeid = 0;
+	if (buf == NULL)
+		buf = *(const char **)luaL_checkcdata(L, 2, &ctypeid);
+	size_t len = lua_tonumber(L, 3);
+
+	int res = coio_popen_write(fh, buf, len);
+	if (res < 0) {
+		lua_pushnil(L);
+		lbox_fio_pushsyserror(L);
+		return 2;
+	}
+	lua_pushinteger(L, res);
+	return 1;
+}
+
+static int
+lbox_fio_popen_get_status(struct lua_State *L)
+{
+	void* fh = fio_popen_get_handle(L, 1);
+	int signal_id = 0;
+	int exit_code = 0;
+	int res = coio_popen_get_status(fh, &signal_id, &exit_code);
+
+	switch (res) {
+		case POPEN_RUNNING:
+			lua_pushnil(L);
+			break;
+
+		case POPEN_KILLED:
+			lua_pushinteger(L, -signal_id);
+			break;
+
+		default:
+			lua_pushinteger(L, exit_code);
+			break;
+	}
+
+	return 1;
+}
+
+static int
+lbox_fio_popen_get_std_file_handle(struct lua_State *L)
+{
+	void* fh = fio_popen_get_handle(L, 1);
+	int file_no = lua_tonumber(L, 2);
+	int res = coio_popen_get_std_file_handle(fh, file_no);
+
+	if (res < 0)
+		lua_pushnil(L);
+	else
+		lua_pushinteger(L, res);
+	return 1;
+}
+
+static int
+lbox_fio_popen_kill(struct lua_State *L)
+{
+	void* fh = fio_popen_get_handle(L, 1);
+	int signal_id = lua_tonumber(L, 2);
+
+	int res = coio_popen_kill(fh, signal_id);
+	if (res < 0){
+		lua_pushnil(L);
+		lbox_fio_pushsyserror(L);
+		return 2;
+	} else {
+		lua_pushboolean(L, true);
+		return 1;
+	}
+}
+
+static int
+lbox_fio_popen_wait(struct lua_State *L)
+{
+	struct fio_popen_wrap *wrap = fio_popen_get_handle_wrap(L, 1);
+	void *fh = wrap->handle;
+	int timeout = lua_tonumber(L, 2);
+
+	/*
+	 * On success function returns an
+	 * exit code as a positive number
+	 * or signal id as a negative number.
+	 * If failed, rc is nul and err
+	 * contains a error message.
+	 */
+
+	int exit_code =0;
+	int res = coio_popen_wait(fh, timeout, &exit_code);
+	if (res < 0){
+		lua_pushnil(L);
+		lbox_fio_pushsyserror(L);
+		return 2;
+	} else {
+		/* Release the allocated resources */
+		coio_popen_destroy(fh);
+		wrap->handle = NULL;
+
+		lua_pushinteger(L, exit_code);
+		return 1;
+	}
+}
 
 
 void
 tarantool_lua_fio_init(struct lua_State *L)
 {
+	static const struct luaL_Reg popen_data_meta[] = {
+		{"__gc",	lbox_fio_popen_gc},
+		{NULL, NULL}
+	};
+	luaL_register_type(L, fio_popen_typename, popen_data_meta);
+
 	static const struct luaL_Reg fio_methods[] = {
 		{ "lstat",		lbox_fio_lstat			},
 		{ "stat",		lbox_fio_stat			},
@@ -747,6 +1029,13 @@ tarantool_lua_fio_init(struct lua_State *L)
 		{ "listdir",		lbox_fio_listdir		},
 		{ "fstat",		lbox_fio_fstat			},
 		{ "copyfile",		lbox_fio_copyfile,		},
+		{ "popen",		lbox_fio_popen			},
+		{ "popen_read",		lbox_fio_popen_read 		},
+		{ "popen_write",	lbox_fio_popen_write 		},
+		{ "popen_get_status",	lbox_fio_popen_get_status	},
+		{ "popen_get_std_file_handle",	lbox_fio_popen_get_std_file_handle },
+		{ "popen_kill",		lbox_fio_popen_kill 		},
+		{ "popen_wait",		lbox_fio_popen_wait 		},
 		{ NULL,			NULL				}
 	};
 	luaL_register(L, NULL, internal_methods);
diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 74a871134..e5b9bdf81 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -3,6 +3,7 @@
 local fio = require('fio')
 local ffi = require('ffi')
 local buffer = require('buffer')
+local signal = require('signal')
 
 ffi.cdef[[
     int umask(int mask);
@@ -15,6 +16,12 @@ local const_char_ptr_t = ffi.typeof('const char *')
 local internal = fio.internal
 fio.internal = nil
 
+fio.STDIN = 0
+fio.STDOUT = 1
+fio.STDERR = 2
+fio.PIPE = -2
+fio.DEVNULL = -3
+
 local function sprintf(fmt, ...)
     if select('#', ...) == 0 then
         return fmt
@@ -206,6 +213,261 @@ fio.open = function(path, flags, mode)
     return fh
 end
 
+local popen_methods = {}
+
+popen_methods.do_read = function(self, buf, size, seconds)
+    if size == nil or type(size) ~= 'number' then
+        error('fio.popen.read: invalid size argument')
+    end
+    if seconds == nil or type(seconds) ~= 'number' then
+        error('fio.popen.read: invalid seconds argument')
+    end
+
+    local tmpbuf
+    if not ffi.istype(const_char_ptr_t, buf) then
+        tmpbuf = buffer.ibuf()
+        buf = tmpbuf:reserve(size)
+    end
+
+    local res, output_no, err = internal.popen_read(self.fh, buf, size, seconds)
+    if res == nil then
+        if tmpbuf ~= nil then
+            tmpbuf:recycle()
+        end
+        return nil, nil, err
+    end
+
+    if tmpbuf ~= nil then
+        tmpbuf:alloc(res)
+        res = ffi.string(tmpbuf.rpos, tmpbuf:size())
+        tmpbuf:recycle()
+    end
+    return res, output_no
+end
+
+-- read stdout & stderr of the process started by fio.popen
+-- read() -> str, source
+-- read(buf) -> len, source
+-- read(size) -> str, source
+-- read(buf, size) -> len, source
+-- source contains id of the stream, fio.STDOUT or fio.STDERR
+popen_methods.read = function(self, buf, size)
+    if self.fh == nil then
+        return nil, nil, 'Invalid object'
+    end
+
+    if buf == nil and size == nil then
+        -- read()
+        size = 512
+    elseif ffi.istype(const_char_ptr_t, buf) and size == nil then
+        -- read(buf)
+        size = 512
+    elseif not ffi.istype(const_char_ptr_t, buf) and
+            buf ~= nil and size == nil then
+        -- read(size)
+        size = tonumber(buf)
+        buf = nil
+    elseif ffi.istype(const_char_ptr_t, buf) and size ~= nil then
+        -- read(buf, size)
+        size = tonumber(size)
+    else
+        error("fio.popen.read: invalid arguments")
+    end
+
+    return self:do_read(buf, size, -1)
+end
+
+-- read stdout & stderr of the process started by fio.popen
+-- read2(seconds) -> str, source
+-- read2(buf, seconds) -> len, source
+-- read2(size, seconds) -> str, source
+-- read2(buf, size, seconds) -> len, source
+-- source contains id of the stream, fio.STDOUT or fio.STDERR
+popen_methods.read2 = function(self, buf, size, seconds)
+    if self.fh == nil then
+        return nil, nil, 'Invalid object'
+    end
+
+    if not ffi.istype(const_char_ptr_t, buf) and buf ~= nil and
+            size == nil and seconds == nil then
+        -- read2(seconds)
+        seconds = tonumber(buf)
+        size = 512
+        buf = nil
+    elseif ffi.istype(const_char_ptr_t, buf) and size ~= nil and
+            seconds == nil then
+        -- read2(buf, seconds)
+        seconds = tonumber(size)
+        size = 512
+    elseif not ffi.istype(const_char_ptr_t, buf) and buf ~= nil and
+            size ~= nil and seconds == nil then
+        -- read2(size, seconds)
+        seconds = tonumber(size)
+        size = tonumber(buf)
+        buf = nil
+    elseif ffi.istype(const_char_ptr_t, buf) and size ~= nil and
+            seconds ~= nil then
+        -- read2(buf, size, seconds)
+        seconds = tonumber(seconds)
+        size = tonumber(size)
+    else
+        error("fio.popen.read2: invalid arguments")
+    end
+
+    return self:do_read(buf, size, seconds)
+end
+
+-- write(str)
+-- write(buf, len)
+popen_methods.write = function(self, data, len)
+    if type(data) == 'string' then
+        if len == nil then
+            len = string.len(data)
+        end
+    elseif not ffi.istype(const_char_ptr_t, data) then
+        data = tostring(data)
+        len = #data
+    end
+
+    local res, err = internal.popen_write(self.fh, data, tonumber(len))
+    if err ~= nil then
+        return false, err
+    end
+    return res >= 0
+end
+
+popen_methods.get_status = function(self)
+    if self.fh ~= nil then
+        return internal.popen_get_status(self.fh)
+    else
+        return self.exit_code
+    end
+end
+
+popen_methods.get_stdin = function (self)
+    if self.fh == nil then
+        return nil, 'Invalid object'
+    end
+
+    return internal.popen_get_std_file_handle(self.fh, fio.STDIN)
+end
+
+popen_methods.get_stdout = function (self)
+    if self.fh == nil then
+        return nil, 'Invalid object'
+    end
+
+    return internal.popen_get_std_file_handle(self.fh, fio.STDOUT)
+end
+
+popen_methods.get_stderr = function (self)
+    if self.fh == nil then
+        return nil, 'Invalid object'
+    end
+
+    return internal.popen_get_std_file_handle(self.fh, fio.STDERR)
+end
+
+popen_methods.kill = function(self, sig)
+    if self.fh == nil then
+        return nil, errno.strerror(errno.ESRCH)
+    end
+
+    if sig == nil then
+        sig = 'SIGTERM'
+    end
+    if type(sig) == 'string' then
+        sig = signal.c.signals[sig]
+        if sig == nil then
+            errno(errno.EINVAL)
+            return nil, sprintf("fio.popen.kill(): unknown signal: %s", sig)
+        end
+    else
+        sig = tonumber(sig)
+    end
+
+    return internal.popen_kill(self.fh, sig)
+end
+
+popen_methods.wait = function(self, timeout)
+    if self.fh == nil then
+        return nil, 'Invalid object'
+    end
+
+    if timeout == nil then
+        timeout = -1
+    else
+        timeout = tonumber(timeout)
+    end
+
+    local rc, err = internal.popen_wait(self.fh, timeout)
+    if rc ~= nil then
+        self.exit_code = tonumber(rc)
+        self.fh = nil
+        return rc
+    else
+        return nil,err
+    end
+end
+
+
+local popen_mt = { __index = popen_methods }
+
+fio.popen = function(params)
+    local argv = params.argv
+    local env = params.environment
+    local hstdin = params.stdin
+    local hstdout = params.stdout
+    local hstderr = params.stderr
+
+    if type(hstdin) == 'table' then
+        hstdin = hstdin.fh
+    end
+    if type(hstdout) == 'table' then
+        hstdout = hstdout.fh
+    end
+    if type(hstderr) == 'table' then
+        hstderr = hstderr.fh
+    end
+
+    if argv == nil or
+       type(argv) ~= 'table' or
+       table.getn(argv) < 1 then
+        local errmsg = [[Usage: fio.popen({parameters}),
+parameters - a table containing arguments to run a process.
+The following arguments are expected:
+
+argv - [mandatory] is a table of argument strings passed
+       to the new program.
+       By convention, the first of these strings should
+       contain the filename associated with the file
+       being executed.
+
+environment - [optional] is a table of strings,
+              conventionally of the form key=value,
+              which are passed as environment to the
+              new program.
+
+stdin  - [optional] overrides the child process's
+         standard input.
+stdout - [optional] overrides the child process's
+         standard output.
+stderr - [optional] overrides the child process's
+         standard error output.
+]]
+        error(errmsg)
+    end
+
+    local fh,err = internal.popen(argv, env, hstdin, hstdout, hstderr)
+    if err ~= nil then
+        return nil, err
+    end
+
+    local pobj = {fh = fh}
+    setmetatable(pobj, popen_mt)
+    return pobj
+end
+
 fio.pathjoin = function(...)
     local i, path = 1, nil
 
diff --git a/src/lua/init.c b/src/lua/init.c
index 5ddc5a4d8..b1da6bf4b 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -56,6 +56,7 @@
 #include "lua/msgpack.h"
 #include "lua/pickle.h"
 #include "lua/fio.h"
+#include "lua/lua_signal.h"
 #include "lua/httpc.h"
 #include "lua/utf8.h"
 #include "digest.h"
@@ -447,6 +448,7 @@ tarantool_lua_init(const char *tarantool_bin, int argc, char **argv)
 	tarantool_lua_errno_init(L);
 	tarantool_lua_error_init(L);
 	tarantool_lua_fio_init(L);
+	tarantool_lua_signal_init(L);
 	tarantool_lua_socket_init(L);
 	tarantool_lua_pickle_init(L);
 	tarantool_lua_digest_init(L);
diff --git a/src/lua/lua_signal.c b/src/lua/lua_signal.c
new file mode 100644
index 000000000..924b0ab51
--- /dev/null
+++ b/src/lua/lua_signal.c
@@ -0,0 +1,99 @@
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "lua/lua_signal.h"
+#include <sys/types.h>
+#include <signal.h>
+
+#include <lua.h>
+#include <lauxlib.h>
+#include <lualib.h>
+
+#include "lua/utils.h"
+
+#ifndef PUSHTABLE
+#define PUSHTABLE(name, method, value)	{	\
+	lua_pushliteral(L, name);		\
+	method(L, value);			\
+	lua_settable(L, -3);			\
+}
+#endif /*PUSHTABLE*/
+
+void
+tarantool_lua_signal_init(struct lua_State *L)
+{
+	static const struct luaL_Reg signal_methods[] = {
+		{ NULL,	NULL }
+	};
+
+	luaL_register_module(L, "signal", signal_methods);
+
+	lua_pushliteral(L, "c");
+	lua_newtable(L);
+
+	lua_pushliteral(L, "signals");
+	lua_newtable(L);
+
+	PUSHTABLE("SIGINT", lua_pushinteger, SIGINT);
+	PUSHTABLE("SIGILL", lua_pushinteger, SIGILL);
+	PUSHTABLE("SIGABRT", lua_pushinteger, SIGABRT);
+	PUSHTABLE("SIGFPE", lua_pushinteger, SIGFPE);
+	PUSHTABLE("SIGSEGV", lua_pushinteger, SIGSEGV);
+	PUSHTABLE("SIGTERM", lua_pushinteger, SIGTERM);
+
+	PUSHTABLE("SIGHUP", lua_pushinteger, SIGHUP);
+	PUSHTABLE("SIGQUIT", lua_pushinteger, SIGQUIT);
+	PUSHTABLE("SIGTRAP", lua_pushinteger, SIGTRAP);
+	PUSHTABLE("SIGKILL", lua_pushinteger, SIGKILL);
+	PUSHTABLE("SIGBUS", lua_pushinteger, SIGBUS);
+	PUSHTABLE("SIGSYS", lua_pushinteger, SIGSYS);
+	PUSHTABLE("SIGPIPE", lua_pushinteger, SIGPIPE);
+	PUSHTABLE("SIGALRM", lua_pushinteger, SIGALRM);
+
+	PUSHTABLE("SIGURG", lua_pushinteger, SIGURG);
+	PUSHTABLE("SIGSTOP", lua_pushinteger, SIGSTOP);
+	PUSHTABLE("SIGTSTP", lua_pushinteger, SIGTSTP);
+	PUSHTABLE("SIGCONT", lua_pushinteger, SIGCONT);
+	PUSHTABLE("SIGCHLD", lua_pushinteger, SIGCHLD);
+	PUSHTABLE("SIGTTIN", lua_pushinteger, SIGTTIN);
+	PUSHTABLE("SIGTTOU", lua_pushinteger, SIGTTOU);
+	PUSHTABLE("SIGPOLL", lua_pushinteger, SIGPOLL);
+	PUSHTABLE("SIGXCPU", lua_pushinteger, SIGXCPU);
+	PUSHTABLE("SIGXFSZ", lua_pushinteger, SIGXFSZ);
+	PUSHTABLE("SIGVTALRM", lua_pushinteger, SIGVTALRM);
+	PUSHTABLE("SIGPROF", lua_pushinteger, SIGPROF);
+	PUSHTABLE("SIGUSR1", lua_pushinteger, SIGUSR1);
+	PUSHTABLE("SIGUSR2", lua_pushinteger, SIGUSR2);
+	lua_settable(L, -3); /* "signals" */
+
+	lua_settable(L, -3); /* "c" */
+	lua_pop(L, 1);
+}
diff --git a/src/lua/lua_signal.h b/src/lua/lua_signal.h
new file mode 100644
index 000000000..814692077
--- /dev/null
+++ b/src/lua/lua_signal.h
@@ -0,0 +1,45 @@
+#ifndef INCLUDES_TARANTOOL_LUA_SIGNAL_H
+#define INCLUDES_TARANTOOL_LUA_SIGNAL_H
+
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct lua_State;
+void tarantool_lua_signal_init(struct lua_State *L);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /*INCLUDES_TARANTOOL_LUA_SIGNAL_H*/
diff --git a/src/main.cc b/src/main.cc
index 569ff4b5f..86258cc50 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -77,6 +77,7 @@
 #include "box/session.h"
 #include "systemd.h"
 #include "crypto/crypto.h"
+#include "coio_popen.h"
 
 static pid_t master_pid = getpid();
 static struct pidfh *pid_file_handle;
@@ -328,7 +329,8 @@ signal_reset()
 	    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 ||
+	    sigaction(SIGCHLD, &sa, NULL) == -1)
 		say_syserror("sigaction");
 
 	/* Unblock any signals blocked by libev. */
@@ -373,6 +375,23 @@ signal_init(void)
 		panic_syserror("sigaction");
 	}
 
+	struct sigaction sa_chld;
+	memset(&sa_chld, 0, sizeof(sa_chld));
+	sigemptyset(&sa_chld.sa_mask);
+
+	/*
+	 * SA_NOCLDWAIT - do not transform children
+	 * into zombies when they terminate.
+	 * SA_NOCLDSTOP - do not receive notification
+	 * when child processes stop
+	 */
+	sa_chld.sa_flags = SA_SIGINFO | SA_NOCLDSTOP | SA_NOCLDWAIT;
+	sa_chld.sa_sigaction = coio_popen_child_is_dead;
+
+	if (sigaction(SIGCHLD, &sa_chld, NULL) == -1) {
+		panic_syserror("sigaction SIGCHLD");
+	}
+
 	ev_signal_init(&ev_sigs[0], sig_checkpoint, SIGUSR1);
 	ev_signal_init(&ev_sigs[1], signal_cb, SIGINT);
 	ev_signal_init(&ev_sigs[2], signal_cb, SIGTERM);
diff --git a/test/box-tap/fio_popen.sample.txt b/test/box-tap/fio_popen.sample.txt
new file mode 100644
index 000000000..43cba8c65
--- /dev/null
+++ b/test/box-tap/fio_popen.sample.txt
@@ -0,0 +1,5 @@
+AAA
+BBB
+CCC
+DDD
+
diff --git a/test/box-tap/fio_popen.test.lua b/test/box-tap/fio_popen.test.lua
new file mode 100755
index 000000000..0c2bd7e70
--- /dev/null
+++ b/test/box-tap/fio_popen.test.lua
@@ -0,0 +1,189 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local fio = require('fio')
+local errno = require('errno')
+local fiber = require('fiber')
+local test = tap.test()
+
+test:plan(5+9+11+9+8)
+
+-- Preliminaries
+local function read_stdout(app)
+    local ss = ""
+
+    local s,src,err = app:read(128)
+
+    while s ~= nil and s ~= "" do
+        ss = ss .. s
+
+        s,src,err = app:read(128)
+    end
+
+    return ss
+end
+
+-- Test 1. Run application, check its status, kill and wait
+local app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
+                        stdout=fio.STDOUT,
+                        stderr=fio.STDOUT})
+test:isnt(app1, nil, "#1. Starting a existing application")
+
+local rc = app1:get_status()
+test:is(rc, nil, "#1. Process is running")
+
+rc,err = app1:kill()
+test:is(rc, true, "#1. Sending kill(15)")
+
+rc,err = app1:wait(5)
+test:is(rc, -15, "#1. Process was killed")
+
+rc = app1:get_status()
+test:is(rc, -15, "#1. Process was killed 2")
+
+app1 = nil
+
+-- Test 2. Run application, write to stdin, read from stdout
+app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
+                  stdout=fio.PIPE,
+                  stdin=fio.PIPE,
+                  stderr=fio.STDOUT})
+test:isnt(app1, nil, "#2. Starting a existing application")
+
+rc = app1:get_status()
+test:is(rc, nil, "#2. Process is running")
+
+local test2str = '123\n456\n789'
+
+app1:write(test2str)
+rc,src,err = app1:read(256)
+
+test:is(src, fio.STDOUT, "#2. Read from STDOUT")
+test:is(rc, test2str, "#2. Received exact string")
+
+test2str = 'abc\ndef\ngh'
+
+app1:write(test2str, 4)
+local rc2,src2,err = app1:read(6)
+
+test:is(err, nil, "#2. Reading ok")
+test:is(src2, fio.STDOUT, "#2. Read from STDOUT 2")
+test:is(rc2, 'abc\n', "#2. Received exact string 2")
+
+rc,err = app1:kill()
+test:is(rc, true, "#2. Sending kill(15)")
+
+rc,err = app1:wait()
+test:is(rc, -15, "#2. Process was killed")
+
+app1 = nil
+
+-- Test 3. read from stdout with timeout
+app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
+                  stdout=fio.PIPE,
+                  stdin=fio.PIPE,
+                  stderr=fio.STDOUT})
+test:isnt(app1, nil, "#3. Starting a existing application")
+test:is(app1:get_stderr(), nil, "#3. STDERR is redirected")
+test:isnt(app1:get_stdout(), nil, "#3. STDOUT is available")
+test:isnt(app1:get_stdin(), nil, "#3. STDIN is available")
+
+
+rc = app1:get_status()
+test:is(rc, nil, "#3. Process is running")
+
+rc,src,err = app1:read2(256, 2)
+
+local e = errno()
+test:is(e, errno.ETIMEDOUT, "#3. Timeout")
+
+
+local test2str = '123\n456\n789'
+
+app1:write(test2str)
+rc,src,err = app1:read2(256, 2)
+
+test:is(err, nil, "#3. Reading ok")
+test:is(src, fio.STDOUT, "#3. Read from STDOUT")
+test:is(rc, test2str, "#3. Received exact string")
+
+rc,err = app1:kill('SIGHUP')
+test:is(rc, true, "#3. Sending kill(1)")
+
+rc,err = app1:wait()
+test:is(rc, -1, "#3. Process was killed")
+
+app1 = nil
+
+-- Test 4. Redirect from file
+local build_path = os.getenv("BUILDDIR")
+local txt_filename = fio.pathjoin(build_path, 'test/box-tap/fio_popen.sample.txt')
+
+local txt_file = fio.open(txt_filename, {'O_RDONLY'})
+test:isnt(txt_file, nil, "#4. Open existing file for reading")
+
+app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
+                  stdout=fio.PIPE,
+                  stdin=txt_file,
+                  stderr=fio.STDOUT})
+test:isnt(app1, nil, "#4. Starting a existing application")
+test:is(app1:get_stderr(), nil, "#4. STDERR is redirected")
+test:isnt(app1:get_stdout(), nil, "#4. STDOUT is available")
+test:is(app1:get_stdin(), nil, "#4. STDIN is redirected")
+
+rc = app1:get_status()
+test:is(rc, nil, "#4. Process is running")
+
+rc,src,err = app1:read2(256, 2)
+
+test:is(src, fio.STDOUT, "#4. Read from STDOUT")
+
+local test2str = 'AAA\nBBB\nCCC\nDDD\n\n'
+
+test:is(rc, test2str, "#4. Received exact string")
+
+rc,err = app1:wait()
+test:is(rc, 0, "#4. Process's exited")
+
+app1 = nil
+txt_file:close()
+
+-- Test 5. Redirect output from one process to another
+local app_path = fio.pathjoin(build_path, 'test/box-tap/fio_popen_test1.sh')
+
+app1 = fio.popen({argv = {'/bin/sh', '-c', app_path},
+                  stdout=fio.PIPE,
+                  stderr=fio.STDOUT})
+
+test:isnt(app1, nil, "#5. Starting application 1")
+test:is(app1:get_stderr(), nil, "#5. STDERR is redirected")
+test:isnt(app1:get_stdout(), nil, "#5. STDOUT is available")
+test:isnt(app1:get_stdin(), nil, "#5. STDIN is available")
+
+fiber.sleep(1)
+
+local app2 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
+                  stdout=fio.PIPE,
+                  stdin=app1:get_stdout()})
+
+test:isnt(app2, nil, "#5. Starting application 2")
+
+local test2str = '1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n'
+
+rc = read_stdout(app2)
+
+test:is(rc, test2str, "#5. Received exact string")
+
+rc,err = app1:wait()
+test:is(rc, 0, "#5. Process's exited 1")
+
+rc,err = app2:wait()
+test:is(rc, 0, "#5. Process's exited 2")
+
+app1 = nil
+app2 = nil
+
+-- --------------------------------------------------------------
+test:check()
+os.exit(0)
+
diff --git a/test/box-tap/fio_popen_test1.sh b/test/box-tap/fio_popen_test1.sh
new file mode 100755
index 000000000..da54ee19a
--- /dev/null
+++ b/test/box-tap/fio_popen_test1.sh
@@ -0,0 +1,7 @@
+#!/bin/bash
+for i in {1..10}
+do
+	echo $i
+#	sleep 1
+done
+
diff --git a/third_party/libeio/eio.c b/third_party/libeio/eio.c
index 7351d5dda..e433b1b3b 100644
--- a/third_party/libeio/eio.c
+++ b/third_party/libeio/eio.c
@@ -1741,7 +1741,24 @@ static int eio_threads;
 static void ecb_cold
 eio_prefork()
 {
-    eio_threads = etp_set_max_parallel(EIO_POOL, 0);
+	/*
+	 * When fork() is called libeio shuts
+	 * down all working threads.
+	 * But it causes a deadlock if fork() was
+	 * called from the libeio thread.
+	 * To avoid this do not close the
+	 * thread who called fork().
+	 * This behaviour is acceptable for the
+	 * case when fork() is immediately followed
+	 * by exec().
+	 * To clone a process call fork() from the
+	 * main thread.
+	 */
+
+	if (etp_is_in_pool_thread())
+		eio_threads = etp_get_max_parallel(EIO_POOL);
+	else
+    		eio_threads = etp_set_max_parallel(EIO_POOL, 0);
 }
 
 static void ecb_cold
diff --git a/third_party/libeio/etp.c b/third_party/libeio/etp.c
index 42f7661eb..38c2d1f9b 100644
--- a/third_party/libeio/etp.c
+++ b/third_party/libeio/etp.c
@@ -164,6 +164,21 @@ struct etp_pool_user
    xmutex_t lock;
 };
 
+static __thread int is_eio_thread = 0;
+
+/**
+ * Check whether the current thread belongs to
+ * libeio thread pool or just a generic thread.
+ *
+ * @return 0 for generic thread
+ * @return 1 for libeio thread pool
+ **/
+ETP_API_DECL int ecb_cold
+etp_is_in_pool_thread()
+{
+	return is_eio_thread;
+}
+
 /* worker threads management */
 
 static void ecb_cold
@@ -322,6 +337,9 @@ X_THREAD_PROC (etp_proc)
   self.pool = pool;
   etp_pool_user user; /* per request */
 
+/* Distinguish libeio threads from the generic threads */
+  is_eio_thread = 1;
+
   etp_proc_init ();
 
   /* try to distribute timeouts somewhat evenly (nanosecond part) */
@@ -616,3 +634,13 @@ etp_set_max_parallel (etp_pool pool, unsigned int threads)
   X_UNLOCK (pool->lock);
   return retval;
 }
+
+ETP_API_DECL int ecb_cold
+etp_get_max_parallel (etp_pool pool)
+{
+	int retval;
+	X_LOCK   (pool->lock);
+	retval = pool->wanted;
+	X_UNLOCK (pool->lock);
+	return retval;
+}
diff --git a/third_party/libev/ev.c b/third_party/libev/ev.c
index 6a2648591..5fa8293a1 100644
--- a/third_party/libev/ev.c
+++ b/third_party/libev/ev.c
@@ -4214,6 +4214,8 @@ noinline
 void
 ev_signal_stop (EV_P_ ev_signal *w) EV_THROW
 {
+	if (!loop)
+		return;
   clear_pending (EV_A_ (W)w);
   if (expect_false (!ev_is_active (w)))
     return;
-- 
2.17.1

Re: [PATCH v2] core: Non-blocking io.popen

[Vladimir Davydov]

Hello,

Please see a few comments inline. Note I haven't reviewed all parts of
the patch yet, because it's quite big and there's already something to
address. I might write more later.

On Wed, May 29, 2019 at 10:08:09AM +0300, Stanislav Zudin wrote:
> Adds nonblocking implementation of popen.
> The method is available in namespace fio.
> fio.popen() returns an object providing facilities
> for dealing with standard input/output, getting
> status of the child process.
> 
> Closes #4031
> 
> @TarantoolBot document
> Title: Nonblocking fio.popen
> 
> handle, err = fio.popen(parameters)
> 
> fio.popen starts a process and redirects its input/output.
> 
> parameters - a table containing arguments to run a process.
> The following arguments are expected:
> 
> argv - [mandatory] is a table of argument strings passed to
> the new program. By convention, the first of these strings should
> contain the filename associated with the file being executed.
> 
> environment - [optional] is a table of strings, conventionally of the
> form key=value, which are passed as environment to the new program.

What environment is used by default? Parent's?

> 
> By default stdin, stdout,stderr of the associated process are
> available for writing/reading using object's methods
> handle:write() and handle:read().
> One can override default behavior to redirect streams to/from file
> or to input/output of another process or to the default input/output
> of the parent process.
> 
> stdin - [optional] overrides the child process's standard input.
> stdout - [optional] overrides the child process's standard output.
> stderr - [optional] overrides the the child process's standard
> error output.
> May accept one of the following values:
> Handle of the file open with fio.open()
> Handle of the standard input/output of another process,
> open by fio.popen
> A constant defining the parent's STDIN, STDOUT or STDERR.
> A constants:
> fio.PIPE - Opens a file descriptor available for reading/writing
> or redirection to/from another process.
> fio.DEVNULL - Makes fio.popen redirect the output to /dev/null.
> 
> On success returns an object providing methods for
> communication with the running process.
> Returns nil if underlying functions calls fail;
> in this case the second return value, err, contains a error message.
> 
> The object created by fio.popen provides the following methods:
> read()
> read2()
> write()
> kill()
> wait()
> get_status()
> get_stdin()
> get_stdout()
> get_stderr()
> 
> number handle:get_stdin()

I'm not quite sure about this, but it looks like we don't typically use
get_ prefix for getter functions in Lua, i.e. we would write simply
status(), stdin(), etc.  Not insisting though - the API is up to Georgy
and Alexander mainly.

> 
> Returns handle of the child process's standard input.
> The handle is available only if it wasn't redirected earlier.
> Use this handle to setup a redirection
> from file or other process to the input of the associated process.
> If handle is unavailable the method returns nil.
> 
> number handle:get_stdout()
> number handle:get_stderr()
> 
> Return STDOUT and STDIN of the associated process accordingly.
> See handle:get_stdin() for details.

Always? Even if stdin/stdout/stderrr is associated with a file or
/dev/null?

> 
> rc,err = handle:wait(timeout)
> 
> The wait() waits for the associated process to terminate
> and returns the exit status of the command.

I like when an API gives exactly one way to do every thing. Here you
can get process exit code by either checking wait() return code or by
calling status(), which is kinda ambiguous. Let's please make this
function return true/false on success/error so that there's the only
way to get process status.

> 
> timeout - an integer specifies number of seconds to wait.
> If the requested time has elapsed the method returns nil,
> the second return value, err, contains a error message.
> To distinguish timeout from the the other errors use errno.
> If timeout is nil, the method waits infinitely until
> the associated process is terminated.
> On success function returns an exit code as a positive number
> or signal id as a negative number.
> If failed, rc is nul and err contains a error message.
> 
> If the associated process is terminated, one can use the following
> methods get the exit status:
> 
> rc = handle:get_status()
> 
> returns nil if process is still running
>  >= 0 if process exited normally
>  < 0 if process was terminated by a signal

Better say

 == 0 if the process exited normally
 error code > 0 if the process terminated with an error
 -signal no < 0 if the process was killed by a signal

> 
> rc, err = handle:kill(sig)
> 
> The kill() sends a specified signal to the associated process.
> On success the method returns true, if failed - nil and error message.

Since you return 'true' on success, I think we better return 'false' on
error, not nil.

> If the sig is nil the default "SIGTERM" is being sent to the process.
> If the signal is unknown then the method fails (with error EINVAL).
> The following signals are acceptable:
> "SIGINT"
> "SIGILL"
> "SIGABRT"
> "SIGFPE"
> "SIGSEGV"
> "SIGTERM"
> 
> "SIGHUP"
> "SIGQUIT"
> "SIGTRAP"
> "SIGKILL"
> "SIGBUS"
> "SIGSYS"
> "SIGPIPE"
> "SIGALRM"
> 
> "SIGURG"
> "SIGSTOP"
> "SIGTSTP"
> "SIGCONT"
> "SIGCHLD"
> "SIGTTIN"
> "SIGTTOU"
> "SIGPOLL"
> "SIGXCPU"
> "SIGXFSZ"
> "SIGVTALRM"
> "SIGPROF"
> "SIGUSR1"
> "SIGUSR2"

Not really necessary to enumerate the signals here - they are standard.
An example of using SIGKILL instead of SIGTERM would be enough. Would be
cool to allow to pass signal numbers (may be not).

> 
> rc,src,err = handle:read(buffer,size)
> rc,src,err = handle:read2(buffer,size,seconds)
> 
> read stdout & stderr of the process started by fio.popen
> read() -> str, source
> read(buf) -> len, source
> read(size) -> str, source
> read(buf, size) -> len, source
> read2(seconds) -> str, source
> read2(buf,seconds) -> len, source
> read2(size,seconds) -> str, source
> read2(buf, size,seconds) -> len, source

Please use the same function name for both variants - read() - as
you can figure out what to do by looking at function arguments, no?

> 
> seconds - an integer specifies number of seconds to wait.
> If the requested time has elapsed the method returns nil.
> 
> src contains id of the stream: fio.STDOUT or fio.STDERR.
> If method failed the rc and src are nil and err contains error message.
> 
> rc, err = handle:write(data, length)
> Writes specified number of bytes
> On success returns number of written bytes.
> If failed the rc is nil and err contains an error message.

write() to the pipe may block if the child process isn't reading its
stdin hence this function should probably have timeout, too.

> ---
> Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-4031-nonblocking-popen

The code doesn't compile on Travis CI. Please fix it and in future check
Travis CI before submitting a patch.

> Issue: https://github.com/tarantool/tarantool/issues/4031
> 
>  src/CMakeLists.txt                |   1 +
>  src/lib/core/CMakeLists.txt       |   1 +
>  src/lib/core/coio_file.c          | 243 +++++++++++
>  src/lib/core/coio_file.h          |  30 ++
>  src/lib/core/coio_popen.c         | 661 ++++++++++++++++++++++++++++++
>  src/lib/core/coio_popen.h         | 243 +++++++++++
>  src/lib/core/coio_task.c          |   2 +
>  src/lib/core/fiber.h              |   4 +-
>  src/lua/fio.c                     | 289 +++++++++++++
>  src/lua/fio.lua                   | 262 ++++++++++++
>  src/lua/init.c                    |   2 +
>  src/lua/lua_signal.c              |  99 +++++
>  src/lua/lua_signal.h              |  45 ++
>  src/main.cc                       |  21 +-
>  test/box-tap/fio_popen.sample.txt |   5 +
>  test/box-tap/fio_popen.test.lua   | 189 +++++++++
>  test/box-tap/fio_popen_test1.sh   |   7 +
>  third_party/libeio/eio.c          |  19 +-
>  third_party/libeio/etp.c          |  28 ++
>  third_party/libev/ev.c            |   2 +
>  20 files changed, 2149 insertions(+), 4 deletions(-)
>  create mode 100644 src/lib/core/coio_popen.c
>  create mode 100644 src/lib/core/coio_popen.h
>  create mode 100644 src/lua/lua_signal.c
>  create mode 100644 src/lua/lua_signal.h
>  create mode 100644 test/box-tap/fio_popen.sample.txt
>  create mode 100755 test/box-tap/fio_popen.test.lua
>  create mode 100755 test/box-tap/fio_popen_test1.sh
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 68674d06a..cfe46dfe3 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -117,6 +117,7 @@ set (server_sources
>       lua/info.c
>       lua/string.c
>       lua/buffer.c
> +     lua/lua_signal.c
>       ${lua_sources}
>       ${PROJECT_SOURCE_DIR}/third_party/lua-yaml/lyaml.cc
>       ${PROJECT_SOURCE_DIR}/third_party/lua-yaml/b64.c
> diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
> index eb10b11c3..8b1f8d32e 100644
> --- a/src/lib/core/CMakeLists.txt
> +++ b/src/lib/core/CMakeLists.txt
> @@ -26,6 +26,7 @@ set(core_sources
>      trigger.cc
>      mpstream.c
>      port.c
> +    coio_popen.c
>  )
>  
>  if (TARGET_OS_NETBSD)
> diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
> index 3359f42bc..93956e533 100644
> --- a/src/lib/core/coio_file.c
> +++ b/src/lib/core/coio_file.c
> @@ -34,6 +34,7 @@
>  #include "say.h"
>  #include "fio.h"
>  #include "errinj.h"
> +#include "coio_popen.h"
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <dirent.h>
> @@ -103,6 +104,36 @@ struct coio_file_task {
>  			const char *source;
>  			const char *dest;
>  		} copyfile;
> +
> +		struct {
> +			char** argv;

We use C-style asterisk - put it closer to the variable name.

> +			int argc;
> +			char** environment;
> +			int environment_size;

argv/argc but environment/environment_size - inconsistent.
Please use the same naming convention, e.g.

  argv/argc and envp/envc

or

  args/arg_count and env/env_count

However, I don't think you really need the sizes anyway - in Unix's
execve NULL is used as an end marker, why not do the same?

Also, is there any particular reason to (ab)use coio_file_task rather
than simply using coio_call? This way you wouldn't need to mess with
coio_file.c at all and would be able to neatly keep the whole popen
implementation in one file - coio_popen.c.

> +			int stdin_fd;
> +			int stdout_fd;
> +			int stderr_fd;
> +			void *handle;

Please don't use void* for popen handle. You can declare a struct and
use pointers to it without having its body definition:

struct popen_handle;

struct popen_handle *handle;

The code would look more readable that way.

> +		} popen_params;
> +

> +		struct {
> +			void *handle;
> +		} pclose_params;

This one isn't used anywhere at all.

> +
> +		struct {
> +			void *handle;
> +			const void* buf;
> +			size_t count;
> +			size_t *written;
> +		} popen_write;
> +
> +		struct {
> +			void *handle;
> +			void *buf;
> +			size_t count;
> +			size_t *read_bytes;

'written', but 'read_bytes' - inconsistent. Pleas strive for consistency
everywhere in your code.

> +			int *output_number;
> +		} popen_read;
>  	};
>  };
>  
> @@ -631,3 +662,215 @@ coio_copyfile(const char *source, const char *dest)
>  	eio_req *req = eio_custom(coio_do_copyfile, 0, coio_complete, &eio);
>  	return coio_wait_done(req, &eio);
>  }
> +
> +static void
> +coio_do_popen(eio_req *req)
> +{
> +	struct coio_file_task *eio = (struct coio_file_task *)req->data;
> +	eio->popen_params.handle = coio_popen_impl(eio->popen_params.argv,
> +						   eio->popen_params.argc,
> +						   eio->popen_params.environment,
> +						   eio->popen_params.environment_size,
> +						   eio->popen_params.stdin_fd,
> +						   eio->popen_params.stdout_fd,
> +						   eio->popen_params.stderr_fd);
> +
> +	eio->result = 0;
> +	eio->errorno = errno;
> +}
> +
> +void *
> +coio_popen(char** argv, int argc,
> +	   char** environment, int environment_size,
> +	   int stdin_fd, int stdout_fd, int stderr_fd)
> +{
> +	INIT_COEIO_FILE(eio);
> +	eio.popen_params.argv = argv;
> +	eio.popen_params.argc = argc;
> +	eio.popen_params.environment = environment;
> +	eio.popen_params.environment_size = environment_size;
> +	eio.popen_params.stdin_fd = stdin_fd;
> +	eio.popen_params.stdout_fd = stdout_fd;
> +	eio.popen_params.stderr_fd = stderr_fd;
> +
> +	eio_req *req = eio_custom(coio_do_popen, 0,
> +				  coio_complete, &eio);
> +	coio_wait_done(req, &eio);
> +	return eio.popen_params.handle;
> +}
> +
> +static void
> +coio_do_popen_read(eio_req *req)
> +{
> +	struct coio_file_task *eio = (struct coio_file_task *)req->data;
> +
> +	int rc = coio_popen_try_to_read(eio->popen_read.handle,
> +					eio->popen_read.buf,
> +					eio->popen_read.count,
> +					eio->popen_read.read_bytes,
> +					eio->popen_read.output_number);
> +
> +	req->result = rc;
> +	req->errorno = errno;
> +}
> +
> +static int
> +coio_do_nonblock_popen_read(void *fh, void *buf, size_t count,
> +	size_t *read_bytes, int *source_id)
> +{
> +	INIT_COEIO_FILE(eio);
> +	eio.popen_read.buf = buf;
> +	eio.popen_read.count = count;
> +	eio.popen_read.handle = fh;
> +	eio.popen_read.read_bytes = read_bytes;
> +	eio.popen_read.output_number = source_id;
> +	eio_req *req = eio_custom(coio_do_popen_read, 0,
> +				  coio_complete, &eio);
> +	return coio_wait_done(req, &eio);
> +}
> +
> +ssize_t
> +coio_popen_read(void *fh, void *buf, size_t count,
> +		int *output_number, int timeout)
> +{
> +	size_t received = 0;
> +	int rc = coio_popen_try_to_read(fh, buf, count,
> +		&received, output_number);

You call the same function coio_popen_try_to_read from both tx and coio.
How's that? If it's blocking, it blocks tx, which is unacceptable. If it
isn't, why call it from coio at all?

> +	if (rc == 0)		/* The reading's succeeded */
> +		return (ssize_t)received;
> +	else if (rc == -1 && errno != EAGAIN)	/* Failed */
> +		return -1;
> +
> +	/* A blocking operation is expected */
> +
> +	time_t start_tt;
> +	time(&start_tt);

There's ev_monotonic_now() for checking time - it's much more
lightweight and precise.

> +
> +	bool in_progress;
> +	do {
> +		if (fiber_is_cancelled()) {
> +			errno = EINTR;
> +			return -1;
> +		}
> +
> +		if (timeout > 0) {
> +			time_t tt;
> +			time(&tt);
> +			if ((tt - start_tt) > timeout) {
> +				errno = ETIMEDOUT;
> +				return -1;
> +			}
> +		}
> +
> +		rc = coio_do_nonblock_popen_read(fh, buf, count,
> +			&received, output_number);
> +		in_progress = (rc == -1 && errno == EAGAIN);

Looks like busy waiting to me.

> +	} while (in_progress);
> +
> +	return (rc == 0) ? (ssize_t)received
> +			 : -1;
> +}
> +
> +static void
> +coio_do_popen_write(eio_req *req)
> +{
> +	struct coio_file_task *eio = (struct coio_file_task *)req->data;
> +
> +	int rc = coio_popen_try_to_write(eio->popen_write.handle,
> +					eio->popen_write.buf,
> +					eio->popen_write.count,
> +					eio->popen_write.written);
> +
> +	req->result = rc;
> +	req->errorno = errno;
> +}
> +
> +static int
> +coio_do_nonblock_popen_write(void *fh, const void *buf, size_t count,
> +	size_t *written)
> +{
> +	INIT_COEIO_FILE(eio);
> +	eio.popen_write.buf = buf;
> +	eio.popen_write.count = count;
> +	eio.popen_write.handle = fh;
> +	eio.popen_write.written = written;
> +	eio_req *req = eio_custom(coio_do_popen_write, 0,
> +				  coio_complete, &eio);
> +	return coio_wait_done(req, &eio);
> +}
> +
> +ssize_t
> +coio_popen_write(void *fh, const void *buf, size_t count)
> +{
> +	ssize_t  total = 0;
> +	size_t written = 0;
> +	int rc = coio_popen_try_to_write(fh, buf, count,
> +					&written);
> +	if (rc == 0 && written == count) /* The writing's succeeded */
> +		return (ssize_t)written;
> +	else if (rc == -1 && errno != EAGAIN) /* Failed */
> +		return -1;
> +
> +	/* A blocking operation is expected */
> +	bool in_progress;
> +
> +	do {
> +		if (fiber_is_cancelled()) {
> +			errno = EINTR;
> +			return -1;
> +		}
> +
> +		buf += written;		/* advance writing position */
> +		total += (ssize_t)written;
> +		count -= written;
> +
> +		if (count == 0)
> +			return total;
> +
> +		written = 0;
> +		rc = coio_do_nonblock_popen_write(fh, buf, count,
> +						  &written);
> +		in_progress = 	(rc == 0 && written < count) ||
> +				(rc == -1 && errno == EAGAIN);
> +	} while (in_progress);
> +
> +	return (rc == 0) ? total
> +			 : -1;
> +}
> +
> +int
> +coio_popen_wait(void *fh, int timeout, int *exit_code)
> +{

This function doesn't depend on coio_file infrastructure. Why define it
here at all?

> +	time_t start_tt;
> +	time(&start_tt);
> +
> +	do {
> +		/* Wait for SIGCHLD */
> +		int sig = 0;
> +		int code = 0;
> +
> +		int rc = coio_popen_get_status(fh, &sig, &code);
> +		if (rc != POPEN_RUNNING) {
> +			*exit_code = (rc == POPEN_EXITED) ? code
> +							  : -sig;
> +			return 0;
> +		}
> +
> +		/* Check for timeout */
> +		if (timeout > 0) {
> +			time_t tt;
> +			time(&tt);
> +			if ((tt - start_tt) > timeout) {
> +				errno = ETIMEDOUT;
> +				return -1;
> +			}
> +		}
> +
> +		fiber_yield_timeout(0);

Busy waiting...

> +
> +	} while(!fiber_is_cancelled());
> +
> +	errno = EINTR;
> +	return -1;
> +}
> +
> diff --git a/src/lib/core/coio_popen.c b/src/lib/core/coio_popen.c
> new file mode 100644
> index 000000000..b44cbf60b
> --- /dev/null
> +++ b/src/lib/core/coio_popen.c
> @@ -0,0 +1,661 @@
> +/*
> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include <stddef.h>
> +#include <signal.h>
> +#include "coio_popen.h"
> +#include "coio_task.h"
> +#include "fiber.h"
> +#include "say.h"
> +#include "fio.h"
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <dirent.h>
> +#include <sys/wait.h>
> +#include <paths.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <bits/types/siginfo_t.h>
> +#include <pthread.h>
> +
> +/*
> + * On OSX this global variable is not declared
> + * in <unistd.h>
> + */
> +extern char **environ;
> +
> +
> +struct popen_data {

popen_handle would be a better name, I guess.

Also, please use the same prefix for all data structures (popen_
everywhere or coio_popen_ everywhere, not intermittently).

> +	/* process id */
> +	pid_t pid;
> +	int fh[3];

This is rather fd, not fh.

> +	/*
> +	 * Three handles:
> +	 * [0] write to stdin of the child process
> +	 * [1] read from stdout of the child process
> +	 * [2] read from stderr of the child process
> +	 */

We usually put comments before variable definition, not after.
Anyway, would be prudent to say when this cases are valid. Only
for pipe or for regular files and /dev/null, as well?

> +
> +	/* Handle to /dev/null */
> +	int devnull_fd;
> +
> +	/* The ID of socket was read recently
> +	 * (STDERR_FILENO or STDOUT_FILENO */
> +	int prev_source;
> +
> +	/*
> +	 * Current process status.
> +	 * The SIGCHLD handler changes this status.
> +	 */
> +	enum popen_status status;
> +
> +	/* exit status of the associated process. */

This might sound pedantic (and it is), but we are very strict about
comment formatting. Please always start a comment from a capital letter
and end it with a dot.

> +	int exit_code;
> +
> +	/*
> +	 * Number of the signal that caused the
> +	 * assosiated process to terminate

Typo: assosiated => associated. Please enable spell checking and fix
typos in comments.

Please apply these minor comments to all your code.

> +	 */
> +	int signal_id;
> +
> +	/*
> +	 * The next entry in the global list
> +	 */
> +	struct popen_data* next;

We have rlist/stailq. No need to invent a list.

> +};
> +
> +void *
> +coio_popen_impl(char** argv, int argc,
> +	char** environment, int environment_size,
> +	int stdin_fd, int stdout_fd, int stderr_fd)
> +{
> +	pid_t pid;
> +	int socket_rd[2] = {-1,-1};
> +	int socket_wr[2] = {-1,-1};
> +	int socket_er[2] = {-1,-1};

It's not a socket. It's a pipe.

> +	errno = 0;
> +
> +	struct popen_data *data = popen_data_new();
> +	if (data == NULL)
> +		return NULL;
> +
> +	/* Setup a /dev/null if necessary */
> +	bool read_devnull = (stdin_fd == FIO_DEVNULL);
> +	bool write_devnull = (stdout_fd == FIO_DEVNULL) ||
> +			     (stderr_fd == FIO_DEVNULL);
> +	int devnull_flags = O_RDWR | O_CREAT;
> +	if (!read_devnull)
> +		devnull_flags = O_WRONLY | O_CREAT;
> +	else if (!write_devnull)
> +		devnull_flags = O_RDONLY | O_CREAT;
> +
> +	if (read_devnull || write_devnull) {
> +		data->devnull_fd = open("/dev/null", devnull_flags, 0666);
> +		if (data->devnull_fd < 0)
> +			goto on_error;
> +		else {
> +			if (stdin_fd == FIO_DEVNULL)
> +				stdin_fd = data->devnull_fd;
> +			if (stdout_fd == FIO_DEVNULL)
> +				stdout_fd = data->devnull_fd;
> +			if (stderr_fd == FIO_DEVNULL)
> +				stderr_fd = data->devnull_fd;
> +		}
> +	}
> +
> +	if (stdin_fd == FIO_PIPE) {
> +		/*
> +		 * Enable non-blocking for the parent side
> +		 * and close-on-exec on the child's side.
> +		 * The socketpair on OSX doesn't support
> +		 * SOCK_NONBLOCK & SOCK_CLOEXEC flags.
> +		 */
> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_rd) < 0 ||
> +		    fcntl(socket_rd[0], F_SETFL, O_NONBLOCK) < 0 ||
> +		    fcntl(socket_rd[1], F_SETFD, FD_CLOEXEC) < 0) {
> +			goto on_error;
> +		}
> +	}
> +
> +	if (stdout_fd == FIO_PIPE) {
> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_wr) < 0 ||
> +		    fcntl(socket_wr[0], F_SETFL, O_NONBLOCK) < 0 ||
> +		    fcntl(socket_wr[1], F_SETFD, FD_CLOEXEC) < 0) {
> +			goto on_error;
> +		}
> +	}
> +
> +	if (stderr_fd == FIO_PIPE) {
> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_er) < 0 ||
> +		    fcntl(socket_er[0], F_SETFL, O_NONBLOCK) < 0 ||
> +		    fcntl(socket_er[1], F_SETFD, FD_CLOEXEC) < 0) {
> +			goto on_error;
> +		}
> +	}
> +
> +	pid = fork();

Please use vfork().

> +int
> +coio_popen_try_to_read(void *fh, void *buf, size_t count,
> +	size_t *read_bytes, int *source_id)
> +{
> +	struct popen_data *data = (struct popen_data *)fh;
> +
> +	if (data == NULL){
> +		errno = EBADF;
> +		return -1;
> +	}
> +
> +	fd_set rfds;
> +	FD_ZERO(&rfds);
> +	ssize_t received = 0;
> +	int num = 0;
> +
> +	if (data->fh[STDOUT_FILENO] >= 0) {
> +		FD_SET(data->fh[STDOUT_FILENO], &rfds);
> +		++num;
> +	}
> +	if (data->fh[STDERR_FILENO] >= 0) {
> +		FD_SET(data->fh[STDERR_FILENO], &rfds);
> +		++num;
> +	}
> +
> +	if (num == 0) {
> +		/*
> +		 * There are no open handles for reading
> +		 */
> +		errno = EBADF;
> +		return -1;
> +	}
> +
> +	struct timeval tv = {0,0};
> +	int max_h = MAX(data->fh[STDOUT_FILENO],
> +			data->fh[STDERR_FILENO]);
> +
> +	errno = 0;
> +	int retv = select(max_h + 1, &rfds, NULL, NULL, &tv);

What's going on here? Would be nice to see a friendly comment.

Anyway, why don't you use evio instead - it already uses select() under
the hood, see ev_io_start.

> +void
> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *context)
> +{
> +	(void)context;	/* UNUSED */
> +
> +	if (sig != SIGCHLD)
> +		return;
> +
> +	/*
> +	 * The sigaction is called with SA_NOCLDWAIT,
> +	 * so no need to call waitpid()
> +	 */
> +
> +	popen_lock_data_list();

This function is called from a signal handler => it might deadlock with
coio_popen_destroy, for instance. Please consider using evio signal
handlers (take a look at ev_signal_init).

> +
> +	struct popen_data *data = popen_lookup_data_by_pid(si->si_pid);
> +	if (data) {
> +		switch (si->si_code) {
> +		case CLD_EXITED:
> +			data->exit_code = si->si_status;
> +			data->status = POPEN_EXITED;
> +			break;
> +		case CLD_KILLED:
> +			data->signal_id = si->si_status;
> +			data->status = POPEN_KILLED;
> +			break;
> +		case CLD_DUMPED:
> +			/* exit_status makes no sense */
> +			data->status = POPEN_DUMPED;
> +			break;
> +		}
> +
> +		/*
> +		 * We shouldn't close file descriptors here.
> +		 * The child process may exit earlier than
> +		 * the parent process finishes reading data.
> +		 * In this case the reading fails.
> +		 */
> +	}
> +
> +	popen_unlock_data_list();
> +}
> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
> index fb168e25e..614b8fefe 100644
> --- a/src/lib/core/fiber.h
> +++ b/src/lib/core/fiber.h
> @@ -494,8 +494,8 @@ struct cord {
>  extern __thread struct cord *cord_ptr;
>  
>  #define cord() cord_ptr
> -#define fiber() cord()->fiber
> -#define loop() (cord()->loop)
> +#define fiber() (cord() ? cord()->fiber : NULL)
> +#define loop() (cord() ? cord()->loop : NULL)

Why is that? A comment would be nice to have.

> diff --git a/src/lua/lua_signal.c b/src/lua/lua_signal.c
> new file mode 100644
> index 000000000..924b0ab51
> --- /dev/null
> +++ b/src/lua/lua_signal.c
> @@ -0,0 +1,99 @@
> +/*
> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include "lua/lua_signal.h"
> +#include <sys/types.h>
> +#include <signal.h>
> +
> +#include <lua.h>
> +#include <lauxlib.h>
> +#include <lualib.h>
> +
> +#include "lua/utils.h"
> +
> +#ifndef PUSHTABLE
> +#define PUSHTABLE(name, method, value)	{	\
> +	lua_pushliteral(L, name);		\
> +	method(L, value);			\
> +	lua_settable(L, -3);			\
> +}
> +#endif /*PUSHTABLE*/
> +
> +void
> +tarantool_lua_signal_init(struct lua_State *L)
> +{
> +	static const struct luaL_Reg signal_methods[] = {
> +		{ NULL,	NULL }
> +	};
> +
> +	luaL_register_module(L, "signal", signal_methods);
> +
> +	lua_pushliteral(L, "c");
> +	lua_newtable(L);
> +
> +	lua_pushliteral(L, "signals");
> +	lua_newtable(L);
> +
> +	PUSHTABLE("SIGINT", lua_pushinteger, SIGINT);
> +	PUSHTABLE("SIGILL", lua_pushinteger, SIGILL);
> +	PUSHTABLE("SIGABRT", lua_pushinteger, SIGABRT);
> +	PUSHTABLE("SIGFPE", lua_pushinteger, SIGFPE);
> +	PUSHTABLE("SIGSEGV", lua_pushinteger, SIGSEGV);
> +	PUSHTABLE("SIGTERM", lua_pushinteger, SIGTERM);
> +
> +	PUSHTABLE("SIGHUP", lua_pushinteger, SIGHUP);
> +	PUSHTABLE("SIGQUIT", lua_pushinteger, SIGQUIT);
> +	PUSHTABLE("SIGTRAP", lua_pushinteger, SIGTRAP);
> +	PUSHTABLE("SIGKILL", lua_pushinteger, SIGKILL);
> +	PUSHTABLE("SIGBUS", lua_pushinteger, SIGBUS);
> +	PUSHTABLE("SIGSYS", lua_pushinteger, SIGSYS);
> +	PUSHTABLE("SIGPIPE", lua_pushinteger, SIGPIPE);
> +	PUSHTABLE("SIGALRM", lua_pushinteger, SIGALRM);
> +
> +	PUSHTABLE("SIGURG", lua_pushinteger, SIGURG);
> +	PUSHTABLE("SIGSTOP", lua_pushinteger, SIGSTOP);
> +	PUSHTABLE("SIGTSTP", lua_pushinteger, SIGTSTP);
> +	PUSHTABLE("SIGCONT", lua_pushinteger, SIGCONT);
> +	PUSHTABLE("SIGCHLD", lua_pushinteger, SIGCHLD);
> +	PUSHTABLE("SIGTTIN", lua_pushinteger, SIGTTIN);
> +	PUSHTABLE("SIGTTOU", lua_pushinteger, SIGTTOU);
> +	PUSHTABLE("SIGPOLL", lua_pushinteger, SIGPOLL);
> +	PUSHTABLE("SIGXCPU", lua_pushinteger, SIGXCPU);
> +	PUSHTABLE("SIGXFSZ", lua_pushinteger, SIGXFSZ);
> +	PUSHTABLE("SIGVTALRM", lua_pushinteger, SIGVTALRM);
> +	PUSHTABLE("SIGPROF", lua_pushinteger, SIGPROF);
> +	PUSHTABLE("SIGUSR1", lua_pushinteger, SIGUSR1);
> +	PUSHTABLE("SIGUSR2", lua_pushinteger, SIGUSR2);
> +	lua_settable(L, -3); /* "signals" */

This could be done from Lua - signal numbers are standard AFAIK.

> +
> +	lua_settable(L, -3); /* "c" */
> +	lua_pop(L, 1);
> +}
> diff --git a/test/box-tap/fio_popen.sample.txt b/test/box-tap/fio_popen.sample.txt
> new file mode 100644
> index 000000000..43cba8c65
> --- /dev/null
> +++ b/test/box-tap/fio_popen.sample.txt
> @@ -0,0 +1,5 @@
> +AAA
> +BBB
> +CCC
> +DDD
> +

I think this file should be created by the test in tmp dir. Just don't
forget to delete it.

> diff --git a/test/box-tap/fio_popen.test.lua b/test/box-tap/fio_popen.test.lua

Should be app-tap? Anyway, why tap test? Normal tests are easier to
write and understand IMO.

> new file mode 100755
> index 000000000..0c2bd7e70
> --- /dev/null
> +++ b/test/box-tap/fio_popen.test.lua
> @@ -0,0 +1,189 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local fio = require('fio')
> +local errno = require('errno')
> +local fiber = require('fiber')
> +local test = tap.test()
> +
> +test:plan(5+9+11+9+8)
> +
> +-- Preliminaries
> +local function read_stdout(app)
> +    local ss = ""
> +
> +    local s,src,err = app:read(128)
> +
> +    while s ~= nil and s ~= "" do
> +        ss = ss .. s
> +
> +        s,src,err = app:read(128)
> +    end
> +
> +    return ss
> +end
> +
> +-- Test 1. Run application, check its status, kill and wait
> +local app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},

Please test passing custom env as well.

> +                        stdout=fio.STDOUT,
> +                        stderr=fio.STDOUT})
> +test:isnt(app1, nil, "#1. Starting a existing application")
> +
> +local rc = app1:get_status()
> +test:is(rc, nil, "#1. Process is running")
> +
> +rc,err = app1:kill()
> +test:is(rc, true, "#1. Sending kill(15)")
> +
> +rc,err = app1:wait(5)
> +test:is(rc, -15, "#1. Process was killed")
> +
> +rc = app1:get_status()
> +test:is(rc, -15, "#1. Process was killed 2")
> +
> +app1 = nil
> +
> +-- Test 2. Run application, write to stdin, read from stdout
> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},

What's the point using '/bin/sh' just to run 'cat'?

> +                  stdout=fio.PIPE,
> +                  stdin=fio.PIPE,
> +                  stderr=fio.STDOUT})
> +test:isnt(app1, nil, "#2. Starting a existing application")
> +
> +rc = app1:get_status()
> +test:is(rc, nil, "#2. Process is running")
> +
> +local test2str = '123\n456\n789'
> +
> +app1:write(test2str)
> +rc,src,err = app1:read(256)
> +
> +test:is(src, fio.STDOUT, "#2. Read from STDOUT")
> +test:is(rc, test2str, "#2. Received exact string")
> +
> +test2str = 'abc\ndef\ngh'
> +
> +app1:write(test2str, 4)
> +local rc2,src2,err = app1:read(6)
> +
> +test:is(err, nil, "#2. Reading ok")
> +test:is(src2, fio.STDOUT, "#2. Read from STDOUT 2")
> +test:is(rc2, 'abc\n', "#2. Received exact string 2")
> +
> +rc,err = app1:kill()
> +test:is(rc, true, "#2. Sending kill(15)")
> +
> +rc,err = app1:wait()
> +test:is(rc, -15, "#2. Process was killed")
> +
> +app1 = nil
> +
> +-- Test 3. read from stdout with timeout
> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> +                  stdout=fio.PIPE,
> +                  stdin=fio.PIPE,
> +                  stderr=fio.STDOUT})
> +test:isnt(app1, nil, "#3. Starting a existing application")
> +test:is(app1:get_stderr(), nil, "#3. STDERR is redirected")
> +test:isnt(app1:get_stdout(), nil, "#3. STDOUT is available")
> +test:isnt(app1:get_stdin(), nil, "#3. STDIN is available")
> +
> +
> +rc = app1:get_status()
> +test:is(rc, nil, "#3. Process is running")
> +
> +rc,src,err = app1:read2(256, 2)

Please use smaller timeouts - we want tests to run real fast.

> +
> +local e = errno()
> +test:is(e, errno.ETIMEDOUT, "#3. Timeout")
> +
> +
> +local test2str = '123\n456\n789'
> +
> +app1:write(test2str)
> +rc,src,err = app1:read2(256, 2)
> +
> +test:is(err, nil, "#3. Reading ok")
> +test:is(src, fio.STDOUT, "#3. Read from STDOUT")
> +test:is(rc, test2str, "#3. Received exact string")
> +
> +rc,err = app1:kill('SIGHUP')
> +test:is(rc, true, "#3. Sending kill(1)")
> +
> +rc,err = app1:wait()
> +test:is(rc, -1, "#3. Process was killed")
> +
> +app1 = nil
> +
> +-- Test 4. Redirect from file
> +local build_path = os.getenv("BUILDDIR")
> +local txt_filename = fio.pathjoin(build_path, 'test/box-tap/fio_popen.sample.txt')
> +
> +local txt_file = fio.open(txt_filename, {'O_RDONLY'})
> +test:isnt(txt_file, nil, "#4. Open existing file for reading")
> +
> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> +                  stdout=fio.PIPE,
> +                  stdin=txt_file,
> +                  stderr=fio.STDOUT})
> +test:isnt(app1, nil, "#4. Starting a existing application")
> +test:is(app1:get_stderr(), nil, "#4. STDERR is redirected")
> +test:isnt(app1:get_stdout(), nil, "#4. STDOUT is available")
> +test:is(app1:get_stdin(), nil, "#4. STDIN is redirected")
> +
> +rc = app1:get_status()
> +test:is(rc, nil, "#4. Process is running")
> +
> +rc,src,err = app1:read2(256, 2)
> +
> +test:is(src, fio.STDOUT, "#4. Read from STDOUT")
> +
> +local test2str = 'AAA\nBBB\nCCC\nDDD\n\n'
> +
> +test:is(rc, test2str, "#4. Received exact string")
> +
> +rc,err = app1:wait()
> +test:is(rc, 0, "#4. Process's exited")
> +
> +app1 = nil
> +txt_file:close()
> +
> +-- Test 5. Redirect output from one process to another
> +local app_path = fio.pathjoin(build_path, 'test/box-tap/fio_popen_test1.sh')
> +
> +app1 = fio.popen({argv = {'/bin/sh', '-c', app_path},
> +                  stdout=fio.PIPE,
> +                  stderr=fio.STDOUT})
> +
> +test:isnt(app1, nil, "#5. Starting application 1")
> +test:is(app1:get_stderr(), nil, "#5. STDERR is redirected")
> +test:isnt(app1:get_stdout(), nil, "#5. STDOUT is available")
> +test:isnt(app1:get_stdin(), nil, "#5. STDIN is available")
> +
> +fiber.sleep(1)
> +
> +local app2 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> +                  stdout=fio.PIPE,
> +                  stdin=app1:get_stdout()})
> +
> +test:isnt(app2, nil, "#5. Starting application 2")
> +
> +local test2str = '1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n'
> +
> +rc = read_stdout(app2)
> +
> +test:is(rc, test2str, "#5. Received exact string")
> +
> +rc,err = app1:wait()
> +test:is(rc, 0, "#5. Process's exited 1")
> +
> +rc,err = app2:wait()
> +test:is(rc, 0, "#5. Process's exited 2")
> +
> +app1 = nil
> +app2 = nil
> +
> +-- --------------------------------------------------------------
> +test:check()
> +os.exit(0)
> +
> diff --git a/test/box-tap/fio_popen_test1.sh b/test/box-tap/fio_popen_test1.sh
> new file mode 100755
> index 000000000..da54ee19a
> --- /dev/null
> +++ b/test/box-tap/fio_popen_test1.sh
> @@ -0,0 +1,7 @@
> +#!/bin/bash
> +for i in {1..10}
> +do
> +	echo $i
> +#	sleep 1

Commented line. Please remove.

> +done

Would be nice to see a test that reads/writes much more to the pipes.

> +
> diff --git a/third_party/libeio/eio.c b/third_party/libeio/eio.c
> index 7351d5dda..e433b1b3b 100644
> --- a/third_party/libeio/eio.c
> +++ b/third_party/libeio/eio.c
> @@ -1741,7 +1741,24 @@ static int eio_threads;
>  static void ecb_cold
>  eio_prefork()
>  {
> -    eio_threads = etp_set_max_parallel(EIO_POOL, 0);
> +	/*
> +	 * When fork() is called libeio shuts
> +	 * down all working threads.
> +	 * But it causes a deadlock if fork() was
> +	 * called from the libeio thread.
> +	 * To avoid this do not close the
> +	 * thread who called fork().
> +	 * This behaviour is acceptable for the
> +	 * case when fork() is immediately followed
> +	 * by exec().
> +	 * To clone a process call fork() from the
> +	 * main thread.
> +	 */
> +
> +	if (etp_is_in_pool_thread())
> +		eio_threads = etp_get_max_parallel(EIO_POOL);
> +	else
> +    		eio_threads = etp_set_max_parallel(EIO_POOL, 0);

Please investigate why it was initially done that way, because
I'm afraid this change might break something.

>  }
>  
>  static void ecb_cold
> diff --git a/third_party/libeio/etp.c b/third_party/libeio/etp.c
> index 42f7661eb..38c2d1f9b 100644
> --- a/third_party/libeio/etp.c
> +++ b/third_party/libeio/etp.c
> @@ -164,6 +164,21 @@ struct etp_pool_user
>     xmutex_t lock;
>  };
>  
> +static __thread int is_eio_thread = 0;
> +
> +/**
> + * Check whether the current thread belongs to
> + * libeio thread pool or just a generic thread.
> + *
> + * @return 0 for generic thread
> + * @return 1 for libeio thread pool
> + **/
> +ETP_API_DECL int ecb_cold
> +etp_is_in_pool_thread()
> +{
> +	return is_eio_thread;
> +}
> +
>  /* worker threads management */
>  
>  static void ecb_cold
> @@ -322,6 +337,9 @@ X_THREAD_PROC (etp_proc)
>    self.pool = pool;
>    etp_pool_user user; /* per request */
>  
> +/* Distinguish libeio threads from the generic threads */
> +  is_eio_thread = 1;
> +
>    etp_proc_init ();
>  
>    /* try to distribute timeouts somewhat evenly (nanosecond part) */
> @@ -616,3 +634,13 @@ etp_set_max_parallel (etp_pool pool, unsigned int threads)
>    X_UNLOCK (pool->lock);
>    return retval;
>  }
> +
> +ETP_API_DECL int ecb_cold
> +etp_get_max_parallel (etp_pool pool)
> +{
> +	int retval;
> +	X_LOCK   (pool->lock);
> +	retval = pool->wanted;
> +	X_UNLOCK (pool->lock);
> +	return retval;
> +}
> diff --git a/third_party/libev/ev.c b/third_party/libev/ev.c
> index 6a2648591..5fa8293a1 100644
> --- a/third_party/libev/ev.c
> +++ b/third_party/libev/ev.c
> @@ -4214,6 +4214,8 @@ noinline
>  void
>  ev_signal_stop (EV_P_ ev_signal *w) EV_THROW
>  {
> +	if (!loop)
> +		return;

Why is that? At least deserves a comment. Better handle it at the upper
level.

Anyway, all these changes to libev deserve a separate patch with a
proper change log comment.

>    clear_pending (EV_A_ (W)w);
>    if (expect_false (!ev_is_active (w)))
>      return;

[tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/30 22:40]:
> > number handle:get_stdin()
> 
> I'm not quite sure about this, but it looks like we don't typically use
> get_ prefix for getter functions in Lua, i.e. we would write simply
> status(), stdin(), etc.  Not insisting though - the API is up to Georgy
> and Alexander mainly.

We add get_ prefix only if there is a set_ in the same api,
otherwise not.


-- 
Konstantin Osipov, Moscow, Russia

Re: [tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Stanislav Zudin]

On 30.05.2019 21:34, Vladimir Davydov wrote:
> Hello,
> 
> Please see a few comments inline. Note I haven't reviewed all parts of
> the patch yet, because it's quite big and there's already something to
> address. I might write more later.
> 
> On Wed, May 29, 2019 at 10:08:09AM +0300, Stanislav Zudin wrote:
>> Adds nonblocking implementation of popen.
>> The method is available in namespace fio.
>> fio.popen() returns an object providing facilities
>> for dealing with standard input/output, getting
>> status of the child process.
>>
>> Closes #4031
>>
>> @TarantoolBot document
>> Title: Nonblocking fio.popen
>>
>> handle, err = fio.popen(parameters)
>>
>> fio.popen starts a process and redirects its input/output.
>>
>> parameters - a table containing arguments to run a process.
>> The following arguments are expected:
>>
>> argv - [mandatory] is a table of argument strings passed to
>> the new program. By convention, the first of these strings should
>> contain the filename associated with the file being executed.
>>
>> environment - [optional] is a table of strings, conventionally of the
>> form key=value, which are passed as environment to the new program.
> 
> What environment is used by default? Parent's?

yep

> 
>>
>> By default stdin, stdout,stderr of the associated process are
>> available for writing/reading using object's methods
>> handle:write() and handle:read().
>> One can override default behavior to redirect streams to/from file
>> or to input/output of another process or to the default input/output
>> of the parent process.
>>
>> stdin - [optional] overrides the child process's standard input.
>> stdout - [optional] overrides the child process's standard output.
>> stderr - [optional] overrides the the child process's standard
>> error output.
>> May accept one of the following values:
>> Handle of the file open with fio.open()
>> Handle of the standard input/output of another process,
>> open by fio.popen
>> A constant defining the parent's STDIN, STDOUT or STDERR.
>> A constants:
>> fio.PIPE - Opens a file descriptor available for reading/writing
>> or redirection to/from another process.
>> fio.DEVNULL - Makes fio.popen redirect the output to /dev/null.
>>
>> On success returns an object providing methods for
>> communication with the running process.
>> Returns nil if underlying functions calls fail;
>> in this case the second return value, err, contains a error message.
>>
>> The object created by fio.popen provides the following methods:
>> read()
>> read2()
>> write()
>> kill()
>> wait()
>> get_status()
>> get_stdin()
>> get_stdout()
>> get_stderr()
>>
>> number handle:get_stdin()
> 
> I'm not quite sure about this, but it looks like we don't typically use
> get_ prefix for getter functions in Lua, i.e. we would write simply
> status(), stdin(), etc.  Not insisting though - the API is up to Georgy
> and Alexander mainly.
> 

Renamed methods to status(), stdin(), stdout(), stderr().

>>
>> Returns handle of the child process's standard input.
>> The handle is available only if it wasn't redirected earlier.
>> Use this handle to setup a redirection
>> from file or other process to the input of the associated process.
>> If handle is unavailable the method returns nil.
>>
>> number handle:get_stdout()
>> number handle:get_stderr()
>>
>> Return STDOUT and STDIN of the associated process accordingly.
>> See handle:get_stdin() for details.
> 
> Always? Even if stdin/stdout/stderrr is associated with a file or
> /dev/null?

Updated the comment.
"The handle is available only if it was created with fio.PIPE option."

> 
>>
>> rc,err = handle:wait(timeout)
>>
>> The wait() waits for the associated process to terminate
>> and returns the exit status of the command.
> 
> I like when an API gives exactly one way to do every thing. Here you
> can get process exit code by either checking wait() return code or by
> calling status(), which is kinda ambiguous. Let's please make this
> function return true/false on success/error so that there's the only
> way to get process status.

Done.

> 
>>
>> timeout - an integer specifies number of seconds to wait.
>> If the requested time has elapsed the method returns nil,
>> the second return value, err, contains a error message.
>> To distinguish timeout from the the other errors use errno.
>> If timeout is nil, the method waits infinitely until
>> the associated process is terminated.
>> On success function returns an exit code as a positive number
>> or signal id as a negative number.
>> If failed, rc is nul and err contains a error message.
>>
>> If the associated process is terminated, one can use the following
>> methods get the exit status:
>>
>> rc = handle:get_status()
>>
>> returns nil if process is still running
>>   >= 0 if process exited normally
>>   < 0 if process was terminated by a signal
> 
> Better say
> 
>   == 0 if the process exited normally
>   error code > 0 if the process terminated with an error
>   -signal no < 0 if the process was killed by a signal
> 

Fixed.

>>
>> rc, err = handle:kill(sig)
>>
>> The kill() sends a specified signal to the associated process.
>> On success the method returns true, if failed - nil and error message.
> 
> Since you return 'true' on success, I think we better return 'false' on
> error, not nil.

Fixed.

> 
>> If the sig is nil the default "SIGTERM" is being sent to the process.
>> If the signal is unknown then the method fails (with error EINVAL).
>> The following signals are acceptable:
>> "SIGINT"
>> "SIGILL"
>> "SIGABRT"
>> "SIGFPE"
>> "SIGSEGV"
>> "SIGTERM"
>>
>> "SIGHUP"
>> "SIGQUIT"
>> "SIGTRAP"
>> "SIGKILL"
>> "SIGBUS"
>> "SIGSYS"
>> "SIGPIPE"
>> "SIGALRM"
>>
>> "SIGURG"
>> "SIGSTOP"
>> "SIGTSTP"
>> "SIGCONT"
>> "SIGCHLD"
>> "SIGTTIN"
>> "SIGTTOU"
>> "SIGPOLL"
>> "SIGXCPU"
>> "SIGXFSZ"
>> "SIGVTALRM"
>> "SIGPROF"
>> "SIGUSR1"
>> "SIGUSR2"
> 
> Not really necessary to enumerate the signals here - they are standard.
> An example of using SIGKILL instead of SIGTERM would be enough. Would be
> cool to allow to pass signal numbers (may be not).

The numbers are acceptable although it's not documented.
Fixed.

> 
>>
>> rc,src,err = handle:read(buffer,size)
>> rc,src,err = handle:read2(buffer,size,seconds)
>>
>> read stdout & stderr of the process started by fio.popen
>> read() -> str, source
>> read(buf) -> len, source
>> read(size) -> str, source
>> read(buf, size) -> len, source
>> read2(seconds) -> str, source
>> read2(buf,seconds) -> len, source
>> read2(size,seconds) -> str, source
>> read2(buf, size,seconds) -> len, source
> 
> Please use the same function name for both variants - read() - as
> you can figure out what to do by looking at function arguments, no?
> 

Well, actually there is an obstacle.
The size and the seconds are both 'number' so we can't distinguish them.
The possible way is to pass the fixed number of arguments, e.g.
read(nil,size) instead of read(size),
read(buf, nil, seconds) instead of read2(buf,seconds)
and so on.
If this approach is acceptable then i'll make the changes.

>>
>> seconds - an integer specifies number of seconds to wait.
>> If the requested time has elapsed the method returns nil.
>>
>> src contains id of the stream: fio.STDOUT or fio.STDERR.
>> If method failed the rc and src are nil and err contains error message.
>>
>> rc, err = handle:write(data, length)
>> Writes specified number of bytes
>> On success returns number of written bytes.
>> If failed the rc is nil and err contains an error message.
> 
> write() to the pipe may block if the child process isn't reading its
> stdin hence this function should probably have timeout, too.

ok

> 
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-4031-nonblocking-popen
> 
> The code doesn't compile on Travis CI. Please fix it and in future check
> Travis CI before submitting a patch.

got it.

> 
>> Issue: https://github.com/tarantool/tarantool/issues/4031
>>
>>   src/CMakeLists.txt                |   1 +
>>   src/lib/core/CMakeLists.txt       |   1 +
>>   src/lib/core/coio_file.c          | 243 +++++++++++
>>   src/lib/core/coio_file.h          |  30 ++
>>   src/lib/core/coio_popen.c         | 661 ++++++++++++++++++++++++++++++
>>   src/lib/core/coio_popen.h         | 243 +++++++++++
>>   src/lib/core/coio_task.c          |   2 +
>>   src/lib/core/fiber.h              |   4 +-
>>   src/lua/fio.c                     | 289 +++++++++++++
>>   src/lua/fio.lua                   | 262 ++++++++++++
>>   src/lua/init.c                    |   2 +
>>   src/lua/lua_signal.c              |  99 +++++
>>   src/lua/lua_signal.h              |  45 ++
>>   src/main.cc                       |  21 +-
>>   test/box-tap/fio_popen.sample.txt |   5 +
>>   test/box-tap/fio_popen.test.lua   | 189 +++++++++
>>   test/box-tap/fio_popen_test1.sh   |   7 +
>>   third_party/libeio/eio.c          |  19 +-
>>   third_party/libeio/etp.c          |  28 ++
>>   third_party/libev/ev.c            |   2 +
>>   20 files changed, 2149 insertions(+), 4 deletions(-)
>>   create mode 100644 src/lib/core/coio_popen.c
>>   create mode 100644 src/lib/core/coio_popen.h
>>   create mode 100644 src/lua/lua_signal.c
>>   create mode 100644 src/lua/lua_signal.h
>>   create mode 100644 test/box-tap/fio_popen.sample.txt
>>   create mode 100755 test/box-tap/fio_popen.test.lua
>>   create mode 100755 test/box-tap/fio_popen_test1.sh
>>
>> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
>> index 68674d06a..cfe46dfe3 100644
>> --- a/src/CMakeLists.txt
>> +++ b/src/CMakeLists.txt
>> @@ -117,6 +117,7 @@ set (server_sources
>>        lua/info.c
>>        lua/string.c
>>        lua/buffer.c
>> +     lua/lua_signal.c
>>        ${lua_sources}
>>        ${PROJECT_SOURCE_DIR}/third_party/lua-yaml/lyaml.cc
>>        ${PROJECT_SOURCE_DIR}/third_party/lua-yaml/b64.c
>> diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
>> index eb10b11c3..8b1f8d32e 100644
>> --- a/src/lib/core/CMakeLists.txt
>> +++ b/src/lib/core/CMakeLists.txt
>> @@ -26,6 +26,7 @@ set(core_sources
>>       trigger.cc
>>       mpstream.c
>>       port.c
>> +    coio_popen.c
>>   )
>>   
>>   if (TARGET_OS_NETBSD)
>> diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
>> index 3359f42bc..93956e533 100644
>> --- a/src/lib/core/coio_file.c
>> +++ b/src/lib/core/coio_file.c
>> @@ -34,6 +34,7 @@
>>   #include "say.h"
>>   #include "fio.h"
>>   #include "errinj.h"
>> +#include "coio_popen.h"
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   #include <dirent.h>
>> @@ -103,6 +104,36 @@ struct coio_file_task {
>>   			const char *source;
>>   			const char *dest;
>>   		} copyfile;
>> +
>> +		struct {
>> +			char** argv;
> 
> We use C-style asterisk - put it closer to the variable name.
> 

Fixed.

>> +			int argc;
>> +			char** environment;
>> +			int environment_size;
> 
> argv/argc but environment/environment_size - inconsistent.
> Please use the same naming convention, e.g.
> 
>    argv/argc and envp/envc
> 

Fixed.

> or
> 
>    args/arg_count and env/env_count
> 
> However, I don't think you really need the sizes anyway - in Unix's
> execve NULL is used as an end marker, why not do the same?

Accepted.

> 
> Also, is there any particular reason to (ab)use coio_file_task rather
> than simply using coio_call? This way you wouldn't need to mess with
> coio_file.c at all and would be able to neatly keep the whole popen
> implementation in one file - coio_popen.c.

ok

> 
>> +			int stdin_fd;
>> +			int stdout_fd;
>> +			int stderr_fd;
>> +			void *handle;
> 
> Please don't use void* for popen handle. You can declare a struct and
> use pointers to it without having its body definition:
> 
> struct popen_handle;
> 
> struct popen_handle *handle;
> 
> The code would look more readable that way.

ok

> 
>> +		} popen_params;
>> +
> 
>> +		struct {
>> +			void *handle;
>> +		} pclose_params;
> 
> This one isn't used anywhere at all.

Fixed.

> 
>> +
>> +		struct {
>> +			void *handle;
>> +			const void* buf;
>> +			size_t count;
>> +			size_t *written;
>> +		} popen_write;
>> +
>> +		struct {
>> +			void *handle;
>> +			void *buf;
>> +			size_t count;
>> +			size_t *read_bytes;
> 
> 'written', but 'read_bytes' - inconsistent. Pleas strive for consistency
> everywhere in your code.

ok

> 
>> +			int *output_number;
>> +		} popen_read;
>>   	};
>>   };
>>   
>> @@ -631,3 +662,215 @@ coio_copyfile(const char *source, const char *dest)
>>   	eio_req *req = eio_custom(coio_do_copyfile, 0, coio_complete, &eio);
>>   	return coio_wait_done(req, &eio);
>>   }
>> +
>> +static void
>> +coio_do_popen(eio_req *req)
>> +{
>> +	struct coio_file_task *eio = (struct coio_file_task *)req->data;
>> +	eio->popen_params.handle = coio_popen_impl(eio->popen_params.argv,
>> +						   eio->popen_params.argc,
>> +						   eio->popen_params.environment,
>> +						   eio->popen_params.environment_size,
>> +						   eio->popen_params.stdin_fd,
>> +						   eio->popen_params.stdout_fd,
>> +						   eio->popen_params.stderr_fd);
>> +
>> +	eio->result = 0;
>> +	eio->errorno = errno;
>> +}
>> +
>> +void *
>> +coio_popen(char** argv, int argc,
>> +	   char** environment, int environment_size,
>> +	   int stdin_fd, int stdout_fd, int stderr_fd)
>> +{
>> +	INIT_COEIO_FILE(eio);
>> +	eio.popen_params.argv = argv;
>> +	eio.popen_params.argc = argc;
>> +	eio.popen_params.environment = environment;
>> +	eio.popen_params.environment_size = environment_size;
>> +	eio.popen_params.stdin_fd = stdin_fd;
>> +	eio.popen_params.stdout_fd = stdout_fd;
>> +	eio.popen_params.stderr_fd = stderr_fd;
>> +
>> +	eio_req *req = eio_custom(coio_do_popen, 0,
>> +				  coio_complete, &eio);
>> +	coio_wait_done(req, &eio);
>> +	return eio.popen_params.handle;
>> +}
>> +
>> +static void
>> +coio_do_popen_read(eio_req *req)
>> +{
>> +	struct coio_file_task *eio = (struct coio_file_task *)req->data;
>> +
>> +	int rc = coio_popen_try_to_read(eio->popen_read.handle,
>> +					eio->popen_read.buf,
>> +					eio->popen_read.count,
>> +					eio->popen_read.read_bytes,
>> +					eio->popen_read.output_number);
>> +
>> +	req->result = rc;
>> +	req->errorno = errno;
>> +}
>> +
>> +static int
>> +coio_do_nonblock_popen_read(void *fh, void *buf, size_t count,
>> +	size_t *read_bytes, int *source_id)
>> +{
>> +	INIT_COEIO_FILE(eio);
>> +	eio.popen_read.buf = buf;
>> +	eio.popen_read.count = count;
>> +	eio.popen_read.handle = fh;
>> +	eio.popen_read.read_bytes = read_bytes;
>> +	eio.popen_read.output_number = source_id;
>> +	eio_req *req = eio_custom(coio_do_popen_read, 0,
>> +				  coio_complete, &eio);
>> +	return coio_wait_done(req, &eio);
>> +}
>> +
>> +ssize_t
>> +coio_popen_read(void *fh, void *buf, size_t count,
>> +		int *output_number, int timeout)
>> +{
>> +	size_t received = 0;
>> +	int rc = coio_popen_try_to_read(fh, buf, count,
>> +		&received, output_number);
> 
> You call the same function coio_popen_try_to_read from both tx and coio.
> How's that? If it's blocking, it blocks tx, which is unacceptable. If it
> isn't, why call it from coio at all?

coio_popen_try_to_read is not blocking.
But somebody should call fiber_yield(). And coio_file.c contains all
facilities.

> 
>> +	if (rc == 0)		/* The reading's succeeded */
>> +		return (ssize_t)received;
>> +	else if (rc == -1 && errno != EAGAIN)	/* Failed */
>> +		return -1;
>> +
>> +	/* A blocking operation is expected */
>> +
>> +	time_t start_tt;
>> +	time(&start_tt);
> 
> There's ev_monotonic_now() for checking time - it's much more
> lightweight and precise.

ok

> 
>> +
>> +	bool in_progress;
>> +	do {
>> +		if (fiber_is_cancelled()) {
>> +			errno = EINTR;
>> +			return -1;
>> +		}
>> +
>> +		if (timeout > 0) {
>> +			time_t tt;
>> +			time(&tt);
>> +			if ((tt - start_tt) > timeout) {
>> +				errno = ETIMEDOUT;
>> +				return -1;
>> +			}
>> +		}
>> +
>> +		rc = coio_do_nonblock_popen_read(fh, buf, count,
>> +			&received, output_number);
>> +		in_progress = (rc == -1 && errno == EAGAIN);
> 
> Looks like busy waiting to me.
> 
>> +	} while (in_progress);
>> +
>> +	return (rc == 0) ? (ssize_t)received
>> +			 : -1;
>> +}
>> +
>> +static void
>> +coio_do_popen_write(eio_req *req)
>> +{
>> +	struct coio_file_task *eio = (struct coio_file_task *)req->data;
>> +
>> +	int rc = coio_popen_try_to_write(eio->popen_write.handle,
>> +					eio->popen_write.buf,
>> +					eio->popen_write.count,
>> +					eio->popen_write.written);
>> +
>> +	req->result = rc;
>> +	req->errorno = errno;
>> +}
>> +
>> +static int
>> +coio_do_nonblock_popen_write(void *fh, const void *buf, size_t count,
>> +	size_t *written)
>> +{
>> +	INIT_COEIO_FILE(eio);
>> +	eio.popen_write.buf = buf;
>> +	eio.popen_write.count = count;
>> +	eio.popen_write.handle = fh;
>> +	eio.popen_write.written = written;
>> +	eio_req *req = eio_custom(coio_do_popen_write, 0,
>> +				  coio_complete, &eio);
>> +	return coio_wait_done(req, &eio);
>> +}
>> +
>> +ssize_t
>> +coio_popen_write(void *fh, const void *buf, size_t count)
>> +{
>> +	ssize_t  total = 0;
>> +	size_t written = 0;
>> +	int rc = coio_popen_try_to_write(fh, buf, count,
>> +					&written);
>> +	if (rc == 0 && written == count) /* The writing's succeeded */
>> +		return (ssize_t)written;
>> +	else if (rc == -1 && errno != EAGAIN) /* Failed */
>> +		return -1;
>> +
>> +	/* A blocking operation is expected */
>> +	bool in_progress;
>> +
>> +	do {
>> +		if (fiber_is_cancelled()) {
>> +			errno = EINTR;
>> +			return -1;
>> +		}
>> +
>> +		buf += written;		/* advance writing position */
>> +		total += (ssize_t)written;
>> +		count -= written;
>> +
>> +		if (count == 0)
>> +			return total;
>> +
>> +		written = 0;
>> +		rc = coio_do_nonblock_popen_write(fh, buf, count,
>> +						  &written);
>> +		in_progress = 	(rc == 0 && written < count) ||
>> +				(rc == -1 && errno == EAGAIN);
>> +	} while (in_progress);
>> +
>> +	return (rc == 0) ? total
>> +			 : -1;
>> +}
>> +
>> +int
>> +coio_popen_wait(void *fh, int timeout, int *exit_code)
>> +{
> 
> This function doesn't depend on coio_file infrastructure. Why define it
> here at all?
> 

popen is part of fio.
fio is implemented in coio_* functions.

>> +	time_t start_tt;
>> +	time(&start_tt);
>> +
>> +	do {
>> +		/* Wait for SIGCHLD */
>> +		int sig = 0;
>> +		int code = 0;
>> +
>> +		int rc = coio_popen_get_status(fh, &sig, &code);
>> +		if (rc != POPEN_RUNNING) {
>> +			*exit_code = (rc == POPEN_EXITED) ? code
>> +							  : -sig;
>> +			return 0;
>> +		}
>> +
>> +		/* Check for timeout */
>> +		if (timeout > 0) {
>> +			time_t tt;
>> +			time(&tt);
>> +			if ((tt - start_tt) > timeout) {
>> +				errno = ETIMEDOUT;
>> +				return -1;
>> +			}
>> +		}
>> +
>> +		fiber_yield_timeout(0);
> 
> Busy waiting...
> 
>> +
>> +	} while(!fiber_is_cancelled());
>> +
>> +	errno = EINTR;
>> +	return -1;
>> +}
>> +
>> diff --git a/src/lib/core/coio_popen.c b/src/lib/core/coio_popen.c
>> new file mode 100644
>> index 000000000..b44cbf60b
>> --- /dev/null
>> +++ b/src/lib/core/coio_popen.c
>> @@ -0,0 +1,661 @@
>> +/*
>> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
>> + *
>> + * Redistribution and use in source and binary forms, with or
>> + * without modification, are permitted provided that the following
>> + * conditions are met:
>> + *
>> + * 1. Redistributions of source code must retain the above
>> + *    copyright notice, this list of conditions and the
>> + *    following disclaimer.
>> + *
>> + * 2. Redistributions in binary form must reproduce the above
>> + *    copyright notice, this list of conditions and the following
>> + *    disclaimer in the documentation and/or other materials
>> + *    provided with the distribution.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
>> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
>> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
>> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> + * SUCH DAMAGE.
>> + */
>> +
>> +#include <stddef.h>
>> +#include <signal.h>
>> +#include "coio_popen.h"
>> +#include "coio_task.h"
>> +#include "fiber.h"
>> +#include "say.h"
>> +#include "fio.h"
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <dirent.h>
>> +#include <sys/wait.h>
>> +#include <paths.h>
>> +#include <unistd.h>
>> +#include <sys/types.h>
>> +#include <sys/socket.h>
>> +#include <bits/types/siginfo_t.h>
>> +#include <pthread.h>
>> +
>> +/*
>> + * On OSX this global variable is not declared
>> + * in <unistd.h>
>> + */
>> +extern char **environ;
>> +
>> +
>> +struct popen_data {
> 
> popen_handle would be a better name, I guess.
> 
> Also, please use the same prefix for all data structures (popen_
> everywhere or coio_popen_ everywhere, not intermittently).

ok

> 
>> +	/* process id */
>> +	pid_t pid;
>> +	int fh[3];
> 
> This is rather fd, not fh.
> 
>> +	/*
>> +	 * Three handles:
>> +	 * [0] write to stdin of the child process
>> +	 * [1] read from stdout of the child process
>> +	 * [2] read from stderr of the child process
>> +	 */
> 
> We usually put comments before variable definition, not after.
> Anyway, would be prudent to say when this cases are valid. Only
> for pipe or for regular files and /dev/null, as well?
> 
>> +
>> +	/* Handle to /dev/null */
>> +	int devnull_fd;
>> +
>> +	/* The ID of socket was read recently
>> +	 * (STDERR_FILENO or STDOUT_FILENO */
>> +	int prev_source;
>> +
>> +	/*
>> +	 * Current process status.
>> +	 * The SIGCHLD handler changes this status.
>> +	 */
>> +	enum popen_status status;
>> +
>> +	/* exit status of the associated process. */
> 
> This might sound pedantic (and it is), but we are very strict about
> comment formatting. Please always start a comment from a capital letter
> and end it with a dot.
> 
>> +	int exit_code;
>> +
>> +	/*
>> +	 * Number of the signal that caused the
>> +	 * assosiated process to terminate
> 
> Typo: assosiated => associated. Please enable spell checking and fix
> typos in comments.
> 
> Please apply these minor comments to all your code.
> 
>> +	 */
>> +	int signal_id;
>> +
>> +	/*
>> +	 * The next entry in the global list
>> +	 */
>> +	struct popen_data* next;
> 
> We have rlist/stailq. No need to invent a list.

ok
> 
>> +};
>> +
>> +void *
>> +coio_popen_impl(char** argv, int argc,
>> +	char** environment, int environment_size,
>> +	int stdin_fd, int stdout_fd, int stderr_fd)
>> +{
>> +	pid_t pid;
>> +	int socket_rd[2] = {-1,-1};
>> +	int socket_wr[2] = {-1,-1};
>> +	int socket_er[2] = {-1,-1};
> 
> It's not a socket. It's a pipe.

ok

> 
>> +	errno = 0;
>> +
>> +	struct popen_data *data = popen_data_new();
>> +	if (data == NULL)
>> +		return NULL;
>> +
>> +	/* Setup a /dev/null if necessary */
>> +	bool read_devnull = (stdin_fd == FIO_DEVNULL);
>> +	bool write_devnull = (stdout_fd == FIO_DEVNULL) ||
>> +			     (stderr_fd == FIO_DEVNULL);
>> +	int devnull_flags = O_RDWR | O_CREAT;
>> +	if (!read_devnull)
>> +		devnull_flags = O_WRONLY | O_CREAT;
>> +	else if (!write_devnull)
>> +		devnull_flags = O_RDONLY | O_CREAT;
>> +
>> +	if (read_devnull || write_devnull) {
>> +		data->devnull_fd = open("/dev/null", devnull_flags, 0666);
>> +		if (data->devnull_fd < 0)
>> +			goto on_error;
>> +		else {
>> +			if (stdin_fd == FIO_DEVNULL)
>> +				stdin_fd = data->devnull_fd;
>> +			if (stdout_fd == FIO_DEVNULL)
>> +				stdout_fd = data->devnull_fd;
>> +			if (stderr_fd == FIO_DEVNULL)
>> +				stderr_fd = data->devnull_fd;
>> +		}
>> +	}
>> +
>> +	if (stdin_fd == FIO_PIPE) {
>> +		/*
>> +		 * Enable non-blocking for the parent side
>> +		 * and close-on-exec on the child's side.
>> +		 * The socketpair on OSX doesn't support
>> +		 * SOCK_NONBLOCK & SOCK_CLOEXEC flags.
>> +		 */
>> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_rd) < 0 ||
>> +		    fcntl(socket_rd[0], F_SETFL, O_NONBLOCK) < 0 ||
>> +		    fcntl(socket_rd[1], F_SETFD, FD_CLOEXEC) < 0) {
>> +			goto on_error;
>> +		}
>> +	}
>> +
>> +	if (stdout_fd == FIO_PIPE) {
>> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_wr) < 0 ||
>> +		    fcntl(socket_wr[0], F_SETFL, O_NONBLOCK) < 0 ||
>> +		    fcntl(socket_wr[1], F_SETFD, FD_CLOEXEC) < 0) {
>> +			goto on_error;
>> +		}
>> +	}
>> +
>> +	if (stderr_fd == FIO_PIPE) {
>> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_er) < 0 ||
>> +		    fcntl(socket_er[0], F_SETFL, O_NONBLOCK) < 0 ||
>> +		    fcntl(socket_er[1], F_SETFD, FD_CLOEXEC) < 0) {
>> +			goto on_error;
>> +		}
>> +	}
>> +
>> +	pid = fork();
> 
> Please use vfork().
> 

I'm afraid it won't work because of at_fork handlers.

>> +int
>> +coio_popen_try_to_read(void *fh, void *buf, size_t count,
>> +	size_t *read_bytes, int *source_id)
>> +{
>> +	struct popen_data *data = (struct popen_data *)fh;
>> +
>> +	if (data == NULL){
>> +		errno = EBADF;
>> +		return -1;
>> +	}
>> +
>> +	fd_set rfds;
>> +	FD_ZERO(&rfds);
>> +	ssize_t received = 0;
>> +	int num = 0;
>> +
>> +	if (data->fh[STDOUT_FILENO] >= 0) {
>> +		FD_SET(data->fh[STDOUT_FILENO], &rfds);
>> +		++num;
>> +	}
>> +	if (data->fh[STDERR_FILENO] >= 0) {
>> +		FD_SET(data->fh[STDERR_FILENO], &rfds);
>> +		++num;
>> +	}
>> +
>> +	if (num == 0) {
>> +		/*
>> +		 * There are no open handles for reading
>> +		 */
>> +		errno = EBADF;
>> +		return -1;
>> +	}
>> +
>> +	struct timeval tv = {0,0};
>> +	int max_h = MAX(data->fh[STDOUT_FILENO],
>> +			data->fh[STDERR_FILENO]);
>> +
>> +	errno = 0;
>> +	int retv = select(max_h + 1, &rfds, NULL, NULL, &tv);
> 
> What's going on here? Would be nice to see a friendly comment.
> 
> Anyway, why don't you use evio instead - it already uses select() under
> the hood, see ev_io_start.

researching...

> 
>> +void
>> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *context)
>> +{
>> +	(void)context;	/* UNUSED */
>> +
>> +	if (sig != SIGCHLD)
>> +		return;
>> +
>> +	/*
>> +	 * The sigaction is called with SA_NOCLDWAIT,
>> +	 * so no need to call waitpid()
>> +	 */
>> +
>> +	popen_lock_data_list();
> 
> This function is called from a signal handler => it might deadlock with
> coio_popen_destroy, for instance. Please consider using evio signal
> handlers (take a look at ev_signal_init).

evio drops all information related to the signal and just reports that 
the signal had been caught.
And since evio is implemented upon the signalfd() the further calls of 
waitpid are useless - the information about child process was discarded.
All you can get is the fact that the process is not running.

So i see the following ways to resolve this problem:
1. Refactoring of evio - to keep siginfo_t received with a signal
(the worst idea IMO).
2. Use signalfd and run the own thread to dispatch SIGCHLD
3. Deal with the lack of child's exit code
4. make the wait() is mandatory, call waitpid() and do not use SIGCHLD
handler at all.

> 
>> +
>> +	struct popen_data *data = popen_lookup_data_by_pid(si->si_pid);
>> +	if (data) {
>> +		switch (si->si_code) {
>> +		case CLD_EXITED:
>> +			data->exit_code = si->si_status;
>> +			data->status = POPEN_EXITED;
>> +			break;
>> +		case CLD_KILLED:
>> +			data->signal_id = si->si_status;
>> +			data->status = POPEN_KILLED;
>> +			break;
>> +		case CLD_DUMPED:
>> +			/* exit_status makes no sense */
>> +			data->status = POPEN_DUMPED;
>> +			break;
>> +		}
>> +
>> +		/*
>> +		 * We shouldn't close file descriptors here.
>> +		 * The child process may exit earlier than
>> +		 * the parent process finishes reading data.
>> +		 * In this case the reading fails.
>> +		 */
>> +	}
>> +
>> +	popen_unlock_data_list();
>> +}
>> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
>> index fb168e25e..614b8fefe 100644
>> --- a/src/lib/core/fiber.h
>> +++ b/src/lib/core/fiber.h
>> @@ -494,8 +494,8 @@ struct cord {
>>   extern __thread struct cord *cord_ptr;
>>   
>>   #define cord() cord_ptr
>> -#define fiber() cord()->fiber
>> -#define loop() (cord()->loop)
>> +#define fiber() (cord() ? cord()->fiber : NULL)
>> +#define loop() (cord() ? cord()->loop : NULL)
> 
> Why is that? A comment would be nice to have.

It's an at_fork() consequences.
Not sure if we can safely remove at_fork handlers.
The parent process is forking to run as a daemon.

> 
>> diff --git a/src/lua/lua_signal.c b/src/lua/lua_signal.c
>> new file mode 100644
>> index 000000000..924b0ab51
>> --- /dev/null
>> +++ b/src/lua/lua_signal.c
>> @@ -0,0 +1,99 @@
>> +/*
>> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
>> + *
>> + * Redistribution and use in source and binary forms, with or
>> + * without modification, are permitted provided that the following
>> + * conditions are met:
>> + *
>> + * 1. Redistributions of source code must retain the above
>> + *    copyright notice, this list of conditions and the
>> + *    following disclaimer.
>> + *
>> + * 2. Redistributions in binary form must reproduce the above
>> + *    copyright notice, this list of conditions and the following
>> + *    disclaimer in the documentation and/or other materials
>> + *    provided with the distribution.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
>> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
>> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
>> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> + * SUCH DAMAGE.
>> + */
>> +
>> +#include "lua/lua_signal.h"
>> +#include <sys/types.h>
>> +#include <signal.h>
>> +
>> +#include <lua.h>
>> +#include <lauxlib.h>
>> +#include <lualib.h>
>> +
>> +#include "lua/utils.h"
>> +
>> +#ifndef PUSHTABLE
>> +#define PUSHTABLE(name, method, value)	{	\
>> +	lua_pushliteral(L, name);		\
>> +	method(L, value);			\
>> +	lua_settable(L, -3);			\
>> +}
>> +#endif /*PUSHTABLE*/
>> +
>> +void
>> +tarantool_lua_signal_init(struct lua_State *L)
>> +{
>> +	static const struct luaL_Reg signal_methods[] = {
>> +		{ NULL,	NULL }
>> +	};
>> +
>> +	luaL_register_module(L, "signal", signal_methods);
>> +
>> +	lua_pushliteral(L, "c");
>> +	lua_newtable(L);
>> +
>> +	lua_pushliteral(L, "signals");
>> +	lua_newtable(L);
>> +
>> +	PUSHTABLE("SIGINT", lua_pushinteger, SIGINT);
>> +	PUSHTABLE("SIGILL", lua_pushinteger, SIGILL);
>> +	PUSHTABLE("SIGABRT", lua_pushinteger, SIGABRT);
>> +	PUSHTABLE("SIGFPE", lua_pushinteger, SIGFPE);
>> +	PUSHTABLE("SIGSEGV", lua_pushinteger, SIGSEGV);
>> +	PUSHTABLE("SIGTERM", lua_pushinteger, SIGTERM);
>> +
>> +	PUSHTABLE("SIGHUP", lua_pushinteger, SIGHUP);
>> +	PUSHTABLE("SIGQUIT", lua_pushinteger, SIGQUIT);
>> +	PUSHTABLE("SIGTRAP", lua_pushinteger, SIGTRAP);
>> +	PUSHTABLE("SIGKILL", lua_pushinteger, SIGKILL);
>> +	PUSHTABLE("SIGBUS", lua_pushinteger, SIGBUS);
>> +	PUSHTABLE("SIGSYS", lua_pushinteger, SIGSYS);
>> +	PUSHTABLE("SIGPIPE", lua_pushinteger, SIGPIPE);
>> +	PUSHTABLE("SIGALRM", lua_pushinteger, SIGALRM);
>> +
>> +	PUSHTABLE("SIGURG", lua_pushinteger, SIGURG);
>> +	PUSHTABLE("SIGSTOP", lua_pushinteger, SIGSTOP);
>> +	PUSHTABLE("SIGTSTP", lua_pushinteger, SIGTSTP);
>> +	PUSHTABLE("SIGCONT", lua_pushinteger, SIGCONT);
>> +	PUSHTABLE("SIGCHLD", lua_pushinteger, SIGCHLD);
>> +	PUSHTABLE("SIGTTIN", lua_pushinteger, SIGTTIN);
>> +	PUSHTABLE("SIGTTOU", lua_pushinteger, SIGTTOU);
>> +	PUSHTABLE("SIGPOLL", lua_pushinteger, SIGPOLL);
>> +	PUSHTABLE("SIGXCPU", lua_pushinteger, SIGXCPU);
>> +	PUSHTABLE("SIGXFSZ", lua_pushinteger, SIGXFSZ);
>> +	PUSHTABLE("SIGVTALRM", lua_pushinteger, SIGVTALRM);
>> +	PUSHTABLE("SIGPROF", lua_pushinteger, SIGPROF);
>> +	PUSHTABLE("SIGUSR1", lua_pushinteger, SIGUSR1);
>> +	PUSHTABLE("SIGUSR2", lua_pushinteger, SIGUSR2);
>> +	lua_settable(L, -3); /* "signals" */
> 
> This could be done from Lua - signal numbers are standard AFAIK.
Did you mean to use constants like fio.STDIN?

Ok.

> 
>> +
>> +	lua_settable(L, -3); /* "c" */
>> +	lua_pop(L, 1);
>> +}
>> diff --git a/test/box-tap/fio_popen.sample.txt b/test/box-tap/fio_popen.sample.txt
>> new file mode 100644
>> index 000000000..43cba8c65
>> --- /dev/null
>> +++ b/test/box-tap/fio_popen.sample.txt
>> @@ -0,0 +1,5 @@
>> +AAA
>> +BBB
>> +CCC
>> +DDD
>> +
> 
> I think this file should be created by the test in tmp dir. Just don't
> forget to delete it.
> 

Ok.


>> diff --git a/test/box-tap/fio_popen.test.lua b/test/box-tap/fio_popen.test.lua
> 
> Should be app-tap? Anyway, why tap test? Normal tests are easier to
> write and understand IMO.
> 

Ok, let it be app-tap.

What a 'Normal' test? The ones in e.g. test/app directory?
It's hard to find the error when result files are quite big.
Have to use external tools, e.g. 'diff' etc.

>> new file mode 100755
>> index 000000000..0c2bd7e70
>> --- /dev/null
>> +++ b/test/box-tap/fio_popen.test.lua
>> @@ -0,0 +1,189 @@
>> +#!/usr/bin/env tarantool
>> +
>> +local tap = require('tap')
>> +local fio = require('fio')
>> +local errno = require('errno')
>> +local fiber = require('fiber')
>> +local test = tap.test()
>> +
>> +test:plan(5+9+11+9+8)
>> +
>> +-- Preliminaries
>> +local function read_stdout(app)
>> +    local ss = ""
>> +
>> +    local s,src,err = app:read(128)
>> +
>> +    while s ~= nil and s ~= "" do
>> +        ss = ss .. s
>> +
>> +        s,src,err = app:read(128)
>> +    end
>> +
>> +    return ss
>> +end
>> +
>> +-- Test 1. Run application, check its status, kill and wait
>> +local app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> 
> Please test passing custom env as well.

ok

> 
>> +                        stdout=fio.STDOUT,
>> +                        stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#1. Starting a existing application")
>> +
>> +local rc = app1:get_status()
>> +test:is(rc, nil, "#1. Process is running")
>> +
>> +rc,err = app1:kill()
>> +test:is(rc, true, "#1. Sending kill(15)")
>> +
>> +rc,err = app1:wait(5)
>> +test:is(rc, -15, "#1. Process was killed")
>> +
>> +rc = app1:get_status()
>> +test:is(rc, -15, "#1. Process was killed 2")
>> +
>> +app1 = nil
>> +
>> +-- Test 2. Run application, write to stdin, read from stdout
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> 
> What's the point using '/bin/sh' just to run 'cat'?
> 

cat fails without it.

>> +                  stdout=fio.PIPE,
>> +                  stdin=fio.PIPE,
>> +                  stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#2. Starting a existing application")
>> +
>> +rc = app1:get_status()
>> +test:is(rc, nil, "#2. Process is running")
>> +
>> +local test2str = '123\n456\n789'
>> +
>> +app1:write(test2str)
>> +rc,src,err = app1:read(256)
>> +
>> +test:is(src, fio.STDOUT, "#2. Read from STDOUT")
>> +test:is(rc, test2str, "#2. Received exact string")
>> +
>> +test2str = 'abc\ndef\ngh'
>> +
>> +app1:write(test2str, 4)
>> +local rc2,src2,err = app1:read(6)
>> +
>> +test:is(err, nil, "#2. Reading ok")
>> +test:is(src2, fio.STDOUT, "#2. Read from STDOUT 2")
>> +test:is(rc2, 'abc\n', "#2. Received exact string 2")
>> +
>> +rc,err = app1:kill()
>> +test:is(rc, true, "#2. Sending kill(15)")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, -15, "#2. Process was killed")
>> +
>> +app1 = nil
>> +
>> +-- Test 3. read from stdout with timeout
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> +                  stdout=fio.PIPE,
>> +                  stdin=fio.PIPE,
>> +                  stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#3. Starting a existing application")
>> +test:is(app1:get_stderr(), nil, "#3. STDERR is redirected")
>> +test:isnt(app1:get_stdout(), nil, "#3. STDOUT is available")
>> +test:isnt(app1:get_stdin(), nil, "#3. STDIN is available")
>> +
>> +
>> +rc = app1:get_status()
>> +test:is(rc, nil, "#3. Process is running")
>> +
>> +rc,src,err = app1:read2(256, 2)
> 
> Please use smaller timeouts - we want tests to run real fast.
> 

ok

>> +
>> +local e = errno()
>> +test:is(e, errno.ETIMEDOUT, "#3. Timeout")
>> +
>> +
>> +local test2str = '123\n456\n789'
>> +
>> +app1:write(test2str)
>> +rc,src,err = app1:read2(256, 2)
>> +
>> +test:is(err, nil, "#3. Reading ok")
>> +test:is(src, fio.STDOUT, "#3. Read from STDOUT")
>> +test:is(rc, test2str, "#3. Received exact string")
>> +
>> +rc,err = app1:kill('SIGHUP')
>> +test:is(rc, true, "#3. Sending kill(1)")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, -1, "#3. Process was killed")
>> +
>> +app1 = nil
>> +
>> +-- Test 4. Redirect from file
>> +local build_path = os.getenv("BUILDDIR")
>> +local txt_filename = fio.pathjoin(build_path, 'test/box-tap/fio_popen.sample.txt')
>> +
>> +local txt_file = fio.open(txt_filename, {'O_RDONLY'})
>> +test:isnt(txt_file, nil, "#4. Open existing file for reading")
>> +
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> +                  stdout=fio.PIPE,
>> +                  stdin=txt_file,
>> +                  stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#4. Starting a existing application")
>> +test:is(app1:get_stderr(), nil, "#4. STDERR is redirected")
>> +test:isnt(app1:get_stdout(), nil, "#4. STDOUT is available")
>> +test:is(app1:get_stdin(), nil, "#4. STDIN is redirected")
>> +
>> +rc = app1:get_status()
>> +test:is(rc, nil, "#4. Process is running")
>> +
>> +rc,src,err = app1:read2(256, 2)
>> +
>> +test:is(src, fio.STDOUT, "#4. Read from STDOUT")
>> +
>> +local test2str = 'AAA\nBBB\nCCC\nDDD\n\n'
>> +
>> +test:is(rc, test2str, "#4. Received exact string")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, 0, "#4. Process's exited")
>> +
>> +app1 = nil
>> +txt_file:close()
>> +
>> +-- Test 5. Redirect output from one process to another
>> +local app_path = fio.pathjoin(build_path, 'test/box-tap/fio_popen_test1.sh')
>> +
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', app_path},
>> +                  stdout=fio.PIPE,
>> +                  stderr=fio.STDOUT})
>> +
>> +test:isnt(app1, nil, "#5. Starting application 1")
>> +test:is(app1:get_stderr(), nil, "#5. STDERR is redirected")
>> +test:isnt(app1:get_stdout(), nil, "#5. STDOUT is available")
>> +test:isnt(app1:get_stdin(), nil, "#5. STDIN is available")
>> +
>> +fiber.sleep(1)
>> +
>> +local app2 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> +                  stdout=fio.PIPE,
>> +                  stdin=app1:get_stdout()})
>> +
>> +test:isnt(app2, nil, "#5. Starting application 2")
>> +
>> +local test2str = '1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n'
>> +
>> +rc = read_stdout(app2)
>> +
>> +test:is(rc, test2str, "#5. Received exact string")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, 0, "#5. Process's exited 1")
>> +
>> +rc,err = app2:wait()
>> +test:is(rc, 0, "#5. Process's exited 2")
>> +
>> +app1 = nil
>> +app2 = nil
>> +
>> +-- --------------------------------------------------------------
>> +test:check()
>> +os.exit(0)
>> +
>> diff --git a/test/box-tap/fio_popen_test1.sh b/test/box-tap/fio_popen_test1.sh
>> new file mode 100755
>> index 000000000..da54ee19a
>> --- /dev/null
>> +++ b/test/box-tap/fio_popen_test1.sh
>> @@ -0,0 +1,7 @@
>> +#!/bin/bash
>> +for i in {1..10}
>> +do
>> +	echo $i
>> +#	sleep 1
> 
> Commented line. Please remove.
> 

Done.

>> +done
> 
> Would be nice to see a test that reads/writes much more to the pipes.
> 

Ok.

>> +
>> diff --git a/third_party/libeio/eio.c b/third_party/libeio/eio.c
>> index 7351d5dda..e433b1b3b 100644
>> --- a/third_party/libeio/eio.c
>> +++ b/third_party/libeio/eio.c
>> @@ -1741,7 +1741,24 @@ static int eio_threads;
>>   static void ecb_cold
>>   eio_prefork()
>>   {
>> -    eio_threads = etp_set_max_parallel(EIO_POOL, 0);
>> +	/*
>> +	 * When fork() is called libeio shuts
>> +	 * down all working threads.
>> +	 * But it causes a deadlock if fork() was
>> +	 * called from the libeio thread.
>> +	 * To avoid this do not close the
>> +	 * thread who called fork().
>> +	 * This behaviour is acceptable for the
>> +	 * case when fork() is immediately followed
>> +	 * by exec().
>> +	 * To clone a process call fork() from the
>> +	 * main thread.
>> +	 */
>> +
>> +	if (etp_is_in_pool_thread())
>> +		eio_threads = etp_get_max_parallel(EIO_POOL);
>> +	else
>> +    		eio_threads = etp_set_max_parallel(EIO_POOL, 0);
> 
> Please investigate why it was initially done that way, because
> I'm afraid this change might break something.
> 

A usual precautions before fork: stop all thread before fork and resume 
after fork.
This works fine if fork() is called in the main thread.
Of course the current workaround has its disadvantages. But they're not
worth mentioning in the case when fork() is followed by exec.

>>   }
>>   
>>   static void ecb_cold
>> diff --git a/third_party/libeio/etp.c b/third_party/libeio/etp.c
>> index 42f7661eb..38c2d1f9b 100644
>> --- a/third_party/libeio/etp.c
>> +++ b/third_party/libeio/etp.c
>> @@ -164,6 +164,21 @@ struct etp_pool_user
>>      xmutex_t lock;
>>   };
>>   
>> +static __thread int is_eio_thread = 0;
>> +
>> +/**
>> + * Check whether the current thread belongs to
>> + * libeio thread pool or just a generic thread.
>> + *
>> + * @return 0 for generic thread
>> + * @return 1 for libeio thread pool
>> + **/
>> +ETP_API_DECL int ecb_cold
>> +etp_is_in_pool_thread()
>> +{
>> +	return is_eio_thread;
>> +}
>> +
>>   /* worker threads management */
>>   
>>   static void ecb_cold
>> @@ -322,6 +337,9 @@ X_THREAD_PROC (etp_proc)
>>     self.pool = pool;
>>     etp_pool_user user; /* per request */
>>   
>> +/* Distinguish libeio threads from the generic threads */
>> +  is_eio_thread = 1;
>> +
>>     etp_proc_init ();
>>   
>>     /* try to distribute timeouts somewhat evenly (nanosecond part) */
>> @@ -616,3 +634,13 @@ etp_set_max_parallel (etp_pool pool, unsigned int threads)
>>     X_UNLOCK (pool->lock);
>>     return retval;
>>   }
>> +
>> +ETP_API_DECL int ecb_cold
>> +etp_get_max_parallel (etp_pool pool)
>> +{
>> +	int retval;
>> +	X_LOCK   (pool->lock);
>> +	retval = pool->wanted;
>> +	X_UNLOCK (pool->lock);
>> +	return retval;
>> +}
>> diff --git a/third_party/libev/ev.c b/third_party/libev/ev.c
>> index 6a2648591..5fa8293a1 100644
>> --- a/third_party/libev/ev.c
>> +++ b/third_party/libev/ev.c
>> @@ -4214,6 +4214,8 @@ noinline
>>   void
>>   ev_signal_stop (EV_P_ ev_signal *w) EV_THROW
>>   {
>> +	if (!loop)
>> +		return;
> 
> Why is that? At least deserves a comment. Better handle it at the upper
> level.

It's an at_fork() consequences.
See my comments above.

> 
> Anyway, all these changes to libev deserve a separate patch with a
> proper change log comment.
> 

ok.

>>     clear_pending (EV_A_ (W)w);
>>     if (expect_false (!ev_is_active (w)))
>>       return;
> 
> 
> 



On 31.05.2019 14:49, Vladimir Davydov wrote:> A few more comments. 
Please take a look.
 >
 > On Wed, May 29, 2019 at 10:08:09AM +0300, Stanislav Zudin wrote:
 >> diff --git a/src/lib/core/coio_popen.c b/src/lib/core/coio_popen.c
 >> +#include <stddef.h>
 >> +#include <signal.h>
 >> +#include "coio_popen.h"
 >> +#include "coio_task.h"
 >> +#include "fiber.h"
 >> +#include "say.h"
 >> +#include "fio.h"
 >> +#include <stdio.h>
 >> +#include <stdlib.h>
 >> +#include <dirent.h>
 >> +#include <sys/wait.h>
 >> +#include <paths.h>
 >
 > Why do you need this header? Looks like you never use anything from it.
 > Same for dirent.h. Please include only those headers you really need.
 >

ok

 >> +static struct popen_data *
 >> +popen_lookup_data_by_pid(pid_t pid)
 >> +{
 >> +	struct popen_data *cur = popen_data_list;
 >> +	for(; cur; cur = cur->next) {
 >> +		if (cur->pid == pid)
 >> +			return cur;
 >> +	}
 >
 > Please write a comment explaining why you think linear search is fine
 > here and why we don't use some kind of hash or tree.
 >

I believe the number of simultaneously running popen processes is rather 
small - tens. In this case hash won't give a significant performance boost.

 >> +
 >> +	return NULL;
 >> +}
 >> +
 >> +static void
 >> +popen_exclude_from_list(struct popen_data *data)
 >> +{
 >> +	if (popen_data_list == data) {
 >> +		popen_data_list = data->next;
 >> +		return;
 >> +	}
 >> +
 >> +	/* Find the previous entry */
 >> +	struct popen_data *prev = popen_data_list;
 >> +	for( ; prev && prev->next != data; prev = prev->next) {
 >> +		/* empty */;
 >> +	}
 >> +
 >> +	if (!prev)
 >> +		return ;
 >> +	assert(prev->next == data);
 >> +	prev->next = data->next;
 >
 > Looks like doubly-linked list (rlist) would suit better here.
 >

ok

 >> +}
 >> +
 >> +/*
 >> + * Returns next socket to read.
 >> + * Use this function when both STDOUT and STDERR outputs
 >> + * are ready for reading.
 >
 > But what does it do? How does it prioritize? Though short, this function
 > looks kinda arcane to me due to this bit manipulation magic. Please
 > clarify. However, if you switch to ev io, you'll probably won't be
 > needing it.
 >

It's just a round-robin.
Used in a case when both STDIN & STDOUT are ready for reading while
popen.read() returns only one result. Doesn't let any stream stuck.

 >> + * */
 >> +static inline int
 >> +get_handle_in_order(struct popen_data *data)
 >> +{
 >> +	/*
 >> +	 * Invert the order of handles to be read
 >> +	 */
 >> +	const int mask = STDERR_FILENO | STDOUT_FILENO;
 >> +	data->prev_source ^= mask;
 >> +
 >> +	/*
 >> +	 * If handle is not available, invert it back
 >> +	 */
 >> +	if (data->fh[data->prev_source] < 0)
 >> +		data->prev_source ^= mask;
 >> +	/*
 >> +	 * if both reading handles are invalid return -1
 >> +	 */
 >> +	return data->fh[data->prev_source];
 >> +}
 >
 >> +void *
 >> +coio_popen_impl(char** argv, int argc,
 >> +	char** environment, int environment_size,
 >> +	int stdin_fd, int stdout_fd, int stderr_fd)
 >> +{
 >> +	pid_t pid;
 >> +	int socket_rd[2] = {-1,-1};
 >> +	int socket_wr[2] = {-1,-1};
 >> +	int socket_er[2] = {-1,-1};
 >> +	errno = 0;
 >> +
 >> +	struct popen_data *data = popen_data_new();
 >> +	if (data == NULL)
 >> +		return NULL;
 >> +
 >> +	/* Setup a /dev/null if necessary */
 >> +	bool read_devnull = (stdin_fd == FIO_DEVNULL);
 >> +	bool write_devnull = (stdout_fd == FIO_DEVNULL) ||
 >> +			     (stderr_fd == FIO_DEVNULL);
 >> +	int devnull_flags = O_RDWR | O_CREAT;
 >> +	if (!read_devnull)
 >> +		devnull_flags = O_WRONLY | O_CREAT;
 >> +	else if (!write_devnull)
 >> +		devnull_flags = O_RDONLY | O_CREAT;
 >> +
 >> +	if (read_devnull || write_devnull) {
 >> +		data->devnull_fd = open("/dev/null", devnull_flags, 0666);
 >
 > No need to pass mode unless you're creating a file.

It doesn't affect the existing files. Thus one line is shorted than two.
  :-)

 >
 >> +		if (data->devnull_fd < 0)
 >> +			goto on_error;
 >> +		else {
 >> +			if (stdin_fd == FIO_DEVNULL)
 >> +				stdin_fd = data->devnull_fd;
 >> +			if (stdout_fd == FIO_DEVNULL)
 >> +				stdout_fd = data->devnull_fd;
 >> +			if (stderr_fd == FIO_DEVNULL)
 >> +				stderr_fd = data->devnull_fd;
 >> +		}
 >> +	}
 >> +
 >> +	if (stdin_fd == FIO_PIPE) {
 >> +		/*
 >> +		 * Enable non-blocking for the parent side
 >> +		 * and close-on-exec on the child's side.
 >> +		 * The socketpair on OSX doesn't support
 >> +		 * SOCK_NONBLOCK & SOCK_CLOEXEC flags.
 >> +		 */
 >> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_rd) < 0 ||
 >
 > I overlooked it when I reviewed the patch last time. Why do you use
 > sockets? Why is a simple pipe not enough? Sockets are full-duplex
 > while in your case a simplex connection would be enough, no?

Ask Georgy, he insisted on replacing pipes by socketpair.


 >
 >> +		    fcntl(socket_rd[0], F_SETFL, O_NONBLOCK) < 0 ||
 >> +		    fcntl(socket_rd[1], F_SETFD, FD_CLOEXEC) < 0) {
 >
 > You don't really need FD_CLOEXEC for this one as you can close it in the
 > child right after fork(). Moreover, looks like you close() them anyway.
 >
 > We do need to use CLOEXEC for other fds (e.g.  xlog or vinyl files), but
 > I guess this can be done in a separate patch.

got it.

 >
 >> +			goto on_error;
 >> +		}
 >> +	}
 >> +
 >> +	if (stdout_fd == FIO_PIPE) {
 >> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_wr) < 0 ||
 >> +		    fcntl(socket_wr[0], F_SETFL, O_NONBLOCK) < 0 ||
 >> +		    fcntl(socket_wr[1], F_SETFD, FD_CLOEXEC) < 0) {
 >> +			goto on_error;
 >> +		}
 >> +	}
 >> +
 >> +	if (stderr_fd == FIO_PIPE) {
 >> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_er) < 0 ||
 >> +		    fcntl(socket_er[0], F_SETFL, O_NONBLOCK) < 0 ||
 >> +		    fcntl(socket_er[1], F_SETFD, FD_CLOEXEC) < 0) {
 >
 > Looks like quite a bit of repetition. Worth moving this to a separate
 > helper function or doing it in a loop somehow instead of copying and
 > pasting?
 >

i'll think about that.

 >> +			goto on_error;
 >> +		}
 >> +	}
 >> +
 >> +	pid = fork();
 >> +
 >> +	if (pid < 0)
 >> +		goto on_error;
 >> +	else if (pid == 0) /* child */ {
 >> +		/* Setup stdin/stdout */
 >> +		if (stdin_fd == FIO_PIPE) {
 >> +			close(socket_rd[0]); /* close parent's side */
 >> +			stdin_fd = socket_rd[1];
 >> +		}
 >> +		if (stdin_fd != STDIN_FILENO) {
 >> +			dup2(stdin_fd, STDIN_FILENO);
 >> +			close(stdin_fd);
 >> +		}
 >> +
 >> +		if (stdout_fd == FIO_PIPE) {
 >> +			close(socket_wr[0]); /* close parent's side */
 >> +			stdout_fd = socket_wr[1];
 >> +		}
 >> +		if (stdout_fd != STDOUT_FILENO) {
 >> +			dup2(stdout_fd, STDOUT_FILENO);
 >> +			if (stdout_fd != STDERR_FILENO)
 >> +				close(stdout_fd);
 >> +		}
 >> +
 >> +		if (stderr_fd == FIO_PIPE) {
 >> +			close(socket_er[0]); /* close parent's side */
 >> +			stderr_fd = socket_er[1];
 >> +		}
 >> +		if (stderr_fd != STDERR_FILENO) {
 >> +			dup2(stderr_fd, STDERR_FILENO);
 >> +			if (stderr_fd != STDOUT_FILENO)
 >> +				close(stderr_fd);
 >> +		}
 >> +
 >> +		execve( argv[0], argv,
 >> +			environment ? environment : environ);
 >> +		_exit(127);
 >
 > Why _exit(), why not exit()? If there's a reason, please explain in a
 > comment. Also, why 127? We prefer to avoid explicit numeric constants.
 > May be, you could use EXIT_FAILURE instead?

_exit() doesn't call atexit(), but in particular this case it doesn't 
matter.
As to exit code i need to distinguish exit code returned by child 
process from execve error.
The native implementation of popen does return 127.

 >
 > Also, may be we need to write something to stderr in this case, just to
 > let the caller now that it's not the child failure, it's execve failure.

Good idea.

 >
 >> +int
 >> +coio_popen_destroy(void *fh)
 >> +{
 >> +	struct popen_data *data = (struct popen_data *)fh;
 >> +
 >> +	if (data == NULL){
 >> +		errno = EBADF;
 >> +		return -1;
 >> +	}
 >
 > What's the point in returning an error from this function?
 > You never check it AFAICS.
 >
 > Anyway, like I mentioned above, I think that one shouldn't pass NULL for
 > a handle to any of popen functions.
 >

ok

 >> +void
 >> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *context)
 >
 > Judging by the name it looks like this function should merely return
 > true/false, not do anything else. Please rename. For instance, we could
 > name it coio_popen_on_child_termination or handle_child_death.

ok

 >
 >> +{
 >> +	(void)context;	/* UNUSED */
 >
 > Why pass it at all if it's unused?
 >

Because C compiler requires the argument to be defined.

[6.9.1.5] If the declarator includes a parameter type list, the 
declaration of each parameter shall include an identifier, except for 
the special case of a parameter list consisting of a single parameter of 
type void, in which case there shall not be an identifier. No 
declaration list shall follow.

Changing the compiler flags is too much, IMO.

 >> +
 >> +	if (sig != SIGCHLD)
 >> +		return;
 >
 > Why call it if it isn't SIGCHLD?
 >

Removed.

 >> +
 >> +	/*
 >> +	 * The sigaction is called with SA_NOCLDWAIT,
 >> +	 * so no need to call waitpid()
 >> +	 */
 >> +
 >> +	popen_lock_data_list();
 >> +
 >> +	struct popen_data *data = popen_lookup_data_by_pid(si->si_pid);
 >> +	if (data) {
 >> +		switch (si->si_code) {
 >> +		case CLD_EXITED:
 >> +			data->exit_code = si->si_status;
 >> +			data->status = POPEN_EXITED;
 >> +			break;
 >> +		case CLD_KILLED:
 >> +			data->signal_id = si->si_status;
 >> +			data->status = POPEN_KILLED;
 >> +			break;
 >> +		case CLD_DUMPED:
 >> +			/* exit_status makes no sense */
 >> +			data->status = POPEN_DUMPED;
 >> +			break;
 >> +		}
 >> +
 >> +		/*
 >> +		 * We shouldn't close file descriptors here.
 >> +		 * The child process may exit earlier than
 >> +		 * the parent process finishes reading data.
 >> +		 * In this case the reading fails.
 >> +		 */
 >> +	}
 >> +
 >> +	popen_unlock_data_list();
 >> +}
 >
 >> diff --git a/src/lib/core/coio_popen.h b/src/lib/core/coio_popen.h
 >> +/**
 >> + * Special values of the file descriptors passed to fio.popen
 >> + * */
 >> +enum {
 >> +	FIO_PIPE = -2,
 >> +	/*
 >> +	 * Tells fio.popen to open a handle for
 >> +	 * direct reading/writing
 >> +	 */
 >> +
 >> +	FIO_DEVNULL = -3
 >> +	/*
 >> +	 * Tells fio.popen to redirect the given standard
 >> +	 * stream into /dev/null
 >> +	 */
 >> +};
 >> +
 >> +/**
 >> + * Possible status of the process started via fio.popen
 >> + **/
 >> +enum popen_status {
 >> +	POPEN_UNKNOWN = -1,
 >
 > Why do you need this mysterious status code? Looking at the code, I see
 > that you coio_popen_get_status() returns it in case of error. But what
 > kind of error? Turns out that this is the case when the handle is NULL.
 > This looks pointless to me - one shouldn't pass NULL to that function.
 > Am I missing something?

Removed.

 >
 >> +
 >> +	POPEN_RUNNING = 1,
 >> +	/*the process is alive and well*/
 >
 > Comments should be above constants, not below. Also, please use
 > formatting typically used in the code (starts with /**, capital
 > letter, a dot at the end).
 >

Ok.

 >> +
 >> +	POPEN_EXITED = 2,
 >> +	/*the process exited*/
 >> +
 >> +	POPEN_KILLED = 3,
 >> +	/*the process terminated by a signal*/
 >> +
 >> +	POPEN_DUMPED = 4
 >> +	/* the process terminated abnormally */
 >> +};
 >
 >> +/**
 >> + * Returns status of the associated process.
 >> + *
 >> + * @param fd - handle returned by fio.popen.
 >> + * @param signal_id - if not NULL accepts the signal
 >> + * sent to terminate the process
 >> + * @param exit_code - if not NULL accepts the exit code
 >> + * if the process terminated normally.
 >> + *
 >> + * @return one of the following values:
 >> + * POPEN_RUNNING if the process is alive
 >> + * POPEN_EXITED if the process was terminated normally
 >> + * POPEN_KILLED if the process was terminated by a signal
 >> + * POPEN_UNKNOWN an error's occurred
 >> + */
 >> +int
 >> +coio_popen_get_status(void *fh, int *signal_id, int *exit_code);
 >> +
 >> +/**
 >> + * Handle SIGCHLD signal
 >> + */
 >> +void
 >> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *);
 >
 > Better not use siginfo_t here - instead pass what you really need.
 > This makes the API clearer and cuts the dependency list.
 >

coio_popen_child_is_dead is passed to sigaction.
To separate it from siginfo_t we need one more function.
No problem, will add.

 >> diff --git a/src/lua/fio.c b/src/lua/fio.c
 >> +/**
 >> + * A wrapper applicable for using with lua's GC.
 >> + * */
 >> +struct fio_popen_wrap {
 >
 > Why do you need a wrapper at all?

Right now it contains only a "handle" (a pointer to
struct popen_data *).
Later we may need to keep something else in the managed memory.
It's more readable than just a pointer to pointer.

 >
 >> +	void* handle;
 >> +};
 >> +static const char* fio_popen_typename = "fio.popen.data";
 >> +
 >> +static struct fio_popen_wrap *
 >> +fio_popen_get_handle_wrap(lua_State *L, int index)
 >> +{
 >> +	luaL_checktype(L, index, LUA_TUSERDATA);
 >> +	struct fio_popen_wrap *wrap = (struct fio_popen_wrap *)
 >> +		luaL_checkudata(L, index, fio_popen_typename);
 >
 > Hmm, why do you use udata? Why not plain cdata? I keep forgetting
 > the difference, to tell the truth.

The Lua finalizer deals only with "managed" memory.
It's not aware of cdata and can't release it.

 >
 >> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
 >> +popen_methods.do_read = function(self, buf, size, seconds)
 >> +    if size == nil or type(size) ~= 'number' then
 >> +        error('fio.popen.read: invalid size argument')
 >> +    end
 >> +    if seconds == nil or type(seconds) ~= 'number' then
 >> +        error('fio.popen.read: invalid seconds argument')
 >> +    end
 >> +
 >> +    local tmpbuf
 >> +    if not ffi.istype(const_char_ptr_t, buf) then
 >> +        tmpbuf = buffer.ibuf()
 >> +        buf = tmpbuf:reserve(size)
 >> +    end
 >> +
 >> +    local res, output_no, err = internal.popen_read(self.fh, buf, 
size, seconds)
 >> +    if res == nil then
 >> +        if tmpbuf ~= nil then
 >> +            tmpbuf:recycle()
 >> +        end
 >> +        return nil, nil, err
 >> +    end
 >> +
 >> +    if tmpbuf ~= nil then
 >> +        tmpbuf:alloc(res)
 >> +        res = ffi.string(tmpbuf.rpos, tmpbuf:size())
 >> +        tmpbuf:recycle()
 >> +    end
 >> +    return res, output_no
 >> +end
 >> +
 >> +-- read stdout & stderr of the process started by fio.popen
 >> +-- read() -> str, source
 >> +-- read(buf) -> len, source
 >> +-- read(size) -> str, source
 >> +-- read(buf, size) -> len, source
 >> +-- source contains id of the stream, fio.STDOUT or fio.STDERR
 >> +popen_methods.read = function(self, buf, size)
 >> +    if self.fh == nil then
 >> +        return nil, nil, 'Invalid object'
 >> +    end
 >> +
 >> +    if buf == nil and size == nil then
 >> +        -- read()
 >> +        size = 512
 >
 > Why 512? Aren't we supposed to read everything till EOF in this case?

The behaviour was borrowed from fio.read()
The reading everything till EOF causes problems in reading from both
sources: STDIN and STDOUT. While you waiting one source another one may
get stuck.

 >
 >> +    elseif ffi.istype(const_char_ptr_t, buf) and size == nil then
 >> +        -- read(buf)
 >> +        size = 512
 >> +    elseif not ffi.istype(const_char_ptr_t, buf) and
 >> +            buf ~= nil and size == nil then
 >> +        -- read(size)
 >> +        size = tonumber(buf)
 >> +        buf = nil
 >> +    elseif ffi.istype(const_char_ptr_t, buf) and size ~= nil then
 >> +        -- read(buf, size)
 >> +        size = tonumber(size)
 >> +    else
 >> +        error("fio.popen.read: invalid arguments")
 >> +    end
 >> +
 >> +    return self:do_read(buf, size, -1)
 >> +end
 >
 >> +popen_methods.wait = function(self, timeout)
 >> +    if self.fh == nil then
 >> +        return nil, 'Invalid object'
 >> +    end
 >> +
 >> +    if timeout == nil then
 >> +        timeout = -1
 >> +    else
 >> +        timeout = tonumber(timeout)
 >> +    end
 >> +
 >> +    local rc, err = internal.popen_wait(self.fh, timeout)
 >> +    if rc ~= nil then
 >> +        self.exit_code = tonumber(rc)
 >> +        self.fh = nil
 >
 > This is kinda unexpected. What if we want to read the process output
 > after waiting for it to complete.

And what do you expect to read after process is complete?
If application is a some kind of linear script it produces some output 
and then exits. In this case the caller reads everything till the EOF.
BTW, in our tests test scripts exit earlier than the parent starts
reading their output.

For interactive applications who may wait for input forever we've got a
kill(). It's allowed to read after sending signal and before calling
wait(). In this case we won't miss anything from the child process output.

So my point is: wait() is a synonim to close() (or join() in C++ world).
It releases all resources.



 >
 > Come to think of it, we might need an explicit knob to destroy an object
 > even before it's collected. May be, consider re-adding close() for this?
 > Please consult with Alexander and/or Georgy re this.
 >

Sasha, George, what do you think?

 >> +        return rc
 >> +    else
 >> +        return nil,err
 >> +    end
 >> +end
 >
 >> diff --git a/test/box-tap/fio_popen.test.lua 
b/test/box-tap/fio_popen.test.lua
 >> new file mode 100755
 >> index 000000000..0c2bd7e70
 >> --- /dev/null
 >> +++ b/test/box-tap/fio_popen.test.lua
 >> @@ -0,0 +1,189 @@
 >> +#!/usr/bin/env tarantool
 >> +
 >> +local tap = require('tap')
 >> +local fio = require('fio')
 >> +local errno = require('errno')
 >> +local fiber = require('fiber')
 >> +local test = tap.test()
 >> +
 >> +test:plan(5+9+11+9+8)
 >> +
 >> +-- Preliminaries
 >> +local function read_stdout(app)
 >> +    local ss = ""
 >> +
 >> +    local s,src,err = app:read(128)
 >> +
 >> +    while s ~= nil and s ~= "" do
 >> +        ss = ss .. s
 >> +
 >> +        s,src,err = app:read(128)
 >> +    end
 >> +
 >> +    return ss
 >> +end
 >> +
 >> +-- Test 1. Run application, check its status, kill and wait
 >> +local app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
 >> +                        stdout=fio.STDOUT,
 >> +                        stderr=fio.STDOUT})
 >> +test:isnt(app1, nil, "#1. Starting a existing application")
 >> +
 >> +local rc = app1:get_status()
 >> +test:is(rc, nil, "#1. Process is running")
 >> +
 >> +rc,err = app1:kill()
 >> +test:is(rc, true, "#1. Sending kill(15)")
 >> +
 >> +rc,err = app1:wait(5)
 >> +test:is(rc, -15, "#1. Process was killed")
 >> +
 >> +rc = app1:get_status()
 >> +test:is(rc, -15, "#1. Process was killed 2")
 >> +
 >> +app1 = nil
 >> +
 >> +-- Test 2. Run application, write to stdin, read from stdout
 >> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
 >> +                  stdout=fio.PIPE,
 >> +                  stdin=fio.PIPE,
 >> +                  stderr=fio.STDOUT})
 >> +test:isnt(app1, nil, "#2. Starting a existing application")
 >> +
 >> +rc = app1:get_status()
 >> +test:is(rc, nil, "#2. Process is running")
 >> +
 >> +local test2str = '123\n456\n789'
 >> +
 >> +app1:write(test2str)
 >> +rc,src,err = app1:read(256)
 >
 > Please also test the case when the child writes both to
 > stdin and stdout (looks like you missed it).

Did you mean stderr & stdout?

 >
 >> +
 >> +test:is(src, fio.STDOUT, "#2. Read from STDOUT")
 >> +test:is(rc, test2str, "#2. Received exact string")
 >> +
 >> +test2str = 'abc\ndef\ngh'
 >> +
 >> +app1:write(test2str, 4)
 >> +local rc2,src2,err = app1:read(6)
 >> +
 >> +test:is(err, nil, "#2. Reading ok")
 >> +test:is(src2, fio.STDOUT, "#2. Read from STDOUT 2")
 >> +test:is(rc2, 'abc\n', "#2. Received exact string 2")
 >> +
 >> +rc,err = app1:kill()
 >> +test:is(rc, true, "#2. Sending kill(15)")
 >> +
 >> +rc,err = app1:wait()
 >> +test:is(rc, -15, "#2. Process was killed")
 >> +
 >> +app1 = nil
 >> +
 >> +-- Test 3. read from stdout with timeout
 >> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
 >> +                  stdout=fio.PIPE,
 >> +                  stdin=fio.PIPE,
 >> +                  stderr=fio.STDOUT})
 >> +test:isnt(app1, nil, "#3. Starting a existing application")
 >> +test:is(app1:get_stderr(), nil, "#3. STDERR is redirected")
 >> +test:isnt(app1:get_stdout(), nil, "#3. STDOUT is available")
 >> +test:isnt(app1:get_stdin(), nil, "#3. STDIN is available")
 >> +
 >> +
 >> +rc = app1:get_status()
 >> +test:is(rc, nil, "#3. Process is running")
 >> +
 >> +rc,src,err = app1:read2(256, 2)
 >> +
 >> +local e = errno()
 >> +test:is(e, errno.ETIMEDOUT, "#3. Timeout")
 >> +
 >> +
 >> +local test2str = '123\n456\n789'
 >> +
 >> +app1:write(test2str)
 >> +rc,src,err = app1:read2(256, 2)
 >> +
 >> +test:is(err, nil, "#3. Reading ok")
 >> +test:is(src, fio.STDOUT, "#3. Read from STDOUT")
 >> +test:is(rc, test2str, "#3. Received exact string")
 >> +
 >> +rc,err = app1:kill('SIGHUP')
 >> +test:is(rc, true, "#3. Sending kill(1)")
 >> +
 >> +rc,err = app1:wait()
 >> +test:is(rc, -1, "#3. Process was killed")
 >> +
 >> +app1 = nil
 >> +
 >> +-- Test 4. Redirect from file
 >> +local build_path = os.getenv("BUILDDIR")
 >> +local txt_filename = fio.pathjoin(build_path, 
'test/box-tap/fio_popen.sample.txt')
 >> +
 >> +local txt_file = fio.open(txt_filename, {'O_RDONLY'})
 >> +test:isnt(txt_file, nil, "#4. Open existing file for reading")
 >> +
 >> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
 >> +                  stdout=fio.PIPE,
 >> +                  stdin=txt_file,
 >> +                  stderr=fio.STDOUT})
 >> +test:isnt(app1, nil, "#4. Starting a existing application")
 >> +test:is(app1:get_stderr(), nil, "#4. STDERR is redirected")
 >> +test:isnt(app1:get_stdout(), nil, "#4. STDOUT is available")
 >> +test:is(app1:get_stdin(), nil, "#4. STDIN is redirected")
 >> +
 >> +rc = app1:get_status()
 >> +test:is(rc, nil, "#4. Process is running")
 >> +
 >> +rc,src,err = app1:read2(256, 2)
 >> +
 >> +test:is(src, fio.STDOUT, "#4. Read from STDOUT")
 >> +
 >> +local test2str = 'AAA\nBBB\nCCC\nDDD\n\n'
 >> +
 >> +test:is(rc, test2str, "#4. Received exact string")
 >> +
 >> +rc,err = app1:wait()
 >> +test:is(rc, 0, "#4. Process's exited")
 >> +
 >> +app1 = nil
 >> +txt_file:close()
 >> +
 >> +-- Test 5. Redirect output from one process to another
 >> +local app_path = fio.pathjoin(build_path, 
'test/box-tap/fio_popen_test1.sh')
 >> +
 >> +app1 = fio.popen({argv = {'/bin/sh', '-c', app_path},
 >> +                  stdout=fio.PIPE,
 >> +                  stderr=fio.STDOUT})
 >> +
 >> +test:isnt(app1, nil, "#5. Starting application 1")
 >> +test:is(app1:get_stderr(), nil, "#5. STDERR is redirected")
 >> +test:isnt(app1:get_stdout(), nil, "#5. STDOUT is available")
 >> +test:isnt(app1:get_stdin(), nil, "#5. STDIN is available")
 >> +
 >> +fiber.sleep(1)
 >> +
 >> +local app2 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
 >> +                  stdout=fio.PIPE,
 >> +                  stdin=app1:get_stdout()})
 >> +
 >> +test:isnt(app2, nil, "#5. Starting application 2")
 >> +
 >> +local test2str = '1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n'
 >> +
 >> +rc = read_stdout(app2)
 >
 > Please also test reading/writing/killing in case when the child is dead.
 >

ok

 >> +
 >> +test:is(rc, test2str, "#5. Received exact string")
 >> +
 >> +rc,err = app1:wait()
 >> +test:is(rc, 0, "#5. Process's exited 1")
 >> +
 >> +rc,err = app2:wait()
 >> +test:is(rc, 0, "#5. Process's exited 2")
 >> +
 >> +app1 = nil
 >> +app2 = nil
 >> +
 >> +-- --------------------------------------------------------------
 >> +test:check()
 >> +os.exit(0)
 >> +
 >> diff --git a/test/box-tap/fio_popen_test1.sh 
b/test/box-tap/fio_popen_test1.sh
 >> new file mode 100755
 >> index 000000000..da54ee19a
 >> --- /dev/null
 >> +++ b/test/box-tap/fio_popen_test1.sh
 >> @@ -0,0 +1,7 @@
 >> +#!/bin/bash
 >> +for i in {1..10}
 >> +do
 >> +	echo $i
 >> +#	sleep 1
 >> +done
 >> +
 >

Re: [tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Alexander Turenko]

> > Come to think of it, we might need an explicit knob to destroy an object
> > even before it's collected. May be, consider re-adding close() for this?
> > Please consult with Alexander and/or Georgy re this.
> >
> 
> Sasha, George, what do you think?

I'm a bit out of context. Why we need explicit close()? To control
reaching a file descriptor limit? It can be worked around with
`handle = nil collectgarbage()`, but an explicit close() looks better.
Maybe it worth to add.

Re: [tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Stanislav Zudin]

On 03.06.2019 20:25, Alexander Turenko wrote:
>>> Come to think of it, we might need an explicit knob to destroy an object
>>> even before it's collected. May be, consider re-adding close() for this?
>>> Please consult with Alexander and/or Georgy re this.
>>>
>>
>> Sasha, George, what do you think?
> 
> I'm a bit out of context. Why we need explicit close()? To control
> reaching a file descriptor limit? It can be worked around with
> `handle = nil collectgarbage()`, but an explicit close() looks better.
> Maybe it worth to add.
> 

For now there is wait() as a final method. It waits for process 
termination and releases resources.
So the question was: should we add close() to release resources
explicitly?
Anyway there is a finalizer who performs a final cleanup for the case 
when user forgot to do it.

Re: [tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Vladimir Davydov]

On Tue, Jun 04, 2019 at 10:11:15AM +0300, Stanislav Zudin wrote:
> 
> 
> On 03.06.2019 20:25, Alexander Turenko wrote:
> > > > Come to think of it, we might need an explicit knob to destroy an object
> > > > even before it's collected. May be, consider re-adding close() for this?
> > > > Please consult with Alexander and/or Georgy re this.
> > > > 
> > > 
> > > Sasha, George, what do you think?
> > 
> > I'm a bit out of context. Why we need explicit close()? To control
> > reaching a file descriptor limit? It can be worked around with
> > `handle = nil collectgarbage()`, but an explicit close() looks better.
> > Maybe it worth to add.
> > 
> 
> For now there is wait() as a final method. It waits for process termination
> and releases resources.

We might want to read process output after waiting for it to complete.
Or it doesn't make any sense?

> So the question was: should we add close() to release resources
> explicitly?
> Anyway there is a finalizer who performs a final cleanup for the case when
> user forgot to do it.

To trigger the finalizer, we need to delete the variable

  p = nil

which isn't very user friendly IMO.

Re: [tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Alexander Turenko]

On Tue, Jun 04, 2019 at 10:59:31AM +0300, Vladimir Davydov wrote:
> On Tue, Jun 04, 2019 at 10:11:15AM +0300, Stanislav Zudin wrote:
> > 
> > 
> > On 03.06.2019 20:25, Alexander Turenko wrote:
> > > > > Come to think of it, we might need an explicit knob to destroy an object
> > > > > even before it's collected. May be, consider re-adding close() for this?
> > > > > Please consult with Alexander and/or Georgy re this.
> > > > > 
> > > > 
> > > > Sasha, George, what do you think?
> > > 
> > > I'm a bit out of context. Why we need explicit close()? To control
> > > reaching a file descriptor limit? It can be worked around with
> > > `handle = nil collectgarbage()`, but an explicit close() looks better.
> > > Maybe it worth to add.
> > > 
> > 
> > For now there is wait() as a final method. It waits for process termination
> > and releases resources.
> 
> We might want to read process output after waiting for it to complete.
> Or it doesn't make any sense?

I imagine this in the following way.

One way to implement the API is to map C/Unix API to work with child
processes / pipes in more or less direct way and so wait() just waits
for a process to terminate, read() / write() performs operations on a
pipe internal buffer. The only significant differences are proper
integration with our event loop and providing helpful popen handle that
accumulate all related file descriptors and information fields.

Another way is to hide deeper real process and pipes under our
abstractions: wait() expects both a process termination and pipes
closing, while read() / write() performs operations on buffers in our
memory and hide real read() / write(), which is performed in background.
This way is safer in the sense of a pipe buffer size exceeding, but
provides less control over using memory for buffers (they can take a
large memory).

If we'll going the first way it seems natural to allow to do read()
after wait() and provide close() to free resources manually. The second
way allows to read everything to the end in wait() and only then really
close pipes. Both ways should allow a user to perform API's read() after
wait(), the difference only when we actually call close() on a pipe.

I don't see a reason to make things more complex then they are and I
would consider popen implementation as the thin layer between Lua and
Unix API.

But maybe I mix aspects of the API that should be considered separately,
don't sure.

> 
> > So the question was: should we add close() to release resources
> > explicitly?
> > Anyway there is a finalizer who performs a final cleanup for the case when
> > user forgot to do it.
> 
> To trigger the finalizer, we need to delete the variable
> 
>   p = nil
> 
> which isn't very user friendly IMO.

I think explicit close() is needed for applications that want to
carefully manage system resources. Say, you writing a harness (runner)
for tests and each run calls popen. You can run hundreds processes per
second and GC may be not aggressive enough to save you from, say, open
file descriptor limit. One can tune GC options or manually call
collectgarbage(), but is looks more as the workaround rather then
solutions for the problem.

Re: [tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Vladimir Davydov]

On Mon, Jun 03, 2019 at 06:52:35PM +0300, Stanislav Zudin wrote:
> > > rc,src,err = handle:read(buffer,size)
> > > rc,src,err = handle:read2(buffer,size,seconds)
> > > 
> > > read stdout & stderr of the process started by fio.popen
> > > read() -> str, source
> > > read(buf) -> len, source
> > > read(size) -> str, source
> > > read(buf, size) -> len, source
> > > read2(seconds) -> str, source
> > > read2(buf,seconds) -> len, source
> > > read2(size,seconds) -> str, source
> > > read2(buf, size,seconds) -> len, source
> > 
> > Please use the same function name for both variants - read() - as
> > you can figure out what to do by looking at function arguments, no?
> > 
> 
> Well, actually there is an obstacle.
> The size and the seconds are both 'number' so we can't distinguish them.
> The possible way is to pass the fixed number of arguments, e.g.
> read(nil,size) instead of read(size),
> read(buf, nil, seconds) instead of read2(buf,seconds)
> and so on.
> If this approach is acceptable then i'll make the changes.

Dunno. Need to think. May be, pass seconds in a table? AKA options?
I'll talk to Alexander T. - may be he knows a better way.

> > > +static int
> > > +coio_do_nonblock_popen_read(void *fh, void *buf, size_t count,
> > > +	size_t *read_bytes, int *source_id)
> > > +{
> > > +	INIT_COEIO_FILE(eio);
> > > +	eio.popen_read.buf = buf;
> > > +	eio.popen_read.count = count;
> > > +	eio.popen_read.handle = fh;
> > > +	eio.popen_read.read_bytes = read_bytes;
> > > +	eio.popen_read.output_number = source_id;
> > > +	eio_req *req = eio_custom(coio_do_popen_read, 0,
> > > +				  coio_complete, &eio);
> > > +	return coio_wait_done(req, &eio);
> > > +}
> > > +
> > > +ssize_t
> > > +coio_popen_read(void *fh, void *buf, size_t count,
> > > +		int *output_number, int timeout)
> > > +{
> > > +	size_t received = 0;
> > > +	int rc = coio_popen_try_to_read(fh, buf, count,
> > > +		&received, output_number);
> > 
> > You call the same function coio_popen_try_to_read from both tx and coio.
> > How's that? If it's blocking, it blocks tx, which is unacceptable. If it
> > isn't, why call it from coio at all?
> 
> coio_popen_try_to_read is not blocking.

Then it doesn't need to be called via coio.

> But somebody should call fiber_yield(). And coio_file.c contains all
> facilities.
> 
> > > +int
> > > +coio_popen_wait(void *fh, int timeout, int *exit_code)
> > > +{
> > 
> > This function doesn't depend on coio_file infrastructure. Why define it
> > here at all?
> > 
> 
> popen is part of fio.
> fio is implemented in coio_* functions.

You could define it in coio_popen.c then. Actually, I think you could
move all popen stuff there and use coio_call for blocking syscalls.

> > > +	pid = fork();
> > 
> > Please use vfork().
> > 
> 
> I'm afraid it won't work because of at_fork handlers.

Why? Please elaborate.

> > > +	popen_lock_data_list();
> > 
> > This function is called from a signal handler => it might deadlock with
> > coio_popen_destroy, for instance. Please consider using evio signal
> > handlers (take a look at ev_signal_init).
> 
> evio drops all information related to the signal and just reports that the
> signal had been caught.
> And since evio is implemented upon the signalfd() the further calls of
> waitpid are useless - the information about child process was discarded.
> All you can get is the fact that the process is not running.
> 
> So i see the following ways to resolve this problem:
> 1. Refactoring of evio - to keep siginfo_t received with a signal
> (the worst idea IMO).
> 2. Use signalfd and run the own thread to dispatch SIGCHLD
> 3. Deal with the lack of child's exit code
> 4. make the wait() is mandatory, call waitpid() and do not use SIGCHLD
> handler at all.

Please take a look at ev_child.

> > > -#define fiber() cord()->fiber
> > > -#define loop() (cord()->loop)
> > > +#define fiber() (cord() ? cord()->fiber : NULL)
> > > +#define loop() (cord() ? cord()->loop : NULL)
> > 
> > Why is that? A comment would be nice to have.
> 
> It's an at_fork() consequences.

What consequences? What happens without this change and why?
Please elaborate.

> Not sure if we can safely remove at_fork handlers.
> The parent process is forking to run as a daemon.
> 
> > > +	PUSHTABLE("SIGUSR1", lua_pushinteger, SIGUSR1);
> > > +	PUSHTABLE("SIGUSR2", lua_pushinteger, SIGUSR2);
> > > +	lua_settable(L, -3); /* "signals" */
> > 
> > This could be done from Lua - signal numbers are standard AFAIK.
> Did you mean to use constants like fio.STDIN?

Yes. Don't see much point in defining them in C.

> > > diff --git a/test/box-tap/fio_popen.test.lua b/test/box-tap/fio_popen.test.lua
> > 
> > Should be app-tap? Anyway, why tap test? Normal tests are easier to
> > write and understand IMO.
> > 
> 
> Ok, let it be app-tap.
> 
> What a 'Normal' test? The ones in e.g. test/app directory?
> It's hard to find the error when result files are quite big.
> Have to use external tools, e.g. 'diff' etc.

Yes, but I find it more convenient than analyzing a tap test result
file. Please ask Alexander T. about our test policy.

> > > +-- Test 2. Run application, write to stdin, read from stdout
> > > +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> > 
> > What's the point using '/bin/sh' just to run 'cat'?
> > 
> 
> cat fails without it.

Why? It should be possible to run 'cat' directly, without the help of
'sh' AFAIU.

> > > diff --git a/third_party/libeio/eio.c b/third_party/libeio/eio.c
> > > index 7351d5dda..e433b1b3b 100644
> > > --- a/third_party/libeio/eio.c
> > > +++ b/third_party/libeio/eio.c
> > > @@ -1741,7 +1741,24 @@ static int eio_threads;
> > >   static void ecb_cold
> > >   eio_prefork()
> > >   {
> > > -    eio_threads = etp_set_max_parallel(EIO_POOL, 0);
> > > +	/*
> > > +	 * When fork() is called libeio shuts
> > > +	 * down all working threads.
> > > +	 * But it causes a deadlock if fork() was
> > > +	 * called from the libeio thread.
> > > +	 * To avoid this do not close the
> > > +	 * thread who called fork().
> > > +	 * This behaviour is acceptable for the
> > > +	 * case when fork() is immediately followed
> > > +	 * by exec().
> > > +	 * To clone a process call fork() from the
> > > +	 * main thread.
> > > +	 */
> > > +
> > > +	if (etp_is_in_pool_thread())
> > > +		eio_threads = etp_get_max_parallel(EIO_POOL);
> > > +	else
> > > +    		eio_threads = etp_set_max_parallel(EIO_POOL, 0);
> > 
> > Please investigate why it was initially done that way, because
> > I'm afraid this change might break something.
> > 
> 
> A usual precautions before fork: stop all thread before fork and resume
> after fork.

If it was just a precaution, then we could remove it altogether rather
than adding a workaround, couldn't we?

However, judging by the commit log, this behavior isn't a part of the
standard libev - it was added by us deliberately. Please check out why.
May be, the commit history and/or ticket description will shed some
light on it.

> This works fine if fork() is called in the main thread.
> Of course the current workaround has its disadvantages. But they're not
> worth mentioning in the case when fork() is followed by exec.

What are the disadvantages?

> > > diff --git a/third_party/libev/ev.c b/third_party/libev/ev.c
> > > index 6a2648591..5fa8293a1 100644
> > > --- a/third_party/libev/ev.c
> > > +++ b/third_party/libev/ev.c
> > > @@ -4214,6 +4214,8 @@ noinline
> > >   void
> > >   ev_signal_stop (EV_P_ ev_signal *w) EV_THROW
> > >   {
> > > +	if (!loop)
> > > +		return;
> > 
> > Why is that? At least deserves a comment. Better handle it at the upper
> > level.
> 
> It's an at_fork() consequences.
> See my comments above.

Please elaborate what exactly happens and why. Can we move this check to
the place where ev_signal_stop is called? It looks very suspicious here.
Especially without a comment.

> >> +static struct popen_data *
> >> +popen_lookup_data_by_pid(pid_t pid)
> >> +{
> >> +	struct popen_data *cur = popen_data_list;
> >> +	for(; cur; cur = cur->next) {
> >> +		if (cur->pid == pid)
> >> +			return cur;
> >> +	}
> >
> > Please write a comment explaining why you think linear search is fine
> > here and why we don't use some kind of hash or tree.
> >
> 
> I believe the number of simultaneously running popen processes is rather
> small - tens. In this case hash won't give a significant performance boost.

I don't really bother, but since Kostja insists on using hash, I'd
consider using mhash for mapping pid->popen_handle if I were you.
It should be pretty trivial to do without cluttering the code and
hence isn't worth arguing about IMO.

> >> +/*
> >> + * Returns next socket to read.
> >> + * Use this function when both STDOUT and STDERR outputs
> >> + * are ready for reading.
> >
> > But what does it do? How does it prioritize? Though short, this function
> > looks kinda arcane to me due to this bit manipulation magic. Please
> > clarify. However, if you switch to ev io, you'll probably won't be
> > needing it.
> >
> 
> It's just a round-robin.
> Used in a case when both STDIN & STDOUT are ready for reading while
> popen.read() returns only one result. Doesn't let any stream stuck.

TBO I don't see any point in RR scheduling in this case. I'd simply use
stderr if it's available (as it's considered high prio typically),
otherwise use stdout. If stdout gets stuck, the process will block and
hence stop writing to stderr eventually and we switch to stdout.
Anyway, worth explaining the logic behind this code in a comment.

> >> +void *
> >> +coio_popen_impl(char** argv, int argc,
> >> +	char** environment, int environment_size,
> >> +	int stdin_fd, int stdout_fd, int stderr_fd)
> >> +{
> >> +	pid_t pid;
> >> +	int socket_rd[2] = {-1,-1};
> >> +	int socket_wr[2] = {-1,-1};
> >> +	int socket_er[2] = {-1,-1};
> >> +	errno = 0;
> >> +
> >> +	struct popen_data *data = popen_data_new();
> >> +	if (data == NULL)
> >> +		return NULL;
> >> +
> >> +	/* Setup a /dev/null if necessary */
> >> +	bool read_devnull = (stdin_fd == FIO_DEVNULL);
> >> +	bool write_devnull = (stdout_fd == FIO_DEVNULL) ||
> >> +			     (stderr_fd == FIO_DEVNULL);
> >> +	int devnull_flags = O_RDWR | O_CREAT;
> >> +	if (!read_devnull)
> >> +		devnull_flags = O_WRONLY | O_CREAT;
> >> +	else if (!write_devnull)
> >> +		devnull_flags = O_RDONLY | O_CREAT;
> >> +
> >> +	if (read_devnull || write_devnull) {
> >> +		data->devnull_fd = open("/dev/null", devnull_flags, 0666);
> >
> > No need to pass mode unless you're creating a file.
> 
> It doesn't affect the existing files. Thus one line is shorted than two.
>  :-)

Oh, I think I see - you use O_CREAT. Please don't. We aren't suppose to
create /dev/null if it doesn't exist - better fail in this case.

> 
> >
> >> +		if (data->devnull_fd < 0)
> >> +			goto on_error;
> >> +		else {
> >> +			if (stdin_fd == FIO_DEVNULL)
> >> +				stdin_fd = data->devnull_fd;
> >> +			if (stdout_fd == FIO_DEVNULL)
> >> +				stdout_fd = data->devnull_fd;
> >> +			if (stderr_fd == FIO_DEVNULL)
> >> +				stderr_fd = data->devnull_fd;
> >> +		}
> >> +	}
> >> +
> >> +	if (stdin_fd == FIO_PIPE) {
> >> +		/*
> >> +		 * Enable non-blocking for the parent side
> >> +		 * and close-on-exec on the child's side.
> >> +		 * The socketpair on OSX doesn't support
> >> +		 * SOCK_NONBLOCK & SOCK_CLOEXEC flags.
> >> +		 */
> >> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_rd) < 0 ||
> >
> > I overlooked it when I reviewed the patch last time. Why do you use
> > sockets? Why is a simple pipe not enough? Sockets are full-duplex
> > while in your case a simplex connection would be enough, no?
> 
> Ask Georgy, he insisted on replacing pipes by socketpair.

Once you've submitted a patch, it's your responsibility to stand up for
every aspect of each design decision made in the code. Simply listening
to anyone without understanding and, most importantly, accepting his
argumentation isn't a good idea.

Please elaborate on why using socketpair() is important. What are the
problems with pipe()? Can we circumvent the problems somehow? Because
to me socketpair() seems to be a bit of an overkill - as I said it's
bi-directional while for our purposes a uni-directional channel is
enough.

> >> +		execve( argv[0], argv,
> >> +			environment ? environment : environ);
> >> +		_exit(127);
> >
> > Why _exit(), why not exit()? If there's a reason, please explain in a
> > comment. Also, why 127? We prefer to avoid explicit numeric constants.
> > May be, you could use EXIT_FAILURE instead?
> 
> _exit() doesn't call atexit(), but in particular this case it doesn't
> matter.

Then we should use exit() so as not to raise questions.

> As to exit code i need to distinguish exit code returned by child process
> from execve error.
> The native implementation of popen does return 127.

Okay, I see. Please put this reasoning in a comment. Also, it might be
worth defining this exit code in a constant (or use an existing one if
it already exists) with a proper comment.

> >> +void
> >> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *context)
> >> +{
> >> +	(void)context;	/* UNUSED */
> >
> > Why pass it at all if it's unused?
> >
> 
> Because C compiler requires the argument to be defined.
> 
> [6.9.1.5] If the declarator includes a parameter type list, the declaration
> of each parameter shall include an identifier, except for the special case
> of a parameter list consisting of a single parameter of type void, in which
> case there shall not be an identifier. No declaration list shall follow.
> 
> Changing the compiler flags is too much, IMO.

Oh, you pass this function directly to sigaction. Missed that.
IMO this isn't a good idea as it makes the popen internal API
depend on siginfo_t. I'd add a wrapper to main.cc and call
this function from it, passing only arguments it requires.

> >> +/**
> >> + * Handle SIGCHLD signal
> >> + */
> >> +void
> >> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *);
> >
> > Better not use siginfo_t here - instead pass what you really need.
> > This makes the API clearer and cuts the dependency list.
> >
> 
> coio_popen_child_is_dead is passed to sigaction.
> To separate it from siginfo_t we need one more function.
> No problem, will add.

Yes, please do.

> 
> >> diff --git a/src/lua/fio.c b/src/lua/fio.c
> >> +/**
> >> + * A wrapper applicable for using with lua's GC.
> >> + * */
> >> +struct fio_popen_wrap {
> >
> > Why do you need a wrapper at all?
> 
> Right now it contains only a "handle" (a pointer to
> struct popen_data *).
> Later we may need to keep something else in the managed memory.
> It's more readable than just a pointer to pointer.

AFAIK we don't usually add such trivial wrappers to export objects to
Lua. Please ask Alexander T. as he's an expert in Lua-C interaction.

> 
> >
> >> +	void* handle;
> >> +};
> >> +static const char* fio_popen_typename = "fio.popen.data";
> >> +
> >> +static struct fio_popen_wrap *
> >> +fio_popen_get_handle_wrap(lua_State *L, int index)
> >> +{
> >> +	luaL_checktype(L, index, LUA_TUSERDATA);
> >> +	struct fio_popen_wrap *wrap = (struct fio_popen_wrap *)
> >> +		luaL_checkudata(L, index, fio_popen_typename);
> >
> > Hmm, why do you use udata? Why not plain cdata? I keep forgetting
> > the difference, to tell the truth.
> 
> The Lua finalizer deals only with "managed" memory.
> It's not aware of cdata and can't release it.

Hmm, but we use finalizers with cdata - take a look at luaT_pushtuble
and how it uses luaL_setcdatagc. Again, it's worth talking to Alex T.
re this.

> 
> >
> >> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> >> +popen_methods.do_read = function(self, buf, size, seconds)
> >> +    if size == nil or type(size) ~= 'number' then
> >> +        error('fio.popen.read: invalid size argument')
> >> +    end
> >> +    if seconds == nil or type(seconds) ~= 'number' then
> >> +        error('fio.popen.read: invalid seconds argument')
> >> +    end
> >> +
> >> +    local tmpbuf
> >> +    if not ffi.istype(const_char_ptr_t, buf) then
> >> +        tmpbuf = buffer.ibuf()
> >> +        buf = tmpbuf:reserve(size)
> >> +    end
> >> +
> >> +    local res, output_no, err = internal.popen_read(self.fh, buf, size,
> seconds)
> >> +    if res == nil then
> >> +        if tmpbuf ~= nil then
> >> +            tmpbuf:recycle()
> >> +        end
> >> +        return nil, nil, err
> >> +    end
> >> +
> >> +    if tmpbuf ~= nil then
> >> +        tmpbuf:alloc(res)
> >> +        res = ffi.string(tmpbuf.rpos, tmpbuf:size())
> >> +        tmpbuf:recycle()
> >> +    end
> >> +    return res, output_no
> >> +end
> >> +
> >> +-- read stdout & stderr of the process started by fio.popen
> >> +-- read() -> str, source
> >> +-- read(buf) -> len, source
> >> +-- read(size) -> str, source
> >> +-- read(buf, size) -> len, source
> >> +-- source contains id of the stream, fio.STDOUT or fio.STDERR
> >> +popen_methods.read = function(self, buf, size)
> >> +    if self.fh == nil then
> >> +        return nil, nil, 'Invalid object'
> >> +    end
> >> +
> >> +    if buf == nil and size == nil then
> >> +        -- read()
> >> +        size = 512
> >
> > Why 512? Aren't we supposed to read everything till EOF in this case?
> 
> The behaviour was borrowed from fio.read()

Hmm, I don't see any 512 in fio.read().

> The reading everything till EOF causes problems in reading from both
> sources: STDIN and STDOUT. While you waiting one source another one may
> get stuck.

I don't quite understand. We don't really wait on a source - we use
select and non-blocking reads. Once a source is ready, we read from it.
If another is full, well, okay, we continue reading the first one until
there's data in it. Am I missing something obvious?

> 
> >
> >> +    elseif ffi.istype(const_char_ptr_t, buf) and size == nil then
> >> +        -- read(buf)
> >> +        size = 512
> >> +    elseif not ffi.istype(const_char_ptr_t, buf) and
> >> +            buf ~= nil and size == nil then
> >> +        -- read(size)
> >> +        size = tonumber(buf)
> >> +        buf = nil
> >> +    elseif ffi.istype(const_char_ptr_t, buf) and size ~= nil then
> >> +        -- read(buf, size)
> >> +        size = tonumber(size)
> >> +    else
> >> +        error("fio.popen.read: invalid arguments")
> >> +    end
> >> +
> >> +    return self:do_read(buf, size, -1)
> >> +end
> >
> >> +popen_methods.wait = function(self, timeout)
> >> +    if self.fh == nil then
> >> +        return nil, 'Invalid object'
> >> +    end
> >> +
> >> +    if timeout == nil then
> >> +        timeout = -1
> >> +    else
> >> +        timeout = tonumber(timeout)
> >> +    end
> >> +
> >> +    local rc, err = internal.popen_wait(self.fh, timeout)
> >> +    if rc ~= nil then
> >> +        self.exit_code = tonumber(rc)
> >> +        self.fh = nil
> >
> > This is kinda unexpected. What if we want to read the process output
> > after waiting for it to complete.
> 
> And what do you expect to read after process is complete?
> If application is a some kind of linear script it produces some output and
> then exits. In this case the caller reads everything till the EOF.
> BTW, in our tests test scripts exit earlier than the parent starts
> reading their output.
> 
> For interactive applications who may wait for input forever we've got a
> kill(). It's allowed to read after sending signal and before calling
> wait(). In this case we won't miss anything from the child process output.
> 
> So my point is: wait() is a synonim to close() (or join() in C++ world).
> It releases all resources.

Okay, makes sense. If George and Alexander agree, I'm okay with it.

> >> +app1:write(test2str)
> >> +rc,src,err = app1:read(256)
> >
> > Please also test the case when the child writes both to
> > stdin and stdout (looks like you missed it).
> 
> Did you mean stderr & stdout?

Yeah, s/stdin/stderr.

Re: [tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Alexander Turenko]

Hi all!

I looked at the questions that were passed directly to me. See my
thoughts below. Hope it'll help.

WBR, Alexander Turenko.

On Tue, Jun 04, 2019 at 02:14:01PM +0300, Vladimir Davydov wrote:
> On Mon, Jun 03, 2019 at 06:52:35PM +0300, Stanislav Zudin wrote:
> > > > rc,src,err = handle:read(buffer,size)
> > > > rc,src,err = handle:read2(buffer,size,seconds)
> > > > 
> > > > read stdout & stderr of the process started by fio.popen
> > > > read() -> str, source
> > > > read(buf) -> len, source
> > > > read(size) -> str, source
> > > > read(buf, size) -> len, source
> > > > read2(seconds) -> str, source
> > > > read2(buf,seconds) -> len, source
> > > > read2(size,seconds) -> str, source
> > > > read2(buf, size,seconds) -> len, source
> > > 
> > > Please use the same function name for both variants - read() - as
> > > you can figure out what to do by looking at function arguments, no?
> > > 
> > 
> > Well, actually there is an obstacle.
> > The size and the seconds are both 'number' so we can't distinguish them.
> > The possible way is to pass the fixed number of arguments, e.g.
> > read(nil,size) instead of read(size),
> > read(buf, nil, seconds) instead of read2(buf,seconds)
> > and so on.
> > If this approach is acceptable then i'll make the changes.
> 
> Dunno. Need to think. May be, pass seconds in a table? AKA options?
> I'll talk to Alexander T. - may be he knows a better way.

I'm sure we should not name the function that consider a timeout as
read2(). The possible solution is to name it read_timeout(). This
naming approach is used in coio for example (but it is C).

Another way is to add 'opts' argument for the function: it will be
distinguishable from 'size', because it is a Lua table. There are
questions however:

* Whether this argument should be always on third
  position? So a user will need to call it like :read(nil, nil, {timeout
  = <...>}. Looks complex. For example I never remember how much nils I
  should add for box.schema.user.grant() before pass {if_not_exists =
  true}.
* If not, the position can be 1st, 2nd or 3rd, but always last.
  Personally I found such convention complex to understand too.
* The reason why all this position-related problems appear is that we
  don't wrap optional arguments into 'opts'. Usually we (at least me) do
  and there are no such problems. However here we want to provide an API
  that looks similar to fio-handle:read().

There is another option:

* read([size])
* read(buf[, size])
* read(opts), where opts are:
  * buf
  * size
  * timeout

So we support all optional arguments as options, but keep fio-like API.

I also suggest to look into the old discussion about net.box, where
similar questions were arisen:
https://github.com/tarantool/tarantool/issues/2195

> > > > diff --git a/test/box-tap/fio_popen.test.lua b/test/box-tap/fio_popen.test.lua
> > > 
> > > Should be app-tap? Anyway, why tap test? Normal tests are easier to
> > > write and understand IMO.
> > > 
> > 
> > Ok, let it be app-tap.
> > 
> > What a 'Normal' test? The ones in e.g. test/app directory?
> > It's hard to find the error when result files are quite big.
> > Have to use external tools, e.g. 'diff' etc.
> 
> Yes, but I find it more convenient than analyzing a tap test result
> file. Please ask Alexander T. about our test policy.

Personally I like 'core = app' tests and using 'tap' module in them. My
points here the following:

* I'm able to check necessary parts of a result as I wish.
* Results are under my eyes when I read a test. No need to move around
  two files.
* I'm able to write a test in declarative manner (when there are many
  similar checks for different inputs).
* I'm able to run a test w/o test-run. It is just Lua file, nothing
  special.

Vladimir likes 'core = tarantool' tests. Points I had listen here from
different persons are the following:

* It is easy to write.
* It can be copy-pasted to console to check.

So there is a thing of preference and I hope one should not push another
developer to use specific approach.

I took glance at your test and suggest to consider `luacheck -r
test/box-tap/fio_popen.test.lua` output (-r is to allow redefinition of
a variable, it is often convenient for 'ok', 'rc' and such variables).
This will make the test look better for ones who write many code on Lua.

Luacheck often reports unknown tarantool-specific globals. You can
ignore those warnings or write .luacheckrc file like this one:
https://github.com/tarantool/graphql/blob/c26aa1e8f65129a938f5af12cc644b8001f517d7/.luacheckrc

> > >> diff --git a/src/lua/fio.c b/src/lua/fio.c
> > >> +/**
> > >> + * A wrapper applicable for using with lua's GC.
> > >> + * */
> > >> +struct fio_popen_wrap {
> > >
> > > Why do you need a wrapper at all?
> > 
> > Right now it contains only a "handle" (a pointer to
> > struct popen_data *).
> > Later we may need to keep something else in the managed memory.
> > It's more readable than just a pointer to pointer.
> 
> AFAIK we don't usually add such trivial wrappers to export objects to
> Lua. Please ask Alexander T. as he's an expert in Lua-C interaction.

What is the problem with GC? I don't sure about userdata, but we use
cdata of <struct foo *> for structures that should be garbage collected
with LuaJIT GC. See lbox_merge_source_new() for example: it set a
callback for GC using luaL_setcdatagc(). buffer.lua however use cdata of
<struct ibuf> (non-pointer type) and it seems it allows to avoid extra
memory allocation for the structure. Using a pointer type however allows
to hide fields from Lua (don't define a structure in ffi.cdef).

I don't have an opinion about using userdata or cdata. They looks quite
similar except that latter provides automatic getters / setters for a
structure fields, comparison operators and so on.

As I see we use userdata across the code, but use cdata more often. I
suggest to discuss this with Nick Z. or Roman T.: I guess they have a
way more context about LuaJIT usage and internals. Ask me for contacts
privately.

I see you already have <struct popen_data>, but cast a pointer to it to
<void *>. Why not to use a pointer to the structure as a handle (you can
just declare it in a header file, but don't define) and expose it to Lua
as cdata<struct popen_data *>? This way looks more clear for me than the
current approach.

Naming is the question too. I would name the handle structure
<struct popen> or <struct popen_handle>.

> > >> +	void* handle;
> > >> +};
> > >> +static const char* fio_popen_typename = "fio.popen.data";
> > >> +
> > >> +static struct fio_popen_wrap *
> > >> +fio_popen_get_handle_wrap(lua_State *L, int index)
> > >> +{
> > >> +	luaL_checktype(L, index, LUA_TUSERDATA);
> > >> +	struct fio_popen_wrap *wrap = (struct fio_popen_wrap *)
> > >> +		luaL_checkudata(L, index, fio_popen_typename);
> > >
> > > Hmm, why do you use udata? Why not plain cdata? I keep forgetting
> > > the difference, to tell the truth.
> > 
> > The Lua finalizer deals only with "managed" memory.
> > It's not aware of cdata and can't release it.
> 
> Hmm, but we use finalizers with cdata - take a look at luaT_pushtuble
> and how it uses luaL_setcdatagc. Again, it's worth talking to Alex T.
> re this.

It seems I already cover this question above. Yep, it is possible to set
a finalizer with cdata. I think userdata allows this too (at least by
setting a metatable with __gc metamethod).

Re: [PATCH v2] core: Non-blocking io.popen

[Vladimir Davydov]

A few more comments. Please take a look.

On Wed, May 29, 2019 at 10:08:09AM +0300, Stanislav Zudin wrote:
> diff --git a/src/lib/core/coio_popen.c b/src/lib/core/coio_popen.c
> +#include <stddef.h>
> +#include <signal.h>
> +#include "coio_popen.h"
> +#include "coio_task.h"
> +#include "fiber.h"
> +#include "say.h"
> +#include "fio.h"
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <dirent.h>
> +#include <sys/wait.h>
> +#include <paths.h>

Why do you need this header? Looks like you never use anything from it.
Same for dirent.h. Please include only those headers you really need.

> +static struct popen_data *
> +popen_lookup_data_by_pid(pid_t pid)
> +{
> +	struct popen_data *cur = popen_data_list;
> +	for(; cur; cur = cur->next) {
> +		if (cur->pid == pid)
> +			return cur;
> +	}

Please write a comment explaining why you think linear search is fine
here and why we don't use some kind of hash or tree.

> +
> +	return NULL;
> +}
> +
> +static void
> +popen_exclude_from_list(struct popen_data *data)
> +{
> +	if (popen_data_list == data) {
> +		popen_data_list = data->next;
> +		return;
> +	}
> +
> +	/* Find the previous entry */
> +	struct popen_data *prev = popen_data_list;
> +	for( ; prev && prev->next != data; prev = prev->next) {
> +		/* empty */;
> +	}
> +
> +	if (!prev)
> +		return ;
> +	assert(prev->next == data);
> +	prev->next = data->next;

Looks like doubly-linked list (rlist) would suit better here.

> +}
> +
> +/*
> + * Returns next socket to read.
> + * Use this function when both STDOUT and STDERR outputs
> + * are ready for reading.

But what does it do? How does it prioritize? Though short, this function
looks kinda arcane to me due to this bit manipulation magic. Please
clarify. However, if you switch to ev io, you'll probably won't be
needing it.

> + * */
> +static inline int
> +get_handle_in_order(struct popen_data *data)
> +{
> +	/*
> +	 * Invert the order of handles to be read
> +	 */
> +	const int mask = STDERR_FILENO | STDOUT_FILENO;
> +	data->prev_source ^= mask;
> +
> +	/*
> +	 * If handle is not available, invert it back
> +	 */
> +	if (data->fh[data->prev_source] < 0)
> +		data->prev_source ^= mask;
> +	/*
> +	 * if both reading handles are invalid return -1
> +	 */
> +	return data->fh[data->prev_source];
> +}

> +void *
> +coio_popen_impl(char** argv, int argc,
> +	char** environment, int environment_size,
> +	int stdin_fd, int stdout_fd, int stderr_fd)
> +{
> +	pid_t pid;
> +	int socket_rd[2] = {-1,-1};
> +	int socket_wr[2] = {-1,-1};
> +	int socket_er[2] = {-1,-1};
> +	errno = 0;
> +
> +	struct popen_data *data = popen_data_new();
> +	if (data == NULL)
> +		return NULL;
> +
> +	/* Setup a /dev/null if necessary */
> +	bool read_devnull = (stdin_fd == FIO_DEVNULL);
> +	bool write_devnull = (stdout_fd == FIO_DEVNULL) ||
> +			     (stderr_fd == FIO_DEVNULL);
> +	int devnull_flags = O_RDWR | O_CREAT;
> +	if (!read_devnull)
> +		devnull_flags = O_WRONLY | O_CREAT;
> +	else if (!write_devnull)
> +		devnull_flags = O_RDONLY | O_CREAT;
> +
> +	if (read_devnull || write_devnull) {
> +		data->devnull_fd = open("/dev/null", devnull_flags, 0666);

No need to pass mode unless you're creating a file.

> +		if (data->devnull_fd < 0)
> +			goto on_error;
> +		else {
> +			if (stdin_fd == FIO_DEVNULL)
> +				stdin_fd = data->devnull_fd;
> +			if (stdout_fd == FIO_DEVNULL)
> +				stdout_fd = data->devnull_fd;
> +			if (stderr_fd == FIO_DEVNULL)
> +				stderr_fd = data->devnull_fd;
> +		}
> +	}
> +
> +	if (stdin_fd == FIO_PIPE) {
> +		/*
> +		 * Enable non-blocking for the parent side
> +		 * and close-on-exec on the child's side.
> +		 * The socketpair on OSX doesn't support
> +		 * SOCK_NONBLOCK & SOCK_CLOEXEC flags.
> +		 */
> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_rd) < 0 ||

I overlooked it when I reviewed the patch last time. Why do you use
sockets? Why is a simple pipe not enough? Sockets are full-duplex
while in your case a simplex connection would be enough, no?

> +		    fcntl(socket_rd[0], F_SETFL, O_NONBLOCK) < 0 ||
> +		    fcntl(socket_rd[1], F_SETFD, FD_CLOEXEC) < 0) {

You don't really need FD_CLOEXEC for this one as you can close it in the
child right after fork(). Moreover, looks like you close() them anyway.

We do need to use CLOEXEC for other fds (e.g.  xlog or vinyl files), but
I guess this can be done in a separate patch.

> +			goto on_error;
> +		}
> +	}
> +
> +	if (stdout_fd == FIO_PIPE) {
> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_wr) < 0 ||
> +		    fcntl(socket_wr[0], F_SETFL, O_NONBLOCK) < 0 ||
> +		    fcntl(socket_wr[1], F_SETFD, FD_CLOEXEC) < 0) {
> +			goto on_error;
> +		}
> +	}
> +
> +	if (stderr_fd == FIO_PIPE) {
> +		if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_er) < 0 ||
> +		    fcntl(socket_er[0], F_SETFL, O_NONBLOCK) < 0 ||
> +		    fcntl(socket_er[1], F_SETFD, FD_CLOEXEC) < 0) {

Looks like quite a bit of repetition. Worth moving this to a separate
helper function or doing it in a loop somehow instead of copying and
pasting?

> +			goto on_error;
> +		}
> +	}
> +
> +	pid = fork();
> +
> +	if (pid < 0)
> +		goto on_error;
> +	else if (pid == 0) /* child */ {
> +		/* Setup stdin/stdout */
> +		if (stdin_fd == FIO_PIPE) {
> +			close(socket_rd[0]); /* close parent's side */
> +			stdin_fd = socket_rd[1];
> +		}
> +		if (stdin_fd != STDIN_FILENO) {
> +			dup2(stdin_fd, STDIN_FILENO);
> +			close(stdin_fd);
> +		}
> +
> +		if (stdout_fd == FIO_PIPE) {
> +			close(socket_wr[0]); /* close parent's side */
> +			stdout_fd = socket_wr[1];
> +		}
> +		if (stdout_fd != STDOUT_FILENO) {
> +			dup2(stdout_fd, STDOUT_FILENO);
> +			if (stdout_fd != STDERR_FILENO)
> +				close(stdout_fd);
> +		}
> +
> +		if (stderr_fd == FIO_PIPE) {
> +			close(socket_er[0]); /* close parent's side */
> +			stderr_fd = socket_er[1];
> +		}
> +		if (stderr_fd != STDERR_FILENO) {
> +			dup2(stderr_fd, STDERR_FILENO);
> +			if (stderr_fd != STDOUT_FILENO)
> +				close(stderr_fd);
> +		}
> +
> +		execve( argv[0], argv,
> +			environment ? environment : environ);
> +		_exit(127);

Why _exit(), why not exit()? If there's a reason, please explain in a
comment. Also, why 127? We prefer to avoid explicit numeric constants.
May be, you could use EXIT_FAILURE instead?

Also, may be we need to write something to stderr in this case, just to
let the caller now that it's not the child failure, it's execve failure.

> +int
> +coio_popen_destroy(void *fh)
> +{
> +	struct popen_data *data = (struct popen_data *)fh;
> +
> +	if (data == NULL){
> +		errno = EBADF;
> +		return -1;
> +	}

What's the point in returning an error from this function?
You never check it AFAICS.

Anyway, like I mentioned above, I think that one shouldn't pass NULL for
a handle to any of popen functions.

> +void
> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *context)

Judging by the name it looks like this function should merely return
true/false, not do anything else. Please rename. For instance, we could
name it coio_popen_on_child_termination or handle_child_death.

> +{
> +	(void)context;	/* UNUSED */

Why pass it at all if it's unused?

> +
> +	if (sig != SIGCHLD)
> +		return;

Why call it if it isn't SIGCHLD?

> +
> +	/*
> +	 * The sigaction is called with SA_NOCLDWAIT,
> +	 * so no need to call waitpid()
> +	 */
> +
> +	popen_lock_data_list();
> +
> +	struct popen_data *data = popen_lookup_data_by_pid(si->si_pid);
> +	if (data) {
> +		switch (si->si_code) {
> +		case CLD_EXITED:
> +			data->exit_code = si->si_status;
> +			data->status = POPEN_EXITED;
> +			break;
> +		case CLD_KILLED:
> +			data->signal_id = si->si_status;
> +			data->status = POPEN_KILLED;
> +			break;
> +		case CLD_DUMPED:
> +			/* exit_status makes no sense */
> +			data->status = POPEN_DUMPED;
> +			break;
> +		}
> +
> +		/*
> +		 * We shouldn't close file descriptors here.
> +		 * The child process may exit earlier than
> +		 * the parent process finishes reading data.
> +		 * In this case the reading fails.
> +		 */
> +	}
> +
> +	popen_unlock_data_list();
> +}

> diff --git a/src/lib/core/coio_popen.h b/src/lib/core/coio_popen.h
> +/**
> + * Special values of the file descriptors passed to fio.popen
> + * */
> +enum {
> +	FIO_PIPE = -2,
> +	/*
> +	 * Tells fio.popen to open a handle for
> +	 * direct reading/writing
> +	 */
> +
> +	FIO_DEVNULL = -3
> +	/*
> +	 * Tells fio.popen to redirect the given standard
> +	 * stream into /dev/null
> +	 */
> +};
> +
> +/**
> + * Possible status of the process started via fio.popen
> + **/
> +enum popen_status {
> +	POPEN_UNKNOWN = -1,

Why do you need this mysterious status code? Looking at the code, I see
that you coio_popen_get_status() returns it in case of error. But what
kind of error? Turns out that this is the case when the handle is NULL.
This looks pointless to me - one shouldn't pass NULL to that function.
Am I missing something?

> +
> +	POPEN_RUNNING = 1,
> +	/*the process is alive and well*/

Comments should be above constants, not below. Also, please use
formatting typically used in the code (starts with /**, capital
letter, a dot at the end).

> +
> +	POPEN_EXITED = 2,
> +	/*the process exited*/
> +
> +	POPEN_KILLED = 3,
> +	/*the process terminated by a signal*/
> +
> +	POPEN_DUMPED = 4
> +	/* the process terminated abnormally */
> +};

> +/**
> + * Returns status of the associated process.
> + *
> + * @param fd - handle returned by fio.popen.
> + * @param signal_id - if not NULL accepts the signal
> + * sent to terminate the process
> + * @param exit_code - if not NULL accepts the exit code
> + * if the process terminated normally.
> + *
> + * @return one of the following values:
> + * POPEN_RUNNING if the process is alive
> + * POPEN_EXITED if the process was terminated normally
> + * POPEN_KILLED if the process was terminated by a signal
> + * POPEN_UNKNOWN an error's occurred
> + */
> +int
> +coio_popen_get_status(void *fh, int *signal_id, int *exit_code);
> +
> +/**
> + * Handle SIGCHLD signal
> + */
> +void
> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *);

Better not use siginfo_t here - instead pass what you really need.
This makes the API clearer and cuts the dependency list.

> diff --git a/src/lua/fio.c b/src/lua/fio.c
> +/**
> + * A wrapper applicable for using with lua's GC.
> + * */
> +struct fio_popen_wrap {

Why do you need a wrapper at all?

> +	void* handle;
> +};
> +static const char* fio_popen_typename = "fio.popen.data";
> +
> +static struct fio_popen_wrap *
> +fio_popen_get_handle_wrap(lua_State *L, int index)
> +{
> +	luaL_checktype(L, index, LUA_TUSERDATA);
> +	struct fio_popen_wrap *wrap = (struct fio_popen_wrap *)
> +		luaL_checkudata(L, index, fio_popen_typename);

Hmm, why do you use udata? Why not plain cdata? I keep forgetting
the difference, to tell the truth.

> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> +popen_methods.do_read = function(self, buf, size, seconds)
> +    if size == nil or type(size) ~= 'number' then
> +        error('fio.popen.read: invalid size argument')
> +    end
> +    if seconds == nil or type(seconds) ~= 'number' then
> +        error('fio.popen.read: invalid seconds argument')
> +    end
> +
> +    local tmpbuf
> +    if not ffi.istype(const_char_ptr_t, buf) then
> +        tmpbuf = buffer.ibuf()
> +        buf = tmpbuf:reserve(size)
> +    end
> +
> +    local res, output_no, err = internal.popen_read(self.fh, buf, size, seconds)
> +    if res == nil then
> +        if tmpbuf ~= nil then
> +            tmpbuf:recycle()
> +        end
> +        return nil, nil, err
> +    end
> +
> +    if tmpbuf ~= nil then
> +        tmpbuf:alloc(res)
> +        res = ffi.string(tmpbuf.rpos, tmpbuf:size())
> +        tmpbuf:recycle()
> +    end
> +    return res, output_no
> +end
> +
> +-- read stdout & stderr of the process started by fio.popen
> +-- read() -> str, source
> +-- read(buf) -> len, source
> +-- read(size) -> str, source
> +-- read(buf, size) -> len, source
> +-- source contains id of the stream, fio.STDOUT or fio.STDERR
> +popen_methods.read = function(self, buf, size)
> +    if self.fh == nil then
> +        return nil, nil, 'Invalid object'
> +    end
> +
> +    if buf == nil and size == nil then
> +        -- read()
> +        size = 512

Why 512? Aren't we supposed to read everything till EOF in this case?

> +    elseif ffi.istype(const_char_ptr_t, buf) and size == nil then
> +        -- read(buf)
> +        size = 512
> +    elseif not ffi.istype(const_char_ptr_t, buf) and
> +            buf ~= nil and size == nil then
> +        -- read(size)
> +        size = tonumber(buf)
> +        buf = nil
> +    elseif ffi.istype(const_char_ptr_t, buf) and size ~= nil then
> +        -- read(buf, size)
> +        size = tonumber(size)
> +    else
> +        error("fio.popen.read: invalid arguments")
> +    end
> +
> +    return self:do_read(buf, size, -1)
> +end

> +popen_methods.wait = function(self, timeout)
> +    if self.fh == nil then
> +        return nil, 'Invalid object'
> +    end
> +
> +    if timeout == nil then
> +        timeout = -1
> +    else
> +        timeout = tonumber(timeout)
> +    end
> +
> +    local rc, err = internal.popen_wait(self.fh, timeout)
> +    if rc ~= nil then
> +        self.exit_code = tonumber(rc)
> +        self.fh = nil

This is kinda unexpected. What if we want to read the process output
after waiting for it to complete.

Come to think of it, we might need an explicit knob to destroy an object
even before it's collected. May be, consider re-adding close() for this?
Please consult with Alexander and/or Georgy re this.

> +        return rc
> +    else
> +        return nil,err
> +    end
> +end

> diff --git a/test/box-tap/fio_popen.test.lua b/test/box-tap/fio_popen.test.lua
> new file mode 100755
> index 000000000..0c2bd7e70
> --- /dev/null
> +++ b/test/box-tap/fio_popen.test.lua
> @@ -0,0 +1,189 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local fio = require('fio')
> +local errno = require('errno')
> +local fiber = require('fiber')
> +local test = tap.test()
> +
> +test:plan(5+9+11+9+8)
> +
> +-- Preliminaries
> +local function read_stdout(app)
> +    local ss = ""
> +
> +    local s,src,err = app:read(128)
> +
> +    while s ~= nil and s ~= "" do
> +        ss = ss .. s
> +
> +        s,src,err = app:read(128)
> +    end
> +
> +    return ss
> +end
> +
> +-- Test 1. Run application, check its status, kill and wait
> +local app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> +                        stdout=fio.STDOUT,
> +                        stderr=fio.STDOUT})
> +test:isnt(app1, nil, "#1. Starting a existing application")
> +
> +local rc = app1:get_status()
> +test:is(rc, nil, "#1. Process is running")
> +
> +rc,err = app1:kill()
> +test:is(rc, true, "#1. Sending kill(15)")
> +
> +rc,err = app1:wait(5)
> +test:is(rc, -15, "#1. Process was killed")
> +
> +rc = app1:get_status()
> +test:is(rc, -15, "#1. Process was killed 2")
> +
> +app1 = nil
> +
> +-- Test 2. Run application, write to stdin, read from stdout
> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> +                  stdout=fio.PIPE,
> +                  stdin=fio.PIPE,
> +                  stderr=fio.STDOUT})
> +test:isnt(app1, nil, "#2. Starting a existing application")
> +
> +rc = app1:get_status()
> +test:is(rc, nil, "#2. Process is running")
> +
> +local test2str = '123\n456\n789'
> +
> +app1:write(test2str)
> +rc,src,err = app1:read(256)

Please also test the case when the child writes both to
stdin and stdout (looks like you missed it).

> +
> +test:is(src, fio.STDOUT, "#2. Read from STDOUT")
> +test:is(rc, test2str, "#2. Received exact string")
> +
> +test2str = 'abc\ndef\ngh'
> +
> +app1:write(test2str, 4)
> +local rc2,src2,err = app1:read(6)
> +
> +test:is(err, nil, "#2. Reading ok")
> +test:is(src2, fio.STDOUT, "#2. Read from STDOUT 2")
> +test:is(rc2, 'abc\n', "#2. Received exact string 2")
> +
> +rc,err = app1:kill()
> +test:is(rc, true, "#2. Sending kill(15)")
> +
> +rc,err = app1:wait()
> +test:is(rc, -15, "#2. Process was killed")
> +
> +app1 = nil
> +
> +-- Test 3. read from stdout with timeout
> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> +                  stdout=fio.PIPE,
> +                  stdin=fio.PIPE,
> +                  stderr=fio.STDOUT})
> +test:isnt(app1, nil, "#3. Starting a existing application")
> +test:is(app1:get_stderr(), nil, "#3. STDERR is redirected")
> +test:isnt(app1:get_stdout(), nil, "#3. STDOUT is available")
> +test:isnt(app1:get_stdin(), nil, "#3. STDIN is available")
> +
> +
> +rc = app1:get_status()
> +test:is(rc, nil, "#3. Process is running")
> +
> +rc,src,err = app1:read2(256, 2)
> +
> +local e = errno()
> +test:is(e, errno.ETIMEDOUT, "#3. Timeout")
> +
> +
> +local test2str = '123\n456\n789'
> +
> +app1:write(test2str)
> +rc,src,err = app1:read2(256, 2)
> +
> +test:is(err, nil, "#3. Reading ok")
> +test:is(src, fio.STDOUT, "#3. Read from STDOUT")
> +test:is(rc, test2str, "#3. Received exact string")
> +
> +rc,err = app1:kill('SIGHUP')
> +test:is(rc, true, "#3. Sending kill(1)")
> +
> +rc,err = app1:wait()
> +test:is(rc, -1, "#3. Process was killed")
> +
> +app1 = nil
> +
> +-- Test 4. Redirect from file
> +local build_path = os.getenv("BUILDDIR")
> +local txt_filename = fio.pathjoin(build_path, 'test/box-tap/fio_popen.sample.txt')
> +
> +local txt_file = fio.open(txt_filename, {'O_RDONLY'})
> +test:isnt(txt_file, nil, "#4. Open existing file for reading")
> +
> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> +                  stdout=fio.PIPE,
> +                  stdin=txt_file,
> +                  stderr=fio.STDOUT})
> +test:isnt(app1, nil, "#4. Starting a existing application")
> +test:is(app1:get_stderr(), nil, "#4. STDERR is redirected")
> +test:isnt(app1:get_stdout(), nil, "#4. STDOUT is available")
> +test:is(app1:get_stdin(), nil, "#4. STDIN is redirected")
> +
> +rc = app1:get_status()
> +test:is(rc, nil, "#4. Process is running")
> +
> +rc,src,err = app1:read2(256, 2)
> +
> +test:is(src, fio.STDOUT, "#4. Read from STDOUT")
> +
> +local test2str = 'AAA\nBBB\nCCC\nDDD\n\n'
> +
> +test:is(rc, test2str, "#4. Received exact string")
> +
> +rc,err = app1:wait()
> +test:is(rc, 0, "#4. Process's exited")
> +
> +app1 = nil
> +txt_file:close()
> +
> +-- Test 5. Redirect output from one process to another
> +local app_path = fio.pathjoin(build_path, 'test/box-tap/fio_popen_test1.sh')
> +
> +app1 = fio.popen({argv = {'/bin/sh', '-c', app_path},
> +                  stdout=fio.PIPE,
> +                  stderr=fio.STDOUT})
> +
> +test:isnt(app1, nil, "#5. Starting application 1")
> +test:is(app1:get_stderr(), nil, "#5. STDERR is redirected")
> +test:isnt(app1:get_stdout(), nil, "#5. STDOUT is available")
> +test:isnt(app1:get_stdin(), nil, "#5. STDIN is available")
> +
> +fiber.sleep(1)
> +
> +local app2 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
> +                  stdout=fio.PIPE,
> +                  stdin=app1:get_stdout()})
> +
> +test:isnt(app2, nil, "#5. Starting application 2")
> +
> +local test2str = '1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n'
> +
> +rc = read_stdout(app2)

Please also test reading/writing/killing in case when the child is dead.

> +
> +test:is(rc, test2str, "#5. Received exact string")
> +
> +rc,err = app1:wait()
> +test:is(rc, 0, "#5. Process's exited 1")
> +
> +rc,err = app2:wait()
> +test:is(rc, 0, "#5. Process's exited 2")
> +
> +app1 = nil
> +app2 = nil
> +
> +-- --------------------------------------------------------------
> +test:check()
> +os.exit(0)
> +
> diff --git a/test/box-tap/fio_popen_test1.sh b/test/box-tap/fio_popen_test1.sh
> new file mode 100755
> index 000000000..da54ee19a
> --- /dev/null
> +++ b/test/box-tap/fio_popen_test1.sh
> @@ -0,0 +1,7 @@
> +#!/bin/bash
> +for i in {1..10}
> +do
> +	echo $i
> +#	sleep 1
> +done
> +

[tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/31 16:01]:
> > +static struct popen_data *
> > +popen_lookup_data_by_pid(pid_t pid)
> > +{
> > +	struct popen_data *cur = popen_data_list;
> > +	for(; cur; cur = cur->next) {
> > +		if (cur->pid == pid)
> > +			return cur;
> > +	}
> 
> Please write a comment explaining why you think linear search is fine
> here and why we don't use some kind of hash or tree.

If it is a list of all popen processes, please avoid.
We can't afford adding a yet another, even unlikely, reason for a
100% hog. This makes support difficult - keeping them all one's
head and checking neither is triggered.

-- 
Konstantin Osipov, Moscow, Russia

Re: [tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Vladimir Davydov]

On Fri, May 31, 2019 at 08:32:44PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/31 16:01]:
> > > +static struct popen_data *
> > > +popen_lookup_data_by_pid(pid_t pid)
> > > +{
> > > +	struct popen_data *cur = popen_data_list;
> > > +	for(; cur; cur = cur->next) {
> > > +		if (cur->pid == pid)
> > > +			return cur;
> > > +	}
> > 
> > Please write a comment explaining why you think linear search is fine
> > here and why we don't use some kind of hash or tree.
> 
> If it is a list of all popen processes, please avoid.
> We can't afford adding a yet another, even unlikely, reason for a
> 100% hog. This makes support difficult - keeping them all one's
> head and checking neither is triggered.

OK, makes sense. We should probably use mhash then. It shouldn't be
difficult to do.

[tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen

[Stanislav Zudin]

On 31.05.2019 20:32, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/31 16:01]:
>>> +static struct popen_data *
>>> +popen_lookup_data_by_pid(pid_t pid)
>>> +{
>>> +	struct popen_data *cur = popen_data_list;
>>> +	for(; cur; cur = cur->next) {
>>> +		if (cur->pid == pid)
>>> +			return cur;
>>> +	}
>>
>> Please write a comment explaining why you think linear search is fine
>> here and why we don't use some kind of hash or tree.
> 
> If it is a list of all popen processes, please avoid.
> We can't afford adding a yet another, even unlikely, reason for a
> 100% hog. This makes support difficult - keeping them all one's
> head and checking neither is triggered.
> 

How many simultaneously running popen processes do you expect? I believe
it's tens, not even hundreds.
In this case the hash won't give any noticeable performance improvement
in comparison with the list.
The newly created process is being put in the head of the list.
It's O(1).
The O(N) occurred only in the SIGCHLD handler and in the finalizer.