From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 249484696C3 for ; Mon, 20 Apr 2020 04:04:48 +0300 (MSK) Date: Mon, 20 Apr 2020 03:57:40 +0300 From: Igor Munkin Message-ID: <20200420005740.GV8314@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.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 > Co-developed-by: Igor Munkin > > Fixes #4031 > --- > src/CMakeLists.txt | 1 + > src/lua/init.c | 2 + > src/lua/popen.c | 2457 +++++++++++++++++++++++++++++++++++ > src/lua/popen.h | 44 + > test/app-tap/popen.test.lua | 605 +++++++++ > 5 files changed, 3109 insertions(+) > create mode 100644 src/lua/popen.c > create mode 100644 src/lua/popen.h > create mode 100755 test/app-tap/popen.test.lua > > diff --git a/src/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 @@ > + > +/* {{{ 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: , or . 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; > +} > + > + > +/** > + * 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; > +} > + > + > +/* }}} */ > + > +/* {{{ Push popen handle info to the Lua stack */ > + > + > +/** > + * 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; > +} > + > + > +/* }}} */ > + > +/* {{{ Parameter parsing */ > + > + > +/** > + * 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; > +} > + > + > +/* }}} */ > + > +/* {{{ Lua API functions and methods */ > + > + > +/** > + * 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; > +} > + > + > +/** > + * 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: and 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 = , " > + "stderr = , " > + "timeout = }])"); > + 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 = }])"); > + return luaT_error(L); > +} > + > +/** > + * 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, )"); Minor: "Bad params, use ph[]" 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; > +} > + > + > +/* }}} */ > + > +/* {{{ Module initialization */ > + > +void > +tarantool_lua_popen_init(struct lua_State *L) > +{ > + > + /* 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]. > -- > 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