Re: [Tarantool-patches] [RFC 3/4] cfg: prepare symbolic evaluation of replication_synchro_quorum

[Mons Anderson <v.perepelitsa@corp.mail.ru>]

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));
> +	}