[Tarantool-patches] [PATCH v2] vinyl: fix check vinyl_dir existence at bootstrap
Aleksandr Lyapunov
alyapunov at tarantool.org
Thu Aug 27 17:16:03 MSK 2020
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 <alexander.turenko at tarantool.org>
> ---
>
> 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)
More information about the Tarantool-patches
mailing list