[tarantool-patches] Re: [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield()

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 2 comments below.

> @@ -525,94 +533,95 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
>   	case LUA_TBOOLEAN:
>   		field->type = MP_BOOL;
>   		field->bval = lua_toboolean(L, index);
> -		return;
> +		return 0;
>   	case LUA_TNIL:
>   		field->type = MP_NIL;
> -		return;
> +		return 0;
>   	case LUA_TSTRING:
>   		field->sval.data = lua_tolstring(L, index, &size);
>   		field->sval.len = (uint32_t) size;
>   		field->type = MP_STR;
> -		return;
> +		return 0;
>   	case LUA_TTABLE:
>   	{
>   		field->compact = false;
> -		lua_field_inspect_table(L, cfg, index, field);
> -		return;
> +		if (lua_field_inspect_table(L, cfg, index, field) < 0)
> +			return -1;
> +		return 0;

1. Why not simply 'return lua_field_inspect_table' ?

>   	}
>   	case LUA_TLIGHTUSERDATA:
>   	case LUA_TUSERDATA:
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index a47e3d2..fde3514 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -345,7 +348,8 @@ static inline void
>   luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
>   		struct luaL_field *field)
>   {
> -	luaL_tofield(L, cfg, idx, field);
> +	if (luaL_tofield(L, cfg, idx, field) < 0)
> +		luaT_error(L);
>   	if (field->type != MP_EXT)
>   		return;
>   	luaL_convertfield(L, cfg, idx, field);
> -- 
> 2.7.4
> 
> 

2. Looking at lua_field_inspect_table I found that if a table has __serialize
metamethod, it is called without a protection (utils.c:409). __serialize is an
arbitrary unprotected user code, that can throw an error deliberately. What are
we gonna do with that? Personally I've already faced with some user code, throwing
an error from __serialize, deliberately. On not critical errors not meaning panic.

As a solution we could 1) do not care, again; 2) finally accept the fact that
wrap into a pcall was not so bad and use it; 3) use lua_pcall in that
particular place. Please, consult Kostja, what does he choose.