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 2878D469719 for ; Tue, 6 Oct 2020 00:59:07 +0300 (MSK) References: <20201001135113.329664-1-gorcunov@gmail.com> <20201001135113.329664-6-gorcunov@gmail.com> <20201005103148.GD2069@grain> From: Vladislav Shpilevoy Message-ID: Date: Mon, 5 Oct 2020 23:59:05 +0200 MIME-Version: 1.0 In-Reply-To: <20201005103148.GD2069@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4 5/6] box/func: implement cfunc Lua module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml , Alexander Turenko Hi! Thanks for the answers! On 05.10.2020 12:31, Cyrill Gorcunov wrote: > On Sat, Oct 03, 2020 at 03:55:26PM +0200, Vladislav Shpilevoy wrote: >> Thanks for the patch! >> >> See 20 comments below. >> >> On 01.10.2020 15:51, Cyrill Gorcunov wrote: >>> Currently to run "C" function from some external module >>> one have to register it first in "_func" system space. This >>> is a problem if node is in read-only mode (replica). >>> >>> Still people would like to have a way to run such functions >>> even in ro mode. For this sake we implement "cfunc" lua >>> module. >>> >>> Fixes #4692 >>> >>> Signed-off-by: Cyrill Gorcunov >>> >>> @TarantoolBot document >>> Title: cfunc module >> >> 1. The documentation will go to a github issue, where it looks super >> broken as I checked. Please, try to use github markdown here. > > You know, I've been using sphinx syntax for a purpose -- this commit > message can be simply extracted and being put into our doc.repo. Otherwise > it becomes a double work: first we write commit message in own format > then out docs team need to reformat it to sphinx. Taking into account > that we simply don't have enough men power we could optimize this procedure. > Note -- I don't mind to rewrite it in MD format but probably sphinx is better? I just stated a fact - whatever you post after "TarantoolBot document" goes to a github issue as is, unedited. Therefore it will be interpreted as github markdown, unconditionally. So you need to write things to make them readable in github. If you use sphinx markdown, it won't be displayed as sphinx markdown anyway. It will be displayed in github markdown. Talking of the idea about writing docs for copy-pasting, I don't think it can be done with the current flow of documentation updates. Doc team anyway needs to find a place to put your text into, translate it to Russian, review and correct your markdown. Also if we need to write in sphinx style, it means we need to patch docbot to somehow turn github markdown off, and make other team members learn this markdown - how to write it and validate. With all that said - why do we even need a whole documentation team, if this is basically all they do? I would better let us write more free-style documentation requests, and let the doc team do their job. >>> + >>> + cfunc = malloc(sizeof(*cfunc) + name_len + 1); >>> + if (cfunc == NULL) { >>> + diag_set(OutOfMemory, sizeof(*cfunc), "malloc", "cfunc"); >> >> 12. You allocated 'sizeof(*cfunc) + name_len + 1)', not only sizeof. > > I know, the name of the function is placed right after the structure. > It is intended. How is it related? diag_set in the second argument takes allocation size. You tried to allocate sizeof(*cfunc) + name_len + 1, but reported sizeof(*cfunc), which is smaller. It does not depend on what you allocate. Only how much. >> In that way the idea to rename cfunc to cbox or something like that looks >> even better. We could provide methods to operate on entire modules. So it >> would be >> >> f = cbox.load_func() -- Create a function. >> f:unload() -- Unload a function. >> cbox.reload_module() -- Reload module and all functions in it. >> cbox.unload_module() -- Unload the module from the memory. > > maybe > > f = cmod.create(name) -- just like in schema.func > f:drop() -- same > cmod.module('reload', name) > cmod.module('unload', name) > > ? I don't mind to use cbox name either but cmod somehow close to > what we're doing. The method name as string 'reload' and 'unload' looks super ugly, and wouldn't work with autocompletion. I would better go for explicit methods module_reload and module_unload. Or like you proposed later - module.reload and module.unload. (Personally I would prefer the first way, because it would mean less Lua tables, and simpler autocompletion.) >>> + } >>> + >>> + size_t name_len; >>> + const char *name = lua_tolstring(L, 1, &name_len); >>> + >>> + struct cfunc *cfunc = cfunc_lookup(name, name_len); >>> + if (cfunc == NULL) { >>> + const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION); >>> + diag_set(IllegalParams, fmt, name); >>> + return luaT_error(L); >>> + } >>> + >>> + lua_State *args_L = luaT_newthread(tarantool_L); >> >> 18. Wtf? Why can't you use L? Why do you even move anything here >> between the stacks? > > Well, actually it comes from lbox_func_call, I couldn't extract this > into a separate helper. To be honest I do not really undertand why > luaT_newthread is used in lbox_func_call but I presume this is the > template of calling external functions in Lua? We don't copy code presuming that it does something. Lets better understand why is it needed instead of blindly copying it. Currently I don't see any reason why is it needed. Tbh I don't know why is it needed in lbox_func_call() either. The comment there is super unclear about that. It just says what is done, and not why.