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 3E5152EFE9 for ; Tue, 27 Nov 2018 14:25:35 -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 VxBiF4RCmYDm for ; Tue, 27 Nov 2018 14:25:35 -0500 (EST) Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 C989F2EFCC for ; Tue, 27 Nov 2018 14:25:34 -0500 (EST) From: Imeev Mergen Subject: [tarantool-patches] Re: [PATCH v2 2/7] box: add method dump_lua to port References: <15aaf24f520f8a1ad834ec417bcde6f42331a583.1542910674.git.imeevma@gmail.com> Message-ID: <2f638853-08a7-f7d1-f5b7-2c233213382c@tarantool.org> Date: Tue, 27 Nov 2018 22:25:33 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------6953A88BF3BE11C883B1A856" Content-Language: en-US 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: Vladislav Shpilevoy , tarantool-patches@freelists.org This is a multi-part message in MIME format. --------------6953A88BF3BE11C883B1A856 Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit 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. --------------6953A88BF3BE11C883B1A856 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit 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.

--------------6953A88BF3BE11C883B1A856--