Re: [Tarantool-patches] [PATCH v20 5/7] box/schema.func: switch to new module api

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

Thanks for the patch!

See 6 comments below.

On 02.04.2021 14:34, Cyrill Gorcunov via Tarantool-patches wrote:
> Since we have low level module api now we can switch
> the box.schema.func code to use it. In particular we
> define schema_module structure as a wrapper over the
> modules api -- it carries a pointer to general module
> structure.
> 
> Because of modules reload functionality (which was actually
> a big mistake to introduce in a first place, because it is too
> fragile and in case of unintended misuse may simply crash the
> application in a best case) the schema modules carry own cache
> of modules instances. Thus to make overall code somehow close
> to modules api we do:
> 
> 1) every schema_module variable should be named as `mod`. this
>    allows to distinguish this kind of modules from general
>   `module` variables;

1. For base objects we often use name 'base'. So the module_cache
module would be called 'base', and the schema_module would be
called 'module' like it was before your patch. 'mod' and 'module'
don't give any hint how are they different, since you want to
separate them so hard everywhere.

> 2) cache operations are renamed to cache_find/put/update/del;
> 3) C functions are switched to use module_func low level api;
> 4) because low level modules api carry own references we can
>    take no explicit reference when calling a function.
> 
> diff --git a/src/box/func.c b/src/box/func.c
> index 08918e6db..54d0cbc68 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -227,190 +169,214 @@ module_cache_put(struct module *module)

<...>

> +static struct schema_module *
> +__schema_module_load(const char *name, size_t len, bool force)

2. You can't use the double underscore unless it is absolutely
necessary and can't be avoided.
https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html

> +{
> +	struct schema_module *mod = malloc(sizeof(*mod));
> +	if (mod != NULL) {
> +		mod->module = NULL;
> +		mod->refs = 0;
> +		rlist_create(&mod->funcs);
> +	} else {
> +		diag_set(OutOfMemory, sizeof(*mod),
> +			 "malloc", "struct schema_module");
> +		return NULL;
>  	}
>  
> -	module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
> -	if (unlink(load_name) != 0)
> -		say_warn("failed to unlink dso link %s", load_name);
> -	if (rmdir(dir_name) != 0)
> -		say_warn("failed to delete temporary dir %s", dir_name);
> -	if (module->handle == NULL) {
> -		diag_set(ClientError, ER_LOAD_MODULE, package_len,
> -			  package, dlerror());
> -		goto error;
> +	if (force)
> +		mod->module = module_load_force(name, len);
> +	else
> +		mod->module = module_load(name, len);
> +
> +	if (!mod->module) {

3. Please, use explicit == NULL.

> +		free(mod);
> +		return NULL;
>  	}

<...>

> +static int
> +schema_func_c_load(struct schema_module *mod, const char *func_name,
> +		   struct func_c *func)

4. There is a simple rule for method naming and the order of
their arguments: always start the method name with the struct
name, and pass the object in the first argument, like C++.

Here it should be func_c_load_from(), func_c_unload(), and
'func' must be the first argument.

func_c_load_from() so as to emphasize it takes the module to
load from explicitly, on the contrary with func_c_load().

Using the prefix 'schema' for that does not make it easier to
understand the difference between schema_func_c_load and
func_c_load.

>  {
> -	box_function_f f = (box_function_f)dlsym(module->handle, name);
> -	if (f == NULL) {
> -		diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
> -		return NULL;
> +	assert(module_func_is_empty(&func->mf));
> +
> +	if (module_func_load(mod->module, func_name, &func->mf) != 0)
> +		return -1;
> +
> +	func->mod = mod;
> +	rlist_move(&mod->funcs, &func->item);
> +	schema_module_ref(mod);
> +
> +	return 0;
> +}
> +
> +static void
> +schema_func_c_unload(struct func_c *func)
> +{
> +	if (!module_func_is_empty(&func->mf)) {
> +		rlist_del(&func->item);
> +		schema_module_unref(func->mod);
> +		module_func_unload(&func->mf);
> +		func_c_create(func);
>  	}
> -	return f;
>  }
>  
>  int
>  schema_module_reload(const char *package, const char *package_end)
>  {
> -	struct module *old_module = module_cache_find(package, package_end);
> -	if (old_module == NULL) {
> +	struct schema_module *old = cache_find(package, package_end);
> +	if (old == NULL) {
>  		/* Module wasn't loaded - do nothing. */
>  		diag_set(ClientError, ER_NO_SUCH_MODULE, package);
>  		return -1;
>  	}
>  
> -	struct module *new_module = module_load(package, package_end);
> -	if (new_module == NULL)
> +	struct schema_module *new = schema_module_load_force(package, package_end);
> +	if (new == NULL)
>  		return -1;
>  
> +	/*
> +	 * Keep an extra reference to the old module
> +	 * so it won't be freed until reload is complete,
> +	 * otherwise we might free old module then fail
> +	 * on some function loading and in result won't
> +	 * be able to restore old symbols.
> +	 */
> +	schema_module_ref(old);
>  	struct func_c *func, *tmp_func;
> -	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
> -		/* Move immediately for restore sake. */
> -		rlist_move(&new_module->funcs, &func->item);
> +	rlist_foreach_entry_safe(func, &old->funcs, item, tmp_func) {
>  		struct func_name name;
>  		func_split_name(func->base.def->name, &name);
> -		func->func = module_sym(new_module, name.sym);
> -		if (func->func == NULL)
> +		schema_func_c_unload(func);
> +		if (schema_func_c_load(new, name.sym, func) != 0) {
> +			/*
> +			 * WARN: A hack accessing new->funcs directly
> +			 * to start restore from this failing function.
> +			 */

5. Please, drop the hack. You can restore this function individually
right here, and proceed to the restoration of the other functions.

There might be other solutions, but this looks the easiest.

> +			rlist_move(&new->funcs, &func->item);
>  			goto restore;
> -		func->module = new_module;
> +		}
>  	}
> diff --git a/test/box/gh-4648-func-load-unload.result b/test/box/gh-4648-func-load-unload.result
> index e695dd365..91b78d083 100644
> --- a/test/box/gh-4648-func-load-unload.result
> +++ b/test/box/gh-4648-func-load-unload.result
> @@ -71,7 +71,8 @@ check_module_count_diff(-1)
>   | ...
>  
>  -- A not finished invocation of a function from a module prevents
> --- its unload. Until the call is finished.
> +-- low level module intance unload while schema level module is
> +-- free to unload immediately when dropped.

6. As you can see from the issue description in this test, it was
exactly about unloading the dlopen's handle. Not about the module
object deletion. After your change, even if the modules are never
truly unloaded, the test will pass which is obviously not what is
supposed to happen. You need to move the error injection to
module_cache.c to make it work like before and fail if the module
wasn't fully unloaded.