* [PATCH v2 1/9] box: refactor box_lua_find helper
2019-06-06 12:03 [PATCH v2 0/9] box: rework functions machinery Kirill Shcherbatov
@ 2019-06-06 12:03 ` 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
` (8 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-06 12:03 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov
The box_lua_find routine used to work with an empty stack only.
It is unacceptable in following patches because this helper
need to be reused in following patches for environment table
construction.
The definition also is moved to lua/utils.h.
Needed for #4182, #1260
---
src/box/lua/call.c | 85 ++++++++--------------------------------------
src/lua/utils.c | 59 ++++++++++++++++++++++++++++++++
src/lua/utils.h | 11 ++++++
3 files changed, 85 insertions(+), 70 deletions(-)
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 04020ef6f..c729778c4 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -44,73 +44,6 @@
#include "trivia/util.h"
#include "mpstream.h"
-/**
- * A helper to find a Lua function by name and put it
- * on top of the stack.
- */
-static int
-box_lua_find(lua_State *L, const char *name, const char *name_end)
-{
- int index = LUA_GLOBALSINDEX;
- int objstack = 0;
- const char *start = name, *end;
-
- while ((end = (const char *) memchr(start, '.', name_end - start))) {
- lua_checkstack(L, 3);
- lua_pushlstring(L, start, end - start);
- lua_gettable(L, index);
- if (! lua_istable(L, -1)) {
- diag_set(ClientError, ER_NO_SUCH_PROC,
- name_end - name, name);
- luaT_error(L);
- }
- start = end + 1; /* next piece of a.b.c */
- index = lua_gettop(L); /* top of the stack */
- }
-
- /* box.something:method */
- if ((end = (const char *) memchr(start, ':', name_end - start))) {
- lua_checkstack(L, 3);
- lua_pushlstring(L, start, end - start);
- lua_gettable(L, index);
- if (! (lua_istable(L, -1) ||
- lua_islightuserdata(L, -1) || lua_isuserdata(L, -1) )) {
- diag_set(ClientError, ER_NO_SUCH_PROC,
- name_end - name, name);
- luaT_error(L);
- }
- start = end + 1; /* next piece of a.b.c */
- index = lua_gettop(L); /* top of the stack */
- objstack = index;
- }
-
-
- lua_pushlstring(L, start, name_end - start);
- lua_gettable(L, index);
- if (!lua_isfunction(L, -1) && !lua_istable(L, -1)) {
- /* lua_call or lua_gettable would raise a type error
- * for us, but our own message is more verbose. */
- diag_set(ClientError, ER_NO_SUCH_PROC,
- name_end - name, name);
- luaT_error(L);
- }
- /* setting stack that it would contain only
- * the function pointer. */
- if (index != LUA_GLOBALSINDEX) {
- if (objstack == 0) { /* no object, only a function */
- lua_replace(L, 1);
- } else if (objstack == 1) { /* just two values, swap them */
- lua_insert(L, -2);
- } else { /* long path */
- lua_insert(L, 1);
- lua_insert(L, 2);
- objstack = 1;
- }
- lua_settop(L, 1 + objstack);
- }
- return 1 + objstack;
-}
-
/**
* A helper to find lua stored procedures for box.call.
* box.call iteslf is pure Lua, to avoid issues
@@ -124,7 +57,12 @@ lbox_call_loadproc(struct lua_State *L)
const char *name;
size_t name_len;
name = lua_tolstring(L, 1, &name_len);
- return box_lua_find(L, name, name + name_len);
+ int count;
+ if (luaT_func_find(L, name, name + name_len, &count) != 0) {
+ diag_set(ClientError, ER_NO_SUCH_PROC, name_len, name);
+ return luaT_error(L);
+ }
+ return count;
}
/*
@@ -292,9 +230,16 @@ execute_lua_call(lua_State *L)
const char *name = request->name;
uint32_t name_len = mp_decode_strl(&name);
- int oc = 0; /* how many objects are on stack after box_lua_find */
+ /*
+ * How many objects are on stack after
+ * luaT_func_find call.
+ */
+ int oc = 0;
/* Try to find a function by name in Lua */
- oc = box_lua_find(L, name, name + name_len);
+ if (luaT_func_find(L, name, name + name_len, &oc) != 0) {
+ diag_set(ClientError, ER_NO_SUCH_PROC, name_len, name);
+ return luaT_error(L);
+ }
/* Push the rest of args (a tuple). */
const char *args = request->args;
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 01a0cd894..27ff6b396 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1189,6 +1189,65 @@ void luaL_iterator_delete(struct luaL_iterator *it)
/* }}} */
+int
+luaT_func_find(struct lua_State *L, const char *name, const char *name_end,
+ int *count)
+{
+ int index = LUA_GLOBALSINDEX;
+ int objstack = 0, top = lua_gettop(L);
+ const char *start = name, *end;
+
+ while ((end = (const char *) memchr(start, '.', name_end - start))) {
+ lua_checkstack(L, 3);
+ lua_pushlstring(L, start, end - start);
+ lua_gettable(L, index);
+ if (! lua_istable(L, -1))
+ return -1;
+ start = end + 1; /* next piece of a.b.c */
+ index = lua_gettop(L); /* top of the stack */
+ }
+
+ /* box.something:method */
+ if ((end = (const char *) memchr(start, ':', name_end - start))) {
+ lua_checkstack(L, 3);
+ lua_pushlstring(L, start, end - start);
+ lua_gettable(L, index);
+ if (! (lua_istable(L, -1) ||
+ lua_islightuserdata(L, -1) || lua_isuserdata(L, -1) ))
+ return -1;
+ start = end + 1; /* next piece of a.b.c */
+ index = lua_gettop(L); /* top of the stack */
+ objstack = index - top;
+ }
+
+ lua_pushlstring(L, start, name_end - start);
+ lua_gettable(L, index);
+ if (!lua_isfunction(L, -1) && !lua_istable(L, -1)) {
+ /* lua_call or lua_gettable would raise a type error
+ * for us, but our own message is more verbose. */
+ return -1;
+ }
+
+ /* setting stack that it would contain only
+ * the function pointer. */
+ if (index != LUA_GLOBALSINDEX) {
+ if (objstack == 0) { /* no object, only a function */
+ lua_replace(L, top + 1);
+ lua_pop(L, lua_gettop(L) - top - 1);
+ } else if (objstack == 1) { /* just two values, swap them */
+ lua_insert(L, -2);
+ lua_pop(L, lua_gettop(L) - top - 2);
+ } else { /* long path */
+ lua_insert(L, top + 1);
+ lua_insert(L, top + 2);
+ lua_pop(L, objstack - 1);
+ objstack = 1;
+ }
+ }
+ *count = 1 + objstack;
+ return 0;
+}
+
int
tarantool_lua_utils_init(struct lua_State *L)
{
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 943840ec0..81e936bee 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -592,6 +592,17 @@ void luaL_iterator_delete(struct luaL_iterator *it);
/* }}} */
+/**
+ * A helper to find a Lua function by name and put it
+ * on top of the stack.
+ * Returns 0 in case of succsess and -1 otherwise; in case of
+ * success also uses count[out] argument to return the how many
+ * objects are pushed on to stack.
+ */
+int
+luaT_func_find(struct lua_State *L, const char *name, const char *name_end,
+ int *count);
+
int
tarantool_lua_utils_init(struct lua_State *L);
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/9] box: refactor box_lua_find helper
2019-06-06 12:03 ` [PATCH v2 1/9] box: refactor box_lua_find helper Kirill Shcherbatov
@ 2019-06-10 9:17 ` Vladimir Davydov
0 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 9:17 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Thu, Jun 06, 2019 at 03:03:57PM +0300, Kirill Shcherbatov wrote:
> The box_lua_find routine used to work with an empty stack only.
> It is unacceptable in following patches because this helper
> need to be reused in following patches for environment table
> construction.
> The definition also is moved to lua/utils.h.
>
> Needed for #4182, #1260
> ---
> src/box/lua/call.c | 85 ++++++++--------------------------------------
> src/lua/utils.c | 59 ++++++++++++++++++++++++++++++++
> src/lua/utils.h | 11 ++++++
> 3 files changed, 85 insertions(+), 70 deletions(-)
>
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 04020ef6f..c729778c4 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -44,73 +44,6 @@
> #include "trivia/util.h"
> #include "mpstream.h"
>
> -/**
> - * A helper to find a Lua function by name and put it
> - * on top of the stack.
> - */
> -static int
> -box_lua_find(lua_State *L, const char *name, const char *name_end)
> -{
> - int index = LUA_GLOBALSINDEX;
> - int objstack = 0;
> - const char *start = name, *end;
> -
> - while ((end = (const char *) memchr(start, '.', name_end - start))) {
> - lua_checkstack(L, 3);
> - lua_pushlstring(L, start, end - start);
> - lua_gettable(L, index);
> - if (! lua_istable(L, -1)) {
> - diag_set(ClientError, ER_NO_SUCH_PROC,
> - name_end - name, name);
> - luaT_error(L);
> - }
> - start = end + 1; /* next piece of a.b.c */
> - index = lua_gettop(L); /* top of the stack */
> - }
> -
> - /* box.something:method */
> - if ((end = (const char *) memchr(start, ':', name_end - start))) {
> - lua_checkstack(L, 3);
> - lua_pushlstring(L, start, end - start);
> - lua_gettable(L, index);
> - if (! (lua_istable(L, -1) ||
> - lua_islightuserdata(L, -1) || lua_isuserdata(L, -1) )) {
> - diag_set(ClientError, ER_NO_SUCH_PROC,
> - name_end - name, name);
> - luaT_error(L);
> - }
> - start = end + 1; /* next piece of a.b.c */
> - index = lua_gettop(L); /* top of the stack */
> - objstack = index;
> - }
> -
> -
> - lua_pushlstring(L, start, name_end - start);
> - lua_gettable(L, index);
> - if (!lua_isfunction(L, -1) && !lua_istable(L, -1)) {
> - /* lua_call or lua_gettable would raise a type error
> - * for us, but our own message is more verbose. */
> - diag_set(ClientError, ER_NO_SUCH_PROC,
> - name_end - name, name);
> - luaT_error(L);
> - }
> - /* setting stack that it would contain only
> - * the function pointer. */
> - if (index != LUA_GLOBALSINDEX) {
> - if (objstack == 0) { /* no object, only a function */
> - lua_replace(L, 1);
> - } else if (objstack == 1) { /* just two values, swap them */
> - lua_insert(L, -2);
> - } else { /* long path */
> - lua_insert(L, 1);
> - lua_insert(L, 2);
> - objstack = 1;
> - }
> - lua_settop(L, 1 + objstack);
> - }
> - return 1 + objstack;
> -}
> -
> /**
> * A helper to find lua stored procedures for box.call.
> * box.call iteslf is pure Lua, to avoid issues
> @@ -124,7 +57,12 @@ lbox_call_loadproc(struct lua_State *L)
> const char *name;
> size_t name_len;
> name = lua_tolstring(L, 1, &name_len);
> - return box_lua_find(L, name, name + name_len);
> + int count;
> + if (luaT_func_find(L, name, name + name_len, &count) != 0) {
> + diag_set(ClientError, ER_NO_SUCH_PROC, name_len, name);
> + return luaT_error(L);
> + }
> + return count;
> }
>
> /*
> @@ -292,9 +230,16 @@ execute_lua_call(lua_State *L)
> const char *name = request->name;
> uint32_t name_len = mp_decode_strl(&name);
>
> - int oc = 0; /* how many objects are on stack after box_lua_find */
> + /*
> + * How many objects are on stack after
> + * luaT_func_find call.
> + */
> + int oc = 0;
> /* Try to find a function by name in Lua */
> - oc = box_lua_find(L, name, name + name_len);
> + if (luaT_func_find(L, name, name + name_len, &oc) != 0) {
> + diag_set(ClientError, ER_NO_SUCH_PROC, name_len, name);
> + return luaT_error(L);
> + }
>
> /* Push the rest of args (a tuple). */
> const char *args = request->args;
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 01a0cd894..27ff6b396 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -1189,6 +1189,65 @@ void luaL_iterator_delete(struct luaL_iterator *it)
>
> /* }}} */
>
> +int
> +luaT_func_find(struct lua_State *L, const char *name, const char *name_end,
> + int *count)
> +{
> + int index = LUA_GLOBALSINDEX;
> + int objstack = 0, top = lua_gettop(L);
> + const char *start = name, *end;
> +
> + while ((end = (const char *) memchr(start, '.', name_end - start))) {
> + lua_checkstack(L, 3);
> + lua_pushlstring(L, start, end - start);
> + lua_gettable(L, index);
> + if (! lua_istable(L, -1))
> + return -1;
> + start = end + 1; /* next piece of a.b.c */
> + index = lua_gettop(L); /* top of the stack */
> + }
> +
> + /* box.something:method */
> + if ((end = (const char *) memchr(start, ':', name_end - start))) {
> + lua_checkstack(L, 3);
> + lua_pushlstring(L, start, end - start);
> + lua_gettable(L, index);
> + if (! (lua_istable(L, -1) ||
> + lua_islightuserdata(L, -1) || lua_isuserdata(L, -1) ))
> + return -1;
> + start = end + 1; /* next piece of a.b.c */
> + index = lua_gettop(L); /* top of the stack */
> + objstack = index - top;
> + }
> +
> + lua_pushlstring(L, start, name_end - start);
> + lua_gettable(L, index);
> + if (!lua_isfunction(L, -1) && !lua_istable(L, -1)) {
> + /* lua_call or lua_gettable would raise a type error
> + * for us, but our own message is more verbose. */
> + return -1;
> + }
> +
> + /* setting stack that it would contain only
> + * the function pointer. */
> + if (index != LUA_GLOBALSINDEX) {
> + if (objstack == 0) { /* no object, only a function */
> + lua_replace(L, top + 1);
> + lua_pop(L, lua_gettop(L) - top - 1);
> + } else if (objstack == 1) { /* just two values, swap them */
> + lua_insert(L, -2);
> + lua_pop(L, lua_gettop(L) - top - 2);
> + } else { /* long path */
> + lua_insert(L, top + 1);
> + lua_insert(L, top + 2);
> + lua_pop(L, objstack - 1);
> + objstack = 1;
> + }
> + }
> + *count = 1 + objstack;
> + return 0;
> +}
Moving code and patching it at the same time is bad, as it's very easy
to overlook a mistake. Please try to avoid that.
Anyway, I don't think we need to move this function anywhere - IMO we
should define func_lua in src/box/lua/call.c instead. Also, patching
this function is really trivial - since it's only needed for sandboxing
I'd fold this change in the patch introducing persistent functions.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 2/9] box: move box_module_reload routine to func.c
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-06 12:03 ` 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
` (7 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-06 12:03 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov
Previously, the box_module_reload function was located in the
box/call module that is wrong. It is moved to a more suitable
place in box/func.
---
src/box/call.c | 21 ---------------------
src/box/call.h | 10 ----------
src/box/func.c | 25 ++++++++++++++++++++++++-
src/box/func.h | 12 ++++--------
src/box/lua/call.c | 1 +
5 files changed, 29 insertions(+), 40 deletions(-)
diff --git a/src/box/call.c b/src/box/call.c
index 56da53fb3..982c95cbf 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -132,27 +132,6 @@ box_c_call(struct func *func, struct call_request *request, struct port *port)
return 0;
}
-int
-box_module_reload(const char *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;
- }
- 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;
-}
-
int
box_process_call(struct call_request *request, struct port *port)
{
diff --git a/src/box/call.h b/src/box/call.h
index 1b54551be..4b3b8614c 100644
--- a/src/box/call.h
+++ b/src/box/call.h
@@ -42,16 +42,6 @@ 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_module_reload(const char *name);
-
int
box_process_call(struct call_request *request, struct port *port);
diff --git a/src/box/func.c b/src/box/func.c
index a817851fd..098bc02b6 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -31,6 +31,7 @@
#include "func.h"
#include "trivia/config.h"
#include "assoc.h"
+#include "session.h"
#include "lua/utils.h"
#include "error.h"
#include "diag.h"
@@ -298,7 +299,16 @@ module_sym(struct module *module, const char *name)
return f;
}
-int
+/**
+ * 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.
+ */
+static int
module_reload(const char *package, const char *package_end, struct module **module)
{
struct module *old_module = module_cache_find(package, package_end);
@@ -355,6 +365,19 @@ restore:
return -1;
}
+int
+box_module_reload(const char *name)
+{
+ if (access_check_universe(PRIV_X | PRIV_U) != 0)
+ return -1;
+ 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;
+}
+
struct func *
func_new(struct func_def *def)
{
diff --git a/src/box/func.h b/src/box/func.h
index 8dcd61d7b..db839a17a 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -114,16 +114,12 @@ 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.
+ * Reload loadable module by a given name.
+ * Returns 0 in case of success and -1 otherwise and sets the
+ * diag message.
*/
int
-module_reload(const char *package, const char *package_end, struct module **module);
+box_module_reload(const char *name);
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index c729778c4..9edb5bf7e 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -31,6 +31,7 @@
#include "box/lua/call.h"
#include "box/call.h"
#include "box/error.h"
+#include "box/func.h"
#include "fiber.h"
#include "lua/utils.h"
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 2/9] box: move box_module_reload routine to func.c
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
0 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 9:19 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Thu, Jun 06, 2019 at 03:03:58PM +0300, Kirill Shcherbatov wrote:
> Previously, the box_module_reload function was located in the
> box/call module that is wrong. It is moved to a more suitable
> place in box/func.
> ---
> src/box/call.c | 21 ---------------------
> src/box/call.h | 10 ----------
> src/box/func.c | 25 ++++++++++++++++++++++++-
> src/box/func.h | 12 ++++--------
> src/box/lua/call.c | 1 +
> 5 files changed, 29 insertions(+), 40 deletions(-)
>
> diff --git a/src/box/call.c b/src/box/call.c
> index 56da53fb3..982c95cbf 100644
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -132,27 +132,6 @@ box_c_call(struct func *func, struct call_request *request, struct port *port)
> return 0;
> }
>
> -int
> -box_module_reload(const char *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;
> - }
> - 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;
> -}
> -
> int
> box_process_call(struct call_request *request, struct port *port)
> {
> diff --git a/src/box/call.h b/src/box/call.h
> index 1b54551be..4b3b8614c 100644
> --- a/src/box/call.h
> +++ b/src/box/call.h
> @@ -42,16 +42,6 @@ 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_module_reload(const char *name);
> -
> int
> box_process_call(struct call_request *request, struct port *port);
>
> diff --git a/src/box/func.c b/src/box/func.c
> index a817851fd..098bc02b6 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -31,6 +31,7 @@
> #include "func.h"
> #include "trivia/config.h"
> #include "assoc.h"
> +#include "session.h"
> #include "lua/utils.h"
> #include "error.h"
> #include "diag.h"
> @@ -298,7 +299,16 @@ module_sym(struct module *module, const char *name)
> return f;
> }
>
> -int
> +/**
> + * 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.
> + */
> +static int
> module_reload(const char *package, const char *package_end, struct module **module)
> {
> struct module *old_module = module_cache_find(package, package_end);
> @@ -355,6 +365,19 @@ restore:
> return -1;
> }
>
> +int
> +box_module_reload(const char *name)
> +{
> + if (access_check_universe(PRIV_X | PRIV_U) != 0)
> + return -1;
> + 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;
> +}
> +
TBO I don't see any point in this change at this time - the function
could as well stay in src/box/call.c, along with all permission checks.
Please try to avoid stray changes so as not to pollute the git history.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 3/9] box: rework func cache update machinery
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-06 12:03 ` [PATCH v2 2/9] box: move box_module_reload routine to func.c Kirill Shcherbatov
@ 2019-06-06 12:03 ` 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
` (6 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-06 12:03 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov
Tarantool used to assume that func_new call must not fail and
it used to build a new func object by given definition just on
func cache replace operation. Fixed in scope of global
refactoring of func module.
Needed for #4182, #1260
---
src/box/alter.cc | 43 ++++++++++++++++++++++++++-----------------
src/box/func.c | 13 +++++++------
src/box/func.h | 12 +++++++++---
src/box/schema.cc | 46 ++++++++++++++++++++--------------------------
src/box/schema.h | 15 +++++++++------
5 files changed, 71 insertions(+), 58 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index ed9e55907..3b57a7d82 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2463,24 +2463,22 @@ func_def_new_from_tuple(struct tuple *tuple)
/** Remove a function from function cache */
static void
-func_cache_remove_func(struct trigger * /* trigger */, void *event)
+func_cache_remove_func(struct trigger *trigger, void * /* event */)
{
- struct txn_stmt *stmt = txn_last_stmt((struct txn *) event);
- uint32_t fid = tuple_field_u32_xc(stmt->old_tuple ?
- stmt->old_tuple : stmt->new_tuple,
- BOX_FUNC_FIELD_ID);
- func_cache_delete(fid);
+ struct func *old_func = (struct func *) trigger->data;
+ func_cache_delete(old_func->def->fid);
+ func_delete(old_func);
}
/** Replace a function in the function cache */
static void
-func_cache_replace_func(struct trigger * /* trigger */, void *event)
+func_cache_replace_func(struct trigger *trigger, void * /* event */)
{
- struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
- struct func_def *def = func_def_new_from_tuple(stmt->new_tuple);
- auto def_guard = make_scoped_guard([=] { free(def); });
- func_cache_replace(def);
- def_guard.is_active = false;
+ struct func *new_func = (struct func *) trigger->data;
+ struct func *old_func;
+ func_cache_replace(new_func, &old_func);
+ assert(old_func != NULL);
+ func_delete(old_func);
}
/**
@@ -2501,13 +2499,20 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
struct func *old_func = func_by_id(fid);
if (new_tuple != NULL && old_func == NULL) { /* INSERT */
struct func_def *def = func_def_new_from_tuple(new_tuple);
+ auto def_guard = make_scoped_guard([=] { free(def); });
access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION,
PRIV_C);
- auto def_guard = make_scoped_guard([=] { free(def); });
- func_cache_replace(def);
+ struct func *func = func_new(def);
+ if (func == NULL)
+ diag_raise();
+ auto func_guard = make_scoped_guard([=] { func_delete(func); });
def_guard.is_active = false;
+ struct func *old_func = NULL;
+ func_cache_replace(func, &old_func);
+ assert(old_func == NULL);
+ func_guard.is_active = false;
struct trigger *on_rollback =
- txn_alter_trigger_new(func_cache_remove_func, NULL);
+ txn_alter_trigger_new(func_cache_remove_func, func);
txn_on_rollback(txn, on_rollback);
} else if (new_tuple == NULL) { /* DELETE */
uint32_t uid;
@@ -2525,15 +2530,19 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
"function has grants");
}
struct trigger *on_commit =
- txn_alter_trigger_new(func_cache_remove_func, NULL);
+ txn_alter_trigger_new(func_cache_remove_func, old_func);
txn_on_commit(txn, on_commit);
} else { /* UPDATE, REPLACE */
struct func_def *def = func_def_new_from_tuple(new_tuple);
auto def_guard = make_scoped_guard([=] { free(def); });
access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION,
PRIV_A);
+ struct func *func = func_new(def);
+ if (func == NULL)
+ diag_raise();
+ def_guard.is_active = false;
struct trigger *on_commit =
- txn_alter_trigger_new(func_cache_replace_func, NULL);
+ txn_alter_trigger_new(func_cache_replace_func, func);
txn_on_commit(txn, on_commit);
}
}
diff --git a/src/box/func.c b/src/box/func.c
index 098bc02b6..5051286a3 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -475,17 +475,18 @@ func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
}
void
-func_update(struct func *func, struct func_def *def)
+func_delete(struct func *func)
{
func_unload(func);
free(func->def);
- func->def = def;
+ free(func);
}
void
-func_delete(struct func *func)
+func_reuse_runtime(struct func *new_func, struct func *old_func)
{
- func_unload(func);
- free(func->def);
- free(func);
+ new_func->module = old_func->module;
+ new_func->func = old_func->func;
+ old_func->module = NULL;
+ old_func->func = NULL;
}
diff --git a/src/box/func.h b/src/box/func.h
index db839a17a..82ac046b7 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -100,9 +100,6 @@ module_free(void);
struct func *
func_new(struct func_def *def);
-void
-func_update(struct func *func, struct func_def *def);
-
void
func_delete(struct func *func);
@@ -113,6 +110,15 @@ int
func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
const char *args_end);
+/**
+ * Capture a given old_func's runtime an reuse in a given new
+ * function if possible. In case of success, the old_func
+ * stops holding it's runtime so delete method would not
+ * release it.
+ */
+void
+func_reuse_runtime(struct func *new_func, struct func *old_func);
+
/**
* Reload loadable module by a given name.
* Returns 0 in case of success and -1 otherwise and sets the
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 9a55c2f14..0508482a6 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -533,40 +533,35 @@ schema_free(void)
}
void
-func_cache_replace(struct func_def *def)
+func_cache_replace(struct func *new_func, struct func **old_func)
{
- struct func *old = func_by_id(def->fid);
- if (old) {
- func_update(old, def);
- return;
- }
if (mh_size(funcs) >= BOX_FUNCTION_MAX)
tnt_raise(ClientError, ER_FUNCTION_MAX, BOX_FUNCTION_MAX);
- struct func *func = func_new(def);
- if (func == NULL) {
-error:
- panic_syserror("Out of memory for the data "
- "dictionary cache (stored function).");
- }
- const struct mh_i32ptr_node_t node = { def->fid, func };
- mh_int_t k1 = mh_i32ptr_put(funcs, &node, NULL, NULL);
- if (k1 == mh_end(funcs)) {
- func->def = NULL;
- func_delete(func);
- goto error;
- }
- size_t def_name_len = strlen(func->def->name);
- uint32_t name_hash = mh_strn_hash(func->def->name, def_name_len);
+
+ mh_int_t k1, k2;
+ struct mh_i32ptr_node_t old_node, *old_node_ptr = &old_node;
+ const struct mh_i32ptr_node_t node = { new_func->def->fid, new_func };
+ size_t def_name_len = strlen(new_func->def->name);
+ uint32_t name_hash = mh_strn_hash(new_func->def->name, def_name_len);
const struct mh_strnptr_node_t strnode = {
- func->def->name, def_name_len, name_hash, func };
+ new_func->def->name, def_name_len, name_hash, new_func };
- mh_int_t k2 = mh_strnptr_put(funcs_by_name, &strnode, NULL, NULL);
+ k1 = mh_i32ptr_put(funcs, &node, &old_node_ptr, NULL);
+ if (k1 == mh_end(funcs))
+ goto error;
+ if (old_node_ptr != NULL)
+ *old_func = (struct func *)old_node_ptr->val;
+ k2 = mh_strnptr_put(funcs_by_name, &strnode, NULL, NULL);
if (k2 == mh_end(funcs_by_name)) {
mh_i32ptr_del(funcs, k1, NULL);
- func->def = NULL;
- func_delete(func);
goto error;
}
+ if (*old_func != NULL)
+ func_reuse_runtime(new_func, *old_func);
+ return;
+error:
+ panic_syserror("Out of memory for the data dictionary cache "
+ "(stored function).");
}
void
@@ -582,7 +577,6 @@ func_cache_delete(uint32_t fid)
strlen(func->def->name));
if (k != mh_end(funcs))
mh_strnptr_del(funcs_by_name, k, NULL);
- func_delete(func);
}
struct func *
diff --git a/src/box/schema.h b/src/box/schema.h
index 6f9a96117..368463536 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -163,14 +163,17 @@ schema_free();
struct space *schema_space(uint32_t id);
/**
- * Insert a new function or update the old one.
- *
- * @param def Function definition. In a case of success the ownership
- * of @a def is transfered to the data dictionary, thus the caller
- * must not delete it.
+ * Replace an existent (if any) function or insert a new one
+ * in function cache.
+ * @param func_new Function object to insert. In case of success,
+ * when old function object is exists, all loaded
+ * modules data are inherent with
+ * func_reuse_runtime() when possible.
+ * @param func_old[out] The replaced function object if any.
+ * @retval Returns 0 on success, -1 otherwise.
*/
void
-func_cache_replace(struct func_def *def);
+func_cache_replace(struct func *new_func, struct func **old_func);
void
func_cache_delete(uint32_t fid);
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 3/9] box: rework func cache update machinery
2019-06-06 12:03 ` [PATCH v2 3/9] box: rework func cache update machinery Kirill Shcherbatov
@ 2019-06-10 9:44 ` Vladimir Davydov
0 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 9:44 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Thu, Jun 06, 2019 at 03:03:59PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/func.c b/src/box/func.c
> index 098bc02b6..5051286a3 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -475,17 +475,18 @@ func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
> }
>
> void
> -func_update(struct func *func, struct func_def *def)
> +func_delete(struct func *func)
> {
> func_unload(func);
> free(func->def);
> - func->def = def;
> + free(func);
> }
>
> void
> -func_delete(struct func *func)
> +func_reuse_runtime(struct func *new_func, struct func *old_func)
> {
> - func_unload(func);
> - free(func->def);
> - free(func);
> + new_func->module = old_func->module;
> + new_func->func = old_func->func;
> + old_func->module = NULL;
> + old_func->func = NULL;
> }
If you look at the code coverage, you'll see that this function isn't
covered by any test, because we never update functions - only create or
drop them. Since this code leads to a new virtual function, let's simply
drop it?
I mean,
- raise ER_UNSUPPORTED in on_replace_dd_func/UPDATE,
- replace func_cache_replace with func_cache_insert, which would panic
if the function with the same id/name already exists.
This would simplify the code quite a bit. If anybody needs to alter the
definition of an existing function, we can do it later.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 4/9] box: rework func object as a function frontend
2019-06-06 12:03 [PATCH v2 0/9] box: rework functions machinery Kirill Shcherbatov
` (2 preceding siblings ...)
2019-06-06 12:03 ` [PATCH v2 3/9] box: rework func cache update machinery Kirill Shcherbatov
@ 2019-06-06 12:04 ` 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
` (5 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-06 12:04 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov
The function func object used to provide a call method only for
C functions. In scope of this patch it reworked to be a uniform
function call frontend both for C and Lua functions.
Needed for #4182, #1260
---
src/box/call.c | 136 +-----------------
src/box/call.h | 4 -
src/box/func.c | 337 ++++++++++++++++++++++++++++++++++++++++-----
src/box/func.h | 29 +++-
src/box/lua/call.c | 39 ------
5 files changed, 332 insertions(+), 213 deletions(-)
diff --git a/src/box/call.c b/src/box/call.c
index 982c95cbf..3e64118c9 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -43,149 +43,17 @@
#include "small/obuf.h"
#include "tt_static.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)
-{
- struct func *func = func_by_name(name, name_len);
- struct credentials *credentials = effective_user();
- /*
- * If the user has universal access, don't bother with checks.
- * No special check for ADMIN user is necessary
- * since ADMIN has universal access.
- */
- if ((credentials->universal_access & (PRIV_X | PRIV_U)) ==
- (PRIV_X | PRIV_U)) {
-
- *funcp = func;
- return 0;
- }
- user_access_t access = PRIV_X | PRIV_U;
- /* Check access for all functions. */
- access &= ~entity_access_get(SC_FUNCTION)[credentials->auth_token].effective;
- user_access_t func_access = access & ~credentials->universal_access;
- if (func == NULL ||
- /* Check for missing Usage access, ignore owner rights. */
- func_access & PRIV_U ||
- /* Check for missing specific access, respect owner rights. */
- (func->def->uid != credentials->uid &&
- func_access & ~func->access[credentials->auth_token].effective)) {
-
- /* Access violation, report error. */
- struct user *user = user_find(credentials->uid);
- if (user != NULL) {
- if (!(access & credentials->universal_access)) {
- diag_set(AccessDeniedError,
- priv_name(PRIV_U),
- schema_object_name(SC_UNIVERSE), "",
- user->def->name);
- } else {
- diag_set(AccessDeniedError,
- priv_name(PRIV_X),
- schema_object_name(SC_FUNCTION),
- tt_cstr(name, name_len),
- user->def->name);
- }
- }
- return -1;
- }
-
- *funcp = func;
- return 0;
-}
-
-static int
-box_c_call(struct func *func, struct call_request *request, struct port *port)
-{
- assert(func != NULL && func->def->language == FUNC_LANGUAGE_C);
-
- /* Create a call context */
- port_tuple_create(port);
- box_function_ctx_t ctx = { port };
-
- /* Clear all previous errors */
- diag_clear(&fiber()->diag);
- assert(!in_txn()); /* transaction is not started */
-
- /* Call function from the shared library */
- int rc = func_call(func, &ctx, request->args, request->args_end);
- func = NULL; /* May be deleted by DDL */
- if (rc != 0) {
- if (diag_last_error(&fiber()->diag) == NULL) {
- /* Stored procedure forget to set diag */
- diag_set(ClientError, ER_PROC_C, "unknown error");
- }
- port_destroy(port);
- return -1;
- }
- return 0;
-}
-
int
box_process_call(struct call_request *request, struct port *port)
{
rmean_collect(rmean_box, IPROTO_CALL, 1);
- /**
- * Find the function definition and check access.
- */
const char *name = request->name;
assert(name != NULL);
uint32_t name_len = mp_decode_strl(&name);
- struct func *func = NULL;
- /**
- * 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)
- return -1; /* permission denied */
-
- /**
- * Change the current user id if the function is
- * a set-definer-uid one. If the function is not
- * defined, it's obviously not a setuid one.
- */
- struct credentials *orig_credentials = NULL;
- if (func && func->def->setuid) {
- orig_credentials = effective_user();
- /* Remember and change the current user id. */
- if (func->owner_credentials.auth_token >= BOX_USER_MAX) {
- /*
- * Fill the cache upon first access, since
- * when func is created, no user may
- * be around to fill it (recovery of
- * system spaces from a snapshot).
- */
- struct user *owner = user_find(func->def->uid);
- if (owner == NULL)
- return -1;
- credentials_init(&func->owner_credentials,
- owner->auth_token,
- owner->def->uid);
- }
- fiber_set_user(fiber(), &func->owner_credentials);
- }
-
int rc;
- if (func && func->def->language == FUNC_LANGUAGE_C) {
- rc = box_c_call(func, request, port);
- } else {
- rc = box_lua_call(request, port);
- }
- /* Restore the original user */
- if (orig_credentials)
- fiber_set_user(fiber(), orig_credentials);
+ rc = box_func_execute_by_name(name, name_len, port, request->args,
+ request->args_end);
if (rc != 0) {
txn_rollback();
diff --git a/src/box/call.h b/src/box/call.h
index 4b3b8614c..05032c0d8 100644
--- a/src/box/call.h
+++ b/src/box/call.h
@@ -38,10 +38,6 @@ extern "C" {
struct port;
struct call_request;
-struct box_function_ctx {
- struct port *port;
-};
-
int
box_process_call(struct call_request *request, struct port *port);
diff --git a/src/box/func.c b/src/box/func.c
index 5051286a3..71c6bb6eb 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -32,7 +32,12 @@
#include "trivia/config.h"
#include "assoc.h"
#include "session.h"
+#include "lua/msgpack.h"
#include "lua/utils.h"
+#include "schema.h"
+#include "txn.h"
+#include "tt_static.h"
+#include "port.h"
#include "error.h"
#include "diag.h"
#include <dlfcn.h>
@@ -326,8 +331,8 @@ module_reload(const char *package, const char *package_end, struct module **modu
rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
struct func_name name;
func_split_name(func->def->name, &name);
- func->func = module_sym(new_module, name.sym);
- if (func->func == NULL)
+ func->c_func = module_sym(new_module, name.sym);
+ if (func->c_func == NULL)
goto restore;
func->module = new_module;
rlist_move(&new_module->funcs, &func->item);
@@ -347,8 +352,8 @@ restore:
do {
struct func_name name;
func_split_name(func->def->name, &name);
- func->func = module_sym(old_module, name.sym);
- if (func->func == NULL) {
+ func->c_func = module_sym(old_module, name.sym);
+ if (func->c_func == NULL) {
/*
* Something strange was happen, an early loaden
* function was not found in an old dso.
@@ -378,6 +383,25 @@ box_module_reload(const char *name)
return -1;
}
+/** Private virtual method table for func object. */
+struct func_vtab {
+ /** Load function runtime environment. */
+ int (*load)(struct func *func);
+ /** Unload function runtime environment. */
+ void (*unload)(struct func *func);
+ /** Call function with given arguments. */
+ int (*call)(struct func *func, struct port *port, const char *args,
+ const char *args_end);
+ /**
+ * Capture a given old_func's runtime and reuse in a given
+ * new function if possible.
+ */
+ void (*reuse_runtime)(struct func *new_func, struct func *old_func);
+};
+
+static struct func_vtab func_c_vtab;
+static struct func_vtab func_lua_vtab;
+
struct func *
func_new(struct func_def *def)
{
@@ -401,15 +425,44 @@ func_new(struct func_def *def)
* checks (see user_has_data()).
*/
func->owner_credentials.auth_token = BOX_USER_MAX; /* invalid value */
- func->func = NULL;
- func->module = NULL;
+ if (def->language == FUNC_LANGUAGE_C) {
+ func->c_func = NULL;
+ func->module = NULL;
+ func->vtab = &func_c_vtab;
+ /*
+ * Due to the fact that the function load cause
+ * loading of a shared library, this action is
+ * performed on demand: so we guarantee that it is
+ * performed by the user who has the authority
+ * to do it.
+ */
+ } else {
+ assert(def->language == FUNC_LANGUAGE_LUA);
+ func->vtab = &func_lua_vtab;
+ }
return func;
}
+void
+func_delete(struct func *func)
+{
+ func->vtab->unload(func);
+ free(func->def);
+ free(func);
+}
+
+void
+func_reuse_runtime(struct func *new_func, struct func *old_func)
+{
+ new_func->vtab->reuse_runtime(new_func, old_func);
+}
+
static void
-func_unload(struct func *func)
+func_c_unload(struct func *func)
{
- if (func->module) {
+ assert(func->vtab == &func_c_vtab);
+ assert(func != NULL && func->def->language == FUNC_LANGUAGE_C);
+ if (func->module != NULL) {
rlist_del(&func->item);
if (rlist_empty(&func->module->funcs)) {
struct func_name name;
@@ -418,8 +471,8 @@ func_unload(struct func *func)
}
module_gc(func->module);
}
+ func->c_func = NULL;
func->module = NULL;
- func->func = NULL;
}
/**
@@ -427,9 +480,10 @@ func_unload(struct func *func)
* symbol from it).
*/
static int
-func_load(struct func *func)
+func_c_load(struct func *func)
{
- assert(func->func == NULL);
+ assert(func != NULL && func->def->language == FUNC_LANGUAGE_C);
+ assert(func->vtab == &func_c_vtab);
struct func_name name;
func_split_name(func->def->name, &name);
@@ -447,46 +501,265 @@ func_load(struct func *func)
}
}
- func->func = module_sym(module, name.sym);
- if (func->func == NULL)
+ func->c_func = module_sym(module, name.sym);
+ if (func->c_func == NULL)
return -1;
func->module = module;
rlist_add(&module->funcs, &func->item);
return 0;
}
-int
-func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
- const char *args_end)
+static int
+func_c_call(struct func *func, struct port *port, const char *args,
+ const char *args_end)
{
- if (func->func == NULL) {
- if (func_load(func) != 0)
- return -1;
- }
+ assert(func != NULL && func->def->language == FUNC_LANGUAGE_C);
+ assert(func->vtab == &func_c_vtab);
+ assert(!in_txn());
+
+ port_tuple_create(port);
+ if (func->c_func == NULL && func_c_load(func) != 0)
+ return -1;
/* Module can be changed after function reload. */
struct module *module = func->module;
assert(module != NULL);
++module->calls;
- int rc = func->func(ctx, args, args_end);
+ box_function_ctx_t ctx = { port };
+ int rc = func->c_func(&ctx, args, args_end);
--module->calls;
module_gc(module);
+ if (rc != 0 && diag_last_error(&fiber()->diag) == NULL) {
+ /* Stored procedure forget to set diag */
+ diag_set(ClientError, ER_PROC_C, "unknown error");
+ }
return rc;
}
-void
-func_delete(struct func *func)
+static void
+func_c_reuse_runtime(struct func *new_func, struct func *old_func)
{
- func_unload(func);
- free(func->def);
- free(func);
-}
+ assert(new_func != NULL && new_func->def->language == FUNC_LANGUAGE_C);
+ assert(new_func->vtab == &func_c_vtab);
+ assert(old_func != NULL && old_func->def->language == FUNC_LANGUAGE_C);
+ assert(old_func->vtab == &func_c_vtab);
-void
-func_reuse_runtime(struct func *new_func, struct func *old_func)
-{
new_func->module = old_func->module;
- new_func->func = old_func->func;
+ new_func->c_func = old_func->c_func;
old_func->module = NULL;
- old_func->func = NULL;
+ old_func->c_func = NULL;
+}
+
+static struct func_vtab func_c_vtab = {
+ .load = func_c_load,
+ .unload = func_c_unload,
+ .call = func_c_call,
+ .reuse_runtime = func_c_reuse_runtime,
+};
+
+static void
+func_lua_unload(struct func *func)
+{
+ assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
+ assert(func->vtab == &func_lua_vtab);
+ (void) func;
+}
+
+struct func_lua_call_impl_ctx {
+ const char *args;
+ const char *args_end;
+ const char *func_name;
+ const char *func_name_end;
+};
+
+static int
+func_lua_call_impl_cb(struct lua_State *L)
+{
+ struct func_lua_call_impl_ctx *ctx =
+ (struct func_lua_call_impl_ctx *) lua_topointer(L, 1);
+ lua_settop(L, 0);
+
+ int oc = 0;
+ if (luaT_func_find(L, ctx->func_name, ctx->func_name_end, &oc) != 0) {
+ diag_set(ClientError, ER_NO_SUCH_PROC,
+ ctx->func_name_end - ctx->func_name, ctx->func_name);
+ return luaT_error(L);
+ }
+
+ const char *args = ctx->args;
+ uint32_t arg_count = mp_decode_array(&args);
+ luaL_checkstack(L, arg_count, "call: out of stack");
+ for (uint32_t i = 0; i < arg_count; i++)
+ luamp_decode(L, luaL_msgpack_default, &args);
+ lua_call(L, arg_count + oc - 1, LUA_MULTRET);
+ return lua_gettop(L);
+}
+
+static inline int
+func_lua_call_impl(lua_CFunction handler, struct func_lua_call_impl_ctx *ctx,
+ struct port *port)
+{
+ struct lua_State *L = lua_newthread(tarantool_L);
+ int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+ port_lua_create(port, L);
+ ((struct port_lua *) port)->ref = coro_ref;
+
+ lua_pushcfunction(L, handler);
+ lua_pushlightuserdata(L, ctx);
+ if (luaT_call(L, 1, LUA_MULTRET) != 0)
+ return -1;
+ return 0;
+}
+
+static int
+func_lua_load(struct func *func)
+{
+ assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
+ assert(func->vtab == &func_lua_vtab);
+ (void) func;
+ return 0;
+}
+
+static inline int
+func_lua_call(struct func *func, struct port *port, const char *args,
+ const char *args_end)
+{
+ assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
+ assert(func->vtab == &func_lua_vtab);
+
+ struct func_lua_call_impl_ctx ctx;
+ ctx.func_name = func->def->name;
+ ctx.func_name_end = func->def->name + strlen(func->def->name);
+ ctx.args = args;
+ ctx.args_end = args_end;
+ return func_lua_call_impl(func_lua_call_impl_cb, &ctx, port);
+}
+
+static void
+func_lua_reuse_runtime(struct func *new_func, struct func *old_func)
+{
+ assert(new_func != NULL && new_func->def->language == FUNC_LANGUAGE_LUA);
+ assert(new_func->vtab == &func_c_vtab);
+ assert(old_func != NULL && old_func->def->language == FUNC_LANGUAGE_LUA);
+ assert(old_func->vtab == &func_c_vtab);
+ (void)new_func;
+ (void)old_func;
+}
+
+static struct func_vtab func_lua_vtab = {
+ .load = func_lua_load,
+ .unload = func_lua_unload,
+ .call = func_lua_call,
+ .reuse_runtime = func_lua_reuse_runtime,
+};
+
+/** Check "EXECUTE" permissions for a given function. */
+static int
+func_access_check(struct func *func, const char *func_name)
+{
+ struct credentials *credentials = effective_user();
+ /*
+ * If the user has universal access, don't bother with checks.
+ * No special check for ADMIN user is necessary
+ * since ADMIN has universal access.
+ */
+ if ((credentials->universal_access & (PRIV_X | PRIV_U)) ==
+ (PRIV_X | PRIV_U))
+ return 0;
+ user_access_t access = PRIV_X | PRIV_U;
+ /* Check access for all functions. */
+ access &= ~entity_access_get(SC_FUNCTION)[credentials->auth_token].effective;
+ user_access_t func_access = access & ~credentials->universal_access;
+ if (func == NULL ||
+ /* Check for missing Usage access, ignore owner rights. */
+ func_access & PRIV_U ||
+ /* Check for missing specific access, respect owner rights. */
+ (func->def->uid != credentials->uid &&
+ func_access & ~func->access[credentials->auth_token].effective)) {
+ /* Access violation, report error. */
+ struct user *user = user_find(credentials->uid);
+ if (user != NULL) {
+ if (!(access & credentials->universal_access)) {
+ diag_set(AccessDeniedError, priv_name(PRIV_U),
+ schema_object_name(SC_UNIVERSE), "",
+ user->def->name);
+ } else {
+ diag_set(AccessDeniedError, priv_name(PRIV_X),
+ schema_object_name(SC_FUNCTION),
+ func_name, user->def->name);
+ }
+ }
+ return -1;
+ }
+ return 0;
+}
+
+int
+func_execute(struct func *func, struct port *port, const char *args,
+ const char *args_end)
+{
+ assert(func_access_check(func, func->def->name) == 0);
+ /**
+ * Change the current user id if the function is
+ * a set-definer-uid one. If the function is not
+ * defined, it's obviously not a setuid one.
+ */
+ struct credentials *orig_credentials = NULL;
+ if (func->def->setuid) {
+ orig_credentials = effective_user();
+ /* Remember and change the current user id. */
+ if (func->owner_credentials.auth_token >= BOX_USER_MAX) {
+ /*
+ * Fill the cache upon first access, since
+ * when func is created, no user may
+ * be around to fill it (recovery of
+ * system spaces from a snapshot).
+ */
+ struct user *owner = user_find(func->def->uid);
+ if (owner == NULL)
+ return -1;
+ credentials_init(&func->owner_credentials,
+ owner->auth_token,
+ owner->def->uid);
+ }
+ fiber_set_user(fiber(), &func->owner_credentials);
+ }
+ int rc = func->vtab->call(func, port, args, args_end);
+ if (rc != 0)
+ port_destroy(port);
+ /* Restore the original user */
+ if (orig_credentials)
+ fiber_set_user(fiber(), orig_credentials);
+ return rc;
+}
+
+int
+box_func_execute_by_name(const char *name, uint32_t name_len, struct port *port,
+ const char *args, const char *args_end)
+{
+ struct func *func = func_by_name(name, name_len);
+ if (func_access_check(func, tt_cstr(name, name_len)) != 0)
+ return -1;
+ /* Clear all previous errors */
+ diag_clear(&fiber()->diag);
+ int rc = 0;
+ if (func != NULL) {
+ rc = func_execute(func, port, args, args_end);
+ } else {
+ /*
+ * A user that has "universe" access may call
+ * any Lua function, even not registered when
+ * it exists.
+ */
+ struct func_lua_call_impl_ctx ctx;
+ ctx.func_name = name;
+ ctx.func_name_end = name + name_len;
+ ctx.args = args;
+ ctx.args_end = args_end;
+ rc = func_lua_call_impl(func_lua_call_impl_cb, &ctx, port);
+ if (rc != 0)
+ port_destroy(port);
+ }
+ assert(rc == 0 || diag_last_error(&fiber()->diag) != NULL);
+ return rc;
}
diff --git a/src/box/func.h b/src/box/func.h
index 82ac046b7..931718bba 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -42,6 +42,9 @@
extern "C" {
#endif /* defined(__cplusplus) */
+struct port;
+struct func_vtab;
+
/**
* Dynamic shared module.
*/
@@ -61,6 +64,8 @@ struct module {
*/
struct func {
struct func_def *def;
+ /** Virtual method table. */
+ struct func_vtab *vtab;
/**
* Anchor for module membership.
*/
@@ -68,7 +73,7 @@ struct func {
/**
* For C functions, the body of the function.
*/
- box_function_f func;
+ box_function_f c_func;
/**
* Each stored function keeps a handle to the
* dynamic library for the C callback.
@@ -97,6 +102,10 @@ module_init(void);
void
module_free(void);
+struct box_function_ctx {
+ struct port *port;
+};
+
struct func *
func_new(struct func_def *def);
@@ -104,11 +113,12 @@ void
func_delete(struct func *func);
/**
- * Call stored C function using @a args.
+ * Execute given function.
+ * The current user must have access to execute it.
*/
int
-func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
- const char *args_end);
+func_execute(struct func *func, struct port *port, const char *args,
+ const char *args_end);
/**
* Capture a given old_func's runtime an reuse in a given new
@@ -127,6 +137,17 @@ func_reuse_runtime(struct func *new_func, struct func *old_func);
int
box_module_reload(const char *name);
+/**
+ * Check the current user privileges and execute a Tarantool
+ * function by the given name.
+ * When function object is not registered in function cache and
+ * the user has "universe" privileges, lookup for a given name
+ * in a Lua global namespace and execute when exists.
+ */
+int
+box_func_execute_by_name(const char *name, uint32_t name_len, struct port *port,
+ const char *args, const char *args_end);
+
#if defined(__cplusplus)
} /* extern "C" */
#endif /* defined(__cplusplus) */
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 9edb5bf7e..4d4521363 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -221,39 +221,6 @@ port_lua_create(struct port *port, struct lua_State *L)
port_lua->ref = -1;
}
-static int
-execute_lua_call(lua_State *L)
-{
- struct call_request *request = (struct call_request *)
- lua_topointer(L, 1);
- lua_settop(L, 0); /* clear the stack to simplify the logic below */
-
- const char *name = request->name;
- uint32_t name_len = mp_decode_strl(&name);
-
- /*
- * How many objects are on stack after
- * luaT_func_find call.
- */
- int oc = 0;
- /* Try to find a function by name in Lua */
- if (luaT_func_find(L, name, name + name_len, &oc) != 0) {
- diag_set(ClientError, ER_NO_SUCH_PROC, name_len, name);
- return luaT_error(L);
- }
-
- /* Push the rest of args (a tuple). */
- const char *args = request->args;
-
- uint32_t arg_count = mp_decode_array(&args);
- luaL_checkstack(L, arg_count, "call: out of stack");
-
- for (uint32_t i = 0; i < arg_count; i++)
- luamp_decode(L, luaL_msgpack_default, &args);
- lua_call(L, arg_count + oc - 1, LUA_MULTRET);
- return lua_gettop(L);
-}
-
static int
execute_lua_eval(lua_State *L)
{
@@ -396,12 +363,6 @@ box_process_lua(struct call_request *request, struct port *base,
return 0;
}
-int
-box_lua_call(struct call_request *request, struct port *port)
-{
- return box_process_lua(request, port, execute_lua_call);
-}
-
int
box_lua_eval(struct call_request *request, struct port *port)
{
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/9] box: rework func object as a function frontend
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
0 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 10:32 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Thu, Jun 06, 2019 at 03:04:00PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/func.c b/src/box/func.c
> +/** Private virtual method table for func object. */
> +struct func_vtab {
> + /** Load function runtime environment. */
> + int (*load)(struct func *func);
'load' could be a part of 'call' or constructor.
> + /** Unload function runtime environment. */
> + void (*unload)(struct func *func);
Let's call this 'destroy' and call it from func_delete.
> + /** Call function with given arguments. */
> + int (*call)(struct func *func, struct port *port, const char *args,
> + const char *args_end);
> + /**
> + * Capture a given old_func's runtime and reuse in a given
> + * new function if possible.
> + */
> + void (*reuse_runtime)(struct func *new_func, struct func *old_func);
> +};
If we drop func_update, we won't need this method.
> +int
> +box_func_execute_by_name(const char *name, uint32_t name_len, struct port *port,
> + const char *args, const char *args_end)
> +{
I think this should live in src/box/call.c:
box_process_call:
- look up function by name
- if found call it
- otherwise check X permission on Universe and call function by
name using the corresponding helper defined in box/lua/call.c.
> diff --git a/src/box/func.h b/src/box/func.h
> index 82ac046b7..931718bba 100644
> --- a/src/box/func.h
> +++ b/src/box/func.h
> @@ -42,6 +42,9 @@
> extern "C" {
> #endif /* defined(__cplusplus) */
>
> +struct port;
> +struct func_vtab;
> +
> /**
> * Dynamic shared module.
> */
> @@ -61,6 +64,8 @@ struct module {
> */
> struct func {
> struct func_def *def;
> + /** Virtual method table. */
> + struct func_vtab *vtab;
> /**
> * Anchor for module membership.
> */
> @@ -68,7 +73,7 @@ struct func {
> /**
> * For C functions, the body of the function.
> */
> - box_function_f func;
> + box_function_f c_func;
I think it would be better to inherit the base struct so as not to mix
C/Lua stuff in the same struct:
struct func {
struct func_vtab *def;
...
};
struct func_c {
struct func base;
box_function_f func;
struct rlist in_module;
...
};
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 5/9] schema: rework _func system space format
2019-06-06 12:03 [PATCH v2 0/9] box: rework functions machinery Kirill Shcherbatov
` (3 preceding siblings ...)
2019-06-06 12:04 ` [PATCH v2 4/9] box: rework func object as a function frontend Kirill Shcherbatov
@ 2019-06-06 12:04 ` 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
` (4 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-06 12:04 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov
This patch updates a _func system space to prepare it for
working with persistent functions. The format of the _func
system space is
[<id> UINT, <owner> UINT, <name> STR, <setuid> UINT,
<language> STR, <body> STR, <returns> STR,
<is_deterministic> BOOL, <opts> MAP]
It is preparational step to introduce persistent Lua function
in Tarantool.
The new <body> field is a string that represents the function
body, returns is the type of value that it returns, and the
is_deterministic flag is responsible whether this routine
deterministic or not (can produce only one result for a given
list of parameters).
Updated function definition structure, decode operation and
migration script correspondingly.
Part of #4182
Needed for #1260
---
src/box/alter.cc | 61 ++++++++++++++++++++++++++++-------
src/box/bootstrap.snap | Bin 4393 -> 4449 bytes
src/box/func_def.h | 14 ++++++--
src/box/lua/schema.lua | 13 ++++++--
src/box/lua/upgrade.lua | 25 +++++++++++++-
src/box/schema_def.h | 4 +++
test/box-py/bootstrap.result | 6 ++--
test/box/access_misc.result | 6 ++--
8 files changed, 108 insertions(+), 21 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 3b57a7d82..11cad77c3 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2426,26 +2426,45 @@ func_def_get_ids_from_tuple(struct tuple *tuple, uint32_t *fid, uint32_t *uid)
static struct func_def *
func_def_new_from_tuple(struct tuple *tuple)
{
- uint32_t len;
- const char *name = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_NAME,
- &len);
- if (len > BOX_NAME_MAX)
+ uint32_t field_count = tuple_field_count(tuple);
+ uint32_t name_len, body_len;
+ const char *name, *body;
+ name = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_NAME, &name_len);
+ if (name_len > BOX_NAME_MAX) {
tnt_raise(ClientError, ER_CREATE_FUNCTION,
tt_cstr(name, BOX_INVALID_NAME_MAX),
"function name is too long");
- identifier_check_xc(name, len);
- struct func_def *def = (struct func_def *) malloc(func_def_sizeof(len));
+ }
+ identifier_check_xc(name, name_len);
+ if (field_count > BOX_FUNC_FIELD_BODY) {
+ body = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_BODY,
+ &body_len);
+ } else {
+ body = NULL;
+ body_len = 0;
+ }
+
+ uint32_t def_sz = func_def_sizeof(name_len, body_len);
+ struct func_def *def = (struct func_def *) malloc(def_sz);
if (def == NULL)
- tnt_raise(OutOfMemory, func_def_sizeof(len), "malloc", "def");
+ tnt_raise(OutOfMemory, def_sz, "malloc", "def");
auto def_guard = make_scoped_guard([=] { free(def); });
func_def_get_ids_from_tuple(tuple, &def->fid, &def->uid);
- memcpy(def->name, name, len);
- def->name[len] = 0;
- if (tuple_field_count(tuple) > BOX_FUNC_FIELD_SETUID)
+ memcpy(def->name, name, name_len);
+ def->name[name_len] = 0;
+ if (body_len > 0) {
+ def->body = def->name + name_len + 1;
+ memcpy(def->body, body, body_len);
+ def->body[body_len] = 0;
+ } else {
+ def->body = NULL;
+ }
+
+ if (field_count > BOX_FUNC_FIELD_SETUID)
def->setuid = tuple_field_u32_xc(tuple, BOX_FUNC_FIELD_SETUID);
else
def->setuid = false;
- if (tuple_field_count(tuple) > BOX_FUNC_FIELD_LANGUAGE) {
+ if (field_count > BOX_FUNC_FIELD_LANGUAGE) {
const char *language =
tuple_field_cstr_xc(tuple, BOX_FUNC_FIELD_LANGUAGE);
def->language = STR2ENUM(func_language, language);
@@ -2457,6 +2476,26 @@ func_def_new_from_tuple(struct tuple *tuple)
/* Lua is the default. */
def->language = FUNC_LANGUAGE_LUA;
}
+ if (def->language != FUNC_LANGUAGE_LUA && body_len > 0) {
+ tnt_raise(ClientError, ER_CREATE_FUNCTION, name,
+ "function body may be specified only for "
+ "Lua language");
+ }
+ if (field_count > BOX_FUNC_FIELD_BODY) {
+ assert(field_count > BOX_FUNC_FIELD_OPTS);
+ const char *type =
+ tuple_field_cstr_xc(tuple, BOX_FUNC_FIELD_RETURNS);
+ def->returns = STR2ENUM(field_type, type);
+ if (def->returns == field_type_MAX) {
+ tnt_raise(ClientError, ER_CREATE_FUNCTION, name,
+ "function return value has unknown field type");
+ }
+ def->is_deterministic =
+ tuple_field_bool_xc(tuple, BOX_FUNC_FIELD_IS_DETERMINISTIC);
+ } else {
+ def->returns = FIELD_TYPE_ANY;
+ def->is_deterministic = false;
+ }
def_guard.is_active = false;
return def;
}
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index bb8fbeba114b1e72a5585548fb7f22796931d90f..440495684401541854e9d9180f55cc9a4bc45b25 100644
diff --git a/src/box/func_def.h b/src/box/func_def.h
index 5b52ab498..7a920f65e 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -32,6 +32,7 @@
*/
#include "trivia/util.h"
+#include "field_def.h"
#include <stdbool.h>
/**
@@ -54,11 +55,17 @@ struct func_def {
uint32_t fid;
/** Owner of the function. */
uint32_t uid;
+ /** Definition of the persistent function. */
+ char *body;
/**
* True if the function requires change of user id before
* invocation.
*/
bool setuid;
+ /** The type of the returned value. */
+ enum field_type returns;
+ /** Whether this function is deterministic. */
+ bool is_deterministic;
/**
* The language of the stored function.
*/
@@ -73,10 +80,13 @@ struct func_def {
* for a function of length @a a name_len.
*/
static inline size_t
-func_def_sizeof(uint32_t name_len)
+func_def_sizeof(uint32_t name_len, uint32_t body_len)
{
/* +1 for '\0' name terminating. */
- return sizeof(struct func_def) + name_len + 1;
+ size_t sz = sizeof(struct func_def) + name_len + 1;
+ if (body_len > 0)
+ sz += body_len + 1;
+ return sz;
}
/**
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 39bd8da6d..b5393e19a 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2105,7 +2105,9 @@ box.schema.func.create = function(name, opts)
opts = opts or {}
check_param_table(opts, { setuid = 'boolean',
if_not_exists = 'boolean',
- language = 'string'})
+ language = 'string', body = 'string',
+ returns = 'string', is_deterministic = 'boolean',
+ opts = 'table'} )
local _func = box.space[box.schema.FUNC_ID]
local _vfunc = box.space[box.schema.VFUNC_ID]
local func = _vfunc.index.name:get{name}
@@ -2115,10 +2117,15 @@ box.schema.func.create = function(name, opts)
end
return
end
- opts = update_param_table(opts, { setuid = false, language = 'lua'})
+ opts = update_param_table(opts, { setuid = false, language = 'lua',
+ body = '', returns = 'any',
+ is_deterministic = false,
+ opts = setmap{}})
opts.language = string.upper(opts.language)
opts.setuid = opts.setuid and 1 or 0
- _func:auto_increment{session.euid(), name, opts.setuid, opts.language}
+ _func:auto_increment{session.euid(), name, opts.setuid, opts.language,
+ opts.body, opts.returns, opts.is_deterministic,
+ opts.opts}
end
box.schema.func.drop = function(name, opts)
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 070662698..6f35e0b5a 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -324,7 +324,8 @@ local function initial_1_7_5()
-- create "box.schema.user.info" function
log.info('create function "box.schema.user.info" with setuid')
- _func:replace{1, ADMIN, 'box.schema.user.info', 1, 'LUA'}
+ _func:replace{1, ADMIN, 'box.schema.user.info', 1, 'LUA', '', 'any',
+ false, MAP}
-- grant 'public' role access to 'box.schema.user.info' function
log.info('grant execute on function "box.schema.user.info" to public')
@@ -774,8 +775,30 @@ local function upgrade_sequence_to_2_2_1()
_space_sequence:format(format)
end
+local function upgrade_func_to_2_2_1()
+ log.info("Update _func format")
+ local _func = box.space[box.schema.FUNC_ID]
+ local format = {}
+ format[1] = {name='id', type='unsigned'}
+ format[2] = {name='owner', type='unsigned'}
+ format[3] = {name='name', type='string'}
+ format[4] = {name='setuid', type='unsigned'}
+ format[5] = {name='language', type='string'}
+ format[6] = {name='body', type='string'}
+ format[7] = {name='returns', type='string'}
+ format[8] = {name='is_deterministic', type='boolean'}
+ format[9] = {name='opts', type='map'}
+ for _, v in box.space._func:pairs() do
+ _ = box.space._func:replace({v.id, v.owner, v.name, v.setuid,
+ v[5] or 'LUA', '', 'any', false,
+ setmap({})})
+ end
+ _func:format(format)
+end
+
local function upgrade_to_2_2_1()
upgrade_sequence_to_2_2_1()
+ upgrade_func_to_2_2_1()
end
--------------------------------------------------------------------------------
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index b817b49f6..b8cbdb12e 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -163,6 +163,10 @@ enum {
BOX_FUNC_FIELD_NAME = 2,
BOX_FUNC_FIELD_SETUID = 3,
BOX_FUNC_FIELD_LANGUAGE = 4,
+ BOX_FUNC_FIELD_BODY = 5,
+ BOX_FUNC_FIELD_RETURNS = 6,
+ BOX_FUNC_FIELD_IS_DETERMINISTIC = 7,
+ BOX_FUNC_FIELD_OPTS = 8,
};
/** _collation fields. */
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0684914c0..305bb9276 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -49,7 +49,9 @@ box.space._space:select{}
'type': 'string'}, {'name': 'opts', 'type': 'map'}, {'name': 'parts', 'type': 'array'}]]
- [296, 1, '_func', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
- 'type': 'unsigned'}]]
+ 'type': 'unsigned'}, {'name': 'language', 'type': 'string'}, {'name': 'body',
+ 'type': 'string'}, {'name': 'returns', 'type': 'string'}, {'name': 'is_deterministic',
+ 'type': 'boolean'}, {'name': 'opts', 'type': 'map'}]]
- [297, 1, '_vfunc', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
'type': 'unsigned'}]]
@@ -142,7 +144,7 @@ box.space._user:select{}
...
box.space._func:select{}
---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA']
+- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'any', false, {}]
...
box.space._priv:select{}
---
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 24bdd9d63..6e4558a19 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -789,7 +789,9 @@ box.space._space:select()
'type': 'string'}, {'name': 'opts', 'type': 'map'}, {'name': 'parts', 'type': 'array'}]]
- [296, 1, '_func', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
- 'type': 'unsigned'}]]
+ 'type': 'unsigned'}, {'name': 'language', 'type': 'string'}, {'name': 'body',
+ 'type': 'string'}, {'name': 'returns', 'type': 'string'}, {'name': 'is_deterministic',
+ 'type': 'boolean'}, {'name': 'opts', 'type': 'map'}]]
- [297, 1, '_vfunc', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
'type': 'unsigned'}]]
@@ -822,7 +824,7 @@ box.space._space:select()
...
box.space._func:select()
---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA']
+- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'any', false, {}]
...
session = nil
---
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/9] schema: rework _func system space format
2019-06-06 12:04 ` [PATCH v2 5/9] schema: rework _func system space format Kirill Shcherbatov
@ 2019-06-10 12:10 ` Vladimir Davydov
0 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 12:10 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Thu, Jun 06, 2019 at 03:04:01PM +0300, Kirill Shcherbatov wrote:
> This patch updates a _func system space to prepare it for
> working with persistent functions. The format of the _func
> system space is
> [<id> UINT, <owner> UINT, <name> STR, <setuid> UINT,
> <language> STR, <body> STR, <returns> STR,
> <is_deterministic> BOOL, <opts> MAP]
>
> It is preparational step to introduce persistent Lua function
> in Tarantool.
>
> The new <body> field is a string that represents the function
> body, returns is the type of value that it returns, and the
> is_deterministic flag is responsible whether this routine
> deterministic or not (can produce only one result for a given
> list of parameters).
>
> Updated function definition structure, decode operation and
> migration script correspondingly.
>
> Part of #4182
> Needed for #1260
> ---
> src/box/alter.cc | 61 ++++++++++++++++++++++++++++-------
> src/box/bootstrap.snap | Bin 4393 -> 4449 bytes
> src/box/func_def.h | 14 ++++++--
> src/box/lua/schema.lua | 13 ++++++--
> src/box/lua/upgrade.lua | 25 +++++++++++++-
> src/box/schema_def.h | 4 +++
> test/box-py/bootstrap.result | 6 ++--
> test/box/access_misc.result | 6 ++--
> 8 files changed, 108 insertions(+), 21 deletions(-)
>
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 3b57a7d82..11cad77c3 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2426,26 +2426,45 @@ func_def_get_ids_from_tuple(struct tuple *tuple, uint32_t *fid, uint32_t *uid)
> static struct func_def *
> func_def_new_from_tuple(struct tuple *tuple)
> {
> - uint32_t len;
> - const char *name = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_NAME,
> - &len);
> - if (len > BOX_NAME_MAX)
> + uint32_t field_count = tuple_field_count(tuple);
> + uint32_t name_len, body_len;
> + const char *name, *body;
> + name = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_NAME, &name_len);
> + if (name_len > BOX_NAME_MAX) {
> tnt_raise(ClientError, ER_CREATE_FUNCTION,
> tt_cstr(name, BOX_INVALID_NAME_MAX),
> "function name is too long");
> - identifier_check_xc(name, len);
> - struct func_def *def = (struct func_def *) malloc(func_def_sizeof(len));
> + }
> + identifier_check_xc(name, name_len);
> + if (field_count > BOX_FUNC_FIELD_BODY) {
> + body = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_BODY,
> + &body_len);
> + } else {
> + body = NULL;
> + body_len = 0;
> + }
> +
> + uint32_t def_sz = func_def_sizeof(name_len, body_len);
> + struct func_def *def = (struct func_def *) malloc(def_sz);
> if (def == NULL)
> - tnt_raise(OutOfMemory, func_def_sizeof(len), "malloc", "def");
> + tnt_raise(OutOfMemory, def_sz, "malloc", "def");
> auto def_guard = make_scoped_guard([=] { free(def); });
> func_def_get_ids_from_tuple(tuple, &def->fid, &def->uid);
> - memcpy(def->name, name, len);
> - def->name[len] = 0;
> - if (tuple_field_count(tuple) > BOX_FUNC_FIELD_SETUID)
> + memcpy(def->name, name, name_len);
> + def->name[name_len] = 0;
> + if (body_len > 0) {
> + def->body = def->name + name_len + 1;
> + memcpy(def->body, body, body_len);
> + def->body[body_len] = 0;
> + } else {
> + def->body = NULL;
> + }
> +
> + if (field_count > BOX_FUNC_FIELD_SETUID)
> def->setuid = tuple_field_u32_xc(tuple, BOX_FUNC_FIELD_SETUID);
> else
> def->setuid = false;
> - if (tuple_field_count(tuple) > BOX_FUNC_FIELD_LANGUAGE) {
> + if (field_count > BOX_FUNC_FIELD_LANGUAGE) {
> const char *language =
> tuple_field_cstr_xc(tuple, BOX_FUNC_FIELD_LANGUAGE);
> def->language = STR2ENUM(func_language, language);
> @@ -2457,6 +2476,26 @@ func_def_new_from_tuple(struct tuple *tuple)
> /* Lua is the default. */
> def->language = FUNC_LANGUAGE_LUA;
> }
> + if (def->language != FUNC_LANGUAGE_LUA && body_len > 0) {
> + tnt_raise(ClientError, ER_CREATE_FUNCTION, name,
> + "function body may be specified only for "
> + "Lua language");
> + }
This check should live in the specific func constructor so that we don't
need to touch it when we introduce SQL functions.
> + if (field_count > BOX_FUNC_FIELD_BODY) {
> + assert(field_count > BOX_FUNC_FIELD_OPTS);
> + const char *type =
> + tuple_field_cstr_xc(tuple, BOX_FUNC_FIELD_RETURNS);
> + def->returns = STR2ENUM(field_type, type);
> + if (def->returns == field_type_MAX) {
> + tnt_raise(ClientError, ER_CREATE_FUNCTION, name,
> + "function return value has unknown field type");
> + }
> + def->is_deterministic =
> + tuple_field_bool_xc(tuple, BOX_FUNC_FIELD_IS_DETERMINISTIC);
> + } else {
> + def->returns = FIELD_TYPE_ANY;
> + def->is_deterministic = false;
> + }
> def_guard.is_active = false;
> return def;
> }
> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> index bb8fbeba114b1e72a5585548fb7f22796931d90f..440495684401541854e9d9180f55cc9a4bc45b25 100644
>
> diff --git a/src/box/func_def.h b/src/box/func_def.h
> index 5b52ab498..7a920f65e 100644
> --- a/src/box/func_def.h
> +++ b/src/box/func_def.h
> @@ -32,6 +32,7 @@
> */
>
> #include "trivia/util.h"
> +#include "field_def.h"
> #include <stdbool.h>
>
> /**
> @@ -54,11 +55,17 @@ struct func_def {
> uint32_t fid;
> /** Owner of the function. */
> uint32_t uid;
> + /** Definition of the persistent function. */
> + char *body;
> /**
> * True if the function requires change of user id before
> * invocation.
> */
> bool setuid;
> + /** The type of the returned value. */
> + enum field_type returns;
As discussed verbally, the only reason we need 'returns' now is to
differentiate between multikey and unikey functional indexes and so it
can be either 'array' or something else we don't care what exactly. We
don't check function return value against this field. This doesn't look
right to me.
IMO creating a multikey index judging by a function definition looks
indirect and confusing - after all whether an index is multikey or not
is a property of the index itself or, to be more exact, of the
relationship between the index and the function. That being said, I
think we better drop 'returns' property for now and instead allow to
specify is_multikey flag when creating a functional index, like this:
box.space.my_space:create_index('my_index',
{function = {id = 'my_func', is_multikey = true}})
Let's add 'returns' (and possibly 'args') when we really need to check
function return value.
> + /** Whether this function is deterministic. */
> + bool is_deterministic;
Please try to keep bools together unless it hurts readability as this
reduces the struct size.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 6/9] box: load persistent Lua functions on creation
2019-06-06 12:03 [PATCH v2 0/9] box: rework functions machinery Kirill Shcherbatov
` (4 preceding siblings ...)
2019-06-06 12:04 ` [PATCH v2 5/9] schema: rework _func system space format Kirill Shcherbatov
@ 2019-06-06 12:04 ` 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
` (3 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-06 12:04 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov
This patch proceed persistent Lua function load on function
object creation.
Part of #4182
Needed for #1260
---
src/box/func.c | 77 +++++++++++++++--
src/box/func.h | 30 +++++--
src/box/func_def.h | 6 ++
test/box/persistent_func.result | 136 ++++++++++++++++++++++++++++++
test/box/persistent_func.test.lua | 61 ++++++++++++++
5 files changed, 292 insertions(+), 18 deletions(-)
create mode 100644 test/box/persistent_func.result
create mode 100644 test/box/persistent_func.test.lua
diff --git a/src/box/func.c b/src/box/func.c
index 71c6bb6eb..b21221d9e 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -438,6 +438,7 @@ func_new(struct func_def *def)
*/
} else {
assert(def->language == FUNC_LANGUAGE_LUA);
+ func->lua_ref = LUA_REFNIL;
func->vtab = &func_lua_vtab;
}
return func;
@@ -562,12 +563,14 @@ func_lua_unload(struct func *func)
{
assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
assert(func->vtab == &func_lua_vtab);
- (void) func;
+ luaL_unref(tarantool_L, LUA_REGISTRYINDEX, func->lua_ref);
+ func->lua_ref = LUA_REFNIL;
}
struct func_lua_call_impl_ctx {
const char *args;
const char *args_end;
+ struct func *func;
const char *func_name;
const char *func_name_end;
};
@@ -579,11 +582,17 @@ func_lua_call_impl_cb(struct lua_State *L)
(struct func_lua_call_impl_ctx *) lua_topointer(L, 1);
lua_settop(L, 0);
- int oc = 0;
- if (luaT_func_find(L, ctx->func_name, ctx->func_name_end, &oc) != 0) {
- diag_set(ClientError, ER_NO_SUCH_PROC,
- ctx->func_name_end - ctx->func_name, ctx->func_name);
- return luaT_error(L);
+ int oc = 1;
+ if (ctx->func != NULL && func_def_is_persistent(ctx->func->def)) {
+ assert(ctx->func->lua_ref != LUA_REFNIL);
+ lua_rawgeti(L, LUA_REGISTRYINDEX, ctx->func->lua_ref);
+ } else {
+ if (luaT_func_find(L, ctx->func_name,
+ ctx->func_name_end, &oc) != 0) {
+ diag_set(ClientError, ER_NO_SUCH_PROC,
+ ctx->func_name_end - ctx->func_name, ctx->func_name);
+ return luaT_error(L);
+ }
}
const char *args = ctx->args;
@@ -611,12 +620,53 @@ func_lua_call_impl(lua_CFunction handler, struct func_lua_call_impl_ctx *ctx,
return 0;
}
+static int
+func_lua_load_cb(struct lua_State *L)
+{
+ struct func *func = (struct func *) lua_topointer(L, 1);
+ assert(func->lua_ref == LUA_REFNIL);
+ lua_settop(L, 0);
+
+ struct region *region = &fiber()->gc;
+ size_t region_svp = region_used(region);
+ const char *load_pref = "return ";
+ uint32_t load_str_sz = strlen(load_pref) + strlen(func->def->body) + 1;
+ char *load_str = region_alloc(region, load_str_sz);
+ if (load_str == NULL) {
+ diag_set(OutOfMemory, load_str_sz, "region", "load_str");
+ goto error;
+ }
+ sprintf(load_str, "%s%s", load_pref, func->def->body);
+ if (luaL_loadstring(L, load_str) != 0 ||
+ luaT_call(L, 0, 1) != 0 || !lua_isfunction(L, -1)) {
+ diag_set(ClientError, ER_LOAD_FUNCTION, func->def->name,
+ func->def->body);
+ goto error;
+ }
+ func->lua_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+ region_truncate(region, region_svp);
+ return lua_gettop(L);
+error:
+ region_truncate(region, region_svp);
+ return luaT_error(L);
+}
+
static int
func_lua_load(struct func *func)
{
assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
assert(func->vtab == &func_lua_vtab);
- (void) func;
+ assert(func->lua_ref == LUA_REFNIL);
+
+ struct lua_State *L = lua_newthread(tarantool_L);
+ int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+ lua_pushcfunction(L, func_lua_load_cb);
+ lua_pushlightuserdata(L, func);
+ int rc = luaT_call(L, 1, LUA_MULTRET);
+ luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
+ if (rc != 0)
+ return -1;
+ assert(func->lua_ref != LUA_REFNIL);
return 0;
}
@@ -627,7 +677,12 @@ func_lua_call(struct func *func, struct port *port, const char *args,
assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
assert(func->vtab == &func_lua_vtab);
+ if (func_def_is_persistent(func->def) && func->lua_ref == LUA_REFNIL &&
+ func_lua_load(func) != 0)
+ return -1;
+
struct func_lua_call_impl_ctx ctx;
+ ctx.func = func;
ctx.func_name = func->def->name;
ctx.func_name_end = func->def->name + strlen(func->def->name);
ctx.args = args;
@@ -642,8 +697,11 @@ func_lua_reuse_runtime(struct func *new_func, struct func *old_func)
assert(new_func->vtab == &func_c_vtab);
assert(old_func != NULL && old_func->def->language == FUNC_LANGUAGE_LUA);
assert(old_func->vtab == &func_c_vtab);
- (void)new_func;
- (void)old_func;
+ if (old_func->def->body != NULL && new_func->def->body != NULL &&
+ strcmp(old_func->def->body, new_func->def->body) == 0) {
+ new_func->lua_ref = old_func->lua_ref;
+ old_func->lua_ref = LUA_REFNIL;
+ }
}
static struct func_vtab func_lua_vtab = {
@@ -752,6 +810,7 @@ box_func_execute_by_name(const char *name, uint32_t name_len, struct port *port,
* it exists.
*/
struct func_lua_call_impl_ctx ctx;
+ ctx.func = NULL;
ctx.func_name = name;
ctx.func_name_end = name + name_len;
ctx.args = args;
diff --git a/src/box/func.h b/src/box/func.h
index 931718bba..7b920d7d3 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -70,15 +70,6 @@ struct func {
* Anchor for module membership.
*/
struct rlist item;
- /**
- * For C functions, the body of the function.
- */
- box_function_f c_func;
- /**
- * Each stored function keeps a handle to the
- * dynamic library for the C callback.
- */
- struct module *module;
/**
* Authentication id of the owner of the function,
* used for set-user-id functions.
@@ -88,6 +79,27 @@ struct func {
* Cached runtime access information.
*/
struct access access[BOX_USER_MAX];
+ /** Function runtime context. */
+ union {
+ /**
+ * The reference index of Lua function object.
+ * Is equal to LUA_REFNIL when undefined.
+ */
+ int lua_ref;
+ struct {
+ /**
+ * For C functions, the body of the
+ * function.
+ */
+ box_function_f c_func;
+ /**
+ * Each stored C function keeps a handle
+ * to the dynamic library for the C
+ * callback.
+ */
+ struct module *module;
+ };
+ };
};
/**
diff --git a/src/box/func_def.h b/src/box/func_def.h
index 7a920f65e..8953e4776 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -89,6 +89,12 @@ func_def_sizeof(uint32_t name_len, uint32_t body_len)
return sz;
}
+static inline bool
+func_def_is_persistent(struct func_def *def)
+{
+ return def->body != NULL;
+}
+
/**
* API of C stored function.
*/
diff --git a/test/box/persistent_func.result b/test/box/persistent_func.result
new file mode 100644
index 000000000..f5e03dd5b
--- /dev/null
+++ b/test/box/persistent_func.result
@@ -0,0 +1,136 @@
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+--
+-- gh-4182: Add persistent LUA functions.
+--
+box.schema.user.grant('guest', 'execute', 'universe')
+---
+...
+net = require('net.box')
+---
+...
+conn = net.connect(box.cfg.listen)
+---
+...
+-- Test valid function.
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+body = [[function(tuple)
+ if type(tuple.address) ~= 'string' then return nil, 'Invalid field type' end
+ local t = tuple.address:lower():split()
+ for k,v in pairs(t) do t[k] = {v} end
+ return t
+end
+]]
+test_run:cmd("setopt delimiter ''");
+---
+...
+box.schema.func.create('test', {body = body, language = "C"})
+---
+- error: 'Failed to create function ''test'': function body may be specified only
+ for Lua language'
+...
+box.schema.func.create('test', {body = body})
+---
+...
+box.schema.func.exists('test')
+---
+- true
+...
+conn:call("test", {{address = "Moscow Dolgoprudny"}})
+---
+- [['moscow'], ['dolgoprudny']]
+...
+box.schema.func.create('test2', {body = body, is_deterministic = true})
+---
+...
+-- Test function with spell error - case 1.
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+body_bad2 = [[function(tuple)
+ ret tuple
+end
+]]
+test_run:cmd("setopt delimiter ''");
+---
+...
+box.schema.func.create('body_bad2', {body = body_bad2})
+---
+...
+conn:call("body_bad2", {{address = "Moscow Dolgoprudny"}})
+---
+- error: "Failed to dynamically load function 'body_bad2': function(tuple) \tret tuple
+ end "
+...
+box.schema.func.drop('body_bad2')
+---
+...
+-- Test function with spell error - case 2.
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+body_bad3 = [[func(tuple)
+ return tuple
+end
+]]
+test_run:cmd("setopt delimiter ''");
+---
+...
+box.schema.func.create('body_bad3', {body = body_bad3})
+---
+...
+conn:call("body_bad3", {{address = "Moscow Dolgoprudny"}})
+---
+- error: "Failed to dynamically load function 'body_bad3': func(tuple) \treturn tuple
+ end "
+...
+box.schema.func.drop('body_bad3')
+---
+...
+conn:close()
+---
+...
+-- Restart server.
+test_run:cmd("restart server default")
+net = require('net.box')
+---
+...
+test_run = require('test_run').new()
+---
+...
+conn = net.connect(box.cfg.listen)
+---
+...
+conn:call("test", {{address = "Moscow Dolgoprudny"}})
+---
+- [['moscow'], ['dolgoprudny']]
+...
+conn:close()
+---
+...
+box.schema.func.exists('test')
+---
+- true
+...
+box.schema.func.drop('test')
+---
+...
+box.schema.func.exists('test')
+---
+- false
+...
+box.schema.func.drop('test2')
+---
+...
+box.schema.user.revoke('guest', 'execute', 'universe')
+---
+...
diff --git a/test/box/persistent_func.test.lua b/test/box/persistent_func.test.lua
new file mode 100644
index 000000000..89b3f6c34
--- /dev/null
+++ b/test/box/persistent_func.test.lua
@@ -0,0 +1,61 @@
+env = require('test_run')
+test_run = env.new()
+
+--
+-- gh-4182: Add persistent LUA functions.
+--
+box.schema.user.grant('guest', 'execute', 'universe')
+net = require('net.box')
+conn = net.connect(box.cfg.listen)
+
+-- Test valid function.
+test_run:cmd("setopt delimiter ';'")
+body = [[function(tuple)
+ if type(tuple.address) ~= 'string' then return nil, 'Invalid field type' end
+ local t = tuple.address:lower():split()
+ for k,v in pairs(t) do t[k] = {v} end
+ return t
+end
+]]
+test_run:cmd("setopt delimiter ''");
+box.schema.func.create('test', {body = body, language = "C"})
+box.schema.func.create('test', {body = body})
+box.schema.func.exists('test')
+conn:call("test", {{address = "Moscow Dolgoprudny"}})
+box.schema.func.create('test2', {body = body, is_deterministic = true})
+
+-- Test function with spell error - case 1.
+test_run:cmd("setopt delimiter ';'")
+body_bad2 = [[function(tuple)
+ ret tuple
+end
+]]
+test_run:cmd("setopt delimiter ''");
+box.schema.func.create('body_bad2', {body = body_bad2})
+conn:call("body_bad2", {{address = "Moscow Dolgoprudny"}})
+box.schema.func.drop('body_bad2')
+
+-- Test function with spell error - case 2.
+test_run:cmd("setopt delimiter ';'")
+body_bad3 = [[func(tuple)
+ return tuple
+end
+]]
+test_run:cmd("setopt delimiter ''");
+box.schema.func.create('body_bad3', {body = body_bad3})
+conn:call("body_bad3", {{address = "Moscow Dolgoprudny"}})
+box.schema.func.drop('body_bad3')
+
+conn:close()
+-- Restart server.
+test_run:cmd("restart server default")
+net = require('net.box')
+test_run = require('test_run').new()
+conn = net.connect(box.cfg.listen)
+conn:call("test", {{address = "Moscow Dolgoprudny"}})
+conn:close()
+box.schema.func.exists('test')
+box.schema.func.drop('test')
+box.schema.func.exists('test')
+box.schema.func.drop('test2')
+box.schema.user.revoke('guest', 'execute', 'universe')
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 6/9] box: load persistent Lua functions on creation
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
0 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 12:19 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Thu, Jun 06, 2019 at 03:04:02PM +0300, Kirill Shcherbatov wrote:
> This patch proceed persistent Lua function load on function
> object creation.
>
> Part of #4182
> Needed for #1260
> ---
> src/box/func.c | 77 +++++++++++++++--
> src/box/func.h | 30 +++++--
> src/box/func_def.h | 6 ++
> test/box/persistent_func.result | 136 ++++++++++++++++++++++++++++++
> test/box/persistent_func.test.lua | 61 ++++++++++++++
> 5 files changed, 292 insertions(+), 18 deletions(-)
> create mode 100644 test/box/persistent_func.result
> create mode 100644 test/box/persistent_func.test.lua
>
> diff --git a/src/box/func.c b/src/box/func.c
> index 71c6bb6eb..b21221d9e 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -438,6 +438,7 @@ func_new(struct func_def *def)
> */
> } else {
> assert(def->language == FUNC_LANGUAGE_LUA);
> + func->lua_ref = LUA_REFNIL;
> func->vtab = &func_lua_vtab;
> }
> return func;
I think we should check function code in the constructor - it's more
user-friendly than checking it on the first execution.
> @@ -627,7 +677,12 @@ func_lua_call(struct func *func, struct port *port, const char *args,
> assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
> assert(func->vtab == &func_lua_vtab);
>
> + if (func_def_is_persistent(func->def) && func->lua_ref == LUA_REFNIL &&
> + func_lua_load(func) != 0)
> + return -1;
> +
Branching in a virtual method doesn't look good IMO. Why not add
a separate class for persistent Lua functions?
> diff --git a/src/box/func_def.h b/src/box/func_def.h
> index 7a920f65e..8953e4776 100644
> --- a/src/box/func_def.h
> +++ b/src/box/func_def.h
> @@ -89,6 +89,12 @@ func_def_sizeof(uint32_t name_len, uint32_t body_len)
> return sz;
> }
>
> +static inline bool
> +func_def_is_persistent(struct func_def *def)
> +{
> + return def->body != NULL;
> +}
> +
Pointless helper IMO - it's okay to check 'body' directly in Lua
constructor.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 7/9] box: sandbox option for persistent functions
2019-06-06 12:03 [PATCH v2 0/9] box: rework functions machinery Kirill Shcherbatov
` (5 preceding siblings ...)
2019-06-06 12:04 ` [PATCH v2 6/9] box: load persistent Lua functions on creation Kirill Shcherbatov
@ 2019-06-06 12:04 ` Kirill Shcherbatov
2019-06-10 14:06 ` Vladimir Davydov
2019-06-06 12:04 ` [PATCH v2 8/9] box: implement lua_port dump to region and to Lua Kirill Shcherbatov
` (2 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-06 12:04 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov
Introduced a new option 'is_sandboxed' to initialize a new
persistent function inside of isolated sandbox where only limited
number of functions and modules is available:
-assert -error -pairs -ipairs -next -pcall -xpcall -type
-print -select -string -tonumber -tostring -unpack -math -utf8
Global variables are forbidden in persistent Lua functions.
To initialize a new persistent function inside of sandbox,
specify is_sandboxed = true:
box.schema.func.create('myfunc', {body = body,
opts = {is_sandboxed = true}})
Part of #4182
Needed for #1260
---
src/box/alter.cc | 12 ++++++
src/box/errcode.h | 1 +
src/box/func.c | 8 ++++
src/box/func_def.c | 9 +++++
src/box/func_def.h | 23 +++++++++++
src/lua/utils.c | 67 +++++++++++++++++++++++++++++++
src/lua/utils.h | 8 ++++
test/box/misc.result | 1 +
test/box/persistent_func.result | 36 +++++++++++++++++
test/box/persistent_func.test.lua | 15 +++++++
10 files changed, 180 insertions(+)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 11cad77c3..c769e4f3d 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2449,6 +2449,7 @@ func_def_new_from_tuple(struct tuple *tuple)
if (def == NULL)
tnt_raise(OutOfMemory, def_sz, "malloc", "def");
auto def_guard = make_scoped_guard([=] { free(def); });
+ func_opts_create(&def->opts);
func_def_get_ids_from_tuple(tuple, &def->fid, &def->uid);
memcpy(def->name, name, name_len);
def->name[name_len] = 0;
@@ -2492,6 +2493,17 @@ func_def_new_from_tuple(struct tuple *tuple)
}
def->is_deterministic =
tuple_field_bool_xc(tuple, BOX_FUNC_FIELD_IS_DETERMINISTIC);
+ const char *opts = tuple_field(tuple, BOX_FUNC_FIELD_OPTS);
+ if (opts_decode(&def->opts, func_opts_reg, &opts,
+ ER_WRONG_FUNCTION_OPTIONS, BOX_FUNC_FIELD_OPTS,
+ NULL) != 0)
+ diag_raise();
+ if (def->opts.is_sandboxed &&
+ (def->language != FUNC_LANGUAGE_LUA || body_len == 0)) {
+ tnt_raise(ClientError, ER_CREATE_FUNCTION, name,
+ "is_sandboxed option is applieble only for "
+ "persistent Lua function");
+ }
} else {
def->returns = FIELD_TYPE_ANY;
def->is_deterministic = false;
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 9c15f3322..1724e89a9 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -247,6 +247,7 @@ struct errcode_record {
/*192 */_(ER_INDEX_DEF_UNSUPPORTED, "%s are prohibited in an index definition") \
/*193 */_(ER_CK_DEF_UNSUPPORTED, "%s are prohibited in a CHECK constraint definition") \
/*194 */_(ER_MULTIKEY_INDEX_MISMATCH, "Field %s is used as multikey in one index and as single key in another") \
+ /*195 */_(ER_WRONG_FUNCTION_OPTIONS, "Wrong function options (field %u): %s") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/func.c b/src/box/func.c
index b21221d9e..f2065422c 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -643,6 +643,14 @@ func_lua_load_cb(struct lua_State *L)
func->def->body);
goto error;
}
+ if (func->def->opts.is_sandboxed) {
+ if (luaT_get_sandbox(L) != 0) {
+ diag_set(ClientError, ER_LOAD_FUNCTION, func->def->name,
+ func->def->body);
+ goto error;
+ }
+ lua_setfenv(L, -2);
+ }
func->lua_ref = luaL_ref(L, LUA_REGISTRYINDEX);
region_truncate(region, region_svp);
return lua_gettop(L);
diff --git a/src/box/func_def.c b/src/box/func_def.c
index 76ed77b24..df74a6d9a 100644
--- a/src/box/func_def.c
+++ b/src/box/func_def.c
@@ -1,3 +1,12 @@
#include "func_def.h"
+#include "opt_def.h"
const char *func_language_strs[] = {"LUA", "C"};
+
+const struct func_opts func_opts_default = {
+ /* .is_sandboxed = */ false,
+};
+
+const struct opt_def func_opts_reg[] = {
+ OPT_DEF("is_sandboxed", OPT_BOOL, struct func_opts, is_sandboxed),
+};
diff --git a/src/box/func_def.h b/src/box/func_def.h
index 8953e4776..b382cde00 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -33,6 +33,7 @@
#include "trivia/util.h"
#include "field_def.h"
+#include "opt_def.h"
#include <stdbool.h>
/**
@@ -46,6 +47,19 @@ enum func_language {
extern const char *func_language_strs[];
+/** Function options. */
+struct func_opts {
+ /**
+ * Whether the routine mast be initialized with isolated
+ * sandbox where only a limited number if functions is
+ * available.
+ */
+ bool is_sandboxed;
+};
+
+extern const struct func_opts func_opts_default;
+extern const struct opt_def func_opts_reg[];
+
/**
* Definition of a function. Function body is not stored
* or replicated (yet).
@@ -70,6 +84,8 @@ struct func_def {
* The language of the stored function.
*/
enum func_language language;
+ /** The function options. */
+ struct func_opts opts;
/** Function name. */
char name[0];
};
@@ -89,6 +105,13 @@ func_def_sizeof(uint32_t name_len, uint32_t body_len)
return sz;
}
+/** Create index options using default values. */
+static inline void
+func_opts_create(struct func_opts *opts)
+{
+ *opts = func_opts_default;
+}
+
static inline bool
func_def_is_persistent(struct func_def *def)
{
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 27ff6b396..0d1cca423 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -46,6 +46,14 @@ static uint32_t CTID_STRUCT_IBUF_PTR;
uint32_t CTID_CHAR_PTR;
uint32_t CTID_CONST_CHAR_PTR;
+static const char *default_sandbox_exports[] =
+ {"assert", "error", "ipairs", "math", "next", "pairs", "pcall",
+ "print", "select", "string", "table", "tonumber", "tostring",
+ "type", "unpack", "xpcall", "utf8"};
+
+static int luaL_deepcopy_func_ref = LUA_REFNIL;
+static int luaL_default_sandbox_ref = LUA_REFNIL;
+
void *
luaL_pushcdata(struct lua_State *L, uint32_t ctypeid)
{
@@ -1248,6 +1256,65 @@ luaT_func_find(struct lua_State *L, const char *name, const char *name_end,
return 0;
}
+/**
+ * Assemble a new sandbox with given exports table on top of the
+ * Lua stack. All modules in exports list are copying deeply
+ * to ensure the immutablility of this system object.
+ */
+static int
+luaT_prepare_sandbox(struct lua_State *L, const char *exports[],
+ uint32_t exports_count)
+{
+ assert(luaL_deepcopy_func_ref != LUA_REFNIL);
+ lua_createtable(L, exports_count, 0);
+ for (unsigned i = 0; i < exports_count; i++) {
+ int count;
+ uint32_t name_len = strlen(exports[i]);
+ if (luaT_func_find(L, exports[i], exports[i] + name_len,
+ &count) != 0)
+ return -1;
+ switch (lua_type(L, -1)) {
+ case LUA_TTABLE:
+ lua_rawgeti(L, LUA_REGISTRYINDEX,
+ luaL_deepcopy_func_ref);
+ lua_insert(L, -2);
+ lua_call(L, 1, LUA_MULTRET);
+ FALLTHROUGH;
+ case LUA_TFUNCTION:
+ break;
+ default:
+ unreachable();
+ }
+ lua_setfield(L, -2, exports[i]);
+ }
+ return 0;
+}
+
+int
+luaT_get_sandbox(struct lua_State *L)
+{
+ if (luaL_deepcopy_func_ref == LUA_REFNIL) {
+ int count;
+ const char *deepcopy = "table.deepcopy";
+ if (luaT_func_find(L, deepcopy, deepcopy + strlen(deepcopy),
+ &count) != 0)
+ return -1;
+ luaL_deepcopy_func_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+ assert(luaL_deepcopy_func_ref != LUA_REFNIL);
+ }
+ if (luaL_default_sandbox_ref == LUA_REFNIL) {
+ if (luaT_prepare_sandbox(L, default_sandbox_exports,
+ nelem(default_sandbox_exports)) != 0)
+ return -1;
+ luaL_default_sandbox_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+ assert(luaL_default_sandbox_ref != LUA_REFNIL);
+ }
+ lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_deepcopy_func_ref);
+ lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_default_sandbox_ref);
+ lua_call(L, 1, LUA_MULTRET);
+ return 0;
+}
+
int
tarantool_lua_utils_init(struct lua_State *L)
{
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 81e936bee..36bb2e53b 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -603,6 +603,14 @@ int
luaT_func_find(struct lua_State *L, const char *name, const char *name_end,
int *count);
+/**
+ * Prepare a new 'default' sandbox table on the top of the Lua
+ * stack. The 'default' sandbox consists of a minimum set of
+ * functions that are sufficient to serve persistent functions.
+ */
+int
+luaT_get_sandbox(struct lua_State *L);
+
int
tarantool_lua_utils_init(struct lua_State *L);
diff --git a/test/box/misc.result b/test/box/misc.result
index 4fcd13a78..a4c8ae498 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -523,6 +523,7 @@ t;
192: box.error.INDEX_DEF_UNSUPPORTED
193: box.error.CK_DEF_UNSUPPORTED
194: box.error.MULTIKEY_INDEX_MISMATCH
+ 195: box.error.WRONG_FUNCTION_OPTIONS
...
test_run:cmd("setopt delimiter ''");
---
diff --git a/test/box/persistent_func.result b/test/box/persistent_func.result
index f5e03dd5b..54548f3b0 100644
--- a/test/box/persistent_func.result
+++ b/test/box/persistent_func.result
@@ -50,6 +50,42 @@ conn:call("test", {{address = "Moscow Dolgoprudny"}})
box.schema.func.create('test2', {body = body, is_deterministic = true})
---
...
+-- Test snadboxed functions.
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+body = [[function(number)
+ math.abs = math.log
+ return math.abs(number)
+ end]]
+test_run:cmd("setopt delimiter ''");
+---
+...
+box.schema.func.create('monkey', {language = 'C', opts = {is_sandboxed = true}})
+---
+- error: 'Failed to create function ''monkey'': is_sandboxed option is applieble only
+ for persistent Lua function'
+...
+box.schema.func.create('monkey', {opts = {is_sandboxed = true}})
+---
+- error: 'Failed to create function ''monkey'': is_sandboxed option is applieble only
+ for persistent Lua function'
+...
+box.schema.func.create('monkey', {body = body, opts = {is_sandboxed = true}})
+---
+...
+conn:call("monkey", {1})
+---
+- 0
+...
+math.abs(1)
+---
+- 1
+...
+box.schema.func.drop('monkey')
+---
+...
-- Test function with spell error - case 1.
test_run:cmd("setopt delimiter ';'")
---
diff --git a/test/box/persistent_func.test.lua b/test/box/persistent_func.test.lua
index 89b3f6c34..095a27872 100644
--- a/test/box/persistent_func.test.lua
+++ b/test/box/persistent_func.test.lua
@@ -24,6 +24,21 @@ box.schema.func.exists('test')
conn:call("test", {{address = "Moscow Dolgoprudny"}})
box.schema.func.create('test2', {body = body, is_deterministic = true})
+-- Test snadboxed functions.
+test_run:cmd("setopt delimiter ';'")
+body = [[function(number)
+ math.abs = math.log
+ return math.abs(number)
+ end]]
+test_run:cmd("setopt delimiter ''");
+
+box.schema.func.create('monkey', {language = 'C', opts = {is_sandboxed = true}})
+box.schema.func.create('monkey', {opts = {is_sandboxed = true}})
+box.schema.func.create('monkey', {body = body, opts = {is_sandboxed = true}})
+conn:call("monkey", {1})
+math.abs(1)
+box.schema.func.drop('monkey')
+
-- Test function with spell error - case 1.
test_run:cmd("setopt delimiter ';'")
body_bad2 = [[function(tuple)
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/9] box: sandbox option for persistent functions
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
0 siblings, 1 reply; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 14:06 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Thu, Jun 06, 2019 at 03:04:03PM +0300, Kirill Shcherbatov wrote:
> Introduced a new option 'is_sandboxed' to initialize a new
> persistent function inside of isolated sandbox where only limited
> number of functions and modules is available:
> -assert -error -pairs -ipairs -next -pcall -xpcall -type
> -print -select -string -tonumber -tostring -unpack -math -utf8
>
> Global variables are forbidden in persistent Lua functions.
>
> To initialize a new persistent function inside of sandbox,
> specify is_sandboxed = true:
> box.schema.func.create('myfunc', {body = body,
> opts = {is_sandboxed = true}})
>
> Part of #4182
> Needed for #1260
> ---
> src/box/alter.cc | 12 ++++++
> src/box/errcode.h | 1 +
> src/box/func.c | 8 ++++
> src/box/func_def.c | 9 +++++
> src/box/func_def.h | 23 +++++++++++
> src/lua/utils.c | 67 +++++++++++++++++++++++++++++++
> src/lua/utils.h | 8 ++++
> test/box/misc.result | 1 +
> test/box/persistent_func.result | 36 +++++++++++++++++
> test/box/persistent_func.test.lua | 15 +++++++
> 10 files changed, 180 insertions(+)
>
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 11cad77c3..c769e4f3d 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2449,6 +2449,7 @@ func_def_new_from_tuple(struct tuple *tuple)
> if (def == NULL)
> tnt_raise(OutOfMemory, def_sz, "malloc", "def");
> auto def_guard = make_scoped_guard([=] { free(def); });
> + func_opts_create(&def->opts);
> func_def_get_ids_from_tuple(tuple, &def->fid, &def->uid);
> memcpy(def->name, name, name_len);
> def->name[name_len] = 0;
> @@ -2492,6 +2493,17 @@ func_def_new_from_tuple(struct tuple *tuple)
> }
> def->is_deterministic =
> tuple_field_bool_xc(tuple, BOX_FUNC_FIELD_IS_DETERMINISTIC);
> + const char *opts = tuple_field(tuple, BOX_FUNC_FIELD_OPTS);
> + if (opts_decode(&def->opts, func_opts_reg, &opts,
> + ER_WRONG_FUNCTION_OPTIONS, BOX_FUNC_FIELD_OPTS,
> + NULL) != 0)
> + diag_raise();
> + if (def->opts.is_sandboxed &&
> + (def->language != FUNC_LANGUAGE_LUA || body_len == 0)) {
> + tnt_raise(ClientError, ER_CREATE_FUNCTION, name,
> + "is_sandboxed option is applieble only for "
> + "persistent Lua function");
> + }
Again, the check should be carried out in function constructors so that
we don't change this code when we introduce SQL functions.
> } else {
> def->returns = FIELD_TYPE_ANY;
> def->is_deterministic = false;
> diff --git a/src/box/func_def.c b/src/box/func_def.c
> index 76ed77b24..df74a6d9a 100644
> --- a/src/box/func_def.c
> +++ b/src/box/func_def.c
> @@ -1,3 +1,12 @@
> #include "func_def.h"
> +#include "opt_def.h"
>
> const char *func_language_strs[] = {"LUA", "C"};
> +
> +const struct func_opts func_opts_default = {
> + /* .is_sandboxed = */ false,
> +};
> +
> +const struct opt_def func_opts_reg[] = {
> + OPT_DEF("is_sandboxed", OPT_BOOL, struct func_opts, is_sandboxed),
> +};
AFAIK we've decided to make all function options as tuple fields while
the 'option' field is here just in case we need to add something later.
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 27ff6b396..0d1cca423 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -46,6 +46,14 @@ static uint32_t CTID_STRUCT_IBUF_PTR;
> uint32_t CTID_CHAR_PTR;
> uint32_t CTID_CONST_CHAR_PTR;
>
> +static const char *default_sandbox_exports[] =
> + {"assert", "error", "ipairs", "math", "next", "pairs", "pcall",
> + "print", "select", "string", "table", "tonumber", "tostring",
> + "type", "unpack", "xpcall", "utf8"};
> +
> +static int luaL_deepcopy_func_ref = LUA_REFNIL;
> +static int luaL_default_sandbox_ref = LUA_REFNIL;
> +
> void *
> luaL_pushcdata(struct lua_State *L, uint32_t ctypeid)
> {
> @@ -1248,6 +1256,65 @@ luaT_func_find(struct lua_State *L, const char *name, const char *name_end,
> return 0;
> }
>
> +/**
> + * Assemble a new sandbox with given exports table on top of the
> + * Lua stack. All modules in exports list are copying deeply
> + * to ensure the immutablility of this system object.
> + */
> +static int
> +luaT_prepare_sandbox(struct lua_State *L, const char *exports[],
> + uint32_t exports_count)
> +{
> + assert(luaL_deepcopy_func_ref != LUA_REFNIL);
> + lua_createtable(L, exports_count, 0);
> + for (unsigned i = 0; i < exports_count; i++) {
> + int count;
> + uint32_t name_len = strlen(exports[i]);
> + if (luaT_func_find(L, exports[i], exports[i] + name_len,
> + &count) != 0)
> + return -1;
> + switch (lua_type(L, -1)) {
> + case LUA_TTABLE:
> + lua_rawgeti(L, LUA_REGISTRYINDEX,
> + luaL_deepcopy_func_ref);
> + lua_insert(L, -2);
> + lua_call(L, 1, LUA_MULTRET);
> + FALLTHROUGH;
> + case LUA_TFUNCTION:
> + break;
> + default:
> + unreachable();
> + }
> + lua_setfield(L, -2, exports[i]);
> + }
> + return 0;
> +}
> +
> +int
> +luaT_get_sandbox(struct lua_State *L)
As I mentioned earlier, I don't think we need to have these highly
specialized functions in src/lua/util.[hc]. I'd define them as static
in src/box/lua/call.[hc] as we only need them to invoke persistent Lua
functions.
> +{
> + if (luaL_deepcopy_func_ref == LUA_REFNIL) {
> + int count;
> + const char *deepcopy = "table.deepcopy";
> + if (luaT_func_find(L, deepcopy, deepcopy + strlen(deepcopy),
> + &count) != 0)
> + return -1;
Wouldn't it be more effecient to copy the table in C, i.e. without the
aid of 'table.deepcopy' method?
> + luaL_deepcopy_func_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> + assert(luaL_deepcopy_func_ref != LUA_REFNIL);
> + }
> + if (luaL_default_sandbox_ref == LUA_REFNIL) {
> + if (luaT_prepare_sandbox(L, default_sandbox_exports,
> + nelem(default_sandbox_exports)) != 0)
> + return -1;
> + luaL_default_sandbox_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> + assert(luaL_default_sandbox_ref != LUA_REFNIL);
> + }
> + lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_deepcopy_func_ref);
> + lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_default_sandbox_ref);
> + lua_call(L, 1, LUA_MULTRET);
> + return 0;
> +}
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [tarantool-patches] Re: [PATCH v2 7/9] box: sandbox option for persistent functions
2019-06-10 14:06 ` Vladimir Davydov
@ 2019-06-10 14:15 ` Kirill Shcherbatov
2019-06-10 14:41 ` Vladimir Davydov
0 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-10 14:15 UTC (permalink / raw)
To: tarantool-patches, Vladimir Davydov
>> +{
>> + if (luaL_deepcopy_func_ref == LUA_REFNIL) {
>> + int count;
>> + const char *deepcopy = "table.deepcopy";
>> + if (luaT_func_find(L, deepcopy, deepcopy + strlen(deepcopy),
>> + &count) != 0)
>> + return -1;
>
> Wouldn't it be more effecient to copy the table in C, i.e. without the
> aid of 'table.deepcopy' method?
What do you mean?
ref: https://www.freelists.org/post/tarantool-patches/PATCH-v1-48-box-load-persistent-Lua-functions-on-creation,2
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [tarantool-patches] Re: [PATCH v2 7/9] box: sandbox option for persistent functions
2019-06-10 14:15 ` [tarantool-patches] " Kirill Shcherbatov
@ 2019-06-10 14:41 ` Vladimir Davydov
0 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 14:41 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Mon, Jun 10, 2019 at 05:15:14PM +0300, Kirill Shcherbatov wrote:
> >> +{
> >> + if (luaL_deepcopy_func_ref == LUA_REFNIL) {
> >> + int count;
> >> + const char *deepcopy = "table.deepcopy";
> >> + if (luaT_func_find(L, deepcopy, deepcopy + strlen(deepcopy),
> >> + &count) != 0)
> >> + return -1;
> >
> > Wouldn't it be more effecient to copy the table in C, i.e. without the
> > aid of 'table.deepcopy' method?
>
> What do you mean?
I mean rather than calling table.deepcopy function written in Lua,
wouldn't it be more effecient to do deepcopy in C using Lua C API?
Well, never mind. I guess we don't need to optimize this case, as we
only create a sandbox on function creation, not invocation.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 8/9] box: implement lua_port dump to region and to Lua
2019-06-06 12:03 [PATCH v2 0/9] box: rework functions machinery Kirill Shcherbatov
` (6 preceding siblings ...)
2019-06-06 12:04 ` [PATCH v2 7/9] box: sandbox option for persistent functions Kirill Shcherbatov
@ 2019-06-06 12:04 ` 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 9:14 ` [PATCH v2 0/9] box: rework functions machinery Vladimir Davydov
9 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-06 12:04 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov
Refactored port_lua class to reuse an existent machinery to
dump info not only for obuf, but to region, that is also
mpstream-compatible. We need this feature in scope of multikey
indexes to work with keys produced with functional index
extractor in memory.
Also introduce a tiny method .dump_lua for port_lua. It is
necessary to export registered on-board functions call endpoints.
Class implementation is moved to a new file port_lua.c.
Part of #4182
Needed for #1260
---
src/box/CMakeLists.txt | 1 +
src/box/execute.c | 1 +
src/box/lua/call.c | 254 +-------------------------------
src/box/lua/port_lua.c | 318 +++++++++++++++++++++++++++++++++++++++++
src/box/port.h | 2 +-
src/lib/core/port.h | 16 +++
6 files changed, 338 insertions(+), 254 deletions(-)
create mode 100644 src/box/lua/port_lua.c
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 0864c3433..5f095f1f0 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -146,6 +146,7 @@ add_library(box STATIC
lua/execute.c
lua/key_def.c
lua/merger.c
+ lua/port_lua.c
${bin_sources})
target_link_libraries(box box_error tuple stat xrow xlog vclock crc32 scramble
diff --git a/src/box/execute.c b/src/box/execute.c
index a3d4a92b8..2e27b6a60 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -106,6 +106,7 @@ port_sql_destroy(struct port *base)
const struct port_vtab port_sql_vtab = {
/* .dump_msgpack = */ port_sql_dump_msgpack,
/* .dump_msgpack_16 = */ NULL,
+ /* .dump_msgpack_region = */ NULL,
/* .dump_lua = */ port_sql_dump_lua,
/* .dump_plain = */ NULL,
/* .destroy = */ port_sql_destroy,
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 4d4521363..e40c9a3f7 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -66,161 +66,6 @@ lbox_call_loadproc(struct lua_State *L)
return count;
}
-/*
- * Encode CALL_16 result.
- *
- * To allow clients to understand a complex return from
- * a procedure, we are compatible with SELECT protocol,
- * and return the number of return values first, and
- * then each return value as a tuple.
- *
- * The following conversion rules apply:
- *
- * If a Lua stack contains at least one scalar, each
- * value on the stack is converted to a tuple. A stack
- * containing a single Lua table with scalars is converted to
- * a tuple with multiple fields.
- *
- * If the stack is a Lua table, each member of which is
- * not scalar, each member of the table is converted to
- * a tuple. This way very large lists of return values can
- * be used, since Lua stack size is limited by 8000 elements,
- * while Lua table size is pretty much unlimited.
- *
- * Please read gh-291 carefully before "fixing" this code.
- */
-static inline uint32_t
-luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream)
-{
- int nrets = lua_gettop(L);
- if (nrets == 0) {
- return 0;
- } else if (nrets > 1) {
- /*
- * Multireturn:
- * `return 1, box.tuple.new(...), array, 3, ...`
- */
- for (int i = 1; i <= nrets; ++i) {
- struct luaL_field field;
- if (luaL_tofield(L, cfg, i, &field) < 0)
- return luaT_error(L);
- struct tuple *tuple;
- if (field.type == MP_EXT &&
- (tuple = luaT_istuple(L, i)) != NULL) {
- /* `return ..., box.tuple.new(...), ...` */
- tuple_to_mpstream(tuple, stream);
- } else if (field.type != MP_ARRAY) {
- /*
- * `return ..., scalar, ... =>
- * ..., { scalar }, ...`
- */
- lua_pushvalue(L, i);
- mpstream_encode_array(stream, 1);
- luamp_encode_r(L, cfg, stream, &field, 0);
- lua_pop(L, 1);
- } else {
- /* `return ..., array, ...` */
- luamp_encode(L, cfg, stream, i);
- }
- }
- return nrets;
- }
- assert(nrets == 1);
-
- /*
- * Inspect the first result
- */
- struct luaL_field root;
- if (luaL_tofield(L, cfg, 1, &root) < 0)
- return luaT_error(L);
- struct tuple *tuple;
- if (root.type == MP_EXT && (tuple = luaT_istuple(L, 1)) != NULL) {
- /* `return box.tuple()` */
- tuple_to_mpstream(tuple, stream);
- return 1;
- } else if (root.type != MP_ARRAY) {
- /*
- * `return scalar`
- * `return map`
- */
- mpstream_encode_array(stream, 1);
- assert(lua_gettop(L) == 1);
- luamp_encode_r(L, cfg, stream, &root, 0);
- return 1;
- }
-
- assert(root.type == MP_ARRAY);
- if (root.size == 0) {
- /* `return {}` => `{ box.tuple() }` */
- mpstream_encode_array(stream, 0);
- return 1;
- }
-
- /* `return { tuple, scalar, tuple }` */
- assert(root.type == MP_ARRAY && root.size > 0);
- for (uint32_t t = 1; t <= root.size; t++) {
- lua_rawgeti(L, 1, t);
- struct luaL_field field;
- if (luaL_tofield(L, cfg, -1, &field) < 0)
- return luaT_error(L);
- if (field.type == MP_EXT && (tuple = luaT_istuple(L, -1))) {
- tuple_to_mpstream(tuple, stream);
- } else if (field.type != MP_ARRAY) {
- /* The first member of root table is not tuple/array */
- if (t == 1) {
- /*
- * `return { scalar, ... } =>
- * box.tuple.new(scalar, ...)`
- */
- mpstream_encode_array(stream, root.size);
- /*
- * Encode the first field of tuple using
- * existing information from luaL_tofield
- */
- luamp_encode_r(L, cfg, stream, &field, 0);
- lua_pop(L, 1);
- assert(lua_gettop(L) == 1);
- /* Encode remaining fields as usual */
- for (uint32_t f = 2; f <= root.size; f++) {
- lua_rawgeti(L, 1, f);
- luamp_encode(L, cfg, stream, -1);
- lua_pop(L, 1);
- }
- return 1;
- }
- /*
- * `return { tuple/array, ..., scalar, ... } =>
- * { tuple/array, ..., { scalar }, ... }`
- */
- mpstream_encode_array(stream, 1);
- luamp_encode_r(L, cfg, stream, &field, 0);
- } else {
- /* `return { tuple/array, ..., tuple/array, ... }` */
- luamp_encode_r(L, cfg, stream, &field, 0);
- }
- lua_pop(L, 1);
- assert(lua_gettop(L) == 1);
- }
- return root.size;
-}
-
-static const struct port_vtab port_lua_vtab;
-
-void
-port_lua_create(struct port *port, struct lua_State *L)
-{
- struct port_lua *port_lua = (struct port_lua *) port;
- memset(port_lua, 0, sizeof(*port_lua));
- port_lua->vtab = &port_lua_vtab;
- port_lua->L = L;
- /*
- * Allow to destroy the port even if no ref.
- * @Sa luaL_unref.
- */
- port_lua->ref = -1;
-}
-
static int
execute_lua_eval(lua_State *L)
{
@@ -248,103 +93,6 @@ execute_lua_eval(lua_State *L)
return lua_gettop(L);
}
-static int
-encode_lua_call(lua_State *L)
-{
- struct port_lua *port = (struct port_lua *) lua_topointer(L, -1);
- /*
- * Add all elements from Lua stack to the buffer.
- *
- * TODO: forbid explicit yield from __serialize or __index here
- */
- struct mpstream stream;
- mpstream_init(&stream, port->out, obuf_reserve_cb, obuf_alloc_cb,
- luamp_error, port->L);
-
- struct luaL_serializer *cfg = luaL_msgpack_default;
- int size = lua_gettop(port->L);
- for (int i = 1; i <= size; ++i)
- luamp_encode(port->L, cfg, &stream, i);
- port->size = size;
- mpstream_flush(&stream);
- return 0;
-}
-
-static int
-encode_lua_call_16(lua_State *L)
-{
- struct port_lua *port = (struct port_lua *) lua_topointer(L, -1);
- /*
- * Add all elements from Lua stack to the buffer.
- *
- * TODO: forbid explicit yield from __serialize or __index here
- */
- struct mpstream stream;
- mpstream_init(&stream, port->out, obuf_reserve_cb, obuf_alloc_cb,
- luamp_error, port->L);
-
- struct luaL_serializer *cfg = luaL_msgpack_default;
- port->size = luamp_encode_call_16(port->L, cfg, &stream);
- mpstream_flush(&stream);
- return 0;
-}
-
-static inline int
-port_lua_do_dump(struct port *base, struct obuf *out, lua_CFunction handler)
-{
- struct port_lua *port = (struct port_lua *)base;
- assert(port->vtab == &port_lua_vtab);
- /* Use port to pass arguments to encoder quickly. */
- port->out = out;
- /*
- * Use the same global state, assuming the encoder doesn't
- * yield.
- */
- struct lua_State *L = tarantool_L;
- int top = lua_gettop(L);
- if (lua_cpcall(L, handler, port) != 0) {
- luaT_toerror(port->L);
- return -1;
- }
- lua_settop(L, top);
- return port->size;
-}
-
-static int
-port_lua_dump(struct port *base, struct obuf *out)
-{
- return port_lua_do_dump(base, out, encode_lua_call);
-}
-
-static int
-port_lua_dump_16(struct port *base, struct obuf *out)
-{
- return port_lua_do_dump(base, out, encode_lua_call_16);
-}
-
-static void
-port_lua_destroy(struct port *base)
-{
- struct port_lua *port = (struct port_lua *)base;
- assert(port->vtab == &port_lua_vtab);
- luaL_unref(tarantool_L, LUA_REGISTRYINDEX, port->ref);
-}
-
-/**
- * Dump port lua as a YAML document. It is extern since depends on
- * lyaml module.
- */
-extern const char *
-port_lua_dump_plain(struct port *port, uint32_t *size);
-
-static const struct port_vtab port_lua_vtab = {
- .dump_msgpack = port_lua_dump,
- .dump_msgpack_16 = port_lua_dump_16,
- .dump_lua = NULL,
- .dump_plain = port_lua_dump_plain,
- .destroy = port_lua_destroy,
-};
-
static inline int
box_process_lua(struct call_request *request, struct port *base,
lua_CFunction handler)
@@ -357,7 +105,7 @@ box_process_lua(struct call_request *request, struct port *base,
lua_pushcfunction(L, handler);
lua_pushlightuserdata(L, request);
if (luaT_call(L, 1, LUA_MULTRET) != 0) {
- port_lua_destroy(base);
+ port_destroy(base);
return -1;
}
return 0;
diff --git a/src/box/lua/port_lua.c b/src/box/lua/port_lua.c
new file mode 100644
index 000000000..087650281
--- /dev/null
+++ b/src/box/lua/port_lua.c
@@ -0,0 +1,318 @@
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the
+ * following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include "mpstream.h"
+#include "box/func.h"
+#include "box/tuple.h"
+#include "box/lua/tuple.h"
+#include "box/lua/misc.h"
+#include "small/region.h"
+#include "lua/utils.h"
+#include "lua/msgpack.h"
+#include "box/port.h"
+#include "small/obuf.h"
+#include <assert.h>
+
+static const struct port_vtab port_lua_vtab;
+
+/*
+ * Encode CALL_16 result.
+ *
+ * To allow clients to understand a complex return from
+ * a procedure, we are compatible with SELECT protocol,
+ * and return the number of return values first, and
+ * then each return value as a tuple.
+ *
+ * The following conversion rules apply:
+ *
+ * If a Lua stack contains at least one scalar, each
+ * value on the stack is converted to a tuple. A stack
+ * containing a single Lua table with scalars is converted to
+ * a tuple with multiple fields.
+ *
+ * If the stack is a Lua table, each member of which is
+ * not scalar, each member of the table is converted to
+ * a tuple. This way very large lists of return values can
+ * be used, since Lua stack size is limited by 8000 elements,
+ * while Lua table size is pretty much unlimited.
+ *
+ * Please read gh-291 carefully before "fixing" this code.
+ */
+static inline uint32_t
+luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
+ struct mpstream *stream)
+{
+ int nrets = lua_gettop(L);
+ if (nrets == 0) {
+ return 0;
+ } else if (nrets > 1) {
+ /*
+ * Multireturn:
+ * `return 1, box.tuple.new(...), array, 3, ...`
+ */
+ for (int i = 1; i <= nrets; ++i) {
+ struct luaL_field field;
+ if (luaL_tofield(L, cfg, i, &field) < 0)
+ return luaT_error(L);
+ struct tuple *tuple;
+ if (field.type == MP_EXT &&
+ (tuple = luaT_istuple(L, i)) != NULL) {
+ /* `return ..., box.tuple.new(...), ...` */
+ tuple_to_mpstream(tuple, stream);
+ } else if (field.type != MP_ARRAY) {
+ /*
+ * `return ..., scalar, ... =>
+ * ..., { scalar }, ...`
+ */
+ lua_pushvalue(L, i);
+ mpstream_encode_array(stream, 1);
+ luamp_encode_r(L, cfg, stream, &field, 0);
+ lua_pop(L, 1);
+ } else {
+ /* `return ..., array, ...` */
+ luamp_encode(L, cfg, stream, i);
+ }
+ }
+ return nrets;
+ }
+ assert(nrets == 1);
+
+ /*
+ * Inspect the first result
+ */
+ struct luaL_field root;
+ if (luaL_tofield(L, cfg, 1, &root) < 0)
+ return luaT_error(L);
+ struct tuple *tuple;
+ if (root.type == MP_EXT && (tuple = luaT_istuple(L, 1)) != NULL) {
+ /* `return box.tuple()` */
+ tuple_to_mpstream(tuple, stream);
+ return 1;
+ } else if (root.type != MP_ARRAY) {
+ /*
+ * `return scalar`
+ * `return map`
+ */
+ mpstream_encode_array(stream, 1);
+ assert(lua_gettop(L) == 1);
+ luamp_encode_r(L, cfg, stream, &root, 0);
+ return 1;
+ }
+
+ assert(root.type == MP_ARRAY);
+ if (root.size == 0) {
+ /* `return {}` => `{ box.tuple() }` */
+ mpstream_encode_array(stream, 0);
+ return 1;
+ }
+
+ /* `return { tuple, scalar, tuple }` */
+ assert(root.type == MP_ARRAY && root.size > 0);
+ for (uint32_t t = 1; t <= root.size; t++) {
+ lua_rawgeti(L, 1, t);
+ struct luaL_field field;
+ if (luaL_tofield(L, cfg, -1, &field) < 0)
+ return luaT_error(L);
+ if (field.type == MP_EXT && (tuple = luaT_istuple(L, -1))) {
+ tuple_to_mpstream(tuple, stream);
+ } else if (field.type != MP_ARRAY) {
+ /* The first member of root table is not tuple/array */
+ if (t == 1) {
+ /*
+ * `return { scalar, ... } =>
+ * box.tuple.new(scalar, ...)`
+ */
+ mpstream_encode_array(stream, root.size);
+ /*
+ * Encode the first field of tuple using
+ * existing information from luaL_tofield
+ */
+ luamp_encode_r(L, cfg, stream, &field, 0);
+ lua_pop(L, 1);
+ assert(lua_gettop(L) == 1);
+ /* Encode remaining fields as usual */
+ for (uint32_t f = 2; f <= root.size; f++) {
+ lua_rawgeti(L, 1, f);
+ luamp_encode(L, cfg, stream, -1);
+ lua_pop(L, 1);
+ }
+ return 1;
+ }
+ /*
+ * `return { tuple/array, ..., scalar, ... } =>
+ * { tuple/array, ..., { scalar }, ... }`
+ */
+ mpstream_encode_array(stream, 1);
+ luamp_encode_r(L, cfg, stream, &field, 0);
+ } else {
+ /* `return { tuple/array, ..., tuple/array, ... }` */
+ luamp_encode_r(L, cfg, stream, &field, 0);
+ }
+ lua_pop(L, 1);
+ assert(lua_gettop(L) == 1);
+ }
+ return root.size;
+}
+
+static inline int
+port_lua_do_dump(struct port *base, struct mpstream *stream,
+ lua_CFunction handler)
+{
+ struct port_lua *port = (struct port_lua *)base;
+ assert(port->vtab == &port_lua_vtab);
+ /* Use port to pass arguments to encoder quickly. */
+ port->stream = stream;
+ /*
+ * Use the same global state, assuming the encoder doesn't
+ * yield.
+ */
+ struct lua_State *L = tarantool_L;
+ int top = lua_gettop(L);
+ if (lua_cpcall(L, handler, port) != 0) {
+ luaT_toerror(port->L);
+ return -1;
+ }
+ lua_settop(L, top);
+ return port->size;
+}
+
+static int
+encode_lua_call(lua_State *L)
+{
+ struct port_lua *port = (struct port_lua *) lua_topointer(L, -1);
+ /*
+ * Add all elements from Lua stack to the buffer.
+ *
+ * TODO: forbid explicit yield from __serialize or __index here
+ */
+ struct luaL_serializer *cfg = luaL_msgpack_default;
+ int size = lua_gettop(port->L);
+ for (int i = 1; i <= size; ++i)
+ luamp_encode(port->L, cfg, port->stream, i);
+ port->size = size;
+ mpstream_flush(port->stream);
+ return 0;
+}
+
+static int
+encode_lua_call_16(lua_State *L)
+{
+ struct port_lua *port = (struct port_lua *) lua_topointer(L, -1);
+ /*
+ * Add all elements from Lua stack to the buffer.
+ *
+ * TODO: forbid explicit yield from __serialize or __index here
+ */
+ struct luaL_serializer *cfg = luaL_msgpack_default;
+ port->size = luamp_encode_call_16(port->L, cfg, port->stream);
+ mpstream_flush(port->stream);
+ return 0;
+}
+
+static int
+port_lua_dump(struct port *base, struct obuf *obuf)
+{
+ struct port_lua *port = (struct port_lua *)base;
+ struct mpstream stream;
+ mpstream_init(&stream, obuf, obuf_reserve_cb, obuf_alloc_cb,
+ luamp_error, port->L);
+ return port_lua_do_dump(base, &stream, encode_lua_call);
+}
+
+static int
+port_lua_dump_16(struct port *base, struct obuf *obuf)
+{
+ struct port_lua *port = (struct port_lua *)base;
+ struct mpstream stream;
+ mpstream_init(&stream, obuf, obuf_reserve_cb, obuf_alloc_cb,
+ luamp_error, port->L);
+ return port_lua_do_dump(base, &stream, encode_lua_call_16);
+}
+
+static int
+port_lua_dump_region(struct port *base, struct region *region)
+{
+ struct port_lua *port = (struct port_lua *)base;
+ struct mpstream stream;
+ mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
+ luamp_error, port->L);
+ return port_lua_do_dump(base, &stream, encode_lua_call);
+}
+
+static void
+port_lua_dump_lua(struct port *base, struct lua_State *L)
+{
+ struct port_lua *port = (struct port_lua *) base;
+ uint32_t size = lua_gettop(port->L);
+ lua_createtable(L, size, 0);
+ for (uint32_t i = 0; i < size; i++) {
+ lua_xmove(port->L, L, 1);
+ lua_rawseti(L, -2, i + 1);
+ }
+ port->size = 1;
+}
+
+static void
+port_lua_destroy(struct port *base)
+{
+ struct port_lua *port = (struct port_lua *)base;
+ assert(port->vtab == &port_lua_vtab);
+ luaL_unref(tarantool_L, LUA_REGISTRYINDEX, port->ref);
+}
+
+/**
+ * Dump port lua as a YAML document. It is extern since depends on
+ * lyaml module.
+ */
+extern const char *
+port_lua_dump_plain(struct port *port, uint32_t *size);
+
+static const struct port_vtab port_lua_vtab = {
+ .dump_msgpack = port_lua_dump,
+ .dump_msgpack_16 = port_lua_dump_16,
+ .dump_msgpack_region = port_lua_dump_region,
+ .dump_lua = port_lua_dump_lua,
+ .dump_plain = port_lua_dump_plain,
+ .destroy = port_lua_destroy,
+};
+
+void
+port_lua_create(struct port *port, struct lua_State *L)
+{
+ struct port_lua *port_lua = (struct port_lua *) port;
+ memset(port_lua, 0, sizeof(*port_lua));
+ port_lua->vtab = &port_lua_vtab;
+ port_lua->L = L;
+ /*
+ * Allow to destroy the port even if no ref.
+ * @Sa luaL_unref.
+ */
+ port_lua->ref = LUA_REFNIL;
+}
diff --git a/src/box/port.h b/src/box/port.h
index f18803660..28fc16ce9 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -88,7 +88,7 @@ struct port_lua {
/** Reference to L in tarantool_L. */
int ref;
/** The argument of port_dump */
- struct obuf *out;
+ struct mpstream *stream;
/** Number of entries dumped to the port. */
int size;
};
diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index 8ace40fc5..e2f657ce0 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -37,6 +37,7 @@ extern "C" {
#endif /* defined(__cplusplus) */
struct obuf;
+struct region;
struct lua_State;
struct port;
@@ -62,6 +63,15 @@ struct port_vtab {
* header. Used by the legacy Tarantool 1.6 format.
*/
int (*dump_msgpack_16)(struct port *port, struct obuf *out);
+ /**
+ * Dump the content of a port to a region.
+ * @param port Port to dump.
+ * @param region Region to dump to.
+ *
+ * @retval >= 0 Number of entries dumped.
+ * @retval < 0 Error.
+ */
+ int (*dump_msgpack_region)(struct port *port, struct region *region);
/** Dump the content of a port to Lua stack. */
void (*dump_lua)(struct port *port, struct lua_State *L);
/**
@@ -108,6 +118,12 @@ port_dump_msgpack_16(struct port *port, struct obuf *out)
return port->vtab->dump_msgpack_16(port, out);
}
+static inline int
+port_dump_msgpack_region(struct port *port, struct region *region)
+{
+ return port->vtab->dump_msgpack_region(port, region);
+}
+
static inline void
port_dump_lua(struct port *port, struct lua_State *L)
{
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 8/9] box: implement lua_port dump to region and to Lua
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
0 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 14:24 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Thu, Jun 06, 2019 at 03:04:04PM +0300, Kirill Shcherbatov wrote:
> Refactored port_lua class to reuse an existent machinery to
> dump info not only for obuf, but to region, that is also
> mpstream-compatible. We need this feature in scope of multikey
> indexes to work with keys produced with functional index
> extractor in memory.
>
> Also introduce a tiny method .dump_lua for port_lua. It is
> necessary to export registered on-board functions call endpoints.
>
> Class implementation is moved to a new file port_lua.c.
>
> Part of #4182
> Needed for #1260
> ---
> src/box/CMakeLists.txt | 1 +
> src/box/execute.c | 1 +
> src/box/lua/call.c | 254 +-------------------------------
> src/box/lua/port_lua.c | 318 +++++++++++++++++++++++++++++++++++++++++
> src/box/port.h | 2 +-
> src/lib/core/port.h | 16 +++
> 6 files changed, 338 insertions(+), 254 deletions(-)
> create mode 100644 src/box/lua/port_lua.c
As I've already mentioned, it isn't a good practice to move and patch
code in the same patch. In this particular case you should've introduced
the new port method first and only then move port_lua to a separate
file.
Anyway, I don't think it's worth moving port_lua out of lua/call.c -
this is the only place where it is used and I don't see why we would
need it anywhere else. Let's please leave is where it is for now.
Regarding the new method. We will need to encapsulate function arguments
in a port so that we can pass a tuple with format information to a Lua
function, not just raw msgpack:
struct port_msgpack {
...
/** Pointer to raw msgpack data. */
void *data;
void *data_end;
};
We will create it from call_request.
To forward it to a C function, we will need a new virtual port method
that would return a pointer to the raw msgpack, because a C function
takes raw msgpack (in contrast to Lua which takes Lua stack for which we
already have dump_lua method). In case of port_lua and port_tuple we
have no other choice but copy data to an area allocated on the region.
However, for port_msgpack we can return a pointer to data stored in it
without any copying. So I guess the new method should be called in such
a way that it doesn't refer to a region. Not port_dump_msgpack_region.
May be, port_get_msgpack or port_to_msgpack. Not sure about the name.
The function should either return a pointer to msgpack data already
stored in the port or allocate it on the region.
We probably need to implement this new method only for port_lua and
port_msgpack, because port_tuple will be used as input argument only
by functional indexes, which can't use a C function for indexing.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 9/9] box: export _func functions with box.func folder
2019-06-06 12:03 [PATCH v2 0/9] box: rework functions machinery Kirill Shcherbatov
` (7 preceding siblings ...)
2019-06-06 12:04 ` [PATCH v2 8/9] box: implement lua_port dump to region and to Lua Kirill Shcherbatov
@ 2019-06-06 12:04 ` Kirill Shcherbatov
2019-06-10 15:18 ` Vladimir Davydov
2019-06-10 9:14 ` [PATCH v2 0/9] box: rework functions machinery Vladimir Davydov
9 siblings, 1 reply; 22+ messages in thread
From: Kirill Shcherbatov @ 2019-06-06 12:04 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov
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.
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.
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
...
---
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);
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. */
+ 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);
+}
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);
+
+ struct port port;
+ if (box_func_execute_by_name(name, name_len, &port, tuple,
+ tuple + tuple_len) != 0)
+ 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))
+ end
+end
+
+local func_f_mt = {}
+
+func_f_mt.drop = function(func, opts)
+ 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
+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.
+ */
+extern struct rlist on_alter_func;
+
/**
* Context passed to on_access_denied trigger.
*/
diff --git a/test/box/misc.result b/test/box/misc.result
index a4c8ae498..c1a81f75b 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -65,6 +65,7 @@ t
- error
- execute
- feedback
+ - func
- index
- info
- internal
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()
+---
+...
+box.schema.user.revoke('guest', 'execute', 'function', 'secret_leak')
+---
+...
+box.schema.func.drop('secret_leak')
+---
+...
+box.schema.func.drop('secret')
+---
+...
diff --git a/test/box/persistent_func.test.lua b/test/box/persistent_func.test.lua
index 095a27872..062c3b060 100644
--- a/test/box/persistent_func.test.lua
+++ b/test/box/persistent_func.test.lua
@@ -39,6 +39,26 @@ conn:call("monkey", {1})
math.abs(1)
box.schema.func.drop('monkey')
+test_run:cmd("setopt delimiter ';'")
+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
+array = {1, 3, 2, 5, 9}
+box.func.minmax:call({array})
+conn:call("minmax", {array})
+
+box.func.minmax:drop()
+box.schema.func.exists("minmax")
+
-- Test function with spell error - case 1.
test_run:cmd("setopt delimiter ';'")
body_bad2 = [[function(tuple)
@@ -74,3 +94,19 @@ box.schema.func.drop('test')
box.schema.func.exists('test')
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({})
+
+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')
+
+conn:close()
+box.schema.user.revoke('guest', 'execute', 'function', 'secret_leak')
+box.schema.func.drop('secret_leak')
+box.schema.func.drop('secret')
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 9/9] box: export _func functions with box.func folder
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
0 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 15:18 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
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')
> +---
> +...
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/9] box: rework functions machinery
2019-06-06 12:03 [PATCH v2 0/9] box: rework functions machinery Kirill Shcherbatov
` (8 preceding siblings ...)
2019-06-06 12:04 ` [PATCH v2 9/9] box: export _func functions with box.func folder Kirill Shcherbatov
@ 2019-06-10 9:14 ` Vladimir Davydov
9 siblings, 0 replies; 22+ messages in thread
From: Vladimir Davydov @ 2019-06-10 9:14 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches
On Thu, Jun 06, 2019 at 03:03:56PM +0300, Kirill Shcherbatov wrote:
> src/box/CMakeLists.txt | 1 +
> src/box/alter.cc | 118 +++++--
> src/box/bootstrap.snap | Bin 4393 -> 4449 bytes
> src/box/call.c | 157 +--------
> src/box/call.h | 14 -
> src/box/errcode.h | 1 +
> src/box/execute.c | 1 +
> src/box/func.c | 564 ++++++++++++++++++++++++++++--
> src/box/func.h | 84 +++--
> src/box/func_def.c | 9 +
> src/box/func_def.h | 43 ++-
> src/box/lua/call.c | 379 ++------------------
> src/box/lua/init.c | 2 +
> src/box/lua/port_lua.c | 318 +++++++++++++++++
> src/box/lua/schema.lua | 38 +-
> src/box/lua/upgrade.lua | 25 +-
> src/box/port.h | 2 +-
> src/box/schema.cc | 47 ++-
> src/box/schema.h | 31 +-
> src/box/schema_def.h | 4 +
> src/lib/core/port.h | 16 +
> src/lua/utils.c | 126 +++++++
> src/lua/utils.h | 19 +
I don't quite like the way you split the code among modules. First, we
try to keep all Lua-related stuff in src/lua or src/box/lua. There are
exceptions (e.g. luaT_module_find located in src/box/func.c), but we try
to avoid them.
I think that the code should be split as follows:
- src/box/func.[hc] should contain definition of the base func class,
C func implementation and module manipulation.
- src/box/lua/call.[ch] should contain implementation of Lua func
class, both for persistent functions and not, and helpers to run Lua
code (eval/call). We might want to rename src/box/lua/call.[hc] to
func.[hc], but I'm not sure it's really necessary at this point.
Also, I don't think it's worth moving sandboxing to util.[hc] - it's
only used to run Lua code so I guess it's okay to leave it in
lua/call along with port_lua and luaT_func_find.
- src/box/call.[hc] should contain helpers to invoke a call request
(CALL/EVAL). CALL code should check permissions, look up function by
name and, if it's found execute, execute it, otherwise call a helper
function defined in lua/func to run the function by name.
A general node: please avoid patching and moving the code at the same
time, except cases when everything you change is names of the moved
functions: this makes the code nearly impossible to review for
correctness.
Here's how I'd try to organize the patch set:
1. Simplify func updates on alter (in a little bit different way than
you did - see my comments to the corresponding patch).
2. Re-factor box_lua_call and box_lua_eval so that they don't take
call_request. I think they should take struct port. Rationale: in
case of a functional index, the user expects to see a tuple with
field names so we should be able to pass not only raw msgpack, but
also a tuple to a Lua call.
3. Introduce base func class to src/box/func.[hc]. Implement func_c in
src/box/func.c and func_lua in src/box/lua/call.c (func_lua_create
should be declared in src/box/lua/call.h so that vtab and other
internals aren't exposed).
4. Add infrastructure to call functions from Lua. The code should live
in src/box/lua/call.c. Please write a separate docbot request for
this one.
5. Add persistent functions. Splitting it in three patches doesn't
really facilitate review IMO: I want to see the complete picture,
including new _func format and sandboxing. BTW I guess is_sandbox
shouldn't be a part of options at this time - it should be a
separate column, like is_deterministic.
I'll post a few more comments re the code in reply to emails with
patches.
Thoughts, objections?
^ permalink raw reply [flat|nested] 22+ messages in thread