From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 C9356446421 for ; Wed, 25 Mar 2020 21:27:49 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\)) From: Chris Sosnin In-Reply-To: <2d332fe0-5c65-a791-8489-86564d70819f@tarantool.org> Date: Wed, 25 Mar 2020 21:27:48 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20200317172109.33111-1-k.sosnin@tarantool.org> <2d332fe0-5c65-a791-8489-86564d70819f@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for the review! > On 22 Mar 2020, at 22:31, Vladislav Shpilevoy = wrote: >=20 > Hi! >=20 > Is this patch in scope of the other patches of the same thread? > If it is, it should be sent together with them. Otherwise I don't > see what is going to be pushed if I will give ok to the patch. I=E2=80=99m sorry, I=E2=80=99ve sent these 2 separately for 2 different = mail threads. Will send them together next time. >=20 > I see that at least sql patch is changed, but it not resent. >=20 > See 3 comments below. >=20 > On 17/03/2020 18:21, 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. >>=20 >> - Prior to this patch sql settings were not accessible before = box.cfg() >> call, even though these flags can be set right after session = creation. >>=20 >> Part of #4711 >> --- >> This patch provides the following syntax as was suggested by Nikita: >> box.session.settings. =3D . Doc request in sql-patch is >=20 > 1. In the commit message you said 'box.session.setting'. Not = 'settings'. > What option did you want to use? The last one. Commit message fix done. >=20 >> updated too. >>=20 >> Note, that I had to break the null - error message rule as long as >> __newindex doesn't return anything, the only way to tell the user >> about the error is to raise it. >>=20 >> I moved it to the new branch: >> = branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session= -settings-v2 >> issuse:https://github.com/tarantool/tarantool/issues/4711 >>=20 >> diff --git a/src/box/lua/session.c b/src/box/lua/session.c >> index c6a600f6f..66d59c8c7 100644 >> --- a/src/box/lua/session.c >> +++ b/src/box/lua/session.c >> @@ -411,6 +413,116 @@ lbox_session_on_access_denied(struct lua_State = *L) >> lbox_push_on_access_denied_event, = NULL); >> } >>=20 >> +static int >> +lbox_session_setting_get_by_id(struct lua_State *L) >=20 > 2. This function is never called from Lua. You don't need > to pass ID via Lua stack. Add it as a C argument. I agree, fixed. +static int +lbox_session_setting_get_by_id(struct lua_State *L, int sid) +{ + assert(sid >=3D 0 && sid < SESSION_SETTING_COUNT); But now I wonder if it should follow lbox_ naming style, this function is not exposed to users, should I rename it? >=20 >> +{ >> + assert(lua_type(L, -1) =3D=3D LUA_TNUMBER); >> + int sid =3D lua_tointeger(L, -1); >> + lua_pop(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); >> + mp_decode_str(&mp_pair, &len); >> + enum field_type field_type =3D session_settings[sid].field_type; >> + if (field_type =3D=3D FIELD_TYPE_BOOLEAN) { >> + bool value =3D mp_decode_bool(&mp_pair); >> + lua_pushboolean(L, value); >> + } else { >> + assert(field_type =3D=3D FIELD_TYPE_STRING); >> + const char *str =3D mp_decode_str(&mp_pair, &len); >> + lua_pushlstring(L, str, len); >> + } >> + return 1; >> +} >> + >> +static int >> +lbox_session_setting_get(struct lua_State *L) { >=20 > 3. Please, use a separate line for {. The same for serialize. Done.