From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.perepelitsa@corp.mail.ru>
Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106])
 (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 23C99469710
 for <tarantool-patches@dev.tarantool.org>;
 Thu, 26 Nov 2020 17:38:34 +0300 (MSK)
Received: by smtp46.i.mail.ru with esmtpa (envelope-from
 <v.perepelitsa@corp.mail.ru>) id 1kiIPp-0002Af-Fk
 for tarantool-patches@dev.tarantool.org; Thu, 26 Nov 2020 17:38:33 +0300
References: <20201119194100.840495-1-gorcunov@gmail.com>
 <20201119194100.840495-4-gorcunov@gmail.com>
From: Mons Anderson <v.perepelitsa@corp.mail.ru>
Message-ID: <1670ea9e-ebd6-771b-e3ec-d9edc97d461a@corp.mail.ru>
Date: Thu, 26 Nov 2020 17:38:32 +0300
MIME-Version: 1.0
In-Reply-To: <20201119194100.840495-4-gorcunov@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Subject: Re: [Tarantool-patches] [RFC 3/4] cfg: prepare symbolic evaluation
 of replication_synchro_quorum
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: tarantool-patches@dev.tarantool.org

Hi, thanks! Some comments below.

On 19.11.2020 22:40, Cyrill Gorcunov wrote:
>   
> +/**
> + * Evaluate replicaion syncro quorum number from a formula.
> + */
> +int
> +eval_replication_synchro_quorum(int nr_replicas)
> +{
> +	const char fmt[] =
> +		"local f, err = loadstring(\"return (%s)\")\n"
> +		"if not f then return 'failed to load \"%s\"' end\n"
> +		"setfenv(f, { n = %d })\n"
> +		"local ok, res = pcall(f)\n"
> +		"if not ok then return res end\n"
> +		"return math.floor(res)\n";

For the formula evaluation I'd propose the following snippet:

local expr = [[%s]]
local f, err = loadstring('return ('..expr..')')
if not f then error(string.format('Failed to load %%s: %%s',expr, err)) end
setfenv(f, { N = %d, math = math })
return math.floor( f() )

The reasons are:

- [[ ]] quotes are not interpolated, no need to repeat substitution again

- You need to handle errors anyway, so no need to return errors as 
return values. Throw them.

- Also, since error handling (like a pcall from C layer) is required, no 
need to do pcall inside

- Use of uppercase N, not lowercase is desired. Lowercase is used for 
variables, uppercase for constants. Here N is more constant than 
variable. (Or we may include eihter N or n)

- math module may be used for formulas, so pass it also.

> +	luaL_loadstring(tarantool_L, buf);
> +	lua_call(tarantool_L, 0, 1);
Use lua_pcall instead lua_call. So then check for absence of error and 
lua_isnumber
> +	if (lua_isnumber(tarantool_L, -1)) {
> +		value = (int)lua_tonumber(tarantool_L, -1);
> +	} else {
> +		assert(lua_isstring(tarantool_L, -1));
> +		errno = EINVAL;
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 lua_tostring(tarantool_L, -1));
> +	}