[Tarantool-patches] [PATCH v20 0/7] box: implement box.lib Lua module

[Tarantool-patches] [PATCH v20 0/7] box: implement box.lib Lua module

[Cyrill Gorcunov via Tarantool-patches]

Please take a look once time permit.

v1-v3 are development ones and not sent.

v5 (by vlad):
 - drop exists, list methods: they are redundant
 - rename cfunc to cbox
 - when create a function make it callable Lua object
 - initialize cbox out of modules
 - fix error in passing module name for reloading
 - make api been cbox.func.[create|drop] and
   cbox.module.reload
 - fix test for OSX sake

v6 (by vlad):
 - move module handling into module_cache file.
v7:
 - development
v8:
 - use rbtree for function instance storage, since
   i don't like the idea of unexpected rehashing of
   values in case of massive number of functions
   allocated
 - use reference counter and free function instance
   if only load/unload are coupled
 - keep a pointer to the function inside Lua object
   so we don't need to lookup on every function call.
   this force us to implement __gc method
 - use new API and update docs
v9:
 - development
v10:
 - use hashes for function names lookup
 - simply function loads counting
 - use luaL_register_module and luaL_register_type for
   easier methods registering
 - carry functions as userdata object
v11:
 - development
v12:
 - switch to new API as been discussed in
   https://lists.tarantool.org/tarantool-patches/e186c454-6765-4776-6433-f3f791ff4c27@tarantool.org/
v13:
 - development
v14:
 - switch to refs to carry module usage
 - drop func_name structure renaming
 - carry two hashes for backward compatibility with
   functions created via box.schema.func help
 - complete rework of cmod and most parts of
   module_cache
 - account for file statistics to invalidate
   module cache
 - new API for cmod, no more :reload, the :load
   procedure uses cache invalidation
 - update test cases
 - still there is no GC test since I didn't
   manage to deal with it
v15:
 - report module state cached/orphan
 - update test cases
 - do not prevent functions lookup in orphan modules
 - there was an idea to use box.shema.func cache as on
   top of cmod's one, but this doesn't work because in case
   if module doesnt exist in any caches we would put it into
   into cmod's one as well but there wont be a module on
   cmod level which would clean it up later (which makes
   code a way more comple if we choose to track state
   of modules).
v16:
 - internal
v17:
 - drop idea of unifying box.schema.func and cmod functions
   cache, it brings more problems than solves due to too
   different context of execution;
 - make cmod self consistent, which shrink patch series
   size ~1/5 in compare with previous attempts;
 - improve tests to account internal states of modules
   and functions (tt_dev key in reports).
v18:
 - implement pass-through cache for modules loading, for
   this sake 'struct module' uses cmod internally;
 - improve tests to cover sole cmod case and a mixture
   of box.schema.func and cmod to make sure the caches
   are not corrupted.

v19:
 - move module handling into separate subsystem;
 - switch box.schema.func and cmod to use this shared
   code to eliminate code duplication;
 - update tests.

v20:
 - fix potential nil dereference in schema_init while I'm in the code;
 - during working on the series found a bug in module recovery,
   fixed it in the series because my further work depends on it;
 - main module which allows to create C functions now named as box.lib;
 - structures renaming:
   - box.schema.func uses struct schema_module to carry underlied
     struct module;
   - box.lib uses struct module directly and struct box_module_func
     to carry cache of functions.
 - name unification:
   - schema_module variables are named as `mod`;
   - box.schema.func api prefixed as `schema_module_x`;
   - debug properties for module counting prefixed as `debug_x`;
 - the test remains disabled because
   - I run it locally all the time
   - better to have it in repo and enable then since patching
     test-run can take weeks, it is slow procedure

Cyrill Gorcunov (7):
  box/schema: make sure hashes are created
  box/func: module_reload -- drop redundant argument
  box/func: fix modules functions restore
  box/module_cache: introduce modules subsystem
  box/schema.func: switch to new module api
  box: implement box.lib module
  test: add box.lib test

 changelogs/unreleased/fix-module-reload.md |   4 +
 src/box/CMakeLists.txt                     |   2 +
 src/box/box.cc                             |   4 +-
 src/box/call.c                             |   9 +-
 src/box/func.c                             | 545 ++++++++-----------
 src/box/func.h                             |  28 +-
 src/box/func_def.h                         |  14 -
 src/box/lua/box_lib.c                      | 590 +++++++++++++++++++++
 src/box/lua/box_lib.h                      |  25 +
 src/box/lua/init.c                         |   2 +
 src/box/module_cache.c                     | 474 +++++++++++++++++
 src/box/module_cache.h                     | 208 ++++++++
 src/box/schema.cc                          |   7 +
 src/main.cc                                |   3 +
 test/box/CMakeLists.txt                    |   6 +
 test/box/cfunc1.c                          |  58 ++
 test/box/cfunc2.c                          | 137 +++++
 test/box/cfunc3.c                          |  25 +
 test/box/cfunc4.c                          |  28 +
 test/box/func_restore.result               | 103 ++++
 test/box/func_restore.test.lua             |  43 ++
 test/box/func_restore1.c                   |  34 ++
 test/box/func_restore2.c                   |  28 +
 test/box/gh-4648-func-load-unload.result   |   7 +-
 test/box/gh-4648-func-load-unload.test.lua |   7 +-
 test/box/lib.result                        | 530 ++++++++++++++++++
 test/box/lib.test.lua                      | 203 +++++++
 test/box/misc.result                       |   1 +
 test/box/suite.ini                         |   2 +-
 29 files changed, 2757 insertions(+), 370 deletions(-)
 create mode 100644 changelogs/unreleased/fix-module-reload.md
 create mode 100644 src/box/lua/box_lib.c
 create mode 100644 src/box/lua/box_lib.h
 create mode 100644 src/box/module_cache.c
 create mode 100644 src/box/module_cache.h
 create mode 100644 test/box/cfunc1.c
 create mode 100644 test/box/cfunc2.c
 create mode 100644 test/box/cfunc3.c
 create mode 100644 test/box/cfunc4.c
 create mode 100644 test/box/func_restore.result
 create mode 100644 test/box/func_restore.test.lua
 create mode 100644 test/box/func_restore1.c
 create mode 100644 test/box/func_restore2.c
 create mode 100644 test/box/lib.result
 create mode 100644 test/box/lib.test.lua


base-commit: 22e2e4eaad7796f7b0142cc7e603f9fe895f6642
-- 
2.30.2

[Tarantool-patches] [PATCH v20 1/7] box/schema: make sure hashes are created

[Cyrill Gorcunov via Tarantool-patches]

These are critical structures, strictly speaking
if malloc has failed we're likely under heavy memory
pressure and won't continue anyway but better to exit
explicitly.

In-scope-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/schema.cc | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/box/schema.cc b/src/box/schema.cc
index 963278b19..89904e4d2 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -372,6 +372,13 @@ schema_init(void)
 	funcs = mh_i32ptr_new();
 	funcs_by_name = mh_strnptr_new();
 	sequences = mh_i32ptr_new();
+
+	if (spaces == NULL || spaces_by_name == NULL ||
+	    funcs == NULL || funcs_by_name == NULL ||
+	    sequences == NULL) {
+		panic("Can't allocate schema hashes");
+	}
+
 	/*
 	 * Create surrogate space objects for the mandatory system
 	 * spaces (the primal eggs from which we get all the
-- 
2.30.2

[Tarantool-patches] [PATCH v20 2/7] box/func: module_reload -- drop redundant argument

[Cyrill Gorcunov via Tarantool-patches]

The only purpose of the module argument is to
notify the caller that the module doesn't exist.
Lets simply the code and drop this argument.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/call.c | 9 +--------
 src/box/func.c | 7 +++----
 src/box/func.h | 3 +--
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/src/box/call.c b/src/box/call.c
index 9c291260e..7839e1f3e 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -128,14 +128,7 @@ box_module_reload(const char *name)
 				 user->def->name);
 		return -1;
 	}
-	struct module *module = NULL;
-	if (module_reload(name, name + strlen(name), &module) == 0) {
-		if (module != NULL)
-			return 0;
-		else
-			diag_set(ClientError, ER_NO_SUCH_MODULE, name);
-	}
-	return -1;
+	return module_reload(name, name + strlen(name));
 }
 
 int
diff --git a/src/box/func.c b/src/box/func.c
index 9909cee45..233696a4f 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -372,13 +372,13 @@ module_sym(struct module *module, const char *name)
 }
 
 int
-module_reload(const char *package, const char *package_end, struct module **module)
+module_reload(const char *package, const char *package_end)
 {
 	struct module *old_module = module_cache_find(package, package_end);
 	if (old_module == NULL) {
 		/* Module wasn't loaded - do nothing. */
-		*module = NULL;
-		return 0;
+		diag_set(ClientError, ER_NO_SUCH_MODULE, package);
+		return -1;
 	}
 
 	struct module *new_module = module_load(package, package_end);
@@ -399,7 +399,6 @@ module_reload(const char *package, const char *package_end, struct module **modu
 	if (module_cache_put(new_module) != 0)
 		goto restore;
 	module_gc(old_module);
-	*module = new_module;
 	return 0;
 restore:
 	/*
diff --git a/src/box/func.h b/src/box/func.h
index 581e468cb..0a08fa465 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -113,12 +113,11 @@ func_call(struct func *func, struct port *args, struct port *ret);
  *
  * @param package name begin pointer.
  * @param package_end package_end name end pointer.
- * @param[out] module a pointer to store module object on success.
  * @retval -1 on error.
  * @retval 0 on success.
  */
 int
-module_reload(const char *package, const char *package_end, struct module **module);
+module_reload(const char *package, const char *package_end);
 
 #if defined(__cplusplus)
 } /* extern "C" */
-- 
2.30.2

[Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Cyrill Gorcunov via Tarantool-patches]

In commit 96938fafb an ability to hot reload of modules
has been introduced. When module is been reloaded his
functions are resolved to new symbols but if something
went wrong it is supposed to restore old symbols from
the old module. Actually current code restores only
one function and may crash if there a bunch of functions
to restore. Lets fix it.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 changelogs/unreleased/fix-module-reload.md |   4 +
 src/box/func.c                             |  13 ++-
 test/box/CMakeLists.txt                    |   2 +
 test/box/func_restore.result               | 103 +++++++++++++++++++++
 test/box/func_restore.test.lua             |  43 +++++++++
 test/box/func_restore1.c                   |  34 +++++++
 test/box/func_restore2.c                   |  28 ++++++
 7 files changed, 220 insertions(+), 7 deletions(-)
 create mode 100644 changelogs/unreleased/fix-module-reload.md
 create mode 100644 test/box/func_restore.result
 create mode 100644 test/box/func_restore.test.lua
 create mode 100644 test/box/func_restore1.c
 create mode 100644 test/box/func_restore2.c

diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md
new file mode 100644
index 000000000..7e189617f
--- /dev/null
+++ b/changelogs/unreleased/fix-module-reload.md
@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Fix module reloading procedure which may crash in case if
+  new module is corrupted (gh-4642).
diff --git a/src/box/func.c b/src/box/func.c
index 233696a4f..1cd7073de 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -387,13 +387,14 @@ module_reload(const char *package, const char *package_end)
 
 	struct func_c *func, *tmp_func;
 	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
+		/* Move immediately for restore sake. */
+		rlist_move(&new_module->funcs, &func->item);
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
 		func->func = module_sym(new_module, name.sym);
 		if (func->func == NULL)
 			goto restore;
 		func->module = new_module;
-		rlist_move(&new_module->funcs, &func->item);
 	}
 	module_cache_del(package, package_end);
 	if (module_cache_put(new_module) != 0)
@@ -402,10 +403,10 @@ module_reload(const char *package, const char *package_end)
 	return 0;
 restore:
 	/*
-	 * Some old-dso func can't be load from new module, restore old
-	 * functions.
+	 * Some old functions are not found int the new module,
+	 * thus restore all migrated functions back to original.
 	 */
-	do {
+	rlist_foreach_entry_safe(func, &new_module->funcs, item, tmp_func) {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
 		func->func = module_sym(old_module, name.sym);
@@ -419,9 +420,7 @@ module_reload(const char *package, const char *package_end)
 		}
 		func->module = old_module;
 		rlist_move(&old_module->funcs, &func->item);
-	} while (func != rlist_first_entry(&old_module->funcs,
-					   struct func_c, item));
-	assert(rlist_empty(&new_module->funcs));
+	}
 	module_delete(new_module);
 	return -1;
 }
diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 06bfbbe9d..944831af2 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -2,4 +2,6 @@ include_directories(${MSGPUCK_INCLUDE_DIRS})
 build_module(function1 function1.c)
 build_module(reload1 reload1.c)
 build_module(reload2 reload2.c)
+build_module(func_restore1 func_restore1.c)
+build_module(func_restore2 func_restore2.c)
 build_module(tuple_bench tuple_bench.c)
diff --git a/test/box/func_restore.result b/test/box/func_restore.result
new file mode 100644
index 000000000..e679f4eb8
--- /dev/null
+++ b/test/box/func_restore.result
@@ -0,0 +1,103 @@
+-- test-run result file version 2
+--
+-- Test that function can be restored to the
+-- former values when new module can't be
+-- loaded for some reason (say there some
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+
+fio = require('fio')
+ | ---
+ | ...
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+ | ---
+ | ...
+
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
+ | ---
+ | ...
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
+ | ---
+ | ...
+path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. ext
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(path_func_restore))
+ | ---
+ | ...
+fio.symlink(path_func_good, path_func_restore)
+ | ---
+ | - true
+ | ...
+
+box.schema.func.create('func_restore.echo_1', {language = "C"})
+ | ---
+ | ...
+box.schema.func.create('func_restore.echo_2', {language = "C"})
+ | ---
+ | ...
+box.schema.func.create('func_restore.echo_3', {language = "C"})
+ | ---
+ | ...
+
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
+ | ---
+ | ...
+
+-- Order *does* matter since we bind functions in
+-- a list where entries are added to the top.
+box.func['func_restore.echo_3']:call()
+ | ---
+ | - 3
+ | ...
+box.func['func_restore.echo_2']:call()
+ | ---
+ | - 2
+ | ...
+box.func['func_restore.echo_1']:call()
+ | ---
+ | - 1
+ | ...
+
+_ = pcall(fio.unlink(path_func_restore))
+ | ---
+ | ...
+fio.symlink(path_func_bad, path_func_restore)
+ | ---
+ | - true
+ | ...
+
+ok, _ = pcall(box.schema.func.reload, "func_restore")
+ | ---
+ | ...
+assert(not ok)
+ | ---
+ | - true
+ | ...
+
+box.func['func_restore.echo_1']:call()
+ | ---
+ | - 1
+ | ...
+box.func['func_restore.echo_2']:call()
+ | ---
+ | - 2
+ | ...
+box.func['func_restore.echo_3']:call()
+ | ---
+ | - 3
+ | ...
diff --git a/test/box/func_restore.test.lua b/test/box/func_restore.test.lua
new file mode 100644
index 000000000..8a38a6074
--- /dev/null
+++ b/test/box/func_restore.test.lua
@@ -0,0 +1,43 @@
+--
+-- Test that function can be restored to the
+-- former values when new module can't be
+-- loaded for some reason (say there some
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+fio = require('fio')
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
+path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. ext
+
+_ = pcall(fio.unlink(path_func_restore))
+fio.symlink(path_func_good, path_func_restore)
+
+box.schema.func.create('func_restore.echo_1', {language = "C"})
+box.schema.func.create('func_restore.echo_2', {language = "C"})
+box.schema.func.create('func_restore.echo_3', {language = "C"})
+
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
+
+-- Order *does* matter since we bind functions in
+-- a list where entries are added to the top.
+box.func['func_restore.echo_3']:call()
+box.func['func_restore.echo_2']:call()
+box.func['func_restore.echo_1']:call()
+
+_ = pcall(fio.unlink(path_func_restore))
+fio.symlink(path_func_bad, path_func_restore)
+
+ok, _ = pcall(box.schema.func.reload, "func_restore")
+assert(not ok)
+
+box.func['func_restore.echo_1']:call()
+box.func['func_restore.echo_2']:call()
+box.func['func_restore.echo_3']:call()
diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
new file mode 100644
index 000000000..ffd69fd2b
--- /dev/null
+++ b/test/box/func_restore1.c
@@ -0,0 +1,34 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+static int
+echo_num(box_function_ctx_t *ctx, const char *args,
+	 const char *args_end, unsigned int num)
+{
+	char res[16];
+	char *end = mp_encode_uint(res, num);
+	box_return_mp(ctx, res, end);
+	return 0;
+}
+
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 1);
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 2);
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 3);
+}
diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c
new file mode 100644
index 000000000..0d77e78b2
--- /dev/null
+++ b/test/box/func_restore2.c
@@ -0,0 +1,28 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+static int
+echo_num(box_function_ctx_t *ctx, const char *args,
+	 const char *args_end, unsigned int num)
+{
+	char res[16];
+	char *end = mp_encode_uint(res, num);
+	box_return_mp(ctx, res, end);
+	return 0;
+}
+
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 1);
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	return echo_num(ctx, args, args_end, 2);
+}
-- 
2.30.2

[Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Cyrill Gorcunov via Tarantool-patches]

The modules subsystem hides some low-level operations under API.
In particular the modules subsystem is responsible for

 - modules lookup in Lua's "package.search" storage
 - modules caching to eliminate expensive load procedure
 - function symbol resolving

Because naming is intersecting with current module functions
sitting in box/func, lets rename the later to schema_module
prefix. We will use this prefix in next patches to point the
modules in box.schema.func are just a particular user of
the general modules engine.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/CMakeLists.txt |   1 +
 src/box/box.cc         |   4 +-
 src/box/call.c         |   2 +-
 src/box/func.c         |   6 +-
 src/box/func.h         |  12 +-
 src/box/module_cache.c | 474 +++++++++++++++++++++++++++++++++++++++++
 src/box/module_cache.h | 208 ++++++++++++++++++
 src/main.cc            |   3 +
 8 files changed, 698 insertions(+), 12 deletions(-)
 create mode 100644 src/box/module_cache.c
 create mode 100644 src/box/module_cache.h

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 19203f770..cc2e17e94 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -126,6 +126,7 @@ add_library(box STATIC
     memtx_rtree.c
     memtx_bitset.c
     memtx_tx.c
+    module_cache.c
     engine.c
     memtx_engine.c
     memtx_space.c
diff --git a/src/box/box.cc b/src/box/box.cc
index e69b7b2ff..b51928ab8 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2598,7 +2598,7 @@ box_free(void)
 		session_free();
 		user_cache_free();
 		schema_free();
-		module_free();
+		schema_module_free();
 		tuple_free();
 		port_free();
 #endif
@@ -3002,7 +3002,7 @@ box_init(void)
 	 */
 	session_init();
 
-	if (module_init() != 0)
+	if (schema_module_init() != 0)
 		diag_raise();
 
 	if (tuple_init(lua_hash) != 0)
diff --git a/src/box/call.c b/src/box/call.c
index 7839e1f3e..a6384efe2 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -128,7 +128,7 @@ box_module_reload(const char *name)
 				 user->def->name);
 		return -1;
 	}
-	return module_reload(name, name + strlen(name));
+	return schema_module_reload(name, name + strlen(name));
 }
 
 int
diff --git a/src/box/func.c b/src/box/func.c
index 1cd7073de..08918e6db 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -172,7 +172,7 @@ static void
 module_gc(struct module *module);
 
 int
-module_init(void)
+schema_module_init(void)
 {
 	modules = mh_strnptr_new();
 	if (modules == NULL) {
@@ -184,7 +184,7 @@ module_init(void)
 }
 
 void
-module_free(void)
+schema_module_free(void)
 {
 	while (mh_size(modules) > 0) {
 		mh_int_t i = mh_first(modules);
@@ -372,7 +372,7 @@ module_sym(struct module *module, const char *name)
 }
 
 int
-module_reload(const char *package, const char *package_end)
+schema_module_reload(const char *package, const char *package_end)
 {
 	struct module *old_module = module_cache_find(package, package_end);
 	if (old_module == NULL) {
diff --git a/src/box/func.h b/src/box/func.h
index 0a08fa465..5a49e34f4 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -85,16 +85,16 @@ struct func {
 };
 
 /**
- * Initialize modules subsystem.
+ * Initialize schema modules subsystem.
  */
 int
-module_init(void);
+schema_module_init(void);
 
 /**
- * Cleanup modules subsystem.
+ * Cleanup schema modules subsystem.
  */
 void
-module_free(void);
+schema_module_free(void);
 
 struct func *
 func_new(struct func_def *def);
@@ -109,7 +109,7 @@ int
 func_call(struct func *func, struct port *args, struct port *ret);
 
 /**
- * Reload dynamically loadable module.
+ * Reload dynamically loadable schema module.
  *
  * @param package name begin pointer.
  * @param package_end package_end name end pointer.
@@ -117,7 +117,7 @@ func_call(struct func *func, struct port *args, struct port *ret);
  * @retval 0 on success.
  */
 int
-module_reload(const char *package, const char *package_end);
+schema_module_reload(const char *package, const char *package_end);
 
 #if defined(__cplusplus)
 } /* extern "C" */
diff --git a/src/box/module_cache.c b/src/box/module_cache.c
new file mode 100644
index 000000000..2cd2f2e8b
--- /dev/null
+++ b/src/box/module_cache.c
@@ -0,0 +1,474 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#include <unistd.h>
+#include <string.h>
+#include <fcntl.h>
+#include <dlfcn.h>
+#include <lua.h>
+
+#include "assoc.h"
+#include "diag.h"
+#include "fiber.h"
+#include "module_cache.h"
+
+#include "box/error.h"
+#include "box/port.h"
+
+#include "lua/utils.h"
+#include "libeio/eio.h"
+
+static struct mh_strnptr_t *module_cache = NULL;
+
+/**
+ * Helpers for cache manipulations.
+ */
+static void *
+cache_find(const char *str, size_t len)
+{
+	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
+	if (e == mh_end(module_cache))
+		return NULL;
+	return mh_strnptr_node(module_cache, e)->val;
+}
+
+static void
+cache_update(struct module *m)
+{
+	const char *str = m->package;
+	size_t len = m->package_len;
+
+	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
+	if (e == mh_end(module_cache))
+		panic("module: failed to update cache: %s", str);
+
+	mh_strnptr_node(module_cache, e)->str = m->package;
+	mh_strnptr_node(module_cache, e)->val = m;
+}
+
+static int
+cache_put(struct module *m)
+{
+	const struct mh_strnptr_node_t nd = {
+		.str	= m->package,
+		.len	= m->package_len,
+		.hash	= mh_strn_hash(m->package, m->package_len),
+		.val	= m,
+	};
+
+	mh_int_t e = mh_strnptr_put(module_cache, &nd, NULL, NULL);
+	if (e == mh_end(module_cache)) {
+		diag_set(OutOfMemory, sizeof(nd), "malloc",
+			 "module_cache node");
+		return -1;
+	}
+	return 0;
+}
+
+static void
+cache_del(struct module *m)
+{
+	const char *str = m->package;
+	size_t len = m->package_len;
+
+	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
+	if (e != mh_end(module_cache)) {
+		struct module *v = mh_strnptr_node(module_cache, e)->val;
+		if (v == m) {
+			/*
+			 * The module in cache might be updated
+			 * via force load and old instance is kept
+			 * by a reference only.
+			 */
+			mh_strnptr_del(module_cache, e, NULL);
+		}
+	}
+}
+
+/** Arguments for lpackage_search. */
+struct find_ctx {
+	const char *package;
+	size_t package_len;
+	char *path;
+	size_t path_len;
+};
+
+/** A helper for find_package(). */
+static int
+lpackage_search(lua_State *L)
+{
+	struct find_ctx *ctx = (void *)lua_topointer(L, 1);
+
+	lua_getglobal(L, "package");
+	lua_getfield(L, -1, "search");
+	lua_pushlstring(L, ctx->package, ctx->package_len);
+
+	lua_call(L, 1, 1);
+	if (lua_isnil(L, -1))
+		return luaL_error(L, "module not found");
+
+	char resolved[PATH_MAX];
+	if (realpath(lua_tostring(L, -1), resolved) == NULL) {
+		diag_set(SystemError, "realpath");
+		return luaT_error(L);
+	}
+
+	/*
+	 * No need for result being trimmed test, it
+	 * is guaranteed by realpath call.
+	 */
+	snprintf(ctx->path, ctx->path_len, "%s", resolved);
+	return 0;
+}
+
+/** Find package in Lua's "package.search". */
+static int
+find_package(const char *package, size_t package_len,
+	     char *path, size_t path_len)
+{
+	struct find_ctx ctx = {
+		.package	= package,
+		.package_len	= package_len,
+		.path		= path,
+		.path_len	= path_len,
+	};
+
+	struct lua_State *L = tarantool_L;
+	int top = lua_gettop(L);
+	if (luaT_cpcall(L, lpackage_search, &ctx) != 0) {
+		diag_set(ClientError, ER_LOAD_MODULE, ctx.package_len,
+			 ctx.package, lua_tostring(L, -1));
+		lua_settop(L, top);
+		return -1;
+	}
+	assert(top == lua_gettop(L));
+	return 0;
+}
+
+void
+module_ref(struct module *m)
+{
+	assert(m->refs >= 0);
+	++m->refs;
+}
+
+void
+module_unref(struct module *m)
+{
+	assert(m->refs > 0);
+	if (--m->refs == 0) {
+		cache_del(m);
+		dlclose(m->handle);
+		TRASH(m);
+		free(m);
+	}
+}
+
+int
+module_func_load(struct module *m, const char *func_name,
+		 struct module_func *mf)
+{
+	void *sym = dlsym(m->handle, func_name);
+	if (sym == NULL) {
+		diag_set(ClientError, ER_LOAD_FUNCTION,
+			 func_name, dlerror());
+		return -1;
+	}
+
+	mf->func = sym;
+	mf->module = m;
+	module_ref(m);
+
+	return 0;
+}
+
+void
+module_func_unload(struct module_func *mf)
+{
+	module_unref(mf->module);
+	/*
+	 * Strictly speaking there is no need
+	 * for implicit creation, it is up to
+	 * the caller to clear the module function,
+	 * but since it is cheap, lets prevent from
+	 * even potential use after free.
+	 */
+	module_func_create(mf);
+}
+
+int
+module_func_call(struct module_func *mf, struct port *args,
+		 struct port *ret)
+{
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+
+	uint32_t data_sz;
+	const char *data = port_get_msgpack(args, &data_sz);
+	if (data == NULL)
+		return -1;
+
+	port_c_create(ret);
+	box_function_ctx_t ctx = {
+		.port = ret,
+	};
+
+	/*
+	 * We don't know what exactly the callee
+	 * gonna do during the execution, it may
+	 * even try to unload itself, thus we make
+	 * sure the dso won't be unloaded until
+	 * execution is complete.
+	 *
+	 * Moreover the callee might release the memory
+	 * associated with the module_func pointer itself
+	 * so keep the address of the module locally.
+	 */
+	struct module *m = mf->module;
+	module_ref(m);
+	int rc = mf->func(&ctx, data, data + data_sz);
+	module_unref(m);
+
+	region_truncate(region, region_svp);
+
+	if (rc != 0) {
+		if (diag_last_error(&fiber()->diag) == NULL)
+			diag_set(ClientError, ER_PROC_C, "unknown error");
+		port_destroy(ret);
+		return -1;
+	}
+
+	return 0;
+}
+
+/** Fill attributes from stat. */
+static void
+module_attr_fill(struct module_attr *attr, struct stat *st)
+{
+	memset(attr, 0, sizeof(*attr));
+
+	attr->st_dev	= (uint64_t)st->st_dev;
+	attr->st_ino	= (uint64_t)st->st_ino;
+	attr->st_size	= (uint64_t)st->st_size;
+#ifdef TARGET_OS_DARWIN
+	attr->tv_sec	= (uint64_t)st->st_mtimespec.tv_sec;
+	attr->tv_nsec	= (uint64_t)st->st_mtimespec.tv_nsec;
+#else
+	attr->tv_sec	= (uint64_t)st->st_mtim.tv_sec;
+	attr->tv_nsec	= (uint64_t)st->st_mtim.tv_nsec;
+#endif
+}
+
+/**
+ * Copy shared library to temp directory and load from there,
+ * then remove it from this temp place leaving in memory. This
+ * is because there was a bug in libc which screw file updates
+ * detection properly such that next dlopen call simply return
+ * a cached version instead of rereading a library from the disk.
+ *
+ * We keep own copy of file attributes and reload the library
+ * on demand.
+ */
+static struct module *
+module_new(const char *package, size_t package_len,
+	   const char *source_path)
+{
+	size_t size = sizeof(struct module) + package_len + 1;
+	struct module *m = malloc(size);
+	if (m == NULL) {
+		diag_set(OutOfMemory, size, "malloc", "module");
+		return NULL;
+	}
+
+	m->package_len = package_len;
+	m->refs = 0;
+
+	memcpy(m->package, package, package_len);
+	m->package[package_len] = 0;
+
+	const char *tmpdir = getenv("TMPDIR");
+	if (tmpdir == NULL)
+		tmpdir = "/tmp";
+
+	char dir_name[PATH_MAX];
+	int rc = snprintf(dir_name, sizeof(dir_name),
+			  "%s/tntXXXXXX", tmpdir);
+	if (rc < 0 || (size_t)rc >= sizeof(dir_name)) {
+		diag_set(SystemError, "failed to generate path to tmp dir");
+		goto error;
+	}
+
+	if (mkdtemp(dir_name) == NULL) {
+		diag_set(SystemError, "failed to create unique dir name: %s",
+			 dir_name);
+		goto error;
+	}
+
+	char load_name[PATH_MAX];
+	rc = snprintf(load_name, sizeof(load_name),
+		      "%s/%.*s." TARANTOOL_LIBEXT,
+		      dir_name, (int)package_len, package);
+	if (rc < 0 || (size_t)rc >= sizeof(dir_name)) {
+		diag_set(SystemError, "failed to generate path to dso");
+		goto error;
+	}
+
+	struct stat st;
+	if (stat(source_path, &st) < 0) {
+		diag_set(SystemError, "failed to stat() module: %s",
+			 source_path);
+		goto error;
+	}
+	module_attr_fill(&m->attr, &st);
+
+	int source_fd = open(source_path, O_RDONLY);
+	if (source_fd < 0) {
+		diag_set(SystemError, "failed to open module %s "
+			 "file for reading", source_path);
+		goto error;
+	}
+	int dest_fd = open(load_name, O_WRONLY | O_CREAT | O_TRUNC,
+			   st.st_mode & 0777);
+	if (dest_fd < 0) {
+		diag_set(SystemError, "failed to open file %s "
+			 "for writing ", load_name);
+		close(source_fd);
+		goto error;
+	}
+
+	off_t ret = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size);
+	close(source_fd);
+	close(dest_fd);
+	if (ret != st.st_size) {
+		diag_set(SystemError, "failed to copy dso %s to %s",
+			 source_path, load_name);
+		goto error;
+	}
+
+	m->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
+	if (unlink(load_name) != 0)
+		say_warn("failed to unlink dso link: %s", load_name);
+	if (rmdir(dir_name) != 0)
+		say_warn("failed to delete temporary dir: %s", dir_name);
+	if (m->handle == NULL) {
+		diag_set(ClientError, ER_LOAD_MODULE, package_len,
+			  package, dlerror());
+		goto error;
+	}
+
+	module_ref(m);
+	return m;
+
+error:
+	free(m);
+	return NULL;
+}
+
+struct module *
+module_load_force(const char *package, size_t package_len)
+{
+	char path[PATH_MAX];
+	size_t size = sizeof(path);
+
+	if (find_package(package, package_len, path, size) != 0)
+		return NULL;
+
+	struct module *m = module_new(package, package_len, path);
+	if (m == NULL)
+		return NULL;
+
+	struct module *c = cache_find(package, package_len);
+	if (c != NULL) {
+		cache_update(m);
+	} else {
+		if (cache_put(m) != 0) {
+			module_unload(m);
+			return NULL;
+		}
+	}
+
+	return m;
+}
+
+struct module *
+module_load(const char *package, size_t package_len)
+{
+	char path[PATH_MAX];
+
+	if (find_package(package, package_len, path, sizeof(path)) != 0)
+		return NULL;
+
+	struct module *m = cache_find(package, package_len);
+	if (m != NULL) {
+		struct module_attr attr;
+		struct stat st;
+		if (stat(path, &st) != 0) {
+			diag_set(SystemError, "failed to stat() %s", path);
+			return NULL;
+		}
+
+		/*
+		 * In case of cache hit we may reuse existing
+		 * module which speedup load procedure.
+		 */
+		module_attr_fill(&attr, &st);
+		if (memcmp(&attr, &m->attr, sizeof(attr)) == 0) {
+			module_ref(m);
+			return m;
+		}
+
+		/*
+		 * Module has been updated on a storage device,
+		 * so load a new instance and update the cache,
+		 * old entry get evicted but continue residing
+		 * in memory, fully functional, until last
+		 * function is unloaded.
+		 */
+		m = module_new(package, package_len, path);
+		if (m != NULL)
+			cache_update(m);
+	} else {
+		m = module_new(package, package_len, path);
+		if (m != NULL && cache_put(m) != 0) {
+			module_unload(m);
+			return NULL;
+		}
+	}
+
+	return m;
+}
+
+void
+module_unload(struct module *m)
+{
+	module_unref(m);
+}
+
+void
+module_free(void)
+{
+	mh_int_t e;
+
+	mh_foreach(module_cache, e) {
+		struct module *m = mh_strnptr_node(module_cache, e)->val;
+		module_unload(m);
+	}
+
+	mh_strnptr_delete(module_cache);
+	module_cache = NULL;
+}
+
+int
+module_init(void)
+{
+	module_cache = mh_strnptr_new();
+	if (module_cache == NULL) {
+		diag_set(OutOfMemory, sizeof(*module_cache),
+			 "malloc", "module_cache");
+		return -1;
+	}
+	return 0;
+}
diff --git a/src/box/module_cache.h b/src/box/module_cache.h
new file mode 100644
index 000000000..18eb3866a
--- /dev/null
+++ b/src/box/module_cache.h
@@ -0,0 +1,208 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#pragma once
+
+#include <stdint.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include "trivia/config.h"
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+/**
+ * API of C stored function.
+ */
+
+struct port;
+
+struct box_function_ctx {
+	struct port *port;
+};
+
+typedef struct box_function_ctx box_function_ctx_t;
+typedef int (*box_function_t)(box_function_ctx_t *ctx,
+			      const char *args,
+			      const char *args_end);
+
+/**
+ * Shared library file attributes for
+ * module cache invalidation.
+ */
+struct module_attr {
+	uint64_t st_dev;
+	uint64_t st_ino;
+	uint64_t st_size;
+	uint64_t tv_sec;
+	uint64_t tv_nsec;
+};
+
+/**
+ * Dynamic shared module.
+ */
+struct module {
+	/**
+	 * Module handle, dlopen() result.
+	 */
+	void *handle;
+	/**
+	 * File attributes.
+	 */
+	struct module_attr attr;
+	/**
+	 * Count of active references.
+	 */
+	int64_t refs;
+	/**
+	 * Length of @a package.
+	 */
+	size_t package_len;
+	/**
+	 * Module's name without file extension.
+	 */
+	char package[0];
+};
+
+/**
+ * Module function.
+ */
+struct module_func {
+	/**
+	 * Function's address, iow dlsym() result.
+	 */
+	 box_function_t func;
+	/**
+	 * Function's module.
+	 */
+	struct module *module;
+};
+
+/**
+ * Load a module.
+ *
+ * Lookup for a module instance in cache and if not found
+ * the module is loaded from a storage device. In case if
+ * the module is present in cache but modified on a storage
+ * device it will be reread as a new and cache entry get
+ * updated.
+ *
+ * @param package module package (without file extension).
+ * @param package_len length of @a package.
+ *
+ * Possible errors:
+ * ClientError: the package is not found on a storage device.
+ * ClientError: an error happened when been loading the package.
+ * SystemError: a system error happened during procedure.
+ * OutOfMemory: unable to allocate new memory for module instance.
+ *
+ * @return a module instance on success, NULL otherwise (diag is set)
+ */
+struct module *
+module_load(const char *package, size_t package_len);
+
+/**
+ * Force load a module.
+ *
+ * Load a module from a storage device in a force way
+ * and update an associated cache entry.
+ *
+ * @param package module package (without file extension).
+ * @param package_len length of @a package.
+ *
+ * Possible errors:
+ * ClientError: the package is not found on a storage device.
+ * ClientError: an error happened when been loading the package.
+ * SystemError: a system error happened during procedure.
+ * OutOfMemory: unable to allocate new memory for module instance.
+ *
+ * @return a module instance on success, NULL otherwise (diag is set)
+ */
+struct module *
+module_load_force(const char *package, size_t package_len);
+
+/**
+ * Unload a module instance.
+ *
+ * @param m a module to unload.
+ */
+void
+module_unload(struct module *m);
+
+/** Test if module function is empty. */
+static inline bool
+module_func_is_empty(struct module_func *mf)
+{
+	return mf->module == NULL;
+}
+
+/** Create new empty module function. */
+static inline void
+module_func_create(struct module_func *mf)
+{
+	mf->module = NULL;
+	mf->func = NULL;
+}
+
+/**
+ * Load a new function.
+ *
+ * @param m a module to load a function from.
+ * @param func_name function name.
+ * @param mf[out] function instance.
+ *
+ * Possible errors:
+ * ClientError: no such function in a module.
+ *
+ * @return 0 on success, -1 otherwise (diag is set).
+ */
+int
+module_func_load(struct module *m, const char *func_name,
+		 struct module_func *mf);
+
+/**
+ * Unload a function.
+ *
+ * @param mf module function.
+ */
+void
+module_func_unload(struct module_func *mf);
+
+/**
+ * Execute a function.
+ *
+ * @param mf a function to execute.
+ * @param args function arguments.
+ * @param ret[out] execution results.
+ *
+ * @return 0 on success, -1 otherwise (diag is set).
+ */
+int
+module_func_call(struct module_func *mf, struct port *args,
+		 struct port *ret);
+
+/** Increment reference to a module. */
+void
+module_ref(struct module *m);
+
+/** Decrement reference of a module. */
+void
+module_unref(struct module *m);
+
+/** Initialize modules subsystem. */
+int
+module_init(void);
+
+/** Free modules subsystem. */
+void
+module_free(void);
+
+#if defined(__cplusplus)
+}
+#endif /* defined(__plusplus) */
diff --git a/src/main.cc b/src/main.cc
index 2be048d77..b74ac5926 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -76,6 +76,7 @@
 #include "box/lua/init.h" /* box_lua_init() */
 #include "box/session.h"
 #include "box/memtx_tx.h"
+#include "box/module_cache.h"
 #include "systemd.h"
 #include "crypto/crypto.h"
 #include "core/popen.h"
@@ -521,6 +522,7 @@ tarantool_free(void)
 	title_free(main_argc, main_argv);
 
 	popen_free();
+	module_free();
 
 	/* unlink pidfile. */
 	if (pid_file_handle != NULL && pidfile_remove(pid_file_handle) == -1)
@@ -703,6 +705,7 @@ main(int argc, char **argv)
 	cbus_init();
 	coll_init();
 	memtx_tx_manager_init();
+	module_init();
 	crypto_init();
 	systemd_init();
 
-- 
2.30.2

[Tarantool-patches] [PATCH v20 5/7] box/schema.func: switch to new module api

[Cyrill Gorcunov via Tarantool-patches]

Since we have low level module api now we can switch
the box.schema.func code to use it. In particular we
define schema_module structure as a wrapper over the
modules api -- it carries a pointer to general module
structure.

Because of modules reload functionality (which was actually
a big mistake to introduce in a first place, because it is too
fragile and in case of unintended misuse may simply crash the
application in a best case) the schema modules carry own cache
of modules instances. Thus to make overall code somehow close
to modules api we do:

1) every schema_module variable should be named as `mod`. this
   allows to distinguish this kind of modules from general
  `module` variables;
2) cache operations are renamed to cache_find/put/update/del;
3) C functions are switched to use module_func low level api;
4) because low level modules api carry own references we can
   take no explicit reference when calling a function.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/func.c                             | 527 +++++++++------------
 src/box/func.h                             |  15 +-
 src/box/func_def.h                         |  14 -
 test/box/gh-4648-func-load-unload.result   |   7 +-
 test/box/gh-4648-func-load-unload.test.lua |   7 +-
 5 files changed, 228 insertions(+), 342 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 08918e6db..54d0cbc68 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -30,19 +30,13 @@
  */
 #include "func.h"
 #include "fiber.h"
-#include "trivia/config.h"
 #include "assoc.h"
-#include "lua/utils.h"
 #include "lua/call.h"
-#include "error.h"
 #include "errinj.h"
 #include "diag.h"
 #include "port.h"
 #include "schema.h"
 #include "session.h"
-#include "libeio/eio.h"
-#include <fcntl.h>
-#include <dlfcn.h>
 
 /**
  * Parsed symbol and package names.
@@ -56,6 +50,19 @@ struct func_name {
 	const char *package_end;
 };
 
+/**
+ * Schema module (box.schema) instance.
+ */
+struct schema_module {
+	/** Low level module instance. */
+	struct module *module;
+	/** List of imported functions. */
+	struct rlist funcs;
+	/** Reference counter. */
+	int64_t refs;
+};
+
+
 struct func_c {
 	/** Function object base class. */
 	struct func base;
@@ -64,16 +71,21 @@ struct func_c {
 	 */
 	struct rlist item;
 	/**
-	 * For C functions, the body of the function.
+	 * C function to call.
 	 */
-	box_function_f func;
+	struct module_func mf;
 	/**
-	 * Each stored function keeps a handle to the
-	 * dynamic library for the C callback.
+	 * A schema module the function belongs to.
 	 */
-	struct module *module;
+	struct schema_module *mod;
 };
 
+static void
+schema_module_ref(struct schema_module *mod);
+
+static void
+schema_module_unref(struct schema_module *mod);
+
 /***
  * Split function name to symbol and package names.
  * For example, str = foo.bar.baz => sym = baz, package = foo.bar
@@ -95,82 +107,9 @@ func_split_name(const char *str, struct func_name *name)
 	}
 }
 
-/**
- * Arguments for luaT_module_find used by lua_cpcall()
- */
-struct module_find_ctx {
-	const char *package;
-	const char *package_end;
-	char *path;
-	size_t path_len;
-};
-
-/**
- * A cpcall() helper for module_find()
- */
-static int
-luaT_module_find(lua_State *L)
-{
-	struct module_find_ctx *ctx = (struct module_find_ctx *)
-		lua_topointer(L, 1);
-
-	/*
-	 * Call package.searchpath(name, package.cpath) and use
-	 * the path to the function in dlopen().
-	 */
-	lua_getglobal(L, "package");
-
-	lua_getfield(L, -1, "search");
-
-	/* Argument of search: name */
-	lua_pushlstring(L, ctx->package, ctx->package_end - ctx->package);
-
-	lua_call(L, 1, 1);
-	if (lua_isnil(L, -1))
-		return luaL_error(L, "module not found");
-	/* Convert path to absolute */
-	char resolved[PATH_MAX];
-	if (realpath(lua_tostring(L, -1), resolved) == NULL) {
-		diag_set(SystemError, "realpath");
-		return luaT_error(L);
-	}
-
-	snprintf(ctx->path, ctx->path_len, "%s", resolved);
-	return 0;
-}
-
-/**
- * Find path to module using Lua's package.cpath
- * @param package package name
- * @param package_end a pointer to the last byte in @a package + 1
- * @param[out] path path to shared library
- * @param path_len size of @a path buffer
- * @retval 0 on success
- * @retval -1 on error, diag is set
- */
-static int
-module_find(const char *package, const char *package_end, char *path,
-	    size_t path_len)
-{
-	struct module_find_ctx ctx = { package, package_end, path, path_len };
-	lua_State *L = tarantool_L;
-	int top = lua_gettop(L);
-	if (luaT_cpcall(L, luaT_module_find, &ctx) != 0) {
-		int package_len = (int) (package_end - package);
-		diag_set(ClientError, ER_LOAD_MODULE, package_len, package,
-			 lua_tostring(L, -1));
-		lua_settop(L, top);
-		return -1;
-	}
-	assert(top == lua_gettop(L)); /* cpcall discard results */
-	return 0;
-}
-
+/** Schema modules hash. */
 static struct mh_strnptr_t *modules = NULL;
 
-static void
-module_gc(struct module *module);
-
 int
 schema_module_init(void)
 {
@@ -188,10 +127,8 @@ schema_module_free(void)
 {
 	while (mh_size(modules) > 0) {
 		mh_int_t i = mh_first(modules);
-		struct module *module =
-			(struct module *) mh_strnptr_node(modules, i)->val;
-		/* Can't delete modules if they have active calls */
-		module_gc(module);
+		struct schema_module *mod = mh_strnptr_node(modules, i)->val;
+		schema_module_unref(mod);
 	}
 	mh_strnptr_delete(modules);
 }
@@ -199,25 +136,30 @@ schema_module_free(void)
 /**
  * Look up a module in the modules cache.
  */
-static struct module *
-module_cache_find(const char *name, const char *name_end)
+static struct schema_module *
+cache_find(const char *name, const char *name_end)
 {
 	mh_int_t i = mh_strnptr_find_inp(modules, name, name_end - name);
 	if (i == mh_end(modules))
 		return NULL;
-	return (struct module *)mh_strnptr_node(modules, i)->val;
+	return mh_strnptr_node(modules, i)->val;
 }
 
 /**
- * Save module to the module cache.
+ * Save a module to the modules cache.
  */
-static inline int
-module_cache_put(struct module *module)
+static int
+cache_put(struct schema_module *mod)
 {
-	size_t package_len = strlen(module->package);
-	uint32_t name_hash = mh_strn_hash(module->package, package_len);
+	const char *str = mod->module->package;
+	size_t len = mod->module->package_len;
+
 	const struct mh_strnptr_node_t strnode = {
-		module->package, package_len, name_hash, module};
+		.str	= str,
+		.len	= len,
+		.hash	= mh_strn_hash(str, len),
+		.val	= mod,
+	};
 
 	if (mh_strnptr_put(modules, &strnode, NULL, NULL) == mh_end(modules)) {
 		diag_set(OutOfMemory, sizeof(strnode), "malloc", "modules");
@@ -227,190 +169,214 @@ module_cache_put(struct module *module)
 }
 
 /**
- * Delete a module from the module cache
+ * Update a module in the modules cache.
  */
 static void
-module_cache_del(const char *name, const char *name_end)
+cache_update(struct schema_module *mod)
 {
-	mh_int_t i = mh_strnptr_find_inp(modules, name, name_end - name);
+	const char *str = mod->module->package;
+	size_t len = mod->module->package_len;
+
+	mh_int_t i = mh_strnptr_find_inp(modules, str, len);
 	if (i == mh_end(modules))
-		return;
-	mh_strnptr_del(modules, i, NULL);
+		panic("func: failed to update cache: %s", str);
+
+	mh_strnptr_node(modules, i)->str = str;
+	mh_strnptr_node(modules, i)->val = mod;
 }
 
-/*
- * Load a dso.
- * Create a new symlink based on temporary directory and try to
- * load via this symink to load a dso twice for cases of a function
- * reload.
+/**
+ * Delete a module from the module cache.
  */
-static struct module *
-module_load(const char *package, const char *package_end)
+static void
+cache_del(struct schema_module *mod)
 {
-	char path[PATH_MAX];
-	if (module_find(package, package_end, path, sizeof(path)) != 0)
-		return NULL;
-
-	int package_len = package_end - package;
-	struct module *module = (struct module *)
-		malloc(sizeof(*module) + package_len + 1);
-	if (module == NULL) {
-		diag_set(OutOfMemory, sizeof(struct module) + package_len + 1,
-			 "malloc", "struct module");
-		return NULL;
-	}
-	memcpy(module->package, package, package_len);
-	module->package[package_len] = 0;
-	rlist_create(&module->funcs);
-	module->calls = 0;
-
-	const char *tmpdir = getenv("TMPDIR");
-	if (tmpdir == NULL)
-		tmpdir = "/tmp";
-	char dir_name[PATH_MAX];
-	int rc = snprintf(dir_name, sizeof(dir_name), "%s/tntXXXXXX", tmpdir);
-	if (rc < 0 || (size_t) rc >= sizeof(dir_name)) {
-		diag_set(SystemError, "failed to generate path to tmp dir");
-		goto error;
+	const char *str = mod->module->package;
+	size_t len = mod->module->package_len;
+
+	mh_int_t i = mh_strnptr_find_inp(modules, str, len);
+	if (i != mh_end(modules)) {
+		struct schema_module *v;
+		v = mh_strnptr_node(modules, i)->val;
+		/*
+		 * The module may be already reloaded so
+		 * the cache carries a new entry instead.
+		 */
+		if (v == mod)
+			mh_strnptr_del(modules, i, NULL);
 	}
+}
 
-	if (mkdtemp(dir_name) == NULL) {
-		diag_set(SystemError, "failed to create unique dir name: %s",
-			 dir_name);
-		goto error;
-	}
-	char load_name[PATH_MAX];
-	rc = snprintf(load_name, sizeof(load_name), "%s/%.*s." TARANTOOL_LIBEXT,
-		      dir_name, package_len, package);
-	if (rc < 0 || (size_t) rc >= sizeof(dir_name)) {
-		diag_set(SystemError, "failed to generate path to DSO");
-		goto error;
-	}
+/** Delete a module. */
+static void
+schema_module_delete(struct schema_module *mod)
+{
+	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
+	if (e != NULL)
+		--e->iparam;
+	module_unload(mod->module);
+	TRASH(mod);
+	free(mod);
+}
 
-	struct stat st;
-	if (stat(path, &st) < 0) {
-		diag_set(SystemError, "failed to stat() module %s", path);
-		goto error;
-	}
+/** Increment reference to a module. */
+static void
+schema_module_ref(struct schema_module *mod)
+{
+	assert(mod->refs >= 0);
+	++mod->refs;
+}
 
-	int source_fd = open(path, O_RDONLY);
-	if (source_fd < 0) {
-		diag_set(SystemError, "failed to open module %s file for" \
-			 " reading", path);
-		goto error;
-	}
-	int dest_fd = open(load_name, O_WRONLY|O_CREAT|O_TRUNC,
-			   st.st_mode & 0777);
-	if (dest_fd < 0) {
-		diag_set(SystemError, "failed to open file %s for writing ",
-			 load_name);
-		close(source_fd);
-		goto error;
+/** Decrement reference to a module and delete it if last one. */
+static void
+schema_module_unref(struct schema_module *mod)
+{
+	assert(mod->refs > 0);
+	if (--mod->refs == 0) {
+		cache_del(mod);
+		schema_module_delete(mod);
 	}
+}
 
-	off_t ret = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size);
-	close(source_fd);
-	close(dest_fd);
-	if (ret != st.st_size) {
-		diag_set(SystemError, "failed to copy DSO %s to %s",
-			 path, load_name);
-		goto error;
+static struct schema_module *
+__schema_module_load(const char *name, size_t len, bool force)
+{
+	struct schema_module *mod = malloc(sizeof(*mod));
+	if (mod != NULL) {
+		mod->module = NULL;
+		mod->refs = 0;
+		rlist_create(&mod->funcs);
+	} else {
+		diag_set(OutOfMemory, sizeof(*mod),
+			 "malloc", "struct schema_module");
+		return NULL;
 	}
 
-	module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
-	if (unlink(load_name) != 0)
-		say_warn("failed to unlink dso link %s", load_name);
-	if (rmdir(dir_name) != 0)
-		say_warn("failed to delete temporary dir %s", dir_name);
-	if (module->handle == NULL) {
-		diag_set(ClientError, ER_LOAD_MODULE, package_len,
-			  package, dlerror());
-		goto error;
+	if (force)
+		mod->module = module_load_force(name, len);
+	else
+		mod->module = module_load(name, len);
+
+	if (!mod->module) {
+		free(mod);
+		return NULL;
 	}
+
 	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
 	if (e != NULL)
 		++e->iparam;
-	return module;
-error:
-	free(module);
-	return NULL;
+
+	schema_module_ref(mod);
+	return mod;
 }
 
-static void
-module_delete(struct module *module)
+/**
+ * Load a new module.
+ */
+static struct schema_module *
+schema_module_load(const char *name, const char *name_end)
 {
-	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
-	if (e != NULL)
-		--e->iparam;
-	dlclose(module->handle);
-	TRASH(module);
-	free(module);
+	return __schema_module_load(name, name_end - name, false);
 }
 
-/*
- * Check if a dso is unused and can be closed.
+/**
+ * Force load a new module.
  */
+static struct schema_module *
+schema_module_load_force(const char *name, const char *name_end)
+{
+	return __schema_module_load(name, name_end - name, true);
+}
+
+static struct func_vtab func_c_vtab;
+
+/** Create a new C function. */
 static void
-module_gc(struct module *module)
+func_c_create(struct func_c *func_c)
 {
-	if (rlist_empty(&module->funcs) && module->calls == 0)
-		module_delete(module);
+	func_c->mod = NULL;
+	func_c->base.vtab = &func_c_vtab;
+	rlist_create(&func_c->item);
+	module_func_create(&func_c->mf);
 }
 
-/*
- * Import a function from the module.
- */
-static box_function_f
-module_sym(struct module *module, const char *name)
+static int
+schema_func_c_load(struct schema_module *mod, const char *func_name,
+		   struct func_c *func)
 {
-	box_function_f f = (box_function_f)dlsym(module->handle, name);
-	if (f == NULL) {
-		diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
-		return NULL;
+	assert(module_func_is_empty(&func->mf));
+
+	if (module_func_load(mod->module, func_name, &func->mf) != 0)
+		return -1;
+
+	func->mod = mod;
+	rlist_move(&mod->funcs, &func->item);
+	schema_module_ref(mod);
+
+	return 0;
+}
+
+static void
+schema_func_c_unload(struct func_c *func)
+{
+	if (!module_func_is_empty(&func->mf)) {
+		rlist_del(&func->item);
+		schema_module_unref(func->mod);
+		module_func_unload(&func->mf);
+		func_c_create(func);
 	}
-	return f;
 }
 
 int
 schema_module_reload(const char *package, const char *package_end)
 {
-	struct module *old_module = module_cache_find(package, package_end);
-	if (old_module == NULL) {
+	struct schema_module *old = cache_find(package, package_end);
+	if (old == NULL) {
 		/* Module wasn't loaded - do nothing. */
 		diag_set(ClientError, ER_NO_SUCH_MODULE, package);
 		return -1;
 	}
 
-	struct module *new_module = module_load(package, package_end);
-	if (new_module == NULL)
+	struct schema_module *new = schema_module_load_force(package, package_end);
+	if (new == NULL)
 		return -1;
 
+	/*
+	 * Keep an extra reference to the old module
+	 * so it won't be freed until reload is complete,
+	 * otherwise we might free old module then fail
+	 * on some function loading and in result won't
+	 * be able to restore old symbols.
+	 */
+	schema_module_ref(old);
 	struct func_c *func, *tmp_func;
-	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
-		/* Move immediately for restore sake. */
-		rlist_move(&new_module->funcs, &func->item);
+	rlist_foreach_entry_safe(func, &old->funcs, item, tmp_func) {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(new_module, name.sym);
-		if (func->func == NULL)
+		schema_func_c_unload(func);
+		if (schema_func_c_load(new, name.sym, func) != 0) {
+			/*
+			 * WARN: A hack accessing new->funcs directly
+			 * to start restore from this failing function.
+			 */
+			rlist_move(&new->funcs, &func->item);
 			goto restore;
-		func->module = new_module;
+		}
 	}
-	module_cache_del(package, package_end);
-	if (module_cache_put(new_module) != 0)
-		goto restore;
-	module_gc(old_module);
+	cache_update(new);
+	schema_module_unref(old);
+	schema_module_unref(new);
 	return 0;
 restore:
 	/*
 	 * Some old functions are not found int the new module,
 	 * thus restore all migrated functions back to original.
 	 */
-	rlist_foreach_entry_safe(func, &new_module->funcs, item, tmp_func) {
+	rlist_foreach_entry_safe(func, &new->funcs, item, tmp_func) {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(old_module, name.sym);
-		if (func->func == NULL) {
+		schema_func_c_unload(func);
+		if (schema_func_c_load(old, name.sym, func) != 0) {
 			/*
 			 * Something strange was happen, an early loaden
 			 * function was not found in an old dso.
@@ -418,10 +384,9 @@ schema_module_reload(const char *package, const char *package_end)
 			panic("Can't restore module function, "
 			      "server state is inconsistent");
 		}
-		func->module = old_module;
-		rlist_move(&old_module->funcs, &func->item);
 	}
-	module_delete(new_module);
+	schema_module_unref(old);
+	schema_module_unref(new);
 	return -1;
 }
 
@@ -469,8 +434,6 @@ func_new(struct func_def *def)
 	return func;
 }
 
-static struct func_vtab func_c_vtab;
-
 static struct func *
 func_c_new(MAYBE_UNUSED struct func_def *def)
 {
@@ -481,35 +444,17 @@ func_c_new(MAYBE_UNUSED struct func_def *def)
 		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
 		return NULL;
 	}
-	func->base.vtab = &func_c_vtab;
-	func->func = NULL;
-	func->module = NULL;
+	func_c_create(func);
 	return &func->base;
 }
 
-static void
-func_c_unload(struct func_c *func)
-{
-	if (func->module) {
-		rlist_del(&func->item);
-		if (rlist_empty(&func->module->funcs)) {
-			struct func_name name;
-			func_split_name(func->base.def->name, &name);
-			module_cache_del(name.package, name.package_end);
-		}
-		module_gc(func->module);
-	}
-	func->module = NULL;
-	func->func = NULL;
-}
-
 static void
 func_c_destroy(struct func *base)
 {
 	assert(base->vtab == &func_c_vtab);
 	assert(base != NULL && base->def->language == FUNC_LANGUAGE_C);
 	struct func_c *func = (struct func_c *) base;
-	func_c_unload(func);
+	schema_func_c_unload(func);
 	TRASH(base);
 	free(func);
 }
@@ -521,45 +466,32 @@ func_c_destroy(struct func *base)
 static int
 func_c_load(struct func_c *func)
 {
-	assert(func->func == NULL);
-
 	struct func_name name;
 	func_split_name(func->base.def->name, &name);
 
-	struct module *cached, *module;
-	cached = module_cache_find(name.package, name.package_end);
+	struct schema_module *cached, *mod;
+	cached = cache_find(name.package, name.package_end);
 	if (cached == NULL) {
-		module = module_load(name.package, name.package_end);
-		if (module == NULL)
+		mod = schema_module_load(name.package, name.package_end);
+		if (mod == NULL)
 			return -1;
-		if (module_cache_put(module)) {
-			module_delete(module);
+		if (cache_put(mod)) {
+			schema_module_unref(mod);
 			return -1;
 		}
 	} else {
-		module = cached;
+		mod = cached;
+		schema_module_ref(mod);
 	}
 
-	func->func = module_sym(module, name.sym);
-	if (func->func == NULL) {
-		if (cached == NULL) {
-			/*
-			 * In case if it was a first load we should
-			 * clean the cache immediately otherwise
-			 * the module continue being referenced even
-			 * if there will be no use of it.
-			 *
-			 * Note the module_sym set an error thus be
-			 * careful to not wipe it.
-			 */
-			module_cache_del(name.package, name.package_end);
-			module_delete(module);
-		}
-		return -1;
-	}
-	func->module = module;
-	rlist_add(&module->funcs, &func->item);
-	return 0;
+	int rc = schema_func_c_load(mod, name.sym, func);
+	/*
+	 * There is no explicit module loading in this
+	 * interface so each function carries a reference
+	 * by their own.
+	 */
+	schema_module_unref(mod);
+	return rc;
 }
 
 int
@@ -568,38 +500,17 @@ func_c_call(struct func *base, struct port *args, struct port *ret)
 	assert(base->vtab == &func_c_vtab);
 	assert(base != NULL && base->def->language == FUNC_LANGUAGE_C);
 	struct func_c *func = (struct func_c *) base;
-	if (func->func == NULL) {
+	if (module_func_is_empty(&func->mf)) {
 		if (func_c_load(func) != 0)
 			return -1;
 	}
-
-	struct region *region = &fiber()->gc;
-	size_t region_svp = region_used(region);
-	uint32_t data_sz;
-	const char *data = port_get_msgpack(args, &data_sz);
-	if (data == NULL)
-		return -1;
-
-	port_c_create(ret);
-	box_function_ctx_t ctx = { ret };
-
-	/* Module can be changed after function reload. */
-	struct module *module = func->module;
-	assert(module != NULL);
-	++module->calls;
-	int rc = func->func(&ctx, data, data + data_sz);
-	--module->calls;
-	module_gc(module);
-	region_truncate(region, region_svp);
-	if (rc != 0) {
-		if (diag_last_error(&fiber()->diag) == NULL) {
-			/* Stored procedure forget to set diag  */
-			diag_set(ClientError, ER_PROC_C, "unknown error");
-		}
-		port_destroy(ret);
-		return -1;
-	}
-	return rc;
+	/*
+	 * Note that we don't take a reference to the
+	 * module, it is handled by low level instance,
+	 * thus while been inside the call the associated
+	 * schema_module can be unreferenced and freed.
+	 */
+	return module_func_call(&func->mf, args, ret);
 }
 
 static struct func_vtab func_c_vtab = {
diff --git a/src/box/func.h b/src/box/func.h
index 5a49e34f4..987ca70db 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -37,6 +37,7 @@
 #include "small/rlist.h"
 #include "func_def.h"
 #include "user_def.h"
+#include "module_cache.h"
 
 #if defined(__cplusplus)
 extern "C" {
@@ -44,20 +45,6 @@ extern "C" {
 
 struct func;
 
-/**
- * Dynamic shared module.
- */
-struct module {
-	/** Module dlhandle. */
-	void *handle;
-	/** List of imported functions. */
-	struct rlist funcs;
-	/** Count of active calls. */
-	size_t calls;
-	/** Module's package name. */
-	char package[0];
-};
-
 /** Virtual method table for func object. */
 struct func_vtab {
 	/** Call function with given arguments. */
diff --git a/src/box/func_def.h b/src/box/func_def.h
index d99d89190..75cd6a0d3 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -168,20 +168,6 @@ func_def_dup(struct func_def *def);
 int
 func_def_check(struct func_def *def);
 
-/**
- * API of C stored function.
- */
-
-struct port;
-
-struct box_function_ctx {
-	struct port *port;
-};
-
-typedef struct box_function_ctx box_function_ctx_t;
-typedef int (*box_function_f)(box_function_ctx_t *ctx,
-	     const char *args, const char *args_end);
-
 #ifdef __cplusplus
 }
 #endif
diff --git a/test/box/gh-4648-func-load-unload.result b/test/box/gh-4648-func-load-unload.result
index e695dd365..91b78d083 100644
--- a/test/box/gh-4648-func-load-unload.result
+++ b/test/box/gh-4648-func-load-unload.result
@@ -71,7 +71,8 @@ check_module_count_diff(-1)
  | ...
 
 -- A not finished invocation of a function from a module prevents
--- its unload. Until the call is finished.
+-- low level module intance unload while schema level module is
+-- free to unload immediately when dropped.
 box.schema.func.create('function1', {language = 'C'})
  | ---
  | ...
@@ -110,7 +111,7 @@ box.schema.func.drop('function1')
 box.schema.func.drop('function1.test_sleep')
  | ---
  | ...
-check_module_count_diff(0)
+check_module_count_diff(-1)
  | ---
  | ...
 
@@ -132,6 +133,6 @@ test_run:wait_cond(function() return f2:status() == 'dead' end)
  | ---
  | - true
  | ...
-check_module_count_diff(-1)
+check_module_count_diff(0)
  | ---
  | ...
diff --git a/test/box/gh-4648-func-load-unload.test.lua b/test/box/gh-4648-func-load-unload.test.lua
index 10b9de87a..4c4f30af7 100644
--- a/test/box/gh-4648-func-load-unload.test.lua
+++ b/test/box/gh-4648-func-load-unload.test.lua
@@ -36,7 +36,8 @@ box.schema.func.drop('function1')
 check_module_count_diff(-1)
 
 -- A not finished invocation of a function from a module prevents
--- its unload. Until the call is finished.
+-- low level module intance unload while schema level module is
+-- free to unload immediately when dropped.
 box.schema.func.create('function1', {language = 'C'})
 box.schema.func.create('function1.test_sleep', {language = 'C'})
 check_module_count_diff(0)
@@ -52,7 +53,7 @@ box.func.function1:call()
 check_module_count_diff(1)
 box.schema.func.drop('function1')
 box.schema.func.drop('function1.test_sleep')
-check_module_count_diff(0)
+check_module_count_diff(-1)
 
 f1:cancel()
 test_run:wait_cond(function() return f1:status() == 'dead' end)
@@ -60,4 +61,4 @@ check_module_count_diff(0)
 
 f2:cancel()
 test_run:wait_cond(function() return f2:status() == 'dead' end)
-check_module_count_diff(-1)
+check_module_count_diff(0)
-- 
2.30.2

[Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module

[Cyrill Gorcunov via Tarantool-patches]

Currently to run "C" function from some external module one
have to register it first in "_func" system space. This is
a problem if node is in read-only mode (replica).

Still people would like to have a way to run such functions
even in ro mode. For this sake we implement "box.lib" lua module.

Unlike `box.schema.func` interface the `box.lib` does not defer module
loading procedure until first call of a function. Instead a module
is loaded immediately and if some error happens (say shared
library is corrupted or not found) it pops up early.

The need of use stored C procedures implies that application is
running under serious loads most likely there is modular structure
present on Lua level (ie same shared library is loaded in different
sources) thus we cache the loaded library and reuse it on next
load attempts. To verify that cached library is up to day the
module_cache engine test for file attributes (device, inode, size,
modification time) on every load attempt.

Since both `box.schema.func` and `box.lib` are using caching to minimize
module loading procedure the pass-through caching scheme is
implemented:

 - box.lib relies on module_cache engine for caching;
 - box.schema.func does snoop into box.lib hash table when attempt
   to load a new module, if module is present in box.lib hash then
   it simply referenced from there and added into own hash table;
   in case if module is not present then it loaded from the scratch
   and put into both hashes;
 - the module_reload action in box.schema.func invalidates module_cache
   or fill it if entry is not present.

Closes #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

@TarantoolBot document
Title: box.lib module

Overview
========

`box.lib` module provides a way to create, delete and execute
`C` procedures from shared libraries. Unlike `box.schema.func`
methods the functions created with `box.lib` help are not persistent
and live purely in memory. Once a node get turned off they are
vanished. An initial purpose for them is to execute them on
nodes which are running in read-only mode.

Module functions
================

`require('box.lib').load(path) -> obj | error`
----------------------------------------------

Loads a module from `path` and return an object instance
associate with the module, otherwise an error is thrown.

The `path` should not end up with shared library extension
(such as `.so`), only a file name shall be there.

Possible errors:

- IllegalParams: module path is either not supplied
  or not a string.
- SystemError: unable to open a module due to a system error.
- ClientError: a module does not exist.
- OutOfMemory: unable to allocate a module.

Example:

``` Lua
-- Without error handling
m = require('box.lib').load('path/to/library)

-- With error handling
m, err = pcall(require('box.lib').load, 'path/to/library')
if err ~= nil then
    print(err)
end
```

`module:unload() -> true | error`
---------------------------------

Unloads a module. Returns `true` on success, otherwise an error
is thrown. Once the module is unloaded one can't load new
functions from this module instance.

Possible errors:

- IllegalParams: a module is not supplied.
- IllegalParams: a module is already unloaded.

Example:

``` Lua
m = require('box.lib').load('path/to/library')
--
-- do something with module
--
m:unload()
```

If there are functions from this module referenced somewhere
in other places of Lua code they still can be executed because
the module continue sitting in memory until the last reference
to it is closed.

If the module become a target to the Lua's garbage collector
then unload is called implicitly.

module:load(name) -> obj | error`
---------------------------------

Loads a new function with name `name` from the previously
loaded `module` and return a callable object instance
associated with the function. On failure an error is thrown.

Possible errors:
 - IllegalParams: function name is either not supplied
   or not a string.
 - IllegalParams: attempt to load a function but module
   has been unloaded already.
 - ClientError: no such function in the module.
 - OutOfMemory: unable to allocate a function.

Example:

``` Lua
-- Load a module if not been loaded yet.
m = require('box.lib').load('path/to/library')
-- Load a function with the `foo` name from the module `m`.
func = m:load('foo')
```

In case if there is no need for further loading of other
functions from the same module then the module might be
unloaded immediately.

``` Lua
m = require('box.lib').load('path/to/library')
func = m:load('foo')
m:unload()
```

`function:unload() -> true | error`
-----------------------------------

Unloads a function. Returns `true` on success, otherwise
an error is thrown.

Possible errors:
 - IllegalParams: function name is either not supplied
   or not a string.
 - IllegalParams: the function already unloaded.

Example:

``` Lua
m = require('box.lib').load('path/to/library')
func = m:load('foo')
--
-- do something with function and cleanup then
--
func:unload()
m:unload()
```

If the function become a target to the Lua's garbage collector
then unload is called implicitly.

Executing a loaded function
===========================

Once function is loaded it can be executed as an ordinary Lua call.
Lets consider the following example. We have a `C` function which
takes two numbers and returns their sum.

``` C
int
cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
	uint32_t arg_count = mp_decode_array(&args);
	if (arg_count != 2) {
		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
				     "invalid argument count");
	}
	uint64_t a = mp_decode_uint(&args);
	uint64_t b = mp_decode_uint(&args);

	char res[16];
	char *end = mp_encode_uint(res, a + b);
	box_return_mp(ctx, res, end);
	return 0;
}
```

The name of the function is `cfunc_sum` and the function is built into
`cfunc.so` shared library.

First we should load it as

``` Lua
m = require('box.lib').load('cfunc')
cfunc_sum = m:load('cfunc_sum')
```

Once successfully loaded we can execute it. Lets call the
`cfunc_sum` with wrong number of arguments

``` Lua
cfunc_sum()
 | ---
 | - error: invalid argument count
```

We will see the `"invalid argument count"` message in output.
The error message has been set by the `box_error_set` in `C`
code above.

On success the sum of arguments will be printed out.

``` Lua
cfunc_sum(1, 2)
 | ---
 | - 3
```

The functions may return multiple results. For example a trivial
echo function which prints arguments passed in.

``` Lua
cfunc_echo(1,2,3)
 | ---
 | - 1
 | - 2
 | - 3
```

Module and function caches
==========================

Loading a module is relatively slow procedure because operating
system needs to read the library, resolve its symbols and etc.
Thus to speedup this procedure if the module is loaded for a first
time we put it into an internal cache. If module is sitting in
the cache already and new request to load comes in -- we simply
reuse a previous copy. In case if module is updated on a storage
device then on new load attempt we detect that file attributes
(such as device number, inode, size, modification time) get changed
and reload module from the scratch. Note that newly loaded module
does not intersect with previously loaded modules, the continue
operating with code previously read from cache.

Thus if there is a need to update a module then all module instances
should be unloaded (together with functions) and loaded again.

Similar caching technique applied to functions -- only first function
allocation cause symbol resolving, next ones are simply obtained from
a function cache.
---
 src/box/CMakeLists.txt |   1 +
 src/box/lua/box_lib.c  | 590 +++++++++++++++++++++++++++++++++++++++++
 src/box/lua/box_lib.h  |  25 ++
 src/box/lua/init.c     |   2 +
 test/box/misc.result   |   1 +
 5 files changed, 619 insertions(+)
 create mode 100644 src/box/lua/box_lib.c
 create mode 100644 src/box/lua/box_lib.h

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index cc2e17e94..95c65eb71 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -196,6 +196,7 @@ add_library(box STATIC
     lua/call.c
     lua/cfg.cc
     lua/console.c
+    lua/box_lib.c
     lua/serialize_lua.c
     lua/tuple.c
     lua/slab.c
diff --git a/src/box/lua/box_lib.c b/src/box/lua/box_lib.c
new file mode 100644
index 000000000..ce2ef8902
--- /dev/null
+++ b/src/box/lua/box_lib.c
@@ -0,0 +1,590 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#include <unistd.h>
+#include <string.h>
+#include <lua.h>
+
+#include "box/error.h"
+#include "box/port.h"
+
+#include "tt_static.h"
+
+#include "assoc.h"
+#include "box_lib.h"
+#include "diag.h"
+#include "module_cache.h"
+
+#include "lua/utils.h"
+
+/**
+ * Function descriptor.
+ */
+struct box_module_func {
+	/** C function to call. */
+	struct module_func mf;
+	/** Number of references. */
+	int64_t refs;
+	/** Length of functon name in @a key. */
+	size_t sym_len;
+	/** Length of @a key. */
+	size_t len;
+	/** Function hash key. */
+	char key[0];
+};
+
+/** Function name to box_module_func hash. */
+static struct mh_strnptr_t *func_hash = NULL;
+
+/** A type to find a module from an object. */
+static const char *uname_lib = "tt_uname_box_lib";
+
+/** A type to find a function from an object. */
+static const char *uname_func = "tt_uname_box_lib_func";
+
+/** Get data associated with an object. */
+static void *
+get_udata(struct lua_State *L, const char *uname)
+{
+	void **pptr = luaL_testudata(L, 1, uname);
+	return pptr != NULL ? *pptr : NULL;
+}
+
+/** Set data to a new value. */
+static void
+set_udata(struct lua_State *L, const char *uname, void *ptr)
+{
+	void **pptr = luaL_testudata(L, 1, uname);
+	assert(pptr != NULL);
+	*pptr = ptr;
+}
+
+/** Setup a new data and associate it with an object. */
+static void
+new_udata(struct lua_State *L, const char *uname, void *ptr)
+{
+	*(void **)lua_newuserdata(L, sizeof(void *)) = ptr;
+	luaL_getmetatable(L, uname);
+	lua_setmetatable(L, -2);
+}
+
+/**
+ * Helpers for function cache.
+ */
+static void *
+cache_find(const char *str, size_t len)
+{
+	mh_int_t e = mh_strnptr_find_inp(func_hash, str, len);
+	if (e == mh_end(func_hash))
+		return NULL;
+	return mh_strnptr_node(func_hash, e)->val;
+}
+
+static int
+cache_put(struct box_module_func *cf)
+{
+	const struct mh_strnptr_node_t nd = {
+		.str	= cf->key,
+		.len	= cf->len,
+		.hash	= mh_strn_hash(cf->key, cf->len),
+		.val	= cf,
+	};
+	mh_int_t e = mh_strnptr_put(func_hash, &nd, NULL, NULL);
+	if (e == mh_end(func_hash)) {
+		diag_set(OutOfMemory, sizeof(nd), "malloc",
+			 "box.lib: hash node");
+		return -1;
+	}
+	return 0;
+}
+
+static void
+cache_del(struct box_module_func *cf)
+{
+	mh_int_t e = mh_strnptr_find_inp(func_hash, cf->key, cf->len);
+	if (e != mh_end(func_hash))
+		mh_strnptr_del(func_hash, e, NULL);
+}
+
+/**
+ * Load a module.
+ *
+ * This function takes a module path from the caller
+ * stack @a L and returns cached module instance or
+ * creates a new module object.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: module path is either not supplied
+ *   or not a string.
+ * - SystemError: unable to open a module due to a system error.
+ * - ClientError: a module does not exist.
+ * - OutOfMemory: unable to allocate a module.
+ *
+ * @returns module object on success or throws an error.
+ */
+static int
+lbox_module_load(struct lua_State *L)
+{
+	const char msg_noname[] = "Expects box.lib.load(\'name\') "
+		"but no name passed";
+
+	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
+		diag_set(IllegalParams, msg_noname);
+		return luaT_error(L);
+	}
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+
+	if (name_len < 1) {
+		diag_set(IllegalParams, msg_noname);
+		return luaT_error(L);
+	}
+
+	struct module *m = module_load(name, name_len);
+	if (m == NULL)
+		return luaT_error(L);
+
+	new_udata(L, uname_lib, m);
+	return 1;
+}
+
+/**
+ * Unload a module.
+ *
+ * Take a module object from the caller stack @a L and unload it.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: module is not supplied.
+ * - IllegalParams: the module is unloaded.
+ *
+ * @returns true on success or throwns an error.
+ */
+static int
+lbox_module_unload(struct lua_State *L)
+{
+	if (lua_gettop(L) != 1) {
+		diag_set(IllegalParams, "Expects module:unload()");
+		return luaT_error(L);
+	}
+
+	struct module *m = get_udata(L, uname_lib);
+	if (m == NULL) {
+		diag_set(IllegalParams, "The module is unloaded");
+		return luaT_error(L);
+	}
+
+	set_udata(L, uname_lib, NULL);
+	module_unload(m);
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/** Handle __index request for a module object. */
+static int
+lbox_module_index(struct lua_State *L)
+{
+	lua_getmetatable(L, 1);
+	lua_pushvalue(L, 2);
+	lua_rawget(L, -2);
+	if (!lua_isnil(L, -1))
+		return 1;
+
+	struct module *m = get_udata(L, uname_lib);
+	if (m == NULL) {
+		lua_pushnil(L);
+		return 1;
+	}
+
+	const char *key = lua_tostring(L, 2);
+	if (key == NULL || lua_type(L, 2) != LUA_TSTRING) {
+		diag_set(IllegalParams,
+			 "Bad params, use __index(<key>)");
+		return luaT_error(L);
+	}
+
+	if (strcmp(key, "path") == 0) {
+		lua_pushstring(L, m->package);
+		return 1;
+	}
+
+	/*
+	 * Internal keys for debug only, not API.
+	 */
+	if (strcmp(key, "debug_refs") == 0) {
+		lua_pushnumber(L, m->refs);
+		return 1;
+	} else if (strcmp(key, "debug_ptr") == 0) {
+		const char *s = tt_sprintf("%p", m);
+		lua_pushstring(L, s);
+		return 1;
+	}
+	return 0;
+}
+
+/** Module representation for REPL (console). */
+static int
+lbox_module_serialize(struct lua_State *L)
+{
+	struct module *m = get_udata(L, uname_lib);
+	if (m == NULL) {
+		lua_pushnil(L);
+		return 1;
+	}
+
+	lua_createtable(L, 0, 1);
+	lua_pushstring(L, m->package);
+	lua_setfield(L, -2, "path");
+	return 1;
+}
+
+/** Collect a module. */
+static int
+lbox_module_gc(struct lua_State *L)
+{
+	struct module *m = get_udata(L, uname_lib);
+	if (m != NULL) {
+		set_udata(L, uname_lib, NULL);
+		module_unload(m);
+	}
+	return 0;
+}
+
+/** Increase reference to a function. */
+static void
+box_module_func_ref(struct box_module_func *cf)
+{
+	assert(cf->refs >= 0);
+	++cf->refs;
+}
+
+/** Free function memory. */
+static void
+box_module_func_delete(struct box_module_func *cf)
+{
+	assert(module_func_is_empty(&cf->mf));
+	TRASH(cf);
+	free(cf);
+}
+
+/** Unreference a function and free if last one. */
+static void
+box_module_func_unref(struct box_module_func *cf)
+{
+	assert(cf->refs > 0);
+	if (--cf->refs == 0) {
+		module_func_unload(&cf->mf);
+		cache_del(cf);
+		box_module_func_delete(cf);
+	}
+}
+
+/** Function name from a hash key. */
+static char *
+box_module_func_name(struct box_module_func *cf)
+{
+	return &cf->key[cf->len - cf->sym_len];
+}
+
+/**
+ * Allocate a new function instance and resolve its address.
+ *
+ * @param m a module the function should be loaded from.
+ * @param key function hash key, ie "addr.module.foo".
+ * @param len length of @a key.
+ * @param sym_len function symbol name length, ie 3 for "foo".
+ *
+ * @returns function instance on success, NULL otherwise (diag is set).
+ */
+static struct box_module_func *
+box_module_func_new(struct module *m, const char *key, size_t len, size_t sym_len)
+{
+	size_t size = sizeof(struct box_module_func) + len + 1;
+	struct box_module_func *cf = malloc(size);
+	if (cf == NULL) {
+		diag_set(OutOfMemory, size, "malloc", "cf");
+		return NULL;
+	}
+
+	cf->len = len;
+	cf->sym_len = sym_len;
+	cf->refs = 0;
+
+	module_func_create(&cf->mf);
+	memcpy(cf->key, key, len);
+	cf->key[len] = '\0';
+
+	if (module_func_load(m, box_module_func_name(cf), &cf->mf) != 0) {
+		box_module_func_delete(cf);
+		return NULL;
+	}
+
+	if (cache_put(cf) != 0) {
+		module_func_unload(&cf->mf);
+		box_module_func_delete(cf);
+		return NULL;
+	}
+
+	/*
+	 * Each new function depends on module presence.
+	 * Module will reside even if been unload
+	 * explicitly after the function creation.
+	 */
+	box_module_func_ref(cf);
+	return cf;
+}
+
+/**
+ * Load a function.
+ *
+ * This function takes a function name from the caller
+ * stack @a L and either returns a cached function or
+ * creates a new function object.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: function name is either not supplied
+ *   or not a string.
+ * - SystemError: unable to open a module due to a system error.
+ * - ClientError: a module does not exist.
+ * - OutOfMemory: unable to allocate a module.
+ *
+ * @returns module object on success or throws an error.
+ */
+static int
+lbox_module_load_func(struct lua_State *L)
+{
+	const char *method = "function = module:load";
+	const char fmt_noname[] = "Expects %s(\'name\') but no name passed";
+
+	if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) {
+		diag_set(IllegalParams, fmt_noname, method);
+		return luaT_error(L);
+	}
+
+	struct module *m = get_udata(L, uname_lib);
+	if (m == NULL) {
+		const char *fmt =
+			"Expects %s(\'name\') but not module object passed";
+		diag_set(IllegalParams, fmt, method);
+		return luaT_error(L);
+	}
+
+	size_t sym_len;
+	const char *sym = lua_tolstring(L, 2, &sym_len);
+
+	if (sym_len < 1) {
+		diag_set(IllegalParams, fmt_noname, method);
+		return luaT_error(L);
+	}
+
+	/*
+	 * Functions are bound to a module symbols, thus
+	 * since the hash is global it should be unique
+	 * per module. The symbol (function name) is the
+	 * last part of the hash key.
+	 */
+	const char *key = tt_sprintf("%p.%s.%s", (void *)m,
+				     m->package, sym);
+	size_t len = strlen(key);
+
+	struct box_module_func *cf = cache_find(key, len);
+	if (cf == NULL) {
+		cf = box_module_func_new(m, key, len, sym_len);
+		if (cf == NULL)
+			return luaT_error(L);
+	} else {
+		box_module_func_ref(cf);
+	}
+
+	new_udata(L, uname_func, cf);
+	return 1;
+}
+
+/**
+ * Unload a function.
+ *
+ * Take a function object from the caller stack @a L and unload it.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: the function is not supplied.
+ * - IllegalParams: the function already unloaded.
+ *
+ * @returns true on success or throwns an error.
+ */
+static int
+lbox_func_unload(struct lua_State *L)
+{
+	if (lua_gettop(L) != 1) {
+		diag_set(IllegalParams, "Expects function:unload()");
+		return luaT_error(L);
+	}
+
+	struct box_module_func *cf = get_udata(L, uname_func);
+	if (cf == NULL) {
+		diag_set(IllegalParams, "The function is unloaded");
+		return luaT_error(L);
+	}
+
+	set_udata(L, uname_func, NULL);
+	box_module_func_unref(cf);
+
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/** Handle __index request for a function object. */
+static int
+lbox_func_index(struct lua_State *L)
+{
+	lua_getmetatable(L, 1);
+	lua_pushvalue(L, 2);
+	lua_rawget(L, -2);
+	if (!lua_isnil(L, -1))
+		return 1;
+
+	struct box_module_func *cf = get_udata(L, uname_func);
+	if (cf == NULL) {
+		lua_pushnil(L);
+		return 1;
+	}
+
+	const char *key = lua_tostring(L, 2);
+	if (key == NULL || lua_type(L, 2) != LUA_TSTRING) {
+		diag_set(IllegalParams,
+			 "Bad params, use __index(<key>)");
+		return luaT_error(L);
+	}
+
+	if (strcmp(key, "name") == 0) {
+		lua_pushstring(L, box_module_func_name(cf));
+		return 1;
+	}
+
+	/*
+	 * Internal keys for debug only, not API.
+	 */
+	if (strcmp(key, "debug_refs") == 0) {
+		lua_pushnumber(L, cf->refs);
+		return 1;
+	} else if (strcmp(key, "debug_key") == 0) {
+		lua_pushstring(L, cf->key);
+		return 1;
+	} else if (strcmp(key, "debug_module_ptr") == 0) {
+		const char *s = tt_sprintf("%p", cf->mf.module);
+		lua_pushstring(L, s);
+		return 1;
+	} else if (strcmp(key, "debug_module_refs") == 0) {
+		lua_pushnumber(L, cf->mf.module->refs);
+		return 1;
+	}
+	return 0;
+}
+
+/** Function representation for REPL (console). */
+static int
+lbox_func_serialize(struct lua_State *L)
+{
+	struct box_module_func *cf = get_udata(L, uname_func);
+	if (cf == NULL) {
+		lua_pushnil(L);
+		return 1;
+	}
+
+	lua_createtable(L, 0, 1);
+	lua_pushstring(L, box_module_func_name(cf));
+	lua_setfield(L, -2, "name");
+	return 1;
+}
+
+/** Collect a function. */
+static int
+lbox_func_gc(struct lua_State *L)
+{
+	struct box_module_func *cf = get_udata(L, uname_func);
+	if (cf != NULL) {
+		set_udata(L, uname_func, NULL);
+		box_module_func_unref(cf);
+	}
+	return 0;
+}
+
+
+/** Call a function by its name from the Lua code. */
+static int
+lbox_func_call(struct lua_State *L)
+{
+	struct box_module_func *cf = get_udata(L, uname_func);
+	if (cf == NULL) {
+		diag_set(IllegalParams, "The function is unloaded");
+		return luaT_error(L);
+	}
+
+	lua_State *args_L = luaT_newthread(tarantool_L);
+	if (args_L == NULL)
+		return luaT_error(L);
+
+	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+	lua_xmove(L, args_L, lua_gettop(L) - 1);
+
+	struct port args;
+	port_lua_create(&args, args_L);
+	((struct port_lua *)&args)->ref = coro_ref;
+
+	struct port ret;
+
+	if (module_func_call(&cf->mf, &args, &ret) != 0) {
+		port_destroy(&args);
+		return luaT_error(L);
+	}
+
+	int top = lua_gettop(L);
+	port_dump_lua(&ret, L, true);
+	int cnt = lua_gettop(L) - top;
+
+	port_destroy(&ret);
+	port_destroy(&args);
+
+	return cnt;
+}
+
+void
+box_lua_lib_init(struct lua_State *L)
+{
+	func_hash = mh_strnptr_new();
+	if (func_hash == NULL)
+		panic("box.lib: Can't allocate func hash table");
+
+	static const struct luaL_Reg top_methods[] = {
+		{ "load",		lbox_module_load	},
+		{ NULL, NULL },
+	};
+	luaL_register(L, "box.lib", top_methods);
+	lua_pop(L, 1);
+
+	static const struct luaL_Reg lbox_module_methods[] = {
+		{ "unload",		lbox_module_unload	},
+		{ "load",		lbox_module_load_func	},
+		{ "__index",		lbox_module_index	},
+		{ "__serialize",	lbox_module_serialize	},
+		{ "__gc",		lbox_module_gc		},
+		{ NULL, NULL },
+	};
+	luaL_register_type(L, uname_lib, lbox_module_methods);
+
+	static const struct luaL_Reg lbox_func_methods[] = {
+		{ "unload",		lbox_func_unload	},
+		{ "__index",		lbox_func_index		},
+		{ "__serialize",	lbox_func_serialize	},
+		{ "__gc",		lbox_func_gc		},
+		{ "__call",		lbox_func_call		},
+		{ NULL, NULL },
+	};
+	luaL_register_type(L, uname_func, lbox_func_methods);
+}
diff --git a/src/box/lua/box_lib.h b/src/box/lua/box_lib.h
new file mode 100644
index 000000000..a86691b42
--- /dev/null
+++ b/src/box/lua/box_lib.h
@@ -0,0 +1,25 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#pragma once
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct lua_State;
+
+/**
+ * Initialize Lua box.lib.
+ *
+ * @param L Lua state where to register.
+ */
+void
+box_lua_lib_init(struct lua_State *L);
+
+#if defined(__cplusplus)
+}
+#endif /* defined(__plusplus) */
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index fbcdfb20b..3fb433e7a 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -60,6 +60,7 @@
 #include "box/lua/cfg.h"
 #include "box/lua/xlog.h"
 #include "box/lua/console.h"
+#include "box/lua/box_lib.h"
 #include "box/lua/tuple.h"
 #include "box/lua/execute.h"
 #include "box/lua/key_def.h"
@@ -465,6 +466,7 @@ box_lua_init(struct lua_State *L)
 	box_lua_tuple_init(L);
 	box_lua_call_init(L);
 	box_lua_cfg_init(L);
+	box_lua_lib_init(L);
 	box_lua_slab_init(L);
 	box_lua_index_init(L);
 	box_lua_space_init(L);
diff --git a/test/box/misc.result b/test/box/misc.result
index e18a46e02..59fc60a22 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -85,6 +85,7 @@ t
   - info
   - internal
   - is_in_txn
+  - lib
   - on_commit
   - on_rollback
   - once
-- 
2.30.2

[Tarantool-patches] [PATCH v20 7/7] test: add box.lib test

[Cyrill Gorcunov via Tarantool-patches]

Note that the test is disabled in suite.ini for a while
because we need to update test-run first like

 | diff --git a/pretest_clean.lua b/pretest_clean.lua
 | index 446ecf2..b356ebd 100644
 | --- a/pretest_clean.lua
 | +++ b/pretest_clean.lua
 | @@ -228,6 +228,7 @@ local function clean()
 |          ['box.internal.sequence'] = true,
 |          ['box.internal.session'] = true,
 |          ['box.internal.space'] = true,
 | +        ['box.lib'] = true,
 |          buffer = true,
 |          clock = true,
 |          console = true,

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/box/CMakeLists.txt |   4 +
 test/box/cfunc1.c       |  58 +++++
 test/box/cfunc2.c       | 137 +++++++++++
 test/box/cfunc3.c       |  25 ++
 test/box/cfunc4.c       |  28 +++
 test/box/lib.result     | 530 ++++++++++++++++++++++++++++++++++++++++
 test/box/lib.test.lua   | 203 +++++++++++++++
 test/box/suite.ini      |   2 +-
 8 files changed, 986 insertions(+), 1 deletion(-)
 create mode 100644 test/box/cfunc1.c
 create mode 100644 test/box/cfunc2.c
 create mode 100644 test/box/cfunc3.c
 create mode 100644 test/box/cfunc4.c
 create mode 100644 test/box/lib.result
 create mode 100644 test/box/lib.test.lua

diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 944831af2..c9dcbbb7b 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -5,3 +5,7 @@ build_module(reload2 reload2.c)
 build_module(func_restore1 func_restore1.c)
 build_module(func_restore2 func_restore2.c)
 build_module(tuple_bench tuple_bench.c)
+build_module(cfunc1 cfunc1.c)
+build_module(cfunc2 cfunc2.c)
+build_module(cfunc3 cfunc3.c)
+build_module(cfunc4 cfunc4.c)
diff --git a/test/box/cfunc1.c b/test/box/cfunc1.c
new file mode 100644
index 000000000..f6829372a
--- /dev/null
+++ b/test/box/cfunc1.c
@@ -0,0 +1,58 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Before the reload functions are just declared
+ * and simply exit with zero.
+ *
+ * After the module reload we should provide real
+ * functionality.
+ */
+
+int
+cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
+
+int
+cfunc_fetch_seq_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
+
+int
+cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
+
+int
+cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
+
+int
+cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
diff --git a/test/box/cfunc2.c b/test/box/cfunc2.c
new file mode 100644
index 000000000..8c583e993
--- /dev/null
+++ b/test/box/cfunc2.c
@@ -0,0 +1,137 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Just make sure we've been called.
+ */
+int
+cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
+
+/*
+ * Fetch first N even numbers (just to make sure the order of
+ * arguments is not screwed).
+ */
+int
+cfunc_fetch_seq_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	int arg_count = mp_decode_array(&args);
+	if (arg_count != 1) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+				     "invalid argument count");
+	}
+	int field_count = mp_decode_array(&args);
+	if (field_count < 1) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+				     "invalid array size");
+	}
+
+	/*
+	 * We expect even numbers sequence here. The idea is
+	 * to test invalid data an issue an error from inside
+	 * of C function.
+	 */
+	for (int i = 1; i <= field_count; i++) {
+		int val = mp_decode_uint(&args);
+		int needed = 2 * i;
+		if (val != needed) {
+			char res[128];
+			snprintf(res, sizeof(res), "%s %d != %d",
+				 "invalid argument", val, needed);
+			return box_error_set(__FILE__, __LINE__,
+					     ER_PROC_C, "%s", res);
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Return one element array twice.
+ */
+int
+cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	char tuple_buf[512];
+	char *d = tuple_buf;
+	d = mp_encode_array(d, 1);
+	d = mp_encode_uint(d, 1);
+	assert(d <= tuple_buf + sizeof(tuple_buf));
+
+	box_tuple_format_t *fmt = box_tuple_format_default();
+	box_tuple_t *tuple_a = box_tuple_new(fmt, tuple_buf, d);
+	if (tuple_a == NULL)
+		return -1;
+	int rc = box_return_tuple(ctx, tuple_a);
+	if (rc == 0)
+		return box_return_tuple(ctx, tuple_a);
+	return rc;
+}
+
+/*
+ * Encode int + string pair back.
+ */
+int
+cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	uint32_t arg_count = mp_decode_array(&args);
+	if (arg_count != 2) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+				     "invalid argument count");
+	}
+
+	if (mp_typeof(*args) != MP_UINT) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+				     "tuple field must be uint");
+	}
+	uint32_t num = mp_decode_uint(&args);
+
+	if (mp_typeof(*args) != MP_STR) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+				     "tuple field must be string");
+	}
+	const char *str = args;
+	uint32_t len = mp_decode_strl(&str);
+
+	char tuple_buf[512];
+	char *d = tuple_buf;
+	d = mp_encode_array(d, 2);
+	d = mp_encode_uint(d, num);
+	d = mp_encode_str(d, str, len);
+	assert(d <= tuple_buf + sizeof(tuple_buf));
+
+	box_tuple_format_t *fmt = box_tuple_format_default();
+	box_tuple_t *tuple = box_tuple_new(fmt, tuple_buf, d);
+	if (tuple == NULL)
+		return -1;
+
+	return box_return_tuple(ctx, tuple);
+}
+
+/*
+ * Sum two integers.
+ */
+int
+cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	uint32_t arg_count = mp_decode_array(&args);
+	if (arg_count != 2) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+				     "invalid argument count");
+	}
+	uint64_t a = mp_decode_uint(&args);
+	uint64_t b = mp_decode_uint(&args);
+
+	char res[16];
+	char *end = mp_encode_uint(res, a + b);
+	box_return_mp(ctx, res, end);
+	return 0;
+}
diff --git a/test/box/cfunc3.c b/test/box/cfunc3.c
new file mode 100644
index 000000000..668790fbf
--- /dev/null
+++ b/test/box/cfunc3.c
@@ -0,0 +1,25 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Sum two integers.
+ */
+int
+cfunc_add(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	uint32_t arg_count = mp_decode_array(&args);
+	if (arg_count != 2) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+				     "invalid argument count");
+	}
+	uint64_t a = mp_decode_uint(&args);
+	uint64_t b = mp_decode_uint(&args);
+
+	char res[16];
+	char *end = mp_encode_uint(res, a + b);
+	box_return_mp(ctx, res, end);
+	return 0;
+}
diff --git a/test/box/cfunc4.c b/test/box/cfunc4.c
new file mode 100644
index 000000000..cc079b335
--- /dev/null
+++ b/test/box/cfunc4.c
@@ -0,0 +1,28 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Sum two integers and add a constant,
+ * so that result will be different after
+ * the reload.
+ */
+int
+cfunc_add(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	const uint32_t delta = 10;
+	uint32_t arg_count = mp_decode_array(&args);
+	if (arg_count != 2) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+				     "invalid argument count");
+	}
+	uint64_t a = mp_decode_uint(&args);
+	uint64_t b = mp_decode_uint(&args);
+
+	char res[16];
+	char *end = mp_encode_uint(res, a + b + delta);
+	box_return_mp(ctx, res, end);
+	return 0;
+}
diff --git a/test/box/lib.result b/test/box/lib.result
new file mode 100644
index 000000000..43c396dbd
--- /dev/null
+++ b/test/box/lib.result
@@ -0,0 +1,530 @@
+-- test-run result file version 2
+--
+-- gh-4642: New box.lib module to be able to
+-- run C stored functions on read only nodes
+-- without requirement to register them with
+-- box.schema.func help.
+--
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+
+boxlib = require('box.lib')
+ | ---
+ | ...
+fio = require('fio')
+ | ---
+ | ...
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+ | ---
+ | ...
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext
+ | ---
+ | ...
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext
+ | ---
+ | ...
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext
+ | ---
+ | ...
+cfunc3_path = fio.pathjoin(build_path, "test/box/cfunc3.") .. ext
+ | ---
+ | ...
+cfunc4_path = fio.pathjoin(build_path, "test/box/cfunc4.") .. ext
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc1_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+_, err = pcall(boxlib.load, 'non-such-module')
+ | ---
+ | ...
+assert(err ~= nil)
+ | ---
+ | - true
+ | ...
+
+-- All functions are sitting in cfunc.so.
+old_module = boxlib.load('cfunc')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+old_module_copy = boxlib.load('cfunc')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+assert(old_module_copy['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+old_module_copy:unload()
+ | ---
+ | - true
+ | ...
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+old_cfunc_nop = old_module:load('cfunc_nop')
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+ | ---
+ | ...
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+ | ---
+ | ...
+old_cfunc_args = old_module:load('cfunc_args')
+ | ---
+ | ...
+old_cfunc_sum = old_module:load('cfunc_sum')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 6)
+ | ---
+ | - true
+ | ...
+
+-- Test for error on nonexisting function.
+_, err = pcall(old_module.load, old_module, 'no-such-func')
+ | ---
+ | ...
+assert(err ~= nil)
+ | ---
+ | - true
+ | ...
+
+-- Make sure they all are callable.
+old_cfunc_nop()
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens()
+ | ---
+ | ...
+old_cfunc_multireturn()
+ | ---
+ | ...
+old_cfunc_args()
+ | ---
+ | ...
+old_cfunc_sum()
+ | ---
+ | ...
+
+-- Unload the module but keep old functions alive, so
+-- they keep reference to NOP module internally
+-- and still callable.
+old_module:unload()
+ | ---
+ | - true
+ | ...
+-- Test refs via function name.
+assert(old_cfunc_nop['debug_module_refs'] == 5)
+ | ---
+ | - true
+ | ...
+old_cfunc_nop()
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens()
+ | ---
+ | ...
+old_cfunc_multireturn()
+ | ---
+ | ...
+old_cfunc_args()
+ | ---
+ | ...
+old_cfunc_sum()
+ | ---
+ | ...
+
+-- The module is unloaded I should not be able
+-- to load new shared library.
+old_module:load('cfunc')
+ | ---
+ | - error: Expects function = module:load('name') but not module object passed
+ | ...
+-- Neither I should be able to unload module twise.
+old_module:unload()
+ | ---
+ | - error: The module is unloaded
+ | ...
+
+-- Clean old functions.
+old_cfunc_nop:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_fetch_seq_evens:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_multireturn:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_args:unload()
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_sum['debug_module_refs'] == 1)
+ | ---
+ | - true
+ | ...
+old_cfunc_sum:unload()
+ | ---
+ | - true
+ | ...
+
+-- And reload old module again.
+old_module = boxlib.load('cfunc')
+ | ---
+ | ...
+old_module_ptr = old_module['debug_ptr']
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+
+-- Overwrite module with new contents.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc2_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+-- Load new module, cache should be updated.
+new_module = boxlib.load('cfunc')
+ | ---
+ | ...
+new_module_ptr = new_module['debug_ptr']
+ | ---
+ | ...
+
+-- Old and new module keep one reference with
+-- different IDs.
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+assert(old_module['debug_refs'] == new_module['debug_refs'])
+ | ---
+ | - true
+ | ...
+assert(old_module_ptr ~= new_module_ptr)
+ | ---
+ | - true
+ | ...
+
+-- All functions from old module should be loadable.
+old_cfunc_nop = old_module:load('cfunc_nop')
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+ | ---
+ | ...
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+ | ---
+ | ...
+old_cfunc_args = old_module:load('cfunc_args')
+ | ---
+ | ...
+old_cfunc_sum = old_module:load('cfunc_sum')
+ | ---
+ | ...
+assert(old_cfunc_nop['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_fetch_seq_evens['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_multireturn['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_args['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_sum['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_module['debug_refs'] == 6)
+ | ---
+ | - true
+ | ...
+
+-- Lookup for updated symbols.
+new_cfunc_nop = new_module:load('cfunc_nop')
+ | ---
+ | ...
+new_cfunc_fetch_seq_evens = new_module:load('cfunc_fetch_seq_evens')
+ | ---
+ | ...
+new_cfunc_multireturn = new_module:load('cfunc_multireturn')
+ | ---
+ | ...
+new_cfunc_args = new_module:load('cfunc_args')
+ | ---
+ | ...
+new_cfunc_sum = new_module:load('cfunc_sum')
+ | ---
+ | ...
+assert(new_cfunc_nop['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_fetch_seq_evens['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_multireturn['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_args['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_sum['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_module['debug_refs'] == 6)
+ | ---
+ | - true
+ | ...
+
+-- Call old functions.
+old_cfunc_nop()
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens()
+ | ---
+ | ...
+old_cfunc_multireturn()
+ | ---
+ | ...
+old_cfunc_args()
+ | ---
+ | ...
+old_cfunc_sum()
+ | ---
+ | ...
+
+-- Call new functions.
+new_cfunc_nop()
+ | ---
+ | ...
+new_cfunc_multireturn()
+ | ---
+ | - [1]
+ | - [1]
+ | ...
+new_cfunc_fetch_seq_evens({2,4,6})
+ | ---
+ | ...
+new_cfunc_fetch_seq_evens({1,2,3})  -- error, odd numbers sequence
+ | ---
+ | - error: invalid argument 1 != 2
+ | ...
+new_cfunc_args(1, "hello")
+ | ---
+ | - [1, 'hello']
+ | ...
+new_cfunc_sum(1) -- error, one arg passed
+ | ---
+ | - error: invalid argument count
+ | ...
+new_cfunc_sum(1,2)
+ | ---
+ | - 3
+ | ...
+
+-- Cleanup old module's functions.
+old_cfunc_nop:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_fetch_seq_evens:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_multireturn:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_args:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_sum:unload()
+ | ---
+ | - true
+ | ...
+old_module:unload()
+ | ---
+ | - true
+ | ...
+
+-- Cleanup new module data.
+new_cfunc_nop:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_multireturn:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_fetch_seq_evens:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_args:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_sum:unload()
+ | ---
+ | - true
+ | ...
+new_module:unload()
+ | ---
+ | - true
+ | ...
+
+-- Cleanup the generated symlink.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+
+-- Test double hashing: create function
+-- in box.schema.fun so that it should
+-- appear in box.lib hash.
+fio.symlink(cfunc3_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+box.schema.func.create('cfunc.cfunc_add', {language = "C"})
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
+ | ---
+ | ...
+box.func['cfunc.cfunc_add']:call({1,2})
+ | ---
+ | - 3
+ | ...
+
+old_module = boxlib.load('cfunc')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
+ | ---
+ | - true
+ | ...
+old_func = old_module:load('cfunc_add')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 4) -- plus function instance
+ | ---
+ | - true
+ | ...
+old_func(1,2)
+ | ---
+ | - 3
+ | ...
+
+-- Now update on disk and reload the module.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc4_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+box.schema.func.reload("cfunc")
+ | ---
+ | ...
+box.func['cfunc.cfunc_add']:call({1,2})
+ | ---
+ | - 13
+ | ...
+
+-- The box.lib instance should carry own
+-- references to the module and old
+-- function. And reloading must not
+-- affect old functions. Thus one for
+-- box.lib and one for box.lib function.
+assert(old_module['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+old_func(1,2)
+ | ---
+ | - 3
+ | ...
+old_func:unload()
+ | ---
+ | - true
+ | ...
+old_module:unload()
+ | ---
+ | - true
+ | ...
+
+-- Same time the reload should update
+-- low level module cache, thus two
+-- for box and box function plus one
+-- new box.lib.
+new_module = boxlib.load('cfunc')
+ | ---
+ | ...
+assert(new_module['debug_refs'] == 3)
+ | ---
+ | - true
+ | ...
+
+-- Box function should carry own module.
+box.func['cfunc.cfunc_add']:call({1,2})
+ | ---
+ | - 13
+ | ...
+
+-- Cleanup.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+new_module:unload()
+ | ---
+ | - true
+ | ...
+box.schema.func.drop('cfunc.cfunc_add')
+ | ---
+ | ...
diff --git a/test/box/lib.test.lua b/test/box/lib.test.lua
new file mode 100644
index 000000000..445c2121c
--- /dev/null
+++ b/test/box/lib.test.lua
@@ -0,0 +1,203 @@
+--
+-- gh-4642: New box.lib module to be able to
+-- run C stored functions on read only nodes
+-- without requirement to register them with
+-- box.schema.func help.
+--
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+boxlib = require('box.lib')
+fio = require('fio')
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext
+cfunc3_path = fio.pathjoin(build_path, "test/box/cfunc3.") .. ext
+cfunc4_path = fio.pathjoin(build_path, "test/box/cfunc4.") .. ext
+
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc1_path, cfunc_path)
+
+_, err = pcall(boxlib.load, 'non-such-module')
+assert(err ~= nil)
+
+-- All functions are sitting in cfunc.so.
+old_module = boxlib.load('cfunc')
+assert(old_module['debug_refs'] == 1)
+old_module_copy = boxlib.load('cfunc')
+assert(old_module['debug_refs'] == 2)
+assert(old_module_copy['debug_refs'] == 2)
+old_module_copy:unload()
+assert(old_module['debug_refs'] == 1)
+old_cfunc_nop = old_module:load('cfunc_nop')
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_sum = old_module:load('cfunc_sum')
+assert(old_module['debug_refs'] == 6)
+
+-- Test for error on nonexisting function.
+_, err = pcall(old_module.load, old_module, 'no-such-func')
+assert(err ~= nil)
+
+-- Make sure they all are callable.
+old_cfunc_nop()
+old_cfunc_fetch_seq_evens()
+old_cfunc_multireturn()
+old_cfunc_args()
+old_cfunc_sum()
+
+-- Unload the module but keep old functions alive, so
+-- they keep reference to NOP module internally
+-- and still callable.
+old_module:unload()
+-- Test refs via function name.
+assert(old_cfunc_nop['debug_module_refs'] == 5)
+old_cfunc_nop()
+old_cfunc_fetch_seq_evens()
+old_cfunc_multireturn()
+old_cfunc_args()
+old_cfunc_sum()
+
+-- The module is unloaded I should not be able
+-- to load new shared library.
+old_module:load('cfunc')
+-- Neither I should be able to unload module twise.
+old_module:unload()
+
+-- Clean old functions.
+old_cfunc_nop:unload()
+old_cfunc_fetch_seq_evens:unload()
+old_cfunc_multireturn:unload()
+old_cfunc_args:unload()
+assert(old_cfunc_sum['debug_module_refs'] == 1)
+old_cfunc_sum:unload()
+
+-- And reload old module again.
+old_module = boxlib.load('cfunc')
+old_module_ptr = old_module['debug_ptr']
+assert(old_module['debug_refs'] == 1)
+
+-- Overwrite module with new contents.
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc2_path, cfunc_path)
+
+-- Load new module, cache should be updated.
+new_module = boxlib.load('cfunc')
+new_module_ptr = new_module['debug_ptr']
+
+-- Old and new module keep one reference with
+-- different IDs.
+assert(old_module['debug_refs'] == 1)
+assert(old_module['debug_refs'] == new_module['debug_refs'])
+assert(old_module_ptr ~= new_module_ptr)
+
+-- All functions from old module should be loadable.
+old_cfunc_nop = old_module:load('cfunc_nop')
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_sum = old_module:load('cfunc_sum')
+assert(old_cfunc_nop['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_fetch_seq_evens['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_multireturn['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_args['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_sum['debug_module_ptr'] == old_module_ptr)
+assert(old_module['debug_refs'] == 6)
+
+-- Lookup for updated symbols.
+new_cfunc_nop = new_module:load('cfunc_nop')
+new_cfunc_fetch_seq_evens = new_module:load('cfunc_fetch_seq_evens')
+new_cfunc_multireturn = new_module:load('cfunc_multireturn')
+new_cfunc_args = new_module:load('cfunc_args')
+new_cfunc_sum = new_module:load('cfunc_sum')
+assert(new_cfunc_nop['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_fetch_seq_evens['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_multireturn['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_args['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_sum['debug_module_ptr'] == new_module_ptr)
+assert(new_module['debug_refs'] == 6)
+
+-- Call old functions.
+old_cfunc_nop()
+old_cfunc_fetch_seq_evens()
+old_cfunc_multireturn()
+old_cfunc_args()
+old_cfunc_sum()
+
+-- Call new functions.
+new_cfunc_nop()
+new_cfunc_multireturn()
+new_cfunc_fetch_seq_evens({2,4,6})
+new_cfunc_fetch_seq_evens({1,2,3})  -- error, odd numbers sequence
+new_cfunc_args(1, "hello")
+new_cfunc_sum(1) -- error, one arg passed
+new_cfunc_sum(1,2)
+
+-- Cleanup old module's functions.
+old_cfunc_nop:unload()
+old_cfunc_fetch_seq_evens:unload()
+old_cfunc_multireturn:unload()
+old_cfunc_args:unload()
+old_cfunc_sum:unload()
+old_module:unload()
+
+-- Cleanup new module data.
+new_cfunc_nop:unload()
+new_cfunc_multireturn:unload()
+new_cfunc_fetch_seq_evens:unload()
+new_cfunc_args:unload()
+new_cfunc_sum:unload()
+new_module:unload()
+
+-- Cleanup the generated symlink.
+_ = pcall(fio.unlink(cfunc_path))
+
+-- Test double hashing: create function
+-- in box.schema.fun so that it should
+-- appear in box.lib hash.
+fio.symlink(cfunc3_path, cfunc_path)
+box.schema.func.create('cfunc.cfunc_add', {language = "C"})
+box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
+box.func['cfunc.cfunc_add']:call({1,2})
+
+old_module = boxlib.load('cfunc')
+assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
+old_func = old_module:load('cfunc_add')
+assert(old_module['debug_refs'] == 4) -- plus function instance
+old_func(1,2)
+
+-- Now update on disk and reload the module.
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc4_path, cfunc_path)
+
+box.schema.func.reload("cfunc")
+box.func['cfunc.cfunc_add']:call({1,2})
+
+-- The box.lib instance should carry own
+-- references to the module and old
+-- function. And reloading must not
+-- affect old functions. Thus one for
+-- box.lib and one for box.lib function.
+assert(old_module['debug_refs'] == 2)
+old_func(1,2)
+old_func:unload()
+old_module:unload()
+
+-- Same time the reload should update
+-- low level module cache, thus two
+-- for box and box function plus one
+-- new box.lib.
+new_module = boxlib.load('cfunc')
+assert(new_module['debug_refs'] == 3)
+
+-- Box function should carry own module.
+box.func['cfunc.cfunc_add']:call({1,2})
+
+-- Cleanup.
+_ = pcall(fio.unlink(cfunc_path))
+new_module:unload()
+box.schema.func.drop('cfunc.cfunc_add')
diff --git a/test/box/suite.ini b/test/box/suite.ini
index d5f72e559..f82115605 100644
--- a/test/box/suite.ini
+++ b/test/box/suite.ini
@@ -2,7 +2,7 @@
 core = tarantool
 description = Database tests
 script = box.lua
-disabled = rtree_errinj.test.lua tuple_bench.test.lua
+disabled = rtree_errinj.test.lua tuple_bench.test.lua lib.test.lua
 long_run = huge_field_map_long.test.lua
 config = engine.cfg
 release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua gh-4648-func-load-unload.test.lua
-- 
2.30.2

Re: [Tarantool-patches] [PATCH v20 1/7] box/schema: make sure hashes are created

[Serge Petrenko via Tarantool-patches]

02.04.2021 15:34, Cyrill Gorcunov пишет:
> These are critical structures, strictly speaking
> if malloc has failed we're likely under heavy memory
> pressure and won't continue anyway but better to exit
> explicitly.
>
> In-scope-of #4642
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/schema.cc | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 963278b19..89904e4d2 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -372,6 +372,13 @@ schema_init(void)
>   	funcs = mh_i32ptr_new();
>   	funcs_by_name = mh_strnptr_new();
>   	sequences = mh_i32ptr_new();
> +
> +	if (spaces == NULL || spaces_by_name == NULL ||
> +	    funcs == NULL || funcs_by_name == NULL ||
> +	    sequences == NULL) {
> +		panic("Can't allocate schema hashes");
> +	}

I've just noticed, all the mh_..._new methods will fail on a null 
dereference
even before returning. So the check above doesn't make much sense.

But I see that some other places have this check
`if (smth = mh_smth_new() == NULL)` and set an error.

Am I missing something?
> +
>   	/*
>   	 * Create surrogate space objects for the mandatory system
>   	 * spaces (the primal eggs from which we get all the

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v20 1/7] box/schema: make sure hashes are created

[Cyrill Gorcunov via Tarantool-patches]

On Mon, Apr 05, 2021 at 12:28:50PM +0300, Serge Petrenko wrote:
> > diff --git a/src/box/schema.cc b/src/box/schema.cc
> > index 963278b19..89904e4d2 100644
> > --- a/src/box/schema.cc
> > +++ b/src/box/schema.cc
> > @@ -372,6 +372,13 @@ schema_init(void)
> >   	funcs = mh_i32ptr_new();
> >   	funcs_by_name = mh_strnptr_new();
> >   	sequences = mh_i32ptr_new();
> > +
> > +	if (spaces == NULL || spaces_by_name == NULL ||
> > +	    funcs == NULL || funcs_by_name == NULL ||
> > +	    sequences == NULL) {
> > +		panic("Can't allocate schema hashes");
> > +	}
> 
> I've just noticed, all the mh_..._new methods will fail on a null
> dereference even before returning. So the check above doesn't
> make much sense.

This is because our lib/salad/mhash.h is simply buggy and it must be:

1) fixed in mhash level
2) get rid of this macros madness and convert it to normal
   library. this macros spreading is over the top already
   and I don't understand why nobody cares about icache footprint

Nevertheless we should check for x_new resuls as we do in our
other code. Let walk over spaces

schema_init
  spaces = mh_i32ptr_new();
  ...
  sc_space_new
    space_cache_replace(NULL, space);
      mh_int_t k = mh_i32ptr_put(spaces, &node_p, ...

which is simply nil dereferencing (once we fix bullet 1).

> 
> But I see that some other places have this check
> `if (smth = mh_smth_new() == NULL)` and set an error.
> 
> Am I missing something?

I think the idea was that mh_x_new doesn't do nil dereference
by its own but behaves in a safe way as any library routine
should. That said I still think we should check for mh_i32ptr_new
results. But won't insist to include this patch, we can drop it
if you prefer.

Re: [Tarantool-patches] [PATCH v20 1/7] box/schema: make sure hashes are created

[Serge Petrenko via Tarantool-patches]

05.04.2021 12:50, Cyrill Gorcunov пишет:
> On Mon, Apr 05, 2021 at 12:28:50PM +0300, Serge Petrenko wrote:
>>> diff --git a/src/box/schema.cc b/src/box/schema.cc
>>> index 963278b19..89904e4d2 100644
>>> --- a/src/box/schema.cc
>>> +++ b/src/box/schema.cc
>>> @@ -372,6 +372,13 @@ schema_init(void)
>>>    	funcs = mh_i32ptr_new();
>>>    	funcs_by_name = mh_strnptr_new();
>>>    	sequences = mh_i32ptr_new();
>>> +
>>> +	if (spaces == NULL || spaces_by_name == NULL ||
>>> +	    funcs == NULL || funcs_by_name == NULL ||
>>> +	    sequences == NULL) {
>>> +		panic("Can't allocate schema hashes");
>>> +	}
>> I've just noticed, all the mh_..._new methods will fail on a null
>> dereference even before returning. So the check above doesn't
>> make much sense.
> This is because our lib/salad/mhash.h is simply buggy and it must be:
>
> 1) fixed in mhash level
> 2) get rid of this macros madness and convert it to normal
>     library. this macros spreading is over the top already
>     and I don't understand why nobody cares about icache footprint
>
> Nevertheless we should check for x_new resuls as we do in our
> other code. Let walk over spaces
>
> schema_init
>    spaces = mh_i32ptr_new();
>    ...
>    sc_space_new
>      space_cache_replace(NULL, space);
>        mh_int_t k = mh_i32ptr_put(spaces, &node_p, ...
>
> which is simply nil dereferencing (once we fix bullet 1).
>
>> But I see that some other places have this check
>> `if (smth = mh_smth_new() == NULL)` and set an error.
>>
>> Am I missing something?
> I think the idea was that mh_x_new doesn't do nil dereference
> by its own but behaves in a safe way as any library routine
> should. That said I still think we should check for mh_i32ptr_new
> results. But won't insist to include this patch, we can drop it
> if you prefer.

I see, let's keep the patch then. LGTM.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v20 2/7] box/func: module_reload -- drop redundant argument

[Serge Petrenko via Tarantool-patches]

02.04.2021 15:34, Cyrill Gorcunov пишет:
> The only purpose of the module argument is to
> notify the caller that the module doesn't exist.
> Lets simply the code and drop this argument.
>
> Part-of #4642
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---

Thanks for the patch! LGTM.

>   src/box/call.c | 9 +--------
>   src/box/func.c | 7 +++----
>   src/box/func.h | 3 +--
>   3 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/src/box/call.c b/src/box/call.c
> index 9c291260e..7839e1f3e 100644
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -128,14 +128,7 @@ box_module_reload(const char *name)
>   				 user->def->name);
>   		return -1;
>   	}
> -	struct module *module = NULL;
> -	if (module_reload(name, name + strlen(name), &module) == 0) {
> -		if (module != NULL)
> -			return 0;
> -		else
> -			diag_set(ClientError, ER_NO_SUCH_MODULE, name);
> -	}
> -	return -1;
> +	return module_reload(name, name + strlen(name));
>   }
>   
>   int
> diff --git a/src/box/func.c b/src/box/func.c
> index 9909cee45..233696a4f 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -372,13 +372,13 @@ module_sym(struct module *module, const char *name)
>   }
>   
>   int
> -module_reload(const char *package, const char *package_end, struct module **module)
> +module_reload(const char *package, const char *package_end)
>   {
>   	struct module *old_module = module_cache_find(package, package_end);
>   	if (old_module == NULL) {
>   		/* Module wasn't loaded - do nothing. */
> -		*module = NULL;
> -		return 0;
> +		diag_set(ClientError, ER_NO_SUCH_MODULE, package);
> +		return -1;
>   	}
>   
>   	struct module *new_module = module_load(package, package_end);
> @@ -399,7 +399,6 @@ module_reload(const char *package, const char *package_end, struct module **modu
>   	if (module_cache_put(new_module) != 0)
>   		goto restore;
>   	module_gc(old_module);
> -	*module = new_module;
>   	return 0;
>   restore:
>   	/*
> diff --git a/src/box/func.h b/src/box/func.h
> index 581e468cb..0a08fa465 100644
> --- a/src/box/func.h
> +++ b/src/box/func.h
> @@ -113,12 +113,11 @@ func_call(struct func *func, struct port *args, struct port *ret);
>    *
>    * @param package name begin pointer.
>    * @param package_end package_end name end pointer.
> - * @param[out] module a pointer to store module object on success.
>    * @retval -1 on error.
>    * @retval 0 on success.
>    */
>   int
> -module_reload(const char *package, const char *package_end, struct module **module);
> +module_reload(const char *package, const char *package_end);
>   
>   #if defined(__cplusplus)
>   } /* extern "C" */

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v20 2/7] box/func: module_reload -- drop redundant argument

[Serge Petrenko via Tarantool-patches]

05.04.2021 13:23, Serge Petrenko via Tarantool-patches пишет:
>
>
> 02.04.2021 15:34, Cyrill Gorcunov пишет:
>> The only purpose of the module argument is to
>> notify the caller that the module doesn't exist.
>> Lets simply the code and drop this argument.
typo: simply -> simplify
>>
>> Part-of #4642
>>
>> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>> ---
>
> Thanks for the patch! LGTM.
>
>>   src/box/call.c | 9 +--------
>>   src/box/func.c | 7 +++----
>>   src/box/func.h | 3 +--
>>   3 files changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/box/call.c b/src/box/call.c
>> index 9c291260e..7839e1f3e 100644
>> --- a/src/box/call.c
>> +++ b/src/box/call.c
>> @@ -128,14 +128,7 @@ box_module_reload(const char *name)
>>                    user->def->name);
>>           return -1;
>>       }
>> -    struct module *module = NULL;
>> -    if (module_reload(name, name + strlen(name), &module) == 0) {
>> -        if (module != NULL)
>> -            return 0;
>> -        else
>> -            diag_set(ClientError, ER_NO_SUCH_MODULE, name);
>> -    }
>> -    return -1;
>> +    return module_reload(name, name + strlen(name));
>>   }
>>     int
>> diff --git a/src/box/func.c b/src/box/func.c
>> index 9909cee45..233696a4f 100644
>> --- a/src/box/func.c
>> +++ b/src/box/func.c
>> @@ -372,13 +372,13 @@ module_sym(struct module *module, const char 
>> *name)
>>   }
>>     int
>> -module_reload(const char *package, const char *package_end, struct 
>> module **module)
>> +module_reload(const char *package, const char *package_end)
>>   {
>>       struct module *old_module = module_cache_find(package, 
>> package_end);
>>       if (old_module == NULL) {
>>           /* Module wasn't loaded - do nothing. */
>> -        *module = NULL;
>> -        return 0;
>> +        diag_set(ClientError, ER_NO_SUCH_MODULE, package);
>> +        return -1;
>>       }
>>         struct module *new_module = module_load(package, package_end);
>> @@ -399,7 +399,6 @@ module_reload(const char *package, const char 
>> *package_end, struct module **modu
>>       if (module_cache_put(new_module) != 0)
>>           goto restore;
>>       module_gc(old_module);
>> -    *module = new_module;
>>       return 0;
>>   restore:
>>       /*
>> diff --git a/src/box/func.h b/src/box/func.h
>> index 581e468cb..0a08fa465 100644
>> --- a/src/box/func.h
>> +++ b/src/box/func.h
>> @@ -113,12 +113,11 @@ func_call(struct func *func, struct port *args, 
>> struct port *ret);
>>    *
>>    * @param package name begin pointer.
>>    * @param package_end package_end name end pointer.
>> - * @param[out] module a pointer to store module object on success.
>>    * @retval -1 on error.
>>    * @retval 0 on success.
>>    */
>>   int
>> -module_reload(const char *package, const char *package_end, struct 
>> module **module);
>> +module_reload(const char *package, const char *package_end);
>>     #if defined(__cplusplus)
>>   } /* extern "C" */
>

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v20 2/7] box/func: module_reload -- drop redundant argument

[Cyrill Gorcunov via Tarantool-patches]

On Mon, Apr 05, 2021 at 01:26:03PM +0300, Serge Petrenko wrote:
> 
> 
> 05.04.2021 13:23, Serge Petrenko via Tarantool-patches пишет:
> > 
> > 
> > 02.04.2021 15:34, Cyrill Gorcunov пишет:
> > > The only purpose of the module argument is to
> > > notify the caller that the module doesn't exist.
> > > Lets simply the code and drop this argument.
> typo: simply -> simplify

Thanks! I'll force update this typo.

Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Serge Petrenko via Tarantool-patches]

Thanks for the patch! Please find 3 minor comments below.

02.04.2021 15:34, Cyrill Gorcunov пишет:
> In commit 96938fafb an ability to hot reload of modules
> has been introduced. When module is been reloaded his

typo: either 'is being reloaded' or 'has been reloaded'.

> functions are resolved to new symbols but if something
> went wrong it is supposed to restore old symbols from
> the old module. Actually current code restores only
> one function and may crash if there a bunch of functions
> to restore. Lets fix it.
>
> Part-of #4642
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   changelogs/unreleased/fix-module-reload.md |   4 +
>   src/box/func.c                             |  13 ++-
>   test/box/CMakeLists.txt                    |   2 +
>   test/box/func_restore.result               | 103 +++++++++++++++++++++
>   test/box/func_restore.test.lua             |  43 +++++++++
>   test/box/func_restore1.c                   |  34 +++++++
>   test/box/func_restore2.c                   |  28 ++++++
>   7 files changed, 220 insertions(+), 7 deletions(-)
>   create mode 100644 changelogs/unreleased/fix-module-reload.md
>   create mode 100644 test/box/func_restore.result
>   create mode 100644 test/box/func_restore.test.lua
>   create mode 100644 test/box/func_restore1.c
>   create mode 100644 test/box/func_restore2.c
>
> diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md
> new file mode 100644
> index 000000000..7e189617f
> --- /dev/null
> +++ b/changelogs/unreleased/fix-module-reload.md
> @@ -0,0 +1,4 @@
> +## bugfix/core
> +
> +* Fix module reloading procedure which may crash in case if
> +  new module is corrupted (gh-4642).
> diff --git a/src/box/func.c b/src/box/func.c
> index 233696a4f..1cd7073de 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -387,13 +387,14 @@ module_reload(const char *package, const char *package_end)
>   
>   	struct func_c *func, *tmp_func;
>   	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
> +		/* Move immediately for restore sake. */
> +		rlist_move(&new_module->funcs, &func->item);
>   		struct func_name name;
>   		func_split_name(func->base.def->name, &name);
>   		func->func = module_sym(new_module, name.sym);
>   		if (func->func == NULL)
>   			goto restore;
>   		func->module = new_module;
> -		rlist_move(&new_module->funcs, &func->item);
>   	}
>   	module_cache_del(package, package_end);
>   	if (module_cache_put(new_module) != 0)
> @@ -402,10 +403,10 @@ module_reload(const char *package, const char *package_end)
>   	return 0;
>   restore:
>   	/*
> -	 * Some old-dso func can't be load from new module, restore old
> -	 * functions.
> +	 * Some old functions are not found int the new module,
> +	 * thus restore all migrated functions back to original.

typo: in the new module.

>   	 */
> -	do {
> +	rlist_foreach_entry_safe(func, &new_module->funcs, item, tmp_func) {
>   		struct func_name name;
>   		func_split_name(func->base.def->name, &name);
>   		func->func = module_sym(old_module, name.sym);
> @@ -419,9 +420,7 @@ module_reload(const char *package, const char *package_end)
>   		}
>   		func->module = old_module;
>   		rlist_move(&old_module->funcs, &func->item);
> -	} while (func != rlist_first_entry(&old_module->funcs,
> -					   struct func_c, item));
> -	assert(rlist_empty(&new_module->funcs));

Let's keep this assert.

> +	}
>   	module_delete(new_module);
>   	return -1;
>   }
> diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
> index 06bfbbe9d..944831af2 100644
> --- a/test/box/CMakeLists.txt
> +++ b/test/box/CMakeLists.txt
> @@ -2,4 +2,6 @@ include_directories(${MSGPUCK_INCLUDE_DIRS})
>   build_module(function1 function1.c)
>   build_module(reload1 reload1.c)
>   build_module(reload2 reload2.c)
> +build_module(func_restore1 func_restore1.c)
> +build_module(func_restore2 func_restore2.c)
>   build_module(tuple_bench tuple_bench.c)
> diff --git a/test/box/func_restore.result b/test/box/func_restore.result
> new file mode 100644
> index 000000000..e679f4eb8
> --- /dev/null
> +++ b/test/box/func_restore.result
> @@ -0,0 +1,103 @@
> +-- test-run result file version 2
> +--
> +-- Test that function can be restored to the
> +-- former values when new module can't be
> +-- loaded for some reason (say there some
> +-- missing functions).
> +--
> +build_path = os.getenv("BUILDDIR")
> + | ---
> + | ...
> +package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
> + | ---
> + | ...
> +
> +fio = require('fio')
> + | ---
> + | ...
> +
> +ext = (jit.os == "OSX" and "dylib" or "so")
> + | ---
> + | ...
> +
> +path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
> + | ---
> + | ...
> +path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
> + | ---
> + | ...
> +path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. ext
> + | ---
> + | ...
> +
> +_ = pcall(fio.unlink(path_func_restore))
> + | ---
> + | ...
> +fio.symlink(path_func_good, path_func_restore)
> + | ---
> + | - true
> + | ...
> +
> +box.schema.func.create('func_restore.echo_1', {language = "C"})
> + | ---
> + | ...
> +box.schema.func.create('func_restore.echo_2', {language = "C"})
> + | ---
> + | ...
> +box.schema.func.create('func_restore.echo_3', {language = "C"})
> + | ---
> + | ...
> +
> +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
> + | ---
> + | ...
> +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
> + | ---
> + | ...
> +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
> + | ---
> + | ...
> +
> +-- Order *does* matter since we bind functions in
> +-- a list where entries are added to the top.
> +box.func['func_restore.echo_3']:call()
> + | ---
> + | - 3
> + | ...
> +box.func['func_restore.echo_2']:call()
> + | ---
> + | - 2
> + | ...
> +box.func['func_restore.echo_1']:call()
> + | ---
> + | - 1
> + | ...
> +
> +_ = pcall(fio.unlink(path_func_restore))
> + | ---
> + | ...
> +fio.symlink(path_func_bad, path_func_restore)
> + | ---
> + | - true
> + | ...
> +
> +ok, _ = pcall(box.schema.func.reload, "func_restore")
> + | ---
> + | ...
> +assert(not ok)
> + | ---
> + | - true
> + | ...
> +
> +box.func['func_restore.echo_1']:call()
> + | ---
> + | - 1
> + | ...
> +box.func['func_restore.echo_2']:call()
> + | ---
> + | - 2
> + | ...
> +box.func['func_restore.echo_3']:call()
> + | ---
> + | - 3
> + | ...
> diff --git a/test/box/func_restore.test.lua b/test/box/func_restore.test.lua
> new file mode 100644
> index 000000000..8a38a6074
> --- /dev/null
> +++ b/test/box/func_restore.test.lua
> @@ -0,0 +1,43 @@
> +--
> +-- Test that function can be restored to the
> +-- former values when new module can't be
> +-- loaded for some reason (say there some
> +-- missing functions).
> +--
> +build_path = os.getenv("BUILDDIR")
> +package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
> +
> +fio = require('fio')
> +
> +ext = (jit.os == "OSX" and "dylib" or "so")
> +
> +path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
> +path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
> +path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. ext
> +
> +_ = pcall(fio.unlink(path_func_restore))
> +fio.symlink(path_func_good, path_func_restore)
> +
> +box.schema.func.create('func_restore.echo_1', {language = "C"})
> +box.schema.func.create('func_restore.echo_2', {language = "C"})
> +box.schema.func.create('func_restore.echo_3', {language = "C"})
> +
> +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
> +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
> +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
> +
> +-- Order *does* matter since we bind functions in
> +-- a list where entries are added to the top.
> +box.func['func_restore.echo_3']:call()
> +box.func['func_restore.echo_2']:call()
> +box.func['func_restore.echo_1']:call()
> +
> +_ = pcall(fio.unlink(path_func_restore))
> +fio.symlink(path_func_bad, path_func_restore)
> +
> +ok, _ = pcall(box.schema.func.reload, "func_restore")
> +assert(not ok)
> +
> +box.func['func_restore.echo_1']:call()
> +box.func['func_restore.echo_2']:call()
> +box.func['func_restore.echo_3']:call()
> diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
> new file mode 100644
> index 000000000..ffd69fd2b
> --- /dev/null
> +++ b/test/box/func_restore1.c
> @@ -0,0 +1,34 @@
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <msgpuck.h>
> +
> +#include "module.h"
> +
> +static int
> +echo_num(box_function_ctx_t *ctx, const char *args,
> +	 const char *args_end, unsigned int num)
> +{
> +	char res[16];
> +	char *end = mp_encode_uint(res, num);
> +	box_return_mp(ctx, res, end);
> +	return 0;
> +}
> +
> +
> +int
> +echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> +	return echo_num(ctx, args, args_end, 1);
> +}
> +
> +int
> +echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> +	return echo_num(ctx, args, args_end, 2);
> +}
> +
> +int
> +echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> +	return echo_num(ctx, args, args_end, 3);
> +}
> diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c
> new file mode 100644
> index 000000000..0d77e78b2
> --- /dev/null
> +++ b/test/box/func_restore2.c
> @@ -0,0 +1,28 @@
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <msgpuck.h>
> +
> +#include "module.h"
> +
> +static int
> +echo_num(box_function_ctx_t *ctx, const char *args,
> +	 const char *args_end, unsigned int num)
> +{
> +	char res[16];
> +	char *end = mp_encode_uint(res, num);
> +	box_return_mp(ctx, res, end);
> +	return 0;
> +}
> +
> +
> +int
> +echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> +	return echo_num(ctx, args, args_end, 1);
> +}
> +
> +int
> +echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> +	return echo_num(ctx, args, args_end, 2);
> +}

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Cyrill Gorcunov via Tarantool-patches]

> 
> 02.04.2021 15:34, Cyrill Gorcunov пишет:
> > In commit 96938fafb an ability to hot reload of modules
> > has been introduced. When module is been reloaded his
> 
> typo: either 'is being reloaded' or 'has been reloaded'.

Thanks! Surely it should be "is being".

> > -	 * Some old-dso func can't be load from new module, restore old
> > -	 * functions.
> > +	 * Some old functions are not found int the new module,
> > +	 * thus restore all migrated functions back to original.
> 
> typo: in the new module.

Thanks!

> > @@ -419,9 +420,7 @@ module_reload(const char *package, const char *package_end)
> >   		}
> >   		func->module = old_module;
> >   		rlist_move(&old_module->funcs, &func->item);
> > -	} while (func != rlist_first_entry(&old_module->funcs,
> > -					   struct func_c, item));
> > -	assert(rlist_empty(&new_module->funcs));
> 
> Let's keep this assert.

The cycle is removing entries from the list and for this purpose
the cycle is using a "safe" form. I would prefer to drop this
useless assert() because a few lines above we are doing entries
removing. Frankly I don't understand why it has been introduced
in first place, maybe because this stange and buggy form of
"func != rlist_first_entry", dunno.

So if there is no really strong reason why should we keep this
assert I would love to drop it.

Look, here is what we have

	list_for_each_entry_safe(item, head, ...)
		list_move(item, new_head)

a few obvious lines. And list is clear once the @item reaches @head.

But if you still think it is better to keep and I didn't convince
you, then sure, I'll bring it back.

Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Serge Petrenko via Tarantool-patches]

05.04.2021 14:26, Cyrill Gorcunov пишет:
>> 02.04.2021 15:34, Cyrill Gorcunov пишет:
>>> In commit 96938fafb an ability to hot reload of modules
>>> has been introduced. When module is been reloaded his
>> typo: either 'is being reloaded' or 'has been reloaded'.
> Thanks! Surely it should be "is being".
>
>>> -	 * Some old-dso func can't be load from new module, restore old
>>> -	 * functions.
>>> +	 * Some old functions are not found int the new module,
>>> +	 * thus restore all migrated functions back to original.
>> typo: in the new module.
> Thanks!
>
>>> @@ -419,9 +420,7 @@ module_reload(const char *package, const char *package_end)
>>>    		}
>>>    		func->module = old_module;
>>>    		rlist_move(&old_module->funcs, &func->item);
>>> -	} while (func != rlist_first_entry(&old_module->funcs,
>>> -					   struct func_c, item));
>>> -	assert(rlist_empty(&new_module->funcs));
>> Let's keep this assert.
> The cycle is removing entries from the list and for this purpose
> the cycle is using a "safe" form. I would prefer to drop this
> useless assert() because a few lines above we are doing entries
> removing. Frankly I don't understand why it has been introduced
> in first place, maybe because this stange and buggy form of
> "func != rlist_first_entry", dunno.
>
> So if there is no really strong reason why should we keep this
> assert I would love to drop it.
>
> Look, here is what we have
>
> 	list_for_each_entry_safe(item, head, ...)
> 		list_move(item, new_head)
>
> a few obvious lines. And list is clear once the @item reaches @head.

Ok, drop it then. I have no serious objections.

>
> But if you still think it is better to keep and I didn't convince
> you, then sure, I'll bring it back.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Serge Petrenko via Tarantool-patches]

02.04.2021 15:34, Cyrill Gorcunov пишет:
> The modules subsystem hides some low-level operations under API.
> In particular the modules subsystem is responsible for
>
>   - modules lookup in Lua's "package.search" storage
>   - modules caching to eliminate expensive load procedure
>   - function symbol resolving
>
> Because naming is intersecting with current module functions
> sitting in box/func, lets rename the later to schema_module
> prefix. We will use this prefix in next patches to point the
> modules in box.schema.func are just a particular user of
> the general modules engine.
>
> Part-of #4642
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---

Thanks for the patch! LGTM.

>   src/box/CMakeLists.txt |   1 +
>   src/box/box.cc         |   4 +-
>   src/box/call.c         |   2 +-
>   src/box/func.c         |   6 +-
>   src/box/func.h         |  12 +-
>   src/box/module_cache.c | 474 +++++++++++++++++++++++++++++++++++++++++
>   src/box/module_cache.h | 208 ++++++++++++++++++
>   src/main.cc            |   3 +
>   8 files changed, 698 insertions(+), 12 deletions(-)
>   create mode 100644 src/box/module_cache.c
>   create mode 100644 src/box/module_cache.h
>
> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
> index 19203f770..cc2e17e94 100644
> --- a/src/box/CMakeLists.txt
> +++ b/src/box/CMakeLists.txt
> @@ -126,6 +126,7 @@ add_library(box STATIC
>       memtx_rtree.c
>       memtx_bitset.c
>       memtx_tx.c
> +    module_cache.c
>       engine.c
>       memtx_engine.c
>       memtx_space.c
> diff --git a/src/box/box.cc b/src/box/box.cc
> index e69b7b2ff..b51928ab8 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -2598,7 +2598,7 @@ box_free(void)
>   		session_free();
>   		user_cache_free();
>   		schema_free();
> -		module_free();
> +		schema_module_free();
>   		tuple_free();
>   		port_free();
>   #endif
> @@ -3002,7 +3002,7 @@ box_init(void)
>   	 */
>   	session_init();
>   
> -	if (module_init() != 0)
> +	if (schema_module_init() != 0)
>   		diag_raise();
>   
>   	if (tuple_init(lua_hash) != 0)
> diff --git a/src/box/call.c b/src/box/call.c
> index 7839e1f3e..a6384efe2 100644
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -128,7 +128,7 @@ box_module_reload(const char *name)
>   				 user->def->name);
>   		return -1;
>   	}
> -	return module_reload(name, name + strlen(name));
> +	return schema_module_reload(name, name + strlen(name));
>   }
>   
>   int
> diff --git a/src/box/func.c b/src/box/func.c
> index 1cd7073de..08918e6db 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -172,7 +172,7 @@ static void
>   module_gc(struct module *module);
>   
>   int
> -module_init(void)
> +schema_module_init(void)
>   {
>   	modules = mh_strnptr_new();
>   	if (modules == NULL) {
> @@ -184,7 +184,7 @@ module_init(void)
>   }
>   
>   void
> -module_free(void)
> +schema_module_free(void)
>   {
>   	while (mh_size(modules) > 0) {
>   		mh_int_t i = mh_first(modules);
> @@ -372,7 +372,7 @@ module_sym(struct module *module, const char *name)
>   }
>   
>   int
> -module_reload(const char *package, const char *package_end)
> +schema_module_reload(const char *package, const char *package_end)
>   {
>   	struct module *old_module = module_cache_find(package, package_end);
>   	if (old_module == NULL) {
> diff --git a/src/box/func.h b/src/box/func.h
> index 0a08fa465..5a49e34f4 100644
> --- a/src/box/func.h
> +++ b/src/box/func.h
> @@ -85,16 +85,16 @@ struct func {
>   };
>   
>   /**
> - * Initialize modules subsystem.
> + * Initialize schema modules subsystem.
>    */
>   int
> -module_init(void);
> +schema_module_init(void);
>   
>   /**
> - * Cleanup modules subsystem.
> + * Cleanup schema modules subsystem.
>    */
>   void
> -module_free(void);
> +schema_module_free(void);
>   
>   struct func *
>   func_new(struct func_def *def);
> @@ -109,7 +109,7 @@ int
>   func_call(struct func *func, struct port *args, struct port *ret);
>   
>   /**
> - * Reload dynamically loadable module.
> + * Reload dynamically loadable schema module.
>    *
>    * @param package name begin pointer.
>    * @param package_end package_end name end pointer.
> @@ -117,7 +117,7 @@ func_call(struct func *func, struct port *args, struct port *ret);
>    * @retval 0 on success.
>    */
>   int
> -module_reload(const char *package, const char *package_end);
> +schema_module_reload(const char *package, const char *package_end);
>   
>   #if defined(__cplusplus)
>   } /* extern "C" */
> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> new file mode 100644
> index 000000000..2cd2f2e8b
> --- /dev/null
> +++ b/src/box/module_cache.c
> @@ -0,0 +1,474 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <dlfcn.h>
> +#include <lua.h>
> +
> +#include "assoc.h"
> +#include "diag.h"
> +#include "fiber.h"
> +#include "module_cache.h"
> +
> +#include "box/error.h"
> +#include "box/port.h"
> +
> +#include "lua/utils.h"
> +#include "libeio/eio.h"
> +
> +static struct mh_strnptr_t *module_cache = NULL;
> +
> +/**
> + * Helpers for cache manipulations.
> + */
> +static void *
> +cache_find(const char *str, size_t len)
> +{
> +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> +	if (e == mh_end(module_cache))
> +		return NULL;
> +	return mh_strnptr_node(module_cache, e)->val;
> +}
> +
> +static void
> +cache_update(struct module *m)
> +{
> +	const char *str = m->package;
> +	size_t len = m->package_len;
> +
> +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> +	if (e == mh_end(module_cache))
> +		panic("module: failed to update cache: %s", str);
> +
> +	mh_strnptr_node(module_cache, e)->str = m->package;
> +	mh_strnptr_node(module_cache, e)->val = m;
> +}
> +
> +static int
> +cache_put(struct module *m)
> +{
> +	const struct mh_strnptr_node_t nd = {
> +		.str	= m->package,
> +		.len	= m->package_len,
> +		.hash	= mh_strn_hash(m->package, m->package_len),
> +		.val	= m,
> +	};
> +
> +	mh_int_t e = mh_strnptr_put(module_cache, &nd, NULL, NULL);
> +	if (e == mh_end(module_cache)) {
> +		diag_set(OutOfMemory, sizeof(nd), "malloc",
> +			 "module_cache node");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static void
> +cache_del(struct module *m)
> +{
> +	const char *str = m->package;
> +	size_t len = m->package_len;
> +
> +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> +	if (e != mh_end(module_cache)) {
> +		struct module *v = mh_strnptr_node(module_cache, e)->val;
> +		if (v == m) {
> +			/*
> +			 * The module in cache might be updated
> +			 * via force load and old instance is kept
> +			 * by a reference only.
> +			 */
> +			mh_strnptr_del(module_cache, e, NULL);
> +		}
> +	}
> +}
> +
> +/** Arguments for lpackage_search. */
> +struct find_ctx {
> +	const char *package;
> +	size_t package_len;
> +	char *path;
> +	size_t path_len;
> +};
> +
> +/** A helper for find_package(). */
> +static int
> +lpackage_search(lua_State *L)
> +{
> +	struct find_ctx *ctx = (void *)lua_topointer(L, 1);
> +
> +	lua_getglobal(L, "package");
> +	lua_getfield(L, -1, "search");
> +	lua_pushlstring(L, ctx->package, ctx->package_len);
> +
> +	lua_call(L, 1, 1);
> +	if (lua_isnil(L, -1))
> +		return luaL_error(L, "module not found");
> +
> +	char resolved[PATH_MAX];
> +	if (realpath(lua_tostring(L, -1), resolved) == NULL) {
> +		diag_set(SystemError, "realpath");
> +		return luaT_error(L);
> +	}
> +
> +	/*
> +	 * No need for result being trimmed test, it
> +	 * is guaranteed by realpath call.
> +	 */
> +	snprintf(ctx->path, ctx->path_len, "%s", resolved);
> +	return 0;
> +}
> +
> +/** Find package in Lua's "package.search". */
> +static int
> +find_package(const char *package, size_t package_len,
> +	     char *path, size_t path_len)
> +{
> +	struct find_ctx ctx = {
> +		.package	= package,
> +		.package_len	= package_len,
> +		.path		= path,
> +		.path_len	= path_len,
> +	};
> +
> +	struct lua_State *L = tarantool_L;
> +	int top = lua_gettop(L);
> +	if (luaT_cpcall(L, lpackage_search, &ctx) != 0) {
> +		diag_set(ClientError, ER_LOAD_MODULE, ctx.package_len,
> +			 ctx.package, lua_tostring(L, -1));
> +		lua_settop(L, top);
> +		return -1;
> +	}
> +	assert(top == lua_gettop(L));
> +	return 0;
> +}
> +
> +void
> +module_ref(struct module *m)
> +{
> +	assert(m->refs >= 0);
> +	++m->refs;
> +}
> +
> +void
> +module_unref(struct module *m)
> +{
> +	assert(m->refs > 0);
> +	if (--m->refs == 0) {
> +		cache_del(m);
> +		dlclose(m->handle);
> +		TRASH(m);
> +		free(m);
> +	}
> +}
> +
> +int
> +module_func_load(struct module *m, const char *func_name,
> +		 struct module_func *mf)
> +{
> +	void *sym = dlsym(m->handle, func_name);
> +	if (sym == NULL) {
> +		diag_set(ClientError, ER_LOAD_FUNCTION,
> +			 func_name, dlerror());
> +		return -1;
> +	}
> +
> +	mf->func = sym;
> +	mf->module = m;
> +	module_ref(m);
> +
> +	return 0;
> +}
> +
> +void
> +module_func_unload(struct module_func *mf)
> +{
> +	module_unref(mf->module);
> +	/*
> +	 * Strictly speaking there is no need
> +	 * for implicit creation, it is up to
> +	 * the caller to clear the module function,
> +	 * but since it is cheap, lets prevent from
> +	 * even potential use after free.
> +	 */
> +	module_func_create(mf);
> +}
> +
> +int
> +module_func_call(struct module_func *mf, struct port *args,
> +		 struct port *ret)
> +{
> +	struct region *region = &fiber()->gc;
> +	size_t region_svp = region_used(region);
> +
> +	uint32_t data_sz;
> +	const char *data = port_get_msgpack(args, &data_sz);
> +	if (data == NULL)
> +		return -1;
> +
> +	port_c_create(ret);
> +	box_function_ctx_t ctx = {
> +		.port = ret,
> +	};
> +
> +	/*
> +	 * We don't know what exactly the callee
> +	 * gonna do during the execution, it may
> +	 * even try to unload itself, thus we make
> +	 * sure the dso won't be unloaded until
> +	 * execution is complete.
> +	 *
> +	 * Moreover the callee might release the memory
> +	 * associated with the module_func pointer itself
> +	 * so keep the address of the module locally.
> +	 */
> +	struct module *m = mf->module;
> +	module_ref(m);
> +	int rc = mf->func(&ctx, data, data + data_sz);
> +	module_unref(m);
> +
> +	region_truncate(region, region_svp);
> +
> +	if (rc != 0) {
> +		if (diag_last_error(&fiber()->diag) == NULL)
> +			diag_set(ClientError, ER_PROC_C, "unknown error");
> +		port_destroy(ret);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/** Fill attributes from stat. */
> +static void
> +module_attr_fill(struct module_attr *attr, struct stat *st)
> +{
> +	memset(attr, 0, sizeof(*attr));
> +
> +	attr->st_dev	= (uint64_t)st->st_dev;
> +	attr->st_ino	= (uint64_t)st->st_ino;
> +	attr->st_size	= (uint64_t)st->st_size;
> +#ifdef TARGET_OS_DARWIN
> +	attr->tv_sec	= (uint64_t)st->st_mtimespec.tv_sec;
> +	attr->tv_nsec	= (uint64_t)st->st_mtimespec.tv_nsec;
> +#else
> +	attr->tv_sec	= (uint64_t)st->st_mtim.tv_sec;
> +	attr->tv_nsec	= (uint64_t)st->st_mtim.tv_nsec;
> +#endif
> +}
> +
> +/**
> + * Copy shared library to temp directory and load from there,
> + * then remove it from this temp place leaving in memory. This
> + * is because there was a bug in libc which screw file updates
> + * detection properly such that next dlopen call simply return
> + * a cached version instead of rereading a library from the disk.
> + *
> + * We keep own copy of file attributes and reload the library
> + * on demand.
> + */
> +static struct module *
> +module_new(const char *package, size_t package_len,
> +	   const char *source_path)
> +{
> +	size_t size = sizeof(struct module) + package_len + 1;
> +	struct module *m = malloc(size);
> +	if (m == NULL) {
> +		diag_set(OutOfMemory, size, "malloc", "module");
> +		return NULL;
> +	}
> +
> +	m->package_len = package_len;
> +	m->refs = 0;
> +
> +	memcpy(m->package, package, package_len);
> +	m->package[package_len] = 0;
> +
> +	const char *tmpdir = getenv("TMPDIR");
> +	if (tmpdir == NULL)
> +		tmpdir = "/tmp";
> +
> +	char dir_name[PATH_MAX];
> +	int rc = snprintf(dir_name, sizeof(dir_name),
> +			  "%s/tntXXXXXX", tmpdir);
> +	if (rc < 0 || (size_t)rc >= sizeof(dir_name)) {
> +		diag_set(SystemError, "failed to generate path to tmp dir");
> +		goto error;
> +	}
> +
> +	if (mkdtemp(dir_name) == NULL) {
> +		diag_set(SystemError, "failed to create unique dir name: %s",
> +			 dir_name);
> +		goto error;
> +	}
> +
> +	char load_name[PATH_MAX];
> +	rc = snprintf(load_name, sizeof(load_name),
> +		      "%s/%.*s." TARANTOOL_LIBEXT,
> +		      dir_name, (int)package_len, package);
> +	if (rc < 0 || (size_t)rc >= sizeof(dir_name)) {
> +		diag_set(SystemError, "failed to generate path to dso");
> +		goto error;
> +	}
> +
> +	struct stat st;
> +	if (stat(source_path, &st) < 0) {
> +		diag_set(SystemError, "failed to stat() module: %s",
> +			 source_path);
> +		goto error;
> +	}
> +	module_attr_fill(&m->attr, &st);
> +
> +	int source_fd = open(source_path, O_RDONLY);
> +	if (source_fd < 0) {
> +		diag_set(SystemError, "failed to open module %s "
> +			 "file for reading", source_path);
> +		goto error;
> +	}
> +	int dest_fd = open(load_name, O_WRONLY | O_CREAT | O_TRUNC,
> +			   st.st_mode & 0777);
> +	if (dest_fd < 0) {
> +		diag_set(SystemError, "failed to open file %s "
> +			 "for writing ", load_name);
> +		close(source_fd);
> +		goto error;
> +	}
> +
> +	off_t ret = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size);
> +	close(source_fd);
> +	close(dest_fd);
> +	if (ret != st.st_size) {
> +		diag_set(SystemError, "failed to copy dso %s to %s",
> +			 source_path, load_name);
> +		goto error;
> +	}
> +
> +	m->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
> +	if (unlink(load_name) != 0)
> +		say_warn("failed to unlink dso link: %s", load_name);
> +	if (rmdir(dir_name) != 0)
> +		say_warn("failed to delete temporary dir: %s", dir_name);
> +	if (m->handle == NULL) {
> +		diag_set(ClientError, ER_LOAD_MODULE, package_len,
> +			  package, dlerror());
> +		goto error;
> +	}
> +
> +	module_ref(m);
> +	return m;
> +
> +error:
> +	free(m);
> +	return NULL;
> +}
> +
> +struct module *
> +module_load_force(const char *package, size_t package_len)
> +{
> +	char path[PATH_MAX];
> +	size_t size = sizeof(path);
> +
> +	if (find_package(package, package_len, path, size) != 0)
> +		return NULL;
> +
> +	struct module *m = module_new(package, package_len, path);
> +	if (m == NULL)
> +		return NULL;
> +
> +	struct module *c = cache_find(package, package_len);
> +	if (c != NULL) {
> +		cache_update(m);
> +	} else {
> +		if (cache_put(m) != 0) {
> +			module_unload(m);
> +			return NULL;
> +		}
> +	}
> +
> +	return m;
> +}
> +
> +struct module *
> +module_load(const char *package, size_t package_len)
> +{
> +	char path[PATH_MAX];
> +
> +	if (find_package(package, package_len, path, sizeof(path)) != 0)
> +		return NULL;
> +
> +	struct module *m = cache_find(package, package_len);
> +	if (m != NULL) {
> +		struct module_attr attr;
> +		struct stat st;
> +		if (stat(path, &st) != 0) {
> +			diag_set(SystemError, "failed to stat() %s", path);
> +			return NULL;
> +		}
> +
> +		/*
> +		 * In case of cache hit we may reuse existing
> +		 * module which speedup load procedure.
> +		 */
> +		module_attr_fill(&attr, &st);
> +		if (memcmp(&attr, &m->attr, sizeof(attr)) == 0) {
> +			module_ref(m);
> +			return m;
> +		}
> +
> +		/*
> +		 * Module has been updated on a storage device,
> +		 * so load a new instance and update the cache,
> +		 * old entry get evicted but continue residing
> +		 * in memory, fully functional, until last
> +		 * function is unloaded.
> +		 */
> +		m = module_new(package, package_len, path);
> +		if (m != NULL)
> +			cache_update(m);
> +	} else {
> +		m = module_new(package, package_len, path);
> +		if (m != NULL && cache_put(m) != 0) {
> +			module_unload(m);
> +			return NULL;
> +		}
> +	}
> +
> +	return m;
> +}
> +
> +void
> +module_unload(struct module *m)
> +{
> +	module_unref(m);
> +}
> +
> +void
> +module_free(void)
> +{
> +	mh_int_t e;
> +
> +	mh_foreach(module_cache, e) {
> +		struct module *m = mh_strnptr_node(module_cache, e)->val;
> +		module_unload(m);
> +	}
> +
> +	mh_strnptr_delete(module_cache);
> +	module_cache = NULL;
> +}
> +
> +int
> +module_init(void)
> +{
> +	module_cache = mh_strnptr_new();
> +	if (module_cache == NULL) {
> +		diag_set(OutOfMemory, sizeof(*module_cache),
> +			 "malloc", "module_cache");
> +		return -1;
> +	}
> +	return 0;
> +}
> diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> new file mode 100644
> index 000000000..18eb3866a
> --- /dev/null
> +++ b/src/box/module_cache.h
> @@ -0,0 +1,208 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#pragma once
> +
> +#include <stdint.h>
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +
> +#include "trivia/config.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +/**
> + * API of C stored function.
> + */
> +
> +struct port;
> +
> +struct box_function_ctx {
> +	struct port *port;
> +};
> +
> +typedef struct box_function_ctx box_function_ctx_t;
> +typedef int (*box_function_t)(box_function_ctx_t *ctx,
> +			      const char *args,
> +			      const char *args_end);
> +
> +/**
> + * Shared library file attributes for
> + * module cache invalidation.
> + */
> +struct module_attr {
> +	uint64_t st_dev;
> +	uint64_t st_ino;
> +	uint64_t st_size;
> +	uint64_t tv_sec;
> +	uint64_t tv_nsec;
> +};
> +
> +/**
> + * Dynamic shared module.
> + */
> +struct module {
> +	/**
> +	 * Module handle, dlopen() result.
> +	 */
> +	void *handle;
> +	/**
> +	 * File attributes.
> +	 */
> +	struct module_attr attr;
> +	/**
> +	 * Count of active references.
> +	 */
> +	int64_t refs;
> +	/**
> +	 * Length of @a package.
> +	 */
> +	size_t package_len;
> +	/**
> +	 * Module's name without file extension.
> +	 */
> +	char package[0];
> +};
> +
> +/**
> + * Module function.
> + */
> +struct module_func {
> +	/**
> +	 * Function's address, iow dlsym() result.
> +	 */
> +	 box_function_t func;
> +	/**
> +	 * Function's module.
> +	 */
> +	struct module *module;
> +};
> +
> +/**
> + * Load a module.
> + *
> + * Lookup for a module instance in cache and if not found
> + * the module is loaded from a storage device. In case if
> + * the module is present in cache but modified on a storage
> + * device it will be reread as a new and cache entry get
> + * updated.
> + *
> + * @param package module package (without file extension).
> + * @param package_len length of @a package.
> + *
> + * Possible errors:
> + * ClientError: the package is not found on a storage device.
> + * ClientError: an error happened when been loading the package.
> + * SystemError: a system error happened during procedure.
> + * OutOfMemory: unable to allocate new memory for module instance.
> + *
> + * @return a module instance on success, NULL otherwise (diag is set)
> + */
> +struct module *
> +module_load(const char *package, size_t package_len);
> +
> +/**
> + * Force load a module.
> + *
> + * Load a module from a storage device in a force way
> + * and update an associated cache entry.
> + *
> + * @param package module package (without file extension).
> + * @param package_len length of @a package.
> + *
> + * Possible errors:
> + * ClientError: the package is not found on a storage device.
> + * ClientError: an error happened when been loading the package.
> + * SystemError: a system error happened during procedure.
> + * OutOfMemory: unable to allocate new memory for module instance.
> + *
> + * @return a module instance on success, NULL otherwise (diag is set)
> + */
> +struct module *
> +module_load_force(const char *package, size_t package_len);
> +
> +/**
> + * Unload a module instance.
> + *
> + * @param m a module to unload.
> + */
> +void
> +module_unload(struct module *m);
> +
> +/** Test if module function is empty. */
> +static inline bool
> +module_func_is_empty(struct module_func *mf)
> +{
> +	return mf->module == NULL;
> +}
> +
> +/** Create new empty module function. */
> +static inline void
> +module_func_create(struct module_func *mf)
> +{
> +	mf->module = NULL;
> +	mf->func = NULL;
> +}
> +
> +/**
> + * Load a new function.
> + *
> + * @param m a module to load a function from.
> + * @param func_name function name.
> + * @param mf[out] function instance.
> + *
> + * Possible errors:
> + * ClientError: no such function in a module.
> + *
> + * @return 0 on success, -1 otherwise (diag is set).
> + */
> +int
> +module_func_load(struct module *m, const char *func_name,
> +		 struct module_func *mf);
> +
> +/**
> + * Unload a function.
> + *
> + * @param mf module function.
> + */
> +void
> +module_func_unload(struct module_func *mf);
> +
> +/**
> + * Execute a function.
> + *
> + * @param mf a function to execute.
> + * @param args function arguments.
> + * @param ret[out] execution results.
> + *
> + * @return 0 on success, -1 otherwise (diag is set).
> + */
> +int
> +module_func_call(struct module_func *mf, struct port *args,
> +		 struct port *ret);
> +
> +/** Increment reference to a module. */
> +void
> +module_ref(struct module *m);
> +
> +/** Decrement reference of a module. */
> +void
> +module_unref(struct module *m);
> +
> +/** Initialize modules subsystem. */
> +int
> +module_init(void);
> +
> +/** Free modules subsystem. */
> +void
> +module_free(void);
> +
> +#if defined(__cplusplus)
> +}
> +#endif /* defined(__plusplus) */
> diff --git a/src/main.cc b/src/main.cc
> index 2be048d77..b74ac5926 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -76,6 +76,7 @@
>   #include "box/lua/init.h" /* box_lua_init() */
>   #include "box/session.h"
>   #include "box/memtx_tx.h"
> +#include "box/module_cache.h"
>   #include "systemd.h"
>   #include "crypto/crypto.h"
>   #include "core/popen.h"
> @@ -521,6 +522,7 @@ tarantool_free(void)
>   	title_free(main_argc, main_argv);
>   
>   	popen_free();
> +	module_free();
>   
>   	/* unlink pidfile. */
>   	if (pid_file_handle != NULL && pidfile_remove(pid_file_handle) == -1)
> @@ -703,6 +705,7 @@ main(int argc, char **argv)
>   	cbus_init();
>   	coll_init();
>   	memtx_tx_manager_init();
> +	module_init();
>   	crypto_init();
>   	systemd_init();
>   

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v20 0/7] box: implement box.lib Lua module

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patchset!

What is the branch name, issue number? Their links
must be in the cover letter.

I assume the branch is gorcunov/gh-4642-func-ro-20
as it has the latest version in its name.

I also see some CI jobs don't pass due to compilation
errors. Please, fix them.

Re: [Tarantool-patches] [PATCH v20 1/7] box/schema: make sure hashes are created

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

I don't think this patch is needed, because it would crash anyway later,
if the hashes couldn't be allocated. On any of their usages. But
whatever.

I also don't see how it is 'in scope of 4642'. It is completely
unrelated. So please, at least drop the reference.

Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

See 6 comments below.

On 02.04.2021 14:34, Cyrill Gorcunov via Tarantool-patches wrote:
> In commit 96938fafb an ability to hot reload of modules

1. Please, add the commit title in parentheses and quotes after the
hash value.

> has been introduced. When module is been reloaded his
> functions are resolved to new symbols but if something
> went wrong it is supposed to restore old symbols from
> the old module. Actually current code restores only
> one function and may crash if there a bunch of functions
> to restore. Lets fix it.
> 
> Part-of #4642

2. How is it a part of 4642? It is totally unrelated. It is a
separate bug, existing before 4642, and which could exist after
4642 without this patch, and which does not block 4642 anyhow.

> diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md
> new file mode 100644
> index 000000000..7e189617f
> --- /dev/null
> +++ b/changelogs/unreleased/fix-module-reload.md
> @@ -0,0 +1,4 @@
> +## bugfix/core
> +
> +* Fix module reloading procedure which may crash in case if
> +  new module is corrupted (gh-4642).

3. 'module' term is used not only for .so/.dylib files, but also
for Lua modules. You need to be more specific that this is about
compiled files.

> diff --git a/src/box/func.c b/src/box/func.c
> index 233696a4f..1cd7073de 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -402,10 +403,10 @@ module_reload(const char *package, const char *package_end)
>  	return 0;
>  restore:
>  	/*
> -	 * Some old-dso func can't be load from new module, restore old
> -	 * functions.
> +	 * Some old functions are not found int the new module,

4. int -> in.

> +	 * thus restore all migrated functions back to original.
>  	 */
> diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
> index 06bfbbe9d..944831af2 100644
> --- a/test/box/CMakeLists.txt
> +++ b/test/box/CMakeLists.txt
> @@ -2,4 +2,6 @@ include_directories(${MSGPUCK_INCLUDE_DIRS})
>  build_module(function1 function1.c)
>  build_module(reload1 reload1.c)
>  build_module(reload2 reload2.c)
> +build_module(func_restore1 func_restore1.c)
> +build_module(func_restore2 func_restore2.c)
>  build_module(tuple_bench tuple_bench.c)
> diff --git a/test/box/func_restore.result b/test/box/func_restore.result

5. The test also passes if I just replace rlist_foreach_entry_safe with
rlist_foreach_entry_safe_reverse in the original code. Which means it
won't test anything in case we ever change how do we put the functions
to the list, or how we walk the list on reload.

I propose you to make the test harder to bypass.

> diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
> new file mode 100644
> index 000000000..ffd69fd2b
> --- /dev/null
> +++ b/test/box/func_restore1.c
> @@ -0,0 +1,34 @@
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <msgpuck.h>
> +
> +#include "module.h"
> +
> +static int
> +echo_num(box_function_ctx_t *ctx, const char *args,
> +	 const char *args_end, unsigned int num)
> +{
> +	char res[16];
> +	char *end = mp_encode_uint(res, num);
> +	box_return_mp(ctx, res, end);
> +	return 0;
> +}
> +
> +

6. Between functions we use a single empty line. The same for the other .c
file.

Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

See 6 comments below.

> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> new file mode 100644
> index 000000000..2cd2f2e8b
> --- /dev/null
> +++ b/src/box/module_cache.c
> @@ -0,0 +1,474 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <dlfcn.h>
> +#include <lua.h>
> +
> +#include "assoc.h"
> +#include "diag.h"
> +#include "fiber.h"
> +#include "module_cache.h"
> +
> +#include "box/error.h"
> +#include "box/port.h"
> +
> +#include "lua/utils.h"
> +#include "libeio/eio.h"
> +
> +static struct mh_strnptr_t *module_cache = NULL;
> +
> +/**
> + * Helpers for cache manipulations.
> + */
> +static void *

1. It returns struct module, therefore the return type must be
'struct module *', not 'void *'. The same for cache_find() in box.lib
implementation.

> +cache_find(const char *str, size_t len)
> +{
> +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> +	if (e == mh_end(module_cache))
> +		return NULL;
> +	return mh_strnptr_node(module_cache, e)->val;
> +}
> +
> +static void
> +cache_update(struct module *m)
> +{
> +	const char *str = m->package;
> +	size_t len = m->package_len;
> +
> +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> +	if (e == mh_end(module_cache))
> +		panic("module: failed to update cache: %s", str);
> +
> +	mh_strnptr_node(module_cache, e)->str = m->package;
> +	mh_strnptr_node(module_cache, e)->val = m;
> +}
> +
> +static int
> +cache_put(struct module *m)
> +{
> +	const struct mh_strnptr_node_t nd = {
> +		.str	= m->package,
> +		.len	= m->package_len,
> +		.hash	= mh_strn_hash(m->package, m->package_len),
> +		.val	= m,
> +	};
> +
> +	mh_int_t e = mh_strnptr_put(module_cache, &nd, NULL, NULL);

2. Put() silently replaces the old value if it is present. I would
recommend to use the next to the last argument to get the old value
and ensure it is mh_end() via an assertion/panic. The same for the other
new put() functions in the other commits.

> +	if (e == mh_end(module_cache)) {
> +		diag_set(OutOfMemory, sizeof(nd), "malloc",
> +			 "module_cache node");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static void
> +cache_del(struct module *m)
> +{
> +	const char *str = m->package;
> +	size_t len = m->package_len;
> +
> +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> +	if (e != mh_end(module_cache)) {

3. Maybe this must be an assertion/panic. I don't see a valid case when
del() is called on an already deleted module. The same for the other
new del() functions in the other commits.

> +		struct module *v = mh_strnptr_node(module_cache, e)->val;
> +		if (v == m) {
> +			/*
> +			 * The module in cache might be updated
> +			 * via force load and old instance is kept
> +			 * by a reference only.
> +			 */
> +			mh_strnptr_del(module_cache, e, NULL);
> +		}
> +	}
> +}

<...>

> +
> +struct module *
> +module_load(const char *package, size_t package_len)
> +{
> +	char path[PATH_MAX];
> +
> +	if (find_package(package, package_len, path, sizeof(path)) != 0)
> +		return NULL;
> +
> +	struct module *m = cache_find(package, package_len);
> +	if (m != NULL) {
> +		struct module_attr attr;
> +		struct stat st;
> +		if (stat(path, &st) != 0) {
> +			diag_set(SystemError, "failed to stat() %s", path);
> +			return NULL;
> +		}
> +
> +		/*
> +		 * In case of cache hit we may reuse existing
> +		 * module which speedup load procedure.
> +		 */
> +		module_attr_fill(&attr, &st);
> +		if (memcmp(&attr, &m->attr, sizeof(attr)) == 0) {

4. Please, add a static assertion, that sizeof(module_attr) == 40.
Otherwise somebody might add a new attribute, which won't be uint64_t,
and would break the comparison without noticing. Also you can make the
attributes be stored as a byte array char[40] to make it impossible to
add any padding into it. Also you can compare the attributes one by
one.

> +			module_ref(m);
> +			return m;
> +		}
> +
> +		/*
> +		 * Module has been updated on a storage device,
> +		 * so load a new instance and update the cache,
> +		 * old entry get evicted but continue residing
> +		 * in memory, fully functional, until last
> +		 * function is unloaded.
> +		 */
> +		m = module_new(package, package_len, path);
> +		if (m != NULL)
> +			cache_update(m);
> +	} else {
> +		m = module_new(package, package_len, path);
> +		if (m != NULL && cache_put(m) != 0) {
> +			module_unload(m);
> +			return NULL;
> +		}
> +	}
> +
> +	return m;
> +}
> +
> +void
> +module_unload(struct module *m)
> +{
> +	module_unref(m);
> +}
> +
> +void
> +module_free(void)
> +{
> +	mh_int_t e;
> +
> +	mh_foreach(module_cache, e) {
> +		struct module *m = mh_strnptr_node(module_cache, e)->val;
> +		module_unload(m);

5. As I said in the previous review, it does not make much sense.
If there are any not unloaded modules, and they try to unload later,
they will see module_cache == NULL and will crash.

Also you can't do unload here, because the module_cache itself does
not keep any references. All the unloads must be done by the module
objects owners. Not by module_cache on its own. For example, if there
is a module having a single reference and used in some other subsystem,
your unload will free it and make it memory invalid. That will crash
in case the module owner will try to access it again.

There should be a panic-check that the module cache is empty already.

> +	}
> +
> +	mh_strnptr_delete(module_cache);
> +	module_cache = NULL;
> +}
> diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> new file mode 100644
> index 000000000..18eb3866a
> --- /dev/null
> +++ b/src/box/module_cache.h
> @@ -0,0 +1,208 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#pragma once
> +
> +#include <stdint.h>
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>

6. You don't need these headers in module_cache.h. They are
needed only in the .c file.

Re: [Tarantool-patches] [PATCH v20 5/7] box/schema.func: switch to new module api

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

See 6 comments below.

On 02.04.2021 14:34, Cyrill Gorcunov via Tarantool-patches wrote:
> Since we have low level module api now we can switch
> the box.schema.func code to use it. In particular we
> define schema_module structure as a wrapper over the
> modules api -- it carries a pointer to general module
> structure.
> 
> Because of modules reload functionality (which was actually
> a big mistake to introduce in a first place, because it is too
> fragile and in case of unintended misuse may simply crash the
> application in a best case) the schema modules carry own cache
> of modules instances. Thus to make overall code somehow close
> to modules api we do:
> 
> 1) every schema_module variable should be named as `mod`. this
>    allows to distinguish this kind of modules from general
>   `module` variables;

1. For base objects we often use name 'base'. So the module_cache
module would be called 'base', and the schema_module would be
called 'module' like it was before your patch. 'mod' and 'module'
don't give any hint how are they different, since you want to
separate them so hard everywhere.

> 2) cache operations are renamed to cache_find/put/update/del;
> 3) C functions are switched to use module_func low level api;
> 4) because low level modules api carry own references we can
>    take no explicit reference when calling a function.
> 
> diff --git a/src/box/func.c b/src/box/func.c
> index 08918e6db..54d0cbc68 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -227,190 +169,214 @@ module_cache_put(struct module *module)

<...>

> +static struct schema_module *
> +__schema_module_load(const char *name, size_t len, bool force)

2. You can't use the double underscore unless it is absolutely
necessary and can't be avoided.
https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html

> +{
> +	struct schema_module *mod = malloc(sizeof(*mod));
> +	if (mod != NULL) {
> +		mod->module = NULL;
> +		mod->refs = 0;
> +		rlist_create(&mod->funcs);
> +	} else {
> +		diag_set(OutOfMemory, sizeof(*mod),
> +			 "malloc", "struct schema_module");
> +		return NULL;
>  	}
>  
> -	module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
> -	if (unlink(load_name) != 0)
> -		say_warn("failed to unlink dso link %s", load_name);
> -	if (rmdir(dir_name) != 0)
> -		say_warn("failed to delete temporary dir %s", dir_name);
> -	if (module->handle == NULL) {
> -		diag_set(ClientError, ER_LOAD_MODULE, package_len,
> -			  package, dlerror());
> -		goto error;
> +	if (force)
> +		mod->module = module_load_force(name, len);
> +	else
> +		mod->module = module_load(name, len);
> +
> +	if (!mod->module) {

3. Please, use explicit == NULL.

> +		free(mod);
> +		return NULL;
>  	}

<...>

> +static int
> +schema_func_c_load(struct schema_module *mod, const char *func_name,
> +		   struct func_c *func)

4. There is a simple rule for method naming and the order of
their arguments: always start the method name with the struct
name, and pass the object in the first argument, like C++.

Here it should be func_c_load_from(), func_c_unload(), and
'func' must be the first argument.

func_c_load_from() so as to emphasize it takes the module to
load from explicitly, on the contrary with func_c_load().

Using the prefix 'schema' for that does not make it easier to
understand the difference between schema_func_c_load and
func_c_load.

>  {
> -	box_function_f f = (box_function_f)dlsym(module->handle, name);
> -	if (f == NULL) {
> -		diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
> -		return NULL;
> +	assert(module_func_is_empty(&func->mf));
> +
> +	if (module_func_load(mod->module, func_name, &func->mf) != 0)
> +		return -1;
> +
> +	func->mod = mod;
> +	rlist_move(&mod->funcs, &func->item);
> +	schema_module_ref(mod);
> +
> +	return 0;
> +}
> +
> +static void
> +schema_func_c_unload(struct func_c *func)
> +{
> +	if (!module_func_is_empty(&func->mf)) {
> +		rlist_del(&func->item);
> +		schema_module_unref(func->mod);
> +		module_func_unload(&func->mf);
> +		func_c_create(func);
>  	}
> -	return f;
>  }
>  
>  int
>  schema_module_reload(const char *package, const char *package_end)
>  {
> -	struct module *old_module = module_cache_find(package, package_end);
> -	if (old_module == NULL) {
> +	struct schema_module *old = cache_find(package, package_end);
> +	if (old == NULL) {
>  		/* Module wasn't loaded - do nothing. */
>  		diag_set(ClientError, ER_NO_SUCH_MODULE, package);
>  		return -1;
>  	}
>  
> -	struct module *new_module = module_load(package, package_end);
> -	if (new_module == NULL)
> +	struct schema_module *new = schema_module_load_force(package, package_end);
> +	if (new == NULL)
>  		return -1;
>  
> +	/*
> +	 * Keep an extra reference to the old module
> +	 * so it won't be freed until reload is complete,
> +	 * otherwise we might free old module then fail
> +	 * on some function loading and in result won't
> +	 * be able to restore old symbols.
> +	 */
> +	schema_module_ref(old);
>  	struct func_c *func, *tmp_func;
> -	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
> -		/* Move immediately for restore sake. */
> -		rlist_move(&new_module->funcs, &func->item);
> +	rlist_foreach_entry_safe(func, &old->funcs, item, tmp_func) {
>  		struct func_name name;
>  		func_split_name(func->base.def->name, &name);
> -		func->func = module_sym(new_module, name.sym);
> -		if (func->func == NULL)
> +		schema_func_c_unload(func);
> +		if (schema_func_c_load(new, name.sym, func) != 0) {
> +			/*
> +			 * WARN: A hack accessing new->funcs directly
> +			 * to start restore from this failing function.
> +			 */

5. Please, drop the hack. You can restore this function individually
right here, and proceed to the restoration of the other functions.

There might be other solutions, but this looks the easiest.

> +			rlist_move(&new->funcs, &func->item);
>  			goto restore;
> -		func->module = new_module;
> +		}
>  	}
> diff --git a/test/box/gh-4648-func-load-unload.result b/test/box/gh-4648-func-load-unload.result
> index e695dd365..91b78d083 100644
> --- a/test/box/gh-4648-func-load-unload.result
> +++ b/test/box/gh-4648-func-load-unload.result
> @@ -71,7 +71,8 @@ check_module_count_diff(-1)
>   | ...
>  
>  -- A not finished invocation of a function from a module prevents
> --- its unload. Until the call is finished.
> +-- low level module intance unload while schema level module is
> +-- free to unload immediately when dropped.

6. As you can see from the issue description in this test, it was
exactly about unloading the dlopen's handle. Not about the module
object deletion. After your change, even if the modules are never
truly unloaded, the test will pass which is obviously not what is
supposed to happen. You need to move the error injection to
module_cache.c to make it work like before and fail if the module
wasn't fully unloaded.

Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

See 10 comments below.

> @TarantoolBot document
> Title: box.lib module
> 
> Overview
> ========
> 
> `box.lib` module provides a way to create, delete and execute
> `C` procedures from shared libraries. Unlike `box.schema.func`
> methods the functions created with `box.lib` help are not persistent
> and live purely in memory. Once a node get turned off they are
> vanished. An initial purpose for them is to execute them on
> nodes which are running in read-only mode.
> 
> Module functions
> ================
> 
> `require('box.lib').load(path) -> obj | error`

1. Implementation in C should allow not to call 'require()' at all. For
example, you don't call require('box.tuple') to use the tuples. The same
for lib.

However I don't know what is the current rule for the new modules - force to
call require() or not. Please, ask Mons what should we do. In case we need to
force to call require() - I have no idea how to do that TBH, since the module
is in C really, not in Lua. It is loaded as a part of the executable file
anyway.


2. Why can't I use box.lib without calling box.cfg{}? I does not depend on it
anyhow. For example, box.tuple works fine, require() on .so files works fine,
even ffi.load() works fine without box.cfg{}. What is the problem with box.lib?

> Unloads a module. Returns `true` on success, otherwise an error
> is thrown. Once the module is unloaded one can't load new
> functions from this module instance.
> 
> Possible errors:
> 
> - IllegalParams: a module is not supplied.
> - IllegalParams: a module is already unloaded.
> 
> Example:
> 
> ``` Lua
> m = require('box.lib').load('path/to/library')
> --
> -- do something with module
> --
> m:unload()
> ```
> 
> If there are functions from this module referenced somewhere
> in other places of Lua code they still can be executed because
> the module continue sitting in memory until the last reference
> to it is closed.
> 
> If the module become a target to the Lua's garbage collector
> then unload is called implicitly.
> 
> module:load(name) -> obj | error`

3. You need to add ` in the beginning too. Otherwise the document
structure would be screwed.

> ---
>  src/box/CMakeLists.txt |   1 +
>  src/box/lua/box_lib.c  | 590 +++++++++++++++++++++++++++++++++++++++++
>  src/box/lua/box_lib.h  |  25 ++

4. It is already in box/ folder, you don't need box_ prefix for
the files. For example, we don't have box_tuple.h/box_tuple.c. We
have just tuple.c and tuple.h, and so on.

>  src/box/lua/init.c     |   2 +
>  test/box/misc.result   |   1 +
>  5 files changed, 619 insertions(+)
>  create mode 100644 src/box/lua/box_lib.c
>  create mode 100644 src/box/lua/box_lib.h
> 
> diff --git a/src/box/lua/box_lib.c b/src/box/lua/box_lib.c
> new file mode 100644
> index 000000000..ce2ef8902
> --- /dev/null
> +++ b/src/box/lua/box_lib.c
> @@ -0,0 +1,590 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <lua.h>
> +
> +#include "box/error.h"
> +#include "box/port.h"
> +
> +#include "tt_static.h"
> +
> +#include "assoc.h"
> +#include "box_lib.h"
> +#include "diag.h"
> +#include "module_cache.h"
> +
> +#include "lua/utils.h"
> +
> +/**
> + * Function descriptor.
> + */
> +struct box_module_func {
> +	/** C function to call. */
> +	struct module_func mf;

5. You could call it 'base' like we normally do when inherit
structures.

> +	/** Number of references. */
> +	int64_t refs;
> +	/** Length of functon name in @a key. */
> +	size_t sym_len;
> +	/** Length of @a key. */
> +	size_t len;
> +	/** Function hash key. */
> +	char key[0];
> +};

<...>

> +/** Handle __index request for a module object. */
> +static int
> +lbox_module_index(struct lua_State *L)
> +{
> +	lua_getmetatable(L, 1);
> +	lua_pushvalue(L, 2);
> +	lua_rawget(L, -2);
> +	if (!lua_isnil(L, -1))
> +		return 1;

6. What is happening here in these 5 lines?

> +
> +	struct module *m = get_udata(L, uname_lib);
> +	if (m == NULL) {
> +		lua_pushnil(L);
> +		return 1;
> +	}

<...>

> +
> +/**
> + * Load a function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and either returns a cached function or
> + * creates a new function object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - SystemError: unable to open a module due to a system error.
> + * - ClientError: a module does not exist.
> + * - OutOfMemory: unable to allocate a module.
> + *
> + * @returns module object on success or throws an error.
> + */
> +static int
> +lbox_module_load_func(struct lua_State *L)
> +{
> +	const char *method = "function = module:load";
> +	const char fmt_noname[] = "Expects %s(\'name\') but no name passed";

7. Why do you randomly jump between const char * and const char []?
Please, use const char * to be consistent, and because it looks simpler.

> +
> +	if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) {

8. Here you use lua_isstring(), in some places you use
lua_type() == LUA_TSTRING. Please, be consistent, and choose one way.

> +		diag_set(IllegalParams, fmt_noname, method);
> +		return luaT_error(L);
> +	}
> +
> +	struct module *m = get_udata(L, uname_lib);
> +	if (m == NULL) {
> +		const char *fmt =
> +			"Expects %s(\'name\') but not module object passed";
> +		diag_set(IllegalParams, fmt, method);
> +		return luaT_error(L);
> +	}
> +
> +	size_t sym_len;
> +	const char *sym = lua_tolstring(L, 2, &sym_len);
> +
> +	if (sym_len < 1) {
> +		diag_set(IllegalParams, fmt_noname, method);
> +		return luaT_error(L);
> +	}
> +
> +	/*
> +	 * Functions are bound to a module symbols, thus
> +	 * since the hash is global it should be unique
> +	 * per module. The symbol (function name) is the
> +	 * last part of the hash key.
> +	 */
> +	const char *key = tt_sprintf("%p.%s.%s", (void *)m,
> +				     m->package, sym);

9. I wouldn't use the static buffer in non-trivial code, and
never in Lua and Lua C code. Especially after the recently
discovered bug.

Firstly, something inside box_module_func_new() might also try
to use the static buffer and would overwrite it. Secondly, deep
inside it accesses Lua to find package.* content, and that might
trigger Lua GC, which also might use the static buffer for
anything.

> +	size_t len = strlen(key);
> +
> +	struct box_module_func *cf = cache_find(key, len);
> +	if (cf == NULL) {
> +		cf = box_module_func_new(m, key, len, sym_len);
> +		if (cf == NULL)
> +			return luaT_error(L);
> +	} else {
> +		box_module_func_ref(cf);
> +	}
> +
> +	new_udata(L, uname_func, cf);
> +	return 1;
> +}
> +
> +/**
> + * Unload a function.
> + *
> + * Take a function object from the caller stack @a L and unload it.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: the function is not supplied.
> + * - IllegalParams: the function already unloaded.
> + *
> + * @returns true on success or throwns an error.
> + */
> +static int
> +lbox_func_unload(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1) {
> +		diag_set(IllegalParams, "Expects function:unload()");
> +		return luaT_error(L);
> +	}
> +
> +	struct box_module_func *cf = get_udata(L, uname_func);
> +	if (cf == NULL) {
> +		diag_set(IllegalParams, "The function is unloaded");
> +		return luaT_error(L);
> +	}
> +
> +	set_udata(L, uname_func, NULL);

10. By having get_udata and set_udata helpers you force yourself to
do double-check for the cdata on each pair of these calls. While
you could get void** first time, and simply *ptr = NULL it instead
of set_udata(NULL). Up to you, just a note.

Re: [Tarantool-patches] [PATCH v20 7/7] test: add box.lib test

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

Please, add tests for automatic garbage collection to
ensure the modules and functions are unloaded.

On 02.04.2021 14:34, Cyrill Gorcunov wrote:
> Note that the test is disabled in suite.ini for a while
> because we need to update test-run first like
> 
>  | diff --git a/pretest_clean.lua b/pretest_clean.lua
>  | index 446ecf2..b356ebd 100644
>  | --- a/pretest_clean.lua
>  | +++ b/pretest_clean.lua
>  | @@ -228,6 +228,7 @@ local function clean()
>  |          ['box.internal.sequence'] = true,
>  |          ['box.internal.session'] = true,
>  |          ['box.internal.space'] = true,
>  | +        ['box.lib'] = true,
>  |          buffer = true,
>  |          clock = true,
>  |          console = true,

I think I already said it, but you could just make that patch in
scope of your patchset. What are you waiting for?

Re: [Tarantool-patches] [PATCH v20 1/7] box/schema: make sure hashes are created

[Cyrill Gorcunov via Tarantool-patches]

On Mon, Apr 05, 2021 at 05:45:54PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> I don't think this patch is needed, because it would crash anyway later,
> if the hashes couldn't be allocated. On any of their usages. But
> whatever.
> 
> I also don't see how it is 'in scope of 4642'. It is completely
> unrelated. So please, at least drop the reference.

I filed a new bug https://github.com/tarantool/tarantool/issues/5967
and will address it separately later then.

Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Cyrill Gorcunov via Tarantool-patches]

On Mon, Apr 05, 2021 at 05:47:32PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 6 comments below.
> 
> On 02.04.2021 14:34, Cyrill Gorcunov via Tarantool-patches wrote:
> > In commit 96938fafb an ability to hot reload of modules
> 
> 1. Please, add the commit title in parentheses and quotes after the
> hash value.

OK

> 
> > has been introduced. When module is been reloaded his
> > functions are resolved to new symbols but if something
> > went wrong it is supposed to restore old symbols from
> > the old module. Actually current code restores only
> > one function and may crash if there a bunch of functions
> > to restore. Lets fix it.
> > 
> > Part-of #4642
> 
> 2. How is it a part of 4642? It is totally unrelated. It is a
> separate bug, existing before 4642, and which could exist after
> 4642 without this patch, and which does not block 4642 anyhow.

It *is* related. I patch this code later, when move to the new module
API interface and I'm not going to continue supporting this bug
in the new code. This was the reason why I had to patch the code
first. And that's why it is part of 4642.

I could make a separate issue for this and fix it if you prefer
but definitely in this series. I don't wanna do a double work.

> 
> > diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md
> > new file mode 100644
> > index 000000000..7e189617f
> > --- /dev/null
> > +++ b/changelogs/unreleased/fix-module-reload.md
> > @@ -0,0 +1,4 @@
> > +## bugfix/core
> > +
> > +* Fix module reloading procedure which may crash in case if
> > +  new module is corrupted (gh-4642).
> 
> 3. 'module' term is used not only for .so/.dylib files, but also
> for Lua modules. You need to be more specific that this is about
> compiled files.

OK

> > +	 * Some old functions are not found int the new module,
> 
> 4. int -> in.

Thanks!

> > +	 * thus restore all migrated functions back to original.
> >  	 */
> > diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
> > index 06bfbbe9d..944831af2 100644
> > --- a/test/box/CMakeLists.txt
> > +++ b/test/box/CMakeLists.txt
> > @@ -2,4 +2,6 @@ include_directories(${MSGPUCK_INCLUDE_DIRS})
> >  build_module(function1 function1.c)
> >  build_module(reload1 reload1.c)
> >  build_module(reload2 reload2.c)
> > +build_module(func_restore1 func_restore1.c)
> > +build_module(func_restore2 func_restore2.c)
> >  build_module(tuple_bench tuple_bench.c)
> > diff --git a/test/box/func_restore.result b/test/box/func_restore.result
> 
> 5. The test also passes if I just replace rlist_foreach_entry_safe with
> rlist_foreach_entry_safe_reverse in the original code. Which means it
> won't test anything in case we ever change how do we put the functions
> to the list, or how we walk the list on reload.

The key moment here is not about the list traverse direction: that's
why I use for_each macro instead of the broken cycle we had before.
The order does matter to trigger the issue *without* this patch,
ie to put functions in reverse order and force the recovery
(as is done in my test, I even put a comment there).

With the new approach using list_for() it is doesn't matter now
how exactly we traverse. If that is what you mean.

> I propose you to make the test harder to bypass.

The original code restores only one last failed function and
my test is done the way to trigger the issue. I would like to
make it more harder to bypass but I don't see an other way.
> > +static int
> > +echo_num(box_function_ctx_t *ctx, const char *args,
> > +	 const char *args_end, unsigned int num)
> > +{
> > +	char res[16];
> > +	char *end = mp_encode_uint(res, num);
> > +	box_return_mp(ctx, res, end);
> > +	return 0;
> > +}
> > +
> > +
> 
> 6. Between functions we use a single empty line. The same for the other .c
> file.

Thanks, will do.

Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Cyrill Gorcunov via Tarantool-patches]

On Mon, Apr 05, 2021 at 05:52:47PM +0200, Vladislav Shpilevoy wrote:
> > +/**
> > + * Helpers for cache manipulations.
> > + */
> > +static void *
> 
> 1. It returns struct module, therefore the return type must be
> 'struct module *', not 'void *'. The same for cache_find() in box.lib
> implementation.

OK

> > +static int
> > +cache_put(struct module *m)
> > +{
> > +	const struct mh_strnptr_node_t nd = {
> > +		.str	= m->package,
> > +		.len	= m->package_len,
> > +		.hash	= mh_strn_hash(m->package, m->package_len),
> > +		.val	= m,
> > +	};
> > +
> > +	mh_int_t e = mh_strnptr_put(module_cache, &nd, NULL, NULL);
> 
> 2. Put() silently replaces the old value if it is present. I would
> recommend to use the next to the last argument to get the old value
> and ensure it is mh_end() via an assertion/panic. The same for the other
> new put() functions in the other commits.

Sure

> 
> > +	if (e == mh_end(module_cache)) {
> > +		diag_set(OutOfMemory, sizeof(nd), "malloc",
> > +			 "module_cache node");
> > +		return -1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void
> > +cache_del(struct module *m)
> > +{
> > +	const char *str = m->package;
> > +	size_t len = m->package_len;
> > +
> > +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> > +	if (e != mh_end(module_cache)) {
> 
> 3. Maybe this must be an assertion/panic. I don't see a valid case when
> del() is called on an already deleted module. The same for the other
> new del() functions in the other commits.

When we put the module in the cache and something is failed we call
generic module_unload which in turn calls cache_del

module_load
  ...
  m = module_new(package, package_len, path);
  if (m != NULL && cache_put(m) != 0) {
    module_unload(m);
      --> module_unref
            if (--m->refs == 0) {
              cache_del(m);

this is done for simplicity. So calling cache_del with
module which is not in cache is fine.

> > +
> > +		/*
> > +		 * In case of cache hit we may reuse existing
> > +		 * module which speedup load procedure.
> > +		 */
> > +		module_attr_fill(&attr, &st);
> > +		if (memcmp(&attr, &m->attr, sizeof(attr)) == 0) {
> 
> 4. Please, add a static assertion, that sizeof(module_attr) == 40.
> Otherwise somebody might add a new attribute, which won't be uint64_t,
> and would break the comparison without noticing. Also you can make the
> attributes be stored as a byte array char[40] to make it impossible to
> add any padding into it. Also you can compare the attributes one by
> one.

Not needed anymore.

static void
module_attr_fill(struct module_attr *attr, struct stat *st)
{
-->	memset(attr, 0, sizeof(*attr));

any possible padding is explicitly cleared. Initially I though
of using __packed attribue or something but at the end realised
that using explicit cleanup is a way more robust.

> > +void
> > +module_free(void)
> > +{
> > +	mh_int_t e;
> > +
> > +	mh_foreach(module_cache, e) {
> > +		struct module *m = mh_strnptr_node(module_cache, e)->val;
> > +		module_unload(m);
> 
> 5. As I said in the previous review, it does not make much sense.
> If there are any not unloaded modules, and they try to unload later,
> they will see module_cache == NULL and will crash.
> 
> Also you can't do unload here, because the module_cache itself does
> not keep any references. All the unloads must be done by the module
> objects owners. Not by module_cache on its own. For example, if there
> is a module having a single reference and used in some other subsystem,
> your unload will free it and make it memory invalid. That will crash
> in case the module owner will try to access it again.
> 
> There should be a panic-check that the module cache is empty already.

Not at all. You can exit tarantool via Ctrl+D inside console and
modules won't be empty and we should clean them up. So I can and
I should unload modules here. Vlad, this is _exit_ path called when
we're exiting tarantool. What I'm missing?

> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> 
> 6. You don't need these headers in module_cache.h. They are
> needed only in the .c file.

Yes, thanks for pointing.

Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Vladislav Shpilevoy via Tarantool-patches]

>>> diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
>>> index 06bfbbe9d..944831af2 100644
>>> --- a/test/box/CMakeLists.txt
>>> +++ b/test/box/CMakeLists.txt
>>> @@ -2,4 +2,6 @@ include_directories(${MSGPUCK_INCLUDE_DIRS})
>>>  build_module(function1 function1.c)
>>>  build_module(reload1 reload1.c)
>>>  build_module(reload2 reload2.c)
>>> +build_module(func_restore1 func_restore1.c)
>>> +build_module(func_restore2 func_restore2.c)
>>>  build_module(tuple_bench tuple_bench.c)
>>> diff --git a/test/box/func_restore.result b/test/box/func_restore.result
>>
>> 5. The test also passes if I just replace rlist_foreach_entry_safe with
>> rlist_foreach_entry_safe_reverse in the original code. Which means it
>> won't test anything in case we ever change how do we put the functions
>> to the list, or how we walk the list on reload.
> 
> The key moment here is not about the list traverse direction: that's
> why I use for_each macro instead of the broken cycle we had before.
> The order does matter to trigger the issue *without* this patch,
> ie to put functions in reverse order and force the recovery
> (as is done in my test, I even put a comment there).
> 
> With the new approach using list_for() it is doesn't matter now
> how exactly we traverse. If that is what you mean.

Nope, that is not what I mean. But I don't know how to say it
easier, even in Russian.

>> I propose you to make the test harder to bypass.
> 
> The original code restores only one last failed function and
> my test is done the way to trigger the issue. I would like to
> make it more harder to bypass but I don't see an other way.

Well, did you try? The test would fail regardless of the order, if
you would try to drop the middle function. Or both the first and
the last functions. Or try all the combinations.

Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Vladislav Shpilevoy via Tarantool-patches]

>>> +void
>>> +module_free(void)
>>> +{
>>> +	mh_int_t e;
>>> +
>>> +	mh_foreach(module_cache, e) {
>>> +		struct module *m = mh_strnptr_node(module_cache, e)->val;
>>> +		module_unload(m);
>>
>> 5. As I said in the previous review, it does not make much sense.
>> If there are any not unloaded modules, and they try to unload later,
>> they will see module_cache == NULL and will crash.
>>
>> Also you can't do unload here, because the module_cache itself does
>> not keep any references. All the unloads must be done by the module
>> objects owners. Not by module_cache on its own. For example, if there
>> is a module having a single reference and used in some other subsystem,
>> your unload will free it and make it memory invalid. That will crash
>> in case the module owner will try to access it again.
>>
>> There should be a panic-check that the module cache is empty already.
> 
> Not at all. You can exit tarantool via Ctrl+D inside console and
> modules won't be empty and we should clean them up. So I can and
> I should unload modules here. Vlad, this is _exit_ path called when
> we're exiting tarantool. What I'm missing?

Well, if there are modules in Lua, they might have more than 1 reference,
and your module_unload won't free them anyway. But that does not matter
much as you try to free the objects which don't belong to you. The
refs do not belong to the module_cache subsystem. They belong to the
callers of module_load.

That is a bug. Freeing what does not belong to you.

And since the Lua land is not properly terminated with freeing all the
references, the only valid way you have here is not to do anything at
all AFAIS. Or free the hash table + set it to NULL so it would at least
crash in a sane way in case we ever start freeing all Lua refs. But
under no circumstances can you unref the modules which you didn't ref.
They were referenced by schema modules and box.lib modules, and
therefore must be unreferenced by them.

Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Cyrill Gorcunov via Tarantool-patches]

On Tue, Apr 06, 2021 at 10:02:40PM +0200, Vladislav Shpilevoy wrote:
> >> 5. The test also passes if I just replace rlist_foreach_entry_safe with
> >> rlist_foreach_entry_safe_reverse in the original code. Which means it
> >> won't test anything in case we ever change how do we put the functions
> >> to the list, or how we walk the list on reload.
> > 
> > The key moment here is not about the list traverse direction: that's
> > why I use for_each macro instead of the broken cycle we had before.
> > The order does matter to trigger the issue *without* this patch,
> > ie to put functions in reverse order and force the recovery
> > (as is done in my test, I even put a comment there).
> > 
> > With the new approach using list_for() it is doesn't matter now
> > how exactly we traverse. If that is what you mean.
> 
> Nope, that is not what I mean. But I don't know how to say it
> easier, even in Russian.

OK, lets go another way. Say we have 3 functions: {1,2,3}.
When we call them in order 1, then 2, then 3 the list of
functions gonna be {3, 2, 1} because they are prepended.

Then we start reloading procedure (we're talking aboout original
code). And we have old = {3, 2, 1}, new = { } and we processed 3
and 2 witout an error, so we have old = { 1 }, new = {3, 2}. When
we tried to resolve function 1 lets assume it has failed, we jump
into restore procedure and restore a sole function 1, which lead
to situation where old = { 1 } and function 3 and 2 are simply
dissapeared from old module. This is because of _when_ we have failed.
If we fail at first element, ie when resolving function {3}, then restore
will pass fine and we will have old = {3, 2, 1} without problem.

That said current restore procedure can handle only one failing
entry andif there were other entries before failing one they will
be simply vanished. That's what I've been trying to say.

After the patch is doesn't matter in which order we resolve symbols
because we resolve every element migrated from old list.

Since current code restores only one entry we have a 3 cases: 1) entry
failing at head of the list, 2) entry failing at the middle of the list
and 3) entry failing at the last element. (1) is restored properly
even in current code while (2) and (3) causes a crash and (2) is
rather a subset of (3). So I tested (3) only.

> >> I propose you to make the test harder to bypass.
> > 
> > The original code restores only one last failed function and
> > my test is done the way to trigger the issue. I would like to
> > make it more harder to bypass but I don't see an other way.
> 
> Well, did you try? The test would fail regardless of the order, if
> you would try to drop the middle function. Or both the first and
> the last functions. Or try all the combinations.

Nope. See above. Case (1) handled without problem. But no problem
I'll extend the test.

Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Cyrill Gorcunov via Tarantool-patches]

On Tue, Apr 06, 2021 at 10:09:20PM +0200, Vladislav Shpilevoy wrote:
> >>> +void
> >>> +module_free(void)
> >>> +{
> >>> +	mh_int_t e;
> >>> +
> >>> +	mh_foreach(module_cache, e) {
> >>> +		struct module *m = mh_strnptr_node(module_cache, e)->val;
> >>> +		module_unload(m);
> >>
> >> 5. As I said in the previous review, it does not make much sense.
> >> If there are any not unloaded modules, and they try to unload later,
> >> they will see module_cache == NULL and will crash.
> >>
> >> Also you can't do unload here, because the module_cache itself does
> >> not keep any references. All the unloads must be done by the module
> >> objects owners. Not by module_cache on its own. For example, if there
> >> is a module having a single reference and used in some other subsystem,
> >> your unload will free it and make it memory invalid. That will crash
> >> in case the module owner will try to access it again.
> >>
> >> There should be a panic-check that the module cache is empty already.
> > 
> > Not at all. You can exit tarantool via Ctrl+D inside console and
> > modules won't be empty and we should clean them up. So I can and
> > I should unload modules here. Vlad, this is _exit_ path called when
> > we're exiting tarantool. What I'm missing?
> 
> Well, if there are modules in Lua, they might have more than 1 reference,
> and your module_unload won't free them anyway. But that does not matter
> much as you try to free the objects which don't belong to you. The
> refs do not belong to the module_cache subsystem. They belong to the
> callers of module_load.
> 
> That is a bug. Freeing what does not belong to you.

I do not agree here, since objects are belong to this subsystem,
and subsystem allocates them. And the bug is rather in caller side
which should had install some hooks to detect exits and unref objects.

But as you pointed below Lua is not properly terminated and the
subsystem does only thing it knows about -- unref objects it has
allocated (we setup a first ref upon allocation). It is still somehow
ugly because of potential extra refs on Lua side and I now think
maybe we should free allocated memory in a force way. But that's
true that even though we won't have a clean exit. I tend to agree
that simply free and zap the hash table is best what we could do
for now. Will update.

> And since the Lua land is not properly terminated with freeing all the
> references, the only valid way you have here is not to do anything at
> all AFAIS. Or free the hash table + set it to NULL so it would at least
> crash in a sane way in case we ever start freeing all Lua refs. But
> under no circumstances can you unref the modules which you didn't ref.
> They were referenced by schema modules and box.lib modules, and
> therefore must be unreferenced by them.

Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Vladislav Shpilevoy via Tarantool-patches]

On 07.04.2021 00:05, Cyrill Gorcunov wrote:
> On Tue, Apr 06, 2021 at 10:09:20PM +0200, Vladislav Shpilevoy wrote:
>>>>> +void
>>>>> +module_free(void)
>>>>> +{
>>>>> +	mh_int_t e;
>>>>> +
>>>>> +	mh_foreach(module_cache, e) {
>>>>> +		struct module *m = mh_strnptr_node(module_cache, e)->val;
>>>>> +		module_unload(m);
>>>>
>>>> 5. As I said in the previous review, it does not make much sense.
>>>> If there are any not unloaded modules, and they try to unload later,
>>>> they will see module_cache == NULL and will crash.
>>>>
>>>> Also you can't do unload here, because the module_cache itself does
>>>> not keep any references. All the unloads must be done by the module
>>>> objects owners. Not by module_cache on its own. For example, if there
>>>> is a module having a single reference and used in some other subsystem,
>>>> your unload will free it and make it memory invalid. That will crash
>>>> in case the module owner will try to access it again.
>>>>
>>>> There should be a panic-check that the module cache is empty already.
>>>
>>> Not at all. You can exit tarantool via Ctrl+D inside console and
>>> modules won't be empty and we should clean them up. So I can and
>>> I should unload modules here. Vlad, this is _exit_ path called when
>>> we're exiting tarantool. What I'm missing?
>>
>> Well, if there are modules in Lua, they might have more than 1 reference,
>> and your module_unload won't free them anyway. But that does not matter
>> much as you try to free the objects which don't belong to you. The
>> refs do not belong to the module_cache subsystem. They belong to the
>> callers of module_load.
>>
>> That is a bug. Freeing what does not belong to you.
> 
> I do not agree here, since objects are belong to this subsystem,
> and subsystem allocates them.

It does not matter who allocated them. It is a matter of ownership.
For example, we have tuples. They are allocated on slabs. But they belong
to the code which called tuple_ref(). And until the ref is gone, the
tuple can't be deleted. And the slabs can't simply free all the memory
under the feet of the tuple owners.

The same for tuple_format. It can't be deleted until its tuples are
all deleted. Even if the entire process is being shutdown, deletion
of the formats must happen naturally. After all the tuples are properly
deleted, the formats would be gone too, automatically.

The same for the other objects having a reference counter. You can't
just unref it whenever you feel like it.

It is the same here. The modules are owned by the code which called
load/ref. Not by the module cache. The cache itself does not keep any
single ref. Otherwise the modules would never be deleted, because they
would have always at least one ref from the cache itself.

The module cache job is to cache, not to own. Owners are the schema
modules and box.lib modules. The cache **does not own**, therefore it
can't just delete whatever it wants.

> And the bug is rather in caller side
> which should had install some hooks to detect exits and unref objects.
> 
> But as you pointed below Lua is not properly terminated and the
> subsystem does only thing it knows about -- unref objects it has
> allocated (we setup a first ref upon allocation). It is still somehow
> ugly because of potential extra refs on Lua side and I now think
> maybe we should free allocated memory in a force way.

As I said, under no circumstances you can free the objects which you do
not own.

> But that's
> true that even though we won't have a clean exit. I tend to agree
> that simply free and zap the hash table is best what we could do
> for now. Will update.

I am fine with freeing the hash table itself and setting it to NULL, if
you want to free something so hard. Then at least in future it would
crash right away on any attempt to load/unload something after the cache
was destroyed. Not at some random time due to memory corruptions if you
would free the modules which you don't own and then they would be
accessed. Might happen easily if we ever would allow to load the modules
from C API, or would terminate Lua properly.

Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Apr 07, 2021 at 01:43:26AM +0200, Vladislav Shpilevoy wrote:
> 
> The module cache job is to cache, not to own. Owners are the schema
> modules and box.lib modules. The cache **does not own**, therefore it
> can't just delete whatever it wants.
> 
> > And the bug is rather in caller side
> > which should had install some hooks to detect exits and unref objects.
> > 
> > But as you pointed below Lua is not properly terminated and the
> > subsystem does only thing it knows about -- unref objects it has
> > allocated (we setup a first ref upon allocation). It is still somehow
> > ugly because of potential extra refs on Lua side and I now think
> > maybe we should free allocated memory in a force way.
> 
> As I said, under no circumstances you can free the objects which you do
> not own.

With this assumption OS should not release userspace memory on process
termination if userspace application didn't call free(), right? OS allocated
memory, cached it in pages but doesn't own it, userspace should call free
first? Vlad, the key difference here is that our engine does not shutdown
and then tarantool continue to work without the cache. It is exit path where
we should release all memory. This as well applies to all other caches,
slabs and etc. It doesn't matter that currently we simply have no way
to jump into Lua internals and force clear all references sits there.

I hardly convince you here but I see your point and partly agree.

> > But that's
> > true that even though we won't have a clean exit. I tend to agree
> > that simply free and zap the hash table is best what we could do
> > for now. Will update.
> 
> I am fine with freeing the hash table itself and setting it to NULL, if
> you want to free something so hard. Then at least in future it would
> crash right away on any attempt to load/unload something after the cache
> was destroyed. Not at some random time due to memory corruptions if you
> would free the modules which you don't own and then they would be
> accessed. Might happen easily if we ever would allow to load the modules
> from C API, or would terminate Lua properly.

Sure. I think this is the best option, will do.

Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module

[Cyrill Gorcunov via Tarantool-patches]

On Mon, Apr 05, 2021 at 06:04:41PM +0200, Vladislav Shpilevoy wrote:
> > `require('box.lib').load(path) -> obj | error`
> 
> 1. Implementation in C should allow not to call 'require()' at all. For
> example, you don't call require('box.tuple') to use the tuples. The same
> for lib.
> 
> However I don't know what is the current rule for the new modules - force to
> call require() or not. Please, ask Mons what should we do. In case we need to
> force to call require() - I have no idea how to do that TBH, since the module
> is in C really, not in Lua. It is loaded as a part of the executable file
> anyway.
> 
> 2. Why can't I use box.lib without calling box.cfg{}? I does not depend on it
> anyhow. For example, box.tuple works fine, require() on .so files works fine,
> even ffi.load() works fine without box.cfg{}. What is the problem with box.lib?

Got rid of it and box.lib works without box.cfg{} call now, I'll send a new version.

> 
> > +/** Handle __index request for a module object. */
> > +static int
> > +lbox_module_index(struct lua_State *L)
> > +{
> > +	lua_getmetatable(L, 1);
> > +	lua_pushvalue(L, 2);
> > +	lua_rawget(L, -2);
> > +	if (!lua_isnil(L, -1))
> > +		return 1;
> 
> 6. What is happening here in these 5 lines?

Actually this snippet comes from popen code where
we have wrappers on lua level and test for metamethods.
While it doesn't hurt it makes no much sense in current
code, thanks! Will drop.

And I agree on all comments, will address them and resend
the series. Thanks for review, Vlad!

Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Apr 07, 2021 at 07:59:35PM +0300, Cyrill Gorcunov wrote:
> > > +/** Handle __index request for a module object. */
> > > +static int
> > > +lbox_module_index(struct lua_State *L)
> > > +{
> > > +	lua_getmetatable(L, 1);
> > > +	lua_pushvalue(L, 2);
> > > +	lua_rawget(L, -2);
> > > +	if (!lua_isnil(L, -1))
> > > +		return 1;
> > 
> > 6. What is happening here in these 5 lines?
> 
> Actually this snippet comes from popen code where
> we have wrappers on lua level and test for metamethods.
> While it doesn't hurt it makes no much sense in current
> code, thanks! Will drop.

Heh, in real we need these to handle metamethods such as
"module:load". I added a comment into the code.

Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module

[Vladislav Shpilevoy via Tarantool-patches]

On 07.04.2021 22:22, Cyrill Gorcunov wrote:
> On Wed, Apr 07, 2021 at 07:59:35PM +0300, Cyrill Gorcunov wrote:
>>>> +/** Handle __index request for a module object. */
>>>> +static int
>>>> +lbox_module_index(struct lua_State *L)
>>>> +{
>>>> +	lua_getmetatable(L, 1);
>>>> +	lua_pushvalue(L, 2);
>>>> +	lua_rawget(L, -2);
>>>> +	if (!lua_isnil(L, -1))
>>>> +		return 1;
>>>
>>> 6. What is happening here in these 5 lines?
>>
>> Actually this snippet comes from popen code where
>> we have wrappers on lua level and test for metamethods.
>> While it doesn't hurt it makes no much sense in current
>> code, thanks! Will drop.
> 
> Heh, in real we need these to handle metamethods such as
> "module:load". I added a comment into the code.

For methods you need to use the metatype. It is defined as
luaL_register_type(), where you can add methods, not just
__index. The latter is more for, well, indexing. Getting an
attribute, for example. See an example in lua/popen.c.

Methods should not end up in __index.

Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Apr 07, 2021 at 10:28:09PM +0200, Vladislav Shpilevoy wrote:
> >>
> >> Actually this snippet comes from popen code where
> >> we have wrappers on lua level and test for metamethods.
> >> While it doesn't hurt it makes no much sense in current
> >> code, thanks! Will drop.
> > 
> > Heh, in real we need these to handle metamethods such as
> > "module:load". I added a comment into the code.
> 
> For methods you need to use the metatype. It is defined as
> luaL_register_type(), where you can add methods, not just
> __index. The latter is more for, well, indexing. Getting an
> attribute, for example. See an example in lua/popen.c.
> 
> Methods should not end up in __index.

Wait. Here is what I have now

void
box_lua_lib_init(struct lua_State *L)
{
	func_hash = mh_strnptr_new();
	if (func_hash == NULL)
		panic("box.lib: Can't allocate func hash table");

	static const struct luaL_Reg top_methods[] = {
		{ "load",		lbox_module_load	},
		{ NULL, NULL },
	};
	luaL_register(L, "box.lib", top_methods);
	lua_pop(L, 1);

	static const struct luaL_Reg lbox_module_methods[] = {
		{ "unload",		lbox_module_unload	},
		{ "load",		lbox_module_load_func	},
		{ "__index",		lbox_module_index	},
		{ "__serialize",	lbox_module_serialize	},
		{ "__gc",		lbox_module_gc		},
		{ NULL, NULL },
	};
	luaL_register_type(L, uname_lib, lbox_module_methods);
	...
}

If I don't lookup for key being some nonnil value in __index
(ie those 5 lines) then module:load simply stop working. As
far as I understand the __index is called when Lua looks up
for a key in table. It sends "load" there and since it is
in metatable it executes lbox_module_load_func as it should.
What I'm missing?

Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Apr 07, 2021 at 11:37:15PM +0300, Cyrill Gorcunov wrote:
...
> 
> If I don't lookup for key being some nonnil value in __index
> (ie those 5 lines) then module:load simply stop working. As
> far as I understand the __index is called when Lua looks up
> for a key in table. It sends "load" there and since it is
> in metatable it executes lbox_module_load_func as it should.
> What I'm missing?

Look into fiber code

static const struct luaL_Reg lbox_fiber_meta [] = {
	...
	{"__serialize", lbox_fiber_serialize},
	{"__tostring", lbox_fiber_tostring},
	{"join", lbox_fiber_join},
	{"set_joinable", lbox_fiber_set_joinable},
	{"wakeup", lbox_fiber_wakeup},
	{"__index", lbox_fiber_index},
	{NULL, NULL}
};

static int
lbox_fiber_index(struct lua_State *L)
{
	if (lua_gettop(L) < 2)
		return 0;
	if (lua_isstring(L, 2) && strcmp(lua_tostring(L, 2), "storage") == 0)
		return lbox_fiber_storage(L);

-->	/* Get value from metatable */
	lua_getmetatable(L, 1);
	lua_pushvalue(L, 2);
	lua_gettable(L, -2);
	return 1;
}

So I need to do the same

	Cyrill

Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module

[Vladislav Shpilevoy via Tarantool-patches]

On 07.04.2021 22:45, Cyrill Gorcunov via Tarantool-patches wrote:
> On Wed, Apr 07, 2021 at 11:37:15PM +0300, Cyrill Gorcunov wrote:
> ...
>>
>> If I don't lookup for key being some nonnil value in __index
>> (ie those 5 lines) then module:load simply stop working. As
>> far as I understand the __index is called when Lua looks up
>> for a key in table. It sends "load" there and since it is
>> in metatable it executes lbox_module_load_func as it should.
>> What I'm missing?
> 
> Look into fiber code
> 
> static const struct luaL_Reg lbox_fiber_meta [] = {
> 	...
> 	{"__serialize", lbox_fiber_serialize},
> 	{"__tostring", lbox_fiber_tostring},
> 	{"join", lbox_fiber_join},
> 	{"set_joinable", lbox_fiber_set_joinable},
> 	{"wakeup", lbox_fiber_wakeup},
> 	{"__index", lbox_fiber_index},
> 	{NULL, NULL}
> };
> 
> static int
> lbox_fiber_index(struct lua_State *L)
> {
> 	if (lua_gettop(L) < 2)
> 		return 0;
> 	if (lua_isstring(L, 2) && strcmp(lua_tostring(L, 2), "storage") == 0)
> 		return lbox_fiber_storage(L);
> 
> -->	/* Get value from metatable */
> 	lua_getmetatable(L, 1);
> 	lua_pushvalue(L, 2);
> 	lua_gettable(L, -2);
> 	return 1;
> }
> 
> So I need to do the same

Yeah, I see now. I thought that __index is called only for the not
present fields. And it is true, but only if they are real fields of
a concrete object, not metafields.

In this example:

	mt = {
	    call = function() print('do call') end,
	    __index = function(self, key)
	        print(key)
	    end,
	}
	t = setmetatable({}, mt)

an attempt to do t.call or t:call() always goes through __index.
While in this example:

	mt = {
	    __index = function(self, key)
	        print(key)
	    end,
	}
	t = setmetatable({call = function() print('do call') end}, mt)

the same code bypasses __index. Your case is the first one, and it
seems we can't do anything about it so far.