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

[Kirill Yukhin <kyukhin@tarantool.org>]

Hello,

On 27 авг 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

LGTM. I've checked your patch into 1.10, 2.4, 2.5 and master.

--
Regards, Kirill Yukhin