Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Cyrill Gorcunov <gorcunov@gmail.com>,
	tml <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure
Date: Thu, 12 Nov 2020 23:53:40 +0100	[thread overview]
Message-ID: <eb1ad741-3e1b-e0aa-203e-bebf1b4cf79c@tarantool.org> (raw)
In-Reply-To: <20201105151808.456573-2-gorcunov@gmail.com>

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.

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.

  reply	other threads:[~2020-11-12 22:53 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-05 15:18 [Tarantool-patches] [PATCH v10 0/4] box/cbox: implement cfunc Lua module Cyrill Gorcunov
2020-11-05 15:18 ` [Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure Cyrill Gorcunov
2020-11-12 22:53   ` Vladislav Shpilevoy [this message]
2020-11-13 17:56     ` Cyrill Gorcunov
2020-11-05 15:18 ` [Tarantool-patches] [PATCH v10 2/4] module_cache: move module handling into own subsystem Cyrill Gorcunov
2020-11-12 22:54   ` Vladislav Shpilevoy
2020-11-16  9:54     ` Cyrill Gorcunov
2020-11-16 14:41     ` Cyrill Gorcunov
2020-11-05 15:18 ` [Tarantool-patches] [PATCH v10 3/4] box/cbox: implement cbox Lua module Cyrill Gorcunov
2020-11-12 22:53   ` Vladislav Shpilevoy
2020-11-16 20:26     ` Cyrill Gorcunov
2020-11-05 15:18 ` [Tarantool-patches] [PATCH v10 4/4] test: box/cfunc -- add simple module test Cyrill Gorcunov
2020-11-12 22:53 ` [Tarantool-patches] [PATCH v10 0/4] box/cbox: implement cfunc Lua module Vladislav Shpilevoy
2020-11-13 17:54   ` Cyrill Gorcunov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=eb1ad741-3e1b-e0aa-203e-bebf1b4cf79c@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox