From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com [209.85.167.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 55774469719 for ; Sat, 31 Oct 2020 18:27:33 +0300 (MSK) Received: by mail-lf1-f65.google.com with SMTP id h6so11693094lfj.3 for ; Sat, 31 Oct 2020 08:27:33 -0700 (PDT) Date: Sat, 31 Oct 2020 18:27:30 +0300 From: Cyrill Gorcunov Message-ID: <20201031152730.GG198833@grain> References: <20201014133535.224573-1-gorcunov@gmail.com> <20201014133535.224573-2-gorcunov@gmail.com> <8706dd6e-0b8d-f3f1-2344-f1b24906eecd@tarantool.org> <20201030095157.GD198833@grain> <1c716ed7-8782-2d0a-2f60-d8ec7c4e95d9@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1c716ed7-8782-2d0a-2f60-d8ec7c4e95d9@tarantool.org> 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: Vladislav Shpilevoy Cc: tml On Sat, Oct 31, 2020 at 01:13:18AM +0100, Vladislav Shpilevoy wrote: > >>> + 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 I assigned this bug for myself. I see your point, and I rather tend to agree in general. I'll wix. > >> > >> 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. Seems we don't understad each other. This is a symbol we retrieve from the module and we literally "call" it then. The symbol has a name and an address, and address used as callable object. I'm not sure how else I could rephrase it.