[tarantool-patches] Re: [PATCH v1 1/1] box: support reload whole module
- From: Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx>
- To: tarantool-patches@xxxxxxxxxxxxx, Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
- Date: Thu, 12 Jul 2018 11:27:13 +0300
During our verbal discussion with Kirill, he noticed that the fact that
box.schema.func.reload() reloads the whole module when it is passed a
function name is rather confusing: the user may not know that and call
box.schema.func.reload() once per each used function, in which case he
will effectively reload the whole module that contains those functions
multiple times, which is pointless. Turns out Kirill isn't the only one
who finds such an API weird, see
https://github.com/tarantool/tarantool/issues/910#issuecomment-331435227
I talked to Kostja and he doesn't mind banning this API, i.e. making
box.schema.func.reload() interpret its argument only as a module name.
So let's rework this patch so that box_func_reload() directly calls
module_reload(), and remove func_reload() altogether. As for the access
checks, box_func_reload() should work only if the caller has the global
execute privilege.
Closes #2946.
@TarantoolBot document
Title: fixed module reload
There was a bug in tarantool documentation:
https://tarantool.io/en/doc/1.7/book/box/
box_schema/#lua-function.box.schema.func.reload
Now it is allowed to reload all functions in loadable
module via one method. Legacy method including finction
name is forbidden.
box.schema.func.reload("utils") -- ok since now
box.schema.func.reload("utils.func1") -- forbidden since now
Global reload is still unsupported because it seems
to be useless.
box.schema.func.reload() -- invalid!
---
https://github.com/tarantool/tarantool/compare/kshch/gh-2946-module-reload
https://github.com/tarantool/tarantool/issues/2946
src/box/call.c | 23 +++++++++++++----------
src/box/call.h | 9 ++++++++-
src/box/errcode.h | 1 +
src/box/func.c | 16 ----------------
src/box/func.h | 11 ++++++++++-
src/box/lua/call.c | 6 +++---
src/box/lua/schema.lua | 2 +-
test/box/func_reload.result | 23 ++++++++---------------
test/box/func_reload.test.lua | 14 ++++++--------
test/box/misc.result | 1 +
10 files changed, 51 insertions(+), 55 deletions(-)
diff --git a/src/box/call.c b/src/box/call.c
index 438be19..b9750c5 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -132,20 +132,23 @@ box_c_call(struct func *func, struct call_request
*request, struct port *port)
}
int
-box_func_reload(const char *name)
+box_module_reload(const char *name)
{
- size_t name_len = strlen(name);
- struct func *func = NULL;
- if ((access_check_func(name, name_len, &func)) != 0)
- return -1;
- if (func == NULL) {
- diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
+ struct credentials *credentials = effective_user();
+ if ((credentials->universal_access & (PRIV_X | PRIV_U)) !=
+ (PRIV_X | PRIV_U)) {
+ struct user *user = user_find(credentials->uid);
+ if (user != NULL)
+ diag_set(AccessDeniedError, priv_name(PRIV_U),
+ schema_object_name(SC_UNIVERSE), "",
+ user->def->name);
return -1;
}
- if (func->def->language != FUNC_LANGUAGE_C || func->func == NULL)
- return 0; /* Nothing to do */
- if (func_reload(func) == 0)
+ struct module *module = NULL;
+ if (module_reload(name, name + strlen(name), &module) == 0 &&
+ module != NULL)
return 0;
+ diag_set(ClientError, ER_NO_SUCH_MODULE, name);
return -1;
}
diff --git a/src/box/call.h b/src/box/call.h
index eabba69..1b54551 100644
--- a/src/box/call.h
+++ b/src/box/call.h
@@ -42,8 +42,15 @@ struct box_function_ctx {
struct port *port;
};
+/**
+ * Reload loadable module by name.
+ *
+ * @param name of the module to reload.
+ * @retval -1 on error.
+ * @retval 0 on success.
+ */
int
-box_func_reload(const char *name);
+box_module_reload(const char *name);
int
box_process_call(struct call_request *request, struct port *port);
diff --git a/src/box/errcode.h b/src/box/errcode.h
index c76018c..64f9af1 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -215,6 +215,7 @@ struct errcode_record {
/*160 */_(ER_ACTION_MISMATCH, "Field %d contains %s on
conflict action, but %s in index parts") \
/*161 */_(ER_VIEW_MISSING_SQL, "Space declared as a view must
have SQL statement") \
/*162 */_(ER_FOREIGN_KEY_CONSTRAINT, "Can not commit transaction:
deferred foreign keys violations are not resolved") \
+ /*163 */_(ER_NO_SUCH_MODULE, "Module '%s' does not exist") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/func.c b/src/box/func.c
index dfbc5f3..a817851 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -298,9 +298,6 @@ module_sym(struct module *module, const char *name)
return f;
}
-/*
- * Reload a dso.
- */
int
module_reload(const char *package, const char *package_end, struct module
**module)
{
@@ -436,19 +433,6 @@ func_load(struct func *func)
}
int
-func_reload(struct func *func)
-{
- struct func_name name;
- func_split_name(func->def->name, &name);
- struct module *module = NULL;
- if (module_reload(name.package, name.package_end, &module) != 0) {
- diag_log();
- return -1;
- }
- return 0;
-}
-
-int
func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
const char *args_end)
{
diff --git a/src/box/func.h b/src/box/func.h
index 0957546..8dcd61d 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -113,8 +113,17 @@ int
func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
const char *args_end);
+/**
+ * Reload dynamically loadable module.
+ *
+ * @param package name begin pointer.
+ * @param package_end package_end name end pointer.
+ * @param[out] module a pointer to store module object on success.
+ * @retval -1 on error.
+ * @retval 0 on success.
+ */
int
-func_reload(struct func *func);
+module_reload(const char *package, const char *package_end, struct module
**module);
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 8f3461a..d20cbbb 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -458,10 +458,10 @@ box_lua_eval(struct call_request *request, struct port
*port)
}
static int
-lbox_func_reload(lua_State *L)
+lbox_module_reload(lua_State *L)
{
const char *name = luaL_checkstring(L, 1);
- if (box_func_reload(name) != 0)
+ if (box_module_reload(name) != 0)
return luaT_error(L);
return 0;
}
@@ -469,7 +469,7 @@ lbox_func_reload(lua_State *L)
static const struct luaL_Reg boxlib_internal[] = {
{"call_loadproc", lbox_call_loadproc},
{"sql_create_function", lbox_sql_create_function},
- {"func_reload", lbox_func_reload},
+ {"module_reload", lbox_module_reload},
{NULL, NULL}
};
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 87c79bd..ab8072c 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1951,7 +1951,7 @@ function box.schema.func.exists(name_or_id)
return tuple ~= nil
end
-box.schema.func.reload = internal.func_reload
+box.schema.func.reload = internal.module_reload
box.internal.collation = {}
box.internal.collation.create = function(name, coll_type, locale, opts)
diff --git a/test/box/func_reload.result b/test/box/func_reload.result
index 5aeb85c..b024cd1 100644
--- a/test/box/func_reload.result
+++ b/test/box/func_reload.result
@@ -51,8 +51,9 @@ fio.symlink(reload1_path, reload_path)
- true
...
--check not fail on non-load func
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
---
+- error: Module 'reload' does not exist
...
-- test of usual case reload. No hanging calls
box.space.test:insert{0}
@@ -74,7 +75,7 @@ fio.symlink(reload2_path, reload_path)
---
- true
...
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
---
...
c:call("reload.foo")
@@ -98,7 +99,7 @@ fio.symlink(reload1_path, reload_path)
---
- true
...
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
---
...
fibers = 10
@@ -111,7 +112,7 @@ while box.space.test:count() < fibers do fiber.sleep(0.001)
end
---
...
-- double reload doesn't fail waiting functions
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
---
...
_ = fio.unlink(reload_path)
@@ -121,7 +122,7 @@ fio.symlink(reload2_path, reload_path)
---
- true
...
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
---
...
c:call("reload.foo")
@@ -208,7 +209,7 @@ _ = fiber.create(function()
ch:put(c:call("reload.test_reload")) end)
while s:get({1}) == nil do fiber.yield(0.0001) end
---
...
-box.schema.func.reload("reload.test_reload")
+box.schema.func.reload("reload")
---
...
_ = fiber.create(function() ch:put(c:call("reload.test_reload")) end)
@@ -242,14 +243,6 @@ fio.symlink(reload1_path, reload_path)
---
- true
...
-s, e = pcall(box.schema.func.reload, "reload.test_reload")
----
-...
-s, string.find(tostring(e), 'test_reload_fail') ~= nil
----
-- false
-- true
-...
c:call("reload.test_reload")
---
- [[2]]
@@ -273,5 +266,5 @@ box.schema.func.reload()
...
box.schema.func.reload("non-existing")
---
-- error: Function 'non-existing' does not exist
+- error: Module 'non-existing' does not exist
...
diff --git a/test/box/func_reload.test.lua b/test/box/func_reload.test.lua
index dc56d84..8906898 100644
--- a/test/box/func_reload.test.lua
+++ b/test/box/func_reload.test.lua
@@ -20,7 +20,7 @@ _ = fio.unlink(reload_path)
fio.symlink(reload1_path, reload_path)
--check not fail on non-load func
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
-- test of usual case reload. No hanging calls
box.space.test:insert{0}
@@ -29,7 +29,7 @@ box.space.test:delete{0}
_ = fio.unlink(reload_path)
fio.symlink(reload2_path, reload_path)
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
c:call("reload.foo")
box.space.test:select{}
box.space.test:truncate()
@@ -37,18 +37,18 @@ box.space.test:truncate()
-- test case with hanging calls
_ = fio.unlink(reload_path)
fio.symlink(reload1_path, reload_path)
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
fibers = 10
for i = 1, fibers do fiber.create(function() c:call("reload.foo", {i}) end) end
while box.space.test:count() < fibers do fiber.sleep(0.001) end
-- double reload doesn't fail waiting functions
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
_ = fio.unlink(reload_path)
fio.symlink(reload2_path, reload_path)
-box.schema.func.reload("reload.foo")
+box.schema.func.reload("reload")
c:call("reload.foo")
while box.space.test:count() < 2 * fibers + 1 do fiber.sleep(0.001) end
@@ -71,7 +71,7 @@ _ = fio.unlink(reload_path)
fio.symlink(reload2_path, reload_path)
_ = fiber.create(function() ch:put(c:call("reload.test_reload")) end)
while s:get({1}) == nil do fiber.yield(0.0001) end
-box.schema.func.reload("reload.test_reload")
+box.schema.func.reload("reload")
_ = fiber.create(function() ch:put(c:call("reload.test_reload")) end)
ch:get()
ch:get()
@@ -82,8 +82,6 @@ box.schema.user.grant('guest', 'execute', 'function',
'reload.test_reload_fail')
c:call("reload.test_reload_fail")
_ = fio.unlink(reload_path)
fio.symlink(reload1_path, reload_path)
-s, e = pcall(box.schema.func.reload, "reload.test_reload")
-s, string.find(tostring(e), 'test_reload_fail') ~= nil
c:call("reload.test_reload")
c:call("reload.test_reload_fail")
diff --git a/test/box/misc.result b/test/box/misc.result
index a00d033..777b40b 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -487,6 +487,7 @@ t;
160: box.error.ACTION_MISMATCH
161: box.error.VIEW_MISSING_SQL
162: box.error.FOREIGN_KEY_CONSTRAINT
+ 163: box.error.NO_SUCH_MODULE
...
test_run:cmd("setopt delimiter ''");
---
--
2.7.4
Other related posts:
