Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v19 0/6] box: implement cmod Lua module
@ 2021-03-01 21:23 Cyrill Gorcunov via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 1/6] box/func: module_reload -- drop redundant argument Cyrill Gorcunov via Tarantool-patches
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-01 21:23 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

Please take a look once time permit.

v1-v3 are development ones and not sent.

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

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

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

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

Cyrill Gorcunov (6):
  box/func: module_reload -- drop redundant argument
  box/func: prepare for transition to modules subsystem
  box/module_cache: introduce modules subsystem
  box/func: switch to module_cache interface
  box/cmod: implement cmod Lua module
  test: box/cfunc -- add cmod test

 src/box/CMakeLists.txt  |   2 +
 src/box/box.cc          |   4 +-
 src/box/call.c          |  11 +-
 src/box/call.h          |   2 +-
 src/box/func.c          | 573 ++++++++++++++++---------------------
 src/box/func.h          |  26 +-
 src/box/func_def.h      |  14 -
 src/box/lua/call.c      |   2 +-
 src/box/lua/cmod.c      | 605 ++++++++++++++++++++++++++++++++++++++++
 src/box/lua/cmod.h      |  25 ++
 src/box/lua/init.c      |   2 +
 src/box/module_cache.c  | 476 +++++++++++++++++++++++++++++++
 src/box/module_cache.h  | 206 ++++++++++++++
 src/main.cc             |   3 +
 test/box/CMakeLists.txt |   4 +
 test/box/cfunc1.c       |  58 ++++
 test/box/cfunc2.c       | 137 +++++++++
 test/box/cfunc3.c       |  25 ++
 test/box/cfunc4.c       |  28 ++
 test/box/cmod.result    | 530 +++++++++++++++++++++++++++++++++++
 test/box/cmod.test.lua  | 203 ++++++++++++++
 test/box/suite.ini      |   2 +-
 22 files changed, 2556 insertions(+), 382 deletions(-)
 create mode 100644 src/box/lua/cmod.c
 create mode 100644 src/box/lua/cmod.h
 create mode 100644 src/box/module_cache.c
 create mode 100644 src/box/module_cache.h
 create mode 100644 test/box/cfunc1.c
 create mode 100644 test/box/cfunc2.c
 create mode 100644 test/box/cfunc3.c
 create mode 100644 test/box/cfunc4.c
 create mode 100644 test/box/cmod.result
 create mode 100644 test/box/cmod.test.lua


base-commit: 1583ec241c25c0efb706d6401f5d28e145189cc2
-- 
2.29.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Tarantool-patches] [PATCH v19 1/6] box/func: module_reload -- drop redundant argument
  2021-03-01 21:23 [Tarantool-patches] [PATCH v19 0/6] box: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
@ 2021-03-01 21:23 ` Cyrill Gorcunov via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 2/6] box/func: prepare for transition to modules subsystem Cyrill Gorcunov via Tarantool-patches
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-01 21:23 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

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

Part-of #4642

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

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Tarantool-patches] [PATCH v19 2/6] box/func: prepare for transition to modules subsystem
  2021-03-01 21:23 [Tarantool-patches] [PATCH v19 0/6] box: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 1/6] box/func: module_reload -- drop redundant argument Cyrill Gorcunov via Tarantool-patches
@ 2021-03-01 21:23 ` Cyrill Gorcunov via Tarantool-patches
  2021-03-08 22:44   ` Vladislav Shpilevoy via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 3/6] box/module_cache: introduce " Cyrill Gorcunov via Tarantool-patches
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-01 21:23 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

Since we will need to implement common module management
API the names used in box/func.c will interfere with "module"
prefix (which we gonna use in module API). Thus lets rename
structures and helpers to not intersect.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc     |   4 +-
 src/box/call.c     |   4 +-
 src/box/call.h     |   2 +-
 src/box/func.c     | 198 +++++++++++++++++++++++----------------------
 src/box/func.h     |   8 +-
 src/box/lua/call.c |   2 +-
 6 files changed, 111 insertions(+), 107 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 26cbe8aab..3fe72bcf8 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2512,7 +2512,7 @@ box_free(void)
 		session_free();
 		user_cache_free();
 		schema_free();
-		module_free();
+		box_module_free();
 		tuple_free();
 		port_free();
 #endif
@@ -2915,7 +2915,7 @@ box_init(void)
 	 */
 	session_init();
 
-	if (module_init() != 0)
+	if (box_module_init() != 0)
 		diag_raise();
 
 	if (tuple_init(lua_hash) != 0)
diff --git a/src/box/call.c b/src/box/call.c
index 7839e1f3e..4b1893f12 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -116,7 +116,7 @@ static const struct port_vtab port_msgpack_vtab = {
 };
 
 int
-box_module_reload(const char *name)
+box_process_module_reload(const char *name)
 {
 	struct credentials *credentials = effective_user();
 	if ((credentials->universal_access & (PRIV_X | PRIV_U)) !=
@@ -128,7 +128,7 @@ box_module_reload(const char *name)
 				 user->def->name);
 		return -1;
 	}
-	return module_reload(name, name + strlen(name));
+	return box_module_reload(name, name + strlen(name));
 }
 
 int
diff --git a/src/box/call.h b/src/box/call.h
index 45580bc9d..de7b42a00 100644
--- a/src/box/call.h
+++ b/src/box/call.h
@@ -46,7 +46,7 @@ struct call_request;
  * @retval 0 on success.
  */
 int
-box_module_reload(const char *name);
+box_process_module_reload(const char *name);
 
 int
 box_process_call(struct call_request *request, struct port *port);
diff --git a/src/box/func.c b/src/box/func.c
index 233696a4f..88903f40e 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -71,7 +71,7 @@ struct func_c {
 	 * Each stored function keeps a handle to the
 	 * dynamic library for the C callback.
 	 */
-	struct module *module;
+	struct box_module *bxmod;
 };
 
 /***
@@ -166,61 +166,65 @@ module_find(const char *package, const char *package_end, char *path,
 	return 0;
 }
 
-static struct mh_strnptr_t *modules = NULL;
+static struct mh_strnptr_t *box_module_hash = NULL;
 
 static void
-module_gc(struct module *module);
+box_module_gc(struct box_module *bxmod);
 
 int
-module_init(void)
+box_module_init(void)
 {
-	modules = mh_strnptr_new();
-	if (modules == NULL) {
-		diag_set(OutOfMemory, sizeof(*modules), "malloc",
-			  "modules hash table");
+	box_module_hash = mh_strnptr_new();
+	if (box_module_hash == NULL) {
+		diag_set(OutOfMemory, sizeof(*box_module_hash),
+			 "malloc", "box modules hash table");
 		return -1;
 	}
 	return 0;
 }
 
 void
-module_free(void)
+box_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;
+	while (mh_size(box_module_hash) > 0) {
+		struct box_module *bxmod;
+
+		mh_int_t i = mh_first(box_module_hash);
+		bxmod = mh_strnptr_node(box_module_hash, i)->val;
 		/* Can't delete modules if they have active calls */
-		module_gc(module);
+		box_module_gc(bxmod);
 	}
-	mh_strnptr_delete(modules);
+	mh_strnptr_delete(box_module_hash);
 }
 
 /**
  * Look up a module in the modules cache.
  */
-static struct module *
-module_cache_find(const char *name, const char *name_end)
+static struct box_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))
+	mh_int_t i = mh_strnptr_find_inp(box_module_hash, name,
+					 name_end - name);
+	if (i == mh_end(box_module_hash))
 		return NULL;
-	return (struct module *)mh_strnptr_node(modules, i)->val;
+	return mh_strnptr_node(box_module_hash, i)->val;
 }
 
 /**
  * Save module to the module cache.
  */
-static inline int
-module_cache_put(struct module *module)
+static int
+cache_put(struct box_module *bxmod)
 {
-	size_t package_len = strlen(module->package);
-	uint32_t name_hash = mh_strn_hash(module->package, package_len);
+	size_t package_len = strlen(bxmod->package);
+	uint32_t name_hash = mh_strn_hash(bxmod->package, package_len);
 	const struct mh_strnptr_node_t strnode = {
-		module->package, package_len, name_hash, module};
+		bxmod->package, package_len, name_hash, bxmod};
 
-	if (mh_strnptr_put(modules, &strnode, NULL, NULL) == mh_end(modules)) {
-		diag_set(OutOfMemory, sizeof(strnode), "malloc", "modules");
+	mh_int_t i = mh_strnptr_put(box_module_hash, &strnode, NULL, NULL);
+	if (i == mh_end(box_module_hash)) {
+		diag_set(OutOfMemory, sizeof(strnode),
+			 "malloc", "box_module_hash");
 		return -1;
 	}
 	return 0;
@@ -230,12 +234,13 @@ module_cache_put(struct module *module)
  * Delete a module from the module cache
  */
 static void
-module_cache_del(const char *name, const char *name_end)
+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))
+	mh_int_t i = mh_strnptr_find_inp(box_module_hash, name,
+					 name_end - name);
+	if (i == mh_end(box_module_hash))
 		return;
-	mh_strnptr_del(modules, i, NULL);
+	mh_strnptr_del(box_module_hash, i, NULL);
 }
 
 /*
@@ -244,25 +249,24 @@ module_cache_del(const char *name, const char *name_end)
  * 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)
+static struct box_module *
+box_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");
+	struct box_module *bxmod = malloc(sizeof(*bxmod) + package_len + 1);
+	if (bxmod == NULL) {
+		diag_set(OutOfMemory, sizeof(*bxmod) + package_len + 1,
+			 "malloc", "struct box_module");
 		return NULL;
 	}
-	memcpy(module->package, package, package_len);
-	module->package[package_len] = 0;
-	rlist_create(&module->funcs);
-	module->calls = 0;
+	memcpy(bxmod->package, package, package_len);
+	bxmod->package[package_len] = 0;
+	rlist_create(&bxmod->funcs);
+	bxmod->calls = 0;
 
 	const char *tmpdir = getenv("TMPDIR");
 	if (tmpdir == NULL)
@@ -317,12 +321,12 @@ module_load(const char *package, const char *package_end)
 		goto error;
 	}
 
-	module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
+	bxmod->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) {
+	if (bxmod->handle == NULL) {
 		diag_set(ClientError, ER_LOAD_MODULE, package_len,
 			  package, dlerror());
 		goto error;
@@ -330,40 +334,40 @@ module_load(const char *package, const char *package_end)
 	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
 	if (e != NULL)
 		++e->iparam;
-	return module;
+	return bxmod;
 error:
-	free(module);
+	free(bxmod);
 	return NULL;
 }
 
 static void
-module_delete(struct module *module)
+box_module_delete(struct box_module *bxmod)
 {
 	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
 	if (e != NULL)
 		--e->iparam;
-	dlclose(module->handle);
-	TRASH(module);
-	free(module);
+	dlclose(bxmod->handle);
+	TRASH(bxmod);
+	free(bxmod);
 }
 
 /*
  * Check if a dso is unused and can be closed.
  */
 static void
-module_gc(struct module *module)
+box_module_gc(struct box_module *bxmod)
 {
-	if (rlist_empty(&module->funcs) && module->calls == 0)
-		module_delete(module);
+	if (rlist_empty(&bxmod->funcs) && bxmod->calls == 0)
+		box_module_delete(bxmod);
 }
 
 /*
  * Import a function from the module.
  */
 static box_function_f
-module_sym(struct module *module, const char *name)
+box_module_sym(struct box_module *bxmod, const char *name)
 {
-	box_function_f f = (box_function_f)dlsym(module->handle, name);
+	box_function_f f = (box_function_f)dlsym(bxmod->handle, name);
 	if (f == NULL) {
 		diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
 		return NULL;
@@ -372,33 +376,33 @@ module_sym(struct module *module, const char *name)
 }
 
 int
-module_reload(const char *package, const char *package_end)
+box_module_reload(const char *package, const char *package_end)
 {
-	struct module *old_module = module_cache_find(package, package_end);
-	if (old_module == NULL) {
+	struct box_module *bxmod_old = cache_find(package, package_end);
+	if (bxmod_old == NULL) {
 		/* Module wasn't loaded - do nothing. */
 		diag_set(ClientError, ER_NO_SUCH_MODULE, package);
 		return -1;
 	}
 
-	struct module *new_module = module_load(package, package_end);
-	if (new_module == NULL)
+	struct box_module *bxmod = box_module_load(package, package_end);
+	if (bxmod == NULL)
 		return -1;
 
 	struct func_c *func, *tmp_func;
-	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
+	rlist_foreach_entry_safe(func, &bxmod_old->funcs, item, tmp_func) {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(new_module, name.sym);
+		func->func = box_module_sym(bxmod, name.sym);
 		if (func->func == NULL)
 			goto restore;
-		func->module = new_module;
-		rlist_move(&new_module->funcs, &func->item);
+		func->bxmod = bxmod;
+		rlist_move(&bxmod->funcs, &func->item);
 	}
-	module_cache_del(package, package_end);
-	if (module_cache_put(new_module) != 0)
+	cache_del(package, package_end);
+	if (cache_put(bxmod) != 0)
 		goto restore;
-	module_gc(old_module);
+	box_module_gc(bxmod_old);
 	return 0;
 restore:
 	/*
@@ -408,7 +412,7 @@ module_reload(const char *package, const char *package_end)
 	do {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(old_module, name.sym);
+		func->func = box_module_sym(bxmod_old, name.sym);
 		if (func->func == NULL) {
 			/*
 			 * Something strange was happen, an early loaden
@@ -417,12 +421,12 @@ module_reload(const char *package, const char *package_end)
 			panic("Can't restore module function, "
 			      "server state is inconsistent");
 		}
-		func->module = old_module;
-		rlist_move(&old_module->funcs, &func->item);
-	} while (func != rlist_first_entry(&old_module->funcs,
+		func->bxmod = bxmod_old;
+		rlist_move(&bxmod_old->funcs, &func->item);
+	} while (func != rlist_first_entry(&bxmod_old->funcs,
 					   struct func_c, item));
-	assert(rlist_empty(&new_module->funcs));
-	module_delete(new_module);
+	assert(rlist_empty(&bxmod->funcs));
+	box_module_delete(bxmod);
 	return -1;
 }
 
@@ -484,23 +488,23 @@ func_c_new(MAYBE_UNUSED struct func_def *def)
 	}
 	func->base.vtab = &func_c_vtab;
 	func->func = NULL;
-	func->module = NULL;
+	func->bxmod = NULL;
 	return &func->base;
 }
 
 static void
 func_c_unload(struct func_c *func)
 {
-	if (func->module) {
+	if (func->bxmod) {
 		rlist_del(&func->item);
-		if (rlist_empty(&func->module->funcs)) {
+		if (rlist_empty(&func->bxmod->funcs)) {
 			struct func_name name;
 			func_split_name(func->base.def->name, &name);
-			module_cache_del(name.package, name.package_end);
+			cache_del(name.package, name.package_end);
 		}
-		module_gc(func->module);
+		box_module_gc(func->bxmod);
 	}
-	func->module = NULL;
+	func->bxmod = NULL;
 	func->func = NULL;
 }
 
@@ -527,21 +531,21 @@ func_c_load(struct func_c *func)
 	struct func_name name;
 	func_split_name(func->base.def->name, &name);
 
-	struct module *cached, *module;
-	cached = module_cache_find(name.package, name.package_end);
+	struct box_module *cached, *bxmod;
+	cached = cache_find(name.package, name.package_end);
 	if (cached == NULL) {
-		module = module_load(name.package, name.package_end);
-		if (module == NULL)
+		bxmod = box_module_load(name.package, name.package_end);
+		if (bxmod == NULL)
 			return -1;
-		if (module_cache_put(module)) {
-			module_delete(module);
+		if (cache_put(bxmod) != 0) {
+			box_module_delete(bxmod);
 			return -1;
 		}
 	} else {
-		module = cached;
+		bxmod = cached;
 	}
 
-	func->func = module_sym(module, name.sym);
+	func->func = box_module_sym(bxmod, name.sym);
 	if (func->func == NULL) {
 		if (cached == NULL) {
 			/*
@@ -550,16 +554,16 @@ func_c_load(struct func_c *func)
 			 * the module continue being referenced even
 			 * if there will be no use of it.
 			 *
-			 * Note the module_sym set an error thus be
+			 * Note the box_module_sym set an error thus be
 			 * careful to not wipe it.
 			 */
-			module_cache_del(name.package, name.package_end);
-			module_delete(module);
+			cache_del(name.package, name.package_end);
+			box_module_delete(bxmod);
 		}
 		return -1;
 	}
-	func->module = module;
-	rlist_add(&module->funcs, &func->item);
+	func->bxmod = bxmod;
+	rlist_add(&bxmod->funcs, &func->item);
 	return 0;
 }
 
@@ -585,12 +589,12 @@ 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;
-	assert(module != NULL);
-	++module->calls;
+	struct box_module *bxmod = func->bxmod;
+	assert(bxmod != NULL);
+	++bxmod->calls;
 	int rc = func->func(&ctx, data, data + data_sz);
-	--module->calls;
-	module_gc(module);
+	--bxmod->calls;
+	box_module_gc(bxmod);
 	region_truncate(region, region_svp);
 	if (rc != 0) {
 		if (diag_last_error(&fiber()->diag) == NULL) {
diff --git a/src/box/func.h b/src/box/func.h
index 0a08fa465..66e4d09bc 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -47,7 +47,7 @@ struct func;
 /**
  * Dynamic shared module.
  */
-struct module {
+struct box_module {
 	/** Module dlhandle. */
 	void *handle;
 	/** List of imported functions. */
@@ -88,13 +88,13 @@ struct func {
  * Initialize modules subsystem.
  */
 int
-module_init(void);
+box_module_init(void);
 
 /**
  * Cleanup modules subsystem.
  */
 void
-module_free(void);
+box_module_free(void);
 
 struct func *
 func_new(struct func_def *def);
@@ -117,7 +117,7 @@ func_call(struct func *func, struct port *args, struct port *ret);
  * @retval 0 on success.
  */
 int
-module_reload(const char *package, const char *package_end);
+box_module_reload(const char *package, const char *package_end);
 
 #if defined(__cplusplus)
 } /* extern "C" */
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 0315e720c..1e4fce581 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -840,7 +840,7 @@ static int
 lbox_module_reload(lua_State *L)
 {
 	const char *name = luaL_checkstring(L, 1);
-	if (box_module_reload(name) != 0)
+	if (box_process_module_reload(name) != 0)
 		return luaT_error(L);
 	return 0;
 }
-- 
2.29.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Tarantool-patches] [PATCH v19 3/6] box/module_cache: introduce modules subsystem
  2021-03-01 21:23 [Tarantool-patches] [PATCH v19 0/6] box: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 1/6] box/func: module_reload -- drop redundant argument Cyrill Gorcunov via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 2/6] box/func: prepare for transition to modules subsystem Cyrill Gorcunov via Tarantool-patches
@ 2021-03-01 21:23 ` Cyrill Gorcunov via Tarantool-patches
  2021-03-08 22:45   ` Vladislav Shpilevoy via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 4/6] box/func: switch to module_cache interface Cyrill Gorcunov via Tarantool-patches
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-01 21:23 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

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

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

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/CMakeLists.txt |   1 +
 src/box/module_cache.c | 476 +++++++++++++++++++++++++++++++++++++++++
 src/box/module_cache.h | 206 ++++++++++++++++++
 src/main.cc            |   3 +
 4 files changed, 686 insertions(+)
 create mode 100644 src/box/module_cache.c
 create mode 100644 src/box/module_cache.h

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Tarantool-patches] [PATCH v19 4/6] box/func: switch to module_cache interface
  2021-03-01 21:23 [Tarantool-patches] [PATCH v19 0/6] box: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 3/6] box/module_cache: introduce " Cyrill Gorcunov via Tarantool-patches
@ 2021-03-01 21:23 ` Cyrill Gorcunov via Tarantool-patches
  2021-03-08 22:47   ` Vladislav Shpilevoy via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 5/6] box/cmod: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 6/6] test: box/cfunc -- add cmod test Cyrill Gorcunov via Tarantool-patches
  5 siblings, 1 reply; 12+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-01 21:23 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

This allows us to drop some low-level functions such as package
lookup, module loading and symbol resolving.

The "box.schema.func" engine operates as a second level cache
over module_cache engine. This is because of module reloading
procedure: "box.schema.func" allows explicit module reload
only, thus even if a module has been changed on disk it should
not be invalidated in second level cache. And we can't change
this due to backward compatibility rule.

Thus:

 - we introduce box_module structure which carries low-level
   module instance, and update lowlevel cache in passthrough
   way;
 - func_c structure (which represent C function) now uses
   low-level module_func structure;
 - cache routines now works with box_module itself;
 - function manipulations are moved to func_c_create, func_c_load
   func_c_unload helpers;
 - all variables with type func_c now named as func_c, plain func
   name is kept for general functions, otherwise it is hard to
   figure out which exactly instance you work with, just to be
   consistent in names because massive code rework is done
   anyway;
 - due to lazy resolving of function symbols keep func_c_load
   name for exactly function loading and symbol resolving is
   done in func_c_resolve helper;
 - make func_c_call static as it should be.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/func.c     | 492 ++++++++++++++++++---------------------------
 src/box/func.h     |  19 +-
 src/box/func_def.h |  14 --
 3 files changed, 200 insertions(+), 325 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 88903f40e..a8401c6ed 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -30,19 +30,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.
@@ -56,6 +49,18 @@ struct func_name {
 	const char *package_end;
 };
 
+/**
+ * Dynamic shared module.
+ */
+struct box_module {
+	/** Low level module instance. */
+	struct module *module;
+	/** List of imported functions. */
+	struct rlist funcs;
+	/** Number of active references. */
+	int64_t refs;
+};
+
 struct func_c {
 	/** Function object base class. */
 	struct func base;
@@ -64,16 +69,18 @@ struct func_c {
 	 */
 	struct rlist item;
 	/**
-	 * For C functions, the body of the function.
+	 * Back reference to a cached module.
 	 */
-	box_function_f func;
+	struct box_module *bxmod;
 	/**
-	 * Each stored function keeps a handle to the
-	 * dynamic library for the C callback.
+	 * C function to call.
 	 */
-	struct box_module *bxmod;
+	struct module_func mf;
 };
 
+/** Hash from module name to its box_module instance. */
+static struct mh_strnptr_t *box_module_hash = NULL;
+
 /***
  * Split function name to symbol and package names.
  * For example, str = foo.bar.baz => sym = baz, package = foo.bar
@@ -95,82 +102,7 @@ func_split_name(const char *str, struct func_name *name)
 	}
 }
 
-/**
- * Arguments for luaT_module_find used by lua_cpcall()
- */
-struct module_find_ctx {
-	const char *package;
-	const char *package_end;
-	char *path;
-	size_t path_len;
-};
-
-/**
- * A cpcall() helper for module_find()
- */
-static int
-luaT_module_find(lua_State *L)
-{
-	struct module_find_ctx *ctx = (struct module_find_ctx *)
-		lua_topointer(L, 1);
-
-	/*
-	 * Call package.searchpath(name, package.cpath) and use
-	 * the path to the function in dlopen().
-	 */
-	lua_getglobal(L, "package");
-
-	lua_getfield(L, -1, "search");
-
-	/* Argument of search: name */
-	lua_pushlstring(L, ctx->package, ctx->package_end - ctx->package);
-
-	lua_call(L, 1, 1);
-	if (lua_isnil(L, -1))
-		return luaL_error(L, "module not found");
-	/* Convert path to absolute */
-	char resolved[PATH_MAX];
-	if (realpath(lua_tostring(L, -1), resolved) == NULL) {
-		diag_set(SystemError, "realpath");
-		return luaT_error(L);
-	}
-
-	snprintf(ctx->path, ctx->path_len, "%s", resolved);
-	return 0;
-}
-
-/**
- * Find path to module using Lua's package.cpath
- * @param package package name
- * @param package_end a pointer to the last byte in @a package + 1
- * @param[out] path path to shared library
- * @param path_len size of @a path buffer
- * @retval 0 on success
- * @retval -1 on error, diag is set
- */
-static int
-module_find(const char *package, const char *package_end, char *path,
-	    size_t path_len)
-{
-	struct module_find_ctx ctx = { package, package_end, path, path_len };
-	lua_State *L = tarantool_L;
-	int top = lua_gettop(L);
-	if (luaT_cpcall(L, luaT_module_find, &ctx) != 0) {
-		int package_len = (int) (package_end - package);
-		diag_set(ClientError, ER_LOAD_MODULE, package_len, package,
-			 lua_tostring(L, -1));
-		lua_settop(L, top);
-		return -1;
-	}
-	assert(top == lua_gettop(L)); /* cpcall discard results */
-	return 0;
-}
-
-static struct mh_strnptr_t *box_module_hash = NULL;
-
-static void
-box_module_gc(struct box_module *bxmod);
-
+/** Initialize box module subsystem. */
 int
 box_module_init(void)
 {
@@ -183,20 +115,6 @@ box_module_init(void)
 	return 0;
 }
 
-void
-box_module_free(void)
-{
-	while (mh_size(box_module_hash) > 0) {
-		struct box_module *bxmod;
-
-		mh_int_t i = mh_first(box_module_hash);
-		bxmod = mh_strnptr_node(box_module_hash, i)->val;
-		/* Can't delete modules if they have active calls */
-		box_module_gc(bxmod);
-	}
-	mh_strnptr_delete(box_module_hash);
-}
-
 /**
  * Look up a module in the modules cache.
  */
@@ -216,10 +134,15 @@ cache_find(const char *name, const char *name_end)
 static int
 cache_put(struct box_module *bxmod)
 {
-	size_t package_len = strlen(bxmod->package);
-	uint32_t name_hash = mh_strn_hash(bxmod->package, package_len);
+	const char *package = bxmod->module->package;
+	size_t package_len = bxmod->module->package_len;
+
 	const struct mh_strnptr_node_t strnode = {
-		bxmod->package, package_len, name_hash, bxmod};
+		.str = package,
+		.len = package_len,
+		.hash = mh_strn_hash(package, package_len),
+		.val = bxmod
+	};
 
 	mh_int_t i = mh_strnptr_put(box_module_hash, &strnode, NULL, NULL);
 	if (i == mh_end(box_module_hash)) {
@@ -231,150 +154,175 @@ cache_put(struct box_module *bxmod)
 }
 
 /**
- * Delete a module from the module cache
+ * Update module in module cache.
  */
 static void
-cache_del(const char *name, const char *name_end)
+cache_update(struct box_module *bxmod)
 {
-	mh_int_t i = mh_strnptr_find_inp(box_module_hash, name,
-					 name_end - name);
-	if (i == mh_end(box_module_hash))
-		return;
-	mh_strnptr_del(box_module_hash, i, NULL);
+	const char *str = bxmod->module->package;
+	size_t len = bxmod->module->package_len;
+
+	mh_int_t e = mh_strnptr_find_inp(box_module_hash, str, len);
+	if (e == mh_end(box_module_hash))
+		panic("func: failed to update cache: %s", str);
+
+	mh_strnptr_node(box_module_hash, e)->str = str;
+	mh_strnptr_node(box_module_hash, e)->val = bxmod;
 }
 
-/*
- * Load a dso.
- * Create a new symlink based on temporary directory and try to
- * load via this symink to load a dso twice for cases of a function
- * reload.
+/**
+ * Delete a module from the module cache
  */
+static void
+cache_del(struct box_module *bxmod)
+{
+	const char *str = bxmod->module->package;
+	size_t len = bxmod->module->package_len;
+
+	mh_int_t e = mh_strnptr_find_inp(box_module_hash, str, len);
+	if (e != mh_end(box_module_hash)) {
+		struct box_module *v;
+		v = mh_strnptr_node(box_module_hash, e)->val;
+		if (v == bxmod)
+			mh_strnptr_del(box_module_hash, e, NULL);
+	}
+}
+
+/** Increment reference to a module. */
+static inline void
+box_module_ref(struct box_module *bxmod)
+{
+	assert(bxmod->refs >= 0);
+	++bxmod->refs;
+}
+
+/** Low-level module loader. */
 static struct box_module *
-box_module_load(const char *package, const char *package_end)
+box_module_ld(const char *package, size_t package_len,
+	      struct module *(ld)(const char *package,
+				  size_t package_len))
 {
-	char path[PATH_MAX];
-	if (module_find(package, package_end, path, sizeof(path)) != 0)
+	struct module *m = ld(package, package_len);
+	if (m == NULL)
 		return NULL;
 
-	int package_len = package_end - package;
-	struct box_module *bxmod = malloc(sizeof(*bxmod) + package_len + 1);
+	struct box_module *bxmod = malloc(sizeof(*bxmod));
 	if (bxmod == NULL) {
+		module_unload(m);
 		diag_set(OutOfMemory, sizeof(*bxmod) + package_len + 1,
 			 "malloc", "struct box_module");
 		return NULL;
 	}
-	memcpy(bxmod->package, package, package_len);
-	bxmod->package[package_len] = 0;
-	rlist_create(&bxmod->funcs);
-	bxmod->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;
-	}
+	bxmod->refs = 0;
+	bxmod->module = m;
+	rlist_create(&bxmod->funcs);
 
-	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;
-	}
+	box_module_ref(bxmod);
+	return bxmod;
+}
 
-	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;
-	}
+/** Load a new module. */
+static struct box_module *
+box_module_load(const char *package, size_t package_len)
+{
+	return box_module_ld(package, package_len,
+			     module_load);
+}
 
-	bxmod->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 (bxmod->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 bxmod;
-error:
-	free(bxmod);
-	return NULL;
+/** Load a new module with force cache invalidation. */
+static struct box_module *
+box_module_load_force(const char *package, size_t package_len)
+{
+	return box_module_ld(package, package_len,
+			     module_load_force);
 }
 
+/** Delete a module. */
 static void
 box_module_delete(struct box_module *bxmod)
 {
 	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
 	if (e != NULL)
 		--e->iparam;
-	dlclose(bxmod->handle);
+	module_unload(bxmod->module);
 	TRASH(bxmod);
 	free(bxmod);
 }
 
-/*
- * Check if a dso is unused and can be closed.
- */
-static void
-box_module_gc(struct box_module *bxmod)
+/** Decrement reference to a module and delete it if last one. */
+static inline void
+box_module_unref(struct box_module *bxmod)
 {
-	if (rlist_empty(&bxmod->funcs) && bxmod->calls == 0)
+	assert(bxmod->refs > 0);
+	if (--bxmod->refs == 0) {
+		cache_del(bxmod);
 		box_module_delete(bxmod);
+	}
 }
 
-/*
- * Import a function from the module.
- */
-static box_function_f
-box_module_sym(struct box_module *bxmod, const char *name)
+/** Free box modules subsystem. */
+void
+box_module_free(void)
 {
-	box_function_f f = (box_function_f)dlsym(bxmod->handle, name);
-	if (f == NULL) {
-		diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
-		return NULL;
+	while (mh_size(box_module_hash) > 0) {
+		struct box_module *bxmod;
+
+		mh_int_t i = mh_first(box_module_hash);
+		bxmod = mh_strnptr_node(box_module_hash, i)->val;
+		/*
+		 * Module won't be deleted if it has
+		 * active functions bound.
+		 */
+		box_module_unref(bxmod);
 	}
-	return f;
+	mh_strnptr_delete(box_module_hash);
+}
+
+static struct func_vtab func_c_vtab;
+
+/** Create new box function. */
+static inline void
+func_c_create(struct func_c *func_c)
+{
+	func_c->bxmod = NULL;
+	func_c->base.vtab = &func_c_vtab;
+	rlist_create(&func_c->item);
+	module_func_create(&func_c->mf);
+}
+
+/** Test if function is not resolved. */
+static inline bool
+is_func_c_emtpy(struct func_c *func_c)
+{
+	return is_module_func_empty(&func_c->mf);
 }
 
+/** Load a new function. */
+static inline int
+func_c_load(struct box_module *bxmod, const char *func_name,
+	      struct func_c *func_c)
+{
+	int rc = module_func_load(bxmod->module, func_name, &func_c->mf);
+	if (rc == 0) {
+		rlist_move(&bxmod->funcs, &func_c->item);
+		func_c->bxmod = bxmod;
+		box_module_ref(bxmod);
+	}
+	return rc;
+}
+
+/** Unload a function. */
+static inline void
+func_c_unload(struct func_c *func_c)
+{
+	module_func_unload(&func_c->mf);
+	rlist_del(&func_c->item);
+	box_module_unref(func_c->bxmod);
+	func_c_create(func_c);
+}
+
+/** Reload module in a force way. */
 int
 box_module_reload(const char *package, const char *package_end)
 {
@@ -385,25 +333,24 @@ box_module_reload(const char *package, const char *package_end)
 		return -1;
 	}
 
-	struct box_module *bxmod = box_module_load(package, package_end);
+	size_t len = package_end - package;
+	struct box_module *bxmod = box_module_load_force(package, len);
 	if (bxmod == NULL)
 		return -1;
 
-	struct func_c *func, *tmp_func;
-	rlist_foreach_entry_safe(func, &bxmod_old->funcs, item, tmp_func) {
+	struct func_c *func, *tmp;
+	rlist_foreach_entry_safe(func, &bxmod_old->funcs, item, tmp) {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
-		func->func = box_module_sym(bxmod, name.sym);
-		if (func->func == NULL)
+		func_c_unload(func);
+		if (func_c_load(bxmod, name.sym, func) != 0)
 			goto restore;
-		func->bxmod = bxmod;
-		rlist_move(&bxmod->funcs, &func->item);
 	}
-	cache_del(package, package_end);
-	if (cache_put(bxmod) != 0)
-		goto restore;
-	box_module_gc(bxmod_old);
+
+	cache_update(bxmod);
+	box_module_unref(bxmod_old);
 	return 0;
+
 restore:
 	/*
 	 * Some old-dso func can't be load from new module, restore old
@@ -412,8 +359,8 @@ box_module_reload(const char *package, const char *package_end)
 	do {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
-		func->func = box_module_sym(bxmod_old, name.sym);
-		if (func->func == NULL) {
+
+		if (func_c_load(bxmod_old, name.sym, func) != 0) {
 			/*
 			 * Something strange was happen, an early loaden
 			 * function was not found in an old dso.
@@ -421,12 +368,10 @@ box_module_reload(const char *package, const char *package_end)
 			panic("Can't restore module function, "
 			      "server state is inconsistent");
 		}
-		func->bxmod = bxmod_old;
-		rlist_move(&bxmod_old->funcs, &func->item);
 	} while (func != rlist_first_entry(&bxmod_old->funcs,
 					   struct func_c, item));
 	assert(rlist_empty(&bxmod->funcs));
-	box_module_delete(bxmod);
+	box_module_unref(bxmod);
 	return -1;
 }
 
@@ -437,6 +382,7 @@ func_c_new(struct func_def *def);
 extern struct func *
 func_sql_builtin_new(struct func_def *def);
 
+/** Allocate a new function. */
 struct func *
 func_new(struct func_def *def)
 {
@@ -474,67 +420,51 @@ func_new(struct func_def *def)
 	return func;
 }
 
-static struct func_vtab func_c_vtab;
-
+/** Create new C function. */
 static struct func *
 func_c_new(MAYBE_UNUSED struct func_def *def)
 {
 	assert(def->language == FUNC_LANGUAGE_C);
 	assert(def->body == NULL && !def->is_sandboxed);
-	struct func_c *func = (struct func_c *) malloc(sizeof(struct func_c));
-	if (func == NULL) {
-		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
+	struct func_c *func_c = malloc(sizeof(struct func_c));
+	if (func_c == NULL) {
+		diag_set(OutOfMemory, sizeof(*func_c), "malloc", "func_c");
 		return NULL;
 	}
-	func->base.vtab = &func_c_vtab;
-	func->func = NULL;
-	func->bxmod = NULL;
-	return &func->base;
-}
-
-static void
-func_c_unload(struct func_c *func)
-{
-	if (func->bxmod) {
-		rlist_del(&func->item);
-		if (rlist_empty(&func->bxmod->funcs)) {
-			struct func_name name;
-			func_split_name(func->base.def->name, &name);
-			cache_del(name.package, name.package_end);
-		}
-		box_module_gc(func->bxmod);
-	}
-	func->bxmod = NULL;
-	func->func = NULL;
+	func_c_create(func_c);
+	return &func_c->base;
 }
 
+/** Destroy C function. */
 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);
+	struct func_c *func_c = (struct func_c *) base;
+	box_module_unref(func_c->bxmod);
+	func_c_unload(func_c);
 	TRASH(base);
-	free(func);
+	free(func_c);
 }
 
 /**
- * Resolve func->func (find the respective DLL and fetch the
+ * Resolve func->func (find the respective share library and fetch the
  * symbol from it).
  */
 static int
-func_c_load(struct func_c *func)
+func_c_resolve(struct func_c *func_c)
 {
-	assert(func->func == NULL);
+	assert(is_func_c_emtpy(func_c));
 
 	struct func_name name;
-	func_split_name(func->base.def->name, &name);
+	func_split_name(func_c->base.def->name, &name);
 
 	struct box_module *cached, *bxmod;
 	cached = cache_find(name.package, name.package_end);
 	if (cached == NULL) {
-		bxmod = box_module_load(name.package, name.package_end);
+		size_t len = name.package_end - name.package;
+		bxmod = box_module_load(name.package, len);
 		if (bxmod == NULL)
 			return -1;
 		if (cache_put(bxmod) != 0) {
@@ -545,8 +475,7 @@ func_c_load(struct func_c *func)
 		bxmod = cached;
 	}
 
-	func->func = box_module_sym(bxmod, name.sym);
-	if (func->func == NULL) {
+	if (func_c_load(bxmod, name.sym, func_c) != 0) {
 		if (cached == NULL) {
 			/*
 			 * In case if it was a first load we should
@@ -557,54 +486,27 @@ func_c_load(struct func_c *func)
 			 * Note the box_module_sym set an error thus be
 			 * careful to not wipe it.
 			 */
-			cache_del(name.package, name.package_end);
+			cache_del(bxmod);
 			box_module_delete(bxmod);
 		}
 		return -1;
 	}
-	func->bxmod = bxmod;
-	rlist_add(&bxmod->funcs, &func->item);
 	return 0;
 }
 
-int
+/** Execute C function. */
+static 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)
+	struct func_c *func_c = (struct func_c *) base;
+	if (is_func_c_emtpy(func_c)) {
+		if (func_c_resolve(func_c) != 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 box_module *bxmod = func->bxmod;
-	assert(bxmod != NULL);
-	++bxmod->calls;
-	int rc = func->func(&ctx, data, data + data_sz);
-	--bxmod->calls;
-	box_module_gc(bxmod);
-	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;
+	return module_func_call(&func_c->mf, args, ret);
 }
 
 static struct func_vtab func_c_vtab = {
diff --git a/src/box/func.h b/src/box/func.h
index 66e4d09bc..2be33a812 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -37,6 +37,7 @@
 #include "small/rlist.h"
 #include "func_def.h"
 #include "user_def.h"
+#include "module_cache.h"
 
 #if defined(__cplusplus)
 extern "C" {
@@ -44,20 +45,6 @@ extern "C" {
 
 struct func;
 
-/**
- * Dynamic shared module.
- */
-struct box_module {
-	/** Module dlhandle. */
-	void *handle;
-	/** List of imported functions. */
-	struct rlist funcs;
-	/** Count of active calls. */
-	size_t calls;
-	/** Module's package name. */
-	char package[0];
-};
-
 /** Virtual method table for func object. */
 struct func_vtab {
 	/** Call function with given arguments. */
@@ -85,13 +72,13 @@ struct func {
 };
 
 /**
- * Initialize modules subsystem.
+ * Initialize box modules subsystem.
  */
 int
 box_module_init(void);
 
 /**
- * Cleanup modules subsystem.
+ * Cleanup box modules subsystem.
  */
 void
 box_module_free(void);
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
-- 
2.29.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Tarantool-patches] [PATCH v19 5/6] box/cmod: implement cmod Lua module
  2021-03-01 21:23 [Tarantool-patches] [PATCH v19 0/6] box: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
                   ` (3 preceding siblings ...)
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 4/6] box/func: switch to module_cache interface Cyrill Gorcunov via Tarantool-patches
@ 2021-03-01 21:23 ` Cyrill Gorcunov via Tarantool-patches
  2021-03-08 22:51   ` Vladislav Shpilevoy via Tarantool-patches
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 6/6] test: box/cfunc -- add cmod test Cyrill Gorcunov via Tarantool-patches
  5 siblings, 1 reply; 12+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-01 21:23 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

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

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

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

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

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

Closes #4642

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

@TarantoolBot document
Title: cmod module

Overview
========

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

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

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

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

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

Possible errors:

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

Example:

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

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

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

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

Possible errors:

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

Example:

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

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

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

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

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

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

Example:

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

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

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

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

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

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

Example:

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

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

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

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

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

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

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

First we should load it as

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

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

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

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

On success the sum of arguments will be printed out.

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

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

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

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

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

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

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

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index cc2e17e94..7115cdf55 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -196,6 +196,7 @@ add_library(box STATIC
     lua/call.c
     lua/cfg.cc
     lua/console.c
+    lua/cmod.c
     lua/serialize_lua.c
     lua/tuple.c
     lua/slab.c
diff --git a/src/box/lua/cmod.c b/src/box/lua/cmod.c
new file mode 100644
index 000000000..ea5acd510
--- /dev/null
+++ b/src/box/lua/cmod.c
@@ -0,0 +1,605 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#include <unistd.h>
+#include <string.h>
+#include <dlfcn.h>
+#include <fcntl.h>
+#include <lua.h>
+
+#include "box/error.h"
+#include "box/port.h"
+#include "box/func_def.h"
+
+#include "tt_static.h"
+
+#include "assoc.h"
+#include "cmod.h"
+#include "fiber.h"
+#include "module_cache.h"
+
+#include "lua/utils.h"
+#include "libeio/eio.h"
+
+/**
+ * Function descriptor.
+ */
+struct cmod_func {
+	/**
+	 * C function to call.
+	 */
+	struct module_func mf;
+	/** Number of references. */
+	int64_t refs;
+	/** Length of functon name in @a key. */
+	size_t sym_len;
+	/** Length of @a key. */
+	size_t len;
+	/** Function hash key. */
+	char key[0];
+};
+
+/** Function name to cmod_func hash. */
+static struct mh_strnptr_t *cmod_func_hash = NULL;
+
+/** A type to find a module from an object. */
+static const char *uname_cmod = "tt_uname_cmod";
+
+/** A type to find a function from an object. */
+static const char *uname_func = "tt_uname_cmod_func";
+
+/** Get data associated with an object. */
+static void *
+get_udata(struct lua_State *L, const char *uname)
+{
+	void **pptr = luaL_testudata(L, 1, uname);
+	return pptr != NULL ? *pptr : NULL;
+}
+
+/** Set data to a new value. */
+static void
+set_udata(struct lua_State *L, const char *uname, void *ptr)
+{
+	void **pptr = luaL_testudata(L, 1, uname);
+	assert(pptr != NULL);
+	*pptr = ptr;
+}
+
+/** Setup a new data and associate it with an object. */
+static void
+new_udata(struct lua_State *L, const char *uname, void *ptr)
+{
+	*(void **)lua_newuserdata(L, sizeof(void *)) = ptr;
+	luaL_getmetatable(L, uname);
+	lua_setmetatable(L, -2);
+}
+
+/**
+ * Helpers for function cache.
+ */
+static void *
+cf_cache_find(const char *str, size_t len)
+{
+	mh_int_t e = mh_strnptr_find_inp(cmod_func_hash, str, len);
+	if (e == mh_end(cmod_func_hash))
+		return NULL;
+	return mh_strnptr_node(cmod_func_hash, e)->val;
+}
+
+static int
+cf_cache_add(struct cmod_func *cf)
+{
+	const struct mh_strnptr_node_t nd = {
+		.str	= cf->key,
+		.len	= cf->len,
+		.hash	= mh_strn_hash(cf->key, cf->len),
+		.val	= cf,
+	};
+	mh_int_t e = mh_strnptr_put(cmod_func_hash, &nd, NULL, NULL);
+	if (e == mh_end(cmod_func_hash)) {
+		diag_set(OutOfMemory, sizeof(nd), "malloc",
+			 "cmod: hash node");
+		return -1;
+	}
+	return 0;
+}
+
+static void
+cache_del(struct cmod_func *cf)
+{
+	mh_int_t e = mh_strnptr_find_inp(cmod_func_hash,
+					 cf->key, cf->len);
+	if (e != mh_end(cmod_func_hash))
+		mh_strnptr_del(cmod_func_hash, e, NULL);
+}
+
+/**
+ * Load a module.
+ *
+ * This function takes a module path from the caller
+ * stack @a L and returns cached module instance or
+ * creates a new module object.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: module path is either not supplied
+ *   or not a string.
+ * - SystemError: unable to open a module due to a system error.
+ * - ClientError: a module does not exist.
+ * - OutOfMemory: unable to allocate a module.
+ *
+ * @returns module object on success or throws an error.
+ */
+static int
+lcmod_load(struct lua_State *L)
+{
+	const char msg_noname[] = "Expects cmod.load(\'name\') "
+		"but no name passed";
+
+	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
+		diag_set(IllegalParams, msg_noname);
+		return luaT_error(L);
+	}
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+
+	if (name_len < 1) {
+		diag_set(IllegalParams, msg_noname);
+		return luaT_error(L);
+	}
+
+	struct module *m = module_load(name, name_len);
+	if (m == NULL)
+		return luaT_error(L);
+
+	new_udata(L, uname_cmod, m);
+	return 1;
+}
+
+/**
+ * Unload a module.
+ *
+ * Take a module object from the caller stack @a L and unload it.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: module is not supplied.
+ * - IllegalParams: the module is unloaded.
+ *
+ * @returns true on success or throwns an error.
+ */
+static int
+lcmod_unload(struct lua_State *L)
+{
+	if (lua_gettop(L) != 1) {
+		diag_set(IllegalParams, "Expects module:unload()");
+		return luaT_error(L);
+	}
+
+	struct module *m = get_udata(L, uname_cmod);
+	if (m == NULL) {
+		diag_set(IllegalParams, "The module is unloaded");
+		return luaT_error(L);
+	}
+
+	set_udata(L, uname_cmod, NULL);
+	module_unload(m);
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/** Handle __index request for a module object. */
+static int
+lcmod_index(struct lua_State *L)
+{
+	lua_getmetatable(L, 1);
+	lua_pushvalue(L, 2);
+	lua_rawget(L, -2);
+	if (!lua_isnil(L, -1))
+		return 1;
+
+	struct module *m = get_udata(L, uname_cmod);
+	if (m == NULL) {
+		lua_pushnil(L);
+		return 1;
+	}
+
+	const char *key = lua_tostring(L, 2);
+	if (key == NULL || lua_type(L, 2) != LUA_TSTRING) {
+		diag_set(IllegalParams,
+			 "Bad params, use __index(obj, <string>)");
+		return luaT_error(L);
+	}
+
+	if (strcmp(key, "path") == 0) {
+		lua_pushstring(L, m->package);
+		return 1;
+	}
+
+	/*
+	 * Internal keys for debug only, not API.
+	 */
+	if (strncmp(key, "tt_dev.", 7) == 0) {
+		const char *subkey = &key[7];
+		if (strcmp(subkey, "refs") == 0) {
+			lua_pushnumber(L, m->refs);
+			return 1;
+		} else if (strcmp(subkey, "ptr") == 0) {
+			const char *s = tt_sprintf("%p", m);
+			lua_pushstring(L, s);
+			return 1;
+		}
+	}
+	return 0;
+}
+
+/** Module representation for REPL (console). */
+static int
+lcmod_serialize(struct lua_State *L)
+{
+	struct module *m = get_udata(L, uname_cmod);
+	if (m == NULL) {
+		lua_pushnil(L);
+		return 1;
+	}
+
+	lua_createtable(L, 0, 1);
+	lua_pushstring(L, m->package);
+	lua_setfield(L, -2, "path");
+	return 1;
+}
+
+/** Collect a module. */
+static int
+lcmod_gc(struct lua_State *L)
+{
+	struct module *m = get_udata(L, uname_cmod);
+	if (m != NULL) {
+		set_udata(L, uname_cmod, NULL);
+		module_unload(m);
+	}
+	return 0;
+}
+
+/** Increase reference to a function. */
+static void
+cmod_func_ref(struct cmod_func *cf)
+{
+	assert(cf->refs >= 0);
+	++cf->refs;
+}
+
+/** Free function memory. */
+static void
+cmod_func_delete(struct cmod_func *cf)
+{
+	TRASH(cf);
+	free(cf);
+}
+
+/** Unreference a function and free if last one. */
+static void
+cmod_func_unref(struct cmod_func *cf)
+{
+	assert(cf->refs > 0);
+	if (--cf->refs == 0) {
+		module_func_unload(&cf->mf);
+		cache_del(cf);
+		cmod_func_delete(cf);
+	}
+}
+
+/** Function name from a hash key. */
+static char *
+cmod_func_name(struct cmod_func *cf)
+{
+	return &cf->key[cf->len - cf->sym_len];
+}
+
+/**
+ * Allocate a new function instance and resolve its address.
+ *
+ * @param m a module the function should be loaded from.
+ * @param key function hash key, ie "addr.module.foo".
+ * @param len length of @a key.
+ * @param sym_len function symbol name length, ie 3 for "foo".
+ *
+ * @returns function instance on success, NULL otherwise (diag is set).
+ */
+static struct cmod_func *
+cmod_func_new(struct module *m, const char *key, size_t len, size_t sym_len)
+{
+	size_t size = sizeof(struct cmod_func) + len + 1;
+	struct cmod_func *cf = malloc(size);
+	if (cf == NULL) {
+		diag_set(OutOfMemory, size, "malloc", "cf");
+		return NULL;
+	}
+
+	cf->len = len;
+	cf->sym_len = sym_len;
+	cf->refs = 0;
+
+	module_func_create(&cf->mf);
+	memcpy(cf->key, key, len);
+	cf->key[len] = '\0';
+
+	if (module_func_load(m, cmod_func_name(cf), &cf->mf) != 0) {
+		diag_set(ClientError, ER_LOAD_FUNCTION,
+			 cmod_func_name(cf), dlerror());
+		cmod_func_delete(cf);
+		return NULL;
+	}
+
+	if (cf_cache_add(cf) != 0) {
+		cmod_func_delete(cf);
+		return NULL;
+	}
+
+	/*
+	 * Each new function depends on module presence.
+	 * Module will reside even if been unload
+	 * explicitly after the function creation.
+	 */
+	cmod_func_ref(cf);
+	return cf;
+}
+
+/**
+ * Load a function.
+ *
+ * This function takes a function name from the caller
+ * stack @a L and either returns a cached function or
+ * creates a new function object.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: function name is either not supplied
+ *   or not a string.
+ * - SystemError: unable to open a module due to a system error.
+ * - ClientError: a module does not exist.
+ * - OutOfMemory: unable to allocate a module.
+ *
+ * @returns module object on success or throws an error.
+ */
+static int
+lcmod_load_func(struct lua_State *L)
+{
+	const char *method = "function = module:load";
+	const char fmt_noname[] = "Expects %s(\'name\') but no name passed";
+
+	if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) {
+		diag_set(IllegalParams, fmt_noname, method);
+		return luaT_error(L);
+	}
+
+	struct module *m = get_udata(L, uname_cmod);
+	if (m == NULL) {
+		const char *fmt =
+			"Expects %s(\'name\') but not module object passed";
+		diag_set(IllegalParams, fmt, method);
+		return luaT_error(L);
+	}
+
+	size_t sym_len;
+	const char *sym = lua_tolstring(L, 2, &sym_len);
+
+	if (sym_len < 1) {
+		diag_set(IllegalParams, fmt_noname, method);
+		return luaT_error(L);
+	}
+
+	/*
+	 * Functions are bound to a module symbols, thus
+	 * since the hash is global it should be unique
+	 * per module. The symbol (function name) is the
+	 * last part of the hash key.
+	 */
+	const char *key = tt_sprintf("%p.%s.%s", (void *)m,
+				     m->package, sym);
+	size_t len = strlen(key);
+
+	struct cmod_func *cf = cf_cache_find(key, len);
+	if (cf == NULL) {
+		cf = cmod_func_new(m, key, len, sym_len);
+		if (cf == NULL)
+			return luaT_error(L);
+	} else {
+		cmod_func_ref(cf);
+	}
+
+	new_udata(L, uname_func, cf);
+	return 1;
+}
+
+/**
+ * Unload a function.
+ *
+ * Take a function object from the caller stack @a L and unload it.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: the function is not supplied.
+ * - IllegalParams: the function already unloaded.
+ *
+ * @returns true on success or throwns an error.
+ */
+static int
+lfunc_unload(struct lua_State *L)
+{
+	if (lua_gettop(L) != 1) {
+		diag_set(IllegalParams, "Expects function:unload()");
+		return luaT_error(L);
+	}
+
+	struct cmod_func *cf = get_udata(L, uname_func);
+	if (cf == NULL) {
+		diag_set(IllegalParams, "The function is unloaded");
+		return luaT_error(L);
+	}
+
+	set_udata(L, uname_func, NULL);
+	cmod_func_unref(cf);
+
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/** Handle __index request for a function object. */
+static int
+lfunc_index(struct lua_State *L)
+{
+	lua_getmetatable(L, 1);
+	lua_pushvalue(L, 2);
+	lua_rawget(L, -2);
+	if (!lua_isnil(L, -1))
+		return 1;
+
+	struct cmod_func *cf = get_udata(L, uname_func);
+	if (cf == NULL) {
+		lua_pushnil(L);
+		return 1;
+	}
+
+	const char *key = lua_tostring(L, 2);
+	if (key == NULL || lua_type(L, 2) != LUA_TSTRING) {
+		diag_set(IllegalParams,
+			 "Bad params, use __index(obj, <string>)");
+		return luaT_error(L);
+	}
+
+	if (strcmp(key, "name") == 0) {
+		lua_pushstring(L, cmod_func_name(cf));
+		return 1;
+	}
+
+	/*
+	 * Internal keys for debug only, not API.
+	 */
+	if (strncmp(key, "tt_dev.", 7) == 0) {
+		const char *subkey = &key[7];
+		if (strcmp(subkey, "refs") == 0) {
+			lua_pushnumber(L, cf->refs);
+			return 1;
+		} else if (strcmp(subkey, "key") == 0) {
+			lua_pushstring(L, cf->key);
+			return 1;
+		} else if (strcmp(subkey, "module.ptr") == 0) {
+			const char *s = tt_sprintf("%p", cf->mf.module);
+			lua_pushstring(L, s);
+			return 1;
+		} else if (strcmp(subkey, "module.refs") == 0) {
+			lua_pushnumber(L, cf->mf.module->refs);
+			return 1;
+		}
+	}
+	return 0;
+}
+
+/** Function representation for REPL (console). */
+static int
+lfunc_serialize(struct lua_State *L)
+{
+	struct cmod_func *cf = get_udata(L, uname_func);
+	if (cf == NULL) {
+		lua_pushnil(L);
+		return 1;
+	}
+
+	lua_createtable(L, 0, 1);
+	lua_pushstring(L, cmod_func_name(cf));
+	lua_setfield(L, -2, "name");
+	return 1;
+}
+
+/** Collect a function. */
+static int
+lfunc_gc(struct lua_State *L)
+{
+	struct cmod_func *cf = get_udata(L, uname_func);
+	if (cf != NULL) {
+		set_udata(L, uname_func, NULL);
+		cmod_func_unref(cf);
+	}
+	return 0;
+}
+
+
+/** Call a function by its name from the Lua code. */
+static int
+lfunc_call(struct lua_State *L)
+{
+	struct cmod_func *cf = get_udata(L, uname_func);
+	if (cf == NULL) {
+		diag_set(IllegalParams, "The function is unloaded");
+		return luaT_error(L);
+	}
+
+	lua_State *args_L = luaT_newthread(tarantool_L);
+	if (args_L == NULL)
+		return luaT_error(L);
+
+	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+	lua_xmove(L, args_L, lua_gettop(L) - 1);
+
+	struct port args;
+	port_lua_create(&args, args_L);
+	((struct port_lua *)&args)->ref = coro_ref;
+
+	struct port ret;
+
+	if (module_func_call(&cf->mf, &args, &ret) != 0) {
+		port_destroy(&args);
+		return luaT_error(L);
+	}
+
+	int top = lua_gettop(L);
+	port_dump_lua(&ret, L, true);
+	int cnt = lua_gettop(L) - top;
+
+	port_destroy(&ret);
+	port_destroy(&args);
+
+	return cnt;
+}
+
+/** Initialize cmod. */
+void
+box_lua_cmod_init(struct lua_State *L)
+{
+	cmod_func_hash = mh_strnptr_new();
+	if (cmod_func_hash == NULL)
+		panic("cmod: Can't allocate func hash table");
+
+	(void)L;
+	static const struct luaL_Reg top_methods[] = {
+		{ "load",		lcmod_load		},
+		{ NULL, NULL },
+	};
+	luaL_register_module(L, "cmod", top_methods);
+	lua_pop(L, 1);
+
+	static const struct luaL_Reg lcmod_methods[] = {
+		{ "unload",		lcmod_unload		},
+		{ "load",		lcmod_load_func		},
+		{ "__index",		lcmod_index		},
+		{ "__serialize",	lcmod_serialize		},
+		{ "__gc",		lcmod_gc		},
+		{ NULL, NULL },
+	};
+	luaL_register_type(L, uname_cmod, lcmod_methods);
+
+	static const struct luaL_Reg lfunc_methods[] = {
+		{ "unload",		lfunc_unload		},
+		{ "__index",		lfunc_index		},
+		{ "__serialize",	lfunc_serialize		},
+		{ "__gc",		lfunc_gc		},
+		{ "__call",		lfunc_call		},
+		{ NULL, NULL },
+	};
+	luaL_register_type(L, uname_func, lfunc_methods);
+}
diff --git a/src/box/lua/cmod.h b/src/box/lua/cmod.h
new file mode 100644
index 000000000..b369c21e0
--- /dev/null
+++ b/src/box/lua/cmod.h
@@ -0,0 +1,25 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#pragma once
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct lua_State;
+
+/**
+ * Initialize cmod Lua module.
+ *
+ * @param L Lua state where to register the cmod module.
+ */
+void
+box_lua_cmod_init(struct lua_State *L);
+
+#if defined(__cplusplus)
+}
+#endif /* defined(__plusplus) */
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index fbcdfb20b..bad2b7ca9 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -60,6 +60,7 @@
 #include "box/lua/cfg.h"
 #include "box/lua/xlog.h"
 #include "box/lua/console.h"
+#include "box/lua/cmod.h"
 #include "box/lua/tuple.h"
 #include "box/lua/execute.h"
 #include "box/lua/key_def.h"
@@ -465,6 +466,7 @@ box_lua_init(struct lua_State *L)
 	box_lua_tuple_init(L);
 	box_lua_call_init(L);
 	box_lua_cfg_init(L);
+	box_lua_cmod_init(L);
 	box_lua_slab_init(L);
 	box_lua_index_init(L);
 	box_lua_space_init(L);
-- 
2.29.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Tarantool-patches] [PATCH v19 6/6] test: box/cfunc -- add cmod test
  2021-03-01 21:23 [Tarantool-patches] [PATCH v19 0/6] box: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
                   ` (4 preceding siblings ...)
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 5/6] box/cmod: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
@ 2021-03-01 21:23 ` Cyrill Gorcunov via Tarantool-patches
  2021-03-08 22:51   ` Vladislav Shpilevoy via Tarantool-patches
  5 siblings, 1 reply; 12+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-01 21:23 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

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

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

Part-of #4642

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

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v19 2/6] box/func: prepare for transition to modules subsystem
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 2/6] box/func: prepare for transition to modules subsystem Cyrill Gorcunov via Tarantool-patches
@ 2021-03-08 22:44   ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-08 22:44 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml

Hi! Thanks for the patch!

See 4 comments below.

> diff --git a/src/box/call.c b/src/box/call.c
> index 7839e1f3e..4b1893f12 100644
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -116,7 +116,7 @@ static const struct port_vtab port_msgpack_vtab = {
>  };
>  
>  int
> -box_module_reload(const char *name)
> +box_process_module_reload(const char *name)

1. The name is ugly. I propose to merge this function into
box_module_reload(). The latter is never used from anywhere
except this 'process' thing.

>  {
>  	struct credentials *credentials = effective_user();
>  	if ((credentials->universal_access & (PRIV_X | PRIV_U)) !=
> diff --git a/src/box/func.c b/src/box/func.c
> index 233696a4f..88903f40e 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -71,7 +71,7 @@ struct func_c {
>  	 * Each stored function keeps a handle to the
>  	 * dynamic library for the C callback.
>  	 */
> -	struct module *module;
> +	struct box_module *bxmod;

2. What was wrong with the old member name? Especially in
the functions. It just significantly increases the diff, ruins
the history, and does not change readability anyhow. The old
name was even better IMO.

>  };
>  
>  /***
> @@ -166,61 +166,65 @@ module_find(const char *package, const char *package_end, char *path,
>  	return 0;
>  }
>  
> -static struct mh_strnptr_t *modules = NULL;
> +static struct mh_strnptr_t *box_module_hash = NULL;

3. The same question. 'modules' was just fine. Diff is
significantly bigger because most of such renames are not
needed at all. You only need to rename the struct. It was
module, and stayed module. Just a new prefix is added to
the struct name and its methods.

> @@ -484,23 +488,23 @@ func_c_new(MAYBE_UNUSED struct func_def *def)
>  	}
>  	func->base.vtab = &func_c_vtab;
>  	func->func = NULL;
> -	func->module = NULL;
> +	func->bxmod = NULL;
>  	return &func->base;
>  }
>  
>  static void
>  func_c_unload(struct func_c *func)
>  {
> -	if (func->module) {
> +	if (func->bxmod) {

4. Since you changed this line, it should be != NULL now. But even better -
keep the old variable and struct member names. Then you wouldn't need to
change almost anything in this file except struct names.

>  		rlist_del(&func->item);
> -		if (rlist_empty(&func->module->funcs)) {
> +		if (rlist_empty(&func->bxmod->funcs)) {
>  			struct func_name name;
>  			func_split_name(func->base.def->name, &name);
> -			module_cache_del(name.package, name.package_end);
> +			cache_del(name.package, name.package_end);
>  		}
> -		module_gc(func->module);
> +		box_module_gc(func->bxmod);
>  	}
> -	func->module = NULL;
> +	func->bxmod = NULL;
>  	func->func = NULL;
>  }

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v19 3/6] box/module_cache: introduce modules subsystem
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 3/6] box/module_cache: introduce " Cyrill Gorcunov via Tarantool-patches
@ 2021-03-08 22:45   ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-08 22:45 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml

Thanks for the patch!

See 10 comments below.

> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> new file mode 100644
> index 000000000..bc2184e5f
> --- /dev/null
> +++ b/src/box/module_cache.c
> @@ -0,0 +1,476 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <dlfcn.h>
> +#include <lua.h>
> +
> +#include "assoc.h"
> +#include "diag.h"
> +#include "fiber.h"
> +#include "module_cache.h"
> +
> +#include "box/error.h"
> +#include "box/port.h"
> +
> +#include "lua/utils.h"
> +#include "libeio/eio.h"
> +
> +static struct mh_strnptr_t *module_cache = NULL;
> +
> +/**
> + * Helpers for cache manipulations.
> + */
> +static void *
> +cache_find(const char *str, size_t len)
> +{
> +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> +	if (e == mh_end(module_cache))
> +		return NULL;
> +	return mh_strnptr_node(module_cache, e)->val;
> +}
> +
> +static void
> +cache_update(struct module *m)
> +{
> +	const char *str = m->package;
> +	size_t len = m->package_len;
> +
> +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> +	if (e == mh_end(module_cache))
> +		panic("module: failed to update cache: %s", str);
> +
> +	mh_strnptr_node(module_cache, e)->str = m->package;
> +	mh_strnptr_node(module_cache, e)->val = m;
> +}
> +
> +static int
> +cache_add(struct module *m)

1. In func.c it is called `cache_put`, but does the same. What is the
difference between `put` and `add` then? If none, then why not use
only one verb?

> +{
> +	const struct mh_strnptr_node_t nd = {
> +		.str	= m->package,
> +		.len	= m->package_len,
> +		.hash	= mh_strn_hash(m->package, m->package_len),
> +		.val	= m,
> +	};
> +
> +	mh_int_t e = mh_strnptr_put(module_cache, &nd, NULL, NULL);
> +	if (e == mh_end(module_cache)) {
> +		diag_set(OutOfMemory, sizeof(nd), "malloc",
> +			 "module_cache node");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static void
> +cache_del(struct module *m)
> +{
> +	const char *str = m->package;
> +	size_t len = m->package_len;
> +
> +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> +	if (e != mh_end(module_cache)) {
> +		struct module *v = mh_strnptr_node(module_cache, e)->val;
> +		if (v == m)
> +			mh_strnptr_del(module_cache, e, NULL);

2. Please, add a comment about why the module pointer in the cache
might be different.

> +	}
> +}
> +
> +struct module *
> +module_load_force(const char *package, size_t package_len)
> +{
> +	char path[PATH_MAX];
> +	size_t size = sizeof(path);
> +
> +	if (find_package(package, package_len, path, size) != 0)
> +		return NULL;
> +
> +	struct module *m = module_new(package, package_len, path);
> +	if (m == NULL)
> +		return NULL;
> +
> +	struct module *c = cache_find(package, package_len);
> +	if (c != NULL) {
> +		cache_update(m);
> +		say_info("module_cache: force load: %s", package);

3. That is not very helpful for a user - he does not know what
'module_cache' is, and what is 'force load'. Try to make a more
user-friendly message. It should not use any internal names.

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

4. That is not safe in case the struct has padding between
members and/or in the end. You either must compare the members
manually, or must ensure there is no any padding. For instance,
reorder the members not to have the padding in between, add a dummy
member in the end to fill the end-padding if necessary and fill it
with 0 always, and add a static_assert its size is equal to the sum
of member sizes. Then memcmp should be safe. Otherwise you risk to
compare garbage.

> +			module_ref(m);
> +			return m;
> +		}
> +
> +		/*
> +		 * Module has been updated on a storage device,
> +		 * so load a new instance and update the cache,
> +		 * old entry get evicted but continue residing
> +		 * in memory, fully functional, until last
> +		 * function is unloaded.
> +		 */
> +		m = module_new(package, package_len, path);
> +		if (m != NULL) {
> +			cache_update(m);
> +			say_info("module_cache: attr changed: %s",
> +				 package);
> +		}
> +	} else {
> +		m = module_new(package, package_len, path);
> +		if (m != NULL) {
> +			if (cache_add(m) != 0) {

5. You could improve readability a bit if you would do early
returns on errors (return NULL right away if m == NULL), or
reduce the indentation. For instance, by using a single `if`
with the condition `m != NULL && cache_add(m) != 0` instead
of 2 `if`s.

> +				module_unload(m);
> +				return NULL;
> +			}
> +		}
> +	}
> +
> +	return m;
> +}
> +
> +void
> +module_unload(struct module *m)
> +{
> +	module_unref(m);
> +}
> +
> +void
> +module_free(void)
> +{
> +	size_t nr_busy = 0;
> +	mh_int_t e;
> +
> +	mh_foreach(module_cache, e) {
> +		struct module *m = mh_strnptr_node(module_cache, e)->val;
> +		if (m->refs > 1)
> +			nr_busy++;
> +		module_unload(m);
> +	}
> +
> +	mh_strnptr_delete(module_cache);
> +	module_cache = NULL;
> +
> +	if (nr_busy > 0)
> +		say_info("module_cache: %zu entries left", nr_busy);

6. This probably must be panic. If there are modules, they are going to
try to delete themselves from the cache later, which is NULL, so it
would crash anyway but without any message.

`module_cache` must be empty when module_free() is called.

> +}
> diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> new file mode 100644
> index 000000000..78dd5c7ad
> --- /dev/null
> +++ b/src/box/module_cache.h
> @@ -0,0 +1,206 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#pragma once
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +
> +#include "trivia/config.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +/**
> + * API of C stored function.
> + */
> +
> +struct port;
> +
> +struct box_function_ctx {
> +	struct port *port;
> +};
> +
> +typedef struct box_function_ctx box_function_ctx_t;
> +typedef int (*box_function_t)(box_function_ctx_t *ctx,
> +			      const char *args,
> +			      const char *args_end);
> +
> +/**
> + * Shared library file attributes for
> + * module cache invalidation.
> + */
> +struct module_attr {
> +#ifdef TARGET_OS_DARWIN
> +	struct timespec st_mtimespec;
> +#else
> +	struct timespec st_mtim;
> +#endif
> +	dev_t st_dev;
> +	ino_t st_ino;
> +	off_t st_size;

7. For the sake of easier memcmp() you could use platform-agnostic types.
For instance, u/int64_t or u/int32_t.

> +};
> +
> +/**
> + * Dynamic shared module.
> + */
> +struct module {
> +	/**
> +	 * Module handle, dlopen() result.
> +	 */
> +	void *handle;
> +	/**
> +	 * File attributes.
> +	 */
> +	struct module_attr attr;
> +	/**
> +	 * Count of active references.
> +	 */
> +	int64_t refs;
> +	/**
> +	 * Length of @a package.
> +	 */
> +	size_t package_len;
> +	/**
> +	 * Module's name without file extension.
> +	 */
> +	char package[0];
> +};
> +
> +/**
> + * Module function.
> + */
> +struct module_func {
> +	/**
> +	 * Function's address, iow dlsym() result.
> +	 */
> +	 box_function_t func;
> +	/**
> +	 * Function's module.
> +	 */
> +	struct module *module;
> +};
> +
> +/**
> + * Load a module.
> + *
> + * Lookup for a module instance in cache and if not found
> + * the module is loaded from a storage device.

8. It also checks for the attributes being changed.

> + *
> + * @param package module package (without file extension).
> + * @param package_len length of @a package.
> + *
> + * Possible errors:
> + * ClientError: the package is not found on a storage device.
> + * ClientError: an error happened when been loading the package.
> + * SystemError: a system error happened during procedure.
> + * OutOfMemory: unable to allocate new memory for module instance.
> + *
> + * @return a module instance on success, NULL otherwise (diag is set)
> + */
> +struct module *
> +module_load(const char *package, size_t package_len);
> +
> +/**
> + * Force load a module.
> + *
> + * Load a module from a storage device and invalidate
> + * an associated cache entry.

9. Reload, not invalidate. It stays cached, but with a new
pointer.

> + *
> + * @param package module package (without file extension).
> + * @param package_len length of @a package.
> + *
> + * Possible errors:
> + * ClientError: the package is not found on a storage device.
> + * ClientError: an error happened when been loading the package.
> + * SystemError: a system error happened during procedure.
> + * OutOfMemory: unable to allocate new memory for module instance.
> + *
> + * @return a module instance on success, NULL otherwise (diag is set)
> + */
> +struct module *
> +module_load_force(const char *package, size_t package_len);
> +
> +/**
> + * Unload a module instance.
> + *
> + * @param m a module to unload.
> + */
> +void
> +module_unload(struct module *m);
> +
> +/** Test if module function is empty. */
> +static inline bool
> +is_module_func_empty(struct module_func *mf)

10. Struct prefix is "stronger" than 'is' prefix. Otherwise
we would end up with tons of 'is' checkers in the code not
related to each other but having the same prefix, and
conflicting with variable names.

Here it should be module_func_is_empty().

> +{
> +	return mf->module == NULL;
> +}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v19 4/6] box/func: switch to module_cache interface
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 4/6] box/func: switch to module_cache interface Cyrill Gorcunov via Tarantool-patches
@ 2021-03-08 22:47   ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-08 22:47 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml

Thanks for the patch!

See 10 comments below.

>  src/box/func.c     | 492 ++++++++++++++++++---------------------------
>  src/box/func.h     |  19 +-
>  src/box/func_def.h |  14 --
>  3 files changed, 200 insertions(+), 325 deletions(-)
> 
> diff --git a/src/box/func.c b/src/box/func.c
> index 88903f40e..a8401c6ed 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -216,10 +134,15 @@ cache_find(const char *name, const char *name_end)
>  static int
>  cache_put(struct box_module *bxmod)
>  {
> -	size_t package_len = strlen(bxmod->package);
> -	uint32_t name_hash = mh_strn_hash(bxmod->package, package_len);
> +	const char *package = bxmod->module->package;
> +	size_t package_len = bxmod->module->package_len;
> +
>  	const struct mh_strnptr_node_t strnode = {
> -		bxmod->package, package_len, name_hash, bxmod};
> +		.str = package,
> +		.len = package_len,
> +		.hash = mh_strn_hash(package, package_len),
> +		.val = bxmod
> +	};

1. How do you decide when to align and when not align the
assignments?

>  
>  	mh_int_t i = mh_strnptr_put(box_module_hash, &strnode, NULL, NULL);
>  	if (i == mh_end(box_module_hash)) {
> @@ -231,150 +154,175 @@ cache_put(struct box_module *bxmod)
>  }
>  
>  /**
> - * Delete a module from the module cache
> + * Update module in module cache.
>   */
>  static void
> -cache_del(const char *name, const char *name_end)
> +cache_update(struct box_module *bxmod)
>  {
> -	mh_int_t i = mh_strnptr_find_inp(box_module_hash, name,
> -					 name_end - name);
> -	if (i == mh_end(box_module_hash))
> -		return;
> -	mh_strnptr_del(box_module_hash, i, NULL);
> +	const char *str = bxmod->module->package;
> +	size_t len = bxmod->module->package_len;
> +
> +	mh_int_t e = mh_strnptr_find_inp(box_module_hash, str, len);
> +	if (e == mh_end(box_module_hash))
> +		panic("func: failed to update cache: %s", str);
> +
> +	mh_strnptr_node(box_module_hash, e)->str = str;
> +	mh_strnptr_node(box_module_hash, e)->val = bxmod;
>  }
>  
> -/*
> - * Load a dso.
> - * Create a new symlink based on temporary directory and try to
> - * load via this symink to load a dso twice for cases of a function
> - * reload.
> +/**
> + * Delete a module from the module cache
>   */
> +static void
> +cache_del(struct box_module *bxmod)
> +{
> +	const char *str = bxmod->module->package;
> +	size_t len = bxmod->module->package_len;
> +
> +	mh_int_t e = mh_strnptr_find_inp(box_module_hash, str, len);
> +	if (e != mh_end(box_module_hash)) {
> +		struct box_module *v;
> +		v = mh_strnptr_node(box_module_hash, e)->val;
> +		if (v == bxmod)
> +			mh_strnptr_del(box_module_hash, e, NULL);

2. The same comment as in the other patch about the other
cache_del. You need to describe when the pointer might be
not the same.

> +	}
> +}
> +
> +/** Increment reference to a module. */
> +static inline void
> +box_module_ref(struct box_module *bxmod)
> +{
> +	assert(bxmod->refs >= 0);
> +	++bxmod->refs;
> +}
> +
> +/** Low-level module loader. */
>  static struct box_module *
> -box_module_load(const char *package, const char *package_end)
> +box_module_ld(const char *package, size_t package_len,
> +	      struct module *(ld)(const char *package,
> +				  size_t package_len))

3. Too complex. I propose to call module_load/module_load_force in
the upper code, and add box_module_new(struct module *) which
wrapps the returned struct module * into struct box_module *.

>  {
> -	char path[PATH_MAX];
> -	if (module_find(package, package_end, path, sizeof(path)) != 0)
> +	struct module *m = ld(package, package_len);
> +	if (m == NULL)
>  		return NULL;

Below this line is box_module_new(). Above this line you move to the
upper level. To box_module_load() and box_module_load_force().

>  
> -	int package_len = package_end - package;
> -	struct box_module *bxmod = malloc(sizeof(*bxmod) + package_len + 1);
> +	struct box_module *bxmod = malloc(sizeof(*bxmod));
>  	if (bxmod == NULL) {
> +		module_unload(m);
>  		diag_set(OutOfMemory, sizeof(*bxmod) + package_len + 1,
>  			 "malloc", "struct box_module");
>  		return NULL;
>  	}
> -	memcpy(bxmod->package, package, package_len);
> -	bxmod->package[package_len] = 0;
> -	rlist_create(&bxmod->funcs);
> -	bxmod->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;
> -	}
> +	bxmod->refs = 0;
> +	bxmod->module = m;
> +	rlist_create(&bxmod->funcs);
>  
> -	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;
> -	}
> +	box_module_ref(bxmod);
> +	return bxmod;
> +}
>  
> -	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;
> -	}
> +/** Load a new module. */
> +static struct box_module *
> +box_module_load(const char *package, size_t package_len)
> +{
> +	return box_module_ld(package, package_len,
> +			     module_load);
> +}
>  
> -	bxmod->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 (bxmod->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;

4. After you removed this injection, the tests don't pass.

> -	return bxmod;
> -error:
> -	free(bxmod);
> -	return NULL;
> +/** Load a new module with force cache invalidation. */
> +static struct box_module *
> +box_module_load_force(const char *package, size_t package_len)
> +{
> +	return box_module_ld(package, package_len,
> +			     module_load_force);
>  }
>  
> +/** Delete a module. */
>  static void
>  box_module_delete(struct box_module *bxmod)
>  {
>  	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
>  	if (e != NULL)
>  		--e->iparam;

5. Add an assertion that ref count is 0.

> -	dlclose(bxmod->handle);
> +	module_unload(bxmod->module);
>  	TRASH(bxmod);
>  	free(bxmod);
>  }
>  
> -/*
> - * Check if a dso is unused and can be closed.
> - */
> -static void
> -box_module_gc(struct box_module *bxmod)
> +/** Decrement reference to a module and delete it if last one. */
> +static inline void
> +box_module_unref(struct box_module *bxmod)
>  {
> -	if (rlist_empty(&bxmod->funcs) && bxmod->calls == 0)
> +	assert(bxmod->refs > 0);
> +	if (--bxmod->refs == 0) {
> +		cache_del(bxmod);
>  		box_module_delete(bxmod);
> +	}
>  }
>  
> -/*
> - * Import a function from the module.
> - */
> -static box_function_f
> -box_module_sym(struct box_module *bxmod, const char *name)
> +/** Free box modules subsystem. */
> +void
> +box_module_free(void)
>  {
> -	box_function_f f = (box_function_f)dlsym(bxmod->handle, name);
> -	if (f == NULL) {
> -		diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
> -		return NULL;
> +	while (mh_size(box_module_hash) > 0) {
> +		struct box_module *bxmod;
> +
> +		mh_int_t i = mh_first(box_module_hash);
> +		bxmod = mh_strnptr_node(box_module_hash, i)->val;
> +		/*
> +		 * Module won't be deleted if it has
> +		 * active functions bound.
> +		 */
> +		box_module_unref(bxmod);
>  	}
> -	return f;
> +	mh_strnptr_delete(box_module_hash);
> +}
> +
> +static struct func_vtab func_c_vtab;
> +
> +/** Create new box function. */
> +static inline void
> +func_c_create(struct func_c *func_c)
> +{
> +	func_c->bxmod = NULL;
> +	func_c->base.vtab = &func_c_vtab;
> +	rlist_create(&func_c->item);
> +	module_func_create(&func_c->mf);
> +}
> +
> +/** Test if function is not resolved. */
> +static inline bool
> +is_func_c_emtpy(struct func_c *func_c)

6. The same comment as about module_cache patch. It should be
func_c_is_empty().

> +{
> +	return is_module_func_empty(&func_c->mf);
>  }
>  
> +/** Load a new function. */
> +static inline int
> +func_c_load(struct box_module *bxmod, const char *func_name,
> +	      struct func_c *func_c)
> +{
> +	int rc = module_func_load(bxmod->module, func_name, &func_c->mf);
> +	if (rc == 0) {
> +		rlist_move(&bxmod->funcs, &func_c->item);
> +		func_c->bxmod = bxmod;
> +		box_module_ref(bxmod);
> +	}
> +	return rc;
> +}
> +
> +/** Unload a function. */
> +static inline void
> +func_c_unload(struct func_c *func_c)
> +{
> +	module_func_unload(&func_c->mf);
> +	rlist_del(&func_c->item);
> +	box_module_unref(func_c->bxmod);
> +	func_c_create(func_c);
> +}
> +
> +/** Reload module in a force way. */
>  int
>  box_module_reload(const char *package, const char *package_end)
>  {
> @@ -385,25 +333,24 @@ box_module_reload(const char *package, const char *package_end)
>  		return -1;
>  	}
>  
> -	struct box_module *bxmod = box_module_load(package, package_end);
> +	size_t len = package_end - package;
> +	struct box_module *bxmod = box_module_load_force(package, len);
>  	if (bxmod == NULL)
>  		return -1;
>  
> -	struct func_c *func, *tmp_func;
> -	rlist_foreach_entry_safe(func, &bxmod_old->funcs, item, tmp_func) {
> +	struct func_c *func, *tmp;

7. You didn't really need to rename tmp_func. Try not to make so much
unnecessary changes. It does not make it easier to find the functional
changes you made, nor read the history when it is needed occasionally.

> +	rlist_foreach_entry_safe(func, &bxmod_old->funcs, item, tmp) {
>  		struct func_name name;
>  		func_split_name(func->base.def->name, &name);
> -		func->func = box_module_sym(bxmod, name.sym);
> -		if (func->func == NULL)
> +		func_c_unload(func);
> +		if (func_c_load(bxmod, name.sym, func) != 0)
>  			goto restore;
> -		func->bxmod = bxmod;
> -		rlist_move(&bxmod->funcs, &func->item);
>  	}
> -	cache_del(package, package_end);
> -	if (cache_put(bxmod) != 0)
> -		goto restore;
> -	box_module_gc(bxmod_old);
> +
> +	cache_update(bxmod);
> +	box_module_unref(bxmod_old);
>  	return 0;
> +
> @@ -474,67 +420,51 @@ func_new(struct func_def *def)
>  	return func;
>  }
>  
> -static struct func_vtab func_c_vtab;
> -
> +/** Create new C function. */
>  static struct func *
>  func_c_new(MAYBE_UNUSED struct func_def *def)
>  {
>  	assert(def->language == FUNC_LANGUAGE_C);
>  	assert(def->body == NULL && !def->is_sandboxed);
> -	struct func_c *func = (struct func_c *) malloc(sizeof(struct func_c));
> -	if (func == NULL) {
> -		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
> +	struct func_c *func_c = malloc(sizeof(struct func_c));

8. You didn't need to change this line. The old name was just fine. It
wouldn't be fine if both 'func' and 'func_c' were used in here. But
there was only one name. The same in many other places in this patch.

Please, go through git diff and keep only the relevant changes. Even if
we would allow to do refactoring for the sake of refactoring, this diff
wouldn't fit here, because rename of 'func' to 'func_c' variables is not
relevant to `switch to module_cache interface` commit title. Both names
existed before the patch. The same can be argued about many other places.
So just don't make unnecessary/irrelevant diff.

Unless you absolutely want to fight over it, and only in a separate patch.
Don't mix it with functional changes.

> +	if (func_c == NULL) {
> +		diag_set(OutOfMemory, sizeof(*func_c), "malloc", "func_c");
>  		return NULL;
>  	}
> -	func->base.vtab = &func_c_vtab;
> -	func->func = NULL;
> -	func->bxmod = NULL;
> -	return &func->base;
> -}
> -
> -static void
> -func_c_unload(struct func_c *func)
> -{
> -	if (func->bxmod) {
> -		rlist_del(&func->item);
> -		if (rlist_empty(&func->bxmod->funcs)) {
> -			struct func_name name;
> -			func_split_name(func->base.def->name, &name);
> -			cache_del(name.package, name.package_end);
> -		}
> -		box_module_gc(func->bxmod);
> -	}
> -	func->bxmod = NULL;
> -	func->func = NULL;
> +	func_c_create(func_c);
> +	return &func_c->base;
>  }
>  
> +/** Destroy C function. */
>  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);
> +	struct func_c *func_c = (struct func_c *) base;
> +	box_module_unref(func_c->bxmod);
> +	func_c_unload(func_c);
>  	TRASH(base);
> -	free(func);
> +	free(func_c);
>  }
>  
>  /**
> - * Resolve func->func (find the respective DLL and fetch the
> + * Resolve func->func (find the respective share library and fetch the

9. The comment was just fine. Please. I beg you. Stop doing unnecessary
changes. Regardless of how many and often you do them, I will continue
asking to stop it. It does not make better literally anything. Not the
code except subjectively for you maybe, not the patch, not the history,
not the perf, not anything,

>   * symbol from it).
>   */
>  static int
> -func_c_load(struct func_c *func)
> +func_c_resolve(struct func_c *func_c)
>  {
> -	assert(func->func == NULL);
> +	assert(is_func_c_emtpy(func_c));
>  
>  	struct func_name name;
> -	func_split_name(func->base.def->name, &name);
> +	func_split_name(func_c->base.def->name, &name);
>  
>  	struct box_module *cached, *bxmod;
>  	cached = cache_find(name.package, name.package_end);
>  	if (cached == NULL) {
> -		bxmod = box_module_load(name.package, name.package_end);
> +		size_t len = name.package_end - name.package;
> +		bxmod = box_module_load(name.package, len);

10. You changed these lines literally in the previous patch by
another refactoring. Please, stop. Don't change code unless it
is necessary for consistency, bug fixing, or feature development.
Or serious refactoring, or optimizations. Here you just changed
`const char *, const char *` to `const char *, size_t`. It did
absolutely nothing except increasing the patch size and masking
the really mattering functional changes in this ocean of diff
hunks about endless renames, code block moves, arguments mixing
back and forth, and so on.

It is also totally not related to `switch to module_cache interface`
commit title.

Fitting into the same diff hunk as some really needed change,
does not make the other change atomic nor relevant. It is not
about changing things while you go alongside, like huricane.

Без негатива.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v19 5/6] box/cmod: implement cmod Lua module
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 5/6] box/cmod: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
@ 2021-03-08 22:51   ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-08 22:51 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml

Thanks for the patch!

Please, add a changelog file.

See 8 comments below.

> diff --git a/src/box/lua/cmod.c b/src/box/lua/cmod.c
> new file mode 100644
> index 000000000..ea5acd510
> --- /dev/null
> +++ b/src/box/lua/cmod.c
> @@ -0,0 +1,605 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <dlfcn.h>

1. You don't need dlfcn.h. I suspect some other headers
as well. For instance, eio.h, fcntl.h. Please, remove the
not needed headers.

> +#include <fcntl.h>
> +#include <lua.h>
> +
> +#include "box/error.h"
> +#include "box/port.h"
> +#include "box/func_def.h"
> +
> +#include "tt_static.h"
> +
> +#include "assoc.h"
> +#include "cmod.h"
> +#include "fiber.h"
> +#include "module_cache.h"
> +
> +#include "lua/utils.h"
> +#include "libeio/eio.h"
> +
> +/**
> + * Function descriptor.
> + */
> +struct cmod_func {
> +	/**
> +	 * C function to call.
> +	 */
> +	struct module_func mf;
> +	/** Number of references. */
> +	int64_t refs;
> +	/** Length of functon name in @a key. */
> +	size_t sym_len;
> +	/** Length of @a key. */
> +	size_t len;
> +	/** Function hash key. */
> +	char key[0];
> +};
> +
> +/** Function name to cmod_func hash. */
> +static struct mh_strnptr_t *cmod_func_hash = NULL;
> +
> +/** A type to find a module from an object. */
> +static const char *uname_cmod = "tt_uname_cmod";
> +
> +/** A type to find a function from an object. */
> +static const char *uname_func = "tt_uname_cmod_func";
> +
> +/** Get data associated with an object. */
> +static void *
> +get_udata(struct lua_State *L, const char *uname)
> +{
> +	void **pptr = luaL_testudata(L, 1, uname);
> +	return pptr != NULL ? *pptr : NULL;
> +}
> +
> +/** Set data to a new value. */
> +static void
> +set_udata(struct lua_State *L, const char *uname, void *ptr)
> +{
> +	void **pptr = luaL_testudata(L, 1, uname);
> +	assert(pptr != NULL);
> +	*pptr = ptr;
> +}
> +
> +/** Setup a new data and associate it with an object. */
> +static void
> +new_udata(struct lua_State *L, const char *uname, void *ptr)
> +{
> +	*(void **)lua_newuserdata(L, sizeof(void *)) = ptr;
> +	luaL_getmetatable(L, uname);
> +	lua_setmetatable(L, -2);
> +}
> +
> +/**
> + * Helpers for function cache.
> + */
> +static void *
> +cf_cache_find(const char *str, size_t len)

2. What is 'cf', and why did you use simply cache_find/add/del in
other patches but now you start adding prefixes?

> +{
> +	mh_int_t e = mh_strnptr_find_inp(cmod_func_hash, str, len);
> +	if (e == mh_end(cmod_func_hash))
> +		return NULL;
> +	return mh_strnptr_node(cmod_func_hash, e)->val;
> +}
> +
> +static int
> +cf_cache_add(struct cmod_func *cf)
> +{
> +	const struct mh_strnptr_node_t nd = {
> +		.str	= cf->key,
> +		.len	= cf->len,
> +		.hash	= mh_strn_hash(cf->key, cf->len),
> +		.val	= cf,
> +	};
> +	mh_int_t e = mh_strnptr_put(cmod_func_hash, &nd, NULL, NULL);
> +	if (e == mh_end(cmod_func_hash)) {
> +		diag_set(OutOfMemory, sizeof(nd), "malloc",
> +			 "cmod: hash node");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static void
> +cache_del(struct cmod_func *cf)
> +{
> +	mh_int_t e = mh_strnptr_find_inp(cmod_func_hash,
> +					 cf->key, cf->len);
> +	if (e != mh_end(cmod_func_hash))
> +		mh_strnptr_del(cmod_func_hash, e, NULL);
> +}
> +
> +/**
> + * Load a module.
> + *
> + * This function takes a module path from the caller
> + * stack @a L and returns cached module instance or
> + * creates a new module object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: module path is either not supplied
> + *   or not a string.
> + * - SystemError: unable to open a module due to a system error.
> + * - ClientError: a module does not exist.
> + * - OutOfMemory: unable to allocate a module.
> + *
> + * @returns module object on success or throws an error.
> + */
> +static int
> +lcmod_load(struct lua_State *L)

3. Why do you use 'struct lua_State' in this patch, and just
'lua_State' in the module_cache patch?

> +{
> +	const char msg_noname[] = "Expects cmod.load(\'name\') "
> +		"but no name passed";
> +
> +	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
> +		diag_set(IllegalParams, msg_noname);
> +		return luaT_error(L);
> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	if (name_len < 1) {
> +		diag_set(IllegalParams, msg_noname);
> +		return luaT_error(L);
> +	}
> +
> +	struct module *m = module_load(name, name_len);
> +	if (m == NULL)
> +		return luaT_error(L);
> +
> +	new_udata(L, uname_cmod, m);
> +	return 1;
> +}
> +
> +/**
> + * Unload a module.
> + *
> + * Take a module object from the caller stack @a L and unload it.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: module is not supplied.
> + * - IllegalParams: the module is unloaded.
> + *
> + * @returns true on success or throwns an error.
> + */
> +static int
> +lcmod_unload(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1) {
> +		diag_set(IllegalParams, "Expects module:unload()");
> +		return luaT_error(L);
> +	}
> +
> +	struct module *m = get_udata(L, uname_cmod);
> +	if (m == NULL) {
> +		diag_set(IllegalParams, "The module is unloaded");
> +		return luaT_error(L);
> +	}
> +
> +	set_udata(L, uname_cmod, NULL);
> +	module_unload(m);
> +	lua_pushboolean(L, true);
> +	return 1;
> +}
> +
> +/** Handle __index request for a module object. */
> +static int
> +lcmod_index(struct lua_State *L)
> +{
> +	lua_getmetatable(L, 1);
> +	lua_pushvalue(L, 2);
> +	lua_rawget(L, -2);
> +	if (!lua_isnil(L, -1))
> +		return 1;
> +
> +	struct module *m = get_udata(L, uname_cmod);
> +	if (m == NULL) {
> +		lua_pushnil(L);
> +		return 1;
> +	}
> +
> +	const char *key = lua_tostring(L, 2);
> +	if (key == NULL || lua_type(L, 2) != LUA_TSTRING) {
> +		diag_set(IllegalParams,
> +			 "Bad params, use __index(obj, <string>)");

4. Nobody ever uses __index explicitly like this. Better tell to
use "[key]".

> +		return luaT_error(L);
> +	}
> +
> +	if (strcmp(key, "path") == 0) {
> +		lua_pushstring(L, m->package);
> +		return 1;
> +	}
> +
> +	/*
> +	 * Internal keys for debug only, not API.
> +	 */
> +	if (strncmp(key, "tt_dev.", 7) == 0) {

5. This is a cool idea about retrieving info for the
tests, but we already have ERRINJ_DYN_MODULE_COUNT. It
helps to find if the modules are correctly allocated and
deleted, which makes not necessary to introduce 'refs'.

Talking of 'ptr', you don't need it either, if you
can see the module count. You just check the count
before and after module load. If it is incremented, a new
module was loaded.

If it is decremented, a module was unloaded.

For testing a single module used both from cmod and
box.schema.func you could add a global variable to your C
module and see that both APIs work with the same global
variable. For instance, add a function which increments it
and returns the old value. Then call it both from cmod and
box.schema.func, and see it is 2 in the end.

Or keep using your hacks, but at least make them easier
to use. By forcing to use `.` you make impossible to
write the indexes without ['...']. Make it easier, and no
need for 'tt' thing. For instance module.debug_refs.
func.debug_module_ptr, and so on. ['...'] looks bulky.

> +		const char *subkey = &key[7];
> +		if (strcmp(subkey, "refs") == 0) {
> +			lua_pushnumber(L, m->refs);
> +			return 1;
> +		} else if (strcmp(subkey, "ptr") == 0) {
> +			const char *s = tt_sprintf("%p", m);
> +			lua_pushstring(L, s);
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/** Module representation for REPL (console). */
> +static int
> +lcmod_serialize(struct lua_State *L)
> +{
> +	struct module *m = get_udata(L, uname_cmod);
> +	if (m == NULL) {
> +		lua_pushnil(L);
> +		return 1;
> +	}
> +
> +	lua_createtable(L, 0, 1);
> +	lua_pushstring(L, m->package);
> +	lua_setfield(L, -2, "path");
> +	return 1;
> +}
> +
> +/** Collect a module. */
> +static int
> +lcmod_gc(struct lua_State *L)
> +{
> +	struct module *m = get_udata(L, uname_cmod);
> +	if (m != NULL) {
> +		set_udata(L, uname_cmod, NULL);
> +		module_unload(m);
> +	}
> +	return 0;
> +}
> +
> +/** Increase reference to a function. */
> +static void
> +cmod_func_ref(struct cmod_func *cf)
> +{
> +	assert(cf->refs >= 0);
> +	++cf->refs;
> +}
> +
> +/** Free function memory. */
> +static void
> +cmod_func_delete(struct cmod_func *cf)
> +{

6. Please, add an assertion it does not keep a reference at
module * anymore.

> +	TRASH(cf);
> +	free(cf);
> +}
> +
> +/** Unreference a function and free if last one. */
> +static void
> +cmod_func_unref(struct cmod_func *cf)
> +{
> +	assert(cf->refs > 0);
> +	if (--cf->refs == 0) {
> +		module_func_unload(&cf->mf);
> +		cache_del(cf);
> +		cmod_func_delete(cf);
> +	}
> +}
> +
> +/** Function name from a hash key. */
> +static char *
> +cmod_func_name(struct cmod_func *cf)
> +{
> +	return &cf->key[cf->len - cf->sym_len];
> +}
> +
> +/**
> + * Allocate a new function instance and resolve its address.
> + *
> + * @param m a module the function should be loaded from.
> + * @param key function hash key, ie "addr.module.foo".
> + * @param len length of @a key.
> + * @param sym_len function symbol name length, ie 3 for "foo".
> + *
> + * @returns function instance on success, NULL otherwise (diag is set).
> + */
> +static struct cmod_func *
> +cmod_func_new(struct module *m, const char *key, size_t len, size_t sym_len)
> +{
> +	size_t size = sizeof(struct cmod_func) + len + 1;
> +	struct cmod_func *cf = malloc(size);
> +	if (cf == NULL) {
> +		diag_set(OutOfMemory, size, "malloc", "cf");
> +		return NULL;
> +	}
> +
> +	cf->len = len;
> +	cf->sym_len = sym_len;
> +	cf->refs = 0;
> +
> +	module_func_create(&cf->mf);
> +	memcpy(cf->key, key, len);
> +	cf->key[len] = '\0';
> +
> +	if (module_func_load(m, cmod_func_name(cf), &cf->mf) != 0) {
> +		diag_set(ClientError, ER_LOAD_FUNCTION,
> +			 cmod_func_name(cf), dlerror());
> +		cmod_func_delete(cf);
> +		return NULL;
> +	}
> +
> +	if (cf_cache_add(cf) != 0) {
> +		cmod_func_delete(cf);

7. You didn't unload the function.

> +		return NULL;
> +	}
> +
> +	/*
> +	 * Each new function depends on module presence.
> +	 * Module will reside even if been unload
> +	 * explicitly after the function creation.
> +	 */
> +	cmod_func_ref(cf);
> +	return cf;
> +}
> +
> +/** Initialize cmod. */
> +void
> +box_lua_cmod_init(struct lua_State *L)
> +{
> +	cmod_func_hash = mh_strnptr_new();
> +	if (cmod_func_hash == NULL)
> +		panic("cmod: Can't allocate func hash table");
> +
> +	(void)L;

8. No need to cast to void. It is used.

> +	static const struct luaL_Reg top_methods[] = {
> +		{ "load",		lcmod_load		},
> +		{ NULL, NULL },
> +	};
> +	luaL_register_module(L, "cmod", top_methods);
> +	lua_pop(L, 1);
> +
> +	static const struct luaL_Reg lcmod_methods[] = {
> +		{ "unload",		lcmod_unload		},
> +		{ "load",		lcmod_load_func		},
> +		{ "__index",		lcmod_index		},
> +		{ "__serialize",	lcmod_serialize		},
> +		{ "__gc",		lcmod_gc		},
> +		{ NULL, NULL },
> +	};
> +	luaL_register_type(L, uname_cmod, lcmod_methods);
> +
> +	static const struct luaL_Reg lfunc_methods[] = {
> +		{ "unload",		lfunc_unload		},
> +		{ "__index",		lfunc_index		},
> +		{ "__serialize",	lfunc_serialize		},
> +		{ "__gc",		lfunc_gc		},
> +		{ "__call",		lfunc_call		},
> +		{ NULL, NULL },
> +	};
> +	luaL_register_type(L, uname_func, lfunc_methods);
> +}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v19 6/6] test: box/cfunc -- add cmod test
  2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 6/6] test: box/cfunc -- add cmod test Cyrill Gorcunov via Tarantool-patches
@ 2021-03-08 22:51   ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-08 22:51 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml

Thanks for the patch!

On 01.03.2021 22:23, Cyrill Gorcunov wrote:
> Note that the test is disabled in suite.ini for a while
> because we need to update test-run first like

You can send a patch for test-run, and rebase your branch
on it. Then in the end Kirill will push test-run first, will
rebase your branch on master, and will push it too.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-03-08 22:52 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-01 21:23 [Tarantool-patches] [PATCH v19 0/6] box: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 1/6] box/func: module_reload -- drop redundant argument Cyrill Gorcunov via Tarantool-patches
2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 2/6] box/func: prepare for transition to modules subsystem Cyrill Gorcunov via Tarantool-patches
2021-03-08 22:44   ` Vladislav Shpilevoy via Tarantool-patches
2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 3/6] box/module_cache: introduce " Cyrill Gorcunov via Tarantool-patches
2021-03-08 22:45   ` Vladislav Shpilevoy via Tarantool-patches
2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 4/6] box/func: switch to module_cache interface Cyrill Gorcunov via Tarantool-patches
2021-03-08 22:47   ` Vladislav Shpilevoy via Tarantool-patches
2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 5/6] box/cmod: implement cmod Lua module Cyrill Gorcunov via Tarantool-patches
2021-03-08 22:51   ` Vladislav Shpilevoy via Tarantool-patches
2021-03-01 21:23 ` [Tarantool-patches] [PATCH v19 6/6] test: box/cfunc -- add cmod test Cyrill Gorcunov via Tarantool-patches
2021-03-08 22:51   ` Vladislav Shpilevoy via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git