Re: [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings

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

Thanks for the patch!

I fixed code style by adding lbox_ prefix to functions
related to Lua box.session.*; added 'struct' to lua_State
mentions; fixed a crash in setting set(); dropped global
type 'session_setting'; renamed session_setting_serialize
to session_setting_get, because in future we likely will
add setting:get() method, and we could use the same function
as __serialize for this.

Consider my fixes below and on the branch.

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

commit ff63c003f7b7332f4b21c92cdcc8d85ac8f2e002
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Feb 6 22:17:14 2020 +0100

    Review fixes

diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index 9aaf9e8dd..a0bce2333 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -414,7 +414,7 @@ lbox_session_on_access_denied(struct lua_State *L)
 }
 
 static int
-session_setting_serialize(lua_State *L)
+lbox_session_setting_get(struct lua_State *L)
 {
 	lua_getfield(L, -1, "_id");
 	int sid = lua_tointeger(L, -1);
@@ -435,10 +435,10 @@ session_setting_serialize(lua_State *L)
 }
 
 static int
-session_setting_set(lua_State *L)
+lbox_session_setting_set(struct lua_State *L)
 {
 	if (lua_gettop(L) != 2)
-		goto error;
+		return luaL_error(L, "Usage: box.session.settings:set(value)");
 	int arg_type = lua_type(L, -1);
 	lua_getfield(L, -2, "_id");
 	int sid = lua_tointeger(L, -1);
@@ -459,43 +459,47 @@ session_setting_set(lua_State *L)
 			size_t len = strlen(str);
 			uint32_t size = mp_sizeof_str(len);
 			char *mp_value = static_alloc(size);
+			if (mp_value == NULL) {
+				diag_set(OutOfMemory, size, "static_alloc",
+					 "mp_value");
+				return luaT_error(L);
+			}
 			mp_encode_str(mp_value, str, len);
 			if (setting->set(sid, mp_value) != 0)
 				return luaT_push_nil_and_error(L);
 			break;
 		}
 		default:
-			goto error;
+			diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+				 session_setting_strs[sid],
+				 field_type_strs[setting->field_type]);
+			return luaT_push_nil_and_error(L);
 	}
-	lua_pushstring(L, session_setting_strs[sid]);
-	lua_pushvalue(L, -2);
-	return 2;
-error:
-	return luaL_error(L, "Usage: setting:set(value)");
+	return 0;
 }
 
 static void
-session_setting_create(struct lua_State *L, int id)
+lbox_session_settings_init(struct lua_State *L)
 {
-	lua_newtable(L);
-	lua_pushstring(L, "_id");
-	lua_pushinteger(L, id);
-	lua_settable(L, -3);
-	luaL_getmetatable(L, "session_setting");
-	lua_setmetatable(L, -2);
-}
+	lua_createtable(L, 0, 2);
+	lua_pushcfunction(L, lbox_session_setting_get);
+	lua_setfield(L, -2, "__serialize");
+	lua_createtable(L, 0, 1);
+	lua_pushcfunction(L, lbox_session_setting_set);
+	lua_setfield(L, -2, "set");
+	lua_setfield(L, -2, "__index");
 
-void
-session_settings_init(struct lua_State *L)
-{
-	lua_pushstring(L, "settings");
 	lua_newtable(L);
 	for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
-		lua_pushstring(L, session_setting_strs[id]);
-		session_setting_create(L, id);
-		lua_settable(L, -3);
+		lua_newtable(L);
+		lua_pushinteger(L, id);
+		lua_setfield(L, -2, "_id");
+		lua_pushvalue(L, -3);
+		lua_setmetatable(L, -2);
+		lua_setfield(L, -2, session_setting_strs[id]);
 	}
-	lua_settable(L, -3);
+	lua_setfield(L, -3, "settings");
+	lua_pop(L, 1);
 }
 
 void
@@ -545,13 +549,6 @@ box_lua_session_init(struct lua_State *L)
 	luaL_register(L, "box.internal.session", session_internal_lib);
 	lua_pop(L, 1);
 
-	static const struct luaL_Reg session_setting_meta[] = {
-		{"__serialize", session_setting_serialize},
-		{"set", session_setting_set},
-		{NULL, NULL}
-	};
-	luaL_register_type(L, "session_setting", session_setting_meta);
-	
 	static const struct luaL_Reg sessionlib[] = {
 		{"id", lbox_session_id},
 		{"type", lbox_session_type},
@@ -572,6 +569,6 @@ box_lua_session_init(struct lua_State *L)
 		{NULL, NULL}
 	};
 	luaL_register_module(L, sessionlib_name, sessionlib);
-	session_settings_init(L);
+	lbox_session_settings_init(L);
 	lua_pop(L, 1);
 }
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index 1bb3757ff..6d7074e8c 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -318,8 +318,6 @@ settings.sql_default_engine
  | ...
 settings.sql_default_engine:set('memtx')
  | ---
- | - sql_default_engine
- | - memtx
  | ...
 s:get('sql_default_engine').value
  | ---
@@ -327,8 +325,6 @@ s:get('sql_default_engine').value
  | ...
 settings.sql_defer_foreign_keys:set(true)
  | ---
- | - sql_defer_foreign_keys
- | - true
  | ...
 s:get('sql_defer_foreign_keys').value
  | ---
@@ -350,10 +346,18 @@ settings.sql_default_engine:set(true)
  | ...
 settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
  | ---
- | - error: 'Usage: setting:set(value)'
+ | - error: 'Usage: box.session.settings:set(value)'
  | ...
 settings.sql_parser_debug:set('string')
  | ---
  | - null
  | - Session setting sql_parser_debug expected a value of type boolean
  | ...
+
+str = string.rep('a', 20 * 1024)
+ | ---
+ | ...
+box.session.settings.sql_default_engine:set(str)
+ | ---
+ | - error: Failed to allocate 20483 bytes in static_alloc for mp_value
+ | ...
diff --git a/test/box/session_settings.test.lua b/test/box/session_settings.test.lua
index 89b98dde2..23799874a 100644
--- a/test/box/session_settings.test.lua
+++ b/test/box/session_settings.test.lua
@@ -135,3 +135,6 @@ settings.sql_defer_foreign_keys
 settings.sql_default_engine:set(true)
 settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
 settings.sql_parser_debug:set('string')
+
+str = string.rep('a', 20 * 1024)
+box.session.settings.sql_default_engine:set(str)