Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [PATCH v2 5/6] net.box: add helpers to decode msgpack headers
Date: Thu, 10 Jan 2019 20:29:33 +0300	[thread overview]
Message-ID: <20190110172933.255ikbqspmcjmqaj@esperanza> (raw)
In-Reply-To: <1f8e93d96735a92500b0ae40d51b19107399a550.1547064388.git.alexander.turenko@tarantool.org>

On Wed, Jan 09, 2019 at 11:20:13PM +0300, Alexander Turenko wrote:
> Needed for #3276.
> 
> @TarantoolBot document
> Title: net.box: helpers to decode msgpack headers
> 
> They allow to skip iproto packet and msgpack array headers and pass raw
> msgpack data to some other function, say, merger.
> 
> Contracts:
> 
> ```
> net_box.check_iproto_data(buf.rpos, buf.wpos - buf.rpos)
>     -> new_rpos
>     -> nil, err_msg

I'd prefer if this was done right in net.box.select or whatever function
writing the response to ibuf. Yes, this is going to break backward
compatibility, but IMO it's OK for 2.1 - I doubt anybody have used this
weird high perf API anyway.

> msgpack.check_array(buf.rpos, buf.wpos - buf.rpos, [, arr_len])
>     -> new_rpos, arr_len
>     -> nil, err_msg

This seems to be OK, although I'm not sure if we really need to check
the length in this function. Looks like we will definitely need it
because of net.box.call, which wraps function return value in an array.
Not sure about the name either, because it doesn't just checks the
msgpack - it decodes it, but can't come up with anything substantially
better. May be, msgpack.decode_array?

> ```
> 
> Below the example with msgpack.decode() as the function that need raw
> msgpack data. It is just to illustrate the approach, there is no sense
> to skip iproto/array headers manually in Lua and then decode the rest in
> Lua. But it worth when the raw msgpack data is subject to process in a C
> module.
> 
> ```lua
> local function single_select(space, ...)
>     return box.space[space]:select(...)
> end
> 
> local function batch_select(spaces, ...)
>     local res = {}
>     for _, space in ipairs(spaces) do
>         table.insert(res, box.space[space]:select(...))
>     end
>     return res
> end
> 
> _G.single_select = single_select
> _G.batch_select = batch_select
> 
> local res
> 
> local buf = buffer.ibuf()
> conn.space.s:select(nil, {buffer = buf})
> -- check and skip iproto_data header
> buf.rpos = assert(net_box.check_iproto_data(buf.rpos, buf.wpos - buf.rpos))
> -- check that we really got data from :select() as result
> res, buf.rpos = msgpack.decode(buf.rpos, buf.wpos - buf.rpos)
> -- check that the buffer ends
> assert(buf.rpos == buf.wpos)
> 
> buf:recycle()
> conn:call('single_select', {'s'}, {buffer = buf})
> -- check and skip the iproto_data header
> buf.rpos = assert(net_box.check_iproto_data(buf.rpos, buf.wpos - buf.rpos))
> -- check and skip the array around return values
> buf.rpos = assert(msgpack.check_array(buf.rpos, buf.wpos - buf.rpos, 1))
> -- check that we really got data from :select() as result
> res, buf.rpos = msgpack.decode(buf.rpos, buf.wpos - buf.rpos)
> -- check that the buffer ends
> assert(buf.rpos == buf.wpos)
> 
> buf:recycle()
> local spaces = {'s', 't'}
> conn:call('batch_select', {spaces}, {buffer = buf})
> -- check and skip the iproto_data header
> buf.rpos = assert(net_box.check_iproto_data(buf.rpos, buf.wpos - buf.rpos))
> -- check and skip the array around return values
> buf.rpos = assert(msgpack.check_array(buf.rpos, buf.wpos - buf.rpos, 1))
> -- check and skip the array header before the first select result
> buf.rpos = assert(msgpack.check_array(buf.rpos, buf.wpos - buf.rpos, #spaces))
> -- check that we really got data from s:select() as result
> res, buf.rpos = msgpack.decode(buf.rpos, buf.wpos - buf.rpos)
> -- t:select() data
> res, buf.rpos = msgpack.decode(buf.rpos, buf.wpos - buf.rpos)
> -- check that the buffer ends
> assert(buf.rpos == buf.wpos)
> ```
> ---
>  src/box/lua/net_box.c         |  49 +++++++++++
>  src/box/lua/net_box.lua       |   1 +
>  src/lua/msgpack.c             |  66 ++++++++++++++
>  test/app-tap/msgpack.test.lua | 157 +++++++++++++++++++++++++++++++++-
>  test/box/net.box.result       |  58 +++++++++++++
>  test/box/net.box.test.lua     |  26 ++++++
>  6 files changed, 356 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index c7063d9c8..d71f33768 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -51,6 +51,9 @@
>  
>  #define cfg luaL_msgpack_default
>  
> +static uint32_t CTID_CHAR_PTR;
> +static uint32_t CTID_CONST_CHAR_PTR;
> +
>  static inline size_t
>  netbox_prepare_request(lua_State *L, struct mpstream *stream, uint32_t r_type)
>  {
> @@ -745,9 +748,54 @@ netbox_decode_execute(struct lua_State *L)
>  	return 2;
>  }
>  
> +/**
> + * net_box.check_iproto_data(buf.rpos, buf.wpos - buf.rpos)
> + *     -> new_rpos
> + *     -> nil, err_msg
> + */
> +int
> +netbox_check_iproto_data(struct lua_State *L)

Instead of adding this function to net_box.c, I'd rather try to add
msgpack helpers for decoding a map, similar to msgpack.check_array added
by your patch, and use them in net_box.lua.

> +{
> +	uint32_t ctypeid;
> +	const char *data = *(const char **) luaL_checkcdata(L, 1, &ctypeid);
> +	if (ctypeid != CTID_CHAR_PTR && ctypeid != CTID_CONST_CHAR_PTR)
> +		return luaL_error(L,
> +			"net_box.check_iproto_data: 'char *' or "
> +			"'const char *' expected");
> +
> +	if (!lua_isnumber(L, 2))
> +		return luaL_error(L, "net_box.check_iproto_data: number "
> +				  "expected as 2nd argument");
> +	const char *end = data + lua_tointeger(L, 2);
> +
> +	int ok = data < end &&
> +		mp_typeof(*data) == MP_MAP &&
> +		mp_check_map(data, end) <= 0 &&
> +		mp_decode_map(&data) == 1 &&
> +		data < end &&
> +		mp_typeof(*data) == MP_UINT &&
> +		mp_check_uint(data, end) <= 0 &&
> +		mp_decode_uint(&data) == IPROTO_DATA;
> +
> +	if (!ok) {
> +		lua_pushnil(L);
> +		lua_pushstring(L,
> +			"net_box.check_iproto_data: wrong iproto data packet");
> +		return 2;
> +	}
> +
> +	*(const char **) luaL_pushcdata(L, ctypeid) = data;
> +	return 1;
> +}
> +
>  int
>  luaopen_net_box(struct lua_State *L)
>  {
> +	CTID_CHAR_PTR = luaL_ctypeid(L, "char *");
> +	assert(CTID_CHAR_PTR != 0);
> +	CTID_CONST_CHAR_PTR = luaL_ctypeid(L, "const char *");
> +	assert(CTID_CONST_CHAR_PTR != 0);
> +
>  	static const luaL_Reg net_box_lib[] = {
>  		{ "encode_ping",    netbox_encode_ping },
>  		{ "encode_call_16", netbox_encode_call_16 },
> @@ -765,6 +813,7 @@ luaopen_net_box(struct lua_State *L)
>  		{ "communicate",    netbox_communicate },
>  		{ "decode_select",  netbox_decode_select },
>  		{ "decode_execute", netbox_decode_execute },
> +		{ "check_iproto_data", netbox_check_iproto_data },
>  		{ NULL, NULL}
>  	};
>  	/* luaL_register_module polutes _G */
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 2bf772aa8..0a38efa5a 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -1424,6 +1424,7 @@ local this_module = {
>      new = connect, -- Tarantool < 1.7.1 compatibility,
>      wrap = wrap,
>      establish_connection = establish_connection,
> +    check_iproto_data = internal.check_iproto_data,
>  }
>  
>  function this_module.timeout(timeout, ...)
> diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
> index b47006038..fca440660 100644
> --- a/src/lua/msgpack.c
> +++ b/src/lua/msgpack.c
> @@ -51,6 +51,7 @@ luamp_error(void *error_ctx)
>  }
>  
>  static uint32_t CTID_CHAR_PTR;
> +static uint32_t CTID_CONST_CHAR_PTR;
>  static uint32_t CTID_STRUCT_IBUF;
>  
>  struct luaL_serializer *luaL_msgpack_default = NULL;
> @@ -418,6 +419,68 @@ lua_ibuf_msgpack_decode(lua_State *L)
>  	return 2;
>  }
>  
> +/**
> + * msgpack.check_array(buf.rpos, buf.wpos - buf.rpos, [, arr_len])
> + *     -> new_rpos, arr_len
> + *     -> nil, err_msg
> + */
> +static int
> +lua_check_array(lua_State *L)
> +{
> +	uint32_t ctypeid;
> +	const char *data = *(const char **) luaL_checkcdata(L, 1, &ctypeid);
> +	if (ctypeid != CTID_CHAR_PTR && ctypeid != CTID_CONST_CHAR_PTR)

Hm, msgpack.decode doesn't care about CTID_CONST_CHAR_PTR. Why should we?

> +		return luaL_error(L, "msgpack.check_array: 'char *' or "
> +				  "'const char *' expected");
> +
> +	if (!lua_isnumber(L, 2))
> +		return luaL_error(L, "msgpack.check_array: number expected as "
> +				  "2nd argument");
> +	const char *end = data + lua_tointeger(L, 2);
> +
> +	if (!lua_isnoneornil(L, 3) && !lua_isnumber(L, 3))
> +		return luaL_error(L, "msgpack.check_array: number or nil "
> +				  "expected as 3rd argument");

Why not simply luaL_checkinteger?

> +
> +	static const char *end_of_buffer_msg = "msgpack.check_array: "
> +		"unexpected end of buffer";

No point to make this variable static.

> +
> +	if (data >= end) {
> +		lua_pushnil(L);
> +		lua_pushstring(L, end_of_buffer_msg);

msgpack.decode throws an error when it fails to decode msgpack data, so
I think this function should throw too.

> +		return 2;
> +	}
> +
> +	if (mp_typeof(*data) != MP_ARRAY) {
> +		lua_pushnil(L);
> +		lua_pushstring(L, "msgpack.check_array: wrong array header");
> +		return 2;
> +	}
> +
> +	if (mp_check_array(data, end) > 0) {
> +		lua_pushnil(L);
> +		lua_pushstring(L, end_of_buffer_msg);
> +		return 2;
> +	}
> +
> +	uint32_t len = mp_decode_array(&data);
> +
> +	if (!lua_isnoneornil(L, 3)) {
> +		uint32_t exp_len = (uint32_t) lua_tointeger(L, 3);

IMO it would be better if you set exp_len when you checked the arguments
(using luaL_checkinteger).

> +		if (len != exp_len) {
> +			lua_pushnil(L);
> +			lua_pushfstring(L, "msgpack.check_array: expected "
> +					"array of length %d, got length %d",
> +					len, exp_len);
> +			return 2;
> +		}
> +	}
> +
> +	*(const char **) luaL_pushcdata(L, ctypeid) = data;
> +	lua_pushinteger(L, len);
> +	return 2;
> +}
> +
>  static int
>  lua_msgpack_new(lua_State *L);
>  
> @@ -426,6 +489,7 @@ static const luaL_Reg msgpacklib[] = {
>  	{ "decode", lua_msgpack_decode },
>  	{ "decode_unchecked", lua_msgpack_decode_unchecked },
>  	{ "ibuf_decode", lua_ibuf_msgpack_decode },
> +	{ "check_array", lua_check_array },
>  	{ "new", lua_msgpack_new },
>  	{ NULL, NULL }
>  };
> @@ -447,6 +511,8 @@ luaopen_msgpack(lua_State *L)
>  	assert(CTID_STRUCT_IBUF != 0);
>  	CTID_CHAR_PTR = luaL_ctypeid(L, "char *");
>  	assert(CTID_CHAR_PTR != 0);
> +	CTID_CONST_CHAR_PTR = luaL_ctypeid(L, "const char *");
> +	assert(CTID_CONST_CHAR_PTR != 0);
>  	luaL_msgpack_default = luaL_newserializer(L, "msgpack", msgpacklib);
>  	return 1;
>  }

  reply	other threads:[~2019-01-10 17:29 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-09 20:20 [PATCH v2 0/6] Merger Alexander Turenko
2019-01-09 20:20 ` [PATCH v2 1/6] Add luaL_iscallable with support of cdata metatype Alexander Turenko
2019-01-10 12:21   ` Vladimir Davydov
2019-01-09 20:20 ` [PATCH v2 2/6] Add functions to ease using Lua iterators from C Alexander Turenko
2019-01-10 12:29   ` Vladimir Davydov
2019-01-15 23:26     ` Alexander Turenko
2019-01-16  8:18       ` Vladimir Davydov
2019-01-16 11:40         ` Alexander Turenko
2019-01-16 12:20           ` Vladimir Davydov
2019-01-17  1:20             ` Alexander Turenko
2019-01-28 18:17         ` Alexander Turenko
2019-03-01  4:04           ` Alexander Turenko
2019-01-09 20:20 ` [PATCH v2 3/6] lua: add luaT_newtuple() Alexander Turenko
2019-01-10 12:44   ` Vladimir Davydov
2019-01-18 21:58     ` Alexander Turenko
2019-01-23 16:12       ` Vladimir Davydov
2019-01-28 16:51         ` Alexander Turenko
2019-03-01  4:08           ` Alexander Turenko
2019-01-09 20:20 ` [PATCH v2 4/6] lua: add luaT_new_key_def() Alexander Turenko
2019-01-10 13:07   ` Vladimir Davydov
2019-01-29 18:52     ` Alexander Turenko
2019-01-30 10:58       ` Alexander Turenko
2019-03-01  4:10         ` Alexander Turenko
2019-01-09 20:20 ` [PATCH v2 5/6] net.box: add helpers to decode msgpack headers Alexander Turenko
2019-01-10 17:29   ` Vladimir Davydov [this message]
2019-02-01 15:11     ` Alexander Turenko
2019-03-05 12:00       ` Alexander Turenko
2019-01-09 20:20 ` [PATCH v2 6/6] Add merger for tuple streams Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190110172933.255ikbqspmcjmqaj@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [PATCH v2 5/6] net.box: add helpers to decode msgpack headers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox