From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 E108E4696C3 for ; Mon, 20 Apr 2020 01:24:39 +0300 (MSK) Date: Mon, 20 Apr 2020 01:24:34 +0300 From: Alexander Turenko Message-ID: <20200419222434.nnus7jnm2hgmeq6p@tkn_work_nb> References: <20200419123845.GP8314@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200419123845.GP8314@tarantool.org> 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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Thanks for the careful proofreading! Aside of changes below I changed `popen.stream.OPENED (== 'opened')` to `popen.stream.OPEN (== 'open')` to match english grammar. Same for statuses -> status (yep, 'statuses' is allowed in English, but it is ugly). Force-pushed Totktonada/gh-4031-popen-13-full-ci. WBR, Alexander Turenko. ---- > > The `popen` module provides two functions to create that named popen > > object `popen.shell` which is the similar to libc `popen` syscall and > > Typo: I guess a colon is missing here. Thanks! Fixed. > > Example: > > > > | local popen = require('popen') > > | > > | local ph = popen.shell('date', 'r') > > | local date = ph:read():rstrip() > > Minor: IMHO call might lead to a misuse, considering there is > no word about it in description below. I guess you can drop it. It is quite minor IMHO, but I don't mind. Added the explanation to .shell() function description in the code and the commit message: diff --git a/src/lua/popen.c b/src/lua/popen.c index 40f4343eb..8ae01b3f1 100644 --- a/src/lua/popen.c +++ b/src/lua/popen.c @@ -1408,6 +1396,18 @@ err: * * Execute 'sh -c date' command, read the output and close the * popen object. + * + * Unix defines a text file as a sequence of lines, each ends + * with the newline symbol. The same convention is usually + * applied for a text output of a command (so when it is + * redirected to a file, the file will be correct). + * + * However internally an application usually operates on + * strings, which are NOT newline terminated (e.g. literals + * for error messages). The newline is usually added right + * before a string is written to the outside world (stdout, + * console or log). :rstrip() in the example above is shown + * for this sake. */ static int lbox_popen_shell(struct lua_State *L) I left .new() description intact: it should be described once and .shell() going first in the documentation request (commit message). > > Execute a child program in a new process. > > > > @param argv an array of a program to run with > > command line options, mandatory; > > absolute path to the program is required > > Absolute path is not required when shell option is set to . Good catch! Added to the comment in the code and to the commit message: diff --git a/src/lua/popen.c b/src/lua/popen.c index 2aa0a5851..bf06bbfa7 100644 --- a/src/lua/popen.c +++ b/src/lua/popen.c @@ -1114,6 +1114,7 @@ luaT_popen_parse_mode(struct lua_State *L, int idx) * @param argv an array of a program to run with * command line options, mandatory; * absolute path to the program is required + * when @a opts.shell is false (default) * * @param opts table of options * > > @param opts.env an array of environment variables to > > be used inside a process; > > - when is not set then the current > > environment is inherited; > > - if set then the environment will be > > replaced > > - if set to an empty array then the > > environment will be dropped > > Minor: I looks more convenient to sort the lines above the following > way: > * when is not set <...> > * if set to an empty table <...> > * if set to a non-empty table <...> I don't mind. Reordered in the code and the commit message. Also I rephrased the description a bit to highlight the fact that the parameter is key-value mapping, not an array. diff --git a/src/lua/popen.c b/src/lua/popen.c index bf06bbfa7..f17e3faf1 100644 --- a/src/lua/popen.c +++ b/src/lua/popen.c @@ -1134,14 +1134,16 @@ luaT_popen_parse_mode(struct lua_State *L, int idx) * feed data from/to the fd to parent * using a pipe * - * @param opts.env an array of environment variables to - * be used inside a process; + * @param opts.env a table of environment variables to + * be used inside a process; key is a + * variable name, value is a variable + * value. * - when is not set then the current * environment is inherited; + * - if set to an empty table then the + * environment will be dropped * - if set then the environment will be * replaced - * - if set to an empty array then the - * environment will be dropped * * @param opts.shell (boolean, default: false) * true run a child process via > > @param opts.setsid (boolean, default: false) > > true start executable inside a new > > session > > false don't do that > > Minor: I reword it in more clear way below: > | @param opts.setsid (boolean, default: false) > | true run the program in a new > | session > | false run the program in the > | tarantool instance's > | session So I would add "...and process group". Changed: diff --git a/src/lua/popen.c b/src/lua/popen.c index f17e3faf1..ec8c95662 100644 --- a/src/lua/popen.c +++ b/src/lua/popen.c @@ -1151,9 +1151,11 @@ luaT_popen_parse_mode(struct lua_State *L, int idx) * false call the executable directly * * @param opts.setsid (boolean, default: false) - * true start executable inside a new + * true run the program in a new * session - * false don't do that + * false run the program in the + * tarantool instance's + * session and process group * * @param opts.close_fds (boolean, default: true) * true close all inherited fds from a > > The returned handle is subject to garbage collection, but > > it may be freed explicitly using :close(). SIGKILL is sent > > to the child process at :close() / GC if it is alive and > > opts.keep_child is not set. > > Minor: Let's adjust the terminology. What do you mean speaking about > "handle"? If it's a GC object, then there is no way to release it > manually or "explicitly", only via platform GC. If it's a GC object > *payload* then it's not managed by Lua GC and need to be explicitly > released via special method or the corresponding __gc metamethod. My intention is to give two ideas here: * Feel free to use explicit :close() or lean on Lua GC. * :close() / GC handler kills the child process. But being strict in terms and wording is goal for me too, so I reword the paragraph to eliminate phrases, which requires a GC object / a GC object payload as an object / a subject. Changed in the code and the commit: diff --git a/src/lua/popen.c b/src/lua/popen.c index ec8c95662..31b4bc2b0 100644 --- a/src/lua/popen.c +++ b/src/lua/popen.c @@ -1184,10 +1184,11 @@ luaT_popen_parse_mode(struct lua_State *L, int idx) * :close() or collecting of the * handle by Lua GC * - * The returned handle is subject to garbage collection, but - * it may be freed explicitly using :close(). SIGKILL is sent - * to the child process at :close() / GC if it is alive and - * opts.keep_child is not set. + * The returned handle provides :close() method to explicitly + * release all occupied resources (including the child process + * itself if @a opts.keep_child is not set). However if the + * method is not called for a handle during its lifetime, the + * same freeing actions will be triggered by Lua GC. * * It is recommended to use opts.setsid + opts.group_signal * if a child process may spawn its own childs and they all > > Example 1: > > > > | local popen = require('popen') > > | > > | local ph = popen.new({'/bin/date'}, { > > | stdout = popen.opts.PIPE, > > | }) > > | local date = ph:read():rstrip() > > | ph:close() > > | print(date) -- Thu 16 Apr 2020 01:40:56 AM MSK > > > > Execute 'date' command, read the result and close the > > popen object. > > Minor: Again, no mention about rstrip. I think once is enough. Added the popen.shell() description. > > Example 4: > > > > | local function call_jq(stdin, filter) > > Minor: look a bit misleading. What about , , etc? Okay, 'input'. diff --git a/src/lua/popen.c b/src/lua/popen.c index 31b4bc2b0..bf79a5d29 100644 --- a/src/lua/popen.c +++ b/src/lua/popen.c @@ -1270,7 +1270,7 @@ luaT_popen_parse_mode(struct lua_State *L, int idx) * * Example 4: * - * | local function call_jq(stdin, filter) + * | local function call_jq(input, filter) * | -- Start jq process, connect to stdin, stdout and stderr. * | local jq_argv = {'/usr/bin/jq', '-M', '--unbuffered', filter} * | local ph, err = popen.new(jq_argv, { @@ -1281,7 +1281,7 @@ luaT_popen_parse_mode(struct lua_State *L, int idx) * | if ph == nil then return nil, err end * | * | -- Write input data to child's stdin and send EOF. - * | local ok, err = ph:write(stdin) + * | local ok, err = ph:write(input) * | if not ok then return nil, err end * | ph:shutdown({stdin = true}) * | > > | -- Read diagnostics from stderr if any. > > | local err = ph:read({stderr = true}) > > | if err ~= '' then > > | ph:close() > > | return nil, err > > | end > > | > > | -- Glue all chunks, strip trailing newline. > > Side note: This comment is definitely needed near calls above. Added once, to popen.shell() example: diff --git a/src/lua/popen.c b/src/lua/popen.c index bf79a5d29..6cfb268a1 100644 --- a/src/lua/popen.c +++ b/src/lua/popen.c @@ -1395,9 +1395,16 @@ err: * * | local popen = require('popen') * | + * | -- Run the program and save its handle. * | local ph = popen.shell('date', 'r') + * | + * | -- Read program's output, strip trailing newline. * | local date = ph:read():rstrip() + * | + * | -- Free resources. The process is killed (but 'date' + * | -- exits itself anyway). * | ph:close() + * | * | print(date) * * Execute 'sh -c date' command, read the output and close the > > If a process writes large text to stderr, it may fill out > > stderr pipe buffer and block in write(2, ...). So we need to > > read stderr too in another fiber to handle this case. > > Typo: s/stderr too in another fiber/stderr in another fiber too/. I want to avoid possible understanding as 'read stderr here and in another fiber too'. I can just remove 'too'. I also changed 'block' to 'stuck': diff --git a/src/lua/popen.c b/src/lua/popen.c index 6cfb268a1..6835286c7 100644 --- a/src/lua/popen.c +++ b/src/lua/popen.c @@ -1318,8 +1318,8 @@ luaT_popen_parse_mode(struct lua_State *L, int idx) * another fiber (start it before a first :write()). * * If a process writes large text to stderr, it may fill out - * stderr pipe buffer and block in write(2, ...). So we need to - * read stderr too in another fiber to handle this case. + * stderr pipe buffer and stuck in write(2, ...). So we need + * to read stderr in a separate fiber to handle this case. */ static int lbox_popen_new(struct lua_State *L) > > Read data from stdout or stderr streams with @a timeout. > > By default it reads from stdout. Set @a opts.stderr to > > `true` to read from stderr. > > If only is set to true, is stdout output also captured? Nope. There is an error about it in the list below, but, yep, it worth to highlight it. Added the note: diff --git a/src/lua/popen.c b/src/lua/popen.c index 6835286c7..a071065b4 100644 --- a/src/lua/popen.c +++ b/src/lua/popen.c @@ -1647,6 +1647,9 @@ lbox_popen_wait(struct lua_State *L) * By default it reads from stdout. Set @a opts.stderr to * `true` to read from stderr. * + * It is not possible to read from stdout and stderr both in + * one call. Set either @a opts.stdout or @a opts.stderr. + * * Raise an error on incorrect parameters or when the fiber is * cancelled: * > > Return `nil, err` on a failure. Possible reasons: > > > > - SocketError: an IO error occurs at read(). > > SocketError on pipe looks ridiculous to me. Maybe an IO one? It comes from coio.cc. It would be good to refactor it someday, but not tonight :) > > `popen_handle:shutdown(opts) -> true` > > Minor: Considering the method description, looks more convenient > name here. On the other hand name might be misguiding one > considering feof(3) standart C function. :close() for ph.{stdin,stdout,stderr} would be ideal, but we don't have something like separate read / write streams now. We have bidirectional handle for reading and writing, so the nearest counterpart would be socket's shutdown(). More on this: closing of stdin is EOF for a child, but closing of stdout / stderr will lead to SIGPIPE (+EPIPE in errno) at write() in the child. I think the name we have now is good. > > `popen_handle:info() -> res, err` > > is never returned. Good catch! Fixed. > > `popen_handle:wait() -> res, err` > > is never returned. Thanks! Fixed.