[Tarantool-patches] [PATCHv2 0/2] lua: fix tarantool -e always enters interactive mode

[Tarantool-patches] [PATCHv2 0/2] lua: fix tarantool -e always enters interactive mode

[Artem Starshov via Tarantool-patches]

Issue: https://github.com/tarantool/tarantool/issues/5040
Branch: https://github.com/tarantool/tarantool/tree/artemreyt/gh-5040-interactive-mode-bug
CI: https://github.com/tarantool/tarantool/runs/1929677579

Previous patch version: https://lists.tarantool.org/pipermail/tarantool-patches/2021-January/022144.html

Changes in v2:
  * core:
    - added `part of #5040` to the commit message;
    - rename `errinj_scan_env()` -> `errinj_set_with_environment_vars()`;
    - whitespaces refactored;
    - add test for setting error injections via environment variables;
  * lua:
    - little code refactoring after Sergey Bronnikov review (renames and move
    includes in the alphabetical order);
    - fixed bug, when test failed under high concurrency. There were about 20
    fails with 100 workers. Now, it's ok, tested with 300 workers om CentOS and
    macOS.
    (Bug resolved with removing temporary output_file after each testcase iteration);
    - moved ChangeLog to the last commit.

Artem Starshov (2):
  core: add setting error injections via env
  lua: fix tarantool -e always enters interactive mode

 src/lib/core/errinj.c                         |  26 ++++
 src/lib/core/errinj.h                         |   6 +
 src/lua/init.c                                |  20 ++-
 src/main.cc                                   |   1 +
 ...40-always-enters-interactive-mode.test.lua | 131 ++++++++++++++++++
 .../errinj_set_with_enviroment_vars.test.lua  |  14 ++
 ...errinj_set_with_enviroment_vars_script.lua |  13 ++
 test/box/errinj.result                        |   1 +
 8 files changed, 208 insertions(+), 4 deletions(-)
 create mode 100755 test/app-tap/gh-5040-always-enters-interactive-mode.test.lua
 create mode 100755 test/box-tap/errinj_set_with_enviroment_vars.test.lua
 create mode 100644 test/box-tap/errinj_set_with_enviroment_vars_script.lua

-- 
2.28.0

[Tarantool-patches] [PATCHv2 1/2] core: add setting error injections via env

[Artem Starshov via Tarantool-patches]

Sometimes, it's useful to set error injections via environment
variables and this commit adds this opportunity.

e.g: `ERRINJ_WAL_WRITE=true tarantool` will be launched with
ERRINJ_WAL_WRITE setted to true.

Errinjs with bool parameters can should be set as "true", "TRUE",
"false" or "FALSE".

Errinjs with int or double parameters should be whole valid ("123s" is invalid).
e.g. for int or double: "123", "-1", "2.34", "+2.34".

Part of #5040
---
 src/lib/core/errinj.c                         | 26 +++++++++++++++++++
 src/lib/core/errinj.h                         |  5 ++++
 src/main.cc                                   |  1 +
 .../errinj_set_with_enviroment_vars.test.lua  | 14 ++++++++++
 ...errinj_set_with_enviroment_vars_script.lua | 13 ++++++++++
 5 files changed, 59 insertions(+)
 create mode 100755 test/box-tap/errinj_set_with_enviroment_vars.test.lua
 create mode 100644 test/box-tap/errinj_set_with_enviroment_vars_script.lua

diff --git a/src/lib/core/errinj.c b/src/lib/core/errinj.c
index d3aa0ca4f..3c1194f20 100644
--- a/src/lib/core/errinj.c
+++ b/src/lib/core/errinj.c
@@ -66,3 +66,29 @@ int errinj_foreach(errinj_cb cb, void *cb_ctx) {
 	}
 	return 0;
 }
+
+void errinj_set_with_environment_vars() {
+    for (enum errinj_id i = 0; i < errinj_id_MAX; i++) {
+        struct errinj *inj = &errinjs[i];
+        const char *env_value = getenv(inj->name);
+        if (!env_value || *env_value == '\0')
+            continue;
+
+        if (inj->type == ERRINJ_INT) {
+            char *end;
+            int64_t int_value = strtoll(env_value, &end, 10);
+            if (*end == '\0')
+                inj->iparam = int_value;
+        } else if (inj->type == ERRINJ_BOOL) {
+            if (strcmp(env_value, "false") == 0 || strcmp(env_value, "FALSE") == 0)
+                inj->bparam = false;
+            else if (strcmp(env_value, "true") == 0 || strcmp(env_value, "TRUE") == 0)
+                inj->bparam = true;
+        } else if (inj->type == ERRINJ_DOUBLE) {
+            char *end;
+            double double_value = strtod(env_value, &end);
+            if (*end == '\0')
+                inj->dparam = double_value;
+        }
+    }
+}
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 814c57c2e..d76aedf7a 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -168,6 +168,11 @@ typedef int (*errinj_cb)(struct errinj *e, void *cb_ctx);
 int
 errinj_foreach(errinj_cb cb, void *cb_ctx);
 
+/**
+ * Set injections by scanning ERRINJ_$(NAME) in environment variables
+ */
+void errinj_set_with_environment_vars();
+
 #ifdef NDEBUG
 #  define ERROR_INJECT(ID, CODE)
 #  define ERROR_INJECT_WHILE(ID, CODE)
diff --git a/src/main.cc b/src/main.cc
index 2fce81bb3..58a660689 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -710,6 +710,7 @@ main(int argc, char **argv)
 	memtx_tx_manager_init();
 	crypto_init();
 	systemd_init();
+	errinj_set_with_environment_vars();
 	tarantool_lua_init(tarantool_bin, main_argc, main_argv);
 
 	start_time = ev_monotonic_time();
diff --git a/test/box-tap/errinj_set_with_enviroment_vars.test.lua b/test/box-tap/errinj_set_with_enviroment_vars.test.lua
new file mode 100755
index 000000000..f52ebcc11
--- /dev/null
+++ b/test/box-tap/errinj_set_with_enviroment_vars.test.lua
@@ -0,0 +1,14 @@
+#!/usr/bin/env tarantool
+local fio = require('fio')
+
+-- Execute errinj_set_with_enviroment_vars_script.lua
+-- via tarantool with presetted environment variables.
+local TARANTOOL_PATH = arg[-1]
+local set_env_str = 'ERRINJ_TESTING=true ERRINJ_WAL_WRITE_PARTIAL=1 ERRINJ_VY_READ_PAGE_TIMEOUT=2.5'
+local script_file = fio.pathjoin(
+        os.getenv('PWD'),
+        'box-tap',
+        'errinj_set_with_enviroment_vars_script.lua')
+local shell_command = ('%s %s %s'):format(set_env_str, TARANTOOL_PATH, script_file)
+
+os.exit(os.execute(shell_command))
diff --git a/test/box-tap/errinj_set_with_enviroment_vars_script.lua b/test/box-tap/errinj_set_with_enviroment_vars_script.lua
new file mode 100644
index 000000000..b8903b907
--- /dev/null
+++ b/test/box-tap/errinj_set_with_enviroment_vars_script.lua
@@ -0,0 +1,13 @@
+-- Script for box-tap/errinj_set_with_enviroment_vars.test.lua test.
+
+local tap = require('tap')
+local errinj = box.error.injection
+
+local res = tap.test('set errinjs via environment variables', function(test)
+    test:plan(3)
+    test:is(errinj.get('ERRINJ_TESTING'), true, "set bool error injection")
+    test:is(errinj.get('ERRINJ_WAL_WRITE_PARTIAL'), 1, "set int error injection")
+    test:is(errinj.get('ERRINJ_VY_READ_PAGE_TIMEOUT'), 2.5, "set double error injection")
+end)
+
+os.exit(res and 0 or 1)
-- 
2.28.0

Re: [Tarantool-patches] [PATCHv2 1/2] core: add setting error injections via env

[Sergey Bronnikov via Tarantool-patches]

Hello,

thanks for the patch!

see my comments below

On 18.02.2021 22:30, Artem Starshov wrote:
> Sometimes, it's useful to set error injections via environment
> variables and this commit adds this opportunity.
>
> e.g: `ERRINJ_WAL_WRITE=true tarantool` will be launched with
> ERRINJ_WAL_WRITE setted to true.
>
> Errinjs with bool parameters can should be set as "true", "TRUE",
> "false" or "FALSE".
>
> Errinjs with int or double parameters should be whole valid ("123s" is invalid).
> e.g. for int or double: "123", "-1", "2.34", "+2.34".
>
> Part of #5040
> ---
>   src/lib/core/errinj.c                         | 26 +++++++++++++++++++
>   src/lib/core/errinj.h                         |  5 ++++
>   src/main.cc                                   |  1 +
>   .../errinj_set_with_enviroment_vars.test.lua  | 14 ++++++++++
>   ...errinj_set_with_enviroment_vars_script.lua | 13 ++++++++++
>   5 files changed, 59 insertions(+)
>   create mode 100755 test/box-tap/errinj_set_with_enviroment_vars.test.lua
>   create mode 100644 test/box-tap/errinj_set_with_enviroment_vars_script.lua
>
> diff --git a/src/lib/core/errinj.c b/src/lib/core/errinj.c
> index d3aa0ca4f..3c1194f20 100644
> --- a/src/lib/core/errinj.c
> +++ b/src/lib/core/errinj.c
> @@ -66,3 +66,29 @@ int errinj_foreach(errinj_cb cb, void *cb_ctx) {
>   	}
>   	return 0;
>   }
> +
> +void errinj_set_with_environment_vars() {
> +    for (enum errinj_id i = 0; i < errinj_id_MAX; i++) {
> +        struct errinj *inj = &errinjs[i];
> +        const char *env_value = getenv(inj->name);
> +        if (!env_value || *env_value == '\0')
> +            continue;
> +
> +        if (inj->type == ERRINJ_INT) {
> +            char *end;
> +            int64_t int_value = strtoll(env_value, &end, 10);
> +            if (*end == '\0')
> +                inj->iparam = int_value;
> +        } else if (inj->type == ERRINJ_BOOL) {
> +            if (strcmp(env_value, "false") == 0 || strcmp(env_value, "FALSE") == 0)

1. What happen if someone set boolean value to "False"?

May be it's worth to convert env_value to lowercase and then compare? 
The same question for "true".


> +                inj->bparam = false;
> +            else if (strcmp(env_value, "true") == 0 || strcmp(env_value, "TRUE") == 0)
> +                inj->bparam = true;
> +        } else if (inj->type == ERRINJ_DOUBLE) {
> +            char *end;
> +            double double_value = strtod(env_value, &end);
> +            if (*end == '\0')
> +                inj->dparam = double_value;
> +        }
> +    }
> +}
> diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
> index 814c57c2e..d76aedf7a 100644
> --- a/src/lib/core/errinj.h
> +++ b/src/lib/core/errinj.h
> @@ -168,6 +168,11 @@ typedef int (*errinj_cb)(struct errinj *e, void *cb_ctx);
>   int
>   errinj_foreach(errinj_cb cb, void *cb_ctx);
>   
> +/**
> + * Set injections by scanning ERRINJ_$(NAME) in environment variables
> + */
> +void errinj_set_with_environment_vars();
> +
>   #ifdef NDEBUG
>   #  define ERROR_INJECT(ID, CODE)
>   #  define ERROR_INJECT_WHILE(ID, CODE)
> diff --git a/src/main.cc b/src/main.cc
> index 2fce81bb3..58a660689 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -710,6 +710,7 @@ main(int argc, char **argv)
>   	memtx_tx_manager_init();
>   	crypto_init();
>   	systemd_init();
> +	errinj_set_with_environment_vars();
>   	tarantool_lua_init(tarantool_bin, main_argc, main_argv);
>   
>   	start_time = ev_monotonic_time();
> diff --git a/test/box-tap/errinj_set_with_enviroment_vars.test.lua b/test/box-tap/errinj_set_with_enviroment_vars.test.lua
> new file mode 100755
> index 000000000..f52ebcc11
> --- /dev/null
> +++ b/test/box-tap/errinj_set_with_enviroment_vars.test.lua
> @@ -0,0 +1,14 @@
> +#!/usr/bin/env tarantool
> +local fio = require('fio')
> +
> +-- Execute errinj_set_with_enviroment_vars_script.lua
> +-- via tarantool with presetted environment variables.
> +local TARANTOOL_PATH = arg[-1]
> +local set_env_str = 'ERRINJ_TESTING=true ERRINJ_WAL_WRITE_PARTIAL=1 ERRINJ_VY_READ_PAGE_TIMEOUT=2.5'
> +local script_file = fio.pathjoin(

2. variable name should reflect a purpose of variable.

name is about script file, when variable contains a path to a file.

> +        os.getenv('PWD'),
> +        'box-tap',
> +        'errinj_set_with_enviroment_vars_script.lua')
> +local shell_command = ('%s %s %s'):format(set_env_str, TARANTOOL_PATH, script_file)
> +
> +os.exit(os.execute(shell_command))
> diff --git a/test/box-tap/errinj_set_with_enviroment_vars_script.lua b/test/box-tap/errinj_set_with_enviroment_vars_script.lua
> new file mode 100644
> index 000000000..b8903b907
> --- /dev/null
> +++ b/test/box-tap/errinj_set_with_enviroment_vars_script.lua
> @@ -0,0 +1,13 @@
> +-- Script for box-tap/errinj_set_with_enviroment_vars.test.lua test.
> +
> +local tap = require('tap')
> +local errinj = box.error.injection
> +
> +local res = tap.test('set errinjs via environment variables', function(test)
> +    test:plan(3)
> +    test:is(errinj.get('ERRINJ_TESTING'), true, "set bool error injection")
> +    test:is(errinj.get('ERRINJ_WAL_WRITE_PARTIAL'), 1, "set int error injection")
> +    test:is(errinj.get('ERRINJ_VY_READ_PAGE_TIMEOUT'), 2.5, "set double error injection")
> +end)
> +
> +os.exit(res and 0 or 1)

Re: [Tarantool-patches] [PATCHv2 1/2] core: add setting error injections via env

[Artem via Tarantool-patches]

Hello,

thanks for the review!

>> +
>> +void errinj_set_with_environment_vars() {
>> +    for (enum errinj_id i = 0; i < errinj_id_MAX; i++) {
>> +        struct errinj *inj = &errinjs[i];
>> +        const char *env_value = getenv(inj->name);
>> +        if (!env_value || *env_value == '\0')
>> +            continue;
>> +
>> +        if (inj->type == ERRINJ_INT) {
>> +            char *end;
>> +            int64_t int_value = strtoll(env_value, &end, 10);
>> +            if (*end == '\0')
>> +                inj->iparam = int_value;
>> +        } else if (inj->type == ERRINJ_BOOL) {
>> +            if (strcmp(env_value, "false") == 0 || strcmp(env_value, 
>> "FALSE") == 0)
>
> 1. What happen if someone set boolean value to "False"?
>
> May be it's worth to convert env_value to lowercase and then compare? 
> The same question for "true".

1. Done. Case-insensetive value can be set to env variable now.


>> +-- Execute errinj_set_with_enviroment_vars_script.lua
>> +-- via tarantool with presetted environment variables.
>> +local TARANTOOL_PATH = arg[-1]
>> +local set_env_str = 'ERRINJ_TESTING=true ERRINJ_WAL_WRITE_PARTIAL=1 
>> ERRINJ_VY_READ_PAGE_TIMEOUT=2.5'
>> +local script_file = fio.pathjoin(
>
> 2. variable name should reflect a purpose of variable.
>
> name is about script file, when variable contains a path to a file.
2. Done. Renamed to path_to_test_file.

Re: [Tarantool-patches] [PATCHv2 1/2] core: add setting error injections via env

[Sergey Bronnikov via Tarantool-patches]

Hi,

On 20.02.2021 14:21, Artem wrote:
> Hello,
>
> thanks for the review!
>
>>> +
>>> +void errinj_set_with_environment_vars() {
>>> +    for (enum errinj_id i = 0; i < errinj_id_MAX; i++) {
>>> +        struct errinj *inj = &errinjs[i];
>>> +        const char *env_value = getenv(inj->name);
>>> +        if (!env_value || *env_value == '\0')
>>> +            continue;
>>> +
>>> +        if (inj->type == ERRINJ_INT) {
>>> +            char *end;
>>> +            int64_t int_value = strtoll(env_value, &end, 10);
>>> +            if (*end == '\0')
>>> +                inj->iparam = int_value;
>>> +        } else if (inj->type == ERRINJ_BOOL) {
>>> +            if (strcmp(env_value, "false") == 0 || 
>>> strcmp(env_value, "FALSE") == 0)
>>
>> 1. What happen if someone set boolean value to "False"?
>>
>> May be it's worth to convert env_value to lowercase and then compare? 
>> The same question for "true".
>
> 1. Done. Case-insensetive value can be set to env variable now.

Thanks!

It's worth to add different values for 'true'/'false' to test

for documentation and regression purposes.

>
>
>>> +-- Execute errinj_set_with_enviroment_vars_script.lua
>>> +-- via tarantool with presetted environment variables.
>>> +local TARANTOOL_PATH = arg[-1]
>>> +local set_env_str = 'ERRINJ_TESTING=true ERRINJ_WAL_WRITE_PARTIAL=1 
>>> ERRINJ_VY_READ_PAGE_TIMEOUT=2.5'
>>> +local script_file = fio.pathjoin(
>>
>> 2. variable name should reflect a purpose of variable.
>>
>> name is about script file, when variable contains a path to a file.
> 2. Done. Renamed to path_to_test_file.

Re: [Tarantool-patches] [PATCHv2 1/2] core: add setting error injections via env

[Artem via Tarantool-patches]

Hi,

20.02.2021 17:27, Sergey Bronnikov пишет:
> Hi,
>
> On 20.02.2021 14:21, Artem wrote:
>> Hello,
>>
>> thanks for the review!
>>
>>>> +
>>>> +void errinj_set_with_environment_vars() {
>>>> +    for (enum errinj_id i = 0; i < errinj_id_MAX; i++) {
>>>> +        struct errinj *inj = &errinjs[i];
>>>> +        const char *env_value = getenv(inj->name);
>>>> +        if (!env_value || *env_value == '\0')
>>>> +            continue;
>>>> +
>>>> +        if (inj->type == ERRINJ_INT) {
>>>> +            char *end;
>>>> +            int64_t int_value = strtoll(env_value, &end, 10);
>>>> +            if (*end == '\0')
>>>> +                inj->iparam = int_value;
>>>> +        } else if (inj->type == ERRINJ_BOOL) {
>>>> +            if (strcmp(env_value, "false") == 0 || 
>>>> strcmp(env_value, "FALSE") == 0)
>>>
>>> 1. What happen if someone set boolean value to "False"?
>>>
>>> May be it's worth to convert env_value to lowercase and then 
>>> compare? The same question for "true".
>>
>> 1. Done. Case-insensetive value can be set to env variable now.
>
> Thanks!
>
> It's worth to add different values for 'true'/'false' to test
>
> for documentation and regression purposes.

Done, added tests for "true", "True", "TRUE" (same for false) and tests 
for numeric injections with plus and minus signs.

Updates you can see on the branch.

Re: [Tarantool-patches] [PATCHv2 1/2] core: add setting error injections via env

[Sergey Bronnikov via Tarantool-patches]

Hello,

thanks for patch!

small nit: with patch below alignment looks more accurately.

--- a/test/box-tap/errinj_set_with_enviroment_vars.test.lua
+++ b/test/box-tap/errinj_set_with_enviroment_vars.test.lua
@@ -4,8 +4,8 @@ local fio = require('fio')
  -- Execute errinj_set_with_enviroment_vars_script.lua
  -- via tarantool with presetted environment variables.
  local TARANTOOL_PATH = arg[-1]
-local bool_env = 'ERRINJ_TESTING=true ERRINJ_WAL_IO=True 
ERRINJ_WAL_SYNC=TRUE' ..
-                ' ERRINJ_WAL_WRITE=false ERRINJ_INDEX_ALLOC=False 
ERRINJ_WAL_WRITE_DISK=FALSE'
+local bool_env = 'ERRINJ_TESTING=true ERRINJ_WAL_IO=True 
ERRINJ_WAL_SYNC=TRUE ' ..
+                 'ERRINJ_WAL_WRITE=false ERRINJ_INDEX_ALLOC=False 
ERRINJ_WAL_WRITE_DISK=FALSE'
  local integer_env = 'ERRINJ_WAL_WRITE_PARTIAL=2 
ERRINJ_WAL_FALLOCATE=+2 ERRINJ_WAL_WRITE_COUNT=-2'
  local double_env = 'ERRINJ_VY_READ_PAGE_TIMEOUT=2.5 
ERRINJ_VY_SCHED_TIMEOUT=+2.5 ERRINJ_RELAY_TIMEOUT=-2.5'
  local set_env_str = ('%s %s %s'):format(bool_env, integer_env, double_env)

On 24.02.2021 12:38, Artem wrote:
> Hi,
>
> 20.02.2021 17:27, Sergey Bronnikov пишет:
>> Hi,
>>
>> On 20.02.2021 14:21, Artem wrote:
>>> Hello,
>>>
>>> thanks for the review!
>>>
>>>>> +
>>>>> +void errinj_set_with_environment_vars() {
>>>>> +    for (enum errinj_id i = 0; i < errinj_id_MAX; i++) {
>>>>> +        struct errinj *inj = &errinjs[i];
>>>>> +        const char *env_value = getenv(inj->name);
>>>>> +        if (!env_value || *env_value == '\0')
>>>>> +            continue;
>>>>> +
>>>>> +        if (inj->type == ERRINJ_INT) {
>>>>> +            char *end;
>>>>> +            int64_t int_value = strtoll(env_value, &end, 10);
>>>>> +            if (*end == '\0')
>>>>> +                inj->iparam = int_value;
>>>>> +        } else if (inj->type == ERRINJ_BOOL) {
>>>>> +            if (strcmp(env_value, "false") == 0 || 
>>>>> strcmp(env_value, "FALSE") == 0)
>>>>
>>>> 1. What happen if someone set boolean value to "False"?
>>>>
>>>> May be it's worth to convert env_value to lowercase and then 
>>>> compare? The same question for "true".
>>>
>>> 1. Done. Case-insensetive value can be set to env variable now.
>>
>> Thanks!
>>
>> It's worth to add different values for 'true'/'false' to test
>>
>> for documentation and regression purposes.
>
> Done, added tests for "true", "True", "TRUE" (same for false) and 
> tests for numeric injections with plus and minus signs.
>
> Updates you can see on the branch.
>

Re: [Tarantool-patches] [PATCHv2 1/2] core: add setting error injections via env

[Leonid Vasiliev via Tarantool-patches]

HI! Thank you for the patch.
Seems like write a test is more difficult than fixing the bug)
Generally OK.
I'll paste the last variation of the diff with comments.


 From c2ff4adb95eda297e61ed6836f391a541ab08fd3 Mon Sep 17 00:00:00 2001
Message-Id: 
<c2ff4adb95eda297e61ed6836f391a541ab08fd3.1614609240.git.lvasiliev@tarantool.org>
In-Reply-To: <cover.1614609240.git.lvasiliev@tarantool.org>
References: <cover.1614609240.git.lvasiliev@tarantool.org>
From: Artem Starshov <artemreyt@tarantool.org>
Date: Tue, 26 Jan 2021 20:15:22 +0300
Subject: [PATCH 1/2] core: add setting error injections via env
Cc: tarantool-patches@dev.tarantool.org

Sometimes, it's useful to set error injections via environment
variables and this commit adds this opportunity.

e.g: `ERRINJ_WAL_WRITE=true tarantool` will be launched with
ERRINJ_WAL_WRITE setted to true.

Errinjs with bool parameters can be set to "true", "false",
"True", "False", "TRUE", "FALSE", etc. (case-insensitive variable).

Errinjs with int or double parameters should be whole valid ("123s" is 
invalid).
e.g. for int or double: "123", "-1", "2.34", "+2.34".

Part of #5040
---
  src/lib/core/errinj.c                         | 31 +++++++++++++++++
  src/lib/core/errinj.h                         |  5 +++
  src/main.cc                                   |  1 +
  .../errinj_set_with_enviroment_vars.test.lua  | 18 ++++++++++
  ...errinj_set_with_enviroment_vars_script.lua | 34 +++++++++++++++++++
  5 files changed, 89 insertions(+)
  create mode 100755 test/box-tap/errinj_set_with_enviroment_vars.test.lua
  create mode 100644 test/box-tap/errinj_set_with_enviroment_vars_script.lua

diff --git a/src/lib/core/errinj.c b/src/lib/core/errinj.c
index d3aa0ca4f..983ead681 100644
--- a/src/lib/core/errinj.c
+++ b/src/lib/core/errinj.c
@@ -28,6 +28,7 @@
   * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
   * SUCH DAMAGE.
   */
+#include <ctype.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
@@ -66,3 +67,33 @@ int errinj_foreach(errinj_cb cb, void *cb_ctx) {
  	}
  	return 0;
  }
+
+void errinj_set_with_environment_vars() {

Comment:
void errinj_set_with_environment_vars(void) see 
https://github.com/tarantool/tarantool/issues/4718

+    for (enum errinj_id i = 0; i < errinj_id_MAX; i++) {
+        struct errinj *inj = &errinjs[i];
+        const char *env_value = getenv(inj->name);
+        if (!env_value || *env_value == '\0')

Comment:
env_value == NULL 
(https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style)

+            continue;
+
+        if (inj->type == ERRINJ_INT) {
+            char *end;
+            int64_t int_value = strtoll(env_value, &end, 10);
+            if (*end == '\0')
+                inj->iparam = int_value;

Comment:
Seems like the `panic()` should be called in the case of `else()`. Here 
and in
other cases of parsing.

+        } else if (inj->type == ERRINJ_BOOL) {
+            char *lower_env_value = (char *)calloc(strlen(env_value) + 
1, sizeof(char));

Comment:
`(char *)` - unneeded in C.
Maximal width of a code line is 80 symbols.

+            for (size_t i = 0; env_value[i]; ++i)
+                lower_env_value[i] = tolower(env_value[i]);

Comment:
I suggest to use `strcasecmp()`.

+            if (strcmp(lower_env_value, "false") == 0)
+                inj->bparam = false;
+            else if (strcmp(lower_env_value, "true") == 0)
+                inj->bparam = true;
+            free(lower_env_value);
+        } else if (inj->type == ERRINJ_DOUBLE) {
+            char *end;
+            double double_value = strtod(env_value, &end);
+            if (*end == '\0')
+                inj->dparam = double_value;
+        }
+    }
+}
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 814c57c2e..d76aedf7a 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -168,6 +168,11 @@ typedef int (*errinj_cb)(struct errinj *e, void 
*cb_ctx);
  int
  errinj_foreach(errinj_cb cb, void *cb_ctx);

+/**
+ * Set injections by scanning ERRINJ_$(NAME) in environment variables
+ */
+void errinj_set_with_environment_vars();
+
  #ifdef NDEBUG
  #  define ERROR_INJECT(ID, CODE)
  #  define ERROR_INJECT_WHILE(ID, CODE)
diff --git a/src/main.cc b/src/main.cc
index 2fce81bb3..58a660689 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -710,6 +710,7 @@ main(int argc, char **argv)
  	memtx_tx_manager_init();
  	crypto_init();
  	systemd_init();
+	errinj_set_with_environment_vars();
  	tarantool_lua_init(tarantool_bin, main_argc, main_argv);

  	start_time = ev_monotonic_time();
diff --git a/test/box-tap/errinj_set_with_enviroment_vars.test.lua 
b/test/box-tap/errinj_set_with_enviroment_vars.test.lua
new file mode 100755
index 000000000..bffcc31e7
--- /dev/null
+++ b/test/box-tap/errinj_set_with_enviroment_vars.test.lua
@@ -0,0 +1,18 @@
+#!/usr/bin/env tarantool
+local fio = require('fio')
+
+-- Execute errinj_set_with_enviroment_vars_script.lua
+-- via tarantool with presetted environment variables.
+local TARANTOOL_PATH = arg[-1]

+local bool_env = 'ERRINJ_TESTING=true ERRINJ_WAL_IO=True 
ERRINJ_WAL_SYNC=TRUE ' ..
+                 'ERRINJ_WAL_WRITE=false ERRINJ_INDEX_ALLOC=False 
ERRINJ_WAL_WRITE_DISK=FALSE'
+local integer_env = 'ERRINJ_WAL_WRITE_PARTIAL=2 ERRINJ_WAL_FALLOCATE=+2 
ERRINJ_WAL_WRITE_COUNT=-2'
+local double_env = 'ERRINJ_VY_READ_PAGE_TIMEOUT=2.5 
ERRINJ_VY_SCHED_TIMEOUT=+2.5 ERRINJ_RELAY_TIMEOUT=-2.5'

Comment:
Maximal width of a code line is 80 symbols.

+local set_env_str = ('%s %s %s'):format(bool_env, integer_env, double_env)
+local path_to_test_file = fio.pathjoin(
+        os.getenv('PWD'),
+        'box-tap',
+        'errinj_set_with_enviroment_vars_script.lua')
+local shell_command = ('%s %s %s'):format(set_env_str, TARANTOOL_PATH, 
path_to_test_file)
+
+os.exit(os.execute(shell_command))
diff --git a/test/box-tap/errinj_set_with_enviroment_vars_script.lua 
b/test/box-tap/errinj_set_with_enviroment_vars_script.lua
new file mode 100644
index 000000000..f15085aa0
--- /dev/null
+++ b/test/box-tap/errinj_set_with_enviroment_vars_script.lua
@@ -0,0 +1,34 @@
+-- Script for box-tap/errinj_set_with_enviroment_vars.test.lua test.
+
+local tap = require('tap')
+local errinj = box.error.injection
+
+local test = tap.test('set errinjs via environment variables')
+
+test:plan(3)
+
+test:test('Set boolean error injections', function(test)
+    test:plan(6)
+    test:is(errinj.get('ERRINJ_TESTING'), true, 'true')
+    test:is(errinj.get('ERRINJ_WAL_IO'), true, 'True')
+    test:is(errinj.get('ERRINJ_WAL_SYNC'), true, 'TRUE')
+    test:is(errinj.get('ERRINJ_WAL_WRITE'), false, 'false')
+    test:is(errinj.get('ERRINJ_INDEX_ALLOC'), false, 'False')
+    test:is(errinj.get('ERRINJ_WAL_WRITE_DISK'), false, 'FALSE')
+end)
+
+test:test('Set integer error injections', function(test)
+    test:plan(3)
+    test:is(errinj.get('ERRINJ_WAL_WRITE_PARTIAL'), 2, '2')
+    test:is(errinj.get('ERRINJ_WAL_FALLOCATE'), 2, '+2')
+    test:is(errinj.get('ERRINJ_WAL_WRITE_COUNT'), -2, '-2')
+end)
+
+test:test('Set double error injections', function(test)
+    test:plan(3)
+    test:is(errinj.get('ERRINJ_VY_READ_PAGE_TIMEOUT'), 2.5, "2.5")
+    test:is(errinj.get('ERRINJ_VY_SCHED_TIMEOUT'), 2.5, "+2.5")
+    test:is(errinj.get('ERRINJ_RELAY_TIMEOUT'), -2.5, "-2.5")
+end)
+
+os.exit(test:check() and 0 or 1)
-- 
2.30.0

Re: [Tarantool-patches] [PATCHv2 1/2] core: add setting error injections via env

[Artem via Tarantool-patches]

Hello! Thanks for your review!

Branch is updated.

See my 'DONE's below)

01.03.2021 17:47, Leonid Vasiliev пишет:
> HI! Thank you for the patch.
> Seems like write a test is more difficult than fixing the bug)
It is what it is...
> Generally OK.
> I'll paste the last variation of the diff with comments.
>
>
> From c2ff4adb95eda297e61ed6836f391a541ab08fd3 Mon Sep 17 00:00:00 2001
> Message-Id: 
> <c2ff4adb95eda297e61ed6836f391a541ab08fd3.1614609240.git.lvasiliev@tarantool.org>
> In-Reply-To: <cover.1614609240.git.lvasiliev@tarantool.org>
> References: <cover.1614609240.git.lvasiliev@tarantool.org>
> From: Artem Starshov <artemreyt@tarantool.org>
> Date: Tue, 26 Jan 2021 20:15:22 +0300
> Subject: [PATCH 1/2] core: add setting error injections via env
> Cc: tarantool-patches@dev.tarantool.org
>
> Sometimes, it's useful to set error injections via environment
> variables and this commit adds this opportunity.
>
> e.g: `ERRINJ_WAL_WRITE=true tarantool` will be launched with
> ERRINJ_WAL_WRITE setted to true.
>
> Errinjs with bool parameters can be set to "true", "false",
> "True", "False", "TRUE", "FALSE", etc. (case-insensitive variable).
>
> Errinjs with int or double parameters should be whole valid ("123s" is 
> invalid).
> e.g. for int or double: "123", "-1", "2.34", "+2.34".
>
> Part of #5040
> ---
>  src/lib/core/errinj.c                         | 31 +++++++++++++++++
>  src/lib/core/errinj.h                         |  5 +++
>  src/main.cc                                   |  1 +
>  .../errinj_set_with_enviroment_vars.test.lua  | 18 ++++++++++
>  ...errinj_set_with_enviroment_vars_script.lua | 34 +++++++++++++++++++
>  5 files changed, 89 insertions(+)
>  create mode 100755 test/box-tap/errinj_set_with_enviroment_vars.test.lua
>  create mode 100644 
> test/box-tap/errinj_set_with_enviroment_vars_script.lua
>
> diff --git a/src/lib/core/errinj.c b/src/lib/core/errinj.c
> index d3aa0ca4f..983ead681 100644
> --- a/src/lib/core/errinj.c
> +++ b/src/lib/core/errinj.c
> @@ -28,6 +28,7 @@
>   * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
> +#include <ctype.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -66,3 +67,33 @@ int errinj_foreach(errinj_cb cb, void *cb_ctx) {
>      }
>      return 0;
>  }
> +
> +void errinj_set_with_environment_vars() {
>
> Comment:
> void errinj_set_with_environment_vars(void) see 
> https://github.com/tarantool/tarantool/issues/4718
1. Done.
> + for (enum errinj_id i = 0; i < errinj_id_MAX; i++) {
> +        struct errinj *inj = &errinjs[i];
> +        const char *env_value = getenv(inj->name);
> +        if (!env_value || *env_value == '\0')
>
> Comment:
> env_value == NULL 
> (https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style)
2. Done.
> + continue;
> +
> +        if (inj->type == ERRINJ_INT) {
> +            char *end;
> +            int64_t int_value = strtoll(env_value, &end, 10);
> +            if (*end == '\0')
> +                inj->iparam = int_value;
>
> Comment:
> Seems like the `panic()` should be called in the case of `else()`. 
> Here and in
> other cases of parsing.
3. Done.
>
> +        } else if (inj->type == ERRINJ_BOOL) {
> +            char *lower_env_value = (char *)calloc(strlen(env_value) 
> + 1, sizeof(char));
>
> Comment:
> `(char *)` - unneeded in C.
> Maximal width of a code line is 80 symbols.
4. Done.
> + for (size_t i = 0; env_value[i]; ++i)
> +                lower_env_value[i] = tolower(env_value[i]);
>
> Comment:
> I suggest to use `strcasecmp()`.
5. Nice suggestion! Done.
> + if (strcmp(lower_env_value, "false") == 0)
> +                inj->bparam = false;
> +            else if (strcmp(lower_env_value, "true") == 0)
> +                inj->bparam = true;
> +            free(lower_env_value);
> +        } else if (inj->type == ERRINJ_DOUBLE) {
> +            char *end;
> +            double double_value = strtod(env_value, &end);
> +            if (*end == '\0')
> +                inj->dparam = double_value;
> +        }
> +    }
> +}
> diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
> index 814c57c2e..d76aedf7a 100644
> --- a/src/lib/core/errinj.h
> +++ b/src/lib/core/errinj.h
> @@ -168,6 +168,11 @@ typedef int (*errinj_cb)(struct errinj *e, void 
> *cb_ctx);
>  int
>  errinj_foreach(errinj_cb cb, void *cb_ctx);
>
> +/**
> + * Set injections by scanning ERRINJ_$(NAME) in environment variables
> + */
> +void errinj_set_with_environment_vars();
> +
>  #ifdef NDEBUG
>  #  define ERROR_INJECT(ID, CODE)
>  #  define ERROR_INJECT_WHILE(ID, CODE)
> diff --git a/src/main.cc b/src/main.cc
> index 2fce81bb3..58a660689 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -710,6 +710,7 @@ main(int argc, char **argv)
>      memtx_tx_manager_init();
>      crypto_init();
>      systemd_init();
> +    errinj_set_with_environment_vars();
>      tarantool_lua_init(tarantool_bin, main_argc, main_argv);
>
>      start_time = ev_monotonic_time();
> diff --git a/test/box-tap/errinj_set_with_enviroment_vars.test.lua 
> b/test/box-tap/errinj_set_with_enviroment_vars.test.lua
> new file mode 100755
> index 000000000..bffcc31e7
> --- /dev/null
> +++ b/test/box-tap/errinj_set_with_enviroment_vars.test.lua
> @@ -0,0 +1,18 @@
> +#!/usr/bin/env tarantool
> +local fio = require('fio')
> +
> +-- Execute errinj_set_with_enviroment_vars_script.lua
> +-- via tarantool with presetted environment variables.
> +local TARANTOOL_PATH = arg[-1]
>
> +local bool_env = 'ERRINJ_TESTING=true ERRINJ_WAL_IO=True 
> ERRINJ_WAL_SYNC=TRUE ' ..
> +                 'ERRINJ_WAL_WRITE=false ERRINJ_INDEX_ALLOC=False 
> ERRINJ_WAL_WRITE_DISK=FALSE'
> +local integer_env = 'ERRINJ_WAL_WRITE_PARTIAL=2 
> ERRINJ_WAL_FALLOCATE=+2 ERRINJ_WAL_WRITE_COUNT=-2'
> +local double_env = 'ERRINJ_VY_READ_PAGE_TIMEOUT=2.5 
> ERRINJ_VY_SCHED_TIMEOUT=+2.5 ERRINJ_RELAY_TIMEOUT=-2.5'
>
> Comment:
> Maximal width of a code line is 80 symbols.
6. Done.
> +local set_env_str = ('%s %s %s'):format(bool_env, integer_env, 
> double_env)
> +local path_to_test_file = fio.pathjoin(
> +        os.getenv('PWD'),
> +        'box-tap',
> +        'errinj_set_with_enviroment_vars_script.lua')
> +local shell_command = ('%s %s %s'):format(set_env_str, 
> TARANTOOL_PATH, path_to_test_file)
> +
> +os.exit(os.execute(shell_command))
> diff --git a/test/box-tap/errinj_set_with_enviroment_vars_script.lua 
> b/test/box-tap/errinj_set_with_enviroment_vars_script.lua
> new file mode 100644
> index 000000000..f15085aa0
> --- /dev/null
> +++ b/test/box-tap/errinj_set_with_enviroment_vars_script.lua
> @@ -0,0 +1,34 @@
> +-- Script for box-tap/errinj_set_with_enviroment_vars.test.lua test.
> +
> +local tap = require('tap')
> +local errinj = box.error.injection
> +
> +local test = tap.test('set errinjs via environment variables')
> +
> +test:plan(3)
> +
> +test:test('Set boolean error injections', function(test)
> +    test:plan(6)
> +    test:is(errinj.get('ERRINJ_TESTING'), true, 'true')
> +    test:is(errinj.get('ERRINJ_WAL_IO'), true, 'True')
> +    test:is(errinj.get('ERRINJ_WAL_SYNC'), true, 'TRUE')
> +    test:is(errinj.get('ERRINJ_WAL_WRITE'), false, 'false')
> +    test:is(errinj.get('ERRINJ_INDEX_ALLOC'), false, 'False')
> +    test:is(errinj.get('ERRINJ_WAL_WRITE_DISK'), false, 'FALSE')
> +end)
> +
> +test:test('Set integer error injections', function(test)
> +    test:plan(3)
> +    test:is(errinj.get('ERRINJ_WAL_WRITE_PARTIAL'), 2, '2')
> +    test:is(errinj.get('ERRINJ_WAL_FALLOCATE'), 2, '+2')
> +    test:is(errinj.get('ERRINJ_WAL_WRITE_COUNT'), -2, '-2')
> +end)
> +
> +test:test('Set double error injections', function(test)
> +    test:plan(3)
> +    test:is(errinj.get('ERRINJ_VY_READ_PAGE_TIMEOUT'), 2.5, "2.5")
> +    test:is(errinj.get('ERRINJ_VY_SCHED_TIMEOUT'), 2.5, "+2.5")
> +    test:is(errinj.get('ERRINJ_RELAY_TIMEOUT'), -2.5, "-2.5")
> +end)
> +
> +os.exit(test:check() and 0 or 1)

[Tarantool-patches] [PATCHv2 2/2] lua: fix tarantool -e always enters interactive mode

[Artem Starshov via Tarantool-patches]

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
---
@ChangeLog: Fixed -e option, when tarantool always entered interactive mode when
stdin is a tty. Now, `tarantool -e 'print"Hello"'` doesn't enter interactive mode
as it was before.
 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..e7e0fe006 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 <small/ibuf.h>
 
 #include <ctype.h>
@@ -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_runned = false;
 
 	/*
 	 * 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_runned = 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_atty;
+	struct errinj *inj = errinj(ERRINJ_STDIN_ISATTY, ERRINJ_INT);
+	if (inj != NULL && inj->iparam >= 0) {
+		is_atty = inj->iparam;
+	} else {
+		is_atty = 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_atty || (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_runned) {
 		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
+    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')
+            end
+        end
+        if process_timeout.process_is_alive(pid) then ffi.C.kill(pid, 9) end
+        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
-- 
2.28.0

Re: [Tarantool-patches] [PATCHv2 2/2] lua: fix tarantool -e always enters interactive mode

[Konstantin Osipov via Tarantool-patches]

* Artem Starshov via Tarantool-patches <tarantool-patches@dev.tarantool.org> [21/02/18 22:36]:
> +	bool option_e_runned = false;

run is irregular


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCHv2 2/2] lua: fix tarantool -e always enters interactive mode

[Artem via Tarantool-patches]

Thanks! Renamed to option_e_ran.

>> +	bool option_e_runned = false;
> run is irregular

Re: [Tarantool-patches] [PATCHv2 2/2] lua: fix tarantool -e always enters interactive mode

[Sergey Bronnikov via Tarantool-patches]

Thanks for the patch!

see my comments below.

On 18.02.2021 22:30, Artem Starshov wrote:
> 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
> ---
> @ChangeLog: Fixed -e option, when tarantool always entered interactive mode when
> stdin is a tty. Now, `tarantool -e 'print"Hello"'` doesn't enter interactive mode
> as it was before.
>   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}) \

1. according to name (ISATTY) variable can be in enabled or disabled 
state so

why do you use integer type here?

>   
>   ENUM0(errinj_id, ERRINJ_LIST);
>   extern struct errinj errinjs[];
> diff --git a/src/lua/init.c b/src/lua/init.c
> index 25e8884a6..e7e0fe006 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 <small/ibuf.h>
>   
>   #include <ctype.h>
> @@ -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_runned = false;
>   
>   	/*
>   	 * 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_runned = 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_atty;

2. is_a_tty?

> +	struct errinj *inj = errinj(ERRINJ_STDIN_ISATTY, ERRINJ_INT);
> +	if (inj != NULL && inj->iparam >= 0) {
> +		is_atty = inj->iparam;
> +	} else {
> +		is_atty = 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_atty || (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_runned) {
>   		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
> +    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')
> +            end
> +        end
> +        if process_timeout.process_is_alive(pid) then ffi.C.kill(pid, 9) end
> +        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

Re: [Tarantool-patches] [PATCHv2 2/2] lua: fix tarantool -e always enters interactive mode

[Artem via Tarantool-patches]

Thanks for the review!

>> 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}) \
>
> 1. according to name (ISATTY) variable can be in enabled or disabled 
> state so
>
> why do you use integer type here?

1. I use integer type for ISATTY cause this injection "overrides" return 
value of

standard isatty function for stdin. It can be set to 0 or 1. Default 
value -1 means

to use return value of standard function.

>>     ENUM0(errinj_id, ERRINJ_LIST);
>>   extern struct errinj errinjs[];
>> diff --git a/src/lua/init.c b/src/lua/init.c
>> index 25e8884a6..e7e0fe006 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 <small/ibuf.h>
>>     #include <ctype.h>
>> @@ -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_runned = false;
>>         /*
>>        * 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_runned = 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_atty;
>
> 2. is_a_tty?
>
2. Done.

Re: [Tarantool-patches] [PATCHv2 2/2] lua: fix tarantool -e always enters interactive mode

[Leonid Vasiliev via Tarantool-patches]

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: <cover.1614609240.git.lvasiliev@tarantool.org>
References: <cover.1614609240.git.lvasiliev@tarantool.org>
From: Artem Starshov <artemreyt@tarantool.org>
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 <small/ibuf.h>

  #include <ctype.h>
@@ -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`.


  	/*
  	 * 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.

+	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.

+    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.

+            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.

+        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
-- 
2.30.0

Re: [Tarantool-patches] [PATCHv2 2/2] lua: fix tarantool -e always enters interactive mode

[Artem via 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: <cover.1614609240.git.lvasiliev@tarantool.org>
> References: <cover.1614609240.git.lvasiliev@tarantool.org>
> From: Artem Starshov <artemreyt@tarantool.org>
> 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 <small/ibuf.h>
>
>  #include <ctype.h>
> @@ -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

Re: [Tarantool-patches] [PATCHv2 2/2] lua: fix tarantool -e always enters interactive mode

[Leonid Vasiliev via Tarantool-patches]

Hi!
Please add the hunks of diff or send a new patchset, because "Done" is a
good, but not detailed enough.(For the future)

>> +
>> +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
> 

After discussion at the standup, it was decided to fix the bug in the
test-run and then return to this task.

Re: [Tarantool-patches] [PATCHv2 0/2] lua: fix tarantool -e always enters interactive mode

[Sergey Bronnikov via Tarantool-patches]

Thanks for the patches!

LGTM

On 18.02.2021 22:30, Artem Starshov wrote:
> Issue: https://github.com/tarantool/tarantool/issues/5040
> Branch: https://github.com/tarantool/tarantool/tree/artemreyt/gh-5040-interactive-mode-bug
> CI: https://github.com/tarantool/tarantool/runs/1929677579
>
> Previous patch version: https://lists.tarantool.org/pipermail/tarantool-patches/2021-January/022144.html
>
> Changes in v2:
>    * core:
>      - added `part of #5040` to the commit message;
>      - rename `errinj_scan_env()` -> `errinj_set_with_environment_vars()`;
>      - whitespaces refactored;
>      - add test for setting error injections via environment variables;
>    * lua:
>      - little code refactoring after Sergey Bronnikov review (renames and move
>      includes in the alphabetical order);
>      - fixed bug, when test failed under high concurrency. There were about 20
>      fails with 100 workers. Now, it's ok, tested with 300 workers om CentOS and
>      macOS.
>      (Bug resolved with removing temporary output_file after each testcase iteration);
>      - moved ChangeLog to the last commit.
>
> Artem Starshov (2):
>    core: add setting error injections via env
>    lua: fix tarantool -e always enters interactive mode
>
>   src/lib/core/errinj.c                         |  26 ++++
>   src/lib/core/errinj.h                         |   6 +
>   src/lua/init.c                                |  20 ++-
>   src/main.cc                                   |   1 +
>   ...40-always-enters-interactive-mode.test.lua | 131 ++++++++++++++++++
>   .../errinj_set_with_enviroment_vars.test.lua  |  14 ++
>   ...errinj_set_with_enviroment_vars_script.lua |  13 ++
>   test/box/errinj.result                        |   1 +
>   8 files changed, 208 insertions(+), 4 deletions(-)
>   create mode 100755 test/app-tap/gh-5040-always-enters-interactive-mode.test.lua
>   create mode 100755 test/box-tap/errinj_set_with_enviroment_vars.test.lua
>   create mode 100644 test/box-tap/errinj_set_with_enviroment_vars_script.lua
>