[Tarantool-patches] [PATCH v21 4/6] box/schema.func: switch to new module api

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Since we have low level module api now we can switch
the box.schema.func code to use it. In particular we
define schema_module structure as a wrapper over the
modules api -- it carries a pointer to general module
structure.

Because of modules reload functionality the schema modules
carry own cache of modules instances. Thus to make overall
code somehow close to modules api we do:

1) cache operations are renamed to cache_find/put/update/del;
2) C functions are switched to use module_func low level api;
3) because low level modules api carry own references we can
   take no explicit reference when calling a function.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/func.c     | 550 +++++++++++++++++++--------------------------
 src/box/func.h     |  15 +-
 src/box/func_def.h |  14 --
 3 files changed, 234 insertions(+), 345 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 10b916e6d..2cb111ba6 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -30,19 +30,12 @@
  */
 #include "func.h"
 #include "fiber.h"
-#include "trivia/config.h"
 #include "assoc.h"
-#include "lua/utils.h"
 #include "lua/call.h"
-#include "error.h"
-#include "errinj.h"
 #include "diag.h"
 #include "port.h"
 #include "schema.h"
 #include "session.h"
-#include "libeio/eio.h"
-#include <fcntl.h>
-#include <dlfcn.h>
 
 /**
  * Parsed symbol and package names.
@@ -56,6 +49,19 @@ struct func_name {
 	const char *package_end;
 };
 
+/**
+ * Schema module (box.schema) instance.
+ */
+struct schema_module {
+	/** Low level module instance. */
+	struct module *base;
+	/** List of imported functions. */
+	struct rlist funcs;
+	/** Reference counter. */
+	int64_t refs;
+};
+
+
 struct func_c {
 	/** Function object base class. */
 	struct func base;
@@ -64,16 +70,21 @@ struct func_c {
 	 */
 	struct rlist item;
 	/**
-	 * For C functions, the body of the function.
+	 * C function to call.
 	 */
-	box_function_f func;
+	struct module_func mf;
 	/**
-	 * Each stored function keeps a handle to the
-	 * dynamic library for the C callback.
+	 * A schema module the function belongs to.
 	 */
-	struct module *module;
+	struct schema_module *module;
 };
 
+static void
+schema_module_ref(struct schema_module *module);
+
+static void
+schema_module_unref(struct schema_module *module);
+
 /***
  * Split function name to symbol and package names.
  * For example, str = foo.bar.baz => sym = baz, package = foo.bar
@@ -95,82 +106,9 @@ func_split_name(const char *str, struct func_name *name)
 	}
 }
 
-/**
- * Arguments for luaT_module_find used by lua_cpcall()
- */
-struct module_find_ctx {
-	const char *package;
-	const char *package_end;
-	char *path;
-	size_t path_len;
-};
-
-/**
- * A cpcall() helper for module_find()
- */
-static int
-luaT_module_find(lua_State *L)
-{
-	struct module_find_ctx *ctx = (struct module_find_ctx *)
-		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 path to module using Lua's package.cpath
- * @param package package name
- * @param package_end a pointer to the last byte in @a package + 1
- * @param[out] path path to shared library
- * @param path_len size of @a path buffer
- * @retval 0 on success
- * @retval -1 on error, diag is set
- */
-static int
-module_find(const char *package, const char *package_end, char *path,
-	    size_t path_len)
-{
-	struct module_find_ctx ctx = { package, package_end, path, path_len };
-	lua_State *L = tarantool_L;
-	int top = lua_gettop(L);
-	if (luaT_cpcall(L, luaT_module_find, &ctx) != 0) {
-		int package_len = (int) (package_end - package);
-		diag_set(ClientError, ER_LOAD_MODULE, package_len, package,
-			 lua_tostring(L, -1));
-		lua_settop(L, top);
-		return -1;
-	}
-	assert(top == lua_gettop(L)); /* cpcall discard results */
-	return 0;
-}
-
+/** Schema modules hash. */
 static struct mh_strnptr_t *modules = NULL;
 
-static void
-module_gc(struct module *module);
-
 int
 schema_module_init(void)
 {
@@ -188,240 +126,272 @@ schema_module_free(void)
 {
 	while (mh_size(modules) > 0) {
 		mh_int_t i = mh_first(modules);
-		struct module *module =
-			(struct module *) mh_strnptr_node(modules, i)->val;
-		/* Can't delete modules if they have active calls */
-		module_gc(module);
+		mh_strnptr_del(modules, i, NULL);
 	}
 	mh_strnptr_delete(modules);
+	modules = NULL;
 }
 
 /**
  * Look up a module in the modules cache.
  */
-static struct module *
-module_cache_find(const char *name, const char *name_end)
+static struct schema_module *
+cache_find(const char *name, const char *name_end)
 {
 	mh_int_t i = mh_strnptr_find_inp(modules, name, name_end - name);
 	if (i == mh_end(modules))
 		return NULL;
-	return (struct module *)mh_strnptr_node(modules, i)->val;
+	return mh_strnptr_node(modules, i)->val;
 }
 
 /**
- * Save module to the module cache.
+ * Save a module to the modules cache.
  */
-static inline int
-module_cache_put(struct module *module)
+static int
+cache_put(struct schema_module *module)
 {
-	size_t package_len = strlen(module->package);
-	uint32_t name_hash = mh_strn_hash(module->package, package_len);
+	const char *str = module->base->package;
+	size_t len = module->base->package_len;
+
 	const struct mh_strnptr_node_t strnode = {
-		module->package, package_len, name_hash, module};
+		.str	= str,
+		.len	= len,
+		.hash	= mh_strn_hash(str, len),
+		.val	= module,
+	};
 
-	if (mh_strnptr_put(modules, &strnode, NULL, NULL) == mh_end(modules)) {
+	struct mh_strnptr_node_t prev;
+	struct mh_strnptr_node_t *prev_ptr = &prev;
+
+	if (mh_strnptr_put(modules, &strnode, &prev_ptr, NULL) == mh_end(modules)) {
 		diag_set(OutOfMemory, sizeof(strnode), "malloc", "modules");
 		return -1;
 	}
+
+	/*
+	 * Just to make sure we haven't replaced something, the
+	 * entries must be explicitly deleted.
+	 */
+	assert(prev_ptr == NULL);
 	return 0;
 }
 
 /**
- * Delete a module from the module cache
+ * Update a module in the modules cache.
  */
 static void
-module_cache_del(const char *name, const char *name_end)
+cache_update(struct schema_module *module)
 {
-	mh_int_t i = mh_strnptr_find_inp(modules, name, name_end - name);
+	const char *str = module->base->package;
+	size_t len = module->base->package_len;
+
+	mh_int_t i = mh_strnptr_find_inp(modules, str, len);
 	if (i == mh_end(modules))
-		return;
-	mh_strnptr_del(modules, i, NULL);
+		panic("func: failed to update cache: %s", str);
+
+	mh_strnptr_node(modules, i)->str = str;
+	mh_strnptr_node(modules, i)->val = module;
 }
 
-/*
- * Load a dso.
- * Create a new symlink based on temporary directory and try to
- * load via this symink to load a dso twice for cases of a function
- * reload.
+/**
+ * Delete a module from the module cache.
  */
-static struct module *
-module_load(const char *package, const char *package_end)
+static void
+cache_del(struct schema_module *module)
 {
-	char path[PATH_MAX];
-	if (module_find(package, package_end, path, sizeof(path)) != 0)
-		return NULL;
-
-	int package_len = package_end - package;
-	struct module *module = (struct module *)
-		malloc(sizeof(*module) + package_len + 1);
-	if (module == NULL) {
-		diag_set(OutOfMemory, sizeof(struct module) + package_len + 1,
-			 "malloc", "struct module");
-		return NULL;
-	}
-	memcpy(module->package, package, package_len);
-	module->package[package_len] = 0;
-	rlist_create(&module->funcs);
-	module->calls = 0;
-
-	const char *tmpdir = getenv("TMPDIR");
-	if (tmpdir == NULL)
-		tmpdir = "/tmp";
-	char dir_name[PATH_MAX];
-	int rc = snprintf(dir_name, sizeof(dir_name), "%s/tntXXXXXX", tmpdir);
-	if (rc < 0 || (size_t) rc >= sizeof(dir_name)) {
-		diag_set(SystemError, "failed to generate path to tmp dir");
-		goto error;
+	const char *str = module->base->package;
+	size_t len = module->base->package_len;
+
+	mh_int_t i = mh_strnptr_find_inp(modules, str, len);
+	if (i != mh_end(modules)) {
+		struct schema_module *v;
+		v = mh_strnptr_node(modules, i)->val;
+		/*
+		 * The module may be already reloaded so
+		 * the cache carries a new entry instead.
+		 */
+		if (v == module)
+			mh_strnptr_del(modules, i, NULL);
 	}
+}
 
-	if (mkdtemp(dir_name) == NULL) {
-		diag_set(SystemError, "failed to create unique dir name: %s",
-			 dir_name);
-		goto error;
-	}
-	char load_name[PATH_MAX];
-	rc = snprintf(load_name, sizeof(load_name), "%s/%.*s." TARANTOOL_LIBEXT,
-		      dir_name, package_len, package);
-	if (rc < 0 || (size_t) rc >= sizeof(dir_name)) {
-		diag_set(SystemError, "failed to generate path to DSO");
-		goto error;
-	}
+/** Delete a module. */
+static void
+schema_module_delete(struct schema_module *module)
+{
+	module_unload(module->base);
+	TRASH(module);
+	free(module);
+}
 
-	struct stat st;
-	if (stat(path, &st) < 0) {
-		diag_set(SystemError, "failed to stat() module %s", path);
-		goto error;
-	}
+/** Increment reference to a module. */
+static void
+schema_module_ref(struct schema_module *module)
+{
+	assert(module->refs >= 0);
+	++module->refs;
+}
 
-	int source_fd = open(path, O_RDONLY);
-	if (source_fd < 0) {
-		diag_set(SystemError, "failed to open module %s file for" \
-			 " reading", path);
-		goto error;
-	}
-	int dest_fd = open(load_name, O_WRONLY|O_CREAT|O_TRUNC,
-			   st.st_mode & 0777);
-	if (dest_fd < 0) {
-		diag_set(SystemError, "failed to open file %s for writing ",
-			 load_name);
-		close(source_fd);
-		goto error;
+/** Decrement reference to a module and delete it if last one. */
+static void
+schema_module_unref(struct schema_module *module)
+{
+	assert(module->refs > 0);
+	if (--module->refs == 0) {
+		cache_del(module);
+		schema_module_delete(module);
 	}
+}
 
-	off_t ret = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size);
-	close(source_fd);
-	close(dest_fd);
-	if (ret != st.st_size) {
-		diag_set(SystemError, "failed to copy DSO %s to %s",
-			 path, load_name);
-		goto error;
+static struct schema_module *
+schema_do_module_load(const char *name, size_t len, bool force)
+{
+	struct schema_module *module = malloc(sizeof(*module));
+	if (module != NULL) {
+		module->base = NULL;
+		module->refs = 0;
+		rlist_create(&module->funcs);
+	} else {
+		diag_set(OutOfMemory, sizeof(*module),
+			 "malloc", "struct schema_module");
+		return NULL;
 	}
 
-	module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
-	if (unlink(load_name) != 0)
-		say_warn("failed to unlink dso link %s", load_name);
-	if (rmdir(dir_name) != 0)
-		say_warn("failed to delete temporary dir %s", dir_name);
-	if (module->handle == NULL) {
-		diag_set(ClientError, ER_LOAD_MODULE, package_len,
-			  package, dlerror());
-		goto error;
+	if (force)
+		module->base = module_load_force(name, len);
+	else
+		module->base = module_load(name, len);
+
+	if (module->base == NULL) {
+		free(module);
+		return NULL;
 	}
-	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
-	if (e != NULL)
-		++e->iparam;
+
+	schema_module_ref(module);
 	return module;
-error:
-	free(module);
-	return NULL;
 }
 
-static void
-module_delete(struct module *module)
+/**
+ * Load a new module.
+ */
+static struct schema_module *
+schema_module_load(const char *name, const char *name_end)
 {
-	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
-	if (e != NULL)
-		--e->iparam;
-	dlclose(module->handle);
-	TRASH(module);
-	free(module);
+	return schema_do_module_load(name, name_end - name, false);
 }
 
-/*
- * Check if a dso is unused and can be closed.
+/**
+ * Force load a new module.
  */
+static struct schema_module *
+schema_module_load_force(const char *name, const char *name_end)
+{
+	return schema_do_module_load(name, name_end - name, true);
+}
+
+static struct func_vtab func_c_vtab;
+
+/** Create a new C function. */
 static void
-module_gc(struct module *module)
+func_c_create(struct func_c *func_c)
 {
-	if (rlist_empty(&module->funcs) && module->calls == 0)
-		module_delete(module);
+	func_c->module = NULL;
+	func_c->base.vtab = &func_c_vtab;
+	rlist_create(&func_c->item);
+	module_func_create(&func_c->mf);
 }
 
-/*
- * Import a function from the module.
- */
-static box_function_f
-module_sym(struct module *module, const char *name)
+static int
+func_c_load_from(struct func_c *func, struct schema_module *module,
+		 const char *func_name)
 {
-	box_function_f f = (box_function_f)dlsym(module->handle, name);
-	if (f == NULL) {
-		diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror());
-		return NULL;
+	assert(module_func_is_empty(&func->mf));
+
+	if (module_func_load(module->base, func_name, &func->mf) != 0)
+		return -1;
+
+	func->module = module;
+	rlist_move(&module->funcs, &func->item);
+	schema_module_ref(module);
+	return 0;
+}
+
+static void
+func_c_unload(struct func_c *func)
+{
+	if (!module_func_is_empty(&func->mf)) {
+		rlist_del(&func->item);
+		schema_module_unref(func->module);
+		module_func_unload(&func->mf);
+		func_c_create(func);
 	}
-	return f;
 }
 
 int
 schema_module_reload(const char *package, const char *package_end)
 {
-	struct module *old_module = module_cache_find(package, package_end);
-	if (old_module == NULL) {
+	struct schema_module *old = cache_find(package, package_end);
+	if (old == NULL) {
 		/* Module wasn't loaded - do nothing. */
 		diag_set(ClientError, ER_NO_SUCH_MODULE, package);
 		return -1;
 	}
 
-	struct module *new_module = module_load(package, package_end);
-	if (new_module == NULL)
+	struct schema_module *new = schema_module_load_force(package, package_end);
+	if (new == NULL)
 		return -1;
 
+	/*
+	 * Keep an extra reference to the old module
+	 * so it won't be freed until reload is complete,
+	 * otherwise we might free old module then fail
+	 * on some function loading and in result won't
+	 * be able to restore old symbols.
+	 */
+	schema_module_ref(old);
 	struct func_c *func, *tmp_func;
-	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
-		/* Move immediately for restore sake. */
-		rlist_move(&new_module->funcs, &func->item);
+	rlist_foreach_entry_safe(func, &old->funcs, item, tmp_func) {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(new_module, name.sym);
-		if (func->func == NULL)
-			goto restore;
-		func->module = new_module;
+		func_c_unload(func);
+		if (func_c_load_from(func, new, name.sym) != 0) {
+			/*
+			 * We can restore failing function immediately
+			 * and then all previously migrated ones.
+			 */
+			if (func_c_load_from(func, old, name.sym) != 0)
+				goto restore_fail;
+			else
+				goto restore;
+		}
 	}
-	module_cache_del(package, package_end);
-	if (module_cache_put(new_module) != 0)
-		goto restore;
-	module_gc(old_module);
+	cache_update(new);
+	schema_module_unref(old);
+	schema_module_unref(new);
 	return 0;
 restore:
 	/*
 	 * Some old functions are not found in the new module,
 	 * thus restore all migrated functions back to the original.
 	 */
-	rlist_foreach_entry_safe(func, &new_module->funcs, item, tmp_func) {
+	rlist_foreach_entry_safe(func, &new->funcs, item, tmp_func) {
 		struct func_name name;
 		func_split_name(func->base.def->name, &name);
-		func->func = module_sym(old_module, name.sym);
-		if (func->func == NULL) {
-			/*
-			 * Something strange was happen, an early loaden
-			 * function was not found in an old dso.
-			 */
-			panic("Can't restore module function, "
-			      "server state is inconsistent");
-		}
-		func->module = old_module;
-		rlist_move(&old_module->funcs, &func->item);
+		func_c_unload(func);
+		if (func_c_load_from(func, old, name.sym) != 0)
+			goto restore_fail;
 	}
-	module_delete(new_module);
+	schema_module_unref(old);
+	schema_module_unref(new);
+	return -1;
+
+restore_fail:
+	/*
+	 * Something strange was happen, an early loaden
+	 * function was not found in an old dso.
+	 */
+	panic("Can't restore module function, "
+	      "server state is inconsistent");
 	return -1;
 }
 
@@ -469,8 +439,6 @@ func_new(struct func_def *def)
 	return func;
 }
 
-static struct func_vtab func_c_vtab;
-
 static struct func *
 func_c_new(MAYBE_UNUSED struct func_def *def)
 {
@@ -481,28 +449,10 @@ func_c_new(MAYBE_UNUSED struct func_def *def)
 		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
 		return NULL;
 	}
-	func->base.vtab = &func_c_vtab;
-	func->func = NULL;
-	func->module = NULL;
+	func_c_create(func);
 	return &func->base;
 }
 
-static void
-func_c_unload(struct func_c *func)
-{
-	if (func->module) {
-		rlist_del(&func->item);
-		if (rlist_empty(&func->module->funcs)) {
-			struct func_name name;
-			func_split_name(func->base.def->name, &name);
-			module_cache_del(name.package, name.package_end);
-		}
-		module_gc(func->module);
-	}
-	func->module = NULL;
-	func->func = NULL;
-}
-
 static void
 func_c_destroy(struct func *base)
 {
@@ -521,45 +471,32 @@ func_c_destroy(struct func *base)
 static int
 func_c_load(struct func_c *func)
 {
-	assert(func->func == NULL);
-
 	struct func_name name;
 	func_split_name(func->base.def->name, &name);
 
-	struct module *cached, *module;
-	cached = module_cache_find(name.package, name.package_end);
+	struct schema_module *cached, *module;
+	cached = cache_find(name.package, name.package_end);
 	if (cached == NULL) {
-		module = module_load(name.package, name.package_end);
+		module = schema_module_load(name.package, name.package_end);
 		if (module == NULL)
 			return -1;
-		if (module_cache_put(module)) {
-			module_delete(module);
+		if (cache_put(module)) {
+			schema_module_unref(module);
 			return -1;
 		}
 	} else {
 		module = cached;
+		schema_module_ref(module);
 	}
 
-	func->func = module_sym(module, name.sym);
-	if (func->func == NULL) {
-		if (cached == NULL) {
-			/*
-			 * In case if it was a first load we should
-			 * clean the cache immediately otherwise
-			 * the module continue being referenced even
-			 * if there will be no use of it.
-			 *
-			 * Note the module_sym set an error thus be
-			 * careful to not wipe it.
-			 */
-			module_cache_del(name.package, name.package_end);
-			module_delete(module);
-		}
-		return -1;
-	}
-	func->module = module;
-	rlist_add(&module->funcs, &func->item);
-	return 0;
+	int rc = func_c_load_from(func, module, name.sym);
+	/*
+	 * There is no explicit module loading in this
+	 * interface so each function carries a reference
+	 * by their own.
+	 */
+	schema_module_unref(module);
+	return rc;
 }
 
 int
@@ -568,38 +505,17 @@ func_c_call(struct func *base, struct port *args, struct port *ret)
 	assert(base->vtab == &func_c_vtab);
 	assert(base != NULL && base->def->language == FUNC_LANGUAGE_C);
 	struct func_c *func = (struct func_c *) base;
-	if (func->func == NULL) {
+	if (module_func_is_empty(&func->mf)) {
 		if (func_c_load(func) != 0)
 			return -1;
 	}
-
-	struct region *region = &fiber()->gc;
-	size_t region_svp = region_used(region);
-	uint32_t data_sz;
-	const char *data = port_get_msgpack(args, &data_sz);
-	if (data == NULL)
-		return -1;
-
-	port_c_create(ret);
-	box_function_ctx_t ctx = { ret };
-
-	/* Module can be changed after function reload. */
-	struct module *module = func->module;
-	assert(module != NULL);
-	++module->calls;
-	int rc = func->func(&ctx, data, data + data_sz);
-	--module->calls;
-	module_gc(module);
-	region_truncate(region, region_svp);
-	if (rc != 0) {
-		if (diag_last_error(&fiber()->diag) == NULL) {
-			/* Stored procedure forget to set diag  */
-			diag_set(ClientError, ER_PROC_C, "unknown error");
-		}
-		port_destroy(ret);
-		return -1;
-	}
-	return rc;
+	/*
+	 * Note that we don't take a reference to the
+	 * module, it is handled by low level instance,
+	 * thus while been inside the call the associated
+	 * schema_module can be unreferenced and freed.
+	 */
+	return module_func_call(&func->mf, args, ret);
 }
 
 static struct func_vtab func_c_vtab = {
diff --git a/src/box/func.h b/src/box/func.h
index 5a49e34f4..987ca70db 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -37,6 +37,7 @@
 #include "small/rlist.h"
 #include "func_def.h"
 #include "user_def.h"
+#include "module_cache.h"
 
 #if defined(__cplusplus)
 extern "C" {
@@ -44,20 +45,6 @@ extern "C" {
 
 struct func;
 
-/**
- * Dynamic shared module.
- */
-struct module {
-	/** Module dlhandle. */
-	void *handle;
-	/** List of imported functions. */
-	struct rlist funcs;
-	/** Count of active calls. */
-	size_t calls;
-	/** Module's package name. */
-	char package[0];
-};
-
 /** Virtual method table for func object. */
 struct func_vtab {
 	/** Call function with given arguments. */
diff --git a/src/box/func_def.h b/src/box/func_def.h
index d99d89190..75cd6a0d3 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -168,20 +168,6 @@ func_def_dup(struct func_def *def);
 int
 func_def_check(struct func_def *def);
 
-/**
- * API of C stored function.
- */
-
-struct port;
-
-struct box_function_ctx {
-	struct port *port;
-};
-
-typedef struct box_function_ctx box_function_ctx_t;
-typedef int (*box_function_f)(box_function_ctx_t *ctx,
-	     const char *args, const char *args_end);
-
 #ifdef __cplusplus
 }
 #endif
-- 
2.30.2