Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 6 comments below.

On 02.04.2021 14:34, Cyrill Gorcunov via Tarantool-patches wrote:
> In commit 96938fafb an ability to hot reload of modules

1. Please, add the commit title in parentheses and quotes after the
hash value.

> 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.
> 
> Part-of #4642

2. How is it a part of 4642? It is totally unrelated. It is a
separate bug, existing before 4642, and which could exist after
4642 without this patch, and which does not block 4642 anyhow.

> 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).

3. 'module' term is used not only for .so/.dylib files, but also
for Lua modules. You need to be more specific that this is about
compiled files.

> 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
> @@ -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,

4. int -> in.

> +	 * thus restore all migrated functions back to original.
>  	 */
> 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

5. The test also passes if I just replace rlist_foreach_entry_safe with
rlist_foreach_entry_safe_reverse in the original code. Which means it
won't test anything in case we ever change how do we put the functions
to the list, or how we walk the list on reload.

I propose you to make the test harder to bypass.

> 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;
> +}
> +
> +

6. Between functions we use a single empty line. The same for the other .c
file.