From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 593D44696C3 for ; Fri, 7 Feb 2020 01:15:23 +0300 (MSK) From: Vladislav Shpilevoy References: <20200204193125.61431-1-k.sosnin@tarantool.org> Message-ID: <86c831a3-ab96-3142-f900-343804f788f2@tarantool.org> Date: Thu, 6 Feb 2020 23:15:21 +0100 MIME-Version: 1.0 In-Reply-To: <20200204193125.61431-1-k.sosnin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin , tarantool-patches@dev.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 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)