Re: [Tarantool-patches] [PATCH v12 6/8] module_cache: provide helpers to load and unload modules

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

Thanks for the patch!

> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> index ae874caab..cb580f342 100644
> --- a/src/box/module_cache.c
> +++ b/src/box/module_cache.c
> @@ -437,6 +431,34 @@ module_cache_update(const char *name, const char *name_end,
>  	return 0;
>  }
>  
> +/**
> + * Load a new module.
> + */
> +struct module *
> +module_load(const char *path, size_t path_len)
> +{
> +	struct module *module = module_cache_find(path, &path[path_len]);
> +	if (module == NULL) {
> +		module = module_dlopen(path, &path[path_len]);
> +		if (module == NULL)
> +			return NULL;
> +		if (module_cache_add(module) != 0) {
> +			module_delete(module);
> +			return NULL;
> +		}
> +	}
> +	return module;
> +}
> +
> +/**
> + * Unload a module.
> + */
> +void
> +module_unload(struct module *module)
> +{
> +	module_gc(module);

From what I see in the main commit, you should increase
the reference counter in load and decrease it in unload.

When you do that - you get another issue: --ref + module_gc() become
a pattern. So it makes sense to move them to a function module_unref().

Also it looks super strange, that the module deletion from the cache
is done in module_sym_unload(). I don't think it should change anything
except its mod_sym argument.

Another issue - module.funcs_list is an implicit reference counter,
which is extra weird. The function list should only be used for
reloads. Each function should increase the counter, so it couldn't happen
that the ref counter is 0, but there are functions keeping a pointer at
the module.

As you can see there is really a pile of issues with the current
reference counting and deletion of modules. Maybe better fix all of
them while you are here. At least the issue in your main patch about
touching ref counter manually.