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

[Cyrill Gorcunov]

On Tue, Apr 06, 2021 at 10:02:40PM +0200, Vladislav Shpilevoy wrote:
> >> 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.
> 
> Nope, that is not what I mean. But I don't know how to say it
> easier, even in Russian.

OK, lets go another way. Say we have 3 functions: {1,2,3}.
When we call them in order 1, then 2, then 3 the list of
functions gonna be {3, 2, 1} because they are prepended.

Then we start reloading procedure (we're talking aboout original
code). And we have old = {3, 2, 1}, new = { } and we processed 3
and 2 witout an error, so we have old = { 1 }, new = {3, 2}. When
we tried to resolve function 1 lets assume it has failed, we jump
into restore procedure and restore a sole function 1, which lead
to situation where old = { 1 } and function 3 and 2 are simply
dissapeared from old module. This is because of _when_ we have failed.
If we fail at first element, ie when resolving function {3}, then restore
will pass fine and we will have old = {3, 2, 1} without problem.

That said current restore procedure can handle only one failing
entry andif there were other entries before failing one they will
be simply vanished. That's what I've been trying to say.

After the patch is doesn't matter in which order we resolve symbols
because we resolve every element migrated from old list.

Since current code restores only one entry we have a 3 cases: 1) entry
failing at head of the list, 2) entry failing at the middle of the list
and 3) entry failing at the last element. (1) is restored properly
even in current code while (2) and (3) causes a crash and (2) is
rather a subset of (3). So I tested (3) only.

> >> 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.
> 
> Well, did you try? The test would fail regardless of the order, if
> you would try to drop the middle function. Or both the first and
> the last functions. Or try all the combinations.

Nope. See above. Case (1) handled without problem. But no problem
I'll extend the test.