[Tarantool-patches] [PATCH v5 0/6] box/cbox: implement cfunc Lua module

[Tarantool-patches] [PATCH v5 0/6] box/cbox: implement cfunc Lua module

[Cyrill Gorcunov]

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

The series implements a bare minimum. 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

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

Cyrill Gorcunov (6):
  box/func: factor out c function entry structure
  box/func: provide module_sym_call
  box/func: more detailed error in module reloading
  box/func: export func_split_name helper
  box/cbox: implement cbox Lua module
  test: box/cfunc -- add simple module test

 src/box/CMakeLists.txt  |   1 +
 src/box/box.cc          |   4 +
 src/box/func.c          | 249 ++++++++++++------------
 src/box/func.h          |  78 ++++++++
 src/box/lua/cbox.c      | 417 ++++++++++++++++++++++++++++++++++++++++
 src/box/lua/cbox.h      |  39 ++++
 src/box/lua/init.c      |   2 +
 test/box/CMakeLists.txt |   2 +
 test/box/cbox.result    | 134 +++++++++++++
 test/box/cbox.test.lua  |  51 +++++
 test/box/cfunc1.c       |  49 +++++
 test/box/cfunc2.c       | 112 +++++++++++
 test/box/suite.ini      |   2 +-
 13 files changed, 1009 insertions(+), 131 deletions(-)
 create mode 100644 src/box/lua/cbox.c
 create mode 100644 src/box/lua/cbox.h
 create mode 100644 test/box/cbox.result
 create mode 100644 test/box/cbox.test.lua
 create mode 100644 test/box/cfunc1.c
 create mode 100644 test/box/cfunc2.c


base-commit: 0dc72812fb78a192945612f0e954026a0ffe4053
-- 
2.26.2

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

[Cyrill Gorcunov]

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

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

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

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

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

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

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/func.c | 156 +++++++++++++++++++++++--------------------------
 src/box/func.h |  43 ++++++++++++++
 2 files changed, 116 insertions(+), 83 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 8087c953f..75d2abb5f 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -59,19 +59,9 @@ struct func_name {
 struct func_c {
 	/** Function object base class. */
 	struct func base;
-	/**
-	 * Anchor for module membership.
-	 */
-	struct rlist item;
-	/**
-	 * For C functions, the body of the function.
-	 */
-	box_function_f func;
-	/**
-	 * Each stored function keeps a handle to the
-	 * dynamic library for the C callback.
-	 */
-	struct module *module;
+
+	/** C function module symbol. */
+	struct module_sym mod_sym;
 };
 
 /***
@@ -371,6 +361,53 @@ module_sym(struct module *module, const char *name)
 	return f;
 }
 
+int
+module_sym_load(struct module_sym *mod_sym)
+{
+	assert(mod_sym->addr == NULL);
+
+	struct func_name name;
+	func_split_name(mod_sym->name, &name);
+
+	struct module *module = module_cache_find(name.package,
+						  name.package_end);
+	if (module == NULL) {
+		/* Try to find loaded module in the cache */
+		module = module_load(name.package, name.package_end);
+		if (module == NULL)
+			return -1;
+		if (module_cache_put(module)) {
+			module_delete(module);
+			return -1;
+		}
+	}
+
+	mod_sym->addr = module_sym(module, name.sym);
+	if (mod_sym->addr == NULL)
+		return -1;
+	mod_sym->module = module;
+	rlist_add(&module->funcs, &mod_sym->item);
+	return 0;
+}
+
+void
+module_sym_unload(struct module_sym *mod_sym)
+{
+	if (mod_sym->module == NULL)
+		return;
+
+	rlist_del(&mod_sym->item);
+	if (rlist_empty(&mod_sym->module->funcs)) {
+		struct func_name name;
+		func_split_name(mod_sym->name, &name);
+		module_cache_del(name.package, name.package_end);
+	}
+	module_gc(mod_sym->module);
+
+	mod_sym->module = NULL;
+	mod_sym->addr = NULL;
+}
+
 int
 module_reload(const char *package, const char *package_end, struct module **module)
 {
@@ -385,15 +422,15 @@ module_reload(const char *package, const char *package_end, struct module **modu
 	if (new_module == NULL)
 		return -1;
 
-	struct func_c *func, *tmp_func;
-	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
+	struct module_sym *mod_sym, *tmp;
+	rlist_foreach_entry_safe(mod_sym, &old_module->funcs, item, tmp) {
 		struct func_name name;
-		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(new_module, name.sym);
-		if (func->func == NULL)
+		func_split_name(mod_sym->name, &name);
+		mod_sym->addr = module_sym(new_module, name.sym);
+		if (mod_sym->addr == NULL)
 			goto restore;
-		func->module = new_module;
-		rlist_move(&new_module->funcs, &func->item);
+		mod_sym->module = new_module;
+		rlist_move(&new_module->funcs, &mod_sym->item);
 	}
 	module_cache_del(package, package_end);
 	if (module_cache_put(new_module) != 0)
@@ -408,9 +445,9 @@ module_reload(const char *package, const char *package_end, struct module **modu
 	 */
 	do {
 		struct func_name name;
-		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(old_module, name.sym);
-		if (func->func == NULL) {
+		func_split_name(mod_sym->name, &name);
+		mod_sym->addr = module_sym(old_module, name.sym);
+		if (mod_sym->addr == NULL) {
 			/*
 			 * Something strange was happen, an early loaden
 			 * function was not found in an old dso.
@@ -418,10 +455,11 @@ module_reload(const char *package, const char *package_end, struct module **modu
 			panic("Can't restore module function, "
 			      "server state is inconsistent");
 		}
-		func->module = old_module;
-		rlist_move(&old_module->funcs, &func->item);
-	} while (func != rlist_first_entry(&old_module->funcs,
-					   struct func_c, item));
+		mod_sym->module = old_module;
+		rlist_move(&old_module->funcs, &mod_sym->item);
+	} while (mod_sym != rlist_first_entry(&old_module->funcs,
+					      struct module_sym,
+					      item));
 	assert(rlist_empty(&new_module->funcs));
 	module_delete(new_module);
 	return -1;
@@ -484,79 +522,31 @@ func_c_new(MAYBE_UNUSED struct func_def *def)
 		return NULL;
 	}
 	func->base.vtab = &func_c_vtab;
-	func->func = NULL;
-	func->module = NULL;
+	func->mod_sym.addr = NULL;
+	func->mod_sym.module = NULL;
+	func->mod_sym.name = def->name;
 	return &func->base;
 }
 
-static void
-func_c_unload(struct func_c *func)
-{
-	if (func->module) {
-		rlist_del(&func->item);
-		if (rlist_empty(&func->module->funcs)) {
-			struct func_name name;
-			func_split_name(func->base.def->name, &name);
-			module_cache_del(name.package, name.package_end);
-		}
-		module_gc(func->module);
-	}
-	func->module = NULL;
-	func->func = NULL;
-}
-
 static void
 func_c_destroy(struct func *base)
 {
 	assert(base->vtab == &func_c_vtab);
 	assert(base != NULL && base->def->language == FUNC_LANGUAGE_C);
 	struct func_c *func = (struct func_c *) base;
-	func_c_unload(func);
+	module_sym_unload(&func->mod_sym);
 	TRASH(base);
 	free(func);
 }
 
-/**
- * Resolve func->func (find the respective DLL and fetch the
- * symbol from it).
- */
-static int
-func_c_load(struct func_c *func)
-{
-	assert(func->func == NULL);
-
-	struct func_name name;
-	func_split_name(func->base.def->name, &name);
-
-	struct module *module = module_cache_find(name.package,
-						  name.package_end);
-	if (module == NULL) {
-		/* Try to find loaded module in the cache */
-		module = module_load(name.package, name.package_end);
-		if (module == NULL)
-			return -1;
-		if (module_cache_put(module)) {
-			module_delete(module);
-			return -1;
-		}
-	}
-
-	func->func = module_sym(module, name.sym);
-	if (func->func == NULL)
-		return -1;
-	func->module = module;
-	rlist_add(&module->funcs, &func->item);
-	return 0;
-}
-
 int
 func_c_call(struct func *base, struct port *args, struct port *ret)
 {
 	assert(base->vtab == &func_c_vtab);
 	assert(base != NULL && base->def->language == FUNC_LANGUAGE_C);
 	struct func_c *func = (struct func_c *) base;
-	if (func->func == NULL) {
-		if (func_c_load(func) != 0)
+	if (func->mod_sym.addr == NULL) {
+		if (module_sym_load(&func->mod_sym) != 0)
 			return -1;
 	}
 
@@ -571,10 +561,10 @@ func_c_call(struct func *base, struct port *args, struct port *ret)
 	box_function_ctx_t ctx = { ret };
 
 	/* Module can be changed after function reload. */
-	struct module *module = func->module;
+	struct module *module = func->mod_sym.module;
 	assert(module != NULL);
 	++module->calls;
-	int rc = func->func(&ctx, data, data + data_sz);
+	int rc = func->mod_sym.addr(&ctx, data, data + data_sz);
 	--module->calls;
 	module_gc(module);
 	region_truncate(region, region_svp);
diff --git a/src/box/func.h b/src/box/func.h
index 581e468cb..acf2df9a9 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -58,6 +58,29 @@ struct module {
 	char package[0];
 };
 
+/**
+ * Callable symbol bound to a module.
+ */
+struct module_sym {
+	/**
+	 * Anchor for module membership.
+	 */
+	struct rlist item;
+	/**
+	 * For C functions, address of the function.
+	 */
+	box_function_f addr;
+	/**
+	 * Each stored function keeps a handle to the
+	 * dynamic library for the C callback.
+	 */
+	struct module *module;
+	/**
+	 * Function name definition.
+	 */
+	char *name;
+};
+
 /** Virtual method table for func object. */
 struct func_vtab {
 	/** Call function with given arguments. */
@@ -108,6 +131,26 @@ func_delete(struct func *func);
 int
 func_call(struct func *func, struct port *args, struct port *ret);
 
+/**
+ * Resolve C entry (find the respective DLL and fetch the
+ * symbol from it).
+ *
+ * @param mod_sym module symbol pointer.
+ * @retval -1 on error.
+ * @retval 0 on success.
+ */
+int
+module_sym_load(struct module_sym *mod_sym);
+
+/**
+ * Unload module symbol and drop it from the package
+ * cache if there is no users left.
+ *
+ * @param mod_sym module symbol pointer.
+ */
+void
+module_sym_unload(struct module_sym *mod_sym);
+
 /**
  * Reload dynamically loadable module.
  *
-- 
2.26.2

[Tarantool-patches] [PATCH v5 2/6] box/func: provide module_sym_call

[Cyrill Gorcunov]

This helpers allow to execute a function bound to
module symbol without claiming to provide credentials
and being registered in "_func" space.

This will be needed to execute unregistered C functions.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/func.c | 78 +++++++++++++++++++++++++++++---------------------
 src/box/func.h | 14 +++++++++
 2 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 75d2abb5f..452866a5d 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -408,6 +408,50 @@ module_sym_unload(struct module_sym *mod_sym)
 	mod_sym->addr = NULL;
 }
 
+int
+module_sym_call(struct module_sym *mod_sym, struct port *args, struct port *ret)
+{
+	if (mod_sym->addr == NULL) {
+		if (module_sym_load(mod_sym) != 0)
+			return -1;
+	}
+
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+	uint32_t data_sz;
+	const char *data = port_get_msgpack(args, &data_sz);
+	if (data == NULL)
+		return -1;
+
+	port_c_create(ret);
+	box_function_ctx_t ctx = {
+		.port = ret,
+	};
+
+	/*
+	 * Module can be changed after function reload. Also
+	 * keep in mind that stored C procedure may yield inside.
+	 */
+	struct module *module = mod_sym->module;
+	assert(module != NULL);
+	++module->calls;
+	int rc = mod_sym->addr(&ctx, data, data + data_sz);
+	--module->calls;
+	module_gc(module);
+	region_truncate(region, region_svp);
+
+	if (rc != 0) {
+		if (diag_last_error(&fiber()->diag) == NULL) {
+			/* Stored procedure forget to set diag  */
+			diag_set(ClientError, ER_PROC_C, "unknown error");
+		}
+		port_destroy(ret);
+		return -1;
+	}
+
+	return rc;
+}
+
 int
 module_reload(const char *package, const char *package_end, struct module **module)
 {
@@ -544,39 +588,9 @@ func_c_call(struct func *base, struct port *args, struct port *ret)
 {
 	assert(base->vtab == &func_c_vtab);
 	assert(base != NULL && base->def->language == FUNC_LANGUAGE_C);
-	struct func_c *func = (struct func_c *) base;
-	if (func->mod_sym.addr == NULL) {
-		if (module_sym_load(&func->mod_sym) != 0)
-			return -1;
-	}
-
-	struct region *region = &fiber()->gc;
-	size_t region_svp = region_used(region);
-	uint32_t data_sz;
-	const char *data = port_get_msgpack(args, &data_sz);
-	if (data == NULL)
-		return -1;
-
-	port_c_create(ret);
-	box_function_ctx_t ctx = { ret };
 
-	/* Module can be changed after function reload. */
-	struct module *module = func->mod_sym.module;
-	assert(module != NULL);
-	++module->calls;
-	int rc = func->mod_sym.addr(&ctx, data, data + data_sz);
-	--module->calls;
-	module_gc(module);
-	region_truncate(region, region_svp);
-	if (rc != 0) {
-		if (diag_last_error(&fiber()->diag) == NULL) {
-			/* Stored procedure forget to set diag  */
-			diag_set(ClientError, ER_PROC_C, "unknown error");
-		}
-		port_destroy(ret);
-		return -1;
-	}
-	return rc;
+	struct func_c *func = (struct func_c *) base;
+	return module_sym_call(&func->mod_sym, args, ret);
 }
 
 static struct func_vtab func_c_vtab = {
diff --git a/src/box/func.h b/src/box/func.h
index acf2df9a9..35f4e320c 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -151,6 +151,20 @@ module_sym_load(struct module_sym *mod_sym);
 void
 module_sym_unload(struct module_sym *mod_sym);
 
+/**
+ * Execute a module symbol call.
+ *
+ * @param mod_sym module symbol pointer.
+ * @param args arguments to pass to the callee.
+ * @param ret execution results passed to the caller.
+ *
+ * @retval -1 on error.
+ * @retval 0 on success.
+ */
+int
+module_sym_call(struct module_sym *mod_sym, struct port *args,
+		struct port *ret);
+
 /**
  * Reload dynamically loadable module.
  *
-- 
2.26.2

[Tarantool-patches] [PATCH v5 3/6] box/func: more detailed error in module reloading

[Cyrill Gorcunov]

Otherwise it is unclear what exactly been failing.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/func.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/box/func.c b/src/box/func.c
index 452866a5d..89796310f 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -471,8 +471,11 @@ module_reload(const char *package, const char *package_end, struct module **modu
 		struct func_name name;
 		func_split_name(mod_sym->name, &name);
 		mod_sym->addr = module_sym(new_module, name.sym);
-		if (mod_sym->addr == NULL)
+		if (mod_sym->addr == NULL) {
+			say_error("module: reload %s, symbol %s not found",
+				  package, name.sym);
 			goto restore;
+		}
 		mod_sym->module = new_module;
 		rlist_move(&new_module->funcs, &mod_sym->item);
 	}
-- 
2.26.2

[Tarantool-patches] [PATCH v5 4/6] box/func: export func_split_name helper

[Cyrill Gorcunov]

To reuse this functionality in next patch.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/func.c | 20 +-------------------
 src/box/func.h | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 89796310f..ba98f0f9e 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -44,18 +44,6 @@
 #include <fcntl.h>
 #include <dlfcn.h>
 
-/**
- * Parsed symbol and package names.
- */
-struct func_name {
-	/** Null-terminated symbol name, e.g. "func" for "mod.submod.func" */
-	const char *sym;
-	/** Package name, e.g. "mod.submod" for "mod.submod.func" */
-	const char *package;
-	/** A pointer to the last character in ->package + 1 */
-	const char *package_end;
-};
-
 struct func_c {
 	/** Function object base class. */
 	struct func base;
@@ -64,13 +52,7 @@ struct func_c {
 	struct module_sym mod_sym;
 };
 
-/***
- * Split function name to symbol and package names.
- * For example, str = foo.bar.baz => sym = baz, package = foo.bar
- * @param str function name, e.g. "module.submodule.function".
- * @param[out] name parsed symbol and package names.
- */
-static void
+void
 func_split_name(const char *str, struct func_name *name)
 {
 	name->package = str;
diff --git a/src/box/func.h b/src/box/func.h
index 35f4e320c..92caef9cc 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -107,6 +107,27 @@ struct func {
 	struct access access[BOX_USER_MAX];
 };
 
+/**
+ * Parsed symbol and package names.
+ */
+struct func_name {
+	/** Null-terminated symbol name, e.g. "func" for "mod.submod.func" */
+	const char *sym;
+	/** Package name, e.g. "mod.submod" for "mod.submod.func" */
+	const char *package;
+	/** A pointer to the last character in ->package + 1 */
+	const char *package_end;
+};
+
+/***
+ * Split function name to symbol and package names.
+ * For example, str = foo.bar.baz => sym = baz, package = foo.bar
+ * @param str function name, e.g. "module.submodule.function".
+ * @param[out] name parsed symbol and package names.
+ */
+void
+func_split_name(const char *str, struct func_name *name);
+
 /**
  * Initialize modules subsystem.
  */
-- 
2.26.2

[Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

[Cyrill Gorcunov]

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

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

Fixes #4692

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

@TarantoolBot document
Title: cbox module

Overview
========

`cbox` module provides a way to create, delete and execute
`C` procedures. Unlinke `box.schema.func` functionality this
functions are not persistent in any space and live purely in
memory, thus once node get turned off they are vanished. Same
time such functions are allowed to be created and executed
when a node is in read-only mode.

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

`cbox.func.create(name) -> obj | nil, err`
------------------------------------------

Create a new function with name `name`.

Possible errors:
 - IllegalParams: function name is either not supplied
   or not a string.
 - IllegalParams: a function is already created.
 - OutOfMemory: unable to allocate a function.

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

Example:

``` Lua
f, err = require('cbox').create('func')
if not f then
    print(err)
end
-- call the function
f(arg1,arg2,...)
```

`cbox.func.drop(name) -> true | nil, err`
-----------------------------------------

Deletes a function with name `name`.

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

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

Example:

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

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

Reloads a module with name `name` and all functions which
were associated for the module.

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

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

Example:

``` Lua
ok, err = require('cbox').module.reload('func')
if not ok then
    print(err)
end
```
---
 src/box/CMakeLists.txt |   1 +
 src/box/box.cc         |   4 +
 src/box/lua/cbox.c     | 417 +++++++++++++++++++++++++++++++++++++++++
 src/box/lua/cbox.h     |  39 ++++
 src/box/lua/init.c     |   2 +
 5 files changed, 463 insertions(+)
 create mode 100644 src/box/lua/cbox.c
 create mode 100644 src/box/lua/cbox.h

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 8b2e704cf..358b0ad39 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -196,6 +196,7 @@ add_library(box STATIC
     lua/init.c
     lua/call.c
     lua/cfg.cc
+    lua/cbox.c
     lua/console.c
     lua/serialize_lua.c
     lua/tuple.c
diff --git a/src/box/box.cc b/src/box/box.cc
index 6ec813c12..512b22c2f 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -75,6 +75,7 @@
 #include "systemd.h"
 #include "call.h"
 #include "func.h"
+#include "lua/cbox.h"
 #include "sequence.h"
 #include "sql_stmt_cache.h"
 #include "msgpack.h"
@@ -2647,6 +2648,9 @@ box_init(void)
 	if (module_init() != 0)
 		diag_raise();
 
+	if (cbox_init() != 0)
+		diag_raise();
+
 	if (tuple_init(lua_hash) != 0)
 		diag_raise();
 
diff --git a/src/box/lua/cbox.c b/src/box/lua/cbox.c
new file mode 100644
index 000000000..58585b44d
--- /dev/null
+++ b/src/box/lua/cbox.c
@@ -0,0 +1,417 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#include <string.h>
+#include <lua.h>
+
+#include "tt_static.h"
+#include "assoc.h"
+#include "diag.h"
+#include "port.h"
+#include "say.h"
+
+#include "box/error.h"
+#include "box/func.h"
+#include "box/call.h"
+#include "box/port.h"
+
+#include "trivia/util.h"
+#include "lua/utils.h"
+
+/** Function names to descriptor map. */
+static struct mh_strnptr_t *func_hash;
+
+/**
+ * Function descriptor.
+ */
+struct cbox_func {
+	/**
+	 * Symbol descriptor for the function in
+	 * an associated module.
+	 */
+	struct module_sym mod_sym;
+
+	/** Function name length */
+	size_t name_len;
+
+	/** Function name */
+	char name[0];
+};
+
+/**
+ * Find function descriptor by its name.
+ */
+struct cbox_func *
+cbox_func_find(const char *name, size_t len)
+{
+	if (name != NULL && len > 0) {
+		mh_int_t e = mh_strnptr_find_inp(func_hash, name, len);
+		if (e != mh_end(func_hash))
+			return mh_strnptr_node(func_hash, e)->val;
+	}
+	return NULL;
+}
+
+/**
+ * Add function descriptor to the cache.
+ */
+static int
+cbox_func_add(struct cbox_func *cfunc)
+{
+	const struct mh_strnptr_node_t nd = {
+		.str	= cfunc->name,
+		.len	= cfunc->name_len,
+		.hash	= mh_strn_hash(cfunc->name, cfunc->name_len),
+		.val	= cfunc,
+	};
+
+	mh_int_t h = mh_strnptr_put(func_hash, &nd, NULL, NULL);
+	if (h == mh_end(func_hash)) {
+		diag_set(OutOfMemory, sizeof(nd), __func__, "h");
+		return -1;
+	}
+	return 0;
+}
+
+/**
+ * Setup a detailed error description into the diagnostics area.
+ */
+static void
+diag_set_param_error(struct lua_State *L, int idx, const char *func_name,
+		     const char *param, const char *exp)
+{
+	const char *typename = idx == 0 ?
+		"<unknown>" : lua_typename(L, lua_type(L, idx));
+	static const char *fmt =
+		"%s: wrong parameter \"%s\": expected %s, got %s";
+	diag_set(IllegalParams, fmt, func_name, param, exp, typename);
+}
+
+/**
+ * Call function by its name from the Lua code.
+ */
+static int
+lcbox_func_call(struct lua_State *L)
+{
+	int nr_args = lua_gettop(L);
+
+	/*
+	 * A function name is associated with the variable.
+	 */
+	lua_getfield(L, -nr_args, "name");
+	size_t name_len;
+	const char *name = lua_tolstring(L, -1, &name_len);
+	struct cbox_func *cf = cbox_func_find(name, name_len);
+	/*
+	 * Do not pop the result early, since Lua's GC may
+	 * drop the name string found.
+	 */
+	lua_pop(L, 1);
+
+	if (cf == NULL) {
+		/*
+		 * This can happen if someone changed the
+		 * name of the function manually.
+		 */
+		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
+		diag_set(IllegalParams, fmt, name);
+		return luaT_push_nil_and_error(L);
+	}
+
+	/*
+	 * FIXME: We should get rid of luaT_newthread but this
+	 * requires serious modifucations. In particular
+	 * port_lua_do_dump uses tarantool_L reference and
+	 * coro_ref must be valid as well.
+	 */
+	lua_State *args_L = luaT_newthread(tarantool_L);
+	if (args_L == NULL)
+		return luaT_push_nil_and_error(L);
+
+	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+	lua_xmove(L, args_L, lua_gettop(L) - 1);
+
+	struct port args;
+	port_lua_create(&args, args_L);
+	((struct port_lua *)&args)->ref = coro_ref;
+
+	struct port ret;
+	if (module_sym_call(&cf->mod_sym, &args, &ret) != 0) {
+		port_destroy(&args);
+		return luaT_push_nil_and_error(L);
+	}
+
+	int top = lua_gettop(L);
+	lua_pushboolean(L, true);
+	port_dump_lua(&ret, L, true);
+	int cnt = lua_gettop(L) - top;
+
+	port_destroy(&ret);
+	port_destroy(&args);
+
+	return cnt;
+}
+
+/**
+ * Create a new function.
+ *
+ * This function takes a function name from the caller
+ * stack @a L and creates a new function object.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: function name is either not supplied
+ *   or not a string.
+ * - IllegalParams: a function is already created.
+ * - OutOfMemory: unable to allocate a function.
+ *
+ * @returns function object on success or {nil,error} on error,
+ * the error is set to the diagnostics area.
+ */
+static int
+lcbox_func_create(struct lua_State *L)
+{
+	const char *method = "cbox.func.create";
+	struct cbox_func *cf = NULL;
+
+	if (lua_gettop(L) != 1) {
+		const char *fmt = "Expects %s(\'name\')";
+		diag_set(IllegalParams, fmt, method);
+		goto out;
+	}
+
+	if (lua_type(L, 1) != LUA_TSTRING) {
+		diag_set_param_error(L, 1, method,
+				     lua_tostring(L, 1),
+				     "function name");
+		goto out;
+	}
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+
+	if (cbox_func_find(name, name_len) != NULL) {
+		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
+		diag_set(IllegalParams, fmt, name);
+		goto out;
+	}
+
+	size_t size = sizeof(*cf) + name_len + 1;
+	cf = malloc(size);
+	if (cf == NULL) {
+		diag_set(OutOfMemory, size, "malloc", "cf");
+		goto out;
+	}
+
+	cf->mod_sym.addr	= NULL;
+	cf->mod_sym.module	= NULL;
+	cf->mod_sym.name	= cf->name;
+	cf->name_len		= name_len;
+
+	memcpy(cf->name, name, name_len);
+	cf->name[name_len] = '\0';
+
+	if (cbox_func_add(cf) != 0)
+		goto out;
+
+	/*
+	 * A new variable should be callable for
+	 * convenient use in Lua.
+	 */
+	lua_newtable(L);
+
+	lua_pushstring(L, "name");
+	lua_pushstring(L, cf->name);
+	lua_settable(L, -3);
+
+	lua_newtable(L);
+
+	lua_pushstring(L, "__call");
+	lua_pushcfunction(L, lcbox_func_call);
+	lua_settable(L, -3);
+
+	lua_setmetatable(L, -2);
+	return 1;
+
+out:
+	free(cf);
+	return luaT_push_nil_and_error(L);
+}
+
+/**
+ * Drop a function.
+ *
+ * This function takes a function name from the caller
+ * stack @a L and drops a function object.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: function name is either not supplied
+ *   or not a string.
+ * - IllegalParams: the function does not exist.
+ *
+ * @returns true on success or {nil,error} on error,
+ * the error is set to the diagnostics area.
+ */
+static int
+lcbox_func_drop(struct lua_State *L)
+{
+	const char *method = "cbox.func.drop";
+	const char *name = NULL;
+
+	if (lua_gettop(L) != 1) {
+		const char *fmt = "expects %s(\'name\')";
+		diag_set(IllegalParams, fmt, method);
+		return luaT_push_nil_and_error(L);
+	}
+
+	if (lua_type(L, 1) != LUA_TSTRING) {
+		diag_set_param_error(L, 1, method,
+				     lua_tostring(L, 1),
+				     "function name or id");
+		return luaT_push_nil_and_error(L);
+	}
+
+	size_t name_len;
+	name = lua_tolstring(L, 1, &name_len);
+
+	mh_int_t e = mh_strnptr_find_inp(func_hash, name, name_len);
+	if (e == mh_end(func_hash)) {
+		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
+		diag_set(IllegalParams, fmt, name);
+		return luaT_push_nil_and_error(L);
+	}
+
+	struct cbox_func *cf = mh_strnptr_node(func_hash, e)->val;
+	mh_strnptr_del(func_hash, e, NULL);
+	free(cf);
+
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/**
+ * Reload a module.
+ *
+ * This function takes a module name from the caller
+ * stack @a L and reloads all functions associated with
+ * the module.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: module name is either not supplied
+ *   or not a string.
+ * - IllegalParams: the function does not exist.
+ * - ClientError: a module with the name provided does
+ *   not exist.
+ *
+ * @returns true on success or {nil,error} on error,
+ * the error is set to the diagnostics area.
+ */
+static int
+lcbox_module_reload(struct lua_State *L)
+{
+	const char *method = "cbox.module.reload";
+	const char *fmt = "Expects %s(\'name\') but no name passed";
+
+	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
+		diag_set(IllegalParams, fmt, method);
+		return luaT_push_nil_and_error(L);
+	}
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+	if (name == NULL || name_len < 1) {
+		diag_set(IllegalParams, fmt, method);
+		return luaT_push_nil_and_error(L);
+	}
+
+	struct module *module = NULL;
+	if (module_reload(name, &name[name_len], &module) == 0) {
+		if (module != NULL) {
+			lua_pushboolean(L, true);
+			return 1;
+		}
+		diag_set(ClientError, ER_NO_SUCH_MODULE, name);
+	}
+	return luaT_push_nil_and_error(L);
+}
+
+/**
+ * Initialize cbox Lua module.
+ *
+ * @param L Lua state where to register the cbox module.
+ */
+void
+box_lua_cbox_init(struct lua_State *L)
+{
+	static const struct luaL_Reg cbox_methods[] = {
+		{ NULL, NULL }
+	};
+	luaL_register_module(L, "cbox", cbox_methods);
+
+	/* func table */
+	static const struct luaL_Reg func_table[] = {
+		{ "create",	lcbox_func_create },
+		{ "drop",	lcbox_func_drop },
+	};
+
+	lua_newtable(L);
+	for (size_t i = 0; i < lengthof(func_table); i++) {
+		lua_pushstring(L, func_table[i].name);
+		lua_pushcfunction(L, func_table[i].func);
+		lua_settable(L, -3);
+	}
+	lua_setfield(L, -2, "func");
+
+	/* module table */
+	static const struct luaL_Reg module_table[] = {
+		{ "reload",	lcbox_module_reload },
+	};
+
+	lua_newtable(L);
+	for (size_t i = 0; i < lengthof(module_table); i++) {
+		lua_pushstring(L, module_table[i].name);
+		lua_pushcfunction(L, module_table[i].func);
+		lua_settable(L, -3);
+	}
+	lua_setfield(L, -2, "module");
+
+	lua_pop(L, 1);
+}
+
+/**
+ * Initialize cbox module.
+ *
+ * @return 0 on success, -1 on error (diag is set).	
+ */
+int
+cbox_init(void)
+{
+	func_hash = mh_strnptr_new();
+	if (func_hash == NULL) {
+		diag_set(OutOfMemory, sizeof(*func_hash), "malloc",
+			  "cbox.func hash table");
+		return -1;
+	}
+	return 0;
+}
+
+/**
+ * Free cbox module.
+ */
+void
+cbox_free(void)
+{
+	while (mh_size(func_hash) > 0) {
+		mh_int_t e = mh_first(func_hash);
+		struct cbox_func *cf = mh_strnptr_node(func_hash, e)->val;
+		module_sym_unload(&cf->mod_sym);
+		mh_strnptr_del(func_hash, e, NULL);
+	}
+	mh_strnptr_delete(func_hash);
+	func_hash = NULL;
+}
diff --git a/src/box/lua/cbox.h b/src/box/lua/cbox.h
new file mode 100644
index 000000000..1e6cd9a9b
--- /dev/null
+++ b/src/box/lua/cbox.h
@@ -0,0 +1,39 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#pragma once
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct lua_State;
+
+/**
+ * Initialize cbox Lua module.
+ *
+ * @param L Lua state where to register the cbox module.
+ */
+void
+box_lua_cbox_init(struct lua_State *L);
+
+/**
+ * Initialize cbox module.
+ *
+ * @return 0 on success, -1 on error (diag is set).	
+ */
+int
+cbox_init(void);
+
+/**
+ * Free cbox module.
+ */
+void
+cbox_free(void);
+
+#if defined(__cplusplus)
+}
+#endif /* defined(__plusplus) */
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index d0316ef86..b37aa284a 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -61,6 +61,7 @@
 #include "box/lua/cfg.h"
 #include "box/lua/xlog.h"
 #include "box/lua/console.h"
+#include "box/lua/cbox.h"
 #include "box/lua/tuple.h"
 #include "box/lua/execute.h"
 #include "box/lua/key_def.h"
@@ -466,6 +467,7 @@ box_lua_init(struct lua_State *L)
 	box_lua_tuple_init(L);
 	box_lua_call_init(L);
 	box_lua_cfg_init(L);
+	box_lua_cbox_init(L);
 	box_lua_slab_init(L);
 	box_lua_index_init(L);
 	box_lua_space_init(L);
-- 
2.26.2

[Tarantool-patches] [PATCH v5 6/6] test: box/cfunc -- add simple module test

[Cyrill Gorcunov]

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

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

ie need to enable cbox module.

Part-of #4642

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

diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 06bfbbe9d..2afbeadc3 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -3,3 +3,5 @@ build_module(function1 function1.c)
 build_module(reload1 reload1.c)
 build_module(reload2 reload2.c)
 build_module(tuple_bench tuple_bench.c)
+build_module(cfunc1 cfunc1.c)
+build_module(cfunc2 cfunc2.c)
diff --git a/test/box/cbox.result b/test/box/cbox.result
new file mode 100644
index 000000000..a8c242983
--- /dev/null
+++ b/test/box/cbox.result
@@ -0,0 +1,134 @@
+-- test-run result file version 2
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+
+cbox = require('cbox')
+ | ---
+ | ...
+fio = require('fio')
+ | ---
+ | ...
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+ | ---
+ | ...
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext
+ | ---
+ | ...
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext
+ | ---
+ | ...
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc1_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+--
+-- They all are sitting in cfunc.so
+cfunc_nop = cbox.func.create('cfunc.cfunc_nop')
+ | ---
+ | ...
+cfunc_fetch_evens = cbox.func.create('cfunc.cfunc_fetch_evens')
+ | ---
+ | ...
+cfunc_multireturn = cbox.func.create('cfunc.cfunc_multireturn')
+ | ---
+ | ...
+cfunc_args = cbox.func.create('cfunc.cfunc_args')
+ | ---
+ | ...
+
+--
+-- Make sure they all are callable
+cfunc_nop()
+ | ---
+ | - true
+ | ...
+cfunc_fetch_evens()
+ | ---
+ | - true
+ | ...
+cfunc_multireturn()
+ | ---
+ | - true
+ | ...
+cfunc_args()
+ | ---
+ | - true
+ | ...
+
+--
+-- Clean old function references and reload a new one.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc2_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+cbox.module.reload('cfunc')
+ | ---
+ | - true
+ | ...
+
+cfunc_nop()
+ | ---
+ | - true
+ | ...
+cfunc_multireturn()
+ | ---
+ | - true
+ | - [1]
+ | - [1]
+ | ...
+cfunc_fetch_evens({2,4,6})
+ | ---
+ | - true
+ | ...
+cfunc_fetch_evens({1,2,3})  -- error
+ | ---
+ | - null
+ | - invalid argument 1 != 2
+ | ...
+cfunc_args(1, "hello")
+ | ---
+ | - true
+ | - [1, 'hello']
+ | ...
+--
+-- Clean it up
+cbox.func.drop('cfunc.cfunc_nop')
+ | ---
+ | - true
+ | ...
+cbox.func.drop('cfunc.cfunc_fetch_evens')
+ | ---
+ | - true
+ | ...
+cbox.func.drop('cfunc.cfunc_multireturn')
+ | ---
+ | - true
+ | ...
+cbox.func.drop('cfunc.cfunc_args')
+ | ---
+ | - true
+ | ...
+
+--
+-- Cleanup the generated symlink
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
diff --git a/test/box/cbox.test.lua b/test/box/cbox.test.lua
new file mode 100644
index 000000000..8cf8b63a6
--- /dev/null
+++ b/test/box/cbox.test.lua
@@ -0,0 +1,51 @@
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+cbox = require('cbox')
+fio = require('fio')
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext
+
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc1_path, cfunc_path)
+
+--
+-- They all are sitting in cfunc.so
+cfunc_nop = cbox.func.create('cfunc.cfunc_nop')
+cfunc_fetch_evens = cbox.func.create('cfunc.cfunc_fetch_evens')
+cfunc_multireturn = cbox.func.create('cfunc.cfunc_multireturn')
+cfunc_args = cbox.func.create('cfunc.cfunc_args')
+
+--
+-- Make sure they all are callable
+cfunc_nop()
+cfunc_fetch_evens()
+cfunc_multireturn()
+cfunc_args()
+
+--
+-- Clean old function references and reload a new one.
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc2_path, cfunc_path)
+
+cbox.module.reload('cfunc')
+
+cfunc_nop()
+cfunc_multireturn()
+cfunc_fetch_evens({2,4,6})
+cfunc_fetch_evens({1,2,3})  -- error
+cfunc_args(1, "hello")
+--
+-- Clean it up
+cbox.func.drop('cfunc.cfunc_nop')
+cbox.func.drop('cfunc.cfunc_fetch_evens')
+cbox.func.drop('cfunc.cfunc_multireturn')
+cbox.func.drop('cfunc.cfunc_args')
+
+--
+-- Cleanup the generated symlink
+_ = pcall(fio.unlink(cfunc_path))
diff --git a/test/box/cfunc1.c b/test/box/cfunc1.c
new file mode 100644
index 000000000..50c979d63
--- /dev/null
+++ b/test/box/cfunc1.c
@@ -0,0 +1,49 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Before the reload functions are just declared
+ * and simply exit with zero.
+ *
+ * After the module reload we should provide real
+ * functionality.
+ */
+
+int
+cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
+
+int
+cfunc_fetch_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
+
+int
+cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
+
+int
+cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
diff --git a/test/box/cfunc2.c b/test/box/cfunc2.c
new file mode 100644
index 000000000..7a4ece843
--- /dev/null
+++ b/test/box/cfunc2.c
@@ -0,0 +1,112 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Just make sure we've been called.
+ */
+int
+cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void)ctx;
+	(void)args;
+	(void)args_end;
+	return 0;
+}
+
+/*
+ * Fetch first N even numbers (just to make sure the order of
+ * arguments is not screwed).
+ */
+int
+cfunc_fetch_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	int arg_count = mp_decode_array(&args);
+	if (arg_count != 1) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+			"invalid argument count");
+	}
+	int field_count = mp_decode_array(&args);
+	if (field_count < 1) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+			"invalid array size");
+	}
+
+	for (int i = 0; i < field_count; i++) {
+		int val = mp_decode_uint(&args);
+		int needed = 2 * (i+1);
+		if (val != needed) {
+			char res[128];
+			snprintf(res, sizeof(res), "%s %d != %d",
+				 "invalid argument", val, needed);
+			return box_error_set(__FILE__, __LINE__,
+					     ER_PROC_C, "%s", res);
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Return one element array twice.
+ */
+int
+cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	char tuple_buf[512];
+	char *d = tuple_buf;
+	d = mp_encode_array(d, 1);
+	d = mp_encode_uint(d, 1);
+	assert(d <= tuple_buf + sizeof(tuple_buf));
+
+	box_tuple_format_t *fmt = box_tuple_format_default();
+	box_tuple_t *tuple_a = box_tuple_new(fmt, tuple_buf, d);
+	if (tuple_a == NULL)
+		return -1;
+	int rc = box_return_tuple(ctx, tuple_a);
+	if (rc != 0)
+		return rc;
+	return box_return_tuple(ctx, tuple_a);
+}
+
+/*
+ * Encode int + string pair back.
+ */
+int
+cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	uint32_t arg_count = mp_decode_array(&args);
+	if (arg_count != 2) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+			"invalid argument count");
+	}
+
+	if (mp_typeof(*args) != MP_UINT) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+			"tuple field must be uint");
+	}
+	uint32_t num = mp_decode_uint(&args);
+
+	if (mp_typeof(*args) != MP_STR) {
+		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+			"tuple field must be string");
+	}
+	const char *str = args;
+	uint32_t len = mp_decode_strl(&str);
+
+	char tuple_buf[512];
+	char *d = tuple_buf;
+	d = mp_encode_array(d, 2);
+	d = mp_encode_uint(d, num);
+	d = mp_encode_str(d, str, len);
+	assert(d <= tuple_buf + sizeof(tuple_buf));
+
+	box_tuple_format_t *fmt = box_tuple_format_default();
+	box_tuple_t *tuple = box_tuple_new(fmt, tuple_buf, d);
+	if (tuple == NULL)
+		return -1;
+
+	return box_return_tuple(ctx, tuple);
+}
diff --git a/test/box/suite.ini b/test/box/suite.ini
index 793b8bf76..95fa6e660 100644
--- a/test/box/suite.ini
+++ b/test/box/suite.ini
@@ -2,7 +2,7 @@
 core = tarantool
 description = Database tests
 script = box.lua
-disabled = rtree_errinj.test.lua tuple_bench.test.lua
+disabled = rtree_errinj.test.lua tuple_bench.test.lua cbox.test.lua
 long_run = huge_field_map_long.test.lua
 config = engine.cfg
 release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua gh-4648-func-load-unload.test.lua
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

[Vladislav Shpilevoy]

Hi! Thanks for the patch! I didn't review it properly yet. Just
a few (3) small comments before I will dive into details.

> diff --git a/src/box/lua/cbox.c b/src/box/lua/cbox.c
> new file mode 100644
> index 000000000..58585b44d
> --- /dev/null
> +++ b/src/box/lua/cbox.c
> @@ -0,0 +1,417 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <string.h>
> +#include <lua.h>
> +
> +#include "tt_static.h"
> +#include "assoc.h"
> +#include "diag.h"
> +#include "port.h"
> +#include "say.h"
> +
> +#include "box/error.h"
> +#include "box/func.h"

1. Why do you still depend on box/func.h? Func.h is
about stored functions with credentials and stuff. In the
previous review I proposed to move needed parts to a new
system: module_sym of whatever it is called. So it would
be module_sym.h and module_sym.c. And it would not depend
on schema at all. Currently func.h brings schema dependency
into cbox.

Maybe a good name for the new system would be module_cache.h
and module_cache.c. Like we have with coll_id_cache.h/.c.
(The structs can remain module_sym.)

> +#include "box/call.h"
> +#include "box/port.h"
> +
> +#include "trivia/util.h"
> +#include "lua/utils.h"
> +
> +/**
> + * Call function by its name from the Lua code.
> + */
> +static int
> +lcbox_func_call(struct lua_State *L)
> +{
> +	int nr_args = lua_gettop(L);
> +
> +	/*
> +	 * A function name is associated with the variable.
> +	 */
> +	lua_getfield(L, -nr_args, "name");
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, -1, &name_len);
> +	struct cbox_func *cf = cbox_func_find(name, name_len);
> +	/*
> +	 * Do not pop the result early, since Lua's GC may
> +	 * drop the name string found.
> +	 */
> +	lua_pop(L, 1);
> +
> +	if (cf == NULL) {
> +		/*
> +		 * This can happen if someone changed the
> +		 * name of the function manually.
> +		 */
> +		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
> +		diag_set(IllegalParams, fmt, name);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	/*
> +	 * FIXME: We should get rid of luaT_newthread but this
> +	 * requires serious modifucations. In particular
> +	 * port_lua_do_dump uses tarantool_L reference and
> +	 * coro_ref must be valid as well.
> +	 */
> +	lua_State *args_L = luaT_newthread(tarantool_L);
> +	if (args_L == NULL)
> +		return luaT_push_nil_and_error(L);
> +
> +	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
> +	lua_xmove(L, args_L, lua_gettop(L) - 1);

2. Why didn't you use lua_remove() to get rid of the unnecessary
parts of the stack?

> +
> +	struct port args;
> +	port_lua_create(&args, args_L);
> +	((struct port_lua *)&args)->ref = coro_ref;
> +
> +	struct port ret;
> +	if (module_sym_call(&cf->mod_sym, &args, &ret) != 0) {
> +		port_destroy(&args);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	int top = lua_gettop(L);
> +	lua_pushboolean(L, true);
> +	port_dump_lua(&ret, L, true);
> +	int cnt = lua_gettop(L) - top;
> +
> +	port_destroy(&ret);
> +	port_destroy(&args);
> +
> +	return cnt;
> +}
> +
> +/**
> + * Create a new function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and creates a new function object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - IllegalParams: a function is already created.
> + * - OutOfMemory: unable to allocate a function.
> + *
> + * @returns function object on success or {nil,error} on error,
> + * the error is set to the diagnostics area.
> + */
> +static int
> +lcbox_func_create(struct lua_State *L)
> +{
> +	const char *method = "cbox.func.create";
> +	struct cbox_func *cf = NULL;
> +
> +	if (lua_gettop(L) != 1) {
> +		const char *fmt = "Expects %s(\'name\')";
> +		diag_set(IllegalParams, fmt, method);
> +		goto out;
> +	}
> +
> +	if (lua_type(L, 1) != LUA_TSTRING) {
> +		diag_set_param_error(L, 1, method,
> +				     lua_tostring(L, 1),
> +				     "function name");
> +		goto out;
> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	if (cbox_func_find(name, name_len) != NULL) {
> +		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
> +		diag_set(IllegalParams, fmt, name);
> +		goto out;
> +	}
> +
> +	size_t size = sizeof(*cf) + name_len + 1;
> +	cf = malloc(size);
> +	if (cf == NULL) {
> +		diag_set(OutOfMemory, size, "malloc", "cf");
> +		goto out;
> +	}
> +
> +	cf->mod_sym.addr	= NULL;
> +	cf->mod_sym.module	= NULL;
> +	cf->mod_sym.name	= cf->name;
> +	cf->name_len		= name_len;
> +
> +	memcpy(cf->name, name, name_len);
> +	cf->name[name_len] = '\0';
> +
> +	if (cbox_func_add(cf) != 0)
> +		goto out;
> +
> +	/*
> +	 * A new variable should be callable for
> +	 * convenient use in Lua.
> +	 */
> +	lua_newtable(L);
> +
> +	lua_pushstring(L, "name");
> +	lua_pushstring(L, cf->name);
> +	lua_settable(L, -3);
> +
> +	lua_newtable(L);
> +
> +	lua_pushstring(L, "__call");
> +	lua_pushcfunction(L, lcbox_func_call);
> +	lua_settable(L, -3);
> +
> +	lua_setmetatable(L, -2);

3. What happens when the function object is deleted by Lua GC?
It seems the user can't get the function again without dropping
it.

For instance, consider this:

	box.cfg{}
	cbox = require('cbox')
	function callpush()
	    local f = cbox.func.create('function1.test_push')
	    return f({1})
	end
	callpush()
	collectgarbage('collect')
	callpush()

The second 'callpush()' will fail. And calling drop() after
each usage also looks bad - super slow because will unload the
module potentially, and won't work if some other function will
throw an exception beforehand.

To fix it I need to drop the 'test_push' function using
cbox.func.drop() right before I call create(). This looks
counter-intuitive.

So here I see 2 options how to fix it:

	- Implement a GC handler, which will drop the function, when its
	  Lua object is deleted by GC.

Or

	- Add option table to func.create, where a single option so far
	  would be {if_exists = <boolean>}. Like we do for _func space
	  in schema.lua. So users will need to call if_exists = false to
	  either load the function or get an existing ref.

Another possibility: replace create() and drop() with load() and unload(),
and add these 3 steps:

	1. Load() called on the same function name will just return the same
	  object. The object is referenced. In GC handler it is unreferenced.

	2. Unload() marks the function to be removed. If the number of loaded
	  Lua refs to this function is > 0, do nothing. If 0, delete now from
	  the hash and probably unload the entire module.

	3. Load() in GC handler checks if this is the last ref dropped. If yes,
	  remove from the hash, and probably unload the module.

Note, that even if a function was loaded, but then its ref counter becomes 0,
it is not unloaded until explicit unload() called. We really-really don't want
to load/unload the module too often, it is a super slow operation.

I personally think the second way looks better. But we can discuss. I am
not sure on 100% that it is correct.

> +	return 1;
> +
> +out:
> +	free(cf);
> +	return luaT_push_nil_and_error(L);
> +}

Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

[Cyrill Gorcunov]

On Fri, Oct 09, 2020 at 12:35:09AM +0200, Vladislav Shpilevoy wrote:
> > +
> > +#include "box/error.h"
> > +#include "box/func.h"
> 
> 1. Why do you still depend on box/func.h? Func.h is
> about stored functions with credentials and stuff. In the
> previous review I proposed to move needed parts to a new
> system: module_sym of whatever it is called. So it would
> be module_sym.h and module_sym.c. And it would not depend
> on schema at all. Currently func.h brings schema dependency
> into cbox.

Well, I remember your proposal. The reason is indeed to include
module_sym structure which sits in func.h. I thought we could
make this separation later together with fix of lazy address
loading. But on the other hands let me try to implement it
today.

> Maybe a good name for the new system would be module_cache.h
> and module_cache.c. Like we have with coll_id_cache.h/.c.
> (The structs can remain module_sym.)

Sounds reasonable.

> > +
> > +	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
> > +	lua_xmove(L, args_L, lua_gettop(L) - 1);
> 
> 2. Why didn't you use lua_remove() to get rid of the unnecessary
> parts of the stack?

Which one you think is redundant? The object itself? If so then
it sits as a bottom element of the stack and removing it would
simply make code slower.

Vlad this "prepare environment for calling" part of code is very
sensitive to the arguments, I tried a various combinations in
attempts to get rid of lua thread and none of them work. So I
left it as is and put a FIXME, because it requires more serious
rework.

Or I misundertood you, and you meant something different?

> > +
> > +	lua_pushstring(L, "__call");
> > +	lua_pushcfunction(L, lcbox_func_call);
> > +	lua_settable(L, -3);
> > +
> > +	lua_setmetatable(L, -2);
> 
> 3. What happens when the function object is deleted by Lua GC?
> It seems the user can't get the function again without dropping
> it.

And that is a good question. I must confess I missed Lua GC aspect
here. Thanks a huge! I'll rework!

Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

[Vladislav Shpilevoy]

>>> +
>>> +	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
>>> +	lua_xmove(L, args_L, lua_gettop(L) - 1);
>>
>> 2. Why didn't you use lua_remove() to get rid of the unnecessary
>> parts of the stack?
> 
> Which one you think is redundant? The object itself? If so then
> it sits as a bottom element of the stack and removing it would
> simply make code slower.

Yes, I see the problem now. First issue is that you can remove
the root argument, because it may triggers its GC. Second issue
is that you still need luaT_newthread(), because port_lua uses
tarantool_L. Both issues can be easily fixed in port_lua.

https://github.com/tarantool/tarantool/issues/5406

> Vlad this "prepare environment for calling" part of code is very
> sensitive to the arguments, I tried a various combinations in
> attempts to get rid of lua thread and none of them work. So I
> left it as is and put a FIXME, because it requires more serious
> rework.

FIXME don't work, they are never fixed after creation unless you
have a ticket. Although these times even a ticket tends to be sent
to the wishlist for years.

>>> +
>>> +	lua_pushstring(L, "__call");
>>> +	lua_pushcfunction(L, lcbox_func_call);
>>> +	lua_settable(L, -3);
>>> +
>>> +	lua_setmetatable(L, -2);
>>
>> 3. What happens when the function object is deleted by Lua GC?
>> It seems the user can't get the function again without dropping
>> it.
> 
> And that is a good question. I must confess I missed Lua GC aspect
> here. Thanks a huge! I'll rework!

Another thing to look at - on each call you perform a lookup in the
function hash. But you don't really need it. You can store the
function pointer in a hidden field. For example, in __index metatable
of the function object. Or inside the function object with _ prefix,
so at least it won't be visible for autocompletion. Or turn the
function object into cdata. Anyway you will need it to properly
implement a GC handler. And this cdata would be a struct
lbox_sym/lbox_func/whatever with the function pointer in it.

Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

[Vladislav Shpilevoy]

Thanks for the patch!

See 10 comments below.

> @TarantoolBot document
> Title: cbox module
> 
> Overview
> ========
> 
> `cbox` module provides a way to create, delete and execute
> `C` procedures. Unlinke `box.schema.func` functionality this
> functions are not persistent in any space and live purely in
> memory, thus once node get turned off they are vanished. Same
> time such functions are allowed to be created and executed
> when a node is in read-only mode.
> 
> Module functions
> ================
> 
> `cbox.func.create(name) -> obj | nil, err`
> ------------------------------------------
> 
> Create a new function with name `name`.

1. I hope this is a draft. Because in the final version it would be
better to explain what does it mean to 'create' a function (or 'load'
in the next version, I guess). That it will load user's dynamic lib
on the first function call. It does not create anything, just loads
an existing function from a shared file.

> Possible errors:
>  - IllegalParams: function name is either not supplied
>    or not a string.
>  - IllegalParams: a function is already created.
>  - OutOfMemory: unable to allocate a function.
> 
> On success the new callable object is returned, otherwise
> `nil,error` pair.
> 
> Example:
> 
> ``` Lua
> f, err = require('cbox').create('func')
> if not f then
>     print(err)
> end
> -- call the function
> f(arg1,arg2,...)
> ```
> 
> `cbox.func.drop(name) -> true | nil, err`
> -----------------------------------------
> 
> Deletes a function with name `name`.
> 
> Possible errors:
>  - IllegalParams: function name is either not supplied
>    or not a string.
>  - IllegalParams: the function does not exist.
> 
> On success `true` is returned, otherwise `nil,error` pair.
> 
> Example:
> 
> ``` Lua
> ok, err = require('cbox').func.drop('func')
> if not ok then
>     print(err)
> end
> ```
> 
> `cbox.module.reload(name) -> true | nil, err`
> ---------------------------------------------
> 
> Reloads a module with name `name` and all functions which
> were associated for the module.

2. What happens with the currently working functions? What
happens, if there are still non-deleted refs to the old
functions? What if all refs are deleted, but not on all
functions was called unload()?

> Possible errors:
>  - IllegalParams: module name is either not supplied
>    or not a string.
>  - IllegalParams: the function does not exist.
>  - ClientError: a module with the name provided does
>    not exist.
> 
> On success `true` is returned, otherwise `nil,error` pair.
> 
> Example:
> 
> ``` Lua
> ok, err = require('cbox').module.reload('func')
> if not ok then
>     print(err)
> end
> ```

3. It seems the function invocation itself is not documented?

> diff --git a/src/box/lua/cbox.c b/src/box/lua/cbox.c
> new file mode 100644
> index 000000000..58585b44d
> --- /dev/null
> +++ b/src/box/lua/cbox.c
> @@ -0,0 +1,417 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <string.h>
> +#include <lua.h>
> +
> +#include "tt_static.h"

4. Why do you need tt_static.h? You don't use any of tt_*
functions here.

> +#include "assoc.h"
> +#include "diag.h"
> +#include "port.h"
> +#include "say.h"

5. You don't use say_* functions either.

> +
> +#include "box/error.h"
> +#include "box/func.h"
> +#include "box/call.h"
> +#include "box/port.h"
> +
> +#include "trivia/util.h"
> +#include "lua/utils.h"
> +
> +/** Function names to descriptor map. */
> +static struct mh_strnptr_t *func_hash;
> +
> +/**
> + * Function descriptor.
> + */
> +struct cbox_func {
> +	/**
> +	 * Symbol descriptor for the function in
> +	 * an associated module.
> +	 */
> +	struct module_sym mod_sym;
> +
> +	/** Function name length */
> +	size_t name_len;
> +
> +	/** Function name */
> +	char name[0];
> +};
> +
> +/**
> + * Find function descriptor by its name.
> + */
> +struct cbox_func *
> +cbox_func_find(const char *name, size_t len)

6. I assume the function should be 'static'. It is never
used out of cbox.c.

> +{
> +	if (name != NULL && len > 0) {
> +		mh_int_t e = mh_strnptr_find_inp(func_hash, name, len);
> +		if (e != mh_end(func_hash))
> +			return mh_strnptr_node(func_hash, e)->val;
> +	}
> +	return NULL;
> +}
> +
> +/**
> + * Add function descriptor to the cache.
> + */
> +static int
> +cbox_func_add(struct cbox_func *cfunc)
> +{
> +	const struct mh_strnptr_node_t nd = {
> +		.str	= cfunc->name,
> +		.len	= cfunc->name_len,
> +		.hash	= mh_strn_hash(cfunc->name, cfunc->name_len),
> +		.val	= cfunc,
> +	};
> +
> +	mh_int_t h = mh_strnptr_put(func_hash, &nd, NULL, NULL);
> +	if (h == mh_end(func_hash)) {
> +		diag_set(OutOfMemory, sizeof(nd), __func__, "h");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * Setup a detailed error description into the diagnostics area.
> + */
> +static void
> +diag_set_param_error(struct lua_State *L, int idx, const char *func_name,
> +		     const char *param, const char *exp)
> +{
> +	const char *typename = idx == 0 ?
> +		"<unknown>" : lua_typename(L, lua_type(L, idx));
> +	static const char *fmt =
> +		"%s: wrong parameter \"%s\": expected %s, got %s";

7. Please, lets omit 'static' when it is not needed. It just raises questions.

> +	diag_set(IllegalParams, fmt, func_name, param, exp, typename);
> +}
> +
> +/**
> + * Call function by its name from the Lua code.
> + */
> +static int
> +lcbox_func_call(struct lua_State *L)
> +{
> +	int nr_args = lua_gettop(L);

8. It is not really a number of arguments. It includes the function
object itself.

> +
> +	/*
> +	 * A function name is associated with the variable.
> +	 */
> +	lua_getfield(L, -nr_args, "name");
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, -1, &name_len);
> +	struct cbox_func *cf = cbox_func_find(name, name_len);
> +	/*
> +	 * Do not pop the result early, since Lua's GC may
> +	 * drop the name string found.
> +	 */
> +	lua_pop(L, 1);
> +
> +	if (cf == NULL) {
> +		/*
> +		 * This can happen if someone changed the
> +		 * name of the function manually.
> +		 */
> +		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
> +		diag_set(IllegalParams, fmt, name);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	/*
> +	 * FIXME: We should get rid of luaT_newthread but this
> +	 * requires serious modifucations. In particular

9. modifucations -> modifications.

> +	 * port_lua_do_dump uses tarantool_L reference and
> +	 * coro_ref must be valid as well.
> +	 */
> +	lua_State *args_L = luaT_newthread(tarantool_L);
> +	if (args_L == NULL)
> +		return luaT_push_nil_and_error(L);
> +
> +	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
> +	lua_xmove(L, args_L, lua_gettop(L) - 1);
> +
> +	struct port args;
> +	port_lua_create(&args, args_L);
> +	((struct port_lua *)&args)->ref = coro_ref;
> +
> +	struct port ret;
> +	if (module_sym_call(&cf->mod_sym, &args, &ret) != 0) {
> +		port_destroy(&args);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	int top = lua_gettop(L);
> +	lua_pushboolean(L, true);
> +	port_dump_lua(&ret, L, true);
> +	int cnt = lua_gettop(L) - top;
> +
> +	port_destroy(&ret);
> +	port_destroy(&args);
> +
> +	return cnt;
> +}
> +
> +/**
> + * Create a new function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and creates a new function object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - IllegalParams: a function is already created.
> + * - OutOfMemory: unable to allocate a function.
> + *
> + * @returns function object on success or {nil,error} on error,
> + * the error is set to the diagnostics area.
> + */
> +static int
> +lcbox_func_create(struct lua_State *L)
> +{
> +	const char *method = "cbox.func.create";
> +	struct cbox_func *cf = NULL;
> +
> +	if (lua_gettop(L) != 1) {
> +		const char *fmt = "Expects %s(\'name\')";
> +		diag_set(IllegalParams, fmt, method);
> +		goto out;
> +	}
> +
> +	if (lua_type(L, 1) != LUA_TSTRING) {
> +		diag_set_param_error(L, 1, method,
> +				     lua_tostring(L, 1),
> +				     "function name");
> +		goto out;
> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	if (cbox_func_find(name, name_len) != NULL) {
> +		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
> +		diag_set(IllegalParams, fmt, name);
> +		goto out;
> +	}
> +
> +	size_t size = sizeof(*cf) + name_len + 1;
> +	cf = malloc(size);
> +	if (cf == NULL) {
> +		diag_set(OutOfMemory, size, "malloc", "cf");
> +		goto out;
> +	}
> +
> +	cf->mod_sym.addr	= NULL;
> +	cf->mod_sym.module	= NULL;
> +	cf->mod_sym.name	= cf->name;
> +	cf->name_len		= name_len;
> +
> +	memcpy(cf->name, name, name_len);
> +	cf->name[name_len] = '\0';
> +
> +	if (cbox_func_add(cf) != 0)
> +		goto out;
> +
> +	/*
> +	 * A new variable should be callable for
> +	 * convenient use in Lua.
> +	 */
> +	lua_newtable(L);
> +
> +	lua_pushstring(L, "name");
> +	lua_pushstring(L, cf->name);
> +	lua_settable(L, -3);
> +
> +	lua_newtable(L);
> +
> +	lua_pushstring(L, "__call");
> +	lua_pushcfunction(L, lcbox_func_call);
> +	lua_settable(L, -3);
> +
> +	lua_setmetatable(L, -2);
> +	return 1;
> +
> +out:
> +	free(cf);
> +	return luaT_push_nil_and_error(L);
> +}
> +
> +/**
> + * Drop a function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and drops a function object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - IllegalParams: the function does not exist.
> + *
> + * @returns true on success or {nil,error} on error,
> + * the error is set to the diagnostics area.
> + */
> +static int
> +lcbox_func_drop(struct lua_State *L)
> +{
> +	const char *method = "cbox.func.drop";
> +	const char *name = NULL;
> +
> +	if (lua_gettop(L) != 1) {
> +		const char *fmt = "expects %s(\'name\')";
> +		diag_set(IllegalParams, fmt, method);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	if (lua_type(L, 1) != LUA_TSTRING) {
> +		diag_set_param_error(L, 1, method,
> +				     lua_tostring(L, 1),
> +				     "function name or id");
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	size_t name_len;
> +	name = lua_tolstring(L, 1, &name_len);
> +
> +	mh_int_t e = mh_strnptr_find_inp(func_hash, name, name_len);
> +	if (e == mh_end(func_hash)) {
> +		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
> +		diag_set(IllegalParams, fmt, name);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	struct cbox_func *cf = mh_strnptr_node(func_hash, e)->val;
> +	mh_strnptr_del(func_hash, e, NULL);
> +	free(cf);

10. I would advise to wrap it into cbox_func_delete(). I am 100% sure
we will add more deletion things to the function object soon.

> +
> +	lua_pushboolean(L, true);
> +	return 1;
> +}

Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

[Cyrill Gorcunov]

On Fri, Oct 09, 2020 at 11:31:12PM +0200, Vladislav Shpilevoy wrote:
> >>> +
> >>> +	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
> >>> +	lua_xmove(L, args_L, lua_gettop(L) - 1);
> >>
> >> 2. Why didn't you use lua_remove() to get rid of the unnecessary
> >> parts of the stack?
> > 
> > Which one you think is redundant? The object itself? If so then
> > it sits as a bottom element of the stack and removing it would
> > simply make code slower.
> 
> Yes, I see the problem now. First issue is that you can remove
> the root argument, because it may triggers its GC. Second issue
> is that you still need luaT_newthread(), because port_lua uses
> tarantool_L. Both issues can be easily fixed in port_lua.
> 
> https://github.com/tarantool/tarantool/issues/5406

Thanks for making an issue but i've a serious doubt about this "easily
fixed" statement.

> > Vlad this "prepare environment for calling" part of code is very
> > sensitive to the arguments, I tried a various combinations in
> > attempts to get rid of lua thread and none of them work. So I
> > left it as is and put a FIXME, because it requires more serious
> > rework.
> 
> FIXME don't work, they are never fixed after creation unless you
> have a ticket. Although these times even a ticket tends to be sent
> to the wishlist for years.

We've a ticket now. Moreover the use of newthread is definitely not
a blocker for the functionality. We've been using it in _func space
for years. As to me -- unitifaction of "call" procedure looks like
a separate task which is of course related to the cbox but we started
implementing cbox code not to eliminate "thread" call in first place
or optimize a code flow. But I agree we've to do something.

> >>> +
> >>> +	lua_pushstring(L, "__call");
> >>> +	lua_pushcfunction(L, lcbox_func_call);
> >>> +	lua_settable(L, -3);
> >>> +
> >>> +	lua_setmetatable(L, -2);
> >>
> >> 3. What happens when the function object is deleted by Lua GC?
> >> It seems the user can't get the function again without dropping
> >> it.
> > 
> > And that is a good question. I must confess I missed Lua GC aspect
> > here. Thanks a huge! I'll rework!
> 
> Another thing to look at - on each call you perform a lookup in the
> function hash. But you don't really need it. You can store the
> function pointer in a hidden field. For example, in __index metatable
> of the function object. Or inside the function object with _ prefix,
> so at least it won't be visible for autocompletion. Or turn the
> function object into cdata. Anyway you will need it to properly
> implement a GC handler. And this cdata would be a struct
> lbox_sym/lbox_func/whatever with the function pointer in it.

I though about it as well but must confess I don't like this approach:
it means the memory allocated by our cbox code then passed to luajit
as a variable and then we always "trust" this object. I'll think about
more. Still the mark about GC handler is correct, thanks!

Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

[Cyrill Gorcunov]

On Fri, Oct 09, 2020 at 11:46:20PM +0200, Vladislav Shpilevoy wrote:
> > 
> > `cbox.func.create(name) -> obj | nil, err`
> > ------------------------------------------
> > 
> > Create a new function with name `name`.
> 
> 1. I hope this is a draft. Because in the final version it would be
> better to explain what does it mean to 'create' a function (or 'load'
> in the next version, I guess). That it will load user's dynamic lib
> on the first function call. It does not create anything, just loads
> an existing function from a shared file.

Yes, this is for review, I'll create more detailed description.

> > `cbox.module.reload(name) -> true | nil, err`
> > ---------------------------------------------
> > 
> > Reloads a module with name `name` and all functions which
> > were associated for the module.
> 
> 2. What happens with the currently working functions? What
> happens, if there are still non-deleted refs to the old
> functions? What if all refs are deleted, but not on all
> functions was called unload()?

Old functions remains active until explicitly unloaded. We
load symbols with RTLD_LOCAL thus they are not interfere
when ld.so resolves the tables. probably i should add more
details into documentation.

> 3. It seems the function invocation itself is not documented?

Good point, seems this would be useful. I though 'cause this
is mostly an alias for "_func" space it would be obvious. But
indeed without real example the doc looks incomplete. Thanks!

> > +
> > +#include "tt_static.h"
> 
> 4. Why do you need tt_static.h? You don't use any of tt_*
> functions here.

Silly me, tnt_errcode_desc doesn't use ststic buffers. Will
drop, thanks!

> 
> > +#include "assoc.h"
> > +#include "diag.h"
> > +#include "port.h"
> > +#include "say.h"
> 
> 5. You don't use say_* functions either.

I use debug print internally, I dropped them off before
this send but I'll add them back on next round.

> > +
> > +/**
> > + * Find function descriptor by its name.
> > + */
> > +struct cbox_func *
> > +cbox_func_find(const char *name, size_t len)
> 
> 6. I assume the function should be 'static'. It is never
> used out of cbox.c.

+1

> > +/**
> > + * Setup a detailed error description into the diagnostics area.
> > + */
> > +static void
> > +diag_set_param_error(struct lua_State *L, int idx, const char *func_name,
> > +		     const char *param, const char *exp)
> > +{
> > +	const char *typename = idx == 0 ?
> > +		"<unknown>" : lua_typename(L, lua_type(L, idx));
> > +	static const char *fmt =
> > +		"%s: wrong parameter \"%s\": expected %s, got %s";
> 
> 7. Please, lets omit 'static' when it is not needed. It just raises questions.

Yeah, this one escaped from previous cleanups.

> > +
> > +/**
> > + * Call function by its name from the Lua code.
> > + */
> > +static int
> > +lcbox_func_call(struct lua_State *L)
> > +{
> > +	int nr_args = lua_gettop(L);
> 
> 8. It is not really a number of arguments. It includes the function
> object itself.

Which is an argument. This is just a calling convention. I can
rename it to plain "top" if you prefer.

> > +
> > +	/*
> > +	 * FIXME: We should get rid of luaT_newthread but this
> > +	 * requires serious modifucations. In particular
> 
> 9. modifucations -> modifications.

thanks!

> > +
> > +	struct cbox_func *cf = mh_strnptr_node(func_hash, e)->val;
> > +	mh_strnptr_del(func_hash, e, NULL);
> > +	free(cf);
> 
> 10. I would advise to wrap it into cbox_func_delete(). I am 100% sure
> we will add more deletion things to the function object soon.

sure