Re: [Tarantool-patches] [PATCH v4 5/6] box/func: implement cfunc Lua module

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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 <gorcunov@gmail.com>
>>>
>>> @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.