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

[Chris Sosnin <k.sosnin@tarantool.org>]

Hi! Thank you for the review!

> On 22 Mar 2020, at 22:31, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi!
> 
> 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’m sorry, I’ve sent these 2 separately for 2 different mail threads.
Will send them together next time.

> 
> I see that at least sql patch is changed, but it not resent.
> 
> See 3 comments below.
> 
> 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.
>> 
>> - 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
>> ---
>> This patch provides the following syntax as was suggested by Nikita:
>> box.session.settings.<name> = <value>. Doc request in sql-patch is
> 
> 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.

> 
>> updated too.
>> 
>> 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.
>> 
>> 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
>> 
>> 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);
>> }
>> 
>> +static int
>> +lbox_session_setting_get_by_id(struct lua_State *L)
> 
> 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 >= 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?

> 
>> +{
>> +	assert(lua_type(L, -1) == LUA_TNUMBER);
>> +	int sid = 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 = session_settings[sid].field_type;
>> +	if (field_type == FIELD_TYPE_BOOLEAN) {
>> +		bool value = mp_decode_bool(&mp_pair);
>> +		lua_pushboolean(L, value);
>> +	} else {
>> +		assert(field_type == FIELD_TYPE_STRING);
>> +		const char *str = mp_decode_str(&mp_pair, &len);
>> +		lua_pushlstring(L, str, len);
>> +	}
>> +	return 1;
>> +}
>> +
>> +static int
>> +lbox_session_setting_get(struct lua_State *L) {
> 
> 3. Please, use a separate line for {. The same for serialize.

Done.