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

[Igor Munkin <imun@tarantool.org>]

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