Re: [Tarantool-patches] [PATCH 12/20] net.box: rewrite error decoder in C

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index c0c3725e5350..e88db6323afa 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -1082,6 +1084,63 @@ netbox_decode_method(struct lua_State *L)
> +static int
> +netbox_decode_error(struct lua_State *L)
> +{
> +	uint32_t ctypeid;
> +	const char **data = luaL_checkcdata(L, 1, &ctypeid);
> +	assert(ctypeid == CTID_CHAR_PTR || ctypeid == CTID_CONST_CHAR_PTR);
> +	uint32_t errcode = lua_tointeger(L, 2);
> +	struct error *error = NULL;
> +	assert(mp_typeof(**data) == MP_MAP);
> +	uint32_t map_size = mp_decode_map(data);
> +	for (uint32_t i = 0; i < map_size; ++i) {
> +		uint32_t key = mp_decode_uint(data);
> +		if (key == IPROTO_ERROR) {
> +			if (error != NULL)
> +				error_unref(error);
> +			error = error_unpack_unsafe(data);
> +			if (error == NULL)
> +				return luaT_error(L);
> +			error_ref(error);
> +			/*
> +			 * IPROTO_ERROR comprises error encoded with
> +			 * IPROTO_ERROR_24, so we may ignore content
> +			 * of that key.
> +			 */
> +			break;
> +		} else if (key == IPROTO_ERROR_24) {
> +			if (error != NULL)
> +				error_unref(error);
> +			const char *reason = "";
> +			uint32_t reason_len = 0;
> +			if (mp_typeof(**data) == MP_STR)
> +				reason = mp_decode_str(data, &reason_len);
> +			box_error_raise(errcode, "%.*s", reason_len, reason);
> +			error = box_error_last();
> +			error_ref(error);
> +			continue;
> +		}
> +		mp_next(data); /* skip value */

1. Could you please write comments on their own lines if possible?

> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 9e41d6c0844b..d7394b088752 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -73,14 +74,6 @@ local M_COUNT       = 16
>  -- Injects raw data into connection. Used by console and tests.
>  local M_INJECT      = 17
>  
> -ffi.cdef[[
> -struct error *
> -error_unpack_unsafe(const char **data);

2. You should be able to remove it from exports.h now. It
is not used by FFI anymore. (error_unref() should stay in
export.h - it is used by error Lua module.)

> -
> -void
> -error_unref(struct error *e);
> -]]
> -
>  -- utility tables
>  local is_final_state         = {closed = 1, error = 1}
>  
> @@ -97,29 +90,6 @@ local function version_at_least(peer_version_id, major, minor, patch)
>      return peer_version_id >= version_id(major, minor, patch)
>  end
>  
> -local function decode_error(raw_data)
> -    local ptr = ffi.new('const char *[1]', raw_data)
> -    local err = ffi.C.error_unpack_unsafe(ptr)
> -    if err ~= nil then
> -        err._refs = err._refs + 1
> -        -- From FFI it is returned as 'struct error *', which is
> -        -- not considered equal to 'const struct error &', and is
> -        -- is not accepted by functions like box.error(). Need to
> -        -- cast explicitly.
> -        err = ffi.cast('const struct error &', err)
> -        err = ffi.gc(err, ffi.C.error_unref)
> -    else
> -        -- Error unpacker installs fail reason into diag.
> -        box.error()
> -    end
> -    return err, ptr[0]
> -end
> -
> -local response_decoder = {
> -    [IPROTO_ERROR_24] = decode,
> -    [IPROTO_ERROR] = decode_error,

3. IPROTO_ERROR is now unused.