From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1B4EC44643A for ; Fri, 13 Nov 2020 01:53:42 +0300 (MSK) References: <20201105151808.456573-1-gorcunov@gmail.com> <20201105151808.456573-2-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Thu, 12 Nov 2020 23:53:40 +0100 MIME-Version: 1.0 In-Reply-To: <20201105151808.456573-2-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml 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.