* [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore
2021-04-08 16:41 [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches
@ 2021-04-08 16:41 ` Cyrill Gorcunov via Tarantool-patches
2021-04-09 23:31 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 2/6] box/func: module_reload -- drop redundant argument Cyrill Gorcunov via Tarantool-patches
` (6 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-08 16:41 UTC (permalink / raw)
To: tml; +Cc: Vladislav Shpilevoy
In commit 96938fafb (Add hot function reload for C procedures)
an ability to hot reload of modules has been introduced.
When module is been reloaded his functions are resolved to
new symbols but if something went wrong it is supposed
to restore old symbols from the old module.
Actually current code restores only one function and may
crash if there a bunch of functions to restore. Lets fix it.
Fixes #5968
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
changelogs/unreleased/fix-module-reload.md | 8 ++
src/box/func.c | 13 ++-
test/box/CMakeLists.txt | 4 +
test/box/func_restore.result | 95 ++++++++++++++++++++++
test/box/func_restore.test.lua | 50 ++++++++++++
test/box/func_restore1.c | 33 ++++++++
test/box/func_restore2.c | 27 ++++++
test/box/func_restore3.c | 27 ++++++
test/box/func_restore4.c | 27 ++++++
9 files changed, 277 insertions(+), 7 deletions(-)
create mode 100644 changelogs/unreleased/fix-module-reload.md
create mode 100644 test/box/func_restore.result
create mode 100644 test/box/func_restore.test.lua
create mode 100644 test/box/func_restore1.c
create mode 100644 test/box/func_restore2.c
create mode 100644 test/box/func_restore3.c
create mode 100644 test/box/func_restore4.c
diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md
new file mode 100644
index 000000000..e44118aa8
--- /dev/null
+++ b/changelogs/unreleased/fix-module-reload.md
@@ -0,0 +1,8 @@
+## bugfix/core
+
+* Fix crash in case of reloading a compiled module when the
+ new module lacks some of functions which were present in the
+ former code. In turn this event triggers a fallback procedure
+ where we restore old functions but instead of restoring each
+ function we process a sole entry only leading to the crash
+ later when these restored functions are called (gh-5968).
diff --git a/src/box/func.c b/src/box/func.c
index 9909cee45..94b14c56c 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -387,13 +387,14 @@ module_reload(const char *package, const char *package_end, struct module **modu
struct func_c *func, *tmp_func;
rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
+ /* Move immediately for restore sake. */
+ rlist_move(&new_module->funcs, &func->item);
struct func_name name;
func_split_name(func->base.def->name, &name);
func->func = module_sym(new_module, name.sym);
if (func->func == NULL)
goto restore;
func->module = new_module;
- rlist_move(&new_module->funcs, &func->item);
}
module_cache_del(package, package_end);
if (module_cache_put(new_module) != 0)
@@ -403,10 +404,10 @@ module_reload(const char *package, const char *package_end, struct module **modu
return 0;
restore:
/*
- * Some old-dso func can't be load from new module, restore old
- * functions.
+ * Some old functions are not found in the new module,
+ * thus restore all migrated functions back to the original.
*/
- do {
+ rlist_foreach_entry_safe(func, &new_module->funcs, item, tmp_func) {
struct func_name name;
func_split_name(func->base.def->name, &name);
func->func = module_sym(old_module, name.sym);
@@ -420,9 +421,7 @@ module_reload(const char *package, const char *package_end, struct module **modu
}
func->module = old_module;
rlist_move(&old_module->funcs, &func->item);
- } while (func != rlist_first_entry(&old_module->funcs,
- struct func_c, item));
- assert(rlist_empty(&new_module->funcs));
+ }
module_delete(new_module);
return -1;
}
diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 06bfbbe9d..4216a0ba9 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -2,4 +2,8 @@ include_directories(${MSGPUCK_INCLUDE_DIRS})
build_module(function1 function1.c)
build_module(reload1 reload1.c)
build_module(reload2 reload2.c)
+build_module(func_restore1 func_restore1.c)
+build_module(func_restore2 func_restore2.c)
+build_module(func_restore3 func_restore3.c)
+build_module(func_restore4 func_restore4.c)
build_module(tuple_bench tuple_bench.c)
diff --git a/test/box/func_restore.result b/test/box/func_restore.result
new file mode 100644
index 000000000..7cd9e67c4
--- /dev/null
+++ b/test/box/func_restore.result
@@ -0,0 +1,95 @@
+-- test-run result file version 2
+--
+-- Test that compiled C function can be restored
+-- to the former values when new module can't be
+-- loaded for some reason (say there some
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+
+fio = require('fio')
+ | ---
+ | ...
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+ | ---
+ | ...
+
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
+ | ---
+ | ...
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(path_func_restore))
+ | ---
+ | ...
+fio.symlink(path_func_good, path_func_restore)
+ | ---
+ | - true
+ | ...
+
+box.schema.func.create('func_restore.echo_1', {language = "C"})
+ | ---
+ | ...
+box.schema.func.create('func_restore.echo_2', {language = "C"})
+ | ---
+ | ...
+box.schema.func.create('func_restore.echo_3', {language = "C"})
+ | ---
+ | ...
+
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
+ | ---
+ | ...
+
+box.func['func_restore.echo_3']:call()
+ | ---
+ | - 3
+ | ...
+box.func['func_restore.echo_2']:call()
+ | ---
+ | - 2
+ | ...
+box.func['func_restore.echo_1']:call()
+ | ---
+ | - 1
+ | ...
+
+function run_restore(path) \
+ _ = pcall(fio.unlink(path_func_restore)) \
+ fio.symlink(path, path_func_restore) \
+ \
+ local ok, _ = pcall(box.schema.func.reload, "func_restore") \
+ assert(not ok) \
+ \
+ box.func['func_restore.echo_1']:call() \
+ box.func['func_restore.echo_2']:call() \
+ box.func['func_restore.echo_3']:call() \
+end
+ | ---
+ | ...
+
+bad_modules = { \
+ fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \
+}
+ | ---
+ | ...
+
+for k, v in ipairs(bad_modules) do run_restore(v) end
+ | ---
+ | ...
diff --git a/test/box/func_restore.test.lua b/test/box/func_restore.test.lua
new file mode 100644
index 000000000..4bd05769d
--- /dev/null
+++ b/test/box/func_restore.test.lua
@@ -0,0 +1,50 @@
+--
+-- Test that compiled C function can be restored
+-- to the former values when new module can't be
+-- loaded for some reason (say there some
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+fio = require('fio')
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
+
+_ = pcall(fio.unlink(path_func_restore))
+fio.symlink(path_func_good, path_func_restore)
+
+box.schema.func.create('func_restore.echo_1', {language = "C"})
+box.schema.func.create('func_restore.echo_2', {language = "C"})
+box.schema.func.create('func_restore.echo_3', {language = "C"})
+
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
+
+box.func['func_restore.echo_3']:call()
+box.func['func_restore.echo_2']:call()
+box.func['func_restore.echo_1']:call()
+
+function run_restore(path) \
+ _ = pcall(fio.unlink(path_func_restore)) \
+ fio.symlink(path, path_func_restore) \
+ \
+ local ok, _ = pcall(box.schema.func.reload, "func_restore") \
+ assert(not ok) \
+ \
+ box.func['func_restore.echo_1']:call() \
+ box.func['func_restore.echo_2']:call() \
+ box.func['func_restore.echo_3']:call() \
+end
+
+bad_modules = { \
+ fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \
+}
+
+for k, v in ipairs(bad_modules) do run_restore(v) end
diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
new file mode 100644
index 000000000..891ec0136
--- /dev/null
+++ b/test/box/func_restore1.c
@@ -0,0 +1,33 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+static int
+echo_num(box_function_ctx_t *ctx, const char *args,
+ const char *args_end, unsigned int num)
+{
+ char res[16];
+ char *end = mp_encode_uint(res, num);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ return echo_num(ctx, args, args_end, 1);
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ return echo_num(ctx, args, args_end, 2);
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ return echo_num(ctx, args, args_end, 3);
+}
diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c
new file mode 100644
index 000000000..d9d6de8df
--- /dev/null
+++ b/test/box/func_restore2.c
@@ -0,0 +1,27 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+static int
+echo_num(box_function_ctx_t *ctx, const char *args,
+ const char *args_end, unsigned int num)
+{
+ char res[16];
+ char *end = mp_encode_uint(res, num);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ return echo_num(ctx, args, args_end, 1);
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ return echo_num(ctx, args, args_end, 2);
+}
diff --git a/test/box/func_restore3.c b/test/box/func_restore3.c
new file mode 100644
index 000000000..e38b44400
--- /dev/null
+++ b/test/box/func_restore3.c
@@ -0,0 +1,27 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+static int
+echo_num(box_function_ctx_t *ctx, const char *args,
+ const char *args_end, unsigned int num)
+{
+ char res[16];
+ char *end = mp_encode_uint(res, num);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ return echo_num(ctx, args, args_end, 2);
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ return echo_num(ctx, args, args_end, 3);
+}
diff --git a/test/box/func_restore4.c b/test/box/func_restore4.c
new file mode 100644
index 000000000..4e97ff0a0
--- /dev/null
+++ b/test/box/func_restore4.c
@@ -0,0 +1,27 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+static int
+echo_num(box_function_ctx_t *ctx, const char *args,
+ const char *args_end, unsigned int num)
+{
+ char res[16];
+ char *end = mp_encode_uint(res, num);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ return echo_num(ctx, args, args_end, 1);
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ return echo_num(ctx, args, args_end, 3);
+}
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore Cyrill Gorcunov via Tarantool-patches
@ 2021-04-09 23:31 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-10 15:02 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-09 23:31 UTC (permalink / raw)
To: Cyrill Gorcunov, tml
Hi! Thanks for working on this!
See 7 comments below and my diff in the end of the email.
> diff --git a/test/box/func_restore.result b/test/box/func_restore.result
> new file mode 100644
> index 000000000..7cd9e67c4
> --- /dev/null
> +++ b/test/box/func_restore.result
1. Please, name the file according to the rules of tests related to
tickets: 'gh-5968-func-restore.test.lua'.
> @@ -0,0 +1,95 @@
> +-- test-run result file version 2
> +--
> +-- Test that compiled C function can be restored
2. There should be a reference to the ticket: 'gh-5968: ...'.
> +-- to the former values when new module can't be
> +-- loaded for some reason (say there some
> +-- missing functions).
> +--
> +build_path = os.getenv("BUILDDIR")
> + | ---
> + | ...
> +package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
> + | ---
> + | ...
> +
> +fio = require('fio')
> + | ---
> + | ...
> +
> +ext = (jit.os == "OSX" and "dylib" or "so")
> + | ---
> + | ...
> +
> +path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
> + | ---
> + | ...
> +path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
> + | ---
> + | ...
> +
> +_ = pcall(fio.unlink(path_func_restore))
> + | ---
> + | ...
> +fio.symlink(path_func_good, path_func_restore)
> + | ---
> + | - true
> + | ...
> +
> +box.schema.func.create('func_restore.echo_1', {language = "C"})
> + | ---
> + | ...
> +box.schema.func.create('func_restore.echo_2', {language = "C"})
> + | ---
> + | ...
> +box.schema.func.create('func_restore.echo_3', {language = "C"})
> + | ---
> + | ...
> +
> +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
> + | ---
> + | ...
> +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
> + | ---
> + | ...
> +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
> + | ---
> + | ...
> +
> +box.func['func_restore.echo_3']:call()
> + | ---
> + | - 3
> + | ...
> +box.func['func_restore.echo_2']:call()
> + | ---
> + | - 2
> + | ...
> +box.func['func_restore.echo_1']:call()
> + | ---
> + | - 1
> + | ...
> +
> +function run_restore(path) \
> + _ = pcall(fio.unlink(path_func_restore)) \
> + fio.symlink(path, path_func_restore) \
> + \
> + local ok, _ = pcall(box.schema.func.reload, "func_restore") \
> + assert(not ok) \
> + \
> + box.func['func_restore.echo_1']:call() \
> + box.func['func_restore.echo_2']:call() \
> + box.func['func_restore.echo_3']:call() \
3. It would be good to have assertions here that the functions still
return what they used to. And the functions in the bad modules should
return something different. So we would see all the functions didn't
change and still return good values.
> +end
> + | ---
> + | ...
> +
> +bad_modules = { \
> + fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \
> + fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \
> + fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \
> +}
> + | ---
> + | ...
> +
> +for k, v in ipairs(bad_modules) do run_restore(v) end
> + | ---
> + | ...
4. You didn't drop the functions.
> diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
> new file mode 100644
> index 000000000..891ec0136
> --- /dev/null
> +++ b/test/box/func_restore1.c
> @@ -0,0 +1,33 @@
> +#include <stdio.h>
> +#include <stdbool.h>
5. You don't need stdio and stdbool headers.
> +#include <msgpuck.h>
> +
> +#include "module.h"
> +
> +static int
> +echo_num(box_function_ctx_t *ctx, const char *args,
> + const char *args_end, unsigned int num)
6. args and args_end are unused.
> +{
> + char res[16];
> + char *end = mp_encode_uint(res, num);
> + box_return_mp(ctx, res, end);
> + return 0;
> +}
> +
> +int
> +echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> + return echo_num(ctx, args, args_end, 1);
> +}
> +
> +int
> +echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> + return echo_num(ctx, args, args_end, 2);
> +}
> +
> +int
> +echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> + return echo_num(ctx, args, args_end, 3);
> +}
> diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c
> new file mode 100644
> index 000000000..d9d6de8df
> --- /dev/null
> +++ b/test/box/func_restore2.c
> @@ -0,0 +1,27 @@
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <msgpuck.h>
> +
> +#include "module.h"
> +
> +static int
> +echo_num(box_function_ctx_t *ctx, const char *args,
> + const char *args_end, unsigned int num)
> +{
> + char res[16];
> + char *end = mp_encode_uint(res, num);
> + box_return_mp(ctx, res, end);
> + return 0;
> +}
> +
> +int
> +echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> + return echo_num(ctx, args, args_end, 1);
7. It is better to make these functions return something different from
the valid module. For example, make them all return 0. Or even make them
crash.
See below the diff I suggest you to apply. I didn't rename the test files
though. Please, do it locally.
====================
diff --git a/test/box/func_restore.result b/test/box/func_restore.result
index 7cd9e67c4..6c3c7aebc 100644
--- a/test/box/func_restore.result
+++ b/test/box/func_restore.result
@@ -1,14 +1,16 @@
-- test-run result file version 2
--
--- Test that compiled C function can be restored
--- to the former values when new module can't be
--- loaded for some reason (say there some
--- missing functions).
+-- gh-5968: test that compiled C function can be restored to the former values
+-- when new module can't be loaded for some reason (say there some missing
+-- functions).
--
build_path = os.getenv("BUILDDIR")
| ---
| ...
-package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+old_cpath = package.cpath
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..old_cpath
| ---
| ...
@@ -16,14 +18,14 @@ fio = require('fio')
| ---
| ...
-ext = (jit.os == "OSX" and "dylib" or "so")
+ext = (jit.os == "OSX" and ".dylib" or ".so")
| ---
| ...
-path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore") .. ext
| ---
| ...
-path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1") .. ext
| ---
| ...
@@ -55,37 +57,36 @@ box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
| ---
| ...
-box.func['func_restore.echo_3']:call()
+assert(box.func['func_restore.echo_1']:call() == 1)
| ---
- | - 3
+ | - true
| ...
-box.func['func_restore.echo_2']:call()
+assert(box.func['func_restore.echo_2']:call() == 2)
| ---
- | - 2
+ | - true
| ...
-box.func['func_restore.echo_1']:call()
+assert(box.func['func_restore.echo_3']:call() == 3)
| ---
- | - 1
+ | - true
| ...
-function run_restore(path) \
- _ = pcall(fio.unlink(path_func_restore)) \
- fio.symlink(path, path_func_restore) \
- \
- local ok, _ = pcall(box.schema.func.reload, "func_restore") \
- assert(not ok) \
- \
- box.func['func_restore.echo_1']:call() \
- box.func['func_restore.echo_2']:call() \
- box.func['func_restore.echo_3']:call() \
+function run_restore(path) \
+ pcall(fio.unlink(path_func_restore)) \
+ fio.symlink(path, path_func_restore) \
+ \
+ assert(not pcall(box.schema.func.reload, "func_restore")) \
+ \
+ assert(box.func['func_restore.echo_1']:call() == 1) \
+ assert(box.func['func_restore.echo_2']:call() == 2) \
+ assert(box.func['func_restore.echo_3']:call() == 3) \
end
| ---
| ...
-bad_modules = { \
- fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \
- fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \
- fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \
+bad_modules = { \
+ fio.pathjoin(build_path, "test/box/func_restore2") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore3") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore4") .. ext, \
}
| ---
| ...
@@ -93,3 +94,16 @@ bad_modules = { \
for k, v in ipairs(bad_modules) do run_restore(v) end
| ---
| ...
+
+box.schema.func.drop('func_restore.echo_1')
+ | ---
+ | ...
+box.schema.func.drop('func_restore.echo_2')
+ | ---
+ | ...
+box.schema.func.drop('func_restore.echo_3')
+ | ---
+ | ...
+package.cpath = old_cpath
+ | ---
+ | ...
diff --git a/test/box/func_restore.test.lua b/test/box/func_restore.test.lua
index 4bd05769d..478f83631 100644
--- a/test/box/func_restore.test.lua
+++ b/test/box/func_restore.test.lua
@@ -1,18 +1,18 @@
--
--- Test that compiled C function can be restored
--- to the former values when new module can't be
--- loaded for some reason (say there some
--- missing functions).
+-- gh-5968: test that compiled C function can be restored to the former values
+-- when new module can't be loaded for some reason (say there some missing
+-- functions).
--
build_path = os.getenv("BUILDDIR")
-package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+old_cpath = package.cpath
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..old_cpath
fio = require('fio')
-ext = (jit.os == "OSX" and "dylib" or "so")
+ext = (jit.os == "OSX" and ".dylib" or ".so")
-path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext
-path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore") .. ext
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1") .. ext
_ = pcall(fio.unlink(path_func_restore))
fio.symlink(path_func_good, path_func_restore)
@@ -25,26 +25,30 @@ box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
-box.func['func_restore.echo_3']:call()
-box.func['func_restore.echo_2']:call()
-box.func['func_restore.echo_1']:call()
-
-function run_restore(path) \
- _ = pcall(fio.unlink(path_func_restore)) \
- fio.symlink(path, path_func_restore) \
- \
- local ok, _ = pcall(box.schema.func.reload, "func_restore") \
- assert(not ok) \
- \
- box.func['func_restore.echo_1']:call() \
- box.func['func_restore.echo_2']:call() \
- box.func['func_restore.echo_3']:call() \
+assert(box.func['func_restore.echo_1']:call() == 1)
+assert(box.func['func_restore.echo_2']:call() == 2)
+assert(box.func['func_restore.echo_3']:call() == 3)
+
+function run_restore(path) \
+ pcall(fio.unlink(path_func_restore)) \
+ fio.symlink(path, path_func_restore) \
+ \
+ assert(not pcall(box.schema.func.reload, "func_restore")) \
+ \
+ assert(box.func['func_restore.echo_1']:call() == 1) \
+ assert(box.func['func_restore.echo_2']:call() == 2) \
+ assert(box.func['func_restore.echo_3']:call() == 3) \
end
-bad_modules = { \
- fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \
- fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \
- fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \
+bad_modules = { \
+ fio.pathjoin(build_path, "test/box/func_restore2") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore3") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore4") .. ext, \
}
for k, v in ipairs(bad_modules) do run_restore(v) end
+
+box.schema.func.drop('func_restore.echo_1')
+box.schema.func.drop('func_restore.echo_2')
+box.schema.func.drop('func_restore.echo_3')
+package.cpath = old_cpath
diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
index 891ec0136..f2d0aaf78 100644
--- a/test/box/func_restore1.c
+++ b/test/box/func_restore1.c
@@ -1,33 +1,22 @@
-#include <stdio.h>
-#include <stdbool.h>
-#include <msgpuck.h>
-
#include "module.h"
-static int
-echo_num(box_function_ctx_t *ctx, const char *args,
- const char *args_end, unsigned int num)
-{
- char res[16];
- char *end = mp_encode_uint(res, num);
- box_return_mp(ctx, res, end);
- return 0;
-}
-
int
echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- return echo_num(ctx, args, args_end, 1);
+ const char *mp1 = "\x01";
+ return box_return_mp(ctx, mp1, mp1 + 1);
}
int
echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- return echo_num(ctx, args, args_end, 2);
+ const char *mp1 = "\x02";
+ return box_return_mp(ctx, mp1, mp1 + 1);
}
int
echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- return echo_num(ctx, args, args_end, 3);
+ const char *mp1 = "\x03";
+ return box_return_mp(ctx, mp1, mp1 + 1);
}
diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c
index d9d6de8df..ef34be12b 100644
--- a/test/box/func_restore2.c
+++ b/test/box/func_restore2.c
@@ -1,27 +1,16 @@
-#include <stdio.h>
-#include <stdbool.h>
-#include <msgpuck.h>
-
+#include "stdlib.h"
#include "module.h"
-static int
-echo_num(box_function_ctx_t *ctx, const char *args,
- const char *args_end, unsigned int num)
-{
- char res[16];
- char *end = mp_encode_uint(res, num);
- box_return_mp(ctx, res, end);
- return 0;
-}
-
int
echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- return echo_num(ctx, args, args_end, 1);
+ abort();
+ return 0;
}
int
echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- return echo_num(ctx, args, args_end, 2);
+ abort();
+ return 0;
}
diff --git a/test/box/func_restore3.c b/test/box/func_restore3.c
index e38b44400..fd6f6f1e3 100644
--- a/test/box/func_restore3.c
+++ b/test/box/func_restore3.c
@@ -1,27 +1,16 @@
-#include <stdio.h>
-#include <stdbool.h>
-#include <msgpuck.h>
-
+#include "stdlib.h"
#include "module.h"
-static int
-echo_num(box_function_ctx_t *ctx, const char *args,
- const char *args_end, unsigned int num)
-{
- char res[16];
- char *end = mp_encode_uint(res, num);
- box_return_mp(ctx, res, end);
- return 0;
-}
-
int
echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- return echo_num(ctx, args, args_end, 2);
+ abort();
+ return 0;
}
int
echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- return echo_num(ctx, args, args_end, 3);
+ abort();
+ return 0;
}
diff --git a/test/box/func_restore4.c b/test/box/func_restore4.c
index 4e97ff0a0..2ebd448f0 100644
--- a/test/box/func_restore4.c
+++ b/test/box/func_restore4.c
@@ -1,27 +1,16 @@
-#include <stdio.h>
-#include <stdbool.h>
-#include <msgpuck.h>
-
+#include "stdlib.h"
#include "module.h"
-static int
-echo_num(box_function_ctx_t *ctx, const char *args,
- const char *args_end, unsigned int num)
-{
- char res[16];
- char *end = mp_encode_uint(res, num);
- box_return_mp(ctx, res, end);
- return 0;
-}
-
int
echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- return echo_num(ctx, args, args_end, 1);
+ abort();
+ return 0;
}
int
echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- return echo_num(ctx, args, args_end, 3);
+ abort();
+ return 0;
}
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore
2021-04-09 23:31 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-10 15:02 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 0 replies; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-10 15:02 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tml
On Sat, Apr 10, 2021 at 01:31:06AM +0200, Vladislav Shpilevoy wrote:
...
>
> 7. It is better to make these functions return something different from
> the valid module. For example, make them all return 0. Or even make them
> crash.
>
> See below the diff I suggest you to apply. I didn't rename the test files
> though. Please, do it locally.
...
Thanks a huge for review, Vlad! I updated the series and force pushed it.
Here is a new version.
---
In commit 96938fafb (Add hot function reload for C procedures)
an ability to hot reload of modules has been introduced.
When module is been reloaded his functions are resolved to
new symbols but if something went wrong it is supposed
to restore old symbols from the old module.
Actually current code restores only one function and may
crash if there a bunch of functions to restore. Lets fix it.
Fixes #5968
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
changelogs/unreleased/fix-module-reload.md | 8 ++
src/box/func.c | 13 ++-
test/box/CMakeLists.txt | 4 +
test/box/func_restore1.c | 22 ++++
test/box/func_restore2.c | 17 ++++
test/box/func_restore3.c | 17 ++++
test/box/func_restore4.c | 17 ++++
test/box/gh-5968-func-restore.result | 111 +++++++++++++++++++++
test/box/gh-5968-func-restore.test.lua | 56 +++++++++++
9 files changed, 258 insertions(+), 7 deletions(-)
create mode 100644 changelogs/unreleased/fix-module-reload.md
create mode 100644 test/box/func_restore1.c
create mode 100644 test/box/func_restore2.c
create mode 100644 test/box/func_restore3.c
create mode 100644 test/box/func_restore4.c
create mode 100644 test/box/gh-5968-func-restore.result
create mode 100644 test/box/gh-5968-func-restore.test.lua
diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md
new file mode 100644
index 000000000..e44118aa8
--- /dev/null
+++ b/changelogs/unreleased/fix-module-reload.md
@@ -0,0 +1,8 @@
+## bugfix/core
+
+* Fix crash in case of reloading a compiled module when the
+ new module lacks some of functions which were present in the
+ former code. In turn this event triggers a fallback procedure
+ where we restore old functions but instead of restoring each
+ function we process a sole entry only leading to the crash
+ later when these restored functions are called (gh-5968).
diff --git a/src/box/func.c b/src/box/func.c
index 9909cee45..94b14c56c 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -387,13 +387,14 @@ module_reload(const char *package, const char *package_end, struct module **modu
struct func_c *func, *tmp_func;
rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
+ /* Move immediately for restore sake. */
+ rlist_move(&new_module->funcs, &func->item);
struct func_name name;
func_split_name(func->base.def->name, &name);
func->func = module_sym(new_module, name.sym);
if (func->func == NULL)
goto restore;
func->module = new_module;
- rlist_move(&new_module->funcs, &func->item);
}
module_cache_del(package, package_end);
if (module_cache_put(new_module) != 0)
@@ -403,10 +404,10 @@ module_reload(const char *package, const char *package_end, struct module **modu
return 0;
restore:
/*
- * Some old-dso func can't be load from new module, restore old
- * functions.
+ * Some old functions are not found in the new module,
+ * thus restore all migrated functions back to the original.
*/
- do {
+ rlist_foreach_entry_safe(func, &new_module->funcs, item, tmp_func) {
struct func_name name;
func_split_name(func->base.def->name, &name);
func->func = module_sym(old_module, name.sym);
@@ -420,9 +421,7 @@ module_reload(const char *package, const char *package_end, struct module **modu
}
func->module = old_module;
rlist_move(&old_module->funcs, &func->item);
- } while (func != rlist_first_entry(&old_module->funcs,
- struct func_c, item));
- assert(rlist_empty(&new_module->funcs));
+ }
module_delete(new_module);
return -1;
}
diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 06bfbbe9d..4216a0ba9 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -2,4 +2,8 @@ include_directories(${MSGPUCK_INCLUDE_DIRS})
build_module(function1 function1.c)
build_module(reload1 reload1.c)
build_module(reload2 reload2.c)
+build_module(func_restore1 func_restore1.c)
+build_module(func_restore2 func_restore2.c)
+build_module(func_restore3 func_restore3.c)
+build_module(func_restore4 func_restore4.c)
build_module(tuple_bench tuple_bench.c)
diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
new file mode 100644
index 000000000..f2d0aaf78
--- /dev/null
+++ b/test/box/func_restore1.c
@@ -0,0 +1,22 @@
+#include "module.h"
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ const char *mp1 = "\x01";
+ return box_return_mp(ctx, mp1, mp1 + 1);
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ const char *mp1 = "\x02";
+ return box_return_mp(ctx, mp1, mp1 + 1);
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ const char *mp1 = "\x03";
+ return box_return_mp(ctx, mp1, mp1 + 1);
+}
diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c
new file mode 100644
index 000000000..c77ff4b42
--- /dev/null
+++ b/test/box/func_restore2.c
@@ -0,0 +1,17 @@
+#include <stdlib.h>
+
+#include "module.h"
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ abort();
+ return 0;
+}
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ abort();
+ return 0;
+}
diff --git a/test/box/func_restore3.c b/test/box/func_restore3.c
new file mode 100644
index 000000000..7dea37817
--- /dev/null
+++ b/test/box/func_restore3.c
@@ -0,0 +1,17 @@
+#include <stdlib.h>
+
+#include "module.h"
+
+int
+echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ abort();
+ return 0;
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ abort();
+ return 0;
+}
diff --git a/test/box/func_restore4.c b/test/box/func_restore4.c
new file mode 100644
index 000000000..545d9e8e3
--- /dev/null
+++ b/test/box/func_restore4.c
@@ -0,0 +1,17 @@
+#include <stdlib.h>
+
+#include "module.h"
+
+int
+echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ abort();
+ return 0;
+}
+
+int
+echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ abort();
+ return 0;
+}
diff --git a/test/box/gh-5968-func-restore.result b/test/box/gh-5968-func-restore.result
new file mode 100644
index 000000000..0914e780a
--- /dev/null
+++ b/test/box/gh-5968-func-restore.result
@@ -0,0 +1,111 @@
+-- test-run result file version 2
+--
+-- gh-5968: Test that compiled C function can be restored to the former
+-- values when a new module can't be loaded for some reason (say there are
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+old_cpath = package.cpath
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+
+fio = require('fio')
+ | ---
+ | ...
+
+ext = (jit.os == "OSX" and ".dylib" or ".so")
+ | ---
+ | ...
+
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore") .. ext
+ | ---
+ | ...
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1") .. ext
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(path_func_restore))
+ | ---
+ | ...
+fio.symlink(path_func_good, path_func_restore)
+ | ---
+ | - true
+ | ...
+
+box.schema.func.create('func_restore.echo_1', {language = "C"})
+ | ---
+ | ...
+box.schema.func.create('func_restore.echo_2', {language = "C"})
+ | ---
+ | ...
+box.schema.func.create('func_restore.echo_3', {language = "C"})
+ | ---
+ | ...
+
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
+ | ---
+ | ...
+
+assert(box.func['func_restore.echo_3']:call() == 3)
+ | ---
+ | - true
+ | ...
+assert(box.func['func_restore.echo_2']:call() == 2)
+ | ---
+ | - true
+ | ...
+assert(box.func['func_restore.echo_1']:call() == 1)
+ | ---
+ | - true
+ | ...
+
+function run_restore(path) \
+ pcall(fio.unlink(path_func_restore)) \
+ fio.symlink(path, path_func_restore) \
+ \
+ local ok, _ = pcall(box.schema.func.reload, "func_restore") \
+ assert(not ok) \
+ \
+ assert(box.func['func_restore.echo_1']:call() == 1) \
+ assert(box.func['func_restore.echo_2']:call() == 2) \
+ assert(box.func['func_restore.echo_3']:call() == 3) \
+end
+ | ---
+ | ...
+
+bad_modules = { \
+ fio.pathjoin(build_path, "test/box/func_restore2") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore3") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore4") .. ext, \
+}
+ | ---
+ | ...
+
+for k, v in ipairs(bad_modules) do run_restore(v) end
+ | ---
+ | ...
+
+box.schema.func.drop('func_restore.echo_1')
+ | ---
+ | ...
+box.schema.func.drop('func_restore.echo_2')
+ | ---
+ | ...
+box.schema.func.drop('func_restore.echo_3')
+ | ---
+ | ...
+
+package.cpath = old_cpath
+ | ---
+ | ...
diff --git a/test/box/gh-5968-func-restore.test.lua b/test/box/gh-5968-func-restore.test.lua
new file mode 100644
index 000000000..c710db8b4
--- /dev/null
+++ b/test/box/gh-5968-func-restore.test.lua
@@ -0,0 +1,56 @@
+--
+-- gh-5968: Test that compiled C function can be restored to the former
+-- values when a new module can't be loaded for some reason (say there are
+-- missing functions).
+--
+build_path = os.getenv("BUILDDIR")
+old_cpath = package.cpath
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+fio = require('fio')
+
+ext = (jit.os == "OSX" and ".dylib" or ".so")
+
+path_func_restore = fio.pathjoin(build_path, "test/box/func_restore") .. ext
+path_func_good = fio.pathjoin(build_path, "test/box/func_restore1") .. ext
+
+_ = pcall(fio.unlink(path_func_restore))
+fio.symlink(path_func_good, path_func_restore)
+
+box.schema.func.create('func_restore.echo_1', {language = "C"})
+box.schema.func.create('func_restore.echo_2', {language = "C"})
+box.schema.func.create('func_restore.echo_3', {language = "C"})
+
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3')
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2')
+box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1')
+
+assert(box.func['func_restore.echo_3']:call() == 3)
+assert(box.func['func_restore.echo_2']:call() == 2)
+assert(box.func['func_restore.echo_1']:call() == 1)
+
+function run_restore(path) \
+ pcall(fio.unlink(path_func_restore)) \
+ fio.symlink(path, path_func_restore) \
+ \
+ local ok, _ = pcall(box.schema.func.reload, "func_restore") \
+ assert(not ok) \
+ \
+ assert(box.func['func_restore.echo_1']:call() == 1) \
+ assert(box.func['func_restore.echo_2']:call() == 2) \
+ assert(box.func['func_restore.echo_3']:call() == 3) \
+end
+
+bad_modules = { \
+ fio.pathjoin(build_path, "test/box/func_restore2") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore3") .. ext, \
+ fio.pathjoin(build_path, "test/box/func_restore4") .. ext, \
+}
+
+for k, v in ipairs(bad_modules) do run_restore(v) end
+
+box.schema.func.drop('func_restore.echo_1')
+box.schema.func.drop('func_restore.echo_2')
+box.schema.func.drop('func_restore.echo_3')
+
+package.cpath = old_cpath
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Tarantool-patches] [PATCH v21 2/6] box/func: module_reload -- drop redundant argument
2021-04-08 16:41 [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore Cyrill Gorcunov via Tarantool-patches
@ 2021-04-08 16:41 ` Cyrill Gorcunov via Tarantool-patches
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 3/6] box/module_cache: introduce modules subsystem Cyrill Gorcunov via Tarantool-patches
` (5 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-08 16:41 UTC (permalink / raw)
To: tml; +Cc: Vladislav Shpilevoy
The only purpose of the module argument is to
notify the caller that the module doesn't exist.
Lets simplify the code and drop this argument.
Part-of #4642
Acked-by: Serge Petrenko <sergepetrenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/box/call.c | 9 +--------
src/box/func.c | 7 +++----
src/box/func.h | 3 +--
3 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/src/box/call.c b/src/box/call.c
index 9c291260e..7839e1f3e 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -128,14 +128,7 @@ box_module_reload(const char *name)
user->def->name);
return -1;
}
- struct module *module = NULL;
- if (module_reload(name, name + strlen(name), &module) == 0) {
- if (module != NULL)
- return 0;
- else
- diag_set(ClientError, ER_NO_SUCH_MODULE, name);
- }
- return -1;
+ return module_reload(name, name + strlen(name));
}
int
diff --git a/src/box/func.c b/src/box/func.c
index 94b14c56c..5fdefe349 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -372,13 +372,13 @@ module_sym(struct module *module, const char *name)
}
int
-module_reload(const char *package, const char *package_end, struct module **module)
+module_reload(const char *package, const char *package_end)
{
struct module *old_module = module_cache_find(package, package_end);
if (old_module == NULL) {
/* Module wasn't loaded - do nothing. */
- *module = NULL;
- return 0;
+ diag_set(ClientError, ER_NO_SUCH_MODULE, package);
+ return -1;
}
struct module *new_module = module_load(package, package_end);
@@ -400,7 +400,6 @@ module_reload(const char *package, const char *package_end, struct module **modu
if (module_cache_put(new_module) != 0)
goto restore;
module_gc(old_module);
- *module = new_module;
return 0;
restore:
/*
diff --git a/src/box/func.h b/src/box/func.h
index 581e468cb..0a08fa465 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -113,12 +113,11 @@ func_call(struct func *func, struct port *args, struct port *ret);
*
* @param package name begin pointer.
* @param package_end package_end name end pointer.
- * @param[out] module a pointer to store module object on success.
* @retval -1 on error.
* @retval 0 on success.
*/
int
-module_reload(const char *package, const char *package_end, struct module **module);
+module_reload(const char *package, const char *package_end);
#if defined(__cplusplus)
} /* extern "C" */
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread* [Tarantool-patches] [PATCH v21 3/6] box/module_cache: introduce modules subsystem
2021-04-08 16:41 [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore Cyrill Gorcunov via Tarantool-patches
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 2/6] box/func: module_reload -- drop redundant argument Cyrill Gorcunov via Tarantool-patches
@ 2021-04-08 16:41 ` Cyrill Gorcunov via Tarantool-patches
2021-04-09 23:54 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 4/6] box/schema.func: switch to new module api Cyrill Gorcunov via Tarantool-patches
` (4 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-08 16:41 UTC (permalink / raw)
To: tml; +Cc: Vladislav Shpilevoy
The modules subsystem hides some low-level operations under API.
In particular the modules subsystem is responsible for
- modules lookup in Lua's "package.search" storage
- modules caching to eliminate expensive load procedure
- function symbol resolving
Because naming is intersecting with current module functions
sitting in box/func, lets rename the later to schema_module
prefix. We will use this prefix in next patches to point the
modules in box.schema.func are just a particular user of
the general modules engine.
Part-of #4642
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/box/CMakeLists.txt | 1 +
src/box/box.cc | 4 +-
src/box/call.c | 2 +-
src/box/func.c | 6 +-
src/box/func.h | 12 +-
src/box/module_cache.c | 491 +++++++++++++++++++++++++++++++++++++++++
src/box/module_cache.h | 205 +++++++++++++++++
src/main.cc | 3 +
8 files changed, 712 insertions(+), 12 deletions(-)
create mode 100644 src/box/module_cache.c
create mode 100644 src/box/module_cache.h
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 19203f770..cc2e17e94 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -126,6 +126,7 @@ add_library(box STATIC
memtx_rtree.c
memtx_bitset.c
memtx_tx.c
+ module_cache.c
engine.c
memtx_engine.c
memtx_space.c
diff --git a/src/box/box.cc b/src/box/box.cc
index b846ba8f5..a825157be 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2771,7 +2771,7 @@ box_free(void)
session_free();
user_cache_free();
schema_free();
- module_free();
+ schema_module_free();
tuple_free();
port_free();
#endif
@@ -3194,7 +3194,7 @@ box_init(void)
*/
session_init();
- if (module_init() != 0)
+ if (schema_module_init() != 0)
diag_raise();
if (tuple_init(lua_hash) != 0)
diff --git a/src/box/call.c b/src/box/call.c
index 7839e1f3e..a6384efe2 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -128,7 +128,7 @@ box_module_reload(const char *name)
user->def->name);
return -1;
}
- return module_reload(name, name + strlen(name));
+ return schema_module_reload(name, name + strlen(name));
}
int
diff --git a/src/box/func.c b/src/box/func.c
index 5fdefe349..10b916e6d 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -172,7 +172,7 @@ static void
module_gc(struct module *module);
int
-module_init(void)
+schema_module_init(void)
{
modules = mh_strnptr_new();
if (modules == NULL) {
@@ -184,7 +184,7 @@ module_init(void)
}
void
-module_free(void)
+schema_module_free(void)
{
while (mh_size(modules) > 0) {
mh_int_t i = mh_first(modules);
@@ -372,7 +372,7 @@ module_sym(struct module *module, const char *name)
}
int
-module_reload(const char *package, const char *package_end)
+schema_module_reload(const char *package, const char *package_end)
{
struct module *old_module = module_cache_find(package, package_end);
if (old_module == NULL) {
diff --git a/src/box/func.h b/src/box/func.h
index 0a08fa465..5a49e34f4 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -85,16 +85,16 @@ struct func {
};
/**
- * Initialize modules subsystem.
+ * Initialize schema modules subsystem.
*/
int
-module_init(void);
+schema_module_init(void);
/**
- * Cleanup modules subsystem.
+ * Cleanup schema modules subsystem.
*/
void
-module_free(void);
+schema_module_free(void);
struct func *
func_new(struct func_def *def);
@@ -109,7 +109,7 @@ int
func_call(struct func *func, struct port *args, struct port *ret);
/**
- * Reload dynamically loadable module.
+ * Reload dynamically loadable schema module.
*
* @param package name begin pointer.
* @param package_end package_end name end pointer.
@@ -117,7 +117,7 @@ func_call(struct func *func, struct port *args, struct port *ret);
* @retval 0 on success.
*/
int
-module_reload(const char *package, const char *package_end);
+schema_module_reload(const char *package, const char *package_end);
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/src/box/module_cache.c b/src/box/module_cache.c
new file mode 100644
index 000000000..3dcb40566
--- /dev/null
+++ b/src/box/module_cache.c
@@ -0,0 +1,491 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#include <unistd.h>
+#include <string.h>
+#include <fcntl.h>
+#include <dlfcn.h>
+#include <lua.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include "assoc.h"
+#include "diag.h"
+#include "fiber.h"
+#include "errinj.h"
+#include "module_cache.h"
+
+#include "box/error.h"
+#include "box/port.h"
+
+#include "lua/utils.h"
+#include "libeio/eio.h"
+
+static struct mh_strnptr_t *module_cache = NULL;
+
+/**
+ * Helpers for cache manipulations.
+ */
+static struct module *
+cache_find(const char *str, size_t len)
+{
+ mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
+ if (e == mh_end(module_cache))
+ return NULL;
+ return mh_strnptr_node(module_cache, e)->val;
+}
+
+static void
+cache_update(struct module *m)
+{
+ const char *str = m->package;
+ size_t len = m->package_len;
+
+ mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
+ if (e == mh_end(module_cache))
+ panic("module: failed to update cache: %s", str);
+
+ mh_strnptr_node(module_cache, e)->str = m->package;
+ mh_strnptr_node(module_cache, e)->val = m;
+}
+
+static int
+cache_put(struct module *m)
+{
+ const struct mh_strnptr_node_t nd = {
+ .str = m->package,
+ .len = m->package_len,
+ .hash = mh_strn_hash(m->package, m->package_len),
+ .val = m,
+ };
+
+ struct mh_strnptr_node_t prev;
+ struct mh_strnptr_node_t *prev_ptr = &prev;
+
+ mh_int_t e = mh_strnptr_put(module_cache, &nd, &prev_ptr, NULL);
+ if (e == mh_end(module_cache)) {
+ diag_set(OutOfMemory, sizeof(nd), "malloc",
+ "module_cache node");
+ return -1;
+ }
+
+ /*
+ * Just to make sure we haven't replaced something, the
+ * entries must be explicitly deleted.
+ */
+ assert(prev_ptr == NULL);
+ return 0;
+}
+
+static void
+cache_del(struct module *m)
+{
+ const char *str = m->package;
+ size_t len = m->package_len;
+
+ mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
+ if (e != mh_end(module_cache)) {
+ struct module *v = mh_strnptr_node(module_cache, e)->val;
+ if (v == m) {
+ /*
+ * The module in cache might be updated
+ * via force load and old instance is kept
+ * by a reference only.
+ */
+ mh_strnptr_del(module_cache, e, NULL);
+ }
+ }
+}
+
+/** Arguments for lpackage_search. */
+struct find_ctx {
+ const char *package;
+ size_t package_len;
+ char *path;
+ size_t path_len;
+};
+
+/** A helper for find_package(). */
+static int
+lpackage_search(lua_State *L)
+{
+ struct find_ctx *ctx = (void *)lua_topointer(L, 1);
+
+ lua_getglobal(L, "package");
+ lua_getfield(L, -1, "search");
+ lua_pushlstring(L, ctx->package, ctx->package_len);
+
+ lua_call(L, 1, 1);
+ if (lua_isnil(L, -1))
+ return luaL_error(L, "module not found");
+
+ char resolved[PATH_MAX];
+ if (realpath(lua_tostring(L, -1), resolved) == NULL) {
+ diag_set(SystemError, "realpath");
+ return luaT_error(L);
+ }
+
+ /*
+ * No need for result being trimmed test, it
+ * is guaranteed by realpath call.
+ */
+ snprintf(ctx->path, ctx->path_len, "%s", resolved);
+ return 0;
+}
+
+/** Find package in Lua's "package.search". */
+static int
+find_package(const char *package, size_t package_len,
+ char *path, size_t path_len)
+{
+ struct find_ctx ctx = {
+ .package = package,
+ .package_len = package_len,
+ .path = path,
+ .path_len = path_len,
+ };
+
+ struct lua_State *L = tarantool_L;
+ int top = lua_gettop(L);
+ if (luaT_cpcall(L, lpackage_search, &ctx) != 0) {
+ diag_set(ClientError, ER_LOAD_MODULE, ctx.package_len,
+ ctx.package, lua_tostring(L, -1));
+ lua_settop(L, top);
+ return -1;
+ }
+ assert(top == lua_gettop(L));
+ return 0;
+}
+
+void
+module_ref(struct module *m)
+{
+ assert(m->refs >= 0);
+ ++m->refs;
+}
+
+void
+module_unref(struct module *m)
+{
+ assert(m->refs > 0);
+ if (--m->refs == 0) {
+ struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
+ if (e != NULL)
+ --e->iparam;
+ cache_del(m);
+ dlclose(m->handle);
+ TRASH(m);
+ free(m);
+ }
+}
+
+int
+module_func_load(struct module *m, const char *func_name,
+ struct module_func *mf)
+{
+ void *sym = dlsym(m->handle, func_name);
+ if (sym == NULL) {
+ diag_set(ClientError, ER_LOAD_FUNCTION,
+ func_name, dlerror());
+ return -1;
+ }
+
+ mf->func = sym;
+ mf->module = m;
+ module_ref(m);
+
+ return 0;
+}
+
+void
+module_func_unload(struct module_func *mf)
+{
+ module_unref(mf->module);
+ /*
+ * Strictly speaking there is no need
+ * for implicit creation, it is up to
+ * the caller to clear the module function,
+ * but since it is cheap, lets prevent from
+ * even potential use after free.
+ */
+ module_func_create(mf);
+}
+
+int
+module_func_call(struct module_func *mf, struct port *args,
+ struct port *ret)
+{
+ struct region *region = &fiber()->gc;
+ size_t region_svp = region_used(region);
+
+ uint32_t data_sz;
+ const char *data = port_get_msgpack(args, &data_sz);
+ if (data == NULL)
+ return -1;
+
+ port_c_create(ret);
+ box_function_ctx_t ctx = {
+ .port = ret,
+ };
+
+ /*
+ * We don't know what exactly the callee
+ * gonna do during the execution, it may
+ * even try to unload itself, thus we make
+ * sure the dso won't be unloaded until
+ * execution is complete.
+ *
+ * Moreover the callee might release the memory
+ * associated with the module_func pointer itself
+ * so keep the address of the module locally.
+ */
+ struct module *m = mf->module;
+ module_ref(m);
+ int rc = mf->func(&ctx, data, data + data_sz);
+ module_unref(m);
+
+ region_truncate(region, region_svp);
+
+ if (rc != 0) {
+ if (diag_last_error(&fiber()->diag) == NULL)
+ diag_set(ClientError, ER_PROC_C, "unknown error");
+ port_destroy(ret);
+ return -1;
+ }
+
+ return 0;
+}
+
+/** Fill attributes from stat. */
+static void
+module_attr_fill(struct module_attr *attr, struct stat *st)
+{
+ memset(attr, 0, sizeof(*attr));
+
+ attr->st_dev = (uint64_t)st->st_dev;
+ attr->st_ino = (uint64_t)st->st_ino;
+ attr->st_size = (uint64_t)st->st_size;
+#ifdef TARGET_OS_DARWIN
+ attr->tv_sec = (uint64_t)st->st_mtimespec.tv_sec;
+ attr->tv_nsec = (uint64_t)st->st_mtimespec.tv_nsec;
+#else
+ attr->tv_sec = (uint64_t)st->st_mtim.tv_sec;
+ attr->tv_nsec = (uint64_t)st->st_mtim.tv_nsec;
+#endif
+}
+
+/**
+ * Copy shared library to temp directory and load from there,
+ * then remove it from this temp place leaving in memory. This
+ * is because there was a bug in libc which screw file updates
+ * detection properly such that next dlopen call simply return
+ * a cached version instead of rereading a library from the disk.
+ *
+ * We keep own copy of file attributes and reload the library
+ * on demand.
+ */
+static struct module *
+module_new(const char *package, size_t package_len,
+ const char *source_path)
+{
+ size_t size = sizeof(struct module) + package_len + 1;
+ struct module *m = malloc(size);
+ if (m == NULL) {
+ diag_set(OutOfMemory, size, "malloc", "module");
+ return NULL;
+ }
+
+ m->package_len = package_len;
+ m->refs = 0;
+
+ memcpy(m->package, package, package_len);
+ m->package[package_len] = 0;
+
+ const char *tmpdir = getenv("TMPDIR");
+ if (tmpdir == NULL)
+ tmpdir = "/tmp";
+
+ char dir_name[PATH_MAX];
+ int rc = snprintf(dir_name, sizeof(dir_name),
+ "%s/tntXXXXXX", tmpdir);
+ if (rc < 0 || (size_t)rc >= sizeof(dir_name)) {
+ diag_set(SystemError, "failed to generate path to tmp dir");
+ goto error;
+ }
+
+ if (mkdtemp(dir_name) == NULL) {
+ diag_set(SystemError, "failed to create unique dir name: %s",
+ dir_name);
+ goto error;
+ }
+
+ char load_name[PATH_MAX];
+ rc = snprintf(load_name, sizeof(load_name),
+ "%s/%.*s." TARANTOOL_LIBEXT,
+ dir_name, (int)package_len, package);
+ if (rc < 0 || (size_t)rc >= sizeof(dir_name)) {
+ diag_set(SystemError, "failed to generate path to dso");
+ goto error;
+ }
+
+ struct stat st;
+ if (stat(source_path, &st) < 0) {
+ diag_set(SystemError, "failed to stat() module: %s",
+ source_path);
+ goto error;
+ }
+ module_attr_fill(&m->attr, &st);
+
+ int source_fd = open(source_path, O_RDONLY);
+ if (source_fd < 0) {
+ diag_set(SystemError, "failed to open module %s "
+ "file for reading", source_path);
+ goto error;
+ }
+ int dest_fd = open(load_name, O_WRONLY | O_CREAT | O_TRUNC,
+ st.st_mode & 0777);
+ if (dest_fd < 0) {
+ diag_set(SystemError, "failed to open file %s "
+ "for writing ", load_name);
+ close(source_fd);
+ goto error;
+ }
+
+ off_t ret = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size);
+ close(source_fd);
+ close(dest_fd);
+ if (ret != st.st_size) {
+ diag_set(SystemError, "failed to copy dso %s to %s",
+ source_path, load_name);
+ goto error;
+ }
+
+ m->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
+ if (unlink(load_name) != 0)
+ say_warn("failed to unlink dso link: %s", load_name);
+ if (rmdir(dir_name) != 0)
+ say_warn("failed to delete temporary dir: %s", dir_name);
+ if (m->handle == NULL) {
+ diag_set(ClientError, ER_LOAD_MODULE, package_len,
+ package, dlerror());
+ goto error;
+ }
+
+ struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
+ if (e != NULL)
+ ++e->iparam;
+
+ module_ref(m);
+ return m;
+
+error:
+ free(m);
+ return NULL;
+}
+
+struct module *
+module_load_force(const char *package, size_t package_len)
+{
+ char path[PATH_MAX];
+ size_t size = sizeof(path);
+
+ if (find_package(package, package_len, path, size) != 0)
+ return NULL;
+
+ struct module *m = module_new(package, package_len, path);
+ if (m == NULL)
+ return NULL;
+
+ struct module *c = cache_find(package, package_len);
+ if (c != NULL) {
+ cache_update(m);
+ } else {
+ if (cache_put(m) != 0) {
+ module_unload(m);
+ return NULL;
+ }
+ }
+
+ return m;
+}
+
+struct module *
+module_load(const char *package, size_t package_len)
+{
+ char path[PATH_MAX];
+
+ if (find_package(package, package_len, path, sizeof(path)) != 0)
+ return NULL;
+
+ struct module *m = cache_find(package, package_len);
+ if (m != NULL) {
+ struct module_attr attr;
+ struct stat st;
+ if (stat(path, &st) != 0) {
+ diag_set(SystemError, "failed to stat() %s", path);
+ return NULL;
+ }
+
+ /*
+ * In case of cache hit we may reuse existing
+ * module which speedup load procedure.
+ */
+ module_attr_fill(&attr, &st);
+ if (memcmp(&attr, &m->attr, sizeof(attr)) == 0) {
+ module_ref(m);
+ return m;
+ }
+
+ /*
+ * Module has been updated on a storage device,
+ * so load a new instance and update the cache,
+ * old entry get evicted but continue residing
+ * in memory, fully functional, until last
+ * function is unloaded.
+ */
+ m = module_new(package, package_len, path);
+ if (m != NULL)
+ cache_update(m);
+ } else {
+ m = module_new(package, package_len, path);
+ if (m != NULL && cache_put(m) != 0) {
+ module_unload(m);
+ return NULL;
+ }
+ }
+
+ return m;
+}
+
+void
+module_unload(struct module *m)
+{
+ module_unref(m);
+}
+
+void
+module_free(void)
+{
+ while (mh_size(module_cache) > 0) {
+ mh_int_t e = mh_first(module_cache);
+ mh_strnptr_del(module_cache, e, NULL);
+ }
+ mh_strnptr_delete(module_cache);
+ module_cache = NULL;
+}
+
+int
+module_init(void)
+{
+ module_cache = mh_strnptr_new();
+ if (module_cache == NULL) {
+ diag_set(OutOfMemory, sizeof(*module_cache),
+ "malloc", "module_cache");
+ return -1;
+ }
+ return 0;
+}
diff --git a/src/box/module_cache.h b/src/box/module_cache.h
new file mode 100644
index 000000000..24fdf8d85
--- /dev/null
+++ b/src/box/module_cache.h
@@ -0,0 +1,205 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#pragma once
+
+#include <stdint.h>
+
+#include "trivia/config.h"
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+/**
+ * API of C stored function.
+ */
+
+struct port;
+
+struct box_function_ctx {
+ struct port *port;
+};
+
+typedef struct box_function_ctx box_function_ctx_t;
+typedef int (*box_function_t)(box_function_ctx_t *ctx,
+ const char *args,
+ const char *args_end);
+
+/**
+ * Shared library file attributes for
+ * module cache invalidation.
+ */
+struct module_attr {
+ uint64_t st_dev;
+ uint64_t st_ino;
+ uint64_t st_size;
+ uint64_t tv_sec;
+ uint64_t tv_nsec;
+};
+
+/**
+ * Dynamic shared module.
+ */
+struct module {
+ /**
+ * Module handle, dlopen() result.
+ */
+ void *handle;
+ /**
+ * File attributes.
+ */
+ struct module_attr attr;
+ /**
+ * Count of active references.
+ */
+ int64_t refs;
+ /**
+ * Length of @a package.
+ */
+ size_t package_len;
+ /**
+ * Module's name without file extension.
+ */
+ char package[0];
+};
+
+/**
+ * Module function.
+ */
+struct module_func {
+ /**
+ * Function's address, iow dlsym() result.
+ */
+ box_function_t func;
+ /**
+ * Function's module.
+ */
+ struct module *module;
+};
+
+/**
+ * Load a module.
+ *
+ * Lookup for a module instance in cache and if not found
+ * the module is loaded from a storage device. In case if
+ * the module is present in cache but modified on a storage
+ * device it will be reread as a new and cache entry get
+ * updated.
+ *
+ * @param package module package (without file extension).
+ * @param package_len length of @a package.
+ *
+ * Possible errors:
+ * ClientError: the package is not found on a storage device.
+ * ClientError: an error happened when been loading the package.
+ * SystemError: a system error happened during procedure.
+ * OutOfMemory: unable to allocate new memory for module instance.
+ *
+ * @return a module instance on success, NULL otherwise (diag is set)
+ */
+struct module *
+module_load(const char *package, size_t package_len);
+
+/**
+ * Force load a module.
+ *
+ * Load a module from a storage device in a force way
+ * and update an associated cache entry.
+ *
+ * @param package module package (without file extension).
+ * @param package_len length of @a package.
+ *
+ * Possible errors:
+ * ClientError: the package is not found on a storage device.
+ * ClientError: an error happened when been loading the package.
+ * SystemError: a system error happened during procedure.
+ * OutOfMemory: unable to allocate new memory for module instance.
+ *
+ * @return a module instance on success, NULL otherwise (diag is set)
+ */
+struct module *
+module_load_force(const char *package, size_t package_len);
+
+/**
+ * Unload a module instance.
+ *
+ * @param m a module to unload.
+ */
+void
+module_unload(struct module *m);
+
+/** Test if module function is empty. */
+static inline bool
+module_func_is_empty(struct module_func *mf)
+{
+ return mf->module == NULL;
+}
+
+/** Create new empty module function. */
+static inline void
+module_func_create(struct module_func *mf)
+{
+ mf->module = NULL;
+ mf->func = NULL;
+}
+
+/**
+ * Load a new function.
+ *
+ * @param m a module to load a function from.
+ * @param func_name function name.
+ * @param mf[out] function instance.
+ *
+ * Possible errors:
+ * ClientError: no such function in a module.
+ *
+ * @return 0 on success, -1 otherwise (diag is set).
+ */
+int
+module_func_load(struct module *m, const char *func_name,
+ struct module_func *mf);
+
+/**
+ * Unload a function.
+ *
+ * @param mf module function.
+ */
+void
+module_func_unload(struct module_func *mf);
+
+/**
+ * Execute a function.
+ *
+ * @param mf a function to execute.
+ * @param args function arguments.
+ * @param ret[out] execution results.
+ *
+ * @return 0 on success, -1 otherwise (diag is set).
+ */
+int
+module_func_call(struct module_func *mf, struct port *args,
+ struct port *ret);
+
+/** Increment reference to a module. */
+void
+module_ref(struct module *m);
+
+/** Decrement reference of a module. */
+void
+module_unref(struct module *m);
+
+/** Initialize modules subsystem. */
+int
+module_init(void);
+
+/** Free modules subsystem. */
+void
+module_free(void);
+
+#if defined(__cplusplus)
+}
+#endif /* defined(__plusplus) */
diff --git a/src/main.cc b/src/main.cc
index 2be048d77..b74ac5926 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -76,6 +76,7 @@
#include "box/lua/init.h" /* box_lua_init() */
#include "box/session.h"
#include "box/memtx_tx.h"
+#include "box/module_cache.h"
#include "systemd.h"
#include "crypto/crypto.h"
#include "core/popen.h"
@@ -521,6 +522,7 @@ tarantool_free(void)
title_free(main_argc, main_argv);
popen_free();
+ module_free();
/* unlink pidfile. */
if (pid_file_handle != NULL && pidfile_remove(pid_file_handle) == -1)
@@ -703,6 +705,7 @@ main(int argc, char **argv)
cbus_init();
coll_init();
memtx_tx_manager_init();
+ module_init();
crypto_init();
systemd_init();
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 3/6] box/module_cache: introduce modules subsystem
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 3/6] box/module_cache: introduce modules subsystem Cyrill Gorcunov via Tarantool-patches
@ 2021-04-09 23:54 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-10 14:59 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-09 23:54 UTC (permalink / raw)
To: Cyrill Gorcunov, tml
I appreciate the work you did here!
The patch looks good. Only one question below.
> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> new file mode 100644
> index 000000000..3dcb40566
> --- /dev/null
> +++ b/src/box/module_cache.c
> @@ -0,0 +1,491 @@
<...>
> +
> +void
> +module_free(void)
> +{
> + while (mh_size(module_cache) > 0) {
> + mh_int_t e = mh_first(module_cache);
> + mh_strnptr_del(module_cache, e, NULL);
Is this loop really needed? It seems the mhash delete()
frees all the memory anyway.
> + }
> + mh_strnptr_delete(module_cache);
> + module_cache = NULL;
> +}
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 3/6] box/module_cache: introduce modules subsystem
2021-04-09 23:54 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-10 14:59 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 0 replies; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-10 14:59 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tml
On Sat, Apr 10, 2021 at 01:54:43AM +0200, Vladislav Shpilevoy wrote:
> > +void
> > +module_free(void)
> > +{
> > + while (mh_size(module_cache) > 0) {
> > + mh_int_t e = mh_first(module_cache);
> > + mh_strnptr_del(module_cache, e, NULL);
>
> Is this loop really needed? It seems the mhash delete()
> frees all the memory anyway.
Thanks, Vlad! Ineed this is not needed. I force-pushed an update.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Tarantool-patches] [PATCH v21 4/6] box/schema.func: switch to new module api
2021-04-08 16:41 [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches
` (2 preceding siblings ...)
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 3/6] box/module_cache: introduce modules subsystem Cyrill Gorcunov via Tarantool-patches
@ 2021-04-08 16:41 ` Cyrill Gorcunov via Tarantool-patches
2021-04-09 23:55 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module Cyrill Gorcunov via Tarantool-patches
` (3 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-08 16:41 UTC (permalink / raw)
To: tml; +Cc: Vladislav Shpilevoy
Since we have low level module api now we can switch
the box.schema.func code to use it. In particular we
define schema_module structure as a wrapper over the
modules api -- it carries a pointer to general module
structure.
Because of modules reload functionality the schema modules
carry own cache of modules instances. Thus to make overall
code somehow close to modules api we do:
1) cache operations are renamed to cache_find/put/update/del;
2) C functions are switched to use module_func low level api;
3) because low level modules api carry own references we can
take no explicit reference when calling a function.
Part-of #4642
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/box/func.c | 550 +++++++++++++++++++--------------------------
src/box/func.h | 15 +-
src/box/func_def.h | 14 --
3 files changed, 234 insertions(+), 345 deletions(-)
diff --git a/src/box/func.c b/src/box/func.c
index 10b916e6d..2cb111ba6 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -30,19 +30,12 @@
*/
#include "func.h"
#include "fiber.h"
-#include "trivia/config.h"
#include "assoc.h"
-#include "lua/utils.h"
#include "lua/call.h"
-#include "error.h"
-#include "errinj.h"
#include "diag.h"
#include "port.h"
#include "schema.h"
#include "session.h"
-#include "libeio/eio.h"
-#include <fcntl.h>
-#include <dlfcn.h>
/**
* Parsed symbol and package names.
@@ -56,6 +49,19 @@ struct func_name {
const char *package_end;
};
+/**
+ * Schema module (box.schema) instance.
+ */
+struct schema_module {
+ /** Low level module instance. */
+ struct module *base;
+ /** List of imported functions. */
+ struct rlist funcs;
+ /** Reference counter. */
+ int64_t refs;
+};
+
+
struct func_c {
/** Function object base class. */
struct func base;
@@ -64,16 +70,21 @@ struct func_c {
*/
struct rlist item;
/**
- * For C functions, the body of the function.
+ * C function to call.
*/
- box_function_f func;
+ struct module_func mf;
/**
- * Each stored function keeps a handle to the
- * dynamic library for the C callback.
+ * A schema module the function belongs to.
*/
- struct module *module;
+ struct schema_module *module;
};
+static void
+schema_module_ref(struct schema_module *module);
+
+static void
+schema_module_unref(struct schema_module *module);
+
/***
* Split function name to symbol and package names.
* For example, str = foo.bar.baz => sym = baz, package = foo.bar
@@ -95,82 +106,9 @@ func_split_name(const char *str, struct func_name *name)
}
}
-/**
- * Arguments for luaT_module_find used by lua_cpcall()
- */
-struct module_find_ctx {
- const char *package;
- const char *package_end;
- char *path;
- size_t path_len;
-};
-
-/**
- * A cpcall() helper for module_find()
- */
-static int
-luaT_module_find(lua_State *L)
-{
- struct module_find_ctx *ctx = (struct module_find_ctx *)
- lua_topointer(L, 1);
-
- /*
- * Call package.searchpath(name, package.cpath) and use
- * the path to the function in dlopen().
- */
- lua_getglobal(L, "package");
-
- lua_getfield(L, -1, "search");
-
- /* Argument of search: name */
- lua_pushlstring(L, ctx->package, ctx->package_end - ctx->package);
-
- lua_call(L, 1, 1);
- if (lua_isnil(L, -1))
- return luaL_error(L, "module not found");
- /* Convert path to absolute */
- char resolved[PATH_MAX];
- if (realpath(lua_tostring(L, -1), resolved) == NULL) {
- diag_set(SystemError, "realpath");
- return luaT_error(L);
- }
-
- snprintf(ctx->path, ctx->path_len, "%s", resolved);
- return 0;
-}
-
-/**
- * Find path to module using Lua's package.cpath
- * @param package package name
- * @param package_end a pointer to the last byte in @a package + 1
- * @param[out] path path to shared library
- * @param path_len size of @a path buffer
- * @retval 0 on success
- * @retval -1 on error, diag is set
- */
-static int
-module_find(const char *package, const char *package_end, char *path,
- size_t path_len)
-{
- struct module_find_ctx ctx = { package, package_end, path, path_len };
- lua_State *L = tarantool_L;
- int top = lua_gettop(L);
- if (luaT_cpcall(L, luaT_module_find, &ctx) != 0) {
- int package_len = (int) (package_end - package);
- diag_set(ClientError, ER_LOAD_MODULE, package_len, package,
- lua_tostring(L, -1));
- lua_settop(L, top);
- return -1;
- }
- assert(top == lua_gettop(L)); /* cpcall discard results */
- return 0;
-}
-
+/** Schema modules hash. */
static struct mh_strnptr_t *modules = NULL;
-static void
-module_gc(struct module *module);
-
int
schema_module_init(void)
{
@@ -188,240 +126,272 @@ schema_module_free(void)
{
while (mh_size(modules) > 0) {
mh_int_t i = mh_first(modules);
- struct module *module =
- (struct module *) mh_strnptr_node(modules, i)->val;
- /* Can't delete modules if they have active calls */
- module_gc(module);
+ mh_strnptr_del(modules, i, NULL);
}
mh_strnptr_delete(modules);
+ modules = NULL;
}
/**
* Look up a module in the modules cache.
*/
-static struct module *
-module_cache_find(const char *name, const char *name_end)
+static struct schema_module *
+cache_find(const char *name, const char *name_end)
{
mh_int_t i = mh_strnptr_find_inp(modules, name, name_end - name);
if (i == mh_end(modules))
return NULL;
- return (struct module *)mh_strnptr_node(modules, i)->val;
+ return mh_strnptr_node(modules, i)->val;
}
/**
- * Save module to the module cache.
+ * Save a module to the modules cache.
*/
-static inline int
-module_cache_put(struct module *module)
+static int
+cache_put(struct schema_module *module)
{
- size_t package_len = strlen(module->package);
- uint32_t name_hash = mh_strn_hash(module->package, package_len);
+ const char *str = module->base->package;
+ size_t len = module->base->package_len;
+
const struct mh_strnptr_node_t strnode = {
- module->package, package_len, name_hash, module};
+ .str = str,
+ .len = len,
+ .hash = mh_strn_hash(str, len),
+ .val = module,
+ };
- if (mh_strnptr_put(modules, &strnode, NULL, NULL) == mh_end(modules)) {
+ struct mh_strnptr_node_t prev;
+ struct mh_strnptr_node_t *prev_ptr = &prev;
+
+ if (mh_strnptr_put(modules, &strnode, &prev_ptr, NULL) == mh_end(modules)) {
diag_set(OutOfMemory, sizeof(strnode), "malloc", "modules");
return -1;
}
+
+ /*
+ * Just to make sure we haven't replaced something, the
+ * entries must be explicitly deleted.
+ */
+ assert(prev_ptr == NULL);
return 0;
}
/**
- * Delete a module from the module cache
+ * Update a module in the modules cache.
*/
static void
-module_cache_del(const char *name, const char *name_end)
+cache_update(struct schema_module *module)
{
- mh_int_t i = mh_strnptr_find_inp(modules, name, name_end - name);
+ const char *str = module->base->package;
+ size_t len = module->base->package_len;
+
+ mh_int_t i = mh_strnptr_find_inp(modules, str, len);
if (i == mh_end(modules))
- return;
- mh_strnptr_del(modules, i, NULL);
+ panic("func: failed to update cache: %s", str);
+
+ mh_strnptr_node(modules, i)->str = str;
+ mh_strnptr_node(modules, i)->val = module;
}
-/*
- * Load a dso.
- * Create a new symlink based on temporary directory and try to
- * load via this symink to load a dso twice for cases of a function
- * reload.
+/**
+ * Delete a module from the module cache.
*/
-static struct module *
-module_load(const char *package, const char *package_end)
+static void
+cache_del(struct schema_module *module)
{
- char path[PATH_MAX];
- if (module_find(package, package_end, path, sizeof(path)) != 0)
- return NULL;
-
- int package_len = package_end - package;
- struct module *module = (struct module *)
- malloc(sizeof(*module) + package_len + 1);
- if (module == NULL) {
- diag_set(OutOfMemory, sizeof(struct module) + package_len + 1,
- "malloc", "struct module");
- return NULL;
- }
- memcpy(module->package, package, package_len);
- module->package[package_len] = 0;
- rlist_create(&module->funcs);
- module->calls = 0;
-
- const char *tmpdir = getenv("TMPDIR");
- if (tmpdir == NULL)
- tmpdir = "/tmp";
- char dir_name[PATH_MAX];
- int rc = snprintf(dir_name, sizeof(dir_name), "%s/tntXXXXXX", tmpdir);
- if (rc < 0 || (size_t) rc >= sizeof(dir_name)) {
- diag_set(SystemError, "failed to generate path to tmp dir");
- goto error;
+ const char *str = module->base->package;
+ size_t len = module->base->package_len;
+
+ mh_int_t i = mh_strnptr_find_inp(modules, str, len);
+ if (i != mh_end(modules)) {
+ struct schema_module *v;
+ v = mh_strnptr_node(modules, i)->val;
+ /*
+ * The module may be already reloaded so
+ * the cache carries a new entry instead.
+ */
+ if (v == module)
+ mh_strnptr_del(modules, i, NULL);
}
+}
- if (mkdtemp(dir_name) == NULL) {
- diag_set(SystemError, "failed to create unique dir name: %s",
- dir_name);
- goto error;
- }
- char load_name[PATH_MAX];
- rc = snprintf(load_name, sizeof(load_name), "%s/%.*s." TARANTOOL_LIBEXT,
- dir_name, package_len, package);
- if (rc < 0 || (size_t) rc >= sizeof(dir_name)) {
- diag_set(SystemError, "failed to generate path to DSO");
- goto error;
- }
+/** Delete a module. */
+static void
+schema_module_delete(struct schema_module *module)
+{
+ module_unload(module->base);
+ TRASH(module);
+ free(module);
+}
- struct stat st;
- if (stat(path, &st) < 0) {
- diag_set(SystemError, "failed to stat() module %s", path);
- goto error;
- }
+/** Increment reference to a module. */
+static void
+schema_module_ref(struct schema_module *module)
+{
+ assert(module->refs >= 0);
+ ++module->refs;
+}
- int source_fd = open(path, O_RDONLY);
- if (source_fd < 0) {
- diag_set(SystemError, "failed to open module %s file for" \
- " reading", path);
- goto error;
- }
- int dest_fd = open(load_name, O_WRONLY|O_CREAT|O_TRUNC,
- st.st_mode & 0777);
- if (dest_fd < 0) {
- diag_set(SystemError, "failed to open file %s for writing ",
- load_name);
- close(source_fd);
- goto error;
+/** Decrement reference to a module and delete it if last one. */
+static void
+schema_module_unref(struct schema_module *module)
+{
+ assert(module->refs > 0);
+ if (--module->refs == 0) {
+ cache_del(module);
+ schema_module_delete(module);
}
+}
- off_t ret = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size);
- close(source_fd);
- close(dest_fd);
- if (ret != st.st_size) {
- diag_set(SystemError, "failed to copy DSO %s to %s",
- path, load_name);
- goto error;
+static struct schema_module *
+schema_do_module_load(const char *name, size_t len, bool force)
+{
+ struct schema_module *module = malloc(sizeof(*module));
+ if (module != NULL) {
+ module->base = NULL;
+ module->refs = 0;
+ rlist_create(&module->funcs);
+ } else {
+ diag_set(OutOfMemory, sizeof(*module),
+ "malloc", "struct schema_module");
+ return NULL;
}
- module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
- if (unlink(load_name) != 0)
- say_warn("failed to unlink dso link %s", load_name);
- if (rmdir(dir_name) != 0)
- say_warn("failed to delete temporary dir %s", dir_name);
- if (module->handle == NULL) {
- diag_set(ClientError, ER_LOAD_MODULE, package_len,
- package, dlerror());
- goto error;
+ if (force)
+ module->base = module_load_force(name, len);
+ else
+ module->base = module_load(name, len);
+
+ if (module->base == NULL) {
+ free(module);
+ return NULL;
}
- struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
- if (e != NULL)
- ++e->iparam;
+
+ schema_module_ref(module);
return module;
-error:
- free(module);
- return NULL;
}
-static void
-module_delete(struct module *module)
+/**
+ * Load a new module.
+ */
+static struct schema_module *
+schema_module_load(const char *name, const char *name_end)
{
- struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
- if (e != NULL)
- --e->iparam;
- dlclose(module->handle);
- TRASH(module);
- free(module);
+ return schema_do_module_load(name, name_end - name, false);
}
-/*
- * Check if a dso is unused and can be closed.
+/**
+ * Force load a new module.
*/
+static struct schema_module *
+schema_module_load_force(const char *name, const char *name_end)
+{
+ return schema_do_module_load(name, name_end - name, true);
+}
+
+static struct func_vtab func_c_vtab;
+
+/** Create a new C function. */
static void
-module_gc(struct module *module)
+func_c_create(struct func_c *func_c)
{
- if (rlist_empty(&module->funcs) && module->calls == 0)
- module_delete(module);
+ func_c->module = NULL;
+ func_c->base.vtab = &func_c_vtab;
+ rlist_create(&func_c->item);
+ module_func_create(&func_c->mf);
}
-/*
- * Import a function from the module.
- */
-static box_function_f
-module_sym(struct module *module, const char *name)
+static int
+func_c_load_from(struct func_c *func, struct schema_module *module,
+ const char *func_name)
{
- box_function_f f = (box_function_f)dlsym(module->handle, name);
- if (f == NULL) {
- diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
- return NULL;
+ assert(module_func_is_empty(&func->mf));
+
+ if (module_func_load(module->base, func_name, &func->mf) != 0)
+ return -1;
+
+ func->module = module;
+ rlist_move(&module->funcs, &func->item);
+ schema_module_ref(module);
+ return 0;
+}
+
+static void
+func_c_unload(struct func_c *func)
+{
+ if (!module_func_is_empty(&func->mf)) {
+ rlist_del(&func->item);
+ schema_module_unref(func->module);
+ module_func_unload(&func->mf);
+ func_c_create(func);
}
- return f;
}
int
schema_module_reload(const char *package, const char *package_end)
{
- struct module *old_module = module_cache_find(package, package_end);
- if (old_module == NULL) {
+ struct schema_module *old = cache_find(package, package_end);
+ if (old == NULL) {
/* Module wasn't loaded - do nothing. */
diag_set(ClientError, ER_NO_SUCH_MODULE, package);
return -1;
}
- struct module *new_module = module_load(package, package_end);
- if (new_module == NULL)
+ struct schema_module *new = schema_module_load_force(package, package_end);
+ if (new == NULL)
return -1;
+ /*
+ * Keep an extra reference to the old module
+ * so it won't be freed until reload is complete,
+ * otherwise we might free old module then fail
+ * on some function loading and in result won't
+ * be able to restore old symbols.
+ */
+ schema_module_ref(old);
struct func_c *func, *tmp_func;
- rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
- /* Move immediately for restore sake. */
- rlist_move(&new_module->funcs, &func->item);
+ rlist_foreach_entry_safe(func, &old->funcs, item, tmp_func) {
struct func_name name;
func_split_name(func->base.def->name, &name);
- func->func = module_sym(new_module, name.sym);
- if (func->func == NULL)
- goto restore;
- func->module = new_module;
+ func_c_unload(func);
+ if (func_c_load_from(func, new, name.sym) != 0) {
+ /*
+ * We can restore failing function immediately
+ * and then all previously migrated ones.
+ */
+ if (func_c_load_from(func, old, name.sym) != 0)
+ goto restore_fail;
+ else
+ goto restore;
+ }
}
- module_cache_del(package, package_end);
- if (module_cache_put(new_module) != 0)
- goto restore;
- module_gc(old_module);
+ cache_update(new);
+ schema_module_unref(old);
+ schema_module_unref(new);
return 0;
restore:
/*
* Some old functions are not found in the new module,
* thus restore all migrated functions back to the original.
*/
- rlist_foreach_entry_safe(func, &new_module->funcs, item, tmp_func) {
+ rlist_foreach_entry_safe(func, &new->funcs, item, tmp_func) {
struct func_name name;
func_split_name(func->base.def->name, &name);
- func->func = module_sym(old_module, name.sym);
- if (func->func == NULL) {
- /*
- * Something strange was happen, an early loaden
- * function was not found in an old dso.
- */
- panic("Can't restore module function, "
- "server state is inconsistent");
- }
- func->module = old_module;
- rlist_move(&old_module->funcs, &func->item);
+ func_c_unload(func);
+ if (func_c_load_from(func, old, name.sym) != 0)
+ goto restore_fail;
}
- module_delete(new_module);
+ schema_module_unref(old);
+ schema_module_unref(new);
+ return -1;
+
+restore_fail:
+ /*
+ * Something strange was happen, an early loaden
+ * function was not found in an old dso.
+ */
+ panic("Can't restore module function, "
+ "server state is inconsistent");
return -1;
}
@@ -469,8 +439,6 @@ func_new(struct func_def *def)
return func;
}
-static struct func_vtab func_c_vtab;
-
static struct func *
func_c_new(MAYBE_UNUSED struct func_def *def)
{
@@ -481,28 +449,10 @@ func_c_new(MAYBE_UNUSED struct func_def *def)
diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
return NULL;
}
- func->base.vtab = &func_c_vtab;
- func->func = NULL;
- func->module = NULL;
+ func_c_create(func);
return &func->base;
}
-static void
-func_c_unload(struct func_c *func)
-{
- if (func->module) {
- rlist_del(&func->item);
- if (rlist_empty(&func->module->funcs)) {
- struct func_name name;
- func_split_name(func->base.def->name, &name);
- module_cache_del(name.package, name.package_end);
- }
- module_gc(func->module);
- }
- func->module = NULL;
- func->func = NULL;
-}
-
static void
func_c_destroy(struct func *base)
{
@@ -521,45 +471,32 @@ func_c_destroy(struct func *base)
static int
func_c_load(struct func_c *func)
{
- assert(func->func == NULL);
-
struct func_name name;
func_split_name(func->base.def->name, &name);
- struct module *cached, *module;
- cached = module_cache_find(name.package, name.package_end);
+ struct schema_module *cached, *module;
+ cached = cache_find(name.package, name.package_end);
if (cached == NULL) {
- module = module_load(name.package, name.package_end);
+ module = schema_module_load(name.package, name.package_end);
if (module == NULL)
return -1;
- if (module_cache_put(module)) {
- module_delete(module);
+ if (cache_put(module)) {
+ schema_module_unref(module);
return -1;
}
} else {
module = cached;
+ schema_module_ref(module);
}
- func->func = module_sym(module, name.sym);
- if (func->func == NULL) {
- if (cached == NULL) {
- /*
- * In case if it was a first load we should
- * clean the cache immediately otherwise
- * the module continue being referenced even
- * if there will be no use of it.
- *
- * Note the module_sym set an error thus be
- * careful to not wipe it.
- */
- module_cache_del(name.package, name.package_end);
- module_delete(module);
- }
- return -1;
- }
- func->module = module;
- rlist_add(&module->funcs, &func->item);
- return 0;
+ int rc = func_c_load_from(func, module, name.sym);
+ /*
+ * There is no explicit module loading in this
+ * interface so each function carries a reference
+ * by their own.
+ */
+ schema_module_unref(module);
+ return rc;
}
int
@@ -568,38 +505,17 @@ func_c_call(struct func *base, struct port *args, struct port *ret)
assert(base->vtab == &func_c_vtab);
assert(base != NULL && base->def->language == FUNC_LANGUAGE_C);
struct func_c *func = (struct func_c *) base;
- if (func->func == NULL) {
+ if (module_func_is_empty(&func->mf)) {
if (func_c_load(func) != 0)
return -1;
}
-
- struct region *region = &fiber()->gc;
- size_t region_svp = region_used(region);
- uint32_t data_sz;
- const char *data = port_get_msgpack(args, &data_sz);
- if (data == NULL)
- return -1;
-
- port_c_create(ret);
- box_function_ctx_t ctx = { ret };
-
- /* Module can be changed after function reload. */
- struct module *module = func->module;
- assert(module != NULL);
- ++module->calls;
- int rc = func->func(&ctx, data, data + data_sz);
- --module->calls;
- module_gc(module);
- region_truncate(region, region_svp);
- if (rc != 0) {
- if (diag_last_error(&fiber()->diag) == NULL) {
- /* Stored procedure forget to set diag */
- diag_set(ClientError, ER_PROC_C, "unknown error");
- }
- port_destroy(ret);
- return -1;
- }
- return rc;
+ /*
+ * Note that we don't take a reference to the
+ * module, it is handled by low level instance,
+ * thus while been inside the call the associated
+ * schema_module can be unreferenced and freed.
+ */
+ return module_func_call(&func->mf, args, ret);
}
static struct func_vtab func_c_vtab = {
diff --git a/src/box/func.h b/src/box/func.h
index 5a49e34f4..987ca70db 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -37,6 +37,7 @@
#include "small/rlist.h"
#include "func_def.h"
#include "user_def.h"
+#include "module_cache.h"
#if defined(__cplusplus)
extern "C" {
@@ -44,20 +45,6 @@ extern "C" {
struct func;
-/**
- * Dynamic shared module.
- */
-struct module {
- /** Module dlhandle. */
- void *handle;
- /** List of imported functions. */
- struct rlist funcs;
- /** Count of active calls. */
- size_t calls;
- /** Module's package name. */
- char package[0];
-};
-
/** Virtual method table for func object. */
struct func_vtab {
/** Call function with given arguments. */
diff --git a/src/box/func_def.h b/src/box/func_def.h
index d99d89190..75cd6a0d3 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -168,20 +168,6 @@ func_def_dup(struct func_def *def);
int
func_def_check(struct func_def *def);
-/**
- * API of C stored function.
- */
-
-struct port;
-
-struct box_function_ctx {
- struct port *port;
-};
-
-typedef struct box_function_ctx box_function_ctx_t;
-typedef int (*box_function_f)(box_function_ctx_t *ctx,
- const char *args, const char *args_end);
-
#ifdef __cplusplus
}
#endif
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 4/6] box/schema.func: switch to new module api
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 4/6] box/schema.func: switch to new module api Cyrill Gorcunov via Tarantool-patches
@ 2021-04-09 23:55 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-10 15:00 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-09 23:55 UTC (permalink / raw)
To: Cyrill Gorcunov, tml
Good job on the patch!
The patch looks good, only one question below, like in the previous
commit.
> diff --git a/src/box/func.c b/src/box/func.c
> index 10b916e6d..2cb111ba6 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -188,240 +126,272 @@ schema_module_free(void)
> {
> while (mh_size(modules) > 0) {
> mh_int_t i = mh_first(modules);
> - struct module *module =
> - (struct module *) mh_strnptr_node(modules, i)->val;
> - /* Can't delete modules if they have active calls */
> - module_gc(module);
> + mh_strnptr_del(modules, i, NULL);
The same question as in the previous commit. Do we really need this
loop now?
> }
> mh_strnptr_delete(modules);
> + modules = NULL;
> }
I will review the next commits later.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module
2021-04-08 16:41 [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches
` (3 preceding siblings ...)
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 4/6] box/schema.func: switch to new module api Cyrill Gorcunov via Tarantool-patches
@ 2021-04-08 16:41 ` Cyrill Gorcunov via Tarantool-patches
2021-04-11 15:38 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test Cyrill Gorcunov via Tarantool-patches
` (2 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-08 16:41 UTC (permalink / raw)
To: tml; +Cc: Vladislav Shpilevoy
Currently to run "C" function from some external module one
have to register it first in "_func" system space. This is
a problem if node is in read-only mode (replica).
Still people would like to have a way to run such functions
even in ro mode. For this sake we implement "box.lib" lua module.
Unlike `box.schema.func` interface the `box.lib` does not defer module
loading procedure until first call of a function. Instead a module
is loaded immediately and if some error happens (say shared
library is corrupted or not found) it pops up early.
The need of use stored C procedures implies that application is
running under serious loads most likely there is modular structure
present on Lua level (ie same shared library is loaded in different
sources) thus we cache the loaded library and reuse it on next
load attempts. To verify that cached library is up to day the
module_cache engine test for file attributes (device, inode, size,
modification time) on every load attempt.
Since both `box.schema.func` and `box.lib` are using caching to minimize
module loading procedure the pass-through caching scheme is
implemented:
- box.lib relies on module_cache engine for caching;
- box.schema.func does snoop into box.lib hash table when attempt
to load a new module, if module is present in box.lib hash then
it simply referenced from there and added into own hash table;
in case if module is not present then it loaded from the scratch
and put into both hashes;
- the module_reload action in box.schema.func invalidates module_cache
or fill it if entry is not present.
Closes #4642
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
@TarantoolBot document
Title: box.lib module
Overview
========
`box.lib` module provides a way to create, delete and execute
`C` procedures from shared libraries. Unlike `box.schema.func`
methods the functions created with `box.lib` help are not persistent
and live purely in memory. Once a node get turned off they are
vanished. An initial purpose for them is to execute them on
nodes which are running in read-only mode.
Module functions
================
`box.lib.load(path) -> obj | error`
-----------------------------------
Loads a module from `path` and return an object instance
associate with the module, otherwise an error is thrown.
The `path` should not end up with shared library extension
(such as `.so`), only a file name shall be there.
Possible errors:
- IllegalParams: module path is either not supplied
or not a string.
- SystemError: unable to open a module due to a system error.
- ClientError: a module does not exist.
- OutOfMemory: unable to allocate a module.
Example:
``` Lua
-- Without error handling
m = box.lib.load('path/to/library)
-- With error handling
m, err = pcall(box.lib.load, 'path/to/library')
if err ~= nil then
print(err)
end
```
`module:unload() -> true | error`
---------------------------------
Unloads a module. Returns `true` on success, otherwise an error
is thrown. Once the module is unloaded one can't load new
functions from this module instance.
Possible errors:
- IllegalParams: a module is not supplied.
- IllegalParams: a module is already unloaded.
Example:
``` Lua
m = box.lib.load('path/to/library')
--
-- do something with module
--
m:unload()
```
If there are functions from this module referenced somewhere
in other places of Lua code they still can be executed because
the module continue sitting in memory until the last reference
to it is closed.
If the module become a target to the Lua's garbage collector
then unload is called implicitly.
module:load(name) -> obj | error`
---------------------------------
Loads a new function with name `name` from the previously
loaded `module` and return a callable object instance
associated with the function. On failure an error is thrown.
Possible errors:
- IllegalParams: function name is either not supplied
or not a string.
- IllegalParams: attempt to load a function but module
has been unloaded already.
- ClientError: no such function in the module.
- OutOfMemory: unable to allocate a function.
Example:
``` Lua
-- Load a module if not been loaded yet.
m = box.lib.load('path/to/library')
-- Load a function with the `foo` name from the module `m`.
func = m:load('foo')
```
In case if there is no need for further loading of other
functions from the same module then the module might be
unloaded immediately.
``` Lua
m = box.lib.load('path/to/library')
func = m:load('foo')
m:unload()
```
`function:unload() -> true | error`
-----------------------------------
Unloads a function. Returns `true` on success, otherwise
an error is thrown.
Possible errors:
- IllegalParams: function name is either not supplied
or not a string.
- IllegalParams: the function already unloaded.
Example:
``` Lua
m = box.lib.load('path/to/library')
func = m:load('foo')
--
-- do something with function and cleanup then
--
func:unload()
m:unload()
```
If the function become a target to the Lua's garbage collector
then unload is called implicitly.
Executing a loaded function
===========================
Once function is loaded it can be executed as an ordinary Lua call.
Lets consider the following example. We have a `C` function which
takes two numbers and returns their sum.
``` C
int
cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
uint32_t arg_count = mp_decode_array(&args);
if (arg_count != 2) {
return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
"invalid argument count");
}
uint64_t a = mp_decode_uint(&args);
uint64_t b = mp_decode_uint(&args);
char res[16];
char *end = mp_encode_uint(res, a + b);
box_return_mp(ctx, res, end);
return 0;
}
```
The name of the function is `cfunc_sum` and the function is built into
`cfunc.so` shared library.
First we should load it as
``` Lua
m = box.lib.load('cfunc')
cfunc_sum = m:load('cfunc_sum')
```
Once successfully loaded we can execute it. Lets call the
`cfunc_sum` with wrong number of arguments
``` Lua
cfunc_sum()
| ---
| - error: invalid argument count
```
We will see the `"invalid argument count"` message in output.
The error message has been set by the `box_error_set` in `C`
code above.
On success the sum of arguments will be printed out.
``` Lua
cfunc_sum(1, 2)
| ---
| - 3
```
The functions may return multiple results. For example a trivial
echo function which prints arguments passed in.
``` Lua
cfunc_echo(1,2,3)
| ---
| - 1
| - 2
| - 3
```
Module and function caches
==========================
Loading a module is relatively slow procedure because operating
system needs to read the library, resolve its symbols and etc.
Thus to speedup this procedure if the module is loaded for a first
time we put it into an internal cache. If module is sitting in
the cache already and new request to load comes in -- we simply
reuse a previous copy. In case if module is updated on a storage
device then on new load attempt we detect that file attributes
(such as device number, inode, size, modification time) get changed
and reload module from the scratch. Note that newly loaded module
does not intersect with previously loaded modules, the continue
operating with code previously read from cache.
Thus if there is a need to update a module then all module instances
should be unloaded (together with functions) and loaded again.
Similar caching technique applied to functions -- only first function
allocation cause symbol resolving, next ones are simply obtained from
a function cache.
---
changelogs/unreleased/add-box-lib.md | 4 +
src/box/CMakeLists.txt | 1 +
src/box/lua/init.c | 2 +
src/box/lua/lib.c | 606 +++++++++++++++++++++++++++
src/box/lua/lib.h | 25 ++
src/box/lua/load_cfg.lua | 1 +
test/box/misc.result | 1 +
7 files changed, 640 insertions(+)
create mode 100644 changelogs/unreleased/add-box-lib.md
create mode 100644 src/box/lua/lib.c
create mode 100644 src/box/lua/lib.h
diff --git a/changelogs/unreleased/add-box-lib.md b/changelogs/unreleased/add-box-lib.md
new file mode 100644
index 000000000..3fe7a1b01
--- /dev/null
+++ b/changelogs/unreleased/add-box-lib.md
@@ -0,0 +1,4 @@
+## feature/core
+
+* Introduce `box.lib` module which allows to load and execute
+ C stored procedures on read-only nodes (gh-4642).
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index cc2e17e94..09f81afd8 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -196,6 +196,7 @@ add_library(box STATIC
lua/call.c
lua/cfg.cc
lua/console.c
+ lua/lib.c
lua/serialize_lua.c
lua/tuple.c
lua/slab.c
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index fbcdfb20b..3a6d60864 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -60,6 +60,7 @@
#include "box/lua/cfg.h"
#include "box/lua/xlog.h"
#include "box/lua/console.h"
+#include "box/lua/lib.h"
#include "box/lua/tuple.h"
#include "box/lua/execute.h"
#include "box/lua/key_def.h"
@@ -465,6 +466,7 @@ box_lua_init(struct lua_State *L)
box_lua_tuple_init(L);
box_lua_call_init(L);
box_lua_cfg_init(L);
+ box_lua_lib_init(L);
box_lua_slab_init(L);
box_lua_index_init(L);
box_lua_space_init(L);
diff --git a/src/box/lua/lib.c b/src/box/lua/lib.c
new file mode 100644
index 000000000..78aee37b5
--- /dev/null
+++ b/src/box/lua/lib.c
@@ -0,0 +1,606 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#include <unistd.h>
+#include <string.h>
+#include <lua.h>
+
+#include "box/error.h"
+#include "box/port.h"
+
+#include "assoc.h"
+#include "lib.h"
+#include "diag.h"
+#include "module_cache.h"
+
+#include "lua/utils.h"
+
+/**
+ * Function descriptor.
+ */
+struct box_module_func {
+ /** C function to call. */
+ struct module_func base;
+ /** Number of references. */
+ int64_t refs;
+ /** Length of functon name in @a key. */
+ size_t sym_len;
+ /** Length of @a key. */
+ size_t len;
+ /** Function hash key. */
+ char key[0];
+};
+
+/** Function name to box_module_func hash. */
+static struct mh_strnptr_t *func_hash = NULL;
+
+/** A type to find a module from an object. */
+static const char *uname_lib = "tt_uname_box_lib";
+
+/** A type to find a function from an object. */
+static const char *uname_func = "tt_uname_box_lib_func";
+
+/** Get data associated with an object. */
+static void *
+get_udata(struct lua_State *L, const char *uname)
+{
+ void **pptr = luaL_testudata(L, 1, uname);
+ return pptr != NULL ? *pptr : NULL;
+}
+
+/**
+ * Get pointer associated with an object and clear it
+ * returning previously associated data.
+ */
+static void *
+clear_udata(struct lua_State *L, const char *uname)
+{
+ void **pptr = luaL_testudata(L, 1, uname);
+ if (pptr != NULL) {
+ void *ptr = *pptr;
+ *pptr = NULL;
+ return ptr;
+ }
+ return NULL;
+}
+
+/** Setup a new data and associate it with an object. */
+static void
+new_udata(struct lua_State *L, const char *uname, void *ptr)
+{
+ *(void **)lua_newuserdata(L, sizeof(void *)) = ptr;
+ luaL_getmetatable(L, uname);
+ lua_setmetatable(L, -2);
+}
+
+/**
+ * Helpers for function cache.
+ */
+static void *
+cache_find(const char *str, size_t len)
+{
+ mh_int_t e = mh_strnptr_find_inp(func_hash, str, len);
+ if (e == mh_end(func_hash))
+ return NULL;
+ return mh_strnptr_node(func_hash, e)->val;
+}
+
+static int
+cache_put(struct box_module_func *cf)
+{
+ const struct mh_strnptr_node_t nd = {
+ .str = cf->key,
+ .len = cf->len,
+ .hash = mh_strn_hash(cf->key, cf->len),
+ .val = cf,
+ };
+ mh_int_t e = mh_strnptr_put(func_hash, &nd, NULL, NULL);
+ if (e == mh_end(func_hash)) {
+ diag_set(OutOfMemory, sizeof(nd), "malloc",
+ "box.lib: hash node");
+ return -1;
+ }
+ return 0;
+}
+
+static void
+cache_del(struct box_module_func *cf)
+{
+ mh_int_t e = mh_strnptr_find_inp(func_hash, cf->key, cf->len);
+ if (e != mh_end(func_hash))
+ mh_strnptr_del(func_hash, e, NULL);
+}
+
+/**
+ * Load a module.
+ *
+ * This function takes a module path from the caller
+ * stack @a L and returns cached module instance or
+ * creates a new module object.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: module path is either not supplied
+ * or not a string.
+ * - SystemError: unable to open a module due to a system error.
+ * - ClientError: a module does not exist.
+ * - OutOfMemory: unable to allocate a module.
+ *
+ * @returns module object on success or throws an error.
+ */
+static int
+lbox_module_load(struct lua_State *L)
+{
+ const char *msg_noname = "Expects box.lib.load(\'name\') "
+ "but no name passed";
+
+ if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
+ diag_set(IllegalParams, msg_noname);
+ return luaT_error(L);
+ }
+
+ size_t name_len;
+ const char *name = lua_tolstring(L, 1, &name_len);
+
+ if (name_len < 1) {
+ diag_set(IllegalParams, msg_noname);
+ return luaT_error(L);
+ }
+
+ struct module *m = module_load(name, name_len);
+ if (m == NULL)
+ return luaT_error(L);
+
+ new_udata(L, uname_lib, m);
+ return 1;
+}
+
+/**
+ * Unload a module.
+ *
+ * Take a module object from the caller stack @a L and unload it.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: module is not supplied.
+ * - IllegalParams: the module is unloaded.
+ *
+ * @returns true on success or throwns an error.
+ */
+static int
+lbox_module_unload(struct lua_State *L)
+{
+ if (lua_gettop(L) != 1) {
+ diag_set(IllegalParams, "Expects module:unload()");
+ return luaT_error(L);
+ }
+
+ struct module *m = clear_udata(L, uname_lib);
+ if (m == NULL) {
+ diag_set(IllegalParams, "The module is unloaded");
+ return luaT_error(L);
+ }
+ module_unload(m);
+
+ lua_pushboolean(L, true);
+ return 1;
+}
+
+/** Handle __index request for a module object. */
+static int
+lbox_module_index(struct lua_State *L)
+{
+ /*
+ * Process metamethods such as "module:load" first.
+ */
+ lua_getmetatable(L, 1);
+ lua_pushvalue(L, 2);
+ lua_rawget(L, -2);
+ if (!lua_isnil(L, -1))
+ return 1;
+
+ struct module *m = get_udata(L, uname_lib);
+ if (m == NULL) {
+ lua_pushnil(L);
+ return 1;
+ }
+
+ const char *key = lua_tostring(L, 2);
+ if (key == NULL || !lua_isstring(L, 2)) {
+ diag_set(IllegalParams,
+ "Bad params, use __index(<key>)");
+ return luaT_error(L);
+ }
+
+ if (strcmp(key, "path") == 0) {
+ lua_pushstring(L, m->package);
+ return 1;
+ }
+
+ /*
+ * Internal keys for debug only, not API.
+ */
+ if (strcmp(key, "debug_refs") == 0) {
+ lua_pushnumber(L, m->refs);
+ return 1;
+ } else if (strcmp(key, "debug_ptr") == 0) {
+ char s[64];
+ snprintf(s, sizeof(s), "%p", m);
+ lua_pushstring(L, s);
+ return 1;
+ }
+ return 0;
+}
+
+/** Module representation for REPL (console). */
+static int
+lbox_module_serialize(struct lua_State *L)
+{
+ struct module *m = get_udata(L, uname_lib);
+ if (m == NULL) {
+ lua_pushnil(L);
+ return 1;
+ }
+
+ lua_createtable(L, 0, 1);
+ lua_pushstring(L, m->package);
+ lua_setfield(L, -2, "path");
+ return 1;
+}
+
+/** Collect a module. */
+static int
+lbox_module_gc(struct lua_State *L)
+{
+ struct module *m = clear_udata(L, uname_lib);
+ if (m != NULL)
+ module_unload(m);
+ return 0;
+}
+
+/** Increase reference to a function. */
+static void
+box_module_func_ref(struct box_module_func *cf)
+{
+ assert(cf->refs >= 0);
+ ++cf->refs;
+}
+
+/** Free function memory. */
+static void
+box_module_func_delete(struct box_module_func *cf)
+{
+ assert(module_func_is_empty(&cf->base));
+ TRASH(cf);
+ free(cf);
+}
+
+/** Unreference a function and free if last one. */
+static void
+box_module_func_unref(struct box_module_func *cf)
+{
+ assert(cf->refs > 0);
+ if (--cf->refs == 0) {
+ module_func_unload(&cf->base);
+ cache_del(cf);
+ box_module_func_delete(cf);
+ }
+}
+
+/** Function name from a hash key. */
+static char *
+box_module_func_name(struct box_module_func *cf)
+{
+ return &cf->key[cf->len - cf->sym_len];
+}
+
+/**
+ * Allocate a new function instance and resolve its address.
+ *
+ * @param m a module the function should be loaded from.
+ * @param key function hash key, ie "addr.module.foo".
+ * @param len length of @a key.
+ * @param sym_len function symbol name length, ie 3 for "foo".
+ *
+ * @returns function instance on success, NULL otherwise (diag is set).
+ */
+static struct box_module_func *
+box_module_func_new(struct module *m, const char *key, size_t len, size_t sym_len)
+{
+ size_t size = sizeof(struct box_module_func) + len + 1;
+ struct box_module_func *cf = malloc(size);
+ if (cf == NULL) {
+ diag_set(OutOfMemory, size, "malloc", "cf");
+ return NULL;
+ }
+
+ cf->len = len;
+ cf->sym_len = sym_len;
+ cf->refs = 0;
+
+ module_func_create(&cf->base);
+ memcpy(cf->key, key, len);
+ cf->key[len] = '\0';
+
+ if (module_func_load(m, box_module_func_name(cf), &cf->base) != 0) {
+ box_module_func_delete(cf);
+ return NULL;
+ }
+
+ if (cache_put(cf) != 0) {
+ module_func_unload(&cf->base);
+ box_module_func_delete(cf);
+ return NULL;
+ }
+
+ /*
+ * Each new function depends on module presence.
+ * Module will reside even if been unload
+ * explicitly after the function creation.
+ */
+ box_module_func_ref(cf);
+ return cf;
+}
+
+/**
+ * Load a function.
+ *
+ * This function takes a function name from the caller
+ * stack @a L and either returns a cached function or
+ * creates a new function object.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: function name is either not supplied
+ * or not a string.
+ * - SystemError: unable to open a module due to a system error.
+ * - ClientError: a module does not exist.
+ * - OutOfMemory: unable to allocate a module.
+ *
+ * @returns module object on success or throws an error.
+ */
+static int
+lbox_module_load_func(struct lua_State *L)
+{
+ const char *method = "function = module:load";
+ const char *fmt_noname = "Expects %s(\'name\') but no name passed";
+
+ if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) {
+ diag_set(IllegalParams, fmt_noname, method);
+ return luaT_error(L);
+ }
+
+ struct module *m = get_udata(L, uname_lib);
+ if (m == NULL) {
+ const char *fmt =
+ "Expects %s(\'name\') but not module object passed";
+ diag_set(IllegalParams, fmt, method);
+ return luaT_error(L);
+ }
+
+ size_t sym_len;
+ const char *sym = lua_tolstring(L, 2, &sym_len);
+ const size_t max_sym_len = 512;
+
+ if (sym_len < 1) {
+ diag_set(IllegalParams, fmt_noname, method);
+ return luaT_error(L);
+ } else if (sym_len > max_sym_len) {
+ diag_set(IllegalParams, "Symbol \'%s\' is too long (max %zd)",
+ sym, max_sym_len);
+ return luaT_error(L);
+ }
+
+ /*
+ * Functions are bound to a module symbols, thus
+ * since the hash is global it should be unique
+ * per module. The symbol (function name) is the
+ * last part of the hash key.
+ *
+ * The key's buffer should be big enough to keep
+ * the longest package path plus symbol name and
+ * the pointer.
+ */
+ char key[PATH_MAX + 2 * max_sym_len];
+ snprintf(key, sizeof(key), "%p.%s.%s",
+ (void *)m, m->package, sym);
+ size_t len = strlen(key);
+
+ struct box_module_func *cf = cache_find(key, len);
+ if (cf == NULL) {
+ cf = box_module_func_new(m, key, len, sym_len);
+ if (cf == NULL)
+ return luaT_error(L);
+ } else {
+ box_module_func_ref(cf);
+ }
+
+ new_udata(L, uname_func, cf);
+ return 1;
+}
+
+/**
+ * Unload a function.
+ *
+ * Take a function object from the caller stack @a L and unload it.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: the function is not supplied.
+ * - IllegalParams: the function already unloaded.
+ *
+ * @returns true on success or throwns an error.
+ */
+static int
+lbox_func_unload(struct lua_State *L)
+{
+ if (lua_gettop(L) != 1) {
+ diag_set(IllegalParams, "Expects function:unload()");
+ return luaT_error(L);
+ }
+
+ struct box_module_func *cf = clear_udata(L, uname_func);
+ if (cf == NULL) {
+ diag_set(IllegalParams, "The function is unloaded");
+ return luaT_error(L);
+ }
+ box_module_func_unref(cf);
+
+ lua_pushboolean(L, true);
+ return 1;
+}
+
+/** Handle __index request for a function object. */
+static int
+lbox_func_index(struct lua_State *L)
+{
+ /*
+ * Process metamethods such as "func:unload" first.
+ */
+ lua_getmetatable(L, 1);
+ lua_pushvalue(L, 2);
+ lua_rawget(L, -2);
+ if (!lua_isnil(L, -1))
+ return 1;
+
+ struct box_module_func *cf = get_udata(L, uname_func);
+ if (cf == NULL) {
+ lua_pushnil(L);
+ return 1;
+ }
+
+ const char *key = lua_tostring(L, 2);
+ if (key == NULL || !lua_isstring(L, 2)) {
+ diag_set(IllegalParams,
+ "Bad params, use __index(<key>)");
+ return luaT_error(L);
+ }
+
+ if (strcmp(key, "name") == 0) {
+ lua_pushstring(L, box_module_func_name(cf));
+ return 1;
+ }
+
+ /*
+ * Internal keys for debug only, not API.
+ */
+ if (strcmp(key, "debug_refs") == 0) {
+ lua_pushnumber(L, cf->refs);
+ return 1;
+ } else if (strcmp(key, "debug_key") == 0) {
+ lua_pushstring(L, cf->key);
+ return 1;
+ } else if (strcmp(key, "debug_module_ptr") == 0) {
+ char s[64];
+ snprintf(s, sizeof(s), "%p", cf->base.module);
+ lua_pushstring(L, s);
+ return 1;
+ } else if (strcmp(key, "debug_module_refs") == 0) {
+ lua_pushnumber(L, cf->base.module->refs);
+ return 1;
+ }
+ return 0;
+}
+
+/** Function representation for REPL (console). */
+static int
+lbox_func_serialize(struct lua_State *L)
+{
+ struct box_module_func *cf = get_udata(L, uname_func);
+ if (cf == NULL) {
+ lua_pushnil(L);
+ return 1;
+ }
+
+ lua_createtable(L, 0, 1);
+ lua_pushstring(L, box_module_func_name(cf));
+ lua_setfield(L, -2, "name");
+ return 1;
+}
+
+/** Collect a function. */
+static int
+lbox_func_gc(struct lua_State *L)
+{
+ struct box_module_func *cf = clear_udata(L, uname_func);
+ if (cf != NULL)
+ box_module_func_unref(cf);
+ return 0;
+}
+
+
+/** Call a function by its name from the Lua code. */
+static int
+lbox_func_call(struct lua_State *L)
+{
+ struct box_module_func *cf = get_udata(L, uname_func);
+ if (cf == NULL) {
+ diag_set(IllegalParams, "The function is unloaded");
+ return luaT_error(L);
+ }
+
+ lua_State *args_L = luaT_newthread(tarantool_L);
+ if (args_L == NULL)
+ return luaT_error(L);
+
+ int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+ lua_xmove(L, args_L, lua_gettop(L) - 1);
+
+ struct port args;
+ port_lua_create(&args, args_L);
+ ((struct port_lua *)&args)->ref = coro_ref;
+
+ struct port ret;
+
+ if (module_func_call(&cf->base, &args, &ret) != 0) {
+ port_destroy(&args);
+ return luaT_error(L);
+ }
+
+ int top = lua_gettop(L);
+ port_dump_lua(&ret, L, true);
+ int cnt = lua_gettop(L) - top;
+
+ port_destroy(&ret);
+ port_destroy(&args);
+
+ return cnt;
+}
+
+void
+box_lua_lib_init(struct lua_State *L)
+{
+ func_hash = mh_strnptr_new();
+ if (func_hash == NULL)
+ panic("box.lib: Can't allocate func hash table");
+
+ static const struct luaL_Reg top_methods[] = {
+ { "load", lbox_module_load },
+ { NULL, NULL },
+ };
+ luaL_register(L, "box.lib", top_methods);
+ lua_pop(L, 1);
+
+ static const struct luaL_Reg lbox_module_methods[] = {
+ { "unload", lbox_module_unload },
+ { "load", lbox_module_load_func },
+ { "__index", lbox_module_index },
+ { "__serialize", lbox_module_serialize },
+ { "__gc", lbox_module_gc },
+ { NULL, NULL },
+ };
+ luaL_register_type(L, uname_lib, lbox_module_methods);
+
+ static const struct luaL_Reg lbox_func_methods[] = {
+ { "unload", lbox_func_unload },
+ { "__index", lbox_func_index },
+ { "__serialize", lbox_func_serialize },
+ { "__gc", lbox_func_gc },
+ { "__call", lbox_func_call },
+ { NULL, NULL },
+ };
+ luaL_register_type(L, uname_func, lbox_func_methods);
+}
diff --git a/src/box/lua/lib.h b/src/box/lua/lib.h
new file mode 100644
index 000000000..a86691b42
--- /dev/null
+++ b/src/box/lua/lib.h
@@ -0,0 +1,25 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+
+#pragma once
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct lua_State;
+
+/**
+ * Initialize Lua box.lib.
+ *
+ * @param L Lua state where to register.
+ */
+void
+box_lua_lib_init(struct lua_State *L);
+
+#if defined(__cplusplus)
+}
+#endif /* defined(__plusplus) */
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 44bb95ed1..374524973 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -597,6 +597,7 @@ local box_cfg_guard_whitelist = {
error = true;
internal = true;
index = true;
+ lib = true;
session = true;
tuple = true;
runtime = true;
diff --git a/test/box/misc.result b/test/box/misc.result
index e18a46e02..59fc60a22 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -85,6 +85,7 @@ t
- info
- internal
- is_in_txn
+ - lib
- on_commit
- on_rollback
- once
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module Cyrill Gorcunov via Tarantool-patches
@ 2021-04-11 15:38 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-11 22:38 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-11 15:38 UTC (permalink / raw)
To: Cyrill Gorcunov, tml
Thanks for the patch!
Please, try to respond to my comments in the original email,
explicitly. When and how you fixed them, or why did not. Not
with promises like "will do", "ok", and so on, they don't help
much.
Otherwise I see you tend to miss some of the comments. Also I
don't remember all of my comments, so when you don't respond to
them, I need to go to my old email, and match the comments
with the places you was supposed to fix, and it takes time.
See how Sergey and Mergen send their patches for examples.
https://github.com/tarantool/tarantool/wiki/Code-review-procedure#during-the-review
See 4 comments below.
> module:load(name) -> obj | error`
1. In the previous review I said you missed ` in the beginning
of the line here. Please, add it. Otherwise the formatting might
be broken.
> ---------------------------------
>
> Loads a new function with name `name` from the previously
> loaded `module` and return a callable object instance
> associated with the function. On failure an error is thrown.
>
> Possible errors:
> - IllegalParams: function name is either not supplied
> or not a string.
> - IllegalParams: attempt to load a function but module
> has been unloaded already.
> - ClientError: no such function in the module.
> - OutOfMemory: unable to allocate a function.
>
> Example:
>
> ``` Lua
> -- Load a module if not been loaded yet.
> m = box.lib.load('path/to/library')
> -- Load a function with the `foo` name from the module `m`.
> func = m:load('foo')
> ```
>
> diff --git a/src/box/lua/lib.c b/src/box/lua/lib.c
> new file mode 100644
> index 000000000..78aee37b5
> --- /dev/null
> +++ b/src/box/lua/lib.c
> @@ -0,0 +1,606 @@
> +
> +static int
> +cache_put(struct box_module_func *cf)
> +{
> + const struct mh_strnptr_node_t nd = {
> + .str = cf->key,
> + .len = cf->len,
> + .hash = mh_strn_hash(cf->key, cf->len),
> + .val = cf,
> + };
> + mh_int_t e = mh_strnptr_put(func_hash, &nd, NULL, NULL);
2. As I said in the previous review, it would be good to get the
old value and ensure it is NULL. So as not to replace something
and not notice it.
> + if (e == mh_end(func_hash)) {
> + diag_set(OutOfMemory, sizeof(nd), "malloc",
> + "box.lib: hash node");
> + return -1;
> + }
> + return 0;
> +}
<...>
> +static int
> +lbox_module_load_func(struct lua_State *L)
> +{
> + const char *method = "function = module:load";
> + const char *fmt_noname = "Expects %s(\'name\') but no name passed";
> +
> + if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) {
> + diag_set(IllegalParams, fmt_noname, method);
> + return luaT_error(L);
> + }
> +
> + struct module *m = get_udata(L, uname_lib);
> + if (m == NULL) {
> + const char *fmt =
> + "Expects %s(\'name\') but not module object passed";
> + diag_set(IllegalParams, fmt, method);
> + return luaT_error(L);
> + }
> +
> + size_t sym_len;
> + const char *sym = lua_tolstring(L, 2, &sym_len);
> + const size_t max_sym_len = 512;
> +
> + if (sym_len < 1) {
> + diag_set(IllegalParams, fmt_noname, method);
> + return luaT_error(L);
> + } else if (sym_len > max_sym_len) {
> + diag_set(IllegalParams, "Symbol \'%s\' is too long (max %zd)",
> + sym, max_sym_len);
> + return luaT_error(L);
> + }
> +
> + /*
> + * Functions are bound to a module symbols, thus
> + * since the hash is global it should be unique
> + * per module. The symbol (function name) is the
> + * last part of the hash key.
> + *
> + * The key's buffer should be big enough to keep
> + * the longest package path plus symbol name and
> + * the pointer.
> + */
> + char key[PATH_MAX + 2 * max_sym_len];
3. +3 for the dots you added below.
> + snprintf(key, sizeof(key), "%p.%s.%s",
> + (void *)m, m->package, sym);
4. Is '(void *)' cast needed? It should work implicitly, no?
> + size_t len = strlen(key);
> +
> + struct box_module_func *cf = cache_find(key, len);
> + if (cf == NULL) {
> + cf = box_module_func_new(m, key, len, sym_len);
> + if (cf == NULL)
> + return luaT_error(L);
> + } else {
> + box_module_func_ref(cf);
> + }
> +
> + new_udata(L, uname_func, cf);
> + return 1;
> +}
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module
2021-04-11 15:38 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-11 22:38 ` Cyrill Gorcunov via Tarantool-patches
2021-04-12 22:08 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-11 22:38 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tml
On Sun, Apr 11, 2021 at 05:38:51PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> Please, try to respond to my comments in the original email,
> explicitly. When and how you fixed them, or why did not. Not
> with promises like "will do", "ok", and so on, they don't help
> much.
>
> Otherwise I see you tend to miss some of the comments. Also I
> don't remember all of my comments, so when you don't respond to
> them, I need to go to my old email, and match the comments
> with the places you was supposed to fix, and it takes time.
>
> See how Sergey and Mergen send their patches for examples.
>
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#during-the-review
>
> See 4 comments below.
I fixed the change log. And here is a diff on top, please take a look.
---
Subject: [PATCH] box.lib fix
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/box/lua/lib.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/src/box/lua/lib.c b/src/box/lua/lib.c
index 5fb616d04..2181b9030 100644
--- a/src/box/lua/lib.c
+++ b/src/box/lua/lib.c
@@ -97,12 +97,22 @@ cache_put(struct box_module_func *cf)
.hash = mh_strn_hash(cf->key, cf->len),
.val = cf,
};
- mh_int_t e = mh_strnptr_put(func_hash, &nd, NULL, NULL);
+
+ struct mh_strnptr_node_t prev;
+ struct mh_strnptr_node_t *prev_ptr = &prev;
+
+ mh_int_t e = mh_strnptr_put(func_hash, &nd, &prev_ptr, NULL);
if (e == mh_end(func_hash)) {
diag_set(OutOfMemory, sizeof(nd), "malloc",
"box.lib: hash node");
return -1;
}
+
+ /*
+ * Just to make sure we haven't replaced something,
+ * the entries must be explicitly deleted.
+ */
+ assert(prev_ptr == NULL);
return 0;
}
@@ -383,7 +393,15 @@ lbox_module_load_func(struct lua_State *L)
size_t sym_len;
const char *sym = lua_tolstring(L, 2, &sym_len);
- const size_t max_sym_len = 512;
+
+ /*
+ * C standard requires at least 63 significant
+ * initial characters, though it advises to not
+ * impose limits. Lets make the max identifier
+ * big enough to keep longest id, which is hardly
+ * be bigger than 256 symbols.
+ */
+ const size_t max_sym_len = 256;
if (sym_len < 1) {
diag_set(IllegalParams, fmt_noname, method);
@@ -400,13 +418,12 @@ lbox_module_load_func(struct lua_State *L)
* per module. The symbol (function name) is the
* last part of the hash key.
*
- * The key's buffer should be big enough to keep
- * the longest package path plus symbol name and
- * the pointer.
+ * Make sure there is enough space for key,
+ * path and formatting.
*/
- char key[PATH_MAX + 2 * max_sym_len];
+ char key[PATH_MAX + max_sym_len + 32];
snprintf(key, sizeof(key), "%p.%s.%s",
- (void *)m, m->package, sym);
+ m, m->package, sym);
size_t len = strlen(key);
struct box_module_func *cf = cache_find(key, len);
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module
2021-04-11 22:38 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-04-12 22:08 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-12 22:34 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-12 22:08 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tml
Hi! Thanks for the patch!
Consider the diff below:
====================
@@ -413,17 +413,13 @@ lbox_module_load_func(struct lua_State *L)
}
/*
- * Functions are bound to a module symbols, thus
- * since the hash is global it should be unique
- * per module. The symbol (function name) is the
- * last part of the hash key.
- *
- * Make sure there is enough space for key,
- * path and formatting.
+ * Functions are bound to a module symbols, thus since the hash is
+ * global it should be unique per module.
+ * Make sure there is enough space for key, and formatting.
*/
- char key[PATH_MAX + max_sym_len + 32];
- snprintf(key, sizeof(key), "%p.%s.%s",
- m, m->package, sym);
+ char key[max_sym_len + 32];
+ int bytes = snprintf(key, sizeof(key), "%p.%s", m, sym) + 1;
+ assert(bytes <= (int)sizeof(key));
size_t len = strlen(key);
struct box_module_func *cf = cache_find(key, len);
====================
You don't need any of the 'm' object attributes in the hash if
the 'm' pointer is already included. It is unique. There can't be
2 objects with the same pointer in the memory.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module
2021-04-12 22:08 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-12 22:34 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 0 replies; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-12 22:34 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tml
On Tue, Apr 13, 2021 at 12:08:49AM +0200, Vladislav Shpilevoy wrote:
>
> You don't need any of the 'm' object attributes in the hash if
> the 'm' pointer is already included. It is unique. There can't be
> 2 objects with the same pointer in the memory.
Yeah. What about even simplier? On top of your patch since we don't
need to call strlen.
---
diff --git a/src/box/lua/lib.c b/src/box/lua/lib.c
index f8ce1be80..fa44163e0 100644
--- a/src/box/lua/lib.c
+++ b/src/box/lua/lib.c
@@ -418,9 +418,8 @@ lbox_module_load_func(struct lua_State *L)
* Make sure there is enough space for key, and formatting.
*/
char key[max_sym_len + 32];
- int bytes = snprintf(key, sizeof(key), "%p.%s", m, sym) + 1;
- assert(bytes <= (int)sizeof(key));
- size_t len = strlen(key);
+ size_t len = (size_t)snprintf(key, sizeof(key), "%p.%s", m, sym);
+ assert(len > 1 && len < sizeof(key));
struct box_module_func *cf = cache_find(key, len);
if (cf == NULL) {
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test
2021-04-08 16:41 [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches
` (4 preceding siblings ...)
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module Cyrill Gorcunov via Tarantool-patches
@ 2021-04-08 16:41 ` Cyrill Gorcunov via Tarantool-patches
2021-04-08 18:53 ` Cyrill Gorcunov via Tarantool-patches
2021-04-13 21:53 ` [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Vladislav Shpilevoy via Tarantool-patches
2021-04-14 8:07 ` Kirill Yukhin via Tarantool-patches
7 siblings, 1 reply; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-08 16:41 UTC (permalink / raw)
To: tml; +Cc: Vladislav Shpilevoy
Part-of #4642
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
test/box/CMakeLists.txt | 4 +
test/box/cfunc1.c | 58 +++++
test/box/cfunc2.c | 137 +++++++++++
test/box/cfunc3.c | 25 ++
test/box/cfunc4.c | 28 +++
test/box/lib.result | 530 ++++++++++++++++++++++++++++++++++++++++
test/box/lib.test.lua | 203 +++++++++++++++
7 files changed, 985 insertions(+)
create mode 100644 test/box/cfunc1.c
create mode 100644 test/box/cfunc2.c
create mode 100644 test/box/cfunc3.c
create mode 100644 test/box/cfunc4.c
create mode 100644 test/box/lib.result
create mode 100644 test/box/lib.test.lua
diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 4216a0ba9..51db15653 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -7,3 +7,7 @@ build_module(func_restore2 func_restore2.c)
build_module(func_restore3 func_restore3.c)
build_module(func_restore4 func_restore4.c)
build_module(tuple_bench tuple_bench.c)
+build_module(cfunc1 cfunc1.c)
+build_module(cfunc2 cfunc2.c)
+build_module(cfunc3 cfunc3.c)
+build_module(cfunc4 cfunc4.c)
diff --git a/test/box/cfunc1.c b/test/box/cfunc1.c
new file mode 100644
index 000000000..f6829372a
--- /dev/null
+++ b/test/box/cfunc1.c
@@ -0,0 +1,58 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Before the reload functions are just declared
+ * and simply exit with zero.
+ *
+ * After the module reload we should provide real
+ * functionality.
+ */
+
+int
+cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+int
+cfunc_fetch_seq_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+int
+cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+int
+cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+int
+cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
diff --git a/test/box/cfunc2.c b/test/box/cfunc2.c
new file mode 100644
index 000000000..8c583e993
--- /dev/null
+++ b/test/box/cfunc2.c
@@ -0,0 +1,137 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Just make sure we've been called.
+ */
+int
+cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+/*
+ * Fetch first N even numbers (just to make sure the order of
+ * arguments is not screwed).
+ */
+int
+cfunc_fetch_seq_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ int arg_count = mp_decode_array(&args);
+ if (arg_count != 1) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+ int field_count = mp_decode_array(&args);
+ if (field_count < 1) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid array size");
+ }
+
+ /*
+ * We expect even numbers sequence here. The idea is
+ * to test invalid data an issue an error from inside
+ * of C function.
+ */
+ for (int i = 1; i <= field_count; i++) {
+ int val = mp_decode_uint(&args);
+ int needed = 2 * i;
+ if (val != needed) {
+ char res[128];
+ snprintf(res, sizeof(res), "%s %d != %d",
+ "invalid argument", val, needed);
+ return box_error_set(__FILE__, __LINE__,
+ ER_PROC_C, "%s", res);
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * Return one element array twice.
+ */
+int
+cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ char tuple_buf[512];
+ char *d = tuple_buf;
+ d = mp_encode_array(d, 1);
+ d = mp_encode_uint(d, 1);
+ assert(d <= tuple_buf + sizeof(tuple_buf));
+
+ box_tuple_format_t *fmt = box_tuple_format_default();
+ box_tuple_t *tuple_a = box_tuple_new(fmt, tuple_buf, d);
+ if (tuple_a == NULL)
+ return -1;
+ int rc = box_return_tuple(ctx, tuple_a);
+ if (rc == 0)
+ return box_return_tuple(ctx, tuple_a);
+ return rc;
+}
+
+/*
+ * Encode int + string pair back.
+ */
+int
+cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ uint32_t arg_count = mp_decode_array(&args);
+ if (arg_count != 2) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+
+ if (mp_typeof(*args) != MP_UINT) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "tuple field must be uint");
+ }
+ uint32_t num = mp_decode_uint(&args);
+
+ if (mp_typeof(*args) != MP_STR) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "tuple field must be string");
+ }
+ const char *str = args;
+ uint32_t len = mp_decode_strl(&str);
+
+ char tuple_buf[512];
+ char *d = tuple_buf;
+ d = mp_encode_array(d, 2);
+ d = mp_encode_uint(d, num);
+ d = mp_encode_str(d, str, len);
+ assert(d <= tuple_buf + sizeof(tuple_buf));
+
+ box_tuple_format_t *fmt = box_tuple_format_default();
+ box_tuple_t *tuple = box_tuple_new(fmt, tuple_buf, d);
+ if (tuple == NULL)
+ return -1;
+
+ return box_return_tuple(ctx, tuple);
+}
+
+/*
+ * Sum two integers.
+ */
+int
+cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ uint32_t arg_count = mp_decode_array(&args);
+ if (arg_count != 2) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+ uint64_t a = mp_decode_uint(&args);
+ uint64_t b = mp_decode_uint(&args);
+
+ char res[16];
+ char *end = mp_encode_uint(res, a + b);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
diff --git a/test/box/cfunc3.c b/test/box/cfunc3.c
new file mode 100644
index 000000000..668790fbf
--- /dev/null
+++ b/test/box/cfunc3.c
@@ -0,0 +1,25 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Sum two integers.
+ */
+int
+cfunc_add(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ uint32_t arg_count = mp_decode_array(&args);
+ if (arg_count != 2) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+ uint64_t a = mp_decode_uint(&args);
+ uint64_t b = mp_decode_uint(&args);
+
+ char res[16];
+ char *end = mp_encode_uint(res, a + b);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
diff --git a/test/box/cfunc4.c b/test/box/cfunc4.c
new file mode 100644
index 000000000..cc079b335
--- /dev/null
+++ b/test/box/cfunc4.c
@@ -0,0 +1,28 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Sum two integers and add a constant,
+ * so that result will be different after
+ * the reload.
+ */
+int
+cfunc_add(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ const uint32_t delta = 10;
+ uint32_t arg_count = mp_decode_array(&args);
+ if (arg_count != 2) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+ uint64_t a = mp_decode_uint(&args);
+ uint64_t b = mp_decode_uint(&args);
+
+ char res[16];
+ char *end = mp_encode_uint(res, a + b + delta);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
diff --git a/test/box/lib.result b/test/box/lib.result
new file mode 100644
index 000000000..43c396dbd
--- /dev/null
+++ b/test/box/lib.result
@@ -0,0 +1,530 @@
+-- test-run result file version 2
+--
+-- gh-4642: New box.lib module to be able to
+-- run C stored functions on read only nodes
+-- without requirement to register them with
+-- box.schema.func help.
+--
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+
+boxlib = require('box.lib')
+ | ---
+ | ...
+fio = require('fio')
+ | ---
+ | ...
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+ | ---
+ | ...
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext
+ | ---
+ | ...
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext
+ | ---
+ | ...
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext
+ | ---
+ | ...
+cfunc3_path = fio.pathjoin(build_path, "test/box/cfunc3.") .. ext
+ | ---
+ | ...
+cfunc4_path = fio.pathjoin(build_path, "test/box/cfunc4.") .. ext
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc1_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+_, err = pcall(boxlib.load, 'non-such-module')
+ | ---
+ | ...
+assert(err ~= nil)
+ | ---
+ | - true
+ | ...
+
+-- All functions are sitting in cfunc.so.
+old_module = boxlib.load('cfunc')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+old_module_copy = boxlib.load('cfunc')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+assert(old_module_copy['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+old_module_copy:unload()
+ | ---
+ | - true
+ | ...
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+old_cfunc_nop = old_module:load('cfunc_nop')
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+ | ---
+ | ...
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+ | ---
+ | ...
+old_cfunc_args = old_module:load('cfunc_args')
+ | ---
+ | ...
+old_cfunc_sum = old_module:load('cfunc_sum')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 6)
+ | ---
+ | - true
+ | ...
+
+-- Test for error on nonexisting function.
+_, err = pcall(old_module.load, old_module, 'no-such-func')
+ | ---
+ | ...
+assert(err ~= nil)
+ | ---
+ | - true
+ | ...
+
+-- Make sure they all are callable.
+old_cfunc_nop()
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens()
+ | ---
+ | ...
+old_cfunc_multireturn()
+ | ---
+ | ...
+old_cfunc_args()
+ | ---
+ | ...
+old_cfunc_sum()
+ | ---
+ | ...
+
+-- Unload the module but keep old functions alive, so
+-- they keep reference to NOP module internally
+-- and still callable.
+old_module:unload()
+ | ---
+ | - true
+ | ...
+-- Test refs via function name.
+assert(old_cfunc_nop['debug_module_refs'] == 5)
+ | ---
+ | - true
+ | ...
+old_cfunc_nop()
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens()
+ | ---
+ | ...
+old_cfunc_multireturn()
+ | ---
+ | ...
+old_cfunc_args()
+ | ---
+ | ...
+old_cfunc_sum()
+ | ---
+ | ...
+
+-- The module is unloaded I should not be able
+-- to load new shared library.
+old_module:load('cfunc')
+ | ---
+ | - error: Expects function = module:load('name') but not module object passed
+ | ...
+-- Neither I should be able to unload module twise.
+old_module:unload()
+ | ---
+ | - error: The module is unloaded
+ | ...
+
+-- Clean old functions.
+old_cfunc_nop:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_fetch_seq_evens:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_multireturn:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_args:unload()
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_sum['debug_module_refs'] == 1)
+ | ---
+ | - true
+ | ...
+old_cfunc_sum:unload()
+ | ---
+ | - true
+ | ...
+
+-- And reload old module again.
+old_module = boxlib.load('cfunc')
+ | ---
+ | ...
+old_module_ptr = old_module['debug_ptr']
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+
+-- Overwrite module with new contents.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc2_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+-- Load new module, cache should be updated.
+new_module = boxlib.load('cfunc')
+ | ---
+ | ...
+new_module_ptr = new_module['debug_ptr']
+ | ---
+ | ...
+
+-- Old and new module keep one reference with
+-- different IDs.
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+assert(old_module['debug_refs'] == new_module['debug_refs'])
+ | ---
+ | - true
+ | ...
+assert(old_module_ptr ~= new_module_ptr)
+ | ---
+ | - true
+ | ...
+
+-- All functions from old module should be loadable.
+old_cfunc_nop = old_module:load('cfunc_nop')
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+ | ---
+ | ...
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+ | ---
+ | ...
+old_cfunc_args = old_module:load('cfunc_args')
+ | ---
+ | ...
+old_cfunc_sum = old_module:load('cfunc_sum')
+ | ---
+ | ...
+assert(old_cfunc_nop['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_fetch_seq_evens['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_multireturn['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_args['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_sum['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_module['debug_refs'] == 6)
+ | ---
+ | - true
+ | ...
+
+-- Lookup for updated symbols.
+new_cfunc_nop = new_module:load('cfunc_nop')
+ | ---
+ | ...
+new_cfunc_fetch_seq_evens = new_module:load('cfunc_fetch_seq_evens')
+ | ---
+ | ...
+new_cfunc_multireturn = new_module:load('cfunc_multireturn')
+ | ---
+ | ...
+new_cfunc_args = new_module:load('cfunc_args')
+ | ---
+ | ...
+new_cfunc_sum = new_module:load('cfunc_sum')
+ | ---
+ | ...
+assert(new_cfunc_nop['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_fetch_seq_evens['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_multireturn['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_args['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_sum['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_module['debug_refs'] == 6)
+ | ---
+ | - true
+ | ...
+
+-- Call old functions.
+old_cfunc_nop()
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens()
+ | ---
+ | ...
+old_cfunc_multireturn()
+ | ---
+ | ...
+old_cfunc_args()
+ | ---
+ | ...
+old_cfunc_sum()
+ | ---
+ | ...
+
+-- Call new functions.
+new_cfunc_nop()
+ | ---
+ | ...
+new_cfunc_multireturn()
+ | ---
+ | - [1]
+ | - [1]
+ | ...
+new_cfunc_fetch_seq_evens({2,4,6})
+ | ---
+ | ...
+new_cfunc_fetch_seq_evens({1,2,3}) -- error, odd numbers sequence
+ | ---
+ | - error: invalid argument 1 != 2
+ | ...
+new_cfunc_args(1, "hello")
+ | ---
+ | - [1, 'hello']
+ | ...
+new_cfunc_sum(1) -- error, one arg passed
+ | ---
+ | - error: invalid argument count
+ | ...
+new_cfunc_sum(1,2)
+ | ---
+ | - 3
+ | ...
+
+-- Cleanup old module's functions.
+old_cfunc_nop:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_fetch_seq_evens:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_multireturn:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_args:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_sum:unload()
+ | ---
+ | - true
+ | ...
+old_module:unload()
+ | ---
+ | - true
+ | ...
+
+-- Cleanup new module data.
+new_cfunc_nop:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_multireturn:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_fetch_seq_evens:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_args:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_sum:unload()
+ | ---
+ | - true
+ | ...
+new_module:unload()
+ | ---
+ | - true
+ | ...
+
+-- Cleanup the generated symlink.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+
+-- Test double hashing: create function
+-- in box.schema.fun so that it should
+-- appear in box.lib hash.
+fio.symlink(cfunc3_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+box.schema.func.create('cfunc.cfunc_add', {language = "C"})
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
+ | ---
+ | ...
+box.func['cfunc.cfunc_add']:call({1,2})
+ | ---
+ | - 3
+ | ...
+
+old_module = boxlib.load('cfunc')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
+ | ---
+ | - true
+ | ...
+old_func = old_module:load('cfunc_add')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 4) -- plus function instance
+ | ---
+ | - true
+ | ...
+old_func(1,2)
+ | ---
+ | - 3
+ | ...
+
+-- Now update on disk and reload the module.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc4_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+box.schema.func.reload("cfunc")
+ | ---
+ | ...
+box.func['cfunc.cfunc_add']:call({1,2})
+ | ---
+ | - 13
+ | ...
+
+-- The box.lib instance should carry own
+-- references to the module and old
+-- function. And reloading must not
+-- affect old functions. Thus one for
+-- box.lib and one for box.lib function.
+assert(old_module['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+old_func(1,2)
+ | ---
+ | - 3
+ | ...
+old_func:unload()
+ | ---
+ | - true
+ | ...
+old_module:unload()
+ | ---
+ | - true
+ | ...
+
+-- Same time the reload should update
+-- low level module cache, thus two
+-- for box and box function plus one
+-- new box.lib.
+new_module = boxlib.load('cfunc')
+ | ---
+ | ...
+assert(new_module['debug_refs'] == 3)
+ | ---
+ | - true
+ | ...
+
+-- Box function should carry own module.
+box.func['cfunc.cfunc_add']:call({1,2})
+ | ---
+ | - 13
+ | ...
+
+-- Cleanup.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+new_module:unload()
+ | ---
+ | - true
+ | ...
+box.schema.func.drop('cfunc.cfunc_add')
+ | ---
+ | ...
diff --git a/test/box/lib.test.lua b/test/box/lib.test.lua
new file mode 100644
index 000000000..445c2121c
--- /dev/null
+++ b/test/box/lib.test.lua
@@ -0,0 +1,203 @@
+--
+-- gh-4642: New box.lib module to be able to
+-- run C stored functions on read only nodes
+-- without requirement to register them with
+-- box.schema.func help.
+--
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+boxlib = require('box.lib')
+fio = require('fio')
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext
+cfunc3_path = fio.pathjoin(build_path, "test/box/cfunc3.") .. ext
+cfunc4_path = fio.pathjoin(build_path, "test/box/cfunc4.") .. ext
+
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc1_path, cfunc_path)
+
+_, err = pcall(boxlib.load, 'non-such-module')
+assert(err ~= nil)
+
+-- All functions are sitting in cfunc.so.
+old_module = boxlib.load('cfunc')
+assert(old_module['debug_refs'] == 1)
+old_module_copy = boxlib.load('cfunc')
+assert(old_module['debug_refs'] == 2)
+assert(old_module_copy['debug_refs'] == 2)
+old_module_copy:unload()
+assert(old_module['debug_refs'] == 1)
+old_cfunc_nop = old_module:load('cfunc_nop')
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_sum = old_module:load('cfunc_sum')
+assert(old_module['debug_refs'] == 6)
+
+-- Test for error on nonexisting function.
+_, err = pcall(old_module.load, old_module, 'no-such-func')
+assert(err ~= nil)
+
+-- Make sure they all are callable.
+old_cfunc_nop()
+old_cfunc_fetch_seq_evens()
+old_cfunc_multireturn()
+old_cfunc_args()
+old_cfunc_sum()
+
+-- Unload the module but keep old functions alive, so
+-- they keep reference to NOP module internally
+-- and still callable.
+old_module:unload()
+-- Test refs via function name.
+assert(old_cfunc_nop['debug_module_refs'] == 5)
+old_cfunc_nop()
+old_cfunc_fetch_seq_evens()
+old_cfunc_multireturn()
+old_cfunc_args()
+old_cfunc_sum()
+
+-- The module is unloaded I should not be able
+-- to load new shared library.
+old_module:load('cfunc')
+-- Neither I should be able to unload module twise.
+old_module:unload()
+
+-- Clean old functions.
+old_cfunc_nop:unload()
+old_cfunc_fetch_seq_evens:unload()
+old_cfunc_multireturn:unload()
+old_cfunc_args:unload()
+assert(old_cfunc_sum['debug_module_refs'] == 1)
+old_cfunc_sum:unload()
+
+-- And reload old module again.
+old_module = boxlib.load('cfunc')
+old_module_ptr = old_module['debug_ptr']
+assert(old_module['debug_refs'] == 1)
+
+-- Overwrite module with new contents.
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc2_path, cfunc_path)
+
+-- Load new module, cache should be updated.
+new_module = boxlib.load('cfunc')
+new_module_ptr = new_module['debug_ptr']
+
+-- Old and new module keep one reference with
+-- different IDs.
+assert(old_module['debug_refs'] == 1)
+assert(old_module['debug_refs'] == new_module['debug_refs'])
+assert(old_module_ptr ~= new_module_ptr)
+
+-- All functions from old module should be loadable.
+old_cfunc_nop = old_module:load('cfunc_nop')
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_sum = old_module:load('cfunc_sum')
+assert(old_cfunc_nop['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_fetch_seq_evens['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_multireturn['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_args['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_sum['debug_module_ptr'] == old_module_ptr)
+assert(old_module['debug_refs'] == 6)
+
+-- Lookup for updated symbols.
+new_cfunc_nop = new_module:load('cfunc_nop')
+new_cfunc_fetch_seq_evens = new_module:load('cfunc_fetch_seq_evens')
+new_cfunc_multireturn = new_module:load('cfunc_multireturn')
+new_cfunc_args = new_module:load('cfunc_args')
+new_cfunc_sum = new_module:load('cfunc_sum')
+assert(new_cfunc_nop['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_fetch_seq_evens['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_multireturn['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_args['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_sum['debug_module_ptr'] == new_module_ptr)
+assert(new_module['debug_refs'] == 6)
+
+-- Call old functions.
+old_cfunc_nop()
+old_cfunc_fetch_seq_evens()
+old_cfunc_multireturn()
+old_cfunc_args()
+old_cfunc_sum()
+
+-- Call new functions.
+new_cfunc_nop()
+new_cfunc_multireturn()
+new_cfunc_fetch_seq_evens({2,4,6})
+new_cfunc_fetch_seq_evens({1,2,3}) -- error, odd numbers sequence
+new_cfunc_args(1, "hello")
+new_cfunc_sum(1) -- error, one arg passed
+new_cfunc_sum(1,2)
+
+-- Cleanup old module's functions.
+old_cfunc_nop:unload()
+old_cfunc_fetch_seq_evens:unload()
+old_cfunc_multireturn:unload()
+old_cfunc_args:unload()
+old_cfunc_sum:unload()
+old_module:unload()
+
+-- Cleanup new module data.
+new_cfunc_nop:unload()
+new_cfunc_multireturn:unload()
+new_cfunc_fetch_seq_evens:unload()
+new_cfunc_args:unload()
+new_cfunc_sum:unload()
+new_module:unload()
+
+-- Cleanup the generated symlink.
+_ = pcall(fio.unlink(cfunc_path))
+
+-- Test double hashing: create function
+-- in box.schema.fun so that it should
+-- appear in box.lib hash.
+fio.symlink(cfunc3_path, cfunc_path)
+box.schema.func.create('cfunc.cfunc_add', {language = "C"})
+box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
+box.func['cfunc.cfunc_add']:call({1,2})
+
+old_module = boxlib.load('cfunc')
+assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
+old_func = old_module:load('cfunc_add')
+assert(old_module['debug_refs'] == 4) -- plus function instance
+old_func(1,2)
+
+-- Now update on disk and reload the module.
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc4_path, cfunc_path)
+
+box.schema.func.reload("cfunc")
+box.func['cfunc.cfunc_add']:call({1,2})
+
+-- The box.lib instance should carry own
+-- references to the module and old
+-- function. And reloading must not
+-- affect old functions. Thus one for
+-- box.lib and one for box.lib function.
+assert(old_module['debug_refs'] == 2)
+old_func(1,2)
+old_func:unload()
+old_module:unload()
+
+-- Same time the reload should update
+-- low level module cache, thus two
+-- for box and box function plus one
+-- new box.lib.
+new_module = boxlib.load('cfunc')
+assert(new_module['debug_refs'] == 3)
+
+-- Box function should carry own module.
+box.func['cfunc.cfunc_add']:call({1,2})
+
+-- Cleanup.
+_ = pcall(fio.unlink(cfunc_path))
+new_module:unload()
+box.schema.func.drop('cfunc.cfunc_add')
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread* [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test Cyrill Gorcunov via Tarantool-patches
@ 2021-04-08 18:53 ` Cyrill Gorcunov via Tarantool-patches
2021-04-11 15:43 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-08 18:53 UTC (permalink / raw)
To: tml; +Cc: Vladislav Shpilevoy
Guys, I had to update the test. I managed to run tests on
patched test-run repo so didn't notice the issue until
read logs from github. I force pushed an update. Here is
the whole patch
---
test/box/CMakeLists.txt | 4 +
test/box/cfunc1.c | 58 +++++
test/box/cfunc2.c | 137 +++++++++++
test/box/cfunc3.c | 25 ++
test/box/cfunc4.c | 28 +++
test/box/lib.result | 527 ++++++++++++++++++++++++++++++++++++++++
test/box/lib.test.lua | 202 +++++++++++++++
7 files changed, 981 insertions(+)
create mode 100644 test/box/cfunc1.c
create mode 100644 test/box/cfunc2.c
create mode 100644 test/box/cfunc3.c
create mode 100644 test/box/cfunc4.c
create mode 100644 test/box/lib.result
create mode 100644 test/box/lib.test.lua
diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
index 4216a0ba9..51db15653 100644
--- a/test/box/CMakeLists.txt
+++ b/test/box/CMakeLists.txt
@@ -7,3 +7,7 @@ build_module(func_restore2 func_restore2.c)
build_module(func_restore3 func_restore3.c)
build_module(func_restore4 func_restore4.c)
build_module(tuple_bench tuple_bench.c)
+build_module(cfunc1 cfunc1.c)
+build_module(cfunc2 cfunc2.c)
+build_module(cfunc3 cfunc3.c)
+build_module(cfunc4 cfunc4.c)
diff --git a/test/box/cfunc1.c b/test/box/cfunc1.c
new file mode 100644
index 000000000..f6829372a
--- /dev/null
+++ b/test/box/cfunc1.c
@@ -0,0 +1,58 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Before the reload functions are just declared
+ * and simply exit with zero.
+ *
+ * After the module reload we should provide real
+ * functionality.
+ */
+
+int
+cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+int
+cfunc_fetch_seq_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+int
+cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+int
+cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+int
+cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
diff --git a/test/box/cfunc2.c b/test/box/cfunc2.c
new file mode 100644
index 000000000..8c583e993
--- /dev/null
+++ b/test/box/cfunc2.c
@@ -0,0 +1,137 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Just make sure we've been called.
+ */
+int
+cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ (void)ctx;
+ (void)args;
+ (void)args_end;
+ return 0;
+}
+
+/*
+ * Fetch first N even numbers (just to make sure the order of
+ * arguments is not screwed).
+ */
+int
+cfunc_fetch_seq_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ int arg_count = mp_decode_array(&args);
+ if (arg_count != 1) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+ int field_count = mp_decode_array(&args);
+ if (field_count < 1) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid array size");
+ }
+
+ /*
+ * We expect even numbers sequence here. The idea is
+ * to test invalid data an issue an error from inside
+ * of C function.
+ */
+ for (int i = 1; i <= field_count; i++) {
+ int val = mp_decode_uint(&args);
+ int needed = 2 * i;
+ if (val != needed) {
+ char res[128];
+ snprintf(res, sizeof(res), "%s %d != %d",
+ "invalid argument", val, needed);
+ return box_error_set(__FILE__, __LINE__,
+ ER_PROC_C, "%s", res);
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * Return one element array twice.
+ */
+int
+cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ char tuple_buf[512];
+ char *d = tuple_buf;
+ d = mp_encode_array(d, 1);
+ d = mp_encode_uint(d, 1);
+ assert(d <= tuple_buf + sizeof(tuple_buf));
+
+ box_tuple_format_t *fmt = box_tuple_format_default();
+ box_tuple_t *tuple_a = box_tuple_new(fmt, tuple_buf, d);
+ if (tuple_a == NULL)
+ return -1;
+ int rc = box_return_tuple(ctx, tuple_a);
+ if (rc == 0)
+ return box_return_tuple(ctx, tuple_a);
+ return rc;
+}
+
+/*
+ * Encode int + string pair back.
+ */
+int
+cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ uint32_t arg_count = mp_decode_array(&args);
+ if (arg_count != 2) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+
+ if (mp_typeof(*args) != MP_UINT) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "tuple field must be uint");
+ }
+ uint32_t num = mp_decode_uint(&args);
+
+ if (mp_typeof(*args) != MP_STR) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "tuple field must be string");
+ }
+ const char *str = args;
+ uint32_t len = mp_decode_strl(&str);
+
+ char tuple_buf[512];
+ char *d = tuple_buf;
+ d = mp_encode_array(d, 2);
+ d = mp_encode_uint(d, num);
+ d = mp_encode_str(d, str, len);
+ assert(d <= tuple_buf + sizeof(tuple_buf));
+
+ box_tuple_format_t *fmt = box_tuple_format_default();
+ box_tuple_t *tuple = box_tuple_new(fmt, tuple_buf, d);
+ if (tuple == NULL)
+ return -1;
+
+ return box_return_tuple(ctx, tuple);
+}
+
+/*
+ * Sum two integers.
+ */
+int
+cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ uint32_t arg_count = mp_decode_array(&args);
+ if (arg_count != 2) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+ uint64_t a = mp_decode_uint(&args);
+ uint64_t b = mp_decode_uint(&args);
+
+ char res[16];
+ char *end = mp_encode_uint(res, a + b);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
diff --git a/test/box/cfunc3.c b/test/box/cfunc3.c
new file mode 100644
index 000000000..668790fbf
--- /dev/null
+++ b/test/box/cfunc3.c
@@ -0,0 +1,25 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Sum two integers.
+ */
+int
+cfunc_add(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ uint32_t arg_count = mp_decode_array(&args);
+ if (arg_count != 2) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+ uint64_t a = mp_decode_uint(&args);
+ uint64_t b = mp_decode_uint(&args);
+
+ char res[16];
+ char *end = mp_encode_uint(res, a + b);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
diff --git a/test/box/cfunc4.c b/test/box/cfunc4.c
new file mode 100644
index 000000000..cc079b335
--- /dev/null
+++ b/test/box/cfunc4.c
@@ -0,0 +1,28 @@
+#include <stdio.h>
+#include <stdbool.h>
+#include <msgpuck.h>
+
+#include "module.h"
+
+/*
+ * Sum two integers and add a constant,
+ * so that result will be different after
+ * the reload.
+ */
+int
+cfunc_add(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+ const uint32_t delta = 10;
+ uint32_t arg_count = mp_decode_array(&args);
+ if (arg_count != 2) {
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ "invalid argument count");
+ }
+ uint64_t a = mp_decode_uint(&args);
+ uint64_t b = mp_decode_uint(&args);
+
+ char res[16];
+ char *end = mp_encode_uint(res, a + b + delta);
+ box_return_mp(ctx, res, end);
+ return 0;
+}
diff --git a/test/box/lib.result b/test/box/lib.result
new file mode 100644
index 000000000..2986df049
--- /dev/null
+++ b/test/box/lib.result
@@ -0,0 +1,527 @@
+-- test-run result file version 2
+--
+-- gh-4642: New box.lib module to be able to
+-- run C stored functions on read only nodes
+-- without requirement to register them with
+-- box.schema.func help.
+--
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+
+fio = require('fio')
+ | ---
+ | ...
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+ | ---
+ | ...
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext
+ | ---
+ | ...
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext
+ | ---
+ | ...
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext
+ | ---
+ | ...
+cfunc3_path = fio.pathjoin(build_path, "test/box/cfunc3.") .. ext
+ | ---
+ | ...
+cfunc4_path = fio.pathjoin(build_path, "test/box/cfunc4.") .. ext
+ | ---
+ | ...
+
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc1_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+_, err = pcall(box.lib.load, 'non-such-module')
+ | ---
+ | ...
+assert(err ~= nil)
+ | ---
+ | - true
+ | ...
+
+-- All functions are sitting in cfunc.so.
+old_module = box.lib.load('cfunc')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+old_module_copy = box.lib.load('cfunc')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+assert(old_module_copy['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+old_module_copy:unload()
+ | ---
+ | - true
+ | ...
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+old_cfunc_nop = old_module:load('cfunc_nop')
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+ | ---
+ | ...
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+ | ---
+ | ...
+old_cfunc_args = old_module:load('cfunc_args')
+ | ---
+ | ...
+old_cfunc_sum = old_module:load('cfunc_sum')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 6)
+ | ---
+ | - true
+ | ...
+
+-- Test for error on nonexisting function.
+_, err = pcall(old_module.load, old_module, 'no-such-func')
+ | ---
+ | ...
+assert(err ~= nil)
+ | ---
+ | - true
+ | ...
+
+-- Make sure they all are callable.
+old_cfunc_nop()
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens()
+ | ---
+ | ...
+old_cfunc_multireturn()
+ | ---
+ | ...
+old_cfunc_args()
+ | ---
+ | ...
+old_cfunc_sum()
+ | ---
+ | ...
+
+-- Unload the module but keep old functions alive, so
+-- they keep reference to NOP module internally
+-- and still callable.
+old_module:unload()
+ | ---
+ | - true
+ | ...
+-- Test refs via function name.
+assert(old_cfunc_nop['debug_module_refs'] == 5)
+ | ---
+ | - true
+ | ...
+old_cfunc_nop()
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens()
+ | ---
+ | ...
+old_cfunc_multireturn()
+ | ---
+ | ...
+old_cfunc_args()
+ | ---
+ | ...
+old_cfunc_sum()
+ | ---
+ | ...
+
+-- The module is unloaded I should not be able
+-- to load new shared library.
+old_module:load('cfunc')
+ | ---
+ | - error: Expects function = module:load('name') but not module object passed
+ | ...
+-- Neither I should be able to unload module twise.
+old_module:unload()
+ | ---
+ | - error: The module is unloaded
+ | ...
+
+-- Clean old functions.
+old_cfunc_nop:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_fetch_seq_evens:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_multireturn:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_args:unload()
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_sum['debug_module_refs'] == 1)
+ | ---
+ | - true
+ | ...
+old_cfunc_sum:unload()
+ | ---
+ | - true
+ | ...
+
+-- And reload old module again.
+old_module = box.lib.load('cfunc')
+ | ---
+ | ...
+old_module_ptr = old_module['debug_ptr']
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+
+-- Overwrite module with new contents.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc2_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+-- Load new module, cache should be updated.
+new_module = box.lib.load('cfunc')
+ | ---
+ | ...
+new_module_ptr = new_module['debug_ptr']
+ | ---
+ | ...
+
+-- Old and new module keep one reference with
+-- different IDs.
+assert(old_module['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+assert(old_module['debug_refs'] == new_module['debug_refs'])
+ | ---
+ | - true
+ | ...
+assert(old_module_ptr ~= new_module_ptr)
+ | ---
+ | - true
+ | ...
+
+-- All functions from old module should be loadable.
+old_cfunc_nop = old_module:load('cfunc_nop')
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+ | ---
+ | ...
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+ | ---
+ | ...
+old_cfunc_args = old_module:load('cfunc_args')
+ | ---
+ | ...
+old_cfunc_sum = old_module:load('cfunc_sum')
+ | ---
+ | ...
+assert(old_cfunc_nop['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_fetch_seq_evens['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_multireturn['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_args['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_cfunc_sum['debug_module_ptr'] == old_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(old_module['debug_refs'] == 6)
+ | ---
+ | - true
+ | ...
+
+-- Lookup for updated symbols.
+new_cfunc_nop = new_module:load('cfunc_nop')
+ | ---
+ | ...
+new_cfunc_fetch_seq_evens = new_module:load('cfunc_fetch_seq_evens')
+ | ---
+ | ...
+new_cfunc_multireturn = new_module:load('cfunc_multireturn')
+ | ---
+ | ...
+new_cfunc_args = new_module:load('cfunc_args')
+ | ---
+ | ...
+new_cfunc_sum = new_module:load('cfunc_sum')
+ | ---
+ | ...
+assert(new_cfunc_nop['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_fetch_seq_evens['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_multireturn['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_args['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_cfunc_sum['debug_module_ptr'] == new_module_ptr)
+ | ---
+ | - true
+ | ...
+assert(new_module['debug_refs'] == 6)
+ | ---
+ | - true
+ | ...
+
+-- Call old functions.
+old_cfunc_nop()
+ | ---
+ | ...
+old_cfunc_fetch_seq_evens()
+ | ---
+ | ...
+old_cfunc_multireturn()
+ | ---
+ | ...
+old_cfunc_args()
+ | ---
+ | ...
+old_cfunc_sum()
+ | ---
+ | ...
+
+-- Call new functions.
+new_cfunc_nop()
+ | ---
+ | ...
+new_cfunc_multireturn()
+ | ---
+ | - [1]
+ | - [1]
+ | ...
+new_cfunc_fetch_seq_evens({2,4,6})
+ | ---
+ | ...
+new_cfunc_fetch_seq_evens({1,2,3}) -- error, odd numbers sequence
+ | ---
+ | - error: invalid argument 1 != 2
+ | ...
+new_cfunc_args(1, "hello")
+ | ---
+ | - [1, 'hello']
+ | ...
+new_cfunc_sum(1) -- error, one arg passed
+ | ---
+ | - error: invalid argument count
+ | ...
+new_cfunc_sum(1,2)
+ | ---
+ | - 3
+ | ...
+
+-- Cleanup old module's functions.
+old_cfunc_nop:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_fetch_seq_evens:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_multireturn:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_args:unload()
+ | ---
+ | - true
+ | ...
+old_cfunc_sum:unload()
+ | ---
+ | - true
+ | ...
+old_module:unload()
+ | ---
+ | - true
+ | ...
+
+-- Cleanup new module data.
+new_cfunc_nop:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_multireturn:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_fetch_seq_evens:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_args:unload()
+ | ---
+ | - true
+ | ...
+new_cfunc_sum:unload()
+ | ---
+ | - true
+ | ...
+new_module:unload()
+ | ---
+ | - true
+ | ...
+
+-- Cleanup the generated symlink.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+
+-- Test double hashing: create function
+-- in box.schema.fun so that it should
+-- appear in box.lib hash.
+fio.symlink(cfunc3_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+box.schema.func.create('cfunc.cfunc_add', {language = "C"})
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
+ | ---
+ | ...
+box.func['cfunc.cfunc_add']:call({1,2})
+ | ---
+ | - 3
+ | ...
+
+old_module = box.lib.load('cfunc')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
+ | ---
+ | - true
+ | ...
+old_func = old_module:load('cfunc_add')
+ | ---
+ | ...
+assert(old_module['debug_refs'] == 4) -- plus function instance
+ | ---
+ | - true
+ | ...
+old_func(1,2)
+ | ---
+ | - 3
+ | ...
+
+-- Now update on disk and reload the module.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+fio.symlink(cfunc4_path, cfunc_path)
+ | ---
+ | - true
+ | ...
+
+box.schema.func.reload("cfunc")
+ | ---
+ | ...
+box.func['cfunc.cfunc_add']:call({1,2})
+ | ---
+ | - 13
+ | ...
+
+-- The box.lib instance should carry own
+-- references to the module and old
+-- function. And reloading must not
+-- affect old functions. Thus one for
+-- box.lib and one for box.lib function.
+assert(old_module['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+old_func(1,2)
+ | ---
+ | - 3
+ | ...
+old_func:unload()
+ | ---
+ | - true
+ | ...
+old_module:unload()
+ | ---
+ | - true
+ | ...
+
+-- Same time the reload should update
+-- low level module cache, thus two
+-- for box and box function plus one
+-- new box.lib.
+new_module = box.lib.load('cfunc')
+ | ---
+ | ...
+assert(new_module['debug_refs'] == 3)
+ | ---
+ | - true
+ | ...
+
+-- Box function should carry own module.
+box.func['cfunc.cfunc_add']:call({1,2})
+ | ---
+ | - 13
+ | ...
+
+-- Cleanup.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
+new_module:unload()
+ | ---
+ | - true
+ | ...
+box.schema.func.drop('cfunc.cfunc_add')
+ | ---
+ | ...
diff --git a/test/box/lib.test.lua b/test/box/lib.test.lua
new file mode 100644
index 000000000..387c813c4
--- /dev/null
+++ b/test/box/lib.test.lua
@@ -0,0 +1,202 @@
+--
+-- gh-4642: New box.lib module to be able to
+-- run C stored functions on read only nodes
+-- without requirement to register them with
+-- box.schema.func help.
+--
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+
+fio = require('fio')
+
+ext = (jit.os == "OSX" and "dylib" or "so")
+
+cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext
+cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext
+cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext
+cfunc3_path = fio.pathjoin(build_path, "test/box/cfunc3.") .. ext
+cfunc4_path = fio.pathjoin(build_path, "test/box/cfunc4.") .. ext
+
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc1_path, cfunc_path)
+
+_, err = pcall(box.lib.load, 'non-such-module')
+assert(err ~= nil)
+
+-- All functions are sitting in cfunc.so.
+old_module = box.lib.load('cfunc')
+assert(old_module['debug_refs'] == 1)
+old_module_copy = box.lib.load('cfunc')
+assert(old_module['debug_refs'] == 2)
+assert(old_module_copy['debug_refs'] == 2)
+old_module_copy:unload()
+assert(old_module['debug_refs'] == 1)
+old_cfunc_nop = old_module:load('cfunc_nop')
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_sum = old_module:load('cfunc_sum')
+assert(old_module['debug_refs'] == 6)
+
+-- Test for error on nonexisting function.
+_, err = pcall(old_module.load, old_module, 'no-such-func')
+assert(err ~= nil)
+
+-- Make sure they all are callable.
+old_cfunc_nop()
+old_cfunc_fetch_seq_evens()
+old_cfunc_multireturn()
+old_cfunc_args()
+old_cfunc_sum()
+
+-- Unload the module but keep old functions alive, so
+-- they keep reference to NOP module internally
+-- and still callable.
+old_module:unload()
+-- Test refs via function name.
+assert(old_cfunc_nop['debug_module_refs'] == 5)
+old_cfunc_nop()
+old_cfunc_fetch_seq_evens()
+old_cfunc_multireturn()
+old_cfunc_args()
+old_cfunc_sum()
+
+-- The module is unloaded I should not be able
+-- to load new shared library.
+old_module:load('cfunc')
+-- Neither I should be able to unload module twise.
+old_module:unload()
+
+-- Clean old functions.
+old_cfunc_nop:unload()
+old_cfunc_fetch_seq_evens:unload()
+old_cfunc_multireturn:unload()
+old_cfunc_args:unload()
+assert(old_cfunc_sum['debug_module_refs'] == 1)
+old_cfunc_sum:unload()
+
+-- And reload old module again.
+old_module = box.lib.load('cfunc')
+old_module_ptr = old_module['debug_ptr']
+assert(old_module['debug_refs'] == 1)
+
+-- Overwrite module with new contents.
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc2_path, cfunc_path)
+
+-- Load new module, cache should be updated.
+new_module = box.lib.load('cfunc')
+new_module_ptr = new_module['debug_ptr']
+
+-- Old and new module keep one reference with
+-- different IDs.
+assert(old_module['debug_refs'] == 1)
+assert(old_module['debug_refs'] == new_module['debug_refs'])
+assert(old_module_ptr ~= new_module_ptr)
+
+-- All functions from old module should be loadable.
+old_cfunc_nop = old_module:load('cfunc_nop')
+old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
+old_cfunc_multireturn = old_module:load('cfunc_multireturn')
+old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_sum = old_module:load('cfunc_sum')
+assert(old_cfunc_nop['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_fetch_seq_evens['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_multireturn['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_args['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_sum['debug_module_ptr'] == old_module_ptr)
+assert(old_module['debug_refs'] == 6)
+
+-- Lookup for updated symbols.
+new_cfunc_nop = new_module:load('cfunc_nop')
+new_cfunc_fetch_seq_evens = new_module:load('cfunc_fetch_seq_evens')
+new_cfunc_multireturn = new_module:load('cfunc_multireturn')
+new_cfunc_args = new_module:load('cfunc_args')
+new_cfunc_sum = new_module:load('cfunc_sum')
+assert(new_cfunc_nop['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_fetch_seq_evens['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_multireturn['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_args['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_sum['debug_module_ptr'] == new_module_ptr)
+assert(new_module['debug_refs'] == 6)
+
+-- Call old functions.
+old_cfunc_nop()
+old_cfunc_fetch_seq_evens()
+old_cfunc_multireturn()
+old_cfunc_args()
+old_cfunc_sum()
+
+-- Call new functions.
+new_cfunc_nop()
+new_cfunc_multireturn()
+new_cfunc_fetch_seq_evens({2,4,6})
+new_cfunc_fetch_seq_evens({1,2,3}) -- error, odd numbers sequence
+new_cfunc_args(1, "hello")
+new_cfunc_sum(1) -- error, one arg passed
+new_cfunc_sum(1,2)
+
+-- Cleanup old module's functions.
+old_cfunc_nop:unload()
+old_cfunc_fetch_seq_evens:unload()
+old_cfunc_multireturn:unload()
+old_cfunc_args:unload()
+old_cfunc_sum:unload()
+old_module:unload()
+
+-- Cleanup new module data.
+new_cfunc_nop:unload()
+new_cfunc_multireturn:unload()
+new_cfunc_fetch_seq_evens:unload()
+new_cfunc_args:unload()
+new_cfunc_sum:unload()
+new_module:unload()
+
+-- Cleanup the generated symlink.
+_ = pcall(fio.unlink(cfunc_path))
+
+-- Test double hashing: create function
+-- in box.schema.fun so that it should
+-- appear in box.lib hash.
+fio.symlink(cfunc3_path, cfunc_path)
+box.schema.func.create('cfunc.cfunc_add', {language = "C"})
+box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
+box.func['cfunc.cfunc_add']:call({1,2})
+
+old_module = box.lib.load('cfunc')
+assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
+old_func = old_module:load('cfunc_add')
+assert(old_module['debug_refs'] == 4) -- plus function instance
+old_func(1,2)
+
+-- Now update on disk and reload the module.
+_ = pcall(fio.unlink(cfunc_path))
+fio.symlink(cfunc4_path, cfunc_path)
+
+box.schema.func.reload("cfunc")
+box.func['cfunc.cfunc_add']:call({1,2})
+
+-- The box.lib instance should carry own
+-- references to the module and old
+-- function. And reloading must not
+-- affect old functions. Thus one for
+-- box.lib and one for box.lib function.
+assert(old_module['debug_refs'] == 2)
+old_func(1,2)
+old_func:unload()
+old_module:unload()
+
+-- Same time the reload should update
+-- low level module cache, thus two
+-- for box and box function plus one
+-- new box.lib.
+new_module = box.lib.load('cfunc')
+assert(new_module['debug_refs'] == 3)
+
+-- Box function should carry own module.
+box.func['cfunc.cfunc_add']:call({1,2})
+
+-- Cleanup.
+_ = pcall(fio.unlink(cfunc_path))
+new_module:unload()
+box.schema.func.drop('cfunc.cfunc_add')
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test
2021-04-08 18:53 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-04-11 15:43 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-11 21:56 ` Cyrill Gorcunov via Tarantool-patches
2021-04-11 22:36 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 2 replies; 25+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-11 15:43 UTC (permalink / raw)
To: Cyrill Gorcunov, tml
Thanks for the patch!
See below 4 comments, my diff in the end of the email, and on
the branch in a separate commit.
> diff --git a/test/box/lib.result b/test/box/lib.result
> new file mode 100644
> index 000000000..2986df049
> --- /dev/null
> +++ b/test/box/lib.result
> @@ -0,0 +1,527 @@
<...>
> +
> +-- Cleanup new module data.
> +new_cfunc_nop:unload()
> + | ---
> + | - true
> + | ...
> +new_cfunc_multireturn:unload()
> + | ---
> + | - true
> + | ...
> +new_cfunc_fetch_seq_evens:unload()
> + | ---
> + | - true
> + | ...
> +new_cfunc_args:unload()
> + | ---
> + | - true
> + | ...
> +new_cfunc_sum:unload()
> + | ---
> + | - true
> + | ...
> +new_module:unload()
> + | ---
> + | - true
> + | ...
> +
> +-- Cleanup the generated symlink.
> +_ = pcall(fio.unlink(cfunc_path))
> + | ---
> + | ...
> +
> +-- Test double hashing: create function
> +-- in box.schema.fun so that it should
> +-- appear in box.lib hash.
> +fio.symlink(cfunc3_path, cfunc_path)
> + | ---
> + | - true
> + | ...
> +box.schema.func.create('cfunc.cfunc_add', {language = "C"})
> + | ---
> + | ...
> +box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
> + | ---
> + | ...
> +box.func['cfunc.cfunc_add']:call({1,2})
> + | ---
> + | - 3
> + | ...
> +
> +old_module = box.lib.load('cfunc')
> + | ---
> + | ...
> +assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
1. Why +2? There is only one function loaded from that module in
box.schema.func.
> + | ---
> + | - true
> + | ...
> +old_func = old_module:load('cfunc_add')
> + | ---
> + | ...
> +assert(old_module['debug_refs'] == 4) -- plus function instance
> + | ---
> + | - true
> + | ...
> +old_func(1,2)
> + | ---
> + | - 3
> + | ...
> +
> +-- Now update on disk and reload the module.
> +_ = pcall(fio.unlink(cfunc_path))
> + | ---
> + | ...
> +fio.symlink(cfunc4_path, cfunc_path)
> + | ---
> + | - true
> + | ...
> +
> +box.schema.func.reload("cfunc")
> + | ---
> + | ...
> +box.func['cfunc.cfunc_add']:call({1,2})
> + | ---
> + | - 13
> + | ...
> +
> +-- The box.lib instance should carry own
> +-- references to the module and old
> +-- function. And reloading must not
> +-- affect old functions. Thus one for
> +-- box.lib and one for box.lib function.
2. What do you mean "one for box.lib"?
> +assert(old_module['debug_refs'] == 2)
> + | ---
> + | - true
> + | ...
> +old_func(1,2)
> + | ---
> + | - 3
> + | ...
> +old_func:unload()
> + | ---
> + | - true
> + | ...
> +old_module:unload()
> + | ---
> + | - true
> + | ...
> +
> +-- Same time the reload should update
> +-- low level module cache, thus two
> +-- for box and box function plus one
3. The same question. What is "for box"?
What exactly keeps these references?
> +-- new box.lib.
> +new_module = box.lib.load('cfunc')
> + | ---
> + | ...
> +assert(new_module['debug_refs'] == 3)
> + | ---
> + | - true
> + | ...
> +
4. In the previous review I asked you to add tests
for automatic unload via GC. Please, add them.
You can use collectgarbage('collect') functions,
and setmetatable(..., {__mode = 'v'}) if it will
help with anything.
See below my review fixes and on the branch in a
separate commit.
====================
diff --git a/test/box/cfunc1.c b/test/box/cfunc1.c
index f6829372a..2ab0e8d78 100644
--- a/test/box/cfunc1.c
+++ b/test/box/cfunc1.c
@@ -1,7 +1,3 @@
-#include <stdio.h>
-#include <stdbool.h>
-#include <msgpuck.h>
-
#include "module.h"
/*
@@ -15,44 +11,17 @@
int
cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- (void)ctx;
- (void)args;
- (void)args_end;
- return 0;
-}
-
-int
-cfunc_fetch_seq_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
-{
- (void)ctx;
- (void)args;
- (void)args_end;
- return 0;
-}
-
-int
-cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
-{
- (void)ctx;
- (void)args;
- (void)args_end;
return 0;
}
int
-cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+cfunc_echo(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- (void)ctx;
- (void)args;
- (void)args_end;
return 0;
}
int
cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- (void)ctx;
- (void)args;
- (void)args_end;
return 0;
}
diff --git a/test/box/cfunc2.c b/test/box/cfunc2.c
index 8c583e993..6de40b866 100644
--- a/test/box/cfunc2.c
+++ b/test/box/cfunc2.c
@@ -1,7 +1,6 @@
-#include <stdio.h>
-#include <stdbool.h>
-#include <msgpuck.h>
====================
As I said earlier, for non-system headers we use "", not <>.
====================
+#include <stdlib.h>
+#include "msgpuck.h"
#include "module.h"
/*
@@ -10,9 +9,6 @@
int
cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- (void)ctx;
- (void)args;
- (void)args_end;
return 0;
}
@@ -21,99 +17,22 @@ cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
* arguments is not screwed).
*/
int
-cfunc_fetch_seq_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
-{
====================
After more thinkning I still couldn't find a reason why this
function has to be so complex. Why not solve an integral equation
then? Or calculcate Fibonachi numbers? For your purpose a simple
'echo' function would solve both the arguments sanity check, and
the miltireturn check.
====================
- int arg_count = mp_decode_array(&args);
- if (arg_count != 1) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
- "invalid argument count");
- }
- int field_count = mp_decode_array(&args);
- if (field_count < 1) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
- "invalid array size");
- }
-
- /*
- * We expect even numbers sequence here. The idea is
- * to test invalid data an issue an error from inside
- * of C function.
- */
- for (int i = 1; i <= field_count; i++) {
- int val = mp_decode_uint(&args);
- int needed = 2 * i;
- if (val != needed) {
- char res[128];
- snprintf(res, sizeof(res), "%s %d != %d",
- "invalid argument", val, needed);
- return box_error_set(__FILE__, __LINE__,
- ER_PROC_C, "%s", res);
- }
- }
-
- return 0;
-}
-
-/*
- * Return one element array twice.
- */
-int
-cfunc_multireturn(box_function_ctx_t *ctx, const char *args, const char *args_end)
-{
- char tuple_buf[512];
- char *d = tuple_buf;
- d = mp_encode_array(d, 1);
- d = mp_encode_uint(d, 1);
- assert(d <= tuple_buf + sizeof(tuple_buf));
-
- box_tuple_format_t *fmt = box_tuple_format_default();
- box_tuple_t *tuple_a = box_tuple_new(fmt, tuple_buf, d);
- if (tuple_a == NULL)
- return -1;
- int rc = box_return_tuple(ctx, tuple_a);
- if (rc == 0)
- return box_return_tuple(ctx, tuple_a);
- return rc;
-}
-
-/*
- * Encode int + string pair back.
- */
-int
-cfunc_args(box_function_ctx_t *ctx, const char *args, const char *args_end)
+cfunc_echo(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
+ const char *pos = args;
+ if (mp_check(&pos, args_end) != 0)
+ abort();
+ if (mp_typeof(*args) != MP_ARRAY)
+ abort();
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");
+ for (uint32_t i = 0; i < arg_count; ++i) {
+ pos = args;
+ mp_next(&pos);
+ if (box_return_mp(ctx, args, pos) != 0)
+ return -1;
+ args = pos;
}
-
- 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);
+ return 0;
}
/*
diff --git a/test/box/cfunc3.c b/test/box/cfunc3.c
index 668790fbf..71fe2717a 100644
--- a/test/box/cfunc3.c
+++ b/test/box/cfunc3.c
@@ -1,7 +1,4 @@
-#include <stdio.h>
-#include <stdbool.h>
-#include <msgpuck.h>
-
+#include "msgpuck.h"
#include "module.h"
/*
diff --git a/test/box/cfunc4.c b/test/box/cfunc4.c
index cc079b335..a03a23026 100644
--- a/test/box/cfunc4.c
+++ b/test/box/cfunc4.c
@@ -1,7 +1,4 @@
-#include <stdio.h>
-#include <stdbool.h>
#include <msgpuck.h>
-
#include "module.h"
/*
diff --git a/test/box/lib.result b/test/box/lib.result
index 2986df049..58acb9c06 100644
--- a/test/box/lib.result
+++ b/test/box/lib.result
@@ -82,19 +82,13 @@ assert(old_module['debug_refs'] == 1)
old_cfunc_nop = old_module:load('cfunc_nop')
| ---
| ...
-old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
- | ---
- | ...
-old_cfunc_multireturn = old_module:load('cfunc_multireturn')
- | ---
- | ...
-old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_echo = old_module:load('cfunc_echo')
| ---
| ...
old_cfunc_sum = old_module:load('cfunc_sum')
| ---
| ...
-assert(old_module['debug_refs'] == 6)
+assert(old_module['debug_refs'] == 4)
| ---
| - true
| ...
@@ -112,13 +106,7 @@ assert(err ~= nil)
old_cfunc_nop()
| ---
| ...
-old_cfunc_fetch_seq_evens()
- | ---
- | ...
-old_cfunc_multireturn()
- | ---
- | ...
-old_cfunc_args()
+old_cfunc_echo()
| ---
| ...
old_cfunc_sum()
@@ -133,20 +121,14 @@ old_module:unload()
| - true
| ...
-- Test refs via function name.
-assert(old_cfunc_nop['debug_module_refs'] == 5)
+assert(old_cfunc_nop['debug_module_refs'] == 3)
| ---
| - true
| ...
old_cfunc_nop()
| ---
| ...
-old_cfunc_fetch_seq_evens()
- | ---
- | ...
-old_cfunc_multireturn()
- | ---
- | ...
-old_cfunc_args()
+old_cfunc_echo()
| ---
| ...
old_cfunc_sum()
@@ -170,15 +152,7 @@ old_cfunc_nop:unload()
| ---
| - true
| ...
-old_cfunc_fetch_seq_evens:unload()
- | ---
- | - true
- | ...
-old_cfunc_multireturn:unload()
- | ---
- | - true
- | ...
-old_cfunc_args:unload()
+old_cfunc_echo:unload()
| ---
| - true
| ...
@@ -239,13 +213,7 @@ assert(old_module_ptr ~= new_module_ptr)
old_cfunc_nop = old_module:load('cfunc_nop')
| ---
| ...
-old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
- | ---
- | ...
-old_cfunc_multireturn = old_module:load('cfunc_multireturn')
- | ---
- | ...
-old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_echo = old_module:load('cfunc_echo')
| ---
| ...
old_cfunc_sum = old_module:load('cfunc_sum')
@@ -255,15 +223,7 @@ assert(old_cfunc_nop['debug_module_ptr'] == old_module_ptr)
| ---
| - true
| ...
-assert(old_cfunc_fetch_seq_evens['debug_module_ptr'] == old_module_ptr)
- | ---
- | - true
- | ...
-assert(old_cfunc_multireturn['debug_module_ptr'] == old_module_ptr)
- | ---
- | - true
- | ...
-assert(old_cfunc_args['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_echo['debug_module_ptr'] == old_module_ptr)
| ---
| - true
| ...
@@ -271,7 +231,7 @@ assert(old_cfunc_sum['debug_module_ptr'] == old_module_ptr)
| ---
| - true
| ...
-assert(old_module['debug_refs'] == 6)
+assert(old_module['debug_refs'] == 4)
| ---
| - true
| ...
@@ -280,13 +240,7 @@ assert(old_module['debug_refs'] == 6)
new_cfunc_nop = new_module:load('cfunc_nop')
| ---
| ...
-new_cfunc_fetch_seq_evens = new_module:load('cfunc_fetch_seq_evens')
- | ---
- | ...
-new_cfunc_multireturn = new_module:load('cfunc_multireturn')
- | ---
- | ...
-new_cfunc_args = new_module:load('cfunc_args')
+new_cfunc_echo = new_module:load('cfunc_echo')
| ---
| ...
new_cfunc_sum = new_module:load('cfunc_sum')
@@ -296,15 +250,7 @@ assert(new_cfunc_nop['debug_module_ptr'] == new_module_ptr)
| ---
| - true
| ...
-assert(new_cfunc_fetch_seq_evens['debug_module_ptr'] == new_module_ptr)
- | ---
- | - true
- | ...
-assert(new_cfunc_multireturn['debug_module_ptr'] == new_module_ptr)
- | ---
- | - true
- | ...
-assert(new_cfunc_args['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_echo['debug_module_ptr'] == new_module_ptr)
| ---
| - true
| ...
@@ -312,7 +258,7 @@ assert(new_cfunc_sum['debug_module_ptr'] == new_module_ptr)
| ---
| - true
| ...
-assert(new_module['debug_refs'] == 6)
+assert(new_module['debug_refs'] == 4)
| ---
| - true
| ...
@@ -321,13 +267,7 @@ assert(new_module['debug_refs'] == 6)
old_cfunc_nop()
| ---
| ...
-old_cfunc_fetch_seq_evens()
- | ---
- | ...
-old_cfunc_multireturn()
- | ---
- | ...
-old_cfunc_args()
+old_cfunc_echo()
| ---
| ...
old_cfunc_sum()
@@ -338,27 +278,21 @@ old_cfunc_sum()
new_cfunc_nop()
| ---
| ...
-new_cfunc_multireturn()
+new_cfunc_echo({1, 2, 3})
| ---
- | - [1]
- | - [1]
+ | - [1, 2, 3]
| ...
-new_cfunc_fetch_seq_evens({2,4,6})
+new_cfunc_echo(1, 2, 3)
| ---
- | ...
-new_cfunc_fetch_seq_evens({1,2,3}) -- error, odd numbers sequence
- | ---
- | - error: invalid argument 1 != 2
- | ...
-new_cfunc_args(1, "hello")
- | ---
- | - [1, 'hello']
+ | - 1
+ | - 2
+ | - 3
| ...
new_cfunc_sum(1) -- error, one arg passed
| ---
| - error: invalid argument count
| ...
-new_cfunc_sum(1,2)
+new_cfunc_sum(1, 2)
| ---
| - 3
| ...
@@ -368,15 +302,7 @@ old_cfunc_nop:unload()
| ---
| - true
| ...
-old_cfunc_fetch_seq_evens:unload()
- | ---
- | - true
- | ...
-old_cfunc_multireturn:unload()
- | ---
- | - true
- | ...
-old_cfunc_args:unload()
+old_cfunc_echo:unload()
| ---
| - true
| ...
@@ -394,15 +320,7 @@ new_cfunc_nop:unload()
| ---
| - true
| ...
-new_cfunc_multireturn:unload()
- | ---
- | - true
- | ...
-new_cfunc_fetch_seq_evens:unload()
- | ---
- | - true
- | ...
-new_cfunc_args:unload()
+new_cfunc_echo:unload()
| ---
| - true
| ...
@@ -433,7 +351,7 @@ box.schema.func.create('cfunc.cfunc_add', {language = "C"})
box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
| ---
| ...
-box.func['cfunc.cfunc_add']:call({1,2})
+box.func['cfunc.cfunc_add']:call({1, 2})
| ---
| - 3
| ...
@@ -452,7 +370,7 @@ assert(old_module['debug_refs'] == 4) -- plus function instance
| ---
| - true
| ...
-old_func(1,2)
+old_func(1, 2)
| ---
| - 3
| ...
@@ -469,7 +387,7 @@ fio.symlink(cfunc4_path, cfunc_path)
box.schema.func.reload("cfunc")
| ---
| ...
-box.func['cfunc.cfunc_add']:call({1,2})
+box.func['cfunc.cfunc_add']:call({1, 2})
| ---
| - 13
| ...
@@ -483,7 +401,7 @@ assert(old_module['debug_refs'] == 2)
| ---
| - true
| ...
-old_func(1,2)
+old_func(1, 2)
| ---
| - 3
| ...
@@ -509,7 +427,7 @@ assert(new_module['debug_refs'] == 3)
| ...
-- Box function should carry own module.
-box.func['cfunc.cfunc_add']:call({1,2})
+box.func['cfunc.cfunc_add']:call({1, 2})
| ---
| - 13
| ...
diff --git a/test/box/lib.test.lua b/test/box/lib.test.lua
index 387c813c4..e0e4b9e4d 100644
--- a/test/box/lib.test.lua
+++ b/test/box/lib.test.lua
@@ -32,11 +32,9 @@ assert(old_module_copy['debug_refs'] == 2)
old_module_copy:unload()
assert(old_module['debug_refs'] == 1)
old_cfunc_nop = old_module:load('cfunc_nop')
-old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
-old_cfunc_multireturn = old_module:load('cfunc_multireturn')
-old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_echo = old_module:load('cfunc_echo')
old_cfunc_sum = old_module:load('cfunc_sum')
-assert(old_module['debug_refs'] == 6)
+assert(old_module['debug_refs'] == 4)
-- Test for error on nonexisting function.
_, err = pcall(old_module.load, old_module, 'no-such-func')
@@ -44,9 +42,7 @@ assert(err ~= nil)
-- Make sure they all are callable.
old_cfunc_nop()
-old_cfunc_fetch_seq_evens()
-old_cfunc_multireturn()
-old_cfunc_args()
+old_cfunc_echo()
old_cfunc_sum()
-- Unload the module but keep old functions alive, so
@@ -54,11 +50,9 @@ old_cfunc_sum()
-- and still callable.
old_module:unload()
-- Test refs via function name.
-assert(old_cfunc_nop['debug_module_refs'] == 5)
+assert(old_cfunc_nop['debug_module_refs'] == 3)
old_cfunc_nop()
-old_cfunc_fetch_seq_evens()
-old_cfunc_multireturn()
-old_cfunc_args()
+old_cfunc_echo()
old_cfunc_sum()
-- The module is unloaded I should not be able
@@ -69,9 +63,7 @@ old_module:unload()
-- Clean old functions.
old_cfunc_nop:unload()
-old_cfunc_fetch_seq_evens:unload()
-old_cfunc_multireturn:unload()
-old_cfunc_args:unload()
+old_cfunc_echo:unload()
assert(old_cfunc_sum['debug_module_refs'] == 1)
old_cfunc_sum:unload()
@@ -96,59 +88,43 @@ assert(old_module_ptr ~= new_module_ptr)
-- All functions from old module should be loadable.
old_cfunc_nop = old_module:load('cfunc_nop')
-old_cfunc_fetch_seq_evens = old_module:load('cfunc_fetch_seq_evens')
-old_cfunc_multireturn = old_module:load('cfunc_multireturn')
-old_cfunc_args = old_module:load('cfunc_args')
+old_cfunc_echo = old_module:load('cfunc_echo')
old_cfunc_sum = old_module:load('cfunc_sum')
assert(old_cfunc_nop['debug_module_ptr'] == old_module_ptr)
-assert(old_cfunc_fetch_seq_evens['debug_module_ptr'] == old_module_ptr)
-assert(old_cfunc_multireturn['debug_module_ptr'] == old_module_ptr)
-assert(old_cfunc_args['debug_module_ptr'] == old_module_ptr)
+assert(old_cfunc_echo['debug_module_ptr'] == old_module_ptr)
assert(old_cfunc_sum['debug_module_ptr'] == old_module_ptr)
-assert(old_module['debug_refs'] == 6)
+assert(old_module['debug_refs'] == 4)
-- Lookup for updated symbols.
new_cfunc_nop = new_module:load('cfunc_nop')
-new_cfunc_fetch_seq_evens = new_module:load('cfunc_fetch_seq_evens')
-new_cfunc_multireturn = new_module:load('cfunc_multireturn')
-new_cfunc_args = new_module:load('cfunc_args')
+new_cfunc_echo = new_module:load('cfunc_echo')
new_cfunc_sum = new_module:load('cfunc_sum')
assert(new_cfunc_nop['debug_module_ptr'] == new_module_ptr)
-assert(new_cfunc_fetch_seq_evens['debug_module_ptr'] == new_module_ptr)
-assert(new_cfunc_multireturn['debug_module_ptr'] == new_module_ptr)
-assert(new_cfunc_args['debug_module_ptr'] == new_module_ptr)
+assert(new_cfunc_echo['debug_module_ptr'] == new_module_ptr)
assert(new_cfunc_sum['debug_module_ptr'] == new_module_ptr)
-assert(new_module['debug_refs'] == 6)
+assert(new_module['debug_refs'] == 4)
-- Call old functions.
old_cfunc_nop()
-old_cfunc_fetch_seq_evens()
-old_cfunc_multireturn()
-old_cfunc_args()
+old_cfunc_echo()
old_cfunc_sum()
-- Call new functions.
new_cfunc_nop()
-new_cfunc_multireturn()
-new_cfunc_fetch_seq_evens({2,4,6})
-new_cfunc_fetch_seq_evens({1,2,3}) -- error, odd numbers sequence
-new_cfunc_args(1, "hello")
+new_cfunc_echo({1, 2, 3})
+new_cfunc_echo(1, 2, 3)
new_cfunc_sum(1) -- error, one arg passed
-new_cfunc_sum(1,2)
+new_cfunc_sum(1, 2)
-- Cleanup old module's functions.
old_cfunc_nop:unload()
-old_cfunc_fetch_seq_evens:unload()
-old_cfunc_multireturn:unload()
-old_cfunc_args:unload()
+old_cfunc_echo:unload()
old_cfunc_sum:unload()
old_module:unload()
-- Cleanup new module data.
new_cfunc_nop:unload()
-new_cfunc_multireturn:unload()
-new_cfunc_fetch_seq_evens:unload()
-new_cfunc_args:unload()
+new_cfunc_echo:unload()
new_cfunc_sum:unload()
new_module:unload()
@@ -161,20 +137,20 @@ _ = pcall(fio.unlink(cfunc_path))
fio.symlink(cfunc3_path, cfunc_path)
box.schema.func.create('cfunc.cfunc_add', {language = "C"})
box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
-box.func['cfunc.cfunc_add']:call({1,2})
+box.func['cfunc.cfunc_add']:call({1, 2})
old_module = box.lib.load('cfunc')
assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
old_func = old_module:load('cfunc_add')
assert(old_module['debug_refs'] == 4) -- plus function instance
-old_func(1,2)
+old_func(1, 2)
-- Now update on disk and reload the module.
_ = pcall(fio.unlink(cfunc_path))
fio.symlink(cfunc4_path, cfunc_path)
box.schema.func.reload("cfunc")
-box.func['cfunc.cfunc_add']:call({1,2})
+box.func['cfunc.cfunc_add']:call({1, 2})
-- The box.lib instance should carry own
-- references to the module and old
@@ -182,7 +158,7 @@ box.func['cfunc.cfunc_add']:call({1,2})
-- affect old functions. Thus one for
-- box.lib and one for box.lib function.
assert(old_module['debug_refs'] == 2)
-old_func(1,2)
+old_func(1, 2)
old_func:unload()
old_module:unload()
@@ -194,7 +170,7 @@ new_module = box.lib.load('cfunc')
assert(new_module['debug_refs'] == 3)
-- Box function should carry own module.
-box.func['cfunc.cfunc_add']:call({1,2})
+box.func['cfunc.cfunc_add']:call({1, 2})
-- Cleanup.
_ = pcall(fio.unlink(cfunc_path))
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test
2021-04-11 15:43 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-11 21:56 ` Cyrill Gorcunov via Tarantool-patches
2021-04-11 22:36 ` Cyrill Gorcunov via Tarantool-patches
1 sibling, 0 replies; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-11 21:56 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tml
On Sun, Apr 11, 2021 at 05:43:02PM +0200, Vladislav Shpilevoy wrote:
> > + | ---
> > + | ...
> > +assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
>
> 1. Why +2? There is only one function loaded from that module in
> box.schema.func.
Since schema.func loads module implicitly: for first load it increments
not only own refs but low level module as well, then one more ref comes
from function creation and finally one from new box.lib interface.
IOW
box.schema.func.create('cfunc.cfunc_add', {language = "C"})
box.func['cfunc.cfunc_add']:call({1,2})
-->
func_c_call
func_c_load
schema_module_load
schema_do_module_load
module_load
module_new
module_ref(m); // ref = 1
func_c_load_from
module_func_load
module_ref(m) // ref = 2
old_module = box.lib.load('cfunc')
-->
lbox_module_load
module_load
cache_find
module_ref(m); // ref = 3
Now when we unload function from box.schema.func
func_c_unload
schema_module_unref
schema_module_delete
module_unload // ref = 2
module_func_unload
module_unref // ref = 1
So it only kept by `old_module`.
> > +-- The box.lib instance should carry own
> > +-- references to the module and old
> > +-- function. And reloading must not
> > +-- affect old functions. Thus one for
> > +-- box.lib and one for box.lib function.
>
> 2. What do you mean "one for box.lib"?
Typo, one for box.schema.func and one for box.lib.
> > +
> > +-- Same time the reload should update
> > +-- low level module cache, thus two
> > +-- for box and box function plus one
>
> 3. The same question. What is "for box"?
> What exactly keeps these references?
Two entries for box.schema.func and one
for box.lib. I'll extend the comments.
>
> > +-- new box.lib.
> > +new_module = box.lib.load('cfunc')
> > + | ---
> > + | ...
> > +assert(new_module['debug_refs'] == 3)
> > + | ---
> > + | - true
> > + | ...
> > +
>
> 4. In the previous review I asked you to add tests
> for automatic unload via GC. Please, add them.
>
> You can use collectgarbage('collect') functions,
> and setmetatable(..., {__mode = 'v'}) if it will
> help with anything.
Will try, thanks, Vlad!
>
> See below my review fixes and on the branch in a
> separate commit.
Thanks, I'll merge them and send a final diff.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test
2021-04-11 15:43 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-11 21:56 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-04-11 22:36 ` Cyrill Gorcunov via Tarantool-patches
2021-04-12 22:08 ` Vladislav Shpilevoy via Tarantool-patches
1 sibling, 1 reply; 25+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-11 22:36 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tml
On Sun, Apr 11, 2021 at 05:43:02PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See below 4 comments, my diff in the end of the email, and on
> the branch in a separate commit.
Vlad, here is a diff on top. Please take a look. I didn't push
out anything yet, because need to squash them first.
---
Subject: [PATCH] merged test
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
test/box/lib.result | 87 ++++++++++++++++++++++++++++++++++++-------
test/box/lib.test.lua | 49 ++++++++++++++++++------
2 files changed, 110 insertions(+), 26 deletions(-)
diff --git a/test/box/lib.result b/test/box/lib.result
index 58acb9c06..5067e9b36 100644
--- a/test/box/lib.result
+++ b/test/box/lib.result
@@ -356,10 +356,19 @@ box.func['cfunc.cfunc_add']:call({1, 2})
| - 3
| ...
+-- Now we have 2 refs for low level module (when function
+-- is loaded for first time from box.schema.func it takes
+-- two refs: one for module itself and one for for the function,
+-- this is because there is no explicit "load" procedure for
+-- box.schema.func, next loads from the same module via box.schema.func
+-- interface grabs the module from cache and add only one reference).
+--
+-- So we have 2 refs for box.schema.func and one for box.lib
+-- interface which grabs the same module.
old_module = box.lib.load('cfunc')
| ---
| ...
-assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
+assert(old_module['debug_refs'] == 3)
| ---
| - true
| ...
@@ -392,11 +401,12 @@ box.func['cfunc.cfunc_add']:call({1, 2})
| - 13
| ...
--- The box.lib instance should carry own
--- references to the module and old
--- function. And reloading must not
--- affect old functions. Thus one for
--- box.lib and one for box.lib function.
+-- The box.lib instance should carry own references to
+-- the module and old function. And reloading must not
+-- affect old functions. Thus one for box.lib _module_ and
+-- one for box.lib _function_. The reloaded box.schema.func
+-- will carry own two references for reloaded module and
+-- bound function.
assert(old_module['debug_refs'] == 2)
| ---
| - true
@@ -414,10 +424,9 @@ old_module:unload()
| - true
| ...
--- Same time the reload should update
--- low level module cache, thus two
--- for box and box function plus one
--- new box.lib.
+-- Same time the reload should update low level module cache,
+-- thus we load a new instance from updated cache entry which
+-- has 2 references already and thus we add one more reference.
new_module = box.lib.load('cfunc')
| ---
| ...
@@ -432,10 +441,6 @@ box.func['cfunc.cfunc_add']:call({1, 2})
| - 13
| ...
--- Cleanup.
-_ = pcall(fio.unlink(cfunc_path))
- | ---
- | ...
new_module:unload()
| ---
| - true
@@ -443,3 +448,57 @@ new_module:unload()
box.schema.func.drop('cfunc.cfunc_add')
| ---
| ...
+
+-- Now lets try to figure out if __gc works as expected.
+module1 = box.lib.load('cfunc')
+ | ---
+ | ...
+module2 = box.lib.load('cfunc')
+ | ---
+ | ...
+assert(module1['debug_module_ptr'] == module2['debug_module_ptr'])
+ | ---
+ | - true
+ | ...
+assert(module1['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+cfunc_add = module2:load('cfunc_add')
+ | ---
+ | ...
+assert(module1['debug_refs'] == 3)
+ | ---
+ | - true
+ | ...
+module2 = nil
+ | ---
+ | ...
+collectgarbage('collect')
+ | ---
+ | - 0
+ | ...
+assert(module1['debug_refs'] == 2)
+ | ---
+ | - true
+ | ...
+cfunc_add = nil
+ | ---
+ | ...
+collectgarbage('collect')
+ | ---
+ | - 0
+ | ...
+assert(module1['debug_refs'] == 1)
+ | ---
+ | - true
+ | ...
+module1:unload()
+ | ---
+ | - true
+ | ...
+
+-- Cleanup.
+_ = pcall(fio.unlink(cfunc_path))
+ | ---
+ | ...
diff --git a/test/box/lib.test.lua b/test/box/lib.test.lua
index e0e4b9e4d..93cea90b6 100644
--- a/test/box/lib.test.lua
+++ b/test/box/lib.test.lua
@@ -139,8 +139,17 @@ box.schema.func.create('cfunc.cfunc_add', {language = "C"})
box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
box.func['cfunc.cfunc_add']:call({1, 2})
+-- Now we have 2 refs for low level module (when function
+-- is loaded for first time from box.schema.func it takes
+-- two refs: one for module itself and one for for the function,
+-- this is because there is no explicit "load" procedure for
+-- box.schema.func, next loads from the same module via box.schema.func
+-- interface grabs the module from cache and add only one reference).
+--
+-- So we have 2 refs for box.schema.func and one for box.lib
+-- interface which grabs the same module.
old_module = box.lib.load('cfunc')
-assert(old_module['debug_refs'] == 3) -- box.lib + 2 box.schema.func
+assert(old_module['debug_refs'] == 3)
old_func = old_module:load('cfunc_add')
assert(old_module['debug_refs'] == 4) -- plus function instance
old_func(1, 2)
@@ -152,27 +161,43 @@ fio.symlink(cfunc4_path, cfunc_path)
box.schema.func.reload("cfunc")
box.func['cfunc.cfunc_add']:call({1, 2})
--- The box.lib instance should carry own
--- references to the module and old
--- function. And reloading must not
--- affect old functions. Thus one for
--- box.lib and one for box.lib function.
+-- The box.lib instance should carry own references to
+-- the module and old function. And reloading must not
+-- affect old functions. Thus one for box.lib _module_ and
+-- one for box.lib _function_. The reloaded box.schema.func
+-- will carry own two references for reloaded module and
+-- bound function.
assert(old_module['debug_refs'] == 2)
old_func(1, 2)
old_func:unload()
old_module:unload()
--- Same time the reload should update
--- low level module cache, thus two
--- for box and box function plus one
--- new box.lib.
+-- Same time the reload should update low level module cache,
+-- thus we load a new instance from updated cache entry which
+-- has 2 references already and thus we add one more reference.
new_module = box.lib.load('cfunc')
assert(new_module['debug_refs'] == 3)
-- Box function should carry own module.
box.func['cfunc.cfunc_add']:call({1, 2})
--- Cleanup.
-_ = pcall(fio.unlink(cfunc_path))
new_module:unload()
box.schema.func.drop('cfunc.cfunc_add')
+
+-- Now lets try to figure out if __gc works as expected.
+module1 = box.lib.load('cfunc')
+module2 = box.lib.load('cfunc')
+assert(module1['debug_module_ptr'] == module2['debug_module_ptr'])
+assert(module1['debug_refs'] == 2)
+cfunc_add = module2:load('cfunc_add')
+assert(module1['debug_refs'] == 3)
+module2 = nil
+collectgarbage('collect')
+assert(module1['debug_refs'] == 2)
+cfunc_add = nil
+collectgarbage('collect')
+assert(module1['debug_refs'] == 1)
+module1:unload()
+
+-- Cleanup.
+_ = pcall(fio.unlink(cfunc_path))
--
2.30.2
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test
2021-04-11 22:36 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-04-12 22:08 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-13 7:10 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-12 22:08 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tml
Thanks for the fixes!
Please, apply this diff:
====================
diff --git a/test/box/cfunc4.c b/test/box/cfunc4.c
index a03a23026..0cf6fd8fa 100644
--- a/test/box/cfunc4.c
+++ b/test/box/cfunc4.c
@@ -1,4 +1,4 @@
-#include <msgpuck.h>
+#include "msgpuck.h"
#include "module.h"
/*
diff --git a/test/box/lib.result b/test/box/lib.result
index 5067e9b36..2a89e78d1 100644
--- a/test/box/lib.result
+++ b/test/box/lib.result
@@ -348,9 +348,6 @@ fio.symlink(cfunc3_path, cfunc_path)
box.schema.func.create('cfunc.cfunc_add', {language = "C"})
| ---
| ...
-box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
- | ---
- | ...
box.func['cfunc.cfunc_add']:call({1, 2})
| ---
| - 3
diff --git a/test/box/lib.test.lua b/test/box/lib.test.lua
index 93cea90b6..076a43389 100644
--- a/test/box/lib.test.lua
+++ b/test/box/lib.test.lua
@@ -136,7 +136,6 @@ _ = pcall(fio.unlink(cfunc_path))
-- appear in box.lib hash.
fio.symlink(cfunc3_path, cfunc_path)
box.schema.func.create('cfunc.cfunc_add', {language = "C"})
-box.schema.user.grant('guest', 'execute', 'function', 'cfunc.cfunc_add')
box.func['cfunc.cfunc_add']:call({1, 2})
-- Now we have 2 refs for low level module (when function
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module
2021-04-08 16:41 [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches
` (5 preceding siblings ...)
2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test Cyrill Gorcunov via Tarantool-patches
@ 2021-04-13 21:53 ` Vladislav Shpilevoy via Tarantool-patches
2021-04-14 8:07 ` Kirill Yukhin via Tarantool-patches
7 siblings, 0 replies; 25+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-13 21:53 UTC (permalink / raw)
To: Cyrill Gorcunov, tml
Hi! Thanks for the patchset!
LGTM.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module
2021-04-08 16:41 [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches
` (6 preceding siblings ...)
2021-04-13 21:53 ` [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-14 8:07 ` Kirill Yukhin via Tarantool-patches
7 siblings, 0 replies; 25+ messages in thread
From: Kirill Yukhin via Tarantool-patches @ 2021-04-14 8:07 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tml, Vladislav Shpilevoy
Hello,
On 08 апр 19:41, Cyrill Gorcunov wrote:
> v1-v3 are development ones and not sent.
>
> v5 (by vlad):
> - drop exists, list methods: they are redundant
> - rename cfunc to cbox
> - when create a function make it callable Lua object
> - initialize cbox out of modules
> - fix error in passing module name for reloading
> - make api been cbox.func.[create|drop] and
> cbox.module.reload
> - fix test for OSX sake
>
> v6 (by vlad):
> - move module handling into module_cache file.
> v7:
> - development
> v8:
> - use rbtree for function instance storage, since
> i don't like the idea of unexpected rehashing of
> values in case of massive number of functions
> allocated
> - use reference counter and free function instance
> if only load/unload are coupled
> - keep a pointer to the function inside Lua object
> so we don't need to lookup on every function call.
> this force us to implement __gc method
> - use new API and update docs
> v9:
> - development
> v10:
> - use hashes for function names lookup
> - simply function loads counting
> - use luaL_register_module and luaL_register_type for
> easier methods registering
> - carry functions as userdata object
> v11:
> - development
> v12:
> - switch to new API as been discussed in
> https://lists.tarantool.org/tarantool-patches/e186c454-6765-4776-6433-f3f791ff4c27@tarantool.org/
> v13:
> - development
> v14:
> - switch to refs to carry module usage
> - drop func_name structure renaming
> - carry two hashes for backward compatibility with
> functions created via box.schema.func help
> - complete rework of cmod and most parts of
> module_cache
> - account for file statistics to invalidate
> module cache
> - new API for cmod, no more :reload, the :load
> procedure uses cache invalidation
> - update test cases
> - still there is no GC test since I didn't
> manage to deal with it
> v15:
> - report module state cached/orphan
> - update test cases
> - do not prevent functions lookup in orphan modules
> - there was an idea to use box.shema.func cache as on
> top of cmod's one, but this doesn't work because in case
> if module doesnt exist in any caches we would put it into
> into cmod's one as well but there wont be a module on
> cmod level which would clean it up later (which makes
> code a way more comple if we choose to track state
> of modules).
> v16:
> - internal
> v17:
> - drop idea of unifying box.schema.func and cmod functions
> cache, it brings more problems than solves due to too
> different context of execution;
> - make cmod self consistent, which shrink patch series
> size ~1/5 in compare with previous attempts;
> - improve tests to account internal states of modules
> and functions (tt_dev key in reports).
> v18:
> - implement pass-through cache for modules loading, for
> this sake 'struct module' uses cmod internally;
> - improve tests to cover sole cmod case and a mixture
> of box.schema.func and cmod to make sure the caches
> are not corrupted.
>
> v19:
> - move module handling into separate subsystem;
> - switch box.schema.func and cmod to use this shared
> code to eliminate code duplication;
> - update tests.
>
> v20:
> - fix potential nil dereference in schema_init while I'm in the code;
> - during working on the series found a bug in module recovery,
> fixed it in the series because my further work depends on it;
> - main module which allows to create C functions now named as box.lib;
> - structures renaming:
> - box.schema.func uses struct schema_module to carry underlied
> struct module;
> - box.lib uses struct module directly and struct box_module_func
> to carry cache of functions.
> - name unification:
> - schema_module variables are named as `mod`;
> - box.schema.func api prefixed as `schema_module_x`;
> - debug properties for module counting prefixed as `debug_x`;
> - the test remains disabled because
> - I run it locally all the time
> - better to have it in repo and enable then since patching
> test-run can take weeks, it is slow procedure
> v21:
> - rename functions and members from Vlad's comments
> - use @base name for lowlevel module and functions
> - use @module name for toplevel instances
> - rename __schema_module_load to schema_do_module_load
> - rename schema_func_c_load to func_c_load_from
> - rename box_lib.[ch] to plain lib.[ch]
> - cache free methods only clear the hash table itself
> - in cache_put make sure there is an empty slot we're touching
> - unify `const char *` usage
> - use lua_isstring instead of LUA_TSTRING test
> - optimize clearing of user data on Lua level via clear_udata
> helper
> - don't use tt_static hepers
> - in schema_module_reload restore failing function immediately
> and the proceed the remaining ones
> - move ERRINJ_DYN_MODULE_COUNT to module_cache so that
> old tests are passing
> - more detailed testing of module loading
> - make box.lib enabled by default even without box.cfg{}
>
> I pushed it and since github actions are not that fast I ran
> tests locally as well
>
> Statistics:
> * pass: 1250
> * disabled: 70
>
> issue https://github.com/tarantool/tarantool/issues/4642
> branch gorcunov/gh-4642-func-ro-21
I've checked your patch set into master.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 25+ messages in thread