From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 6D3666EC5F; Tue, 2 Mar 2021 20:05:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6D3666EC5F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614704758; bh=tbyeqTqagkugLbnmK1CikHqBcq03D3G2KOrG/MGYnwU=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=nW2OUxsHTNTr9Z2HMgKek+vK0p2g8MZd7y6UkuSBcZOIVA7voupuW7JUzvju0PqFz bv0F8tlxgVHQr8dtTxkBqBP2kHgr44ChESisZq7NGRV5bxDR60u27hvShL77WWnlT1 u2evZCWBKYsQjonvz3zgJ3zvWRpobCreWziS3Ce0= Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 274606EC5F for ; Tue, 2 Mar 2021 20:05:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 274606EC5F Received: by smtp33.i.mail.ru with esmtpa (envelope-from ) id 1lH8T5-0007yF-V6; Tue, 02 Mar 2021 20:05:56 +0300 To: Leonid Vasiliev , Alexander Turenko , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org References: <612d38a3f3349c1c8cb6d3f549fd85720b7a1a23.1613674486.git.artemreyt@tarantool.org> Message-ID: <73fdad5e-f886-56cd-4dde-65214b8a58c6@tarantool.org> Date: Tue, 2 Mar 2021 20:05:55 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: ru X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92A98208ECBDD29F592DFB44E0F725E61F2319814AA890CAA182A05F538085040593CFF32B8DC4FF809897DCE18290201EF60AEE73E3A3C1415B02CE84AF38834 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F68DA2F3749BB650EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378997215BCAA11D778638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FC6C97C573DA3A27DB12B44A595DD1B010F19898B23980C755389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6D082881546D93491CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249F99AF4D8A163DCE876E601842F6C81A12EF20D2F80756B5F7E9C4E3C761E06A776E601842F6C81A127C277FBC8AE2E8BE6A2F950A10922ED3AA81AA40904B5D9DBF02ECDB25306B2B25CBF701D1BE8734AD6D5ED66289B5278DA827A17800CE77DCDFB3399A2F72893EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B6B42AA39C143418A6089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E7028C9DFF55498CEFB0BD9CCCA9EDD067B1EDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5521D186533969C900294945B7596877093C635276344289DD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344F2126FF337FA6513DDF98C4F7F0C3D7F37BA6E2CE93CBEB51363A4C1439E3C18B02964E27AC7A231D7E09C32AA3244C4824FFE89E002F1C8A4C6D60B29631B2CE0B41342B755BCD927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnllmSZCgRqvDwB1FsFoNjA== X-Mailru-Sender: 65CD03CFE33A0EC7DB9F8DC6996C93071FA1C58AFFEE6C74402C29D88E86A5AEA9CCB844FDC0014992F02B39BF137DED0A8ED71B308007E347E5C1256AE813B3E2FBE92986FDCCC89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCHv2 2/2] lua: fix tarantool -e always enters interactive mode X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Artem via Tarantool-patches Reply-To: Artem Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hello! Thanks for your review! Branch is updated. See my answers below. 01.03.2021 18:10, Leonid Vasiliev пишет: > HI! Thank you for the patch. > Generally OK. > I'll paste the last variation of the diff with comments. > > > From 9b04075006f24a8226aca8398ae643b8f8cd8f05 Mon Sep 17 00:00:00 2001 > Message-Id: > <9b04075006f24a8226aca8398ae643b8f8cd8f05.1614609240.git.lvasiliev@tarantool.org> > In-Reply-To: > References: > From: Artem Starshov > Date: Tue, 19 Jan 2021 23:21:05 +0300 > Subject: [PATCH 2/2] lua: fix tarantool -e always enters interactive mode > Cc: tarantool-patches@dev.tarantool.org > > The reason why tarantool -e always enters interactive mode is that > statement after option -e isn't considered as a script. > > In man PUC-Rio lua there are different names for statement -e (stat) > and script, but they have the same behavior regarding interactive > mode. (Also cases, when interpreter loads stdin, have the same > behaviour). > > Fixes #5040 > --- >  src/lib/core/errinj.h                         |   1 + >  src/lua/init.c                                |  20 ++- >  ...40-always-enters-interactive-mode.test.lua | 131 ++++++++++++++++++ >  test/box/errinj.result                        |   1 + >  4 files changed, 149 insertions(+), 4 deletions(-) >  create mode 100755 > test/app-tap/gh-5040-always-enters-interactive-mode.test.lua > > diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h > index d76aedf7a..d5fdbc209 100644 > --- a/src/lib/core/errinj.h > +++ b/src/lib/core/errinj.h > @@ -149,6 +149,7 @@ struct errinj { >      _(ERRINJ_AUTO_UPGRADE, ERRINJ_BOOL, {.bparam = false})\ >      _(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_BOOL, {.bparam = false}) \ >      _(ERRINJ_APPLIER_SLOW_ACK, ERRINJ_BOOL, {.bparam = false}) \ > +    _(ERRINJ_STDIN_ISATTY, ERRINJ_INT, {.iparam = -1}) \ > >  ENUM0(errinj_id, ERRINJ_LIST); >  extern struct errinj errinjs[]; > diff --git a/src/lua/init.c b/src/lua/init.c > index 25e8884a6..1a95b9748 100644 > --- a/src/lua/init.c > +++ b/src/lua/init.c > @@ -62,6 +62,7 @@ >  #include "lua/swim.h" >  #include "lua/decimal.h" >  #include "digest.h" > +#include "errinj.h" >  #include > >  #include > @@ -583,6 +584,7 @@ run_script_f(va_list ap) >       */ >      struct diag *diag = va_arg(ap, struct diag *); >      bool aux_loop_is_run = false; > +    bool option_e_ran = false; > > Comment: > For a boolean variable name typically `is` is used. > Maybe `is_option_e_ran`. 1. Done. > >      /* >       * Load libraries and execute chunks passed by -l and -e > @@ -613,6 +615,7 @@ run_script_f(va_list ap) >              if (luaT_call(L, 0, 0) != 0) >                  goto error; >              lua_settop(L, 0); > +            option_e_ran = true; >              break; >          default: >              unreachable(); /* checked by getopt() in main() */ > @@ -627,25 +630,34 @@ run_script_f(va_list ap) >      fiber_sleep(0.0); >      aux_loop_is_run = true; > > +    int is_a_tty; > +    struct errinj *inj = errinj(ERRINJ_STDIN_ISATTY, ERRINJ_INT); > > Comment: > I think it would be nice to add a comment as to why > `ERRINJ_STDIN_ISATTY` is not a boolean. > 2. Done. > + if (inj != NULL && inj->iparam >= 0) { > +        is_a_tty = inj->iparam; > +    } else { > +        is_a_tty = isatty(STDIN_FILENO); > +    } > + >      if (path && strcmp(path, "-") != 0 && access(path, F_OK) == 0) { >          /* Execute script. */ >          if (luaL_loadfile(L, path) != 0) >              goto luajit_error; >          if (lua_main(L, argc, argv) != 0) >              goto error; > -    } else if (!isatty(STDIN_FILENO) || (path && strcmp(path, "-") == > 0)) { > +    } else if (!is_a_tty || (path && strcmp(path, "-") == 0)) { >          /* Execute stdin */ >          if (luaL_loadfile(L, NULL) != 0) >              goto luajit_error; >          if (lua_main(L, argc, argv) != 0) >              goto error; > -    } else { > +    } else if (!option_e_ran) { >          interactive = true; >      } > >      /* > -     * Start interactive mode when it was explicitly requested > -     * by "-i" option or stdin is TTY or there are no script. > +     * Start interactive mode in any of the cases: > +     * - it was explicitly requested by "-i" option; > +     * - stdin is TTY and there are no script (-e is considered as a > script). >       */ >      if (interactive) { >          say_crit("%s %s\ntype 'help' for interactive help", > diff --git > a/test/app-tap/gh-5040-always-enters-interactive-mode.test.lua > b/test/app-tap/gh-5040-always-enters-interactive-mode.test.lua > new file mode 100755 > index 000000000..df3ee2f2d > --- /dev/null > +++ b/test/app-tap/gh-5040-always-enters-interactive-mode.test.lua > @@ -0,0 +1,131 @@ > +#!/usr/bin/env tarantool > + > +local process_timeout = require('process_timeout') > +local ffi = require('ffi') > +local tap = require('tap') > +local fio = require('fio') > + > +-- > +-- Tests to check if the tarantool binary enters > +-- interactive mode or not. > +-- > + > +local build_target = require('tarantool').build.target > +local is_debug = build_target:match('Debug$') ~= nil > + > +local TARANTOOL_PATH = arg[-1] > +local output_file = fio.abspath('out.txt') > +local cmd_end = (' >%s & echo $!'):format(output_file) > + > +-- Like a default timeout for `cond_wait` in test-run > +local process_waiting_timeout = 60.0 > +local file_read_timeout = 60.0 > +local file_read_interval = 0.2 > +local file_open_timeout = 60.0 > + > +-- Each testcase consists of: > +--  * cmd_args - command line arguments for tarantool binary > +--  * stdin - stdin for tarantool > +--  * interactive - true if interactive mode expected > +--  * empty_output - true if command should have empty output > +local testcases = { > +    { > +        cmd_args = '', > +        stdin = 'tty', > +        interactive = true > +    }, > +    { > +        cmd_args = '', > +        stdin = '/dev/null', > +        interactive = false, > +        empty_output = true > +    }, > + > +    { > +        cmd_args = ' -e "print(_VERSION)"', > +        stdin = 'tty', > +        interactive = false > +    }, > +    { > +        cmd_args = ' -e "print(_VERSION)"', > +        stdin = '/dev/null', > +        interactive = false > +    }, > + > +    { > +        cmd_args = ' -i -e "print(_VERSION)"', > +        stdin = 'tty', > +        interactive = true > +    }, > +    { > +        cmd_args = ' -i -e "print(_VERSION)"', > +        stdin = '/dev/null', > +        interactive = true > +    } > +} > + > +local test = tap.test('gh-5040') > + > +if not is_debug then > > Comment: > AFAIU `release_disabled` from "suite.ini" is used for this purpose. 3. Unfortunately, in app-tap directory this option doesn't work. https://github.com/tarantool/test-run/issues/199 > + test:plan(1) > +    test:skip('This test runs only with Debug build. Build: ' .. > build_target) > +    os.exit(test:check() and 0 or 1) > +end > + > +test:plan(#testcases) > +for _, cmd in pairs(testcases) do > +    local full_cmd = '' > +    if cmd.stdin == 'tty' then > +        cmd.stdin = '' > +        full_cmd = 'ERRINJ_STDIN_ISATTY=1 ' > +    else > +        cmd.stdin = '< ' .. cmd.stdin > +    end > + > +    local full_cmd = full_cmd .. ('%s %s %s %s'):format( > +            TARANTOOL_PATH, > +            cmd.cmd_args, > +            cmd.stdin, > +            cmd_end > +    ) > +    test:test(full_cmd, function(test) > +        test:plan(cmd.interactive and 1 or 2) > + > +        local pid = tonumber(io.popen(full_cmd):read("*line")) > +        assert(pid, "pipe error for: " .. cmd.cmd_args) > + > +        local fh = process_timeout.open_with_timeout(output_file, > +                file_open_timeout) > +        assert(fh, 'error while opening ' .. output_file) > + > +        if cmd.interactive then > +            local data = process_timeout.read_with_timeout(fh, > +                    file_read_timeout, > +                    file_read_interval) > +            test:like(data, 'tarantool>', 'interactive mode detected') > +        else > +            local process_completed = > process_timeout.wait_process_completion( > +                    pid, > +                    process_waiting_timeout) > +            test:ok(process_completed, 'process completed') > + > +            -- If empty output expected, then don't wait full > file_read_timeout > +            -- for non-empty output_file, only a little time to be > sure that > +            -- file is empty. > +            local read_timeout = cmd.empty_output and file_read_interval > +                    or file_read_timeout > +            local data = process_timeout.read_with_timeout(fh, > read_timeout, > +                    file_read_interval) > +            if cmd.empty_output then > +                test:ok(#data == 0, 'output is empty') > +            else > +                test:unlike(data, 'tarantool>', 'iteractive mode > wasn\'t detected') > > Comment: > Maximal width of a code line is 80 symbols. 4. Done. > > +            end > +        end > +        if process_timeout.process_is_alive(pid) then ffi.C.kill(pid, > 9) end > > Comment: > From C code style: "Don't put multiple statements on a single line > unless you have something to hide". > Up to you. 5. Done. > + fh:close() > +        os.remove(output_file) > +    end) > +end > + > +os.exit(test:check() and 0 or 1) > diff --git a/test/box/errinj.result b/test/box/errinj.result > index b8c2476c3..a962dbe2d 100644 > --- a/test/box/errinj.result > +++ b/test/box/errinj.result > @@ -74,6 +74,7 @@ evals >    - ERRINJ_SNAP_COMMIT_DELAY: false >    - ERRINJ_SNAP_WRITE_DELAY: false >    - ERRINJ_SQL_NAME_NORMALIZATION: false > +  - ERRINJ_STDIN_ISATTY: -1 >    - ERRINJ_SWIM_FD_ONLY: false >    - ERRINJ_TESTING: false >    - ERRINJ_TUPLE_ALLOC: false