From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (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 1151146970E for ; Tue, 4 Feb 2020 01:17:20 +0300 (MSK) References: <2da617affc8e5f94e6b4b9b4d6126d5fcd0c6c1b.1580215539.git.k.sosnin@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 3 Feb 2020 23:17:18 +0100 MIME-Version: 1.0 In-Reply-To: <2da617affc8e5f94e6b4b9b4d6126d5fcd0c6c1b.1580215539.git.k.sosnin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4 3/3] 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! See 12 comments below. On 28/01/2020 13:50, Chris Sosnin wrote: > - space_object:update() is hard to use for configuring session settings, > so we provide box.session.setting table, which can be used in a much more > native way. > > - Prior to this patch sql settings were not accessible before box.cfg() > call, even though these flags can be set right after session creation. > > Part of #4711 > --- > src/box/lua/session.c | 106 ++++++++++++++++++ > src/box/sql.c | 5 - > ...1-access-settings-from-any-frontend.result | 57 ++++++++++ > ...access-settings-from-any-frontend.test.lua | 17 +++ > 4 files changed, 180 insertions(+), 5 deletions(-) > > diff --git a/src/box/lua/session.c b/src/box/lua/session.c > index c6a600f6f..3ba1253c7 100644 > --- a/src/box/lua/session.c > +++ b/src/box/lua/session.c > @@ -42,6 +42,9 @@ > #include "box/user.h" > #include "box/schema.h" > #include "box/port.h" > +#include "box/session_settings.h" > + > +extern void sql_session_settings_init(); 1. Please, don't mix SQL and Lua. At least here. Call it in session.cc in session_init(). This file should not initialize any session backends. > static const char *sessionlib_name = "box.session"; > > @@ -411,6 +414,108 @@ lbox_session_on_access_denied(struct lua_State *L) > lbox_push_on_access_denied_event, NULL); > } > > +static int > +session_setting_serialize(lua_State *L) > +{ > + lua_getfield(L, -1, "_id"); > + int sid = lua_tointeger(L, -1); > + const char *mp_pair, *mp_pair_end; > + session_settings[sid].get(sid, &mp_pair, &mp_pair_end); > + uint32_t len; > + mp_decode_array(&mp_pair); /* [setting_name : value] */ 2. Avoid writing comments in the same line as the code, unless this is a special case like a structure inlined initialization. This has simple reasons: lines are shorter and therefore easier to read; diff is smaller when only the code or only the comment are changed. > + mp_decode_str(&mp_pair, &len); > + uint32_t field_type = session_settings[sid].metadata.field_type; > + if (field_type == FIELD_TYPE_BOOLEAN) { > + bool value = mp_decode_bool(&mp_pair); > + lua_pushboolean(L, value); > + } else { > + const char *str = mp_decode_str(&mp_pair, &len); > + lua_pushlstring(L, str, len); > + } > + return 1; > +} > + > +static int > +session_setting_set(lua_State *L) > +{ > + if (lua_gettop(L) != 2) > + goto error; > + int arg_type = lua_type(L, -1); > + lua_getfield(L, -2, "_id"); > + int sid = lua_tointeger(L, -1); > + struct session_setting *setting = &session_settings[sid]; > + lua_pop(L, 1); > + switch (arg_type) { > + case LUA_TBOOLEAN: { > + bool value = lua_toboolean(L, -1); > + size_t size = mp_sizeof_bool(value); > + char *mp_value = (char *)region_alloc(&fiber()->gc, > + size); 3. Just use static_alloc. The same below. > + mp_encode_bool(mp_value, value); > + if (setting->set(sid, mp_value) != 0) > + luaT_error(L); 4. According to the new agreement, all new Lua functions should return nil, err when an error happens. Except OOM and invalid usage of API. So if set() returns != 0, then please, use luaT_push_nil_and_error(). > + lua_pushstring(L, setting->name); > + lua_pushboolean(L, value); > + break; > + } > + case LUA_TSTRING: { > + const char *str = lua_tostring(L, -1); > + size_t len = strlen(str); > + uint32_t size = mp_sizeof_str(len); > + char *mp_value = (char *)region_alloc(&fiber()->gc, > + size); > + mp_encode_str(mp_value, str, len); > + if (setting->set(sid, mp_value) != 0) > + luaT_error(L); > + lua_pushstring(L, setting->name); > + lua_pushstring(L, str); > + break; > + } > + default: > + goto error; > + } > + return 2; > +error: > + luaL_error(L, "box.session.settings.set(): bad arguments"); > + return -1; > +} > + > +static void > +session_setting_create(struct lua_State *L, int id) > +{ > + lua_newtable(L); > + lua_pushstring(L, "_id"); > + lua_pushinteger(L, id); > + lua_settable(L, -3); > + lua_newtable(L); /* setting metatable */ 5. The same as above about code + comment in one line. Also, please, use capital letter to start a sentence, and put a dot in the end. > + lua_pushstring(L, "__serialize"); > + lua_pushcfunction(L, session_setting_serialize); > + lua_settable(L, -3); > + lua_setmetatable(L, -2); 6. You create lots of duplicate metatables here. I would try to create it only once and use that one object for all lua_setmetatable(). > + lua_pushcfunction(L, session_setting_set); > + lua_setfield(L, -2, "set"); > +} > + > +void > +session_settings_init(struct lua_State *L) > +{ > + /* Init settings that are avaliable right after session creation. */ 7. avaliable -> available. 8. Please, keep comments in 66 line width limit. > + sql_session_settings_init(); > + static const struct luaL_Reg settingslib[] = { > + {NULL, NULL} > + }; > + luaL_register_module(L, "box.session.settings", settingslib); > + int id = sql_session_setting_BEGIN; 9. If you start from SQL, you will need to change this code in case something will be added prior to SQL settings. Lets start from 0 and iterate to SESSION_SETTING_COUNT. > + for (; id <= sql_session_setting_END; ++id) { > + /* Store settings as name : id */ 10. Not sure this comment is still correct. You use name : table, rather than name : id. But honestly here this is obvious anyway, so I would drop that comment at all. > + struct session_setting *setting = &session_settings[id]; > + lua_pushstring(L, setting->name); > + session_setting_create(L, id); > + lua_settable(L, -3); > + } > + lua_pop(L, 1); > +} > + > void > session_storage_cleanup(int sid) > { > @@ -448,6 +553,7 @@ exit: > void > box_lua_session_init(struct lua_State *L) > { > + session_settings_init(L); 11. I propose you to move it to the end of this function. You initialize box.session.settings before box.session is created. If you move it to the end, you will be able to drop this dummy module registration, which you are doing above: static const struct luaL_Reg settingslib[] = { {NULL, NULL} }; Because after luaL_register_module(L, sessionlib_name, sessionlib) you have box.session on the stack and can add whatever you want there, including settings. > static const struct luaL_Reg session_internal_lib[] = { > {"create", lbox_session_create}, > {"run_on_connect", lbox_session_run_on_connect}, > diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua > index 40a58ad04..53f03450d 100644 > --- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua > +++ b/test/box/gh-4511-access-settings-from-any-frontend.test.lua > @@ -118,3 +118,20 @@ s:update('sql_defer_foreign_keys', {{'=', 'some text', true}}) > s:update('sql_defer_foreign_keys', {{'=', 'value', 1}}) > s:update('sql_defer_foreign_keys', {{'=', 'value', {1}}}) > s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}}) > + > +-- gh-4711: Provide a user-friendly frontend for accessing session settings. 12. The file has name 'gh-4511-*', so you can't add issue 4711 tests here. Either add to a new file, or rename the test file to box/session_settings. > +settings = box.session.settings > +assert(settings ~= nil) > + > +s:update('sql_default_engine', {{'=', 2, 'vinyl'}}) > +settings.sql_default_engine > +settings.sql_default_engine:set('memtx') > +assert(s:get('sql_default_engine').value == 'memtx') > +settings.sql_defer_foreign_keys:set(true) > +assert(s:get('sql_defer_foreign_keys').value == true) > +s:update('sql_defer_foreign_keys', {{'=', 2, false}}) > +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') >