[Tarantool-patches] [PATCH v2 0/3] gitlab-ci: add openSuSE packages build jobs

[Tarantool-patches] [PATCH v2 0/3] gitlab-ci: add openSuSE packages build jobs

[Alexander V. Tikhonov]

gitlab-ci: add openSuSE packages build jobs
    
Implemented openSuSE packages build with testing for images:
opensuse-leap:15.[0-2]
    
Added %{sle_version} checks in Tarantool spec file according to
https://en.opensuse.org/openSUSE:Packaging_for_Leap#RPM_Distro_Version_Macros
    
Found that openSuSE adding linker flag '--no-undefined' which produces
the fails on building tests. Decided to block this flag due to dynamic
libraries will be loaded from tarantool executable and will use symbols
from it. So it is completely okay to have unresolved symbols at build
time.
    
Added opensuse-leap of 15.1 and 15.2 versions to Gitlab-CI packages
building/deploing jobs.
    
Found that test fails on its subtest checking:
   "gh-2872 bootstrap is aborted if vinyl_dir
   contains vylog files left from previous runs"
    
Debugging src/box/xlog.c found that all checks
were correct, but at function:
  src/box/vy_log.c:vy_log_bootstrap()
the check on of the errno on ENOENT blocked the
negative return from function:
  src/box/xlog.c:xdir_scan()
    
Found that errno was already set to ENOENT before
the xdir_scan() call. To fix the issue the errno
must be clean before the call to xdir_scan, because
we are interesting in it only from this function.
The same issue fixed at function:
  src/box/vy_log.c:vy_log_begin_recovery()
    
Found that opensuse adding linker flag '--no-undefined' which produces
the fails on building tests. Decided to block this flag due to dynamic
libraries will be loaded from tarantool executable and will use symbols
from it. So it is completely okay to have unresolved symbols at build
time.
    
Closes #4562
Closes #4594
Needed for #4562

Alexander V. Tikhonov (5):
  Bump luajit repository
  Fix test box-tap/cfg.test.lua on openSuSE
  gitlab-ci: add openSuSE packages build jobs
  TMP: check packpack SuSE
  TMP: Check

---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4562-suse-pack-full-ci
Issue: https://github.com/tarantool/tarantool/issues/4562
Issue: https://github.com/tarantool/tarantool/issues/4594

 .gitlab-ci.yml              | 24 ++++++++++++++++++++++++
 .gitlab.mk                  |  9 +++++----
 rpm/tarantool.spec          | 16 +++++++++++-----
 src/box/vy_log.c            |  2 ++
 test/app-tap/CMakeLists.txt |  4 ++++
 test/app/CMakeLists.txt     |  4 ++++
 test/box-tap/cfg.skipcond   | 10 ++++++++++
 test/box/CMakeLists.txt     |  4 ++++
 third_party/luajit          |  2 +-
 tools/update_repo.sh        |  6 ++++--
 10 files changed, 69 insertions(+), 12 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v2 1/3] Bump luajit repository

[Alexander V. Tikhonov]

Found that opensuse adding linker flag '--no-undefined' which produces
the fails on building tests. Decided to block this flag due to dynamic
libraries will be loaded from tarantool executable and will use symbols
from it. So it is completely okay to have unresolved symbols at build
time.

Needed for #4562
---
 third_party/luajit | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/third_party/luajit b/third_party/luajit
index 377198b88..106539de2 160000
--- a/third_party/luajit
+++ b/third_party/luajit
@@ -1 +1 @@
-Subproject commit 377198b88382960a2f0af9c09014284e34630513
+Subproject commit 106539de29e1273b8b388dcfe66f5dfa5f3a6b76
-- 
2.17.1

[Tarantool-patches] [PATCH v2 2/3] Fix test box-tap/cfg.test.lua on openSuSE

[Alexander V. Tikhonov]

Found that test fails on its subtest checking:
  "gh-2872 bootstrap is aborted if vinyl_dir
   contains vylog files left from previous runs"

Debugging src/box/xlog.c found that all checks
were correct, but at function:
  src/box/vy_log.c:vy_log_bootstrap()
the check on of the errno on ENOENT blocked the
negative return from function:
  src/box/xlog.c:xdir_scan()

Found that errno was already set to ENOENT before
the xdir_scan() call. To fix the issue the errno
must be clean before the call to xdir_scan, because
we are interesting in it only from this function.
The same issue fixed at function:
  src/box/vy_log.c:vy_log_begin_recovery()

Closes #4594
Needed for #4562
---
 src/box/vy_log.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 311985c72..86599fd15 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -1014,6 +1014,7 @@ vy_log_rebootstrap(void)
 int
 vy_log_bootstrap(void)
 {
+	errno = 0;
 	if (xdir_scan(&vy_log.dir) < 0 && errno != ENOENT)
 		return -1;
 	if (xdir_last_vclock(&vy_log.dir, &vy_log.last_checkpoint) >= 0)
@@ -1036,6 +1037,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.
 	 */
+	errno = 0;
 	if (xdir_scan(&vy_log.dir) < 0 && errno != ENOENT)
 		return NULL;
 
-- 
2.17.1

[Tarantool-patches] [PATCH v2 3/3] gitlab-ci: add openSuSE packages build jobs

[Alexander V. Tikhonov]

Implemented openSuSE packages build with testing for images:
opensuse-leap:15.[0-2]

Added %{sle_version} checks in Tarantool spec file according to
https://en.opensuse.org/openSUSE:Packaging_for_Leap#RPM_Distro_Version_Macros

Found that openSuSE adding linker flag '--no-undefined' which produces
the fails on building tests. Decided to block this flag due to dynamic
libraries will be loaded from tarantool executable and will use symbols
from it. So it is completely okay to have unresolved symbols at build
time.

Added opensuse-leap of 15.1 and 15.2 versions to Gitlab-CI packages
building/deploing jobs.

Closes #4562
---
 .gitlab-ci.yml                | 24 ++++++++++++++++++++++++
 rpm/prebuild-opensuse-leap.sh |  7 +++++++
 rpm/tarantool.spec            | 16 +++++++++++-----
 test/app-tap/CMakeLists.txt   |  4 ++++
 test/app/CMakeLists.txt       |  4 ++++
 test/box-tap/cfg.skipcond     | 10 ++++++++++
 test/box/CMakeLists.txt       |  4 ++++
 tools/update_repo.sh          |  6 ++++--
 8 files changed, 68 insertions(+), 7 deletions(-)
 create mode 100755 rpm/prebuild-opensuse-leap.sh

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 962bbcbff..860a4706b 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -415,6 +415,18 @@ debian_10:
     OS: 'debian'
     DIST: 'buster'
 
+opensuse_15_1:
+  <<: *pack_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.1'
+
+opensuse_15_2:
+  <<: *pack_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.2'
+
 # Deploy
 
 sources_deploy:
@@ -512,6 +524,18 @@ debian_10_deploy:
     OS: 'debian'
     DIST: 'buster'
 
+opensuse_15_1_deploy:
+  <<: *deploy_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.1'
+
+opensuse_15_2_deploy:
+  <<: *deploy_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.2'
+
 # Static builds
 
 static_build:
diff --git a/rpm/prebuild-opensuse-leap.sh b/rpm/prebuild-opensuse-leap.sh
new file mode 100755
index 000000000..3861a5308
--- /dev/null
+++ b/rpm/prebuild-opensuse-leap.sh
@@ -0,0 +1,7 @@
+#!/bin/sh -ex
+
+# Found that packages repositories can have broken
+# repositories in zypper cache. To fix it, zypper
+# cache should be refreshed.
+sudo zypper clean
+sudo zypper refresh
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index 88b1d6b5c..eedc0312c 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -1,5 +1,5 @@
 # Enable systemd for on RHEL >= 7 and Fedora >= 15
-%if (0%{?fedora} >= 15 || 0%{?rhel} >= 7)
+%if (0%{?fedora} >= 15 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
 %bcond_without systemd
 %else
 %bcond_with systemd
@@ -7,7 +7,7 @@
 
 BuildRequires: cmake >= 2.8
 BuildRequires: make
-%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7)
+%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
 # RHEL 6 requires devtoolset
 BuildRequires: gcc >= 4.5
 BuildRequires: gcc-c++ >= 4.5
@@ -53,6 +53,12 @@ Requires(preun): initscripts
 %bcond_without backtrace
 %endif
 
+# openSuSE sets its own build directory in its macros, but we
+# want to use in-source build there to simplify the RPM spec.
+%if (0%{?sle_version} >= 1500)
+%global __builddir .
+%endif
+
 %if %{with backtrace}
 BuildRequires: libunwind-devel
 #
@@ -78,7 +84,7 @@ BuildRequires: python2-six >= 1.9.0
 BuildRequires: python2-gevent >= 1.0
 BuildRequires: python2-yaml >= 3.0.9
 %else
-%if (0%{?rhel} != 6)
+%if (0%{?rhel} != 6 || 0%{?sle_version} >= 1500)
 BuildRequires: python >= 2.7
 BuildRequires: python-six >= 1.9.0
 BuildRequires: python-gevent >= 1.0
@@ -105,7 +111,7 @@ Requires: /etc/services
 # Deps for built-in package manager
 # https://github.com/tarantool/tarantool/issues/2612
 Requires: openssl
-%if (0%{?fedora} >= 22 || 0%{?rhel} >= 8)
+%if (0%{?fedora} >= 22 || 0%{?rhel} >= 8 || 0%{?sle_version} >= 1500)
 # RHEL <= 7 doesn't support Recommends:
 Recommends: tarantool-devel
 Recommends: git-core
@@ -164,7 +170,7 @@ rm -rf %{buildroot}%{_datarootdir}/doc/tarantool/
 
 %check
 %if "%{_ci}" == "travis"
-%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7)
+%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
 cd test && ./test-run.py --force -j 1 unit/ app/ app-tap/ box/ box-tap/ engine/ vinyl/
 %endif
 %else
diff --git a/test/app-tap/CMakeLists.txt b/test/app-tap/CMakeLists.txt
index ee67cf533..529ea055f 100644
--- a/test/app-tap/CMakeLists.txt
+++ b/test/app-tap/CMakeLists.txt
@@ -1 +1,5 @@
+# The dynamic libraries will be loaded from tarantool executable
+# and will use symbols from it. So it is completely okay to have
+# unresolved symbols at build time.
+string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
 build_module(module_api module_api.c)
diff --git a/test/app/CMakeLists.txt b/test/app/CMakeLists.txt
index 059ee8f3d..28a2c0698 100644
--- a/test/app/CMakeLists.txt
+++ b/test/app/CMakeLists.txt
@@ -1 +1,5 @@
+# The dynamic libraries will be loaded from tarantool executable
+# and will use symbols from it. So it is completely okay to have
+# unresolved symbols at build time.
+string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
 build_module(loaderslib loaderslib.c)
diff --git a/test/box-tap/cfg.skipcond b/test/box-tap/cfg.skipcond
index 33cafc12b..5f4256b6a 100644
--- a/test/box-tap/cfg.skipcond
+++ b/test/box-tap/cfg.skipcond
@@ -1,3 +1,4 @@
+import os
 import platform
 
 # Disabled on FreeBSD due to fail #4271:
@@ -5,6 +6,15 @@ import platform
 if platform.system() == 'FreeBSD':
     self.skip = 1
 
+# Disabled on openSuSE due to issue #4594:
+#   Test not ready to be used, because of problems
+#   around forking processes from Tarantool.
+try:
+    if os.popen('lsb_release -i').read().split(':')[1].strip() == 'openSUSE':
+        self.skip = 1
+except IndexError:
+    pass
+
 # Disabled on OpenBSD due to fail #XXXX:
 if platform.system() == 'OpenBSD':
     self.skip = 1
diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 06bfbbe9d..8bc547f35 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -1,3 +1,7 @@
+# The dynamic libraries will be loaded from tarantool executable
+# and will use symbols from it. So it is completely okay to have
+# unresolved symbols at build time.
+string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
 include_directories(${MSGPUCK_INCLUDE_DIRS})
 build_module(function1 function1.c)
 build_module(reload1 reload1.c)
diff --git a/tools/update_repo.sh b/tools/update_repo.sh
index 47eadd189..65bafe757 100755
--- a/tools/update_repo.sh
+++ b/tools/update_repo.sh
@@ -6,7 +6,7 @@ rm_dir='rm -rf'
 mk_dir='mkdir -p'
 ws_prefix=/tmp/tarantool_repo_s3
 
-alloss='ubuntu debian el fedora'
+alloss='ubuntu debian el fedora opensuse-leap'
 product=tarantool
 remove=
 force=
@@ -31,6 +31,8 @@ function get_os_dists {
         alldists='6 7 8'
     elif [ "$os" == "fedora" ]; then
         alldists='27 28 29 30 31'
+    elif [ "$os" == "opensuse-leap" ]; then
+        alldists='15.0 15.1 15.2'
     fi
 
     echo "$alldists"
@@ -934,7 +936,7 @@ if [ "$os" == "ubuntu" -o "$os" == "debian" ]; then
     # unlock the publishing
     $rm_file $ws_lockfile
     popd
-elif [ "$os" == "el" -o "$os" == "fedora" ]; then
+elif [ "$os" == "el" -o "$os" == "fedora" -o "$os" == "opensuse-leap" ]; then
     # RPM packages structure needs different paths for binaries and sources
     # packages, in this way it is needed to call the packages registering
     # script twice with the given format:
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v2 2/3] Fix test box-tap/cfg.test.lua on openSuSE

[Alexander Turenko]

> Fix test box-tap/cfg.test.lua on openSuSE
>
> Found that test fails on its subtest checking:
>   "gh-2872 bootstrap is aborted if vinyl_dir
>    contains vylog files left from previous runs"
> 
> Debugging src/box/xlog.c found that all checks
> were correct, but at function:
>   src/box/vy_log.c:vy_log_bootstrap()
> the check on of the errno on ENOENT blocked the
> negative return from function:
>   src/box/xlog.c:xdir_scan()
> 
> Found that errno was already set to ENOENT before
> the xdir_scan() call. To fix the issue the errno
> must be clean before the call to xdir_scan, because
> we are interesting in it only from this function.
> The same issue fixed at function:
>   src/box/vy_log.c:vy_log_begin_recovery()
> 
> Closes #4594
> Needed for #4562

My wish is to find the following information in the commit message:

- The header: the subsystem prefix (vinyl) and what is fixed (vinyl_dir
  existence check at bootstrap).
- What was wrong and becomes right (high-level or from a user
  perspective): when memtx_dir is not exists, but vinyl_dir exists and
  errno is set to ENOENT, box configuration succeeds, however it is
  should not.
- What is the reason of the wrong behaviour: not all failure paths in
  xdir_scan() are set errno, but the caller assumes it.
- From where the problem appears: openSUSE and box-tap/cfg.test.lua.

> ---
>  src/box/vy_log.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

> diff --git a/src/box/vy_log.c b/src/box/vy_log.c
> index 311985c72..86599fd15 100644
> --- a/src/box/vy_log.c
> +++ b/src/box/vy_log.c
> @@ -1014,6 +1014,7 @@ vy_log_rebootstrap(void)
>  int
>  vy_log_bootstrap(void)
>  {
> +	errno = 0;
>  	if (xdir_scan(&vy_log.dir) < 0 && errno != ENOENT)
>  		return -1;

You spotted the problem right.

A bit more context:

    Usual C convention is to report success or failure using a return
    value and set errno at any error. So a caller usually just checks a
    return value and if it means a failure (usually -1), it checks errno
    to determine an exact reason.

    Usual convention in tarantool is a bit different: we use a special
    diagnostics area to report a reason of a failure.

    Not all failure paths of xdir_scan() sets errno (including our
    'invalid instance UUID' case), so we cannot be sure that errno is
    not remains unchanged after a failure of the function.

So, again, your fix is correct.

However the approach with checking errno against ENOENT (No such file or
directory) is not good (with or without your patch). For example:

- What if xdir_scan() would be changed in future and, say, some call
  will rewrite errno after the opendir() call?
- What if some other call inside xdir_scan() will set ENOENT: say,
  open() in xdir_open_cursor() due to some race?

We lean on implementation details of the callee, not its contract.  I
think this way is too fragile and we should either:

- check whether the directory exists before xdir_scan() call
- or pass a flag to xdir_scan() whether the directory should exist.

The latter looks better for me: it does not lead to code duplication.
See the proposed diff below the email.

BTW, there is the simple way to test the problem on any OS:

 | diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
 | index 569b5f463..2eda2f3c2 100755
 | --- a/test/box-tap/cfg.test.lua
 | +++ b/test/box-tap/cfg.test.lua
 | @@ -392,6 +392,8 @@ s:create_index('pk')
 |  os.exit(0)
 |  ]], vinyl_dir))
 |  code = string.format([[
 | +local errno = require('errno')
 | +errno(errno.ENOENT)
 |  box.cfg{vinyl_dir = '%s'}
 |  os.exit(0)
 |  ]], vinyl_dir)

Of course, we should not add it right to this test case, let's copy it
somewhere and add separately.

----

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 311985c72..da3c50e87 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 d8c92aa36..2b894d680 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -559,7 +559,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..74f761994 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -511,13 +511,15 @@ xdir_open_cursor(struct xdir *dir, int64_t signature,
  * @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..3400eb75f 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -187,7 +187,7 @@ xdir_destroy(struct xdir *dir);
  * snapshot or scan through all logs.
  */
 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 +821,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();
 }

Re: [Tarantool-patches] [PATCH v2 2/3] Fix test box-tap/cfg.test.lua on openSuSE

[Alexander V. Tikhonov]

Hi Alexander, thanks a lot for a deep investigation. I've used your
patch and rewrote the commit message. Also I've made there all your
suggestions.

On Fri, Aug 14, 2020 at 05:44:42AM +0300, Alexander Turenko wrote:
> > Fix test box-tap/cfg.test.lua on openSuSE
> >
> > Found that test fails on its subtest checking:
> >   "gh-2872 bootstrap is aborted if vinyl_dir
> >    contains vylog files left from previous runs"
> > 
> > Debugging src/box/xlog.c found that all checks
> > were correct, but at function:
> >   src/box/vy_log.c:vy_log_bootstrap()
> > the check on of the errno on ENOENT blocked the
> > negative return from function:
> >   src/box/xlog.c:xdir_scan()
> > 
> > Found that errno was already set to ENOENT before
> > the xdir_scan() call. To fix the issue the errno
> > must be clean before the call to xdir_scan, because
> > we are interesting in it only from this function.
> > The same issue fixed at function:
> >   src/box/vy_log.c:vy_log_begin_recovery()
> > 
> > Closes #4594
> > Needed for #4562
> 
> My wish is to find the following information in the commit message:
> 
> - The header: the subsystem prefix (vinyl) and what is fixed (vinyl_dir
>   existence check at bootstrap).
> - What was wrong and becomes right (high-level or from a user
>   perspective): when memtx_dir is not exists, but vinyl_dir exists and
>   errno is set to ENOENT, box configuration succeeds, however it is
>   should not.
> - What is the reason of the wrong behaviour: not all failure paths in
>   xdir_scan() are set errno, but the caller assumes it.
> - From where the problem appears: openSUSE and box-tap/cfg.test.lua.
> 
> > ---
> >  src/box/vy_log.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> 
> > diff --git a/src/box/vy_log.c b/src/box/vy_log.c
> > index 311985c72..86599fd15 100644
> > --- a/src/box/vy_log.c
> > +++ b/src/box/vy_log.c
> > @@ -1014,6 +1014,7 @@ vy_log_rebootstrap(void)
> >  int
> >  vy_log_bootstrap(void)
> >  {
> > +	errno = 0;
> >  	if (xdir_scan(&vy_log.dir) < 0 && errno != ENOENT)
> >  		return -1;
> 
> You spotted the problem right.
> 
> A bit more context:
> 
>     Usual C convention is to report success or failure using a return
>     value and set errno at any error. So a caller usually just checks a
>     return value and if it means a failure (usually -1), it checks errno
>     to determine an exact reason.
> 
>     Usual convention in tarantool is a bit different: we use a special
>     diagnostics area to report a reason of a failure.
> 
>     Not all failure paths of xdir_scan() sets errno (including our
>     'invalid instance UUID' case), so we cannot be sure that errno is
>     not remains unchanged after a failure of the function.
> 
> So, again, your fix is correct.
> 
> However the approach with checking errno against ENOENT (No such file or
> directory) is not good (with or without your patch). For example:
> 
> - What if xdir_scan() would be changed in future and, say, some call
>   will rewrite errno after the opendir() call?
> - What if some other call inside xdir_scan() will set ENOENT: say,
>   open() in xdir_open_cursor() due to some race?
> 
> We lean on implementation details of the callee, not its contract.  I
> think this way is too fragile and we should either:
> 
> - check whether the directory exists before xdir_scan() call
> - or pass a flag to xdir_scan() whether the directory should exist.
> 
> The latter looks better for me: it does not lead to code duplication.
> See the proposed diff below the email.
> 
> BTW, there is the simple way to test the problem on any OS:
> 
>  | diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
>  | index 569b5f463..2eda2f3c2 100755
>  | --- a/test/box-tap/cfg.test.lua
>  | +++ b/test/box-tap/cfg.test.lua
>  | @@ -392,6 +392,8 @@ s:create_index('pk')
>  |  os.exit(0)
>  |  ]], vinyl_dir))
>  |  code = string.format([[
>  | +local errno = require('errno')
>  | +errno(errno.ENOENT)
>  |  box.cfg{vinyl_dir = '%s'}
>  |  os.exit(0)
>  |  ]], vinyl_dir)
> 
> Of course, we should not add it right to this test case, let's copy it
> somewhere and add separately.
> 
> ----
> 
> 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 311985c72..da3c50e87 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 d8c92aa36..2b894d680 100644
> --- a/src/box/wal.c
> +++ b/src/box/wal.c
> @@ -559,7 +559,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..74f761994 100644
> --- a/src/box/xlog.c
> +++ b/src/box/xlog.c
> @@ -511,13 +511,15 @@ xdir_open_cursor(struct xdir *dir, int64_t signature,
>   * @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..3400eb75f 100644
> --- a/src/box/xlog.h
> +++ b/src/box/xlog.h
> @@ -187,7 +187,7 @@ xdir_destroy(struct xdir *dir);
>   * snapshot or scan through all logs.
>   */
>  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 +821,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();
>  }
>