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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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;