[Tarantool-patches] [PATCH 2.X v3 0/3] module api: extend for external merger Lua module

[Tarantool-patches] [PATCH 2.X v3 0/3] 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.

Changelog for v3
----------------
- Reshuffled patchset to merge tests with their corresponding code;

I'm sending this version because it's too late already and I need to sleep...

Plans for v3.1
--------------
- Add ibuf wrapper, despite all controversies
  it's coming soon...


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-v3
  (top 3 commits above of a bunch of @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 (3):
  module api: export box_tuple_validate
  module api: export box_key_def_dup
  module api: luaL_checkibuf

 src/box/key_def.c                |  6 +++++
 src/box/key_def.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 v3 1/3] 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 +
 test/app-tap/module_api.c        | 16 ++++++++++++++++
 test/app-tap/module_api.test.lua | 25 ++++++++++++++++++++++++-
 5 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/src/box/tuple.c b/src/box/tuple.c
index f3965476e..6fce4143e 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_t *tuple, box_tuple_format_t *format)
+{
+	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..755aee506 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 tuple  Tuple to validate.
+ * @param format Format to which the tuple must match.
+ *
+ * @retval  0 The tuple is valid.
+ * @retval -1 The tuple is invalid.
+ */
+int
+box_tuple_validate(box_tuple_t *tuple, box_tuple_format_t *format);
+
 /** \endcond public */
 
 /**
diff --git a/src/exports.h b/src/exports.h
index 1d7deb518..051818ef7 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -78,6 +78,7 @@ EXPORT(box_tuple_to_buf)
 EXPORT(box_tuple_unref)
 EXPORT(box_tuple_update)
 EXPORT(box_tuple_upsert)
+EXPORT(box_tuple_validate)
 EXPORT(box_txn)
 EXPORT(box_txn_alloc)
 EXPORT(box_txn_begin)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 8b46cdb0d..7d8941bfb 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -1155,6 +1155,21 @@ test_tuple_new(struct lua_State *L)
 
 /* }}} test_tuple_new */
 
+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(tuple, format) == 0;
+        }
+	lua_pushboolean(L, valid);
+
+	return 1;
+}
+
 LUA_API int
 luaopen_module_api(lua_State *L)
 {
@@ -1190,6 +1205,7 @@ luaopen_module_api(lua_State *L)
 		{"test_key_def_new_v2", test_key_def_new_v2},
 		{"test_key_def_dump_parts", test_key_def_dump_parts},
 		{"test_key_def_validate_tuple", test_key_def_validate_tuple},
+		{"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 cfb0ca00b..6797f8150 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -38,6 +38,28 @@ local function test_pushcdata(test, module)
     test:is(gc_counter, 1, 'pushcdata gc')
 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')
 
@@ -172,7 +194,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(31)
+    test:plan(32)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
@@ -199,6 +221,7 @@ local test = require('tap').test("module_api", function(test)
     test:test("pushcdata", test_pushcdata, module)
     test:test("iscallable", test_iscallable, module)
     test:test("iscdata", test_iscdata, module)
+    test:test("validate", test_tuples, module)
 
     space:drop()
 end)
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X v3 2/3] 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.c |  6 ++++++
 src/box/key_def.h | 10 ++++++++++
 src/exports.h     |  1 +
 3 files changed, 17 insertions(+)

diff --git a/src/box/key_def.c b/src/box/key_def.c
index af4e8f4cd..322a62046 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -507,6 +507,12 @@ box_key_def_new_v2(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.h b/src/box/key_def.h
index 9222c9240..dccad78db 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -437,6 +437,16 @@ box_key_part_def_create(box_key_part_def_t *part);
 API_EXPORT box_key_def_t *
 box_key_def_new_v2(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.
+ */
+API_EXPORT 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 051818ef7..ff0660e24 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_extract_key)
 EXPORT(box_key_def_merge)
 EXPORT(box_key_def_new)
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X v3 3/3] 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 ++++++++--------
 test/app-tap/module_api.c        | 10 ++++++++++
 test/app-tap/module_api.test.lua | 22 +++++++++++++++++++++-
 4 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/src/exports.h b/src/exports.h
index ff0660e24..034b6c4a3 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 e80e2b1a2..7658c67f8 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
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 7d8941bfb..502d734d2 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -155,6 +155,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)
 {
@@ -1183,6 +1192,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 },
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 6797f8150..17f4ff477 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -38,6 +38,25 @@ 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)
 
@@ -194,7 +213,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(32)
+    test:plan(33)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
@@ -221,6 +240,7 @@ local test = require('tap').test("module_api", function(test)
     test:test("pushcdata", test_pushcdata, module)
     test:test("iscallable", test_iscallable, module)
     test:test("iscdata", test_iscdata, module)
+    test:test("buffers", test_buffers, module)
     test:test("validate", test_tuples, module)
 
     space:drop()
-- 
2.20.1

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

[Alexander Turenko]

> +int
> +box_tuple_validate(box_tuple_t *tuple, box_tuple_format_t *format)
> +{
> +	return tuple_validate_raw(format, tuple_data(tuple));
> +}
> +

I would invoke tuple_validate() here.

> +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(tuple, format) == 0;
> +        }

Tab / spaces mix.

All tuples are valid against the runtime (default) format. For the sake
of minimal testing I would create a format with at least one specified
field and check a tuple that is valid and one that is invalid. You can
use box_tuple_format_new() to create a format, see example in
test_key_def_api().

> +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}

What is the purpose? You test your test_tuple_validate() wrapper here.

> @@ -199,6 +221,7 @@ local test = require('tap').test("module_api", function(test)
>      test:test("pushcdata", test_pushcdata, module)
>      test:test("iscallable", test_iscallable, module)
>      test:test("iscdata", test_iscdata, module)
> +    test:test("validate", test_tuples, module)

Nit: Feels a bit inconsistent: either validate + test_validate or
tuples + test_tuples or any other <foo> + test_<foo>. I would borrow the
name from the function we test: tuple_validate + test_tuple_validate.

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

[Timur Safin]

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [PATCH 2.X v3 1/3] module api: export box_tuple_validate
: 
: > +int
: > +box_tuple_validate(box_tuple_t *tuple, box_tuple_format_t *format)
: > +{
: > +	return tuple_validate_raw(format, tuple_data(tuple));
: > +}
: > +
: 
: I would invoke tuple_validate() here.

Yup, indeed. Good point.

: 
: > +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(tuple, format) == 0;
: > +        }
: 
: Tab / spaces mix.

Indeed. I have found it later during further ibuf tests additions. 
Will fix.

: 
: All tuples are valid against the runtime (default) format. For the sake
: of minimal testing I would create a format with at least one specified
: field and check a tuple that is valid and one that is invalid. You can
: use box_tuple_format_new() to create a format, see example in
: test_key_def_api().

Will add some format there.

: 
: > +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}
: 
: What is the purpose? You test your test_tuple_validate() wrapper here.

:) at least it distinguish Tarantool tuple objects from generic Lua tables.

: 
: > @@ -199,6 +221,7 @@ local test = require('tap').test("module_api",
: function(test)
: >      test:test("pushcdata", test_pushcdata, module)
: >      test:test("iscallable", test_iscallable, module)
: >      test:test("iscdata", test_iscdata, module)
: > +    test:test("validate", test_tuples, module)
: 
: Nit: Feels a bit inconsistent: either validate + test_validate or
: tuples + test_tuples or any other <foo> + test_<foo>. I would borrow the
: name from the function we test: tuple_validate + test_tuple_validate.

Will streamline name. Thanks

Timur

Re: [Tarantool-patches] [PATCH 2.X v3 2/3] module api: export box_key_def_dup

[Alexander Turenko]

Everything is okay, but I would add a test case. You can use the
box_key_def_dump_parts() function + the key_part_def_check_equal()
helper to verify equality of two key_defs or create your own wrapper for
two key_defs similar to key_def_check_merge() (see the recent branch re
key_def APIs, I have updated it today).

On Mon, Oct 12, 2020 at 03:44:10AM +0300, Timur Safin wrote:
> Exporting `box_key_def_dup` as accessor to the internal `key_def_dup`
> 
> Part of #5384

Re: [Tarantool-patches] [PATCH 2.X v3 3/3] module api: luaL_checkibuf

[Alexander Turenko]

I CCed Igor, because he is maybe interested about the topic regarding
naming of Lua/C helpers.

Answered inline.

WBR, Alexander Turenko.

> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index e80e2b1a2..7658c67f8 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);
> +

First, some background: luaL_checkfoo() raise a Lua error when type
checking fails. lua_isfoo() returns zero in such case, lua_tofoo()
return NULL. IMHO, lua_isfoo() / lua_tofoo() is often more convenient
for Lua/C code, because you may need to free some resources explicitly.
It also allows to return / raise a domain specific error (say, give
short usage information).

 | [-0, +0, -]
 | int lua_isuserdata (lua_State *L, int index);
 |
 | Returns 1 if the value at the given acceptable index is a userdata
 | (either full or light), and 0 otherwise. 

 | [-0, +0, -]
 | void *lua_touserdata (lua_State *L, int index);
 |
 | If the value at the given acceptable index is a full userdata,
 | returns its block address. If the value is a light userdata, returns
 | its pointer. Otherwise, returns NULL. 

You may find more examples in https://pgl.yoyo.org/luai/i/_

So I would either implement `int luaT_isibuf(L, idx, box_ibuf_t **)` or
rename the function to `box_ibuf_t *luaT_toibuf(L, idx)`.

Regarding prefixes: we use 'luaL_' for general purpose functions, but
'luaT_' for tarantool specific objects. ibuf is tarantool's thing.

BTW, Vlad tells me it possibly should be named as 'box ibuf', not just
'ibuf'. However the name would be a bit ugly (luaT_is_box_ibuf()), so I
guess 'luaT_' prefix is enough to say that it is something tarantool
specific.

BTW, it seems the patch re ibuf functions should come before this one to
use the public type name (box_ibuf_t) in the declaration.

> +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()

Let's also check <struct ibuf *>.

 | tarantool> ffi.typeof(buffer.ibuf())
 | ---
 | - ctype<struct ibuf>
 | ...
 | 
 | tarantool> ffi.typeof(buffer.IBUF_SHARED)
 | ---
 | - ctype<struct ibuf *>
 | ...

[Tarantool-patches] [PATCH 2.X v3] module api: box_ibuf_* wrappers

[Timur Safin]

Introduced the bare minimum of ibuf accessors, which make
external merger possible:
- box_ibuf_reserve
- box_ibuf_read_range
- box_ibuf_write_range

Part of #5384
---

 Vlad, Sasha, I'm sending this patch as a follow-up to v3 of patchset, to make
 possible early review, before other patches in patchset have been
 finally polished.

 This pretty much compatible to that we discussed early on in Zoom (though...
 some extra, awkward level of indirection was introduced, but I had to)


 src/CMakeLists.txt               |  1 +
 src/box/CMakeLists.txt           |  1 +
 src/box/box_ibuf.c               | 62 +++++++++++++++++++++++++++++
 src/box/box_ibuf.h               | 68 ++++++++++++++++++++++++++++++++
 src/exports.h                    |  3 ++
 test/app-tap/module_api.c        | 34 ++++++++++++++++
 test/app-tap/module_api.test.lua |  2 +-
 7 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100644 src/box/box_ibuf.c
 create mode 100644 src/box/box_ibuf.h

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 699536652..0cff406e0 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -152,6 +152,7 @@ set(api_headers
     ${CMAKE_SOURCE_DIR}/src/box/tuple_extract_key.h
     ${CMAKE_SOURCE_DIR}/src/box/schema_def.h
     ${CMAKE_SOURCE_DIR}/src/box/box.h
+    ${CMAKE_SOURCE_DIR}/src/box/box_ibuf.h
     ${CMAKE_SOURCE_DIR}/src/box/index.h
     ${CMAKE_SOURCE_DIR}/src/box/iterator_type.h
     ${CMAKE_SOURCE_DIR}/src/box/error.h
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 8b2e704cf..07d10dae8 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -191,6 +191,7 @@ add_library(box STATIC
     wal.c
     call.c
     merger.c
+    box_ibuf.c
     ${sql_sources}
     ${lua_sources}
     lua/init.c
diff --git a/src/box/box_ibuf.c b/src/box/box_ibuf.c
new file mode 100644
index 000000000..5cf3ab711
--- /dev/null
+++ b/src/box/box_ibuf.c
@@ -0,0 +1,62 @@
+/*
+ * Copyright 2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <stdlib.h>
+#include "box_ibuf.h"
+
+void *
+box_ibuf_reserve(box_ibuf_t *ibuf, size_t size)
+{
+	return ibuf_reserve(ibuf, size);
+}
+
+void
+box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
+{
+	if (!ibuf)
+		return;
+	if (rpos)
+		*rpos = &ibuf->rpos;
+	if (wpos)
+		*wpos = &ibuf->wpos;
+}
+
+void
+box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end)
+{
+	if (!ibuf)
+		return;
+	if (wpos)
+		*wpos = &ibuf->wpos;
+	if (end)
+		*end = &ibuf->end;
+
+}
diff --git a/src/box/box_ibuf.h b/src/box/box_ibuf.h
new file mode 100644
index 000000000..235b87560
--- /dev/null
+++ b/src/box/box_ibuf.h
@@ -0,0 +1,68 @@
+#pragma once
+/*
+ * Copyright 2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <stddef.h>
+
+#include <trivia/util.h>
+#include <small/ibuf.h>
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+/** \cond public */
+
+typedef struct ibuf box_ibuf_t;
+
+/**
+ * Reserve requested amount of bytes in ibuf buffer
+ */
+API_EXPORT void *
+box_ibuf_reserve(box_ibuf_t *ibuf, size_t size);
+
+/**
+ * Return pointers to read range pointers used [rpos..wpos)
+ */
+API_EXPORT void
+box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos);
+
+/**
+ * Return pointers to write range pointers used [wpos..end)
+ */
+API_EXPORT void
+box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end);
+
+/** \endcond public */
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
diff --git a/src/exports.h b/src/exports.h
index 034b6c4a3..1342d34c4 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -17,6 +17,9 @@ EXPORT(box_error_last)
 EXPORT(box_error_message)
 EXPORT(box_error_set)
 EXPORT(box_error_type)
+EXPORT(box_ibuf_read_range)
+EXPORT(box_ibuf_reserve)
+EXPORT(box_ibuf_write_range)
 EXPORT(box_index_bsize)
 EXPORT(box_index_count)
 EXPORT(box_index_get)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 502d734d2..a681826bf 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -164,6 +164,39 @@ test_checkibuf(lua_State *L)
 	return 1;
 }
 
+static int
+test_box_ibuf(lua_State *L)
+{
+	(void)L;
+	struct slab_cache *slabc = cord_slab_cache();
+	assert(slabc != NULL);
+	box_ibuf_t ibuf;
+
+	ibuf_create(&ibuf, slabc, 16320);
+	assert(ibuf_used(&ibuf) == 0);
+	box_ibuf_reserve(&ibuf, 65536);
+	char **rpos;
+	char **wpos;
+	box_ibuf_read_range(&ibuf, &rpos, &wpos);
+
+	void *ptr = ibuf_alloc(&ibuf, 10);
+	assert(ptr != NULL);
+
+	assert(ibuf_used(&ibuf) == 10);
+	assert((*wpos - *rpos) == 10);
+
+	ptr = ibuf_alloc(&ibuf, 10000);
+	assert(ptr);
+	assert(ibuf_used(&ibuf) == 10010);
+	assert((*wpos - *rpos) == 10010);
+
+	ibuf_reset(&ibuf);
+	assert(ibuf_used(&ibuf) == 0);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
 static int
 test_touint64(lua_State *L)
 {
@@ -1216,6 +1249,7 @@ luaopen_module_api(lua_State *L)
 		{"test_key_def_dump_parts", test_key_def_dump_parts},
 		{"test_key_def_validate_tuple", test_key_def_validate_tuple},
 		{"tuple_validate", test_tuple_validate},
+		{"test_box_ibuf", test_box_ibuf},
 		{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 17f4ff477..ff64bc36c 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -213,7 +213,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(33)
+    test:plan(34)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 2.X v3] module api: box_ibuf_* wrappers

[Alexander Turenko]

>  create mode 100644 src/box/box_ibuf.c
>  create mode 100644 src/box/box_ibuf.h

I would name it just src/box/ibuf.[ch].

> +void
> +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
> +{
> +	if (!ibuf)
> +		return;

I would assert on this.

> +	if (rpos)
> +		*rpos = &ibuf->rpos;
> +	if (wpos)
> +		*wpos = &ibuf->wpos;

We use explicit == 0 / == NULL checks across our code.

> index 000000000..235b87560
> --- /dev/null
> +++ b/src/box/box_ibuf.h
> @@ -0,0 +1,68 @@
> +#pragma once
> +/*
> <...>
> +
> +#include <stddef.h>

It defines NULL and size_t as well as stdlib.h, but you include stdlib.h
in the .c file, so I would use the same header in both. Not sure which
one should be preferred.

> +
> +#include <trivia/util.h>
> +#include <small/ibuf.h>

How about just define an opaque structure?

 | struct ibuf;

But include small/ibuf.h explicitly in the .c file?

> +static int
> +test_box_ibuf(lua_State *L)
> +{
> +	(void)L;

There is usage of L at end of the function.

> +	struct slab_cache *slabc = cord_slab_cache();
> +	assert(slabc != NULL);
> +	box_ibuf_t ibuf;
> +
> +	ibuf_create(&ibuf, slabc, 16320);
> +	assert(ibuf_used(&ibuf) == 0);
> +	box_ibuf_reserve(&ibuf, 65536);
> +	char **rpos;
> +	char **wpos;
> +	box_ibuf_read_range(&ibuf, &rpos, &wpos);
> +
> +	void *ptr = ibuf_alloc(&ibuf, 10);
> +	assert(ptr != NULL);
> +
> +	assert(ibuf_used(&ibuf) == 10);
> +	assert((*wpos - *rpos) == 10);

Now box_ibuf_read_range() should give the updated wpos.

> +
> +	ptr = ibuf_alloc(&ibuf, 10000);
> +	assert(ptr);
> +	assert(ibuf_used(&ibuf) == 10010);
> +	assert((*wpos - *rpos) == 10010);

Same here.

> +
> +	ibuf_reset(&ibuf);
> +	assert(ibuf_used(&ibuf) == 0);

I would test box_ibuf_write_range() as well.

The test per se is more like how internal ibuf operations are
interoperate with the public API rathen than a unit test of the public
API. However it is okay, it'll also do the work and I would not bother
much now.

Re: [Tarantool-patches] [PATCH 2.X v3] module api: box_ibuf_* wrappers

[Timur Safin]

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [PATCH 2.X v3] module api: box_ibuf_* wrappers
: 
: >  create mode 100644 src/box/box_ibuf.c
: >  create mode 100644 src/box/box_ibuf.h
: 
: I would name it just src/box/ibuf.[ch].

Worth to note that version I've shown in Zoom did have ibuf.[hc]
but I've got an impression that Vlad had some complain against
it. 

: 
: > +void
: > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
: > +{
: > +	if (!ibuf)
: > +		return;
: 
: I would assert on this.

Will add - good point.

: 
: > +	if (rpos)
: > +		*rpos = &ibuf->rpos;
: > +	if (wpos)
: > +		*wpos = &ibuf->wpos;
: 
: We use explicit == 0 / == NULL checks across our code.

It's already so in the branch - didn't send update.

: 
: > index 000000000..235b87560
: > --- /dev/null
: > +++ b/src/box/box_ibuf.h
: > @@ -0,0 +1,68 @@
: > +#pragma once
: > +/*
: > <...>
: > +
: > +#include <stddef.h>
: 
: It defines NULL and size_t as well as stdlib.h, but you include stdlib.h
: in the .c file, so I would use the same header in both. Not sure which
: one should be preferred.
: 
: > +
: > +#include <trivia/util.h>
: > +#include <small/ibuf.h>
: 
: How about just define an opaque structure?

Yeah, good point - we are introducing it here just to avoid to have 
explicit dependency on it in header.

: 
:  | struct ibuf;
: 
: But include small/ibuf.h explicitly in the .c file?
: 
: > +static int
: > +test_box_ibuf(lua_State *L)
: > +{
: > +	(void)L;
: 
: There is usage of L at end of the function.

Yeah, that was introduced here at the moment I didn't return 
any value :)

: 
: > +	struct slab_cache *slabc = cord_slab_cache();
: > +	assert(slabc != NULL);
: > +	box_ibuf_t ibuf;
: > +
: > +	ibuf_create(&ibuf, slabc, 16320);
: > +	assert(ibuf_used(&ibuf) == 0);
: > +	box_ibuf_reserve(&ibuf, 65536);
: > +	char **rpos;
: > +	char **wpos;
: > +	box_ibuf_read_range(&ibuf, &rpos, &wpos);
: > +
: > +	void *ptr = ibuf_alloc(&ibuf, 10);
: > +	assert(ptr != NULL);
: > +
: > +	assert(ibuf_used(&ibuf) == 10);
: > +	assert((*wpos - *rpos) == 10);
: 
: Now box_ibuf_read_range() should give the updated wpos.

Here I didn't get your point - wpos and rpos are pointers 
to fields inside of ibuf structure, they will not change 
regardless the number of calls to we perform. 

: 
: > +
: > +	ptr = ibuf_alloc(&ibuf, 10000);
: > +	assert(ptr);
: > +	assert(ibuf_used(&ibuf) == 10010);
: > +	assert((*wpos - *rpos) == 10010);
: 
: Same here.
: 
: > +
: > +	ibuf_reset(&ibuf);
: > +	assert(ibuf_used(&ibuf) == 0);
: 
: I would test box_ibuf_write_range() as well.
: 
: The test per se is more like how internal ibuf operations are
: interoperate with the public API rathen than a unit test of the public
: API. However it is okay, it'll also do the work and I would not bother
: much now.

The problem with the current API we have exposed - it's not 
self-contained, you could not do much with it, without calling 
other (internal yet) ibuf_* calls. That's why I essentially have 
copied here unit test from small but checked consistency of small
calls with results we could retrieve using newer api.

Regards,
Timur

Re: [Tarantool-patches] [PATCH 2.X v3 3/3] module api: luaL_checkibuf

[Igor Munkin]

Sasha,

On 13.10.20, Alexander Turenko wrote:
> I CCed Igor, because he is maybe interested about the topic regarding
> naming of Lua/C helpers.

Totally fine with the one you proposed (i.e. <luaT_toibuf>).

> 
> Answered inline.
> 
> WBR, Alexander Turenko.
> 

<snipped>

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2.X v3] module api: box_ibuf_* wrappers

[Alexander Turenko]

> : > +
> : > +#include <trivia/util.h>
> : > +#include <small/ibuf.h>
> : 
> : How about just define an opaque structure?
> 
> Yeah, good point - we are introducing it here just to avoid to have 
> explicit dependency on it in header.

Hm. It is out of /* \cond public */ and /* \endcond public */ markers,
so it's fine from this point of view. But using of `struct ibuf;` in the
header instead of `#include <small/ibuf.h>` should decrease the compile
time a bit. In fact, the time difference is negligible, so the point is
more regarding rules of thumb and feeling about the code as 'clean'.

> 
> : 
> :  | struct ibuf;
> : 
> : But include small/ibuf.h explicitly in the .c file?
> : 

(Cited for convenience.)

> : > +	struct slab_cache *slabc = cord_slab_cache();
> : > +	assert(slabc != NULL);
> : > +	box_ibuf_t ibuf;
> : > +
> : > +	ibuf_create(&ibuf, slabc, 16320);
> : > +	assert(ibuf_used(&ibuf) == 0);
> : > +	box_ibuf_reserve(&ibuf, 65536);
> : > +	char **rpos;
> : > +	char **wpos;
> : > +	box_ibuf_read_range(&ibuf, &rpos, &wpos);
> : > +
> : > +	void *ptr = ibuf_alloc(&ibuf, 10);
> : > +	assert(ptr != NULL);
> : > +
> : > +	assert(ibuf_used(&ibuf) == 10);
> : > +	assert((*wpos - *rpos) == 10);
> : 
> : Now box_ibuf_read_range() should give the updated wpos.
> 
> Here I didn't get your point - wpos and rpos are pointers 
> to fields inside of ibuf structure, they will not change 
> regardless the number of calls to we perform. 

Let's show a sketch:

 | box_ibuf_read_range(&ibuf, &rpos, &wpos);
 | <...do some allocations...>
 | assert((*wpos - *rpos) == 10);

This test case verifies that the positions we obtained before the
allocations are valid after them. I proposed to also verify that
box_ibuf_read_range() will correctly return the positions after the
allocations:

 | box_ibuf_read_range(&ibuf, &rpos, &wpos);
 | <...do some allocations...>
 | assert((*wpos - *rpos) == 10);
 |
 | /* Renew positions and verify them. */
 | box_ibuf_read_range(&ibuf, &rpos, &wpos);
 | assert((*wpos - *rpos) == 10);

> 
> : 
> : > +
> : > +	ptr = ibuf_alloc(&ibuf, 10000);
> : > +	assert(ptr);
> : > +	assert(ibuf_used(&ibuf) == 10010);
> : > +	assert((*wpos - *rpos) == 10010);
> : 
> : Same here.

(Cited for convenience.)