Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 0/2] lua: fix -e always enters interactive mode
@ 2021-01-27 21:15 Artem Starshov via Tarantool-patches
  2021-01-27 21:15 ` [Tarantool-patches] [PATCH 1/2] core: add setting error injections via env Artem Starshov via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Artem Starshov via Tarantool-patches @ 2021-01-27 21:15 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: tarantool-patches

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

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 | 130 ++++++++++++++++++
 test/box/errinj.result                        |   1 +
 6 files changed, 180 insertions(+), 4 deletions(-)
 create mode 100755 test/app-tap/gh-5040-always-enters-interactive-mode.test.lua

-- 
2.28.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH 1/2] core: add setting error injections via env
  2021-01-27 21:15 [Tarantool-patches] [PATCH 0/2] lua: fix -e always enters interactive mode Artem Starshov via Tarantool-patches
@ 2021-01-27 21:15 ` Artem Starshov via Tarantool-patches
  2021-01-28 15:37   ` Artem via Tarantool-patches
  2021-01-29 13:36   ` Sergey Bronnikov via Tarantool-patches
  2021-01-27 21:15 ` [Tarantool-patches] [PATCH 2/2] lua: fix tarantool -e always enters interactive mode Artem Starshov via Tarantool-patches
  2021-01-29 13:00 ` [Tarantool-patches] [PATCH 0/2] lua: fix " Sergey Bronnikov via Tarantool-patches
  2 siblings, 2 replies; 7+ messages in thread
From: Artem Starshov via Tarantool-patches @ 2021-01-27 21:15 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: 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".
---
@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.c | 26 ++++++++++++++++++++++++++
 src/lib/core/errinj.h |  5 +++++
 src/main.cc           |  1 +
 3 files changed, 32 insertions(+)

diff --git a/src/lib/core/errinj.c b/src/lib/core/errinj.c
index d3aa0ca4f..576811073 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_scan_env() {
+    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"))
+                inj->bparam = false;
+            else if (strcmp(env_value, "true") == 0 || strcmp(env_value, "TRUE"))
+                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..dc0657614 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_scan_env();
+
 #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..2d85c0b24 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_scan_env();
 	tarantool_lua_init(tarantool_bin, main_argc, main_argv);
 
 	start_time = ev_monotonic_time();
-- 
2.28.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH 2/2] lua: fix tarantool -e always enters interactive mode
  2021-01-27 21:15 [Tarantool-patches] [PATCH 0/2] lua: fix -e always enters interactive mode Artem Starshov via Tarantool-patches
  2021-01-27 21:15 ` [Tarantool-patches] [PATCH 1/2] core: add setting error injections via env Artem Starshov via Tarantool-patches
@ 2021-01-27 21:15 ` Artem Starshov via Tarantool-patches
  2021-01-29 13:44   ` Sergey Bronnikov via Tarantool-patches
  2021-01-29 13:00 ` [Tarantool-patches] [PATCH 0/2] lua: fix " Sergey Bronnikov via Tarantool-patches
  2 siblings, 1 reply; 7+ messages in thread
From: Artem Starshov via Tarantool-patches @ 2021-01-27 21:15 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: 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
---
 src/lib/core/errinj.h                         |   1 +
 src/lua/init.c                                |  20 ++-
 ...40-always-enters-interactive-mode.test.lua | 130 ++++++++++++++++++
 test/box/errinj.result                        |   1 +
 4 files changed, 148 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 dc0657614..89d2d3314 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..fc56e7fe0 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -61,6 +61,7 @@
 #include "lua/utf8.h"
 #include "lua/swim.h"
 #include "lua/decimal.h"
+#include "errinj.h"
 #include "digest.h"
 #include <small/ibuf.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..e1a92465e
--- /dev/null
+++ b/test/app-tap/gh-5040-always-enters-interactive-mode.test.lua
@@ -0,0 +1,130 @@
+#!/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 command 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 commands = {
+    {
+        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(#commands)
+for _, cmd in pairs(commands) 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()
+    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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/2] core: add setting error injections via env
  2021-01-27 21:15 ` [Tarantool-patches] [PATCH 1/2] core: add setting error injections via env Artem Starshov via Tarantool-patches
@ 2021-01-28 15:37   ` Artem via Tarantool-patches
  2021-01-29 13:36   ` Sergey Bronnikov via Tarantool-patches
  1 sibling, 0 replies; 7+ messages in thread
From: Artem via Tarantool-patches @ 2021-01-28 15:37 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: tarantool-patches

[-- Attachment #1: Type: text/plain, Size: 4293 bytes --]

Fixed mistake in function `errinj_scan_env` when check strcmp(env_value, 
"FALSE") and strcmp(env_value, "TRUE").

(Didn't compare it with 0)


--- 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_scan_env() {
+    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;
+        }
+    }
+}

28.01.2021 00:15, Artem Starshov пишет:
> 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".
> ---
> @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.c | 26 ++++++++++++++++++++++++++
>   src/lib/core/errinj.h |  5 +++++
>   src/main.cc           |  1 +
>   3 files changed, 32 insertions(+)
>
> diff --git a/src/lib/core/errinj.c b/src/lib/core/errinj.c
> index d3aa0ca4f..576811073 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_scan_env() {
> +    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"))
> +                inj->bparam = false;
> +            else if (strcmp(env_value, "true") == 0 || strcmp(env_value, "TRUE"))
> +                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..dc0657614 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_scan_env();
> +
>   #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..2d85c0b24 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_scan_env();
>   	tarantool_lua_init(tarantool_bin, main_argc, main_argv);
>   
>   	start_time = ev_monotonic_time();

[-- Attachment #2: Type: text/html, Size: 4718 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/2] lua: fix -e always enters interactive mode
  2021-01-27 21:15 [Tarantool-patches] [PATCH 0/2] lua: fix -e always enters interactive mode Artem Starshov via Tarantool-patches
  2021-01-27 21:15 ` [Tarantool-patches] [PATCH 1/2] core: add setting error injections via env Artem Starshov via Tarantool-patches
  2021-01-27 21:15 ` [Tarantool-patches] [PATCH 2/2] lua: fix tarantool -e always enters interactive mode Artem Starshov via Tarantool-patches
@ 2021-01-29 13:00 ` Sergey Bronnikov via Tarantool-patches
  2 siblings, 0 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2021-01-29 13:00 UTC (permalink / raw)
  To: Artem Starshov, Alexander Turenko; +Cc: tarantool-patches

Thanks for patches!


On 28.01.2021 00:15, Artem Starshov wrote:
> Issue: https://github.com/tarantool/tarantool/issues/5040
> Branch: https://github.com/tarantool/tarantool/tree/artemreyt/gh-5040-interactive-mode-bug
Usually we also provide a link to CI.
> 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 | 130 ++++++++++++++++++
>   test/box/errinj.result                        |   1 +
>   6 files changed, 180 insertions(+), 4 deletions(-)
>   create mode 100755 test/app-tap/gh-5040-always-enters-interactive-mode.test.lua
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/2] core: add setting error injections via env
  2021-01-27 21:15 ` [Tarantool-patches] [PATCH 1/2] core: add setting error injections via env Artem Starshov via Tarantool-patches
  2021-01-28 15:37   ` Artem via Tarantool-patches
@ 2021-01-29 13:36   ` Sergey Bronnikov via Tarantool-patches
  1 sibling, 0 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2021-01-29 13:36 UTC (permalink / raw)
  To: Artem Starshov, Alexander Turenko; +Cc: tarantool-patches

Thanks for the patch! See my 4 comments below.

On 28.01.2021 00:15, 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".
> ---
> @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.

1. This commit is required by next commit with fix.

I propose to link it to issue too by adding "Part of #5040".

>
>   src/lib/core/errinj.c | 26 ++++++++++++++++++++++++++
>   src/lib/core/errinj.h |  5 +++++
>   src/main.cc           |  1 +
>   3 files changed, 32 insertions(+)
>
> diff --git a/src/lib/core/errinj.c b/src/lib/core/errinj.c
> index d3aa0ca4f..576811073 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_scan_env() {

2. With function name it is not clear what function does.

Please consider rename. For example 'set_errinj_with_environment_vars' 
or something else.

> +    for (enum errinj_id i = 0 ; i < errinj_id_MAX ; i++) {
3. What for added these whitespaces before commas?
> +        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"))
> +                inj->bparam = false;
> +            else if (strcmp(env_value, "true") == 0 || strcmp(env_value, "TRUE"))
> +                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;
> +        }
> +    }
> +}

4. Let's add a simple test that will pass three error injection 
variables with different types (bool, int and double) via environment

and check that error injections that same in Lua.

> diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
> index 814c57c2e..dc0657614 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_scan_env();
> +
>   #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..2d85c0b24 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_scan_env();
>   	tarantool_lua_init(tarantool_bin, main_argc, main_argv);
>   
>   	start_time = ev_monotonic_time();

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] lua: fix tarantool -e always enters interactive mode
  2021-01-27 21:15 ` [Tarantool-patches] [PATCH 2/2] lua: fix tarantool -e always enters interactive mode Artem Starshov via Tarantool-patches
@ 2021-01-29 13:44   ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2021-01-29 13:44 UTC (permalink / raw)
  To: Artem Starshov, Alexander Turenko; +Cc: tarantool-patches

Thanks for the patch! See my 3 comments below.

On 28.01.2021 00:15, 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
> ---
>   src/lib/core/errinj.h                         |   1 +
>   src/lua/init.c                                |  20 ++-
>   ...40-always-enters-interactive-mode.test.lua | 130 ++++++++++++++++++
>   test/box/errinj.result                        |   1 +
>   4 files changed, 148 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 dc0657614..89d2d3314 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..fc56e7fe0 100644
> --- a/src/lua/init.c
> +++ b/src/lua/init.c
> @@ -61,6 +61,7 @@
>   #include "lua/utf8.h"
>   #include "lua/swim.h"
>   #include "lua/decimal.h"
> +#include "errinj.h"
1. Looks like it is sorted alphabetically order, so "errinj.h" should be 
right after "digest.h" header.
>   #include "digest.h"
>   #include <small/ibuf.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..e1a92465e
> --- /dev/null
> +++ b/test/app-tap/gh-5040-always-enters-interactive-mode.test.lua
> @@ -0,0 +1,130 @@
> +#!/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 command 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 commands = {
2. I would rename table to '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(#commands)
> +for _, cmd in pairs(commands) 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()
> +    end)
> +end
> +

3. ~18 tests out of 100 failed when I run with command below:

  ../../test/test-run.py 
--builddir=/home/s.bronnikov/work/tarantool/build 
--vardir=/home/s.bronnikov/work/tarantool/build/test/var -j 100 $(yes 
app-tap/gh-5040-always-enters-interactive-mode.test.lua | head -n 100)

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-01-29 13:44 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-27 21:15 [Tarantool-patches] [PATCH 0/2] lua: fix -e always enters interactive mode Artem Starshov via Tarantool-patches
2021-01-27 21:15 ` [Tarantool-patches] [PATCH 1/2] core: add setting error injections via env Artem Starshov via Tarantool-patches
2021-01-28 15:37   ` Artem via Tarantool-patches
2021-01-29 13:36   ` Sergey Bronnikov via Tarantool-patches
2021-01-27 21:15 ` [Tarantool-patches] [PATCH 2/2] lua: fix tarantool -e always enters interactive mode Artem Starshov via Tarantool-patches
2021-01-29 13:44   ` Sergey Bronnikov via Tarantool-patches
2021-01-29 13:00 ` [Tarantool-patches] [PATCH 0/2] lua: fix " Sergey Bronnikov via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox