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

[Cyrill Gorcunov <gorcunov@gmail.com>]

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?

> > .. _cfunc-list:
> > 
> > .. function:: list()
> > 
> >     List created functions.
> > 
> >     **Example**
> > 
> >     .. code-block:: lua
> > 
> >         require('cfunc').list()
> >         ---
> >         - - easy
> >         ...
> 
> 2. Lets make the API minimal for now, without exists(), list(). I can't
> think of why are they needed in an application. We can add more methods
> if they are obviously needed for something, or when users ask.

Ok, good point! Will do.

> > diff --git a/src/box/func.c b/src/box/func.c
> > index ba98f0f9e..032b6062c 100644
> > --- a/src/box/func.c
> > +++ b/src/box/func.c
> > @@ -34,6 +34,7 @@
> >  #include "assoc.h"
> >  #include "lua/utils.h"
> >  #include "lua/call.h"
> > +#include "lua/cfunc.h"
> 
> 3. Why do you need cfunc here? Also it looks like a cyclic
> dependency - cfunc depends on box/func and vise versa.

I fear it is leftover from previous attempts. Thanks for catching!

> > @@ -152,6 +153,14 @@ module_init(void)
> >  			  "modules hash table");
> >  		return -1;
> >  	}
> > +	/*
> > +	 * cfunc depends on module engine,
> > +	 * so initialize them together.
> > +	 */
> > +	if (cfunc_init() != 0) {
> > +		module_free();
> > +		return -1;
> > +	}
> 
> 4. Better not enclose modules. We have lots of modules depending
> on each other but still it is much more clear when they are all
> initialized in one place. So I would move it to box_init() and
> box_free().
> 
> For example, func_c depend on port, but we don't call port_init()
> in module_init(). Actually, port is needed in several modules. The
> same for tuple module, tuple format, fiber. I wouldn't consider
> cfunc as something different.

+1, thanks!

> 5. The more I look at the names, the more I think that we probably
> should rename cfunc to cmod, or cmodule, or boxmod, or cbox, or
> modbox, or another name.
> 
> Because we don't allow to load and call any function. Only module API
> functions, which have strict signature - box_function_f.
> 
> Also cfunc looks too similar to func_c, while they don't have much
> in common. func_c is a helper for _func space, and cfunc has nothing
> to do with spaces and schema.

cmod sounds great to me

> 6. Why do you need box/func.h, box/call.h, box/session.h,
> box/user.h? All these files are about schema and credentials.
> cfunc does not need any of this. If anything from them does
> not depend on schema, and you need it, then you probably
> should move these code pieces out first.

I'm *really sorry* Vlad for this "include" crap! I managed to
forget clean them up while been reworking the series from
another approach :( I'll re-check and clean them up, thanks!

> For instance, module_sym does not have intersections with
> func_c, but somewhy is stored in box/func.h and .c.
> 
> Also why do you need box/identifier.h? We don't store the
> names in any schema, so we don't need them to be valid
> identifiers. Let dlsym() decide what names are valid.

Agreed.

> 
> > +#include "say.h"
> > +#include "diag.h"
> > +#include "tt_static.h"
> > +
> > +#include "box/lua/cfunc.h"
> > +#include "box/lua/call.h"
> 
> 7. Why do you need box/lua/call.h? You are not calling Lua
> functions here.

Sigh, leftover again. Sorry and thanks!

> > +
> > +static struct mh_strnptr_t *cfunc_hash;
> > +static unsigned int nr_cfunc = 0;
> 
> 8. What is 'nr'? If it is a number of functions, then why
> couldn't you use mh_size()?

Yes, it is number of functions. Yes we can use mh_size instead.
Moreover since we're going to drop list() we don't need this
anymore, thanks!

> > +
> > +static int
> > +lbox_cfunc_create(struct lua_State *L)
> 
> 9. Better not use lbox_ prefix. Because this is not box.cfunc,
> it is just cfunc.

Sure

> 
> > +{
> > +	static const char method[] = "cfunc.create";
> 
> 10. Why so complex? And why static? Why not
> 
> 	const char *method = "cfunc.create";
> 
> like we do everywhere else?

This helps compiler to understand that the variable
allocated just once. Rather a habbit. I'll drop it.

> > +	if (lua_gettop(L) != 1) {
> > +		static const char *fmt =
> > +			"%s: expects %s(\'name\')";
> 
> 11. Lets not use 'static' when it does not change anything.
> The same for all other places where 'static' can be removed
> without any changes.

ok

> > +
> > +	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.

> 
> > +		goto out;
> > +	}
> > +
> > +	cfunc->mod_sym.addr	= NULL;
> > +	cfunc->mod_sym.module	= NULL;
> > +	cfunc->mod_sym.name	= cfunc->name;
> > +	cfunc->name_len		= name_len;
> > +
> > +	memcpy(cfunc->name, name, name_len);
> > +	cfunc->name[name_len] = '\0';

Here we copy it right after.

> > +
> > +out:
> > +	free(cfunc);
> > +	return luaT_error(L);
> 
> 13. Our agreement for the new code is that it should not throw. It
> should return nil + error. This was somewhere in the documentation.
> The same for all other functions.
> 
> Although I don't remember what is the agreement to return success.
> If you return nothing in case of success, and nil+error on an error,
> then you need to write code like this:
> 
> 	unused, err = func()
> 	if not err then
> 		-- Success
> 	else
> 		-- Error
> 	end
> 
> The first value becomes never used, in both cases. Maybe it is worth
> returning 'true', so at least I could write something like:
> 
> 	ok, err = func()
> 	if ok then
> 		-- Success
> 	else
> 		-- Error
> 	end
> 
> But I don't know the officient agreement here. In SWIM I used the
> second way, for example.

Thanks for pointing, Vlad! Will rework.

> > +
> > +	size_t name_len;
> > +	const char *name = lua_tolstring(L, 1, &name_len);
> > +
> > +	struct cfunc *cfunc = cfunc_lookup(name, name_len);
> > +	lua_pushboolean(L, cfunc != NULL ? true : false);
> 
> 14. Result of comparison is boolean anyway. No need to use '?'
> and these constants.

ok

> > +
> > +	func_split_name(name, &n);
> > +	if (module_reload(n.package, n.package_end, &module) == 0) {
> 
> 15. The API to reload a single function looks not so good. You can't
> reload one function and not reload the others anyway. Or can you?

True, only the whole module can be reloaded. I need to adjust the code.

> 
> For instance, we have box.schema.func.reload(). It takes a module
> name.

Yeah, and I have to do the same.

> 
> 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.

> 
> We need to put more thoughts into the API design.
> 
> > +		if (module != NULL)
> > +			return 0;
> > +		diag_set(ClientError, ER_NO_SUCH_MODULE, name);
> > +	}
> > +
> > +	return luaT_error(L);
> > +}
> > +
> > +static int
> > +lbox_cfunc_list(struct lua_State *L)
> > +{
> > +	if (nr_cfunc == 0)
> > +		return 0;
> 
> 16. So the function either returns nil or a table? This
> looks bad. So a user can't just write
> 
> 	for i, func in pairs(cfunc.list()) do
> 		...
> 	end
> 
> In your case he needs to check for nil. Does not look
> like a good idea. Maybe worth returning an empty table.
> Or just drop this function, because I don't know why
> would a user need it. In an application you anyway know
> all the functions you use.

Agreed. Lets drop it for a while. If users really would need
for listing we will implement on demand.

> > +static int
> > +lbox_cfunc_call(struct lua_State *L)
> > +{
> > +	static const char method[] = "cfunc.call";
> > +	if (lua_gettop(L) < 1 || !lua_isstring(L, 1)) {
> > +		static const char *fmt =
> > +			"%s: expects %s(\'name\')";
> > +		diag_set(IllegalParams, fmt, method, method);
> 
> 17. I suppose you forgot a return here. And also to cover it
> with a test.

True, thanks!

> 
> > +	}
> > +
> > +	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?

> > +++ b/src/box/lua/cfunc.h
> > @@ -0,0 +1,55 @@
> > +#ifndef INCLUDES_TARANTOOL_MOD_BOX_LUA_CFUNC_H
> > +#define INCLUDES_TARANTOOL_MOD_BOX_LUA_CFUNC_H
> 
> 19. In the new code we agreed to use #pragma once.

ok

> > +struct lua_State;
> > +
> > +void
> > +box_lua_cfunc_init(struct lua_State *L);
> > +
> > +int
> > +cfunc_init(void);
> 
> 20. Why do you need 2 inits? All Lua modules always
> have a single init and a single free.

box_lua_cfunc_init needed to register the module inside
Lua machine while cfunc_init allocates internal structures
for cfunc functionaliy (such as hash).

And no -- the lua modules might require additional initialization
as well. See popen for example: it has popen_init and not that
obvious but module initalization from popen.lua module as well.

Since we gonna change API this will be eliminated in cmod.

Thanks huge for review, Vlad!