Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 10 comments below.

> @TarantoolBot document
> Title: box.lib module
> 
> Overview
> ========
> 
> `box.lib` module provides a way to create, delete and execute
> `C` procedures from shared libraries. Unlike `box.schema.func`
> methods the functions created with `box.lib` help are not persistent
> and live purely in memory. Once a node get turned off they are
> vanished. An initial purpose for them is to execute them on
> nodes which are running in read-only mode.
> 
> Module functions
> ================
> 
> `require('box.lib').load(path) -> obj | error`

1. Implementation in C should allow not to call 'require()' at all. For
example, you don't call require('box.tuple') to use the tuples. The same
for lib.

However I don't know what is the current rule for the new modules - force to
call require() or not. Please, ask Mons what should we do. In case we need to
force to call require() - I have no idea how to do that TBH, since the module
is in C really, not in Lua. It is loaded as a part of the executable file
anyway.


2. Why can't I use box.lib without calling box.cfg{}? I does not depend on it
anyhow. For example, box.tuple works fine, require() on .so files works fine,
even ffi.load() works fine without box.cfg{}. What is the problem with box.lib?

> Unloads a module. Returns `true` on success, otherwise an error
> is thrown. Once the module is unloaded one can't load new
> functions from this module instance.
> 
> Possible errors:
> 
> - IllegalParams: a module is not supplied.
> - IllegalParams: a module is already unloaded.
> 
> Example:
> 
> ``` Lua
> m = require('box.lib').load('path/to/library')
> --
> -- do something with module
> --
> m:unload()
> ```
> 
> If there are functions from this module referenced somewhere
> in other places of Lua code they still can be executed because
> the module continue sitting in memory until the last reference
> to it is closed.
> 
> If the module become a target to the Lua's garbage collector
> then unload is called implicitly.
> 
> module:load(name) -> obj | error`

3. You need to add ` in the beginning too. Otherwise the document
structure would be screwed.

> ---
>  src/box/CMakeLists.txt |   1 +
>  src/box/lua/box_lib.c  | 590 +++++++++++++++++++++++++++++++++++++++++
>  src/box/lua/box_lib.h  |  25 ++

4. It is already in box/ folder, you don't need box_ prefix for
the files. For example, we don't have box_tuple.h/box_tuple.c. We
have just tuple.c and tuple.h, and so on.

>  src/box/lua/init.c     |   2 +
>  test/box/misc.result   |   1 +
>  5 files changed, 619 insertions(+)
>  create mode 100644 src/box/lua/box_lib.c
>  create mode 100644 src/box/lua/box_lib.h
> 
> diff --git a/src/box/lua/box_lib.c b/src/box/lua/box_lib.c
> new file mode 100644
> index 000000000..ce2ef8902
> --- /dev/null
> +++ b/src/box/lua/box_lib.c
> @@ -0,0 +1,590 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <lua.h>
> +
> +#include "box/error.h"
> +#include "box/port.h"
> +
> +#include "tt_static.h"
> +
> +#include "assoc.h"
> +#include "box_lib.h"
> +#include "diag.h"
> +#include "module_cache.h"
> +
> +#include "lua/utils.h"
> +
> +/**
> + * Function descriptor.
> + */
> +struct box_module_func {
> +	/** C function to call. */
> +	struct module_func mf;

5. You could call it 'base' like we normally do when inherit
structures.

> +	/** Number of references. */
> +	int64_t refs;
> +	/** Length of functon name in @a key. */
> +	size_t sym_len;
> +	/** Length of @a key. */
> +	size_t len;
> +	/** Function hash key. */
> +	char key[0];
> +};

<...>

> +/** Handle __index request for a module object. */
> +static int
> +lbox_module_index(struct lua_State *L)
> +{
> +	lua_getmetatable(L, 1);
> +	lua_pushvalue(L, 2);
> +	lua_rawget(L, -2);
> +	if (!lua_isnil(L, -1))
> +		return 1;

6. What is happening here in these 5 lines?

> +
> +	struct module *m = get_udata(L, uname_lib);
> +	if (m == NULL) {
> +		lua_pushnil(L);
> +		return 1;
> +	}

<...>

> +
> +/**
> + * Load a function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and either returns a cached function or
> + * creates a new function object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - SystemError: unable to open a module due to a system error.
> + * - ClientError: a module does not exist.
> + * - OutOfMemory: unable to allocate a module.
> + *
> + * @returns module object on success or throws an error.
> + */
> +static int
> +lbox_module_load_func(struct lua_State *L)
> +{
> +	const char *method = "function = module:load";
> +	const char fmt_noname[] = "Expects %s(\'name\') but no name passed";

7. Why do you randomly jump between const char * and const char []?
Please, use const char * to be consistent, and because it looks simpler.

> +
> +	if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) {

8. Here you use lua_isstring(), in some places you use
lua_type() == LUA_TSTRING. Please, be consistent, and choose one way.

> +		diag_set(IllegalParams, fmt_noname, method);
> +		return luaT_error(L);
> +	}
> +
> +	struct module *m = get_udata(L, uname_lib);
> +	if (m == NULL) {
> +		const char *fmt =
> +			"Expects %s(\'name\') but not module object passed";
> +		diag_set(IllegalParams, fmt, method);
> +		return luaT_error(L);
> +	}
> +
> +	size_t sym_len;
> +	const char *sym = lua_tolstring(L, 2, &sym_len);
> +
> +	if (sym_len < 1) {
> +		diag_set(IllegalParams, fmt_noname, method);
> +		return luaT_error(L);
> +	}
> +
> +	/*
> +	 * Functions are bound to a module symbols, thus
> +	 * since the hash is global it should be unique
> +	 * per module. The symbol (function name) is the
> +	 * last part of the hash key.
> +	 */
> +	const char *key = tt_sprintf("%p.%s.%s", (void *)m,
> +				     m->package, sym);

9. I wouldn't use the static buffer in non-trivial code, and
never in Lua and Lua C code. Especially after the recently
discovered bug.

Firstly, something inside box_module_func_new() might also try
to use the static buffer and would overwrite it. Secondly, deep
inside it accesses Lua to find package.* content, and that might
trigger Lua GC, which also might use the static buffer for
anything.

> +	size_t len = strlen(key);
> +
> +	struct box_module_func *cf = cache_find(key, len);
> +	if (cf == NULL) {
> +		cf = box_module_func_new(m, key, len, sym_len);
> +		if (cf == NULL)
> +			return luaT_error(L);
> +	} else {
> +		box_module_func_ref(cf);
> +	}
> +
> +	new_udata(L, uname_func, cf);
> +	return 1;
> +}
> +
> +/**
> + * Unload a function.
> + *
> + * Take a function object from the caller stack @a L and unload it.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: the function is not supplied.
> + * - IllegalParams: the function already unloaded.
> + *
> + * @returns true on success or throwns an error.
> + */
> +static int
> +lbox_func_unload(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1) {
> +		diag_set(IllegalParams, "Expects function:unload()");
> +		return luaT_error(L);
> +	}
> +
> +	struct box_module_func *cf = get_udata(L, uname_func);
> +	if (cf == NULL) {
> +		diag_set(IllegalParams, "The function is unloaded");
> +		return luaT_error(L);
> +	}
> +
> +	set_udata(L, uname_func, NULL);

10. By having get_udata and set_udata helpers you force yourself to
do double-check for the cdata on each pair of these calls. While
you could get void** first time, and simply *ptr = NULL it instead
of set_udata(NULL). Up to you, just a note.