Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module

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

Thanks for the patch!

See 10 comments below.

> @TarantoolBot document
> Title: cbox module
> 
> Overview
> ========
> 
> `cbox` module provides a way to create, delete and execute
> `C` procedures. Unlinke `box.schema.func` functionality this
> functions are not persistent in any space and live purely in
> memory, thus once node get turned off they are vanished. Same
> time such functions are allowed to be created and executed
> when a node is in read-only mode.
> 
> Module functions
> ================
> 
> `cbox.func.create(name) -> obj | nil, err`
> ------------------------------------------
> 
> Create a new function with name `name`.

1. I hope this is a draft. Because in the final version it would be
better to explain what does it mean to 'create' a function (or 'load'
in the next version, I guess). That it will load user's dynamic lib
on the first function call. It does not create anything, just loads
an existing function from a shared file.

> Possible errors:
>  - IllegalParams: function name is either not supplied
>    or not a string.
>  - IllegalParams: a function is already created.
>  - OutOfMemory: unable to allocate a function.
> 
> On success the new callable object is returned, otherwise
> `nil,error` pair.
> 
> Example:
> 
> ``` Lua
> f, err = require('cbox').create('func')
> if not f then
>     print(err)
> end
> -- call the function
> f(arg1,arg2,...)
> ```
> 
> `cbox.func.drop(name) -> true | nil, err`
> -----------------------------------------
> 
> Deletes a function with name `name`.
> 
> Possible errors:
>  - IllegalParams: function name is either not supplied
>    or not a string.
>  - IllegalParams: the function does not exist.
> 
> On success `true` is returned, otherwise `nil,error` pair.
> 
> Example:
> 
> ``` Lua
> ok, err = require('cbox').func.drop('func')
> if not ok then
>     print(err)
> end
> ```
> 
> `cbox.module.reload(name) -> true | nil, err`
> ---------------------------------------------
> 
> Reloads a module with name `name` and all functions which
> were associated for the module.

2. What happens with the currently working functions? What
happens, if there are still non-deleted refs to the old
functions? What if all refs are deleted, but not on all
functions was called unload()?

> Possible errors:
>  - IllegalParams: module name is either not supplied
>    or not a string.
>  - IllegalParams: the function does not exist.
>  - ClientError: a module with the name provided does
>    not exist.
> 
> On success `true` is returned, otherwise `nil,error` pair.
> 
> Example:
> 
> ``` Lua
> ok, err = require('cbox').module.reload('func')
> if not ok then
>     print(err)
> end
> ```

3. It seems the function invocation itself is not documented?

> diff --git a/src/box/lua/cbox.c b/src/box/lua/cbox.c
> new file mode 100644
> index 000000000..58585b44d
> --- /dev/null
> +++ b/src/box/lua/cbox.c
> @@ -0,0 +1,417 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <string.h>
> +#include <lua.h>
> +
> +#include "tt_static.h"

4. Why do you need tt_static.h? You don't use any of tt_*
functions here.

> +#include "assoc.h"
> +#include "diag.h"
> +#include "port.h"
> +#include "say.h"

5. You don't use say_* functions either.

> +
> +#include "box/error.h"
> +#include "box/func.h"
> +#include "box/call.h"
> +#include "box/port.h"
> +
> +#include "trivia/util.h"
> +#include "lua/utils.h"
> +
> +/** Function names to descriptor map. */
> +static struct mh_strnptr_t *func_hash;
> +
> +/**
> + * Function descriptor.
> + */
> +struct cbox_func {
> +	/**
> +	 * Symbol descriptor for the function in
> +	 * an associated module.
> +	 */
> +	struct module_sym mod_sym;
> +
> +	/** Function name length */
> +	size_t name_len;
> +
> +	/** Function name */
> +	char name[0];
> +};
> +
> +/**
> + * Find function descriptor by its name.
> + */
> +struct cbox_func *
> +cbox_func_find(const char *name, size_t len)

6. I assume the function should be 'static'. It is never
used out of cbox.c.

> +{
> +	if (name != NULL && len > 0) {
> +		mh_int_t e = mh_strnptr_find_inp(func_hash, name, len);
> +		if (e != mh_end(func_hash))
> +			return mh_strnptr_node(func_hash, e)->val;
> +	}
> +	return NULL;
> +}
> +
> +/**
> + * Add function descriptor to the cache.
> + */
> +static int
> +cbox_func_add(struct cbox_func *cfunc)
> +{
> +	const struct mh_strnptr_node_t nd = {
> +		.str	= cfunc->name,
> +		.len	= cfunc->name_len,
> +		.hash	= mh_strn_hash(cfunc->name, cfunc->name_len),
> +		.val	= cfunc,
> +	};
> +
> +	mh_int_t h = mh_strnptr_put(func_hash, &nd, NULL, NULL);
> +	if (h == mh_end(func_hash)) {
> +		diag_set(OutOfMemory, sizeof(nd), __func__, "h");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * Setup a detailed error description into the diagnostics area.
> + */
> +static void
> +diag_set_param_error(struct lua_State *L, int idx, const char *func_name,
> +		     const char *param, const char *exp)
> +{
> +	const char *typename = idx == 0 ?
> +		"<unknown>" : lua_typename(L, lua_type(L, idx));
> +	static const char *fmt =
> +		"%s: wrong parameter \"%s\": expected %s, got %s";

7. Please, lets omit 'static' when it is not needed. It just raises questions.

> +	diag_set(IllegalParams, fmt, func_name, param, exp, typename);
> +}
> +
> +/**
> + * Call function by its name from the Lua code.
> + */
> +static int
> +lcbox_func_call(struct lua_State *L)
> +{
> +	int nr_args = lua_gettop(L);

8. It is not really a number of arguments. It includes the function
object itself.

> +
> +	/*
> +	 * A function name is associated with the variable.
> +	 */
> +	lua_getfield(L, -nr_args, "name");
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, -1, &name_len);
> +	struct cbox_func *cf = cbox_func_find(name, name_len);
> +	/*
> +	 * Do not pop the result early, since Lua's GC may
> +	 * drop the name string found.
> +	 */
> +	lua_pop(L, 1);
> +
> +	if (cf == NULL) {
> +		/*
> +		 * This can happen if someone changed the
> +		 * name of the function manually.
> +		 */
> +		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
> +		diag_set(IllegalParams, fmt, name);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	/*
> +	 * FIXME: We should get rid of luaT_newthread but this
> +	 * requires serious modifucations. In particular

9. modifucations -> modifications.

> +	 * port_lua_do_dump uses tarantool_L reference and
> +	 * coro_ref must be valid as well.
> +	 */
> +	lua_State *args_L = luaT_newthread(tarantool_L);
> +	if (args_L == NULL)
> +		return luaT_push_nil_and_error(L);
> +
> +	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
> +	lua_xmove(L, args_L, lua_gettop(L) - 1);
> +
> +	struct port args;
> +	port_lua_create(&args, args_L);
> +	((struct port_lua *)&args)->ref = coro_ref;
> +
> +	struct port ret;
> +	if (module_sym_call(&cf->mod_sym, &args, &ret) != 0) {
> +		port_destroy(&args);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	int top = lua_gettop(L);
> +	lua_pushboolean(L, true);
> +	port_dump_lua(&ret, L, true);
> +	int cnt = lua_gettop(L) - top;
> +
> +	port_destroy(&ret);
> +	port_destroy(&args);
> +
> +	return cnt;
> +}
> +
> +/**
> + * Create a new function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and creates a new function object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - IllegalParams: a function is already created.
> + * - OutOfMemory: unable to allocate a function.
> + *
> + * @returns function object on success or {nil,error} on error,
> + * the error is set to the diagnostics area.
> + */
> +static int
> +lcbox_func_create(struct lua_State *L)
> +{
> +	const char *method = "cbox.func.create";
> +	struct cbox_func *cf = NULL;
> +
> +	if (lua_gettop(L) != 1) {
> +		const char *fmt = "Expects %s(\'name\')";
> +		diag_set(IllegalParams, fmt, method);
> +		goto out;
> +	}
> +
> +	if (lua_type(L, 1) != LUA_TSTRING) {
> +		diag_set_param_error(L, 1, method,
> +				     lua_tostring(L, 1),
> +				     "function name");
> +		goto out;
> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	if (cbox_func_find(name, name_len) != NULL) {
> +		const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS);
> +		diag_set(IllegalParams, fmt, name);
> +		goto out;
> +	}
> +
> +	size_t size = sizeof(*cf) + name_len + 1;
> +	cf = malloc(size);
> +	if (cf == NULL) {
> +		diag_set(OutOfMemory, size, "malloc", "cf");
> +		goto out;
> +	}
> +
> +	cf->mod_sym.addr	= NULL;
> +	cf->mod_sym.module	= NULL;
> +	cf->mod_sym.name	= cf->name;
> +	cf->name_len		= name_len;
> +
> +	memcpy(cf->name, name, name_len);
> +	cf->name[name_len] = '\0';
> +
> +	if (cbox_func_add(cf) != 0)
> +		goto out;
> +
> +	/*
> +	 * A new variable should be callable for
> +	 * convenient use in Lua.
> +	 */
> +	lua_newtable(L);
> +
> +	lua_pushstring(L, "name");
> +	lua_pushstring(L, cf->name);
> +	lua_settable(L, -3);
> +
> +	lua_newtable(L);
> +
> +	lua_pushstring(L, "__call");
> +	lua_pushcfunction(L, lcbox_func_call);
> +	lua_settable(L, -3);
> +
> +	lua_setmetatable(L, -2);
> +	return 1;
> +
> +out:
> +	free(cf);
> +	return luaT_push_nil_and_error(L);
> +}
> +
> +/**
> + * Drop a function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and drops a function object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - IllegalParams: the function does not exist.
> + *
> + * @returns true on success or {nil,error} on error,
> + * the error is set to the diagnostics area.
> + */
> +static int
> +lcbox_func_drop(struct lua_State *L)
> +{
> +	const char *method = "cbox.func.drop";
> +	const char *name = NULL;
> +
> +	if (lua_gettop(L) != 1) {
> +		const char *fmt = "expects %s(\'name\')";
> +		diag_set(IllegalParams, fmt, method);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	if (lua_type(L, 1) != LUA_TSTRING) {
> +		diag_set_param_error(L, 1, method,
> +				     lua_tostring(L, 1),
> +				     "function name or id");
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	size_t name_len;
> +	name = lua_tolstring(L, 1, &name_len);
> +
> +	mh_int_t e = mh_strnptr_find_inp(func_hash, name, name_len);
> +	if (e == mh_end(func_hash)) {
> +		const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION);
> +		diag_set(IllegalParams, fmt, name);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	struct cbox_func *cf = mh_strnptr_node(func_hash, e)->val;
> +	mh_strnptr_del(func_hash, e, NULL);
> +	free(cf);

10. I would advise to wrap it into cbox_func_delete(). I am 100% sure
we will add more deletion things to the function object soon.

> +
> +	lua_pushboolean(L, true);
> +	return 1;
> +}