[Tarantool-patches] [PATCH v10 0/4] box/cbox: implement cfunc Lua module

[Tarantool-patches] [PATCH v10 0/4] box/cbox: implement cfunc Lua module

[Cyrill Gorcunov]

The cbox module provide a way to execute C stored procedures
on read only nodes without registering them in `_func` system space.

The series implements a bare minimum. Vlad, please take a look
once time permit. The issue with `call` simplification where
we plan to drop `tarantooL` will be addressed in another issue
which I already assigned to myself.

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

branch gorcunov/gh-4642-func-ro-10
issue https://github.com/tarantool/tarantool/issues/4642

Cyrill Gorcunov (4):
  box/func: factor out c function entry structure
  module_cache: move module handling into own subsystem
  box/cbox: implement cbox Lua module
  test: box/cfunc -- add simple module test

 src/box/CMakeLists.txt  |   2 +
 src/box/box.cc          |   2 +-
 src/box/func.c          | 476 +--------------------------------------
 src/box/func.h          |  41 +---
 src/box/func_def.h      |  14 --
 src/box/lua/cbox.c      | 443 ++++++++++++++++++++++++++++++++++++
 src/box/lua/cbox.h      |  24 ++
 src/box/lua/init.c      |   2 +
 src/box/module_cache.c  | 484 ++++++++++++++++++++++++++++++++++++++++
 src/box/module_cache.h  | 158 +++++++++++++
 test/box/CMakeLists.txt |   2 +
 test/box/cbox.result    | 148 ++++++++++++
 test/box/cbox.test.lua  |  55 +++++
 test/box/cfunc1.c       |  58 +++++
 test/box/cfunc2.c       | 132 +++++++++++
 test/box/suite.ini      |   2 +-
 16 files changed, 1520 insertions(+), 523 deletions(-)
 create mode 100644 src/box/lua/cbox.c
 create mode 100644 src/box/lua/cbox.h
 create mode 100644 src/box/module_cache.c
 create mode 100644 src/box/module_cache.h
 create mode 100644 test/box/cbox.result
 create mode 100644 test/box/cbox.test.lua
 create mode 100644 test/box/cfunc1.c
 create mode 100644 test/box/cfunc2.c


base-commit: bc6caac60d5ef070460374149cb7abe2b0db3cde
-- 
2.26.2

[Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure

[Cyrill Gorcunov]

Currently func_c structure is a wrapper over struct func
which in turn handles function definition, credentials and etc,
mostly to handle internal "_func" space.

Such tight bound doesn't allow to reuse func_c in any
other context. But we're going to support C function
execution in read-only instances and for this we better
reuse already existing structures as much as possible
instead of breeding new ones.

Thus lets extract module symbols into module_sym structure,
this allows us to reuse module path cache in other code.

While extracting the structure rename "func" member to
"addr" since this exactly what the member represents:
an address of symbol in memory.

Same time to be completely independent of "_func" specific lets
carry symbolic name inplace (ie member "name" is kind of redundant
when module_sym is used for "_func" handling where we can retrieve
the name via function definition but such definition won't be
available for non-persistent C functions which we will support
in next patches).

The new structure allows us to load/unload general symbols via
module_sym_load() and module_sym_unload() helpers.

While been factoring out Vlad reported that we've an issue:
if a module has been loaded for a first time but doesn't have
symbol requested it will remain in memory keeping a reference
to a shared library for nothing.

Fixes #5475
Part-of #4642

Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/func.c | 172 +++++++++++++++++++++++++------------------------
 src/box/func.h |  42 ++++++++++++
 2 files changed, 131 insertions(+), 83 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 8087c953f..36bfca33f 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -59,19 +59,8 @@ struct func_name {
 struct func_c {
 	/** Function object base class. */
 	struct func base;
-	/**
-	 * Anchor for module membership.
-	 */
-	struct rlist item;
-	/**
-	 * For C functions, the body of the function.
-	 */
-	box_function_f func;
-	/**
-	 * Each stored function keeps a handle to the
-	 * dynamic library for the C callback.
-	 */
-	struct module *module;
+	/** C function module symbol. */
+	struct module_sym mod_sym;
 };
 
 /***
@@ -371,6 +360,70 @@ module_sym(struct module *module, const char *name)
 	return f;
 }
 
+int
+module_sym_load(struct module_sym *mod_sym)
+{
+	assert(mod_sym->addr == NULL);
+
+	struct func_name name;
+	func_split_name(mod_sym->name, &name);
+
+	/*
+	 * In case if module has been loaded already by
+	 * some previous call we can eliminate redundant
+	 * loading and take it from the cache.
+	 */
+	struct module *cached, *module;
+	cached = module_cache_find(name.package, name.package_end);
+	if (cached == NULL) {
+		module = module_load(name.package, name.package_end);
+		if (module == NULL)
+			return -1;
+		if (module_cache_put(module) != 0) {
+			module_delete(module);
+			return -1;
+		}
+	} else {
+		module = cached;
+	}
+
+	mod_sym->addr = module_sym(module, name.sym);
+	if (mod_sym->addr == NULL) {
+		if (cached == NULL) {
+			/*
+			 * In case if it was a first load we
+			 * should clean the cache immediately otherwise
+			 * the module may continue being referenced even
+			 * if noone gonna use it.
+			 */
+			module_cache_del(name.package, name.package_end);
+			module_delete(module);
+		}
+		return -1;
+	}
+	mod_sym->module = module;
+	rlist_add(&module->funcs, &mod_sym->item);
+	return 0;
+}
+
+void
+module_sym_unload(struct module_sym *mod_sym)
+{
+	if (mod_sym->module == NULL)
+		return;
+
+	rlist_del(&mod_sym->item);
+	if (rlist_empty(&mod_sym->module->funcs)) {
+		struct func_name name;
+		func_split_name(mod_sym->name, &name);
+		module_cache_del(name.package, name.package_end);
+	}
+	module_gc(mod_sym->module);
+
+	mod_sym->module = NULL;
+	mod_sym->addr = NULL;
+}
+
 int
 module_reload(const char *package, const char *package_end, struct module **module)
 {
@@ -385,15 +438,15 @@ module_reload(const char *package, const char *package_end, struct module **modu
 	if (new_module == NULL)
 		return -1;
 
-	struct func_c *func, *tmp_func;
-	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
+	struct module_sym *mod_sym, *tmp;
+	rlist_foreach_entry_safe(mod_sym, &old_module->funcs, item, tmp) {
 		struct func_name name;
-		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(new_module, name.sym);
-		if (func->func == NULL)
+		func_split_name(mod_sym->name, &name);
+		mod_sym->addr = module_sym(new_module, name.sym);
+		if (mod_sym->addr == NULL)
 			goto restore;
-		func->module = new_module;
-		rlist_move(&new_module->funcs, &func->item);
+		mod_sym->module = new_module;
+		rlist_move(&new_module->funcs, &mod_sym->item);
 	}
 	module_cache_del(package, package_end);
 	if (module_cache_put(new_module) != 0)
@@ -408,9 +461,9 @@ module_reload(const char *package, const char *package_end, struct module **modu
 	 */
 	do {
 		struct func_name name;
-		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(old_module, name.sym);
-		if (func->func == NULL) {
+		func_split_name(mod_sym->name, &name);
+		mod_sym->addr = module_sym(old_module, name.sym);
+		if (mod_sym->addr == NULL) {
 			/*
 			 * Something strange was happen, an early loaden
 			 * function was not found in an old dso.
@@ -418,10 +471,11 @@ module_reload(const char *package, const char *package_end, struct module **modu
 			panic("Can't restore module function, "
 			      "server state is inconsistent");
 		}
-		func->module = old_module;
-		rlist_move(&old_module->funcs, &func->item);
-	} while (func != rlist_first_entry(&old_module->funcs,
-					   struct func_c, item));
+		mod_sym->module = old_module;
+		rlist_move(&old_module->funcs, &mod_sym->item);
+	} while (mod_sym != rlist_first_entry(&old_module->funcs,
+					      struct module_sym,
+					      item));
 	assert(rlist_empty(&new_module->funcs));
 	module_delete(new_module);
 	return -1;
@@ -484,79 +538,31 @@ func_c_new(MAYBE_UNUSED struct func_def *def)
 		return NULL;
 	}
 	func->base.vtab = &func_c_vtab;
-	func->func = NULL;
-	func->module = NULL;
+	func->mod_sym.addr = NULL;
+	func->mod_sym.module = NULL;
+	func->mod_sym.name = def->name;
 	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);
+	module_sym_unload(&func->mod_sym);
 	TRASH(base);
 	free(func);
 }
 
-/**
- * Resolve func->func (find the respective DLL and fetch the
- * symbol from it).
- */
-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 *module = module_cache_find(name.package,
-						  name.package_end);
-	if (module == NULL) {
-		/* Try to find loaded module in the cache */
-		module = module_load(name.package, name.package_end);
-		if (module == NULL)
-			return -1;
-		if (module_cache_put(module)) {
-			module_delete(module);
-			return -1;
-		}
-	}
-
-	func->func = module_sym(module, name.sym);
-	if (func->func == NULL)
-		return -1;
-	func->module = module;
-	rlist_add(&module->funcs, &func->item);
-	return 0;
-}
-
 int
 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 (func_c_load(func) != 0)
+	if (func->mod_sym.addr == NULL) {
+		if (module_sym_load(&func->mod_sym) != 0)
 			return -1;
 	}
 
@@ -571,10 +577,10 @@ func_c_call(struct func *base, struct port *args, struct port *ret)
 	box_function_ctx_t ctx = { ret };
 
 	/* Module can be changed after function reload. */
-	struct module *module = func->module;
+	struct module *module = func->mod_sym.module;
 	assert(module != NULL);
 	++module->calls;
-	int rc = func->func(&ctx, data, data + data_sz);
+	int rc = func->mod_sym.addr(&ctx, data, data + data_sz);
 	--module->calls;
 	module_gc(module);
 	region_truncate(region, region_svp);
diff --git a/src/box/func.h b/src/box/func.h
index 581e468cb..9a7f17446 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -58,6 +58,28 @@ struct module {
 	char package[0];
 };
 
+/**
+ * Callable symbol bound to a module.
+ */
+struct module_sym {
+	/**
+	 * Anchor for module membership.
+	 */
+	struct rlist item;
+	/**
+	 * For C functions, address of the function.
+	 */
+	box_function_f addr;
+	/**
+	 * A module the symbol belongs to.
+	 */
+	struct module *module;
+	/**
+	 * Function name definition.
+	 */
+	char *name;
+};
+
 /** Virtual method table for func object. */
 struct func_vtab {
 	/** Call function with given arguments. */
@@ -108,6 +130,26 @@ func_delete(struct func *func);
 int
 func_call(struct func *func, struct port *args, struct port *ret);
 
+/**
+ * Resolve C entry (find the respective DLL and fetch the
+ * symbol from it).
+ *
+ * @param mod_sym module symbol pointer.
+ * @retval -1 on error.
+ * @retval 0 on success.
+ */
+int
+module_sym_load(struct module_sym *mod_sym);
+
+/**
+ * Unload module symbol and drop it from the package
+ * cache if there is no users left.
+ *
+ * @param mod_sym module symbol pointer.
+ */
+void
+module_sym_unload(struct module_sym *mod_sym);
+
 /**
  * Reload dynamically loadable module.
  *
-- 
2.26.2

[Tarantool-patches] [PATCH v10 2/4] module_cache: move module handling into own subsystem

[Cyrill Gorcunov]

The module handling should not be bound to particular
function implementation (we will have two users: already
existing functions for "_func" space, and a new upcoming
one which will be serving cbox submodule in next patch).

For this sake all module related code is moved to
module_cache file where we do symbol resolving,
calling and tracking of symbol usage.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/CMakeLists.txt |   1 +
 src/box/box.cc         |   2 +-
 src/box/func.c         | 470 +--------------------------------------
 src/box/func.h         |  83 +------
 src/box/func_def.h     |  14 --
 src/box/module_cache.c | 484 +++++++++++++++++++++++++++++++++++++++++
 src/box/module_cache.h | 158 ++++++++++++++
 7 files changed, 648 insertions(+), 564 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 df243ac33..6b0ba1f58 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -191,6 +191,7 @@ add_library(box STATIC
     sql_stmt_cache.c
     wal.c
     call.c
+    module_cache.c
     merger.c
     ibuf.c
     ${sql_sources}
diff --git a/src/box/box.cc b/src/box/box.cc
index 18568df3b..4114979f6 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -74,7 +74,7 @@
 #include "sql.h"
 #include "systemd.h"
 #include "call.h"
-#include "func.h"
+#include "module_cache.h"
 #include "sequence.h"
 #include "sql_stmt_cache.h"
 #include "msgpack.h"
diff --git a/src/box/func.c b/src/box/func.c
index 36bfca33f..4e7025fdb 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -31,30 +31,12 @@
 #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.
- */
-struct func_name {
-	/** Null-terminated symbol name, e.g. "func" for "mod.submod.func" */
-	const char *sym;
-	/** Package name, e.g. "mod.submod" for "mod.submod.func" */
-	const char *package;
-	/** A pointer to the last character in ->package + 1 */
-	const char *package_end;
-};
 
 struct func_c {
 	/** Function object base class. */
@@ -63,424 +45,6 @@ struct func_c {
 	struct module_sym mod_sym;
 };
 
-/***
- * Split function name to symbol and package names.
- * For example, str = foo.bar.baz => sym = baz, package = foo.bar
- * @param str function name, e.g. "module.submodule.function".
- * @param[out] name parsed symbol and package names.
- */
-static void
-func_split_name(const char *str, struct func_name *name)
-{
-	name->package = str;
-	name->package_end = strrchr(str, '.');
-	if (name->package_end != NULL) {
-		/* module.submodule.function => module.submodule, function */
-		name->sym = name->package_end + 1; /* skip '.' */
-	} else {
-		/* package == function => function, function */
-		name->sym = name->package;
-		name->package_end = str + strlen(str);
-	}
-}
-
-/**
- * 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;
-}
-
-static struct mh_strnptr_t *modules = NULL;
-
-static void
-module_gc(struct module *module);
-
-int
-module_init(void)
-{
-	modules = mh_strnptr_new();
-	if (modules == NULL) {
-		diag_set(OutOfMemory, sizeof(*modules), "malloc",
-			  "modules hash table");
-		return -1;
-	}
-	return 0;
-}
-
-void
-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);
-	}
-	mh_strnptr_delete(modules);
-}
-
-/**
- * Look up a module in the modules cache.
- */
-static struct module *
-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;
-}
-
-/**
- * Save module to the module cache.
- */
-static inline int
-module_cache_put(struct module *module)
-{
-	size_t package_len = strlen(module->package);
-	uint32_t name_hash = mh_strn_hash(module->package, package_len);
-	const struct mh_strnptr_node_t strnode = {
-		module->package, package_len, name_hash, module};
-
-	if (mh_strnptr_put(modules, &strnode, NULL, NULL) == mh_end(modules)) {
-		diag_set(OutOfMemory, sizeof(strnode), "malloc", "modules");
-		return -1;
-	}
-	return 0;
-}
-
-/**
- * Delete a module from the module cache
- */
-static void
-module_cache_del(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;
-	mh_strnptr_del(modules, i, NULL);
-}
-
-/*
- * 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.
- */
-static struct module *
-module_load(const char *package, const char *package_end)
-{
-	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;
-	}
-
-	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;
-	}
-
-	struct stat st;
-	if (stat(path, &st) < 0) {
-		diag_set(SystemError, "failed to stat() module %s", path);
-		goto error;
-	}
-
-	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;
-	}
-
-	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;
-	}
-
-	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;
-	}
-	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
-	if (e != NULL)
-		++e->iparam;
-	return module;
-error:
-	free(module);
-	return NULL;
-}
-
-static void
-module_delete(struct module *module)
-{
-	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
-	if (e != NULL)
-		--e->iparam;
-	dlclose(module->handle);
-	TRASH(module);
-	free(module);
-}
-
-/*
- * Check if a dso is unused and can be closed.
- */
-static void
-module_gc(struct module *module)
-{
-	if (rlist_empty(&module->funcs) && module->calls == 0)
-		module_delete(module);
-}
-
-/*
- * Import a function from the module.
- */
-static box_function_f
-module_sym(struct module *module, const char *name)
-{
-	box_function_f f = (box_function_f)dlsym(module->handle, name);
-	if (f == NULL) {
-		diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
-		return NULL;
-	}
-	return f;
-}
-
-int
-module_sym_load(struct module_sym *mod_sym)
-{
-	assert(mod_sym->addr == NULL);
-
-	struct func_name name;
-	func_split_name(mod_sym->name, &name);
-
-	/*
-	 * In case if module has been loaded already by
-	 * some previous call we can eliminate redundant
-	 * loading and take it from the cache.
-	 */
-	struct module *cached, *module;
-	cached = module_cache_find(name.package, name.package_end);
-	if (cached == NULL) {
-		module = module_load(name.package, name.package_end);
-		if (module == NULL)
-			return -1;
-		if (module_cache_put(module) != 0) {
-			module_delete(module);
-			return -1;
-		}
-	} else {
-		module = cached;
-	}
-
-	mod_sym->addr = module_sym(module, name.sym);
-	if (mod_sym->addr == NULL) {
-		if (cached == NULL) {
-			/*
-			 * In case if it was a first load we
-			 * should clean the cache immediately otherwise
-			 * the module may continue being referenced even
-			 * if noone gonna use it.
-			 */
-			module_cache_del(name.package, name.package_end);
-			module_delete(module);
-		}
-		return -1;
-	}
-	mod_sym->module = module;
-	rlist_add(&module->funcs, &mod_sym->item);
-	return 0;
-}
-
-void
-module_sym_unload(struct module_sym *mod_sym)
-{
-	if (mod_sym->module == NULL)
-		return;
-
-	rlist_del(&mod_sym->item);
-	if (rlist_empty(&mod_sym->module->funcs)) {
-		struct func_name name;
-		func_split_name(mod_sym->name, &name);
-		module_cache_del(name.package, name.package_end);
-	}
-	module_gc(mod_sym->module);
-
-	mod_sym->module = NULL;
-	mod_sym->addr = NULL;
-}
-
-int
-module_reload(const char *package, const char *package_end, struct module **module)
-{
-	struct module *old_module = module_cache_find(package, package_end);
-	if (old_module == NULL) {
-		/* Module wasn't loaded - do nothing. */
-		*module = NULL;
-		return 0;
-	}
-
-	struct module *new_module = module_load(package, package_end);
-	if (new_module == NULL)
-		return -1;
-
-	struct module_sym *mod_sym, *tmp;
-	rlist_foreach_entry_safe(mod_sym, &old_module->funcs, item, tmp) {
-		struct func_name name;
-		func_split_name(mod_sym->name, &name);
-		mod_sym->addr = module_sym(new_module, name.sym);
-		if (mod_sym->addr == NULL)
-			goto restore;
-		mod_sym->module = new_module;
-		rlist_move(&new_module->funcs, &mod_sym->item);
-	}
-	module_cache_del(package, package_end);
-	if (module_cache_put(new_module) != 0)
-		goto restore;
-	module_gc(old_module);
-	*module = new_module;
-	return 0;
-restore:
-	/*
-	 * Some old-dso func can't be load from new module, restore old
-	 * functions.
-	 */
-	do {
-		struct func_name name;
-		func_split_name(mod_sym->name, &name);
-		mod_sym->addr = module_sym(old_module, name.sym);
-		if (mod_sym->addr == NULL) {
-			/*
-			 * Something strange was happen, an early loaden
-			 * function was not found in an old dso.
-			 */
-			panic("Can't restore module function, "
-			      "server state is inconsistent");
-		}
-		mod_sym->module = old_module;
-		rlist_move(&old_module->funcs, &mod_sym->item);
-	} while (mod_sym != rlist_first_entry(&old_module->funcs,
-					      struct module_sym,
-					      item));
-	assert(rlist_empty(&new_module->funcs));
-	module_delete(new_module);
-	return -1;
-}
-
 static struct func *
 func_c_new(struct func_def *def);
 
@@ -560,39 +124,9 @@ 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->mod_sym.addr == NULL) {
-		if (module_sym_load(&func->mod_sym) != 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->mod_sym.module;
-	assert(module != NULL);
-	++module->calls;
-	int rc = func->mod_sym.addr(&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;
+	struct func_c *func = (struct func_c *)base;
+	return module_sym_call(&func->mod_sym, args, ret);
 }
 
 static struct func_vtab func_c_vtab = {
diff --git a/src/box/func.h b/src/box/func.h
index 9a7f17446..11a466b28 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -34,7 +34,8 @@
 #include <stddef.h>
 #include <stdint.h>
 #include <stdbool.h>
-#include "small/rlist.h"
+
+#include "module_cache.h"
 #include "func_def.h"
 #include "user_def.h"
 
@@ -44,42 +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];
-};
-
-/**
- * Callable symbol bound to a module.
- */
-struct module_sym {
-	/**
-	 * Anchor for module membership.
-	 */
-	struct rlist item;
-	/**
-	 * For C functions, address of the function.
-	 */
-	box_function_f addr;
-	/**
-	 * A module the symbol belongs to.
-	 */
-	struct module *module;
-	/**
-	 * Function name definition.
-	 */
-	char *name;
-};
-
 /** Virtual method table for func object. */
 struct func_vtab {
 	/** Call function with given arguments. */
@@ -106,18 +71,6 @@ struct func {
 	struct access access[BOX_USER_MAX];
 };
 
-/**
- * Initialize modules subsystem.
- */
-int
-module_init(void);
-
-/**
- * Cleanup modules subsystem.
- */
-void
-module_free(void);
-
 struct func *
 func_new(struct func_def *def);
 
@@ -130,38 +83,6 @@ func_delete(struct func *func);
 int
 func_call(struct func *func, struct port *args, struct port *ret);
 
-/**
- * Resolve C entry (find the respective DLL and fetch the
- * symbol from it).
- *
- * @param mod_sym module symbol pointer.
- * @retval -1 on error.
- * @retval 0 on success.
- */
-int
-module_sym_load(struct module_sym *mod_sym);
-
-/**
- * Unload module symbol and drop it from the package
- * cache if there is no users left.
- *
- * @param mod_sym module symbol pointer.
- */
-void
-module_sym_unload(struct module_sym *mod_sym);
-
-/**
- * Reload dynamically loadable module.
- *
- * @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);
-
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
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/src/box/module_cache.c b/src/box/module_cache.c
new file mode 100644
index 000000000..9107bec54
--- /dev/null
+++ b/src/box/module_cache.c
@@ -0,0 +1,484 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#include <dlfcn.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "assoc.h"
+#include "diag.h"
+#include "error.h"
+#include "errinj.h"
+#include "fiber.h"
+#include "port.h"
+
+#include "box/error.h"
+#include "lua/utils.h"
+#include "libeio/eio.h"
+
+#include "module_cache.h"
+
+/** Modules name to descriptor hash. */
+static struct mh_strnptr_t *mod_hash = NULL;
+
+/***
+ * Parse function name into a name descriptor.
+ *
+ * For example, str = foo.bar.baz => sym = baz, package = foo.bar
+ *
+ * @param str function name, e.g. "module.submodule.function".
+ * @param[out] d parsed symbol and a package name.
+ */
+static void
+parse_func_name(const char *str, struct func_name_desc *d)
+{
+	d->package = str;
+	d->package_end = strrchr(str, '.');
+	if (d->package_end != NULL) {
+		/* module.submodule.function => module.submodule, function */
+		d->sym = d->package_end + 1; /* skip '.' */
+	} else {
+		/* package == function => function, function */
+		d->sym = d->package;
+		d->package_end = str + strlen(str);
+	}
+}
+
+/**
+ * Look up a module in the modules cache.
+ */
+static struct module *
+module_cache_find(const char *name, const char *name_end)
+{
+	mh_int_t e = mh_strnptr_find_inp(mod_hash, name, name_end - name);
+	if (e == mh_end(mod_hash))
+		return NULL;
+	return mh_strnptr_node(mod_hash, e)->val;
+}
+
+/**
+ * Save a module to the modules cache.
+ */
+static int
+module_cache_add(struct module *module)
+{
+	size_t package_len = strlen(module->package);
+	const struct mh_strnptr_node_t nd = {
+		.str	= module->package,
+		.len	= package_len,
+		.hash	= mh_strn_hash(module->package, package_len),
+		.val	= module,
+	};
+
+	if (mh_strnptr_put(mod_hash, &nd, NULL, NULL) == mh_end(mod_hash)) {
+		diag_set(OutOfMemory, sizeof(nd), "malloc",
+			 "module cache node");
+		return -1;
+	}
+	return 0;
+}
+
+/**
+ * Delete a module from the modules cache.
+ */
+static void
+module_cache_del(const char *name, const char *name_end)
+{
+	mh_int_t e = mh_strnptr_find_inp(mod_hash, name, name_end - name);
+	if (e != mh_end(mod_hash))
+		mh_strnptr_del(mod_hash, e, NULL);
+}
+
+/**
+ * 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 = (void *)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 a path to a module using Lua's package.cpath.
+ */
+static int
+module_find(struct module_find_ctx *ctx)
+{
+	lua_State *L = tarantool_L;
+	int top = lua_gettop(L);
+	if (luaT_cpcall(L, luaT_module_find, ctx) != 0) {
+		diag_set(ClientError, ER_LOAD_MODULE,
+			 (int)(ctx->package_end - ctx->package),
+			 ctx->package, lua_tostring(L, -1));
+		lua_settop(L, top);
+		return -1;
+	}
+	assert(top == lua_gettop(L)); /* cpcall discard results */
+	return 0;
+}
+
+/**
+ * Load dynamic shared object, ie module library.
+ *
+ * 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.
+ */
+static struct module *
+module_load(const char *package, const char *package_end)
+{
+	char path[PATH_MAX];
+	struct module_find_ctx ctx = {
+		.package	= package,
+		.package_end	= package_end,
+		.path		= path,
+		.path_len	= sizeof(path),
+	};
+	if (module_find(&ctx) != 0)
+		return NULL;
+
+	int package_len = package_end - package;
+	struct module *module = malloc(sizeof(*module) + package_len + 1);
+	if (module == NULL) {
+		diag_set(OutOfMemory, sizeof(*module) + package_len + 1,
+			 "malloc", "struct module");
+		return NULL;
+	}
+	memcpy(module->package, package, package_len);
+	module->package[package_len] = 0;
+	rlist_create(&module->mod_sym_head);
+	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;
+	}
+
+	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;
+	}
+
+	struct stat st;
+	if (stat(path, &st) < 0) {
+		diag_set(SystemError, "failed to stat() module %s", path);
+		goto error;
+	}
+
+	int source_fd = open(path, O_RDONLY);
+	if (source_fd < 0) {
+		diag_set(SystemError, "failed to open module %s", path);
+		goto error;
+	}
+
+	int dest_fd = open(load_name, O_WRONLY | O_CREAT | O_TRUNC,
+			   st.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO));
+	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",
+			 path, load_name);
+		goto error;
+	}
+
+	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;
+	}
+
+	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
+	if (e != NULL)
+		++e->iparam;
+	return module;
+
+error:
+	free(module);
+	return NULL;
+}
+
+/**
+ * Delete a module.
+ */
+static void
+module_delete(struct module *module)
+{
+	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
+	if (e != NULL)
+		--e->iparam;
+	dlclose(module->handle);
+	TRASH(module);
+	free(module);
+}
+
+/**
+ * Check if a module is unused and delete it then.
+ */
+static void
+module_gc(struct module *module)
+{
+	if (rlist_empty(&module->mod_sym_head) && module->calls == 0)
+		module_delete(module);
+}
+
+/**
+ * Import a function from a module.
+ */
+static box_function_f
+module_sym(struct module *module, const char *name)
+{
+	box_function_f f = dlsym(module->handle, name);
+	if (f == NULL) {
+		diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
+		return NULL;
+	}
+	return f;
+}
+
+int
+module_sym_load(struct module_sym *mod_sym)
+{
+	assert(mod_sym->addr == NULL);
+
+	struct func_name_desc d;
+	parse_func_name(mod_sym->name, &d);
+
+	struct module *module = module_cache_find(d.package, d.package_end);
+	if (module == NULL) {
+		module = module_load(d.package, d.package_end);
+		if (module == NULL)
+			return -1;
+		if (module_cache_add(module)) {
+			module_delete(module);
+			return -1;
+		}
+	}
+
+	mod_sym->addr = module_sym(module, d.sym);
+	if (mod_sym->addr == NULL)
+		return -1;
+
+	mod_sym->module = module;
+	rlist_add(&module->mod_sym_head, &mod_sym->list);
+	return 0;
+}
+
+void
+module_sym_unload(struct module_sym *mod_sym)
+{
+	if (mod_sym->module == NULL)
+		return;
+
+	rlist_del(&mod_sym->list);
+	if (rlist_empty(&mod_sym->module->mod_sym_head)) {
+		struct func_name_desc d;
+		parse_func_name(mod_sym->name, &d);
+		module_cache_del(d.package, d.package_end);
+	}
+	module_gc(mod_sym->module);
+
+	mod_sym->module = NULL;
+	mod_sym->addr = NULL;
+}
+
+int
+module_sym_call(struct module_sym *mod_sym, struct port *args,
+		struct port *ret)
+{
+	if (mod_sym->addr == NULL) {
+		if (module_sym_load(mod_sym) != 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 = {
+		.port = ret,
+	};
+
+	/*
+	 * Module can be changed after function reload. Also
+	 * keep in mind that stored C procedure may yield inside.
+	 */
+	struct module *module = mod_sym->module;
+	assert(module != NULL);
+	++module->calls;
+	int rc = mod_sym->addr(&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;
+}
+
+int
+module_reload(const char *package, const char *package_end,
+	      struct module **module)
+{
+	struct module *old = module_cache_find(package, package_end);
+	if (old == NULL) {
+		/* Module wasn't loaded - do nothing. */
+		*module = NULL;
+		return 0;
+	}
+
+	struct module *new = module_load(package, package_end);
+	if (new == NULL)
+		return -1;
+
+	struct module_sym *mod_sym, *tmp;
+	rlist_foreach_entry_safe(mod_sym, &old->mod_sym_head, list, tmp) {
+		struct func_name_desc d;
+		parse_func_name(mod_sym->name, &d);
+
+		mod_sym->addr = module_sym(new, d.sym);
+		if (mod_sym->addr == NULL) {
+			say_error("module: reload %s, symbol %s not found",
+				  package, d.sym);
+			goto restore;
+		}
+
+		mod_sym->module = new;
+		rlist_move(&new->mod_sym_head, &mod_sym->list);
+	}
+
+	module_cache_del(package, package_end);
+	if (module_cache_add(new) != 0)
+		goto restore;
+
+	module_gc(old);
+	*module = new;
+	return 0;
+
+restore:
+	/*
+	 * Some old-dso func can't be load from new module,
+	 * restore old functions.
+	 */
+	do {
+		struct func_name_desc d;
+		parse_func_name(mod_sym->name, &d);
+		mod_sym->addr = module_sym(old, d.sym);
+		if (mod_sym->addr == NULL) {
+			/*
+			 * Something strange was happen, an early loaden
+			 * function was not found in an old dso.
+			 */
+			panic("Can't restore module function, "
+			      "server state is inconsistent");
+		}
+		mod_sym->module = old;
+		rlist_move(&old->mod_sym_head, &mod_sym->list);
+	} while (mod_sym != rlist_first_entry(&old->mod_sym_head,
+					      struct module_sym,
+					      list));
+	assert(rlist_empty(&new->mod_sym_head));
+	module_delete(new);
+	return -1;
+}
+
+int
+module_init(void)
+{
+	mod_hash = mh_strnptr_new();
+	if (mod_hash == NULL) {
+		diag_set(OutOfMemory, sizeof(*mod_hash),
+			 "malloc", "modules hash table");
+		return -1;
+	}
+	return 0;
+}
+
+void
+module_free(void)
+{
+	while (mh_size(mod_hash) > 0) {
+		mh_int_t i = mh_first(mod_hash);
+		struct module *m = mh_strnptr_node(mod_hash, i)->val;
+		module_gc(m);
+	}
+	mh_strnptr_delete(mod_hash);
+	mod_hash = NULL;
+}
diff --git a/src/box/module_cache.h b/src/box/module_cache.h
new file mode 100644
index 000000000..8b2510ec9
--- /dev/null
+++ b/src/box/module_cache.h
@@ -0,0 +1,158 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#pragma once
+
+#include <small/rlist.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_f)(box_function_ctx_t *ctx,
+			      const char *args,
+			      const char *args_end);
+
+/**
+ * Function name descriptor: a symbol and a package.
+ */
+struct func_name_desc {
+	/**
+	 * Null-terminated symbol name, e.g.
+	 * "func" for "mod.submod.func".
+	 */
+	const char *sym;
+	/**
+	 * Package name, e.g. "mod.submod" for
+	 * "mod.submod.func".
+	 */
+	const char *package;
+	/**
+	 * A pointer to the last character in ->package + 1.
+	 */
+	const char *package_end;
+};
+
+/**
+ * Dynamic shared module.
+ */
+struct module {
+	/**
+	 * Module dlhandle.
+	 */
+	void *handle;
+	/**
+	 * List of associated symbols (functions).
+	 */
+	struct rlist mod_sym_head;
+	/**
+	 * Count of active calls.
+	 */
+	size_t calls;
+	/**
+	 * Module's package name.
+	 */
+	char package[0];
+};
+
+/**
+ * Callable symbol bound to a module.
+ */
+struct module_sym {
+	/**
+	 * Anchor for module membership.
+	 */
+	struct rlist list;
+	/**
+	 * For C functions, address of the function.
+	 */
+	box_function_f addr;
+	/**
+	 * A module the symbol belongs to.
+	 */
+	struct module *module;
+	/**
+	 * Symbol (function) name definition.
+	 */
+	char *name;
+};
+
+/**
+ * Load a new module symbol.
+ *
+ * @param mod_sym symbol to load.
+ *
+ * @returns 0 on succse, -1 otherwise, diag is set.
+ */
+int
+module_sym_load(struct module_sym *mod_sym);
+
+/**
+ * Unload a module's symbol.
+ *
+ * @param mod_sym symbol to unload.
+ */
+void
+module_sym_unload(struct module_sym *mod_sym);
+
+/**
+ * Execute a module symbol (run a function).
+ *
+ * The function packs function arguments into a message pack
+ * and send it as a function argument. Function may return
+ * results via @a ret stream.
+ *
+ * @param mod_sym module symbol to run.
+ * @param args function arguments.
+ * @param[out] ret execution results.
+ *
+ * @returns 0 on success, -1 otherwise, diag is set.
+ */
+int
+module_sym_call(struct module_sym *mod_sym, struct port *args,
+		struct port *ret);
+
+/**
+ * Reload a module and all associated symbols.
+ *
+ * @param str function name, e.g. "module.submodule.function".
+ * @param[out] d parsed symbol and a package name.
+ *
+ * @return 0 on succes, -1 otherwise, diag is set.
+ */
+int
+module_reload(const char *package, const char *package_end,
+	      struct module **module);
+
+/**
+ * Initialize modules subsystem.
+ *
+ * @return 0 on succes, -1 otherwise, diag is set.
+ */
+int
+module_init(void);
+
+/**
+ * Free modules subsystem.
+ */
+void
+module_free(void);
+
+#if defined(__cplusplus)
+}
+#endif /* defined(__plusplus) */
-- 
2.26.2

[Tarantool-patches] [PATCH v10 3/4] box/cbox: implement cbox Lua module

[Cyrill Gorcunov]

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 "cbox" lua module.

Fixes #4692

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

@TarantoolBot document
Title: cbox module

Overview
========

`cbox` module provides a way to create, delete and execute
`C` procedures. Unlinke `box.schema.func` functionality this
the functions created with `cbox` 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
================

`cbox.func.load([dso.]name) -> obj | nil, err`
----------------------------------------------

Loads a new function with name `name` from module `dso.`.
The module name is optional and if not provided implies
to be the same as `name`.

The `load` call must be paired with `unload` and these
calls are accounted. Until coupled `unload` is called
the instance is present in memory. Any `load` calls
followed by another `load` with same name simply
increase a reference to the existing function.

Possible errors:
 - IllegalParams: function name is either not supplied
   or not a string.
 - IllegalParams: function name is too long.
 - IllegalParams: function references limit exceeded.
 - OutOfMemory: unable to allocate a function.

On success a new callable object is returned,
otherwise `nil, error` pair.

Example:

``` Lua
f, err = require('cbox').func.load('func')
if not f then
    print(err)
end
```

`cbox.func.unload([dso.]name) -> true | nil, err`
-------------------------------------------------

Unload a function with name `[dso.]name`. Since function
instances are accounted the function is not unloaded until
number of `unload` calls matched to the number of `load`
calls.

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

On success `true` is returned, otherwise `nil, error` pair.

Example:

``` Lua
ok, err = require('cbox').func.unload('func')
if not ok then
    print(err)
end
```

`cbox.module.reload(name) -> true | nil, err`
---------------------------------------------

Reloads a module with name `name` and all functions which
were associated for the module. Each module keeps a list of
functions belonging to the module and reload procedure cause
the bound function to update their addresses such that
function execution will be routed via a new library.

Modules are loaded with that named local binding which means
that reload of module symbols won't affect the functions which
are started execution already, only new calls will be rerouted.

Possible errors:
 - IllegalParams: module name is either not supplied
   or not a string.
 - ClientError: a module with the name provided does
   not exist.

On success `true` is returned, otherwise `nil,error` pair.

Example:

``` Lua
ok, err = require('cbox').module.reload('func')
if not ok then
    print(err)
end

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

Once function is loaded it can be executed by 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
cfunc_sum, err = require('cbox').func.load('cfunc.cfunc_sum')
if not cfunc_sum then
    print(err)
end
```

Once successfully loaded we can execute it. Note that unlike regular
Lua functions the context of `C` functions is different. They never
thrown an exception but return `true|nil, res` form where first value set
to `nil` in case of error condition and `res` carries an error description.

Lets call the `cfunc_sum` with wrong number of arguments

``` Lua
local ok, res = cfunc_sum()
if not ok then
    print(res)
end
```

We will 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 first returned value set to `true` and `res` represent
function execution result.

``` Lua
local ok, res = cfunc_sum(1, 2)
assert(ok);
print(res)
```

We will see the number `3` in output.
---
 src/box/CMakeLists.txt |   1 +
 src/box/lua/cbox.c     | 443 +++++++++++++++++++++++++++++++++++++++++
 src/box/lua/cbox.h     |  24 +++
 src/box/lua/init.c     |   2 +
 4 files changed, 470 insertions(+)
 create mode 100644 src/box/lua/cbox.c
 create mode 100644 src/box/lua/cbox.h

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 6b0ba1f58..586cd330f 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -199,6 +199,7 @@ add_library(box STATIC
     lua/init.c
     lua/call.c
     lua/cfg.cc
+    lua/cbox.c
     lua/console.c
     lua/serialize_lua.c
     lua/tuple.c
diff --git a/src/box/lua/cbox.c b/src/box/lua/cbox.c
new file mode 100644
index 000000000..708850d3e
--- /dev/null
+++ b/src/box/lua/cbox.c
@@ -0,0 +1,443 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#include <string.h>
+#include <lua.h>
+
+#include "assoc.h"
+#include "diag.h"
+
+#include "box/module_cache.h"
+#include "box/error.h"
+#include "box/port.h"
+
+#include "trivia/util.h"
+#include "lua/utils.h"
+
+/** A type to find a function handle from an object. */
+static const char *cbox_func_handle_uname = "cbox_func_handle";
+
+/**
+ * Function descriptor.
+ */
+struct cbox_func {
+	/**
+	 * Symbol descriptor for the function in
+	 * an associated module.
+	 */
+	struct module_sym mod_sym;
+	/**
+	 * Number of loads of the function.
+	 */
+	int64_t load_count;
+	/**
+	 * Function name.
+	 */
+	const char *name;
+	/**
+	 * Function name length.
+	 */
+	size_t name_len;
+	/**
+	 * Function name in-place keeper.
+	 */
+	char inplace[0];
+};
+
+/**
+ * Function name to cbox_func hash.
+ */
+static struct mh_strnptr_t *func_hash = NULL;
+
+/**
+ * Find function in cbox_func hash.
+ */
+struct cbox_func *
+cbox_func_find(const char *name, size_t name_len)
+{
+	mh_int_t e = mh_strnptr_find_inp(func_hash, name, name_len);
+	if (e == mh_end(func_hash))
+		return NULL;
+	return mh_strnptr_node(func_hash, e)->val;
+}
+
+/**
+ * Delete a function instance from the hash or decrease
+ * a reference if the function is still loaded.
+ */
+static void
+cbox_func_del(struct cbox_func *cf)
+{
+	assert(cf->load_count > 0);
+	if (cf->load_count-- != 1)
+		return;
+
+	mh_int_t e = mh_strnptr_find_inp(func_hash, cf->name, cf->name_len);
+	assert(e != mh_end(func_hash));
+	mh_strnptr_del(func_hash, e, NULL);
+}
+
+/**
+ * Add a function instance into the hash or increase
+ * a reference if the function is already exist.
+ */
+static bool
+cbox_func_add(struct cbox_func *cf)
+{
+	assert(cf->load_count >= 0);
+	if (cf->load_count++ != 0)
+		return true;
+
+	const struct mh_strnptr_node_t nd = {
+		.str	= cf->name,
+		.len	= cf->name_len,
+		.hash	= mh_strn_hash(cf->name, cf->name_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", "cbox_func node");
+		return false;
+	}
+	return true;
+}
+
+/**
+ * Allocate a new function instance.
+ */
+static struct cbox_func *
+cbox_func_new(const char *name, size_t name_len)
+{
+	const ssize_t cf_size = sizeof(struct cbox_func);
+	size_t size = cf_size + name_len + 1;
+	struct cbox_func *cf = malloc(size);
+	if (cf == NULL) {
+		diag_set(OutOfMemory, size, "malloc", "cf");
+		return NULL;
+	}
+
+	cf->mod_sym.addr	= NULL;
+	cf->mod_sym.module	= NULL;
+	cf->load_count		= 0;
+	cf->mod_sym.name	= cf->inplace;
+	cf->name		= cf->inplace;
+	cf->name_len		= name_len;
+
+	memcpy(cf->inplace, name, name_len);
+	cf->inplace[name_len] = '\0';
+
+	return cf;
+}
+
+/**
+ * Load a new function.
+ *
+ * This function takes a function name from the caller
+ * stack @a L and creates a new function object. If
+ * the function is already loaded we simply return
+ * a reference to existing one.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: function name is either not supplied
+ *   or not a string.
+ * - IllegalParams: function references limit exceeded.
+ * - OutOfMemory: unable to allocate a function.
+ *
+ * @returns function object on success or {nil,error} on error,
+ * the error is set to the diagnostics area.
+ */
+static int
+lcbox_func_load(struct lua_State *L)
+{
+	const char *method = "cbox.func.load";
+	struct cbox_func *cf = NULL;
+
+	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
+		const char *fmt =
+			"Expects %s(\'name\') but no name passed";
+		diag_set(IllegalParams, fmt, method);
+		return luaT_push_nil_and_error(L);
+	}
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+
+	cf = cbox_func_find(name, name_len);
+	if (cf == NULL) {
+		cf = cbox_func_new(name, name_len);
+		if (cf == NULL)
+			return luaT_push_nil_and_error(L);
+	}
+	if (!cbox_func_add(cf))
+		return luaT_push_nil_and_error(L);
+
+	*(struct cbox_func **)lua_newuserdata(L, sizeof(cf)) = cf;
+	luaL_getmetatable(L, cbox_func_handle_uname);
+	lua_setmetatable(L, -2);
+	return 1;
+}
+
+/**
+ * Unload a function.
+ *
+ * This function takes a function name from the caller
+ * stack @a L and unloads a function object.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: function name is either not supplied
+ *   or not a string.
+ * - IllegalParams: the function does not exist.
+ *
+ * @returns true on success or {nil,error} on error,
+ * the error is set to the diagnostics area.
+ */
+static int
+lcbox_func_unload(struct lua_State *L)
+{
+	const char *method = "cbox.func.unload";
+	const char *name = NULL;
+
+	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
+		const char *fmt =
+			"Expects %s(\'name\') but no name passed";
+		diag_set(IllegalParams, fmt, method);
+		return luaT_push_nil_and_error(L);
+	}
+
+	size_t name_len;
+	name = lua_tolstring(L, 1, &name_len);
+
+	struct cbox_func *cf = cbox_func_find(name, name_len);
+	if (cf == NULL) {
+		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
+		diag_set(IllegalParams, fmt, name);
+		return luaT_push_nil_and_error(L);
+	}
+
+	cbox_func_del(cf);
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/**
+ * Reload a module.
+ *
+ * This function takes a module name from the caller
+ * stack @a L and reloads all functions associated with
+ * the module.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: module name is either not supplied
+ *   or not a string.
+ * - IllegalParams: the function does not exist.
+ * - ClientError: a module with the name provided does
+ *   not exist.
+ *
+ * @returns true on success or {nil,error} on error,
+ * the error is set to the diagnostics area.
+ */
+static int
+lcbox_module_reload(struct lua_State *L)
+{
+	const char *method = "cbox.module.reload";
+	const char *fmt = "Expects %s(\'name\') but no name passed";
+
+	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
+		diag_set(IllegalParams, fmt, method);
+		return luaT_push_nil_and_error(L);
+	}
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+	if (name == NULL || name_len < 1) {
+		diag_set(IllegalParams, fmt, method);
+		return luaT_push_nil_and_error(L);
+	}
+
+	struct module *module = NULL;
+	if (module_reload(name, &name[name_len], &module) == 0) {
+		if (module != NULL) {
+			lua_pushboolean(L, true);
+			return 1;
+		}
+		diag_set(ClientError, ER_NO_SUCH_MODULE, name);
+	}
+	return luaT_push_nil_and_error(L);
+}
+
+/**
+ * Fetch cbox_func instance from an object.
+ */
+static struct cbox_func *
+cbox_fetch_func_handle(struct lua_State *L)
+{
+	struct cbox_func **cf_ptr = luaL_testudata(L, 1, cbox_func_handle_uname);
+	if (cf_ptr != NULL) {
+		assert(*cf_ptr != NULL);
+		return *cf_ptr;
+	}
+	return NULL;
+}
+
+/**
+ * Function handle representation for REPL (console).
+ */
+static int
+lcbox_handle_serialize(struct lua_State *L)
+{
+	struct cbox_func *cf = cbox_fetch_func_handle(L);
+	if (cf == NULL) {
+		diag_set(IllegalParams, "Bad params, use __serialize(obj)");
+		return luaT_error(L);
+	}
+
+	lua_createtable(L, 0, 0);
+	lua_pushstring(L, cf->name);
+	lua_setfield(L, -2, "name");
+
+	return 1;
+}
+
+/**
+ * Handle __index request for a function object.
+ */
+static int
+lcbox_handle_index(struct lua_State *L)
+{
+	/*
+	 * Instead of showing userdata pointer
+	 * lets provide a serialized value.
+	 */
+	lua_getmetatable(L, 1);
+	lua_pushvalue(L, 2);
+	lua_rawget(L, -2);
+	if (!lua_isnil(L, -1))
+		return 1;
+
+	struct cbox_func *cf = cbox_fetch_func_handle(L);
+	size_t len = 0;
+	const char *key = lua_tolstring(L, 2, &len);
+
+	if (lua_type(L, 2) != LUA_TSTRING || cf == NULL || key == NULL) {
+		diag_set(IllegalParams,
+			 "Bad params, use __index(obj, <string>)");
+		return luaT_error(L);
+	}
+
+	if (strcmp(key, "name") == 0) {
+		lua_pushstring(L, cf->name);
+		return 1;
+	}
+
+	return 0;
+}
+
+/**
+ * Free function handle if there is no active loads left.
+ */
+static int
+lcbox_handle_gc(struct lua_State *L)
+{
+	struct cbox_func *cf = cbox_fetch_func_handle(L);
+	if (cf != NULL && cf->load_count == 0) {
+		TRASH(cf);
+		free(cf);
+	}
+	return 0;
+}
+
+/**
+ * Call a function by its name from the Lua code.
+ */
+static int
+lcbox_handle_call(struct lua_State *L)
+{
+	struct cbox_func *cf = cbox_fetch_func_handle(L);
+	if (cf == NULL) {
+		diag_set(IllegalParams, "Function is corrupted");
+		return luaT_push_nil_and_error(L);
+	}
+
+	/*
+	 * FIXME: We should get rid of luaT_newthread but this
+	 * requires serious modifications. In particular
+	 * port_lua_do_dump uses tarantool_L reference and
+	 * coro_ref must be valid as well.
+	 */
+	lua_State *args_L = luaT_newthread(tarantool_L);
+	if (args_L == NULL)
+		return luaT_push_nil_and_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_sym_call(&cf->mod_sym, &args, &ret) != 0) {
+		port_destroy(&args);
+		return luaT_push_nil_and_error(L);
+	}
+
+	int top = lua_gettop(L);
+	lua_pushboolean(L, true);
+	port_dump_lua(&ret, L, true);
+	int cnt = lua_gettop(L) - top;
+
+	port_destroy(&ret);
+	port_destroy(&args);
+
+	return cnt;
+}
+
+/**
+ * Initialize cbox module.
+ */
+void
+box_lua_cbox_init(struct lua_State *L)
+{
+	func_hash = mh_strnptr_new();
+	if (func_hash == NULL) {
+		panic("Can't allocate cbox hash table");
+	}
+
+	static const struct luaL_Reg cbox_methods[] = {
+		{ NULL, NULL },
+	};
+	luaL_register_module(L, "cbox", cbox_methods);
+	lua_pop(L, 1);
+
+	static const struct luaL_Reg func_methods[] = {
+		{ "load",		lcbox_func_load		},
+		{ "unload",		lcbox_func_unload	},
+		{ NULL, NULL },
+	};
+	luaL_register_module(L, "cbox.func", func_methods);
+	lua_pop(L, 1);
+
+	static const struct luaL_Reg module_methods[] = {
+		{ "reload",		lcbox_module_reload	},
+		{ NULL, NULL },
+	};
+	luaL_register_module(L, "cbox.module", module_methods);
+	lua_pop(L, 1);
+
+	static const struct luaL_Reg func_handle_methods[] = {
+		{ "__index",		lcbox_handle_index	},
+		{ "__serialize",	lcbox_handle_serialize	},
+		{ "__call",		lcbox_handle_call	},
+		{ "__gc",		lcbox_handle_gc		},
+		{ NULL, NULL },
+	};
+	luaL_register_type(L, cbox_func_handle_uname, func_handle_methods);
+}
diff --git a/src/box/lua/cbox.h b/src/box/lua/cbox.h
new file mode 100644
index 000000000..17955fb44
--- /dev/null
+++ b/src/box/lua/cbox.h
@@ -0,0 +1,24 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#pragma once
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct lua_State;
+
+/**
+ * Initialize cbox Lua module.
+ *
+ * @param L Lua state where to register the cbox module.
+ */
+void
+box_lua_cbox_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 d0316ef86..b37aa284a 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -61,6 +61,7 @@
 #include "box/lua/cfg.h"
 #include "box/lua/xlog.h"
 #include "box/lua/console.h"
+#include "box/lua/cbox.h"
 #include "box/lua/tuple.h"
 #include "box/lua/execute.h"
 #include "box/lua/key_def.h"
@@ -466,6 +467,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_cbox_init(L);
 	box_lua_slab_init(L);
 	box_lua_index_init(L);
 	box_lua_space_init(L);
-- 
2.26.2

[Tarantool-patches] [PATCH v10 4/4] test: box/cfunc -- add simple module test

[Cyrill Gorcunov]

Note that the test is disabled for a while since we
need to update test-run first like

 | diff --git a/pretest_clean.lua b/pretest_clean.lua
 | index 9b5ac9d..b0280c4 100644
 | --- a/pretest_clean.lua
 | +++ b/pretest_clean.lua
 | @@ -272,6 +272,7 @@ local function clean()
 |          package = true,
 |          pickle = true,
 |          popen = true,
 | +        cbox = true,
 |          pwd = true,
 |          socket = true,
 |          strict = true,

ie need to enable cbox module.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/box/CMakeLists.txt |   2 +
 test/box/cbox.result    | 148 ++++++++++++++++++++++++++++++++++++++++
 test/box/cbox.test.lua  |  55 +++++++++++++++
 test/box/cfunc1.c       |  58 ++++++++++++++++
 test/box/cfunc2.c       | 132 +++++++++++++++++++++++++++++++++++
 test/box/suite.ini      |   2 +-
 6 files changed, 396 insertions(+), 1 deletion(-)
 create mode 100644 test/box/cbox.result
 create mode 100644 test/box/cbox.test.lua
 create mode 100644 test/box/cfunc1.c
 create mode 100644 test/box/cfunc2.c

diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 06bfbbe9d..2afbeadc3 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -3,3 +3,5 @@ build_module(function1 function1.c)
 build_module(reload1 reload1.c)
 build_module(reload2 reload2.c)
 build_module(tuple_bench tuple_bench.c)
+build_module(cfunc1 cfunc1.c)
+build_module(cfunc2 cfunc2.c)
diff --git a/test/box/cbox.result b/test/box/cbox.result
new file mode 100644
index 000000000..f6050cacf
--- /dev/null
+++ b/test/box/cbox.result
@@ -0,0 +1,148 @@
+-- test-run result file version 2
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+
+cbox = require('cbox')
+ | ---
+ | ...
+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
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc1_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+--
+-- They all are sitting in cfunc.so
+cfunc_nop = cbox.func.load('cfunc.cfunc_nop')
+ | ---
+ | ...
+cfunc_fetch_evens = cbox.func.load('cfunc.cfunc_fetch_evens')
+ | ---
+ | ...
+cfunc_multireturn = cbox.func.load('cfunc.cfunc_multireturn')
+ | ---
+ | ...
+cfunc_args = cbox.func.load('cfunc.cfunc_args')
+ | ---
+ | ...
+cfunc_sum = cbox.func.load('cfunc.cfunc_sum')
+ | ---
+ | ...
+
+--
+-- Make sure they all are callable
+cfunc_nop()
+ | ---
+ | - true
+ | ...
+cfunc_fetch_evens()
+ | ---
+ | - true
+ | ...
+cfunc_multireturn()
+ | ---
+ | - true
+ | ...
+cfunc_args()
+ | ---
+ | - true
+ | ...
+
+--
+-- Clean old function references and reload a new one.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc2_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+cbox.module.reload('cfunc')
+ | ---
+ | - true
+ | ...
+
+cfunc_nop()
+ | ---
+ | - true
+ | ...
+cfunc_multireturn()
+ | ---
+ | - true
+ | - [1]
+ | - [1]
+ | ...
+cfunc_fetch_evens({2,4,6})
+ | ---
+ | - true
+ | ...
+cfunc_fetch_evens({1,2,3})  -- error
+ | ---
+ | - null
+ | - invalid argument 1 != 2
+ | ...
+cfunc_args(1, "hello")
+ | ---
+ | - true
+ | - [1, 'hello']
+ | ...
+cfunc_sum(1) -- error
+ | ---
+ | - null
+ | - invalid argument count
+ | ...
+cfunc_sum(1,2)
+ | ---
+ | - true
+ | - 3
+ | ...
+
+--
+-- Clean it up
+cbox.func.unload('cfunc.cfunc_nop')
+ | ---
+ | - true
+ | ...
+cbox.func.unload('cfunc.cfunc_fetch_evens')
+ | ---
+ | - true
+ | ...
+cbox.func.unload('cfunc.cfunc_multireturn')
+ | ---
+ | - true
+ | ...
+cbox.func.unload('cfunc.cfunc_args')
+ | ---
+ | - true
+ | ...
+
+--
+-- Cleanup the generated symlink
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
diff --git a/test/box/cbox.test.lua b/test/box/cbox.test.lua
new file mode 100644
index 000000000..e879fa093
--- /dev/null
+++ b/test/box/cbox.test.lua
@@ -0,0 +1,55 @@
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+cbox = require('cbox')
+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
+
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc1_path, cfunc_path)
+
+--
+-- They all are sitting in cfunc.so
+cfunc_nop = cbox.func.load('cfunc.cfunc_nop')
+cfunc_fetch_evens = cbox.func.load('cfunc.cfunc_fetch_evens')
+cfunc_multireturn = cbox.func.load('cfunc.cfunc_multireturn')
+cfunc_args = cbox.func.load('cfunc.cfunc_args')
+cfunc_sum = cbox.func.load('cfunc.cfunc_sum')
+
+--
+-- Make sure they all are callable
+cfunc_nop()
+cfunc_fetch_evens()
+cfunc_multireturn()
+cfunc_args()
+
+--
+-- Clean old function references and reload a new one.
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc2_path, cfunc_path)
+
+cbox.module.reload('cfunc')
+
+cfunc_nop()
+cfunc_multireturn()
+cfunc_fetch_evens({2,4,6})
+cfunc_fetch_evens({1,2,3})  -- error
+cfunc_args(1, "hello")
+cfunc_sum(1) -- error
+cfunc_sum(1,2)
+
+--
+-- Clean it up
+cbox.func.unload('cfunc.cfunc_nop')
+cbox.func.unload('cfunc.cfunc_fetch_evens')
+cbox.func.unload('cfunc.cfunc_multireturn')
+cbox.func.unload('cfunc.cfunc_args')
+
+--
+-- Cleanup the generated symlink
+_ = pcall(fio.unlink(cfunc_path))
diff --git a/test/box/cfunc1.c b/test/box/cfunc1.c
new file mode 100644
index 000000000..8f6d3990c
--- /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_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..f6d826183
--- /dev/null
+++ b/test/box/cfunc2.c
@@ -0,0 +1,132 @@
+#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_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");
+	}
+
+	for (int i = 0; i < field_count; i++) {
+		int val = mp_decode_uint(&args);
+		int needed = 2 * (i+1);
+		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 rc;
+	return box_return_tuple(ctx, tuple_a);
+}
+
+/*
+ * 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/suite.ini b/test/box/suite.ini
index 6833228f2..6f2c3e77a 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 cbox.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.26.2

Re: [Tarantool-patches] [PATCH v10 0/4] box/cbox: implement cfunc Lua module

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

I am going to ask you to send responses to my comments in a
format of diff hunks fixing the comments. Usually you rush to
answer to my comments the same day they are sent with some
promises like "I will take a look", "I will see what can be done",
etc. But then you send a new version of the patchset without
responding to my comments with what you actually did regarding
each of them.

That makes me go through the comments on my own and look for the
commented places in the new patch to see the difference. It takes
significant time.

https://github.com/tarantool/tarantool/wiki/Code-review-procedure#during-the-review
Point 1.

Re: [Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure

[Vladislav Shpilevoy]

Thanks for the patch!

On 05.11.2020 16:18, Cyrill Gorcunov wrote:
> Currently func_c structure is a wrapper over struct func
> which in turn handles function definition, credentials and etc,
> mostly to handle internal "_func" space.
> 
> Such tight bound doesn't allow to reuse func_c in any
> other context. But we're going to support C function
> execution in read-only instances and for this we better
> reuse already existing structures as much as possible
> instead of breeding new ones.
> 
> Thus lets extract module symbols into module_sym structure,
> this allows us to reuse module path cache in other code.
> 
> While extracting the structure rename "func" member to
> "addr" since this exactly what the member represents:
> an address of symbol in memory.
> 
> Same time to be completely independent of "_func" specific lets
> carry symbolic name inplace (ie member "name" is kind of redundant
> when module_sym is used for "_func" handling where we can retrieve
> the name via function definition but such definition won't be
> available for non-persistent C functions which we will support
> in next patches).
> 
> The new structure allows us to load/unload general symbols via
> module_sym_load() and module_sym_unload() helpers.
> 
> While been factoring out Vlad reported that we've an issue:
> if a module has been loaded for a first time but doesn't have
> symbol requested it will remain in memory keeping a reference
> to a shared library for nothing.

Please, never squash refatoring and a bugfix into a single commit.
That makes the commit not atomic, and creates problems with
backporting such commit, which is clearly needed here.

Move the bugfix to a separate commit which can be cherry-picked to
the older versions.

https://github.com/tarantool/tarantool/wiki/Code-review-procedure#general-coding-points-to-check
Point 3, "Don't do a change if it is not related to the patch".
The issues 5475 and 4642 are not related.

It is not a cargo cult rule. I just really don't see the actual fix in
all this refactoring, because too much code is moved, and only some
tiny part of it is changed.

Re: [Tarantool-patches] [PATCH v10 3/4] box/cbox: implement cbox Lua module

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

On 05.11.2020 16:18, Cyrill Gorcunov wrote:
> 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 "cbox" lua module.
> 
> Fixes #4692

1. Wrong issue number.

> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> @TarantoolBot document
> Title: cbox module
> 
> Overview
> ========
> 
> `cbox` module provides a way to create, delete and execute
> `C` procedures. Unlinke `box.schema.func` functionality this
> the functions created with `cbox` help are not persistent and

2. "functionality this the functions created"? Couldn't parse.
"this" seems to be unnecessary.

> 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
> ================
> 
> `cbox.func.load([dso.]name) -> obj | nil, err`
> ----------------------------------------------
> 
> Loads a new function with name `name` from module `dso.`.
> The module name is optional and if not provided implies
> to be the same as `name`.
> 
> The `load` call must be paired with `unload` and these
> calls are accounted. Until coupled `unload` is called
> the instance is present in memory. Any `load` calls
> followed by another `load` with same name simply
> increase a reference to the existing function.

3. I thought about it more, and I am not sure unload should
be called only manually. Because Lua has exceptions. So the
user need to wrap everything into pcall between he called
load() and the code calling unload(). Otherwise an exception
may arise somewhere, and the user won't get to unload(). As
a result, he won't decrease the reference counter.

For example:

	local f = cbox.load('func')
	box.space.test:insert({f()})
	cbox.unload('func')

If the insert will throw, unload is never called. Moreover, if
the code is called repeatedly, and often fails on insertion,
then the reference counter will constantly grow, and number of
leaked unload() calls will grow as well.

Also it looks ugly that 'unload' is not bound anyhow to the function
objects. In Lua we usually provide a method to destroy cdata object
content in the object itself. Not in the module function-set.

For example, look at fio. When we create a file handle via

	fh = fio.open('file_name')

, we then call

	fh:close()

, not

	fio.close('file_name')

Also if :close() is not called, and 'fh' is garbage-collected, it is
closed automatically.

Your API for cbox looks exactly like the second case. While IMO
it should look like the first one. Otherwise the users will leak the
modules, 100%.

On the other hand, if we will unload the function when its last
Lua object is deleted, then we may get oscillation in case the
user frequently gets the function object using cbox.load(). So
maybe even if the last function is unloaded, we must keep the
module loaded, but with 0 reference counter. This, in turn, may
lead to the module never being deleted, if the user will not ever
use it or reload. I don't think I have a ready to use solution for that.

I have a raw idea how to resolve that. We can separate function load
and function get from each other. So cbox.load() only does the load
and returns nothing. And to actually get the function we add something
like cbox.get/fetch.

Or we can remove the load reference counting. So the users will be
forced not to call it too often and track the usages more carefully.
Or load the functions one time in a global namespace, and unload them
also only one time when the script is re-loaded, for example.

I advise to ask somebody who uses C stored functions in real code,
how is it usually handled. Solution team should have experience, I
hear they discuss C functions sometimes. Also Mons and Igor should
now good patterns for this, because it is about Lua as well.

Re: [Tarantool-patches] [PATCH v10 2/4] module_cache: move module handling into own subsystem

[Vladislav Shpilevoy]

Thanks for the patch!

See 7 comments below.

On 05.11.2020 16:18, Cyrill Gorcunov wrote:
> The module handling should not be bound to particular
> function implementation (we will have two users: already
> existing functions for "_func" space, and a new upcoming
> one which will be serving cbox submodule in next patch).
> 
> For this sake all module related code is moved to
> module_cache file where we do symbol resolving,
> calling and tracking of symbol usage.

1. I see it is not just moved. I by luck noticed, that the code
looks suspiciously different. And after a closed look I
found that you did some renames, and functional changes. As I
said in the other email - please, don't do that. It is extremely
hard to spot what you changed or renamed in this huge diff with
lots of moves. I will commment below some changes that I noticed.

But I ask you to split the renames and functional changes into
separate commits, so as they could be reviewer properly.

> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> new file mode 100644
> index 000000000..9107bec54
> --- /dev/null
> +++ b/src/box/module_cache.c
> @@ -0,0 +1,484 @@
> +
> +/**
> + * A cpcall() helper for module_find().
> + */
> +static int
> +luaT_module_find(lua_State *L)
> +{
> +	struct module_find_ctx *ctx = (void *)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 a path to a module using Lua's package.cpath.
> + */
> +static int
> +module_find(struct module_find_ctx *ctx)
> +{

2. In the original code the function took package name and
path, which was way more clear than a ctx. Which in the
old code was constructed inside of this function.

But you somewhy moved it to the caller code, to module_load(),
which creates the ctx just to call module_find(), and never
uses it again. So what was the point? I honestly do not
understand.

> +	lua_State *L = tarantool_L;
> +	int top = lua_gettop(L);
> +	if (luaT_cpcall(L, luaT_module_find, ctx) != 0) {
> +		diag_set(ClientError, ER_LOAD_MODULE,
> +			 (int)(ctx->package_end - ctx->package),
> +			 ctx->package, lua_tostring(L, -1));
> +		lua_settop(L, top);
> +		return -1;
> +	}
> +	assert(top == lua_gettop(L)); /* cpcall discard results */
> +	return 0;
> +}
> +
> +int
> +module_sym_load(struct module_sym *mod_sym)
> +{
> +	assert(mod_sym->addr == NULL);
> +
> +	struct func_name_desc d;
> +	parse_func_name(mod_sym->name, &d);
> +
> +	struct module *module = module_cache_find(d.package, d.package_end);
> +	if (module == NULL) {
> +		module = module_load(d.package, d.package_end);
> +		if (module == NULL)
> +			return -1;
> +		if (module_cache_add(module)) {
> +			module_delete(module);
> +			return -1;
> +		}
> +	}
> +
> +	mod_sym->addr = module_sym(module, d.sym);
> +	if (mod_sym->addr == NULL)
> +		return -1;

3. Is it correct, that you just removed the bugfix you did
in the previous commit? Here, if the module was loaded first
time, you again do not unload it.

> +
> +	mod_sym->module = module;
> +	rlist_add(&module->mod_sym_head, &mod_sym->list);
> +	return 0;
> +}
> diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> new file mode 100644
> index 000000000..8b2510ec9
> --- /dev/null
> +++ b/src/box/module_cache.h
> @@ -0,0 +1,158 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#pragma once
> +
> +#include <small/rlist.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_f)(box_function_ctx_t *ctx,
> +			      const char *args,
> +			      const char *args_end);
> +
> +/**
> + * Function name descriptor: a symbol and a package.
> + */
> +struct func_name_desc {

4. It is never used out of module_cache.c. Why do you need it
in the header?

> +	/**
> +	 * Null-terminated symbol name, e.g.
> +	 * "func" for "mod.submod.func".
> +	 */
> +	const char *sym;
> +	/**
> +	 * Package name, e.g. "mod.submod" for
> +	 * "mod.submod.func".
> +	 */
> +	const char *package;
> +	/**
> +	 * A pointer to the last character in ->package + 1.
> +	 */
> +	const char *package_end;
> +};
> +
> +/**
> + * Dynamic shared module.
> + */
> +struct module {
> +	/**
> +	 * Module dlhandle.
> +	 */
> +	void *handle;
> +	/**
> +	 * List of associated symbols (functions).
> +	 */
> +	struct rlist mod_sym_head;

5. I see you renamed it from 'funcs'. It was much better, tbh.
I ask you to keep the old name. Or rename it to 'syms', if you like
the 'sym' name so much. But when you named it 'head', you stated
that this is a head of the list, its first element. However it is
not. This rlist is the list itself. If you will call rlist_entry()
on this member, you won't get a valid module_sym object.

Also since you decided to change the comment, it would be better
to say what objects are stored here. Their type.

> +	/**
> +	 * Count of active calls.
> +	 */
> +	size_t calls;
> +	/**
> +	 * Module's package name.
> +	 */
> +	char package[0];
> +};
> +
> +/**
> + * Callable symbol bound to a module.
> + */
> +struct module_sym {
> +	/**
> +	 * Anchor for module membership.
> +	 */
> +	struct rlist list;

6. But it is not a list. It is just a link of the intrusive list.
It is not a self-sufficient list. Also since you changed the
comment anyway, I ask you to say what is the list this
anchor is stored in (module.funcs).

> +	/**
> +	 * For C functions, address of the function.
> +	 */
> +	box_function_f addr;
> +	/**
> +	 * A module the symbol belongs to.
> +	 */
> +	struct module *module;
> +	/**
> +	 * Symbol (function) name definition.
> +	 */
> +	char *name;
> +};
> +
> +/**
> + * Load a new module symbol.
> + *
> + * @param mod_sym symbol to load.
> + *
> + * @returns 0 on succse, -1 otherwise, diag is set.
> + */
> +int
> +module_sym_load(struct module_sym *mod_sym);
> +
> +/**
> + * Unload a module's symbol.
> + *
> + * @param mod_sym symbol to unload.
> + */
> +void
> +module_sym_unload(struct module_sym *mod_sym);
> +
> +/**
> + * Execute a module symbol (run a function).
> + *
> + * The function packs function arguments into a message pack
> + * and send it as a function argument. Function may return
> + * results via @a ret stream.
> + *
> + * @param mod_sym module symbol to run.
> + * @param args function arguments.
> + * @param[out] ret execution results.
> + *
> + * @returns 0 on success, -1 otherwise, diag is set.
> + */
> +int
> +module_sym_call(struct module_sym *mod_sym, struct port *args,
> +		struct port *ret);
> +
> +/**
> + * Reload a module and all associated symbols.
> + *
> + * @param str function name, e.g. "module.submodule.function".
> + * @param[out] d parsed symbol and a package name.

7. There are no such parameters as 'str' and 'd'.

> + *
> + * @return 0 on succes, -1 otherwise, diag is set.
> + */
> +int
> +module_reload(const char *package, const char *package_end,
> +	      struct module **module);
> +
> +/**
> + * Initialize modules subsystem.
> + *
> + * @return 0 on succes, -1 otherwise, diag is set.
> + */
> +int
> +module_init(void);
> +
> +/**
> + * Free modules subsystem.
> + */
> +void
> +module_free(void);
> +
> +#if defined(__cplusplus)
> +}
> +#endif /* defined(__plusplus) */
> 

Re: [Tarantool-patches] [PATCH v10 0/4] box/cbox: implement cfunc Lua module

[Cyrill Gorcunov]

On Thu, Nov 12, 2020 at 11:53:35PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> 
> I am going to ask you to send responses to my comments in a
> format of diff hunks fixing the comments. Usually you rush to
> answer to my comments the same day they are sent with some
> promises like "I will take a look", "I will see what can be done",
> etc. But then you send a new version of the patchset without
> responding to my comments with what you actually did regarding
> each of them.
> 
> That makes me go through the comments on my own and look for the
> commented places in the new patch to see the difference. It takes
> significant time.
> 
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#during-the-review
> Point 1.

OK. I did the whole resend simply thought you gonna be more comfortable with.

Re: [Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure

[Cyrill Gorcunov]

On Thu, Nov 12, 2020 at 11:53:40PM +0100, Vladislav Shpilevoy wrote:
> 
> Please, never squash refatoring and a bugfix into a single commit.
> That makes the commit not atomic, and creates problems with
> backporting such commit, which is clearly needed here.
> 
> Move the bugfix to a separate commit which can be cherry-picked to
> the older versions.

Done. Resent as a separate patch. Note though that we will need to
merge it first before I'll be able to continue since this change
is blocking for the series.

Re: [Tarantool-patches] [PATCH v10 2/4] module_cache: move module handling into own subsystem

[Cyrill Gorcunov]

On Thu, Nov 12, 2020 at 11:54:01PM +0100, Vladislav Shpilevoy wrote:
> 
> 1. I see it is not just moved. I by luck noticed, that the code
> looks suspiciously different. And after a closed look I
> found that you did some renames, and functional changes. As I
> said in the other email - please, don't do that. It is extremely
> hard to spot what you changed or renamed in this huge diff with
> lots of moves. I will commment below some changes that I noticed.
> 
> But I ask you to split the renames and functional changes into
> separate commits, so as they could be reviewer properly.

Sure.

> > +
> > +/**
> > + * Find a path to a module using Lua's package.cpath.
> > + */
> > +static int
> > +module_find(struct module_find_ctx *ctx)
> > +{
> 
> 2. In the original code the function took package name and
> path, which was way more clear than a ctx. Which in the
> old code was constructed inside of this function.
> 
> But you somewhy moved it to the caller code, to module_load(),
> which creates the ctx just to call module_find(), and never
> uses it again. So what was the point? I honestly do not
> understand.

I thought it gonna be a bit more readable. If you prefer old style
then sure I'll keep it.

> > +	mod_sym->addr = module_sym(module, d.sym);
> > +	if (mod_sym->addr == NULL)
> > +		return -1;
> 
> 3. Is it correct, that you just removed the bugfix you did
> in the previous commit? Here, if the module was loaded first
> time, you again do not unload it.

Sigh... you're right, the chunk is sneaked in. Thanks for spotting!

> > +
> > +	mod_sym->module = module;
> > +	rlist_add(&module->mod_sym_head, &mod_sym->list);
> > +	return 0;
> > +}
> > diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> > new file mode 100644
> > index 000000000..8b2510ec9
> > --- /dev/null
> > +++ b/src/box/module_cache.h
> > @@ -0,0 +1,158 @@
> > +/*
> > + * SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> > + */
> > +
> > +#pragma once
> > +
> > +#include <small/rlist.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_f)(box_function_ctx_t *ctx,
> > +			      const char *args,
> > +			      const char *args_end);
> > +
> > +/**
> > + * Function name descriptor: a symbol and a package.
> > + */
> > +struct func_name_desc {
> 
> 4. It is never used out of module_cache.c. Why do you need it
> in the header?

I think we gonna rework modules engine more a bit later, in particular
we will need to test symbols early on function definition and I've
a plan to reuse this structure. But for this patchset it looks out
of context, so I'll move it back to source file.

> > +
> > +/**
> > + * Dynamic shared module.
> > + */
> > +struct module {
> > +	/**
> > +	 * Module dlhandle.
> > +	 */
> > +	void *handle;
> > +	/**
> > +	 * List of associated symbols (functions).
> > +	 */
> > +	struct rlist mod_sym_head;
> 
> 5. I see you renamed it from 'funcs'. It was much better, tbh.
> I ask you to keep the old name. Or rename it to 'syms', if you like
> the 'sym' name so much. But when you named it 'head', you stated
> that this is a head of the list, its first element. However it is
> not. This rlist is the list itself. If you will call rlist_entry()
> on this member, you won't get a valid module_sym object.
> 
> Also since you decided to change the comment, it would be better
> to say what objects are stored here. Their type.

We already have "funcs".

src/box/schema.cc:static struct mh_i32ptr_t *funcs;
src/box/schema.cc:static struct mh_strnptr_t *funcs_by_name;

Having it here simply confusing. We should be able to greap easily
the context of variables, moreover the engine operates with addresses
of symbols not functions. The name exposes the meaning.

To the "head" name -- this *is* the head of the list. The entries
are linked into this list. And a head of a list is *not* its first
element, I'm not sure where you get this assumption from. And it
never been. The first element is

inline static struct rlist *
rlist_first(struct rlist *head)
{
	return head->next;
}

I could rename it to syms_list if you prefer. But please no "funcs".
This really makes grepping a way more worse :(

> > +/**
> > + * Callable symbol bound to a module.
> > + */
> > +struct module_sym {
> > +	/**
> > +	 * Anchor for module membership.
> > +	 */
> > +	struct rlist list;
> 
> 6. But it is not a list. It is just a link of the intrusive list.
> It is not a self-sufficient list. Also since you changed the
> comment anyway, I ask you to say what is the list this
> anchor is stored in (module.funcs).

I'll rename it to "item". Sounds ok?

You know I would prefer to not point in comment where exactly this
entries are linked into: the name of keeping structure may change,
moreover we might introduce some temporary storage later or something
else and such comment become misleading :( But since you insist I'll
add.

> > +/**
> > + * Reload a module and all associated symbols.
> > + *
> > + * @param str function name, e.g. "module.submodule.function".
> > + * @param[out] d parsed symbol and a package name.
> 
> 7. There are no such parameters as 'str' and 'd'.

Ouch, sneaked from merging :( Thanks!

Re: [Tarantool-patches] [PATCH v10 2/4] module_cache: move module handling into own subsystem

[Cyrill Gorcunov]

On Thu, Nov 12, 2020 at 11:54:01PM +0100, Vladislav Shpilevoy wrote:
> > +
> > +/**
> > + * Find a path to a module using Lua's package.cpath.
> > + */
> > +static int
> > +module_find(struct module_find_ctx *ctx)
> > +{
> 
> 2. In the original code the function took package name and
> path, which was way more clear than a ctx. Which in the
> old code was constructed inside of this function.
> 
> But you somewhy moved it to the caller code, to module_load(),
> which creates the ctx just to call module_find(), and never
> uses it again. So what was the point? I honestly do not
> understand.

Some more details on this function. Previously we have

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) {
	...
}

The caller provides *all* 4 parameters which are then packed into
module_find_ctx and sent to luaT_cpcall. The luaT_cpcall extracts
them and fills the @path inside.

Thus instead of passing module_find_ctx right from the point where
path[] and path_len are allocated (ie just one argument) we pass
4 instead, for absolutely no reason. So bloody ugly :( I simply
fixed it since I'm reworking this code.

If you prefer I can do such transition as a separate patch before
moving code into new place.

Re: [Tarantool-patches] [PATCH v10 3/4] box/cbox: implement cbox Lua module

[Cyrill Gorcunov]

On Thu, Nov 12, 2020 at 11:53:45PM +0100, Vladislav Shpilevoy wrote:
> > 
> > Still people would like to have a way to run such functions
> > even in ro mode. For this sake we implement "cbox" lua module.
> > 
> > Fixes #4692
> 
> 1. Wrong issue number.

Thanks, typo, fixed.

> 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > 
> > @TarantoolBot document
> > Title: cbox module
> > 
> > Overview
> > ========
> > 
> > `cbox` module provides a way to create, delete and execute
> > `C` procedures. Unlinke `box.schema.func` functionality this
> > the functions created with `cbox` help are not persistent and
> 
> 2. "functionality this the functions created"? Couldn't parse.
> "this" seems to be unnecessary.

Thanks! Changed to

    Overview
    ========
    
    `cbox` module provides a way to create, delete and execute
    `C` procedures. Unlinke `box.schema.func` the functions created
    with `cbox` 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.

> > `cbox.func.load([dso.]name) -> obj | nil, err`
> > ----------------------------------------------
> > 
> > Loads a new function with name `name` from module `dso.`.
> > The module name is optional and if not provided implies
> > to be the same as `name`.
> > 
> > The `load` call must be paired with `unload` and these
> > calls are accounted. Until coupled `unload` is called
> > the instance is present in memory. Any `load` calls
> > followed by another `load` with same name simply
> > increase a reference to the existing function.
> 
> 3. I thought about it more, and I am not sure unload should
> be called only manually. Because Lua has exceptions. So the
> user need to wrap everything into pcall between he called
> load() and the code calling unload(). Otherwise an exception
> may arise somewhere, and the user won't get to unload(). As
> a result, he won't decrease the reference counter.
> 
> For example:
> 
> 	local f = cbox.load('func')
> 	box.space.test:insert({f()})
> 	cbox.unload('func')
> 
> If the insert will throw, unload is never called. Moreover, if
> the code is called repeatedly, and often fails on insertion,
> then the reference counter will constantly grow, and number of
> leaked unload() calls will grow as well.
> 
> Also it looks ugly that 'unload' is not bound anyhow to the function
> objects. In Lua we usually provide a method to destroy cdata object
> content in the object itself. Not in the module function-set.
> 
> For example, look at fio. When we create a file handle via
> 
> 	fh = fio.open('file_name')
> 
> , we then call
> 
> 	fh:close()
> 
> , not
> 
> 	fio.close('file_name')
> 
> Also if :close() is not called, and 'fh' is garbage-collected, it is
> closed automatically.
> 
> Your API for cbox looks exactly like the second case. While IMO
> it should look like the first one. Otherwise the users will leak the
> modules, 100%.
> 
> On the other hand, if we will unload the function when its last
> Lua object is deleted, then we may get oscillation in case the
> user frequently gets the function object using cbox.load(). So
> maybe even if the last function is unloaded, we must keep the
> module loaded, but with 0 reference counter. This, in turn, may
> lead to the module never being deleted, if the user will not ever
> use it or reload. I don't think I have a ready to use solution for that.
> 
> I have a raw idea how to resolve that. We can separate function load
> and function get from each other. So cbox.load() only does the load
> and returns nothing. And to actually get the function we add something
> like cbox.get/fetch.

What you're describing is rather module interface. You know I though of
redesigning the modules more deeply: currently we dont even check if
module exist when create a new function. Thus if there is no such .so
file we won't hit an error until we run first :call method. And we've
been talkin to Mons a couple of weeks or month ago, he pointed that
such behaviour is really bad designed. And I completely agree.

I think we rather need something like

cbox.module.load(path)
	if module is already in cache we simply return EEXIST
cbox.module.unload(path)
	-EBUSY if someone is using it
	-ENOENT if not found
cbox.module.reload(path)
	-ENOENT if not found

The errors above is just an example, we wrap them with diag_set().

Until the module is explicitly unloaded the exported functions should
be alive. This is close to how kernel modules work: you can't unload
module if there are some active reference exist (for us "users" are
active functions created).

For functions we will have traditional interface

f = cbox.func.create()
f:delete()
f.__call()
f.__gc()

> Or we can remove the load reference counting. So the users will be
> forced not to call it too often and track the usages more carefully.
> Or load the functions one time in a global namespace, and unload them
> also only one time when the script is re-loaded, for example.
> 
> I advise to ask somebody who uses C stored functions in real code,
> how is it usually handled. Solution team should have experience, I
> hear they discuss C functions sometimes. Also Mons and Igor should
> now good patterns for this, because it is about Lua as well.

Will do, thanks!