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

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Mon, Apr 05, 2021 at 05:47:32PM +0200, Vladislav Shpilevoy wrote:
> 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.

OK

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

It *is* related. I patch this code later, when move to the new module
API interface and I'm not going to continue supporting this bug
in the new code. This was the reason why I had to patch the code
first. And that's why it is part of 4642.

I could make a separate issue for this and fix it if you prefer
but definitely in this series. I don't wanna do a double work.

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

OK

> > +	 * Some old functions are not found int the new module,
> 
> 4. int -> in.

Thanks!

> > +	 * 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.

The key moment here is not about the list traverse direction: that's
why I use for_each macro instead of the broken cycle we had before.
The order does matter to trigger the issue *without* this patch,
ie to put functions in reverse order and force the recovery
(as is done in my test, I even put a comment there).

With the new approach using list_for() it is doesn't matter now
how exactly we traverse. If that is what you mean.

> I propose you to make the test harder to bypass.

The original code restores only one last failed function and
my test is done the way to trigger the issue. I would like to
make it more harder to bypass but I don't see an other way.
> > +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.

Thanks, will do.