[Tarantool-patches] [PATCH v5 1/2] vinyl: fix check vinyl_dir existence at bootstrap
Alexander V. Tikhonov
avtikhon at tarantool.org
Mon Aug 17 08:29:14 MSK 2020
During implementation of openSUSE got failed box-tap/cfg.test.lua test.
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 src/box/xlog.c found that all checks were correct, but at:
src/box/vy_log.c:vy_log_bootstrap()
src/box/vy_log.c:vy_log_begin_recovery()
the checks on of the errno on ENOENT blocked the negative return from:
src/box/xlog.c:xdir_scan()
Found that errno was already set to ENOENT before the xdir_scan() call.
To fix the issue the errno could be clean before the call to xdir_scan,
because we are interesting in it only from xdir_scan function.
After discussions found that there were alternative better solution to
fix it. The fix with resetting errno was not good because xdir_scan()
was not system call in real and some internal routines could set it
to ENOENT itself, so it couldn't be controled from outside of function.
To be sure in behaviour of the changing errno decided to pass a flag to
xdir_scan() if the directory should exist.
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/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 | 47 +++++++++++++++++++
7 files changed, 59 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..cbf7b1f35
--- /dev/null
+++ b/test/box-tap/gh-4562-errno-at-xdir_scan.test.lua
@@ -0,0 +1,47 @@
+#!/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
+
+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
+--
+vinyl_dir = fio.tempdir()
+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))
+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")
+fio.rmtree(vinyl_dir)
+
+test:check()
+os.exit(0)
--
2.17.1
More information about the Tarantool-patches
mailing list