Re: [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module

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

Thanks for the patch!

Please, try to respond to my comments in the original email,
explicitly. When and how you fixed them, or why did not. Not
with promises like "will do", "ok", and so on, they don't help
much.

Otherwise I see you tend to miss some of the comments. Also I
don't remember all of my comments, so when you don't respond to
them, I need to go to my old email, and match the comments
with the places you was supposed to fix, and it takes time.

See how Sergey and Mergen send their patches for examples.

https://github.com/tarantool/tarantool/wiki/Code-review-procedure#during-the-review

See 4 comments below.

> module:load(name) -> obj | error`

1. In the previous review I said you missed ` in the beginning
of the line here. Please, add it. Otherwise the formatting might
be broken.

> ---------------------------------
> 
> Loads a new function with name `name` from the previously
> loaded `module` and return a callable object instance
> associated with the function. On failure an error is thrown.
> 
> Possible errors:
>  - IllegalParams: function name is either not supplied
>    or not a string.
>  - IllegalParams: attempt to load a function but module
>    has been unloaded already.
>  - ClientError: no such function in the module.
>  - OutOfMemory: unable to allocate a function.
> 
> Example:
> 
> ``` Lua
> -- Load a module if not been loaded yet.
> m = box.lib.load('path/to/library')
> -- Load a function with the `foo` name from the module `m`.
> func = m:load('foo')
> ```
> 
> diff --git a/src/box/lua/lib.c b/src/box/lua/lib.c
> new file mode 100644
> index 000000000..78aee37b5
> --- /dev/null
> +++ b/src/box/lua/lib.c
> @@ -0,0 +1,606 @@
> +
> +static int
> +cache_put(struct box_module_func *cf)
> +{
> +	const struct mh_strnptr_node_t nd = {
> +		.str	= cf->key,
> +		.len	= cf->len,
> +		.hash	= mh_strn_hash(cf->key, cf->len),
> +		.val	= cf,
> +	};
> +	mh_int_t e = mh_strnptr_put(func_hash, &nd, NULL, NULL);

2. As I said in the previous review, it would be good to get the
old value and ensure it is NULL. So as not to replace something
and not notice it.

> +	if (e == mh_end(func_hash)) {
> +		diag_set(OutOfMemory, sizeof(nd), "malloc",
> +			 "box.lib: hash node");
> +		return -1;
> +	}
> +	return 0;
> +}

<...>

> +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";
> +
> +	if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) {
> +		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);
> +	const size_t max_sym_len = 512;
> +
> +	if (sym_len < 1) {
> +		diag_set(IllegalParams, fmt_noname, method);
> +		return luaT_error(L);
> +	} else if (sym_len > max_sym_len) {
> +		diag_set(IllegalParams, "Symbol \'%s\' is too long (max %zd)",
> +			 sym, max_sym_len);
> +		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.
> +	 *
> +	 * The key's buffer should be big enough to keep
> +	 * the longest package path plus symbol name and
> +	 * the pointer.
> +	 */
> +	char key[PATH_MAX + 2 * max_sym_len];

3. +3 for the dots you added below.

> +	snprintf(key, sizeof(key), "%p.%s.%s",
> +		 (void *)m, m->package, sym);

4. Is '(void *)' cast needed? It should work implicitly, no?

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