[Tarantool-patches] [PATCH v15 10/11] box/cmod: implement cmod Lua module
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Feb 7 20:20:45 MSK 2021
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.
> */
More information about the Tarantool-patches
mailing list