[tarantool-patches] [PATCH] travis-ci: set right flags in release testing jobs

[tarantool-patches] [PATCH] travis-ci: set right flags in release testing jobs

[Alexander Turenko]

It is important to have testing jobs that build the project with both
-Werror and -O2 to keep the code clean. -O2 is needed, because some
compiler warnings are available only after extra analyzing passes that
are disabled with lesser optimization levels.

The first attempt to add -Werror for release testing jobs was made in
da505ee72883195121b7006bbd52bc66e57321f3 ('Add -Werror for CI (1.10
part)'), but it mistakely doesn't enable -O2 for RelWithDebInfoWError
build. It is possible to fix it in this way:

 | --- a/cmake/compiler.cmake
 | +++ b/cmake/compiler.cmake
 | @@ -113,10 +113,14 @@ set (CMAKE_C_FLAGS_DEBUG
 |      "${CMAKE_C_FLAGS_DEBUG} ${CC_DEBUG_OPT} -O0")
 |  set (CMAKE_C_FLAGS_RELWITHDEBINFO
 |      "${CMAKE_C_FLAGS_RELWITHDEBINFO} ${CC_DEBUG_OPT} -O2")
 | +set (CMAKE_C_FLAGS_RELWITHDEBINFOWERROR
 | +    "${CMAKE_C_FLAGS_RELWITHDEBINFOWERROR} ${CC_DEBUG_OPT} -O2")
 |  set (CMAKE_CXX_FLAGS_DEBUG
 |      "${CMAKE_CXX_FLAGS_DEBUG} ${CC_DEBUG_OPT} -O0")
 |  set (CMAKE_CXX_FLAGS_RELWITHDEBINFO
 |      "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} ${CC_DEBUG_OPT} -O2")
 | +set (CMAKE_CXX_FLAGS_RELWITHDEBINFOWERROR
 | +    "${CMAKE_CXX_FLAGS_RELWITHDEBINFOWERROR} ${CC_DEBUG_OPT} -O2")
 |
 |  unset(CC_DEBUG_OPT)

However I think that a build type (and so `tarantool --version`) should
not show whether -Werror was passed or not. So I have added
ENABLE_WERROR CMake option for that. It can be set like so:

 | cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON

Enabled the option in testing Travis-CI jobs with the RelWithDebInfo
build type. Deploy jobs don't include it as before.

Fixed all -Wmaybe-uninitialized warnings. A few notes about the fixes:

* net.box does not validate received data in general, so I don't add a
  check for autoincrement IDs too.
* xrow_decode_*() functions could read uninitialized data when printing
  a hex code for a row if the received msgpack is empty, but there are
  expected keys.

Added comments and quotes into .travis.yml to ease reading. Removed
"test" word from the CentOS 6 job name, because we don't run tests on
this distro (disabled in the RPM spec).

Fixes #4178.
---

I'm a bit tentative about xrow.c changes. They are correct in the sense
that the code will work, but maybe there is a better way to eliminate
the warnings.

This patch targets all currently supported long-term branches.

https://github.com/tarantool/tarantool/issues/4178
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4178-set-right-flags-in-release-testing-jobs

 .travis.mk            |  4 ++--
 .travis.yml           | 45 +++++++++++++++++++++++--------------------
 cmake/compiler.cmake  | 16 ++++++++++-----
 src/box/lua/net_box.c |  4 ++--
 src/box/sql/func.c    | 13 +++++++------
 src/box/vy_cache.c    |  2 +-
 src/box/vy_mem.c      |  2 +-
 src/box/xrow.c        | 33 ++++++++++++++++++++-----------
 src/lua/init.c        |  4 +++-
 9 files changed, 73 insertions(+), 50 deletions(-)

diff --git a/.travis.mk b/.travis.mk
index 247bb2a2d..973c6a7c7 100644
--- a/.travis.mk
+++ b/.travis.mk
@@ -40,7 +40,7 @@ deps_ubuntu:
 		lcov ruby
 
 test_ubuntu: deps_ubuntu
-	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS}
+	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
 	make -j8
 	cd test && /usr/bin/python test-run.py -j 1
 
@@ -51,7 +51,7 @@ deps_osx:
 	pip install -r test-run/requirements.txt
 
 test_osx: deps_osx
-	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS}
+	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
 	# Increase the maximum number of open file descriptors on macOS
 	sudo sysctl -w kern.maxfiles=20480 || :
 	sudo sysctl -w kern.maxfilesperproc=20480 || :
diff --git a/.travis.yml b/.travis.yml
index e30540837..6e79cf2fe 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -19,69 +19,72 @@ git:
 
 jobs:
     include:
-      - name: RelWithDebInfoWError build + test (Linux, gcc)
+      # Testing targets (just run tests on Debian Stretch or OS X).
+      - name: "RelWithDebInfo build + test (Linux, gcc)"
         env: TARGET=test
-      - name: RelWithDebInfoWError build + test (Linux, clang)
+      - name: "RelWithDebInfo build + test (Linux, clang)"
         env: TARGET=test
         compiler: clang
-      - name: RelWithDebInfoWError build + test (OS X Mojave 10.14)
+      - name: "RelWithDebInfo build + test (OS X Mojave 10.14)"
         env: TARGET=test
         os: osx
-      - name: Debug build + test + coverage (Linux, gcc)
+      - name: "Debug build + test + coverage (Linux, gcc)"
         env: TARGET=coverage
-      - name: RelWithDebInfoWError build + test (OS X High Sierra 10.13)
+      - name: "RelWithDebInfo build + test (OS X High Sierra 10.13)"
         env: TARGET=test
         os: osx
         osx_image: xcode9.4
         if: branch = "master"
-      - name: LTO build + test (Linux, gcc)
+      # Special targets (only LTO for now).
+      - name: "LTO build + test (Linux, gcc)"
         env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
         if: branch = "master"
-      - name: LTO build + test (Linux, clang)
+      - name: "LTO build + test (Linux, clang)"
         env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
         if: branch = "master"
         compiler: clang
-      - name: LTO build + test (OS X Mojave 10.14)
+      - name: "LTO build + test (OS X Mojave 10.14)"
         os: osx
         env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
         if: branch = "master"
-      - name: Create and deploy tarball
+      # Deploy targets (they also catch distro-specific problems).
+      - name: "Create and deploy tarball"
         env: TARGET=source
         if: branch = "master"
-      - name: CentOS 6 build + test + deploy RPM
+      - name: "CentOS 6 build + deploy RPM"
         env: OS=el DIST=6
         if: branch = "master"
-      - name: CentOS 7 build + test + deploy RPM
+      - name: "CentOS 7 build + test + deploy RPM"
         env: OS=el DIST=7
         if: branch = "master"
-      - name: Fedora 28 build + test + deploy RPM
+      - name: "Fedora 28 build + test + deploy RPM"
         env: OS=fedora DIST=28
         if: branch = "master"
-      - name: Fedora 29 build + test + deploy RPM
+      - name: "Fedora 29 build + test + deploy RPM"
         env: OS=fedora DIST=29
         if: branch = "master"
-      - name: Ubuntu Trusty (14.04) build + deploy DEB
+      - name: "Ubuntu Trusty (14.04) build + deploy DEB"
         env: OS=ubuntu DIST=trusty
         if: branch = "master"
-      - name: Ubuntu Xenial (16.04) build + deploy DEB
+      - name: "Ubuntu Xenial (16.04) build + deploy DEB"
         env: OS=ubuntu DIST=xenial
         if: branch = "master"
-      - name: Ubuntu Bionic (18.04) build + deploy DEB
+      - name: "Ubuntu Bionic (18.04) build + deploy DEB"
         env: OS=ubuntu DIST=bionic
         if: branch = "master"
-      - name: Ubuntu Cosmic (18.10) build + deploy DEB
+      - name: "Ubuntu Cosmic (18.10) build + deploy DEB"
         env: OS=ubuntu DIST=cosmic
         if: branch = "master"
-      - name: Ubuntu Disco (19.04) build + deploy DEB
+      - name: "Ubuntu Disco (19.04) build + deploy DEB"
         env: OS=ubuntu DIST=disco
         if: branch = "master"
-      - name: Debian Jessie (8) build + deploy DEB
+      - name: "Debian Jessie (8) build + deploy DEB"
         env: OS=debian DIST=jessie
         if: branch = "master"
-      - name: Debian Stretch (9) build + deploy DEB
+      - name: "Debian Stretch (9) build + deploy DEB"
         env: OS=debian DIST=stretch
         if: branch = "master"
-      - name: Debian Buster (10) build + deploy DEB
+      - name: "Debian Buster (10) build + deploy DEB"
         env: OS=debian DIST=buster
         if: branch = "master"
 
diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
index 4062d13ec..887485c80 100644
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@@ -223,6 +223,8 @@ if (NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
     add_definitions("-DNDEBUG" "-DNVALGRIND")
 endif()
 
+option(ENABLE_WERROR "Make all compiler warnings into errors" OFF)
+
 macro(enable_tnt_compile_flags)
     # Tarantool code is written in GNU C dialect.
     # Additionally, compile it with more strict flags than the rest
@@ -286,11 +288,15 @@ macro(enable_tnt_compile_flags)
     add_definitions("-D__STDC_LIMIT_MACROS=1")
     add_definitions("-D__STDC_CONSTANT_MACROS=1")
 
-    # Only add -Werror if it's a Debug or RelWithDebInfoWError build, done by
-    # developers. Release builds should not cause extra trouble.
-    if (((${CMAKE_BUILD_TYPE} STREQUAL "Debug") OR
-        (${CMAKE_BUILD_TYPE} STREQUAL "RelWithDebInfoWError")) AND
-        HAVE_STD_C11 AND HAVE_STD_CXX11)
+    # Only add -Werror if it's a debug build, done by developers.
+    # Release builds should not cause extra trouble.
+    if ((${CMAKE_BUILD_TYPE} STREQUAL "Debug")
+        AND HAVE_STD_C11 AND HAVE_STD_CXX11)
+        add_compile_flags("C;CXX" "-Werror")
+    endif()
+
+    # Add -Werror if it is requested explicitly.
+    if (ENABLE_WERROR)
         add_compile_flags("C;CXX" "-Werror")
     endif()
 endmacro(enable_tnt_compile_flags)
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 2dfc4fc9d..14177aaab 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
 	if (map_size == 2) {
 		key = mp_decode_uint(data);
 		assert(key == SQL_INFO_AUTOINCREMENT_IDS);
-		(void)key;
+		(void) key;
 		uint64_t count = mp_decode_array(data);
 		assert(count > 0);
 		lua_createtable(L, 0, count);
 		for (uint32_t j = 0; j < count; ++j) {
-			int64_t id;
+			int64_t id = INT64_MIN;
 			mp_read_int64(data, &id);
 			luaL_pushint64(L, id);
 			lua_rawseti(L, -2, j + 1);
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f3790ba01..f96709a75 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -761,12 +761,13 @@ sql_utf8_pattern_compare(const char *pattern,
 				if (c != MATCH_ALL_WILDCARD &&
 				    c != MATCH_ONE_WILDCARD)
 					break;
-				if (c == MATCH_ONE_WILDCARD &&
-				    (c2 = Utf8Read(string, string_end)) ==
-				    SQL_END_OF_STRING)
-					return NO_WILDCARD_MATCH;
-				if (c2 == SQL_INVALID_UTF8_SYMBOL)
-					return NO_MATCH;
+				if (c == MATCH_ONE_WILDCARD) {
+					c2 = Utf8Read(string, string_end);
+					if (c2 == SQL_INVALID_UTF8_SYMBOL)
+						return NO_MATCH;
+					if (c2 == SQL_END_OF_STRING)
+						return NO_WILDCARD_MATCH;
+				}
 			}
 			/*
 			 * "%" at the end of the pattern matches.
diff --git a/src/box/vy_cache.c b/src/box/vy_cache.c
index be11b986f..7007d0e35 100644
--- a/src/box/vy_cache.c
+++ b/src/box/vy_cache.c
@@ -661,7 +661,7 @@ vy_cache_iterator_seek(struct vy_cache_iterator *itr, struct vy_entry last)
 				ITER_GT : ITER_LT;
 	}
 
-	bool exact;
+	bool exact = false;
 	if (!vy_stmt_is_empty_key(key.stmt)) {
 		itr->curr_pos = iterator_type == ITER_EQ ||
 				iterator_type == ITER_GE ||
diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
index 9513d5b80..a4fae26e2 100644
--- a/src/box/vy_mem.c
+++ b/src/box/vy_mem.c
@@ -392,7 +392,7 @@ vy_mem_iterator_seek(struct vy_mem_iterator *itr, struct vy_entry last)
 				ITER_GT : ITER_LT;
 	}
 
-	bool exact;
+	bool exact = false;
 	struct vy_mem_tree_key tree_key;
 	tree_key.entry = key;
 	/* (lsn == INT64_MAX - 1) means that lsn is ignored in comparison */
diff --git a/src/box/xrow.c b/src/box/xrow.c
index aaed84e38..ccdd9def5 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -509,9 +509,14 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
 			     schema_version, sizeof(body) + msg_len);
 
 	body.v_data_len = mp_bswap_u32(msg_len);
-	(void) write(fd, header, sizeof(header));
-	(void) write(fd, &body, sizeof(body));
-	(void) write(fd, e->errmsg, msg_len);
+
+	int rc1, rc2, rc3;
+	rc1 = write(fd, header, sizeof(header));
+	rc2 = write(fd, &body, sizeof(body));
+	rc3 = write(fd, e->errmsg, msg_len);
+	(void) rc1;
+	(void) rc2;
+	(void) rc3;
 }
 
 int
@@ -624,12 +629,15 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
 	request->header = row;
 	request->type = row->type;
 
+	const char *start = NULL;
+	const char *end = NULL;
+
 	if (row->bodycnt == 0)
 		goto done;
 
 	assert(row->bodycnt == 1);
-	const char *data = (const char *) row->body[0].iov_base;
-	const char *end = data + row->body[0].iov_len;
+	const char *data = start = (const char *) row->body[0].iov_base;
+	end = data + row->body[0].iov_len;
 	assert((end - data) > 0);
 
 	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
@@ -701,8 +709,8 @@ error:
 done:
 	if (key_map) {
 		enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
-		xrow_on_decode_err(row->body[0].iov_base, end,
-				   ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
+		xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
+				   iproto_key_name(key));
 		return -1;
 	}
 	return 0;
@@ -1071,12 +1079,16 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 	ballot->is_ro = false;
 	vclock_create(&ballot->vclock);
 
+	const char *start = NULL;
+	const char *end = NULL;
+
 	if (row->bodycnt == 0)
 		goto err;
+
 	assert(row->bodycnt == 1);
 
-	const char *data = (const char *) row->body[0].iov_base;
-	const char *end = data + row->body[0].iov_len;
+	const char *data = start = (const char *) row->body[0].iov_base;
+	end = data + row->body[0].iov_len;
 	const char *tmp = data;
 	if (mp_check(&tmp, end) != 0 || mp_typeof(*data) != MP_MAP)
 		goto err;
@@ -1124,8 +1136,7 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 	}
 	return 0;
 err:
-	xrow_on_decode_err(row->body[0].iov_base, end, ER_INVALID_MSGPACK,
-			   "packet body");
+	xrow_on_decode_err(start, end, ER_INVALID_MSGPACK, "packet body");
 	return -1;
 }
 
diff --git a/src/lua/init.c b/src/lua/init.c
index d18c8af94..f66a3eb46 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -319,7 +319,9 @@ tarantool_lua_setpaths(struct lua_State *L)
 {
 	const char *home = getenv("HOME");
 	char cwd[PATH_MAX] = {'\0'};
-	getcwd(cwd, sizeof(cwd));
+	char *rc = getcwd(cwd, sizeof(cwd));
+	if (rc == NULL)
+		panic("can't get cwd; errno: %d", errno);
 	lua_getglobal(L, "package");
 	int top = lua_gettop(L);
 
-- 
2.21.0

[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

Firstly, after your patch I can't build Tarantool with gcc-7
on Mac with this new -DENABLE_WERROR=ON option. I get these
errors:

error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
errorScanning dependencies of target rope_stress.test
error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]

After that I found that master branch also can't be built, but
for another reason:
https://github.com/tarantool/tarantool/issues/4184

I think, that your commit should not exacerbate it, at least.

See 9 comments below, and my fixes on the branch in a separate commit.

> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 2dfc4fc9d..14177aaab 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
>  	if (map_size == 2) {
>  		key = mp_decode_uint(data);
>  		assert(key == SQL_INFO_AUTOINCREMENT_IDS);
> -		(void)key;
> +		(void) key;

1. Unnecessary diff. Please, drop.

>  		uint64_t count = mp_decode_array(data);
>  		assert(count > 0);
>  		lua_createtable(L, 0, count);
>  		for (uint32_t j = 0; j < count; ++j) {
> -			int64_t id;
> +			int64_t id = INT64_MIN;

2. Is there any concrete reason why the value is so
non-trivial? Why not just simple zero?

>  			mp_read_int64(data, &id);
>  			luaL_pushint64(L, id);
>  			lua_rawseti(L, -2, j + 1);
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index f3790ba01..f96709a75 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -761,12 +761,13 @@ sql_utf8_pattern_compare(const char *pattern,
>  				if (c != MATCH_ALL_WILDCARD &&
>  				    c != MATCH_ONE_WILDCARD)
>  					break;
> -				if (c == MATCH_ONE_WILDCARD &&
> -				    (c2 = Utf8Read(string, string_end)) ==
> -				    SQL_END_OF_STRING)
> -					return NO_WILDCARD_MATCH;
> -				if (c2 == SQL_INVALID_UTF8_SYMBOL)
> -					return NO_MATCH;
> +				if (c == MATCH_ONE_WILDCARD) {

3. Now you check for 'c == MATCH_ONE_WILDCARD' two times: here and
two lines above. Please, consider my fixes:

=====================================================================
 			       SQL_END_OF_STRING) {
 				if (c == SQL_INVALID_UTF8_SYMBOL)
 					return INVALID_PATTERN;
-				if (c != MATCH_ALL_WILDCARD &&
-				    c != MATCH_ONE_WILDCARD)
-					break;
 				if (c == MATCH_ONE_WILDCARD) {
 					c2 = Utf8Read(string, string_end);
 					if (c2 == SQL_INVALID_UTF8_SYMBOL)
 						return NO_MATCH;
 					if (c2 == SQL_END_OF_STRING)
 						return NO_WILDCARD_MATCH;
+				} else if (c != MATCH_ALL_WILDCARD) {
+					break;
 				}
 			}
 			/*
=====================================================================

> +					c2 = Utf8Read(string, string_end);
> +					if (c2 == SQL_INVALID_UTF8_SYMBOL)
> +						return NO_MATCH;
> +					if (c2 == SQL_END_OF_STRING)
> +						return NO_WILDCARD_MATCH;
> +				}
>  			}
>  			/*
>  			 * "%" at the end of the pattern matches.
> diff --git a/src/box/vy_cache.c b/src/box/vy_cache.c
> index be11b986f..7007d0e35 100644
> --- a/src/box/vy_cache.c
> +++ b/src/box/vy_cache.c
> @@ -661,7 +661,7 @@ vy_cache_iterator_seek(struct vy_cache_iterator *itr, struct vy_entry last)
>  				ITER_GT : ITER_LT;
>  	}
>  
> -	bool exact;
> +	bool exact = false;
>  	if (!vy_stmt_is_empty_key(key.stmt)) {
>  		itr->curr_pos = iterator_type == ITER_EQ ||
>  				iterator_type == ITER_GE ||
> diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
> index 9513d5b80..a4fae26e2 100644
> --- a/src/box/vy_mem.c
> +++ b/src/box/vy_mem.c
> @@ -392,7 +392,7 @@ vy_mem_iterator_seek(struct vy_mem_iterator *itr, struct vy_entry last)
>  				ITER_GT : ITER_LT;
>  	}
>  
> -	bool exact;
> +	bool exact = false;

4. Sorry, can't review Vinyl part, because I do not understand this code.
But anyway your solution is better than an uninitialized variable. By the way,
can it be the problem Kirill investigates right now, when some tuples are
not returned from 'select'?

>  	struct vy_mem_tree_key tree_key;
>  	tree_key.entry = key;
>  	/* (lsn == INT64_MAX - 1) means that lsn is ignored in comparison */
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index aaed84e38..ccdd9def5 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -509,9 +509,14 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
>  			     schema_version, sizeof(body) + msg_len);
>  
>  	body.v_data_len = mp_bswap_u32(msg_len);
> -	(void) write(fd, header, sizeof(header));
> -	(void) write(fd, &body, sizeof(body));
> -	(void) write(fd, e->errmsg, msg_len);
> +
> +	int rc1, rc2, rc3;
> +	rc1 = write(fd, header, sizeof(header));
> +	rc2 = write(fd, &body, sizeof(body));
> +	rc3 = write(fd, e->errmsg, msg_len);
> +	(void) rc1;
> +	(void) rc2;
> +	(void) rc3;

5. Firstly, you could use one 'rc' for all of them. It would be shorter.
Secondly, why not just remove these (void) casts? We ignore returned value
in many places and it is ok, if it isn't really important.

In this concrete place an error has already happened, and it makes
no sense to create one another - this is why the values are ignored.

=====================================================================
 	body.v_data_len = mp_bswap_u32(msg_len);
-
-	int rc1, rc2, rc3;
-	rc1 = write(fd, header, sizeof(header));
-	rc2 = write(fd, &body, sizeof(body));
-	rc3 = write(fd, e->errmsg, msg_len);
-	(void) rc1;
-	(void) rc2;
-	(void) rc3;
+	write(fd, header, sizeof(header));
+	write(fd, &body, sizeof(body));
+	write(fd, e->errmsg, msg_len);
=====================================================================

>  }
>  
>  int
> @@ -624,12 +629,15 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
>  	request->header = row;
>  	request->type = row->type;
>  
> +	const char *start = NULL;
> +	const char *end = NULL;
> +
>  	if (row->bodycnt == 0)
>  		goto done;
>  
>  	assert(row->bodycnt == 1);
> -	const char *data = (const char *) row->body[0].iov_base;
> -	const char *end = data + row->body[0].iov_len;
> +	const char *data = start = (const char *) row->body[0].iov_base;
> +	end = data + row->body[0].iov_len;
>  	assert((end - data) > 0);
>  
>  	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
> @@ -701,8 +709,8 @@ error:
>  done:
>  	if (key_map) {
>  		enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
> -		xrow_on_decode_err(row->body[0].iov_base, end,
> -				   ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
> +		xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
> +				   iproto_key_name(key));

6. Why did you add 'start' variable? This code was fully okay, as I
understand. Compiler could not raise an error on 'row->body[0].iov_base'
usage here. Please, drop this hunk. A patch should not contain unrelated
changes making review harder and git history dirtier.

>  		return -1;
>  	}
>  	return 0;
> @@ -1071,12 +1079,16 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
>  	ballot->is_ro = false;
>  	vclock_create(&ballot->vclock);
>  
> +	const char *start = NULL;
> +	const char *end = NULL;
> +
>  	if (row->bodycnt == 0)
>  		goto err;
> +

7. Excessive empty line. Please, drop.

>  	assert(row->bodycnt == 1);
>  
> -	const char *data = (const char *) row->body[0].iov_base;
> -	const char *end = data + row->body[0].iov_len;
> +	const char *data = start = (const char *) row->body[0].iov_base;
> +	end = data + row->body[0].iov_len;
>  	const char *tmp = data;
>  	if (mp_check(&tmp, end) != 0 || mp_typeof(*data) != MP_MAP)
>  		goto err;
> @@ -1124,8 +1136,7 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
>  	}
>  	return 0;
>  err:
> -	xrow_on_decode_err(row->body[0].iov_base, end, ER_INVALID_MSGPACK,
> -			   "packet body");
> +	xrow_on_decode_err(start, end, ER_INVALID_MSGPACK, "packet body");

8. The same as 6.

>  	return -1;
>  }
>  
> diff --git a/src/lua/init.c b/src/lua/init.c
> index d18c8af94..f66a3eb46 100644
> --- a/src/lua/init.c
> +++ b/src/lua/init.c
> @@ -319,7 +319,9 @@ tarantool_lua_setpaths(struct lua_State *L)
>  {
>  	const char *home = getenv("HOME");
>  	char cwd[PATH_MAX] = {'\0'};
> -	getcwd(cwd, sizeof(cwd));
> +	char *rc = getcwd(cwd, sizeof(cwd));
> +	if (rc == NULL)
> +		panic("can't get cwd; errno: %d", errno);

9. 'cwd' variable is not used at all and can be removed. It was
introduced here 9e7c421758af156d19a66cebf713e24b42d8654b but
was forgotten to remove sometime.

=====================================================================
 	const char *home = getenv("HOME");
-	char cwd[PATH_MAX] = {'\0'};
-	char *rc = getcwd(cwd, sizeof(cwd));
-	if (rc == NULL)
-		panic("can't get cwd; errno: %d", errno);
 	lua_getglobal(L, "package");
 	int top = lua_gettop(L);
=====================================================================

>  	lua_getglobal(L, "package");
>  	int top = lua_gettop(L);
>  
> -- 
> 2.21.0
> 

[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Alexander Turenko]

Hi!

Thanks for your time. Answered inline.

Pasted diff from my previous commit at end of the email.

WBR, Alexander Turenko.

On Fri, Apr 26, 2019 at 01:18:28PM +0300, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> Firstly, after your patch I can't build Tarantool with gcc-7
> on Mac with this new -DENABLE_WERROR=ON option. I get these
> errors:
> 
> error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
> errorScanning dependencies of target rope_stress.test
> error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
> error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
> error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
> error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
> 
> After that I found that master branch also can't be built, but
> for another reason:
> https://github.com/tarantool/tarantool/issues/4184
> 
> I think, that your commit should not exacerbate it, at least.

GCC ignores unknown -W... options to allow to enable or disable warnings
that is introduced in newer GCC versions unconditionally. However when
some error occurs GCC reports about unknown -W... options.

This is just how GCC operates. You can safely ignore this output: it
will not appear when a build is successful.

> 
> See 9 comments below, and my fixes on the branch in a separate commit.
> 
> > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> > index 2dfc4fc9d..14177aaab 100644
> > --- a/src/box/lua/net_box.c
> > +++ b/src/box/lua/net_box.c
> > @@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
> >  	if (map_size == 2) {
> >  		key = mp_decode_uint(data);
> >  		assert(key == SQL_INFO_AUTOINCREMENT_IDS);
> > -		(void)key;
> > +		(void) key;
> 
> 1. Unnecessary diff. Please, drop.

I don't think so. It is okay to fix a code block that you touch in a
commit to fix it according to a code style.

This would be applicable if I would change some unrelated block of code:
here I'm agree with you.

> 
> >  		uint64_t count = mp_decode_array(data);
> >  		assert(count > 0);
> >  		lua_createtable(L, 0, count);
> >  		for (uint32_t j = 0; j < count; ++j) {
> > -			int64_t id;
> > +			int64_t id = INT64_MIN;
> 
> 2. Is there any concrete reason why the value is so
> non-trivial? Why not just simple zero?

We don't verify a data received from a network in net.box for
correctness. AFAIU from your words it was the design decision. So I
consider this broken by design, but try to at least set a value that
less probably appear in a normal output to signal a user that something
going wrong (when we get something other then int64 here).

Of course, we should give an error in the case, but I left this case
unverified to keep things consistent.

> 
> >  			mp_read_int64(data, &id);
> >  			luaL_pushint64(L, id);
> >  			lua_rawseti(L, -2, j + 1);
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index f3790ba01..f96709a75 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -761,12 +761,13 @@ sql_utf8_pattern_compare(const char *pattern,
> >  				if (c != MATCH_ALL_WILDCARD &&
> >  				    c != MATCH_ONE_WILDCARD)
> >  					break;
> > -				if (c == MATCH_ONE_WILDCARD &&
> > -				    (c2 = Utf8Read(string, string_end)) ==
> > -				    SQL_END_OF_STRING)
> > -					return NO_WILDCARD_MATCH;
> > -				if (c2 == SQL_INVALID_UTF8_SYMBOL)
> > -					return NO_MATCH;
> > +				if (c == MATCH_ONE_WILDCARD) {
> 
> 3. Now you check for 'c == MATCH_ONE_WILDCARD' two times: here and
> two lines above. Please, consider my fixes:
> 
> =====================================================================
>  			       SQL_END_OF_STRING) {
>  				if (c == SQL_INVALID_UTF8_SYMBOL)
>  					return INVALID_PATTERN;
> -				if (c != MATCH_ALL_WILDCARD &&
> -				    c != MATCH_ONE_WILDCARD)
> -					break;
>  				if (c == MATCH_ONE_WILDCARD) {
>  					c2 = Utf8Read(string, string_end);
>  					if (c2 == SQL_INVALID_UTF8_SYMBOL)
>  						return NO_MATCH;
>  					if (c2 == SQL_END_OF_STRING)
>  						return NO_WILDCARD_MATCH;
> +				} else if (c != MATCH_ALL_WILDCARD) {
> +					break;
>  				}
>  			}
>  			/*
> =====================================================================

Thanks. Applied.

> 
> > +					c2 = Utf8Read(string, string_end);
> > +					if (c2 == SQL_INVALID_UTF8_SYMBOL)
> > +						return NO_MATCH;
> > +					if (c2 == SQL_END_OF_STRING)
> > +						return NO_WILDCARD_MATCH;
> > +				}
> >  			}
> >  			/*
> >  			 * "%" at the end of the pattern matches.
> > diff --git a/src/box/vy_cache.c b/src/box/vy_cache.c
> > index be11b986f..7007d0e35 100644
> > --- a/src/box/vy_cache.c
> > +++ b/src/box/vy_cache.c
> > @@ -661,7 +661,7 @@ vy_cache_iterator_seek(struct vy_cache_iterator *itr, struct vy_entry last)
> >  				ITER_GT : ITER_LT;
> >  	}
> >  
> > -	bool exact;
> > +	bool exact = false;
> >  	if (!vy_stmt_is_empty_key(key.stmt)) {
> >  		itr->curr_pos = iterator_type == ITER_EQ ||
> >  				iterator_type == ITER_GE ||
> > diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
> > index 9513d5b80..a4fae26e2 100644
> > --- a/src/box/vy_mem.c
> > +++ b/src/box/vy_mem.c
> > @@ -392,7 +392,7 @@ vy_mem_iterator_seek(struct vy_mem_iterator *itr, struct vy_entry last)
> >  				ITER_GT : ITER_LT;
> >  	}
> >  
> > -	bool exact;
> > +	bool exact = false;
> 
> 4. Sorry, can't review Vinyl part, because I do not understand this code.
> But anyway your solution is better than an uninitialized variable. By the way,
> can it be the problem Kirill investigates right now, when some tuples are
> not returned from 'select'?

Unlikely. This warning is false positive here and the patch does not
really change any behaviour around vinyl.

> 
> >  	struct vy_mem_tree_key tree_key;
> >  	tree_key.entry = key;
> >  	/* (lsn == INT64_MAX - 1) means that lsn is ignored in comparison */
> > diff --git a/src/box/xrow.c b/src/box/xrow.c
> > index aaed84e38..ccdd9def5 100644
> > --- a/src/box/xrow.c
> > +++ b/src/box/xrow.c
> > @@ -509,9 +509,14 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
> >  			     schema_version, sizeof(body) + msg_len);
> >  
> >  	body.v_data_len = mp_bswap_u32(msg_len);
> > -	(void) write(fd, header, sizeof(header));
> > -	(void) write(fd, &body, sizeof(body));
> > -	(void) write(fd, e->errmsg, msg_len);
> > +
> > +	int rc1, rc2, rc3;
> > +	rc1 = write(fd, header, sizeof(header));
> > +	rc2 = write(fd, &body, sizeof(body));
> > +	rc3 = write(fd, e->errmsg, msg_len);
> > +	(void) rc1;
> > +	(void) rc2;
> > +	(void) rc3;
> 
> 5. Firstly, you could use one 'rc' for all of them. It would be shorter.
> Secondly, why not just remove these (void) casts? We ignore returned value
> in many places and it is ok, if it isn't really important.
> 
> In this concrete place an error has already happened, and it makes
> no sense to create one another - this is why the values are ignored.
> 
> =====================================================================
>  	body.v_data_len = mp_bswap_u32(msg_len);
> -
> -	int rc1, rc2, rc3;
> -	rc1 = write(fd, header, sizeof(header));
> -	rc2 = write(fd, &body, sizeof(body));
> -	rc3 = write(fd, e->errmsg, msg_len);
> -	(void) rc1;
> -	(void) rc2;
> -	(void) rc3;
> +	write(fd, header, sizeof(header));
> +	write(fd, &body, sizeof(body));
> +	write(fd, e->errmsg, msg_len);
> =====================================================================

The write() glibc syscall wrapper is marked with
__attribute__((__warn_unused_result__)), so we cannot ignore this result
when -Werror is set. Maybe it is not so for your libc implementation /
version: in this case you'll not get this warning.

Re using one rc: I had the thought that clang tracks whether a result is
used or discarded with a next assingment, so I wrote the code in that
way. But I don't observe such behaviour with clang-6, 7, 8 and 3.9. It
seems I did remember it wrongly and a result is considered as 'used'
when we assign it; a cast to void is just to eliminate the 'variable set
but not used' warning.

GCC complains about (void) write(...);, so we need to assign a result
into a variable: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

Rewritten as follows:

ssize_t unused;
unused = write(fd, header, sizeof(header));
unused = write(fd, &body, sizeof(body));
unused = write(fd, e->errmsg, msg_len);
(void) unused;

> 
> >  }
> >  
> >  int
> > @@ -624,12 +629,15 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
> >  	request->header = row;
> >  	request->type = row->type;
> >  
> > +	const char *start = NULL;
> > +	const char *end = NULL;
> > +
> >  	if (row->bodycnt == 0)
> >  		goto done;
> >  
> >  	assert(row->bodycnt == 1);
> > -	const char *data = (const char *) row->body[0].iov_base;
> > -	const char *end = data + row->body[0].iov_len;
> > +	const char *data = start = (const char *) row->body[0].iov_base;
> > +	end = data + row->body[0].iov_len;
> >  	assert((end - data) > 0);
> >  
> >  	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
> > @@ -701,8 +709,8 @@ error:
> >  done:
> >  	if (key_map) {
> >  		enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
> > -		xrow_on_decode_err(row->body[0].iov_base, end,
> > -				   ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
> > +		xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
> > +				   iproto_key_name(key));
> 
> 6. Why did you add 'start' variable? This code was fully okay, as I
> understand. Compiler could not raise an error on 'row->body[0].iov_base'
> usage here. Please, drop this hunk. A patch should not contain unrelated
> changes making review harder and git history dirtier.

The compiler warning is not false positive here: the code was not okay.
I'll cite the commit message:

> * xrow_decode_*() functions could read uninitialized data when printing
>   a hex code for a row if the received msgpack is empty, but there are
>   expected keys.

Please, suggest a change for wording if the current one is not clear.

My change causes `for (const char *cur = start; cur < end;) { ... }` in
dump_row_hex() to walk around the loop body and so prevents reading from
`start` in the loop (NULL < NULL is false).

> 
> >  		return -1;
> >  	}
> >  	return 0;
> > @@ -1071,12 +1079,16 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
> >  	ballot->is_ro = false;
> >  	vclock_create(&ballot->vclock);
> >  
> > +	const char *start = NULL;
> > +	const char *end = NULL;
> > +
> >  	if (row->bodycnt == 0)
> >  		goto err;
> > +
> 
> 7. Excessive empty line. Please, drop.

Don't know which empty line you considered as excessive. If you can link
a guide clause about that or describe a formal rule you want to make
mandatory, then put it here. In the latter case it would be good to
strengthen it with examples from our code. (However I propose to don't
spend time on that: it is counter-productive.)

BTW, I looked at the code again and removed an empty line after
assert(row->bodycnt == 1); in xrow_decode_dml() to make corresponding
lines look as in xrow_decode_ballot().

> 
> >  	assert(row->bodycnt == 1);
> >  
> > -	const char *data = (const char *) row->body[0].iov_base;
> > -	const char *end = data + row->body[0].iov_len;
> > +	const char *data = start = (const char *) row->body[0].iov_base;
> > +	end = data + row->body[0].iov_len;
> >  	const char *tmp = data;
> >  	if (mp_check(&tmp, end) != 0 || mp_typeof(*data) != MP_MAP)
> >  		goto err;
> > @@ -1124,8 +1136,7 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
> >  	}
> >  	return 0;
> >  err:
> > -	xrow_on_decode_err(row->body[0].iov_base, end, ER_INVALID_MSGPACK,
> > -			   "packet body");
> > +	xrow_on_decode_err(start, end, ER_INVALID_MSGPACK, "packet body");
> 
> 8. The same as 6.

Answered above.

> 
> >  	return -1;
> >  }
> >  
> > diff --git a/src/lua/init.c b/src/lua/init.c
> > index d18c8af94..f66a3eb46 100644
> > --- a/src/lua/init.c
> > +++ b/src/lua/init.c
> > @@ -319,7 +319,9 @@ tarantool_lua_setpaths(struct lua_State *L)
> >  {
> >  	const char *home = getenv("HOME");
> >  	char cwd[PATH_MAX] = {'\0'};
> > -	getcwd(cwd, sizeof(cwd));
> > +	char *rc = getcwd(cwd, sizeof(cwd));
> > +	if (rc == NULL)
> > +		panic("can't get cwd; errno: %d", errno);
> 
> 9. 'cwd' variable is not used at all and can be removed. It was
> introduced here 9e7c421758af156d19a66cebf713e24b42d8654b but
> was forgotten to remove sometime.
> 
> =====================================================================
>  	const char *home = getenv("HOME");
> -	char cwd[PATH_MAX] = {'\0'};
> -	char *rc = getcwd(cwd, sizeof(cwd));
> -	if (rc == NULL)
> -		panic("can't get cwd; errno: %d", errno);
>  	lua_getglobal(L, "package");
>  	int top = lua_gettop(L);
> =====================================================================

Nice catch, thanks. Removed.

> 
> >  	lua_getglobal(L, "package");
> >  	int top = lua_gettop(L);
> >  
> > -- 
> > 2.21.0
> > 

Diff from my previous commit:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f96709a75..bb7405e68 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -758,15 +758,14 @@ sql_utf8_pattern_compare(const char *pattern,
 			       SQL_END_OF_STRING) {
 				if (c == SQL_INVALID_UTF8_SYMBOL)
 					return INVALID_PATTERN;
-				if (c != MATCH_ALL_WILDCARD &&
-				    c != MATCH_ONE_WILDCARD)
-					break;
 				if (c == MATCH_ONE_WILDCARD) {
 					c2 = Utf8Read(string, string_end);
 					if (c2 == SQL_INVALID_UTF8_SYMBOL)
 						return NO_MATCH;
 					if (c2 == SQL_END_OF_STRING)
 						return NO_WILDCARD_MATCH;
+				} else if (c != MATCH_ALL_WILDCARD) {
+					break;
 				}
 			}
 			/*
diff --git a/src/box/xrow.c b/src/box/xrow.c
index ccdd9def5..22a5be318 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -510,13 +510,11 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
 
 	body.v_data_len = mp_bswap_u32(msg_len);
 
-	int rc1, rc2, rc3;
-	rc1 = write(fd, header, sizeof(header));
-	rc2 = write(fd, &body, sizeof(body));
-	rc3 = write(fd, e->errmsg, msg_len);
-	(void) rc1;
-	(void) rc2;
-	(void) rc3;
+	ssize_t unused;
+	unused = write(fd, header, sizeof(header));
+	unused = write(fd, &body, sizeof(body));
+	unused = write(fd, e->errmsg, msg_len);
+	(void) unused;
 }
 
 int
@@ -1086,7 +1084,6 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 		goto err;
 
 	assert(row->bodycnt == 1);
-
 	const char *data = start = (const char *) row->body[0].iov_base;
 	end = data + row->body[0].iov_len;
 	const char *tmp = data;
diff --git a/src/lua/init.c b/src/lua/init.c
index f66a3eb46..be164bdfb 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -318,10 +318,6 @@ static void
 tarantool_lua_setpaths(struct lua_State *L)
 {
 	const char *home = getenv("HOME");
-	char cwd[PATH_MAX] = {'\0'};
-	char *rc = getcwd(cwd, sizeof(cwd));
-	if (rc == NULL)
-		panic("can't get cwd; errno: %d", errno);
 	lua_getglobal(L, "package");
 	int top = lua_gettop(L);
 

[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Vladislav Shpilevoy]

Hi! Thanks for the fixes! See 5 comments below.

>> See 9 comments below, and my fixes on the branch in a separate commit.
>>
>>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
>>> index 2dfc4fc9d..14177aaab 100644
>>> --- a/src/box/lua/net_box.c
>>> +++ b/src/box/lua/net_box.c
>>> @@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
>>>  	if (map_size == 2) {
>>>  		key = mp_decode_uint(data);
>>>  		assert(key == SQL_INFO_AUTOINCREMENT_IDS);
>>> -		(void)key;
>>> +		(void) key;
>>
>> 1. Unnecessary diff. Please, drop.
> 
> I don't think so. It is okay to fix a code block that you touch in a
> commit to fix it according to a code style.

1. You did not touch this block. This concrete line was not an error nor
warning. In this commit you did not touch 'key' variable at all. So
this change is unrelated. Please, drop. You just took an arbitrary variable
and decided to add a whitespace - this is what is called an unrelated change.

> 
> This would be applicable if I would change some unrelated block of code:

This is what you did - 'key' variable is unrelated to other changes and
to the commit message and title.

> here I'm agree with you.
> 
>>
>>>  		uint64_t count = mp_decode_array(data);
>>>  		assert(count > 0);
>>>  		lua_createtable(L, 0, count);
>>>  		for (uint32_t j = 0; j < count; ++j) {
>>> -			int64_t id;
>>> +			int64_t id = INT64_MIN;
>>
>> 2. Is there any concrete reason why the value is so
>> non-trivial? Why not just simple zero?
> 
> We don't verify a data received from a network in net.box for
> correctness. AFAIU from your words it was the design decision. So I
> consider this broken by design, but try to at least set a value that
> less probably appear in a normal output to signal a user that something
> going wrong (when we get something other then int64 here).
> 
> Of course, we should give an error in the case, but I left this case
> unverified to keep things consistent.

2. Ok, I do not agree with broken by design, but I do not mind against
INT64_MIN.

>>> diff --git a/src/box/xrow.c b/src/box/xrow.c
>>> index aaed84e38..ccdd9def5 100644
>>> --- a/src/box/xrow.c
>>> +++ b/src/box/xrow.c
>>> @@ -509,9 +509,14 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
>>>  			     schema_version, sizeof(body) + msg_len);
>>>  
>>>  	body.v_data_len = mp_bswap_u32(msg_len);
>>> -	(void) write(fd, header, sizeof(header));
>>> -	(void) write(fd, &body, sizeof(body));
>>> -	(void) write(fd, e->errmsg, msg_len);
>>> +
>>> +	int rc1, rc2, rc3;
>>> +	rc1 = write(fd, header, sizeof(header));
>>> +	rc2 = write(fd, &body, sizeof(body));
>>> +	rc3 = write(fd, e->errmsg, msg_len);
>>> +	(void) rc1;
>>> +	(void) rc2;
>>> +	(void) rc3;
>>
>> 5. Firstly, you could use one 'rc' for all of them. It would be shorter.
>> Secondly, why not just remove these (void) casts? We ignore returned value
>> in many places and it is ok, if it isn't really important.
>>
>> In this concrete place an error has already happened, and it makes
>> no sense to create one another - this is why the values are ignored.
>>
>> =====================================================================
>>  	body.v_data_len = mp_bswap_u32(msg_len);
>> -
>> -	int rc1, rc2, rc3;
>> -	rc1 = write(fd, header, sizeof(header));
>> -	rc2 = write(fd, &body, sizeof(body));
>> -	rc3 = write(fd, e->errmsg, msg_len);
>> -	(void) rc1;
>> -	(void) rc2;
>> -	(void) rc3;
>> +	write(fd, header, sizeof(header));
>> +	write(fd, &body, sizeof(body));
>> +	write(fd, e->errmsg, msg_len);
>> =====================================================================
> 
> The write() glibc syscall wrapper is marked with
> __attribute__((__warn_unused_result__)), so we cannot ignore this result
> when -Werror is set. Maybe it is not so for your libc implementation /
> version: in this case you'll not get this warning.
> 
> Re using one rc: I had the thought that clang tracks whether a result is
> used or discarded with a next assingment, so I wrote the code in that
> way. But I don't observe such behaviour with clang-6, 7, 8 and 3.9. It
> seems I did remember it wrongly and a result is considered as 'used'
> when we assign it; a cast to void is just to eliminate the 'variable set
> but not used' warning.
> 
> GCC complains about (void) write(...);, so we need to assign a result
> into a variable: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

3. I've tried on my virtual Ubuntu - yep. You are right. The fix below
is ok then.

> 
> Rewritten as follows:
> 
> ssize_t unused;
> unused = write(fd, header, sizeof(header));
> unused = write(fd, &body, sizeof(body));
> unused = write(fd, e->errmsg, msg_len);
> (void) unused;
> 
>>
>>>  }
>>>  
>>>  int
>>> @@ -624,12 +629,15 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
>>>  	request->header = row;
>>>  	request->type = row->type;
>>>  
>>> +	const char *start = NULL;
>>> +	const char *end = NULL;
>>> +
>>>  	if (row->bodycnt == 0)
>>>  		goto done;
>>>  
>>>  	assert(row->bodycnt == 1);
>>> -	const char *data = (const char *) row->body[0].iov_base;
>>> -	const char *end = data + row->body[0].iov_len;
>>> +	const char *data = start = (const char *) row->body[0].iov_base;
>>> +	end = data + row->body[0].iov_len;
>>>  	assert((end - data) > 0);
>>>  
>>>  	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
>>> @@ -701,8 +709,8 @@ error:
>>>  done:
>>>  	if (key_map) {
>>>  		enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
>>> -		xrow_on_decode_err(row->body[0].iov_base, end,
>>> -				   ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
>>> +		xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
>>> +				   iproto_key_name(key));
>>
>> 6. Why did you add 'start' variable? This code was fully okay, as I
>> understand. Compiler could not raise an error on 'row->body[0].iov_base'
>> usage here. Please, drop this hunk. A patch should not contain unrelated
>> changes making review harder and git history dirtier.
> 
> The compiler warning is not false positive here: the code was not okay.
> I'll cite the commit message:

4. There was no compiler warning at all except 'end' variable used after
'done' label, but declared after 'goto' to it. 'start' variable is not
needed here. I've dropped it and nothing happened (I tried on Linux,
gcc-7, with your new compiler option - no errors).

Please, apply the diff below.

============================================================================
@@ -627,14 +627,13 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
 	request->header = row;
 	request->type = row->type;
 
-	const char *start = NULL;
 	const char *end = NULL;
 
 	if (row->bodycnt == 0)
 		goto done;
 
 	assert(row->bodycnt == 1);
-	const char *data = start = (const char *) row->body[0].iov_base;
+	const char *data = (const char *) row->body[0].iov_base;
 	end = data + row->body[0].iov_len;
 	assert((end - data) > 0);
 
@@ -707,8 +706,8 @@ error:
 done:
 	if (key_map) {
 		enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
-		xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
-				   iproto_key_name(key));
+		xrow_on_decode_err(row->body[0].iov_base, end,
+				   ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
 		return -1;
 	}
 	return 0;
============================================================================

> 
>> * xrow_decode_*() functions could read uninitialized data when printing
>>   a hex code for a row if the received msgpack is empty, but there are
>>   expected keys.
> 
> Please, suggest a change for wording if the current one is not clear.

I thought that it was about 'end' variable. There is nothing wrong
about 'row->body[0].iov_base' usage after 'done' label.

> 
> My change causes `for (const char *cur = start; cur < end;) { ... }` in
> dump_row_hex() to walk around the loop body and so prevents reading from
> `start` in the loop (NULL < NULL is false).

You assigned 'start' from 'row->body[0].iov_base', and the latter is not
changed in the function. So I do not see what was wrong with reading
this 'iov_base' anywhere in the function.

What is more, in the same function you still use 'row->body[0].iov_base'
here
https://github.com/tarantool/tarantool/blob/Totktonada/gh-4178-set-right-flags-in-release-testing-jobs/src/box/xrow.c#L643
and here
https://github.com/tarantool/tarantool/blob/Totktonada/gh-4178-set-right-flags-in-release-testing-jobs/src/box/xrow.c#L703
So 'start' not only is unrelated, but also inconsistent. I dropped this
diff and everything works without any errors nor warnings.

Please, apply it, for ballot() function:

============================================================================
@@ -1077,14 +1076,13 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 	ballot->is_ro = false;
 	vclock_create(&ballot->vclock);
 
-	const char *start = NULL;
 	const char *end = NULL;
 
 	if (row->bodycnt == 0)
 		goto err;
-
 	assert(row->bodycnt == 1);
-	const char *data = start = (const char *) row->body[0].iov_base;
+
+	const char *data = (const char *) row->body[0].iov_base;
 	end = data + row->body[0].iov_len;
 	const char *tmp = data;
 	if (mp_check(&tmp, end) != 0 || mp_typeof(*data) != MP_MAP)
@@ -1133,7 +1131,8 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 	}
 	return 0;
 err:
-	xrow_on_decode_err(start, end, ER_INVALID_MSGPACK, "packet body");
+	xrow_on_decode_err(row->body[0].iov_base, end, ER_INVALID_MSGPACK,
+			   "packet body");
 	return -1;
 }
============================================================================

> 
>>
>>>  		return -1;
>>>  	}
>>>  	return 0;
>>> @@ -1071,12 +1079,16 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
>>>  	ballot->is_ro = false;
>>>  	vclock_create(&ballot->vclock);
>>>  
>>> +	const char *start = NULL;
>>> +	const char *end = NULL;
>>> +
>>>  	if (row->bodycnt == 0)
>>>  		goto err;
>>> +
>>
>> 7. Excessive empty line. Please, drop.
> 
> Don't know which empty line you considered as excessive.

5. We both know, that it is the line I've commented above - right
below 'goto err'.

> If you can link
> a guide clause about that or describe a formal rule you want to make
> mandatory, then put it here.

There is a rule about not making unrelated changes. This is a sense of
atomic commits. A change is unrelated, when it stands away from other
changes and can be dropped without consequences. Here this empty line
1) stands away from other changes, 2) can be dropped. So it is unrelated.

In addition to formal definition of 'commit' we have something even
better - Vladimir D. responses in freelists.

Grep by 'unrelated':
https://www.freelists.org/post/tarantool-patches/PATCH-22-libcorefiber-Allow-to-extend-default-stack-size,1
https://www.freelists.org/post/tarantool-patches/PATCH-v2-34-sql-use-space-by-name-in-SQL,1

> In the latter case it would be good to
> strengthen it with examples from our code. (However I propose to don't
> spend time on that: it is counter-productive.)

It *is* productive. Because it teaches how to make diffs more atomic
and simple. It makes reviewer's life easier and simplifies search for
errors - the smaller is diff, the easier it is to find all errors.

Talking of 'code examples' - it is not about code. It is about
commits and their diff. First version of this commit looked like a
scattered number of '+' and '-' many of which were like:

---
-	(void)key;
+	(void) key;
---
 	if (row->bodycnt == 0)
 		goto err;
+
 	assert(row->bodycnt == 1);
---

And some of the others were just unnecessary.

Obviously, it does not make review process easier. I needed to somehow
determine which of these changes were about the bug, which of them were
just about your personal taste of adding empty lines in the code you've
seen alongside. It is not ok. If you want to fix code style, then send
a separate commit. This commit is declared as
'set right flags in release testing jobs' - how do these new empty lines
and new whitespaces in the random code relate to the commit title and to
the given changes?

Together with the changes proposed by me in this mail, we've dropped
from your original commit already 20 lines of pure diff, both of '+'
and '-'.

[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Vladislav Shpilevoy]

>>
>>> * xrow_decode_*() functions could read uninitialized data when printing
>>>   a hex code for a row if the received msgpack is empty, but there are
>>>   expected keys.
>>
>> Please, suggest a change for wording if the current one is not clear.

Ahh, now I see. It is possible that row->bodycnt == 0 and we jump to
'done'. It tries to write an error via xrow_on_decode_err, which reads
row->body[0]. Probably then I can suggest adding an explicit mention of
xrow_on_decode_err() function and an attempt to access an array by index
out of range.

I did not understand your explanation below about cycles and
dump_row_hex(), because I did not know, that they are not used in
xrow_decode_dml() directly, but via xrow_on_decode_err().

>> My change causes `for (const char *cur = start; cur < end;) { ... }` in
>> dump_row_hex() to walk around the loop body and so prevents reading from
>> `start` in the loop (NULL < NULL is false).

Then you can ignore my comments about xrow_decode_dml and xrow_decode_ballot.
Others are still valid.

By the way,

                  \\\ ,
                    \ `|
                     ) (   .-""-.
                     | |  /_  {  '.
                     | | (/ `\   } )
                     | |  ^/ ^`}   {
                     \  \ \=  ( {   )
                      \  \ '-, {   {{
                       \  \_.'  ) }  )
                        \.-'   (     (
                        /'-.'_. ) (  }
                        \_(    {   _/\
                         ) '--' `-;\  \
                     _.-'       /  / /
              <\/>_.'         .'  / /
          <\/></\>/.  '      /<\// /
          </\>  _ |\`- _ . -/|<// (
       <\/>    - _- `  _.-'`_/- |  \
       </\>        -  - -  -     \\\
        }`<\/>                <\/>`{
        { </\>-<\/>_<\/>_<\/>-</\> }
        }      </\> </\> </\>      {
     <\/>.                         <\/>
     </\>                          </\>
      {`<\/>                     <\/>`}
      } </\>-<\/>_<\/>_<\/>_<\/>-</\> {
      {      </\> </\> </\> </\>      }
      }                               }
      {           H A P P Y           {
   <\/>        B I R T H D A Y        <\/>
   </\>                               </\>
     `<\/>                          <\/>'
      </\>-<\/>_<\/>_<\/>_<\/>_<\/>-</\>
           </\> </\> </\> </\> </\>

[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Alexander Turenko]

> >>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> >>> index 2dfc4fc9d..14177aaab 100644
> >>> --- a/src/box/lua/net_box.c
> >>> +++ b/src/box/lua/net_box.c
> >>> @@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
> >>>  	if (map_size == 2) {
> >>>  		key = mp_decode_uint(data);
> >>>  		assert(key == SQL_INFO_AUTOINCREMENT_IDS);
> >>> -		(void)key;
> >>> +		(void) key;
> >>
> >> 1. Unnecessary diff. Please, drop.
> > 
> > I don't think so. It is okay to fix a code block that you touch in a
> > commit to fix it according to a code style.
> 
> 1. You did not touch this block. This concrete line was not an error nor
> warning. In this commit you did not touch 'key' variable at all. So
> this change is unrelated. Please, drop. You just took an arbitrary variable
> and decided to add a whitespace - this is what is called an unrelated change.
> 
> > 
> > This would be applicable if I would change some unrelated block of code:
> 
> This is what you did - 'key' variable is unrelated to other changes and
> to the commit message and title.
> 
> > here I'm agree with you.
> > 

We discussed it voicely. In short: my position was that when you touch a
code block (or even a function) it is okay to fix the code style within.
Vlad adds another criteria here: it is okay if changes are big enough.

This is more or less format rule I can agree with. So I dropped this
change.

> >>>  		return -1;
> >>>  	}
> >>>  	return 0;
> >>> @@ -1071,12 +1079,16 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
> >>>  	ballot->is_ro = false;
> >>>  	vclock_create(&ballot->vclock);
> >>>  
> >>> +	const char *start = NULL;
> >>> +	const char *end = NULL;
> >>> +
> >>>  	if (row->bodycnt == 0)
> >>>  		goto err;
> >>> +
> >>
> >> 7. Excessive empty line. Please, drop.
> > 
> > Don't know which empty line you considered as excessive.
> 
> 5. We both know, that it is the line I've commented above - right
> below 'goto err'.
> 
> > If you can link
> > a guide clause about that or describe a formal rule you want to make
> > mandatory, then put it here.
> 
> There is a rule about not making unrelated changes. This is a sense of
> atomic commits. A change is unrelated, when it stands away from other
> changes and can be dropped without consequences. Here this empty line
> 1) stands away from other changes, 2) can be dropped. So it is unrelated.
> 
> In addition to formal definition of 'commit' we have something even
> better - Vladimir D. responses in freelists.
> 
> Grep by 'unrelated':
> https://www.freelists.org/post/tarantool-patches/PATCH-22-libcorefiber-Allow-to-extend-default-stack-size,1
> https://www.freelists.org/post/tarantool-patches/PATCH-v2-34-sql-use-space-by-name-in-SQL,1
> 
> > In the latter case it would be good to
> > strengthen it with examples from our code. (However I propose to don't
> > spend time on that: it is counter-productive.)
> 
> It *is* productive. Because it teaches how to make diffs more atomic
> and simple. It makes reviewer's life easier and simplifies search for
> errors - the smaller is diff, the easier it is to find all errors.
> 
> Talking of 'code examples' - it is not about code. It is about
> commits and their diff. First version of this commit looked like a
> scattered number of '+' and '-' many of which were like:
> 
> ---
> -	(void)key;
> +	(void) key;
> ---
>  	if (row->bodycnt == 0)
>  		goto err;
> +
>  	assert(row->bodycnt == 1);
> ---
> 
> And some of the others were just unnecessary.
> 
> Obviously, it does not make review process easier. I needed to somehow
> determine which of these changes were about the bug, which of them were
> just about your personal taste of adding empty lines in the code you've
> seen alongside. It is not ok. If you want to fix code style, then send
> a separate commit. This commit is declared as
> 'set right flags in release testing jobs' - how do these new empty lines
> and new whitespaces in the random code relate to the commit title and to
> the given changes?
> 
> Together with the changes proposed by me in this mail, we've dropped
> from your original commit already 20 lines of pure diff, both of '+'
> and '-'.

I think it is okay to push some amount of code style changes together
with significant changes. The question is about a balance between
readability for reviewers and readers and converging a code quality to
an ideal across a time. Both ends (lack of a balance) is not good.

In such questions I usually lean on intuitive understanding of
maintainers and valuable contributors where the balance should be.

In this case I made xrow_decode_dml() and xrow_decode_ballot() a bit
more similar in the sense how blocks are separated. This is mostly a
thing of preference, so I don't mind to remove this empty line.

So I changed it as you suggested.

[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Alexander Turenko]

On Sun, Apr 28, 2019 at 12:11:28PM +0300, Vladislav Shpilevoy wrote:
> 
> >>
> >>> * xrow_decode_*() functions could read uninitialized data when printing
> >>>   a hex code for a row if the received msgpack is empty, but there are
> >>>   expected keys.
> >>
> >> Please, suggest a change for wording if the current one is not clear.
> 
> Ahh, now I see. It is possible that row->bodycnt == 0 and we jump to
> 'done'. It tries to write an error via xrow_on_decode_err, which reads
> row->body[0]. Probably then I can suggest adding an explicit mention of
> xrow_on_decode_err() function and an attempt to access an array by index
> out of range.
> 
> I did not understand your explanation below about cycles and
> dump_row_hex(), because I did not know, that they are not used in
> xrow_decode_dml() directly, but via xrow_on_decode_err().
> 
> >> My change causes `for (const char *cur = start; cur < end;) { ... }` in
> >> dump_row_hex() to walk around the loop body and so prevents reading from
> >> `start` in the loop (NULL < NULL is false).
> 
> Then you can ignore my comments about xrow_decode_dml and xrow_decode_ballot.
> Others are still valid.

I have updated the comment about the uninitialized data read, added the
comment about getcwd() and the comment about Vinyl:

> Fixed all -Wmaybe-uninitialized and -Wunused-result warnings. A few 
> notes about the fixes:
>
> * net.box does not validate received data in general, so I don't add a
>   check for autoincrement IDs too. Set the ID to INT64_MIN, because this
>   value is less probably will appear here in a normal case and so is the
>   best one to signal a user that something probably going wrongly.
> * xrow_decode_*() functions could read uninitialized data from
>   row->body[0].iov_base in xrow_on_decode_err() when printing a hex code
>   for a row. It could be possible when the received msgpack was empty
>   (row->bodycnt == 0), but there were expected keys (key_map != 0).
> * getcwd() is marked with __attribute__((__warn_unused_result__)) in
>   glibc, but the buffer filled by this call is not used anywhere and so
>   just removed.
> * Vinyl -Wmaybe-uninitialized warnings are false positive ones.

If the patch is okay for you, I'll resend it to Vladimir D. as v2.

> By the way,
> 
>                   \\\ ,
>                     \ `|
>                      ) (   .-""-.
>                      | |  /_  {  '.
>                      | | (/ `\   } )
>                      | |  ^/ ^`}   {
>                      \  \ \=  ( {   )
>                       \  \ '-, {   {{
>                        \  \_.'  ) }  )
>                         \.-'   (     (
>                         /'-.'_. ) (  }
>                         \_(    {   _/\
>                          ) '--' `-;\  \
>                      _.-'       /  / /
>               <\/>_.'         .'  / /
>           <\/></\>/.  '      /<\// /
>           </\>  _ |\`- _ . -/|<// (
>        <\/>    - _- `  _.-'`_/- |  \
>        </\>        -  - -  -     \\\
>         }`<\/>                <\/>`{
>         { </\>-<\/>_<\/>_<\/>-</\> }
>         }      </\> </\> </\>      {
>      <\/>.                         <\/>
>      </\>                          </\>
>       {`<\/>                     <\/>`}
>       } </\>-<\/>_<\/>_<\/>_<\/>-</\> {
>       {      </\> </\> </\> </\>      }
>       }                               }
>       {           H A P P Y           {
>    <\/>        B I R T H D A Y        <\/>
>    </\>                               </\>
>      `<\/>                          <\/>'
>       </\>-<\/>_<\/>_<\/>_<\/>_<\/>-</\>
>            </\> </\> </\> </\> </\>

Thanks!

Re: [tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Vladislav Shpilevoy]

LGTM. Vova, please, do a second review.

On 29/04/2019 00:35, Alexander Turenko wrote:
>>>>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
>>>>> index 2dfc4fc9d..14177aaab 100644
>>>>> --- a/src/box/lua/net_box.c
>>>>> +++ b/src/box/lua/net_box.c
>>>>> @@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
>>>>>  	if (map_size == 2) {
>>>>>  		key = mp_decode_uint(data);
>>>>>  		assert(key == SQL_INFO_AUTOINCREMENT_IDS);
>>>>> -		(void)key;
>>>>> +		(void) key;
>>>>
>>>> 1. Unnecessary diff. Please, drop.
>>>
>>> I don't think so. It is okay to fix a code block that you touch in a
>>> commit to fix it according to a code style.
>>
>> 1. You did not touch this block. This concrete line was not an error nor
>> warning. In this commit you did not touch 'key' variable at all. So
>> this change is unrelated. Please, drop. You just took an arbitrary variable
>> and decided to add a whitespace - this is what is called an unrelated change.
>>
>>>
>>> This would be applicable if I would change some unrelated block of code:
>>
>> This is what you did - 'key' variable is unrelated to other changes and
>> to the commit message and title.
>>
>>> here I'm agree with you.
>>>
> 
> We discussed it voicely. In short: my position was that when you touch a
> code block (or even a function) it is okay to fix the code style within.
> Vlad adds another criteria here: it is okay if changes are big enough.
> 
> This is more or less format rule I can agree with. So I dropped this
> change.
> 
>>>>>  		return -1;
>>>>>  	}
>>>>>  	return 0;
>>>>> @@ -1071,12 +1079,16 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
>>>>>  	ballot->is_ro = false;
>>>>>  	vclock_create(&ballot->vclock);
>>>>>  
>>>>> +	const char *start = NULL;
>>>>> +	const char *end = NULL;
>>>>> +
>>>>>  	if (row->bodycnt == 0)
>>>>>  		goto err;
>>>>> +
>>>>
>>>> 7. Excessive empty line. Please, drop.
>>>
>>> Don't know which empty line you considered as excessive.
>>
>> 5. We both know, that it is the line I've commented above - right
>> below 'goto err'.
>>
>>> If you can link
>>> a guide clause about that or describe a formal rule you want to make
>>> mandatory, then put it here.
>>
>> There is a rule about not making unrelated changes. This is a sense of
>> atomic commits. A change is unrelated, when it stands away from other
>> changes and can be dropped without consequences. Here this empty line
>> 1) stands away from other changes, 2) can be dropped. So it is unrelated.
>>
>> In addition to formal definition of 'commit' we have something even
>> better - Vladimir D. responses in freelists.
>>
>> Grep by 'unrelated':
>> https://www.freelists.org/post/tarantool-patches/PATCH-22-libcorefiber-Allow-to-extend-default-stack-size,1
>> https://www.freelists.org/post/tarantool-patches/PATCH-v2-34-sql-use-space-by-name-in-SQL,1
>>
>>> In the latter case it would be good to
>>> strengthen it with examples from our code. (However I propose to don't
>>> spend time on that: it is counter-productive.)
>>
>> It *is* productive. Because it teaches how to make diffs more atomic
>> and simple. It makes reviewer's life easier and simplifies search for
>> errors - the smaller is diff, the easier it is to find all errors.
>>
>> Talking of 'code examples' - it is not about code. It is about
>> commits and their diff. First version of this commit looked like a
>> scattered number of '+' and '-' many of which were like:
>>
>> ---
>> -	(void)key;
>> +	(void) key;
>> ---
>>  	if (row->bodycnt == 0)
>>  		goto err;
>> +
>>  	assert(row->bodycnt == 1);
>> ---
>>
>> And some of the others were just unnecessary.
>>
>> Obviously, it does not make review process easier. I needed to somehow
>> determine which of these changes were about the bug, which of them were
>> just about your personal taste of adding empty lines in the code you've
>> seen alongside. It is not ok. If you want to fix code style, then send
>> a separate commit. This commit is declared as
>> 'set right flags in release testing jobs' - how do these new empty lines
>> and new whitespaces in the random code relate to the commit title and to
>> the given changes?
>>
>> Together with the changes proposed by me in this mail, we've dropped
>> from your original commit already 20 lines of pure diff, both of '+'
>> and '-'.
> 
> I think it is okay to push some amount of code style changes together
> with significant changes. The question is about a balance between
> readability for reviewers and readers and converging a code quality to
> an ideal across a time. Both ends (lack of a balance) is not good.
> 
> In such questions I usually lean on intuitive understanding of
> maintainers and valuable contributors where the balance should be.
> 
> In this case I made xrow_decode_dml() and xrow_decode_ballot() a bit
> more similar in the sense how blocks are separated. This is mostly a
> thing of preference, so I don't mind to remove this empty line.
> 
> So I changed it as you suggested.
> 

[PATCH v2] travis-ci: set right flags in release testing jobs

[Alexander Turenko]

It is important to have testing jobs that build the project with both
-Werror and -O2 to keep the code clean. -O2 is needed, because some
compiler warnings are available only after extra analyzing passes that
are disabled with lesser optimization levels.

The first attempt to add -Werror for release testing jobs was made in
da505ee72883195121b7006bbd52bc66e57321f3 ('Add -Werror for CI (1.10
part)'), but it mistakely doesn't enable -O2 for RelWithDebInfoWError
build. It is possible to fix it in this way:

 | --- a/cmake/compiler.cmake
 | +++ b/cmake/compiler.cmake
 | @@ -113,10 +113,14 @@ set (CMAKE_C_FLAGS_DEBUG
 |      "${CMAKE_C_FLAGS_DEBUG} ${CC_DEBUG_OPT} -O0")
 |  set (CMAKE_C_FLAGS_RELWITHDEBINFO
 |      "${CMAKE_C_FLAGS_RELWITHDEBINFO} ${CC_DEBUG_OPT} -O2")
 | +set (CMAKE_C_FLAGS_RELWITHDEBINFOWERROR
 | +    "${CMAKE_C_FLAGS_RELWITHDEBINFOWERROR} ${CC_DEBUG_OPT} -O2")
 |  set (CMAKE_CXX_FLAGS_DEBUG
 |      "${CMAKE_CXX_FLAGS_DEBUG} ${CC_DEBUG_OPT} -O0")
 |  set (CMAKE_CXX_FLAGS_RELWITHDEBINFO
 |      "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} ${CC_DEBUG_OPT} -O2")
 | +set (CMAKE_CXX_FLAGS_RELWITHDEBINFOWERROR
 | +    "${CMAKE_CXX_FLAGS_RELWITHDEBINFOWERROR} ${CC_DEBUG_OPT} -O2")
 |
 |  unset(CC_DEBUG_OPT)

However I think that a build type (and so `tarantool --version`) should
not show whether -Werror was passed or not. So I have added
ENABLE_WERROR CMake option for that. It can be set like so:

 | cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON

Enabled the option in testing Travis-CI jobs with the RelWithDebInfo
build type. Deploy jobs don't include it as before.

Fixed all -Wmaybe-uninitialized and -Wunused-result warnings. A few
notes about the fixes:

* net.box does not validate received data in general, so I don't add a
  check for autoincrement IDs too. Set the ID to INT64_MIN, because this
  value is less probably will appear here in a normal case and so is the
  best one to signal a user that something probably going wrongly.
* xrow_decode_*() functions could read uninitialized data from
  row->body[0].iov_base in xrow_on_decode_err() when printing a hex code
  for a row. It could be possible when the received msgpack was empty
  (row->bodycnt == 0), but there were expected keys (key_map != 0).
* getcwd() is marked with __attribute__((__warn_unused_result__)) in
  glibc, but the buffer filled by this call is not used anywhere and so
  just removed.
* Vinyl -Wmaybe-uninitialized warnings are false positive ones.

Added comments and quotes into .travis.yml to ease reading. Removed
"test" word from the CentOS 6 job name, because we don't run tests on
this distro (disabled in the RPM spec).

Fixes #4178.
---

This patch targets all currently supported long-term branches.

https://github.com/tarantool/tarantool/issues/4178
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4178-set-right-flags-in-release-testing-jobs

 .travis.mk            |  4 ++--
 .travis.yml           | 45 +++++++++++++++++++++++--------------------
 cmake/compiler.cmake  | 16 ++++++++++-----
 src/box/lua/net_box.c |  2 +-
 src/box/sql/func.c    | 16 +++++++--------
 src/box/vy_cache.c    |  2 +-
 src/box/vy_mem.c      |  2 +-
 src/box/xrow.c        | 30 ++++++++++++++++++-----------
 src/lua/init.c        |  2 --
 9 files changed, 67 insertions(+), 52 deletions(-)

diff --git a/.travis.mk b/.travis.mk
index 247bb2a2d..973c6a7c7 100644
--- a/.travis.mk
+++ b/.travis.mk
@@ -40,7 +40,7 @@ deps_ubuntu:
 		lcov ruby
 
 test_ubuntu: deps_ubuntu
-	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS}
+	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
 	make -j8
 	cd test && /usr/bin/python test-run.py -j 1
 
@@ -51,7 +51,7 @@ deps_osx:
 	pip install -r test-run/requirements.txt
 
 test_osx: deps_osx
-	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS}
+	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
 	# Increase the maximum number of open file descriptors on macOS
 	sudo sysctl -w kern.maxfiles=20480 || :
 	sudo sysctl -w kern.maxfilesperproc=20480 || :
diff --git a/.travis.yml b/.travis.yml
index e30540837..6e79cf2fe 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -19,69 +19,72 @@ git:
 
 jobs:
     include:
-      - name: RelWithDebInfoWError build + test (Linux, gcc)
+      # Testing targets (just run tests on Debian Stretch or OS X).
+      - name: "RelWithDebInfo build + test (Linux, gcc)"
         env: TARGET=test
-      - name: RelWithDebInfoWError build + test (Linux, clang)
+      - name: "RelWithDebInfo build + test (Linux, clang)"
         env: TARGET=test
         compiler: clang
-      - name: RelWithDebInfoWError build + test (OS X Mojave 10.14)
+      - name: "RelWithDebInfo build + test (OS X Mojave 10.14)"
         env: TARGET=test
         os: osx
-      - name: Debug build + test + coverage (Linux, gcc)
+      - name: "Debug build + test + coverage (Linux, gcc)"
         env: TARGET=coverage
-      - name: RelWithDebInfoWError build + test (OS X High Sierra 10.13)
+      - name: "RelWithDebInfo build + test (OS X High Sierra 10.13)"
         env: TARGET=test
         os: osx
         osx_image: xcode9.4
         if: branch = "master"
-      - name: LTO build + test (Linux, gcc)
+      # Special targets (only LTO for now).
+      - name: "LTO build + test (Linux, gcc)"
         env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
         if: branch = "master"
-      - name: LTO build + test (Linux, clang)
+      - name: "LTO build + test (Linux, clang)"
         env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
         if: branch = "master"
         compiler: clang
-      - name: LTO build + test (OS X Mojave 10.14)
+      - name: "LTO build + test (OS X Mojave 10.14)"
         os: osx
         env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
         if: branch = "master"
-      - name: Create and deploy tarball
+      # Deploy targets (they also catch distro-specific problems).
+      - name: "Create and deploy tarball"
         env: TARGET=source
         if: branch = "master"
-      - name: CentOS 6 build + test + deploy RPM
+      - name: "CentOS 6 build + deploy RPM"
         env: OS=el DIST=6
         if: branch = "master"
-      - name: CentOS 7 build + test + deploy RPM
+      - name: "CentOS 7 build + test + deploy RPM"
         env: OS=el DIST=7
         if: branch = "master"
-      - name: Fedora 28 build + test + deploy RPM
+      - name: "Fedora 28 build + test + deploy RPM"
         env: OS=fedora DIST=28
         if: branch = "master"
-      - name: Fedora 29 build + test + deploy RPM
+      - name: "Fedora 29 build + test + deploy RPM"
         env: OS=fedora DIST=29
         if: branch = "master"
-      - name: Ubuntu Trusty (14.04) build + deploy DEB
+      - name: "Ubuntu Trusty (14.04) build + deploy DEB"
         env: OS=ubuntu DIST=trusty
         if: branch = "master"
-      - name: Ubuntu Xenial (16.04) build + deploy DEB
+      - name: "Ubuntu Xenial (16.04) build + deploy DEB"
         env: OS=ubuntu DIST=xenial
         if: branch = "master"
-      - name: Ubuntu Bionic (18.04) build + deploy DEB
+      - name: "Ubuntu Bionic (18.04) build + deploy DEB"
         env: OS=ubuntu DIST=bionic
         if: branch = "master"
-      - name: Ubuntu Cosmic (18.10) build + deploy DEB
+      - name: "Ubuntu Cosmic (18.10) build + deploy DEB"
         env: OS=ubuntu DIST=cosmic
         if: branch = "master"
-      - name: Ubuntu Disco (19.04) build + deploy DEB
+      - name: "Ubuntu Disco (19.04) build + deploy DEB"
         env: OS=ubuntu DIST=disco
         if: branch = "master"
-      - name: Debian Jessie (8) build + deploy DEB
+      - name: "Debian Jessie (8) build + deploy DEB"
         env: OS=debian DIST=jessie
         if: branch = "master"
-      - name: Debian Stretch (9) build + deploy DEB
+      - name: "Debian Stretch (9) build + deploy DEB"
         env: OS=debian DIST=stretch
         if: branch = "master"
-      - name: Debian Buster (10) build + deploy DEB
+      - name: "Debian Buster (10) build + deploy DEB"
         env: OS=debian DIST=buster
         if: branch = "master"
 
diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
index 4062d13ec..887485c80 100644
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@@ -223,6 +223,8 @@ if (NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
     add_definitions("-DNDEBUG" "-DNVALGRIND")
 endif()
 
+option(ENABLE_WERROR "Make all compiler warnings into errors" OFF)
+
 macro(enable_tnt_compile_flags)
     # Tarantool code is written in GNU C dialect.
     # Additionally, compile it with more strict flags than the rest
@@ -286,11 +288,15 @@ macro(enable_tnt_compile_flags)
     add_definitions("-D__STDC_LIMIT_MACROS=1")
     add_definitions("-D__STDC_CONSTANT_MACROS=1")
 
-    # Only add -Werror if it's a Debug or RelWithDebInfoWError build, done by
-    # developers. Release builds should not cause extra trouble.
-    if (((${CMAKE_BUILD_TYPE} STREQUAL "Debug") OR
-        (${CMAKE_BUILD_TYPE} STREQUAL "RelWithDebInfoWError")) AND
-        HAVE_STD_C11 AND HAVE_STD_CXX11)
+    # Only add -Werror if it's a debug build, done by developers.
+    # Release builds should not cause extra trouble.
+    if ((${CMAKE_BUILD_TYPE} STREQUAL "Debug")
+        AND HAVE_STD_C11 AND HAVE_STD_CXX11)
+        add_compile_flags("C;CXX" "-Werror")
+    endif()
+
+    # Add -Werror if it is requested explicitly.
+    if (ENABLE_WERROR)
         add_compile_flags("C;CXX" "-Werror")
     endif()
 endmacro(enable_tnt_compile_flags)
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 2dfc4fc9d..7484a86bf 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -694,7 +694,7 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
 		assert(count > 0);
 		lua_createtable(L, 0, count);
 		for (uint32_t j = 0; j < count; ++j) {
-			int64_t id;
+			int64_t id = INT64_MIN;
 			mp_read_int64(data, &id);
 			luaL_pushint64(L, id);
 			lua_rawseti(L, -2, j + 1);
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f3790ba01..bb7405e68 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -758,15 +758,15 @@ sql_utf8_pattern_compare(const char *pattern,
 			       SQL_END_OF_STRING) {
 				if (c == SQL_INVALID_UTF8_SYMBOL)
 					return INVALID_PATTERN;
-				if (c != MATCH_ALL_WILDCARD &&
-				    c != MATCH_ONE_WILDCARD)
+				if (c == MATCH_ONE_WILDCARD) {
+					c2 = Utf8Read(string, string_end);
+					if (c2 == SQL_INVALID_UTF8_SYMBOL)
+						return NO_MATCH;
+					if (c2 == SQL_END_OF_STRING)
+						return NO_WILDCARD_MATCH;
+				} else if (c != MATCH_ALL_WILDCARD) {
 					break;
-				if (c == MATCH_ONE_WILDCARD &&
-				    (c2 = Utf8Read(string, string_end)) ==
-				    SQL_END_OF_STRING)
-					return NO_WILDCARD_MATCH;
-				if (c2 == SQL_INVALID_UTF8_SYMBOL)
-					return NO_MATCH;
+				}
 			}
 			/*
 			 * "%" at the end of the pattern matches.
diff --git a/src/box/vy_cache.c b/src/box/vy_cache.c
index be11b986f..7007d0e35 100644
--- a/src/box/vy_cache.c
+++ b/src/box/vy_cache.c
@@ -661,7 +661,7 @@ vy_cache_iterator_seek(struct vy_cache_iterator *itr, struct vy_entry last)
 				ITER_GT : ITER_LT;
 	}
 
-	bool exact;
+	bool exact = false;
 	if (!vy_stmt_is_empty_key(key.stmt)) {
 		itr->curr_pos = iterator_type == ITER_EQ ||
 				iterator_type == ITER_GE ||
diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
index 9513d5b80..a4fae26e2 100644
--- a/src/box/vy_mem.c
+++ b/src/box/vy_mem.c
@@ -392,7 +392,7 @@ vy_mem_iterator_seek(struct vy_mem_iterator *itr, struct vy_entry last)
 				ITER_GT : ITER_LT;
 	}
 
-	bool exact;
+	bool exact = false;
 	struct vy_mem_tree_key tree_key;
 	tree_key.entry = key;
 	/* (lsn == INT64_MAX - 1) means that lsn is ignored in comparison */
diff --git a/src/box/xrow.c b/src/box/xrow.c
index aaed84e38..a3c9b3218 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -509,9 +509,12 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
 			     schema_version, sizeof(body) + msg_len);
 
 	body.v_data_len = mp_bswap_u32(msg_len);
-	(void) write(fd, header, sizeof(header));
-	(void) write(fd, &body, sizeof(body));
-	(void) write(fd, e->errmsg, msg_len);
+
+	ssize_t unused;
+	unused = write(fd, header, sizeof(header));
+	unused = write(fd, &body, sizeof(body));
+	unused = write(fd, e->errmsg, msg_len);
+	(void) unused;
 }
 
 int
@@ -624,12 +627,15 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
 	request->header = row;
 	request->type = row->type;
 
+	const char *start = NULL;
+	const char *end = NULL;
+
 	if (row->bodycnt == 0)
 		goto done;
 
 	assert(row->bodycnt == 1);
-	const char *data = (const char *) row->body[0].iov_base;
-	const char *end = data + row->body[0].iov_len;
+	const char *data = start = (const char *) row->body[0].iov_base;
+	end = data + row->body[0].iov_len;
 	assert((end - data) > 0);
 
 	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
@@ -701,8 +707,8 @@ error:
 done:
 	if (key_map) {
 		enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
-		xrow_on_decode_err(row->body[0].iov_base, end,
-				   ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
+		xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
+				   iproto_key_name(key));
 		return -1;
 	}
 	return 0;
@@ -1071,12 +1077,15 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 	ballot->is_ro = false;
 	vclock_create(&ballot->vclock);
 
+	const char *start = NULL;
+	const char *end = NULL;
+
 	if (row->bodycnt == 0)
 		goto err;
 	assert(row->bodycnt == 1);
 
-	const char *data = (const char *) row->body[0].iov_base;
-	const char *end = data + row->body[0].iov_len;
+	const char *data = start = (const char *) row->body[0].iov_base;
+	end = data + row->body[0].iov_len;
 	const char *tmp = data;
 	if (mp_check(&tmp, end) != 0 || mp_typeof(*data) != MP_MAP)
 		goto err;
@@ -1124,8 +1133,7 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 	}
 	return 0;
 err:
-	xrow_on_decode_err(row->body[0].iov_base, end, ER_INVALID_MSGPACK,
-			   "packet body");
+	xrow_on_decode_err(start, end, ER_INVALID_MSGPACK, "packet body");
 	return -1;
 }
 
diff --git a/src/lua/init.c b/src/lua/init.c
index d18c8af94..be164bdfb 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -318,8 +318,6 @@ static void
 tarantool_lua_setpaths(struct lua_State *L)
 {
 	const char *home = getenv("HOME");
-	char cwd[PATH_MAX] = {'\0'};
-	getcwd(cwd, sizeof(cwd));
 	lua_getglobal(L, "package");
 	int top = lua_gettop(L);
 
-- 
2.21.0

Re: [PATCH v2] travis-ci: set right flags in release testing jobs

[Vladimir Davydov]

Pushed to master, thanks!