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

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

Thanks for the patch!

See 4 comments below.

On 18.01.2021 21:35, 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).

1. It is now cmod, not cbox.

> For this sake all module related code is moved to
> module_cache file where we do symbol resolving, calling
> and tracking of symbol usage.
> 
> Part-of #4642
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> new file mode 100644
> index 000000000..9fe316807
> --- /dev/null
> +++ b/src/box/module_cache.c
> @@ -0,0 +1,513 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <dlfcn.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "assoc.h"
> +#include "diag.h"
> +#include "error.h"
> +#include "errinj.h"
> +#include "fiber.h"
> +#include "port.h"
> +
> +#include "box/error.h"

2. #include "error.h" and #include "box/error.h" are the same
files. Because you are in box/ folder.

> +#include "lua/utils.h"
> +#include "libeio/eio.h"
> +
> +#include "module_cache.h"
> +
> +/** Modules name to descriptor hash. */
> +static struct mh_strnptr_t *mod_hash = NULL;
> +
> +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) {
> +		module = module_load(name.package, name.package_end);
> +		if (module == NULL)
> +			return -1;
> +		if (module_cache_add(module) != 0) {
> +			module_delete(module);
> +			return -1;
> +		}
> +	}
> +
> +	mod_sym->addr = module_sym(module, name.sym);
> +	if (mod_sym->addr == NULL)
> +		return -1;

3. Is it correct, that you second time has deleted the bugfix
which you did about module not being unloaded when first
symbol load fails?

> +
> +	mod_sym->module = module;
> +	rlist_add(&module->funcs_list, &mod_sym->item);
> +	return 0;
> +}
> diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> new file mode 100644
> index 000000000..fd789f603
> --- /dev/null
> +++ b/src/box/module_cache.h
> @@ -0,0 +1,139 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#pragma once
> +
> +#include <small/rlist.h>

4. Please, use "" instead of <> for non-system headers.