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')
> +---
> +...
next prev parent 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