From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E2F8E430408 for ; Fri, 14 Aug 2020 13:00:02 +0300 (MSK) From: "Alexander V. Tikhonov" Date: Fri, 14 Aug 2020 12:59:57 +0300 Message-Id: <4eb7b26b9f80393b19536f49ab3985d72ba13d89.1597398597.git.avtikhon@tarantool.org> In-Reply-To: References: In-Reply-To: References: Subject: [Tarantool-patches] [PATCH v4 1/2] vinyl: check vinyl_dir existence at bootstrap List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirill Yukhin , Alexander Turenko Cc: tarantool-patches@dev.tarantool.org 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 --- 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