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 31409469719 for ; Sat, 31 Oct 2020 03:13:21 +0300 (MSK) References: <20201014133535.224573-1-gorcunov@gmail.com> <20201014133535.224573-2-gorcunov@gmail.com> <8706dd6e-0b8d-f3f1-2344-f1b24906eecd@tarantool.org> <20201030095157.GD198833@grain> From: Vladislav Shpilevoy Message-ID: <1c716ed7-8782-2d0a-2f60-d8ec7c4e95d9@tarantool.org> Date: Sat, 31 Oct 2020 01:13:18 +0100 MIME-Version: 1.0 In-Reply-To: <20201030095157.GD198833@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v8 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 Cc: tml >>> + module_delete(module); >>> + return -1; >>> + } >>> + } >>> + >>> + mod_sym->addr = module_sym(module, name.sym); >>> + if (mod_sym->addr == NULL) >>> + return -1; >> >> 2. If the module was loaded first time here, it is not unloaded in case of >> an error in this place. > > Just like it was before the patch. The patch simply factor outs the old > code. It doesn't improve it (because, lets be honest this is a min problem > for module management -- we've to check for module symbol not at moment > of calling it but rather at the moment when we load a function). That > said the issue with module management is known and I think we need to > rework modules code more deeply, but not in this series. In the > series it remains exactly as it was. There is no even a ticket for that. And if you add it, it will go to wishlist, I can assure you. The rule is that if you see a bug, the first thing you do is ensure there is a ticket on it. This issue is definitely a bug. So you can't rely on some refactoring of something sometime in the future. If there is a bug, it must not be lost, and must be fixed. Bugs have higher priority than refactoring almost always, and there is more chances it will be fixed when filed separately from refactoring. https://github.com/tarantool/tarantool/issues/5475 >>> +/** >>> + * Callable symbol bound to a module. >>> + */ >>> +struct module_sym { >>> + /** >>> + * Anchor for module membership. >>> + */ >>> + struct rlist item; >>> + /** >>> + * For C functions, address of the function. >>> + */ >>> + box_function_f addr; >>> + /** >>> + * Each stored function keeps a handle to the >>> + * dynamic library for the C callback. >>> + */ >> >> 3. Can't parse the comment. What is the 'C callback'? >> And why is this function stored? After you extracted it >> from struct func_c, it is not related to _func space, and >> is not stored. > > It is stored in memory. Everything is stored in memory in Tarantool except Vinyl. I see the comment is just copy-pasted from the previous place in struct func_c, where it meant stored as stored in the schema. In spaces. > The C callback is the function we > call, so we keep a pointer to a module. You don't need a module pointer to call a function. It can be called by (), without a module. It is saved here to load it and unload. At unload it may delete the module if the function object was the last user of the module.