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

[Imeev Mergen <imeevma@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 7429 bytes --]

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 <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.
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 <imeevma@gmail.com>
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.


[-- Attachment #2: Type: text/html, Size: 11255 bytes --]