[tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

On 03.04.2018 12:47, Vladislav Shpilevoy wrote:
> Hello. Please, consider 4 comments below.
>> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
>> index 29a9aca..2ff9a11 100644
>> --- a/src/box/lua/space.cc
>> +++ b/src/box/lua/space.cc
>> @@ -32,11 +32,13 @@
>>   #include "box/lua/tuple.h"
>>   #include "lua/utils.h"
>>   #include "lua/trigger.h"
>> +#include "box/schema.h"
>>   extern "C" {
>>       #include <lua.h>
>>       #include <lauxlib.h>
>>       #include <lualib.h>
>> +    #include <trivia/util.h>
>>   } /* extern "C" */
> 1. Schema.h is included below. Trivia/util.h is not needed here.
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 2ff9a11..9004c0e 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -38,7 +38,6 @@ extern "C" {
  	#include <lua.h>
  	#include <lauxlib.h>
  	#include <lualib.h>
-	#include <trivia/util.h>
  } /* extern "C" */

  #include "box/space.h"
> 
>> +        space = space_by_id(id);
>> +        if (!space)
>> +            luaL_error(L, "Specified space is not exists");
>> +
> 2. Please, for pointers use explicit != NULL.
> 
> 3. As I noted earlier, a function must not raise an error. It must 
> return nil + error message/object.
> And please, check your comments grammar: you can not say "is not 
> exists". Perhaps,
> you meant "does not exist".
> 
@@ -423,8 +422,14 @@ lbox_space_ptr_cached(struct lua_State *L)
  		lua_getfield(L, 1, "id");
  		uint32_t id = luaL_touint64(L, -1);
  		space = space_by_id(id);
-		if (!space)
-			luaL_error(L, "Specified space is not exists");
+		if (space == NULL) {
+			const char *err_msg;
+			err_msg = tt_sprintf("Space with id '%d' doesn't exists",
+			                     id);
+			lua_pushnil(L);
+			lua_pushstring(L, err_msg);
+			return 2;
+		}

  		/** update the cache */
  		luaL_pushuint64(L, global_shema_version);
@@ -462,8 +467,14 @@ lbox_space_frommap(struct lua_State *L)
  		table = lua_toboolean(L, -1);
  	}

-	lbox_space_ptr_cached(L);
-	space = (struct space *)lua_topointer(L, -1);
+	if (lbox_space_ptr_cached(L) == 1) {
+		space = (struct space *) lua_topointer(L, -1);
+	} else {
+		const char *err_msg = lua_tostring(L, -1);
+		lua_pushnil(L);
+		lua_pushstring(L, err_msg);
+		return 2;
+	}
  	dict = space->format->dict;
  	lua_createtable(L, space->def->field_count, 0);
> 4. I do not see my crash test in your space_frommap.test.lua. When I 
> provide you a test, or you
> found an own, you must add it to the patch.
> 
There is the same test case here:

diff --git a/test/box/space_frommap.test.lua 
b/test/box/space_frommap.test.lua
index ff7ef98..647d761 100644
--- a/test/box/space_frommap.test.lua
+++ b/test/box/space_frommap.test.lua
@@ -21,4 +21,7 @@ subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 
4}, {dummy = true})
  _ = subscriber:create_index('primary', {parts={'aaa'}})
  tuple = subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
  subscriber:replace(tuple)
+_ = box.internal.space._cptr(subscriber)
  subscriber:drop()
+_ = subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+_ = box.internal.space._cptr(subscriber)