From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 3402843040C for ; Thu, 27 Aug 2020 17:16:05 +0300 (MSK) References: <77367c061961d33a6fd11325f4c6abb443ffb15a.1598537487.git.avtikhon@tarantool.org> From: Aleksandr Lyapunov Message-ID: <174b6002-0b28-4cf3-f064-b3c19bce3b9b@tarantool.org> Date: Thu, 27 Aug 2020 17:16:03 +0300 MIME-Version: 1.0 In-Reply-To: <77367c061961d33a6fd11325f4c6abb443ffb15a.1598537487.git.avtikhon@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH v2] vinyl: fix check vinyl_dir existence at bootstrap List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Alexander V. Tikhonov" , Kirill Yukhin Cc: tarantool-patches@dev.tarantool.org, Alexander Turenko Hi and thanks again, great, LGTM! On 27.08.2020 17:12, Alexander V. Tikhonov wrote: > 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 > --- > > 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)