[Tarantool-patches] [PATCH v2] box: frommap() bug fix

[Tarantool-patches] [PATCH v2] box: frommap() bug fix

[Chris Sosnin]

Thank you for the review!
Fixed version below.
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4262-frommap-error-handle
issue: https://github.com/tarantool/tarantool/issues/4262

- If an optional argument is provided for
  space_object:frommap() (which is {table = true|false}),
  type match for first arguments is omitted, which is
  incorrect. We should return the result only after making
  sure it is possible to build a tuple.

- If there is a type mismatch, however, frommap() does not
  return nil, err as it is mentioned in the description, so we
  change it to be this way.

Closes #4262
---
 src/box/lua/space.cc    | 13 +++++++++----
 test/box/tuple.result   |  8 +++++++-
 test/box/tuple.test.lua |  1 +
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index f6e96f0c0..e772a924d 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -580,14 +580,19 @@ lbox_space_frommap(struct lua_State *L)
 		}
 		lua_rawseti(L, -3, fieldno+1);
 	}
-	if (table)
-		return 1;

 	lua_replace(L, 1);
 	lua_settop(L, 1);
 	tuple = luaT_tuple_new(L, -1, space->format);
-	if (tuple == NULL)
-		return luaT_error(L);
+	if (tuple == NULL) {
+		struct error *e = diag_last_error(&fiber()->diag);
+		lua_pushnil(L);
+		lua_pushstring(L, e->errmsg);
+		return 2;
+	}
+	if (table) {
+		return 1;
+	}
 	luaT_pushtuple(L, tuple);
 	return 1;
 usage_error:
diff --git a/test/box/tuple.result b/test/box/tuple.result
index 9140211b7..78f919deb 100644
--- a/test/box/tuple.result
+++ b/test/box/tuple.result
@@ -1121,7 +1121,13 @@ s:frommap()
 ...
 s:frommap({})
 ---
-- error: Tuple field 1 required by space format is missing
+- null
+- Tuple field 1 required by space format is missing
+...
+s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
+---
+- null
+- 'Tuple field 4 type does not match one required by operation: expected unsigned'
 ...
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
 ---
diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
index 3ac2902fe..baf2f22d5 100644
--- a/test/box/tuple.test.lua
+++ b/test/box/tuple.test.lua
@@ -373,6 +373,7 @@ s:frommap({ddd = 1, aaa = 2, bbb = 3})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
 s:frommap()
 s:frommap({})
+s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
--
2.21.0 (Apple Git-122.2)

[Tarantool-patches] [PATCH v2] build: GCC warning on strncpy

[Chris Sosnin]

Thank you for the review!
See second version below.
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4515-build-warning
issue: https://github.com/tarantool/tarantool/issues/4515

As long as we are sure, that strlen(sd_unix_path) < sizeof(sa.sun_path)
we can assume that there is always enough space and the path will be
null-terminated. Thus, copy 1 byte less to get rid of the warning.

Closes #4515
---
 src/systemd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/systemd.c b/src/systemd.c
index 6686c3ce0..c80259f06 100644
--- a/src/systemd.c
+++ b/src/systemd.c
@@ -67,7 +67,7 @@ int systemd_init() {
 		.sun_path = { '\0' }
 	};
 	if (strlen(sd_unix_path) >= sizeof(sa.sun_path)) {
-		say_error("systemd: NOTIFY_SOCKET is longer that MAX_UNIX_PATH");
+		say_error("systemd: NOTIFY_SOCKET is longer than MAX_UNIX_PATH");
 		goto error;
 	}
 	if ((systemd_fd = socket(AF_UNIX, SOCK_DGRAM, 0)) == -1) {
@@ -117,7 +117,7 @@ int systemd_notify(const char *message) {
 		.sun_family = AF_UNIX,
 	};

-	strncpy(sa.sun_path, sd_unix_path, sizeof(sa.sun_path));
+	strncpy(sa.sun_path, sd_unix_path, sizeof(sa.sun_path) - 1);
 	if (sa.sun_path[0] == '@')
 		sa.sun_path[0] = '\0';

--
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH v2] build: GCC warning on strncpy

[Vladislav Shpilevoy]

Thanks for the patch!

I see, that you've sent this patch in the same email
thread as another totally unrelated patch about 4262
ticket. Please, don't try to batch not related emails.

If a patch consists of one commit, it should be sent
as one thread with a single email in it.

This patch LGTM.

On 27/11/2019 22:30, Chris Sosnin wrote:
> Thank you for the review!
> See second version below.
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4515-build-warning
> issue: https://github.com/tarantool/tarantool/issues/4515
> 
> As long as we are sure, that strlen(sd_unix_path) < sizeof(sa.sun_path)
> we can assume that there is always enough space and the path will be
> null-terminated. Thus, copy 1 byte less to get rid of the warning.
> 
> Closes #4515
> ---
>  src/systemd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/systemd.c b/src/systemd.c
> index 6686c3ce0..c80259f06 100644
> --- a/src/systemd.c
> +++ b/src/systemd.c
> @@ -67,7 +67,7 @@ int systemd_init() {
>  		.sun_path = { '\0' }
>  	};
>  	if (strlen(sd_unix_path) >= sizeof(sa.sun_path)) {
> -		say_error("systemd: NOTIFY_SOCKET is longer that MAX_UNIX_PATH");
> +		say_error("systemd: NOTIFY_SOCKET is longer than MAX_UNIX_PATH");
>  		goto error;
>  	}
>  	if ((systemd_fd = socket(AF_UNIX, SOCK_DGRAM, 0)) == -1) {
> @@ -117,7 +117,7 @@ int systemd_notify(const char *message) {
>  		.sun_family = AF_UNIX,
>  	};
> 
> -	strncpy(sa.sun_path, sd_unix_path, sizeof(sa.sun_path));
> +	strncpy(sa.sun_path, sd_unix_path, sizeof(sa.sun_path) - 1);
>  	if (sa.sun_path[0] == '@')
>  		sa.sun_path[0] = '\0';
> 
> --
> 2.21.0 (Apple Git-122.2)
> 

Re: [Tarantool-patches] [PATCH v2] build: GCC warning on strncpy

[Kirill Yukhin]

Hello,

On 28 ноя 00:30, Chris Sosnin wrote:
> Thank you for the review!
> See second version below.
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4515-build-warning
> issue: https://github.com/tarantool/tarantool/issues/4515
> 
> As long as we are sure, that strlen(sd_unix_path) < sizeof(sa.sun_path)
> we can assume that there is always enough space and the path will be
> null-terminated. Thus, copy 1 byte less to get rid of the warning.
> 
> Closes #4515

I've checked your patch into 1.10, 2.2 and master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v2] box: frommap() bug fix

[Vladislav Shpilevoy]

Thanks for the patch!

When changes are minor, it is not worth sending a new
version. Just respond to my comments in the same thread,
and append a new patch version to the end of the response.

Also, please, when address my objections, answer to them
inline with a piece of diff for that concrete objection.

So your response email should look like this:

    > Cite of my comment 1

    Your answer 1.
    Your diff to that comment.

    > Cite of my comment 2

    Your answer 2.
    Your diff to that comment.

    Etc.

    New version of the patch.

In case you send a new version of the patch in a new thread,
you should respond to my comments in the old thread.

When you cite my comments, and respond to each of them, you
make the review simpler and faster.

This patch LGTM.

On 27/11/2019 22:30, Chris Sosnin wrote:
> Thank you for the review!
> Fixed version below.
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4262-frommap-error-handle
> issue: https://github.com/tarantool/tarantool/issues/4262
> 
> - If an optional argument is provided for
>   space_object:frommap() (which is {table = true|false}),
>   type match for first arguments is omitted, which is
>   incorrect. We should return the result only after making
>   sure it is possible to build a tuple.
> 
> - If there is a type mismatch, however, frommap() does not
>   return nil, err as it is mentioned in the description, so we
>   change it to be this way.
> 
> Closes #4262
> ---
>  src/box/lua/space.cc    | 13 +++++++++----
>  test/box/tuple.result   |  8 +++++++-
>  test/box/tuple.test.lua |  1 +
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index f6e96f0c0..e772a924d 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -580,14 +580,19 @@ lbox_space_frommap(struct lua_State *L)
>  		}
>  		lua_rawseti(L, -3, fieldno+1);
>  	}
> -	if (table)
> -		return 1;
> 
>  	lua_replace(L, 1);
>  	lua_settop(L, 1);
>  	tuple = luaT_tuple_new(L, -1, space->format);
> -	if (tuple == NULL)
> -		return luaT_error(L);
> +	if (tuple == NULL) {
> +		struct error *e = diag_last_error(&fiber()->diag);
> +		lua_pushnil(L);
> +		lua_pushstring(L, e->errmsg);
> +		return 2;
> +	}
> +	if (table) {
> +		return 1;
> +	}
>  	luaT_pushtuple(L, tuple);
>  	return 1;
>  usage_error:
> diff --git a/test/box/tuple.result b/test/box/tuple.result
> index 9140211b7..78f919deb 100644
> --- a/test/box/tuple.result
> +++ b/test/box/tuple.result
> @@ -1121,7 +1121,13 @@ s:frommap()
>  ...
>  s:frommap({})
>  ---
> -- error: Tuple field 1 required by space format is missing
> +- null
> +- Tuple field 1 required by space format is missing
> +...
> +s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
> +---
> +- null
> +- 'Tuple field 4 type does not match one required by operation: expected unsigned'
>  ...
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
>  ---
> diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
> index 3ac2902fe..baf2f22d5 100644
> --- a/test/box/tuple.test.lua
> +++ b/test/box/tuple.test.lua
> @@ -373,6 +373,7 @@ s:frommap({ddd = 1, aaa = 2, bbb = 3})
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
>  s:frommap()
>  s:frommap({})
> +s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
> --
> 2.21.0 (Apple Git-122.2)
>