Re: [Tarantool-patches] [PATCH v2] vinyl: fix check vinyl_dir existence at bootstrap

[Aleksandr Lyapunov <alyapunov@tarantool.org>]

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@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)