[Tarantool-patches] [PATCH 2/2] popen: add popen Lua module
Igor Munkin
imun at tarantool.org
Mon Apr 20 03:57:40 MSK 2020
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 at gmail.com>
> Co-developed-by: Igor Munkin <imun at 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
More information about the Tarantool-patches
mailing list