From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id EAEFB2DD6F for ; Thu, 22 Nov 2018 16:49:18 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UgrmVfRczDhf for ; Thu, 22 Nov 2018 16:49:18 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id A719A2DA4B for ; Thu, 22 Nov 2018 16:49:18 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 2/7] box: add method dump_lua to port References: <15aaf24f520f8a1ad834ec417bcde6f42331a583.1542910674.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Fri, 23 Nov 2018 00:49:11 +0300 MIME-Version: 1.0 In-Reply-To: <15aaf24f520f8a1ad834ec417bcde6f42331a583.1542910674.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: imeevma@tarantool.org, tarantool-patches@freelists.org Hi! Thanks for the fixes! See my 5 comments below, fix at the end of the email and on the branch. Also, please, do not forget next time to answer to my comments inlined. Do not just send v2 without any answers. On 22/11/2018 22:10, imeevma@tarantool.org wrote: > New method dump_lua dumps saved in port tuples to Lua stack. It > will allow us to call this method without any other interaction > with port. > > Needed for #3505 > --- > src/box/lua/call.c | 1 + > src/box/port.c | 22 ++++++++++++++++++++++ > src/box/port.h | 12 ++++++++++++ > 3 files changed, 35 insertions(+) > > diff --git a/src/box/port.c b/src/box/port.c > index 266cf3d..a65a32d 100644 > --- a/src/box/port.c > +++ b/src/box/port.c > @@ -36,6 +36,8 @@ > #include > #include > #include "errinj.h" > +#include "lua/utils.h" > +#include "lua/tuple.h" 1. src/box/ files should not depend in Lua. I moved implementation to misc.cc and put here extern declaration. > > static struct mempool port_tuple_entry_pool; > > @@ -121,6 +123,19 @@ port_tuple_dump_msgpack(struct port *base, struct obuf *out) > return 1; > } > > +static int 2. Why do you need to return number of dumped tuples? Dump_msgpack returns number of tuples since in iproto.cc it firstly reserves msgpack array header, then dumps port and then fills the reserved header. For Lua we do not need it. > +port_tuple_dump_lua(struct port *base, struct lua_State *L) > +{ > + struct port_tuple *port = port_tuple(base); > + struct port_tuple_entry *pe; > + int i = 0; 3. Why did not you add here lua_createtable(L, port->size, 0); as it is done in misc.cc in the original implementation? 4. Why did not you replaced lbox_port_to_table with this new method? I did this points and it works. > + for (pe = port->first; pe != NULL; pe = pe->next) { > + luaT_pushtuple(L, pe->tuple); > + lua_rawseti(L, -2, ++i); > + } > + return port->size; > +} > + > void > port_destroy(struct port *port) > { > diff --git a/src/box/port.h b/src/box/port.h > index 882bb37..3bd83b0 100644 > --- a/src/box/port.h > +++ b/src/box/port.h > @@ -185,6 +190,13 @@ int > port_dump_msgpack_16(struct port *port, struct obuf *out); > > /** > + * Same as port_dump(), but use the legacy Tarantool 1.6 > + * format. 5. This comment does not make sense to this function. > + */ > +int > +port_dump_lua(struct port *port, struct lua_State *L); > + > +/** > * Dump a port content as a plain text into a buffer, > * allocated inside. > * @param port Port with data to dump. > =========================================================== diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc index 8bd33aed1..6af811f7d 100644 --- a/src/box/lua/misc.cc +++ b/src/box/lua/misc.cc @@ -56,23 +56,26 @@ lbox_encode_tuple_on_gc(lua_State *L, int idx, size_t *p_len) return (char *) region_join_xc(gc, *p_len); } -/* }}} */ - -/** {{{ Lua/C implementation of index:select(): used only by Vinyl **/ - -static inline void -lbox_port_to_table(lua_State *L, struct port *port_base) +/** + * Dump port_tuple content to Lua as a table. Used in box/port.c, + * but implemented here to eliminate port.c dependency on Lua. + */ +extern "C" void +port_tuple_dump_lua(struct port *base, struct lua_State *L) { - struct port_tuple *port = port_tuple(port_base); + struct port_tuple *port = port_tuple(base); lua_createtable(L, port->size, 0); - struct port_tuple_entry *entry = port->first; - for (int i = 0 ; i < port->size; i++) { - luaT_pushtuple(L, entry->tuple); - lua_rawseti(L, -2, i + 1); - entry = entry->next; + struct port_tuple_entry *pe = port->first; + for (int i = 0; pe != NULL; pe = pe->next) { + luaT_pushtuple(L, pe->tuple); + lua_rawseti(L, -2, ++i); } } +/* }}} */ + +/** {{{ Lua/C implementation of index:select(): used only by Vinyl **/ + static int lbox_select(lua_State *L) { @@ -106,7 +109,7 @@ lbox_select(lua_State *L) * table always crashed the first (can't be fixed with pcall). * https://github.com/tarantool/tarantool/issues/1182 */ - lbox_port_to_table(L, &port); + port_dump_lua(&port, L); port_destroy(&port); return 1; /* lua table with tuples */ } diff --git a/src/box/port.c b/src/box/port.c index a65a32d96..df94eb507 100644 --- a/src/box/port.c +++ b/src/box/port.c @@ -123,18 +123,8 @@ port_tuple_dump_msgpack(struct port *base, struct obuf *out) return 1; } -static int -port_tuple_dump_lua(struct port *base, struct lua_State *L) -{ - struct port_tuple *port = port_tuple(base); - struct port_tuple_entry *pe; - int i = 0; - for (pe = port->first; pe != NULL; pe = pe->next) { - luaT_pushtuple(L, pe->tuple); - lua_rawseti(L, -2, ++i); - } - return port->size; -} +extern void +port_tuple_dump_lua(struct port *base, struct lua_State *L); void port_destroy(struct port *port) @@ -154,10 +144,10 @@ port_dump_msgpack_16(struct port *port, struct obuf *out) return port->vtab->dump_msgpack_16(port, out); } -int +void port_dump_lua(struct port *port, struct lua_State *L) { - return port->vtab->dump_lua(port, L); + port->vtab->dump_lua(port, L); } const char * diff --git a/src/box/port.h b/src/box/port.h index 3bd83b092..751e44efe 100644 --- a/src/box/port.h +++ b/src/box/port.h @@ -77,11 +77,8 @@ struct port_vtab { * 1.6 format. */ int (*dump_msgpack_16)(struct port *port, struct obuf *out); - /** - * Dump the content of a port to Lua stack. - * On success returns number of entries dumped. - */ - int (*dump_lua)(struct port *port, struct lua_State *L); + /** Dump the content of a port to Lua stack. */ + void (*dump_lua)(struct port *port, struct lua_State *L); /** * Dump a port content as a plain text into a buffer, * allocated inside. @@ -189,11 +186,8 @@ port_dump_msgpack(struct port *port, struct obuf *out); int port_dump_msgpack_16(struct port *port, struct obuf *out); -/** - * Same as port_dump(), but use the legacy Tarantool 1.6 - * format. - */ -int +/** Dump port content to Lua stack. */ +void port_dump_lua(struct port *port, struct lua_State *L); /**