[Tarantool-patches] [PATCH v6] vinyl: fix check vinyl_dir existence at bootstrap
Alexander V. Tikhonov
avtikhon at tarantool.org
Wed Aug 19 09:17:39 MSK 2020
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 at 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
More information about the Tarantool-patches
mailing list