[Tarantool-patches] [PATCH v12 7/8] box/cmod: implement cmod Lua module
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Jan 24 19:28:27 MSK 2021
Thanks for the patch!
See 21 comments below.
On 18.01.2021 21:35, Cyrill Gorcunov wrote:
> Currently to run "C" function from some external module
> one have to register it first in "_func" system space. This
> is a problem if node is in read-only mode (replica).
>
> Still people would like to have a way to run such functions
> even in ro mode. For this sake we implement "cmod" lua module.
>
> Fixes #4642
>
> Signed-off-by: Cyrill Gorcunov <gorcunov at gmail.com>
>
> @TarantoolBot document
> Title: cmod module
>
> Overview
> ========
>
> `cmod` module provides a way to create, delete and execute
> `C` procedures. Unlike `box.schema.func` methods the functions
> created with `cmod` 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('cmod').load(path) -> obj | nil, err`
1. I don't like it, but I must say: according to one another
change in Lua guidelines now we must again throw exceptions
from all new functions present on the board instead of returning
errors. You can try to raise it in the chat if you want.
> ----------------------------------------------
>
> Loads a module from `path` and return an object instance
> associate with the module. The `path` should not end up
> with shared object extension (such as `.so`), only a file
> name shall be there.
>
> Possible errors:
>
> - IllegalParams: module path 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.
>
> `module:unload() -> true | nil, err`
> ------------------------------------
>
> Unloads a module. 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, err = require('cmod').load('path')
> if not m then
> print(err)
> end
> ok, err = m:unload()
> if not ok then
> print(err)
> end
> ```
>
> If there are functions from this module referenced somewhere
> in other Lua code they still can be called because module
> continue sitting in memory until the last reference is closed.
>
> If the module become a target to the Lua's garbage collector
> then unload is called implicitly.
>
> `module:reload() -> true | nil, err`
> ------------------------------------
>
> Reloads a module and all functions which were associated with
> the module. Each module keeps a list of functions belonging to
> the module and reload procedure cause the bound function to update
> their addresses such that function execution will be routed via
> a new library.
>
> Modules are loaded with that named local binding which means
> that reload of module symbols won't affect the functions which
> are started execution already, only new calls will be rerouted.
>
> Possible errors:
> - IllegalParams: a module is not supplied.
> - ClientError: a module does not exist.
2. What happens to the existing function objects on reload()?
How is reload different from unload() + load()?
> On success `true` is returned, otherwise `nil,error` pair.
>
> Example:
>
> ``` Lua
> m, err = require('cmod').load('path')
> if not m then
> print(err)
> end
> ok, err = m:reload()
> if not ok then
> print(err)
> end
> ```
>
> module:load(name) -> obj | nil, err`
> ------------------------------------
>
> Loads a new function with name `name` from the `module` object
> and return a callable object instance associate with the function.
>
> Possible errors:
> - IllegalParams: function name is either not supplied
> or not a string.
> - OutOfMemory: unable to allocate a function.
3. What if the function is not found?
> Example:
>
> ``` Lua
> m, err = require('cmod').load('path')
> if not m then
> print(err)
> end
> f, err = m:load('foo')
> if not ok then
> print(err)
> end
> ```
>
> `function:unload() -> true | nil, err`
> --------------------------------------
>
> Unloads a function.
>
> Possible errors:
> - IllegalParams: function name is either not supplied
> or not a string.
> - IllegalParams: the function does not exist.
4. Is this unloaded automatically, when the function object is
GCed?
> Example:
>
> ``` Lua
> m, err = require('cmod').load('path')
> if not m then
> print(err)
> end
> f, err = m:load('foo')
> if not ok then
> print(err)
> end
> ok, err = f:unload()
> if not ok then
> print(err)
> end
> ```
>
> Executing a loaded function
> ===========================
>
> Once function is loaded it can be executed by 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, err = require('cmod').load('cfunc')
> if not m then
> print(err)
> end
> cfunc_sum, err = m:load('cfunc_sum')
> if not cfunc_sum then
> print(err)
> end
> ```
>
> 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.
>
> 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.
> The error message has been set by the `box_error_set` in `C`
> code above.
>
> On success the first returned value set to `true` and `res` represent
> function execution result.
>
> ``` Lua
> local ok, res = cfunc_sum(1, 2)
> assert(ok);
> print(res)
> ```
>
> We will see the number `3` in output.
5. What happens when I do multireturn?
> diff --git a/src/box/lua/cmod.c b/src/box/lua/cmod.c
> new file mode 100644
> index 000000000..c3a44e3df
> --- /dev/null
> +++ b/src/box/lua/cmod.c
> @@ -0,0 +1,718 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <string.h>
> +#include <lua.h>
> +
> +#include "assoc.h"
> +#include "diag.h"
> +
> +#include "box/module_cache.h"
> +#include "box/error.h"
> +#include "box/port.h"
> +#include "tt_static.h"
> +
> +#include "trivia/util.h"
> +#include "lua/utils.h"
> +
> +/**
> + * Function descriptor.
> + */
> +struct cmod_func {
> + /**
> + * Symbol descriptor for the function in
> + * an associated module.
> + */
> + struct module_sym mod_sym;
> + /**
> + * Number of loads of the function.
> + */
> + int64_t load_count;
> + /**
> + * The function name (without a package path).
> + */
> + const char *func_name;
6. This member is never used. I removed it and nothing
changed.
====================
@@ -31,10 +31,6 @@ struct cmod_func {
* Number of loads of the function.
*/
int64_t load_count;
- /**
- * The function name (without a package path).
- */
- const char *func_name;
/**
* Length of @a name member.
*/
@@ -231,12 +227,11 @@ cmod_func_free(struct cmod_func *cf)
*
* @param name package path and a function name, ie "module.foo"
* @param len length of @a name.
- * @param func_name_len function name length, ie "3" for "module.foo"
*
* @returns function instance on success, NULL otherwise setting diag area.
*/
static struct cmod_func *
-cmod_func_new(const char *name, size_t len, size_t func_name_len)
+cmod_func_new(const char *name, size_t len)
{
const ssize_t cf_size = sizeof(struct cmod_func);
size_t size = cf_size + len + 1;
@@ -250,7 +245,6 @@ cmod_func_new(const char *name, size_t len, size_t func_name_len)
cf->mod_sym.module = NULL;
cf->load_count = 0;
cf->mod_sym.name = cf->name;
- cf->func_name = &cf->name[len - func_name_len];
cf->len = len;
memcpy(cf->name, name, len);
@@ -309,7 +303,7 @@ lcmod_func_load(struct lua_State *L)
cf = cmod_func_find(name, len);
if (cf == NULL) {
- cf = cmod_func_new(name, len, strlen(func_name));
+ cf = cmod_func_new(name, len);
if (cf == NULL)
return luaT_push_nil_and_error(L);
====================
> + /**
> + * Length of @a name member.
> + */
> + size_t len;
> + /**
> + * Module path with function name separated
> + * by a point, like "module.func".
> + *
> + * See parse_func_name helper, we simply have
> + * to keep this strange architecture for backward
> + * compatibility sake (ie for functions which
> + * are created via box.schema.func).
> + */
> + char name[0];
> +};
> +
> +/** Get a handle associated with an object. */
> +static void *
> +cmod_get_handle(struct lua_State *L, const char *uname)
> +{
> + void **pptr = luaL_testudata(L, 1, uname);
> + return pptr != NULL ? *pptr : NULL;
> +}
> +
> +/** Set a handle to a new value. */
> +static void
> +cmod_set_handle(struct lua_State *L, const char *uname, void *ptr)
> +{
> + void **pptr = luaL_testudata(L, 1, uname);
> + if (pptr != NULL)
> + *pptr = ptr;
> +}
> +
> +/** Setup a new handle and associate it with an object. */
> +static void
> +cmod_setup_handle(struct lua_State *L, const char *uname, void *ptr)
> +{
> + *(void **)lua_newuserdata(L, sizeof(void *)) = ptr;
> + luaL_getmetatable(L, uname);
> + lua_setmetatable(L, -2);
> +}
> +
> +/** A type to find a module handle from an object. */
> +static const char *cmod_module_handle_uname = "cmod_module_handle";
> +
> +static struct module *
> +cmod_get_module_handle(struct lua_State *L, bool mandatory)
> +{
> + struct module *m = cmod_get_handle(L, cmod_module_handle_uname);
> + if (mandatory && m == NULL) {
> + const char *fmt = "The module is already unloaded";
> + diag_set(IllegalParams, fmt);
> + }
> + return m;
> +}
> +
> +static void
> +cmod_set_module_handle(struct lua_State *L, struct module *module)
> +{
> + cmod_set_handle(L, cmod_module_handle_uname, module);
> +}
> +
> +static void
> +cmod_setup_module_handle(struct lua_State *L, struct module *module)
> +{
> + cmod_setup_handle(L, cmod_module_handle_uname, module);
> +}
> +
> +/** A type to find a function handle from an object. */
> +static const char *cmod_func_handle_uname = "cmod_func_handle";
7. Please, move global declarations to the beginning of the file
so as it would be easier to find them, and to be consistent with
other files.
> +
> +static struct cmod_func *
> +cmod_get_func_handle(struct lua_State *L, bool mandatory)
8. According to the code style, flags must have 'is' prefix or
a similar one like 'has', 'does', etc. Here it should be 'is_mandatory'.
But a better idea - just drop it. The single place where it is not
mandatory is GC handler.
Another option - introduce separate get() and check(). Get() only
returns it without checking for NULL. Check() will ensure it is not NULL
and set the error otherwise.
Also the function takes a Lua state, so it seems it must have lcmod
prefix, not cmod. According to the naming convention you established
in this file.
> +{
> + struct cmod_func *cf = cmod_get_handle(L, cmod_func_handle_uname);
> + if (mandatory && cf == NULL) {
> + const char *fmt = "The function is already unloaded";
> + diag_set(IllegalParams, fmt);
> + }
> + return cf;
> +}
> +
> +static void
> +cmod_set_func_handle(struct lua_State *L, struct cmod_func *cf)
> +{
> + cmod_set_handle(L, cmod_func_handle_uname, cf);
> +}
> +
> +static void
> +cmod_setup_func_handle(struct lua_State *L, struct cmod_func *cf)
> +{
> + cmod_setup_handle(L, cmod_func_handle_uname, cf);
> +}
9. Amount of boilerplate code and double checks of the same
conditions you need just to push some udata is terrifying, tbh.
For instance, take lcmod_func_handle_call(). It calls
cmod_get_func_handle() and checks result for NULL. This one
calls cmod_get_handle() and also checks result for NULL.
This one calls luaL_testudata() and also checks result for NULL.
You did 3 NULL checks which could be one, if you would have
a set of simpler functions working directly with luaL_testudata().
10. Why do you have 'handle' word in each function name? For
example, how 'handle_call' is any different from just 'call' in
scope of code in this file? Or how is 'cmod_setup_func' different
from 'cmod_setup_func_handle'? It seems that by 'handle' you
try not to clash names with the non-Lua part of cmod. But you
should do it via prefixes, not via suffixes. lcmod instead of cmod.
> +
> +/**
> + * Function name to cmod_func hash. The name includes
> + * module package path without file extension.
> + */
> +static struct mh_strnptr_t *func_hash = NULL;
> +
> +/**
> + * Find function in cmod_func hash.
> + *
> + * @param name function name.
> + * @param name_len function name length.
> + *
> + * @returns function descriptor if found, NULL otherwise.
> + */
> +struct cmod_func *
> +cmod_func_find(const char *name, size_t name_len)
11. The function must be static.
> +{
> + mh_int_t e = mh_strnptr_find_inp(func_hash, name, name_len);
> + if (e == mh_end(func_hash))
> + return NULL;
> + return mh_strnptr_node(func_hash, e)->val;
> +}
> +
> +/**
> + * Delete a function instance from the hash or decrease
> + * a reference if the function is still loaded.
> + *
> + * @param cf function descriptor.
> + *
> + * @retval true if the function has no more loaded instances
> + * and removed from the hash.
> + *
> + * @retval false if there are loaded instances left and function
> + * is kept in the hash.
> + */
> +static bool
> +cmod_func_del(struct cmod_func *cf)
12. This whole cmod and lcmod API looks extremely inconsistent and complex.
Here are the functions you have (not counting the boilerplate code above):
// Find a function in the cache, ok.
cmod_func_find(name)
// It is called 'delete'. But it does not do the
// delete. Moreover, it also decreases 'load_count',
// which is super surprising. What does it have to do
// with loads? You don't call module_sym_unload() here,
// but decrease load_count.
cmod_func_del(f)
// Ok, this looks like it should add the function to the
// hash. But wait, it also increases load_count, and does
// not call module_sym_load()! WTF?
cmod_func_add(f)
// It should free the memory, right? But it calls
// module_sym_unload(). Why!? Also, we have a code style
// agreement, that function freeing the memory should
// have suffix 'delete', not 'free'. As soon as you have
// 'new'.
cmod_func_free(f)
// Fine, it should allocate a new function object, right?
// But it also calls module_sym_load()! And does not increase
// load_count! I don't understand. It seems like the names
// of these functions has very little to do with what they are
// really doing. The only way to make them work is to call them
// in some special sequence, in which a next function fixes
// leftovers of previous functions.
cmod_func_new(name)
This must be seriously redesigned. Starting from the function
names.
Here is an option:
// Find the function in the hash. If found, increase load_count
// and return. Otherwise allocate a new function object, fill its
// members, do the load, and make load_count = 1. Add to the hash.
cmod_func_load(name)
// Decrease load_count. If became 0, remove from the hash,
// unload mod_sym, and free the memory.
cmod_func_unload(f)
I don't really see why would you need the other functions. These 2
are going to be extra simple anyway. Module_cache API for module
objects is literally that simple - load, unload, reload. Why do
you need so many for cmod, especially with so unclear behaviour?
> +{
> + assert(cf->load_count > 0);
> + if (cf->load_count-- != 1)
> + return false;
> +
> + mh_int_t e = mh_strnptr_find_inp(func_hash, cf->name, cf->len);
> + assert(e != mh_end(func_hash));
> + mh_strnptr_del(func_hash, e, NULL);
> + return true;
> +}
> +
> +/**
> + * Add a function instance into the hash or increase
> + * a reference if the function is already exist.
> + *
> + * @param cf Function descriptor.
> + *
> + * Possible errors:
> + *
> + * - OutOfMemory: unable to allocate a hash entry.
> + *
> + * @retval 0 on success.
> + * @retval -1 on error, diag is set.
> + */
> +static int
> +cmod_func_add(struct cmod_func *cf)
> +{
> + assert(cf->load_count >= 0);
> + if (cf->load_count++ != 0)
13. I already proposed an option how to rework cmod API above, but
still will leave some comments alongside.
Why the heck 'add' function increases load_count?
> + return 0;
> +
> + const struct mh_strnptr_node_t nd = {
> + .str = cf->name,
> + .len = cf->len,
> + .hash = mh_strn_hash(cf->name, cf->len),
> + .val = cf,
> + };
> +
> + mh_int_t e = mh_strnptr_put(func_hash, &nd, NULL, NULL);
> + if (e == mh_end(func_hash)) {
> + diag_set(OutOfMemory, sizeof(nd),
> + "malloc", "cmod_func node");
> + return -1;
> + }
> + return 0;
14. Why so complex? Just add it to the hash when it is created. This
function is used in a single place, where you already do the check
if it exists.
> +}
> +
> +/**
> + * Unload a symbol and free a function instance.
> + *
> + * @param cf function descriptor.
> + */
> +static void
> +cmod_func_free(struct cmod_func *cf)
> +{
> + module_sym_unload(&cf->mod_sym);
15. Worth adding load_count == 0 assertion.
Or rather do the unload where it belongs - where
load_count becomes 0.
> + TRASH(cf);
> + free(cf);
> +}
> +
> +/**
> + * Allocate a new function instance and resolve a symbol address.
> + *
> + * @param name package path and a function name, ie "module.foo"
> + * @param len length of @a name.
> + * @param func_name_len function name length, ie "3" for "module.foo"
> + *
> + * @returns function instance on success, NULL otherwise setting diag area.
> + */
> +static struct cmod_func *
> +cmod_func_new(const char *name, size_t len, size_t func_name_len)
> +{
> + const ssize_t cf_size = sizeof(struct cmod_func);
> + size_t size = cf_size + len + 1;
> + struct cmod_func *cf = malloc(size);
> + if (cf == NULL) {
> + diag_set(OutOfMemory, size, "malloc", "cf");
> + return NULL;
> + }
> +
> + cf->mod_sym.addr = NULL;
> + cf->mod_sym.module = NULL;
> + cf->load_count = 0;
> + cf->mod_sym.name = cf->name;
> + cf->func_name = &cf->name[len - func_name_len];
> + cf->len = len;
> +
> + memcpy(cf->name, name, len);
> + cf->name[len] = '\0';
> +
> + if (module_sym_load(&cf->mod_sym) != 0) {
16. You load, and don't increase load_count. Does not it look
contradictory to you?
> + cmod_func_free(cf);
> + return NULL;
> + }
> +
> + return 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.
> + *
> + * @returns function object on success or {nil,error} on error,
> + * the error is set to the diagnostics area.
> + */
> +static int
> +lcmod_func_load(struct lua_State *L)
> +{
> + const char *method = "function = module:load";
> + struct cmod_func *cf = NULL;
> +
> + if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) {
> + const char *fmt =
> + "Expects %s(\'name\') but no name passed";
> + diag_set(IllegalParams, fmt, method);
17. Why couldn't you just inline 'fmt' string? The current version
is not any shorter nor easier to read. The same in many other places.
> + return luaT_push_nil_and_error(L);
> + }
> +
> + struct module *m = cmod_get_module_handle(L, false);
> + if (m == NULL) {
> + const char *fmt =
> + "Expects %s(\'name\') but not module object passed";
> + diag_set(IllegalParams, fmt, method);
> + return luaT_push_nil_and_error(L);
> + }
> +
> + const char *func_name = lua_tostring(L, 2);
> + const char *name = tt_sprintf("%s.%s", m->package, func_name);
> + size_t len = strlen(name);
> +
> + cf = cmod_func_find(name, len);
18. Or you could pass package name and function name to the constructor
and copy them right to the allocated memory, without double copying
via the static buffer.
> + if (cf == NULL) {
> + cf = cmod_func_new(name, len, strlen(func_name));
> + if (cf == NULL)
> + return luaT_push_nil_and_error(L);
> + }
> +
> + if (cmod_func_add(cf) != 0) {
19. From this code it looks like you can fail addition of an
already existing function and you just delete it. I know it
can't happen, but the code says the opposite, and it is
confusing.
Another reason why 'add' is a bad name - it seems like it
should fail when added second time due to being already
added.
> + cmod_func_free(cf);
> + return luaT_push_nil_and_error(L);
> + }
> +
> + cmod_setup_func_handle(L, cf);
> + return 1;
> +}
> +
> +/**
> + * Unload a function.
> + *
> + * This function takes a function object from
> + * the caller stack @a L and unloads it.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function is not supplied.
> + * - 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
> +lcmod_func_unload(struct lua_State *L)
> +{
> + if (lua_gettop(L) != 1) {
> + const char *fmt = "Expects function:unload()";
> + diag_set(IllegalParams, fmt);
> + return luaT_push_nil_and_error(L);
> + }
> +
> + struct cmod_func *cf = cmod_get_func_handle(L, true);
> + if (cf == NULL)
> + return luaT_push_nil_and_error(L);
> +
> + cmod_set_func_handle(L, NULL);
> + if (cmod_func_del(cf))
> + cmod_func_free(cf);
> +
> + lua_pushboolean(L, true);
> + return 1;
> +}
> +
> +/**
> + * Load a new module.
> + *
> + * This function takes a module patch from the caller
> + * stack @a L and creates a new module object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: module path 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 {nil,error} on error,
> + * the error is set to the diagnostics area.
> + */
> +static int
> +lcmod_module_load(struct lua_State *L)
> +{
> + if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
> + const char *fmt =
> + "Expects cmod.load(\'name\') but no name passed";
> + diag_set(IllegalParams, fmt);
> + return luaT_push_nil_and_error(L);
> + }
> +
> + size_t name_len;
> + const char *name = lua_tolstring(L, 1, &name_len);
> +
> + struct module *module = module_load(name, name_len);
> + if (module == NULL)
> + return luaT_push_nil_and_error(L);
> +
> + cmod_setup_module_handle(L, module);
> + /*
> + * Make sure the module won't disappear until
> + * it is GC'ed or unloaded explicitly.
> + */
> + module->ref++;
20. The fact that you need to touch reference counter of an object
from a different subsystem should make you start to question
sanity of the API. Why module_load() does not do the increase,
and module_unload() - decrease?
> + return 1;
> +}> +
> +/**
> + * Module handle representation for REPL (console).
> + */
> +static int
> +lcmod_module_handle_serialize(struct lua_State *L)
> +{
> + struct module *m = cmod_get_module_handle(L, true);
> + if (m == NULL) {
> + lua_pushnil(L);
> + return 1;
> + }
> +
> + lua_createtable(L, 0, 0);
21. Since you decided to use createtable instead of newtable,
I suggest you to fill the proper parameters, which should
be (0, 1) in this case I think.
> + lua_pushstring(L, m->package);
> + lua_setfield(L, -2, "path");
> +
> + return 1;
> +}
More information about the Tarantool-patches
mailing list