Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2] vinyl: fix check vinyl_dir existence at bootstrap
@ 2020-08-27 14:12 Alexander V. Tikhonov
  2020-08-27 14:16 ` Aleksandr Lyapunov
  2020-08-31  9:51 ` Kirill Yukhin
  0 siblings, 2 replies; 3+ messages in thread
From: Alexander V. Tikhonov @ 2020-08-27 14:12 UTC (permalink / raw)
  To: Kirill Yukhin, Aleksandr Lyapunov; +Cc: tarantool-patches, Alexander Turenko

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 that we must not check errno after call 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>
---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4562-vinyl-fix
Issue: https://github.com/tarantool/tarantool/issues/4594
Issue: https://github.com/tarantool/tarantool/issues/4562
Review: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019130.html

 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                                | 14 ++++-
 src/box/xlog.h                                |  7 +--
 .../gh-4562-errno-at-xdir_scan.test.lua       | 54 +++++++++++++++++++
 7 files changed, 76 insertions(+), 11 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 de4c5205c..d23b1c18a 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 045006b60..ea707aa5e 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -556,7 +556,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..974f460be 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -487,6 +487,15 @@ xdir_open_cursor(struct xdir *dir, int64_t signature,
  * replication set (see also _cluster system space and vclock.h
  * comments).
  *
+ * @param dir - directory to scan
+ * @param is_dir_required - flag set if the directory should exist
+ *
+ * @return:
+ *   0 - on success or flag 'is_dir_required' was set to False in
+ *   xdir_scan() arguments and opendir() failed with errno ENOENT;
+ *   -1 - if opendir() failed, with other than ENOENT errno either
+ *   when 'is_dir_required' was set to True in xdir_scan() arguments.
+ *
  * This function tries to avoid re-reading a file if
  * it is already in the set of files "known" to the log
  * dir object. This is done to speed up local hot standby and
@@ -508,16 +517,17 @@ xdir_open_cursor(struct xdir *dir, int64_t signature,
  * silence conditions such as out of memory or lack of OS
  * resources.
  *
- * @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..5b1f42ce1 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -185,9 +185,10 @@ xdir_destroy(struct xdir *dir);
  * index with all log files (or snapshots) in the directory.
  * Must be used if it is necessary to find the last log/
  * snapshot or scan through all logs.
+ * Function arguments described in xlog.c source file.
  */
 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 +822,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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-31  9:51 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-27 14:12 [Tarantool-patches] [PATCH v2] vinyl: fix check vinyl_dir existence at bootstrap Alexander V. Tikhonov
2020-08-27 14:16 ` Aleksandr Lyapunov
2020-08-31  9:51 ` Kirill Yukhin

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