[Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Feb 7 01:15:21 MSK 2020
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 at 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)
More information about the Tarantool-patches
mailing list