[Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore
Serge Petrenko
sergepetrenko at tarantool.org
Mon Apr 5 13:53:07 MSK 2021
Thanks for the patch! Please find 3 minor comments below.
02.04.2021 15:34, Cyrill Gorcunov пишет:
> In commit 96938fafb an ability to hot reload of modules
> has been introduced. When module is been reloaded his
typo: either 'is being reloaded' or 'has been reloaded'.
> 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.
>
> Part-of #4642
>
> Signed-off-by: Cyrill Gorcunov <gorcunov at gmail.com>
> ---
> changelogs/unreleased/fix-module-reload.md | 4 +
> src/box/func.c | 13 ++-
> test/box/CMakeLists.txt | 2 +
> test/box/func_restore.result | 103 +++++++++++++++++++++
> test/box/func_restore.test.lua | 43 +++++++++
> test/box/func_restore1.c | 34 +++++++
> test/box/func_restore2.c | 28 ++++++
> 7 files changed, 220 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
>
> diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md
> new file mode 100644
> index 000000000..7e189617f
> --- /dev/null
> +++ b/changelogs/unreleased/fix-module-reload.md
> @@ -0,0 +1,4 @@
> +## bugfix/core
> +
> +* Fix module reloading procedure which may crash in case if
> + new module is corrupted (gh-4642).
> diff --git a/src/box/func.c b/src/box/func.c
> index 233696a4f..1cd7073de 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 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)
> @@ -402,10 +403,10 @@ module_reload(const char *package, const char *package_end)
> return 0;
> restore:
> /*
> - * Some old-dso func can't be load from new module, restore old
> - * functions.
> + * Some old functions are not found int the new module,
> + * thus restore all migrated functions back to original.
typo: in the new module.
> */
> - 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);
> @@ -419,9 +420,7 @@ module_reload(const char *package, const char *package_end)
> }
> 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));
Let's keep this assert.
> + }
> module_delete(new_module);
> return -1;
> }
> diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
> index 06bfbbe9d..944831af2 100644
> --- a/test/box/CMakeLists.txt
> +++ b/test/box/CMakeLists.txt
> @@ -2,4 +2,6 @@ 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(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..e679f4eb8
> --- /dev/null
> +++ b/test/box/func_restore.result
> @@ -0,0 +1,103 @@
> +-- test-run result file version 2
> +--
> +-- Test that 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
> + | ---
> + | ...
> +path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. 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')
> + | ---
> + | ...
> +
> +-- Order *does* matter since we bind functions in
> +-- a list where entries are added to the top.
> +box.func['func_restore.echo_3']:call()
> + | ---
> + | - 3
> + | ...
> +box.func['func_restore.echo_2']:call()
> + | ---
> + | - 2
> + | ...
> +box.func['func_restore.echo_1']:call()
> + | ---
> + | - 1
> + | ...
> +
> +_ = pcall(fio.unlink(path_func_restore))
> + | ---
> + | ...
> +fio.symlink(path_func_bad, path_func_restore)
> + | ---
> + | - true
> + | ...
> +
> +ok, _ = pcall(box.schema.func.reload, "func_restore")
> + | ---
> + | ...
> +assert(not ok)
> + | ---
> + | - true
> + | ...
> +
> +box.func['func_restore.echo_1']:call()
> + | ---
> + | - 1
> + | ...
> +box.func['func_restore.echo_2']:call()
> + | ---
> + | - 2
> + | ...
> +box.func['func_restore.echo_3']:call()
> + | ---
> + | - 3
> + | ...
> diff --git a/test/box/func_restore.test.lua b/test/box/func_restore.test.lua
> new file mode 100644
> index 000000000..8a38a6074
> --- /dev/null
> +++ b/test/box/func_restore.test.lua
> @@ -0,0 +1,43 @@
> +--
> +-- Test that 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
> +path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. 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')
> +
> +-- Order *does* matter since we bind functions in
> +-- a list where entries are added to the top.
> +box.func['func_restore.echo_3']:call()
> +box.func['func_restore.echo_2']:call()
> +box.func['func_restore.echo_1']:call()
> +
> +_ = pcall(fio.unlink(path_func_restore))
> +fio.symlink(path_func_bad, path_func_restore)
> +
> +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()
> diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c
> new file mode 100644
> index 000000000..ffd69fd2b
> --- /dev/null
> +++ b/test/box/func_restore1.c
> @@ -0,0 +1,34 @@
> +#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..0d77e78b2
> --- /dev/null
> +++ b/test/box/func_restore2.c
> @@ -0,0 +1,28 @@
> +#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);
> +}
--
Serge Petrenko
More information about the Tarantool-patches
mailing list