From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 10 Jun 2019 18:18:46 +0300 From: Vladimir Davydov Subject: Re: [PATCH v2 9/9] box: export _func functions with box.func folder Message-ID: <20190610151846.4azti2a2ob5jur6e@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: 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 > [ UINT, UINT, STR, UINT, > STR, STR, STR, > BOOL, MAP] > > and box.schema.func.create endpoint is changed correspondingly: > > box.schema.func.create('funcname', , > , , > , , > , > } >) > > 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') > +--- > +...