From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 1C899446439 for ; Sun, 11 Oct 2020 18:25:25 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <6b107979-103c-52b7-9102-9b55bc11d733@tarantool.org> Date: Sun, 11 Oct 2020 17:25:23 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 06/15] WIP: module api/lua: add luaT_tuple_encode() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! See 4 comments below. On 11.10.2020 14:57, Alexander Turenko wrote: > It is the same as luaT_tuple_new(), but returns raw MsgPack data (not > ) allocated on the box region. > > The reason to expose this function is to provide ability to use > box_tuple_compare_with_key() function from an external module for a key > passed as a Lua table. The compare function has the following signature: > > | API_EXPORT int > | box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b, > | box_key_def_t *key_def); > > The second parameter is a key encoded as an MsgPack array, not a tuple > structure. So luaT_tuple_new() is not applicable here (it is not > worthful to create a tuple structure if we need just MsgPack data). > > Some complexity was introduced to support encoding on the Lua shared > buffer and the box region both. The motivation is the following: > > - luaT_tuple_encode() is exposed with encoding to the box region, > because it is more usual to the module API. In particular a user of > the API able to control when the tuple data should be released. > - Encoding to the Lua shared buffer is kept internally, because there is > no strong reason to change it to the box region for box.tuple.new(). > > Part of #5273 > > XXX: Try to get rid of luaT_mpstream_init_*() functions: call > mpstream_init() outside of the protected section with changed error > handler. > --- > src/box/lua/tuple.c | 99 ++++++++++++++++++++++---- > src/box/lua/tuple.h | 17 +++++ > src/exports.h | 1 + > test/app-tap/module_api.c | 115 +++++++++++++++++++++++++++++++ > test/app-tap/module_api.test.lua | 2 +- > 5 files changed, 220 insertions(+), 14 deletions(-) > > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c > index 18cfef979..8e2255a2b 100644 > --- a/src/box/lua/tuple.c > +++ b/src/box/lua/tuple.c > @@ -100,10 +100,31 @@ luaT_istuple(struct lua_State *L, int narg) > return *(struct tuple **) data; > } > > +/* {{{ Encode a Lua table as an MsgPack array */ > + > +/* > + * A lot of functions are there, however the task per se looks > + * simple. Reasons are the following. > + * > + * 1. box.tuple.new() supports two parameters conventions. > + * () implements the old API. > + * 2. Serializer from Lua to MsgPack may raise a Lua error, > + * so it should be run under pcall. The dangerous code is > + * encapsulated into (). > + * 3. In particular () may raise an error in case > + * of OOM, so it also is run under pcall. > + * 4. box.tuple.new() and () use shared Lua ibuf > + * under the hood (because there is no strong reason to change > + * it), while () uses the box region > + * (because it is usual for the module API). > + */ > + > /** > * Encode a Lua values on a Lua stack as an MsgPack array. > * > * Raise a Lua error when encoding fails. > + * > + * Helper for (). 1. Should be a part of the previous patch? > */ > static int > luaT_tuple_encode_values(struct lua_State *L) > @@ -122,6 +143,22 @@ luaT_tuple_encode_values(struct lua_State *L) > return 0; > } > > +typedef void luaT_mpstream_init_f(struct mpstream *stream, struct lua_State *L); > + > +static void > +luaT_mpstream_init_lua_ibuf(struct mpstream *stream, struct lua_State *L) > +{ > + mpstream_init(stream, tarantool_lua_ibuf, ibuf_reserve_cb, > + ibuf_alloc_cb, luamp_error, L); > +} > + > +static void > +luaT_mpstream_init_box_region(struct mpstream *stream, struct lua_State *L) > +{ > + mpstream_init(stream, &fiber()->gc, region_reserve_cb, region_alloc_cb, > + luamp_error, L); 2. If you throw after some allocations are done, will the region leak? I don't see a truncate in case luaT_tuple_encode_helper fails. > +} > + > /** > * Encode a Lua table or a tuple as MsgPack. > * > @@ -132,25 +169,26 @@ luaT_tuple_encode_values(struct lua_State *L) > static int > luaT_tuple_encode_table(struct lua_State *L) > { > - struct ibuf *buf = tarantool_lua_ibuf; > - ibuf_reset(buf); > struct mpstream stream; > - mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error, > - L); > - luamp_encode_tuple(L, &tuple_serializer, &stream, 1); > + luaT_mpstream_init_f *luaT_mpstream_init_f = lua_topointer(L, 1); > + luaT_mpstream_init_f(&stream, L); > + luamp_encode_tuple(L, &tuple_serializer, &stream, 2); > mpstream_flush(&stream); > return 0; > } > > -static char * > -luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx, > - size_t *tuple_len_ptr) > +/** > + * Encode a Lua table / tuple to given mpstream. > + */ > +static int > +luaT_tuple_encode_helper(struct lua_State *L, int idx, > + luaT_mpstream_init_f *luaT_mpstream_init_f) 3. Function name is usually a verb. Here I can propose luaT_tuple_encode_on_mpstream. > { > assert(idx != 0); > if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) { > diag_set(IllegalParams, "A tuple or a table expected, got %s", > lua_typename(L, lua_type(L, idx))); > - return NULL; > + return -1; > } > > int top = lua_gettop(L); > @@ -163,18 +201,53 @@ luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx, > lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref); > assert(lua_isfunction(L, -1)); > > + lua_pushlightuserdata(L, luaT_mpstream_init_f); > lua_pushvalue(L, idx); > > - int rc = luaT_call(L, 1, 0); > + int rc = luaT_call(L, 2, 0); > lua_settop(L, top); > - if (rc != 0) > + return rc == 0 ? 0 : -1; 4. Why not simply 'return rc;'?