Re: [Tarantool-patches] [PATCH v15 10/11] box/cmod: implement cmod Lua module

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

Thanks for the patch!

See 10 comments below.

Overall no major issues, much better now.

> module:load(name) -> obj | error`
> ---------------------------------
> 
> 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.
>  - ClientError: module has been updated on disk and not
>    reloaded.

1. We decided not to throw an errors in this case.

>  - OutOfMemory: unable to allocate a function.
> 
> Executing a loaded function
> ===========================
> 
> Once function is loaded it can be executed as an ordinary Lua call.
> Lets consider the following example. We have a `C` function which
> takes two numbers and returns their sum.
> 
> ``` C
> int
> cfunc_sum(box_function_ctx_t *ctx, const char *args, const char *args_end)
> {
> 	uint32_t arg_count = mp_decode_array(&args);
> 	if (arg_count != 2) {
> 		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
> 				     "invalid argument count");
> 	}
> 	uint64_t a = mp_decode_uint(&args);
> 	uint64_t b = mp_decode_uint(&args);
> 
> 	char res[16];
> 	char *end = mp_encode_uint(res, a + b);
> 	box_return_mp(ctx, res, end);
> 	return 0;
> }
> ```
> 
> The name of the function is `cfunc_sum` and the function is built into
> `cfunc.so` shared library.
> 
> First we should load it as
> 
> ``` Lua
> m = require('cmod').load('cfunc')
> cfunc_sum = m:load('cfunc_sum')
> ```
> 
> Once successfully loaded we can execute it. Note that unlike regular
> Lua functions the context of `C` functions is different. They never
> thrown an exception but return `true|nil, res` form where first value
> set to `nil` in case of error condition and `res` carries an error
> description.

2. In the previous review I reminded you that now we throw errors
again, according to the updated Lua guidelines.

> Lets call the `cfunc_sum` with wrong number of arguments
> 
> ``` Lua
> local ok, res = cfunc_sum()
> if not ok then
>     print(res)
> end
> ```
> 
> We will the `"invalid argument count"` message in output.

3. Will what? "See"?

> The error message has been set by the `box_error_set` in `C`
> code above.
> 
> Module and function caches
> ==========================
> 
> Loading a module is relatively slow procedure because operating
> system needs to read the library, resolve its symbols and etc.
> Thus to speedup this procedure if the module is loaded for a first
> time we put it into an internal cache. If module is sitting in
> the cache already and new request to load comes in we simply
> reuse a previous copy immediately. Same applies to functions:
> while symbol lookup is a way faster than loading module from
> disk it is not completely cheap, thus we cache functions as
> well. Functions entries in cache are identified by a module
> path and function name.
> 
> Still the following situation is possible: the module is loaded but
> user does recompile it and overwrite on a storage device. Thus
> cache content no longer matches the shared library on the disk.
> 
> To handle this situation we use that named cache invalidation procedure:
> on every attempt to load the same module again we test low level file
> attributes (such as storage device number, inode, size and modification
> time) and if they are differ from ones kept by the cache then old module
> marked as orphan, new instance is loaded and become a valid cache
> entry. Module state could be seen in module variable output
> 
> ```Lua
> m = require('cmod').load('cfunc')
> m
> ```
> which will output
> ```
> tarantool> m
> ---
> - path: cfunc
>   state: cached
> ```

4. Lets not expose any states or other attributes until it is requested
explicitly by someone.

> The `state` is either `cached` if module is present in cache
> and valid or `orphan` if entry in cache has been updated.
> 
> diff --git a/src/box/lua/cmod.c b/src/box/lua/cmod.c
> new file mode 100644
> index 000000000..60fd2e812
> --- /dev/null
> +++ b/src/box/lua/cmod.c
> +
> +/**
> + * Unload a symbol and free a function instance.
> + */
> +static void
> +func_delete(struct cmod_func *cf)

5. Methods of a struct should have the struct name as a prefix.
So it should be cmod_func_delete(). The same for other
cmod_func methods. For Lua the prefix 'lcmod_func' is good.

> +{
> +	assert(cf->refs == 0);
> +	module_sym_unload(&cf->mod_sym);
> +	TRASH(cf);
> +	free(cf);
> +}> +
> +/**
> + * Load a new function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and creates a new function object. If
> + * the function is already loaded we simply return
> + * a reference to existing one.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - IllegalParams: function references limit exceeded.
> + * - OutOfMemory: unable to allocate a function.
> + * - ClientError: no such function in the module.
> + * - ClientError: module has been updated on disk and not
> + *   yet unloaded and loaded back.

6. It was decided not to throw an error in this case.

> + *
> + * @returns function object on success or throwns an error.
> + */
> +static int
> +lcmod_func_load(struct lua_State *L)
> diff --git a/src/box/lua/cmod.h b/src/box/lua/cmod.h
> new file mode 100644
> index 000000000..f0ea2d34d
> --- /dev/null
> +++ b/src/box/lua/cmod.h
> @@ -0,0 +1,24 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#pragma once
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +struct lua_State;
> +
> +/**
> + * Initialize cmod Lua module.
> + *
> + * @param L Lua state where to register the cmod module.
> + */
> +void
> +box_lua_cmod_init(struct lua_State *L);

7. Please, add an empty line after the declaration.

> +#if defined(__cplusplus)
> +}
> +#endif /* defined(__plusplus) */
> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> index 22b906fd7..e96cbd1f8 100644
> --- a/src/box/module_cache.c
> +++ b/src/box/module_cache.c

8. Most of changes in this file should not happen, as I
described in the comments to the previous patches.

You will probably need to add a few more commits for this.
One to add a second hash in box/func; next to make module
loading always check stat in module_cache.

> @@ -514,6 +553,74 @@ module_sym_call(struct module_sym *mod_sym, struct port *args,
>  	return rc;
>  }
>  
> +struct module *
> +module_load(const char *package, const char *package_end)
> +{
> +	char path[PATH_MAX];
> +	if (module_find(package, package_end, path, sizeof(path)) != 0)
> +		return NULL;
> +
> +	struct module *cached, *module;
> +	struct mh_strnptr_t *h = hash_tbl(false);
> +	cached = module_cache_find(h, package, package_end);
> +	if (cached == NULL) {
> +		module = module_new(path, h, package, package_end);
> +		if (module == NULL)
> +			return NULL;
> +		if (module_cache_add(module) != 0) {
> +			module_unref(module);
> +			return NULL;
> +		}
> +		return module;
> +	}
> +
> +	struct stat st;
> +	if (stat(path, &st) != 0) {
> +		diag_set(SystemError, "module: stat() module %s", path);
> +		return NULL;
> +	}
> +
> +	/*
> +	 * When module comes from cache make sure that
> +	 * it is not changed on the storage device. The
> +	 * test below still can miss update if cpu data
> +	 * been manually moved backward and device/inode
> +	 * persisted but this is a really rare situation.
> +	 *
> +	 * If update is needed one can simply "touch file.so"
> +	 * to invalidate the cache entry.
> +	 */
> +	if (cached->st.st_dev == st.st_dev &&
> +	    cached->st.st_ino == st.st_ino &&
> +	    cached->st.st_size == st.st_size &&
> +	    memcmp(&cached->st.st_mtim, &st.st_mtim,
> +		   sizeof(st.st_mtim)) == 0) {
> +		module_ref(cached);
> +		return cached;

9. Build fails on Mac:

/Users/gerold/Work/Repositories/tarantool/src/box/module_cache.c:596:25: error: no member named 'st_mtim' in 'struct stat'
            memcmp(&cached->st.st_mtim, &st.st_mtim,
                    ~~~~~~~~~~ ^
/Users/gerold/Work/Repositories/tarantool/src/box/module_cache.c:596:38: error: no member named 'st_mtim' in 'struct stat'
            memcmp(&cached->st.st_mtim, &st.st_mtim,
                                         ~~ ^
/Users/gerold/Work/Repositories/tarantool/src/box/module_cache.c:597:16: error: no member named 'st_mtim' in 'struct stat'
                   sizeof(st.st_mtim)) == 0) {
                          ~~ ^
3 errors generated.

This is because here last modification time is stored as a
timespec struct, and has name 'st_mtimespec'.

> +	}
> +
> +	/*
> +	 * Load a new module, update the cache
> +	 * and orphan an old module instance.
> +	 */
> +	module = module_new(path, h, package, package_end);
> +	if (module == NULL)
> +		return NULL;
> +	if (module_cache_update(module) != 0) {
> +		module_unref(module);
> +		return NULL;
> +	}
> +
> +	module_set_orphan(cached);
> +	return module;
> +}
> +
> +void
> +module_unload(struct module *module)
> +{
> +	module_unref(module);
> +}
> +
>  int
>  module_reload(const char *package, const char *package_end)
>  {
> diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> index 875f2eb3c..17a2e27bb 100644
> --- a/src/box/module_cache.h
> +++ b/src/box/module_cache.h
> @@ -48,6 +52,10 @@ struct module {
>  	 * Count of active references to the module.
>  	 */
>  	int64_t refs;
> +	/**
> +	 * Storage stat for identity check.
> +	 */
> +	struct stat st;

10. The structure if huge. And you use just a few fields. Better
store only the need fields.

>  	/**
>  	 * Module's package name.
>  	 */