Re: [Tarantool-patches] [PATCH v19 2/6] box/func: prepare for transition to modules subsystem

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

Hi! Thanks for the patch!

See 4 comments below.

> diff --git a/src/box/call.c b/src/box/call.c
> index 7839e1f3e..4b1893f12 100644
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -116,7 +116,7 @@ static const struct port_vtab port_msgpack_vtab = {
>  };
>  
>  int
> -box_module_reload(const char *name)
> +box_process_module_reload(const char *name)

1. The name is ugly. I propose to merge this function into
box_module_reload(). The latter is never used from anywhere
except this 'process' thing.

>  {
>  	struct credentials *credentials = effective_user();
>  	if ((credentials->universal_access & (PRIV_X | PRIV_U)) !=
> diff --git a/src/box/func.c b/src/box/func.c
> index 233696a4f..88903f40e 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -71,7 +71,7 @@ struct func_c {
>  	 * Each stored function keeps a handle to the
>  	 * dynamic library for the C callback.
>  	 */
> -	struct module *module;
> +	struct box_module *bxmod;

2. What was wrong with the old member name? Especially in
the functions. It just significantly increases the diff, ruins
the history, and does not change readability anyhow. The old
name was even better IMO.

>  };
>  
>  /***
> @@ -166,61 +166,65 @@ module_find(const char *package, const char *package_end, char *path,
>  	return 0;
>  }
>  
> -static struct mh_strnptr_t *modules = NULL;
> +static struct mh_strnptr_t *box_module_hash = NULL;

3. The same question. 'modules' was just fine. Diff is
significantly bigger because most of such renames are not
needed at all. You only need to rename the struct. It was
module, and stayed module. Just a new prefix is added to
the struct name and its methods.

> @@ -484,23 +488,23 @@ func_c_new(MAYBE_UNUSED struct func_def *def)
>  	}
>  	func->base.vtab = &func_c_vtab;
>  	func->func = NULL;
> -	func->module = NULL;
> +	func->bxmod = NULL;
>  	return &func->base;
>  }
>  
>  static void
>  func_c_unload(struct func_c *func)
>  {
> -	if (func->module) {
> +	if (func->bxmod) {

4. Since you changed this line, it should be != NULL now. But even better -
keep the old variable and struct member names. Then you wouldn't need to
change almost anything in this file except struct names.

>  		rlist_del(&func->item);
> -		if (rlist_empty(&func->module->funcs)) {
> +		if (rlist_empty(&func->bxmod->funcs)) {
>  			struct func_name name;
>  			func_split_name(func->base.def->name, &name);
> -			module_cache_del(name.package, name.package_end);
> +			cache_del(name.package, name.package_end);
>  		}
> -		module_gc(func->module);
> +		box_module_gc(func->bxmod);
>  	}
> -	func->module = NULL;
> +	func->bxmod = NULL;
>  	func->func = NULL;
>  }