Re: [Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

On 05.11.2020 16:18, Cyrill Gorcunov wrote:
> Currently func_c structure is a wrapper over struct func
> which in turn handles function definition, credentials and etc,
> mostly to handle internal "_func" space.
> 
> Such tight bound doesn't allow to reuse func_c in any
> other context. But we're going to support C function
> execution in read-only instances and for this we better
> reuse already existing structures as much as possible
> instead of breeding new ones.
> 
> Thus lets extract module symbols into module_sym structure,
> this allows us to reuse module path cache in other code.
> 
> While extracting the structure rename "func" member to
> "addr" since this exactly what the member represents:
> an address of symbol in memory.
> 
> Same time to be completely independent of "_func" specific lets
> carry symbolic name inplace (ie member "name" is kind of redundant
> when module_sym is used for "_func" handling where we can retrieve
> the name via function definition but such definition won't be
> available for non-persistent C functions which we will support
> in next patches).
> 
> The new structure allows us to load/unload general symbols via
> module_sym_load() and module_sym_unload() helpers.
> 
> While been factoring out Vlad reported that we've an issue:
> if a module has been loaded for a first time but doesn't have
> symbol requested it will remain in memory keeping a reference
> to a shared library for nothing.

Please, never squash refatoring and a bugfix into a single commit.
That makes the commit not atomic, and creates problems with
backporting such commit, which is clearly needed here.

Move the bugfix to a separate commit which can be cherry-picked to
the older versions.

https://github.com/tarantool/tarantool/wiki/Code-review-procedure#general-coding-points-to-check
Point 3, "Don't do a change if it is not related to the patch".
The issues 5475 and 4642 are not related.

It is not a cargo cult rule. I just really don't see the actual fix in
all this refactoring, because too much code is moved, and only some
tiny part of it is changed.