[Tarantool-patches] [PATCH v4 0/6] box/func: implement cfunc Lua module

[Tarantool-patches] [PATCH v4 0/6] box/func: implement cfunc Lua module

[Cyrill Gorcunov]

The cfunc 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. There was a request to make
cfunc been Lua object with appropriate methods. If this is really
preferred then I'll implement this wrapping Lua code on top. I mean
that currently all operations are done via

 > require('cfunc').[create|drop|call|reload]

interface while there was a proposal to operate as

 > a = require('cfunc').create
 > a:call(args)

Please take a look once time permit.

v1-v3 are development ones and not sent.

branch gorcunov/gh-4642-func-ro-4
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/func: implement cfunc Lua module
  test: box/cfunc -- add simple module test

 src/box/CMakeLists.txt  |   1 +
 src/box/func.c          | 259 ++++++++++++++--------------
 src/box/func.h          |  78 +++++++++
 src/box/lua/cfunc.c     | 362 ++++++++++++++++++++++++++++++++++++++++
 src/box/lua/cfunc.h     |  55 ++++++
 src/box/lua/init.c      |   2 +
 test/box/CMakeLists.txt |   2 +
 test/box/cfunc.result   | 116 +++++++++++++
 test/box/cfunc.test.lua |  56 +++++++
 test/box/cfunc1.c       |  46 +++++
 test/box/cfunc2.c       |  46 +++++
 test/box/suite.ini      |   2 +-
 12 files changed, 894 insertions(+), 131 deletions(-)
 create mode 100644 src/box/lua/cfunc.c
 create mode 100644 src/box/lua/cfunc.h
 create mode 100644 test/box/cfunc.result
 create mode 100644 test/box/cfunc.test.lua
 create mode 100644 test/box/cfunc1.c
 create mode 100644 test/box/cfunc2.c


base-commit: d25ecab48832340dc2ef9c072cf506924c03af52
-- 
2.26.2

[Tarantool-patches] [PATCH v4 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 v4 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 v4 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 v4 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 v4 5/6] box/func: implement cfunc 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 "cfunc" lua
module.

Fixes #4692

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

@TarantoolBot document
Title: cfunc module
---
.. _cfunc-module:

-------------------------------------------------------------------------------
                                   Module `cfunc`
-------------------------------------------------------------------------------

.. module:: cfunc

===============================================================================
                                   Overview
===============================================================================

``cfunc`` module provides a way to create and execute external C functions without
registering them in system ``_func`` space. These functions are not persistent and
dissapear once instance goes offline. Also they are not replicated and can be
executed even if node is in read-only mode.

===============================================================================
                                    Index
===============================================================================

Below is a list of all ``cfunc`` functions and handle methods.

.. container:: table

    .. rst-class:: left-align-column-1
    .. rst-class:: left-align-column-2

    +-----------------------------+-----------------------------+
    | Name                        | Use                         |
    +=============================+=============================+
    | :ref:`cfunc.create()        | Create a new function       |
    | <cfunc-create>`             |                             |
    +-----------------------------+-----------------------------+
    | :ref:`cfunc.drop()          | Drop a function             |
    | <cfunc-drop>`               |                             |
    +-----------------------------+-----------------------------+
    | :ref:`cfunc.exists()        | Test if a function exists   |
    | <cfunc-exists>`             |                             |
    +-----------------------------+-----------------------------+
    | :ref:`cfunc.call()          | Execute a function          |
    | <cfunc-call>`               |                             |
    +-----------------------------+-----------------------------+
    | :ref:`cfunc.reload()        | Reload a module             |
    | <cfunc-reload>`             |                             |
    +-----------------------------+-----------------------------+
    | :ref:`cfunc.list()          | List functions              |
    | <cfunc-list>`               |                             |
    +-----------------------------+-----------------------------+

.. _cfunc-create:

.. function:: create(name)

    Create a new function.

    :param string name: a function name to create
    :return: (if success) ``nil``

    Possible raised errors are:

    * IllegalParams: incorrect type or value of a parameter
    * IllegalParams: a function is already exist
    * ClientError: function name is not valid identifier
    * OutOfMemory: no free memory available

    **Example**

    .. code-block:: lua

        require('cfunc').create('easy')

    Here we create a ``C`` function with name ``easy`` from
    ``easy.so`` shared library.

.. _cfunc-drop:

.. function:: drop(name)

    Drop previously created function.

    :param string name: function name to drop
    :return: (if success) ``nil``

    Possible raised errors are:

    * IllegalParams: incorrect type or value of a parameter
    * IllegalParams: the function does not exist

    **Example**

    .. code-block:: lua

        require('cfunc').drop('easy')

.. _cfunc-exists:

.. function:: exists(name)

    Check if a function exists.

    :param string name: function name to test
    :return: (if exists) ``true``
             (if does not exist) ``false``

    Possible raised errors are:

    * IllegalParams: incorrect type or value of a parameter

    **Example**

    .. code-block:: lua

        require('cfunc').exists('easy')

.. _cfunc-call:

.. function:: call(name[, {arguments}])

    Execute a function.

    :param string name: function name to run
    :param table arguments: table of arguments
    :return: (if success) ``nil``

    Possible raised errors are:

    * IllegalParams: incorrect type or value of a parameter
    * IllegalParams: the function is not created

    **Example**

    .. code-block:: lua

        require('cfunc').call('easy')

.. _cfunc-reload:

.. function:: reload(name)

    Reload a module.

    :param string name: modlule name
    :return: (if success) ``nil``

    Possible raised errors are:

    * IllegalParams: incorrect type or value of a parameter
    * IllegalParams: no such module registered
    * ClientError: module has not been loaded or not found

    **Example**

    .. code-block:: lua

        require('cfunc').reload('easy')

.. _cfunc-list:

.. function:: list()

    List created functions.

    **Example**

    .. code-block:: lua

        require('cfunc').list()
        ---
        - - easy
        ...
---
 src/box/CMakeLists.txt |   1 +
 src/box/func.c         |  10 ++
 src/box/lua/cfunc.c    | 362 +++++++++++++++++++++++++++++++++++++++++
 src/box/lua/cfunc.h    |  55 +++++++
 src/box/lua/init.c     |   2 +
 5 files changed, 430 insertions(+)
 create mode 100644 src/box/lua/cfunc.c
 create mode 100644 src/box/lua/cfunc.h

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 8b2e704cf..3d283618a 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -194,6 +194,7 @@ add_library(box STATIC
     ${sql_sources}
     ${lua_sources}
     lua/init.c
+    lua/cfunc.c
     lua/call.c
     lua/cfg.cc
     lua/console.c
diff --git a/src/box/func.c b/src/box/func.c
index ba98f0f9e..032b6062c 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -34,6 +34,7 @@
 #include "assoc.h"
 #include "lua/utils.h"
 #include "lua/call.h"
+#include "lua/cfunc.h"
 #include "error.h"
 #include "errinj.h"
 #include "diag.h"
@@ -152,6 +153,14 @@ module_init(void)
 			  "modules hash table");
 		return -1;
 	}
+	/*
+	 * cfunc depends on module engine,
+	 * so initialize them together.
+	 */
+	if (cfunc_init() != 0) {
+		module_free();
+		return -1;
+	}
 	return 0;
 }
 
@@ -166,6 +175,7 @@ module_free(void)
 		module_gc(module);
 	}
 	mh_strnptr_delete(modules);
+	cfunc_free();
 }
 
 /**
diff --git a/src/box/lua/cfunc.c b/src/box/lua/cfunc.c
new file mode 100644
index 000000000..d57ed7d1b
--- /dev/null
+++ b/src/box/lua/cfunc.c
@@ -0,0 +1,362 @@
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include <string.h>
+
+#include <lua.h>
+#include <lauxlib.h>
+#include <lualib.h>
+
+#include "assoc.h"
+#include "lua/utils.h"
+#include "trivia/util.h"
+#include "box/func.h"
+#include "box/call.h"
+#include "box/port.h"
+#include "box/identifier.h"
+#include "box/session.h"
+#include "box/user.h"
+#include "say.h"
+#include "diag.h"
+#include "tt_static.h"
+
+#include "box/lua/cfunc.h"
+#include "box/lua/call.h"
+
+static struct mh_strnptr_t *cfunc_hash;
+static unsigned int nr_cfunc = 0;
+
+struct cfunc {
+	struct module_sym	mod_sym;
+	size_t			name_len;
+	char			name[0];
+};
+
+struct cfunc *
+cfunc_lookup(const char *name, size_t len)
+{
+	if (name != NULL && len > 0) {
+		mh_int_t e = mh_strnptr_find_inp(cfunc_hash, name, len);
+		if (e != mh_end(cfunc_hash))
+			return mh_strnptr_node(cfunc_hash, e)->val;
+	}
+	return NULL;
+}
+
+static int
+cfunc_add(struct cfunc *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(cfunc_hash, &nd, NULL, NULL);
+	if (h == mh_end(cfunc_hash)) {
+		diag_set(OutOfMemory, sizeof(nd), "cfunc_hash_add", "h");
+		return -1;
+	}
+	return 0;
+}
+
+static void
+luaT_param_type_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);
+}
+
+static int
+lbox_cfunc_create(struct lua_State *L)
+{
+	static const char method[] = "cfunc.create";
+	struct cfunc *cfunc = NULL;
+
+	if (lua_gettop(L) != 1) {
+		static const char *fmt =
+			"%s: expects %s(\'name\')";
+		diag_set(IllegalParams, fmt, method, method);
+		goto out;
+	}
+
+	if (lua_type(L, 1) != LUA_TSTRING) {
+		luaT_param_type_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 (identifier_check(name, name_len) != 0)
+		goto out;
+
+	if (cfunc_lookup(name, name_len) != NULL) {
+		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
+		diag_set(IllegalParams, fmt, name);
+		goto out;
+	}
+
+	cfunc = malloc(sizeof(*cfunc) + name_len + 1);
+	if (cfunc == NULL) {
+		diag_set(OutOfMemory, sizeof(*cfunc), "malloc", "cfunc");
+		goto out;
+	}
+
+	cfunc->mod_sym.addr	= NULL;
+	cfunc->mod_sym.module	= NULL;
+	cfunc->mod_sym.name	= cfunc->name;
+	cfunc->name_len		= name_len;
+
+	memcpy(cfunc->name, name, name_len);
+	cfunc->name[name_len] = '\0';
+
+	if (cfunc_add(cfunc) != 0)
+		goto out;
+
+	nr_cfunc++;
+	return 0;
+
+out:
+	free(cfunc);
+	return luaT_error(L);
+}
+
+static int
+lbox_cfunc_drop(struct lua_State *L)
+{
+	static const char method[] = "cfunc.drop";
+	const char *name = NULL;
+
+	if (lua_gettop(L) != 1) {
+		static const char *fmt =
+			"%s: expects %s(\'name\')";
+		diag_set(IllegalParams, fmt, method, method);
+		return luaT_error(L);
+	}
+
+	if (lua_type(L, 1) != LUA_TSTRING) {
+		luaT_param_type_error(L, 1, method,
+				      lua_tostring(L, 1),
+				      "function name or id");
+		return luaT_error(L);
+	}
+
+	size_t name_len;
+	name = lua_tolstring(L, 1, &name_len);
+
+	mh_int_t e = mh_strnptr_find_inp(cfunc_hash, name, name_len);
+	if (e == mh_end(cfunc_hash)) {
+		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
+		diag_set(IllegalParams, fmt, name);
+		return luaT_error(L);
+	}
+
+	struct cfunc *cfunc = mh_strnptr_node(cfunc_hash, e)->val;
+	mh_strnptr_del(cfunc_hash, e, NULL);
+
+	nr_cfunc--;
+	free(cfunc);
+
+	return 0;
+}
+
+static int
+lbox_cfunc_exists(struct lua_State *L)
+{
+	static const char method[] = "cfunc.exists";
+	if (lua_gettop(L) != 1) {
+		static const char *fmt =
+			"%s: expects %s(\'name\') but no name passed";
+		diag_set(IllegalParams, fmt, method, method);
+		return luaT_error(L);
+	}
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+
+	struct cfunc *cfunc = cfunc_lookup(name, name_len);
+	lua_pushboolean(L, cfunc != NULL ? true : false);
+
+	return 1;
+}
+
+static int
+lbox_cfunc_reload(struct lua_State *L)
+{
+	static const char method[] = "cfunc.reload";
+	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
+		static const char *fmt =
+			"%s: expects %s(\'name\') but no name passed";
+		diag_set(IllegalParams, fmt, method, method);
+		return luaT_error(L);
+	}
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+
+	/*
+	 * Since we use module engine do not allow to
+	 * access arbitrary names only the ones we've
+	 * really created.
+	 */
+	struct cfunc *cfunc = cfunc_lookup(name, name_len);
+	if (cfunc == NULL) {
+		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
+		diag_set(IllegalParams, fmt, name);
+		return luaT_error(L);
+	}
+
+	struct module *module = NULL;
+	struct func_name n;
+
+	func_split_name(name, &n);
+	if (module_reload(n.package, n.package_end, &module) == 0) {
+		if (module != NULL)
+			return 0;
+		diag_set(ClientError, ER_NO_SUCH_MODULE, name);
+	}
+
+	return luaT_error(L);
+}
+
+static int
+lbox_cfunc_list(struct lua_State *L)
+{
+	if (nr_cfunc == 0)
+		return 0;
+
+	lua_createtable(L, nr_cfunc, 0);
+
+	int nr = 1;
+	mh_int_t i;
+	mh_foreach(cfunc_hash, i) {
+		struct cfunc *c = mh_strnptr_node(cfunc_hash, i)->val;
+		lua_pushstring(L, c->name);
+		lua_rawseti(L, -2, nr++);
+	}
+
+	return 1;
+}
+
+static int
+lbox_cfunc_call(struct lua_State *L)
+{
+	static const char method[] = "cfunc.call";
+	if (lua_gettop(L) < 1 || !lua_isstring(L, 1)) {
+		static const char *fmt =
+			"%s: expects %s(\'name\')";
+		diag_set(IllegalParams, fmt, method, method);
+	}
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+
+	struct cfunc *cfunc = cfunc_lookup(name, name_len);
+	if (cfunc == NULL) {
+		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
+		diag_set(IllegalParams, fmt, name);
+		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_sym_call(&cfunc->mod_sym,  &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;
+}
+
+int
+cfunc_init(void)
+{
+	cfunc_hash = mh_strnptr_new();
+	if (cfunc_hash == NULL) {
+		diag_set(OutOfMemory, sizeof(*cfunc_hash), "malloc",
+			  "cfunc hash table");
+		return -1;
+	}
+	return 0;
+}
+
+void
+cfunc_free(void)
+{
+	while (mh_size(cfunc_hash) > 0) {
+		mh_int_t e = mh_first(cfunc_hash);
+		struct cfunc *c = mh_strnptr_node(cfunc_hash, e)->val;
+		module_sym_unload(&c->mod_sym);
+		mh_strnptr_del(cfunc_hash, e, NULL);
+	}
+	mh_strnptr_delete(cfunc_hash);
+	cfunc_hash = NULL;
+}
+
+void
+box_lua_cfunc_init(struct lua_State *L)
+{
+	static const struct luaL_Reg cfunclib[] = {
+		{ "create",	lbox_cfunc_create },
+		{ "drop",	lbox_cfunc_drop },
+		{ "exists",	lbox_cfunc_exists },
+		{ "call",	lbox_cfunc_call },
+		{ "reload",	lbox_cfunc_reload },
+		{ "list",	lbox_cfunc_list },
+		{ }
+	};
+
+	luaL_register_module(L, "cfunc", cfunclib);
+	lua_pop(L, 1);
+}
diff --git a/src/box/lua/cfunc.h b/src/box/lua/cfunc.h
new file mode 100644
index 000000000..51cec28b8
--- /dev/null
+++ b/src/box/lua/cfunc.h
@@ -0,0 +1,55 @@
+#ifndef INCLUDES_TARANTOOL_MOD_BOX_LUA_CFUNC_H
+#define INCLUDES_TARANTOOL_MOD_BOX_LUA_CFUNC_H
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <stdint.h>
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct lua_State;
+
+void
+box_lua_cfunc_init(struct lua_State *L);
+
+int
+cfunc_init(void);
+
+void
+cfunc_free(void);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* INCLUDES_TARANTOOL_MOD_BOX_LUA_CFUNC_H */
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index d0316ef86..972ab9d8c 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -48,6 +48,7 @@
 #include "box/lua/error.h"
 #include "box/lua/tuple.h"
 #include "box/lua/call.h"
+#include "box/lua/cfunc.h"
 #include "box/lua/slab.h"
 #include "box/lua/index.h"
 #include "box/lua/space.h"
@@ -465,6 +466,7 @@ box_lua_init(struct lua_State *L)
 	box_lua_error_init(L);
 	box_lua_tuple_init(L);
 	box_lua_call_init(L);
+	box_lua_cfunc_init(L);
 	box_lua_cfg_init(L);
 	box_lua_slab_init(L);
 	box_lua_index_init(L);
-- 
2.26.2

[Tarantool-patches] [PATCH v4 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,
 | +        cfunc = true,
 |          pwd = true,
 |          socket = true,
 |          strict = true,

ie need to enable cfunc module.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/box/CMakeLists.txt |   2 +
 test/box/cfunc.result   | 116 ++++++++++++++++++++++++++++++++++++++++
 test/box/cfunc.test.lua |  56 +++++++++++++++++++
 test/box/cfunc1.c       |  46 ++++++++++++++++
 test/box/cfunc2.c       |  46 ++++++++++++++++
 test/box/suite.ini      |   2 +-
 6 files changed, 267 insertions(+), 1 deletion(-)
 create mode 100644 test/box/cfunc.result
 create mode 100644 test/box/cfunc.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/cfunc.result b/test/box/cfunc.result
new file mode 100644
index 000000000..beb07d775
--- /dev/null
+++ b/test/box/cfunc.result
@@ -0,0 +1,116 @@
+-- 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
+ | ---
+ | ...
+
+cfunc = require('cfunc')
+ | ---
+ | ...
+fio = require('fio')
+ | ---
+ | ...
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.so")
+ | ---
+ | ...
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.so")
+ | ---
+ | ...
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.so")
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc1_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+--
+-- Both of them are sitting in cfunc.so
+cfunc.create('cfunc.cfunc_sole_call')
+ | ---
+ | ...
+cfunc.create('cfunc.cfunc_fetch_array')
+ | ---
+ | ...
+
+--
+-- Make sure they are registered fine
+cfunc.exists('cfunc.cfunc_sole_call')
+ | ---
+ | - true
+ | ...
+cfunc.exists('cfunc.cfunc_fetch_array')
+ | ---
+ | - true
+ | ...
+
+--
+-- And they are listed
+cfunc.list()
+ | ---
+ | - - cfunc.cfunc_sole_call
+ |   - cfunc.cfunc_fetch_array
+ | ...
+
+--
+-- Make sure they all collable
+cfunc.call('cfunc.cfunc_sole_call')
+ | ---
+ | ...
+cfunc.call('cfunc.cfunc_fetch_array', {1,2,3,4})
+ | ---
+ | ...
+-- This one should issue an error
+cfunc.call('cfunc.cfunc_fetch_array', {1,2,4})
+ | ---
+ | - error: invalid argument 4 != 3
+ | ...
+
+--
+-- Clean old function references and reload new one.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc2_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+cfunc.reload('cfunc.cfunc_fetch_array')
+ | ---
+ | ...
+
+cfunc.call('cfunc.cfunc_sole_call')
+ | ---
+ | ...
+cfunc.call('cfunc.cfunc_fetch_array', {2,4,6})
+ | ---
+ | ...
+
+--
+-- Clean it up
+cfunc.drop('cfunc.cfunc_sole_call')
+ | ---
+ | ...
+cfunc.drop('cfunc.cfunc_fetch_array')
+ | ---
+ | ...
+
+--
+-- List should be empty
+cfunc.list()
+ | ---
+ | ...
+
+--
+-- Cleanup the generated symlink
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
diff --git a/test/box/cfunc.test.lua b/test/box/cfunc.test.lua
new file mode 100644
index 000000000..01e5fbebb
--- /dev/null
+++ b/test/box/cfunc.test.lua
@@ -0,0 +1,56 @@
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+cfunc = require('cfunc')
+fio = require('fio')
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.so")
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.so")
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.so")
+
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc1_path, cfunc_path)
+
+--
+-- Both of them are sitting in cfunc.so
+cfunc.create('cfunc.cfunc_sole_call')
+cfunc.create('cfunc.cfunc_fetch_array')
+
+--
+-- Make sure they are registered fine
+cfunc.exists('cfunc.cfunc_sole_call')
+cfunc.exists('cfunc.cfunc_fetch_array')
+
+--
+-- And they are listed
+cfunc.list()
+
+--
+-- Make sure they all collable
+cfunc.call('cfunc.cfunc_sole_call')
+cfunc.call('cfunc.cfunc_fetch_array', {1,2,3,4})
+-- This one should issue an error
+cfunc.call('cfunc.cfunc_fetch_array', {1,2,4})
+
+--
+-- Clean old function references and reload new one.
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc2_path, cfunc_path)
+
+cfunc.reload('cfunc.cfunc_fetch_array')
+
+cfunc.call('cfunc.cfunc_sole_call')
+cfunc.call('cfunc.cfunc_fetch_array', {2,4,6})
+
+--
+-- Clean it up
+cfunc.drop('cfunc.cfunc_sole_call')
+cfunc.drop('cfunc.cfunc_fetch_array')
+
+--
+-- List should be empty
+cfunc.list()
+
+--
+-- 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..4334f28da
--- /dev/null
+++ b/test/box/cfunc1.c
@@ -0,0 +1,46 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Just make sure we've been called.
+ */
+int
+cfunc_sole_call(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	say_info("-- cfunc_sole_call -  called --");
+	printf("ok - cfunc_sole_call\n");
+	return 0;
+}
+
+/*
+ * Fetch N-array
+ */
+int
+cfunc_fetch_array(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	int arg_count = mp_decode_array(&args);
+	int field_count = mp_decode_array(&args);
+
+	(void)arg_count;
+	(void)field_count;
+
+	say_info("-- cfunc_fetch_array -  called --");
+
+	for (int i = 0; i < field_count; i++) {
+		int val = mp_decode_uint(&args);
+		int needed = (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);
+		}
+	}
+
+	printf("ok - cfunc_fetch_array\n");
+	return 0;
+}
diff --git a/test/box/cfunc2.c b/test/box/cfunc2.c
new file mode 100644
index 000000000..30ac48010
--- /dev/null
+++ b/test/box/cfunc2.c
@@ -0,0 +1,46 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Just make sure we've been called.
+ */
+int
+cfunc_sole_call(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	say_info("-- cfunc_sole_call -  called --");
+	printf("ok - cfunc_sole_call\n");
+	return 0;
+}
+
+/*
+ * Fetch N-array
+ */
+int
+cfunc_fetch_array(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	int arg_count = mp_decode_array(&args);
+	int field_count = mp_decode_array(&args);
+
+	(void)arg_count;
+	(void)field_count;
+
+	say_info("-- cfunc_fetch_array -  called --");
+
+	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);
+		}
+	}
+
+	printf("ok - cfunc_fetch_array\n");
+	return 0;
+}
diff --git a/test/box/suite.ini b/test/box/suite.ini
index 793b8bf76..54130f43f 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 cfunc.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 v4 0/6] box/func: implement cfunc Lua module

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

On 01.10.2020 15:51, Cyrill Gorcunov wrote:
> The cfunc 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. There was a request to make
> cfunc been Lua object with appropriate methods. If this is really
> preferred then I'll implement this wrapping Lua code on top. I mean
> that currently all operations are done via
> 
>  > require('cfunc').[create|drop|call|reload]
> 
> interface while there was a proposal to operate as
> 
>  > a = require('cfunc').create
>  > a:call(args)

No, the proposal was rather

	cfunc = require('cfunc')
	f = cfunc.load('test_box_c_function')
	f(1, 2, 3)

Sorry I didn't describe it properly anywhere. But it is easy to fix
anyway.

In Lua you can define __call and make an object look exactly
like a function. That is the idea behind making it possible to
use Lua and C functions in the same way.

	if load_c then
		f = cfunc.load('test_box_c_function')
	else
		f = function(a, b, c) return a + b + c end
	end
	f(1, 2, 3) -- Works and looks the same for C and Lua.

With this way even IPROTO_CALL should work on such functions, if the
function object is global and the user has access to executing everything
here.

Re: [Tarantool-patches] [PATCH v4 5/6] box/func: implement cfunc Lua module

[Vladislav Shpilevoy]

Thanks for the patch!

See 20 comments below.

On 01.10.2020 15:51, Cyrill Gorcunov wrote:
> Currently to run "C" function from some external module
> one have to register it first in "_func" system space. This
> is a problem if node is in read-only mode (replica).
> 
> Still people would like to have a way to run such functions
> even in ro mode. For this sake we implement "cfunc" lua
> module.
> 
> Fixes #4692
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> @TarantoolBot document
> Title: cfunc module

1. The documentation will go to a github issue, where it looks super
broken as I checked. Please, try to use github markdown here.

> .. _cfunc-list:
> 
> .. function:: list()
> 
>     List created functions.
> 
>     **Example**
> 
>     .. code-block:: lua
> 
>         require('cfunc').list()
>         ---
>         - - easy
>         ...

2. Lets make the API minimal for now, without exists(), list(). I can't
think of why are they needed in an application. We can add more methods
if they are obviously needed for something, or when users ask.

> ---
>  src/box/CMakeLists.txt |   1 +
>  src/box/func.c         |  10 ++
>  src/box/lua/cfunc.c    | 362 +++++++++++++++++++++++++++++++++++++++++
>  src/box/lua/cfunc.h    |  55 +++++++
>  src/box/lua/init.c     |   2 +
>  5 files changed, 430 insertions(+)
>  create mode 100644 src/box/lua/cfunc.c
>  create mode 100644 src/box/lua/cfunc.h
> 
> diff --git a/src/box/func.c b/src/box/func.c
> index ba98f0f9e..032b6062c 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -34,6 +34,7 @@
>  #include "assoc.h"
>  #include "lua/utils.h"
>  #include "lua/call.h"
> +#include "lua/cfunc.h"

3. Why do you need cfunc here? Also it looks like a cyclic
dependency - cfunc depends on box/func and vise versa.

>  #include "error.h"
>  #include "errinj.h"
>  #include "diag.h"
> @@ -152,6 +153,14 @@ module_init(void)
>  			  "modules hash table");
>  		return -1;
>  	}
> +	/*
> +	 * cfunc depends on module engine,
> +	 * so initialize them together.
> +	 */
> +	if (cfunc_init() != 0) {
> +		module_free();
> +		return -1;
> +	}

4. Better not enclose modules. We have lots of modules depending
on each other but still it is much more clear when they are all
initialized in one place. So I would move it to box_init() and
box_free().

For example, func_c depend on port, but we don't call port_init()
in module_init(). Actually, port is needed in several modules. The
same for tuple module, tuple format, fiber. I wouldn't consider
cfunc as something different.


5. The more I look at the names, the more I think that we probably
should rename cfunc to cmod, or cmodule, or boxmod, or cbox, or
modbox, or another name.

Because we don't allow to load and call any function. Only module API
functions, which have strict signature - box_function_f.

Also cfunc looks too similar to func_c, while they don't have much
in common. func_c is a helper for _func space, and cfunc has nothing
to do with spaces and schema.

>  	return 0;
>  }
> diff --git a/src/box/lua/cfunc.c b/src/box/lua/cfunc.c
> new file mode 100644
> index 000000000..d57ed7d1b
> --- /dev/null
> +++ b/src/box/lua/cfunc.c
> @@ -0,0 +1,362 @@
> +#include <string.h>
> +
> +#include <lua.h>
> +#include <lauxlib.h>
> +#include <lualib.h>
> +
> +#include "assoc.h"
> +#include "lua/utils.h"
> +#include "trivia/util.h"
> +#include "box/func.h"
> +#include "box/call.h"
> +#include "box/port.h"
> +#include "box/identifier.h"
> +#include "box/session.h"
> +#include "box/user.h"

6. Why do you need box/func.h, box/call.h, box/session.h,
box/user.h? All these files are about schema and credentials.
cfunc does not need any of this. If anything from them does
not depend on schema, and you need it, then you probably
should move these code pieces out first.

For instance, module_sym does not have intersections with
func_c, but somewhy is stored in box/func.h and .c.

Also why do you need box/identifier.h? We don't store the
names in any schema, so we don't need them to be valid
identifiers. Let dlsym() decide what names are valid.

> +#include "say.h"
> +#include "diag.h"
> +#include "tt_static.h"
> +
> +#include "box/lua/cfunc.h"
> +#include "box/lua/call.h"

7. Why do you need box/lua/call.h? You are not calling Lua
functions here.

> +
> +static struct mh_strnptr_t *cfunc_hash;
> +static unsigned int nr_cfunc = 0;

8. What is 'nr'? If it is a number of functions, then why
couldn't you use mh_size()?

> +
> +struct cfunc {
> +	struct module_sym	mod_sym;
> +	size_t			name_len;
> +	char			name[0];
> +};
> +
> +struct cfunc *
> +cfunc_lookup(const char *name, size_t len)
> +{
> +	if (name != NULL && len > 0) {
> +		mh_int_t e = mh_strnptr_find_inp(cfunc_hash, name, len);
> +		if (e != mh_end(cfunc_hash))
> +			return mh_strnptr_node(cfunc_hash, e)->val;
> +	}
> +	return NULL;
> +}
> +
> +static int
> +cfunc_add(struct cfunc *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(cfunc_hash, &nd, NULL, NULL);
> +	if (h == mh_end(cfunc_hash)) {
> +		diag_set(OutOfMemory, sizeof(nd), "cfunc_hash_add", "h");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static void
> +luaT_param_type_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);
> +}
> +
> +static int
> +lbox_cfunc_create(struct lua_State *L)

9. Better not use lbox_ prefix. Because this is not box.cfunc,
it is just cfunc.

> +{
> +	static const char method[] = "cfunc.create";

10. Why so complex? And why static? Why not

	const char *method = "cfunc.create";

like we do everywhere else?

> +	struct cfunc *cfunc = NULL;
> +
> +	if (lua_gettop(L) != 1) {
> +		static const char *fmt =
> +			"%s: expects %s(\'name\')";

11. Lets not use 'static' when it does not change anything.
The same for all other places where 'static' can be removed
without any changes.

> +		diag_set(IllegalParams, fmt, method, method);
> +		goto out;
> +	}
> +
> +	if (lua_type(L, 1) != LUA_TSTRING) {
> +		luaT_param_type_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 (identifier_check(name, name_len) != 0)
> +		goto out;
> +
> +	if (cfunc_lookup(name, name_len) != NULL) {
> +		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
> +		diag_set(IllegalParams, fmt, name);
> +		goto out;
> +	}
> +
> +	cfunc = malloc(sizeof(*cfunc) + name_len + 1);
> +	if (cfunc == NULL) {
> +		diag_set(OutOfMemory, sizeof(*cfunc), "malloc", "cfunc");

12. You allocated 'sizeof(*cfunc) + name_len + 1)', not only sizeof.

> +		goto out;
> +	}
> +
> +	cfunc->mod_sym.addr	= NULL;
> +	cfunc->mod_sym.module	= NULL;
> +	cfunc->mod_sym.name	= cfunc->name;
> +	cfunc->name_len		= name_len;
> +
> +	memcpy(cfunc->name, name, name_len);
> +	cfunc->name[name_len] = '\0';
> +
> +	if (cfunc_add(cfunc) != 0)
> +		goto out;
> +
> +	nr_cfunc++;
> +	return 0;
> +
> +out:
> +	free(cfunc);
> +	return luaT_error(L);

13. Our agreement for the new code is that it should not throw. It
should return nil + error. This was somewhere in the documentation.
The same for all other functions.

Although I don't remember what is the agreement to return success.
If you return nothing in case of success, and nil+error on an error,
then you need to write code like this:

	unused, err = func()
	if not err then
		-- Success
	else
		-- Error
	end

The first value becomes never used, in both cases. Maybe it is worth
returning 'true', so at least I could write something like:

	ok, err = func()
	if ok then
		-- Success
	else
		-- Error
	end

But I don't know the officient agreement here. In SWIM I used the
second way, for example.

> +}
> +
> +static int
> +lbox_cfunc_drop(struct lua_State *L)
> +{
> +	static const char method[] = "cfunc.drop";
> +	const char *name = NULL;
> +
> +	if (lua_gettop(L) != 1) {
> +		static const char *fmt =
> +			"%s: expects %s(\'name\')";
> +		diag_set(IllegalParams, fmt, method, method);
> +		return luaT_error(L);
> +	}
> +
> +	if (lua_type(L, 1) != LUA_TSTRING) {
> +		luaT_param_type_error(L, 1, method,
> +				      lua_tostring(L, 1),
> +				      "function name or id");
> +		return luaT_error(L);
> +	}
> +
> +	size_t name_len;
> +	name = lua_tolstring(L, 1, &name_len);
> +
> +	mh_int_t e = mh_strnptr_find_inp(cfunc_hash, name, name_len);
> +	if (e == mh_end(cfunc_hash)) {
> +		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
> +		diag_set(IllegalParams, fmt, name);
> +		return luaT_error(L);
> +	}
> +
> +	struct cfunc *cfunc = mh_strnptr_node(cfunc_hash, e)->val;
> +	mh_strnptr_del(cfunc_hash, e, NULL);
> +
> +	nr_cfunc--;
> +	free(cfunc);
> +
> +	return 0;
> +}
> +
> +static int
> +lbox_cfunc_exists(struct lua_State *L)
> +{
> +	static const char method[] = "cfunc.exists";
> +	if (lua_gettop(L) != 1) {
> +		static const char *fmt =
> +			"%s: expects %s(\'name\') but no name passed";
> +		diag_set(IllegalParams, fmt, method, method);
> +		return luaT_error(L);
> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	struct cfunc *cfunc = cfunc_lookup(name, name_len);
> +	lua_pushboolean(L, cfunc != NULL ? true : false);

14. Result of comparison is boolean anyway. No need to use '?'
and these constants.

> +
> +	return 1;
> +}
> +
> +static int
> +lbox_cfunc_reload(struct lua_State *L)
> +{
> +	static const char method[] = "cfunc.reload";
> +	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
> +		static const char *fmt =
> +			"%s: expects %s(\'name\') but no name passed";
> +		diag_set(IllegalParams, fmt, method, method);
> +		return luaT_error(L);
> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	/*
> +	 * Since we use module engine do not allow to
> +	 * access arbitrary names only the ones we've
> +	 * really created.
> +	 */
> +	struct cfunc *cfunc = cfunc_lookup(name, name_len);
> +	if (cfunc == NULL) {
> +		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
> +		diag_set(IllegalParams, fmt, name);
> +		return luaT_error(L);
> +	}
> +
> +	struct module *module = NULL;
> +	struct func_name n;
> +
> +	func_split_name(name, &n);
> +	if (module_reload(n.package, n.package_end, &module) == 0) {

15. The API to reload a single function looks not so good. You can't
reload one function and not reload the others anyway. Or can you?

For instance, we have box.schema.func.reload(). It takes a module
name.

In that way the idea to rename cfunc to cbox or something like that looks
even better. We could provide methods to operate on entire modules. So it
would be

	f = cbox.load_func() -- Create a function.
	f:unload()           -- Unload a function.
	cbox.reload_module() -- Reload module and all functions in it.
	cbox.unload_module() -- Unload the module from the memory.

We need to put more thoughts into the API design.

> +		if (module != NULL)
> +			return 0;
> +		diag_set(ClientError, ER_NO_SUCH_MODULE, name);
> +	}
> +
> +	return luaT_error(L);
> +}
> +
> +static int
> +lbox_cfunc_list(struct lua_State *L)
> +{
> +	if (nr_cfunc == 0)
> +		return 0;

16. So the function either returns nil or a table? This
looks bad. So a user can't just write

	for i, func in pairs(cfunc.list()) do
		...
	end

In your case he needs to check for nil. Does not look
like a good idea. Maybe worth returning an empty table.
Or just drop this function, because I don't know why
would a user need it. In an application you anyway know
all the functions you use.

> +
> +	lua_createtable(L, nr_cfunc, 0);
> +
> +	int nr = 1;
> +	mh_int_t i;
> +	mh_foreach(cfunc_hash, i) {
> +		struct cfunc *c = mh_strnptr_node(cfunc_hash, i)->val;
> +		lua_pushstring(L, c->name);
> +		lua_rawseti(L, -2, nr++);
> +	}
> +
> +	return 1;
> +}
> +
> +static int
> +lbox_cfunc_call(struct lua_State *L)
> +{
> +	static const char method[] = "cfunc.call";
> +	if (lua_gettop(L) < 1 || !lua_isstring(L, 1)) {
> +		static const char *fmt =
> +			"%s: expects %s(\'name\')";
> +		diag_set(IllegalParams, fmt, method, method);

17. I suppose you forgot a return here. And also to cover it
with a test.

> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	struct cfunc *cfunc = cfunc_lookup(name, name_len);
> +	if (cfunc == NULL) {
> +		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
> +		diag_set(IllegalParams, fmt, name);
> +		return luaT_error(L);
> +	}
> +
> +	lua_State *args_L = luaT_newthread(tarantool_L);

18. Wtf? Why can't you use L? Why do you even move anything here
between the stacks?

> +	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_sym_call(&cfunc->mod_sym,  &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;
> +}
> +
> +int
> +cfunc_init(void)
> +{
> +	cfunc_hash = mh_strnptr_new();
> +	if (cfunc_hash == NULL) {
> +		diag_set(OutOfMemory, sizeof(*cfunc_hash), "malloc",
> +			  "cfunc hash table");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +void
> +cfunc_free(void)
> +{
> +	while (mh_size(cfunc_hash) > 0) {
> +		mh_int_t e = mh_first(cfunc_hash);
> +		struct cfunc *c = mh_strnptr_node(cfunc_hash, e)->val;
> +		module_sym_unload(&c->mod_sym);
> +		mh_strnptr_del(cfunc_hash, e, NULL);
> +	}
> +	mh_strnptr_delete(cfunc_hash);
> +	cfunc_hash = NULL;
> +}
> +
> +void
> +box_lua_cfunc_init(struct lua_State *L)
> +{
> +	static const struct luaL_Reg cfunclib[] = {
> +		{ "create",	lbox_cfunc_create },
> +		{ "drop",	lbox_cfunc_drop },
> +		{ "exists",	lbox_cfunc_exists },
> +		{ "call",	lbox_cfunc_call },
> +		{ "reload",	lbox_cfunc_reload },
> +		{ "list",	lbox_cfunc_list },
> +		{ }
> +	};
> +
> +	luaL_register_module(L, "cfunc", cfunclib);
> +	lua_pop(L, 1);
> +}
> diff --git a/src/box/lua/cfunc.h b/src/box/lua/cfunc.h
> new file mode 100644
> index 000000000..51cec28b8
> --- /dev/null
> +++ b/src/box/lua/cfunc.h
> @@ -0,0 +1,55 @@
> +#ifndef INCLUDES_TARANTOOL_MOD_BOX_LUA_CFUNC_H
> +#define INCLUDES_TARANTOOL_MOD_BOX_LUA_CFUNC_H

19. In the new code we agreed to use #pragma once.

> +
> +#include <stdint.h>
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +struct lua_State;
> +
> +void
> +box_lua_cfunc_init(struct lua_State *L);
> +
> +int
> +cfunc_init(void);

20. Why do you need 2 inits? All Lua modules always
have a single init and a single free.

> +
> +void
> +cfunc_free(void);
> +

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

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/test/box/cfunc.result b/test/box/cfunc.result
> new file mode 100644
> index 000000000..beb07d775
> --- /dev/null
> +++ b/test/box/cfunc.result
> @@ -0,0 +1,116 @@
> +-- 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
> + | ---
> + | ...
> +
> +cfunc = require('cfunc')
> + | ---
> + | ...
> +fio = require('fio')
> + | ---
> + | ...
> +
> +cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.so")
> + | ---
> + | ...
> +cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.so")
> + | ---
> + | ...
> +cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.so")

.so files don't exist on Mac.

I didn't read the tests carefully yet, until we agree on API.

Re: [Tarantool-patches] [PATCH v4 5/6] box/func: implement cfunc Lua module

[Cyrill Gorcunov]

On Sat, Oct 03, 2020 at 03:55:26PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 20 comments below.
> 
> On 01.10.2020 15:51, Cyrill Gorcunov wrote:
> > Currently to run "C" function from some external module
> > one have to register it first in "_func" system space. This
> > is a problem if node is in read-only mode (replica).
> > 
> > Still people would like to have a way to run such functions
> > even in ro mode. For this sake we implement "cfunc" lua
> > module.
> > 
> > Fixes #4692
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > 
> > @TarantoolBot document
> > Title: cfunc module
> 
> 1. The documentation will go to a github issue, where it looks super
> broken as I checked. Please, try to use github markdown here.

You know, I've been using sphinx syntax for a purpose -- this commit
message can be simply extracted and being put into our doc.repo. Otherwise
it becomes a double work: first we write commit message in own format
then out docs team need to reformat it to sphinx. Taking into account
that we simply don't have enough men power we could optimize this procedure.
Note -- I don't mind to rewrite it in MD format but probably sphinx is better?

> > .. _cfunc-list:
> > 
> > .. function:: list()
> > 
> >     List created functions.
> > 
> >     **Example**
> > 
> >     .. code-block:: lua
> > 
> >         require('cfunc').list()
> >         ---
> >         - - easy
> >         ...
> 
> 2. Lets make the API minimal for now, without exists(), list(). I can't
> think of why are they needed in an application. We can add more methods
> if they are obviously needed for something, or when users ask.

Ok, good point! Will do.

> > diff --git a/src/box/func.c b/src/box/func.c
> > index ba98f0f9e..032b6062c 100644
> > --- a/src/box/func.c
> > +++ b/src/box/func.c
> > @@ -34,6 +34,7 @@
> >  #include "assoc.h"
> >  #include "lua/utils.h"
> >  #include "lua/call.h"
> > +#include "lua/cfunc.h"
> 
> 3. Why do you need cfunc here? Also it looks like a cyclic
> dependency - cfunc depends on box/func and vise versa.

I fear it is leftover from previous attempts. Thanks for catching!

> > @@ -152,6 +153,14 @@ module_init(void)
> >  			  "modules hash table");
> >  		return -1;
> >  	}
> > +	/*
> > +	 * cfunc depends on module engine,
> > +	 * so initialize them together.
> > +	 */
> > +	if (cfunc_init() != 0) {
> > +		module_free();
> > +		return -1;
> > +	}
> 
> 4. Better not enclose modules. We have lots of modules depending
> on each other but still it is much more clear when they are all
> initialized in one place. So I would move it to box_init() and
> box_free().
> 
> For example, func_c depend on port, but we don't call port_init()
> in module_init(). Actually, port is needed in several modules. The
> same for tuple module, tuple format, fiber. I wouldn't consider
> cfunc as something different.

+1, thanks!

> 5. The more I look at the names, the more I think that we probably
> should rename cfunc to cmod, or cmodule, or boxmod, or cbox, or
> modbox, or another name.
> 
> Because we don't allow to load and call any function. Only module API
> functions, which have strict signature - box_function_f.
> 
> Also cfunc looks too similar to func_c, while they don't have much
> in common. func_c is a helper for _func space, and cfunc has nothing
> to do with spaces and schema.

cmod sounds great to me

> 6. Why do you need box/func.h, box/call.h, box/session.h,
> box/user.h? All these files are about schema and credentials.
> cfunc does not need any of this. If anything from them does
> not depend on schema, and you need it, then you probably
> should move these code pieces out first.

I'm *really sorry* Vlad for this "include" crap! I managed to
forget clean them up while been reworking the series from
another approach :( I'll re-check and clean them up, thanks!

> For instance, module_sym does not have intersections with
> func_c, but somewhy is stored in box/func.h and .c.
> 
> Also why do you need box/identifier.h? We don't store the
> names in any schema, so we don't need them to be valid
> identifiers. Let dlsym() decide what names are valid.

Agreed.

> 
> > +#include "say.h"
> > +#include "diag.h"
> > +#include "tt_static.h"
> > +
> > +#include "box/lua/cfunc.h"
> > +#include "box/lua/call.h"
> 
> 7. Why do you need box/lua/call.h? You are not calling Lua
> functions here.

Sigh, leftover again. Sorry and thanks!

> > +
> > +static struct mh_strnptr_t *cfunc_hash;
> > +static unsigned int nr_cfunc = 0;
> 
> 8. What is 'nr'? If it is a number of functions, then why
> couldn't you use mh_size()?

Yes, it is number of functions. Yes we can use mh_size instead.
Moreover since we're going to drop list() we don't need this
anymore, thanks!

> > +
> > +static int
> > +lbox_cfunc_create(struct lua_State *L)
> 
> 9. Better not use lbox_ prefix. Because this is not box.cfunc,
> it is just cfunc.

Sure

> 
> > +{
> > +	static const char method[] = "cfunc.create";
> 
> 10. Why so complex? And why static? Why not
> 
> 	const char *method = "cfunc.create";
> 
> like we do everywhere else?

This helps compiler to understand that the variable
allocated just once. Rather a habbit. I'll drop it.

> > +	if (lua_gettop(L) != 1) {
> > +		static const char *fmt =
> > +			"%s: expects %s(\'name\')";
> 
> 11. Lets not use 'static' when it does not change anything.
> The same for all other places where 'static' can be removed
> without any changes.

ok

> > +
> > +	cfunc = malloc(sizeof(*cfunc) + name_len + 1);
> > +	if (cfunc == NULL) {
> > +		diag_set(OutOfMemory, sizeof(*cfunc), "malloc", "cfunc");
> 
> 12. You allocated 'sizeof(*cfunc) + name_len + 1)', not only sizeof.

I know, the name of the function is placed right after the structure.
It is intended.

> 
> > +		goto out;
> > +	}
> > +
> > +	cfunc->mod_sym.addr	= NULL;
> > +	cfunc->mod_sym.module	= NULL;
> > +	cfunc->mod_sym.name	= cfunc->name;
> > +	cfunc->name_len		= name_len;
> > +
> > +	memcpy(cfunc->name, name, name_len);
> > +	cfunc->name[name_len] = '\0';

Here we copy it right after.

> > +
> > +out:
> > +	free(cfunc);
> > +	return luaT_error(L);
> 
> 13. Our agreement for the new code is that it should not throw. It
> should return nil + error. This was somewhere in the documentation.
> The same for all other functions.
> 
> Although I don't remember what is the agreement to return success.
> If you return nothing in case of success, and nil+error on an error,
> then you need to write code like this:
> 
> 	unused, err = func()
> 	if not err then
> 		-- Success
> 	else
> 		-- Error
> 	end
> 
> The first value becomes never used, in both cases. Maybe it is worth
> returning 'true', so at least I could write something like:
> 
> 	ok, err = func()
> 	if ok then
> 		-- Success
> 	else
> 		-- Error
> 	end
> 
> But I don't know the officient agreement here. In SWIM I used the
> second way, for example.

Thanks for pointing, Vlad! Will rework.

> > +
> > +	size_t name_len;
> > +	const char *name = lua_tolstring(L, 1, &name_len);
> > +
> > +	struct cfunc *cfunc = cfunc_lookup(name, name_len);
> > +	lua_pushboolean(L, cfunc != NULL ? true : false);
> 
> 14. Result of comparison is boolean anyway. No need to use '?'
> and these constants.

ok

> > +
> > +	func_split_name(name, &n);
> > +	if (module_reload(n.package, n.package_end, &module) == 0) {
> 
> 15. The API to reload a single function looks not so good. You can't
> reload one function and not reload the others anyway. Or can you?

True, only the whole module can be reloaded. I need to adjust the code.

> 
> For instance, we have box.schema.func.reload(). It takes a module
> name.

Yeah, and I have to do the same.

> 
> In that way the idea to rename cfunc to cbox or something like that looks
> even better. We could provide methods to operate on entire modules. So it
> would be
> 
> 	f = cbox.load_func() -- Create a function.
> 	f:unload()           -- Unload a function.
> 	cbox.reload_module() -- Reload module and all functions in it.
> 	cbox.unload_module() -- Unload the module from the memory.

maybe

	f = cmod.create(name)		-- just like in schema.func
	f:drop()			-- same
	cmod.module('reload', name)
	cmod.module('unload', name)

? I don't mind to use cbox name either but cmod somehow close to
what we're doing.

> 
> We need to put more thoughts into the API design.
> 
> > +		if (module != NULL)
> > +			return 0;
> > +		diag_set(ClientError, ER_NO_SUCH_MODULE, name);
> > +	}
> > +
> > +	return luaT_error(L);
> > +}
> > +
> > +static int
> > +lbox_cfunc_list(struct lua_State *L)
> > +{
> > +	if (nr_cfunc == 0)
> > +		return 0;
> 
> 16. So the function either returns nil or a table? This
> looks bad. So a user can't just write
> 
> 	for i, func in pairs(cfunc.list()) do
> 		...
> 	end
> 
> In your case he needs to check for nil. Does not look
> like a good idea. Maybe worth returning an empty table.
> Or just drop this function, because I don't know why
> would a user need it. In an application you anyway know
> all the functions you use.

Agreed. Lets drop it for a while. If users really would need
for listing we will implement on demand.

> > +static int
> > +lbox_cfunc_call(struct lua_State *L)
> > +{
> > +	static const char method[] = "cfunc.call";
> > +	if (lua_gettop(L) < 1 || !lua_isstring(L, 1)) {
> > +		static const char *fmt =
> > +			"%s: expects %s(\'name\')";
> > +		diag_set(IllegalParams, fmt, method, method);
> 
> 17. I suppose you forgot a return here. And also to cover it
> with a test.

True, thanks!

> 
> > +	}
> > +
> > +	size_t name_len;
> > +	const char *name = lua_tolstring(L, 1, &name_len);
> > +
> > +	struct cfunc *cfunc = cfunc_lookup(name, name_len);
> > +	if (cfunc == NULL) {
> > +		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
> > +		diag_set(IllegalParams, fmt, name);
> > +		return luaT_error(L);
> > +	}
> > +
> > +	lua_State *args_L = luaT_newthread(tarantool_L);
> 
> 18. Wtf? Why can't you use L? Why do you even move anything here
> between the stacks?

Well, actually it comes from lbox_func_call, I couldn't extract this
into a separate helper. To be honest I do not really undertand why
luaT_newthread is used in lbox_func_call but I presume this is the
template of calling external functions in Lua?

> > +++ b/src/box/lua/cfunc.h
> > @@ -0,0 +1,55 @@
> > +#ifndef INCLUDES_TARANTOOL_MOD_BOX_LUA_CFUNC_H
> > +#define INCLUDES_TARANTOOL_MOD_BOX_LUA_CFUNC_H
> 
> 19. In the new code we agreed to use #pragma once.

ok

> > +struct lua_State;
> > +
> > +void
> > +box_lua_cfunc_init(struct lua_State *L);
> > +
> > +int
> > +cfunc_init(void);
> 
> 20. Why do you need 2 inits? All Lua modules always
> have a single init and a single free.

box_lua_cfunc_init needed to register the module inside
Lua machine while cfunc_init allocates internal structures
for cfunc functionaliy (such as hash).

And no -- the lua modules might require additional initialization
as well. See popen for example: it has popen_init and not that
obvious but module initalization from popen.lua module as well.

Since we gonna change API this will be eliminated in cmod.

Thanks huge for review, Vlad!

Re: [Tarantool-patches] [PATCH v4 0/6] box/func: implement cfunc Lua module

[Cyrill Gorcunov]

On Sat, Oct 03, 2020 at 03:55:23PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> 
> On 01.10.2020 15:51, Cyrill Gorcunov wrote:
> > The cfunc 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. There was a request to make
> > cfunc been Lua object with appropriate methods. If this is really
> > preferred then I'll implement this wrapping Lua code on top. I mean
> > that currently all operations are done via
> > 
> >  > require('cfunc').[create|drop|call|reload]
> > 
> > interface while there was a proposal to operate as
> > 
> >  > a = require('cfunc').create
> >  > a:call(args)
> 
> No, the proposal was rather
> 
> 	cfunc = require('cfunc')
> 	f = cfunc.load('test_box_c_function')
> 	f(1, 2, 3)
> 
> Sorry I didn't describe it properly anywhere. But it is easy to fix
> anyway.
> 
> In Lua you can define __call and make an object look exactly
> like a function. That is the idea behind making it possible to
> use Lua and C functions in the same way.
> 
> 	if load_c then
> 		f = cfunc.load('test_box_c_function')
> 	else
> 		f = function(a, b, c) return a + b + c end
> 	end
> 	f(1, 2, 3) -- Works and looks the same for C and Lua.
> 
> With this way even IPROTO_CALL should work on such functions, if the
> function object is global and the user has access to executing everything
> here.

OK, so here is a proposal. The module get called cbox, where we will be
implementing all C related features.

cbox.func.create(name)		-- create a function
cbox.func.drop(name)		-- delete a function
cbox.module.reload(name)	-- reload module

f = cbox.func.create(name)
f(1,2,3)			-- create and call a function

Comments?

Re: [Tarantool-patches] [PATCH v4 5/6] box/func: implement cfunc Lua module

[Vladislav Shpilevoy]

Hi! Thanks for the answers!

On 05.10.2020 12:31, Cyrill Gorcunov wrote:
> On Sat, Oct 03, 2020 at 03:55:26PM +0200, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> See 20 comments below.
>>
>> On 01.10.2020 15:51, Cyrill Gorcunov wrote:
>>> Currently to run "C" function from some external module
>>> one have to register it first in "_func" system space. This
>>> is a problem if node is in read-only mode (replica).
>>>
>>> Still people would like to have a way to run such functions
>>> even in ro mode. For this sake we implement "cfunc" lua
>>> module.
>>>
>>> Fixes #4692
>>>
>>> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>>>
>>> @TarantoolBot document
>>> Title: cfunc module
>>
>> 1. The documentation will go to a github issue, where it looks super
>> broken as I checked. Please, try to use github markdown here.
> 
> You know, I've been using sphinx syntax for a purpose -- this commit
> message can be simply extracted and being put into our doc.repo. Otherwise
> it becomes a double work: first we write commit message in own format
> then out docs team need to reformat it to sphinx. Taking into account
> that we simply don't have enough men power we could optimize this procedure.
> Note -- I don't mind to rewrite it in MD format but probably sphinx is better?

I just stated a fact - whatever you post after "TarantoolBot document" goes to
a github issue as is, unedited. Therefore it will be interpreted as github
markdown, unconditionally. So you need to write things to make them readable
in github. If you use sphinx markdown, it won't be displayed as sphinx markdown
anyway. It will be displayed in github markdown.

Talking of the idea about writing docs for copy-pasting, I don't think it can be
done with the current flow of documentation updates. Doc team anyway needs to
find a place to put your text into, translate it to Russian, review and correct
your markdown. Also if we need to write in sphinx style, it means we need to
patch docbot to somehow turn github markdown off, and make other team members
learn this markdown - how to write it and validate. With all that said - why
do we even need a whole documentation team, if this is basically all they do?

I would better let us write more free-style documentation requests, and let the
doc team do their job.

>>> +
>>> +	cfunc = malloc(sizeof(*cfunc) + name_len + 1);
>>> +	if (cfunc == NULL) {
>>> +		diag_set(OutOfMemory, sizeof(*cfunc), "malloc", "cfunc");
>>
>> 12. You allocated 'sizeof(*cfunc) + name_len + 1)', not only sizeof.
> 
> I know, the name of the function is placed right after the structure.
> It is intended.

How is it related? diag_set in the second argument takes allocation
size. You tried to allocate sizeof(*cfunc) + name_len + 1, but
reported sizeof(*cfunc), which is smaller. It does not depend on what
you allocate. Only how much.

>> In that way the idea to rename cfunc to cbox or something like that looks
>> even better. We could provide methods to operate on entire modules. So it
>> would be
>>
>> 	f = cbox.load_func() -- Create a function.
>> 	f:unload()           -- Unload a function.
>> 	cbox.reload_module() -- Reload module and all functions in it.
>> 	cbox.unload_module() -- Unload the module from the memory.
> 
> maybe
> 
> 	f = cmod.create(name)		-- just like in schema.func
> 	f:drop()			-- same
> 	cmod.module('reload', name)
> 	cmod.module('unload', name)
> 
> ? I don't mind to use cbox name either but cmod somehow close to
> what we're doing.

The method name as string 'reload' and 'unload' looks super ugly,
and wouldn't work with autocompletion. I would better go for explicit
methods module_reload and module_unload. Or like you proposed later -
module.reload and module.unload. (Personally I would prefer the first
way, because it would mean less Lua tables, and simpler autocompletion.)

>>> +	}
>>> +
>>> +	size_t name_len;
>>> +	const char *name = lua_tolstring(L, 1, &name_len);
>>> +
>>> +	struct cfunc *cfunc = cfunc_lookup(name, name_len);
>>> +	if (cfunc == NULL) {
>>> +		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
>>> +		diag_set(IllegalParams, fmt, name);
>>> +		return luaT_error(L);
>>> +	}
>>> +
>>> +	lua_State *args_L = luaT_newthread(tarantool_L);
>>
>> 18. Wtf? Why can't you use L? Why do you even move anything here
>> between the stacks?
> 
> Well, actually it comes from lbox_func_call, I couldn't extract this
> into a separate helper. To be honest I do not really undertand why
> luaT_newthread is used in lbox_func_call but I presume this is the
> template of calling external functions in Lua?

We don't copy code presuming that it does something. Lets better
understand why is it needed instead of blindly copying it.
Currently I don't see any reason why is it needed.

Tbh I don't know why is it needed in lbox_func_call() either. The
comment there is super unclear about that. It just says what is
done, and not why.

Re: [Tarantool-patches] [PATCH v4 0/6] box/func: implement cfunc Lua module

[Vladislav Shpilevoy]

Thanks for the answer!

On 05.10.2020 13:51, Cyrill Gorcunov wrote:
> On Sat, Oct 03, 2020 at 03:55:23PM +0200, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patchset!
>>
>> On 01.10.2020 15:51, Cyrill Gorcunov wrote:
>>> The cfunc 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. There was a request to make
>>> cfunc been Lua object with appropriate methods. If this is really
>>> preferred then I'll implement this wrapping Lua code on top. I mean
>>> that currently all operations are done via
>>>
>>>  > require('cfunc').[create|drop|call|reload]
>>>
>>> interface while there was a proposal to operate as
>>>
>>>  > a = require('cfunc').create
>>>  > a:call(args)
>>
>> No, the proposal was rather
>>
>> 	cfunc = require('cfunc')
>> 	f = cfunc.load('test_box_c_function')
>> 	f(1, 2, 3)
>>
>> Sorry I didn't describe it properly anywhere. But it is easy to fix
>> anyway.
>>
>> In Lua you can define __call and make an object look exactly
>> like a function. That is the idea behind making it possible to
>> use Lua and C functions in the same way.
>>
>> 	if load_c then
>> 		f = cfunc.load('test_box_c_function')
>> 	else
>> 		f = function(a, b, c) return a + b + c end
>> 	end
>> 	f(1, 2, 3) -- Works and looks the same for C and Lua.
>>
>> With this way even IPROTO_CALL should work on such functions, if the
>> function object is global and the user has access to executing everything
>> here.
> 
> OK, so here is a proposal. The module get called cbox, where we will be
> implementing all C related features.
> 
> cbox.func.create(name)		-- create a function
> cbox.func.drop(name)		-- delete a function
> cbox.module.reload(name)	-- reload module
> 
> f = cbox.func.create(name)
> f(1,2,3)			-- create and call a function
> 
> Comments?

Looks good.

Re: [Tarantool-patches] [PATCH v4 5/6] box/func: implement cfunc Lua module

[Cyrill Gorcunov]

On Mon, Oct 05, 2020 at 11:59:05PM +0200, Vladislav Shpilevoy wrote:
...
> 
> I just stated a fact - whatever you post after "TarantoolBot document" goes to
> a github issue as is, unedited. Therefore it will be interpreted as github
> markdown, unconditionally. So you need to write things to make them readable
> in github. If you use sphinx markdown, it won't be displayed as sphinx markdown
> anyway. It will be displayed in github markdown.
> 
> Talking of the idea about writing docs for copy-pasting, I don't think it can be
> done with the current flow of documentation updates. Doc team anyway needs to
> find a place to put your text into, translate it to Russian, review and correct
> your markdown. Also if we need to write in sphinx style, it means we need to
> patch docbot to somehow turn github markdown off, and make other team members
> learn this markdown - how to write it and validate. With all that said - why
> do we even need a whole documentation team, if this is basically all they do?
> 
> I would better let us write more free-style documentation requests, and let the
> doc team do their job.

OK

> >>
> >> 12. You allocated 'sizeof(*cfunc) + name_len + 1)', not only sizeof.
> > 
> > I know, the name of the function is placed right after the structure.
> > It is intended.
> 
> How is it related? diag_set in the second argument takes allocation
> size. You tried to allocate sizeof(*cfunc) + name_len + 1, but
> reported sizeof(*cfunc), which is smaller. It does not depend on what
> you allocate. Only how much.

Wait, I got it -- you meant the diag_set report. Sorry misundertood you
in first place. Thanks, will fix.

> >>> +	struct cfunc *cfunc = cfunc_lookup(name, name_len);
> >>> +	if (cfunc == NULL) {
> >>> +		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
> >>> +		diag_set(IllegalParams, fmt, name);
> >>> +		return luaT_error(L);
> >>> +	}
> >>> +
> >>> +	lua_State *args_L = luaT_newthread(tarantool_L);
> >>
> >> 18. Wtf? Why can't you use L? Why do you even move anything here
> >> between the stacks?
> > 
> > Well, actually it comes from lbox_func_call, I couldn't extract this
> > into a separate helper. To be honest I do not really undertand why
> > luaT_newthread is used in lbox_func_call but I presume this is the
> > template of calling external functions in Lua?
> 
> We don't copy code presuming that it does something. Lets better
> understand why is it needed instead of blindly copying it.
> Currently I don't see any reason why is it needed.
> 
> Tbh I don't know why is it needed in lbox_func_call() either. The
> comment there is super unclear about that. It just says what is
> done, and not why.

I presume it is because of coio thread, will take more precise look
once I manage with general api.