[Tarantool-patches] [PATCH v4 1/2] vinyl: check vinyl_dir existence at bootstrap
Alexander V. Tikhonov
avtikhon at tarantool.org
Fri Aug 14 12:59:57 MSK 2020
During implementation of openSUSE got failed box-tap/cfg.test.lua test.
Found that when memtx_dir wasn't existed, while vinyl_dir existed and
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() were set errno, but the caller assumed it.
Usual C convention is to report success or failure using a return
value and set errno at any error. So a caller usually just checks a
return value and if it means a failure (usually -1), it checks errno
to determine an exact reason.
Usual convention in tarantool is a bit different: we use a special
diagnostics area to report a reason of a failure.
Not all failure paths of xdir_scan() sets errno (including our
'invalid instance UUID' case), so we cannot be sure that errno is
not remains unchanged after a failure of the function.
However the solution with checking errno against ENOENT (No such file
or directory) is not good. For example:
- What if xdir_scan() would be changed in future and, say, some call
will rewrite errno after the opendir() call?
- What if some other call inside xdir_scan() will set ENOENT: say,
open() in xdir_open_cursor() due to some race?
We lean on implementation details of the callee, not its contract. This
way is too fragile and it should either check whether the directory
exists before xdir_scan() call or pass a flag to xdir_scan() whether
the directory should exist. Decided to use second variant - it does not
lead to code duplication.
Added subtest to box-tap/cfg.test.lua test file, to check the currently
fixed issue.
Closes #4594
Needed for #4562
Co-authored-by: Alexander Turenko <alexander.turenko at tarantool.org>
---
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 +++---
test/box-tap/cfg.test.lua | 23 ++++++++++++++++++++++-
7 files changed, 34 insertions(+), 11 deletions(-)
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/cfg.test.lua b/test/box-tap/cfg.test.lua
index 569b5f463..a60aa848e 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -6,7 +6,7 @@ local socket = require('socket')
local fio = require('fio')
local uuid = require('uuid')
local msgpack = require('msgpack')
-test:plan(108)
+test:plan(109)
--------------------------------------------------------------------------------
-- Invalid values
@@ -605,5 +605,26 @@ test:ok(not box.info.listen:match(':0'), 'real port in info.listen')
box.cfg{listen = box.NULL}
test:is(nil, box.info.listen, 'cfg.listen reset drops info.listen')
+--
+-- 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