[tarantool-patches] Re: [PATCH v1 1/1] box: support reload whole module

[Georgy Kirichenko <georgy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 8446 bytes --]

Ok for me

On Tuesday, July 10, 2018 9:49:14 AM MSK Kirill Shcherbatov wrote:
> Tnx for comment; I've changed commit message and fixed few
> codestyle things which I missed yesterday.
> 
> 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.
> box.schema.func.reload("utils")    -- ok 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                | 37 ++++++++++++++++---------------------
>  src/box/call.h                | 18 ++++++++++++++++++
>  src/box/func.c                | 20 +++++++++++++++++---
>  src/box/func.h                | 15 +++++++++++++++
>  test/box/func_reload.result   |  7 +++++++
>  test/box/func_reload.test.lua |  2 ++
>  6 files changed, 75 insertions(+), 24 deletions(-)
> 
> diff --git a/src/box/call.c b/src/box/call.c
> index 438be19..ef30abc 100644
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -42,20 +42,8 @@
>  #include "rmean.h"
>  #include "small/obuf.h"
> 
> -/**
> - * Find a function by name and check "EXECUTE" permissions.
> - *
> - * @param name function name
> - * @param name_len length of @a name
> - * @param[out] funcp function object
> - * Sic: *pfunc == NULL means that perhaps the user has a global
> - * "EXECUTE" privilege, so no specific grant to a function.
> - *
> - * @retval -1 on access denied
> - * @retval  0 on success
> - */
> -static inline int
> -access_check_func(const char *name, uint32_t name_len, struct func **funcp)
> +int
> +box_func_check_access(const char *name, uint32_t name_len, struct func
> **funcp) {
>  	struct func *func = func_by_name(name, name_len);
>  	struct credentials *credentials = effective_user();
> @@ -136,17 +124,24 @@ box_func_reload(const char *name)
>  {
>  	size_t name_len = strlen(name);
>  	struct func *func = NULL;
> -	if ((access_check_func(name, name_len, &func)) != 0)
> +	if ((box_func_check_access(name, name_len, &func)) != 0)
>  		return -1;
>  	if (func == NULL) {
> +		/* Try to interpret name as a module name. */
> +		struct module *module;
> +		if (module_reload(name, name + name_len, &module, true) == 0 &&
> +		    module != NULL)
> +			return 0;
>  		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
>  		return -1;
> +	} else {
> +		/* Such function exists. Try to reload it. */
> +		if (func->def->language != FUNC_LANGUAGE_C || func->func == NULL)
> +			return 0;
> +		if (func_reload(func) == 0)
> +			return 0;
> +		return -1;
>  	}
> -	if (func->def->language != FUNC_LANGUAGE_C || func->func == NULL)
> -		return 0; /* Nothing to do */
> -	if (func_reload(func) == 0)
> -		return 0;
> -	return -1;
>  }
> 
>  int
> @@ -165,7 +160,7 @@ box_process_call(struct call_request *request, struct
> port *port) * Sic: func == NULL means that perhaps the user has a global
>  	 * "EXECUTE" privilege, so no specific grant to a function.
>  	 */
> -	if (access_check_func(name, name_len, &func) != 0)
> +	if (box_func_check_access(name, name_len, &func) != 0)
>  		return -1; /* permission denied */
> 
>  	/**
> diff --git a/src/box/call.h b/src/box/call.h
> index eabba69..55cf925 100644
> --- a/src/box/call.h
> +++ b/src/box/call.h
> @@ -35,8 +35,11 @@
>  extern "C" {
>  #endif /* defined(__cplusplus) */
> 
> +#include <inttypes.h>
> +
>  struct port;
>  struct call_request;
> +struct func;
> 
>  struct box_function_ctx {
>  	struct port *port;
> @@ -51,6 +54,21 @@ box_process_call(struct call_request *request, struct
> port *port); int
>  box_process_eval(struct call_request *request, struct port *port);
> 
> +/**
> + * Find a function by name and check "EXECUTE" permissions.
> + *
> + * @param name function name
> + * @param name_len length of @a name
> + * @param[out] funcp function object
> + * Sic: *pfunc == NULL means that perhaps the user has a global
> + * "EXECUTE" privilege, so no specific grant to a function.
> + *
> + * @retval -1 on access denied.
> + * @retval  0 on success.
> + */
> +int
> +box_func_check_access(const char *name, uint32_t name_len, struct func
> **funcp); +
>  #if defined(__cplusplus)
>  } /* extern "C" */
>  #endif /* defined(__cplusplus) */
> diff --git a/src/box/func.c b/src/box/func.c
> index dfbc5f3..245dbe7 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -34,6 +34,7 @@
>  #include "lua/utils.h"
>  #include "error.h"
>  #include "diag.h"
> +#include "call.h"
>  #include <dlfcn.h>
> 
>  /**
> @@ -302,7 +303,8 @@ module_sym(struct module *module, const char *name)
>   * Reload a dso.
>   */
>  int
> -module_reload(const char *package, const char *package_end, struct module
> **module) +module_reload(const char *package, const char *package_end,
> +	      struct module **module, bool check_access)
>  {
>  	struct module *old_module = module_cache_find(package, package_end);
>  	if (old_module == NULL) {
> @@ -318,7 +320,19 @@ module_reload(const char *package, const char
> *package_end, struct module **modu struct func *func, *tmp_func;
>  	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
>  		struct func_name name;
> -		func_split_name(func->def->name, &name);
> +		const char *func_name = func->def->name;
> +		func_split_name(func_name, &name);
> +
> +		/*
> +		 * Allow to reload only functions that belongs to
> +		 * current user. Skip other.
> +		 */
> +		struct func *dummy;
> +		if (check_access &&
> +		    box_func_check_access(func_name, strlen(func_name),
> +					  &dummy) != 0)
> +			continue;
> +
>  		func->func = module_sym(new_module, name.sym);
>  		if (func->func == NULL)
>  			goto restore;
> @@ -441,7 +455,7 @@ 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) {
> +	if (module_reload(name.package, name.package_end, &module, false) != 0) {
>  		diag_log();
>  		return -1;
>  	}
> diff --git a/src/box/func.h b/src/box/func.h
> index 0957546..168c2fc 100644
> --- a/src/box/func.h
> +++ b/src/box/func.h
> @@ -116,6 +116,21 @@ func_call(struct func *func, box_function_ctx_t *ctx,
> const char *args, int
>  func_reload(struct func *func);
> 
> +/**
> + * Reload dynamically loadable module.
> + *
> + * @param package name begin pointer.
> + * @param package_end name end pointer.
> + * @param[out] module a pointer to store module object on success.
> + * @param check_access do ACL checks if specified.
> + *
> + * @retval -1 on error
> + * @retval 0 on success
> + */
> +int
> +module_reload(const char *package, const char *package_end,
> +	      struct module **module, bool check_access);
> +
>  #if defined(__cplusplus)
>  } /* extern "C" */
>  #endif /* defined(__cplusplus) */
> diff --git a/test/box/func_reload.result b/test/box/func_reload.result
> index 5aeb85c..9c05555 100644
> --- a/test/box/func_reload.result
> +++ b/test/box/func_reload.result
> @@ -54,6 +54,10 @@ fio.symlink(reload1_path, reload_path)
>  box.schema.func.reload("reload.foo")
>  ---
>  ...
> +box.schema.func.reload("reload")
> +---
> +- error: Function 'reload' does not exist
> +...
>  -- test of usual case reload. No hanging calls
>  box.space.test:insert{0}
>  ---
> @@ -77,6 +81,9 @@ fio.symlink(reload2_path, reload_path)
>  box.schema.func.reload("reload.foo")
>  ---
>  ...
> +box.schema.func.reload("reload")
> +---
> +...
>  c:call("reload.foo")
>  ---
>  - []
> diff --git a/test/box/func_reload.test.lua b/test/box/func_reload.test.lua
> index dc56d84..5569dcd 100644
> --- a/test/box/func_reload.test.lua
> +++ b/test/box/func_reload.test.lua
> @@ -21,6 +21,7 @@ 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}
> @@ -30,6 +31,7 @@ _ = 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()


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]