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

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

Hi Alexander, thanks a lot for your help. As we discussed the commit
message I've made all the changes in it and commited to branch.

On Thu, Aug 20, 2020 at 01:35:52AM +0300, Alexander Turenko wrote:
> This description becomes much better!
> 
> On Wed, Aug 19, 2020 at 09:17:39AM +0300, 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 behaviour 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
> > whould 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 than the check was wrong. The previous logic of the
> > check was to catch the error ENOENT from inside the xdir_scan()
> > function to handle the situation when vinyl_dir was not exist. In this
> > way errno should be reseted before xdir_scan() call to give the ability
> > for xdir_scan() to use return value without errno set and correctly
> > handle errno from inside the xdir_scan().
> 
> 'from inside' can be interpreted as 'to catch from inside', not as
> 'ENOENT from inside'. The same for 'handle errno for inside': 'to handle
> from inside' or 'errno from inside'?
> 
> 'to use return value without errno set' is unclear for me. Are not it is
> intended to give the same idea as the previous 'errno should be reset
> before xdir_scan()'? If so, it look redundant for me.
> 
> Typo: whould.
> 
> Typo: reseted -> reset (irregular verb).
> 
> > 
> > After discussions found that there was alternative better solution to
> > fix it. As mentioned above xdir_scan() function is not system call and
> > can be changed inside it in any possible way. So check outside of this
> > function on errno could be broken, because of the xdir_scan() changes.
> > To avoid of it we must avoid of errno checks outside of the function.
> > Better solution was 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-suse-pack-full-ci
> > Issue: https://github.com/tarantool/tarantool/issues/4594
> > Issue: https://github.com/tarantool/tarantool/issues/4562