Re: [Tarantool-patches] [PATCH v19 5/6] box/cmod: implement cmod Lua module

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

Thanks for the patch!

Please, add a changelog file.

See 8 comments below.

> diff --git a/src/box/lua/cmod.c b/src/box/lua/cmod.c
> new file mode 100644
> index 000000000..ea5acd510
> --- /dev/null
> +++ b/src/box/lua/cmod.c
> @@ -0,0 +1,605 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <dlfcn.h>

1. You don't need dlfcn.h. I suspect some other headers
as well. For instance, eio.h, fcntl.h. Please, remove the
not needed headers.

> +#include <fcntl.h>
> +#include <lua.h>
> +
> +#include "box/error.h"
> +#include "box/port.h"
> +#include "box/func_def.h"
> +
> +#include "tt_static.h"
> +
> +#include "assoc.h"
> +#include "cmod.h"
> +#include "fiber.h"
> +#include "module_cache.h"
> +
> +#include "lua/utils.h"
> +#include "libeio/eio.h"
> +
> +/**
> + * Function descriptor.
> + */
> +struct cmod_func {
> +	/**
> +	 * C function to call.
> +	 */
> +	struct module_func mf;
> +	/** 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];
> +};
> +
> +/** Function name to cmod_func hash. */
> +static struct mh_strnptr_t *cmod_func_hash = NULL;
> +
> +/** A type to find a module from an object. */
> +static const char *uname_cmod = "tt_uname_cmod";
> +
> +/** A type to find a function from an object. */
> +static const char *uname_func = "tt_uname_cmod_func";
> +
> +/** Get data associated with an object. */
> +static void *
> +get_udata(struct lua_State *L, const char *uname)
> +{
> +	void **pptr = luaL_testudata(L, 1, uname);
> +	return pptr != NULL ? *pptr : NULL;
> +}
> +
> +/** Set data to a new value. */
> +static void
> +set_udata(struct lua_State *L, const char *uname, void *ptr)
> +{
> +	void **pptr = luaL_testudata(L, 1, uname);
> +	assert(pptr != NULL);
> +	*pptr = ptr;
> +}
> +
> +/** Setup a new data and associate it with an object. */
> +static void
> +new_udata(struct lua_State *L, const char *uname, void *ptr)
> +{
> +	*(void **)lua_newuserdata(L, sizeof(void *)) = ptr;
> +	luaL_getmetatable(L, uname);
> +	lua_setmetatable(L, -2);
> +}
> +
> +/**
> + * Helpers for function cache.
> + */
> +static void *
> +cf_cache_find(const char *str, size_t len)

2. What is 'cf', and why did you use simply cache_find/add/del in
other patches but now you start adding prefixes?

> +{
> +	mh_int_t e = mh_strnptr_find_inp(cmod_func_hash, str, len);
> +	if (e == mh_end(cmod_func_hash))
> +		return NULL;
> +	return mh_strnptr_node(cmod_func_hash, e)->val;
> +}
> +
> +static int
> +cf_cache_add(struct cmod_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(cmod_func_hash, &nd, NULL, NULL);
> +	if (e == mh_end(cmod_func_hash)) {
> +		diag_set(OutOfMemory, sizeof(nd), "malloc",
> +			 "cmod: hash node");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static void
> +cache_del(struct cmod_func *cf)
> +{
> +	mh_int_t e = mh_strnptr_find_inp(cmod_func_hash,
> +					 cf->key, cf->len);
> +	if (e != mh_end(cmod_func_hash))
> +		mh_strnptr_del(cmod_func_hash, e, NULL);
> +}
> +
> +/**
> + * Load a module.
> + *
> + * This function takes a module path from the caller
> + * stack @a L and returns cached module instance or
> + * 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 throws an error.
> + */
> +static int
> +lcmod_load(struct lua_State *L)

3. Why do you use 'struct lua_State' in this patch, and just
'lua_State' in the module_cache patch?

> +{
> +	const char msg_noname[] = "Expects cmod.load(\'name\') "
> +		"but no name passed";
> +
> +	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
> +		diag_set(IllegalParams, msg_noname);
> +		return luaT_error(L);
> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	if (name_len < 1) {
> +		diag_set(IllegalParams, msg_noname);
> +		return luaT_error(L);
> +	}
> +
> +	struct module *m = module_load(name, name_len);
> +	if (m == NULL)
> +		return luaT_error(L);
> +
> +	new_udata(L, uname_cmod, m);
> +	return 1;
> +}
> +
> +/**
> + * Unload a module.
> + *
> + * Take a module object from the caller stack @a L and unload it.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: module is not supplied.
> + * - IllegalParams: the module is unloaded.
> + *
> + * @returns true on success or throwns an error.
> + */
> +static int
> +lcmod_unload(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1) {
> +		diag_set(IllegalParams, "Expects module:unload()");
> +		return luaT_error(L);
> +	}
> +
> +	struct module *m = get_udata(L, uname_cmod);
> +	if (m == NULL) {
> +		diag_set(IllegalParams, "The module is unloaded");
> +		return luaT_error(L);
> +	}
> +
> +	set_udata(L, uname_cmod, NULL);
> +	module_unload(m);
> +	lua_pushboolean(L, true);
> +	return 1;
> +}
> +
> +/** Handle __index request for a module object. */
> +static int
> +lcmod_index(struct lua_State *L)
> +{
> +	lua_getmetatable(L, 1);
> +	lua_pushvalue(L, 2);
> +	lua_rawget(L, -2);
> +	if (!lua_isnil(L, -1))
> +		return 1;
> +
> +	struct module *m = get_udata(L, uname_cmod);
> +	if (m == NULL) {
> +		lua_pushnil(L);
> +		return 1;
> +	}
> +
> +	const char *key = lua_tostring(L, 2);
> +	if (key == NULL || lua_type(L, 2) != LUA_TSTRING) {
> +		diag_set(IllegalParams,
> +			 "Bad params, use __index(obj, <string>)");

4. Nobody ever uses __index explicitly like this. Better tell to
use "[key]".

> +		return luaT_error(L);
> +	}
> +
> +	if (strcmp(key, "path") == 0) {
> +		lua_pushstring(L, m->package);
> +		return 1;
> +	}
> +
> +	/*
> +	 * Internal keys for debug only, not API.
> +	 */
> +	if (strncmp(key, "tt_dev.", 7) == 0) {

5. This is a cool idea about retrieving info for the
tests, but we already have ERRINJ_DYN_MODULE_COUNT. It
helps to find if the modules are correctly allocated and
deleted, which makes not necessary to introduce 'refs'.

Talking of 'ptr', you don't need it either, if you
can see the module count. You just check the count
before and after module load. If it is incremented, a new
module was loaded.

If it is decremented, a module was unloaded.

For testing a single module used both from cmod and
box.schema.func you could add a global variable to your C
module and see that both APIs work with the same global
variable. For instance, add a function which increments it
and returns the old value. Then call it both from cmod and
box.schema.func, and see it is 2 in the end.

Or keep using your hacks, but at least make them easier
to use. By forcing to use `.` you make impossible to
write the indexes without ['...']. Make it easier, and no
need for 'tt' thing. For instance module.debug_refs.
func.debug_module_ptr, and so on. ['...'] looks bulky.

> +		const char *subkey = &key[7];
> +		if (strcmp(subkey, "refs") == 0) {
> +			lua_pushnumber(L, m->refs);
> +			return 1;
> +		} else if (strcmp(subkey, "ptr") == 0) {
> +			const char *s = tt_sprintf("%p", m);
> +			lua_pushstring(L, s);
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/** Module representation for REPL (console). */
> +static int
> +lcmod_serialize(struct lua_State *L)
> +{
> +	struct module *m = get_udata(L, uname_cmod);
> +	if (m == NULL) {
> +		lua_pushnil(L);
> +		return 1;
> +	}
> +
> +	lua_createtable(L, 0, 1);
> +	lua_pushstring(L, m->package);
> +	lua_setfield(L, -2, "path");
> +	return 1;
> +}
> +
> +/** Collect a module. */
> +static int
> +lcmod_gc(struct lua_State *L)
> +{
> +	struct module *m = get_udata(L, uname_cmod);
> +	if (m != NULL) {
> +		set_udata(L, uname_cmod, NULL);
> +		module_unload(m);
> +	}
> +	return 0;
> +}
> +
> +/** Increase reference to a function. */
> +static void
> +cmod_func_ref(struct cmod_func *cf)
> +{
> +	assert(cf->refs >= 0);
> +	++cf->refs;
> +}
> +
> +/** Free function memory. */
> +static void
> +cmod_func_delete(struct cmod_func *cf)
> +{

6. Please, add an assertion it does not keep a reference at
module * anymore.

> +	TRASH(cf);
> +	free(cf);
> +}
> +
> +/** Unreference a function and free if last one. */
> +static void
> +cmod_func_unref(struct cmod_func *cf)
> +{
> +	assert(cf->refs > 0);
> +	if (--cf->refs == 0) {
> +		module_func_unload(&cf->mf);
> +		cache_del(cf);
> +		cmod_func_delete(cf);
> +	}
> +}
> +
> +/** Function name from a hash key. */
> +static char *
> +cmod_func_name(struct cmod_func *cf)
> +{
> +	return &cf->key[cf->len - cf->sym_len];
> +}
> +
> +/**
> + * Allocate a new function instance and resolve its address.
> + *
> + * @param m a module the function should be loaded from.
> + * @param key function hash key, ie "addr.module.foo".
> + * @param len length of @a key.
> + * @param sym_len function symbol name length, ie 3 for "foo".
> + *
> + * @returns function instance on success, NULL otherwise (diag is set).
> + */
> +static struct cmod_func *
> +cmod_func_new(struct module *m, const char *key, size_t len, size_t sym_len)
> +{
> +	size_t size = sizeof(struct cmod_func) + len + 1;
> +	struct cmod_func *cf = malloc(size);
> +	if (cf == NULL) {
> +		diag_set(OutOfMemory, size, "malloc", "cf");
> +		return NULL;
> +	}
> +
> +	cf->len = len;
> +	cf->sym_len = sym_len;
> +	cf->refs = 0;
> +
> +	module_func_create(&cf->mf);
> +	memcpy(cf->key, key, len);
> +	cf->key[len] = '\0';
> +
> +	if (module_func_load(m, cmod_func_name(cf), &cf->mf) != 0) {
> +		diag_set(ClientError, ER_LOAD_FUNCTION,
> +			 cmod_func_name(cf), dlerror());
> +		cmod_func_delete(cf);
> +		return NULL;
> +	}
> +
> +	if (cf_cache_add(cf) != 0) {
> +		cmod_func_delete(cf);

7. You didn't unload the function.

> +		return NULL;
> +	}
> +
> +	/*
> +	 * Each new function depends on module presence.
> +	 * Module will reside even if been unload
> +	 * explicitly after the function creation.
> +	 */
> +	cmod_func_ref(cf);
> +	return cf;
> +}
> +
> +/** Initialize cmod. */
> +void
> +box_lua_cmod_init(struct lua_State *L)
> +{
> +	cmod_func_hash = mh_strnptr_new();
> +	if (cmod_func_hash == NULL)
> +		panic("cmod: Can't allocate func hash table");
> +
> +	(void)L;

8. No need to cast to void. It is used.

> +	static const struct luaL_Reg top_methods[] = {
> +		{ "load",		lcmod_load		},
> +		{ NULL, NULL },
> +	};
> +	luaL_register_module(L, "cmod", top_methods);
> +	lua_pop(L, 1);
> +
> +	static const struct luaL_Reg lcmod_methods[] = {
> +		{ "unload",		lcmod_unload		},
> +		{ "load",		lcmod_load_func		},
> +		{ "__index",		lcmod_index		},
> +		{ "__serialize",	lcmod_serialize		},
> +		{ "__gc",		lcmod_gc		},
> +		{ NULL, NULL },
> +	};
> +	luaL_register_type(L, uname_cmod, lcmod_methods);
> +
> +	static const struct luaL_Reg lfunc_methods[] = {
> +		{ "unload",		lfunc_unload		},
> +		{ "__index",		lfunc_index		},
> +		{ "__serialize",	lfunc_serialize		},
> +		{ "__gc",		lfunc_gc		},
> +		{ "__call",		lfunc_call		},
> +		{ NULL, NULL },
> +	};
> +	luaL_register_type(L, uname_func, lfunc_methods);
> +}