[Tarantool-patches] [PATCH v6] vinyl: fix check vinyl_dir existence at bootstrap

[Tarantool-patches] [PATCH v6] vinyl: fix check vinyl_dir existence at bootstrap

[Alexander V. Tikhonov]

During implementation of openSUSE build with testing got failed test
box-tap/cfg.test.lua. Found that when memtx_dir didn't exist and
vinyl_dir existed and also errno was set to ENOENT, box configuration
succeeded, but it shouldn't. Reason of this wrong behaviour was that
not all of the failure paths in xdir_scan() set errno, but the caller
assumed it.

Debugging the issue found that after xdir_scan() there was incorrect
check for errno when it returned negative values. xdir_scan() is not
system call and negative return value from it doesn't mean that errno
whould be set too. Found that in situations when errno was left from
previous commands before xdir_scan() and xdir_scan() returned negative
value by itself than the check was wrong. The previous logic of the
check was to catch the error ENOENT from inside the xdir_scan()
function to handle the situation when vinyl_dir was not exist. In this
way errno should be reseted before xdir_scan() call to give the ability
for xdir_scan() to use return value without errno set and correctly
handle errno from inside the xdir_scan().

After discussions found that there was alternative better solution to
fix it. As mentioned above xdir_scan() function is not system call and
can be changed inside it in any possible way. So check outside of this
function on errno could be broken, because of the xdir_scan() changes.
To avoid of it we must avoid of errno checks outside of the function.
Better solution was to use the flag in xdir_scan(), to check if the
directory should exist. So errno check was removed and instead of it
the check for vinyl_dir existence using flag added.

Closes #4594
Needed for #4562

Co-authored-by: Alexander Turenko <alexander.turenko@tarantool.org>
---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4562-suse-pack-full-ci
Issue: https://github.com/tarantool/tarantool/issues/4594
Issue: https://github.com/tarantool/tarantool/issues/4562

 src/box/memtx_engine.c                        |  2 +-
 src/box/recovery.cc                           |  4 +-
 src/box/vy_log.c                              |  4 +-
 src/box/wal.c                                 |  2 +-
 src/box/xlog.c                                |  4 +-
 src/box/xlog.h                                |  6 +--
 .../gh-4562-errno-at-xdir_scan.test.lua       | 54 +++++++++++++++++++
 7 files changed, 66 insertions(+), 10 deletions(-)
 create mode 100755 test/box-tap/gh-4562-errno-at-xdir_scan.test.lua

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index dfd6fce6e..9f079a6b5 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -992,7 +992,7 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,
 		    &xlog_opts_default);
 	memtx->snap_dir.force_recovery = force_recovery;
 
-	if (xdir_scan(&memtx->snap_dir) != 0)
+	if (xdir_scan(&memtx->snap_dir, true) != 0)
 		goto fail;
 
 	/*
diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index d1a503cfc..cd33e7635 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -121,7 +121,7 @@ void
 recovery_scan(struct recovery *r, struct vclock *end_vclock,
 	      struct vclock *gc_vclock)
 {
-	xdir_scan_xc(&r->wal_dir);
+	xdir_scan_xc(&r->wal_dir, true);
 
 	if (xdir_last_vclock(&r->wal_dir, end_vclock) < 0 ||
 	    vclock_compare(end_vclock, &r->vclock) < 0) {
@@ -307,7 +307,7 @@ recover_remaining_wals(struct recovery *r, struct xstream *stream,
 	struct vclock *clock;
 
 	if (scan_dir)
-		xdir_scan_xc(&r->wal_dir);
+		xdir_scan_xc(&r->wal_dir, true);
 
 	if (xlog_cursor_is_open(&r->cursor)) {
 		/* If there's a WAL open, recover from it first. */
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 311985c72..da3c50e87 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -1014,7 +1014,7 @@ vy_log_rebootstrap(void)
 int
 vy_log_bootstrap(void)
 {
-	if (xdir_scan(&vy_log.dir) < 0 && errno != ENOENT)
+	if (xdir_scan(&vy_log.dir, false) < 0)
 		return -1;
 	if (xdir_last_vclock(&vy_log.dir, &vy_log.last_checkpoint) >= 0)
 		return vy_log_rebootstrap();
@@ -1036,7 +1036,7 @@ vy_log_begin_recovery(const struct vclock *vclock)
 	 * because vinyl might not be even in use. Complain only
 	 * on an attempt to write a vylog.
 	 */
-	if (xdir_scan(&vy_log.dir) < 0 && errno != ENOENT)
+	if (xdir_scan(&vy_log.dir, false) < 0)
 		return NULL;
 
 	if (xdir_last_vclock(&vy_log.dir, &vy_log.last_checkpoint) < 0) {
diff --git a/src/box/wal.c b/src/box/wal.c
index d8c92aa36..2b894d680 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -559,7 +559,7 @@ wal_enable(void)
 	 * existing WAL files. Required for garbage collection,
 	 * see wal_collect_garbage().
 	 */
-	if (xdir_scan(&writer->wal_dir))
+	if (xdir_scan(&writer->wal_dir, true))
 		return -1;
 
 	/* Open the most recent WAL file. */
diff --git a/src/box/xlog.c b/src/box/xlog.c
index 6ccd3d68d..74f761994 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -511,13 +511,15 @@ xdir_open_cursor(struct xdir *dir, int64_t signature,
  * @return nothing.
  */
 int
-xdir_scan(struct xdir *dir)
+xdir_scan(struct xdir *dir, bool is_dir_required)
 {
 	DIR *dh = opendir(dir->dirname);        /* log dir */
 	int64_t *signatures = NULL;             /* log file names */
 	size_t s_count = 0, s_capacity = 0;
 
 	if (dh == NULL) {
+		if (!is_dir_required && errno == ENOENT)
+			return 0;
 		diag_set(SystemError, "error reading directory '%s'",
 			  dir->dirname);
 		return -1;
diff --git a/src/box/xlog.h b/src/box/xlog.h
index 9ffce598b..3400eb75f 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -187,7 +187,7 @@ xdir_destroy(struct xdir *dir);
  * snapshot or scan through all logs.
  */
 int
-xdir_scan(struct xdir *dir);
+xdir_scan(struct xdir *dir, bool is_dir_required);
 
 /**
  * Check that a directory exists and is writable.
@@ -821,9 +821,9 @@ xdir_open_cursor(struct xdir *dir, int64_t signature,
 #include "exception.h"
 
 static inline void
-xdir_scan_xc(struct xdir *dir)
+xdir_scan_xc(struct xdir *dir, bool is_dir_required)
 {
-	if (xdir_scan(dir) == -1)
+	if (xdir_scan(dir, is_dir_required) == -1)
 		diag_raise();
 }
 
diff --git a/test/box-tap/gh-4562-errno-at-xdir_scan.test.lua b/test/box-tap/gh-4562-errno-at-xdir_scan.test.lua
new file mode 100755
index 000000000..e6dd68d54
--- /dev/null
+++ b/test/box-tap/gh-4562-errno-at-xdir_scan.test.lua
@@ -0,0 +1,54 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local test = tap.test('cfg')
+local fio = require('fio')
+test:plan(1)
+
+local tarantool_bin = arg[-1]
+local PANIC = 256
+
+local function run_script(code)
+    local dir = fio.tempdir()
+    local script_path = fio.pathjoin(dir, 'script.lua')
+    local script = fio.open(script_path, {'O_CREAT', 'O_WRONLY', 'O_APPEND'},
+        tonumber('0777', 8))
+    script:write(code)
+    script:write("\nos.exit(0)")
+    script:close()
+    local cmd = [[/bin/sh -c 'cd "%s" && "%s" ./script.lua 2> /dev/null']]
+    local res = os.execute(string.format(cmd, dir, tarantool_bin))
+    fio.rmtree(dir)
+    return res
+end
+
+--
+-- gh-4594: when memtx_dir is not exists, but vinyl_dir exists and
+-- errno is set to ENOENT, box configuration succeeds, however it
+-- should not
+--
+
+-- create vinyl_dir as temporary path
+local vinyl_dir = fio.tempdir()
+
+-- fill vinyl_dir
+run_script(string.format([[
+box.cfg{vinyl_dir = '%s'}
+s = box.schema.space.create('test', {engine = 'vinyl'})
+s:create_index('pk')
+os.exit(0)
+]], vinyl_dir))
+
+-- verify the case described above
+local code = string.format([[
+local errno = require('errno')
+errno(errno.ENOENT)
+box.cfg{vinyl_dir = '%s'}
+os.exit(0)
+]], vinyl_dir)
+test:is(run_script(code), PANIC, "bootstrap with ENOENT from non-empty vinyl_dir")
+
+-- remove vinyl_dir
+fio.rmtree(vinyl_dir)
+
+os.exit(test:check() and 0 or 1)
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v6] vinyl: fix check vinyl_dir existence at bootstrap

[Alexander Turenko]

This description becomes much better!

On Wed, Aug 19, 2020 at 09:17:39AM +0300, Alexander V. Tikhonov wrote:
> During implementation of openSUSE build with testing got failed test
> box-tap/cfg.test.lua. Found that when memtx_dir didn't exist and
> vinyl_dir existed and also errno was set to ENOENT, box configuration
> succeeded, but it shouldn't. Reason of this wrong behaviour was that
> not all of the failure paths in xdir_scan() set errno, but the caller
> assumed it.
> 
> Debugging the issue found that after xdir_scan() there was incorrect
> check for errno when it returned negative values. xdir_scan() is not
> system call and negative return value from it doesn't mean that errno
> whould be set too. Found that in situations when errno was left from
> previous commands before xdir_scan() and xdir_scan() returned negative
> value by itself than the check was wrong. The previous logic of the
> check was to catch the error ENOENT from inside the xdir_scan()
> function to handle the situation when vinyl_dir was not exist. In this
> way errno should be reseted before xdir_scan() call to give the ability
> for xdir_scan() to use return value without errno set and correctly
> handle errno from inside the xdir_scan().

'from inside' can be interpreted as 'to catch from inside', not as
'ENOENT from inside'. The same for 'handle errno for inside': 'to handle
from inside' or 'errno from inside'?

'to use return value without errno set' is unclear for me. Are not it is
intended to give the same idea as the previous 'errno should be reset
before xdir_scan()'? If so, it look redundant for me.

Typo: whould.

Typo: reseted -> reset (irregular verb).

> 
> After discussions found that there was alternative better solution to
> fix it. As mentioned above xdir_scan() function is not system call and
> can be changed inside it in any possible way. So check outside of this
> function on errno could be broken, because of the xdir_scan() changes.
> To avoid of it we must avoid of errno checks outside of the function.
> Better solution was to use the flag in xdir_scan(), to check if the
> directory should exist. So errno check was removed and instead of it
> the check for vinyl_dir existence using flag added.
> 
> Closes #4594
> Needed for #4562
> 
> Co-authored-by: Alexander Turenko <alexander.turenko@tarantool.org>
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4562-suse-pack-full-ci
> Issue: https://github.com/tarantool/tarantool/issues/4594
> Issue: https://github.com/tarantool/tarantool/issues/4562

Re: [Tarantool-patches] [PATCH v6] vinyl: fix check vinyl_dir existence at bootstrap

[Alexander V. Tikhonov]

Hi Alexander, thanks a lot for your help. As we discussed the commit
message I've made all the changes in it and commited to branch.

On Thu, Aug 20, 2020 at 01:35:52AM +0300, Alexander Turenko wrote:
> This description becomes much better!
> 
> On Wed, Aug 19, 2020 at 09:17:39AM +0300, Alexander V. Tikhonov wrote:
> > During implementation of openSUSE build with testing got failed test
> > box-tap/cfg.test.lua. Found that when memtx_dir didn't exist and
> > vinyl_dir existed and also errno was set to ENOENT, box configuration
> > succeeded, but it shouldn't. Reason of this wrong behaviour was that
> > not all of the failure paths in xdir_scan() set errno, but the caller
> > assumed it.
> > 
> > Debugging the issue found that after xdir_scan() there was incorrect
> > check for errno when it returned negative values. xdir_scan() is not
> > system call and negative return value from it doesn't mean that errno
> > whould be set too. Found that in situations when errno was left from
> > previous commands before xdir_scan() and xdir_scan() returned negative
> > value by itself than the check was wrong. The previous logic of the
> > check was to catch the error ENOENT from inside the xdir_scan()
> > function to handle the situation when vinyl_dir was not exist. In this
> > way errno should be reseted before xdir_scan() call to give the ability
> > for xdir_scan() to use return value without errno set and correctly
> > handle errno from inside the xdir_scan().
> 
> 'from inside' can be interpreted as 'to catch from inside', not as
> 'ENOENT from inside'. The same for 'handle errno for inside': 'to handle
> from inside' or 'errno from inside'?
> 
> 'to use return value without errno set' is unclear for me. Are not it is
> intended to give the same idea as the previous 'errno should be reset
> before xdir_scan()'? If so, it look redundant for me.
> 
> Typo: whould.
> 
> Typo: reseted -> reset (irregular verb).
> 
> > 
> > After discussions found that there was alternative better solution to
> > fix it. As mentioned above xdir_scan() function is not system call and
> > can be changed inside it in any possible way. So check outside of this
> > function on errno could be broken, because of the xdir_scan() changes.
> > To avoid of it we must avoid of errno checks outside of the function.
> > Better solution was to use the flag in xdir_scan(), to check if the
> > directory should exist. So errno check was removed and instead of it
> > the check for vinyl_dir existence using flag added.
> > 
> > Closes #4594
> > Needed for #4562
> > 
> > Co-authored-by: Alexander Turenko <alexander.turenko@tarantool.org>
> > ---
> > 
> > Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4562-suse-pack-full-ci
> > Issue: https://github.com/tarantool/tarantool/issues/4594
> > Issue: https://github.com/tarantool/tarantool/issues/4562

Re: [Tarantool-patches] [PATCH v6] vinyl: fix check vinyl_dir existence at bootstrap

[Alexander Turenko]

Please, resend the patchset to Nikita Pettik and Aleksandr Lyapunov (because it
is related to vinyl).

Or extract this particular path to its own branch and send it as a singleton
patch.

WBR, Alexander Turenko.

On Thu, Aug 20, 2020 at 05:25:38PM +0300, Alexander V. Tikhonov wrote:
> Hi Alexander, thanks a lot for your help. As we discussed the commit
> message I've made all the changes in it and commited to branch.

I think it is good enough. LGTM.

Cited below for the history.

> commit e18b7e8f56787a9099429ff9e6fc6c1185d74723
> Author: Alexander V. Tikhonov <avtikhon@tarantool.org>
> Date:   Fri Aug 14 11:18:25 2020 +0300
> 
>     vinyl: fix check vinyl_dir existence at bootstrap
>     
>     During implementation of openSUSE build with testing got failed test
>     box-tap/cfg.test.lua. Found that when memtx_dir didn't exist and
>     vinyl_dir existed and also errno was set to ENOENT, box configuration
>     succeeded, but it shouldn't. Reason of this wrong behavior was that
>     not all of the failure paths in xdir_scan() set errno, but the caller
>     assumed it.
>     
>     Debugging the issue found that after xdir_scan() there was incorrect
>     check for errno when it returned negative values. xdir_scan() is not
>     system call and negative return value from it doesn't mean that errno
>     would be set too. Found that in situations when errno was left from
>     previous commands before xdir_scan() and xdir_scan() returned negative
>     value by itself it produced the wrong check.
>     
>     The previous failed logic of the check was to catch the error ENOENT
>     which set in the xdir_scan() function to handle the situation when
>     vinyl_dir was not exist. It failed, because checking ENOENT outside
>     the xdir_scan() function, we had to be sure that ENOENT had come from
>     xdir_scan() function call indeed and not from any other functions
>     before. To be sure in it possible fix could be reset errno before
>     xdir_scan() call, because errno could be passed from any other function
>     before call to xdir_scan().
>     
>     As mentioned above xdir_scan() function is not system call and can be
>     changed in any possible way and it can return any result value without
>     need to setup errno. So check outside of this function on errno could
>     be broken.
>     
>     To avoid of it we must avoid of errno checks outside of the function.
>     Better solution is to use the flag in xdir_scan(), to check if the
>     directory should exist. So errno check was removed and instead of it
>     the check for vinyl_dir existence using flag added.
>     
>     Closes #4594
>     Needed for #4562
>     
>     Co-authored-by: Alexander Turenko <alexander.turenko@tarantool.org>