Re: [tarantool-patches] [PATCH v1 1/1] netbox: follow-ups

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi!

Thanks for your fixes. I appreciate that you decided
to fix the patch after push. See 2 comments below.

>> Thanks for the patch!
>>
>>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
>>> @@ -618,6 +618,13 @@ static int
>>>  netbox_decode_select(struct lua_State *L)
>>>  {
>>>      uint32_t ctypeid;
>>> +    int top = lua_gettop(L);
>>> +    assert(top == 1 || top == 2);
>>> +    struct tuple_format *format;
>>> +    if (top == 2 && lua_type(L, 2) == LUA_TCDATA)
>>> +        format = lbox_check_tuple_format(L, 2);
>>
>> How is it possible, that we do not have a format here?
>>
> 
> it is possible since decode_call_16 calls select without format.

1. Oh, I forgot that call16 uses select decoder.

> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 251ad40..f7e1688 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -64,12 +64,12 @@ local function decode_data(raw_data)
>      local response, raw_end = decode(raw_data)
>      return response[IPROTO_DATA_KEY], raw_end
>  end
> -local function decode_tuple(raw_data)
> -    local response, raw_end = internal.decode_select(raw_data)
> +local function decode_tuple(raw_data, raw_data_end, format)
> +    local response, raw_end = internal.decode_select(raw_data, format)
>      return response[1], raw_end
>  end
> -local function decode_get(raw_data)
> -    local body, raw_end = internal.decode_select(raw_data)
> +local function decode_get(raw_data, raw_data_end, format)
> +    local body, raw_end = internal.decode_select(raw_data, format)
>      if body[2] then
>          return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE
>      end
> @@ -83,6 +83,12 @@ local function decode_push(raw_data)
>      local response, raw_end = decode(raw_data)
>      return response[IPROTO_DATA_KEY][1], raw_end
>  end
> +local function decode_call_16(raw_data)
> +    return internal.decode_select(raw_data)
> +end

2. Why do you need that wrapper now? You could keep
direct reference to internal.decode_select in method_decoder
table. Couldn't you?

> +local function decode_select(raw_data, raw_data_end, format)
> +    return internal.decode_select(raw_data, format)
> +end
>  

See my proposals below and on the branch in a separate
commit:

===================================================================

diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index 3340070b0..a0a72aa28 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -201,6 +201,7 @@ lbox_tuple_format_new(struct lua_State *L)
 		if (fields == NULL) {
 			diag_set(OutOfMemory, size, "region_alloc",
 				 "fields[i].name");
+			region_truncate(region, region_svp);
 			return luaT_error(L);
 		}
 		memcpy(fields[i].name, name, len);
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index ad7bc6adf..a30105988 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -619,7 +619,7 @@ netbox_decode_select(struct lua_State *L)
 {
 	uint32_t ctypeid;
 	int top = lua_gettop(L);
-	assert(top == 1 || top == 2);
+	assert(top == 1 || top == 2 || top == 3 && lua_isnil(L, 3));
 	struct tuple_format *format;
 	if (top == 2 && lua_type(L, 2) == LUA_TCDATA)
 		format = lbox_check_tuple_format(L, 2);
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index f7e1688bb..c0e85e1c9 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -83,9 +83,6 @@ local function decode_push(raw_data)
     local response, raw_end = decode(raw_data)
     return response[IPROTO_DATA_KEY][1], raw_end
 end
-local function decode_call_16(raw_data)
-    return internal.decode_select(raw_data)
-end
 local function decode_select(raw_data, raw_data_end, format)
     return internal.decode_select(raw_data, format)
 end
@@ -116,7 +113,7 @@ local method_encoder = {
 
 local method_decoder = {
     ping    = decode_nil,
-    call_16 = decode_call_16,
+    call_16 = internal.decode_select,
     call_17 = decode_data,
     eval    = decode_data,
     insert  = decode_tuple,