Re: [Tarantool-patches] [PATCH v10 2/4] module_cache: move module handling into own subsystem

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 7 comments below.

On 05.11.2020 16:18, Cyrill Gorcunov wrote:
> The module handling should not be bound to particular
> function implementation (we will have two users: already
> existing functions for "_func" space, and a new upcoming
> one which will be serving cbox submodule in next patch).
> 
> For this sake all module related code is moved to
> module_cache file where we do symbol resolving,
> calling and tracking of symbol usage.

1. I see it is not just moved. I by luck noticed, that the code
looks suspiciously different. And after a closed look I
found that you did some renames, and functional changes. As I
said in the other email - please, don't do that. It is extremely
hard to spot what you changed or renamed in this huge diff with
lots of moves. I will commment below some changes that I noticed.

But I ask you to split the renames and functional changes into
separate commits, so as they could be reviewer properly.

> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> new file mode 100644
> index 000000000..9107bec54
> --- /dev/null
> +++ b/src/box/module_cache.c
> @@ -0,0 +1,484 @@
> +
> +/**
> + * A cpcall() helper for module_find().
> + */
> +static int
> +luaT_module_find(lua_State *L)
> +{
> +	struct module_find_ctx *ctx = (void *)lua_topointer(L, 1);
> +
> +	/*
> +	 * Call package.searchpath(name, package.cpath) and use
> +	 * the path to the function in dlopen().
> +	 */
> +	lua_getglobal(L, "package");
> +	lua_getfield(L, -1, "search");
> +
> +	/* Argument of search: name */
> +	lua_pushlstring(L, ctx->package, ctx->package_end - ctx->package);
> +
> +	lua_call(L, 1, 1);
> +	if (lua_isnil(L, -1))
> +		return luaL_error(L, "module not found");
> +
> +	/* Convert path to absolute */
> +	char resolved[PATH_MAX];
> +	if (realpath(lua_tostring(L, -1), resolved) == NULL) {
> +		diag_set(SystemError, "realpath");
> +		return luaT_error(L);
> +	}
> +
> +	snprintf(ctx->path, ctx->path_len, "%s", resolved);
> +	return 0;
> +}
> +
> +/**
> + * Find a path to a module using Lua's package.cpath.
> + */
> +static int
> +module_find(struct module_find_ctx *ctx)
> +{

2. In the original code the function took package name and
path, which was way more clear than a ctx. Which in the
old code was constructed inside of this function.

But you somewhy moved it to the caller code, to module_load(),
which creates the ctx just to call module_find(), and never
uses it again. So what was the point? I honestly do not
understand.

> +	lua_State *L = tarantool_L;
> +	int top = lua_gettop(L);
> +	if (luaT_cpcall(L, luaT_module_find, ctx) != 0) {
> +		diag_set(ClientError, ER_LOAD_MODULE,
> +			 (int)(ctx->package_end - ctx->package),
> +			 ctx->package, lua_tostring(L, -1));
> +		lua_settop(L, top);
> +		return -1;
> +	}
> +	assert(top == lua_gettop(L)); /* cpcall discard results */
> +	return 0;
> +}
> +
> +int
> +module_sym_load(struct module_sym *mod_sym)
> +{
> +	assert(mod_sym->addr == NULL);
> +
> +	struct func_name_desc d;
> +	parse_func_name(mod_sym->name, &d);
> +
> +	struct module *module = module_cache_find(d.package, d.package_end);
> +	if (module == NULL) {
> +		module = module_load(d.package, d.package_end);
> +		if (module == NULL)
> +			return -1;
> +		if (module_cache_add(module)) {
> +			module_delete(module);
> +			return -1;
> +		}
> +	}
> +
> +	mod_sym->addr = module_sym(module, d.sym);
> +	if (mod_sym->addr == NULL)
> +		return -1;

3. Is it correct, that you just removed the bugfix you did
in the previous commit? Here, if the module was loaded first
time, you again do not unload it.

> +
> +	mod_sym->module = module;
> +	rlist_add(&module->mod_sym_head, &mod_sym->list);
> +	return 0;
> +}
> diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> new file mode 100644
> index 000000000..8b2510ec9
> --- /dev/null
> +++ b/src/box/module_cache.h
> @@ -0,0 +1,158 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#pragma once
> +
> +#include <small/rlist.h>
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +
> +/**
> + * API of C stored function.
> + */
> +
> +struct port;
> +
> +struct box_function_ctx {
> +	struct port *port;
> +};
> +
> +typedef struct box_function_ctx box_function_ctx_t;
> +typedef int (*box_function_f)(box_function_ctx_t *ctx,
> +			      const char *args,
> +			      const char *args_end);
> +
> +/**
> + * Function name descriptor: a symbol and a package.
> + */
> +struct func_name_desc {

4. It is never used out of module_cache.c. Why do you need it
in the header?

> +	/**
> +	 * Null-terminated symbol name, e.g.
> +	 * "func" for "mod.submod.func".
> +	 */
> +	const char *sym;
> +	/**
> +	 * Package name, e.g. "mod.submod" for
> +	 * "mod.submod.func".
> +	 */
> +	const char *package;
> +	/**
> +	 * A pointer to the last character in ->package + 1.
> +	 */
> +	const char *package_end;
> +};
> +
> +/**
> + * Dynamic shared module.
> + */
> +struct module {
> +	/**
> +	 * Module dlhandle.
> +	 */
> +	void *handle;
> +	/**
> +	 * List of associated symbols (functions).
> +	 */
> +	struct rlist mod_sym_head;

5. I see you renamed it from 'funcs'. It was much better, tbh.
I ask you to keep the old name. Or rename it to 'syms', if you like
the 'sym' name so much. But when you named it 'head', you stated
that this is a head of the list, its first element. However it is
not. This rlist is the list itself. If you will call rlist_entry()
on this member, you won't get a valid module_sym object.

Also since you decided to change the comment, it would be better
to say what objects are stored here. Their type.

> +	/**
> +	 * Count of active calls.
> +	 */
> +	size_t calls;
> +	/**
> +	 * Module's package name.
> +	 */
> +	char package[0];
> +};
> +
> +/**
> + * Callable symbol bound to a module.
> + */
> +struct module_sym {
> +	/**
> +	 * Anchor for module membership.
> +	 */
> +	struct rlist list;

6. But it is not a list. It is just a link of the intrusive list.
It is not a self-sufficient list. Also since you changed the
comment anyway, I ask you to say what is the list this
anchor is stored in (module.funcs).

> +	/**
> +	 * For C functions, address of the function.
> +	 */
> +	box_function_f addr;
> +	/**
> +	 * A module the symbol belongs to.
> +	 */
> +	struct module *module;
> +	/**
> +	 * Symbol (function) name definition.
> +	 */
> +	char *name;
> +};
> +
> +/**
> + * Load a new module symbol.
> + *
> + * @param mod_sym symbol to load.
> + *
> + * @returns 0 on succse, -1 otherwise, diag is set.
> + */
> +int
> +module_sym_load(struct module_sym *mod_sym);
> +
> +/**
> + * Unload a module's symbol.
> + *
> + * @param mod_sym symbol to unload.
> + */
> +void
> +module_sym_unload(struct module_sym *mod_sym);
> +
> +/**
> + * Execute a module symbol (run a function).
> + *
> + * The function packs function arguments into a message pack
> + * and send it as a function argument. Function may return
> + * results via @a ret stream.
> + *
> + * @param mod_sym module symbol to run.
> + * @param args function arguments.
> + * @param[out] ret execution results.
> + *
> + * @returns 0 on success, -1 otherwise, diag is set.
> + */
> +int
> +module_sym_call(struct module_sym *mod_sym, struct port *args,
> +		struct port *ret);
> +
> +/**
> + * Reload a module and all associated symbols.
> + *
> + * @param str function name, e.g. "module.submodule.function".
> + * @param[out] d parsed symbol and a package name.

7. There are no such parameters as 'str' and 'd'.

> + *
> + * @return 0 on succes, -1 otherwise, diag is set.
> + */
> +int
> +module_reload(const char *package, const char *package_end,
> +	      struct module **module);
> +
> +/**
> + * Initialize modules subsystem.
> + *
> + * @return 0 on succes, -1 otherwise, diag is set.
> + */
> +int
> +module_init(void);
> +
> +/**
> + * Free modules subsystem.
> + */
> +void
> +module_free(void);
> +
> +#if defined(__cplusplus)
> +}
> +#endif /* defined(__plusplus) */
>