Hi! Thank you for review and fixes! I squashed your fixes and removed libs from port.c that were added in previous version. New version below. On 11/23/18 12:49 AM, Vladislav Shpilevoy wrote: > 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. Squashed. > >>     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. Squashed. > >> +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? Squashed. > > 4. Why did not you replaced lbox_port_to_table with this new > method? > > I did this points and it works. Squashed. > >> +    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. Squashed. > >> + */ >> +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. >> > *New version:* commit ff2fc3fd58dd99d3a98ad790c8e82363949cb3db Author: Mergen Imeev Date:   Sat Nov 17 15:37:17 2018 +0300     box: add method dump_lua to port     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 diff --git a/src/box/lua/call.c b/src/box/lua/call.c index 1f20426..52939ae 100644 --- a/src/box/lua/call.c +++ b/src/box/lua/call.c @@ -424,6 +424,7 @@ port_lua_dump_plain(struct port *port, uint32_t *size);  static const struct port_vtab port_lua_vtab = {      .dump_msgpack = port_lua_dump,      .dump_msgpack_16 = port_lua_dump_16, +    .dump_lua = NULL,      .dump_plain = port_lua_dump_plain,      .destroy = port_lua_destroy,  }; diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc index 8bd33ae..8de7401 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 266cf3d..853d24c 100644 --- a/src/box/port.c +++ b/src/box/port.c @@ -121,6 +121,9 @@ port_tuple_dump_msgpack(struct port *base, struct obuf *out)      return 1;  } +extern void +port_tuple_dump_lua(struct port *base, struct lua_State *L); +  void  port_destroy(struct port *port)  { @@ -139,6 +142,12 @@ port_dump_msgpack_16(struct port *port, struct obuf *out)      return port->vtab->dump_msgpack_16(port, out);  } +void +port_dump_lua(struct port *port, struct lua_State *L) +{ +    port->vtab->dump_lua(port, L); +} +  const char *  port_dump_plain(struct port *port, uint32_t *size)  { @@ -161,6 +170,7 @@ port_free(void)  const struct port_vtab port_tuple_vtab = {      .dump_msgpack = port_tuple_dump_msgpack,      .dump_msgpack_16 = port_tuple_dump_msgpack_16, +    .dump_lua = port_tuple_dump_lua,      .dump_plain = NULL,      .destroy = port_tuple_destroy,  }; diff --git a/src/box/port.h b/src/box/port.h index 882bb37..751e44e 100644 --- a/src/box/port.h +++ b/src/box/port.h @@ -77,6 +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. */ +    void (*dump_lua)(struct port *port, struct lua_State *L);      /**       * Dump a port content as a plain text into a buffer,       * allocated inside. @@ -184,6 +186,10 @@ port_dump_msgpack(struct port *port, struct obuf *out);  int  port_dump_msgpack_16(struct port *port, struct obuf *out); +/** Dump port content to Lua stack. */ +void +port_dump_lua(struct port *port, struct lua_State *L); +  /**   * Dump a port content as a plain text into a buffer,   * allocated inside.