[tarantool-patches] Re: [PATCH v2 2/7] box: add method dump_lua to port

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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 <small/mempool.h>
>   #include <fiber.h>
>   #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);
  
  /**