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

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

05.04.2021 14:26, Cyrill Gorcunov пишет:
>> 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'.
> Thanks! Surely it should be "is being".
>
>>> -	 * 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.
> Thanks!
>
>>> @@ -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.
> The cycle is removing entries from the list and for this purpose
> the cycle is using a "safe" form. I would prefer to drop this
> useless assert() because a few lines above we are doing entries
> removing. Frankly I don't understand why it has been introduced
> in first place, maybe because this stange and buggy form of
> "func != rlist_first_entry", dunno.
>
> So if there is no really strong reason why should we keep this
> assert I would love to drop it.
>
> Look, here is what we have
>
> 	list_for_each_entry_safe(item, head, ...)
> 		list_move(item, new_head)
>
> a few obvious lines. And list is clear once the @item reaches @head.

Ok, drop it then. I have no serious objections.

>
> But if you still think it is better to keep and I didn't convince
> you, then sure, I'll bring it back.

-- 
Serge Petrenko