From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v1 1/1] box: support reload whole module References: <8846b566-24c5-9f11-ca29-18baed551921@tarantool.org> <20180711124656.pzuh254bjku3k2xc@esperanza> <20180711155705.zkwqen2q5aeozzkq@esperanza> From: Kirill Shcherbatov Message-ID: <0c098b97-27f4-bd1f-23a0-6b136361d16f@tarantool.org> Date: Thu, 12 Jul 2018 11:27:13 +0300 MIME-Version: 1.0 In-Reply-To: <20180711155705.zkwqen2q5aeozzkq@esperanza> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov List-ID: > 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