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

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

[Timur Safin]

Blah-blah-blah (see v1..v3 introductions), this version is hopefully looking 
close to final. Even, all patches have been completed with reasonable tests.

Enjoy!

SashaTu, Vlad, please pay some attention!

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...

Changelog v3.1
--------------
- Added ibuf wrapper, the simpler way (only reserve, and rpos, wpos accessors)

Changelog v4
------------
- final polishing of examples
- tests for luaT_toibuf, with ibuf consistency checks
- tests for key_def_dup
- tests for tuple_validate

Hope it's good enough for feature freeze...


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-v4
  (top 4 commits above of a bunch of @Totktonada's commits)


Timur Safin (4):
  module api: export box_tuple_validate
  module api: export box_key_def_dup
  module api: introduced luaT_toibuf instead of luaL_checkibuf
  module api: box_ibuf_* wrappers

 src/CMakeLists.txt               |   1 +
 src/box/CMakeLists.txt           |   1 +
 src/box/ibuf.c                   |  65 +++++++++++++++
 src/box/ibuf.h                   |  67 ++++++++++++++++
 src/box/key_def.c                |   6 ++
 src/box/key_def.h                |  10 +++
 src/box/lua/merger.c             |   4 +-
 src/box/tuple.c                  |   6 ++
 src/box/tuple.h                  |  11 +++
 src/exports.h                    |   6 ++
 src/lua/msgpack.c                |   2 +-
 src/lua/utils.c                  |   2 +-
 src/lua/utils.h                  |  16 ++--
 test/app-tap/module_api.c        | 131 +++++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua |  50 +++++++++++-
 15 files changed, 365 insertions(+), 13 deletions(-)
 create mode 100644 src/box/ibuf.c
 create mode 100644 src/box/ibuf.h

-- 
2.20.1

[Tarantool-patches] [PATCH 2.X v4 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` 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        | 48 ++++++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua | 29 ++++++++++++++++++-
 5 files changed, 94 insertions(+), 1 deletion(-)

diff --git a/src/box/tuple.c b/src/box/tuple.c
index f3965476e..e83377c3a 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(format, 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..361b75873 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -1155,6 +1155,52 @@ test_tuple_new(struct lua_State *L)
 
 /* }}} test_tuple_new */
 
+/*
+ * Check that argument is a tuple of any format, without
+ * its verification
+ */
+static int
+test_tuple_validate_default(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;
+}
+
+/*
+ * Validate tuple with format of single boolean field
+ */
+static int
+test_tuple_validate_formatted(lua_State *L)
+{
+	int valid = 0;
+	box_tuple_t *tuple = luaT_istuple(L, -1);
+
+	if (tuple != NULL) {
+		uint32_t fields[] = { 0 };
+		uint32_t types[] = { FIELD_TYPE_BOOLEAN };
+		box_key_def_t *key_defs[] = {
+			box_key_def_new(fields, types, 1)
+		};
+		assert(key_defs[0] != NULL);
+		struct tuple_format *format =
+			box_tuple_format_new(key_defs, 1);
+		assert(format);
+
+		valid = box_tuple_validate(tuple, format) == 0;
+	}
+	lua_pushboolean(L, valid);
+
+	return 1;
+}
+
 LUA_API int
 luaopen_module_api(lua_State *L)
 {
@@ -1190,6 +1236,8 @@ 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_def", test_tuple_validate_default},
+		{"tuple_validate_fmt", test_tuple_validate_formatted},
 		{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..d250580cf 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -38,6 +38,32 @@ local function test_pushcdata(test, module)
     test:is(gc_counter, 1, 'pushcdata gc')
 end
 
+local function test_tuple_validate(test, module)
+    test:plan(12)
+
+    local nottuple1 = {}
+    local nottuple2 = {true, 2}
+    local nottuple3 = {false, 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_def(nottuple1), "not tuple 1")
+    test:ok(not module.tuple_validate_def(nottuple2), "not tuple 2")
+    test:ok(not module.tuple_validate_def(nottuple3), "not tuple 3")
+    test:ok(not module.tuple_validate_def(nottuple4), "not tuple 4")
+    test:ok(module.tuple_validate_def(tuple1), "tuple 1")
+    test:ok(module.tuple_validate_def(tuple2), "tuple 2")
+    test:ok(module.tuple_validate_def(tuple3), "tuple 3")
+    test:ok(module.tuple_validate_def(tuple4), "tuple 4")
+    test:ok(not module.tuple_validate_fmt(tuple1), "tuple 1 (fmt)")
+    test:ok(module.tuple_validate_fmt(tuple2), "tuple 2 (fmt)")
+    test:ok(module.tuple_validate_fmt(tuple3), "tuple 3 (fmt)")
+    test:ok(not module.tuple_validate_fmt(tuple4), "tuple 4 (fmt)")
+end
+
 local function test_iscallable(test, module)
     local ffi = require('ffi')
 
@@ -172,7 +198,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 +225,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("tuple_validate", test_tuple_validate, module)
 
     space:drop()
 end)
-- 
2.20.1

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

[Alexander Turenko]

LGTM for the patch.

Just few notes, feel free to ignore.

> +/*
> + * Validate tuple with format of single boolean field
> + */

Nits: /* -> /**; period at the end.

> +static int
> +test_tuple_validate_formatted(lua_State *L)
> +{
> +	int valid = 0;
> +	box_tuple_t *tuple = luaT_istuple(L, -1);
> +
> +	if (tuple != NULL) {
> +		uint32_t fields[] = { 0 };
> +		uint32_t types[] = { FIELD_TYPE_BOOLEAN };
> +		box_key_def_t *key_defs[] = {
> +			box_key_def_new(fields, types, 1)
> +		};
> +		assert(key_defs[0] != NULL);
> +		struct tuple_format *format =
> +			box_tuple_format_new(key_defs, 1);
> +		assert(format);
> +
> +		valid = box_tuple_validate(tuple, format) == 0;

Nit: I would call box_tuple_format_unref() and box_key_def_delete().

I know, it is just test, so you can ignore this comment. I'm personally
a bit afraid of using bad patterns without a proper comment even in
tests. What if someone will copy it somewhere..?

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

[Timur Safin]

Thanks! Will update test(s) in the follow-up patchset, not now.

Timur

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [PATCH 2.X v4 1/4] module api: export box_tuple_validate
: 
: LGTM for the patch.
: 
: Just few notes, feel free to ignore.
: 
: > +/*
: > + * Validate tuple with format of single boolean field
: > + */
: 
: Nits: /* -> /**; period at the end.
: 
: > +static int
: > +test_tuple_validate_formatted(lua_State *L)
: > +{
: > +	int valid = 0;
: > +	box_tuple_t *tuple = luaT_istuple(L, -1);
: > +
: > +	if (tuple != NULL) {
: > +		uint32_t fields[] = { 0 };
: > +		uint32_t types[] = { FIELD_TYPE_BOOLEAN };
: > +		box_key_def_t *key_defs[] = {
: > +			box_key_def_new(fields, types, 1)
: > +		};
: > +		assert(key_defs[0] != NULL);
: > +		struct tuple_format *format =
: > +			box_tuple_format_new(key_defs, 1);
: > +		assert(format);
: > +
: > +		valid = box_tuple_validate(tuple, format) == 0;
: 
: Nit: I would call box_tuple_format_unref() and box_key_def_delete().
: 
: I know, it is just test, so you can ignore this comment. I'm personally
: a bit afraid of using bad patterns without a proper comment even in
: tests. What if someone will copy it somewhere..?

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> 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.

The public functions should say in their comments if they
set a diag in case of an error. Here and in the other patches.
I didn't say it earlier, because didn't think of it. Realized
it today.

> + */
> +int
> +box_tuple_validate(box_tuple_t *tuple, box_tuple_format_t *format);

[Tarantool-patches] [PATCH 2.X v4 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.c                |  6 ++++++
 src/box/key_def.h                | 10 ++++++++++
 src/exports.h                    |  1 +
 test/app-tap/module_api.c        | 29 +++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua |  2 +-
 5 files changed, 47 insertions(+), 1 deletion(-)

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)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 361b75873..17048081f 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -810,6 +810,34 @@ test_key_def_validate_tuple(struct lua_State *L)
 	return 1;
 }
 
+static int
+test_key_def_dup(lua_State *L)
+{
+	box_key_def_t *key_def = NULL;
+	box_key_part_def_t part;
+	box_key_part_def_t *dump = NULL;
+	uint32_t dump_part_count = 0;
+
+	key_part_def_set_nondefault(&part);
+	key_def = box_key_def_new_v2(&part, 1);
+	assert(key_def != NULL);
+	box_key_def_t *key_def_dup = box_key_def_dup(key_def);
+	assert(key_def_dup != NULL);
+
+	dump = box_key_def_dump_parts(key_def_dup, &dump_part_count);
+	assert(dump != NULL);
+	assert(dump_part_count == 1);
+
+	key_part_def_check_equal(&part, &dump[0]);
+	key_part_def_check_zeros(&dump[0]);
+
+	box_key_def_delete(key_def_dup);
+	box_key_def_delete(key_def);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
 /* }}} key_def api v2 */
 
 static int
@@ -1238,6 +1266,7 @@ luaopen_module_api(lua_State *L)
 		{"test_key_def_validate_tuple", test_key_def_validate_tuple},
 		{"tuple_validate_def", test_tuple_validate_default},
 		{"tuple_validate_fmt", test_tuple_validate_formatted},
+		{"test_key_def_dup", test_key_def_dup},
 		{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 d250580cf..ab5c89c8c 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -198,7 +198,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")
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X v4 3/4] module api: introduced luaT_toibuf instead of luaL_checkibuf

[Timur Safin]

* made `luaL_checkibuf` public;
* renamed it to `luaT_toibuf` to follow naming convention
  (it's not raising error, and is casting to ibuf type).

Part of #5384
---
 src/box/lua/merger.c             |  4 ++--
 src/exports.h                    |  1 +
 src/lua/msgpack.c                |  2 +-
 src/lua/utils.c                  |  2 +-
 src/lua/utils.h                  | 16 ++++++++--------
 test/app-tap/module_api.c        | 10 ++++++++++
 test/app-tap/module_api.test.lua | 23 ++++++++++++++++++++++-
 7 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c
index 583946c4c..17e237902 100644
--- a/src/box/lua/merger.c
+++ b/src/box/lua/merger.c
@@ -503,7 +503,7 @@ luaL_merge_source_buffer_fetch_impl(struct merge_source_buffer *source,
 		source->ref = 0;
 	}
 	lua_pushvalue(L, -nresult + 1); /* Popped by luaL_ref(). */
-	source->buf = luaL_checkibuf(L, -1);
+	source->buf = luaT_toibuf(L, -1);
 	if (source->buf == NULL) {
 		diag_set(IllegalParams, "Expected <state>, <buffer>");
 		return -1;
@@ -1225,7 +1225,7 @@ lbox_merge_source_select(struct lua_State *L)
 		lua_pushstring(L, "buffer");
 		lua_gettable(L, 2);
 		if (!lua_isnil(L, -1)) {
-			if ((output_buffer = luaL_checkibuf(L, -1)) == NULL)
+			if ((output_buffer = luaT_toibuf(L, -1)) == NULL)
 				return lbox_merge_source_select_usage(L,
 					"buffer");
 		}
diff --git a/src/exports.h b/src/exports.h
index ff0660e24..9c56686db 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -415,6 +415,7 @@ EXPORT(luaT_istuple)
 EXPORT(luaT_pushtuple)
 EXPORT(luaT_state)
 EXPORT(luaT_tolstring)
+EXPORT(luaT_toibuf)
 EXPORT(luaT_tuple_encode)
 EXPORT(luaT_tuple_new)
 EXPORT(mp_char2escape)
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index 4c4ada39c..24b0d2ccd 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -356,7 +356,7 @@ lua_msgpack_encode(lua_State *L)
 
 	struct ibuf *buf;
 	if (index > 1) {
-		buf = luaL_checkibuf(L, 2);
+		buf = luaT_toibuf(L, 2);
 		if (buf == NULL) {
 			return luaL_error(L, "msgpack.encode: argument 2 "
 					  "must be of type 'struct ibuf'");
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 3e1c491f8..004ea8ba5 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1107,7 +1107,7 @@ luaL_iscallable(lua_State *L, int idx)
 }
 
 struct ibuf *
-luaL_checkibuf(struct lua_State *L, int idx)
+luaT_toibuf(struct lua_State *L, int idx)
 {
 	if (lua_type(L, idx) != LUA_TCDATA)
 		return NULL;
diff --git a/src/lua/utils.h b/src/lua/utils.h
index e80e2b1a2..5e902b94e 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 *
+luaT_toibuf(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 17048081f..a095416fe 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 = luaT_toibuf(L, -1);
+	lua_pushboolean(L, buf != NULL);
+	return 1;
+}
+
 static int
 test_touint64(lua_State *L)
 {
@@ -1242,6 +1251,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 ab5c89c8c..ff251b338 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -38,6 +38,26 @@ local function test_pushcdata(test, module)
     test:is(gc_counter, 1, 'pushcdata gc')
 end
 
+local function test_buffers(test, module)
+    test:plan(9)
+    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(buffer.IBUF_SHARED), "checkibuf of ibuf*")
+    test:ok(module.checkibuf(ibuf), 'checkibuf of ibuf')
+    test:ok(not module.checkibuf(pbuf), 'checkibuf of pointer to ibuf data')
+end
+
 local function test_tuple_validate(test, module)
     test:plan(12)
 
@@ -198,7 +218,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")
@@ -225,6 +245,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("tuple_validate", test_tuple_validate, module)
 
     space:drop()
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 2.X v4 3/4] module api: introduced luaT_toibuf instead of luaL_checkibuf

[Alexander Turenko]

> module api: introduced luaT_toibuf instead of luaL_checkibuf

We usually try to fit into 50 symbols. Okay, sometimes we overrun it a
bit (and I was one of persons who asked to don't make it hard limit),
where it is hard to give a short description, but it is not the case,
right?

BTW, imperative mood ('introduce') is suggested for a commit message
header (just header, not the entire body).

(I would name it 'module api/lua: expose luaT_toibuf()' if you want my
opinion on this essential topic.)

> * made `luaL_checkibuf` public;
> * renamed it to `luaT_toibuf` to follow naming convention
>   (it's not raising error, and is casting to ibuf type).

Did you mean 'returns a pointer to' by 'is casting to'? It tooks some
time to get the idea. Hmm, but lua_check<...> also returns a pointer.

I don't push you to anything, just noted that I'm a bit confused here as
a reader.

> +static int
> +test_checkibuf(lua_State *L)
> +{
> +	struct ibuf *buf;
> +	buf = luaT_toibuf(L, -1);
> +	lua_pushboolean(L, buf != NULL);
> +	return 1;
> +}

I don't bother enough about the test to insist on it now, but it looks
just as inconsistency. Why not test_toibuf()?

> +local function test_buffers(test, module)
> +    test:plan(9)
> +    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(buffer.IBUF_SHARED), "checkibuf of ibuf*")
> +    test:ok(module.checkibuf(ibuf), 'checkibuf of ibuf')
> +    test:ok(not module.checkibuf(pbuf), 'checkibuf of pointer to ibuf data')

And here too: why 'checkibuf of nil'? The subject of the test is
luaT_isibuf().

Re: [Tarantool-patches] [PATCH 2.X v4 3/4] module api: introduced luaT_toibuf instead of luaL_checkibuf

[Timur Safin]

Renamed function in the test, and slightly reworded commit message:

commit e0e1421c660dcae8bcced91f392a2d1093ecf30e
Author: Timur Safin <tsafin@tarantool.org>
Date:   Sun Oct 11 16:08:02 2020 +0300

    module api: luaT_toibuf instead of luaL_checkibuf

    * made `luaL_checkibuf` public and then renamed to
      `luaT_toibuf` to follow naming convention
      (it's not raising error, simply returning ibuf
      structure).

    Part of #5384

...
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 17048081f..1c5723693 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_toibuf(lua_State *L)
+{
+       struct ibuf *buf;
+       buf = luaT_toibuf(L, -1);
+       lua_pushboolean(L, buf != NULL);
+       return 1;
+}
+
 static int
 test_touint64(lua_State *L)
 {
@@ -1242,6 +1251,7 @@ luaopen_module_api(lua_State *L)
                {"test_pushint64", test_pushint64 },
                {"test_checkuint64", test_checkuint64 },
                {"test_checkint64", test_checkint64 },
+               {"toibuf", test_toibuf},
                {"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 ab5c89c8c..c1aff6797 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -38,6 +38,26 @@ local function test_pushcdata(test, module)
     test:is(gc_counter, 1, 'pushcdata gc')
 end

+local function test_buffers(test, module)
+    test:plan(9)
+    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.toibuf(nil), 'toibuf of nil')
+    test:ok(not module.toibuf({}), 'toibuf of {}')
+    test:ok(not module.toibuf(1LL), 'toibuf of 1LL')
+    test:ok(not module.toibuf(box.NULL), 'toibuf of box.NULL')
+    test:ok(not module.toibuf(buffer.reg1), 'toibuf of reg1')
+    test:ok(not module.toibuf(bufalloc), 'toibuf of allocated buffer')
+    test:ok(module.toibuf(buffer.IBUF_SHARED), "toibuf of ibuf*")
+    test:ok(module.toibuf(ibuf), 'toibuf of ibuf')
+    test:ok(not module.toibuf(pbuf), 'toibuf of pointer to ibuf data')
+end
+
 local function test_tuple_validate(test, module)
     test:plan(12)

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [PATCH 2.X v4 3/4] module api: introduced luaT_toibuf instead
: of luaL_checkibuf
: 
: > module api: introduced luaT_toibuf instead of luaL_checkibuf
: 
: We usually try to fit into 50 symbols. Okay, sometimes we overrun it a
: bit (and I was one of persons who asked to don't make it hard limit),
: where it is hard to give a short description, but it is not the case,
: right?
: 
: BTW, imperative mood ('introduce') is suggested for a commit message
: header (just header, not the entire body).
: 
: (I would name it 'module api/lua: expose luaT_toibuf()' if you want my
: opinion on this essential topic.)
: 
: > * made `luaL_checkibuf` public;
: > * renamed it to `luaT_toibuf` to follow naming convention
: >   (it's not raising error, and is casting to ibuf type).
: 
: Did you mean 'returns a pointer to' by 'is casting to'? It tooks some
: time to get the idea. Hmm, but lua_check<...> also returns a pointer.
: 
: I don't push you to anything, just noted that I'm a bit confused here as
: a reader.
: 
: > +static int
: > +test_checkibuf(lua_State *L)
: > +{
: > +	struct ibuf *buf;
: > +	buf = luaT_toibuf(L, -1);
: > +	lua_pushboolean(L, buf != NULL);
: > +	return 1;
: > +}
: 
: I don't bother enough about the test to insist on it now, but it looks
: just as inconsistency. Why not test_toibuf()?
: 
: > +local function test_buffers(test, module)
: > +    test:plan(9)
: > +    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(buffer.IBUF_SHARED), "checkibuf of ibuf*")
: > +    test:ok(module.checkibuf(ibuf), 'checkibuf of ibuf')
: > +    test:ok(not module.checkibuf(pbuf), 'checkibuf of pointer to ibuf
: data')
: 
: And here too: why 'checkibuf of nil'? The subject of the test is
: luaT_isibuf().

Re: [Tarantool-patches] [PATCH 2.X v4 3/4] module api: introduced luaT_toibuf instead of luaL_checkibuf

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

Firstly, I agree with Alexander's comments.

Secondly, see my comment below.

> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index e80e2b1a2..5e902b94e 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 *
> +luaT_toibuf(struct lua_State *L, int idx);

AFAIR, we have decided to use box_ibuf_t, and not export struct ibuf
in module.h.

[Tarantool-patches] [PATCH 2.X v4 4/4] 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
---
 src/CMakeLists.txt               |  1 +
 src/box/CMakeLists.txt           |  1 +
 src/box/ibuf.c                   | 65 +++++++++++++++++++++++++++++++
 src/box/ibuf.h                   | 67 ++++++++++++++++++++++++++++++++
 src/exports.h                    |  3 ++
 test/app-tap/module_api.c        | 44 +++++++++++++++++++++
 test/app-tap/module_api.test.lua |  2 +-
 7 files changed, 182 insertions(+), 1 deletion(-)
 create mode 100644 src/box/ibuf.c
 create mode 100644 src/box/ibuf.h

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 699536652..3817f5c62 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/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..690de5e83 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
+    ibuf.c
     ${sql_sources}
     ${lua_sources}
     lua/init.c
diff --git a/src/box/ibuf.c b/src/box/ibuf.c
new file mode 100644
index 000000000..12b9f2781
--- /dev/null
+++ b/src/box/ibuf.c
@@ -0,0 +1,65 @@
+/*
+ * 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 "ibuf.h"
+#include <small/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)
+{
+	assert(ibuf != NULL);
+	if (ibuf == NULL)
+		return;
+	if (rpos != NULL)
+		*rpos = &ibuf->rpos;
+	if (wpos != NULL)
+		*wpos = &ibuf->wpos;
+}
+
+void
+box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end)
+{
+	if (ibuf == NULL)
+		return;
+	if (wpos != NULL)
+		*wpos = &ibuf->wpos;
+	if (end != NULL)
+		*end = &ibuf->end;
+
+}
diff --git a/src/box/ibuf.h b/src/box/ibuf.h
new file mode 100644
index 000000000..d17f8f4b4
--- /dev/null
+++ b/src/box/ibuf.h
@@ -0,0 +1,67 @@
+#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 <stdlib.h>
+
+#include <trivia/util.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 9c56686db..6d8138ff5 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 a095416fe..e73ad6d72 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -164,6 +164,49 @@ test_checkibuf(lua_State *L)
 	return 1;
 }
 
+static int
+test_box_ibuf(lua_State *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);
+	void *ptr = box_ibuf_reserve(&ibuf, 65536);
+	assert(ptr != NULL);
+	char **rpos;
+	char **wpos;
+	box_ibuf_read_range(&ibuf, &rpos, &wpos);
+
+	ptr = ibuf_alloc(&ibuf, 10);
+	assert(ptr != NULL);
+
+	assert(ibuf_used(&ibuf) == 10);
+	assert((*wpos - *rpos) == 10);
+
+	/* let be a little bit paranoid and double check */
+	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);
+
+	size_t unused = ibuf_unused(&ibuf);
+	char **end;
+	box_ibuf_write_range(&ibuf, &wpos, &end);
+	assert((*end - *wpos) == (ptrdiff_t)unused);
+
+	ibuf_reset(&ibuf);
+	assert(ibuf_used(&ibuf) == 0);
+	assert(*rpos == *wpos);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
 static int
 test_touint64(lua_State *L)
 {
@@ -1277,6 +1320,7 @@ luaopen_module_api(lua_State *L)
 		{"tuple_validate_def", test_tuple_validate_default},
 		{"tuple_validate_fmt", test_tuple_validate_formatted},
 		{"test_key_def_dup", test_key_def_dup},
+		{"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 ff251b338..33150d91e 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -218,7 +218,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(34)
+    test:plan(35)
     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 v4 4/4] module api: box_ibuf_* wrappers

[Alexander Turenko]

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

So it'll not (at least stochastically) fail on ibuf pointer
dereferencing on RelWithDebInfo, but will just silently ignore the usage
error. I guess it is the bad pattern. At least I don't see anything
similar in our code: we either assert() or fail with setting an error to
the diagnostics area and returning of -1 / NULL or kinda.

So I propose to jeave assert(), but remove the if (ibuf == NULL) {<...>}.

Don't forget to update box_ibuf_write_range() in sync (not it has no
assert()).

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

[Timur Safin]

After our discussions within messenger, I've updated ibuf.c to
raise error if there is invalid argument passed, to follow
current box_* functions convention. E.g.

+void
+box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
+{
+       if (ibuf == NULL) {
+               diag_set(ClientError, ER_ILLEGAL_PARAMS,
+                        "Invalid ibuf argument");
+               return;
+       }
+       if (rpos != NULL)
+               *rpos = &ibuf->rpos;
+       if (wpos != NULL)
+               *wpos = &ibuf->wpos;
+}
+
+void
+box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end)
+{
+       if (ibuf == NULL) {
+               diag_set(ClientError, ER_ILLEGAL_PARAMS,
+                        "Invalid ibuf argument");
+               return;
+       }
+       if (wpos != NULL)
+               *wpos = &ibuf->wpos;
+       if (end != NULL)
+               *end = &ibuf->end;
+
+}

Code has been updated in the branch tsafin/gh-5273-expand-module-api-v4

Regards,
Timur

: -----Original Message-----
: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Sent: Wednesday, October 14, 2020 6:31 AM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: v.shpilevoy@tarantool.org; tarantool-patches@dev.tarantool.org
: Subject: Re: [PATCH 2.X v4 4/4] module api: box_ibuf_* wrappers
: 
: > +void
: > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
: > +{
: > +	assert(ibuf != NULL);
: > +	if (ibuf == NULL)
: > +		return;
: 
: So it'll not (at least stochastically) fail on ibuf pointer
: dereferencing on RelWithDebInfo, but will just silently ignore the usage
: error. I guess it is the bad pattern. At least I don't see anything
: similar in our code: we either assert() or fail with setting an error to
: the diagnostics area and returning of -1 / NULL or kinda.
: 
: So I propose to jeave assert(), but remove the if (ibuf == NULL) {<...>}.
: 
: Don't forget to update box_ibuf_write_range() in sync (not it has no
: assert()).

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

[Alexander Turenko]

On Fri, Oct 16, 2020 at 12:35:05AM +0300, Timur Safin wrote:
> After our discussions within messenger, I've updated ibuf.c to
> raise error if there is invalid argument passed, to follow
> current box_* functions convention. E.g.

Nit: it *sets* an error, not raise.

> 
> +void
> +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
> +{
> +       if (ibuf == NULL) {
> +               diag_set(ClientError, ER_ILLEGAL_PARAMS,
> +                        "Invalid ibuf argument");
> +               return;
> +       }
> +       if (rpos != NULL)
> +               *rpos = &ibuf->rpos;
> +       if (wpos != NULL)
> +               *wpos = &ibuf->wpos;
> +}

You should return a code (0 / -1) so. The diagnostics area is like
errno: a caller should lean on the code, not the diagnostics area to
determine whether an error took place. If it occurs, then the
diagnostics is guaranteed to exist and to be actual.

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

[Timur Safin]

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X v4 4/4] module api: box_ibuf_*
: wrappers
: 
...
: >
: > +void
: > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
: > +{
: > +       if (ibuf == NULL) {
: > +               diag_set(ClientError, ER_ILLEGAL_PARAMS,
: > +                        "Invalid ibuf argument");
: > +               return;
: > +       }
: > +       if (rpos != NULL)
: > +               *rpos = &ibuf->rpos;
: > +       if (wpos != NULL)
: > +               *wpos = &ibuf->wpos;
: > +}
: 
: You should return a code (0 / -1) so. The diagnostics area is like
: errno: a caller should lean on the code, not the diagnostics area to
: determine whether an error took place. If it occurs, then the
: diagnostics is guaranteed to exist and to be actual.

Indeed - wait a minute, will change.

Timur

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

[Alexander Turenko]

On Fri, Oct 16, 2020 at 12:42:26AM +0300, Alexander Turenko wrote:
> On Fri, Oct 16, 2020 at 12:35:05AM +0300, Timur Safin wrote:
> > After our discussions within messenger, I've updated ibuf.c to
> > raise error if there is invalid argument passed, to follow
> > current box_* functions convention. E.g.
> 
> Nit: it *sets* an error, not raise.
> 
> > 
> > +void
> > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
> > +{
> > +       if (ibuf == NULL) {
> > +               diag_set(ClientError, ER_ILLEGAL_PARAMS,
> > +                        "Invalid ibuf argument");
> > +               return;
> > +       }
> > +       if (rpos != NULL)
> > +               *rpos = &ibuf->rpos;
> > +       if (wpos != NULL)
> > +               *wpos = &ibuf->wpos;
> > +}
> 
> You should return a code (0 / -1) so. The diagnostics area is like
> errno: a caller should lean on the code, not the diagnostics area to
> determine whether an error took place. If it occurs, then the
> diagnostics is guaranteed to exist and to be actual.

Hey, stop. Is there any other function in the module API that allows to
pass NULL instead of an object? Is there a valid use case for this?
Should a user check box_ibuf_read_range() return value and handle a
possible error?

I don't see any sense in handling NULL here. Let's leave assert(ibuf !=
NULL) or nothing.

The practice about silengly ignore incorrect input parameters that you
mentioned before is certainly bad patters. I suggest to avoid it.

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

[Vladislav Shpilevoy]

Thanks for the patch!

See 4 comments below.

On 14.10.2020 01:01, Timur Safin wrote:
> 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
> ---
>  src/CMakeLists.txt               |  1 +
>  src/box/CMakeLists.txt           |  1 +
>  src/box/ibuf.c                   | 65 +++++++++++++++++++++++++++++++
>  src/box/ibuf.h                   | 67 ++++++++++++++++++++++++++++++++
>  src/exports.h                    |  3 ++
>  test/app-tap/module_api.c        | 44 +++++++++++++++++++++
>  test/app-tap/module_api.test.lua |  2 +-
>  7 files changed, 182 insertions(+), 1 deletion(-)
>  create mode 100644 src/box/ibuf.c
>  create mode 100644 src/box/ibuf.h
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 699536652..3817f5c62 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/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..690de5e83 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
> +    ibuf.c
>      ${sql_sources}
>      ${lua_sources}
>      lua/init.c
> diff --git a/src/box/ibuf.c b/src/box/ibuf.c
> new file mode 100644
> index 000000000..12b9f2781
> --- /dev/null
> +++ b/src/box/ibuf.c
> @@ -0,0 +1,65 @@
> +/*
> + * 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 "ibuf.h"
> +#include <small/ibuf.h>

1. Please, don't include non-system headers using <>. Use "".
The same for box/ibuf.h.

> +
> +void *
> +box_ibuf_reserve(box_ibuf_t *ibuf, size_t size)
> +{
> +	return ibuf_reserve(ibuf, size);

2. It should set a diag error in case of fail.

> +}
> +
> +void
> +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
> +{
> +	assert(ibuf != NULL);
> +	if (ibuf == NULL)
> +		return;
> +	if (rpos != NULL)
> +		*rpos = &ibuf->rpos;
> +	if (wpos != NULL)
> +		*wpos = &ibuf->wpos;

3. I would better assume all the arguments are not NULL, and document
it. Especially ibuf itself. We need some border where to stop the
sanity checks, and this looks like an overkill already. For example,
box_tuple_ref() also works with a pointer, but it does not check for
it being not NULL. It is just stupid to call method of an object with
that object passed as NULL. The same below.

> +}
> +
> +void
> +box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end)
> +{
> +	if (ibuf == NULL)
> +		return;
> +	if (wpos != NULL)
> +		*wpos = &ibuf->wpos;
> +	if (end != NULL)
> +		*end = &ibuf->end;
> +
> +}
> diff --git a/src/box/ibuf.h b/src/box/ibuf.h
> new file mode 100644
> index 000000000..d17f8f4b4
> --- /dev/null
> +++ b/src/box/ibuf.h
> @@ -0,0 +1,67 @@
> +#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 <stdlib.h>
> +
> +#include <trivia/util.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

4. Please, finish sentences with a dot. I don't understand.
Is it so hard for people to put a dot in the end? Does it
cost money or something? Do people write the same way in
other places, like emails, documents? Why is the code allowed
to be treated worse?

Talking of the comments content - they are mostly useless.
What is read range, what is write range? How do they relate?
Is a user supposed to update them manually, when work with the
buffer? Current description makes it impossible to use
box_ibuf_t without reading the source code. What users are
not supposed to do usually to be able to use the API.

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

[Alexander Turenko]

> > +void
> > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
> > +{
> > +	assert(ibuf != NULL);
> > +	if (ibuf == NULL)
> > +		return;
> > +	if (rpos != NULL)
> > +		*rpos = &ibuf->rpos;
> > +	if (wpos != NULL)
> > +		*wpos = &ibuf->wpos;
> 
> 3. I would better assume all the arguments are not NULL, and document
> it. Especially ibuf itself. We need some border where to stop the
> sanity checks, and this looks like an overkill already. For example,
> box_tuple_ref() also works with a pointer, but it does not check for
> it being not NULL. It is just stupid to call method of an object with
> that object passed as NULL. The same below.

I would allow 'end' pointer to be NULL in box_ibuf_write_range() at
least. When I want to do a reserve+fill in a loop, the end position is
not needed. The same for 'wpos' in box_ibuf_read_range(): say, you're
sure about the data and just want to use mp_next() or something like.

What about allowing to omit the second parameters: I don't know a case
for this. Maybe it actually worth to forbid to pass NULL to them.

> > +}
> > +
> > +void
> > +box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end)
> > +{
> > +	if (ibuf == NULL)
> > +		return;
> > +	if (wpos != NULL)
> > +		*wpos = &ibuf->wpos;
> > +	if (end != NULL)
> > +		*end = &ibuf->end;
> > +
> > +}

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

[Timur Safin]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X v4 4/4] module api: box_ibuf_*
: wrappers
: 
: Thanks for the patch!
: 
: See 4 comments below.
: 
...
: > +
: > +#include <stdlib.h>
: > +
: > +#include "ibuf.h"
: > +#include <small/ibuf.h>
: 
: 1. Please, don't include non-system headers using <>. Use "".
: The same for box/ibuf.h.

Ok.

: 
: > +
: > +void *
: > +box_ibuf_reserve(box_ibuf_t *ibuf, size_t size)
: > +{
: > +	return ibuf_reserve(ibuf, size);
: 
: 2. It should set a diag error in case of fail.

This is already updated accordingly in the branch
tsafin/gh-5273-expand-module-api-v4.

: 
: > +}
: > +
: > +void
: > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
: > +{
: > +	assert(ibuf != NULL);
: > +	if (ibuf == NULL)
: > +		return;
: > +	if (rpos != NULL)
: > +		*rpos = &ibuf->rpos;
: > +	if (wpos != NULL)
: > +		*wpos = &ibuf->wpos;
: 
: 3. I would better assume all the arguments are not NULL, and document
: it. Especially ibuf itself. We need some border where to stop the
: sanity checks, and this looks like an overkill already. For example,
: box_tuple_ref() also works with a pointer, but it does not check for
: it being not NULL. It is just stupid to call method of an object with
: that object passed as NULL. The same below.

Please see updated branch tsafin/gh-5273-expand-module-api-v4 for the 
current version. For rpos and wpos I need to check for NULL because 
we may call for only single, particular argument (like we do in some 
cases in current external module) thus we pass NULL there. 


: 
: > +}
: > +
: > +void
: > +box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end)
: > +{
: > +	if (ibuf == NULL)
: > +		return;
: > +	if (wpos != NULL)
: > +		*wpos = &ibuf->wpos;
: > +	if (end != NULL)
: > +		*end = &ibuf->end;
: > +
: > +}
...
: > +
: > +/**
: > + * Reserve requested amount of bytes in ibuf buffer
: 
: 4. Please, finish sentences with a dot. I don't understand.
: Is it so hard for people to put a dot in the end? Does it
: cost money or something? Do people write the same way in
: other places, like emails, documents? Why is the code allowed
: to be treated worse?
: 
: Talking of the comments content - they are mostly useless.
: What is read range, what is write range? How do they relate?
: Is a user supposed to update them manually, when work with the
: buffer? Current description makes it impossible to use
: box_ibuf_t without reading the source code. What users are
: not supposed to do usually to be able to use the API.

I'll return to comments and test in a follow-up patchset 
(see my earlier discussion with A.Turenko) and there I'll
put all dots. Promise! In very meaningful comments. Promise!!

Timur

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

[Alexander Turenko]

> : > +}
> : > +
> : > +void
> : > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
> : > +{
> : > +	assert(ibuf != NULL);
> : > +	if (ibuf == NULL)
> : > +		return;
> : > +	if (rpos != NULL)
> : > +		*rpos = &ibuf->rpos;
> : > +	if (wpos != NULL)
> : > +		*wpos = &ibuf->wpos;
> : 
> : 3. I would better assume all the arguments are not NULL, and document
> : it. Especially ibuf itself. We need some border where to stop the
> : sanity checks, and this looks like an overkill already. For example,
> : box_tuple_ref() also works with a pointer, but it does not check for
> : it being not NULL. It is just stupid to call method of an object with
> : that object passed as NULL. The same below.
> 
> Please see updated branch tsafin/gh-5273-expand-module-api-v4 for the 
> current version. For rpos and wpos I need to check for NULL because 
> we may call for only single, particular argument (like we do in some 
> cases in current external module) thus we pass NULL there.

I think we should allow to pass NULL to the third parameters, but not to
the second ones. I don't see a reason to obtain 'wpos' using
_read_range() (it looks more natural to do so using _write_range()) and
to obtain just 'end' without left side of the buffer. To copy something
from right to left without the left border check? Maybe... However it
does not look how the for 'ibuf', where both pointers are moving forward
(it is defined by the _reserve() semantic).

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

[Alexander Turenko]

 | diag_set(OutOfMemory, size, "ibuf_reserve", "box_ibuf_reserve");

Nit: The fourth parameter is an object name. In my patch regarding
exposing of the box region, I name it "data" in the box_region_alloc()
function.

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

[Alexander Turenko]

> +/**
> + * Return pointers to read range pointers used [rpos..wpos)
> + * @param ibuf ibuf structure
> + * @param rpos where to place ibuf.rpos address
> + * @param rpos where to place ibuf.wpos address
> + */
> +API_EXPORT void
> +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos);
> +
> +/**
> + * Return pointers to write range pointers used [wpos..end)
> + * @param ibuf ibuf structure
> + * @param rpos where to place ibuf.rpos address
> + * @param rpos where to place ibuf.wpos address
> + */
> +API_EXPORT void
> +box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end);

Nit: Too many 'rpos'es :)

[Tarantool-patches] [PATCH 2.X v4.1] 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
---

This is updated version for box_ibuf* wrappers, which accumulate fixes for
most of Sasha and Vlad comments, with exceptions of doxygen comments and 
some tests. Extra verbose comments with all dots put in will be sent 
in follow-up patchset to be sent quite soon, but not today.


Branches:
* https://github.com/tarantool/tarantool/tree/tsafin/gh-5273-expand-module-api-v4


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

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 699536652..3817f5c62 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/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..690de5e83 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
+    ibuf.c
     ${sql_sources}
     ${lua_sources}
     lua/init.c
diff --git a/src/box/ibuf.c b/src/box/ibuf.c
new file mode 100644
index 000000000..36ab2918d
--- /dev/null
+++ b/src/box/ibuf.c
@@ -0,0 +1,68 @@
+/*
+ * 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 "ibuf.h"
+#include "error.h"
+#include "diag.h"
+#include "small/ibuf.h"
+
+void *
+box_ibuf_reserve(box_ibuf_t *ibuf, size_t size)
+{
+	void * p = ibuf_reserve(ibuf, size);
+	if (p == NULL)
+		diag_set(OutOfMemory, size, "ibuf_reserve", "box_ibuf_reserve");
+
+	return p;
+}
+
+void
+box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
+{
+	assert(ibuf != NULL);
+	if (rpos != NULL)
+		*rpos = &ibuf->rpos;
+	if (wpos != NULL)
+		*wpos = &ibuf->wpos;
+}
+
+void
+box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end)
+{
+	assert(ibuf != NULL);
+	if (wpos != NULL)
+		*wpos = &ibuf->wpos;
+	if (end != NULL)
+		*end = &ibuf->end;
+}
+
diff --git a/src/box/ibuf.h b/src/box/ibuf.h
new file mode 100644
index 000000000..7e0bba0d7
--- /dev/null
+++ b/src/box/ibuf.h
@@ -0,0 +1,76 @@
+#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 <stdlib.h>
+
+#include <trivia/util.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
+ * @param ibuf buffer used for allocation
+ * @param size allocated bytes
+ * @retval NULL on error, check diag.
+ */
+API_EXPORT void *
+box_ibuf_reserve(box_ibuf_t *ibuf, size_t size);
+
+/**
+ * Return pointers to read range pointers used [rpos..wpos)
+ * @param ibuf ibuf structure
+ * @param rpos where to place ibuf.rpos address
+ * @param rpos where to place ibuf.wpos address
+ */
+API_EXPORT void
+box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos);
+
+/**
+ * Return pointers to write range pointers used [wpos..end)
+ * @param ibuf ibuf structure
+ * @param rpos where to place ibuf.rpos address
+ * @param rpos where to place ibuf.wpos address
+ */
+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 9c56686db..6d8138ff5 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 1c5723693..7a1c0c31d 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -164,6 +164,49 @@ test_toibuf(lua_State *L)
 	return 1;
 }
 
+static int
+test_box_ibuf(lua_State *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);
+	void *ptr = box_ibuf_reserve(&ibuf, 65536);
+	assert(ptr != NULL);
+	char **rpos;
+	char **wpos;
+	box_ibuf_read_range(&ibuf, &rpos, &wpos);
+
+	ptr = ibuf_alloc(&ibuf, 10);
+	assert(ptr != NULL);
+
+	assert(ibuf_used(&ibuf) == 10);
+	assert((*wpos - *rpos) == 10);
+
+	/* let be a little bit paranoid and double check */
+	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);
+
+	size_t unused = ibuf_unused(&ibuf);
+	char **end;
+	box_ibuf_write_range(&ibuf, &wpos, &end);
+	assert((*end - *wpos) == (ptrdiff_t)unused);
+
+	ibuf_reset(&ibuf);
+	assert(ibuf_used(&ibuf) == 0);
+	assert(*rpos == *wpos);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
 static int
 test_touint64(lua_State *L)
 {
@@ -1277,6 +1320,7 @@ luaopen_module_api(lua_State *L)
 		{"tuple_validate_def", test_tuple_validate_default},
 		{"tuple_validate_fmt", test_tuple_validate_formatted},
 		{"test_key_def_dup", test_key_def_dup},
+		{"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 c1aff6797..f91762e54 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -218,7 +218,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(34)
+    test:plan(35)
     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 v4 0/4] module api: extend for external merger Lua module

[Alexander Turenko]

The patchset looks good to me in general, however I agree with Vlad that
it would be good to polish it if we would not have so tight timeline.

I made several changes before pushing:

- 'module api: box_ibuf_* wrappers' is applied first (we need it to use
  <box_ibuf_t> for luaT_toibuf() later).
- Use <box_ibuf_t> in luaT_toibuf(). And so moved the box/ibuf.h header
  before lua/utils.h in the list of the API headers.
- Marked the last commit as 'Closes #5384' instead of 'Part of #5384'.
- Moved the luaL_iscallable 1.10 backport as first to form the proper
  chain of 'part of', 'part of', ... 'closes'.
- Found and fixed 'unused variable' warnings on module_api.c on
  RelWithDebInfo build.

Pushed to master, 2.5, 2.4 and 1.10.

WBR, Alexander Turenko.