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

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

Hi! Thanks for the patch!

See 5 comments below.

> diff --git a/src/box/module_cache.c b/src/box/module_cache.c
> new file mode 100644
> index 000000000..a44f3d110
> --- /dev/null
> +++ b/src/box/module_cache.c
> @@ -0,0 +1,503 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, 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"
> +#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;
> +
> +/**
> + * Parse function descriptor from a string.
> + */
> +void
> +parse_func_name(const char *str, struct func_name_desc *d)

1. The function is not used out of this file. Please, make it
static.

> +{
> +	d->package = str;
> +	d->package_end = strrchr(str, '.');
> +	if (d->package_end != NULL) {
> +		/* module.submodule.function => module.submodule, function */
> +		d->sym = d->package_end + 1; /* skip '.' */
> +	} else {
> +		/* package == function => function, function */
> +		d->sym = d->package;
> +		d->package_end = str + strlen(str);
> +	}
> +}
> +
> +/**
> + * A cpcall() helper for module_find().
> + */
> +static int
> +luaT_module_find(lua_State *L)
> +{
> +	struct module_find_ctx *ctx = (void *)lua_topointer(L, 1);
> +
> +	/*
> +	 * Call package.searchpath(name, package.cpath) and use
> +	 * the path to the function in dlopen().
> +	 */
> +	lua_getglobal(L, "package");
> +	lua_getfield(L, -1, "search");
> +
> +	/* Argument of search: name */
> +	lua_pushlstring(L, ctx->package, ctx->package_end - ctx->package);
> +
> +	lua_call(L, 1, 1);
> +	if (lua_isnil(L, -1))
> +		return luaL_error(L, "module not found");
> +
> +	/* Convert path to absolute */
> +	char resolved[PATH_MAX];
> +	if (realpath(lua_tostring(L, -1), resolved) == NULL) {
> +		diag_set(SystemError, "realpath");
> +		return luaT_error(L);
> +	}
> +
> +	snprintf(ctx->path, ctx->path_len, "%s", resolved);
> +	return 0;
> +}
> +
> +/**
> + * Find a path to a module using Lua's package.cpath.
> + *
> + * @param ctx module find context
> + * @param[out] ctx->path path to shared library.
> + * @param[out] ctx->path_len size of @a ctx->path buffer.

2. The last two lines are not params. The only parameter is ctx. If
you want to describe it, it should be done under the same @param.
Otherwise the comment looks mismatching the signature. I gonna say
that again. I don't force to use Doxygen, but if you decide to use it,
it should be done correctly.

> + *
> + * @return 0 on succes, -1 otherwise, diag is set.
> + */
> +static int
> +module_find(struct module_find_ctx *ctx)
> diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> new file mode 100644
> index 000000000..3f275024a
> --- /dev/null
> +++ b/src/box/module_cache.h
> @@ -0,0 +1,156 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#pragma once
> +
> +#include <small/rlist.h>
> +
> +#include "func_def.h"

3. Func_def heavily depends on schema. Also it is a part of
all the _func handling, meaning this is a cyclic dependency now -
module_cache depends on func and vice versa.

> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +/**
> + * Function name descriptor: a symbol and a package.
> + */
> +struct func_name_desc {
> +	/**
> +	 * Null-terminated symbol name, e.g.
> +	 * "func" for "mod.submod.func".
> +	 */
> +	const char *sym;
> +	/**
> +	 * Package name, e.g. "mod.submod" for
> +	 * "mod.submod.func".
> +	 */
> +	const char *package;
> +	/**
> +	 * A pointer to the last character in ->package + 1.
> +	 */
> +	const char *package_end;
> +};
> +
> +/***
> + * Parse function name into a name descriptor.
> + * 

4. Trailing whitespace.

> + * For example, str = foo.bar.baz => sym = baz, package = foo.bar
> + *
> + * @param str function name, e.g. "module.submodule.function".
> + * @param[out] d parsed symbol and a package name.
> + */
> +void
> +parse_func_name(const char *str, struct func_name_desc *d);
> +
> +/**
> + * Load a new module symbol.
> + *
> + * @param mod_sym symbol to load.
> + *
> + * @returns 0 on succse, -1 otherwise, diag is set.
> + */

5. I see you added comments for each function twice - in its
declaration and implementation. Please, don't. Such double
comments usually quicky get desynchronized. Even now they are
not the same. We always comment functions on their declaration,
if it is separated from implementation.

The same for the other commits and other places in this commit.

> +int
> +module_sym_load(struct module_sym *mod_sym);