[Tarantool-patches] [PATCH 2.X v2.1 0/4] module api: extend for external merger Lua module

[Tarantool-patches] [PATCH 2.X v2.1 0/4] module api: extend for external merger Lua module

[Timur Safin]

This patchset is a continuation of patch series which Alexander Turenko sends
elsewhere. It extends module api with the necessary wrappers we should have 
in api to make external merger possible. Some internal functionality which
was acessible to internal merger, should be wrapped via accessors if merger
is becoming external.

- most of introduced functions are simple public wrappers around of internal 
  functions, e.g. `box_tuple_validate` is a wrapper around 
  `tuple_validate_raw`, or `bax_key_def_dup` is a wrapper for `key_def_dup`.
  They were necessary to not reveal implementation specific details 
  (i.e. `tuple_data` needed in `tuple_validate_raw` should know `box_tuple_t`
  layout);
- wherever we used to use `struct tuple *` we replacing them with public alias
  `box_tuple_t`. Same is for `struct tuple_format` -> `box_tuple_format_t` and 
  other typedefs.

Changelog of v2 to v1
---------------------
Please do not be surprised to not seen v2 in your inbox - it was cancelled 
after discussion with Alexander Turenko (see v2.1 below with descriptions)

- Unpublished again `lauT_temp_state`, `luaT_release_temp_state` - they are
  performance wrappers around creation of Lua thread states. Agreed that 
  performance impact not that big, thus created similar compatibility wrappers
  on the module side;
- unpublished `luaT_newthread`, because for module usage `lua_newthread` is 
  enough;
- unpublished `luaL_register_module` & `luaL_register_type` and reworked module
  code with `luaL_register`.

Smallish changelog of v2.1 to v2
--------------------------------
Alexander Turenko has provided valuable feedback to the v2 branch I've shown,
so we have yet more reduced this patchset
- Unpublished `luaL_cdata_iscallable`, it's unnecessary for external module
  which is using now public `luaL_iscallable` module api call, instead 
  of compatibility layer in module;
- Unpublished `luaL_checkconstchar` because it's not yet needed anywhere
  externally;
- Accordingly to thsoe changes above we have reduced code in module_api
  test files.

Plans for v3
------------
1. Reshuffle patchet to merge test with corresponding code;
2. Add ibuf wrapper, despite all controversies

Stay tuned...


Issue:
* https://github.com/tarantool/tarantool/issues/5384
  ('module api: expose everything that is needed for external merger module')

Branches:
* https://github.com/tarantool/tarantool/tree/tsafin/gh-5273-expand-module-api-v2
  (top 5 commits above of 14 @Totktonada's commits)
* 1.10 branch will be sent momentarily as a followup. Stay tuned

== External merger module

Currently external merger is residing here https://github.com/tsafin/tarantool-merge
and it uses key_def from https://github.com/Totktonada/key_def repository
via simple luarocks dependency (we had to introduce debugging rock-server to
make this working).

CI is installed and is running on GitHub Actions, thus we know that it's 
sort of working together (UPD: used to work before key_def api reshuffled recently),
but more extensive testing of merger and key_def yet to be proceeded.


Timur Safin (4):
  module api: export box_tuple_validate
  module api: export box_key_def_dup
  module api: luaL_checkibuf
  module api: external merger tests

 src/box/key_def_api.c            |  6 +++++
 src/box/key_def_api.h            | 10 +++++++
 src/box/tuple.c                  |  6 +++++
 src/box/tuple.h                  | 11 ++++++++
 src/exports.h                    |  3 +++
 src/lua/utils.h                  | 16 ++++++------
 test/app-tap/module_api.c        | 26 ++++++++++++++++++
 test/app-tap/module_api.test.lua | 45 +++++++++++++++++++++++++++++++-
 8 files changed, 114 insertions(+), 9 deletions(-)

-- 
2.20.1

[Tarantool-patches] [PATCH 2.X v2.1 1/4] module api: export box_tuple_validate

[Timur Safin]

For external merger we need means to validate tuple data,
thus exporting `box_tuple_validate` which is wrapper around
`tuple_validate_raw` without revealing access to tuple
internals.

Part of #5384
---
 src/box/tuple.c |  6 ++++++
 src/box/tuple.h | 11 +++++++++++
 src/exports.h   |  1 +
 3 files changed, 18 insertions(+)

diff --git a/src/box/tuple.c b/src/box/tuple.c
index f3965476e..ddf41567c 100644
--- a/src/box/tuple.c
+++ b/src/box/tuple.c
@@ -748,6 +748,12 @@ box_tuple_new(box_tuple_format_t *format, const char *data, const char *end)
 	return tuple_bless(ret);
 }
 
+int
+box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple)
+{
+	return tuple_validate_raw(format, tuple_data(tuple));
+}
+
 /* }}} box_tuple_* */
 
 int
diff --git a/src/box/tuple.h b/src/box/tuple.h
index 53ae690cc..ed0501464 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -283,6 +283,17 @@ box_tuple_update(box_tuple_t *tuple, const char *expr, const char *expr_end);
 box_tuple_t *
 box_tuple_upsert(box_tuple_t *tuple, const char *expr, const char *expr_end);
 
+/**
+ * Check tuple data correspondence to the space format.
+ * @param format Format to which the tuple must match.
+ * @param tuple  Tuple to validate.
+ *
+ * @retval  0 The tuple is valid.
+ * @retval -1 The tuple is invalid.
+ */
+int
+box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple);
+
 /** \endcond public */
 
 /**
diff --git a/src/exports.h b/src/exports.h
index 48894ea72..592b388bb 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -77,6 +77,7 @@ EXPORT(box_tuple_to_buf)
 EXPORT(box_tuple_unref)
 EXPORT(box_tuple_update)
 EXPORT(box_tuple_upsert)
+EXPORT(box_tuple_validate)
 EXPORT(box_tuple_validate_key_parts)
 EXPORT(box_txn)
 EXPORT(box_txn_alloc)
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 2.X v2.1 1/4] module api: export box_tuple_validate

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 11.10.2020 16:39, Timur Safin wrote:
> For external merger we need means to validate tuple data,
> thus exporting `box_tuple_validate` which is wrapper around
> `tuple_validate_raw` without revealing access to tuple
> internals.
> 
> Part of #5384
> ---
>  src/box/tuple.c |  6 ++++++
>  src/box/tuple.h | 11 +++++++++++
>  src/exports.h   |  1 +
>  3 files changed, 18 insertions(+)
> 
> diff --git a/src/box/tuple.c b/src/box/tuple.c
> index f3965476e..ddf41567c 100644
> --- a/src/box/tuple.c
> +++ b/src/box/tuple.c
> @@ -748,6 +748,12 @@ box_tuple_new(box_tuple_format_t *format, const char *data, const char *end)
>  	return tuple_bless(ret);
>  }
>  
> +int
> +box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple)

If a function is a method of an object, it should have the object
name in prefix, and have the object pointer in the first argument.
For example, box_tuple_to_buf(), box_tuple_field(), box_tuple_update(),
box_tuple_upsert().

So here the tuple should be the first argument.

Re: [Tarantool-patches] [PATCH 2.X v2.1 1/4] module api: export box_tuple_validate

[Timur Safin]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X v2.1 1/4] module api: export
: box_tuple_validate
: 
: Hi! Thanks for the patch!
: 
: On 11.10.2020 16:39, Timur Safin wrote:
: > For external merger we need means to validate tuple data,
: > thus exporting `box_tuple_validate` which is wrapper around
: > `tuple_validate_raw` without revealing access to tuple
: > internals.
: >
: > Part of #5384
: > ---
: >  src/box/tuple.c |  6 ++++++
: >  src/box/tuple.h | 11 +++++++++++
: >  src/exports.h   |  1 +
: >  3 files changed, 18 insertions(+)
: >
: > diff --git a/src/box/tuple.c b/src/box/tuple.c
: > index f3965476e..ddf41567c 100644
: > --- a/src/box/tuple.c
: > +++ b/src/box/tuple.c
: > @@ -748,6 +748,12 @@ box_tuple_new(box_tuple_format_t *format, const
: char *data, const char *end)
: >  	return tuple_bless(ret);
: >  }
: >
: > +int
: > +box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple)
: 
: If a function is a method of an object, it should have the object
: name in prefix, and have the object pointer in the first argument.
: For example, box_tuple_to_buf(), box_tuple_field(), box_tuple_update(),
: box_tuple_upsert().
: 
: So here the tuple should be the first argument.

Indeed, that makes sense. 

[Tarantool-patches] [PATCH 2.X v2.1 2/4] module api: export box_key_def_dup

[Timur Safin]

Exporting `box_key_def_dup` as accessor to the internal `key_def_dup`

Part of #5384
---
 src/box/key_def_api.c |  6 ++++++
 src/box/key_def_api.h | 10 ++++++++++
 src/exports.h         |  1 +
 3 files changed, 17 insertions(+)

diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 2555b9fdd..98602ff24 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -199,6 +199,12 @@ box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count)
 	return key_def;
 }
 
+box_key_def_t *
+box_key_def_dup(const box_key_def_t *key_def)
+{
+	return key_def_dup(key_def);
+}
+
 void
 box_key_def_delete(box_key_def_t *key_def)
 {
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index 8dd6eb10b..a419b712c 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -173,6 +173,16 @@ box_key_part_def_create(box_key_part_def_t *part);
 API_EXPORT box_key_def_t *
 box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count);
 
+/**
+ * Duplicate key_def.
+ * \param key_def Original key_def.
+ *
+ * \retval not NULL Duplicate of src.
+ * \retval     NULL Memory error.
+ */
+box_key_def_t *
+box_key_def_dup(const box_key_def_t *key_def);
+
 /**
  * Delete key definition
  *
diff --git a/src/exports.h b/src/exports.h
index 592b388bb..202f5bf19 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -31,6 +31,7 @@ EXPORT(box_iterator_free)
 EXPORT(box_iterator_next)
 EXPORT(box_key_def_delete)
 EXPORT(box_key_def_dump_parts)
+EXPORT(box_key_def_dup)
 EXPORT(box_key_def_merge)
 EXPORT(box_key_def_new)
 EXPORT(box_key_def_new_ex)
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X v2.1 3/4] module api: luaL_checkibuf

[Timur Safin]

Moved `luaL_checkibuf` to the public part of module api.

Part of #5384
---
 src/exports.h   |  1 +
 src/lua/utils.h | 16 ++++++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/exports.h b/src/exports.h
index 202f5bf19..aa1cc6a4d 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -348,6 +348,7 @@ EXPORT(luaL_callmeta)
 EXPORT(luaL_cdef)
 EXPORT(luaL_checkany)
 EXPORT(luaL_checkcdata)
+EXPORT(luaL_checkibuf)
 EXPORT(luaL_checkint64)
 EXPORT(luaL_checkinteger)
 EXPORT(luaL_checklstring)
diff --git a/src/lua/utils.h b/src/lua/utils.h
index e9dd27d08..d05319f97 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -539,6 +539,14 @@ luaT_tolstring(lua_State *L, int idx, size_t *ssize);
 LUA_API int
 luaL_iscallable(lua_State *L, int idx);
 
+/**
+ * Check if a value on @a L stack by index @a idx is an ibuf
+ * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
+ * Returns NULL, if can't convert - not an ibuf object.
+ */
+struct ibuf *
+luaL_checkibuf(struct lua_State *L, int idx);
+
 /** \endcond public */
 
 /**
@@ -628,14 +636,6 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg,
 struct lua_State *
 luaT_newthread(struct lua_State *L);
 
-/**
- * Check if a value on @a L stack by index @a idx is an ibuf
- * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
- * Returns NULL, if can't convert - not an ibuf object.
- */
-struct ibuf *
-luaL_checkibuf(struct lua_State *L, int idx);
-
 /**
  * Check if a value on @a L stack by index @a idx is pointer at
  * char or const char. '(char *)NULL' is also considered a valid
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X v2.1 4/4] module api: external merger tests

[Timur Safin]

Added few simple tests for newly introduced
module api functions:
- luaL_checkibuf
- box_tuple_validate

Part of #5384
---
 test/app-tap/module_api.c        | 26 ++++++++++++++++++
 test/app-tap/module_api.test.lua | 45 +++++++++++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index a79fbed0d..fdf1c2d62 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -150,6 +150,15 @@ test_checkint64(lua_State *L)
 	return 1;
 }
 
+static int
+test_checkibuf(lua_State *L)
+{
+	struct ibuf *buf;
+	buf = luaL_checkibuf(L, -1);
+	lua_pushboolean(L, buf != NULL);
+	return 1;
+}
+
 static int
 test_touint64(lua_State *L)
 {
@@ -451,6 +460,21 @@ test_iscallable(lua_State *L)
 	return 1;
 }
 
+static int
+test_tuple_validate(lua_State *L)
+{
+	int valid = 0;
+	box_tuple_t *tuple = luaT_istuple(L, -1);
+
+	if (tuple != NULL) {
+		box_tuple_format_t *format = box_tuple_format_default();
+		valid = box_tuple_validate(format, tuple) == 0;
+	}
+	lua_pushboolean(L, valid);
+
+	return 1;
+}
+
 LUA_API int
 luaopen_module_api(lua_State *L)
 {
@@ -464,6 +488,7 @@ luaopen_module_api(lua_State *L)
 		{"test_pushint64", test_pushint64 },
 		{"test_checkuint64", test_checkuint64 },
 		{"test_checkint64", test_checkint64 },
+		{"checkibuf", test_checkibuf},
 		{"test_touint64", test_touint64 },
 		{"test_toint64", test_toint64 },
 		{"test_fiber", test_fiber },
@@ -479,6 +504,7 @@ luaopen_module_api(lua_State *L)
 		{"test_state", test_state},
 		{"test_tostring", test_tostring},
 		{"iscallable", test_iscallable},
+		{"tuple_validate", test_tuple_validate},
 		{NULL, NULL}
 	};
 	luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index a6658cc61..12a3ab5ca 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -38,6 +38,47 @@ local function test_pushcdata(test, module)
     test:is(gc_counter, 1, 'pushcdata gc')
 end
 
+local function test_buffers(test, module)
+    test:plan(8)
+    local ffi = require('ffi')
+    local buffer = require('buffer')
+
+    local bufalloc = buffer.static_alloc("char", 128)
+    local ibuf = buffer.ibuf()
+    local pbuf = ibuf:alloc(128)
+
+    test:ok(not module.checkibuf(nil), 'checkibuf of nil')
+    test:ok(not module.checkibuf({}), 'checkibuf of {}')
+    test:ok(not module.checkibuf(1LL), 'checkibuf of 1LL')
+    test:ok(not module.checkibuf(box.NULL), 'checkibuf of box.NULL')
+    test:ok(not module.checkibuf(buffer.reg1), 'checkibuf of reg1')
+    test:ok(not module.checkibuf(bufalloc), 'checkibuf of allocated buffer')
+    test:ok(module.checkibuf(ibuf), 'checkibuf of ibuf')
+    test:ok(not module.checkibuf(pbuf), 'checkibuf of pointer to ibuf data')
+end
+
+local function test_tuples(test, module)
+    test:plan(8)
+
+    local nottuple1 = {}
+    local nottuple2 = {1, 2}
+    local nottuple3 = {1, nil, 2}
+    local nottuple4 = {1, box.NULL, 2, 3}
+    local tuple1 = box.tuple.new(nottuple1)
+    local tuple2 = box.tuple.new(nottuple2)
+    local tuple3 = box.tuple.new(nottuple3)
+    local tuple4 = box.tuple.new(nottuple4)
+
+    test:ok(not module.tuple_validate(nottuple1), "not tuple 1")
+    test:ok(not module.tuple_validate(nottuple2), "not tuple 2")
+    test:ok(not module.tuple_validate(nottuple3), "not tuple 3")
+    test:ok(not module.tuple_validate(nottuple4), "not tuple 4")
+    test:ok(module.tuple_validate(tuple1), "tuple 1")
+    test:ok(module.tuple_validate(tuple2), "tuple 2")
+    test:ok(module.tuple_validate(tuple3), "tuple 3")
+    test:ok(module.tuple_validate(tuple4), "tuple 4")
+end
+
 local function test_iscallable(test, module)
     local ffi = require('ffi')
 
@@ -117,7 +158,7 @@ local function test_iscallable(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(24)
+    test:plan(26)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
@@ -143,6 +184,8 @@ local test = require('tap').test("module_api", function(test)
 
     test:test("pushcdata", test_pushcdata, module)
     test:test("iscallable", test_iscallable, module)
+    test:test("buffers", test_buffers, module)
+    test:test("validate", test_tuples, module)
 
     space:drop()
 end)
-- 
2.20.1