Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [PATCH v2 9/9] box: export _func functions with box.func folder
Date: Mon, 10 Jun 2019 18:18:46 +0300	[thread overview]
Message-ID: <20190610151846.4azti2a2ob5jur6e@esperanza> (raw)
In-Reply-To: <c0b80ad33ebcd3eacb4f231f7e40353df57cf408.1559822429.git.kshcherbatov@tarantool.org>

On Thu, Jun 06, 2019 at 03:04:05PM +0300, Kirill Shcherbatov wrote:
> Closes #4182
> Needed for #1260
> 
> @TarantoolBot document
> Title: New function machinery
> 
> This patchset introduces a set of changes in Tarantool function
> machinery.
> 1. At first, the format of _func space is changed. A new space
> format is
> [<id> UINT, <owner> UINT, <name> STR, <setuid> UINT,
>  <language> STR, <body> STR, <returns> STR,
>  <is_deterministic> BOOL, <opts> MAP]
> 
> and box.schema.func.create endpoint is changed correspondingly:
> 
> box.schema.func.create('funcname', <setuid = true|FALSE>,
> 		<if_not_exists = true|FALSE>, <language = LUA|c>,
> 		<body = string ('')>, <returns = string (ANY)>,
> 		<is_deterministic = true|FALSE>,
> 		<opts = { <is_sandboxed = true|FALSE>} >)
> 
> 2. Now all registered with box.schema.func.create functions are
> exported in box.func folder.
> Each function have :call and :drop method. The :drop method
> just a shortcut for legacy box.schema.func.drop interface.

I wouldn't call box.schema.func.drop "legacy".

> The :call method is similar to net.box connection:call method
> except the format or returned value: the func:call method
> returns a table of values like space:select does.

Hmm, why is that? I'd expect 'call' to return exactly what the function
returns.

> 
> 3. This patchset also introduces 'persistent' Lua functions.
> Such functions are stored in snapshoot and are available after
> restart.
> To create a persistent Lua function, specify function body
> in box.schema.func.create call:
> e.g. body = "function(a, b) return a + b end"
> 
> 4. A Lua persistent function may be 'sandboxed'. The 'sandboxed'
> function executed in isolated environment:
>   a. only limited set of Lua functions and modules are available:
>     -assert -error -pairs -ipairs -next -pcall -xpcall -type
>     -print -select -string -tonumber -tostring -unpack -math -utf8;
>   b. global variables are forbidden
> 
> Example:
> lua_code = [[function(a, b) return a + b end]]
> box.schema.func.create('sum', {body = lua_code,
> 		is_deterministic = true, returns = 'integer',
> 		opts = {is_sandboxed = true}})
> tarantool> box.func.sum
> ---
> - is_sandboxed: true
>   returns: integer
>   is_deterministic: true
>   id: 2
>   language: LUA
>   name: sum
>   is_persistent: true
>   setuid: false
> ...
> box.func.sum:call({1, 2})
> ---
> - - 3
> ...
> conn:call("sum", {1, 3})
> ---
> - 3
> ...
> ---

Please split the docbot request in two:

 1. box.func
 2. persistent functions

Each should go to the corresponding commit.

>  src/box/alter.cc                  |   6 +-
>  src/box/func.c                    | 138 ++++++++++++++++++++++++++++++
>  src/box/func.h                    |   5 ++
>  src/box/lua/call.c                |  22 +++++
>  src/box/lua/init.c                |   2 +
>  src/box/lua/schema.lua            |  25 ++++++
>  src/box/schema.cc                 |   1 +
>  src/box/schema.h                  |  16 ++--
>  test/box/misc.result              |   1 +
>  test/box/persistent_func.result   |  85 ++++++++++++++++++
>  test/box/persistent_func.test.lua |  36 ++++++++
>  11 files changed, 331 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index c769e4f3d..79477aabf 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2517,7 +2517,9 @@ static void
>  func_cache_remove_func(struct trigger *trigger, void * /* event */)
>  {
>  	struct func *old_func = (struct func *) trigger->data;
> -	func_cache_delete(old_func->def->fid);
> +	uint32_t fid = old_func->def->fid;
> +	func_cache_delete(fid);
> +	trigger_run_xc(&on_alter_func, (void *)(uintptr_t)fid);

Ouch. Wouldn't it be better to pass the func pointer instead, similarly
to how on_alter_space does?

>  	func_delete(old_func);
>  }
>  
> @@ -2530,6 +2532,7 @@ func_cache_replace_func(struct trigger *trigger, void * /* event */)
>  	func_cache_replace(new_func, &old_func);
>  	assert(old_func != NULL);
>  	func_delete(old_func);
> +	trigger_run_xc(&on_alter_func, (void *)(uintptr_t)new_func->def->fid);
>  }
>  
>  /**
> @@ -2562,6 +2565,7 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
>  		func_cache_replace(func, &old_func);
>  		assert(old_func == NULL);
>  		func_guard.is_active = false;
> +		trigger_run_xc(&on_alter_func, (void *)(uintptr_t)fid);
>  		struct trigger *on_rollback =
>  			txn_alter_trigger_new(func_cache_remove_func, func);
>  		txn_on_rollback(txn, on_rollback);
> diff --git a/src/box/func.c b/src/box/func.c
> index f2065422c..fee7a6aed 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -28,11 +28,13 @@
>   * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
> +#include "fiber.h"
>  #include "func.h"
>  #include "trivia/config.h"
>  #include "assoc.h"
>  #include "session.h"
>  #include "lua/msgpack.h"
> +#include "lua/trigger.h"
>  #include "lua/utils.h"
>  #include "schema.h"
>  #include "txn.h"
> @@ -830,3 +832,139 @@ box_func_execute_by_name(const char *name, uint32_t name_len, struct port *port,
>  	assert(rc == 0 || diag_last_error(&fiber()->diag) != NULL);
>  	return rc;
>  }
> +
> +static void
> +lbox_func_new(struct lua_State *L, struct func *func)
> +{
> +	lua_getfield(L, LUA_GLOBALSINDEX, "box");
> +	lua_getfield(L, -1, "func");
> +	if (!lua_istable(L, -1)) {
> +		lua_pop(L, 1); /* pop nil */
> +		lua_newtable(L);
> +		lua_setfield(L, -2, "func");
> +		lua_getfield(L, -1, "func");
> +	}
> +	lua_rawgeti(L, -1, func->def->fid);
> +	if (lua_isnil(L, -1)) {
> +		/*
> +		 * If the function already exists, modify it,
> +		 * rather than create a new one -- to not
> +		 * invalidate Lua variable references to old func
> +		 * outside the box.schema.func[].
> +		 */
> +		lua_pop(L, 1);
> +		lua_newtable(L);
> +		lua_rawseti(L, -2, func->def->fid);
> +		lua_rawgeti(L, -1, func->def->fid);
> +	} else {
> +		/* Clear the reference to old space by old name. */

space?

> +		lua_getfield(L, -1, "name");
> +		lua_pushnil(L);
> +		lua_settable(L, -4);
> +	}
> +
> +	int top = lua_gettop(L);
> +	lua_pushstring(L, "id");
> +	lua_pushnumber(L, func->def->fid);
> +	lua_settable(L, top);
> +
> +	lua_pushstring(L, "name");
> +	lua_pushstring(L, func->def->name);
> +	lua_settable(L, top);
> +
> +	lua_pushstring(L, "is_persistent");
> +	lua_pushboolean(L, func_def_is_persistent(func->def));
> +	lua_settable(L, top);
> +
> +	lua_pushstring(L, "setuid");
> +	lua_pushboolean(L, func->def->setuid);
> +	lua_settable(L, top);
> +
> +	lua_pushstring(L, "is_deterministic");
> +	lua_pushboolean(L, func->def->is_deterministic);
> +	lua_settable(L, top);
> +
> +	lua_pushstring(L, "language");
> +	lua_pushstring(L, func_language_strs[func->def->language]);
> +	lua_settable(L, top);
> +
> +	lua_pushstring(L, "returns");
> +	lua_pushstring(L, field_type_strs[func->def->returns]);
> +	lua_settable(L, top);
> +
> +	if (func->def->opts.is_sandboxed) {
> +		lua_pushstring(L, "is_sandboxed");
> +		lua_pushboolean(L, func->def->opts.is_sandboxed);
> +		lua_settable(L, top);
> +	}
> +
> +	/* Bless func object. */
> +	lua_getfield(L, LUA_GLOBALSINDEX, "box");
> +	lua_pushstring(L, "schema");
> +	lua_gettable(L, -2);
> +	lua_pushstring(L, "func");
> +	lua_gettable(L, -2);
> +	lua_pushstring(L, "bless");
> +	lua_gettable(L, -2);
> +
> +	lua_pushvalue(L, top);
> +	lua_call(L, 1, 0);
> +	lua_pop(L, 3);
> +
> +	lua_setfield(L, -2, func->def->name);
> +
> +	lua_pop(L, 2);
> +}
> +
> +static void
> +lbox_func_delete(struct lua_State *L, uint32_t fid)
> +{
> +	lua_getfield(L, LUA_GLOBALSINDEX, "box");
> +	lua_getfield(L, -1, "func");
> +	lua_rawgeti(L, -1, fid);
> +	if (!lua_isnil(L, -1)) {
> +		lua_getfield(L, -1, "name");
> +		lua_pushnil(L);
> +		lua_rawset(L, -4);
> +		lua_pop(L, 1); /* pop func */
> +
> +		lua_pushnil(L);
> +		lua_rawseti(L, -2, fid);
> +	} else {
> +		lua_pop(L, 1);
> +	}
> +	lua_pop(L, 2); /* box, func */
> +}
> +
> +static void
> +lbox_func_new_or_delete(struct trigger *trigger, void *event)
> +{
> +	struct lua_State *L = (struct lua_State *) trigger->data;
> +	uint32_t fid = (uint32_t)(uintptr_t)event;
> +	struct func *func = func_by_id(fid);
> +	/* Export only persistent Lua functions. */
> +	if (func != NULL)
> +		lbox_func_new(L, func);
> +	else
> +		lbox_func_delete(L, fid);
> +}
> +
> +static struct trigger on_alter_func_in_lua = {
> +	RLIST_LINK_INITIALIZER, lbox_func_new_or_delete, NULL, NULL
> +};
> +
> +void
> +box_lua_func_init(struct lua_State *L)
> +{
> +	/*
> +	 * Register the trigger that will push persistent
> +	 * Lua functions objects to Lua.
> +	 */
> +	on_alter_func_in_lua.data = L;
> +	trigger_add(&on_alter_func, &on_alter_func_in_lua);
> +	static const struct luaL_Reg funclib [] = {
> +		{NULL, NULL}
> +	};
> +	luaL_register_module(L, "box.func", funclib);
> +	lua_pop(L, 1);

Hmm, is it really necessary to register a new module from C?

> +}
> diff --git a/src/box/func.h b/src/box/func.h
> index 7b920d7d3..89f503eea 100644
> --- a/src/box/func.h
> +++ b/src/box/func.h
> @@ -44,6 +44,7 @@ extern "C" {
>  
>  struct port;
>  struct func_vtab;
> +struct lua_State;
>  
>  /**
>   * Dynamic shared module.
> @@ -160,6 +161,10 @@ int
>  box_func_execute_by_name(const char *name, uint32_t name_len, struct port *port,
>  			 const char *args, const char *args_end);
>  
> +/** Initialize Lua export trigger for _func objects. */
> +void
> +box_lua_func_init(struct lua_State *L);
> +
>  #if defined(__cplusplus)
>  } /* extern "C" */
>  #endif /* defined(__cplusplus) */
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index e40c9a3f7..f85e1fc30 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -29,6 +29,7 @@
>   * SUCH DAMAGE.
>   */
>  #include "box/lua/call.h"
> +#include "box/lua/misc.h"
>  #include "box/call.h"
>  #include "box/error.h"
>  #include "box/func.h"
> @@ -126,10 +127,31 @@ lbox_module_reload(lua_State *L)
>  	return 0;
>  }
>  
> +int
> +lbox_func_call(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 2 || !lua_isstring(L, 1))
> +		return luaL_error(L, "Usage func:call(table)");
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +	size_t tuple_len;
> +	const char *tuple = lbox_encode_tuple_on_gc(L, 2, &tuple_len);

Please, create a port_lua from Lua stack and pass it to func_call.

> +
> +	struct port port;
> +	if (box_func_execute_by_name(name, name_len, &port, tuple,
> +				     tuple + tuple_len) != 0)

IMO we should return an error if the function isn't registered, not call
any existing Lua function. In other words, I think we should use
func_by_id/name + func_call here.

> +		return luaT_error(L);
> +	port_dump_lua(&port, L);
> +	port_destroy(&port);
> +	return 1;
> +}
> +
>  static const struct luaL_Reg boxlib_internal[] = {
>  	{"call_loadproc",  lbox_call_loadproc},
>  	{"sql_create_function",  lbox_sql_create_function},
>  	{"module_reload", lbox_module_reload},
> +	{"func_call", lbox_func_call},
>  	{NULL, NULL}
>  };
>  
> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
> index 76b987b4b..67a822eb4 100644
> --- a/src/box/lua/init.c
> +++ b/src/box/lua/init.c
> @@ -39,6 +39,7 @@
>  
>  #include "box/box.h"
>  #include "box/txn.h"
> +#include "box/func.h"
>  #include "box/vclock.h"
>  
>  #include "box/lua/error.h"
> @@ -315,6 +316,7 @@ box_lua_init(struct lua_State *L)
>  	box_lua_xlog_init(L);
>  	box_lua_execute_init(L);
>  	luaopen_net_box(L);
> +	box_lua_func_init(L);
>  	lua_pop(L, 1);
>  	tarantool_lua_console_init(L);
>  	lua_pop(L, 1);
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index b5393e19a..1b09758d9 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -2164,7 +2164,32 @@ function box.schema.func.exists(name_or_id)
>      return tuple ~= nil
>  end
>  
> +-- Helper function to check func:method() usage
> +local function check_func_arg(func, method)
> +    if type(func) ~= 'table' or func.name == nil then
> +        local fmt = 'Use func:%s(...) instead of func.%s(...)'
> +        error(string.format(fmt, method, method))

Please add a test case for this error.

> +    end
> +end
> +
> +local func_f_mt = {}
> +
> +func_f_mt.drop = function(func, opts)

func_f_mt? Why not simply func_mt?

> +    check_func_arg(func, 'drop')
> +    box.schema.func.drop(func.name, opts)
> +end
> +
> +func_f_mt.call = function(func, args)
> +    check_func_arg(func, 'call')
> +    return box.schema.func.call(func.name, args or {})
> +end
> +
> +function box.schema.func.bless(func)
> +    setmetatable(func, {__index = func_f_mt})
> +end
> +
>  box.schema.func.reload = internal.module_reload

Let's add 'reload' method to func_mt as well?

> +box.schema.func.call = internal.func_call
>  
>  box.internal.collation = {}
>  box.internal.collation.create = function(name, coll_type, locale, opts)
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 0508482a6..90b18b347 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -72,6 +72,7 @@ uint32_t space_cache_version = 0;
>  struct rlist on_schema_init = RLIST_HEAD_INITIALIZER(on_schema_init);
>  struct rlist on_alter_space = RLIST_HEAD_INITIALIZER(on_alter_space);
>  struct rlist on_alter_sequence = RLIST_HEAD_INITIALIZER(on_alter_sequence);
> +struct rlist on_alter_func = RLIST_HEAD_INITIALIZER(on_alter_func);
>  
>  /**
>   * Lock of scheme modification
> diff --git a/src/box/schema.h b/src/box/schema.h
> index 368463536..e569e3f9d 100644
> --- a/src/box/schema.h
> +++ b/src/box/schema.h
> @@ -130,6 +130,11 @@ int
>  schema_find_id(uint32_t system_space_id, uint32_t index_id, const char *name,
>  	       uint32_t len, uint32_t *object_id);
>  
> +struct func;
> +
> +struct func *
> +func_by_id(uint32_t fid);
> +
>  #if defined(__cplusplus)
>  } /* extern "C" */
>  
> @@ -178,11 +183,6 @@ func_cache_replace(struct func *new_func, struct func **old_func);
>  void
>  func_cache_delete(uint32_t fid);
>  
> -struct func;
> -
> -struct func *
> -func_by_id(uint32_t fid);
> -
>  static inline struct func *
>  func_cache_find(uint32_t fid)
>  {
> @@ -243,6 +243,12 @@ extern struct rlist on_alter_sequence;
>   */
>  extern struct rlist on_access_denied;
>  
> +/**
> + * Triggers fired after committing a change in _func space.
> + * It is passed the txn statement that altered the space.

txn statement? Really?

> + */
> +extern struct rlist on_alter_func;
> +
>  /**
>   * Context passed to on_access_denied trigger.
>   */
> diff --git a/test/box/persistent_func.result b/test/box/persistent_func.result
> index 54548f3b0..bf0a123e2 100644
> --- a/test/box/persistent_func.result
> +++ b/test/box/persistent_func.result
> @@ -86,6 +86,56 @@ math.abs(1)
>  box.schema.func.drop('monkey')
>  ---
>  ...
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +body = [[function(array)
> +		local min = 999
> +		local max = -1
> +		for _, v in pairs(array) do
> +			min = math.min(min, v)
> +			max = math.max(max, v)
> +		end
> +		return min, max
> +	end]]
> +test_run:cmd("setopt delimiter ''");
> +---
> +...
> +box.schema.func.create("minmax", {body = body, opts = {is_sandboxed = true}})
> +---
> +...
> +box.func.minmax
> +---
> +- is_sandboxed: true
> +  returns: any
> +  is_deterministic: false
> +  id: 4
> +  language: LUA
> +  name: minmax
> +  is_persistent: true
> +  setuid: false
> +...
> +array = {1, 3, 2, 5, 9}
> +---
> +...
> +box.func.minmax:call({array})
> +---
> +- - 9
> +  - 1
> +...
> +conn:call("minmax", {array})
> +---
> +- 1
> +- 9
> +...
> +box.func.minmax:drop()
> +---
> +...
> +box.schema.func.exists("minmax")
> +---
> +- false
> +...
>  -- Test function with spell error - case 1.
>  test_run:cmd("setopt delimiter ';'")
>  ---
> @@ -170,3 +220,38 @@ box.schema.func.drop('test2')
>  box.schema.user.revoke('guest', 'execute', 'universe')
>  ---
>  ...
> +box.schema.func.create("secret", {body = "function() return 1 end"})
> +---
> +...
> +box.func.secret:call({})
> +---
> +- - 1
> +...
> +function secret_leak() return box.func.secret:call() end
> +---
> +...
> +box.schema.func.create('secret_leak')
> +---
> +...
> +box.schema.user.grant('guest', 'execute', 'function', 'secret_leak')
> +---
> +...
> +conn = net.connect(box.cfg.listen)
> +---
> +...
> +conn:call('secret_leak')
> +---
> +- error: Execute access to function 'secret' is denied for user 'guest'
> +...
> +conn:close()

Better use box.session.su for checking permissions.

> +---
> +...
> +box.schema.user.revoke('guest', 'execute', 'function', 'secret_leak')
> +---
> +...
> +box.schema.func.drop('secret_leak')
> +---
> +...
> +box.schema.func.drop('secret')
> +---
> +...

  reply	other threads:[~2019-06-10 15:18 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-06 12:03 [PATCH v2 0/9] box: rework functions machinery Kirill Shcherbatov
2019-06-06 12:03 ` [PATCH v2 1/9] box: refactor box_lua_find helper Kirill Shcherbatov
2019-06-10  9:17   ` Vladimir Davydov
2019-06-06 12:03 ` [PATCH v2 2/9] box: move box_module_reload routine to func.c Kirill Shcherbatov
2019-06-10  9:19   ` Vladimir Davydov
2019-06-06 12:03 ` [PATCH v2 3/9] box: rework func cache update machinery Kirill Shcherbatov
2019-06-10  9:44   ` Vladimir Davydov
2019-06-06 12:04 ` [PATCH v2 4/9] box: rework func object as a function frontend Kirill Shcherbatov
2019-06-10 10:32   ` Vladimir Davydov
2019-06-06 12:04 ` [PATCH v2 5/9] schema: rework _func system space format Kirill Shcherbatov
2019-06-10 12:10   ` Vladimir Davydov
2019-06-06 12:04 ` [PATCH v2 6/9] box: load persistent Lua functions on creation Kirill Shcherbatov
2019-06-10 12:19   ` Vladimir Davydov
2019-06-06 12:04 ` [PATCH v2 7/9] box: sandbox option for persistent functions Kirill Shcherbatov
2019-06-10 14:06   ` Vladimir Davydov
2019-06-10 14:15     ` [tarantool-patches] " Kirill Shcherbatov
2019-06-10 14:41       ` Vladimir Davydov
2019-06-06 12:04 ` [PATCH v2 8/9] box: implement lua_port dump to region and to Lua Kirill Shcherbatov
2019-06-10 14:24   ` Vladimir Davydov
2019-06-06 12:04 ` [PATCH v2 9/9] box: export _func functions with box.func folder Kirill Shcherbatov
2019-06-10 15:18   ` Vladimir Davydov [this message]
2019-06-10  9:14 ` [PATCH v2 0/9] box: rework functions machinery Vladimir Davydov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190610151846.4azti2a2ob5jur6e@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [PATCH v2 9/9] box: export _func functions with box.func folder' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox