Re: [Tarantool-patches] [PATCH v8 1/4] box/func: factor out c function entry structure

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

Hi! Thanks for the patch!

See 3 comments below.

On 14.10.2020 15:35, Cyrill Gorcunov wrote:
> Currently func_c structure is a wrapper over struct func
> which in turn handles function definition, credentials
> and etc, mostly to handle internal "_func" space.
> 
> Such tight bound doesn't allow to reuse func_c in any
> other context. But we're going to support C function
> execution in read-only instances and for this we better
> reuse already existing structures as much as possible
> instead of breeding new ones.
> 
> Thus lets extract module symbols into module_sym structure,
> this allows us to reuse module path cache in other code.
> 
> While extracting the structure rename "func" member to
> "addr" since this exactly what the member represents:
> an address of symbol in memory.
> 
> Same time to be completely independent of "_func" specific lets
> carry symbolic name inplace (ie member "name" is kind of redundant
> when module_sym is used for "_func" handling where we can retrieve
> the name via function definition but such definition won't be
> available for non-persistent C functions which we will support
> in next patches).
> 
> The new structure allows us to load/unload general symbols via
> module_sym_load() and module_sym_unload() helpers.
> 
> Part-of #4642
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/func.c | 156 +++++++++++++++++++++++--------------------------
>  src/box/func.h |  43 ++++++++++++++
>  2 files changed, 116 insertions(+), 83 deletions(-)
> 
> diff --git a/src/box/func.c b/src/box/func.c
> index 8087c953f..75d2abb5f 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -371,6 +361,53 @@ module_sym(struct module *module, const char *name)
>  	return f;
>  }
>  
> +int
> +module_sym_load(struct module_sym *mod_sym)
> +{
> +	assert(mod_sym->addr == NULL);
> +
> +	struct func_name name;
> +	func_split_name(mod_sym->name, &name);
> +
> +	struct module *module = module_cache_find(name.package,
> +						  name.package_end);
> +	if (module == NULL) {
> +		/* Try to find loaded module in the cache */
> +		module = module_load(name.package, name.package_end);
> +		if (module == NULL)
> +			return -1;
> +		if (module_cache_put(module)) {

1. Please, use explicit != 0.
https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style

> +			module_delete(module);
> +			return -1;
> +		}
> +	}
> +
> +	mod_sym->addr = module_sym(module, name.sym);
> +	if (mod_sym->addr == NULL)
> +		return -1;

2. If the module was loaded first time here, it is not unloaded in case of
an error in this place.

> +	mod_sym->module = module;
> +	rlist_add(&module->funcs, &mod_sym->item);
> +	return 0;
> +}
> diff --git a/src/box/func.h b/src/box/func.h
> index 581e468cb..acf2df9a9 100644
> --- a/src/box/func.h
> +++ b/src/box/func.h
> @@ -58,6 +58,29 @@ struct module {
>  	char package[0];
>  };
>  
> +/**
> + * Callable symbol bound to a module.
> + */
> +struct module_sym {
> +	/**
> +	 * Anchor for module membership.
> +	 */
> +	struct rlist item;
> +	/**
> +	 * For C functions, address of the function.
> +	 */
> +	box_function_f addr;
> +	/**
> +	 * Each stored function keeps a handle to the
> +	 * dynamic library for the C callback.
> +	 */

3. Can't parse the comment. What is the 'C callback'?
And why is this function stored? After you extracted it
from struct func_c, it is not related to _func space, and
is not stored.

> +	struct module *module;
> +	/**
> +	 * Function name definition.
> +	 */
> +	char *name;
> +};