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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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/