[Tarantool-patches] [PATCH 0/2] Popen Lua module

[Tarantool-patches] [PATCH 0/2] Popen Lua module

[Alexander Turenko]

The patchset implements popen Lua module upward existing backend popen
engine.

The first patch changes popen_delete() behaviour to always free
resources even when killing a process (or a process group) fails. We
(Alexander, Cyrill and Igor) revisited the contract for closing the
handle after observing killpg() behaviour on a group of zombie processes
on Mac OS. I added those details to API documentation comments to don't
surprise a user with this.

The first patch continues previous preliminary popen engine series:

https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015609.html
https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015888.html

The second patch implements the Lua API for popen. It is quite
strightforward and should be considered as very basic implementation. We
plan to enhance it further in the upcoming releases (issues are to be
filed).

The main goal of the module it to provide popen implementation that is
integrated into our event loop (similar to fio and socket modules). Side
goal is to provide ability to feed data to a child process input and
read data from its output and so work with streaming programs (like
`grep`) and even interactive programs (like `python -i` interpreter).

Let's consider the API of module as beta: it may be change in
backward-incompatible manner in future releases if it will be valuable
enough.

It seems the main application of the module it to write various testing
code and so the API should be extended in the future with convenient
shortcuts: developers usually don't very like writting tests and it
should be at least more-or-less convenient.

I plan to extend the API for reading with ability to read certain amount
of bytes, to read line-by-line (or based on other delimiter), to read
into a buffer (ibuf): like it is implemented in the socket module. I
plan to share an RFC document about read streams.

ph:wait() should gain ability to set a timeout and should be rewritten
to use triggers / fiber conds instead of check-yield-again loop.

----

https://github.com/tarantool/tarantool/issues/4031
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-13-full-ci

Changelog entry:

## Functionality added or changed
### Lua

* Added `popen` built-in module (gh-4031).

  The module provides popen implementation that is integrated with
  tarantool's event loop (like built-in `fio` and `socket` modules).

  It support bidirectional communication with a process: the module can
  feed input to a process and capture its output. This way it allows to
  run streaming programs (like `grep`) and even work interactively with
  outside REPL (say, `python -i`).

  A key feature of the implementation is that it uses vfork() under hood
  and so does not copy virtual memory tables. Copying of them may be
  quite time consuming: os.execute() takes ~2.5 seconds when 80 GiB is
  allocated for memtx. Moreover, when memory overcommit is disabled
  (which is default) it would be not possible to fork a process when
  more then half of available physical memory is mapped to tarantool's
  process.

  The API should be considered as beta: it is quite basic and will be
  extended with convenience features. On the other hand, it may be
  changed in a backward-incompatible manner in the future releases if it
  will be valuable enough.

Alexander Turenko (2):
  popen: always free resources in popen_delete()
  popen: add popen Lua module

 src/CMakeLists.txt          |    1 +
 src/lib/core/popen.c        |   88 +-
 src/lua/init.c              |    2 +
 src/lua/popen.c             | 2457 +++++++++++++++++++++++++++++++++++
 src/lua/popen.h             |   44 +
 test/app-tap/popen.test.lua |  605 +++++++++
 6 files changed, 3182 insertions(+), 15 deletions(-)
 create mode 100644 src/lua/popen.c
 create mode 100644 src/lua/popen.h
 create mode 100755 test/app-tap/popen.test.lua

-- 
2.25.0

[Tarantool-patches] [PATCH 1/2] popen: always free resources in popen_delete()

[Alexander Turenko]

The function still set a diagnostics when a signal sending fails and
returns -1, but it is purely informational result: for logging or so. It
reflects notes about dealing with failures in Linux's `man 2 close`:

 | Note, however, that a failure return should be used only for
 | diagnostic purposes <...> or remedial purposes <...>.
 |
 | <...> Linux  kernel always releases the file descriptor early in the
 | close operation, freeing it for reuse; the steps that may return an
 | error <...> occur only later in the close operation.
 |
 | Many other implementations similarly always close the file descriptor
 | <...> even if they subsequently report an error on return from
 | close(). POSIX.1 is currently silent on this point, but there are
 | plans to mandate this behavior in the next major release of the
 | standard.

When kill or killpg returns EPERM a caller usually unable to overcome it
somehow: retrying is not sufficient here. So there are no needs to keep
the handle: a caller refuses the handle and don't want to perform any
other operation on it.  The open engine do its best to kill a child
process or a process group, but when it is not possible, just set the a
diagnostic and free handle resources anyway.

Left comments about observed Mac OS behaviour regarding killing a
process group, where all processes are zombies (or just when a process
group leader is zombie, don't sure): it gives EPERM instead of ESRCH
from killpg(). This result should not surprise a user, so it should be
documented. See [1] for another description of the problem (I don't find
any official information about this).

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1329528

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

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 3a12a439b..da4b89757 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -688,11 +688,27 @@ popen_state_str(unsigned int state)
  * Possible errors:
  *
  * - SystemError: a process does not exists anymore
+ *
+ *                Aside of a non-exist process it is also
+ *                set for a zombie process or when all
+ *                processes in a group are zombies (but
+ *                see note re Mac OS below).
+ *
  * - SystemError: invalid signal number
+ *
  * - SystemError: no permission to send a signal to
  *                a process or a process group
  *
- * Set errno to ESRCH when a process does not exist.
+ *                It is set on Mac OS when a signal is sent
+ *                to a process group, where a group leader
+ *                is zombie (or when all processes in it
+ *                are zombies, don't sure).
+ *
+ *                Whether it may appear due to other
+ *                reasons is unclear.
+ *
+ * Set errno to ESRCH when a process does not exist or is
+ * zombie.
  */
 int
 popen_send_signal(struct popen_handle *handle, int signo)
@@ -732,39 +748,74 @@ popen_send_signal(struct popen_handle *handle, int signo)
 /**
  * Delete a popen handle.
  *
- * The function performs the following actions:
+ * Send SIGKILL and free the handle.
  *
- * - Kill a child process or a process group depending of
- *   POPEN_FLAG_GROUP_SIGNAL (if the process is alive and
- *   POPEN_FLAG_KEEP_CHILD flag is not set).
- * - Close all fds occupied by the handle.
- * - Remove the handle from a living list.
- * - Free all occupied memory.
+ * Details about signaling:
  *
- * @see popen_send_signal() for note about ..._GROUP_SIGNAL.
+ * - The signal is sent only when ...KEEP_CHILD is not set.
+ * - The signal is sent only when a process is alive according
+ *   to the information available on current even loop iteration.
+ *   (There is a gap here: a zombie may be signaled; it is
+ *   harmless.)
+ * - The signal is sent to a process or a grocess group depending
+ *   of ..._GROUP_SIGNAL flag. @see popen_send_signal() for note
+ *   about ..._GROUP_SIGNAL.
  *
- * Return 0 at success and -1 at failure (and set a diag).
+ * Resources are released disregarding of whether a signal
+ * sending succeeds: all fds occupied by the handle are closed,
+ * the handle is removed from a living list, all occupied memory
+ * is freed.
  *
- * Possible errors:
+ * The function may return 0 or -1 (and set a diag) as usual,
+ * but it always frees the handle resources. So any return
+ * value usually means success for a caller. The return
+ * value and diagnostics are purely informational: it is
+ * for logging or same kind of reporting.
+ *
+ * Possible diagnostics (don't consider them as errors):
  *
  * - SystemError: no permission to send a signal to
  *                a process or a process group
+ *
+ *                This error may appear due to Mac OS
+ *                behaviour on zombies when
+ *                ..._GROUP_SIGNAL is set,
+ *                @see popen_send_signal().
+ *
+ *                Whether it may appear due to other
+ *                reasons is unclear.
+ *
+ * Always return 0 when a process is known as dead: no signal
+ * will be send, so no 'failure' may appear.
  */
 int
 popen_delete(struct popen_handle *handle)
 {
+	/*
+	 * Save a result and a failure reason of the
+	 * popen_send_signal() call.
+	 */
+	int rc = 0;
+	struct diag *diag = diag_get();
+	struct error *e = NULL;
+
 	size_t i;
 
 	assert(handle != NULL);
 
 	if ((handle->flags & POPEN_FLAG_KEEP_CHILD) == 0) {
 		/*
-		 * Unable to kill the process -- give an error.
+		 * Unable to kill the process -- save the error
+		 * and pass over.
 		 * The process is not exist already -- pass over.
 		 */
 		if (popen_send_signal(handle, SIGKILL) != 0 &&
-		    errno != ESRCH)
-			return -1;
+		    errno != ESRCH) {
+			rc = -1;
+			e = diag_last_error(diag);
+			assert(e != NULL);
+			error_ref(e);
+		}
 	}
 
 	for (i = 0; i < lengthof(handle->ios); i++) {
@@ -787,7 +838,14 @@ popen_delete(struct popen_handle *handle)
 
 	rlist_del(&handle->list);
 	handle_free(handle);
-	return 0;
+
+	/* Restore an error from popen_send_signal() if any. */
+	if (rc != 0) {
+		diag_set_error(diag, e);
+		error_unref(e);
+	}
+
+	return rc;
 }
 
 /**
-- 
2.25.0

[Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Alexander Turenko]

Co-developed-by: Cyrill Gorcunov <gorcunov@gmail.com>
Co-developed-by: Igor Munkin <imun@tarantool.org>

Fixes #4031

@TarantoolBot document
Title: popen module

```
Overview
========

Tarantool supports execution of external programs similarly to well
known Python's `subprocess` or Ruby's `Open3`. Note though the `popen`
module does not match one to one to the helpers these languages provide
and provides only basic functions. The popen object creation is
implemented via `vfork()` system call which means the caller thread is
blocked until execution of a child process begins.

Module functions
================

The `popen` module provides two functions to create that named popen
object `popen.shell` which is the similar to libc `popen` syscall and
`popen.new` to create popen object with more specific options.

`popen.shell(command[, mode]) -> handle, err`
---------------------------------------------

Execute a shell command.

@param command  a command to run, mandatory
@param mode     communication mode, optional
                'w'    to use ph:write()
                'r'    to use ph:read()
                'R'    to use ph:read({stderr = true})
                nil    inherit parent's std* file descriptors

Several mode characters can be set together: 'rw', 'rRw', etc.

This function is just shortcut for popen.new({command}, opts)
with opts.{shell,setsid,group_signal} set to `true` and
and opts.{stdin,stdout,stderr} set based on `mode` parameter.

All std* streams are inherited from parent by default if it is
not changed using mode: 'r' for stdout, 'R' for stderr, 'w' for
stdin.

Raise an error on incorrect parameters:

- IllegalParams: incorrect type or value of a parameter.

Return a popen handle on success.

Return `nil, err` on a failure.
@see popen.new() for possible reasons.

Example:

 | local popen = require('popen')
 |
 | local ph = popen.shell('date', 'r')
 | local date = ph:read():rstrip()
 | ph:close()
 | print(date)

Execute 'sh -c date' command, read the output and close the
popen object.

`popen.new(argv[, opts]) -> handle, err`
----------------------------------------

Execute a child program in a new process.

@param argv  an array of a program to run with
             command line options, mandatory;
             absolute path to the program is required

@param opts  table of options

@param opts.stdin   action on STDIN_FILENO
@param opts.stdout  action on STDOUT_FILENO
@param opts.stderr  action on STDERR_FILENO

File descriptor actions:

    popen.opts.INHERIT  (== 'inherit') [default]
                        inherit the fd from the parent
    popen.opts.DEVNULL  (== 'devnull')
                        open /dev/null on the fd
    popen.opts.CLOSE    (== 'close')
                        close the fd
    popen.opts.PIPE     (== 'pipe')
                        feed data from/to the fd to parent
                        using a pipe

@param opts.env  an array of environment variables to
                 be used inside a process;
                 - when is not set then the current
                   environment is inherited;
                 - if set then the environment will be
                   replaced
                 - if set to an empty array then the
                   environment will be dropped

@param opts.shell            (boolean, default: false)
       true                  run a child process via
                             'sh -c "${opts.argv}"'
       false                 call the executable directly

@param opts.setsid           (boolean, default: false)
       true                  start executable inside a new
                             session
       false                 don't do that

@param opts.close_fds        (boolean, default: true)
       true                  close all inherited fds from a
                             parent
       false                 don't do that

@param opts.restore_signals  (boolean, default: true)
       true                  reset all signal actions
                             modified in parent's process
       false                 inherit changed actions

@param opts.group_signal     (boolean, default: false)
       true                  send signal to a child process
                             group (only when opts.setsid is
                             enabled)
       false                 send signal to a child process
                             only

@param opts.keep_child       (boolean, default: false)
       true                  don't send SIGKILL to a child
                             process at freeing (by :close()
                             or Lua GC)
       false                 send SIGKILL to a child process
                             (or a process group if
                             opts.group_signal is enabled) at
                             :close() or collecting of the
                             handle by Lua GC

The returned handle is subject to garbage collection, but
it may be freed explicitly using :close(). SIGKILL is sent
to the child process at :close() / GC if it is alive and
opts.keep_child is not set.

It is recommended to use opts.setsid + opts.group_signal
if a child process may spawn its own childs and they all
should be killed together.

Note: A signal will not be sent if the child process is
already dead: otherwise we might kill another process that
occupies the same PID later. This means that if the child
process dies before its own childs, the function will not
send a signal to the process group even when opts.setsid and
opts.group_signal are set.

Use os.environ() to pass copy of current environment with
several replacements (see example 2 below).

Raise an error on incorrect parameters:

- IllegalParams: incorrect type or value of a parameter.
- IllegalParams: group signal is set, while setsid is not.

Return a popen handle on success.

Return `nil, err` on a failure. Possible reasons:

- SystemError: dup(), fcntl(), pipe(), vfork() or close()
               fails in the parent process.
- SystemError: (temporary restriction) the parent process
               has closed stdin, stdout or stderr.
- OutOfMemory: unable to allocate the handle or a temporary
               buffer.

Example 1:

 | local popen = require('popen')
 |
 | local ph = popen.new({'/bin/date'}, {
 |     stdout = popen.opts.PIPE,
 | })
 | local date = ph:read():rstrip()
 | ph:close()
 | print(date) -- Thu 16 Apr 2020 01:40:56 AM MSK

Execute 'date' command, read the result and close the
popen object.

Example 2:

 | local popen = require('popen')
 |
 | local env = os.environ()
 | env['FOO'] = 'bar'
 |
 | local ph = popen.new({'echo "${FOO}"'}, {
 |     stdout = popen.opts.PIPE,
 |     shell = true,
 |     env = env,
 | })
 | local res = ph:read():rstrip()
 | ph:close()
 | print(res) -- bar

It is quite similar to the previous one, but sets the
environment variable and uses shell builtin 'echo' to
show it.

Example 3:

 | local popen = require('popen')
 |
 | local ph = popen.new({'echo hello >&2'}, { -- !!
 |     stderr = popen.opts.PIPE,              -- !!
 |     shell = true,
 | })
 | local res = ph:read({stderr = true}):rstrip()
 | ph:close()
 | print(res) -- hello

This example demonstrates how to capture child's stderr.

Example 4:

 | local function call_jq(stdin, filter)
 |     -- Start jq process, connect to stdin, stdout and stderr.
 |     local jq_argv = {'/usr/bin/jq', '-M', '--unbuffered', filter}
 |     local ph, err = popen.new(jq_argv, {
 |         stdin = popen.opts.PIPE,
 |         stdout = popen.opts.PIPE,
 |         stderr = popen.opts.PIPE,
 |     })
 |     if ph == nil then return nil, err end
 |
 |     -- Write input data to child's stdin and send EOF.
 |     local ok, err = ph:write(stdin)
 |     if not ok then return nil, err end
 |     ph:shutdown({stdin = true})
 |
 |     -- Read everything until EOF.
 |     local chunks = {}
 |     while true do
 |         local chunk, err = ph:read()
 |         if chunk == nil then
 |             ph:close()
 |             return nil, err
 |         end
 |         if chunk == '' then break end -- EOF
 |         table.insert(chunks, chunk)
 |     end
 |
 |     -- Read diagnostics from stderr if any.
 |     local err = ph:read({stderr = true})
 |     if err ~= '' then
 |         ph:close()
 |         return nil, err
 |     end
 |
 |     -- Glue all chunks, strip trailing newline.
 |     return table.concat(chunks):rstrip()
 | end

Demonstrates how to run a stream program (like `grep`, `sed`
and so), write to its stdin and read from its stdout.

The example assumes that input data are small enough to fit
a pipe buffer (typically 64 KiB, but depends on a platform
and its configuration). It will stuck in :write() for large
data. How to handle this case: call :read() in a loop in
another fiber (start it before a first :write()).

If a process writes large text to stderr, it may fill out
stderr pipe buffer and block in write(2, ...). So we need to
read stderr too in another fiber to handle this case.

Handle methods
==============

`popen_handle:read([opts]) -> str, err`
---------------------------------------

Read data from a child peer.

@param handle        handle of a child process
@param opts          an options table
@param opts.stdout   whether to read from stdout, boolean
                     (default: true)
@param opts.stderr   whether to read from stderr, boolean
                     (default: false)
@param opts.timeout  time quota in seconds
                     (default: 100 years)

Read data from stdout or stderr streams with @a timeout.
By default it reads from stdout. Set @a opts.stderr to
`true` to read from stderr.

Raise an error on incorrect parameters or when the fiber is
cancelled:

- IllegalParams:    incorrect type or value of a parameter.
- IllegalParams:    called on a closed handle.
- IllegalParams:    opts.stdout and opts.stderr are set both
- IllegalParams:    a requested IO operation is not supported
                    by the handle (stdout / stderr is not
                    piped).
- IllegalParams:    attempt to operate on a closed file
                    descriptor.
- FiberIsCancelled: cancelled by an outside code.

Return a string on success, an empty string at EOF.

Return `nil, err` on a failure. Possible reasons:

- SocketError: an IO error occurs at read().
- TimedOut:    @a timeout quota is exceeded.
- OutOfMemory: no memory space for a buffer to read into.
- LuajitError: ("not enough memory"): no memory space for
               the Lua string.

`popen_handle:write(str[, opts]) -> str, err`
---------------------------------------------

Write data to a child peer.

@param handle        a handle of a child process
@param str           a string to write
@param opts          table of options
@param opts.timeout  time quota in seconds
                     (default: 100 years)

Write string @a str to stdin stream of a child process.

The function may yield forever if a child process does
not read data from stdin and a pipe buffer becomes full.
Size of this buffer depends on a platform. Use
@a opts.timeout when unsure.

When @a opts.timeout is not set, the function blocks
(yields the fiber) until all data is written or an error
happened.

Raise an error on incorrect parameters or when the fiber is
cancelled:

- IllegalParams:    incorrect type or value of a parameter.
- IllegalParams:    called on a closed handle.
- IllegalParams:    string length is greater then SSIZE_MAX.
- IllegalParams:    a requested IO operation is not supported
                    by the handle (stdin is not piped).
- IllegalParams:    attempt to operate on a closed file
                    descriptor.
- FiberIsCancelled: cancelled by an outside code.

Return `true` on success.

Return `nil, err` on a failure. Possible reasons:

- SocketError: an IO error occurs at write().
- TimedOut:    @a timeout quota is exceeded.

`popen_handle:shutdown(opts) -> true`
------------------------------------------

Close parent's ends of std* fds.

@param handle        handle of a child process
@param opts          an options table
@param opts.stdin    close parent's end of stdin, boolean
@param opts.stdout   close parent's end of stdout, boolean
@param opts.stderr   close parent's end of stderr, boolean

The main reason to use this function is to send EOF to
child's stdin. However parent's end of stdout / stderr
may be closed too.

The function does not fail on already closed fds (idempotence).
However it fails on attempt to close the end of a pipe that was
never exist. In other words, only those std* options that
were set to popen.opts.PIPE at a handle creation may be used
here (for popen.shell: 'r' corresponds to stdout, 'R' to stderr
and 'w' to stdin).

The function does not close any fds on a failure: either all
requested fds are closed or neither of them.

Example:

 | local popen = require('popen')
 |
 | local ph = popen.shell('sed s/foo/bar/', 'rw')
 | ph:write('lorem foo ipsum')
 | ph:shutdown({stdin = true})
 | local res = ph:read()
 | ph:close()
 | print(res) -- lorem bar ipsum

Raise an error on incorrect parameters:

- IllegalParams:  an incorrect handle parameter.
- IllegalParams:  called on a closed handle.
- IllegalParams:  neither stdin, stdout nor stderr is choosen.
- IllegalParams:  a requested IO operation is not supported
                  by the handle (one of std* is not piped).

Return `true` on success.

`popen_handle:terminate() -> ok, err`
-------------------------------------

Send SIGTERM signal to a child process.

@param handle  a handle carries child process to terminate

The function only sends SIGTERM signal and does NOT
free any resources (popen handle memory and file
descriptors).

@see popen_handle:signal() for errors and return values.

`popen_handle:kill() -> ok, err`
--------------------------------

Send SIGKILL signal to a child process.

@param handle  a handle carries child process to kill

The function only sends SIGKILL signal and does NOT
free any resources (popen handle memory and file
descriptors).

@see popen_handle:signal() for errors and return values.

`popen_handle:signal(signo) -> ok, err`
---------------------------------------

Send signal to a child process.

@param handle  a handle carries child process to be signaled
@param signo   signal number to send

When opts.setsid and opts.group_signal are set on the handle
the signal is sent to the process group rather than to the
process. @see popen.new() for details about group
signaling.

Note: The module offers popen.signal.SIG* constants, because
some signals have different numbers on different platforms.

Raise an error on incorrect parameters:

- IllegalParams:    an incorrect handle parameter.
- IllegalParams:    called on a closed handle.

Return `true` if signal is sent.

Return `nil, err` on a failure. Possible reasons:

- SystemError: a process does not exists anymore

               Aside of a non-exist process it is also
               returned for a zombie process or when all
               processes in a group are zombies (but
               see note re Mac OS below).

- SystemError: invalid signal number

- SystemError: no permission to send a signal to
               a process or a process group

               It is returned on Mac OS when a signal is
               sent to a process group, where a group leader
               is zombie (or when all processes in it
               are zombies, don't sure).

               Whether it may appear due to other
               reasons is unclear.

`popen_handle:info() -> res, err`
---------------------------------

Return information about popen handle.

@param handle  a handle of a child process

Raise an error on incorrect parameters:

- IllegalParams: an incorrect handle parameter.
- IllegalParams: called on a closed handle.

Return information about the handle in the following
format:

    {
        pid = <number> or <nil>,
        command = <string>,
        opts = <table>,
        status = <table>,
        stdin = one-of(
            popen.stream.OPENED (== 'opened'),
            popen.stream.CLOSED (== 'closed'),
            nil,
        ),
        stdout = one-of(
            popen.stream.OPENED (== 'opened'),
            popen.stream.CLOSED (== 'closed'),
            nil,
        ),
        stderr = one-of(
            popen.stream.OPENED (== 'opened'),
            popen.stream.CLOSED (== 'closed'),
            nil,
        ),
    }

`pid` is a process id of the process when it is alive,
otherwise `pid` is nil.

`command` is a concatenation of space separated arguments
that were passed to execve(). Multiword arguments are quoted.
Quotes inside arguments are not escaped.

`opts` is a table of handle options in the format of
popen.new() `opts` parameter. `opts.env` is not shown here,
because the environment variables map is not stored in a
handle.

`status` is a table that represents a process status in the
following format:

    {
        state = one-of(
            popen.state.ALIVE    (== 'alive'),
            popen.state.EXITED   (== 'exited'),
            popen.state.SIGNALED (== 'signaled'),
        )

        -- Present when `state` is 'exited'.
        exit_code = <number>,

        -- Present when `state` is 'signaled'.
        signo = <number>,
        signame = <string>,
    }

`stdin`, `stdout`, `stderr` reflect status of parent's end
of a piped stream. When a stream is not piped the field is
not present (`nil`). When it is piped, the status may be
one of the following:

- popen.stream.OPENED  (== 'opened')
- popen.stream.CLOSED  (== 'closed')

The status may be changed from 'opened' to 'closed'
by :shutdown({std... = true}) call.

Example 1 (tarantool console):

 | tarantool> require('popen').new({'/usr/bin/touch', '/tmp/foo'})
 | ---
 | - command: /usr/bin/touch /tmp/foo
 |   status:
 |     state: alive
 |   opts:
 |     stdout: inherit
 |     stdin: inherit
 |     group_signal: false
 |     keep_child: false
 |     close_fds: true
 |     restore_signals: true
 |     shell: false
 |     setsid: false
 |     stderr: inherit
 |   pid: 9499
 | ...

Example 2 (tarantool console):

 | tarantool> require('popen').shell('grep foo', 'wrR')
 | ---
 | - stdout: opened
 |   command: sh -c 'grep foo'
 |   stderr: opened
 |   status:
 |     state: alive
 |   stdin: opened
 |   opts:
 |     stdout: pipe
 |     stdin: pipe
 |     group_signal: true
 |     keep_child: false
 |     close_fds: true
 |     restore_signals: true
 |     shell: true
 |     setsid: true
 |     stderr: pipe
 |   pid: 10497
 | ...

`popen_handle:wait() -> res, err`
---------------------------------

Wait until a child process get exited or signaled.

@param handle  a handle of process to wait

Raise an error on incorrect parameters or when the fiber is
cancelled:

- IllegalParams:    an incorrect handle parameter.
- IllegalParams:    called on a closed handle.
- FiberIsCancelled: cancelled by an outside code.

Return a process status table (the same as ph.status and
ph.info().status). @see popen_handle:info() for the format
of the table.

`popen_handle:close() -> ok, err`
---------------------------------

Close a popen handle.

@param handle  a handle to close

Basically it kills a process using SIGKILL and releases all
resources assosiated with the popen handle.

Details about signaling:

- The signal is sent only when opts.keep_child is not set.
- The signal is sent only when a process is alive according
  to the information available on current even loop iteration.
  (There is a gap here: a zombie may be signaled; it is
  harmless.)
- The signal is sent to a process or a grocess group depending
  of opts.group_signal. (@see lbox_popen_new() for details of
  group signaling).

Resources are released disregarding of whether a signal
sending succeeds: fds are closed, memory is released,
the handle is marked as closed.

No operation is possible on a closed handle except
:close(), which always successful on closed handle
(idempotence).

Raise an error on incorrect parameters:

- IllegalParams: an incorrect handle parameter.

The function may return `true` or `nil, err`, but it always
frees the handle resources. So any return value usually
means success for a caller. The return values are purely
informational: it is for logging or same kind of reporting.

Possible diagnostics (don't consider them as errors):

- SystemError: no permission to send a signal to
               a process or a process group

               This diagnostics may appear due to
               Mac OS behaviour on zombies when
               opts.group_signal is set,
               @see lbox_popen_signal().

               Whether it may appear due to other
               reasons is unclear.

Always return `true` when a process is known as dead (say,
after ph:wait()): no signal will be send, so no 'failure'
may appear.

Handle fields
=============

- popen_handle.pid
- popen_handle.command
- popen_handle.opts
- popen_handle.status
- popen_handle.stdin
- popen_handle.stdout
- popen_handle.stderr

See popen_handle:info() for description of those fields.

Module constants
================

- popen.opts
  - INHERIT (== 'inherit')
  - DEVNULL (== 'devnull')
  - CLOSE   (== 'close')
  - PIPE    (== 'pipe')

- popen.signal
  - SIGTERM (== 9)
  - SIGKILL (== 15)
  - ...

- popen.state
  - ALIVE    (== 'alive')
  - EXITED   (== 'exited')
  - SIGNALED (== 'signaled')

- popen.stream
  - OPENED  (== 'opened')
  - CLOSED  (== 'closed')
```
---
 src/CMakeLists.txt          |    1 +
 src/lua/init.c              |    2 +
 src/lua/popen.c             | 2457 +++++++++++++++++++++++++++++++++++
 src/lua/popen.h             |   44 +
 test/app-tap/popen.test.lua |  605 +++++++++
 5 files changed, 3109 insertions(+)
 create mode 100644 src/lua/popen.c
 create mode 100644 src/lua/popen.h
 create mode 100755 test/app-tap/popen.test.lua

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index de9680bcc..2b5ea1b87 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -114,6 +114,7 @@ set (server_sources
      lua/socket.c
      lua/pickle.c
      lua/fio.c
+     lua/popen.c
      lua/httpc.c
      lua/utf8.c
      lua/info.c
diff --git a/src/lua/init.c b/src/lua/init.c
index 28b6b2d62..5b4b5b463 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/popen.h"
 #include "lua/httpc.h"
 #include "lua/utf8.h"
 #include "lua/swim.h"
@@ -455,6 +456,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_popen_init(L);
 	tarantool_lua_socket_init(L);
 	tarantool_lua_pickle_init(L);
 	tarantool_lua_digest_init(L);
diff --git a/src/lua/popen.c b/src/lua/popen.c
new file mode 100644
index 000000000..40f4343eb
--- /dev/null
+++ b/src/lua/popen.c
@@ -0,0 +1,2457 @@
+/*
+ * Copyright 2010-2020, 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 <sys/types.h>
+
+#include <lua.h>
+#include <lauxlib.h>
+#include <lualib.h>
+
+#include <small/region.h>
+
+#include "diag.h"
+#include "core/popen.h"
+#include "core/fiber.h"
+#include "core/exception.h"
+#include "tarantool_ev.h"
+
+#include "lua/utils.h"
+#include "lua/fiber.h"
+#include "lua/popen.h"
+
+/* {{{ Constants */
+
+static const char *popen_handle_uname = "popen_handle";
+static const char *popen_handle_closed_uname = "popen_handle_closed";
+
+#define POPEN_LUA_READ_BUF_SIZE        4096
+#define POPEN_LUA_WAIT_DELAY           0.1
+#define POPEN_LUA_ENV_CAPACITY_DEFAULT 256
+
+/**
+ * Helper map for transformation between std* popen.new() options
+ * and popen backend engine flags.
+ */
+static const struct {
+	/* Name for error messages. */
+	const char *option_name;
+
+	unsigned int mask_devnull;
+	unsigned int mask_close;
+	unsigned int mask_pipe;
+} pfd_map[POPEN_FLAG_FD_STDEND_BIT] = {
+	{
+		.option_name	= "opts.stdin",
+		.mask_devnull	= POPEN_FLAG_FD_STDIN_DEVNULL,
+		.mask_close	= POPEN_FLAG_FD_STDIN_CLOSE,
+		.mask_pipe	= POPEN_FLAG_FD_STDIN,
+	}, {
+		.option_name	= "opts.stdout",
+		.mask_devnull	= POPEN_FLAG_FD_STDOUT_DEVNULL,
+		.mask_close	= POPEN_FLAG_FD_STDOUT_CLOSE,
+		.mask_pipe	= POPEN_FLAG_FD_STDOUT,
+	}, {
+		.option_name	= "opts.stderr",
+		.mask_devnull	= POPEN_FLAG_FD_STDERR_DEVNULL,
+		.mask_close	= POPEN_FLAG_FD_STDERR_CLOSE,
+		.mask_pipe	= POPEN_FLAG_FD_STDERR,
+	},
+};
+
+/* }}} */
+
+/* {{{ Signals */
+
+static const struct {
+	const char *signame;
+	int signo;
+} popen_lua_signals[] = {
+#ifdef SIGHUP
+	{"SIGHUP", SIGHUP},
+#endif
+#ifdef SIGINT
+	{"SIGINT", SIGINT},
+#endif
+#ifdef SIGQUIT
+	{"SIGQUIT", SIGQUIT},
+#endif
+#ifdef SIGILL
+	{"SIGILL", SIGILL},
+#endif
+#ifdef SIGTRAP
+	{"SIGTRAP", SIGTRAP},
+#endif
+#ifdef SIGABRT
+	{"SIGABRT", SIGABRT},
+#endif
+#ifdef SIGIOT
+	{"SIGIOT", SIGIOT},
+#endif
+#ifdef SIGBUS
+	{"SIGBUS", SIGBUS},
+#endif
+#ifdef SIGFPE
+	{"SIGFPE", SIGFPE},
+#endif
+#ifdef SIGKILL
+	{"SIGKILL", SIGKILL},
+#endif
+#ifdef SIGUSR1
+	{"SIGUSR1", SIGUSR1},
+#endif
+#ifdef SIGSEGV
+	{"SIGSEGV", SIGSEGV},
+#endif
+#ifdef SIGUSR2
+	{"SIGUSR2", SIGUSR2},
+#endif
+#ifdef SIGPIPE
+	{"SIGPIPE", SIGPIPE},
+#endif
+#ifdef SIGALRM
+	{"SIGALRM", SIGALRM},
+#endif
+#ifdef SIGTERM
+	{"SIGTERM", SIGTERM},
+#endif
+#ifdef SIGSTKFLT
+	{"SIGSTKFLT", SIGSTKFLT},
+#endif
+#ifdef SIGCHLD
+	{"SIGCHLD", SIGCHLD},
+#endif
+#ifdef SIGCONT
+	{"SIGCONT", SIGCONT},
+#endif
+#ifdef SIGSTOP
+	{"SIGSTOP", SIGSTOP},
+#endif
+#ifdef SIGTSTP
+	{"SIGTSTP", SIGTSTP},
+#endif
+#ifdef SIGTTIN
+	{"SIGTTIN", SIGTTIN},
+#endif
+#ifdef SIGTTOU
+	{"SIGTTOU", SIGTTOU},
+#endif
+#ifdef SIGURG
+	{"SIGURG", SIGURG},
+#endif
+#ifdef SIGXCPU
+	{"SIGXCPU", SIGXCPU},
+#endif
+#ifdef SIGXFSZ
+	{"SIGXFSZ", SIGXFSZ},
+#endif
+#ifdef SIGVTALRM
+	{"SIGVTALRM", SIGVTALRM},
+#endif
+#ifdef SIGPROF
+	{"SIGPROF", SIGPROF},
+#endif
+#ifdef SIGWINCH
+	{"SIGWINCH", SIGWINCH},
+#endif
+#ifdef SIGIO
+	{"SIGIO", SIGIO},
+#endif
+#ifdef SIGPOLL
+	{"SIGPOLL", SIGPOLL},
+#endif
+#ifdef SIGPWR
+	{"SIGPWR", SIGPWR},
+#endif
+#ifdef SIGSYS
+	{"SIGSYS", SIGSYS},
+#endif
+	{NULL, 0},
+};
+
+/* }}} */
+
+/* {{{ Stream actions */
+
+#define POPEN_LUA_STREAM_INHERIT	"inherit"
+#define POPEN_LUA_STREAM_DEVNULL	"devnull"
+#define POPEN_LUA_STREAM_CLOSE		"close"
+#define POPEN_LUA_STREAM_PIPE		"pipe"
+
+static const struct {
+	const char *name;
+	const char *value;
+	bool devnull;
+	bool close;
+	bool pipe;
+} popen_lua_actions[] = {
+	{
+		.name		= "INHERIT",
+		.value		= POPEN_LUA_STREAM_INHERIT,
+		.devnull	= false,
+		.close		= false,
+		.pipe		= false,
+	},
+	{
+		.name		= "DEVNULL",
+		.value		= POPEN_LUA_STREAM_DEVNULL,
+		.devnull	= true,
+		.close		= false,
+		.pipe		= false,
+	},
+	{
+		.name		= "CLOSE",
+		.value		= POPEN_LUA_STREAM_CLOSE,
+		.devnull	= false,
+		.close		= true,
+		.pipe		= false,
+	},
+	{
+		.name		= "PIPE",
+		.value		= POPEN_LUA_STREAM_PIPE,
+		.devnull	= false,
+		.close		= false,
+		.pipe		= true,
+	},
+	{NULL, NULL, false, false, false},
+};
+
+/* }}} */
+
+/* {{{ Stream statuses */
+
+#define POPEN_LUA_STREAM_STATUS_OPENED "opened"
+#define POPEN_LUA_STREAM_STATUS_CLOSED "closed"
+
+static const struct {
+	const char *name;
+	const char *value;
+} popen_lua_stream_statuses[] = {
+	{"OPENED",	POPEN_LUA_STREAM_STATUS_OPENED},
+	{"CLOSED",	POPEN_LUA_STREAM_STATUS_CLOSED},
+	{NULL, NULL},
+};
+
+/* }}} */
+
+/* {{{ Process states */
+
+#define POPEN_LUA_STATE_ALIVE "alive"
+#define POPEN_LUA_STATE_EXITED "exited"
+#define POPEN_LUA_STATE_SIGNALED "signaled"
+
+static const struct {
+	const char *name;
+	const char *value;
+} popen_lua_states[] = {
+	{"ALIVE",	POPEN_LUA_STATE_ALIVE},
+	{"EXITED",	POPEN_LUA_STATE_EXITED},
+	{"SIGNALED",	POPEN_LUA_STATE_SIGNALED},
+	{NULL, NULL},
+};
+
+/* }}} */
+
+/* {{{ General-purpose Lua helpers */
+
+/**
+ * Extract a string from the Lua stack.
+ *
+ * Return (const char *) if so, otherwise return NULL.
+ *
+ * Unlike luaL_checkstring() it accepts only a string and does not
+ * accept a number.
+ *
+ * Unlike luaL_checkstring() it does not raise an error, but
+ * returns NULL when a type is not what is excepted.
+ */
+static const char *
+luaL_check_string_strict(struct lua_State *L, int idx, size_t *len_ptr)
+{
+	if (lua_type(L, idx) != LUA_TSTRING)
+		return NULL;
+
+	const char *res = lua_tolstring(L, idx, len_ptr);
+	assert(res != NULL);
+	return res;
+}
+
+/**
+ * Extract a timeout value from the Lua stack.
+ *
+ * Return -1.0 when error occurs.
+ */
+static ev_tstamp
+luaT_check_timeout(struct lua_State *L, int idx)
+{
+	if (lua_type(L, idx) == LUA_TNUMBER)
+		return lua_tonumber(L, idx);
+	/* FIXME: Support cdata<int64_t> and cdata<uint64_t>. */
+	return -1.0;
+}
+
+/**
+ * Helper for luaT_push_string_noxc().
+ */
+static int
+luaT_push_string_noxc_wrapper(struct lua_State *L)
+{
+	char *str = (char *)lua_topointer(L, 1);
+	size_t len = lua_tointeger(L, 2);
+	lua_pushlstring(L, str, len);
+	return 1;
+}
+
+/**
+ * Push a string to the Lua stack.
+ *
+ * Return 0 at success, -1 at failure and set a diag.
+ *
+ * Possible errors:
+ *
+ * - LuajitError ("not enough memory"): no memory space for the
+ *   Lua string.
+ */
+static int
+luaT_push_string_noxc(struct lua_State *L, char *str, size_t len)
+{
+	lua_pushcfunction(L, luaT_push_string_noxc_wrapper);
+	lua_pushlightuserdata(L, str);
+	lua_pushinteger(L, len);
+	return luaT_call(L, 2, 1);
+}
+
+/* }}} */
+
+/* {{{ Popen handle userdata manipulations */
+
+/**
+ * Extract popen handle from the Lua stack.
+ *
+ * Return NULL in case of unexpected type.
+ */
+static struct popen_handle *
+luaT_check_popen_handle(struct lua_State *L, int idx, bool *is_closed_ptr)
+{
+	struct popen_handle **handle_ptr =
+		luaL_testudata(L, idx, popen_handle_uname);
+	bool is_closed = false;
+
+	if (handle_ptr == NULL) {
+		handle_ptr = luaL_testudata(L, idx, popen_handle_closed_uname);
+		is_closed = true;
+	}
+
+	if (handle_ptr == NULL)
+		return NULL;
+	assert(*handle_ptr != NULL);
+
+	if (is_closed_ptr != NULL)
+		*is_closed_ptr = is_closed;
+	return *handle_ptr;
+}
+
+/**
+ * Push popen handle into the Lua stack.
+ *
+ * Return 1 -- amount of pushed values.
+ */
+static int
+luaT_push_popen_handle(struct lua_State *L, struct popen_handle *handle)
+{
+	*(struct popen_handle **)lua_newuserdata(L, sizeof(handle)) = handle;
+	luaL_getmetatable(L, popen_handle_uname);
+	lua_setmetatable(L, -2);
+	return 1;
+}
+
+/**
+ * Mark popen handle as closed.
+ *
+ * Does not perform any checks whether @a idx points
+ * to a popen handle.
+ *
+ * The closed state is needed primarily to protect a
+ * handle from double freeing.
+ */
+static void
+luaT_mark_popen_handle_closed(struct lua_State *L, int idx)
+{
+	luaL_getmetatable(L, popen_handle_closed_uname);
+	lua_setmetatable(L, idx);
+}
+
+/* }}} */
+
+/* {{{ Push popen handle info to the Lua stack */
+
+/**
+ * Convert ...FD_STD* flags to a popen.opts.<...> constant.
+ *
+ * If flags are invalid, push 'invalid' string.
+ *
+ * Push the result onto the Lua stack.
+ */
+static int
+luaT_push_popen_stdX_action(struct lua_State *L, int fd, unsigned int flags)
+{
+	for (size_t i = 0; popen_lua_actions[i].name != NULL; ++i) {
+		bool devnull	= (flags & pfd_map[fd].mask_devnull) != 0;
+		bool close	= (flags & pfd_map[fd].mask_close) != 0;
+		bool pipe	= (flags & pfd_map[fd].mask_pipe) != 0;
+
+		if (devnull == popen_lua_actions[i].devnull &&
+		    close   == popen_lua_actions[i].close &&
+		    pipe    == popen_lua_actions[i].pipe) {
+			lua_pushstring(L, popen_lua_actions[i].value);
+			return 1;
+		}
+	}
+
+	lua_pushliteral(L, "invalid");
+	return 1;
+}
+
+/**
+ * Push a piped stream status (opened or closed) to the Lua stack.
+ */
+static int
+luaT_push_popen_stdX_status(struct lua_State *L, struct popen_handle *handle,
+			    int idx)
+{
+	if ((handle->flags & pfd_map[idx].mask_pipe) == 0) {
+		/* Stream action: INHERIT, DEVNULL or CLOSE. */
+		lua_pushnil(L);
+		return 1;
+	}
+
+	/* Stream action: PIPE. */
+	if (handle->ios[idx].fd < 0)
+		lua_pushliteral(L, POPEN_LUA_STREAM_STATUS_CLOSED);
+	else
+		lua_pushliteral(L, POPEN_LUA_STREAM_STATUS_OPENED);
+
+	return 1;
+}
+
+/**
+ * Push popen options as a Lua table.
+ *
+ * Environment variables are not stored in a popen handle and so
+ * missed here.
+ */
+static int
+luaT_push_popen_opts(struct lua_State *L, unsigned int flags)
+{
+	lua_createtable(L, 0, 8);
+
+	luaT_push_popen_stdX_action(L, STDIN_FILENO, flags);
+	lua_setfield(L, -2, "stdin");
+
+	luaT_push_popen_stdX_action(L, STDOUT_FILENO, flags);
+	lua_setfield(L, -2, "stdout");
+
+	luaT_push_popen_stdX_action(L, STDERR_FILENO, flags);
+	lua_setfield(L, -2, "stderr");
+
+	/* env is skipped */
+
+	lua_pushboolean(L, (flags & POPEN_FLAG_SHELL) != 0);
+	lua_setfield(L, -2, "shell");
+
+	lua_pushboolean(L, (flags & POPEN_FLAG_SETSID) != 0);
+	lua_setfield(L, -2, "setsid");
+
+	lua_pushboolean(L, (flags & POPEN_FLAG_CLOSE_FDS) != 0);
+	lua_setfield(L, -2, "close_fds");
+
+	lua_pushboolean(L, (flags & POPEN_FLAG_RESTORE_SIGNALS) != 0);
+	lua_setfield(L, -2, "restore_signals");
+
+	lua_pushboolean(L, (flags & POPEN_FLAG_GROUP_SIGNAL) != 0);
+	lua_setfield(L, -2, "group_signal");
+
+	lua_pushboolean(L, (flags & POPEN_FLAG_KEEP_CHILD) != 0);
+	lua_setfield(L, -2, "keep_child");
+
+	return 1;
+}
+
+/**
+ * Push a process status to the Lua stack as a table.
+ *
+ * The format of the resulting table:
+ *
+ *     {
+ *         state = one-of(
+ *             popen.state.ALIVE    (== 'alive'),
+ *             popen.state.EXITED   (== 'exited'),
+ *             popen.state.SIGNALED (== 'signaled'),
+ *         )
+ *
+ *         -- Present when `state` is 'exited'.
+ *         exit_code = <number>,
+ *
+ *         -- Present when `state` is 'signaled'.
+ *         signo = <number>,
+ *         signame = <string>,
+ *     }
+ *
+ * @param state POPEN_STATE_{ALIVE,EXITED,SIGNALED}
+ *
+ * @param exit_code is exit code when the process is exited and a
+ * signal number when a process is signaled.
+ *
+ * @see enum popen_states
+ * @see popen_state()
+ */
+static int
+luaT_push_popen_process_status(struct lua_State *L, int state, int exit_code)
+{
+	lua_createtable(L, 0, 3);
+
+	switch (state) {
+	case POPEN_STATE_ALIVE:
+		lua_pushliteral(L, POPEN_LUA_STATE_ALIVE);
+		lua_setfield(L, -2, "state");
+		break;
+	case POPEN_STATE_EXITED:
+		lua_pushliteral(L, POPEN_LUA_STATE_EXITED);
+		lua_setfield(L, -2, "state");
+		lua_pushinteger(L, exit_code);
+		lua_setfield(L, -2, "exit_code");
+		break;
+	case POPEN_STATE_SIGNALED:
+		lua_pushliteral(L, POPEN_LUA_STATE_SIGNALED);
+		lua_setfield(L, -2, "state");
+		lua_pushinteger(L, exit_code);
+		lua_setfield(L, -2, "signo");
+
+		/*
+		 * FIXME: Preallocate signo -> signal name
+		 * mapping.
+		 */
+		const char *signame = "unknown";
+		for (int i = 0; popen_lua_signals[i].signame != NULL; ++i) {
+			if (popen_lua_signals[i].signo == exit_code)
+				signame = popen_lua_signals[i].signame;
+		}
+		lua_pushstring(L, signame);
+		lua_setfield(L, -2, "signame");
+
+		break;
+	default:
+		unreachable();
+	}
+
+	return 1;
+}
+
+/* }}} */
+
+/* {{{ Errors */
+
+/**
+ * Raise IllegalParams error re closed popen handle.
+ */
+static int
+luaT_popen_handle_closed_error(struct lua_State *L)
+{
+	diag_set(IllegalParams, "popen: attempt to operate on a closed handle");
+	return luaT_error(L);
+}
+
+/**
+ * Raise IllegalParams error re wrong parameter.
+ */
+static int
+luaT_popen_param_value_error(struct lua_State *L, const char *got,
+			     const char *func_name, const char *param,
+			     const char *exp)
+{
+	static const char *fmt =
+		"%s: wrong parameter \"%s\": expected %s, got %s";
+	diag_set(IllegalParams, fmt, func_name, param, exp, got);
+	return luaT_error(L);
+}
+
+/**
+ * Raise IllegalParams error re wrong parameter type.
+ */
+static int
+luaT_popen_param_type_error(struct lua_State *L, int idx, const char *func_name,
+			    const char *param, const char *exp)
+{
+	const char *typename = idx == 0 ?
+		"<unknown>" : lua_typename(L, lua_type(L, idx));
+	static const char *fmt =
+		"%s: wrong parameter \"%s\": expected %s, got %s";
+	diag_set(IllegalParams, fmt, func_name, param, exp, typename);
+	return luaT_error(L);
+}
+
+/**
+ * Raise IllegalParams error re wrong parameter type in an array.
+ */
+static int
+luaT_popen_array_elem_type_error(struct lua_State *L, int idx,
+				 const char *func_name, const char *param,
+				 int num, const char *exp)
+{
+	const char *typename = idx == 0 ?
+		"<unknown>" : lua_typename(L, lua_type(L, idx));
+	static const char *fmt =
+		"%s: wrong parameter \"%s[%d]\": expected %s, got %s";
+	diag_set(IllegalParams, fmt, func_name, param, num, exp, typename);
+	return luaT_error(L);
+}
+
+/* }}} */
+
+/* {{{ Parameter parsing */
+
+/**
+ * Parse popen.new() "opts.{stdin,stdout,stderr}" parameter.
+ *
+ * Result: @a flags_p is updated.
+ *
+ * Raise an error in case of the incorrect parameter.
+ */
+static void
+luaT_popen_parse_stdX(struct lua_State *L, int idx, int fd,
+		      unsigned int *flags_p)
+{
+	const char *action;
+	size_t action_len;
+	if ((action = luaL_check_string_strict(L, idx, &action_len)) == NULL)
+		luaT_popen_param_type_error(L, idx, "popen.new",
+					    pfd_map[fd].option_name,
+					    "string or nil");
+
+	unsigned int flags = *flags_p;
+
+	/* See popen_lua_actions. */
+	if (strncmp(action, POPEN_LUA_STREAM_INHERIT, action_len) == 0) {
+		flags &= ~pfd_map[fd].mask_devnull;
+		flags &= ~pfd_map[fd].mask_close;
+		flags &= ~pfd_map[fd].mask_pipe;
+	} else if (strncmp(action, POPEN_LUA_STREAM_DEVNULL, action_len) == 0) {
+		flags |= pfd_map[fd].mask_devnull;
+		flags &= ~pfd_map[fd].mask_close;
+		flags &= ~pfd_map[fd].mask_pipe;
+	} else if (strncmp(action, POPEN_LUA_STREAM_CLOSE, action_len) == 0) {
+		flags &= ~pfd_map[fd].mask_devnull;
+		flags |= pfd_map[fd].mask_close;
+		flags &= ~pfd_map[fd].mask_pipe;
+	} else if (strncmp(action, POPEN_LUA_STREAM_PIPE, action_len) == 0) {
+		flags &= ~pfd_map[fd].mask_devnull;
+		flags &= ~pfd_map[fd].mask_close;
+		flags |= pfd_map[fd].mask_pipe;
+	} else {
+		luaT_popen_param_value_error(L, action, "popen.new",
+					     pfd_map[fd].option_name,
+					     "popen.opts.<...> constant");
+		unreachable();
+	}
+
+	*flags_p = flags;
+}
+
+/**
+ * Glue key and value on the Lua stack into "key=value" entry.
+ *
+ * Raise an error in case of the incorrect parameter.
+ *
+ * Return NULL in case of an allocation error and set a diag
+ * (OutOfMemory).
+ */
+static char *
+luaT_popen_parse_env_entry(struct lua_State *L, int key_idx, int value_idx,
+			   struct region *region)
+{
+	size_t key_len;
+	size_t value_len;
+	const char *key = luaL_check_string_strict(L, key_idx, &key_len);
+	const char *value = luaL_check_string_strict(L, value_idx, &value_len);
+	if (key == NULL || value == NULL) {
+		luaT_popen_param_value_error(L, "a non-string key or value",
+					     "popen.new", "opts.env",
+					     "{[<string>] = <string>, ...}");
+		unreachable();
+		return NULL;
+	}
+
+	/*
+	 * FIXME: Don't sure, but maybe it would be right to
+	 * validate key and value here against '=', '\0' and
+	 * maybe other symbols.
+	 */
+
+	/* entry = "${key}=${value}" */
+	size_t entry_size = key_len + value_len + 2;
+	char *entry = region_alloc(region, entry_size);
+	if (entry == NULL) {
+		diag_set(OutOfMemory, entry_size, "region_alloc", "env entry");
+		return NULL;
+	}
+	memcpy(entry, key, key_len);
+	size_t pos = key_len;
+	entry[pos++] = '=';
+	memcpy(entry + pos, value, value_len);
+	pos += value_len;
+	entry[pos++] = '\0';
+	assert(pos == entry_size);
+
+	return entry;
+}
+
+/**
+ * Parse popen.new() "opts.env" parameter.
+ *
+ * Return a new array in the `extern char **environ` format
+ * (NULL terminated array of "foo=bar" strings).
+ *
+ * Strings and array of them are allocated on the provided
+ * region. A caller should call region_used() before invoking
+ * this function and call region_truncate() when the result
+ * is not needed anymore. Alternatively a caller may assume
+ * that fiber_gc() will collect this memory eventually, but
+ * it is recommended to do so only for rare paths.
+ *
+ * Raise an error in case of the incorrect parameter.
+ *
+ * Return NULL in case of an allocation error and set a diag
+ * (OutOfMemory).
+ */
+static char **
+luaT_popen_parse_env(struct lua_State *L, int idx, struct region *region)
+{
+	if (lua_type(L, idx) != LUA_TTABLE) {
+		luaT_popen_param_type_error(L, idx, "popen.new", "opts.env",
+					    "table or nil");
+		unreachable();
+		return NULL;
+	}
+
+	/* Calculate absolute value in the stack. */
+	if (idx < 0)
+		idx = lua_gettop(L) + idx + 1;
+
+	size_t capacity = POPEN_LUA_ENV_CAPACITY_DEFAULT;
+	size_t size = capacity * sizeof(char *);
+	size_t region_svp = region_used(region);
+	char **env = region_alloc(region, size);
+	if (env == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "env array");
+		return NULL;
+	}
+	size_t nr_env = 0;
+
+	bool only_count = false;
+
+	/*
+	 * Traverse over the table and fill `env` array. If
+	 * default `env` capacity is not enough, discard
+	 * everything, but continue iterating to count entries.
+	 */
+	lua_pushnil(L);
+	while (lua_next(L, idx) != 0) {
+		/*
+		 * Can we store the next entry and trailing NULL?
+		 */
+		if (nr_env >= capacity - 1) {
+			env = NULL;
+			region_truncate(region, region_svp);
+			only_count = true;
+		}
+		if (only_count) {
+			++nr_env;
+			lua_pop(L, 1);
+			continue;
+		}
+		char *entry = luaT_popen_parse_env_entry(L, -2, -1, region);
+		if (entry == NULL) {
+			region_truncate(region, region_svp);
+			return NULL;
+		}
+		env[nr_env++] = entry;
+		lua_pop(L, 1);
+	}
+
+	if (! only_count) {
+		assert(nr_env < capacity);
+		env[nr_env] = NULL;
+		return env;
+	}
+
+	/*
+	 * Now we know exact amount of elements. Run
+	 * the traverse again and fill `env` array.
+	 */
+	capacity = nr_env + 1;
+	size = capacity * sizeof(char *);
+	if ((env = region_alloc(region, size)) == NULL) {
+		region_truncate(region, region_svp);
+		diag_set(OutOfMemory, size, "region_alloc", "env array");
+		return NULL;
+	}
+	nr_env = 0;
+	lua_pushnil(L);
+	while (lua_next(L, idx) != 0) {
+		char *entry = luaT_popen_parse_env_entry(L, -2, -1, region);
+		if (entry == NULL) {
+			region_truncate(region, region_svp);
+			return NULL;
+		}
+		assert(nr_env < capacity - 1);
+		env[nr_env++] = entry;
+		lua_pop(L, 1);
+	}
+	assert(nr_env == capacity - 1);
+	env[nr_env] = NULL;
+	return env;
+}
+
+/**
+ * Parse popen.new() "opts" parameter.
+ *
+ * Prerequisite: @a opts should be zero filled.
+ *
+ * Result: @a opts structure is filled.
+ *
+ * Raise an error in case of the incorrect parameter.
+ *
+ * Return 0 at success. Allocates opts->env on @a region if
+ * needed. @see luaT_popen_parse_env() for details how to
+ * free it.
+ *
+ * Return -1 in case of an allocation error and set a diag
+ * (OutOfMemory).
+ */
+static int
+luaT_popen_parse_opts(struct lua_State *L, int idx, struct popen_opts *opts,
+		      struct region *region)
+{
+	/*
+	 * Default flags: inherit std*, close other fds,
+	 * restore signals.
+	 */
+	opts->flags = POPEN_FLAG_NONE		|
+		POPEN_FLAG_CLOSE_FDS		|
+		POPEN_FLAG_RESTORE_SIGNALS;
+
+	/* Parse options. */
+	if (lua_type(L, idx) == LUA_TTABLE) {
+		lua_getfield(L, idx, "stdin");
+		if (! lua_isnil(L, -1)) {
+			luaT_popen_parse_stdX(L, -1, STDIN_FILENO,
+					      &opts->flags);
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, idx, "stdout");
+		if (! lua_isnil(L, -1))
+			luaT_popen_parse_stdX(L, -1, STDOUT_FILENO,
+					      &opts->flags);
+		lua_pop(L, 1);
+
+		lua_getfield(L, idx, "stderr");
+		if (! lua_isnil(L, -1))
+			luaT_popen_parse_stdX(L, -1, STDERR_FILENO,
+					      &opts->flags);
+		lua_pop(L, 1);
+
+		lua_getfield(L, idx, "env");
+		if (! lua_isnil(L, -1)) {
+			opts->env = luaT_popen_parse_env(L, -1, region);
+			if (opts->env == NULL)
+				return -1;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, idx, "shell");
+		if (! lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				luaT_popen_param_type_error(L, -1, "popen.new",
+							    "opts.shell",
+							    "boolean or nil");
+			if (lua_toboolean(L, -1) == 0)
+				opts->flags &= ~POPEN_FLAG_SHELL;
+			else
+				opts->flags |= POPEN_FLAG_SHELL;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, idx, "setsid");
+		if (! lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				luaT_popen_param_type_error(L, -1, "popen.new",
+							    "opts.setsid",
+							    "boolean or nil");
+			if (lua_toboolean(L, -1) == 0)
+				opts->flags &= ~POPEN_FLAG_SETSID;
+			else
+				opts->flags |= POPEN_FLAG_SETSID;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, idx, "close_fds");
+		if (! lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				luaT_popen_param_type_error(L, -1, "popen.new",
+							    "opts.close_fds",
+							    "boolean or nil");
+			if (lua_toboolean(L, -1) == 0)
+				opts->flags &= ~POPEN_FLAG_CLOSE_FDS;
+			else
+				opts->flags |= POPEN_FLAG_CLOSE_FDS;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, idx, "restore_signals");
+		if (! lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				luaT_popen_param_type_error(
+					L, -1, "popen.new",
+					"opts.restore_signals",
+					"boolean or nil");
+			if (lua_toboolean(L, -1) == 0)
+				opts->flags &= ~POPEN_FLAG_RESTORE_SIGNALS;
+			else
+				opts->flags |= POPEN_FLAG_RESTORE_SIGNALS;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, idx, "group_signal");
+		if (! lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				luaT_popen_param_type_error(L, -1, "popen.new",
+							    "opts.group_signal",
+							    "boolean or nil");
+			if (lua_toboolean(L, -1) == 0)
+				opts->flags &= ~POPEN_FLAG_GROUP_SIGNAL;
+			else
+				opts->flags |= POPEN_FLAG_GROUP_SIGNAL;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, idx, "keep_child");
+		if (! lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				luaT_popen_param_type_error(L, -1, "popen.new",
+							    "opts.keep_child",
+							    "boolean or nil");
+			if (lua_toboolean(L, -1) == 0)
+				opts->flags &= ~POPEN_FLAG_KEEP_CHILD;
+			else
+				opts->flags |= POPEN_FLAG_KEEP_CHILD;
+		}
+		lua_pop(L, 1);
+	}
+
+	return 0;
+}
+
+/**
+ * Parse popen.new() "argv" parameter.
+ *
+ * Prerequisite: opts->flags & POPEN_FLAG_SHELL should be
+ * the same in a call of this function and when paired
+ * popen_new() is invoked.
+ *
+ * Raise an error in case of the incorrect parameter.
+ *
+ * Return 0 at success. Sets opts->args and opts->nr_args.
+ * Allocates opts->argv on @a region (@see
+ * luaT_popen_parse_env() for details how to free it).
+ *
+ * Return -1 in case of an allocation error and set a diag
+ * (OutOfMemory).
+ */
+static int
+luaT_popen_parse_argv(struct lua_State *L, int idx, struct popen_opts *opts,
+		      struct region *region)
+{
+	size_t region_svp = region_used(region);
+	size_t argv_len = lua_objlen(L, idx);
+
+	/* ["sh", "-c", ]..., NULL. */
+	opts->nr_argv = argv_len + 1;
+	if (opts->flags & POPEN_FLAG_SHELL)
+		opts->nr_argv += 2;
+
+	size_t size = sizeof(char *) * opts->nr_argv;
+	opts->argv = region_alloc(region, size);
+	if (opts->argv == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "argv");
+		return -1;
+	}
+
+	const char **to = (const char **)opts->argv;
+	if (opts->flags & POPEN_FLAG_SHELL) {
+		opts->argv[0] = NULL;
+		opts->argv[1] = NULL;
+		to += 2;
+	}
+
+	for (size_t i = 0; i < argv_len; ++i) {
+		lua_rawgeti(L, idx, i + 1);
+		const char *arg = luaL_check_string_strict(L, -1, NULL);
+		if (arg == NULL) {
+			region_truncate(region, region_svp);
+			return luaT_popen_array_elem_type_error(
+				L, -1, "popen.new", "argv", i + 1, "string");
+		}
+		*to++ = arg;
+		lua_pop(L, 1);
+	}
+	*to++ = NULL;
+	assert((const char **)opts->argv + opts->nr_argv == to);
+
+	return 0;
+}
+
+/**
+ * Parse popen.shell() "mode" parameter.
+ *
+ * Convert "mode" parameter into options table for popen.new().
+ * Push the table to the Lua stack.
+ *
+ * Raise an error in case of the incorrect parameter.
+ */
+static void
+luaT_popen_parse_mode(struct lua_State *L, int idx)
+{
+	if (lua_type(L, idx) != LUA_TSTRING &&
+	    lua_type(L, idx) != LUA_TNONE &&
+	    lua_type(L, idx) != LUA_TNIL)
+		luaT_popen_param_type_error(L, idx, "popen.shell", "mode",
+					    "string or nil");
+
+	/*
+	 * Create options table for popen.new().
+	 *
+	 * Preallocate space for shell, setsid, group_signal and
+	 * std{in,out,err} options.
+	 */
+	lua_createtable(L, 0, 5);
+
+	lua_pushboolean(L, true);
+	lua_setfield(L, -2, "shell");
+
+	lua_pushboolean(L, true);
+	lua_setfield(L, -2, "setsid");
+
+	lua_pushboolean(L, true);
+	lua_setfield(L, -2, "group_signal");
+
+	/*
+	 * When mode is nil, left std* params default, which means
+	 * to inherit parent's file descriptors in a child
+	 * process.
+	 */
+	if (lua_isnoneornil(L, idx))
+		return;
+
+	size_t mode_len;
+	const char *mode = lua_tolstring(L, idx, &mode_len);
+	for (size_t i = 0; i < mode_len; ++i) {
+		switch (mode[i]) {
+		case 'r':
+			lua_pushstring(L, POPEN_LUA_STREAM_PIPE);
+			lua_setfield(L, -2, "stdout");
+			break;
+		case 'R':
+			lua_pushstring(L, POPEN_LUA_STREAM_PIPE);
+			lua_setfield(L, -2, "stderr");
+			break;
+		case 'w':
+			lua_pushstring(L, POPEN_LUA_STREAM_PIPE);
+			lua_setfield(L, -2, "stdin");
+			break;
+		default:
+			luaT_popen_param_value_error(
+				L, mode, "popen.shell", "mode",
+				"'r', 'w', 'R' or its combination");
+		}
+	}
+}
+
+/* }}} */
+
+/* {{{ Lua API functions and methods */
+
+/**
+ * Execute a child program in a new process.
+ *
+ * @param argv  an array of a program to run with
+ *              command line options, mandatory;
+ *              absolute path to the program is required
+ *
+ * @param opts  table of options
+ *
+ * @param opts.stdin   action on STDIN_FILENO
+ * @param opts.stdout  action on STDOUT_FILENO
+ * @param opts.stderr  action on STDERR_FILENO
+ *
+ * File descriptor actions:
+ *
+ *     popen.opts.INHERIT  (== 'inherit') [default]
+ *                         inherit the fd from the parent
+ *     popen.opts.DEVNULL  (== 'devnull')
+ *                         open /dev/null on the fd
+ *     popen.opts.CLOSE    (== 'close')
+ *                         close the fd
+ *     popen.opts.PIPE     (== 'pipe')
+ *                         feed data from/to the fd to parent
+ *                         using a pipe
+ *
+ * @param opts.env  an array of environment variables to
+ *                  be used inside a process;
+ *                  - when is not set then the current
+ *                    environment is inherited;
+ *                  - if set then the environment will be
+ *                    replaced
+ *                  - if set to an empty array then the
+ *                    environment will be dropped
+ *
+ * @param opts.shell            (boolean, default: false)
+ *        true                  run a child process via
+ *                              'sh -c "${opts.argv}"'
+ *        false                 call the executable directly
+ *
+ * @param opts.setsid           (boolean, default: false)
+ *        true                  start executable inside a new
+ *                              session
+ *        false                 don't do that
+ *
+ * @param opts.close_fds        (boolean, default: true)
+ *        true                  close all inherited fds from a
+ *                              parent
+ *        false                 don't do that
+ *
+ * @param opts.restore_signals  (boolean, default: true)
+ *        true                  reset all signal actions
+ *                              modified in parent's process
+ *        false                 inherit changed actions
+ *
+ * @param opts.group_signal     (boolean, default: false)
+ *        true                  send signal to a child process
+ *                              group (only when opts.setsid is
+ *                              enabled)
+ *        false                 send signal to a child process
+ *                              only
+ *
+ * @param opts.keep_child       (boolean, default: false)
+ *        true                  don't send SIGKILL to a child
+ *                              process at freeing (by :close()
+ *                              or Lua GC)
+ *        false                 send SIGKILL to a child process
+ *                              (or a process group if
+ *                              opts.group_signal is enabled) at
+ *                              :close() or collecting of the
+ *                              handle by Lua GC
+ *
+ * The returned handle is subject to garbage collection, but
+ * it may be freed explicitly using :close(). SIGKILL is sent
+ * to the child process at :close() / GC if it is alive and
+ * opts.keep_child is not set.
+ *
+ * It is recommended to use opts.setsid + opts.group_signal
+ * if a child process may spawn its own childs and they all
+ * should be killed together.
+ *
+ * Note: A signal will not be sent if the child process is
+ * already dead: otherwise we might kill another process that
+ * occupies the same PID later. This means that if the child
+ * process dies before its own childs, the function will not
+ * send a signal to the process group even when opts.setsid and
+ * opts.group_signal are set.
+ *
+ * Use os.environ() to pass copy of current environment with
+ * several replacements (see example 2 below).
+ *
+ * Raise an error on incorrect parameters:
+ *
+ * - IllegalParams: incorrect type or value of a parameter.
+ * - IllegalParams: group signal is set, while setsid is not.
+ *
+ * Return a popen handle on success.
+ *
+ * Return `nil, err` on a failure. Possible reasons:
+ *
+ * - SystemError: dup(), fcntl(), pipe(), vfork() or close()
+ *                fails in the parent process.
+ * - SystemError: (temporary restriction) the parent process
+ *                has closed stdin, stdout or stderr.
+ * - OutOfMemory: unable to allocate the handle or a temporary
+ *                buffer.
+ *
+ * Example 1:
+ *
+ *  | local popen = require('popen')
+ *  |
+ *  | local ph = popen.new({'/bin/date'}, {
+ *  |     stdout = popen.opts.PIPE,
+ *  | })
+ *  | local date = ph:read():rstrip()
+ *  | ph:close()
+ *  | print(date) -- Thu 16 Apr 2020 01:40:56 AM MSK
+ *
+ * Execute 'date' command, read the result and close the
+ * popen object.
+ *
+ * Example 2:
+ *
+ *  | local popen = require('popen')
+ *  |
+ *  | local env = os.environ()
+ *  | env['FOO'] = 'bar'
+ *  |
+ *  | local ph = popen.new({'echo "${FOO}"'}, {
+ *  |     stdout = popen.opts.PIPE,
+ *  |     shell = true,
+ *  |     env = env,
+ *  | })
+ *  | local res = ph:read():rstrip()
+ *  | ph:close()
+ *  | print(res) -- bar
+ *
+ * It is quite similar to the previous one, but sets the
+ * environment variable and uses shell builtin 'echo' to
+ * show it.
+ *
+ * Example 3:
+ *
+ *  | local popen = require('popen')
+ *  |
+ *  | local ph = popen.new({'echo hello >&2'}, { -- !!
+ *  |     stderr = popen.opts.PIPE,              -- !!
+ *  |     shell = true,
+ *  | })
+ *  | local res = ph:read({stderr = true}):rstrip()
+ *  | ph:close()
+ *  | print(res) -- hello
+ *
+ * This example demonstrates how to capture child's stderr.
+ *
+ * Example 4:
+ *
+ *  | local function call_jq(stdin, filter)
+ *  |     -- Start jq process, connect to stdin, stdout and stderr.
+ *  |     local jq_argv = {'/usr/bin/jq', '-M', '--unbuffered', filter}
+ *  |     local ph, err = popen.new(jq_argv, {
+ *  |         stdin = popen.opts.PIPE,
+ *  |         stdout = popen.opts.PIPE,
+ *  |         stderr = popen.opts.PIPE,
+ *  |     })
+ *  |     if ph == nil then return nil, err end
+ *  |
+ *  |     -- Write input data to child's stdin and send EOF.
+ *  |     local ok, err = ph:write(stdin)
+ *  |     if not ok then return nil, err end
+ *  |     ph:shutdown({stdin = true})
+ *  |
+ *  |     -- Read everything until EOF.
+ *  |     local chunks = {}
+ *  |     while true do
+ *  |         local chunk, err = ph:read()
+ *  |         if chunk == nil then
+ *  |             ph:close()
+ *  |             return nil, err
+ *  |         end
+ *  |         if chunk == '' then break end -- EOF
+ *  |         table.insert(chunks, chunk)
+ *  |     end
+ *  |
+ *  |     -- Read diagnostics from stderr if any.
+ *  |     local err = ph:read({stderr = true})
+ *  |     if err ~= '' then
+ *  |         ph:close()
+ *  |         return nil, err
+ *  |     end
+ *  |
+ *  |     -- Glue all chunks, strip trailing newline.
+ *  |     return table.concat(chunks):rstrip()
+ *  | end
+ *
+ * Demonstrates how to run a stream program (like `grep`, `sed`
+ * and so), write to its stdin and read from its stdout.
+ *
+ * The example assumes that input data are small enough to fit
+ * a pipe buffer (typically 64 KiB, but depends on a platform
+ * and its configuration). It will stuck in :write() for large
+ * data. How to handle this case: call :read() in a loop in
+ * another fiber (start it before a first :write()).
+ *
+ * If a process writes large text to stderr, it may fill out
+ * stderr pipe buffer and block in write(2, ...). So we need to
+ * read stderr too in another fiber to handle this case.
+ */
+static int
+lbox_popen_new(struct lua_State *L)
+{
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+
+	if (lua_type(L, 1) != LUA_TTABLE)
+		return luaT_popen_param_type_error(L, 1, "popen.new", "argv",
+						   "table");
+	else if (lua_type(L, 2) != LUA_TTABLE &&
+		 lua_type(L, 2) != LUA_TNONE &&
+		 lua_type(L, 2) != LUA_TNIL)
+		return luaT_popen_param_type_error(L, 2, "popen.new", "opts",
+						   "table or nil");
+
+	/* Parse opts and argv. */
+	struct popen_opts opts = {};
+	int rc = luaT_popen_parse_opts(L, 2, &opts, region);
+	if (rc != 0)
+		goto err;
+	rc = luaT_popen_parse_argv(L, 1, &opts, region);
+	if (rc != 0)
+		goto err;
+
+	struct popen_handle *handle = popen_new(&opts);
+
+	if (handle == NULL)
+		goto err;
+
+	region_truncate(region, region_svp);
+	luaT_push_popen_handle(L, handle);
+	return 1;
+
+err:
+	region_truncate(region, region_svp);
+	struct error *e = diag_last_error(diag_get());
+	if (e->type == &type_IllegalParams)
+		return luaT_error(L);
+	return luaT_push_nil_and_error(L);
+}
+
+/**
+ * Execute a shell command.
+ *
+ * @param command  a command to run, mandatory
+ * @param mode     communication mode, optional
+ *                 'w'    to use ph:write()
+ *                 'r'    to use ph:read()
+ *                 'R'    to use ph:read({stderr = true})
+ *                 nil    inherit parent's std* file descriptors
+ *
+ * Several mode characters can be set together: 'rw', 'rRw', etc.
+ *
+ * This function is just shortcut for popen.new({command}, opts)
+ * with opts.{shell,setsid,group_signal} set to `true` and
+ * and opts.{stdin,stdout,stderr} set based on `mode` parameter.
+ *
+ * All std* streams are inherited from parent by default if it is
+ * not changed using mode: 'r' for stdout, 'R' for stderr, 'w' for
+ * stdin.
+ *
+ * Raise an error on incorrect parameters:
+ *
+ * - IllegalParams: incorrect type or value of a parameter.
+ *
+ * Return a popen handle on success.
+ *
+ * Return `nil, err` on a failure.
+ * @see lbox_popen_new() for possible reasons.
+ *
+ * Example:
+ *
+ *  | local popen = require('popen')
+ *  |
+ *  | local ph = popen.shell('date', 'r')
+ *  | local date = ph:read():rstrip()
+ *  | ph:close()
+ *  | print(date)
+ *
+ * Execute 'sh -c date' command, read the output and close the
+ * popen object.
+ */
+static int
+lbox_popen_shell(struct lua_State *L)
+{
+	if (lua_type(L, 1) != LUA_TSTRING)
+		return luaT_popen_param_type_error(L, 1, "popen.shell",
+						   "command", "string");
+
+	/*
+	 * Ensure that at least two stack slots are occupied.
+	 *
+	 * Otherwise we can pass `top` as `idx` to lua_replace().
+	 * lua_replace() on `top` index copies a value to itself
+	 * first and then pops it from the stack.
+	 */
+	if (lua_gettop(L) == 1)
+		lua_pushnil(L);
+
+	/* Create argv table for popen.new(). */
+	lua_createtable(L, 1, 0);
+	/* argv[1] = command */
+	lua_pushvalue(L, 1);
+	lua_rawseti(L, -2, 1);
+	/* {...}[1] == argv */
+	lua_replace(L, 1);
+
+	/* opts = parse_mode(mode) */
+	luaT_popen_parse_mode(L, 2);
+	/* {...}[2] == opts */
+	lua_replace(L, 2);
+
+	return lbox_popen_new(L);
+}
+
+/**
+ * Send signal to a child process.
+ *
+ * @param handle  a handle carries child process to be signaled
+ * @param signo   signal number to send
+ *
+ * When opts.setsid and opts.group_signal are set on the handle
+ * the signal is sent to the process group rather than to the
+ * process. @see lbox_popen_new() for details about group
+ * signaling.
+ *
+ * Note: The module offers popen.signal.SIG* constants, because
+ * some signals have different numbers on different platforms.
+ *
+ * Raise an error on incorrect parameters:
+ *
+ * - IllegalParams:    an incorrect handle parameter.
+ * - IllegalParams:    called on a closed handle.
+ *
+ * Return `true` if signal is sent.
+ *
+ * Return `nil, err` on a failure. Possible reasons:
+ *
+ * - SystemError: a process does not exists anymore
+ *
+ *                Aside of a non-exist process it is also
+ *                returned for a zombie process or when all
+ *                processes in a group are zombies (but
+ *                see note re Mac OS below).
+ *
+ * - SystemError: invalid signal number
+ *
+ * - SystemError: no permission to send a signal to
+ *                a process or a process group
+ *
+ *                It is returned on Mac OS when a signal is
+ *                sent to a process group, where a group leader
+ *                is zombie (or when all processes in it
+ *                are zombies, don't sure).
+ *
+ *                Whether it may appear due to other
+ *                reasons is unclear.
+ */
+static int
+lbox_popen_signal(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL ||
+	    !lua_isnumber(L, 2)) {
+		diag_set(IllegalParams, "Bad params, use: ph:signal(signo)");
+		return luaT_error(L);
+	}
+	if (is_closed)
+		return luaT_popen_handle_closed_error(L);
+
+	int signo = lua_tonumber(L, 2);
+
+	if (popen_send_signal(handle, signo) != 0)
+		return luaT_push_nil_and_error(L);
+
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/**
+ * Send SIGTERM signal to a child process.
+ *
+ * @param handle  a handle carries child process to terminate
+ *
+ * The function only sends SIGTERM signal and does NOT
+ * free any resources (popen handle memory and file
+ * descriptors).
+ *
+ * @see lbox_popen_signal() for errors and return values.
+ */
+static int
+lbox_popen_terminate(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
+		diag_set(IllegalParams, "Bad params, use: ph:terminate()");
+		return luaT_error(L);
+	}
+	if (is_closed)
+		return luaT_popen_handle_closed_error(L);
+
+	int signo = SIGTERM;
+
+	if (popen_send_signal(handle, signo) != 0)
+		return luaT_push_nil_and_error(L);
+
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/**
+ * Send SIGKILL signal to a child process.
+ *
+ * @param handle  a handle carries child process to kill
+ *
+ * The function only sends SIGKILL signal and does NOT
+ * free any resources (popen handle memory and file
+ * descriptors).
+ *
+ * @see lbox_popen_signal() for errors and return values.
+ */
+static int
+lbox_popen_kill(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
+		diag_set(IllegalParams, "Bad params, use: ph:kill()");
+		return luaT_error(L);
+	}
+	if (is_closed)
+		return luaT_popen_handle_closed_error(L);
+
+	int signo = SIGKILL;
+
+	if (popen_send_signal(handle, signo) != 0)
+		return luaT_push_nil_and_error(L);
+
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/**
+ * Wait until a child process get exited or signaled.
+ *
+ * @param handle  a handle of process to wait
+ *
+ * Raise an error on incorrect parameters or when the fiber is
+ * cancelled:
+ *
+ * - IllegalParams:    an incorrect handle parameter.
+ * - IllegalParams:    called on a closed handle.
+ * - FiberIsCancelled: cancelled by an outside code.
+ *
+ * Return a process status table (the same as ph.status and
+ * ph.info().status). @see lbox_popen_info() for the format
+ * of the table.
+ */
+static int
+lbox_popen_wait(struct lua_State *L)
+{
+	/*
+	 * FIXME: Use trigger or fiber conds to sleep and wake up.
+	 * FIXME: Add timeout option: ph:wait({timeout = <...>})
+	 */
+	struct popen_handle *handle;
+	bool is_closed;
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
+		diag_set(IllegalParams, "Bad params, use: ph:wait()");
+		return luaT_error(L);
+	}
+	if (is_closed)
+		return luaT_popen_handle_closed_error(L);
+
+	int state;
+	int exit_code;
+
+	while (true) {
+		popen_state(handle, &state, &exit_code);
+		assert(state < POPEN_STATE_MAX);
+		if (state != POPEN_STATE_ALIVE)
+			break;
+		fiber_sleep(POPEN_LUA_WAIT_DELAY);
+		luaL_testcancel(L);
+	}
+
+	return luaT_push_popen_process_status(L, state, exit_code);
+}
+
+/**
+ * Read data from a child peer.
+ *
+ * @param handle        handle of a child process
+ * @param opts          an options table
+ * @param opts.stdout   whether to read from stdout, boolean
+ *                      (default: true)
+ * @param opts.stderr   whether to read from stderr, boolean
+ *                      (default: false)
+ * @param opts.timeout  time quota in seconds
+ *                      (default: 100 years)
+ *
+ * Read data from stdout or stderr streams with @a timeout.
+ * By default it reads from stdout. Set @a opts.stderr to
+ * `true` to read from stderr.
+ *
+ * Raise an error on incorrect parameters or when the fiber is
+ * cancelled:
+ *
+ * - IllegalParams:    incorrect type or value of a parameter.
+ * - IllegalParams:    called on a closed handle.
+ * - IllegalParams:    opts.stdout and opts.stderr are set both
+ * - IllegalParams:    a requested IO operation is not supported
+ *                     by the handle (stdout / stderr is not
+ *                     piped).
+ * - IllegalParams:    attempt to operate on a closed file
+ *                     descriptor.
+ * - FiberIsCancelled: cancelled by an outside code.
+ *
+ * Return a string on success, an empty string at EOF.
+ *
+ * Return `nil, err` on a failure. Possible reasons:
+ *
+ * - SocketError: an IO error occurs at read().
+ * - TimedOut:    @a timeout quota is exceeded.
+ * - OutOfMemory: no memory space for a buffer to read into.
+ * - LuajitError: ("not enough memory"): no memory space for
+ *                the Lua string.
+ */
+static int
+lbox_popen_read(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+	unsigned int flags = POPEN_FLAG_NONE;
+	ev_tstamp timeout = TIMEOUT_INFINITY;
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+
+	/* Extract handle. */
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL)
+		goto usage;
+	if (is_closed)
+		return luaT_popen_handle_closed_error(L);
+
+	/* Extract options. */
+	if (!lua_isnoneornil(L, 2)) {
+		if (lua_type(L, 2) != LUA_TTABLE)
+			goto usage;
+
+		lua_getfield(L, 2, "stdout");
+		if (!lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				goto usage;
+			if (lua_toboolean(L, -1) == 0)
+				flags &= ~POPEN_FLAG_FD_STDOUT;
+			else
+				flags |= POPEN_FLAG_FD_STDOUT;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, 2, "stderr");
+		if (!lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				goto usage;
+			if (lua_toboolean(L, -1) == 0)
+				flags &= ~POPEN_FLAG_FD_STDERR;
+			else
+				flags |= POPEN_FLAG_FD_STDERR;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, 2, "timeout");
+		if (!lua_isnil(L, -1) &&
+		    (timeout = luaT_check_timeout(L, -1)) < 0.0)
+			goto usage;
+		lua_pop(L, 1);
+	}
+
+	/* Read from stdout by default. */
+	if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR)))
+		flags |= POPEN_FLAG_FD_STDOUT;
+
+	size_t size = POPEN_LUA_READ_BUF_SIZE;
+	char *buf = region_alloc(region, size);
+	if (buf == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "read buffer");
+		return luaT_push_nil_and_error(L);
+	}
+
+	static_assert(POPEN_LUA_READ_BUF_SIZE <= SSIZE_MAX,
+		      "popen: read buffer is too big");
+	ssize_t rc = popen_read_timeout(handle, buf, size, flags, timeout);
+	if (rc < 0 || luaT_push_string_noxc(L, buf, rc) != 0)
+		goto err;
+	region_truncate(region, region_svp);
+	return 1;
+
+usage:
+	diag_set(IllegalParams, "Bad params, use: ph:read([{"
+		 "stdout = <boolean>, "
+		 "stderr = <boolean>, "
+		 "timeout = <number>}])");
+	return luaT_error(L);
+err:
+	region_truncate(region, region_svp);
+	struct error *e = diag_last_error(diag_get());
+	if (e->type == &type_IllegalParams ||
+	    e->type == &type_FiberIsCancelled)
+		return luaT_error(L);
+	return luaT_push_nil_and_error(L);
+}
+
+/**
+ * Write data to a child peer.
+ *
+ * @param handle        a handle of a child process
+ * @param str           a string to write
+ * @param opts          table of options
+ * @param opts.timeout  time quota in seconds
+ *                      (default: 100 years)
+ *
+ * Write string @a str to stdin stream of a child process.
+ *
+ * The function may yield forever if a child process does
+ * not read data from stdin and a pipe buffer becomes full.
+ * Size of this buffer depends on a platform. Use
+ * @a opts.timeout when unsure.
+ *
+ * When @a opts.timeout is not set, the function blocks
+ * (yields the fiber) until all data is written or an error
+ * happened.
+ *
+ * Raise an error on incorrect parameters or when the fiber is
+ * cancelled:
+ *
+ * - IllegalParams:    incorrect type or value of a parameter.
+ * - IllegalParams:    called on a closed handle.
+ * - IllegalParams:    string length is greater then SSIZE_MAX.
+ * - IllegalParams:    a requested IO operation is not supported
+ *                     by the handle (stdin is not piped).
+ * - IllegalParams:    attempt to operate on a closed file
+ *                     descriptor.
+ * - FiberIsCancelled: cancelled by an outside code.
+ *
+ * Return `true` on success.
+ *
+ * Return `nil, err` on a failure. Possible reasons:
+ *
+ * - SocketError: an IO error occurs at write().
+ * - TimedOut:    @a timeout quota is exceeded.
+ */
+static int
+lbox_popen_write(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+	const char *str;
+	size_t len;
+	ev_tstamp timeout = TIMEOUT_INFINITY;
+
+	/* Extract handle and string to write. */
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL ||
+	    (str = luaL_check_string_strict(L, 2, &len)) == NULL)
+		goto usage;
+	if (is_closed)
+		return luaT_popen_handle_closed_error(L);
+
+	/* Extract options. */
+	if (!lua_isnoneornil(L, 3)) {
+		if (lua_type(L, 3) != LUA_TTABLE)
+			goto usage;
+
+		lua_getfield(L, 3, "timeout");
+		if (!lua_isnil(L, -1) &&
+		    (timeout = luaT_check_timeout(L, -1)) < 0.0)
+			goto usage;
+		lua_pop(L, 1);
+	}
+
+	unsigned int flags = POPEN_FLAG_FD_STDIN;
+	ssize_t rc = popen_write_timeout(handle, str, len, flags, timeout);
+	assert(rc < 0 || rc == (ssize_t)len);
+	if (rc < 0) {
+		struct error *e = diag_last_error(diag_get());
+		if (e->type == &type_IllegalParams ||
+		    e->type == &type_FiberIsCancelled)
+			return luaT_error(L);
+		return luaT_push_nil_and_error(L);
+	}
+	lua_pushboolean(L, true);
+	return 1;
+
+usage:
+	diag_set(IllegalParams, "Bad params, use: ph:write(str[, {"
+		 "timeout = <number>}])");
+	return luaT_error(L);
+}
+
+/**
+ * Close parent's ends of std* fds.
+ *
+ * @param handle        handle of a child process
+ * @param opts          an options table
+ * @param opts.stdin    close parent's end of stdin, boolean
+ * @param opts.stdout   close parent's end of stdout, boolean
+ * @param opts.stderr   close parent's end of stderr, boolean
+ *
+ * The main reason to use this function is to send EOF to
+ * child's stdin. However parent's end of stdout / stderr
+ * may be closed too.
+ *
+ * The function does not fail on already closed fds (idempotence).
+ * However it fails on attempt to close the end of a pipe that was
+ * never exist. In other words, only those std* options that
+ * were set to popen.opts.PIPE at a handle creation may be used
+ * here (for popen.shell: 'r' corresponds to stdout, 'R' to stderr
+ * and 'w' to stdin).
+ *
+ * The function does not close any fds on a failure: either all
+ * requested fds are closed or neither of them.
+ *
+ * Example:
+ *
+ *  | local popen = require('popen')
+ *  |
+ *  | local ph = popen.shell('sed s/foo/bar/', 'rw')
+ *  | ph:write('lorem foo ipsum')
+ *  | ph:shutdown({stdin = true})
+ *  | local res = ph:read()
+ *  | ph:close()
+ *  | print(res) -- lorem bar ipsum
+ *
+ * Raise an error on incorrect parameters:
+ *
+ * - IllegalParams:  an incorrect handle parameter.
+ * - IllegalParams:  called on a closed handle.
+ * - IllegalParams:  neither stdin, stdout nor stderr is choosen.
+ * - IllegalParams:  a requested IO operation is not supported
+ *                   by the handle (one of std* is not piped).
+ *
+ * Return `true` on success.
+ */
+static int
+lbox_popen_shutdown(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+
+	/* Extract handle. */
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL)
+		goto usage;
+	if (is_closed)
+		return luaT_popen_handle_closed_error(L);
+
+	unsigned int flags = POPEN_FLAG_NONE;
+
+	/* Extract options. */
+	if (!lua_isnoneornil(L, 2)) {
+		if (lua_type(L, 2) != LUA_TTABLE)
+			goto usage;
+
+		/*
+		 * FIXME: Those blocks duplicates ones from
+		 * lbox_popen_read().
+		 *
+		 * Let's introduce a helper like
+		 * luaT_popen_parse_stdX() but about boolean
+		 * flags rather than stream actions.
+		 */
+
+		lua_getfield(L, 2, "stdin");
+		if (!lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				goto usage;
+			if (lua_toboolean(L, -1) == 0)
+				flags &= ~POPEN_FLAG_FD_STDIN;
+			else
+				flags |= POPEN_FLAG_FD_STDIN;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, 2, "stdout");
+		if (!lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				goto usage;
+			if (lua_toboolean(L, -1) == 0)
+				flags &= ~POPEN_FLAG_FD_STDOUT;
+			else
+				flags |= POPEN_FLAG_FD_STDOUT;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, 2, "stderr");
+		if (!lua_isnil(L, -1)) {
+			if (lua_type(L, -1) != LUA_TBOOLEAN)
+				goto usage;
+			if (lua_toboolean(L, -1) == 0)
+				flags &= ~POPEN_FLAG_FD_STDERR;
+			else
+				flags |= POPEN_FLAG_FD_STDERR;
+		}
+		lua_pop(L, 1);
+	}
+
+	if (popen_shutdown(handle, flags) != 0) {
+		/*
+		 * FIXME: This block is often duplicated,
+		 * let's extract it to a helper.
+		 */
+		struct error *e = diag_last_error(diag_get());
+		if (e->type == &type_IllegalParams)
+			return luaT_error(L);
+		return luaT_push_nil_and_error(L);
+	}
+
+	lua_pushboolean(L, true);
+	return 1;
+
+usage:
+	diag_set(IllegalParams, "Bad params, use: ph:shutdown({"
+		 "stdin = <boolean>, "
+		 "stdout = <boolean>, "
+		 "stderr = <boolean>})");
+	return luaT_error(L);
+
+}
+
+/**
+ * Return information about popen handle.
+ *
+ * @param handle  a handle of a child process
+ *
+ * Raise an error on incorrect parameters:
+ *
+ * - IllegalParams: an incorrect handle parameter.
+ * - IllegalParams: called on a closed handle.
+ *
+ * Return information about the handle in the following
+ * format:
+ *
+ *     {
+ *         pid = <number> or <nil>,
+ *         command = <string>,
+ *         opts = <table>,
+ *         status = <table>,
+ *         stdin = one-of(
+ *             popen.stream.OPENED (== 'opened'),
+ *             popen.stream.CLOSED (== 'closed'),
+ *             nil,
+ *         ),
+ *         stdout = one-of(
+ *             popen.stream.OPENED (== 'opened'),
+ *             popen.stream.CLOSED (== 'closed'),
+ *             nil,
+ *         ),
+ *         stderr = one-of(
+ *             popen.stream.OPENED (== 'opened'),
+ *             popen.stream.CLOSED (== 'closed'),
+ *             nil,
+ *         ),
+ *     }
+ *
+ * `pid` is a process id of the process when it is alive,
+ * otherwise `pid` is nil.
+ *
+ * `command` is a concatenation of space separated arguments
+ * that were passed to execve(). Multiword arguments are quoted.
+ * Quotes inside arguments are not escaped.
+ *
+ * `opts` is a table of handle options in the format of
+ * popen.new() `opts` parameter. `opts.env` is not shown here,
+ * because the environment variables map is not stored in a
+ * handle.
+ *
+ * `status` is a table that represents a process status in the
+ * following format:
+ *
+ *     {
+ *         state = one-of(
+ *             popen.state.ALIVE    (== 'alive'),
+ *             popen.state.EXITED   (== 'exited'),
+ *             popen.state.SIGNALED (== 'signaled'),
+ *         )
+ *
+ *         -- Present when `state` is 'exited'.
+ *         exit_code = <number>,
+ *
+ *         -- Present when `state` is 'signaled'.
+ *         signo = <number>,
+ *         signame = <string>,
+ *     }
+ *
+ * `stdin`, `stdout`, `stderr` reflect status of parent's end
+ * of a piped stream. When a stream is not piped the field is
+ * not present (`nil`). When it is piped, the status may be
+ * one of the following:
+ *
+ * - popen.stream.OPENED  (== 'opened')
+ * - popen.stream.CLOSED  (== 'closed')
+ *
+ * The status may be changed from 'opened' to 'closed'
+ * by :shutdown({std... = true}) call.
+ *
+ * @see luaT_push_popen_opts()
+ * @see luaT_push_popen_process_status()
+ *
+ * Example 1 (tarantool console):
+ *
+ *  | tarantool> require('popen').new({'/usr/bin/touch', '/tmp/foo'})
+ *  | ---
+ *  | - command: /usr/bin/touch /tmp/foo
+ *  |   status:
+ *  |     state: alive
+ *  |   opts:
+ *  |     stdout: inherit
+ *  |     stdin: inherit
+ *  |     group_signal: false
+ *  |     keep_child: false
+ *  |     close_fds: true
+ *  |     restore_signals: true
+ *  |     shell: false
+ *  |     setsid: false
+ *  |     stderr: inherit
+ *  |   pid: 9499
+ *  | ...
+ *
+ * Example 2 (tarantool console):
+ *
+ *  | tarantool> require('popen').shell('grep foo', 'wrR')
+ *  | ---
+ *  | - stdout: opened
+ *  |   command: sh -c 'grep foo'
+ *  |   stderr: opened
+ *  |   status:
+ *  |     state: alive
+ *  |   stdin: opened
+ *  |   opts:
+ *  |     stdout: pipe
+ *  |     stdin: pipe
+ *  |     group_signal: true
+ *  |     keep_child: false
+ *  |     close_fds: true
+ *  |     restore_signals: true
+ *  |     shell: true
+ *  |     setsid: true
+ *  |     stderr: pipe
+ *  |   pid: 10497
+ *  | ...
+ */
+static int
+lbox_popen_info(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
+		diag_set(IllegalParams, "Bad params, use: ph:info()");
+		return luaT_error(L);
+	}
+	if (is_closed)
+		return luaT_popen_handle_closed_error(L);
+
+	struct popen_stat st = {};
+
+	popen_stat(handle, &st);
+
+	lua_createtable(L, 0, 7);
+
+	if (st.pid >= 0) {
+		lua_pushinteger(L, st.pid);
+		lua_setfield(L, -2, "pid");
+	}
+
+	lua_pushstring(L, popen_command(handle));
+	lua_setfield(L, -2, "command");
+
+	luaT_push_popen_opts(L, st.flags);
+	lua_setfield(L, -2, "opts");
+
+	int state;
+	int exit_code;
+	popen_state(handle, &state, &exit_code);
+	assert(state < POPEN_STATE_MAX);
+	luaT_push_popen_process_status(L, state, exit_code);
+	lua_setfield(L, -2, "status");
+
+	luaT_push_popen_stdX_status(L, handle, STDIN_FILENO);
+	lua_setfield(L, -2, "stdin");
+
+	luaT_push_popen_stdX_status(L, handle, STDOUT_FILENO);
+	lua_setfield(L, -2, "stdout");
+
+	luaT_push_popen_stdX_status(L, handle, STDERR_FILENO);
+	lua_setfield(L, -2, "stderr");
+
+	return 1;
+}
+
+/**
+ * Close a popen handle.
+ *
+ * @param handle  a handle to close
+ *
+ * Basically it kills a process using SIGKILL and releases all
+ * resources assosiated with the popen handle.
+ *
+ * Details about signaling:
+ *
+ * - The signal is sent only when opts.keep_child is not set.
+ * - The signal is sent only when a process is alive according
+ *   to the information available on current even loop iteration.
+ *   (There is a gap here: a zombie may be signaled; it is
+ *   harmless.)
+ * - The signal is sent to a process or a grocess group depending
+ *   of opts.group_signal. (@see lbox_popen_new() for details of
+ *   group signaling).
+ *
+ * Resources are released disregarding of whether a signal
+ * sending succeeds: fds are closed, memory is released,
+ * the handle is marked as closed.
+ *
+ * No operation is possible on a closed handle except
+ * :close(), which always successful on closed handle
+ * (idempotence).
+ *
+ * Raise an error on incorrect parameters:
+ *
+ * - IllegalParams: an incorrect handle parameter.
+ *
+ * The function may return `true` or `nil, err`, but it always
+ * frees the handle resources. So any return value usually
+ * means success for a caller. The return values are purely
+ * informational: it is for logging or same kind of reporting.
+ *
+ * Possible diagnostics (don't consider them as errors):
+ *
+ * - SystemError: no permission to send a signal to
+ *                a process or a process group
+ *
+ *                This diagnostics may appear due to
+ *                Mac OS behaviour on zombies when
+ *                opts.group_signal is set,
+ *                @see lbox_popen_signal().
+ *
+ *                Whether it may appear due to other
+ *                reasons is unclear.
+ *
+ * Always return `true` when a process is known as dead (say,
+ * after ph:wait()): no signal will be send, so no 'failure'
+ * may appear.
+ */
+static int
+lbox_popen_close(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
+		diag_set(IllegalParams, "Bad params, use: ph:close()");
+		return luaT_error(L);
+	}
+
+	/* Do nothing on a closed handle. */
+	if (is_closed) {
+		lua_pushboolean(L, true);
+		return 1;
+	}
+
+	if (popen_delete(handle) != 0)
+		return luaT_push_nil_and_error(L);
+
+	luaT_mark_popen_handle_closed(L, 1);
+
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/**
+ * Get a field from a handle.
+ *
+ * @param handle  a handle of a child process
+ * @param key     a field name, string
+ *
+ * The function performs the following steps.
+ *
+ * Raise an error on incorrect parameters:
+ *
+ * - IllegalParams: incorrect type or value of a parameter.
+ *
+ * If there is a handle method with @a key name, return it.
+ *
+ * Raise an error on closed popen handle:
+ *
+ * - IllegalParams: called on a closed handle.
+ *
+ * If a @key is one of the following, return a value for it:
+ *
+ * - pid
+ * - command
+ * - opts
+ * - status
+ * - stdin
+ * - stdout
+ * - stderr
+ *
+ * @see lbox_popen_info() for description of those fields.
+ *
+ * Otherwise return `nil`.
+ */
+static int
+lbox_popen_index(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+	const char *key;
+
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL ||
+	    (key = luaL_check_string_strict(L, 2, NULL)) == NULL) {
+		diag_set(IllegalParams,
+			 "Bad params, use __index(ph, <string>)");
+		return luaT_error(L);
+	}
+
+	/* Get a value from the metatable. */
+	lua_getmetatable(L, 1);
+	lua_pushvalue(L, 2);
+	lua_rawget(L, -2);
+	if (! lua_isnil(L, -1))
+		return 1;
+
+	if (is_closed) {
+		diag_set(IllegalParams,
+			 "Attempt to index a closed popen handle");
+		return luaT_error(L);
+	}
+
+	if (strcmp(key, "pid") == 0) {
+		if (handle->pid >= 0)
+			lua_pushinteger(L, handle->pid);
+		else
+			lua_pushnil(L);
+		return 1;
+	}
+
+	if (strcmp(key, "command") == 0) {
+		lua_pushstring(L, popen_command(handle));
+		return 1;
+	}
+
+	if (strcmp(key, "opts") == 0) {
+		luaT_push_popen_opts(L, handle->flags);
+		return 1;
+	}
+
+	int state;
+	int exit_code;
+	popen_state(handle, &state, &exit_code);
+	assert(state < POPEN_STATE_MAX);
+	if (strcmp(key, "status") == 0)
+		return luaT_push_popen_process_status(L, state, exit_code);
+
+	if (strcmp(key, "stdin") == 0)
+		return luaT_push_popen_stdX_status(L, handle, STDIN_FILENO);
+
+	if (strcmp(key, "stdout") == 0)
+		return luaT_push_popen_stdX_status(L, handle, STDOUT_FILENO);
+
+	if (strcmp(key, "stderr") == 0)
+		return luaT_push_popen_stdX_status(L, handle, STDERR_FILENO);
+
+	lua_pushnil(L);
+	return 1;
+}
+
+/**
+ * Popen handle representation for REPL (console).
+ *
+ * @param handle  a handle of a child process
+ *
+ * The function just calls lbox_popen_info() for a
+ * handle when it is not closed.
+ *
+ * Return '<closed popen handle>' string for a closed
+ * handle.
+ *
+ * @see lbox_popen_info()
+ */
+static int
+lbox_popen_serialize(struct lua_State *L)
+{
+	struct popen_handle *handle;
+	bool is_closed;
+
+	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
+		diag_set(IllegalParams, "Bad params, use: __serialize(ph)");
+		return luaT_error(L);
+	}
+
+	if (is_closed) {
+		lua_pushliteral(L, "<closed popen handle>");
+		return 1;
+	}
+
+	return lbox_popen_info(L);
+}
+
+/**
+ * Free popen handle resources.
+ *
+ * @param handle  a handle to free
+ *
+ * The same as lbox_popen_close(), but silently exits on any
+ * failure.
+ *
+ * The method may be called manually from Lua, so it is able to
+ * proceed with an incorrect and a closed handle. It also marks
+ * a handle as closed to don't free resources twice if the
+ * handle is collected by Lua GC after a manual call of the
+ * method.
+ *
+ * Don't return a value.
+ */
+static int
+lbox_popen_gc(struct lua_State *L)
+{
+	bool is_closed;
+	struct popen_handle *handle = luaT_check_popen_handle(L, 1, &is_closed);
+	if (handle == NULL || is_closed)
+		return 0;
+	popen_delete(handle);
+	luaT_mark_popen_handle_closed(L, 1);
+	return 0;
+}
+
+/* }}} */
+
+/* {{{ Module initialization */
+
+/**
+ * Create popen functions and methods.
+ *
+ * Module functions
+ * ----------------
+ *
+ * - popen.new()
+ * - popen.shell()
+ *
+ * Module constants
+ * ----------------
+ *
+ * - popen.opts
+ *   - INHERIT (== 'inherit')
+ *   - DEVNULL (== 'devnull')
+ *   - CLOSE   (== 'close')
+ *   - PIPE    (== 'pipe')
+ *
+ * - popen.signal
+ *   - SIGTERM (== 9)
+ *   - SIGKILL (== 15)
+ *   - ...
+ *
+ * - popen.state
+ *   - ALIVE    (== 'alive')
+ *   - EXITED   (== 'exited')
+ *   - SIGNALED (== 'signaled')
+ *
+ * - popen.stream
+ *   - OPENED  (== 'opened')
+ *   - CLOSED  (== 'closed')
+ */
+void
+tarantool_lua_popen_init(struct lua_State *L)
+{
+	/* Popen module methods. */
+	static const struct luaL_Reg popen_methods[] = {
+		{"new",		lbox_popen_new,		},
+		{"shell",	lbox_popen_shell,	},
+		{NULL, NULL},
+	};
+	luaL_register_module(L, "popen", popen_methods);
+
+	/*
+	 * Popen handle methods and metamethods.
+	 *
+	 * Usual and closed popen handle userdata types have
+	 * the same set of methods and metamethods.
+	 */
+	static const struct luaL_Reg popen_handle_methods[] = {
+		{"signal",		lbox_popen_signal,	},
+		{"terminate",		lbox_popen_terminate,	},
+		{"kill",		lbox_popen_kill,	},
+		{"wait",		lbox_popen_wait,	},
+		{"read",		lbox_popen_read,	},
+		{"write",		lbox_popen_write,	},
+		{"shutdown",		lbox_popen_shutdown,	},
+		{"info",		lbox_popen_info,	},
+		{"close",		lbox_popen_close,	},
+		{"__index",		lbox_popen_index	},
+		{"__serialize",		lbox_popen_serialize	},
+		{"__gc",		lbox_popen_gc		},
+		{NULL, NULL},
+	};
+	luaL_register_type(L, popen_handle_uname, popen_handle_methods);
+	luaL_register_type(L, popen_handle_closed_uname, popen_handle_methods);
+
+	/* Signals. */
+	lua_newtable(L);
+	for (int i = 0; popen_lua_signals[i].signame != NULL; ++i) {
+		lua_pushinteger(L, popen_lua_signals[i].signo);
+		lua_setfield(L, -2, popen_lua_signals[i].signame);
+	}
+	lua_setfield(L, -2, "signal");
+
+	/* Stream actions. */
+	lua_newtable(L);
+	for (int i = 0; popen_lua_actions[i].name != NULL; ++i) {
+		lua_pushstring(L, popen_lua_actions[i].value);
+		lua_setfield(L, -2, popen_lua_actions[i].name);
+	}
+	lua_setfield(L, -2, "opts");
+
+	/* Stream statuses. */
+	lua_newtable(L);
+	for (int i = 0; popen_lua_stream_statuses[i].name != NULL; ++i) {
+		lua_pushstring(L, popen_lua_stream_statuses[i].value);
+		lua_setfield(L, -2, popen_lua_stream_statuses[i].name);
+	}
+	lua_setfield(L, -2, "stream");
+
+	/* Process states. */
+	lua_newtable(L);
+	for (int i = 0; popen_lua_states[i].name != NULL; ++i) {
+		lua_pushstring(L, popen_lua_states[i].value);
+		lua_setfield(L, -2, popen_lua_states[i].name);
+	}
+	lua_setfield(L, -2, "state");
+}
+
+/* }}} */
diff --git a/src/lua/popen.h b/src/lua/popen.h
new file mode 100644
index 000000000..e23c5d7e5
--- /dev/null
+++ b/src/lua/popen.h
@@ -0,0 +1,44 @@
+#ifndef TARANTOOL_LUA_POPEN_H_INCLUDED
+#define TARANTOOL_LUA_POPEN_H_INCLUDED
+/*
+ * Copyright 2010-2020, 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_popen_init(struct lua_State *L);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_LUA_POPEN_H_INCLUDED */
diff --git a/test/app-tap/popen.test.lua b/test/app-tap/popen.test.lua
new file mode 100755
index 000000000..65fdc28ac
--- /dev/null
+++ b/test/app-tap/popen.test.lua
@@ -0,0 +1,605 @@
+#!/usr/bin/env tarantool
+
+local popen = require('popen')
+local ffi = require('ffi')
+local errno = require('errno')
+local fiber = require('fiber')
+local clock = require('clock')
+local tap = require('tap')
+local fun = require('fun')
+
+-- For process_is_alive().
+ffi.cdef([[
+    int
+    kill(pid_t pid, int signo);
+]])
+
+-- {{{ Helpers
+
+--
+-- Verify whether a process is alive.
+--
+local function process_is_alive(pid)
+    local rc = ffi.C.kill(pid, 0)
+    return rc == 0 or errno() ~= errno.ESRCH
+end
+
+--
+-- Verify whether a process is dead or not exist.
+--
+local function process_is_dead(pid)
+    return not process_is_alive(pid)
+end
+
+--
+-- Yield the current fiber until a condition becomes true or
+-- timeout (60 seconds) exceeds.
+--
+-- Don't use test-run's function to allow to run the test w/o
+-- test-run. It is often convenient during debugging.
+--
+local function wait_cond(func, ...)
+    local timeout = 60
+    local delay = 0.1
+
+    local deadline = clock.monotonic() + timeout
+    local res
+
+    while true do
+        res = {func(...)}
+        -- Success or timeout.
+        if res[1] or clock.monotonic() > deadline then break end
+        fiber.sleep(delay)
+    end
+
+    return unpack(res, 1, table.maxn(res))
+end
+
+-- }}}
+
+--
+-- Trivial echo output.
+--
+local function test_trivial_echo_output(test)
+    test:plan(6)
+
+    local script = 'printf "1 2 3 4 5"'
+    local exp_script_output = '1 2 3 4 5'
+
+    -- Start printf, wait it to finish, read the output and close
+    -- the handler.
+    local ph = popen.shell(script, 'r')
+    local pid = ph.pid
+    local exp_status = {
+        state = popen.state.EXITED,
+        exit_code = 0,
+    }
+    local status = ph:wait()
+    test:is_deeply(status, exp_status, 'verify process status')
+    local script_output = ph:read()
+    test:is(script_output, exp_script_output, 'verify script output')
+
+    -- Note: EPERM due to opts.group_signal and a zombie process
+    -- on Mac OS is not possible here, because we waited for the
+    -- process: it is not zombie, but not exists at all.
+    local res, err = ph:close()
+    test:is_deeply({res, err}, {true, nil}, 'close() successful')
+
+    -- Verify that the process is actually killed.
+    local is_dead = wait_cond(process_is_dead, pid)
+    test:ok(is_dead, 'the process is killed after close()')
+
+    -- Verify that :close() is idempotent.
+    local res, err = ph:close()
+    test:is_deeply({res, err}, {true, nil}, 'close() is idempotent')
+
+    -- Sending a signal using a closed handle gives an error.
+    local exp_err = 'popen: attempt to operate on a closed handle'
+    local ok, err = pcall(ph.signal, ph, popen.signal.SIGTERM)
+    test:is_deeply({ok, err.type, tostring(err)},
+                   {false, 'IllegalParams', exp_err},
+                   'signal() on closed handle gives an error')
+end
+
+--
+-- Test info and force killing of a child process.
+--
+local function test_kill_child_process(test)
+    test:plan(10)
+
+    -- Run and kill a process.
+    local script = 'while true; do sleep 10; done'
+    local ph = popen.shell(script, 'r')
+    local res, err = ph:kill()
+    test:is_deeply({res, err}, {true, nil}, 'kill() successful')
+
+    local exp_status = {
+        state = popen.state.SIGNALED,
+        signo = popen.signal.SIGKILL,
+        signame = 'SIGKILL',
+    }
+
+    -- Wait for the process termination, verify wait() return
+    -- values.
+    local status = ph:wait()
+    test:is_deeply(status, exp_status, 'wait() return values')
+
+    -- Verify status() return values for a terminated process.
+    test:is_deeply(ph.status, exp_status, 'status() return values')
+
+    -- Verify info for a terminated process.
+    local info = ph:info()
+    test:is(info.pid, nil, 'info.pid is nil')
+
+    local exp_command = ("sh -c '%s'"):format(script)
+    test:is(info.command, exp_command, 'verify info.script')
+
+    local exp_opts = {
+        stdin = popen.opts.INHERIT,
+        stdout = popen.opts.PIPE,
+        stderr = popen.opts.INHERIT,
+        shell = true,
+        setsid = true,
+        close_fds = true,
+        restore_signals = true,
+        group_signal = true,
+        keep_child = false,
+    }
+    test:is_deeply(info.opts, exp_opts, 'verify info.opts')
+
+    test:is_deeply(info.status, exp_status, 'verify info.status')
+
+    test:is(info.stdin, nil, 'verify info.stdin')
+    test:is(info.stdout, popen.stream.OPENED, 'verify info.stdout')
+    test:is(info.stderr, nil, 'verify info.stderr')
+
+    ph:close()
+end
+
+--
+-- Test that a loss handle does not leak (at least the
+-- corresponding process is killed).
+--
+local function test_gc(test)
+    test:plan(1)
+
+    -- Run a process, verify that it exists.
+    local script = 'while true; do sleep 10; done'
+    local ph = popen.shell(script, 'r')
+    local pid = ph.pid
+    assert(process_is_alive(pid))
+
+    -- Loss the handle.
+    ph = nil -- luacheck: no unused
+    collectgarbage()
+
+    -- Verify that the process is actually killed.
+    local is_dead = wait_cond(process_is_dead, pid)
+    test:ok(is_dead, 'the process is killed when the handle is collected')
+end
+
+--
+-- Simple read() / write() test.
+--
+local function test_read_write(test)
+    test:plan(8)
+
+    local payload = 'hello'
+
+    -- The script copies data from stdin to stdout.
+    local script = 'prompt=""; read -r prompt; printf "$prompt"'
+    local ph = popen.shell(script, 'rw')
+
+    -- Write to stdin, read from stdout.
+    local res, err = ph:write(payload .. '\n')
+    test:is_deeply({res, err}, {true, nil}, 'write() succeeds')
+    local ok = ph:shutdown({stdin = true})
+    test:ok(ok, 'shutdown() succeeds')
+    local res, err = ph:read()
+    test:is_deeply({res, err}, {payload, nil}, 'read() from stdout succeeds')
+
+    ph:close()
+
+    -- The script copies data from stdin to stderr.
+    local script = 'prompt=""; read -r prompt; printf "$prompt" 1>&2'
+    local ph = popen.shell(script, 'Rw')
+
+    -- Write to stdin, read from stderr.
+    local res, err = ph:write(payload .. '\n')
+    test:is_deeply({res, err}, {true, nil}, 'write() succeeds')
+    local ok = ph:shutdown({stdin = true})
+    test:ok(ok, 'shutdown() succeeds')
+    local res, err = ph:read({stderr = true})
+    test:is_deeply({res, err}, {payload, nil}, 'read() from stderr succeeds')
+
+    ph:close()
+
+    -- The same script: copy from stdin to stderr.
+    local script = 'prompt=""; read -r prompt; printf "$prompt" 1>&2'
+    local ph = popen.shell(script, 'Rw')
+
+    -- Ensure that read waits for data and does not return
+    -- prematurely.
+    local res_w, err_w
+    fiber.create(function()
+        fiber.sleep(0.1)
+        res_w, err_w = ph:write(payload .. '\n')
+        ph:shutdown({stdin = true})
+    end)
+    local res, err = ph:read({stderr = true})
+    test:is_deeply({res_w, err_w}, {true, nil}, 'write() succeeds')
+    test:is_deeply({res, err}, {payload, nil}, 'read() from stderr succeeds')
+
+    ph:close()
+end
+
+--
+-- Test timeouts: just wait for 0.1 second to elapse, then write
+-- data and re-read for sure.
+--
+local function test_read_timeout(test)
+    test:plan(3)
+
+    local payload = 'hello'
+    local script = 'prompt=""; read -r prompt; printf "$prompt"'
+    local ph = popen.shell(script, 'rw')
+
+    -- Read and get a timeout error.
+    local exp_err = 'timed out'
+    local res, err = ph:read({timeout = 0.1})
+    test:is_deeply({res, err.type, tostring(err)}, {nil, 'TimedOut', exp_err},
+                   'timeout error')
+
+    -- Write and read after the timeout error.
+    local res, err = ph:write(payload .. '\n')
+    test:is_deeply({res, err}, {true, nil}, 'write data')
+    ph:shutdown({stdin = true})
+    local res, err = ph:read()
+    test:is_deeply({res, err}, {payload, nil}, 'read data')
+
+    ph:close()
+end
+
+--
+-- Ensure that read() returns when some data is available (even if
+-- it is one byte).
+--
+local function test_read_chunk(test)
+    test:plan(1)
+
+    local payload = 'hello'
+    local script = ('printf "%s"; sleep 120'):format(payload)
+    local ph = popen.shell(script, 'r')
+
+    -- When a first byte is available, read() should return all
+    -- bytes arrived at the time.
+    local latch = fiber.channel(1)
+    local res, err
+    fiber.create(function()
+        res, err = ph:read()
+        latch:put(true)
+    end)
+    -- Wait 1 second at max.
+    latch:get(1)
+    test:is_deeply({res, err}, {payload, nil}, 'data available prior to EOF')
+
+    ph:close()
+end
+
+--
+-- Ensure that shutdown() closes asked streams: at least
+-- it is reflected in a handle information.
+--
+local function test_shutdown(test)
+    test:plan(9)
+
+    -- Verify std* statuses.
+    local function test_stream_statuses(test, ph, pstream, exp_pstream)
+        test:plan(6)
+        local info = ph:info()
+        for _, s in ipairs({'stdin', 'stdout', 'stderr'}) do
+            local exp_status = s == pstream and exp_pstream or nil
+            test:is(ph[s], exp_status, ('%s opened'):format(s))
+            test:is(info[s], exp_status, ('%s opened'):format(s))
+        end
+    end
+
+    -- Create, verify pstream status, shutdown it,
+    -- verify status again.
+    for _, pstream in ipairs({'stdin', 'stdout', 'stderr'}) do
+        local ph = popen.new({'/bin/true'}, {[pstream] = popen.opts.PIPE})
+
+        test:test(('%s before shutdown'):format(pstream),
+                  test_stream_statuses, ph, pstream,
+                  popen.stream.OPENED)
+
+        local ok = ph:shutdown({[pstream] = true})
+        test:ok(ok, ('shutdown({%s = true}) successful'):format(pstream))
+
+        test:test(('%s after shutdown'):format(pstream),
+                  test_stream_statuses, ph, pstream,
+                  popen.stream.CLOSED)
+
+        -- FIXME: Verify that read / write from pstream gives
+        -- certain error.
+
+        ph:close()
+    end
+end
+
+local function test_shell_invalid_args(test)
+    local function argerr(slot, _)
+        if slot == 1 then
+            return 'popen.shell: wrong parameter'
+        elseif slot == 2 then
+            return 'popen.shell: wrong parameter'
+        else
+            error('Invalid argument check')
+        end
+    end
+
+    -- 1st parameter.
+    local cases1 = {
+        [{nil}]                              = argerr(1, 'no value'),
+        [{true}]                             = argerr(1, 'boolean'),
+        [{false}]                            = argerr(1, 'boolean'),
+        [{0}]                                = argerr(1, 'number'),
+        -- A string is ok.
+        [{''}]                               = nil,
+        [{{}}]                               = argerr(1, 'table'),
+        [{popen.shell}]                      = argerr(1, 'function'),
+        [{io.stdin}]                         = argerr(1, 'userdata'),
+        [{coroutine.create(function() end)}] = argerr(1, 'thread'),
+        [{require('ffi').new('void *')}]     = argerr(1, 'cdata'),
+    }
+
+    -- 2nd parameter.
+    local cases2 = {
+        -- nil is ok ('wrR' is optional).
+        [{nil}]                              = nil,
+        [{true}]                             = argerr(2, 'boolean'),
+        [{false}]                            = argerr(2, 'boolean'),
+        [{0}]                                = argerr(2, 'number'),
+        -- A string is ok.
+        [{''}]                               = nil,
+        [{{}}]                               = argerr(2, 'table'),
+        [{popen.shell}]                      = argerr(2, 'function'),
+        [{io.stdin}]                         = argerr(2, 'userdata'),
+        [{coroutine.create(function() end)}] = argerr(2, 'thread'),
+        [{require('ffi').new('void *')}]     = argerr(2, 'cdata'),
+    }
+
+    test:plan(fun.iter(cases1):length() * 2 + fun.iter(cases2):length() * 2)
+
+    -- Call popen.shell() with
+    for args, err in pairs(cases1) do
+        local arg = unpack(args)
+        local ok, res = pcall(popen.shell, arg)
+        test:ok(not ok, ('command (ok): expected string, got %s')
+                        :format(type(arg)))
+        test:ok(res:match(err), ('command (err): expected string, got %s')
+                                :format(type(arg)))
+    end
+
+    for args, err in pairs(cases2) do
+        local arg = unpack(args)
+        local ok, res = pcall(popen.shell, 'printf test', arg)
+        test:ok(not ok, ('mode (ok): expected string, got %s')
+                        :format(type(arg)))
+        test:ok(res:match(err), ('mode (err): expected string, got %s')
+                                :format(type(arg)))
+    end
+end
+
+local function test_new_invalid_args(test)
+    local function argerr(arg, typename)
+        if arg == 'argv' then
+            return ('popen.new: wrong parameter "%s": expected table, got %s')
+                :format(arg, typename)
+        else
+            error('Invalid argument check')
+        end
+    end
+
+    -- 1st parameter.
+    local cases1 = {
+        [{nil}]                              = argerr('argv', 'nil'),
+        [{true}]                             = argerr('argv', 'boolean'),
+        [{false}]                            = argerr('argv', 'boolean'),
+        [{0}]                                = argerr('argv', 'number'),
+        [{''}]                               = argerr('argv', 'string'),
+        -- FIXME: A table is ok, but not an empty one.
+        [{{}}]                               = nil,
+        [{popen.shell}]                      = argerr('argv', 'function'),
+        [{io.stdin}]                         = argerr('argv', 'userdata'),
+        [{coroutine.create(function() end)}] = argerr('argv', 'thread'),
+        [{require('ffi').new('void *')}]     = argerr('argv', 'cdata'),
+    }
+
+    test:plan(fun.iter(cases1):length() * 2)
+
+    -- Call popen.new() with wrong "argv" parameter.
+    for args, err in pairs(cases1) do
+        local arg = unpack(args)
+        local ok, res = pcall(popen.new, arg)
+        test:ok(not ok, ('new argv (ok): expected table, got %s')
+                        :format(type(arg)))
+        test:ok(res:match(err), ('new argv (err): expected table, got %s')
+                                :format(type(arg)))
+    end
+end
+
+local function test_methods_on_closed_handle(test)
+    local methods = {
+        signal    = {popen.signal.SIGTERM},
+        terminate = {},
+        kill      = {},
+        wait      = {},
+        read      = {},
+        write     = {'hello'},
+        info      = {},
+        -- Close call is idempotent one.
+        close     = nil,
+    }
+
+    test:plan(fun.iter(methods):length() * 2)
+
+    local ph = popen.shell('printf "1 2 3 4 5"', 'r')
+    ph:close()
+
+    -- Call methods on a closed handle.
+    for method, args in pairs(methods) do
+        local ok, err = pcall(ph[method], ph, unpack(args))
+        test:ok(not ok, ('%s (ok) on closed handle'):format(method))
+        test:ok(err:match('popen: attempt to operate on a closed handle'),
+                ('%s (err) on closed handle'):format(method))
+    end
+end
+
+local function test_methods_on_invalid_handle(test)
+    local methods = {
+        signal    = {popen.signal.SIGTERM},
+        terminate = {},
+        kill      = {},
+        wait      = {},
+        read      = {},
+        write     = {'hello'},
+        info      = {},
+        close     = {},
+    }
+
+    test:plan(fun.iter(methods):length() * 4)
+
+    local ph = popen.shell('printf "1 2 3 4 5"', 'r')
+
+    -- Call methods without parameters.
+    for method in pairs(methods) do
+        local ok, err = pcall(ph[method])
+        test:ok(not ok, ('%s (ok) no handle and args'):format(method))
+        test:ok(err:match('Bad params, use: ph:' .. method),
+                ('%s (err) no handle and args'):format(method))
+    end
+
+    ph:close()
+
+    -- A table looks like a totally bad handler.
+    local bh = {}
+
+    -- Call methods on a bad handle.
+    for method, args in pairs(methods) do
+        local ok, err = pcall(ph[method], bh, unpack(args))
+        test:ok(not ok, ('%s (ok) on invalid handle'):format(method))
+        test:ok(err:match('Bad params, use: ph:' .. method),
+                ('%s (err) on invalid handle'):format(method))
+    end
+end
+
+local test = tap.test('popen')
+test:plan(11)
+
+test:test('trivial_echo_output', test_trivial_echo_output)
+test:test('kill_child_process', test_kill_child_process)
+test:test('gc', test_gc)
+test:test('read_write', test_read_write)
+test:test('read_timeout', test_read_timeout)
+test:test('read_chunk', test_read_chunk)
+test:test('test_shutdown', test_shutdown)
+test:test('shell_invalid_args', test_shell_invalid_args)
+test:test('new_invalid_args', test_new_invalid_args)
+test:test('methods_on_closed_handle', test_methods_on_closed_handle)
+test:test('methods_on_invalid_handle', test_methods_on_invalid_handle)
+
+-- Testing plan
+--
+-- FIXME: Implement this plan.
+--
+-- - api usage
+--   - new
+--     - no argv / nil argv
+--     - bad argv
+--       - wrong type
+--       - hole in the table (nil in a middle)
+--       - item
+--         - wrong type
+--       - zero size (w/ / w/o shell)
+--     - bad opts
+--       - wrong type
+--       - {stdin,stdout,stderr}
+--         - wrong type
+--         - wrong string value
+--       - env
+--         - wrong type
+--         - env item
+--           - wrong key type
+--           - wrong value type
+--           - '=' in key
+--           - '\0' in key
+--           - '=' in value
+--           - '\0' in value
+--       - (boolean options)
+--         - wrong type
+--         - conflicting options (!setsid && signal_group)
+--   - shell
+--     - bad handle
+--     - bad mode
+--   - signal
+--     - signal
+--       - wrong type
+--       - unknown value
+--   - read
+--     - FIXME: more cases
+--   - write
+--     - FIXME: more cases
+--   - __index
+--     - zero args (no even handle)
+--     - bad handle
+--     - FIXME: more cases
+--   - __serialize
+--     - zero args (no even handle)
+--     - bad handle
+--   - __gc
+--     - zero args (no even handle)
+--     - bad handle
+--
+-- - verify behaviour
+--   - popen.new: effect of boolean options
+--   - info: verify all four opts.std* actions
+--   - info: get both true and false for each opts.<...> boolean
+--     option
+--   - FiberIsCancelled is raised from read(), write() and wait()
+--
+-- - verify dubious code paths
+--   - popen.new
+--     - env: pass os.environ() with one replaced value
+--     - env: reallocation of env array
+--     - argv construction with and without shell option
+--     - std* actions actual behaviour
+--     - boolean opts: verify true, false and default values has expected
+--       effect (eps.: explicit false when it is default is not considered as
+--       true); at least look at then in :info()
+--   - read / write
+--     - write that needs several write() calls
+--     - feed large input over a process: write it, read in
+--       several chunks; process should terminate on EOF
+--     - child process die during read / write
+--     - boolean opts: verify true, false and default values has expected
+--       effect (eps.: explicit false when it is default is not considered as
+--       true)
+--     - FIXME: more cases
+--   - no extra fds are seen when close_fds is set
+--     verify with different std* options
+--
+-- - protect against inquisitive persons
+--   - ph.__gc(1)
+--   - ph.__gc(ph); ph = nil; collectgarbage()
+--
+-- - unsorted
+--   - test read / write after shutdown
+--   - shutdown
+--     - should be idempotent
+--     - fails w/o opts or w/ empty opts; i.e. at least one stream should be
+--       chosen
+--     - does not close any fds on a failure
+--     - fails when one piped and one non-piped fd are chosen
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 1/2] popen: always free resources in popen_delete()

[Cyrill Gorcunov]

On Sat, Apr 18, 2020 at 07:13:54AM +0300, Alexander Turenko wrote:
...
> 
> [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1329528
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Cyrill Gorcunov]

On Sat, Apr 18, 2020 at 07:13:55AM +0300, Alexander Turenko wrote:
> Co-developed-by: Cyrill Gorcunov <gorcunov@gmail.com>
> Co-developed-by: Igor Munkin <imun@tarantool.org>
> 
> Fixes #4031
> 

Brilliant!

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

Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Igor Munkin]

Sasha,

Thanks for the patch! I left several comments regarding a docbot request
with API description and usage, though they can be freely ignored.

On 18.04.20, Alexander Turenko wrote:
> Co-developed-by: Cyrill Gorcunov <gorcunov@gmail.com>
> Co-developed-by: Igor Munkin <imun@tarantool.org>
> 
> Fixes #4031
> 
> @TarantoolBot document
> Title: popen module
> 

<snipped>

> 
> Module functions
> ================
> 
> The `popen` module provides two functions to create that named popen
> object `popen.shell` which is the similar to libc `popen` syscall and

Typo: I guess a colon is missing here.

> `popen.new` to create popen object with more specific options.
> 
> `popen.shell(command[, mode]) -> handle, err`
> ---------------------------------------------
> 
> Execute a shell command.
> 
> @param command  a command to run, mandatory
> @param mode     communication mode, optional
>                 'w'    to use ph:write()
>                 'r'    to use ph:read()
>                 'R'    to use ph:read({stderr = true})
>                 nil    inherit parent's std* file descriptors
> 
> Several mode characters can be set together: 'rw', 'rRw', etc.
> 
> This function is just shortcut for popen.new({command}, opts)
> with opts.{shell,setsid,group_signal} set to `true` and
> and opts.{stdin,stdout,stderr} set based on `mode` parameter.
> 
> All std* streams are inherited from parent by default if it is
> not changed using mode: 'r' for stdout, 'R' for stderr, 'w' for
> stdin.
> 
> Raise an error on incorrect parameters:
> 
> - IllegalParams: incorrect type or value of a parameter.
> 
> Return a popen handle on success.
> 
> Return `nil, err` on a failure.
> @see popen.new() for possible reasons.
> 
> Example:
> 
>  | local popen = require('popen')
>  |
>  | local ph = popen.shell('date', 'r')
>  | local date = ph:read():rstrip()

Minor: IMHO <rstrip> call might lead to a misuse, considering there is
no word about it in description below. I guess you can drop it.

>  | ph:close()
>  | print(date)
> 
> Execute 'sh -c date' command, read the output and close the
> popen object.
> 
> `popen.new(argv[, opts]) -> handle, err`
> ----------------------------------------
> 
> Execute a child program in a new process.
> 
> @param argv  an array of a program to run with
>              command line options, mandatory;
>              absolute path to the program is required

Absolute path is not required when shell option is set to <true>.

> 
> @param opts  table of options
> 
> @param opts.stdin   action on STDIN_FILENO
> @param opts.stdout  action on STDOUT_FILENO
> @param opts.stderr  action on STDERR_FILENO
> 
> File descriptor actions:
> 
>     popen.opts.INHERIT  (== 'inherit') [default]
>                         inherit the fd from the parent
>     popen.opts.DEVNULL  (== 'devnull')
>                         open /dev/null on the fd
>     popen.opts.CLOSE    (== 'close')
>                         close the fd
>     popen.opts.PIPE     (== 'pipe')
>                         feed data from/to the fd to parent
>                         using a pipe
> 
> @param opts.env  an array of environment variables to
>                  be used inside a process;
>                  - when is not set then the current
>                    environment is inherited;
>                  - if set then the environment will be
>                    replaced
>                  - if set to an empty array then the
>                    environment will be dropped

Minor: I looks more convenient to sort the lines above the following
way:
* when is not set <...>
* if set to an empty table <...>
* if set to a non-empty table <...>

> 
> @param opts.shell            (boolean, default: false)
>        true                  run a child process via
>                              'sh -c "${opts.argv}"'
>        false                 call the executable directly
> 
> @param opts.setsid           (boolean, default: false)
>        true                  start executable inside a new
>                              session
>        false                 don't do that

Minor: I reword it in more clear way below:
| @param opts.setsid           (boolean, default: false)
|        true                  run the program in a new
|                              session
|        false                 run the program in the
|                              tarantool instance's
|                              session

> 
> @param opts.close_fds        (boolean, default: true)
>        true                  close all inherited fds from a
>                              parent
>        false                 don't do that
> 
> @param opts.restore_signals  (boolean, default: true)
>        true                  reset all signal actions
>                              modified in parent's process
>        false                 inherit changed actions
> 
> @param opts.group_signal     (boolean, default: false)
>        true                  send signal to a child process
>                              group (only when opts.setsid is
>                              enabled)
>        false                 send signal to a child process
>                              only
> 
> @param opts.keep_child       (boolean, default: false)
>        true                  don't send SIGKILL to a child
>                              process at freeing (by :close()
>                              or Lua GC)
>        false                 send SIGKILL to a child process
>                              (or a process group if
>                              opts.group_signal is enabled) at
>                              :close() or collecting of the
>                              handle by Lua GC
> 
> The returned handle is subject to garbage collection, but
> it may be freed explicitly using :close(). SIGKILL is sent
> to the child process at :close() / GC if it is alive and
> opts.keep_child is not set.

Minor: Let's adjust the terminology. What do you mean speaking about
"handle"? If it's a GC object, then there is no way to release it
manually or "explicitly", only via platform GC. If it's a GC object
*payload* then it's not managed by Lua GC and need to be explicitly
released via special method or the corresponding __gc metamethod.

> 
> It is recommended to use opts.setsid + opts.group_signal
> if a child process may spawn its own childs and they all
> should be killed together.
> 
> Note: A signal will not be sent if the child process is
> already dead: otherwise we might kill another process that
> occupies the same PID later. This means that if the child
> process dies before its own childs, the function will not
> send a signal to the process group even when opts.setsid and
> opts.group_signal are set.
> 
> Use os.environ() to pass copy of current environment with
> several replacements (see example 2 below).
> 
> Raise an error on incorrect parameters:
> 
> - IllegalParams: incorrect type or value of a parameter.
> - IllegalParams: group signal is set, while setsid is not.
> 
> Return a popen handle on success.
> 
> Return `nil, err` on a failure. Possible reasons:
> 
> - SystemError: dup(), fcntl(), pipe(), vfork() or close()
>                fails in the parent process.
> - SystemError: (temporary restriction) the parent process
>                has closed stdin, stdout or stderr.
> - OutOfMemory: unable to allocate the handle or a temporary
>                buffer.
> 
> Example 1:
> 
>  | local popen = require('popen')
>  |
>  | local ph = popen.new({'/bin/date'}, {
>  |     stdout = popen.opts.PIPE,
>  | })
>  | local date = ph:read():rstrip()
>  | ph:close()
>  | print(date) -- Thu 16 Apr 2020 01:40:56 AM MSK
> 
> Execute 'date' command, read the result and close the
> popen object.

Minor: Again, no mention about rstrip.

> 
> Example 2:
> 
>  | local popen = require('popen')
>  |
>  | local env = os.environ()
>  | env['FOO'] = 'bar'
>  |
>  | local ph = popen.new({'echo "${FOO}"'}, {
>  |     stdout = popen.opts.PIPE,
>  |     shell = true,
>  |     env = env,
>  | })
>  | local res = ph:read():rstrip()
>  | ph:close()
>  | print(res) -- bar
> 
> It is quite similar to the previous one, but sets the
> environment variable and uses shell builtin 'echo' to
> show it.
> 
> Example 3:
> 
>  | local popen = require('popen')
>  |
>  | local ph = popen.new({'echo hello >&2'}, { -- !!
>  |     stderr = popen.opts.PIPE,              -- !!
>  |     shell = true,
>  | })
>  | local res = ph:read({stderr = true}):rstrip()
>  | ph:close()
>  | print(res) -- hello
> 
> This example demonstrates how to capture child's stderr.
> 
> Example 4:
> 
>  | local function call_jq(stdin, filter)

Minor: <stdin> look a bit misleading. What about <input>, <json>, etc?

>  |     -- Start jq process, connect to stdin, stdout and stderr.
>  |     local jq_argv = {'/usr/bin/jq', '-M', '--unbuffered', filter}
>  |     local ph, err = popen.new(jq_argv, {
>  |         stdin = popen.opts.PIPE,
>  |         stdout = popen.opts.PIPE,
>  |         stderr = popen.opts.PIPE,
>  |     })
>  |     if ph == nil then return nil, err end
>  |
>  |     -- Write input data to child's stdin and send EOF.
>  |     local ok, err = ph:write(stdin)
>  |     if not ok then return nil, err end
>  |     ph:shutdown({stdin = true})
>  |
>  |     -- Read everything until EOF.
>  |     local chunks = {}
>  |     while true do
>  |         local chunk, err = ph:read()
>  |         if chunk == nil then
>  |             ph:close()
>  |             return nil, err
>  |         end
>  |         if chunk == '' then break end -- EOF
>  |         table.insert(chunks, chunk)
>  |     end
>  |
>  |     -- Read diagnostics from stderr if any.
>  |     local err = ph:read({stderr = true})
>  |     if err ~= '' then
>  |         ph:close()
>  |         return nil, err
>  |     end
>  |
>  |     -- Glue all chunks, strip trailing newline.

Side note: This comment is definitely needed near <rstrip> calls above.

>  |     return table.concat(chunks):rstrip()
>  | end
> 
> Demonstrates how to run a stream program (like `grep`, `sed`
> and so), write to its stdin and read from its stdout.
> 
> The example assumes that input data are small enough to fit
> a pipe buffer (typically 64 KiB, but depends on a platform
> and its configuration). It will stuck in :write() for large
> data. How to handle this case: call :read() in a loop in
> another fiber (start it before a first :write()).
> 
> If a process writes large text to stderr, it may fill out
> stderr pipe buffer and block in write(2, ...). So we need to
> read stderr too in another fiber to handle this case.

Typo: s/stderr too in another fiber/stderr in another fiber too/.

> 
> Handle methods
> ==============
> 
> `popen_handle:read([opts]) -> str, err`
> ---------------------------------------
> 
> Read data from a child peer.
> 
> @param handle        handle of a child process
> @param opts          an options table
> @param opts.stdout   whether to read from stdout, boolean
>                      (default: true)
> @param opts.stderr   whether to read from stderr, boolean
>                      (default: false)
> @param opts.timeout  time quota in seconds
>                      (default: 100 years)
> 
> Read data from stdout or stderr streams with @a timeout.
> By default it reads from stdout. Set @a opts.stderr to
> `true` to read from stderr.

If only <stderr> is set to true, is stdout output also captured?

> 
> Raise an error on incorrect parameters or when the fiber is
> cancelled:
> 
> - IllegalParams:    incorrect type or value of a parameter.
> - IllegalParams:    called on a closed handle.
> - IllegalParams:    opts.stdout and opts.stderr are set both
> - IllegalParams:    a requested IO operation is not supported
>                     by the handle (stdout / stderr is not
>                     piped).
> - IllegalParams:    attempt to operate on a closed file
>                     descriptor.
> - FiberIsCancelled: cancelled by an outside code.
> 
> Return a string on success, an empty string at EOF.
> 
> Return `nil, err` on a failure. Possible reasons:
> 
> - SocketError: an IO error occurs at read().

SocketError on pipe looks ridiculous to me. Maybe an IO one?

> - TimedOut:    @a timeout quota is exceeded.
> - OutOfMemory: no memory space for a buffer to read into.
> - LuajitError: ("not enough memory"): no memory space for
>                the Lua string.
> 

<snipped>

> 
> `popen_handle:shutdown(opts) -> true`

Minor: Considering the method description, <eof> looks more convenient
name here. On the other hand <eof> name might be misguiding one
considering feof(3) standart C function.

> ------------------------------------------
> 
> Close parent's ends of std* fds.
> 
> @param handle        handle of a child process
> @param opts          an options table
> @param opts.stdin    close parent's end of stdin, boolean
> @param opts.stdout   close parent's end of stdout, boolean
> @param opts.stderr   close parent's end of stderr, boolean
> 
> The main reason to use this function is to send EOF to
> child's stdin. However parent's end of stdout / stderr
> may be closed too.
> 
> The function does not fail on already closed fds (idempotence).
> However it fails on attempt to close the end of a pipe that was
> never exist. In other words, only those std* options that
> were set to popen.opts.PIPE at a handle creation may be used
> here (for popen.shell: 'r' corresponds to stdout, 'R' to stderr
> and 'w' to stdin).
> 
> The function does not close any fds on a failure: either all
> requested fds are closed or neither of them.
> 
> Example:
> 
>  | local popen = require('popen')
>  |
>  | local ph = popen.shell('sed s/foo/bar/', 'rw')
>  | ph:write('lorem foo ipsum')
>  | ph:shutdown({stdin = true})
>  | local res = ph:read()
>  | ph:close()
>  | print(res) -- lorem bar ipsum
> 
> Raise an error on incorrect parameters:
> 
> - IllegalParams:  an incorrect handle parameter.
> - IllegalParams:  called on a closed handle.
> - IllegalParams:  neither stdin, stdout nor stderr is choosen.
> - IllegalParams:  a requested IO operation is not supported
>                   by the handle (one of std* is not piped).
> 
> Return `true` on success.
> 

<snipped>

> 
> `popen_handle:info() -> res, err`

<err> is never returned.

> ---------------------------------
> 
> Return information about popen handle.
> 
> @param handle  a handle of a child process
> 
> Raise an error on incorrect parameters:
> 
> - IllegalParams: an incorrect handle parameter.
> - IllegalParams: called on a closed handle.
> 
> Return information about the handle in the following
> format:
> 
>     {
>         pid = <number> or <nil>,
>         command = <string>,
>         opts = <table>,
>         status = <table>,
>         stdin = one-of(
>             popen.stream.OPENED (== 'opened'),
>             popen.stream.CLOSED (== 'closed'),
>             nil,
>         ),
>         stdout = one-of(
>             popen.stream.OPENED (== 'opened'),
>             popen.stream.CLOSED (== 'closed'),
>             nil,
>         ),
>         stderr = one-of(
>             popen.stream.OPENED (== 'opened'),
>             popen.stream.CLOSED (== 'closed'),
>             nil,
>         ),
>     }
> 
> `pid` is a process id of the process when it is alive,
> otherwise `pid` is nil.
> 
> `command` is a concatenation of space separated arguments
> that were passed to execve(). Multiword arguments are quoted.
> Quotes inside arguments are not escaped.
> 
> `opts` is a table of handle options in the format of
> popen.new() `opts` parameter. `opts.env` is not shown here,
> because the environment variables map is not stored in a
> handle.
> 
> `status` is a table that represents a process status in the
> following format:
> 
>     {
>         state = one-of(
>             popen.state.ALIVE    (== 'alive'),
>             popen.state.EXITED   (== 'exited'),
>             popen.state.SIGNALED (== 'signaled'),
>         )
> 
>         -- Present when `state` is 'exited'.
>         exit_code = <number>,
> 
>         -- Present when `state` is 'signaled'.
>         signo = <number>,
>         signame = <string>,
>     }
> 
> `stdin`, `stdout`, `stderr` reflect status of parent's end
> of a piped stream. When a stream is not piped the field is
> not present (`nil`). When it is piped, the status may be
> one of the following:
> 
> - popen.stream.OPENED  (== 'opened')
> - popen.stream.CLOSED  (== 'closed')
> 
> The status may be changed from 'opened' to 'closed'
> by :shutdown({std... = true}) call.
> 
> Example 1 (tarantool console):
> 
>  | tarantool> require('popen').new({'/usr/bin/touch', '/tmp/foo'})
>  | ---
>  | - command: /usr/bin/touch /tmp/foo
>  |   status:
>  |     state: alive
>  |   opts:
>  |     stdout: inherit
>  |     stdin: inherit
>  |     group_signal: false
>  |     keep_child: false
>  |     close_fds: true
>  |     restore_signals: true
>  |     shell: false
>  |     setsid: false
>  |     stderr: inherit
>  |   pid: 9499
>  | ...
> 
> Example 2 (tarantool console):
> 
>  | tarantool> require('popen').shell('grep foo', 'wrR')
>  | ---
>  | - stdout: opened
>  |   command: sh -c 'grep foo'
>  |   stderr: opened
>  |   status:
>  |     state: alive
>  |   stdin: opened
>  |   opts:
>  |     stdout: pipe
>  |     stdin: pipe
>  |     group_signal: true
>  |     keep_child: false
>  |     close_fds: true
>  |     restore_signals: true
>  |     shell: true
>  |     setsid: true
>  |     stderr: pipe
>  |   pid: 10497
>  | ...
> 
> `popen_handle:wait() -> res, err`

<err> is never returned.

> ---------------------------------
> 
> Wait until a child process get exited or signaled.
> 
> @param handle  a handle of process to wait
> 
> Raise an error on incorrect parameters or when the fiber is
> cancelled:
> 
> - IllegalParams:    an incorrect handle parameter.
> - IllegalParams:    called on a closed handle.
> - FiberIsCancelled: cancelled by an outside code.
> 
> Return a process status table (the same as ph.status and
> ph.info().status). @see popen_handle:info() for the format
> of the table.
> 

<snipped>

> ---

<snipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Alexander Turenko]

Thanks for the careful proofreading!

Aside of changes below I changed `popen.stream.OPENED (== 'opened')` to
`popen.stream.OPEN (== 'open')` to match english grammar. Same for
statuses -> status (yep, 'statuses' is allowed in English, but it is
ugly).

Force-pushed Totktonada/gh-4031-popen-13-full-ci.

WBR, Alexander Turenko.

----

> > The `popen` module provides two functions to create that named popen
> > object `popen.shell` which is the similar to libc `popen` syscall and
> 
> Typo: I guess a colon is missing here.

Thanks! Fixed.

> > Example:
> > 
> >  | local popen = require('popen')
> >  |
> >  | local ph = popen.shell('date', 'r')
> >  | local date = ph:read():rstrip()
> 
> Minor: IMHO <rstrip> call might lead to a misuse, considering there is
> no word about it in description below. I guess you can drop it.

It is quite minor IMHO, but I don't mind.

Added the explanation to .shell() function description in the code and
the commit message:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 40f4343eb..8ae01b3f1 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1408,6 +1396,18 @@ err:
  *
  * Execute 'sh -c date' command, read the output and close the
  * popen object.
+ *
+ * Unix defines a text file as a sequence of lines, each ends
+ * with the newline symbol. The same convention is usually
+ * applied for a text output of a command (so when it is
+ * redirected to a file, the file will be correct).
+ *
+ * However internally an application usually operates on
+ * strings, which are NOT newline terminated (e.g. literals
+ * for error messages). The newline is usually added right
+ * before a string is written to the outside world (stdout,
+ * console or log). :rstrip() in the example above is shown
+ * for this sake.
  */
 static int
 lbox_popen_shell(struct lua_State *L)

I left .new() description intact: it should be described once and
.shell() going first in the documentation request (commit message).

> > Execute a child program in a new process.
> > 
> > @param argv  an array of a program to run with
> >              command line options, mandatory;
> >              absolute path to the program is required
> 
> Absolute path is not required when shell option is set to <true>.

Good catch!

Added to the comment in the code and to the commit message:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 2aa0a5851..bf06bbfa7 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1114,6 +1114,7 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  * @param argv  an array of a program to run with
  *              command line options, mandatory;
  *              absolute path to the program is required
+ *              when @a opts.shell is false (default)
  *
  * @param opts  table of options
  *

> > @param opts.env  an array of environment variables to
> >                  be used inside a process;
> >                  - when is not set then the current
> >                    environment is inherited;
> >                  - if set then the environment will be
> >                    replaced
> >                  - if set to an empty array then the
> >                    environment will be dropped
> 
> Minor: I looks more convenient to sort the lines above the following
> way:
> * when is not set <...>
> * if set to an empty table <...>
> * if set to a non-empty table <...>

I don't mind.

Reordered in the code and the commit message.

Also I rephrased the description a bit to highlight the fact that the
parameter is key-value mapping, not an array.

diff --git a/src/lua/popen.c b/src/lua/popen.c
index bf06bbfa7..f17e3faf1 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1134,14 +1134,16 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *                         feed data from/to the fd to parent
  *                         using a pipe
  *
- * @param opts.env  an array of environment variables to
- *                  be used inside a process;
+ * @param opts.env  a table of environment variables to
+ *                  be used inside a process; key is a
+ *                  variable name, value is a variable
+ *                  value.
  *                  - when is not set then the current
  *                    environment is inherited;
+ *                  - if set to an empty table then the
+ *                    environment will be dropped
  *                  - if set then the environment will be
  *                    replaced
- *                  - if set to an empty array then the
- *                    environment will be dropped
  *
  * @param opts.shell            (boolean, default: false)
  *        true                  run a child process via

> > @param opts.setsid           (boolean, default: false)
> >        true                  start executable inside a new
> >                              session
> >        false                 don't do that
> 
> Minor: I reword it in more clear way below:
> | @param opts.setsid           (boolean, default: false)
> |        true                  run the program in a new
> |                              session
> |        false                 run the program in the
> |                              tarantool instance's
> |                              session

So I would add "...and process group". Changed:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index f17e3faf1..ec8c95662 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1151,9 +1151,11 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *        false                 call the executable directly
  *
  * @param opts.setsid           (boolean, default: false)
- *        true                  start executable inside a new
+ *        true                  run the program in a new
  *                              session
- *        false                 don't do that
+ *        false                 run the program in the
+ *                              tarantool instance's
+ *                              session and process group
  *
  * @param opts.close_fds        (boolean, default: true)
  *        true                  close all inherited fds from a

> > The returned handle is subject to garbage collection, but
> > it may be freed explicitly using :close(). SIGKILL is sent
> > to the child process at :close() / GC if it is alive and
> > opts.keep_child is not set.
> 
> Minor: Let's adjust the terminology. What do you mean speaking about
> "handle"? If it's a GC object, then there is no way to release it
> manually or "explicitly", only via platform GC. If it's a GC object
> *payload* then it's not managed by Lua GC and need to be explicitly
> released via special method or the corresponding __gc metamethod.

My intention is to give two ideas here:

* Feel free to use explicit :close() or lean on Lua GC.
* :close() / GC handler kills the child process.

But being strict in terms and wording is goal for me too, so I reword
the paragraph to eliminate phrases, which requires a GC object / a GC
object payload as an object / a subject.

Changed in the code and the commit:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index ec8c95662..31b4bc2b0 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1184,10 +1184,11 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *                              :close() or collecting of the
  *                              handle by Lua GC
  *
- * The returned handle is subject to garbage collection, but
- * it may be freed explicitly using :close(). SIGKILL is sent
- * to the child process at :close() / GC if it is alive and
- * opts.keep_child is not set.
+ * The returned handle provides :close() method to explicitly
+ * release all occupied resources (including the child process
+ * itself if @a opts.keep_child is not set). However if the
+ * method is not called for a handle during its lifetime, the
+ * same freeing actions will be triggered by Lua GC.
  *
  * It is recommended to use opts.setsid + opts.group_signal
  * if a child process may spawn its own childs and they all

> > Example 1:
> > 
> >  | local popen = require('popen')
> >  |
> >  | local ph = popen.new({'/bin/date'}, {
> >  |     stdout = popen.opts.PIPE,
> >  | })
> >  | local date = ph:read():rstrip()
> >  | ph:close()
> >  | print(date) -- Thu 16 Apr 2020 01:40:56 AM MSK
> > 
> > Execute 'date' command, read the result and close the
> > popen object.
> 
> Minor: Again, no mention about rstrip.

I think once is enough. Added the popen.shell() description.

> > Example 4:
> > 
> >  | local function call_jq(stdin, filter)
> 
> Minor: <stdin> look a bit misleading. What about <input>, <json>, etc?

Okay, 'input'.

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 31b4bc2b0..bf79a5d29 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1270,7 +1270,7 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *
  * Example 4:
  *
- *  | local function call_jq(stdin, filter)
+ *  | local function call_jq(input, filter)
  *  |     -- Start jq process, connect to stdin, stdout and stderr.
  *  |     local jq_argv = {'/usr/bin/jq', '-M', '--unbuffered', filter}
  *  |     local ph, err = popen.new(jq_argv, {
@@ -1281,7 +1281,7 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *  |     if ph == nil then return nil, err end
  *  |
  *  |     -- Write input data to child's stdin and send EOF.
- *  |     local ok, err = ph:write(stdin)
+ *  |     local ok, err = ph:write(input)
  *  |     if not ok then return nil, err end
  *  |     ph:shutdown({stdin = true})
  *  |

> >  |     -- Read diagnostics from stderr if any.
> >  |     local err = ph:read({stderr = true})
> >  |     if err ~= '' then
> >  |         ph:close()
> >  |         return nil, err
> >  |     end
> >  |
> >  |     -- Glue all chunks, strip trailing newline.
> 
> Side note: This comment is definitely needed near <rstrip> calls above.

Added once, to popen.shell() example:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index bf79a5d29..6cfb268a1 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1395,9 +1395,16 @@ err:
  *
  *  | local popen = require('popen')
  *  |
+ *  | -- Run the program and save its handle.
  *  | local ph = popen.shell('date', 'r')
+ *  |
+ *  | -- Read program's output, strip trailing newline.
  *  | local date = ph:read():rstrip()
+ *  |
+ *  | -- Free resources. The process is killed (but 'date'
+ *  | -- exits itself anyway).
  *  | ph:close()
+ *  |
  *  | print(date)
  *
  * Execute 'sh -c date' command, read the output and close the

> > If a process writes large text to stderr, it may fill out
> > stderr pipe buffer and block in write(2, ...). So we need to
> > read stderr too in another fiber to handle this case.
> 
> Typo: s/stderr too in another fiber/stderr in another fiber too/.

I want to avoid possible understanding as 'read stderr here and in
another fiber too'. I can just remove 'too'. I also changed 'block' to
'stuck':

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 6cfb268a1..6835286c7 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1318,8 +1318,8 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  * another fiber (start it before a first :write()).
  *
  * If a process writes large text to stderr, it may fill out
- * stderr pipe buffer and block in write(2, ...). So we need to
- * read stderr too in another fiber to handle this case.
+ * stderr pipe buffer and stuck in write(2, ...). So we need
+ * to read stderr in a separate fiber to handle this case.
  */
 static int
 lbox_popen_new(struct lua_State *L)

> > Read data from stdout or stderr streams with @a timeout.
> > By default it reads from stdout. Set @a opts.stderr to
> > `true` to read from stderr.
> 
> If only <stderr> is set to true, is stdout output also captured?

Nope. There is an error about it in the list below, but, yep, it worth
to highlight it. Added the note:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 6835286c7..a071065b4 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1647,6 +1647,9 @@ lbox_popen_wait(struct lua_State *L)
  * By default it reads from stdout. Set @a opts.stderr to
  * `true` to read from stderr.
  *
+ * It is not possible to read from stdout and stderr both in
+ * one call. Set either @a opts.stdout or @a opts.stderr.
+ *
  * Raise an error on incorrect parameters or when the fiber is
  * cancelled:
  *

> > Return `nil, err` on a failure. Possible reasons:
> > 
> > - SocketError: an IO error occurs at read().
> 
> SocketError on pipe looks ridiculous to me. Maybe an IO one?

It comes from coio.cc. It would be good to refactor it someday, but not
tonight :)

> > `popen_handle:shutdown(opts) -> true`
> 
> Minor: Considering the method description, <eof> looks more convenient
> name here. On the other hand <eof> name might be misguiding one
> considering feof(3) standart C function.

:close() for ph.{stdin,stdout,stderr} would be ideal, but we don't have
something like separate read / write streams now. We have bidirectional
handle for reading and writing, so the nearest counterpart would be
socket's shutdown().

More on this: closing of stdin is EOF for a child, but closing of stdout
/ stderr will lead to SIGPIPE (+EPIPE in errno) at write() in the child.

I think the name we have now is good.

> > `popen_handle:info() -> res, err`
> 
> <err> is never returned.

Good catch!

Fixed.

> > `popen_handle:wait() -> res, err`
> 
> <err> is never returned.

Thanks!

Fixed.

Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Igor Munkin]

Sasha,

Thanks for the patch! I left several nits, please consider them.
Otherwise, LGTM.

On 18.04.20, Alexander Turenko wrote:
> Co-developed-by: Cyrill Gorcunov <gorcunov@gmail.com>
> Co-developed-by: Igor Munkin <imun@tarantool.org>
> 
> Fixes #4031

<snipped>

> ---
>  src/CMakeLists.txt          |    1 +
>  src/lua/init.c              |    2 +
>  src/lua/popen.c             | 2457 +++++++++++++++++++++++++++++++++++
>  src/lua/popen.h             |   44 +
>  test/app-tap/popen.test.lua |  605 +++++++++
>  5 files changed, 3109 insertions(+)
>  create mode 100644 src/lua/popen.c
>  create mode 100644 src/lua/popen.h
>  create mode 100755 test/app-tap/popen.test.lua
> 

<snipped>

> diff --git a/src/lua/popen.c b/src/lua/popen.c
> new file mode 100644
> index 000000000..40f4343eb
> --- /dev/null
> +++ b/src/lua/popen.c
> @@ -0,0 +1,2457 @@

<snipped>

> +
> +/* {{{ General-purpose Lua helpers */
> +
> +/**
> + * Extract a string from the Lua stack.
> + *
> + * Return (const char *) if so, otherwise return NULL.
> + *
> + * Unlike luaL_checkstring() it accepts only a string and does not
> + * accept a number.
> + *
> + * Unlike luaL_checkstring() it does not raise an error, but
> + * returns NULL when a type is not what is excepted.
> + */
> +static const char *
> +luaL_check_string_strict(struct lua_State *L, int idx, size_t *len_ptr)

Minor: I propose the following names: <luaL_tolstring_strict>,
<luaL_checkstring_nothrow> or <luaL_checkstring_noxc>.

Side note: Sorry, but I can't pass by it. Sasha, we both know each
other's opinion regarding luaL_* prefix usage, and I'm not going to
argue about it within this review. I'll try to live with it since it's
a static one, but I see no reasons to not try to name it other way.

> +{
> +	if (lua_type(L, idx) != LUA_TSTRING)
> +		return NULL;
> +
> +	const char *res = lua_tolstring(L, idx, len_ptr);
> +	assert(res != NULL);
> +	return res;
> +}
> +

<snipped>

> +
> +/**
> + * Helper for luaT_push_string_noxc().
> + */
> +static int
> +luaT_push_string_noxc_wrapper(struct lua_State *L)
> +{
> +	char *str = (char *)lua_topointer(L, 1);
> +	size_t len = lua_tointeger(L, 2);
> +	lua_pushlstring(L, str, len);
> +	return 1;
> +}
> +
> +/**
> + * Push a string to the Lua stack.
> + *
> + * Return 0 at success, -1 at failure and set a diag.
> + *
> + * Possible errors:
> + *
> + * - LuajitError ("not enough memory"): no memory space for the
> + *   Lua string.
> + */
> +static int
> +luaT_push_string_noxc(struct lua_State *L, char *str, size_t len)
> +{
> +	lua_pushcfunction(L, luaT_push_string_noxc_wrapper);
> +	lua_pushlightuserdata(L, str);
> +	lua_pushinteger(L, len);
> +	return luaT_call(L, 2, 1);
> +}

Minor: IMHO luaT_pushstring_* is more Lua-like naming.

> +
> +/* }}} */
> +
> +/* {{{ Popen handle userdata manipulations */
> +
> +/**
> + * Extract popen handle from the Lua stack.
> + *
> + * Return NULL in case of unexpected type.
> + */
> +static struct popen_handle *
> +luaT_check_popen_handle(struct lua_State *L, int idx, bool *is_closed_ptr)
> +{
> +	struct popen_handle **handle_ptr =
> +		luaL_testudata(L, idx, popen_handle_uname);
> +	bool is_closed = false;
> +
> +	if (handle_ptr == NULL) {
> +		handle_ptr = luaL_testudata(L, idx, popen_handle_closed_uname);
> +		is_closed = true;
> +	}
> +
> +	if (handle_ptr == NULL)
> +		return NULL;

Minor: If NULL is returned, then is_closed_ptr payload is undefined. It
would be nice to mention it in contract above.

> +	assert(*handle_ptr != NULL);
> +
> +	if (is_closed_ptr != NULL)

Minor: Considering function usage this check is excess.

> +		*is_closed_ptr = is_closed;
> +	return *handle_ptr;
> +}
> +

<snipped>

> +
> +/* }}} */
> +
> +/* {{{ Push popen handle info to the Lua stack */
> +

<snipped>

> +
> +/**
> + * Push popen options as a Lua table.
> + *
> + * Environment variables are not stored in a popen handle and so
> + * missed here.
> + */
> +static int
> +luaT_push_popen_opts(struct lua_State *L, unsigned int flags)
> +{
> +	lua_createtable(L, 0, 8);
> +
> +	luaT_push_popen_stdX_action(L, STDIN_FILENO, flags);
> +	lua_setfield(L, -2, "stdin");
> +
> +	luaT_push_popen_stdX_action(L, STDOUT_FILENO, flags);
> +	lua_setfield(L, -2, "stdout");
> +
> +	luaT_push_popen_stdX_action(L, STDERR_FILENO, flags);
> +	lua_setfield(L, -2, "stderr");

Minor: This can be turned into a loop.

> +
> +	/* env is skipped */
> +
> +	lua_pushboolean(L, (flags & POPEN_FLAG_SHELL) != 0);
> +	lua_setfield(L, -2, "shell");
> +
> +	lua_pushboolean(L, (flags & POPEN_FLAG_SETSID) != 0);
> +	lua_setfield(L, -2, "setsid");
> +
> +	lua_pushboolean(L, (flags & POPEN_FLAG_CLOSE_FDS) != 0);
> +	lua_setfield(L, -2, "close_fds");
> +
> +	lua_pushboolean(L, (flags & POPEN_FLAG_RESTORE_SIGNALS) != 0);
> +	lua_setfield(L, -2, "restore_signals");
> +
> +	lua_pushboolean(L, (flags & POPEN_FLAG_GROUP_SIGNAL) != 0);
> +	lua_setfield(L, -2, "group_signal");
> +
> +	lua_pushboolean(L, (flags & POPEN_FLAG_KEEP_CHILD) != 0);
> +	lua_setfield(L, -2, "keep_child");

Minor: This can be also turned into a loop.

> +
> +	return 1;
> +}
> +

<snipped>

> +
> +/* }}} */
> +
> +/* {{{ Parameter parsing */
> +

<snipped>

> +
> +/**
> + * Parse popen.new() "opts" parameter.
> + *
> + * Prerequisite: @a opts should be zero filled.
> + *
> + * Result: @a opts structure is filled.
> + *
> + * Raise an error in case of the incorrect parameter.
> + *
> + * Return 0 at success. Allocates opts->env on @a region if
> + * needed. @see luaT_popen_parse_env() for details how to
> + * free it.
> + *
> + * Return -1 in case of an allocation error and set a diag
> + * (OutOfMemory).
> + */
> +static int
> +luaT_popen_parse_opts(struct lua_State *L, int idx, struct popen_opts *opts,
> +		      struct region *region)

Minor: I rewrote it a bit, consider the diff below:

================================================================================

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 40f4343eb..f5d651401 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -869,24 +869,19 @@ luaT_popen_parse_opts(struct lua_State *L, int idx, struct popen_opts *opts,
 
 	/* Parse options. */
 	if (lua_type(L, idx) == LUA_TTABLE) {
-		lua_getfield(L, idx, "stdin");
-		if (! lua_isnil(L, -1)) {
-			luaT_popen_parse_stdX(L, -1, STDIN_FILENO,
-					      &opts->flags);
-		}
-		lua_pop(L, 1);
 
-		lua_getfield(L, idx, "stdout");
-		if (! lua_isnil(L, -1))
-			luaT_popen_parse_stdX(L, -1, STDOUT_FILENO,
-					      &opts->flags);
-		lua_pop(L, 1);
+#define setstdX(L, idx, stream, fileno) do {                        \
+	lua_getfield(L, idx, stream);                               \
+	if (!lua_isnil(L, -1))                                      \
+		luaT_popen_parse_stdX(L, -1, fileno, &opts->flags); \
+	lua_pop(L, 1);                                              \
+} while(0)
 
-		lua_getfield(L, idx, "stderr");
-		if (! lua_isnil(L, -1))
-			luaT_popen_parse_stdX(L, -1, STDERR_FILENO,
-					      &opts->flags);
-		lua_pop(L, 1);
+		setstdX(L, idx, "stdin", STDIN_FILENO);
+		setstdX(L, idx, "stdout", STDOUT_FILENO);
+		setstdX(L, idx, "stderr", STDERR_FILENO);
+
+#undef setstdX
 
 		lua_getfield(L, idx, "env");
 		if (! lua_isnil(L, -1)) {
@@ -896,84 +891,30 @@ luaT_popen_parse_opts(struct lua_State *L, int idx, struct popen_opts *opts,
 		}
 		lua_pop(L, 1);
 
-		lua_getfield(L, idx, "shell");
-		if (! lua_isnil(L, -1)) {
-			if (lua_type(L, -1) != LUA_TBOOLEAN)
-				luaT_popen_param_type_error(L, -1, "popen.new",
-							    "opts.shell",
-							    "boolean or nil");
-			if (lua_toboolean(L, -1) == 0)
-				opts->flags &= ~POPEN_FLAG_SHELL;
-			else
-				opts->flags |= POPEN_FLAG_SHELL;
-		}
-		lua_pop(L, 1);
-
-		lua_getfield(L, idx, "setsid");
-		if (! lua_isnil(L, -1)) {
-			if (lua_type(L, -1) != LUA_TBOOLEAN)
-				luaT_popen_param_type_error(L, -1, "popen.new",
-							    "opts.setsid",
-							    "boolean or nil");
-			if (lua_toboolean(L, -1) == 0)
-				opts->flags &= ~POPEN_FLAG_SETSID;
-			else
-				opts->flags |= POPEN_FLAG_SETSID;
-		}
-		lua_pop(L, 1);
-
-		lua_getfield(L, idx, "close_fds");
-		if (! lua_isnil(L, -1)) {
-			if (lua_type(L, -1) != LUA_TBOOLEAN)
-				luaT_popen_param_type_error(L, -1, "popen.new",
-							    "opts.close_fds",
-							    "boolean or nil");
-			if (lua_toboolean(L, -1) == 0)
-				opts->flags &= ~POPEN_FLAG_CLOSE_FDS;
-			else
-				opts->flags |= POPEN_FLAG_CLOSE_FDS;
-		}
-		lua_pop(L, 1);
-
-		lua_getfield(L, idx, "restore_signals");
-		if (! lua_isnil(L, -1)) {
-			if (lua_type(L, -1) != LUA_TBOOLEAN)
-				luaT_popen_param_type_error(
-					L, -1, "popen.new",
-					"opts.restore_signals",
-					"boolean or nil");
-			if (lua_toboolean(L, -1) == 0)
-				opts->flags &= ~POPEN_FLAG_RESTORE_SIGNALS;
-			else
-				opts->flags |= POPEN_FLAG_RESTORE_SIGNALS;
-		}
-		lua_pop(L, 1);
+#define setflag(L, idx, key, flag) do {                                 \
+	lua_getfield(L, idx, key);                                      \
+	if (!lua_isnil(L, -1)) {                                        \
+		if (lua_type(L, -1) != LUA_TBOOLEAN)                    \
+			luaT_popen_param_type_error(L, -1, "popen.new", \
+						    "opts." key,        \
+						    "boolean or nil");  \
+		if (lua_toboolean(L, -1) == 0)                          \
+			opts->flags &= ~flag;                           \
+		else                                                    \
+			opts->flags |= flag;                            \
+	}                                                               \
+	lua_pop(L, 1);                                                  \
+} while(0)
+
+		setflag(L, idx, "shell", POPEN_FLAG_SHELL);
+		setflag(L, idx, "setsid", POPEN_FLAG_SETSID);
+		setflag(L, idx, "close_fds", POPEN_FLAG_CLOSE_FDS);
+		setflag(L, idx, "restore_signals", POPEN_FLAG_RESTORE_SIGNALS);
+		setflag(L, idx, "group_signal", POPEN_FLAG_GROUP_SIGNAL);
+		setflag(L, idx, "keep_child", POPEN_FLAG_KEEP_CHILD);
+
+#undef setflag
 
-		lua_getfield(L, idx, "group_signal");
-		if (! lua_isnil(L, -1)) {
-			if (lua_type(L, -1) != LUA_TBOOLEAN)
-				luaT_popen_param_type_error(L, -1, "popen.new",
-							    "opts.group_signal",
-							    "boolean or nil");
-			if (lua_toboolean(L, -1) == 0)
-				opts->flags &= ~POPEN_FLAG_GROUP_SIGNAL;
-			else
-				opts->flags |= POPEN_FLAG_GROUP_SIGNAL;
-		}
-		lua_pop(L, 1);
-
-		lua_getfield(L, idx, "keep_child");
-		if (! lua_isnil(L, -1)) {
-			if (lua_type(L, -1) != LUA_TBOOLEAN)
-				luaT_popen_param_type_error(L, -1, "popen.new",
-							    "opts.keep_child",
-							    "boolean or nil");
-			if (lua_toboolean(L, -1) == 0)
-				opts->flags &= ~POPEN_FLAG_KEEP_CHILD;
-			else
-				opts->flags |= POPEN_FLAG_KEEP_CHILD;
-		}
-		lua_pop(L, 1);
 	}
 
 	return 0;

================================================================================

> +{
> +	/*
> +	 * Default flags: inherit std*, close other fds,
> +	 * restore signals.
> +	 */
> +	opts->flags = POPEN_FLAG_NONE		|
> +		POPEN_FLAG_CLOSE_FDS		|
> +		POPEN_FLAG_RESTORE_SIGNALS;
> +
> +	/* Parse options. */
> +	if (lua_type(L, idx) == LUA_TTABLE) {
> +		lua_getfield(L, idx, "stdin");
> +		if (! lua_isnil(L, -1)) {
> +			luaT_popen_parse_stdX(L, -1, STDIN_FILENO,
> +					      &opts->flags);
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, idx, "stdout");
> +		if (! lua_isnil(L, -1))
> +			luaT_popen_parse_stdX(L, -1, STDOUT_FILENO,
> +					      &opts->flags);
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, idx, "stderr");
> +		if (! lua_isnil(L, -1))
> +			luaT_popen_parse_stdX(L, -1, STDERR_FILENO,
> +					      &opts->flags);
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, idx, "env");
> +		if (! lua_isnil(L, -1)) {
> +			opts->env = luaT_popen_parse_env(L, -1, region);
> +			if (opts->env == NULL)
> +				return -1;
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, idx, "shell");
> +		if (! lua_isnil(L, -1)) {
> +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> +				luaT_popen_param_type_error(L, -1, "popen.new",
> +							    "opts.shell",
> +							    "boolean or nil");
> +			if (lua_toboolean(L, -1) == 0)
> +				opts->flags &= ~POPEN_FLAG_SHELL;
> +			else
> +				opts->flags |= POPEN_FLAG_SHELL;
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, idx, "setsid");
> +		if (! lua_isnil(L, -1)) {
> +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> +				luaT_popen_param_type_error(L, -1, "popen.new",
> +							    "opts.setsid",
> +							    "boolean or nil");
> +			if (lua_toboolean(L, -1) == 0)
> +				opts->flags &= ~POPEN_FLAG_SETSID;
> +			else
> +				opts->flags |= POPEN_FLAG_SETSID;
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, idx, "close_fds");
> +		if (! lua_isnil(L, -1)) {
> +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> +				luaT_popen_param_type_error(L, -1, "popen.new",
> +							    "opts.close_fds",
> +							    "boolean or nil");
> +			if (lua_toboolean(L, -1) == 0)
> +				opts->flags &= ~POPEN_FLAG_CLOSE_FDS;
> +			else
> +				opts->flags |= POPEN_FLAG_CLOSE_FDS;
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, idx, "restore_signals");
> +		if (! lua_isnil(L, -1)) {
> +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> +				luaT_popen_param_type_error(
> +					L, -1, "popen.new",
> +					"opts.restore_signals",
> +					"boolean or nil");
> +			if (lua_toboolean(L, -1) == 0)
> +				opts->flags &= ~POPEN_FLAG_RESTORE_SIGNALS;
> +			else
> +				opts->flags |= POPEN_FLAG_RESTORE_SIGNALS;
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, idx, "group_signal");
> +		if (! lua_isnil(L, -1)) {
> +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> +				luaT_popen_param_type_error(L, -1, "popen.new",
> +							    "opts.group_signal",
> +							    "boolean or nil");
> +			if (lua_toboolean(L, -1) == 0)
> +				opts->flags &= ~POPEN_FLAG_GROUP_SIGNAL;
> +			else
> +				opts->flags |= POPEN_FLAG_GROUP_SIGNAL;
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, idx, "keep_child");
> +		if (! lua_isnil(L, -1)) {
> +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> +				luaT_popen_param_type_error(L, -1, "popen.new",
> +							    "opts.keep_child",
> +							    "boolean or nil");
> +			if (lua_toboolean(L, -1) == 0)
> +				opts->flags &= ~POPEN_FLAG_KEEP_CHILD;
> +			else
> +				opts->flags |= POPEN_FLAG_KEEP_CHILD;
> +		}
> +		lua_pop(L, 1);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * Parse popen.new() "argv" parameter.
> + *
> + * Prerequisite: opts->flags & POPEN_FLAG_SHELL should be
> + * the same in a call of this function and when paired
> + * popen_new() is invoked.
> + *
> + * Raise an error in case of the incorrect parameter.
> + *
> + * Return 0 at success. Sets opts->args and opts->nr_args.

Typo: s/args/argv/g.

Minor: argc looks to be a good complement to argv field.

> + * Allocates opts->argv on @a region (@see
> + * luaT_popen_parse_env() for details how to free it).
> + *
> + * Return -1 in case of an allocation error and set a diag
> + * (OutOfMemory).
> + */
> +static int
> +luaT_popen_parse_argv(struct lua_State *L, int idx, struct popen_opts *opts,
> +		      struct region *region)
> +{
> +	size_t region_svp = region_used(region);
> +	size_t argv_len = lua_objlen(L, idx);

There are no guarantees the table doesn't have gaps. Consider the
comment from lj_tab.c[1] and the following example:
| $ ./luajit
| LuaJIT 2.1.0-beta3 -- Copyright (C) 2005-2017 Mike Pall.
| http://luajit.org/
| JIT: ON SSE2 SSE3 SSE4.1 BMI2 fold cse dce fwd dse narrow loop abc sink
| fuse
| > print(#{ 'echo', 'QQ', nil, 'rm' })
| 4
Thereby the safe way to parse popen argv is iterating by numeric indices
until lua_rawgeti yields nil.

> +
> +	/* ["sh", "-c", ]..., NULL. */
> +	opts->nr_argv = argv_len + 1;
> +	if (opts->flags & POPEN_FLAG_SHELL)
> +		opts->nr_argv += 2;
> +
> +	size_t size = sizeof(char *) * opts->nr_argv;
> +	opts->argv = region_alloc(region, size);
> +	if (opts->argv == NULL) {
> +		diag_set(OutOfMemory, size, "region_alloc", "argv");
> +		return -1;
> +	}
> +
> +	const char **to = (const char **)opts->argv;
> +	if (opts->flags & POPEN_FLAG_SHELL) {
> +		opts->argv[0] = NULL;
> +		opts->argv[1] = NULL;

Minor: Please leave a comment why these slots are NULLs. I don't get it
from the given context.

> +		to += 2;
> +	}
> +
> +	for (size_t i = 0; i < argv_len; ++i) {
> +		lua_rawgeti(L, idx, i + 1);
> +		const char *arg = luaL_check_string_strict(L, -1, NULL);

Minor: OK, now I see. Please leave a comment here regarding the gap
check and benefits we have omitting dynamic reallocation, as we
discussed.

> +		if (arg == NULL) {
> +			region_truncate(region, region_svp);
> +			return luaT_popen_array_elem_type_error(
> +				L, -1, "popen.new", "argv", i + 1, "string");
> +		}
> +		*to++ = arg;
> +		lua_pop(L, 1);
> +	}
> +	*to++ = NULL;
> +	assert((const char **)opts->argv + opts->nr_argv == to);
> +
> +	return 0;
> +}
> +

<snipped>

> +
> +/* }}} */
> +
> +/* {{{ Lua API functions and methods */
> +

<snipped>

> +
> +/**
> + * Send SIGTERM signal to a child process.
> + *
> + * @param handle  a handle carries child process to terminate
> + *
> + * The function only sends SIGTERM signal and does NOT
> + * free any resources (popen handle memory and file
> + * descriptors).
> + *
> + * @see lbox_popen_signal() for errors and return values.
> + */
> +static int
> +lbox_popen_terminate(struct lua_State *L)
> +{
> +	struct popen_handle *handle;
> +	bool is_closed;
> +	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
> +		diag_set(IllegalParams, "Bad params, use: ph:terminate()");
> +		return luaT_error(L);
> +	}
> +	if (is_closed)
> +		return luaT_popen_handle_closed_error(L);

Minor: In almost all cases you just throw an error due to popen handle
is closed right in a next line after obtaining the handle. I guess you
definitely need a routine for it.

> +
> +	int signo = SIGTERM;

Minor: This variable is excess considering its usage and function name.

> +
> +	if (popen_send_signal(handle, signo) != 0)
> +		return luaT_push_nil_and_error(L);
> +
> +	lua_pushboolean(L, true);
> +	return 1;
> +}
> +
> +/**
> + * Send SIGKILL signal to a child process.
> + *
> + * @param handle  a handle carries child process to kill
> + *
> + * The function only sends SIGKILL signal and does NOT
> + * free any resources (popen handle memory and file
> + * descriptors).
> + *
> + * @see lbox_popen_signal() for errors and return values.
> + */
> +static int
> +lbox_popen_kill(struct lua_State *L)
> +{
> +	struct popen_handle *handle;
> +	bool is_closed;
> +	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
> +		diag_set(IllegalParams, "Bad params, use: ph:kill()");
> +		return luaT_error(L);
> +	}
> +	if (is_closed)
> +		return luaT_popen_handle_closed_error(L);
> +
> +	int signo = SIGKILL;

Minor: This variable is excess considering its usage and function name.

> +
> +	if (popen_send_signal(handle, signo) != 0)
> +		return luaT_push_nil_and_error(L);
> +
> +	lua_pushboolean(L, true);
> +	return 1;
> +}
> +

<snipped>

> +
> +/**
> + * Read data from a child peer.
> + *
> + * @param handle        handle of a child process
> + * @param opts          an options table
> + * @param opts.stdout   whether to read from stdout, boolean
> + *                      (default: true)
> + * @param opts.stderr   whether to read from stderr, boolean
> + *                      (default: false)
> + * @param opts.timeout  time quota in seconds
> + *                      (default: 100 years)
> + *
> + * Read data from stdout or stderr streams with @a timeout.
> + * By default it reads from stdout. Set @a opts.stderr to
> + * `true` to read from stderr.
> + *
> + * Raise an error on incorrect parameters or when the fiber is
> + * cancelled:
> + *
> + * - IllegalParams:    incorrect type or value of a parameter.
> + * - IllegalParams:    called on a closed handle.
> + * - IllegalParams:    opts.stdout and opts.stderr are set both
> + * - IllegalParams:    a requested IO operation is not supported
> + *                     by the handle (stdout / stderr is not
> + *                     piped).
> + * - IllegalParams:    attempt to operate on a closed file
> + *                     descriptor.
> + * - FiberIsCancelled: cancelled by an outside code.
> + *
> + * Return a string on success, an empty string at EOF.
> + *
> + * Return `nil, err` on a failure. Possible reasons:
> + *
> + * - SocketError: an IO error occurs at read().
> + * - TimedOut:    @a timeout quota is exceeded.
> + * - OutOfMemory: no memory space for a buffer to read into.
> + * - LuajitError: ("not enough memory"): no memory space for
> + *                the Lua string.
> + */
> +static int
> +lbox_popen_read(struct lua_State *L)
> +{
> +	struct popen_handle *handle;
> +	bool is_closed;
> +	unsigned int flags = POPEN_FLAG_NONE;
> +	ev_tstamp timeout = TIMEOUT_INFINITY;
> +	struct region *region = &fiber()->gc;
> +	size_t region_svp = region_used(region);
> +
> +	/* Extract handle. */
> +	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL)
> +		goto usage;
> +	if (is_closed)
> +		return luaT_popen_handle_closed_error(L);
> +
> +	/* Extract options. */
> +	if (!lua_isnoneornil(L, 2)) {
> +		if (lua_type(L, 2) != LUA_TTABLE)
> +			goto usage;
> +
> +		lua_getfield(L, 2, "stdout");
> +		if (!lua_isnil(L, -1)) {
> +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> +				goto usage;
> +			if (lua_toboolean(L, -1) == 0)
> +				flags &= ~POPEN_FLAG_FD_STDOUT;
> +			else
> +				flags |= POPEN_FLAG_FD_STDOUT;
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_getfield(L, 2, "stderr");
> +		if (!lua_isnil(L, -1)) {
> +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> +				goto usage;
> +			if (lua_toboolean(L, -1) == 0)
> +				flags &= ~POPEN_FLAG_FD_STDERR;
> +			else
> +				flags |= POPEN_FLAG_FD_STDERR;
> +		}
> +		lua_pop(L, 1);

Minor: <stdout> and <stderr> options handling can be turned into a loop,
function or macro. Please choose the more convenient way to you.

> +
> +		lua_getfield(L, 2, "timeout");
> +		if (!lua_isnil(L, -1) &&
> +		    (timeout = luaT_check_timeout(L, -1)) < 0.0)
> +			goto usage;
> +		lua_pop(L, 1);

Minor: I see this passage only in two similar places, so I propose to
move everything to a timeout-related helper.

> +	}
> +
> +	/* Read from stdout by default. */
> +	if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR)))
> +		flags |= POPEN_FLAG_FD_STDOUT;

Minor: Why not to hoist defaults initialization to drop this heavy
condition?

> +
> +	size_t size = POPEN_LUA_READ_BUF_SIZE;
> +	char *buf = region_alloc(region, size);
> +	if (buf == NULL) {
> +		diag_set(OutOfMemory, size, "region_alloc", "read buffer");
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	static_assert(POPEN_LUA_READ_BUF_SIZE <= SSIZE_MAX,
> +		      "popen: read buffer is too big");
> +	ssize_t rc = popen_read_timeout(handle, buf, size, flags, timeout);
> +	if (rc < 0 || luaT_push_string_noxc(L, buf, rc) != 0)
> +		goto err;
> +	region_truncate(region, region_svp);
> +	return 1;
> +
> +usage:
> +	diag_set(IllegalParams, "Bad params, use: ph:read([{"
> +		 "stdout = <boolean>, "
> +		 "stderr = <boolean>, "
> +		 "timeout = <number>}])");
> +	return luaT_error(L);
> +err:
> +	region_truncate(region, region_svp);
> +	struct error *e = diag_last_error(diag_get());
> +	if (e->type == &type_IllegalParams ||
> +	    e->type == &type_FiberIsCancelled)
> +		return luaT_error(L);
> +	return luaT_push_nil_and_error(L);
> +}
> +
> +/**
> + * Write data to a child peer.
> + *
> + * @param handle        a handle of a child process
> + * @param str           a string to write
> + * @param opts          table of options
> + * @param opts.timeout  time quota in seconds
> + *                      (default: 100 years)
> + *
> + * Write string @a str to stdin stream of a child process.
> + *
> + * The function may yield forever if a child process does
> + * not read data from stdin and a pipe buffer becomes full.
> + * Size of this buffer depends on a platform. Use
> + * @a opts.timeout when unsure.
> + *
> + * When @a opts.timeout is not set, the function blocks
> + * (yields the fiber) until all data is written or an error
> + * happened.
> + *
> + * Raise an error on incorrect parameters or when the fiber is
> + * cancelled:
> + *
> + * - IllegalParams:    incorrect type or value of a parameter.
> + * - IllegalParams:    called on a closed handle.
> + * - IllegalParams:    string length is greater then SSIZE_MAX.
> + * - IllegalParams:    a requested IO operation is not supported
> + *                     by the handle (stdin is not piped).
> + * - IllegalParams:    attempt to operate on a closed file
> + *                     descriptor.
> + * - FiberIsCancelled: cancelled by an outside code.
> + *
> + * Return `true` on success.
> + *
> + * Return `nil, err` on a failure. Possible reasons:
> + *
> + * - SocketError: an IO error occurs at write().
> + * - TimedOut:    @a timeout quota is exceeded.
> + */
> +static int
> +lbox_popen_write(struct lua_State *L)
> +{
> +	struct popen_handle *handle;
> +	bool is_closed;
> +	const char *str;
> +	size_t len;
> +	ev_tstamp timeout = TIMEOUT_INFINITY;
> +
> +	/* Extract handle and string to write. */
> +	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL ||
> +	    (str = luaL_check_string_strict(L, 2, &len)) == NULL)
> +		goto usage;
> +	if (is_closed)
> +		return luaT_popen_handle_closed_error(L);
> +
> +	/* Extract options. */
> +	if (!lua_isnoneornil(L, 3)) {
> +		if (lua_type(L, 3) != LUA_TTABLE)
> +			goto usage;
> +
> +		lua_getfield(L, 3, "timeout");
> +		if (!lua_isnil(L, -1) &&
> +		    (timeout = luaT_check_timeout(L, -1)) < 0.0)
> +			goto usage;
> +		lua_pop(L, 1);
> +	}
> +
> +	unsigned int flags = POPEN_FLAG_FD_STDIN;

Minor: This variable is excess considering its usage.

> +	ssize_t rc = popen_write_timeout(handle, str, len, flags, timeout);
> +	assert(rc < 0 || rc == (ssize_t)len);
> +	if (rc < 0) {
> +		struct error *e = diag_last_error(diag_get());
> +		if (e->type == &type_IllegalParams ||
> +		    e->type == &type_FiberIsCancelled)
> +			return luaT_error(L);
> +		return luaT_push_nil_and_error(L);
> +	}
> +	lua_pushboolean(L, true);
> +	return 1;
> +
> +usage:
> +	diag_set(IllegalParams, "Bad params, use: ph:write(str[, {"
> +		 "timeout = <number>}])");
> +	return luaT_error(L);
> +}

<snipped>

> +
> +/**
> + * Get a field from a handle.
> + *
> + * @param handle  a handle of a child process
> + * @param key     a field name, string
> + *
> + * The function performs the following steps.
> + *
> + * Raise an error on incorrect parameters:
> + *
> + * - IllegalParams: incorrect type or value of a parameter.
> + *
> + * If there is a handle method with @a key name, return it.
> + *
> + * Raise an error on closed popen handle:
> + *
> + * - IllegalParams: called on a closed handle.
> + *
> + * If a @key is one of the following, return a value for it:
> + *
> + * - pid
> + * - command
> + * - opts
> + * - status
> + * - stdin
> + * - stdout
> + * - stderr
> + *
> + * @see lbox_popen_info() for description of those fields.
> + *
> + * Otherwise return `nil`.
> + */
> +static int
> +lbox_popen_index(struct lua_State *L)
> +{
> +	struct popen_handle *handle;
> +	bool is_closed;
> +	const char *key;
> +
> +	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL ||
> +	    (key = luaL_check_string_strict(L, 2, NULL)) == NULL) {
> +		diag_set(IllegalParams,
> +			 "Bad params, use __index(ph, <string>)");

Minor: "Bad params, use ph[<string>]" is more clear.

> +		return luaT_error(L);
> +	}
> +
> +	/* Get a value from the metatable. */
> +	lua_getmetatable(L, 1);
> +	lua_pushvalue(L, 2);
> +	lua_rawget(L, -2);
> +	if (! lua_isnil(L, -1))
> +		return 1;
> +
> +	if (is_closed) {
> +		diag_set(IllegalParams,
> +			 "Attempt to index a closed popen handle");
> +		return luaT_error(L);
> +	}

Minor: It's worth to leave a comment regarding __serialize method for
closed handle, since it's the only reason against hoisting this check
(like in all other methods).

> +
> +	if (strcmp(key, "pid") == 0) {
> +		if (handle->pid >= 0)
> +			lua_pushinteger(L, handle->pid);
> +		else
> +			lua_pushnil(L);
> +		return 1;
> +	}
> +
> +	if (strcmp(key, "command") == 0) {
> +		lua_pushstring(L, popen_command(handle));
> +		return 1;
> +	}
> +
> +	if (strcmp(key, "opts") == 0) {
> +		luaT_push_popen_opts(L, handle->flags);
> +		return 1;
> +	}
> +
> +	int state;
> +	int exit_code;
> +	popen_state(handle, &state, &exit_code);
> +	assert(state < POPEN_STATE_MAX);
> +	if (strcmp(key, "status") == 0)
> +		return luaT_push_popen_process_status(L, state, exit_code);
> +
> +	if (strcmp(key, "stdin") == 0)
> +		return luaT_push_popen_stdX_status(L, handle, STDIN_FILENO);
> +
> +	if (strcmp(key, "stdout") == 0)
> +		return luaT_push_popen_stdX_status(L, handle, STDOUT_FILENO);
> +
> +	if (strcmp(key, "stderr") == 0)
> +		return luaT_push_popen_stdX_status(L, handle, STDERR_FILENO);
> +
> +	lua_pushnil(L);
> +	return 1;
> +}
> +

<snipped>

> +
> +/* }}} */
> +
> +/* {{{ Module initialization */
> +

<snipped>

> +void
> +tarantool_lua_popen_init(struct lua_State *L)
> +{

<snipped>

> +
> +	/* Signals. */
> +	lua_newtable(L);
> +	for (int i = 0; popen_lua_signals[i].signame != NULL; ++i) {
> +		lua_pushinteger(L, popen_lua_signals[i].signo);
> +		lua_setfield(L, -2, popen_lua_signals[i].signame);
> +	}
> +	lua_setfield(L, -2, "signal");
> +
> +	/* Stream actions. */
> +	lua_newtable(L);
> +	for (int i = 0; popen_lua_actions[i].name != NULL; ++i) {
> +		lua_pushstring(L, popen_lua_actions[i].value);
> +		lua_setfield(L, -2, popen_lua_actions[i].name);
> +	}
> +	lua_setfield(L, -2, "opts");
> +
> +	/* Stream statuses. */
> +	lua_newtable(L);
> +	for (int i = 0; popen_lua_stream_statuses[i].name != NULL; ++i) {
> +		lua_pushstring(L, popen_lua_stream_statuses[i].value);
> +		lua_setfield(L, -2, popen_lua_stream_statuses[i].name);
> +	}
> +	lua_setfield(L, -2, "stream");
> +
> +	/* Process states. */
> +	lua_newtable(L);
> +	for (int i = 0; popen_lua_states[i].name != NULL; ++i) {
> +		lua_pushstring(L, popen_lua_states[i].value);
> +		lua_setfield(L, -2, popen_lua_states[i].name);
> +	}
> +	lua_setfield(L, -2, "state");

Minor: four similar loops above can be replaced with four macro calls.
Consider the following diff:

================================================================================

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 40f4343eb..319ab3d77 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -2421,36 +2421,24 @@ tarantool_lua_popen_init(struct lua_State *L)
 	luaL_register_type(L, popen_handle_uname, popen_handle_methods);
 	luaL_register_type(L, popen_handle_closed_uname, popen_handle_methods);
 
+#define setconsts(L, namespace, consts, kfield, vtype, vfield) do { \
+	lua_newtable(L);                                            \
+	for (int i = 0; consts[i].kfield != NULL; ++i) {            \
+		lua_push ## vtype(L, consts[i].vfield);             \
+		lua_setfield(L, -2, consts[i].kfield);              \
+	}                                                           \
+	lua_setfield(L, -2, namespace);                             \
+} while(0)
+
 	/* Signals. */
-	lua_newtable(L);
-	for (int i = 0; popen_lua_signals[i].signame != NULL; ++i) {
-		lua_pushinteger(L, popen_lua_signals[i].signo);
-		lua_setfield(L, -2, popen_lua_signals[i].signame);
-	}
-	lua_setfield(L, -2, "signal");
-
+	setconsts(L, "signal", popen_lua_signals, signame, number, signo);
 	/* Stream actions. */
-	lua_newtable(L);
-	for (int i = 0; popen_lua_actions[i].name != NULL; ++i) {
-		lua_pushstring(L, popen_lua_actions[i].value);
-		lua_setfield(L, -2, popen_lua_actions[i].name);
-	}
-	lua_setfield(L, -2, "opts");
-
+	setconsts(L, "opts", popen_lua_actions, name, string, value);
 	/* Stream statuses. */
-	lua_newtable(L);
-	for (int i = 0; popen_lua_stream_statuses[i].name != NULL; ++i) {
-		lua_pushstring(L, popen_lua_stream_statuses[i].value);
-		lua_setfield(L, -2, popen_lua_stream_statuses[i].name);
-	}
-	lua_setfield(L, -2, "stream");
-
+	setconsts(L, "stream", popen_lua_stream_statuses, name, string, value);
 	/* Process states. */
-	lua_newtable(L);
-	for (int i = 0; popen_lua_states[i].name != NULL; ++i) {
-		lua_pushstring(L, popen_lua_states[i].value);
-		lua_setfield(L, -2, popen_lua_states[i].name);
-	}
-	lua_setfield(L, -2, "state");
+	setconsts(L, "state", popen_lua_states, name, string, value);
+
+#undef setconsts
 }
 

================================================================================

> +}
> +
> +/* }}} */
> diff --git a/src/lua/popen.h b/src/lua/popen.h
> new file mode 100644
> index 000000000..e23c5d7e5
> --- /dev/null
> +++ b/src/lua/popen.h
> @@ -0,0 +1,44 @@
> +#ifndef TARANTOOL_LUA_POPEN_H_INCLUDED
> +#define TARANTOOL_LUA_POPEN_H_INCLUDED

Minor: AFAIR, we decided to use pragma, but I see no word about it in
our C style guide[2].

<snipped>

> -- 
> 2.25.0
> 

[1]: https://github.com/tarantool/luajit/blob/tarantool/src/lj_tab.c#L694695
[2]: https://www.tarantool.io/en/doc/2.2/dev_guide/c_style_guide/

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Igor Munkin]

Sasha,

On 20.04.20, Alexander Turenko wrote:
> Thanks for the careful proofreading!
> 
> Aside of changes below I changed `popen.stream.OPENED (== 'opened')` to
> `popen.stream.OPEN (== 'open')` to match english grammar. Same for
> statuses -> status (yep, 'statuses' is allowed in English, but it is
> ugly).

Yep, that's the way much better.

> 
> Force-pushed Totktonada/gh-4031-popen-13-full-ci.
> 
> WBR, Alexander Turenko.
> 
> ----
> 

<snipped>

> 
> > > @param opts.env  an array of environment variables to
> > >                  be used inside a process;
> > >                  - when is not set then the current
> > >                    environment is inherited;
> > >                  - if set then the environment will be
> > >                    replaced
> > >                  - if set to an empty array then the
> > >                    environment will be dropped
> > 
> > Minor: I looks more convenient to sort the lines above the following
> > way:
> > * when is not set <...>
> > * if set to an empty table <...>
> > * if set to a non-empty table <...>
> 
> I don't mind.
> 
> Reordered in the code and the commit message.
> 
> Also I rephrased the description a bit to highlight the fact that the
> parameter is key-value mapping, not an array.

Good remark, thanks.

> 

<snipped>

> 
> > > The returned handle is subject to garbage collection, but
> > > it may be freed explicitly using :close(). SIGKILL is sent
> > > to the child process at :close() / GC if it is alive and
> > > opts.keep_child is not set.
> > 
> > Minor: Let's adjust the terminology. What do you mean speaking about
> > "handle"? If it's a GC object, then there is no way to release it
> > manually or "explicitly", only via platform GC. If it's a GC object
> > *payload* then it's not managed by Lua GC and need to be explicitly
> > released via special method or the corresponding __gc metamethod.
> 
> My intention is to give two ideas here:
> 
> * Feel free to use explicit :close() or lean on Lua GC.
> * :close() / GC handler kills the child process.
> 
> But being strict in terms and wording is goal for me too, so I reword
> the paragraph to eliminate phrases, which requires a GC object / a GC
> object payload as an object / a subject.
> 
> Changed in the code and the commit:
> 
> diff --git a/src/lua/popen.c b/src/lua/popen.c
> index ec8c95662..31b4bc2b0 100644
> --- a/src/lua/popen.c
> +++ b/src/lua/popen.c
> @@ -1184,10 +1184,11 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
>   *                              :close() or collecting of the
>   *                              handle by Lua GC
>   *
> - * The returned handle is subject to garbage collection, but
> - * it may be freed explicitly using :close(). SIGKILL is sent
> - * to the child process at :close() / GC if it is alive and
> - * opts.keep_child is not set.
> + * The returned handle provides :close() method to explicitly
> + * release all occupied resources (including the child process
> + * itself if @a opts.keep_child is not set). However if the
> + * method is not called for a handle during its lifetime, the
> + * same freeing actions will be triggered by Lua GC.
>   *
>   * It is recommended to use opts.setsid + opts.group_signal
>   * if a child process may spawn its own childs and they all

Nice, now it's clear, thanks!

> 

<snipped>

> 
> > > Return `nil, err` on a failure. Possible reasons:
> > > 
> > > - SocketError: an IO error occurs at read().
> > 
> > SocketError on pipe looks ridiculous to me. Maybe an IO one?
> 
> It comes from coio.cc. It would be good to refactor it someday, but not
> tonight :)

And there is only one thing we say to Refactoring: “not today” :)

> 

<snipped>

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Alexander Turenko]

Thank you, Igor!

I made simple changes and postponed ones where I need a bit more time.
Hope, you don't mind.

Marked postponed questions as (postponed) in the email.

My answers are below.

WBR, Alexande Turenko.

> > +/* {{{ General-purpose Lua helpers */
> > +
> > +/**
> > + * Extract a string from the Lua stack.
> > + *
> > + * Return (const char *) if so, otherwise return NULL.
> > + *
> > + * Unlike luaL_checkstring() it accepts only a string and does not
> > + * accept a number.
> > + *
> > + * Unlike luaL_checkstring() it does not raise an error, but
> > + * returns NULL when a type is not what is excepted.
> > + */
> > +static const char *
> > +luaL_check_string_strict(struct lua_State *L, int idx, size_t *len_ptr)
> 
> Minor: I propose the following names: <luaL_tolstring_strict>,
> <luaL_checkstring_nothrow> or <luaL_checkstring_noxc>.

Thanks! luaL_tolstring_strict() is much better.

Changed. Adjusted the comment:

 /**
  * Extract a string from the Lua stack.
  *
- * Return (const char *) if so, otherwise return NULL.
+ * Return (const char *) for a string, otherwise return NULL.
  *
- * Unlike luaL_checkstring() it accepts only a string and does not
+ * Unlike luaL_tolstring() it accepts only a string and does not
  * accept a number.
- *
- * Unlike luaL_checkstring() it does not raise an error, but
- * returns NULL when a type is not what is excepted.
  */

> 
> Side note: Sorry, but I can't pass by it. Sasha, we both know each
> other's opinion regarding luaL_* prefix usage, and I'm not going to
> argue about it within this review. I'll try to live with it since it's
> a static one, but I see no reasons to not try to name it other way.

We are not decided anything certain, so I continue using past agreements
for now. It would be worse to try to use some new namespace and make a
wrong guess.

And, yep, those functions are static: we're free to change its names
when (if) we'll do it for all names across tarantool code.

> > +static int
> > +luaT_push_string_noxc(struct lua_State *L, char *str, size_t len)
> > +{
> > +	lua_pushcfunction(L, luaT_push_string_noxc_wrapper);
> > +	lua_pushlightuserdata(L, str);
> > +	lua_pushinteger(L, len);
> > +	return luaT_call(L, 2, 1);
> > +}
> 
> Minor: IMHO luaT_pushstring_* is more Lua-like naming.

It is from past agreements too: we discussed this with Vladimir D. and
agreed to split 'foo' and 'bar' in 'lua*_foo_bar_baz()', when there is
'baz'. When there is no 'baz', it is 'lua*_foobar()'.

> 
> > +
> > +/* }}} */
> > +
> > +/* {{{ Popen handle userdata manipulations */
> > +
> > +/**
> > + * Extract popen handle from the Lua stack.
> > + *
> > + * Return NULL in case of unexpected type.
> > + */
> > +static struct popen_handle *
> > +luaT_check_popen_handle(struct lua_State *L, int idx, bool *is_closed_ptr)
> > +{
> > +	struct popen_handle **handle_ptr =
> > +		luaL_testudata(L, idx, popen_handle_uname);
> > +	bool is_closed = false;
> > +
> > +	if (handle_ptr == NULL) {
> > +		handle_ptr = luaL_testudata(L, idx, popen_handle_closed_uname);
> > +		is_closed = true;
> > +	}
> > +
> > +	if (handle_ptr == NULL)
> > +		return NULL;
> 
> Minor: If NULL is returned, then is_closed_ptr payload is undefined. It
> would be nice to mention it in contract above.

It is unchanged, not undefined. I find it good style to left output
parameters intact when a function fails. However I don't mention it in
the comment, because I think that good code should not lean on this
assumption: there are many such functions that may clobber output
parameters at failure.

It seems it is somewhat similar to particular application of 'robustness
principle'.

> 
> > +	assert(*handle_ptr != NULL);
> > +
> > +	if (is_closed_ptr != NULL)
> 
> Minor: Considering function usage this check is excess.

I prefer to follow the same pattern for those functions: it just a line
of code.

> > +
> > +/**
> > + * Push popen options as a Lua table.
> > + *
> > + * Environment variables are not stored in a popen handle and so
> > + * missed here.
> > + */
> > +static int
> > +luaT_push_popen_opts(struct lua_State *L, unsigned int flags)
> > +{
> > +	lua_createtable(L, 0, 8);
> > +
> > +	luaT_push_popen_stdX_action(L, STDIN_FILENO, flags);
> > +	lua_setfield(L, -2, "stdin");
> > +
> > +	luaT_push_popen_stdX_action(L, STDOUT_FILENO, flags);
> > +	lua_setfield(L, -2, "stdout");
> > +
> > +	luaT_push_popen_stdX_action(L, STDERR_FILENO, flags);
> > +	lua_setfield(L, -2, "stderr");
> 
> Minor: This can be turned into a loop.

Don't sure it would be easier to read if would be written in this way.

However, yep, if I'll create some mapping for those options to reduce
code duplication in some other place, it'll make sense to loop over this
mapping here too.

Added the comment:

@@ -469,6 +469,20 @@ luaT_push_popen_opts(struct lua_State *L, unsigned int flags)
 {
        lua_createtable(L, 0, 8);
 
+       /*
+        * FIXME: Loop over a static array of stdX options.
+        *
+        * static const struct {
+        *      const char *option_name;
+        *      int fd;
+        * } popen_lua_stdX_options = {
+        *      {"stdin",       STDIN_FILENO    },
+        *      {"stdout",      STDOUT_FILENO   },
+        *      {"stderr",      STDERR_FILENO   },
+        *      {NULL,          -1              },
+        * };
+        */
+
        luaT_push_popen_stdX_action(L, STDIN_FILENO, flags);
        lua_setfield(L, -2, "stdin");

(postponed)

> 
> > +
> > +	/* env is skipped */
> > +
> > +	lua_pushboolean(L, (flags & POPEN_FLAG_SHELL) != 0);
> > +	lua_setfield(L, -2, "shell");
> > +
> > +	lua_pushboolean(L, (flags & POPEN_FLAG_SETSID) != 0);
> > +	lua_setfield(L, -2, "setsid");
> > +
> > +	lua_pushboolean(L, (flags & POPEN_FLAG_CLOSE_FDS) != 0);
> > +	lua_setfield(L, -2, "close_fds");
> > +
> > +	lua_pushboolean(L, (flags & POPEN_FLAG_RESTORE_SIGNALS) != 0);
> > +	lua_setfield(L, -2, "restore_signals");
> > +
> > +	lua_pushboolean(L, (flags & POPEN_FLAG_GROUP_SIGNAL) != 0);
> > +	lua_setfield(L, -2, "group_signal");
> > +
> > +	lua_pushboolean(L, (flags & POPEN_FLAG_KEEP_CHILD) != 0);
> > +	lua_setfield(L, -2, "keep_child");
> 
> Minor: This can be also turned into a loop.

Same as above.

If we'll introduce some mapping from/to boolean option names and flags
it will make sense.

Added the comment:

@@ -480,6 +494,8 @@ luaT_push_popen_opts(struct lua_State *L, unsigned int flags)
 
        /* env is skipped */
 
+       /* FIXME: Loop over a static array of boolean options. */
+
        lua_pushboolean(L, (flags & POPEN_FLAG_SHELL) != 0);
        lua_setfield(L, -2, "shell");

(postponed)

> > +/**
> > + * Parse popen.new() "opts" parameter.
> > + *
> > + * Prerequisite: @a opts should be zero filled.
> > + *
> > + * Result: @a opts structure is filled.
> > + *
> > + * Raise an error in case of the incorrect parameter.
> > + *
> > + * Return 0 at success. Allocates opts->env on @a region if
> > + * needed. @see luaT_popen_parse_env() for details how to
> > + * free it.
> > + *
> > + * Return -1 in case of an allocation error and set a diag
> > + * (OutOfMemory).
> > + */
> > +static int
> > +luaT_popen_parse_opts(struct lua_State *L, int idx, struct popen_opts *opts,
> > +		      struct region *region)
> 
> Minor: I rewrote it a bit, consider the diff below:
> 
> +#define setstdX(L, idx, stream, fileno) do {                        \
> +	lua_getfield(L, idx, stream);                               \
> +	if (!lua_isnil(L, -1))                                      \
> +		luaT_popen_parse_stdX(L, -1, fileno, &opts->flags); \
> +	lua_pop(L, 1);                                              \
> +} while(0)
> +		setstdX(L, idx, "stdin", STDIN_FILENO);
> +		setstdX(L, idx, "stdout", STDOUT_FILENO);
> +		setstdX(L, idx, "stderr", STDERR_FILENO);
> +
> +#undef setstdX
>  
> +#define setflag(L, idx, key, flag) do {                                 \
> +	lua_getfield(L, idx, key);                                      \
> +	if (!lua_isnil(L, -1)) {                                        \
> +		if (lua_type(L, -1) != LUA_TBOOLEAN)                    \
> +			luaT_popen_param_type_error(L, -1, "popen.new", \
> +						    "opts." key,        \
> +						    "boolean or nil");  \
> +		if (lua_toboolean(L, -1) == 0)                          \
> +			opts->flags &= ~flag;                           \
> +		else                                                    \
> +			opts->flags |= flag;                            \
> +	}                                                               \
> +	lua_pop(L, 1);                                                  \
> +} while(0)
> +
> +		setflag(L, idx, "shell", POPEN_FLAG_SHELL);
> +		setflag(L, idx, "setsid", POPEN_FLAG_SETSID);
> +		setflag(L, idx, "close_fds", POPEN_FLAG_CLOSE_FDS);
> +		setflag(L, idx, "restore_signals", POPEN_FLAG_RESTORE_SIGNALS);
> +		setflag(L, idx, "group_signal", POPEN_FLAG_GROUP_SIGNAL);
> +		setflag(L, idx, "keep_child", POPEN_FLAG_KEEP_CHILD);
> +
> +#undef setflag
>  
> ================================================================================

(Cut removed lines from your diff above.)

I see, yep, it is much smaller. I'll postpone actions for reducing code
duplication (to make them under less tight deadline).

I would try to avoid a macro: use a function or maybe list all those
boolean options in a static array and loop over them.

Anyway, I'll postpone deduplication (there are a couple of FIXMEs in the
code for this). Hope you don't mind.

Added the comments:

@@ -866,6 +882,11 @@ luaT_popen_parse_opts(struct lua_State *L, int idx, struct popen_opts *opts,
 
        /* Parse options. */
        if (lua_type(L, idx) == LUA_TTABLE) {
+               /*
+                * FIXME: Loop over a static array of stdX
+                * options.
+                */
+
                lua_getfield(L, idx, "stdin");
                if (! lua_isnil(L, -1)) {
                        luaT_popen_parse_stdX(L, -1, STDIN_FILENO,
@@ -893,6 +914,11 @@ luaT_popen_parse_opts(struct lua_State *L, int idx, struct popen_opts *opts,
                }
                lua_pop(L, 1);
 
+               /*
+                * FIXME: Loop over a static array of boolean
+                * options.
+                */
+
                lua_getfield(L, idx, "shell");
                if (! lua_isnil(L, -1)) {
                        if (lua_type(L, -1) != LUA_TBOOLEAN)

(postponed)

> 
> > +{
> > +	/*
> > +	 * Default flags: inherit std*, close other fds,
> > +	 * restore signals.
> > +	 */
> > +	opts->flags = POPEN_FLAG_NONE		|
> > +		POPEN_FLAG_CLOSE_FDS		|
> > +		POPEN_FLAG_RESTORE_SIGNALS;
> > +
> > +	/* Parse options. */
> > +	if (lua_type(L, idx) == LUA_TTABLE) {
> > +		lua_getfield(L, idx, "stdin");
> > +		if (! lua_isnil(L, -1)) {
> > +			luaT_popen_parse_stdX(L, -1, STDIN_FILENO,
> > +					      &opts->flags);
> > +		}
> > +		lua_pop(L, 1);
> > +
> > +		lua_getfield(L, idx, "stdout");
> > +		if (! lua_isnil(L, -1))
> > +			luaT_popen_parse_stdX(L, -1, STDOUT_FILENO,
> > +					      &opts->flags);
> > +		lua_pop(L, 1);
> > +
> > +		lua_getfield(L, idx, "stderr");
> > +		if (! lua_isnil(L, -1))
> > +			luaT_popen_parse_stdX(L, -1, STDERR_FILENO,
> > +					      &opts->flags);
> > +		lua_pop(L, 1);
> > +
> > +		lua_getfield(L, idx, "env");
> > +		if (! lua_isnil(L, -1)) {
> > +			opts->env = luaT_popen_parse_env(L, -1, region);
> > +			if (opts->env == NULL)
> > +				return -1;
> > +		}
> > +		lua_pop(L, 1);
> > +
> > +		lua_getfield(L, idx, "shell");
> > +		if (! lua_isnil(L, -1)) {
> > +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> > +				luaT_popen_param_type_error(L, -1, "popen.new",
> > +							    "opts.shell",
> > +							    "boolean or nil");
> > +			if (lua_toboolean(L, -1) == 0)
> > +				opts->flags &= ~POPEN_FLAG_SHELL;
> > +			else
> > +				opts->flags |= POPEN_FLAG_SHELL;
> > +		}
> > +		lua_pop(L, 1);
> > +
> > +		lua_getfield(L, idx, "setsid");
> > +		if (! lua_isnil(L, -1)) {
> > +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> > +				luaT_popen_param_type_error(L, -1, "popen.new",
> > +							    "opts.setsid",
> > +							    "boolean or nil");
> > +			if (lua_toboolean(L, -1) == 0)
> > +				opts->flags &= ~POPEN_FLAG_SETSID;
> > +			else
> > +				opts->flags |= POPEN_FLAG_SETSID;
> > +		}
> > +		lua_pop(L, 1);
> > +
> > +		lua_getfield(L, idx, "close_fds");
> > +		if (! lua_isnil(L, -1)) {
> > +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> > +				luaT_popen_param_type_error(L, -1, "popen.new",
> > +							    "opts.close_fds",
> > +							    "boolean or nil");
> > +			if (lua_toboolean(L, -1) == 0)
> > +				opts->flags &= ~POPEN_FLAG_CLOSE_FDS;
> > +			else
> > +				opts->flags |= POPEN_FLAG_CLOSE_FDS;
> > +		}
> > +		lua_pop(L, 1);
> > +
> > +		lua_getfield(L, idx, "restore_signals");
> > +		if (! lua_isnil(L, -1)) {
> > +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> > +				luaT_popen_param_type_error(
> > +					L, -1, "popen.new",
> > +					"opts.restore_signals",
> > +					"boolean or nil");
> > +			if (lua_toboolean(L, -1) == 0)
> > +				opts->flags &= ~POPEN_FLAG_RESTORE_SIGNALS;
> > +			else
> > +				opts->flags |= POPEN_FLAG_RESTORE_SIGNALS;
> > +		}
> > +		lua_pop(L, 1);
> > +
> > +		lua_getfield(L, idx, "group_signal");
> > +		if (! lua_isnil(L, -1)) {
> > +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> > +				luaT_popen_param_type_error(L, -1, "popen.new",
> > +							    "opts.group_signal",
> > +							    "boolean or nil");
> > +			if (lua_toboolean(L, -1) == 0)
> > +				opts->flags &= ~POPEN_FLAG_GROUP_SIGNAL;
> > +			else
> > +				opts->flags |= POPEN_FLAG_GROUP_SIGNAL;
> > +		}
> > +		lua_pop(L, 1);
> > +
> > +		lua_getfield(L, idx, "keep_child");
> > +		if (! lua_isnil(L, -1)) {
> > +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> > +				luaT_popen_param_type_error(L, -1, "popen.new",
> > +							    "opts.keep_child",
> > +							    "boolean or nil");
> > +			if (lua_toboolean(L, -1) == 0)
> > +				opts->flags &= ~POPEN_FLAG_KEEP_CHILD;
> > +			else
> > +				opts->flags |= POPEN_FLAG_KEEP_CHILD;
> > +		}
> > +		lua_pop(L, 1);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * Parse popen.new() "argv" parameter.
> > + *
> > + * Prerequisite: opts->flags & POPEN_FLAG_SHELL should be
> > + * the same in a call of this function and when paired
> > + * popen_new() is invoked.
> > + *
> > + * Raise an error in case of the incorrect parameter.
> > + *
> > + * Return 0 at success. Sets opts->args and opts->nr_args.
> 
> Typo: s/args/argv/g.

Thanks! Fixed.

> 
> Minor: argc looks to be a good complement to argv field.

'struct popen_opts' is from backend. I tring to be conservative in
changes of everything that is already in master.

> 
> > + * Allocates opts->argv on @a region (@see
> > + * luaT_popen_parse_env() for details how to free it).
> > + *
> > + * Return -1 in case of an allocation error and set a diag
> > + * (OutOfMemory).
> > + */
> > +static int
> > +luaT_popen_parse_argv(struct lua_State *L, int idx, struct popen_opts *opts,
> > +		      struct region *region)
> > +{
> > +	size_t region_svp = region_used(region);
> > +	size_t argv_len = lua_objlen(L, idx);
> 
> There are no guarantees the table doesn't have gaps. Consider the
> comment from lj_tab.c[1] and the following example:
> | $ ./luajit
> | LuaJIT 2.1.0-beta3 -- Copyright (C) 2005-2017 Mike Pall.
> | http://luajit.org/
> | JIT: ON SSE2 SSE3 SSE4.1 BMI2 fold cse dce fwd dse narrow loop abc sink
> | fuse
> | > print(#{ 'echo', 'QQ', nil, 'rm' })
> | 4
> Thereby the safe way to parse popen argv is iterating by numeric indices
> until lua_rawgeti yields nil.

Added the comment:

@@ -1023,6 +1023,15 @@ luaT_popen_parse_argv(struct lua_State *L, int idx, struct popen_opts *opts,
                      struct region *region)
 {
        size_t region_svp = region_used(region);
+
+       /*
+        * We need to know exact size of the array to allocate
+        * a memory for opts->argv without reallocations.
+        *
+        * lua_objlen() does not guarantee that there are no
+        * holes in the array, but we check it within a loop
+        * later.
+        */
        size_t argv_len = lua_objlen(L, idx);
 
        /* ["sh", "-c", ]..., NULL. */

> 
> > +
> > +	/* ["sh", "-c", ]..., NULL. */
> > +	opts->nr_argv = argv_len + 1;
> > +	if (opts->flags & POPEN_FLAG_SHELL)
> > +		opts->nr_argv += 2;
> > +
> > +	size_t size = sizeof(char *) * opts->nr_argv;
> > +	opts->argv = region_alloc(region, size);
> > +	if (opts->argv == NULL) {
> > +		diag_set(OutOfMemory, size, "region_alloc", "argv");
> > +		return -1;
> > +	}
> > +
> > +	const char **to = (const char **)opts->argv;
> > +	if (opts->flags & POPEN_FLAG_SHELL) {
> > +		opts->argv[0] = NULL;
> > +		opts->argv[1] = NULL;
> 
> Minor: Please leave a comment why these slots are NULLs. I don't get it
> from the given context.

Okay.

@@ -1046,6 +1046,7 @@ luaT_popen_parse_argv(struct lua_State *L, int idx, struct popen_opts *opts,
                return -1;
        }

+       /* Keep place for "sh", "-c" as popen_new() expects. */
        const char **to = (const char **)opts->argv;
        if (opts->flags & POPEN_FLAG_SHELL) {
                opts->argv[0] = NULL;

> 
> > +		to += 2;
> > +	}
> > +
> > +	for (size_t i = 0; i < argv_len; ++i) {
> > +		lua_rawgeti(L, idx, i + 1);
> > +		const char *arg = luaL_check_string_strict(L, -1, NULL);
> 
> Minor: OK, now I see. Please leave a comment here regarding the gap
> check and benefits we have omitting dynamic reallocation, as we
> discussed.

I said 'without reallocations' (see above) and I guess it is enough.

> > +static int
> > +lbox_popen_terminate(struct lua_State *L)
> > +{
> > +	struct popen_handle *handle;
> > +	bool is_closed;
> > +	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
> > +		diag_set(IllegalParams, "Bad params, use: ph:terminate()");
> > +		return luaT_error(L);
> > +	}
> > +	if (is_closed)
> > +		return luaT_popen_handle_closed_error(L);
> 
> Minor: In almost all cases you just throw an error due to popen handle
> is closed right in a next line after obtaining the handle. I guess you
> definitely need a routine for it.

Added the comment:

@@ -1533,6 +1533,11 @@ lbox_popen_shell(struct lua_State *L)
 static int
 lbox_popen_signal(struct lua_State *L)
 {
+       /*
+        * FIXME: Extracting a handle and raising an error when
+        * it is closed is repeating pattern within the file. It
+        * worth to extract it to a function.
+        */
        struct popen_handle *handle;
        bool is_closed;
        if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL ||

(postponed)

> 
> > +
> > +	int signo = SIGTERM;
> 
> Minor: This variable is excess considering its usage and function name.

This way the code for :signal(), :kill() and :terminate() looks more
similar and I like this property.

> > +static int
> > +lbox_popen_kill(struct lua_State *L)
> > +{
> > +	struct popen_handle *handle;
> > +	bool is_closed;
> > +	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL) {
> > +		diag_set(IllegalParams, "Bad params, use: ph:kill()");
> > +		return luaT_error(L);
> > +	}
> > +	if (is_closed)
> > +		return luaT_popen_handle_closed_error(L);
> > +
> > +	int signo = SIGKILL;
> 
> Minor: This variable is excess considering its usage and function name.

Same.

> > +		lua_getfield(L, 2, "stderr");
> > +		if (!lua_isnil(L, -1)) {
> > +			if (lua_type(L, -1) != LUA_TBOOLEAN)
> > +				goto usage;
> > +			if (lua_toboolean(L, -1) == 0)
> > +				flags &= ~POPEN_FLAG_FD_STDERR;
> > +			else
> > +				flags |= POPEN_FLAG_FD_STDERR;
> > +		}
> > +		lua_pop(L, 1);
> 
> Minor: <stdout> and <stderr> options handling can be turned into a loop,
> function or macro. Please choose the more convenient way to you.

Added the comment:

@@ -1731,6 +1731,8 @@ lbox_popen_read(struct lua_State *L)
                if (lua_type(L, 2) != LUA_TTABLE)
                        goto usage;
 
+               /* FIXME: Shorten boolean options parsing. */
+
                lua_getfield(L, 2, "stdout");
                if (!lua_isnil(L, -1)) {
                        if (lua_type(L, -1) != LUA_TBOOLEAN)

> > +
> > +		lua_getfield(L, 2, "timeout");
> > +		if (!lua_isnil(L, -1) &&
> > +		    (timeout = luaT_check_timeout(L, -1)) < 0.0)
> > +			goto usage;
> > +		lua_pop(L, 1);
> 
> Minor: I see this passage only in two similar places, so I propose to
> move everything to a timeout-related helper.

luaT_check_timeout() is already this helper. Don't sure it worth to add
one more wrapping level.

> 
> > +	}
> > +
> > +	/* Read from stdout by default. */
> > +	if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR)))
> > +		flags |= POPEN_FLAG_FD_STDOUT;
> 
> Minor: Why not to hoist defaults initialization to drop this heavy
> condition?

Added the comment:

@@ -1715,7 +1715,13 @@ lbox_popen_read(struct lua_State *L)
 {
        struct popen_handle *handle;
        bool is_closed;
+
+       /*
+        * Actual default is POPEN_FLAG_FD_STDOUT, but
+        * it is set only when no std* option is passed.
+        */
        unsigned int flags = POPEN_FLAG_NONE;
+
        ev_tstamp timeout = TIMEOUT_INFINITY;
        struct region *region = &fiber()->gc;
        size_t region_svp = region_used(region);

> > +	/* Extract options. */
> > +	if (!lua_isnoneornil(L, 3)) {
> > +		if (lua_type(L, 3) != LUA_TTABLE)
> > +			goto usage;
> > +
> > +		lua_getfield(L, 3, "timeout");
> > +		if (!lua_isnil(L, -1) &&
> > +		    (timeout = luaT_check_timeout(L, -1)) < 0.0)
> > +			goto usage;
> > +		lua_pop(L, 1);
> > +	}
> > +
> > +	unsigned int flags = POPEN_FLAG_FD_STDIN;
> 
> Minor: This variable is excess considering its usage.

I like this way to make calls look similar: popen_{read,write}_timeout()
in the case. It is also used in the backend, see popen_write_timeout().

In fact I borrow the approach from the popen backend code, because found
it useful.

> > +static int
> > +lbox_popen_index(struct lua_State *L)
> > +{
> > +	struct popen_handle *handle;
> > +	bool is_closed;
> > +	const char *key;
> > +
> > +	if ((handle = luaT_check_popen_handle(L, 1, &is_closed)) == NULL ||
> > +	    (key = luaL_check_string_strict(L, 2, NULL)) == NULL) {
> > +		diag_set(IllegalParams,
> > +			 "Bad params, use __index(ph, <string>)");
> 
> Minor: "Bad params, use ph[<string>]" is more clear.

I would even suggest ph.<field>, but it is not possible to receive this
error when a property is acquired this way. When a user got this error
it likely doing something with the metamethod itself and it looks logical
to show its "name" ("__index") explicitly. However ph[1] is possible too.

Don't sure here, so I'll left it intact for now. At least now it is
consistent with __serialize.

> 
> > +		return luaT_error(L);
> > +	}
> > +
> > +	/* Get a value from the metatable. */
> > +	lua_getmetatable(L, 1);
> > +	lua_pushvalue(L, 2);
> > +	lua_rawget(L, -2);
> > +	if (! lua_isnil(L, -1))
> > +		return 1;
> > +
> > +	if (is_closed) {
> > +		diag_set(IllegalParams,
> > +			 "Attempt to index a closed popen handle");
> > +		return luaT_error(L);
> > +	}
> 
> Minor: It's worth to leave a comment regarding __serialize method for
> closed handle, since it's the only reason against hoisting this check
> (like in all other methods).

I also prefer to keep arguments verification within methods be 'fair'.
It is better to wrap open/closed check to write it shorter, then move it
here, IMHO.

The __index metamethod does not look for me as the point, where all
common validation should occur.

Added the comment:

@@ -2312,13 +2312,24 @@ lbox_popen_index(struct lua_State *L)
                return luaT_error(L);
        }

-       /* Get a value from the metatable. */
+       /*
+        * If `key` is a method name, return it.
+        *
+        * The __index metamethod performs only checks that
+        * it needs on its own. Despite there are common parts
+        * across methods, it is better to validate all
+        * parameters within a method itself.
+        *
+        * In particular, methods should perform a check for
+        * closed handles.
+        */
        lua_getmetatable(L, 1);
        lua_pushvalue(L, 2);
        lua_rawget(L, -2);
        if (! lua_isnil(L, -1))
                return 1;

+       /* Does not allow to get a field from a closed handle. */
        if (is_closed) {
                diag_set(IllegalParams,
                         "Attempt to index a closed popen handle");

> > +	/* Signals. */
> > +	lua_newtable(L);
> > +	for (int i = 0; popen_lua_signals[i].signame != NULL; ++i) {
> > +		lua_pushinteger(L, popen_lua_signals[i].signo);
> > +		lua_setfield(L, -2, popen_lua_signals[i].signame);
> > +	}
> > +	lua_setfield(L, -2, "signal");
> > +
> > +	/* Stream actions. */
> > +	lua_newtable(L);
> > +	for (int i = 0; popen_lua_actions[i].name != NULL; ++i) {
> > +		lua_pushstring(L, popen_lua_actions[i].value);
> > +		lua_setfield(L, -2, popen_lua_actions[i].name);
> > +	}
> > +	lua_setfield(L, -2, "opts");
> > +
> > +	/* Stream statuses. */
> > +	lua_newtable(L);
> > +	for (int i = 0; popen_lua_stream_statuses[i].name != NULL; ++i) {
> > +		lua_pushstring(L, popen_lua_stream_statuses[i].value);
> > +		lua_setfield(L, -2, popen_lua_stream_statuses[i].name);
> > +	}
> > +	lua_setfield(L, -2, "stream");
> > +
> > +	/* Process states. */
> > +	lua_newtable(L);
> > +	for (int i = 0; popen_lua_states[i].name != NULL; ++i) {
> > +		lua_pushstring(L, popen_lua_states[i].value);
> > +		lua_setfield(L, -2, popen_lua_states[i].name);
> > +	}
> > +	lua_setfield(L, -2, "state");
> 
> Minor: four similar loops above can be replaced with four macro calls.
> Consider the following diff:
> 
> ================================================================================
> 
> diff --git a/src/lua/popen.c b/src/lua/popen.c
> index 40f4343eb..319ab3d77 100644
> --- a/src/lua/popen.c
> +++ b/src/lua/popen.c
> @@ -2421,36 +2421,24 @@ tarantool_lua_popen_init(struct lua_State *L)
>  	luaL_register_type(L, popen_handle_uname, popen_handle_methods);
>  	luaL_register_type(L, popen_handle_closed_uname, popen_handle_methods);
>  
> +#define setconsts(L, namespace, consts, kfield, vtype, vfield) do { \
> +	lua_newtable(L);                                            \
> +	for (int i = 0; consts[i].kfield != NULL; ++i) {            \
> +		lua_push ## vtype(L, consts[i].vfield);             \
> +		lua_setfield(L, -2, consts[i].kfield);              \
> +	}                                                           \
> +	lua_setfield(L, -2, namespace);                             \
> +} while(0)
> +
>  	/* Signals. */
> -	lua_newtable(L);
> -	for (int i = 0; popen_lua_signals[i].signame != NULL; ++i) {
> -		lua_pushinteger(L, popen_lua_signals[i].signo);
> -		lua_setfield(L, -2, popen_lua_signals[i].signame);
> -	}
> -	lua_setfield(L, -2, "signal");
> -
> +	setconsts(L, "signal", popen_lua_signals, signame, number, signo);
>  	/* Stream actions. */
> -	lua_newtable(L);
> -	for (int i = 0; popen_lua_actions[i].name != NULL; ++i) {
> -		lua_pushstring(L, popen_lua_actions[i].value);
> -		lua_setfield(L, -2, popen_lua_actions[i].name);
> -	}
> -	lua_setfield(L, -2, "opts");
> -
> +	setconsts(L, "opts", popen_lua_actions, name, string, value);
>  	/* Stream statuses. */
> -	lua_newtable(L);
> -	for (int i = 0; popen_lua_stream_statuses[i].name != NULL; ++i) {
> -		lua_pushstring(L, popen_lua_stream_statuses[i].value);
> -		lua_setfield(L, -2, popen_lua_stream_statuses[i].name);
> -	}
> -	lua_setfield(L, -2, "stream");
> -
> +	setconsts(L, "stream", popen_lua_stream_statuses, name, string, value);
>  	/* Process states. */
> -	lua_newtable(L);
> -	for (int i = 0; popen_lua_states[i].name != NULL; ++i) {
> -		lua_pushstring(L, popen_lua_states[i].value);
> -		lua_setfield(L, -2, popen_lua_states[i].name);
> -	}
> -	lua_setfield(L, -2, "state");
> +	setconsts(L, "state", popen_lua_states, name, string, value);
> +
> +#undef setconsts
>  }

You know, I'm very suspectful for macros 'to safe typing'. Whether it'll
ease reading? Don't sure.

If you don't mind, I'll think about this later.

(postponed)

> > diff --git a/src/lua/popen.h b/src/lua/popen.h
> > new file mode 100644
> > index 000000000..e23c5d7e5
> > --- /dev/null
> > +++ b/src/lua/popen.h
> > @@ -0,0 +1,44 @@
> > +#ifndef TARANTOOL_LUA_POPEN_H_INCLUDED
> > +#define TARANTOOL_LUA_POPEN_H_INCLUDED
> 
> Minor: AFAIR, we decided to use pragma, but I see no word about it in
> our C style guide[2].

There were some discussions, but:

* Nothing was changed in the guide.
* Other code was not changed to fit this style.

Since personally I don't like 'pragma once' and the style document says
'Use header guards' I prefer to keep current way for the new code.

> [1]: https://github.com/tarantool/luajit/blob/tarantool/src/lj_tab.c#L694695
> [2]: https://www.tarantool.io/en/doc/2.2/dev_guide/c_style_guide/

Re: [Tarantool-patches] [PATCH 0/2] Popen Lua module

[Kirill Yukhin]

Hello,

On 18 апр 07:13, Alexander Turenko wrote:
> The patchset implements popen Lua module upward existing backend popen
> engine.
> 
> The first patch changes popen_delete() behaviour to always free
> resources even when killing a process (or a process group) fails. We
> (Alexander, Cyrill and Igor) revisited the contract for closing the
> handle after observing killpg() behaviour on a group of zombie processes
> on Mac OS. I added those details to API documentation comments to don't
> surprise a user with this.
> 
> The first patch continues previous preliminary popen engine series:
> 
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015609.html
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015888.html
> 
> The second patch implements the Lua API for popen. It is quite
> strightforward and should be considered as very basic implementation. We
> plan to enhance it further in the upcoming releases (issues are to be
> filed).
> 
> The main goal of the module it to provide popen implementation that is
> integrated into our event loop (similar to fio and socket modules). Side
> goal is to provide ability to feed data to a child process input and
> read data from its output and so work with streaming programs (like
> `grep`) and even interactive programs (like `python -i` interpreter).
> 
> Let's consider the API of module as beta: it may be change in
> backward-incompatible manner in future releases if it will be valuable
> enough.
> 
> It seems the main application of the module it to write various testing
> code and so the API should be extended in the future with convenient
> shortcuts: developers usually don't very like writting tests and it
> should be at least more-or-less convenient.
> 
> I plan to extend the API for reading with ability to read certain amount
> of bytes, to read line-by-line (or based on other delimiter), to read
> into a buffer (ibuf): like it is implemented in the socket module. I
> plan to share an RFC document about read streams.
> 
> ph:wait() should gain ability to set a timeout and should be rewritten
> to use triggers / fiber conds instead of check-yield-again loop.
> 
> ----
> 
> https://github.com/tarantool/tarantool/issues/4031
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-13-full-ci

I've checked your patchset into master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Igor Munkin]

Sasha,

On 20.04.20, Alexander Turenko wrote:
> Thank you, Igor!
> 
> I made simple changes and postponed ones where I need a bit more time.
> Hope, you don't mind.

Totally OK with it.

> 
> Marked postponed questions as (postponed) in the email.
> 
> My answers are below.
> 
> WBR, Alexande Turenko.
> 

<snipped>

> 
> > > +static int
> > > +luaT_push_string_noxc(struct lua_State *L, char *str, size_t len)
> > > +{
> > > +	lua_pushcfunction(L, luaT_push_string_noxc_wrapper);
> > > +	lua_pushlightuserdata(L, str);
> > > +	lua_pushinteger(L, len);
> > > +	return luaT_call(L, 2, 1);
> > > +}
> > 
> > Minor: IMHO luaT_pushstring_* is more Lua-like naming.
> 
> It is from past agreements too: we discussed this with Vladimir D. and
> agreed to split 'foo' and 'bar' in 'lua*_foo_bar_baz()', when there is
> 'baz'. When there is no 'baz', it is 'lua*_foobar()'.

Are there any docs/guides/notes as a result of this discussion?

> 

<snipped>

> 
> > > +
> > > +		lua_getfield(L, 2, "timeout");
> > > +		if (!lua_isnil(L, -1) &&
> > > +		    (timeout = luaT_check_timeout(L, -1)) < 0.0)
> > > +			goto usage;
> > > +		lua_pop(L, 1);
> > 
> > Minor: I see this passage only in two similar places, so I propose to
> > move everything to a timeout-related helper.
> 
> luaT_check_timeout() is already this helper. Don't sure it worth to add
> one more wrapping level.

I mean you can also move there all table manipulations and checks.

> 

<snipped>

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Alexander Turenko]

> > > > +static int
> > > > +luaT_push_string_noxc(struct lua_State *L, char *str, size_t len)
> > > > +{
> > > > +	lua_pushcfunction(L, luaT_push_string_noxc_wrapper);
> > > > +	lua_pushlightuserdata(L, str);
> > > > +	lua_pushinteger(L, len);
> > > > +	return luaT_call(L, 2, 1);
> > > > +}
> > > 
> > > Minor: IMHO luaT_pushstring_* is more Lua-like naming.
> > 
> > It is from past agreements too: we discussed this with Vladimir D. and
> > agreed to split 'foo' and 'bar' in 'lua*_foo_bar_baz()', when there is
> > 'baz'. When there is no 'baz', it is 'lua*_foobar()'.
> 
> Are there any docs/guides/notes as a result of this discussion?

Nope.

> > > > +
> > > > +		lua_getfield(L, 2, "timeout");
> > > > +		if (!lua_isnil(L, -1) &&
> > > > +		    (timeout = luaT_check_timeout(L, -1)) < 0.0)
> > > > +			goto usage;
> > > > +		lua_pop(L, 1);
> > > 
> > > Minor: I see this passage only in two similar places, so I propose to
> > > move everything to a timeout-related helper.
> > 
> > luaT_check_timeout() is already this helper. Don't sure it worth to add
> > one more wrapping level.
> 
> I mean you can also move there all table manipulations and checks.

But fail branch do a function specific logic. I don't sure here. It
seems we can pass an error string to the function.

However lua*_check_foo() pattern is used across tarantool (see
luaL_checkibuf() for example) and timeout helper may become common
sooner or later.

WBR, Alexander Turenko.