Tarantool development patches archive
 help / color / mirror / Atom feed
* [PATCH v4 0/6] box: rework functions machinery
@ 2019-06-23 13:57 Kirill Shcherbatov
  2019-06-23 13:57 ` [PATCH v4 1/6] box: rework func cache update machinery Kirill Shcherbatov
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-06-23 13:57 UTC (permalink / raw)
  To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov

This patchset reworks Tarantool's functions machinery to support
persistent Lua functions and provide an universal way to call any
registered function.

This patchset consists of following conceptual changes:
1. Func updates on alter were simplified: now replace _func tuple is
   forbidden and func_new constructor may fail. This makes possible to
   define a failable function object constructors.
2. box_lua_call and box_lua_eval were refactored to do nottake call_request
   argument and to use input and output port instead. A new msgpack_port allows
   to use this abstraction also for a regular memory msgpack.
3. Introduced an abstract function class and func_c and func_lua
   implementations that allow to call C and Lua functions correspondingly
   with uniform API defined on base func class.
4. Introduced an infrastructure to call functions from Lua. A new box.func
   folder exports functions objects that have :call and :drop methods and their
   definition.
4. Introduced persistent Lua functions. Such functions are a part of Tarantool
   snapshoot so they are available even after restart. The new is_sandboxed
   option initializes a new persistent Lua function is a separate 'sandbox':
   an isolated environment with strong restrictions: only limited number of
   on-board functions are available, no global state is available at all.

Persistent Lua functions is an important step to implement functional indexes
in Tarantool. Only a deterministic, sandboxed, persistent Lua function may
be used as a functional index extractor.

Changes in version 4:
   Build of persistent Lua function in a sandbox
   New space format
   Minor review fixes

Changes in version 3:
   Source code is re-organised to be bit clear and to provide a better function
   object abstraction.

http://github.com/tarantool/tarantool/tree/kshch/gh-4182-persistent-lua-functions
https://github.com/tarantool/tarantool/issues/4182

Kirill Shcherbatov (6):
  box: rework func cache update machinery
  box: rework box_lua_{call, eval} to use input port
  box: rework func object as a function frontend
  box: export registered functions in box.func folder
  box: refactor box_lua_find helper
  box: introduce Lua persistent functions

 src/box/alter.cc             | 147 ++++++---
 src/box/bootstrap.snap       | Bin 4475 -> 4650 bytes
 src/box/call.c               | 168 +++-------
 src/box/call.h               |   4 -
 src/box/errcode.h            |   2 +-
 src/box/execute.c            |   1 +
 src/box/func.c               | 198 ++++++++++--
 src/box/func.h               |  34 +-
 src/box/func_def.c           |  25 ++
 src/box/func_def.h           |  51 ++-
 src/box/lua/call.c           | 608 +++++++++++++++++++++++++++++++----
 src/box/lua/call.h           |  13 +-
 src/box/lua/execute.c        |   8 +-
 src/box/lua/execute.h        |   4 +-
 src/box/lua/init.c           |   1 +
 src/box/lua/misc.cc          |  23 +-
 src/box/lua/schema.lua       |  47 ++-
 src/box/lua/upgrade.lua      |  34 +-
 src/box/port.c               |   3 +-
 src/box/port.h               |  17 +-
 src/box/schema.cc            |  31 +-
 src/box/schema.h             |  14 +-
 src/box/schema_def.h         |  11 +
 src/box/session.cc           |  11 +-
 src/box/session.h            |   9 +
 src/lib/core/port.h          |  38 ++-
 test/box-py/bootstrap.result |   6 +-
 test/box/access_misc.result  |   6 +-
 test/box/function1.c         |  33 ++
 test/box/function1.result    | 413 ++++++++++++++++++++++++
 test/box/function1.test.lua  | 139 ++++++++
 test/box/misc.result         |   2 +-
 test/wal_off/func_max.result |   4 +-
 33 files changed, 1766 insertions(+), 339 deletions(-)

-- 
2.21.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4 1/6] box: rework func cache update machinery
  2019-06-23 13:57 [PATCH v4 0/6] box: rework functions machinery Kirill Shcherbatov
@ 2019-06-23 13:57 ` Kirill Shcherbatov
  2019-06-24 10:22   ` Vladimir Davydov
  2019-06-23 13:57 ` [PATCH v4 2/6] box: rework box_lua_{call, eval} to use input port Kirill Shcherbatov
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-06-23 13:57 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. We need to fix it to perform
user-dependent risky actions like Lua function assemble in
further patches.

The replace method is disallowed for _func space because it is
redundant and difficult to maintain in case of functions that
have pre-compiled runtime.

Needed for #4182, #1260
---
 src/box/alter.cc             | 60 +++++++++++++++++++-----------------
 src/box/errcode.h            |  2 +-
 src/box/func.c               |  8 -----
 src/box/func.h               |  3 --
 src/box/func_def.c           | 17 ++++++++++
 src/box/func_def.h           | 12 ++++++++
 src/box/schema.cc            | 30 ++++++------------
 src/box/schema.h             |  9 ++----
 test/box/function1.result    |  4 +++
 test/box/function1.test.lua  |  1 +
 test/box/misc.result         |  1 -
 test/wal_off/func_max.result |  4 +--
 12 files changed, 81 insertions(+), 70 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index a37a68ce4..adad957f2 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2550,6 +2550,10 @@ func_def_new_from_tuple(struct tuple *tuple)
 		tnt_raise(OutOfMemory, func_def_sizeof(len), "malloc", "def");
 	auto def_guard = make_scoped_guard([=] { free(def); });
 	func_def_get_ids_from_tuple(tuple, &def->fid, &def->uid);
+	if (def->fid > BOX_FUNCTION_MAX) {
+		tnt_raise(ClientError, ER_CREATE_FUNCTION,
+			  tt_cstr(name, len), "function id is too big");
+	}
 	memcpy(def->name, name, len);
 	def->name[len] = 0;
 	if (tuple_field_count(tuple) > BOX_FUNC_FIELD_SETUID)
@@ -2574,24 +2578,11 @@ 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)
-{
-	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);
-}
-
-/** Replace a function in the function cache */
-static void
-func_cache_replace_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);
-	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 *old_func = (struct func *) trigger->data;
+	func_cache_delete(old_func->def->fid);
+	func_delete(old_func);
 }
 
 /**
@@ -2612,13 +2603,17 @@ 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);
-		def_guard.is_active = false;
 		struct trigger *on_rollback =
 			txn_alter_trigger_new(func_cache_remove_func, NULL);
+		struct func *func = func_new(def);
+		if (func == NULL)
+			diag_raise();
+		def_guard.is_active = false;
+		func_cache_insert(func);
+		on_rollback->data = func;
 		txn_on_rollback(txn, on_rollback);
 	} else if (new_tuple == NULL) {         /* DELETE */
 		uint32_t uid;
@@ -2636,16 +2631,25 @@ 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 trigger *on_commit =
-			txn_alter_trigger_new(func_cache_replace_func, NULL);
-		txn_on_commit(txn, on_commit);
+		assert(new_tuple != NULL && old_tuple != NULL);
+		/**
+		 * Allow an alter that doesn't change the
+		 * definition to support upgrade script.
+		 */
+		struct func_def *old_def = NULL, *new_def = NULL;
+		auto guard = make_scoped_guard([=] {
+			free(old_def);
+			free(new_def);
+		});
+		old_def = func_def_new_from_tuple(old_tuple);
+		new_def = func_def_new_from_tuple(new_tuple);
+		if (func_def_cmp(new_def, old_def) != 0) {
+			tnt_raise(ClientError, ER_UNSUPPORTED, "function",
+				  "alter");
+		}
 	}
 }
 
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 28d438333..55299b735 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -106,7 +106,7 @@ struct errcode_record {
 	/* 51 */_(ER_NO_SUCH_FUNCTION,		"Function '%s' does not exist") \
 	/* 52 */_(ER_FUNCTION_EXISTS,		"Function '%s' already exists") \
 	/* 53 */_(ER_BEFORE_REPLACE_RET,	"Invalid return value of space:before_replace trigger: expected tuple or nil, got %s") \
-	/* 54 */_(ER_FUNCTION_MAX,		"A limit on the total number of functions has been reached: %u") \
+	/* 54 */_(ER_UNUSED2,			"") \
 	/* 55 */_(ER_TRIGGER_EXISTS,		"Trigger '%s' already exists") \
 	/* 56 */_(ER_USER_MAX,			"A limit on the total number of users has been reached: %u") \
 	/* 57 */_(ER_NO_SUCH_ENGINE,		"Space engine '%s' does not exist") \
diff --git a/src/box/func.c b/src/box/func.c
index a817851fd..d1b8879ad 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -451,14 +451,6 @@ func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
 	return rc;
 }
 
-void
-func_update(struct func *func, struct func_def *def)
-{
-	func_unload(func);
-	free(func->def);
-	func->def = def;
-}
-
 void
 func_delete(struct func *func)
 {
diff --git a/src/box/func.h b/src/box/func.h
index 8dcd61d7b..fa4d738ab 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);
 
diff --git a/src/box/func_def.c b/src/box/func_def.c
index 76ed77b24..2b135e2d7 100644
--- a/src/box/func_def.c
+++ b/src/box/func_def.c
@@ -1,3 +1,20 @@
 #include "func_def.h"
+#include "string.h"
 
 const char *func_language_strs[] = {"LUA", "C"};
+
+int
+func_def_cmp(struct func_def *def1, struct func_def *def2)
+{
+	if (def1->fid != def2->fid)
+		return def1->fid - def2->fid;
+	if (def1->uid != def2->uid)
+		return def1->uid - def2->uid;
+	if (def1->setuid != def2->setuid)
+		return def1->setuid - def2->setuid;
+	if (def1->language != def2->language)
+		return def1->language - def2->language;
+	if (strcmp(def1->name, def2->name) != 0)
+		return strcmp(def1->name, def2->name);
+	return 0;
+}
diff --git a/src/box/func_def.h b/src/box/func_def.h
index 5b52ab498..4c9738aea 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -34,6 +34,10 @@
 #include "trivia/util.h"
 #include <stdbool.h>
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 /**
  * The supported language of the stored function.
  */
@@ -79,6 +83,10 @@ func_def_sizeof(uint32_t name_len)
 	return sizeof(struct func_def) + name_len + 1;
 }
 
+/** Compare two given function definitions. */
+int
+func_def_cmp(struct func_def *def1, struct func_def *def2);
+
 /**
  * API of C stored function.
  */
@@ -86,4 +94,8 @@ typedef struct box_function_ctx box_function_ctx_t;
 typedef int (*box_function_f)(box_function_ctx_t *ctx,
 	     const char *args, const char *args_end);
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif /* TARANTOOL_BOX_FUNC_DEF_H_INCLUDED */
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 6d3153815..d63add535 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -528,6 +528,7 @@ schema_free(void)
 		struct func *func = ((struct func *)
 				     mh_i32ptr_node(funcs, i)->val);
 		func_cache_delete(func->def->fid);
+		func_delete(func);
 	}
 	mh_i32ptr_delete(funcs);
 	while (mh_size(sequences) > 0) {
@@ -541,39 +542,27 @@ schema_free(void)
 }
 
 void
-func_cache_replace(struct func_def *def)
+func_cache_insert(struct func *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 };
+	assert(func_by_id(func->def->fid) == NULL);
+	assert(func_by_name(func->def->name, strlen(func->def->name)) == NULL);
+	const struct mh_i32ptr_node_t node = { func->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;
+		panic_syserror("Out of memory for the data dictionary cache "
+			       "(stored function).");
 	}
 	size_t def_name_len = strlen(func->def->name);
 	uint32_t name_hash = mh_strn_hash(func->def->name, def_name_len);
 	const struct mh_strnptr_node_t strnode = {
 		func->def->name, def_name_len, name_hash, func };
-
 	mh_int_t 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;
+		panic_syserror("Out of memory for the data dictionary cache "
+			       "(stored function).");
 	}
 }
 
@@ -590,7 +579,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..7e8ac6c11 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -163,14 +163,11 @@ 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.
+ * Insert a new function object in the function cache.
+ * @param func Function object to insert.
  */
 void
-func_cache_replace(struct func_def *def);
+func_cache_insert(struct func *func);
 
 void
 func_cache_delete(uint32_t fid);
diff --git a/test/box/function1.result b/test/box/function1.result
index 5b04fa97d..cadeb0467 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -16,6 +16,10 @@ c = net.connect(os.getenv("LISTEN"))
 box.schema.func.create('function1', {language = "C"})
 ---
 ...
+box.space._func:replace{2, 1, 'function1', 0, 'LUA'}
+---
+- error: function does not support alter
+...
 box.schema.user.grant('guest', 'execute', 'function', 'function1')
 ---
 ...
diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
index 855dd5382..e983495b6 100644
--- a/test/box/function1.test.lua
+++ b/test/box/function1.test.lua
@@ -7,6 +7,7 @@ net = require('net.box')
 c = net.connect(os.getenv("LISTEN"))
 
 box.schema.func.create('function1', {language = "C"})
+box.space._func:replace{2, 1, 'function1', 0, 'LUA'}
 box.schema.user.grant('guest', 'execute', 'function', 'function1')
 _ = box.schema.space.create('test')
 _ = box.space.test:create_index('primary')
diff --git a/test/box/misc.result b/test/box/misc.result
index 5c42e33e8..dfa06724c 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -384,7 +384,6 @@ t;
   51: box.error.NO_SUCH_FUNCTION
   52: box.error.FUNCTION_EXISTS
   53: box.error.BEFORE_REPLACE_RET
-  54: box.error.FUNCTION_MAX
   55: box.error.TRIGGER_EXISTS
   56: box.error.USER_MAX
   57: box.error.NO_SUCH_ENGINE
diff --git a/test/wal_off/func_max.result b/test/wal_off/func_max.result
index 5a43821b2..ab4217845 100644
--- a/test/wal_off/func_max.result
+++ b/test/wal_off/func_max.result
@@ -42,7 +42,7 @@ test_run:cmd("setopt delimiter ''");
 ...
 func_limit()
 ---
-- error: 'A limit on the total number of functions has been reached: 32000'
+- error: 'Failed to create function ''func32000'': function id is too big'
 ...
 drop_limit_func()
 ---
@@ -62,7 +62,7 @@ session.su('testuser')
 ...
 func_limit()
 ---
-- error: 'A limit on the total number of functions has been reached: 32000'
+- error: 'Failed to create function ''func32000'': function id is too big'
 ...
 drop_limit_func()
 ---
-- 
2.21.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4 2/6] box: rework box_lua_{call, eval} to use input port
  2019-06-23 13:57 [PATCH v4 0/6] box: rework functions machinery Kirill Shcherbatov
  2019-06-23 13:57 ` [PATCH v4 1/6] box: rework func cache update machinery Kirill Shcherbatov
@ 2019-06-23 13:57 ` Kirill Shcherbatov
  2019-06-24 10:23   ` Vladimir Davydov
  2019-06-23 13:57 ` [PATCH v4 3/6] box: rework func object as a function frontend Kirill Shcherbatov
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-06-23 13:57 UTC (permalink / raw)
  To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov

Re-factor box_lua_call and box_lua_eval so that they don't take
call_request. This approach is more scalable: 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 so we need an universal way to pass
arguments to _call methods.

To pass a tuple msgpack introduced a new port_msgpack: the port
class with dump_lua method.
A new method get_msgpack returns a content of a port as a
msgpack data. The lifecycle of the returned value is
implementation-specific: it may either be returned directly from
the port, in which case the data will stay alive as long as the
port is alive, or it may be allocated on the fiber()->gc, in
which case the caller is responsible for cleaning up.

Needed for #4182, #1260
---
 src/box/call.c        | 57 +++++++++++++++++++++++++++-------
 src/box/call.h        |  4 ---
 src/box/execute.c     |  1 +
 src/box/func.c        | 23 ++++++++++++--
 src/box/func.h        |  8 +++--
 src/box/func_def.h    |  7 +++++
 src/box/lua/call.c    | 71 +++++++++++++++++++++++++------------------
 src/box/lua/call.h    |  8 +++--
 src/box/lua/execute.c |  8 +++--
 src/box/lua/execute.h |  4 ++-
 src/box/lua/misc.cc   | 19 ++++++++++--
 src/box/port.c        |  3 +-
 src/box/port.h        | 15 +++++++++
 src/lib/core/port.h   | 38 ++++++++++++++++++++---
 14 files changed, 202 insertions(+), 64 deletions(-)

diff --git a/src/box/call.c b/src/box/call.c
index 56da53fb3..573a0d698 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -43,6 +43,39 @@
 #include "small/obuf.h"
 #include "tt_static.h"
 
+static const struct port_vtab port_msgpack_vtab;
+
+void
+port_msgpack_create(struct port *base, const char *data, uint32_t data_sz)
+{
+	struct port_msgpack *port_msgpack = (struct port_msgpack *) base;
+	memset(port_msgpack, 0, sizeof(*port_msgpack));
+	port_msgpack->vtab = &port_msgpack_vtab;
+	port_msgpack->data = data;
+	port_msgpack->data_sz = data_sz;
+}
+
+static const char *
+port_msgpack_get_msgpack(struct port *base, uint32_t *size)
+{
+	struct port_msgpack *port = (struct port_msgpack *) base;
+	assert(port->vtab == &port_msgpack_vtab);
+	*size = port->data_sz;
+	return port->data;
+}
+
+extern void
+port_msgpack_dump_lua(struct port *base, struct lua_State *L, bool is_flat);
+
+static const struct port_vtab port_msgpack_vtab = {
+	.dump_msgpack = NULL,
+	.dump_msgpack_16 = NULL,
+	.dump_lua = port_msgpack_dump_lua,
+	.dump_plain = NULL,
+	.get_msgpack = port_msgpack_get_msgpack,
+	.destroy = NULL,
+};
+
 /**
  * Find a function by name and check "EXECUTE" permissions.
  *
@@ -106,27 +139,22 @@ access_check_func(const char *name, uint32_t name_len, struct func **funcp)
 }
 
 static int
-box_c_call(struct func *func, struct call_request *request, struct port *port)
+box_c_call(struct func *func, struct port *args, struct port *ret)
 {
 	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);
+	int rc = func_call(func, args, ret);
 	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;
@@ -199,10 +227,13 @@ box_process_call(struct call_request *request, struct port *port)
 	}
 
 	int rc;
+	struct port args;
+	port_msgpack_create(&args, request->args,
+			    request->args_end - request->args);
 	if (func && func->def->language == FUNC_LANGUAGE_C) {
-		rc = box_c_call(func, request, port);
+		rc = box_c_call(func, &args, port);
 	} else {
-		rc = box_lua_call(request, port);
+		rc = box_lua_call(name, name_len, &args, port);
 	}
 	/* Restore the original user */
 	if (orig_credentials)
@@ -229,7 +260,12 @@ box_process_eval(struct call_request *request, struct port *port)
 	/* Check permissions */
 	if (access_check_universe(PRIV_X) != 0)
 		return -1;
-	if (box_lua_eval(request, port) != 0) {
+	struct port args;
+	port_msgpack_create(&args, request->args,
+			    request->args_end - request->args);
+	const char *expr = request->expr;
+	uint32_t expr_len = mp_decode_strl(&expr);
+	if (box_lua_eval(expr, expr_len, &args, port) != 0) {
 		txn_rollback();
 		return -1;
 	}
@@ -239,6 +275,5 @@ box_process_eval(struct call_request *request, struct port *port)
 		txn_rollback();
 		return -1;
 	}
-
 	return 0;
 }
diff --git a/src/box/call.h b/src/box/call.h
index 1b54551be..45580bc9d 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;
-};
-
 /**
  * Reload loadable module by name.
  *
diff --git a/src/box/execute.c b/src/box/execute.c
index 64ed3d46c..a9ca2e67a 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -108,6 +108,7 @@ const struct port_vtab port_sql_vtab = {
 	/* .dump_msgpack_16 = */ NULL,
 	/* .dump_lua = */ port_sql_dump_lua,
 	/* .dump_plain = */ NULL,
+	/* .get_msgpack = */ NULL,
 	/* .destroy = */ port_sql_destroy,
 };
 
diff --git a/src/box/func.c b/src/box/func.c
index d1b8879ad..1c37f6523 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -29,11 +29,13 @@
  * SUCH DAMAGE.
  */
 #include "func.h"
+#include "fiber.h"
 #include "trivia/config.h"
 #include "assoc.h"
 #include "lua/utils.h"
 #include "error.h"
 #include "diag.h"
+#include "port.h"
 #include <dlfcn.h>
 
 /**
@@ -433,21 +435,36 @@ func_load(struct func *func)
 }
 
 int
-func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
-	  const char *args_end)
+func_call(struct func *func, struct port *args, struct port *ret)
 {
+	assert(func != NULL && func->def->language == FUNC_LANGUAGE_C);
 	if (func->func == NULL) {
 		if (func_load(func) != 0)
 			return -1;
 	}
 
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+	uint32_t data_sz;
+	const char *data = port_get_msgpack(args, &data_sz);
+	if (data == NULL)
+		return -1;
+
+	port_tuple_create(ret);
+	box_function_ctx_t ctx = { ret };
+
 	/* 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);
+	int rc = func->func(&ctx, data, data + data_sz);
 	--module->calls;
 	module_gc(module);
+	region_truncate(region, region_svp);
+	if (rc != 0) {
+		port_destroy(ret);
+		return -1;
+	}
 	return rc;
 }
 
diff --git a/src/box/func.h b/src/box/func.h
index fa4d738ab..f6494d064 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -42,6 +42,9 @@
 extern "C" {
 #endif /* defined(__cplusplus) */
 
+struct func;
+struct lua_State;
+
 /**
  * Dynamic shared module.
  */
@@ -104,11 +107,10 @@ void
 func_delete(struct func *func);
 
 /**
- * Call stored C function using @a args.
+ * Call function with arguments represented with given args.
  */
 int
-func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
-	  const char *args_end);
+func_call(struct func *func, struct port *args, struct port *ret);
 
 /**
  * Reload dynamically loadable module.
diff --git a/src/box/func_def.h b/src/box/func_def.h
index 4c9738aea..ef33cbbaa 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -90,6 +90,13 @@ func_def_cmp(struct func_def *def1, struct func_def *def2);
 /**
  * API of C stored function.
  */
+
+struct port;
+
+struct box_function_ctx {
+	struct port *port;
+};
+
 typedef struct box_function_ctx box_function_ctx_t;
 typedef int (*box_function_f)(box_function_ctx_t *ctx,
 	     const char *args, const char *args_end);
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 04020ef6f..7cac90f3b 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -36,7 +36,6 @@
 #include "lua/utils.h"
 #include "lua/msgpack.h"
 
-#include "box/xrow.h"
 #include "box/port.h"
 #include "box/lua/tuple.h"
 #include "small/obuf.h"
@@ -282,28 +281,31 @@ port_lua_create(struct port *port, struct lua_State *L)
 	port_lua->ref = -1;
 }
 
+struct execute_lua_ctx {
+	const char *name;
+	uint32_t name_len;
+	struct port *args;
+};
+
 static int
 execute_lua_call(lua_State *L)
 {
-	struct call_request *request = (struct call_request *)
-		lua_topointer(L, 1);
+	struct execute_lua_ctx *ctx =
+		(struct execute_lua_ctx *) 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);
+	const char *name = ctx->name;
+	uint32_t name_len = ctx->name_len;
 
 	int oc = 0; /* how many objects are on stack after box_lua_find */
 	/* Try to find a function by name in Lua */
 	oc = box_lua_find(L, name, name + name_len);
 
 	/* 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");
+	int top = lua_gettop(L);
+	port_dump_lua(ctx->args, L, true);
+	int arg_count = lua_gettop(L) - top;
 
-	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);
 }
@@ -311,24 +313,22 @@ execute_lua_call(lua_State *L)
 static int
 execute_lua_eval(lua_State *L)
 {
-	struct call_request *request = (struct call_request *)
-		lua_topointer(L, 1);
+	struct execute_lua_ctx *ctx =
+		(struct execute_lua_ctx *) lua_topointer(L, 1);
 	lua_settop(L, 0); /* clear the stack to simplify the logic below */
 
 	/* Compile expression */
-	const char *expr = request->expr;
-	uint32_t expr_len = mp_decode_strl(&expr);
+	const char *expr = ctx->name;
+	uint32_t expr_len = ctx->name_len;
 	if (luaL_loadbuffer(L, expr, expr_len, "=eval")) {
 		diag_set(LuajitError, lua_tostring(L, -1));
 		luaT_error(L);
 	}
 
 	/* Unpack arguments */
-	const char *args = request->args;
-	uint32_t arg_count = mp_decode_array(&args);
-	luaL_checkstack(L, arg_count, "eval: out of stack");
-	for (uint32_t i = 0; i < arg_count; i++)
-		luamp_decode(L, luaL_msgpack_default, &args);
+	int top = lua_gettop(L);
+	port_dump_lua(ctx->args, L, true);
+	int arg_count = lua_gettop(L) - top;
 
 	/* Call compiled code */
 	lua_call(L, arg_count, LUA_MULTRET);
@@ -429,37 +429,48 @@ static const struct port_vtab port_lua_vtab = {
 	.dump_msgpack_16 = port_lua_dump_16,
 	.dump_lua = NULL,
 	.dump_plain = port_lua_dump_plain,
+	.get_msgpack = NULL,
 	.destroy = port_lua_destroy,
 };
 
 static inline int
-box_process_lua(struct call_request *request, struct port *base,
-		lua_CFunction handler)
+box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx,
+		struct port *ret)
 {
 	lua_State *L = lua_newthread(tarantool_L);
 	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
-	port_lua_create(base, L);
-	((struct port_lua *) base)->ref = coro_ref;
+	port_lua_create(ret, L);
+	((struct port_lua *) ret)->ref = coro_ref;
 
 	lua_pushcfunction(L, handler);
-	lua_pushlightuserdata(L, request);
+	lua_pushlightuserdata(L, ctx);
 	if (luaT_call(L, 1, LUA_MULTRET) != 0) {
-		port_lua_destroy(base);
+		port_lua_destroy(ret);
 		return -1;
 	}
 	return 0;
 }
 
 int
-box_lua_call(struct call_request *request, struct port *port)
+box_lua_call(const char *name, uint32_t name_len,
+	     struct port *args, struct port *ret)
 {
-	return box_process_lua(request, port, execute_lua_call);
+	struct execute_lua_ctx ctx;
+	ctx.name = name;
+	ctx.name_len = name_len;
+	ctx.args = args;
+	return box_process_lua(execute_lua_call, &ctx, ret);
 }
 
 int
-box_lua_eval(struct call_request *request, struct port *port)
+box_lua_eval(const char *expr, uint32_t expr_len,
+	     struct port *args, struct port *ret)
 {
-	return box_process_lua(request, port, execute_lua_eval);
+	struct execute_lua_ctx ctx;
+	ctx.name = expr;
+	ctx.name_len = expr_len;
+	ctx.args = args;
+	return box_process_lua(execute_lua_eval, &ctx, ret);
 }
 
 static int
diff --git a/src/box/lua/call.h b/src/box/lua/call.h
index 0542123da..d03bcd0f8 100644
--- a/src/box/lua/call.h
+++ b/src/box/lua/call.h
@@ -31,6 +31,8 @@
  * SUCH DAMAGE.
  */
 
+#include <stdint.h>
+
 #if defined(__cplusplus)
 extern "C" {
 #endif /* defined(__cplusplus) */
@@ -48,10 +50,12 @@ struct call_request;
  * (implementation of 'CALL' command code).
  */
 int
-box_lua_call(struct call_request *request, struct port *port);
+box_lua_call(const char *name, uint32_t name_len,
+	     struct port *args, struct port *ret);
 
 int
-box_lua_eval(struct call_request *request, struct port *port);
+box_lua_eval(const char *expr, uint32_t expr_len,
+	     struct port *args, struct port *ret);
 
 #if defined(__cplusplus)
 } /* extern "C" */
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 239aba47b..10fa35de2 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -39,8 +39,10 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
 }
 
 void
-port_sql_dump_lua(struct port *port, struct lua_State *L)
+port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
 {
+	(void) is_flat;
+	assert(is_flat == false);
 	assert(port->vtab == &port_sql_vtab);
 	struct sql *db = sql_get();
 	struct sql_stmt *stmt = ((struct port_sql *)port)->stmt;
@@ -49,7 +51,7 @@ port_sql_dump_lua(struct port *port, struct lua_State *L)
 		lua_createtable(L, 0, 2);
 		lua_sql_get_metadata(stmt, L, column_count);
 		lua_setfield(L, -2, "metadata");
-		port_tuple_vtab.dump_lua(port, L);
+		port_tuple_vtab.dump_lua(port, L, false);
 		lua_setfield(L, -2, "rows");
 	} else {
 		assert(((struct port_tuple *)port)->size == 0);
@@ -262,7 +264,7 @@ lbox_execute(struct lua_State *L)
 	if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
 				    &fiber()->gc) != 0)
 		return luaT_error(L);
-	port_dump_lua(&port, L);
+	port_dump_lua(&port, L, false);
 	port_destroy(&port);
 	return 1;
 }
diff --git a/src/box/lua/execute.h b/src/box/lua/execute.h
index bc4df19f5..23e193fa4 100644
--- a/src/box/lua/execute.h
+++ b/src/box/lua/execute.h
@@ -30,6 +30,8 @@
  * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
+#include <stdbool.h>
+
 struct port;
 struct sql_bind;
 struct lua_State;
@@ -42,7 +44,7 @@ struct lua_State;
  * @param L Lua stack.
  */
 void
-port_sql_dump_lua(struct port *port, struct lua_State *L);
+port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat);
 
 /**
  * Parse Lua table of SQL parameters.
diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index a835d3d6f..de9ed407b 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -65,8 +65,10 @@ lbox_encode_tuple_on_gc(lua_State *L, int idx, size_t *p_len)
  * but implemented here to eliminate port.c dependency on Lua.
  */
 extern "C" void
-port_tuple_dump_lua(struct port *base, struct lua_State *L)
+port_tuple_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
 {
+	(void) is_flat;
+	assert(is_flat == false);
 	struct port_tuple *port = port_tuple(base);
 	lua_createtable(L, port->size, 0);
 	struct port_tuple_entry *pe = port->first;
@@ -76,6 +78,19 @@ port_tuple_dump_lua(struct port *base, struct lua_State *L)
 	}
 }
 
+extern "C" void
+port_msgpack_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
+{
+	(void) is_flat;
+	assert(is_flat == true);
+	struct port_msgpack *port = (struct port_msgpack *) base;
+
+	const char *args = port->data;
+	uint32_t arg_count = mp_decode_array(&args);
+	for (uint32_t i = 0; i < arg_count; i++)
+		luamp_decode(L, luaL_msgpack_default, &args);
+}
+
 /* }}} */
 
 /** {{{ Lua/C implementation of index:select(): used only by Vinyl **/
@@ -113,7 +128,7 @@ lbox_select(lua_State *L)
 	 * table always crashed the first (can't be fixed with pcall).
 	 * https://github.com/tarantool/tarantool/issues/1182
 	 */
-	port_dump_lua(&port, L);
+	port_dump_lua(&port, L, false);
 	port_destroy(&port);
 	return 1; /* lua table with tuples */
 }
diff --git a/src/box/port.c b/src/box/port.c
index 99046449a..7f552bcfe 100644
--- a/src/box/port.c
+++ b/src/box/port.c
@@ -125,7 +125,7 @@ port_tuple_dump_msgpack(struct port *base, struct obuf *out)
 }
 
 extern void
-port_tuple_dump_lua(struct port *base, struct lua_State *L);
+port_tuple_dump_lua(struct port *base, struct lua_State *L, bool is_flat);
 
 void
 port_init(void)
@@ -145,5 +145,6 @@ const struct port_vtab port_tuple_vtab = {
 	.dump_msgpack_16 = port_tuple_dump_msgpack_16,
 	.dump_lua = port_tuple_dump_lua,
 	.dump_plain = NULL,
+	.get_msgpack = NULL,
 	.destroy = port_tuple_destroy,
 };
diff --git a/src/box/port.h b/src/box/port.h
index f18803660..db93f8eea 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -32,6 +32,7 @@
  */
 #include "trivia/util.h"
 #include <port.h>
+#include <stdbool.h>
 
 #if defined(__cplusplus)
 extern "C" {
@@ -80,6 +81,20 @@ port_tuple_create(struct port *port);
 int
 port_tuple_add(struct port *port, struct tuple *tuple);
 
+/** Port implementation used for storing raw data. */
+struct port_msgpack {
+	const struct port_vtab *vtab;
+	const char *data;
+	uint32_t data_sz;
+};
+
+static_assert(sizeof(struct port_msgpack) <= sizeof(struct port),
+	      "sizeof(struct port_msgpack) must be <= sizeof(struct port)");
+
+/** Initialize a port to dump raw data. */
+void
+port_msgpack_create(struct port *port, const char *data, uint32_t data_sz);
+
 /** Port for storing the result of a Lua CALL/EVAL. */
 struct port_lua {
 	const struct port_vtab *vtab;
diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index 8ace40fc5..09a026df5 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -30,6 +30,7 @@
  * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
+#include <stdbool.h>
 #include <stdint.h>
 
 #if defined(__cplusplus)
@@ -62,8 +63,17 @@ 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 Lua stack. */
-	void (*dump_lua)(struct port *port, struct lua_State *L);
+	/**
+	 * Dump the content of a port to a given Lua stack.
+	 * When is_flat == true is specified, the data is dumped
+	 * directly to Lua stack, item-by-item. Otherwise, a
+	 * result table is created. The is_flat == true mode
+	 * follows Lua functions convention to pass arguments
+	 * and return a results, while is_flat == false follows
+	 * Tarantool's :select convention when the table of
+	 * results is returned.
+	 */
+	void (*dump_lua)(struct port *port, struct lua_State *L, bool is_flat);
 	/**
 	 * Dump a port content as a plain text into a buffer,
 	 * allocated inside.
@@ -74,6 +84,20 @@ struct port_vtab {
 	 * @retval not nil Plain text.
 	 */
 	const char *(*dump_plain)(struct port *port, uint32_t *size);
+	/**
+	 * Get the content of a port as a msgpack data.
+	 * By an obsolete convention, C stored routines expect
+	 * msgpack data as input format for its arguments.
+	 * This API is also usefull to process a function
+	 * returned value as msgpack data in memory.
+	 * The lifecycle of the returned value is
+	 * implementation-specific: it may either be returned
+	 * directly from the port, in which case the data will
+	 * stay alive as long as the port is alive, or it may be
+	 * allocated on the fiber()->gc, in which case the caller
+	 * is responsible for cleaning up.
+	 **/
+	const char *(*get_msgpack)(struct port *port, uint32_t *size);
 	/** Destroy a port and release associated resources. */
 	void (*destroy)(struct port *port);
 };
@@ -109,9 +133,9 @@ port_dump_msgpack_16(struct port *port, struct obuf *out)
 }
 
 static inline void
-port_dump_lua(struct port *port, struct lua_State *L)
+port_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
 {
-	port->vtab->dump_lua(port, L);
+	port->vtab->dump_lua(port, L, is_flat);
 }
 
 static inline const char *
@@ -120,6 +144,12 @@ port_dump_plain(struct port *port, uint32_t *size)
 	return port->vtab->dump_plain(port, size);
 }
 
+static inline const char *
+port_get_msgpack(struct port *port, uint32_t *size)
+{
+	return port->vtab->get_msgpack(port, size);
+}
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined __cplusplus */
-- 
2.21.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4 3/6] box: rework func object as a function frontend
  2019-06-23 13:57 [PATCH v4 0/6] box: rework functions machinery Kirill Shcherbatov
  2019-06-23 13:57 ` [PATCH v4 1/6] box: rework func cache update machinery Kirill Shcherbatov
  2019-06-23 13:57 ` [PATCH v4 2/6] box: rework box_lua_{call, eval} to use input port Kirill Shcherbatov
@ 2019-06-23 13:57 ` Kirill Shcherbatov
  2019-06-24 10:23   ` Vladimir Davydov
  2019-06-23 13:57 ` [PATCH v4 4/6] box: export registered functions in box.func folder Kirill Shcherbatov
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-06-23 13:57 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.

Introduced classes func_c and func_lua, that provide own
constructors which produce implementation-specific object with
call and destroy methods.

Needed for #4182, #1260
---
 src/box/alter.cc   |   1 +
 src/box/call.c     | 131 ++--------------------------------
 src/box/func.c     | 170 ++++++++++++++++++++++++++++++++++++++++-----
 src/box/func.h     |  23 +++---
 src/box/func_def.h |   2 +
 src/box/lua/call.c |  45 ++++++++++++
 src/box/lua/call.h |   5 ++
 src/box/session.cc |  11 ++-
 src/box/session.h  |   9 +++
 9 files changed, 240 insertions(+), 157 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index adad957f2..32c4b566a 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2556,6 +2556,7 @@ func_def_new_from_tuple(struct tuple *tuple)
 	}
 	memcpy(def->name, name, len);
 	def->name[len] = 0;
+	def->name_len = len;
 	if (tuple_field_count(tuple) > BOX_FUNC_FIELD_SETUID)
 		def->setuid = tuple_field_u32_xc(tuple, BOX_FUNC_FIELD_SETUID);
 	else
diff --git a/src/box/call.c b/src/box/call.c
index 573a0d698..bd03e8b96 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -76,90 +76,6 @@ static const struct port_vtab port_msgpack_vtab = {
 	.destroy = NULL,
 };
 
-/**
- * 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 port *args, struct port *ret)
-{
-	assert(func != NULL && func->def->language == FUNC_LANGUAGE_C);
-
-	/* 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, args, ret);
-	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");
-		}
-		return -1;
-	}
-	return 0;
-}
-
 int
 box_module_reload(const char *name)
 {
@@ -191,53 +107,20 @@ box_process_call(struct call_request *request, struct port *port)
 	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);
-	}
+	/* Transaction is not started. */
+	assert(!in_txn());
 
 	int rc;
 	struct port args;
 	port_msgpack_create(&args, request->args,
 			    request->args_end - request->args);
-	if (func && func->def->language == FUNC_LANGUAGE_C) {
-		rc = box_c_call(func, &args, port);
-	} else {
+	struct func *func = func_by_name(name, name_len);
+	if (func != NULL) {
+		rc = func_call(func, &args, port);
+	} else if ((rc = access_check_universe_object(PRIV_X | PRIV_U,
+				SC_FUNCTION, tt_cstr(name, name_len))) == 0) {
 		rc = box_lua_call(name, name_len, &args, port);
 	}
-	/* Restore the original user */
-	if (orig_credentials)
-		fiber_set_user(fiber(), orig_credentials);
 
 	if (rc != 0) {
 		txn_rollback();
diff --git a/src/box/func.c b/src/box/func.c
index 1c37f6523..c57027809 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -33,9 +33,12 @@
 #include "trivia/config.h"
 #include "assoc.h"
 #include "lua/utils.h"
+#include "lua/call.h"
 #include "error.h"
 #include "diag.h"
 #include "port.h"
+#include "schema.h"
+#include "session.h"
 #include <dlfcn.h>
 
 /**
@@ -50,6 +53,24 @@ struct func_name {
 	const char *package_end;
 };
 
+struct func_c {
+	/** Function object base class. */
+	struct func base;
+	/**
+	 * Anchor for module membership.
+	 */
+	struct rlist item;
+	/**
+	 * For C functions, the body of the function.
+	 */
+	box_function_f func;
+	/**
+	 * Each stored function keeps a handle to the
+	 * dynamic library for the C callback.
+	 */
+	struct module *module;
+};
+
 /***
  * Split function name to symbol and package names.
  * For example, str = foo.bar.baz => sym = baz, package = foo.bar
@@ -314,10 +335,10 @@ module_reload(const char *package, const char *package_end, struct module **modu
 	if (new_module == NULL)
 		return -1;
 
-	struct func *func, *tmp_func;
+	struct func_c *func, *tmp_func;
 	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
 		struct func_name name;
-		func_split_name(func->def->name, &name);
+		func_split_name(func->base.def->name, &name);
 		func->func = module_sym(new_module, name.sym);
 		if (func->func == NULL)
 			goto restore;
@@ -338,7 +359,7 @@ restore:
 	 */
 	do {
 		struct func_name name;
-		func_split_name(func->def->name, &name);
+		func_split_name(func->base.def->name, &name);
 		func->func = module_sym(old_module, name.sym);
 		if (func->func == NULL) {
 			/*
@@ -351,20 +372,27 @@ restore:
 		func->module = old_module;
 		rlist_move(&old_module->funcs, &func->item);
 	} while (func != rlist_first_entry(&old_module->funcs,
-					   struct func, item));
+					   struct func_c, item));
 	assert(rlist_empty(&new_module->funcs));
 	module_delete(new_module);
 	return -1;
 }
 
+static struct func *
+func_c_new(struct func_def *def);
+
 struct func *
 func_new(struct func_def *def)
 {
-	struct func *func = (struct func *) malloc(sizeof(struct func));
-	if (func == NULL) {
-		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
-		return NULL;
+	struct func *func;
+	if (def->language == FUNC_LANGUAGE_C) {
+		func = func_c_new(def);
+	} else {
+		assert(def->language == FUNC_LANGUAGE_LUA);
+		func = func_lua_new(def);
 	}
+	if (func == NULL)
+		return NULL;
 	func->def = def;
 	/** Nobody has access to the function but the owner. */
 	memset(func->access, 0, sizeof(func->access));
@@ -380,19 +408,35 @@ func_new(struct func_def *def)
 	 * checks (see user_has_data()).
 	 */
 	func->owner_credentials.auth_token = BOX_USER_MAX; /* invalid value */
+	return func;
+}
+
+static struct func_vtab func_c_vtab;
+
+static struct func *
+func_c_new(struct func_def *def)
+{
+	(void) def;
+	assert(def->language == FUNC_LANGUAGE_C);
+	struct func_c *func = (struct func_c *) malloc(sizeof(struct func_c));
+	if (func == NULL) {
+		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
+		return NULL;
+	}
+	func->base.vtab = &func_c_vtab;
 	func->func = NULL;
 	func->module = NULL;
-	return func;
+	return &func->base;
 }
 
 static void
-func_unload(struct func *func)
+func_c_unload(struct func_c *func)
 {
 	if (func->module) {
 		rlist_del(&func->item);
 		if (rlist_empty(&func->module->funcs)) {
 			struct func_name name;
-			func_split_name(func->def->name, &name);
+			func_split_name(func->base.def->name, &name);
 			module_cache_del(name.package, name.package_end);
 		}
 		module_gc(func->module);
@@ -401,17 +445,27 @@ func_unload(struct func *func)
 	func->func = NULL;
 }
 
+static void
+func_c_destroy(struct func *base)
+{
+	assert(base->vtab == &func_c_vtab);
+	assert(base != NULL && base->def->language == FUNC_LANGUAGE_C);
+	struct func_c *func = (struct func_c *) base;
+	func_c_unload(func);
+	free(func);
+}
+
 /**
  * Resolve func->func (find the respective DLL and fetch the
  * symbol from it).
  */
 static int
-func_load(struct func *func)
+func_c_load(struct func_c *func)
 {
 	assert(func->func == NULL);
 
 	struct func_name name;
-	func_split_name(func->def->name, &name);
+	func_split_name(func->base.def->name, &name);
 
 	struct module *module = module_cache_find(name.package,
 						  name.package_end);
@@ -435,11 +489,13 @@ func_load(struct func *func)
 }
 
 int
-func_call(struct func *func, struct port *args, struct port *ret)
+func_c_call(struct func *base, struct port *args, struct port *ret)
 {
-	assert(func != NULL && func->def->language == FUNC_LANGUAGE_C);
+	assert(base->vtab == &func_c_vtab);
+	assert(base != NULL && base->def->language == FUNC_LANGUAGE_C);
+	struct func_c *func = (struct func_c *) base;
 	if (func->func == NULL) {
-		if (func_load(func) != 0)
+		if (func_c_load(func) != 0)
 			return -1;
 	}
 
@@ -462,16 +518,94 @@ func_call(struct func *func, struct port *args, struct port *ret)
 	module_gc(module);
 	region_truncate(region, region_svp);
 	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(ret);
 		return -1;
 	}
 	return rc;
 }
 
+static struct func_vtab func_c_vtab = {
+	.call = func_c_call,
+	.destroy = func_c_destroy,
+};
+
 void
 func_delete(struct func *func)
 {
-	func_unload(func);
 	free(func->def);
-	free(func);
+	func->vtab->destroy(func);
+}
+
+/** Check "EXECUTE" permissions for a given function. */
+static int
+func_access_check(struct func *func)
+{
+	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_access & PRIV_U) != 0 ||
+	    (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) {
+			diag_set(AccessDeniedError, priv_name(PRIV_X),
+				 schema_object_name(SC_FUNCTION),
+				 func->def->name, user->def->name);
+		}
+		return -1;
+	}
+	return 0;
+}
+
+int
+func_call(struct func *base, struct port *args, struct port *ret)
+{
+	if (func_access_check(base) != 0)
+		return -1;
+	/**
+	 * 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 (base->def->setuid) {
+		orig_credentials = effective_user();
+		/* Remember and change the current user id. */
+		if (base->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(base->def->uid);
+			if (owner == NULL)
+				return -1;
+			credentials_init(&base->owner_credentials,
+					 owner->auth_token,
+					 owner->def->uid);
+		}
+		fiber_set_user(fiber(), &base->owner_credentials);
+	}
+	int rc = base->vtab->call(base, args, ret);
+	/* Restore the original user */
+	if (orig_credentials)
+		fiber_set_user(fiber(), orig_credentials);
+	return rc;
 }
diff --git a/src/box/func.h b/src/box/func.h
index f6494d064..e121e6bb9 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -59,24 +59,21 @@ struct module {
 	bool is_unloading;
 };
 
+/** Virtual method table for func object. */
+struct func_vtab {
+	/** Call function with given arguments. */
+	int (*call)(struct func *func, struct port *args, struct port *ret);
+	/** Release implementation-specific function context. */
+	void (*destroy)(struct func *func);
+};
+
 /**
  * Stored function.
  */
 struct func {
 	struct func_def *def;
-	/**
-	 * Anchor for module membership.
-	 */
-	struct rlist item;
-	/**
-	 * For C functions, the body of the function.
-	 */
-	box_function_f func;
-	/**
-	 * Each stored function keeps a handle to the
-	 * dynamic library for the C callback.
-	 */
-	struct module *module;
+	/** Virtual method table. */
+	const struct func_vtab *vtab;
 	/**
 	 * Authentication id of the owner of the function,
 	 * used for set-user-id functions.
diff --git a/src/box/func_def.h b/src/box/func_def.h
index ef33cbbaa..866d425a1 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -67,6 +67,8 @@ struct func_def {
 	 * The language of the stored function.
 	 */
 	enum func_language language;
+	/** The length of the function name. */
+	uint32_t name_len;
 	/** Function name. */
 	char name[0];
 };
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 7cac90f3b..8b7223a7c 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -31,6 +31,8 @@
 #include "box/lua/call.h"
 #include "box/call.h"
 #include "box/error.h"
+#include "box/func.h"
+#include "box/func_def.h"
 #include "fiber.h"
 
 #include "lua/utils.h"
@@ -473,6 +475,49 @@ box_lua_eval(const char *expr, uint32_t expr_len,
 	return box_process_lua(execute_lua_eval, &ctx, ret);
 }
 
+struct func_lua {
+	/** Function object base class. */
+	struct func base;
+};
+
+static struct func_vtab func_lua_vtab;
+
+struct func *
+func_lua_new(struct func_def *def)
+{
+	(void) def;
+	assert(def->language == FUNC_LANGUAGE_LUA);
+	struct func_lua *func =
+		(struct func_lua *) malloc(sizeof(struct func_lua));
+	if (func == NULL) {
+		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
+		return NULL;
+	}
+	func->base.vtab = &func_lua_vtab;
+	return &func->base;
+}
+
+static void
+func_lua_destroy(struct func *func)
+{
+	assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
+	assert(func->vtab == &func_lua_vtab);
+	free(func);
+}
+
+static inline int
+func_lua_call(struct func *func, struct port *args, struct port *ret)
+{
+	assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
+	assert(func->vtab == &func_lua_vtab);
+	return box_lua_call(func->def->name, func->def->name_len, args, ret);
+}
+
+static struct func_vtab func_lua_vtab = {
+	.call = func_lua_call,
+	.destroy = func_lua_destroy,
+};
+
 static int
 lbox_module_reload(lua_State *L)
 {
diff --git a/src/box/lua/call.h b/src/box/lua/call.h
index d03bcd0f8..83aa43949 100644
--- a/src/box/lua/call.h
+++ b/src/box/lua/call.h
@@ -44,6 +44,7 @@ box_lua_call_init(struct lua_State *L);
 
 struct port;
 struct call_request;
+struct func_def;
 
 /**
  * Invoke a Lua stored procedure from the binary protocol
@@ -57,6 +58,10 @@ int
 box_lua_eval(const char *expr, uint32_t expr_len,
 	     struct port *args, struct port *ret);
 
+/** Construct a Lua function object. */
+struct func *
+func_lua_new(struct func_def *def);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/src/box/session.cc b/src/box/session.cc
index 4bb13d031..3a7c02401 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -297,7 +297,8 @@ access_check_session(struct user *user)
 }
 
 int
-access_check_universe(user_access_t access)
+access_check_universe_object(user_access_t access,
+		enum schema_object_type object_type, const char *object_name)
 {
 	struct credentials *credentials = effective_user();
 	access |= PRIV_U;
@@ -313,7 +314,7 @@ access_check_universe(user_access_t access)
 		if (user != NULL) {
 			diag_set(AccessDeniedError,
 				 priv_name(denied_access),
-				 schema_object_name(SC_UNIVERSE), "",
+				 schema_object_name(object_type), object_name,
 				 user->def->name);
 		} else {
 			/*
@@ -328,6 +329,12 @@ access_check_universe(user_access_t access)
 	return 0;
 }
 
+int
+access_check_universe(user_access_t access)
+{
+	return access_check_universe_object(access, SC_UNIVERSE, "");
+}
+
 int
 generic_session_push(struct session *session, uint64_t sync, struct port *port)
 {
diff --git a/src/box/session.h b/src/box/session.h
index 3a7397146..54a02536e 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -312,6 +312,15 @@ access_check_session(struct user *user);
 int
 access_check_universe(user_access_t access);
 
+/**
+ * Same as access_check_universe(), but in case the current user
+ * doesn't have universal access, set AccessDeniedError for the
+ * given object type and name.
+ */
+int
+access_check_universe_object(user_access_t access,
+		enum schema_object_type object_type, const char *object_name);
+
 static inline int
 session_push(struct session *session, uint64_t sync, struct port *port)
 {
-- 
2.21.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4 4/6] box: export registered functions in box.func folder
  2019-06-23 13:57 [PATCH v4 0/6] box: rework functions machinery Kirill Shcherbatov
                   ` (2 preceding siblings ...)
  2019-06-23 13:57 ` [PATCH v4 3/6] box: rework func object as a function frontend Kirill Shcherbatov
@ 2019-06-23 13:57 ` Kirill Shcherbatov
  2019-06-24 10:25   ` Vladimir Davydov
  2019-06-23 13:57 ` [PATCH v4 5/6] box: refactor box_lua_find helper Kirill Shcherbatov
  2019-06-23 13:57 ` [PATCH v4 6/6] box: introduce Lua persistent functions Kirill Shcherbatov
  5 siblings, 1 reply; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-06-23 13:57 UTC (permalink / raw)
  To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov

Needed for #4182, #1260

@TarantoolBot document
Title: Export registered functions to box.func folder

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 box.schema.func.drop interface.
The :call method is similar to net.box connection:call method
and allows to call a registered function directly.
All access checks are performed on each function call.

Example:
function sum(a, b) return a + b end
box.schema.func.create('sum')
box.func.sum
---
- language: LUA
  setuid: false
  name: sum
  id: 2
...
box.func.sum:call({1, 3})
---
- 4
...
box.func.sum:drop()
---
 src/box/alter.cc            |   2 +
 src/box/lua/call.c          | 247 ++++++++++++++++++++++++++++++++----
 src/box/lua/init.c          |   1 +
 src/box/lua/misc.cc         |   8 +-
 src/box/lua/schema.lua      |  29 +++++
 src/box/port.h              |   2 -
 src/box/schema.cc           |   1 +
 src/box/schema.h            |   5 +
 test/box/function1.c        |  33 +++++
 test/box/function1.result   | 215 +++++++++++++++++++++++++++++++
 test/box/function1.test.lua |  69 ++++++++++
 test/box/misc.result        |   1 +
 12 files changed, 581 insertions(+), 32 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 32c4b566a..33f9b0a71 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2583,6 +2583,7 @@ func_cache_remove_func(struct trigger *trigger, void * /* event */)
 {
 	struct func *old_func = (struct func *) trigger->data;
 	func_cache_delete(old_func->def->fid);
+	trigger_run_xc(&on_alter_func, old_func);
 	func_delete(old_func);
 }
 
@@ -2615,6 +2616,7 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
 		def_guard.is_active = false;
 		func_cache_insert(func);
 		on_rollback->data = func;
+		trigger_run_xc(&on_alter_func, func);
 		txn_on_rollback(txn, on_rollback);
 	} else if (new_tuple == NULL) {         /* DELETE */
 		uint32_t uid;
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 8b7223a7c..8c70c1088 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -33,10 +33,13 @@
 #include "box/error.h"
 #include "box/func.h"
 #include "box/func_def.h"
+#include "box/schema.h"
 #include "fiber.h"
+#include "tt_static.h"
 
 #include "lua/utils.h"
 #include "lua/msgpack.h"
+#include "lua/trigger.h"
 
 #include "box/port.h"
 #include "box/lua/tuple.h"
@@ -284,8 +287,13 @@ port_lua_create(struct port *port, struct lua_State *L)
 }
 
 struct execute_lua_ctx {
-	const char *name;
-	uint32_t name_len;
+	union {
+		struct {
+			const char *name;
+			uint32_t name_len;
+		};
+		struct mpstream *stream;
+	};
 	struct port *args;
 };
 
@@ -340,58 +348,56 @@ execute_lua_eval(lua_State *L)
 static int
 encode_lua_call(lua_State *L)
 {
-	struct port_lua *port = (struct port_lua *) lua_topointer(L, -1);
+	struct execute_lua_ctx *ctx =
+		(struct execute_lua_ctx *) 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;
+	struct port_lua *port = (struct port_lua *) ctx->args;
 	int size = lua_gettop(port->L);
 	for (int i = 1; i <= size; ++i)
-		luamp_encode(port->L, cfg, &stream, i);
+		luamp_encode(port->L, cfg, ctx->stream, i);
 	port->size = size;
-	mpstream_flush(&stream);
+	mpstream_flush(ctx->stream);
 	return 0;
 }
 
 static int
 encode_lua_call_16(lua_State *L)
 {
-	struct port_lua *port = (struct port_lua *) lua_topointer(L, -1);
+	struct execute_lua_ctx *ctx =
+		(struct execute_lua_ctx *) 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);
+	struct port_lua *port = (struct port_lua *) ctx->args;
+	port->size = luamp_encode_call_16(port->L, cfg, ctx->stream);
+	mpstream_flush(ctx->stream);
 	return 0;
 }
 
 static inline int
-port_lua_do_dump(struct port *base, struct obuf *out, lua_CFunction handler)
+port_lua_do_dump(struct port *base, struct mpstream *stream,
+		 lua_CFunction handler)
 {
-	struct port_lua *port = (struct port_lua *)base;
+	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 execute_lua_ctx ctx;
+	ctx.args = base;
+	ctx.stream = stream;
 	struct lua_State *L = tarantool_L;
 	int top = lua_gettop(L);
-	if (lua_cpcall(L, handler, port) != 0) {
+	if (lua_cpcall(L, handler, &ctx) != 0) {
 		luaT_toerror(port->L);
 		return -1;
 	}
@@ -402,13 +408,57 @@ port_lua_do_dump(struct port *base, struct obuf *out, lua_CFunction handler)
 static int
 port_lua_dump(struct port *base, struct obuf *out)
 {
-	return port_lua_do_dump(base, out, encode_lua_call);
+	struct port_lua *port = (struct port_lua *) base;
+	struct mpstream stream;
+	mpstream_init(&stream, out, 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 *out)
 {
-	return port_lua_do_dump(base, out, encode_lua_call_16);
+	struct port_lua *port = (struct port_lua *)base;
+	struct mpstream stream;
+	mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
+		      luamp_error, port->L);
+	return port_lua_do_dump(base, &stream, encode_lua_call_16);
+}
+
+static void
+port_lua_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
+{
+	(void) is_flat;
+	assert(is_flat == true);
+	struct port_lua *port = (struct port_lua *) base;
+	uint32_t size = lua_gettop(port->L);
+	lua_xmove(port->L, L, size);
+	port->size = size;
+}
+
+static const char *
+port_lua_get_msgpack(struct port *base, uint32_t *size)
+{
+	struct port_lua *port = (struct port_lua *) base;
+	struct region *region = &fiber()->gc;
+	uint32_t region_svp = region_used(region);
+	struct mpstream stream;
+	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
+		      luamp_error, port->L);
+	mpstream_encode_array(&stream, lua_gettop(port->L));
+	int rc = port_lua_do_dump(base, &stream, encode_lua_call);
+	if (rc < 0) {
+		region_truncate(region, region_svp);
+		return NULL;
+	}
+	*size = region_used(region) - region_svp;
+	const char *data = region_join(region, *size);
+	if (data == NULL) {
+		diag_set(OutOfMemory, *size, "region", "data");
+		region_truncate(region, region_svp);
+		return NULL;
+	}
+	return data;
 }
 
 static void
@@ -429,9 +479,9 @@ 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_lua = port_lua_dump_lua,
 	.dump_plain = port_lua_dump_plain,
-	.get_msgpack = NULL,
+	.get_msgpack = port_lua_get_msgpack,
 	.destroy = port_lua_destroy,
 };
 
@@ -527,10 +577,150 @@ lbox_module_reload(lua_State *L)
 	return 0;
 }
 
+int
+lbox_func_call(struct lua_State *L)
+{
+	if (lua_gettop(L) < 1 || !lua_isstring(L, 1))
+		return luaL_error(L, "Use func:call(...)");
+
+	size_t name_len;
+	const char *name = lua_tolstring(L, 1, &name_len);
+	struct func *func = func_by_name(name, name_len);
+	if (func == NULL) {
+		diag_set(ClientError, ER_NO_SUCH_FUNCTION,
+			 tt_cstr(name, name_len));
+		return luaT_error(L);
+	}
+
+	/*
+	 * Prepare a new Lua stack for input arguments
+	 * before the function call to pass it into the
+	 * pcall-sandboxed tarantool_L handler.
+	 */
+	lua_State *args_L = lua_newthread(tarantool_L);
+	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+	lua_xmove(L, args_L, lua_gettop(L) - 1);
+	struct port args;
+	port_lua_create(&args, args_L);
+	((struct port_lua *) &args)->ref = coro_ref;
+
+	struct port ret;
+	if (func_call(func, &args, &ret) != 0) {
+		port_destroy(&args);
+		return luaT_error(L);
+	}
+
+	int top = lua_gettop(L);
+	port_dump_lua(&ret, L, true);
+	int cnt = lua_gettop(L) - top;
+
+	port_destroy(&ret);
+	port_destroy(&args);
+	return cnt;
+}
+
+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 func 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, "setuid");
+	lua_pushboolean(L, func->def->setuid);
+	lua_settable(L, top);
+	lua_pushstring(L, "language");
+	lua_pushstring(L, func_language_strs[func->def->language]);
+	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, struct func *func)
+{
+	uint32_t fid = func->def->fid;
+	lua_getfield(L, LUA_GLOBALSINDEX, "box");
+	lua_getfield(L, -1, "func");
+	assert(!lua_isnil(L, -1));
+	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;
+	struct func *func = (struct func *)event;
+	if (func != NULL)
+		lbox_func_new(L, func);
+	else
+		lbox_func_delete(L, func);
+}
+
+static struct trigger on_alter_func_in_lua = {
+	RLIST_LINK_INITIALIZER, lbox_func_new_or_delete, NULL, NULL
+};
+
 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}
 };
 
@@ -539,7 +729,12 @@ box_lua_call_init(struct lua_State *L)
 {
 	luaL_register(L, "box.internal", boxlib_internal);
 	lua_pop(L, 1);
-
+	/*
+	 * 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);
 #if 0
 	/* Get CTypeID for `struct port *' */
 	int rc = luaL_cdef(L, "struct port;");
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 76b987b4b..7ffed409d 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"
diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index de9ed407b..54f1d7a58 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -67,14 +67,14 @@ lbox_encode_tuple_on_gc(lua_State *L, int idx, size_t *p_len)
 extern "C" void
 port_tuple_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
 {
-	(void) is_flat;
-	assert(is_flat == false);
 	struct port_tuple *port = port_tuple(base);
-	lua_createtable(L, port->size, 0);
+	if (!is_flat)
+		lua_createtable(L, port->size, 0);
 	struct port_tuple_entry *pe = port->first;
 	for (int i = 0; pe != NULL; pe = pe->next) {
 		luaT_pushtuple(L, pe->tuple);
-		lua_rawseti(L, -2, ++i);
+		if (!is_flat)
+			lua_rawseti(L, -2, ++i);
 	}
 }
 
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 3f080eced..9c3ee063c 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2190,7 +2190,36 @@ 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_mt = {}
+
+func_mt.drop = function(func, opts)
+    check_func_arg(func, 'drop')
+    box.schema.func.drop(func.name, opts)
+end
+
+func_mt.call = function(func, args)
+    check_func_arg(func, 'call')
+    args = args or {}
+    if type(args) ~= 'table' then
+        error('Use func:call(table)')
+    end
+    return box.schema.func.call(func.name, unpack(args))
+end
+
+function box.schema.func.bless(func)
+    setmetatable(func, {__index = func_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/port.h b/src/box/port.h
index db93f8eea..a7f5d81bd 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -102,8 +102,6 @@ struct port_lua {
 	struct lua_State *L;
 	/** Reference to L in tarantool_L. */
 	int ref;
-	/** The argument of port_dump */
-	struct obuf *out;
 	/** Number of entries dumped to the port. */
 	int size;
 };
diff --git a/src/box/schema.cc b/src/box/schema.cc
index d63add535..bf0ed33b7 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 7e8ac6c11..f0039b29d 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -237,6 +237,11 @@ extern struct rlist on_alter_sequence;
  */
 extern struct rlist on_access_denied;
 
+/**
+ * Triggers fired after committing a change in _func space.
+ */
+extern struct rlist on_alter_func;
+
 /**
  * Context passed to on_access_denied trigger.
  */
diff --git a/test/box/function1.c b/test/box/function1.c
index 053e4fe91..ee5a422b5 100644
--- a/test/box/function1.c
+++ b/test/box/function1.c
@@ -42,6 +42,39 @@ args(box_function_ctx_t *ctx, const char *args, const char *args_end)
 	return box_return_tuple(ctx, tuple);
 }
 
+int
+divide(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	uint32_t arg_count = mp_decode_array(&args);
+	if (arg_count < 2)
+		goto error;
+
+	if (mp_typeof(*args) != MP_UINT)
+		goto error;
+	double a = mp_decode_uint(&args);
+	if (mp_typeof(*args) != MP_UINT)
+		goto error;
+	double b = mp_decode_uint(&args);
+	if (b == 0)
+		goto error;
+
+	char tuple_buf[512];
+	char *d = tuple_buf;
+	d = mp_encode_array(d, 1);
+	d = mp_encode_double(d, a / b);
+	assert(d <= tuple_buf + sizeof(tuple_buf));
+
+	box_tuple_format_t *fmt = box_tuple_format_default();
+	box_tuple_t *tuple = box_tuple_new(fmt, tuple_buf, d);
+	if (tuple == NULL)
+		return -1;
+	return box_return_tuple(ctx, tuple);
+error:
+	return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+			     "invalid argument");
+}
+
+
 /*
  * For each UINT key in arguments create or increment counter in
  * box.space.test space.
diff --git a/test/box/function1.result b/test/box/function1.result
index cadeb0467..99006926e 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -57,6 +57,25 @@ c:call('function1.args', { 15 })
 ---
 - [[15, 'hello']]
 ...
+box.func["function1.args"]
+---
+- language: C
+  setuid: false
+  name: function1.args
+  id: 2
+...
+box.func["function1.args"]:call()
+---
+- error: invalid argument count
+...
+box.func["function1.args"]:call({ "xx" })
+---
+- error: first tuple field must be uint
+...
+box.func["function1.args"]:call({ 15 })
+---
+- [15, 'hello']
+...
 box.schema.func.drop("function1.args")
 ---
 ...
@@ -260,6 +279,10 @@ s:drop()
 test_run = require('test_run').new()
 ---
 ...
+test_run:cmd("push filter '(.builtin/.*.lua):[0-9]+' to '\\1'")
+---
+- true
+...
 identifier = require("identifier")
 ---
 ...
@@ -299,3 +322,195 @@ test_run:cmd("setopt delimiter ''");
 c:close()
 ---
 ...
+-- Test registered functions interface.
+function divide(a, b) return a / b end
+---
+...
+box.schema.func.create("divide")
+---
+...
+func = box.func.divide
+---
+...
+func.call({4, 2})
+---
+- error: 'builtin/box/schema.lua: Use func:call(...) instead of func.call(...)'
+...
+func:call(4, 2)
+---
+- error: 'builtin/box/schema.lua: Use func:call(table)'
+...
+func:call()
+---
+- error: '[string "function divide(a, b) return a / b end "]:1: attempt to perform
+    arithmetic on local ''a'' (a nil value)'
+...
+func:call({})
+---
+- error: '[string "function divide(a, b) return a / b end "]:1: attempt to perform
+    arithmetic on local ''a'' (a nil value)'
+...
+func:call({4})
+---
+- error: '[string "function divide(a, b) return a / b end "]:1: attempt to perform
+    arithmetic on local ''b'' (a nil value)'
+...
+func:call({4, 2})
+---
+- 2
+...
+func:call({4, 2, 1})
+---
+- 2
+...
+func:drop()
+---
+...
+func
+---
+- language: LUA
+  setuid: false
+  name: divide
+  id: 2
+...
+func.drop()
+---
+- error: 'builtin/box/schema.lua: Use func:drop(...) instead of func.drop(...)'
+...
+func:drop()
+---
+- error: Function 'divide' does not exist
+...
+func:call({4, 2})
+---
+- error: Function 'divide' does not exist
+...
+box.internal.func_call('divide', 4, 2)
+---
+- error: Function 'divide' does not exist
+...
+box.schema.func.create("function1.divide", {language = 'C'})
+---
+...
+func = box.func["function1.divide"]
+---
+...
+func:call(4, 2)
+---
+- error: 'builtin/box/schema.lua: Use func:call(table)'
+...
+func:call()
+---
+- error: invalid argument
+...
+func:call({})
+---
+- error: invalid argument
+...
+func:call({4})
+---
+- error: invalid argument
+...
+func:call({4, 2})
+---
+- [2]
+...
+func:call({4, 2, 1})
+---
+- [2]
+...
+func:drop()
+---
+...
+func
+---
+- language: C
+  setuid: false
+  name: function1.divide
+  id: 2
+...
+func:drop()
+---
+- error: Function 'function1.divide' does not exist
+...
+func:call({4, 2})
+---
+- error: Function 'function1.divide' does not exist
+...
+box.internal.func_call('function1.divide', 4, 2)
+---
+- error: Function 'function1.divide' does not exist
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+function minmax(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")
+---
+...
+func = box.func.minmax
+---
+...
+func:call({{1, 2, 99, 3, -1}})
+---
+- -1
+- 99
+...
+func:drop()
+---
+...
+-- Test access checks for registered functions.
+function secret() return 1 end
+---
+...
+box.schema.func.create("secret")
+---
+...
+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')
+---
+...
+test_run:cmd("clear filter")
+---
+- true
+...
diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
index e983495b6..25966b915 100644
--- a/test/box/function1.test.lua
+++ b/test/box/function1.test.lua
@@ -21,6 +21,10 @@ box.schema.user.grant('guest', 'execute', 'function', 'function1.args')
 c:call('function1.args')
 c:call('function1.args', { "xx" })
 c:call('function1.args', { 15 })
+box.func["function1.args"]
+box.func["function1.args"]:call()
+box.func["function1.args"]:call({ "xx" })
+box.func["function1.args"]:call({ 15 })
 box.schema.func.drop("function1.args")
 
 box.schema.func.create('function1.multi_inc', {language = "C"})
@@ -86,6 +90,7 @@ s:drop()
 
 -- gh-2914: check identifier constraints.
 test_run = require('test_run').new()
+test_run:cmd("push filter '(.builtin/.*.lua):[0-9]+' to '\\1'")
 identifier = require("identifier")
 test_run:cmd("setopt delimiter ';'")
 --
@@ -112,3 +117,67 @@ identifier.run_test(
 );
 test_run:cmd("setopt delimiter ''");
 c:close()
+
+-- Test registered functions interface.
+function divide(a, b) return a / b end
+box.schema.func.create("divide")
+func = box.func.divide
+func.call({4, 2})
+func:call(4, 2)
+func:call()
+func:call({})
+func:call({4})
+func:call({4, 2})
+func:call({4, 2, 1})
+func:drop()
+func
+func.drop()
+func:drop()
+func:call({4, 2})
+box.internal.func_call('divide', 4, 2)
+
+box.schema.func.create("function1.divide", {language = 'C'})
+func = box.func["function1.divide"]
+func:call(4, 2)
+func:call()
+func:call({})
+func:call({4})
+func:call({4, 2})
+func:call({4, 2, 1})
+func:drop()
+func
+func:drop()
+func:call({4, 2})
+box.internal.func_call('function1.divide', 4, 2)
+
+test_run:cmd("setopt delimiter ';'")
+function minmax(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")
+func = box.func.minmax
+func:call({{1, 2, 99, 3, -1}})
+func:drop()
+
+-- Test access checks for registered functions.
+function secret() return 1 end
+box.schema.func.create("secret")
+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')
+
+test_run:cmd("clear filter")
diff --git a/test/box/misc.result b/test/box/misc.result
index dfa06724c..ec2c4fa95 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -65,6 +65,7 @@ t
   - error
   - execute
   - feedback
+  - func
   - index
   - info
   - internal
-- 
2.21.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4 5/6] box: refactor box_lua_find helper
  2019-06-23 13:57 [PATCH v4 0/6] box: rework functions machinery Kirill Shcherbatov
                   ` (3 preceding siblings ...)
  2019-06-23 13:57 ` [PATCH v4 4/6] box: export registered functions in box.func folder Kirill Shcherbatov
@ 2019-06-23 13:57 ` Kirill Shcherbatov
  2019-06-24 12:35   ` Vladimir Davydov
  2019-06-23 13:57 ` [PATCH v4 6/6] box: introduce Lua persistent functions Kirill Shcherbatov
  5 siblings, 1 reply; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-06-23 13:57 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.

Needed for #4182, #1260
---
 src/box/lua/call.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 8c70c1088..f98ab42ac 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -56,7 +56,7 @@ static int
 box_lua_find(lua_State *L, const char *name, const char *name_end)
 {
 	int index = LUA_GLOBALSINDEX;
-	int objstack = 0;
+	int objstack = 0, top = lua_gettop(L);
 	const char *start = name, *end;
 
 	while ((end = (const char *) memchr(start, '.', name_end - start))) {
@@ -66,7 +66,7 @@ box_lua_find(lua_State *L, const char *name, const char *name_end)
 		if (! lua_istable(L, -1)) {
 			diag_set(ClientError, ER_NO_SUCH_PROC,
 				 name_end - name, name);
-			luaT_error(L);
+			return -1;
 		}
 		start = end + 1; /* next piece of a.b.c */
 		index = lua_gettop(L); /* top of the stack */
@@ -81,11 +81,12 @@ box_lua_find(lua_State *L, const char *name, const char *name_end)
 			lua_islightuserdata(L, -1) || lua_isuserdata(L, -1) )) {
 				diag_set(ClientError, ER_NO_SUCH_PROC,
 					  name_end - name, name);
-				luaT_error(L);
+				return -1;
 		}
+
 		start = end + 1; /* next piece of a.b.c */
 		index = lua_gettop(L); /* top of the stack */
-		objstack = index;
+		objstack = index - top;
 	}
 
 
@@ -96,21 +97,24 @@ box_lua_find(lua_State *L, const char *name, const char *name_end)
 		 * for us, but our own message is more verbose. */
 		diag_set(ClientError, ER_NO_SUCH_PROC,
 			  name_end - name, name);
-		luaT_error(L);
+		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, 1);
+			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, 1);
-			lua_insert(L, 2);
+			lua_insert(L, top + 1);
+			lua_insert(L, top + 2);
+			lua_pop(L, objstack - 1);
 			objstack = 1;
 		}
-		lua_settop(L, 1 + objstack);
 	}
 	return 1 + objstack;
 }
@@ -128,7 +132,10 @@ 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 = box_lua_find(L, name, name + name_len);
+	if (count < 0)
+		return luaT_error(L);
+	return count;
 }
 
 /*
@@ -307,9 +314,10 @@ execute_lua_call(lua_State *L)
 	const char *name = ctx->name;
 	uint32_t name_len = ctx->name_len;
 
-	int oc = 0; /* how many objects are on stack after box_lua_find */
-	/* Try to find a function by name in Lua */
-	oc = box_lua_find(L, name, name + name_len);
+	/* How many objects are on stack after box_lua_find. */
+	int oc = box_lua_find(L, name, name + name_len);
+	if (oc < 0)
+		return luaT_error(L);
 
 	/* Push the rest of args (a tuple). */
 	int top = lua_gettop(L);
-- 
2.21.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4 6/6] box: introduce Lua persistent functions
  2019-06-23 13:57 [PATCH v4 0/6] box: rework functions machinery Kirill Shcherbatov
                   ` (4 preceding siblings ...)
  2019-06-23 13:57 ` [PATCH v4 5/6] box: refactor box_lua_find helper Kirill Shcherbatov
@ 2019-06-23 13:57 ` Kirill Shcherbatov
  2019-06-24 12:38   ` Vladimir Davydov
  5 siblings, 1 reply; 14+ messages in thread
From: Kirill Shcherbatov @ 2019-06-23 13:57 UTC (permalink / raw)
  To: tarantool-patches, vdavydov.dev; +Cc: Kirill Shcherbatov

Closes #4182
Needed for #1260

@TarantoolBot document
Title: Persistent Lua functions

Now Tarantool supports 'persistent' Lua functions.
Such functions are stored in snapshot and are available after
restart.
To create a persistent Lua function, specify a function body
in box.schema.func.create call:
e.g. body = "function(a, b) return a + b end"

A Lua persistent function may be 'sandboxed'. The 'sandboxed'
function is 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

Finally, the new 'is_deterministic' flag allows to mark a
registered function as deterministic, i.e. the function that
can produce only one result for a given list of parameters.

The new box.schema.func.create interface is:
box.schema.func.create('funcname', <setuid = true|FALSE>,
	<if_not_exists = true|FALSE>, <language = LUA|c>,
	<body = string ('')>, <is_deterministic = true|FALSE>,
	<is_sandboxed = true|FALSE>, <comment = string ('')>)

Example:
lua_code = [[function(a, b) return a + b end]]
box.schema.func.create('sum', {body = lua_code,
		is_deterministic = true, is_sandboxed = true})
box.func.sum
---
- is_sandboxed: true
  is_deterministic: true
  id: 2
  setuid: false
  body: function(a, b) return a + b end
  name: sum
  language: LUA
...
box.func.sum:call({1, 3})
---
- 4
...
---
 src/box/alter.cc             |  88 +++++++++++---
 src/box/bootstrap.snap       | Bin 4475 -> 4650 bytes
 src/box/func.c               |   7 +-
 src/box/func_def.c           |   8 ++
 src/box/func_def.h           |  30 ++++-
 src/box/lua/call.c           | 221 ++++++++++++++++++++++++++++++++++-
 src/box/lua/schema.lua       |  18 ++-
 src/box/lua/upgrade.lua      |  34 +++++-
 src/box/schema_def.h         |  11 ++
 test/box-py/bootstrap.result |   6 +-
 test/box/access_misc.result  |   6 +-
 test/box/function1.result    | 210 +++++++++++++++++++++++++++++++--
 test/box/function1.test.lua  |  73 +++++++++++-
 13 files changed, 675 insertions(+), 37 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 33f9b0a71..1da3a6172 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2537,31 +2537,83 @@ 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, comment_len;
+	const char *name, *body, *comment;
+	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);
+		comment = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_COMMENT,
+					     &comment_len);
+		uint32_t len;
+		const char *routine_type = tuple_field_str_xc(tuple,
+					BOX_FUNC_FIELD_ROUTINE_TYPE, &len);
+		if (len != strlen("function") ||
+		    strncasecmp(routine_type, "function", len) != 0) {
+			tnt_raise(ClientError, ER_CREATE_FUNCTION, name,
+				  "unsupported routine_type value");
+		}
+		const char *sql_data_access = tuple_field_str_xc(tuple,
+					BOX_FUNC_FIELD_SQL_DATA_ACCESS, &len);
+		if (len != strlen("none") ||
+		    strncasecmp(sql_data_access, "none", len) != 0) {
+			tnt_raise(ClientError, ER_CREATE_FUNCTION, name,
+				  "unsupported sql_data_access value");
+		}
+		bool is_null_call = tuple_field_bool_xc(tuple,
+						BOX_FUNC_FIELD_IS_NULL_CALL);
+		if (is_null_call != true) {
+			tnt_raise(ClientError, ER_CREATE_FUNCTION, name,
+				  "unsupported is_null_call value");
+		}
+	} else {
+		body = NULL;
+		body_len = 0;
+		comment = NULL;
+		comment_len = 0;
+	}
+	uint32_t body_offset, comment_offset;
+	uint32_t def_sz = func_def_sizeof(name_len, body_len, comment_len,
+					  &body_offset, &comment_offset);
+	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);
 	if (def->fid > BOX_FUNCTION_MAX) {
 		tnt_raise(ClientError, ER_CREATE_FUNCTION,
-			  tt_cstr(name, len), "function id is too big");
+			  tt_cstr(name, name_len), "function id is too big");
 	}
-	memcpy(def->name, name, len);
-	def->name[len] = 0;
-	def->name_len = len;
-	if (tuple_field_count(tuple) > BOX_FUNC_FIELD_SETUID)
+	memcpy(def->name, name, name_len);
+	def->name[name_len] = 0;
+	def->name_len = name_len;
+	if (body_len > 0) {
+		def->body = (char *)def + body_offset;
+		memcpy(def->body, body, body_len);
+		def->body[body_len] = 0;
+	} else {
+		def->body = NULL;
+	}
+	if (comment_len > 0) {
+		def->comment = (char *)def + comment_offset;
+		memcpy(def->comment, comment, comment_len);
+		def->comment[comment_len] = 0;
+	} else {
+		def->comment = 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);
@@ -2573,6 +2625,16 @@ func_def_new_from_tuple(struct tuple *tuple)
 		/* Lua is the default. */
 		def->language = FUNC_LANGUAGE_LUA;
 	}
+	if (field_count > BOX_FUNC_FIELD_BODY) {
+		def->is_deterministic =
+			tuple_field_bool_xc(tuple,
+					    BOX_FUNC_FIELD_IS_DETERMINISTIC);
+		def->is_sandboxed =
+			tuple_field_bool_xc(tuple,
+					    BOX_FUNC_FIELD_IS_SANDBOXED);
+	} else {
+		def->is_deterministic = false;
+	}
 	def_guard.is_active = false;
 	return def;
 }
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 56943ef7e7d0fe0e3dbb5d346544e2c78c3dc154..786403e4aeaa768fe6565c9a6669411feb3e880b 100644
GIT binary patch
literal 4650

diff --git a/src/box/func.c b/src/box/func.c
index c57027809..d7c35cf68 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -416,8 +416,13 @@ static struct func_vtab func_c_vtab;
 static struct func *
 func_c_new(struct func_def *def)
 {
-	(void) def;
 	assert(def->language == FUNC_LANGUAGE_C);
+	if (def->body != NULL || def->is_sandboxed) {
+		diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
+			 "body and is_sandboxed options are not compatible "
+			 "with C language");
+		return NULL;
+	}
 	struct func_c *func = (struct func_c *) malloc(sizeof(struct func_c));
 	if (func == NULL) {
 		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
diff --git a/src/box/func_def.c b/src/box/func_def.c
index 2b135e2d7..73e493786 100644
--- a/src/box/func_def.c
+++ b/src/box/func_def.c
@@ -14,7 +14,15 @@ func_def_cmp(struct func_def *def1, struct func_def *def2)
 		return def1->setuid - def2->setuid;
 	if (def1->language != def2->language)
 		return def1->language - def2->language;
+	if (def1->is_deterministic != def2->is_deterministic)
+		return def1->is_deterministic - def2->is_deterministic;
+	if (def1->is_sandboxed != def2->is_sandboxed)
+		return def1->is_sandboxed - def2->is_sandboxed;
 	if (strcmp(def1->name, def2->name) != 0)
 		return strcmp(def1->name, def2->name);
+	if ((def1->body != NULL) != (def2->body != NULL))
+		return def1->body - def2->body;
+	if (def1->body != NULL && strcmp(def1->body, def2->body) != 0)
+		return strcmp(def1->body, def2->body);
 	return 0;
 }
diff --git a/src/box/func_def.h b/src/box/func_def.h
index 866d425a1..170aef536 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -58,11 +58,26 @@ struct func_def {
 	uint32_t fid;
 	/** Owner of the function. */
 	uint32_t uid;
+	/** Definition of the persistent function. */
+	char *body;
+	/** User-defined comment for a function. */
+	char *comment;
 	/**
 	 * True if the function requires change of user id before
 	 * invocation.
 	 */
 	bool setuid;
+	/**
+	 * Whether this function is deterministic (can produce
+	 * only one result for a given list of parameters).
+	 */
+	bool is_deterministic;
+	/**
+	 * Whether the routine must be initialized with isolated
+	 * sandbox where only a limited number if functions is
+	 * available.
+	 */
+	bool is_sandboxed;
 	/**
 	 * The language of the stored function.
 	 */
@@ -76,13 +91,22 @@ struct func_def {
 /**
  * @param name_len length of func_def->name
  * @returns size in bytes needed to allocate for struct func_def
- * for a function of length @a a name_len.
+ * for a function of length @a a name_len, body @a body_len and
+ * with comment @a comment_len.
  */
 static inline size_t
-func_def_sizeof(uint32_t name_len)
+func_def_sizeof(uint32_t name_len, uint32_t body_len, uint32_t comment_len,
+		uint32_t *body_offset, uint32_t *comment_offset)
 {
 	/* +1 for '\0' name terminating. */
-	return sizeof(struct func_def) + name_len + 1;
+	size_t sz = sizeof(struct func_def) + name_len + 1;
+	*body_offset = sz;
+	if (body_len > 0)
+		sz += body_len + 1;
+	*comment_offset = sz;
+	if (comment_len > 0)
+		sz += comment_len + 1;
+	return sz;
 }
 
 /** Compare two given function definitions. */
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index f98ab42ac..d3d874477 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -300,6 +300,7 @@ struct execute_lua_ctx {
 			uint32_t name_len;
 		};
 		struct mpstream *stream;
+		int lua_ref;
 	};
 	struct port *args;
 };
@@ -328,6 +329,24 @@ execute_lua_call(lua_State *L)
 	return lua_gettop(L);
 }
 
+static int
+execute_lua_call_by_ref(lua_State *L)
+{
+	struct execute_lua_ctx *ctx =
+		(struct execute_lua_ctx *) lua_topointer(L, 1);
+	lua_settop(L, 0); /* clear the stack to simplify the logic below */
+
+	lua_rawgeti(L, LUA_REGISTRYINDEX, ctx->lua_ref);
+
+	/* Push the rest of args (a tuple). */
+	int top = lua_gettop(L);
+	port_dump_lua(ctx->args, L, true);
+	int arg_count = lua_gettop(L) - top;
+
+	lua_call(L, arg_count, LUA_MULTRET);
+	return lua_gettop(L);
+}
+
 static int
 execute_lua_eval(lua_State *L)
 {
@@ -536,22 +555,168 @@ box_lua_eval(const char *expr, uint32_t expr_len,
 struct func_lua {
 	/** Function object base class. */
 	struct func base;
+	/**
+	 * For a persistent function: a reference to the
+	 * function body. Otherwise LUA_REFNIL.
+	 */
+	int lua_ref;
 };
 
 static struct func_vtab func_lua_vtab;
+static struct func_vtab func_persistent_lua_vtab;
+
+static const char *default_sandbox_exports[] = {
+	"assert", "error", "ipairs", "math", "next", "pairs", "pcall", "print",
+	"select", "string", "table", "tonumber", "tostring", "type", "unpack",
+	"xpcall", "utf8",
+};
+
+/**
+ * Assemble a new sandbox with given exports table on the top of
+ * a given Lua stack. All modules in exports list are copying
+ * deeply to ensure the immutability of this system object.
+ */
+static int
+prepare_lua_sandbox(struct lua_State *L, const char *exports[],
+		    int export_count)
+{
+	lua_createtable(L, export_count, 0);
+	if (export_count == 0)
+		return 0;
+	int rc = -1;
+	const char *deepcopy = "table.deepcopy";
+	int luaL_deepcopy_func_ref = LUA_REFNIL;
+	int ret = box_lua_find(L, deepcopy, deepcopy + strlen(deepcopy));
+	if (ret < 0)
+		goto end;
+	luaL_deepcopy_func_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+	assert(luaL_deepcopy_func_ref != LUA_REFNIL);
+	for (int i = 0; i < export_count; i++) {
+		uint32_t name_len = strlen(exports[i]);
+		ret = box_lua_find(L, exports[i], exports[i] + name_len);
+		if (ret < 0)
+			goto end;
+		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, 1);
+			break;
+		case LUA_TFUNCTION:
+			break;
+		default:
+			unreachable();
+		}
+		lua_setfield(L, -2, exports[i]);
+	}
+	rc = 0;
+end:
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, luaL_deepcopy_func_ref);
+	return rc;
+}
+
+/**
+ * Assemble a Lua function object by user-defined function body.
+ */
+static int
+func_persistent_lua_load(struct func_lua *func)
+{
+	int rc = -1;
+	int top = lua_gettop(tarantool_L);
+	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->base.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");
+		return -1;
+	}
+	sprintf(load_str, "%s%s", load_pref, func->base.def->body);
+
+	/*
+	 * Perform loading of the persistent Lua function
+	 * in a new sandboxed Lua thread. The sandbox is
+	 * required to guarantee the safety of executing
+	 * an arbitrary user-defined code
+	 * (e.g. body = 'fiber.yield()').
+	 */
+	struct lua_State *coro_L = lua_newthread(tarantool_L);
+	if (!func->base.def->is_sandboxed) {
+		/*
+		 * Keep an original env to apply for non-sandboxed
+		 * persistent function. It is required because
+		 * built object inherits parent env.
+		 */
+		lua_getfenv(tarantool_L, -1);
+		lua_insert(tarantool_L, -2);
+	}
+	if (prepare_lua_sandbox(tarantool_L, NULL, 0) != 0)
+		unreachable();
+	lua_setfenv(tarantool_L, -2);
+	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+	if (luaL_loadstring(coro_L, load_str) != 0 ||
+	    lua_pcall(coro_L, 0, 1, 0) != 0) {
+		diag_set(ClientError, ER_LOAD_FUNCTION, func->base.def->name,
+			 luaT_tolstring(coro_L, -1, NULL));
+		goto end;
+	}
+	if (!lua_isfunction(coro_L, -1)) {
+		diag_set(ClientError, ER_LOAD_FUNCTION, func->base.def->name,
+			 "given body doesn't define a function");
+		goto end;
+	}
+	lua_xmove(coro_L, tarantool_L, 1);
+	if (func->base.def->is_sandboxed) {
+		if (prepare_lua_sandbox(tarantool_L, default_sandbox_exports,
+					nelem(default_sandbox_exports)) != 0) {
+			diag_set(ClientError, ER_LOAD_FUNCTION,
+				func->base.def->name,
+				diag_last_error(diag_get())->errmsg);
+			goto end;
+		}
+	} else {
+		lua_insert(tarantool_L, -2);
+	}
+	lua_setfenv(tarantool_L, -2);
+	func->lua_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+	rc = 0;
+end:
+	lua_settop(tarantool_L, top);
+	region_truncate(region, region_svp);
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
+	return rc;
+}
 
 struct func *
 func_lua_new(struct func_def *def)
 {
-	(void) def;
 	assert(def->language == FUNC_LANGUAGE_LUA);
+	if (def->is_sandboxed && def->body == NULL) {
+		diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
+			 "is_sandboxed option may be set only for persistent "
+			 "Lua function (when body option is set)");
+		return NULL;
+	}
 	struct func_lua *func =
 		(struct func_lua *) malloc(sizeof(struct func_lua));
 	if (func == NULL) {
 		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
 		return NULL;
 	}
-	func->base.vtab = &func_lua_vtab;
+	if (def->body != NULL) {
+		func->base.def = def;
+		func->base.vtab = &func_persistent_lua_vtab;
+		if (func_persistent_lua_load(func) != 0) {
+			free(func);
+			return NULL;
+		}
+	} else {
+		func->lua_ref = LUA_REFNIL;
+		func->base.vtab = &func_lua_vtab;
+	}
 	return &func->base;
 }
 
@@ -576,6 +741,42 @@ static struct func_vtab func_lua_vtab = {
 	.destroy = func_lua_destroy,
 };
 
+static void
+func_persistent_lua_unload(struct func_lua *func)
+{
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, func->lua_ref);
+}
+
+static void
+func_persistent_lua_destroy(struct func *base)
+{
+	assert(base != NULL && base->def->language == FUNC_LANGUAGE_LUA &&
+	       base->def->body != NULL);
+	assert(base->vtab == &func_persistent_lua_vtab);
+	struct func_lua *func = (struct func_lua *) base;
+	func_persistent_lua_unload(func);
+	free(func);
+}
+
+static inline int
+func_persistent_lua_call(struct func *base, struct port *args, struct port *ret)
+{
+	assert(base != NULL && base->def->language == FUNC_LANGUAGE_LUA &&
+	       base->def->body != NULL);
+	assert(base->vtab == &func_persistent_lua_vtab);
+	struct func_lua *func = (struct func_lua *)base;
+	struct execute_lua_ctx ctx;
+	ctx.lua_ref = func->lua_ref;
+	ctx.args = args;
+	return box_process_lua(execute_lua_call_by_ref, &ctx, ret);
+
+}
+
+static struct func_vtab func_persistent_lua_vtab = {
+	.call = func_persistent_lua_call,
+	.destroy = func_persistent_lua_destroy,
+};
+
 static int
 lbox_module_reload(lua_State *L)
 {
@@ -669,6 +870,22 @@ lbox_func_new(struct lua_State *L, struct func *func)
 	lua_pushstring(L, "language");
 	lua_pushstring(L, func_language_strs[func->def->language]);
 	lua_settable(L, top);
+	lua_pushstring(L, "is_deterministic");
+	lua_pushboolean(L, func->def->is_deterministic);
+	lua_settable(L, top);
+	if (func->def->body != NULL) {
+		lua_pushstring(L, "body");
+		lua_pushstring(L, func->def->body);
+		lua_settable(L, top);
+		lua_pushstring(L, "is_sandboxed");
+		lua_pushboolean(L, func->def->is_sandboxed);
+		lua_settable(L, top);
+	}
+	if (func->def->comment != NULL) {
+		lua_pushstring(L, "comment");
+		lua_pushstring(L, func->def->comment);
+		lua_settable(L, top);
+	}
 
 	/* Bless func object. */
 	lua_getfield(L, LUA_GLOBALSINDEX, "box");
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 9c3ee063c..455d1e18a 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2138,7 +2138,10 @@ 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',
+                              is_deterministic = 'boolean',
+                              is_sandboxed = 'boolean', opts = 'table',
+                              comment = 'string'})
     local _func = box.space[box.schema.FUNC_ID]
     local _vfunc = box.space[box.schema.VFUNC_ID]
     local func = _vfunc.index.name:get{name}
@@ -2148,10 +2151,19 @@ box.schema.func.create = function(name, opts)
         end
         return
     end
-    opts = update_param_table(opts, { setuid = false, language = 'lua'})
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    opts = update_param_table(opts, { setuid = false, language = 'lua',
+                    body = '', routine_type = 'function', data_type = setmap{},
+                    sql_data_access = 'none', is_deterministic = false,
+                    is_sandboxed = false, is_null_call = true, opts = setmap{},
+                    comment = '', created = datetime, last_altered = datetime})
     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.routine_type, opts.data_type,
+                         opts.sql_data_access, opts.is_deterministic,
+                         opts.is_sandboxed, opts.is_null_call, opts.opts,
+                         opts.comment, opts.created, opts.last_altered}
 end
 
 box.schema.func.drop = function(name, opts)
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 3385b8e17..36f755585 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -326,7 +326,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',
+                  '', false, false, MAP}
 
     -- grant 'public' role access to 'box.schema.user.info' function
     log.info('grant execute on function "box.schema.user.info" to public')
@@ -820,10 +821,41 @@ local function create_vcollation_space()
     box.space[box.schema.VCOLLATION_ID]: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='routine_type', type='string'}
+    format[8] = {name='data_type', type='map'}
+    format[9] = {name='sql_data_access', type='string'}
+    format[10] = {name='is_deterministic', type='boolean'}
+    format[11] = {name='is_sandboxed', type='boolean'}
+    format[12] = {name='is_null_call', type='boolean'}
+    format[13] = {name='opts', type='map'}
+    format[14] = {name='comment', type='string'}
+    format[15] = {name='created', type='string'}
+    format[16] = {name='last_altered', type='string'}
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    for _, v in box.space._func:pairs() do
+        _ = box.space._func:replace({v.id, v.owner, v.name, v.setuid,
+                                     v[5] or 'LUA', '', 'function', setmap({}),
+                                     'none', false, false, true, setmap({}),
+                                     '', datetime, datetime})
+    end
+    _func:format(format)
+end
+
 local function upgrade_to_2_2_1()
     upgrade_sequence_to_2_2_1()
     upgrade_ck_constraint_to_2_2_1()
     create_vcollation_space()
+    upgrade_func_to_2_2_1()
 end
 
 --------------------------------------------------------------------------------
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index 88b5502b8..8543a3a77 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -167,6 +167,17 @@ enum {
 	BOX_FUNC_FIELD_NAME = 2,
 	BOX_FUNC_FIELD_SETUID = 3,
 	BOX_FUNC_FIELD_LANGUAGE = 4,
+	BOX_FUNC_FIELD_BODY = 5,
+	BOX_FUNC_FIELD_ROUTINE_TYPE = 6,
+	BOX_FUNC_FIELD_DATA_TYPE = 7,
+	BOX_FUNC_FIELD_SQL_DATA_ACCESS = 8,
+	BOX_FUNC_FIELD_IS_DETERMINISTIC = 9,
+	BOX_FUNC_FIELD_IS_SANDBOXED = 10,
+	BOX_FUNC_FIELD_IS_NULL_CALL = 11,
+	BOX_FUNC_FIELD_OPTS = 12,
+	BOX_FUNC_FIELD_COMMENT = 13,
+	BOX_FUNC_FIELD_CREATED = 14,
+	BOX_FUNC_FIELD_LAST_ALTERED = 15
 };
 
 /** _collation fields. */
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index b20dc41e5..5ca7f3740 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -53,7 +53,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': 'is_deterministic', 'type': 'boolean'}, {'name': 'is_sandboxed',
+        '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'}]]
@@ -152,7 +154,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', '', false, false, {}]
 ...
 box.space._priv:select{}
 ---
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 53d366106..e7a6f0984 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -793,7 +793,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': 'is_deterministic', 'type': 'boolean'}, {'name': 'is_sandboxed',
+        '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'}]]
@@ -829,7 +831,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', '', false, false, {}]
 ...
 session = nil
 ---
diff --git a/test/box/function1.result b/test/box/function1.result
index 99006926e..969798ea0 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -16,7 +16,25 @@ c = net.connect(os.getenv("LISTEN"))
 box.schema.func.create('function1', {language = "C"})
 ---
 ...
-box.space._func:replace{2, 1, 'function1', 0, 'LUA'}
+function setmap(tab) return setmetatable(tab, { __serialize = 'map' }) end
+---
+...
+datetime = os.date("%Y-%m-%d %H:%M:%S")
+---
+...
+box.space._func:replace{2, 1, 'function1', 0, 'LUA', '', 'procedure', setmap({}), 'none', false, false, true, setmap({}), '', datetime, datetime}
+---
+- error: 'Failed to create function ''function1'': unsupported routine_type value'
+...
+box.space._func:replace{2, 1, 'function1', 0, 'LUA', '', 'function', setmap({}), 'reads', false, false, true, setmap({}), '', datetime, datetime}
+---
+- error: 'Failed to create function ''function1'': unsupported sql_data_access value'
+...
+box.space._func:replace{2, 1, 'function1', 0, 'LUA', '', 'function', setmap({}), 'none', false, false, false, setmap({}), '', datetime, datetime}
+---
+- error: 'Failed to create function ''function1'': unsupported is_null_call value'
+...
+box.space._func:replace{2, 1, 'function1', 0, 'LUA', '', 'function', setmap({}), 'none', false, false, true, setmap({}), '', datetime, datetime}
 ---
 - error: function does not support alter
 ...
@@ -59,10 +77,11 @@ c:call('function1.args', { 15 })
 ...
 box.func["function1.args"]
 ---
-- language: C
+- is_deterministic: false
+  id: 2
   setuid: false
   name: function1.args
-  id: 2
+  language: C
 ...
 box.func["function1.args"]:call()
 ---
@@ -326,7 +345,7 @@ c:close()
 function divide(a, b) return a / b end
 ---
 ...
-box.schema.func.create("divide")
+box.schema.func.create("divide", {comment = 'Divide two values'})
 ---
 ...
 func = box.func.divide
@@ -368,10 +387,12 @@ func:drop()
 ...
 func
 ---
-- language: LUA
+- is_deterministic: false
+  id: 2
   setuid: false
+  comment: Divide two values
   name: divide
-  id: 2
+  language: LUA
 ...
 func.drop()
 ---
@@ -424,10 +445,12 @@ func:drop()
 ...
 func
 ---
-- language: C
+- is_deterministic: false
+  id: 2
   setuid: false
+  comment: Divide two values
   name: function1.divide
-  id: 2
+  language: C
 ...
 func:drop()
 ---
@@ -510,6 +533,177 @@ box.schema.func.drop('secret_leak')
 box.schema.func.drop('secret')
 ---
 ...
+--
+-- gh-4182: Introduce persistent Lua functions.
+--
+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:upper():split()
+		for k,v in pairs(t) do t[k] = v end
+		return t
+	end
+]]
+test_run:cmd("setopt delimiter ''");
+---
+...
+box.schema.func.create('addrsplit', {body = body, language = "C"})
+---
+- error: 'Failed to create function ''addrsplit'': body and is_sandboxed options are
+    not compatible with C language'
+...
+box.schema.func.create('addrsplit', {is_sandboxed = true, language = "C"})
+---
+- error: 'Failed to create function ''addrsplit'': body and is_sandboxed options are
+    not compatible with C language'
+...
+box.schema.func.create('addrsplit', {is_sandboxed = true})
+---
+- error: 'Failed to create function ''addrsplit'': is_sandboxed option may be set
+    only for persistent Lua function (when body option is set)'
+...
+box.schema.func.create('invalid', {body = "function(tuple) ret tuple"})
+---
+- error: 'Failed to dynamically load function ''invalid'': [string "return function(tuple)
+    ret tuple"]:1: ''='' expected near ''tuple'''
+...
+box.schema.func.create('addrsplit', {body = body, is_deterministic = true})
+---
+...
+box.schema.user.grant('guest', 'execute', 'function', 'addrsplit')
+---
+...
+conn = net.connect(box.cfg.listen)
+---
+...
+conn:call('addrsplit', {{address = "Moscow Dolgoprudny"}})
+---
+- ['MOSCOW', 'DOLGOPRUDNY']
+...
+box.func.addrsplit:call({{address = "Moscow Dolgoprudny"}})
+---
+- - MOSCOW
+  - DOLGOPRUDNY
+...
+conn:close()
+---
+...
+box.snapshot()
+---
+- ok
+...
+test_run:cmd("restart server default")
+test_run = require('test_run').new()
+---
+...
+test_run:cmd("push filter '(.builtin/.*.lua):[0-9]+' to '\\1'")
+---
+- true
+...
+net = require('net.box')
+---
+...
+conn = net.connect(box.cfg.listen)
+---
+...
+conn:call('addrsplit', {{address = "Moscow Dolgoprudny"}})
+---
+- ['MOSCOW', 'DOLGOPRUDNY']
+...
+box.func.addrsplit:call({{address = "Moscow Dolgoprudny"}})
+---
+- - MOSCOW
+  - DOLGOPRUDNY
+...
+conn:close()
+---
+...
+box.schema.user.revoke('guest', 'execute', 'function', 'addrsplit')
+---
+...
+box.func.addrsplit:drop()
+---
+...
+-- Test sandboxed 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', {body = body, is_sandboxed = true})
+---
+...
+box.func.monkey:call({1})
+---
+- 0
+...
+math.abs(1)
+---
+- 1
+...
+box.func.monkey:drop()
+---
+...
+sum = 0
+---
+...
+function inc_g(val) sum = sum + val end
+---
+...
+box.schema.func.create('call_inc_g', {body = "function(val) inc_g(val) end"})
+---
+...
+box.func.call_inc_g:call({1})
+---
+...
+assert(sum == 1)
+---
+- true
+...
+box.schema.func.create('call_inc_g_safe', {body = "function(val) inc_g(val) end", is_sandboxed = true})
+---
+...
+box.func.call_inc_g_safe:call({1})
+---
+- error: '[string "return function(val) inc_g(val) end"]:1: attempt to call global
+    ''inc_g'' (a nil value)'
+...
+assert(sum == 1)
+---
+- true
+...
+box.func.call_inc_g:drop()
+---
+...
+box.func.call_inc_g_safe:drop()
+---
+...
+-- Test persistent function assemble corner cases
+box.schema.func.create('compiletime_tablef', {body = "{}"})
+---
+- error: 'Failed to dynamically load function ''compiletime_tablef'': given body doesn''t
+    define a function'
+...
+box.schema.func.create('compiletime_call_inc_g', {body = "inc_g()"})
+---
+- error: 'Failed to dynamically load function ''compiletime_call_inc_g'': [string
+    "return inc_g()"]:1: attempt to call global ''inc_g'' (a nil value)'
+...
+assert(sum == 1)
+---
+- true
+...
 test_run:cmd("clear filter")
 ---
 - true
diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
index 25966b915..243f78eb1 100644
--- a/test/box/function1.test.lua
+++ b/test/box/function1.test.lua
@@ -7,7 +7,12 @@ net = require('net.box')
 c = net.connect(os.getenv("LISTEN"))
 
 box.schema.func.create('function1', {language = "C"})
-box.space._func:replace{2, 1, 'function1', 0, 'LUA'}
+function setmap(tab) return setmetatable(tab, { __serialize = 'map' }) end
+datetime = os.date("%Y-%m-%d %H:%M:%S")
+box.space._func:replace{2, 1, 'function1', 0, 'LUA', '', 'procedure', setmap({}), 'none', false, false, true, setmap({}), '', datetime, datetime}
+box.space._func:replace{2, 1, 'function1', 0, 'LUA', '', 'function', setmap({}), 'reads', false, false, true, setmap({}), '', datetime, datetime}
+box.space._func:replace{2, 1, 'function1', 0, 'LUA', '', 'function', setmap({}), 'none', false, false, false, setmap({}), '', datetime, datetime}
+box.space._func:replace{2, 1, 'function1', 0, 'LUA', '', 'function', setmap({}), 'none', false, false, true, setmap({}), '', datetime, datetime}
 box.schema.user.grant('guest', 'execute', 'function', 'function1')
 _ = box.schema.space.create('test')
 _ = box.space.test:create_index('primary')
@@ -120,7 +125,7 @@ c:close()
 
 -- Test registered functions interface.
 function divide(a, b) return a / b end
-box.schema.func.create("divide")
+box.schema.func.create("divide", {comment = 'Divide two values'})
 func = box.func.divide
 func.call({4, 2})
 func:call(4, 2)
@@ -180,4 +185,68 @@ box.schema.user.revoke('guest', 'execute', 'function', 'secret_leak')
 box.schema.func.drop('secret_leak')
 box.schema.func.drop('secret')
 
+--
+-- gh-4182: Introduce persistent Lua functions.
+--
+test_run:cmd("setopt delimiter ';'")
+body = [[function(tuple)
+		if type(tuple.address) ~= 'string' then
+			return nil, 'Invalid field type'
+		end
+		local t = tuple.address:upper():split()
+		for k,v in pairs(t) do t[k] = v end
+		return t
+	end
+]]
+test_run:cmd("setopt delimiter ''");
+box.schema.func.create('addrsplit', {body = body, language = "C"})
+box.schema.func.create('addrsplit', {is_sandboxed = true, language = "C"})
+box.schema.func.create('addrsplit', {is_sandboxed = true})
+box.schema.func.create('invalid', {body = "function(tuple) ret tuple"})
+box.schema.func.create('addrsplit', {body = body, is_deterministic = true})
+box.schema.user.grant('guest', 'execute', 'function', 'addrsplit')
+conn = net.connect(box.cfg.listen)
+conn:call('addrsplit', {{address = "Moscow Dolgoprudny"}})
+box.func.addrsplit:call({{address = "Moscow Dolgoprudny"}})
+conn:close()
+box.snapshot()
+test_run:cmd("restart server default")
+test_run = require('test_run').new()
+test_run:cmd("push filter '(.builtin/.*.lua):[0-9]+' to '\\1'")
+net = require('net.box')
+conn = net.connect(box.cfg.listen)
+conn:call('addrsplit', {{address = "Moscow Dolgoprudny"}})
+box.func.addrsplit:call({{address = "Moscow Dolgoprudny"}})
+conn:close()
+box.schema.user.revoke('guest', 'execute', 'function', 'addrsplit')
+box.func.addrsplit:drop()
+
+-- Test sandboxed 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', {body = body, is_sandboxed = true})
+box.func.monkey:call({1})
+math.abs(1)
+box.func.monkey:drop()
+
+sum = 0
+function inc_g(val) sum = sum + val end
+box.schema.func.create('call_inc_g', {body = "function(val) inc_g(val) end"})
+box.func.call_inc_g:call({1})
+assert(sum == 1)
+box.schema.func.create('call_inc_g_safe', {body = "function(val) inc_g(val) end", is_sandboxed = true})
+box.func.call_inc_g_safe:call({1})
+assert(sum == 1)
+box.func.call_inc_g:drop()
+box.func.call_inc_g_safe:drop()
+
+-- Test persistent function assemble corner cases
+box.schema.func.create('compiletime_tablef', {body = "{}"})
+box.schema.func.create('compiletime_call_inc_g', {body = "inc_g()"})
+assert(sum == 1)
+
 test_run:cmd("clear filter")
-- 
2.21.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 1/6] box: rework func cache update machinery
  2019-06-23 13:57 ` [PATCH v4 1/6] box: rework func cache update machinery Kirill Shcherbatov
@ 2019-06-24 10:22   ` Vladimir Davydov
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Davydov @ 2019-06-24 10:22 UTC (permalink / raw)
  To: Kirill Shcherbatov; +Cc: tarantool-patches

On Sun, Jun 23, 2019 at 04:57:52PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 6d3153815..d63add535 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -541,39 +542,27 @@ schema_free(void)
>  }
>  
>  void
> -func_cache_replace(struct func_def *def)
> +func_cache_insert(struct func *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 };
> +	assert(func_by_id(func->def->fid) == NULL);
> +	assert(func_by_name(func->def->name, strlen(func->def->name)) == NULL);
> +	const struct mh_i32ptr_node_t node = { func->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;
> +		panic_syserror("Out of memory for the data dictionary cache "
> +			       "(stored function).");
>  	}
>  	size_t def_name_len = strlen(func->def->name);
>  	uint32_t name_hash = mh_strn_hash(func->def->name, def_name_len);
>  	const struct mh_strnptr_node_t strnode = {
>  		func->def->name, def_name_len, name_hash, func };
> -
>  	mh_int_t 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);

No need to delete the function before panic. Moreover, we shouldn't do
it even if we didn't panic, because the function was created by the
caller.

> -		goto error;
> +		panic_syserror("Out of memory for the data dictionary cache "
> +			       "(stored function).");

Code duplication.

>  	}
>  }
>  

Pushed to master with the following changes:

diff --git a/src/box/schema.cc b/src/box/schema.cc
index d63add53..87fe18f3 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -549,9 +549,9 @@ func_cache_insert(struct func *func)
 	const struct mh_i32ptr_node_t node = { func->def->fid, func };
 	mh_int_t k1 = mh_i32ptr_put(funcs, &node, NULL, NULL);
 	if (k1 == mh_end(funcs)) {
-		func_delete(func);
-		panic_syserror("Out of memory for the data dictionary cache "
-			       "(stored function).");
+error:
+		panic_syserror("Out of memory for the data "
+			       "dictionary cache (stored function).");
 	}
 	size_t def_name_len = strlen(func->def->name);
 	uint32_t name_hash = mh_strn_hash(func->def->name, def_name_len);
@@ -560,9 +560,7 @@ func_cache_insert(struct func *func)
 	mh_int_t k2 = mh_strnptr_put(funcs_by_name, &strnode, NULL, NULL);
 	if (k2 == mh_end(funcs_by_name)) {
 		mh_i32ptr_del(funcs, k1, NULL);
-		func_delete(func);
-		panic_syserror("Out of memory for the data dictionary cache "
-			       "(stored function).");
+		goto error;
 	}
 }
 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 2/6] box: rework box_lua_{call, eval} to use input port
  2019-06-23 13:57 ` [PATCH v4 2/6] box: rework box_lua_{call, eval} to use input port Kirill Shcherbatov
@ 2019-06-24 10:23   ` Vladimir Davydov
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Davydov @ 2019-06-24 10:23 UTC (permalink / raw)
  To: Kirill Shcherbatov; +Cc: tarantool-patches

On Sun, Jun 23, 2019 at 04:57:53PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/func.h b/src/box/func.h
> index fa4d738ab..f6494d064 100644
> --- a/src/box/func.h
> +++ b/src/box/func.h
> @@ -42,6 +42,9 @@
>  extern "C" {
>  #endif /* defined(__cplusplus) */
>  
> +struct func;
> +struct lua_State;
> +

Pointless forward declarations. Removed them and pushed the patch to
master.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 3/6] box: rework func object as a function frontend
  2019-06-23 13:57 ` [PATCH v4 3/6] box: rework func object as a function frontend Kirill Shcherbatov
@ 2019-06-24 10:23   ` Vladimir Davydov
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Davydov @ 2019-06-24 10:23 UTC (permalink / raw)
  To: Kirill Shcherbatov; +Cc: tarantool-patches

Pushed to master.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 4/6] box: export registered functions in box.func folder
  2019-06-23 13:57 ` [PATCH v4 4/6] box: export registered functions in box.func folder Kirill Shcherbatov
@ 2019-06-24 10:25   ` Vladimir Davydov
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Davydov @ 2019-06-24 10:25 UTC (permalink / raw)
  To: Kirill Shcherbatov; +Cc: tarantool-patches

On Sun, Jun 23, 2019 at 04:57:55PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 8b7223a7c..8c70c1088 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -284,8 +287,13 @@ port_lua_create(struct port *port, struct lua_State *L)
>  }
>  
>  struct execute_lua_ctx {
> -	const char *name;
> -	uint32_t name_len;
> +	union {
> +		struct {
> +			const char *name;
> +			uint32_t name_len;
> +		};
> +		struct mpstream *stream;
> +	};
>  	struct port *args;
>  };
>  
> @@ -340,58 +348,56 @@ execute_lua_eval(lua_State *L)
>  static int
>  encode_lua_call(lua_State *L)
>  {
> -	struct port_lua *port = (struct port_lua *) lua_topointer(L, -1);
> +	struct execute_lua_ctx *ctx =
> +		(struct execute_lua_ctx *) lua_topointer(L, 1);

Using the same struct for executing a function and encoding its result
looks ugly. I introduced a separate struct for this - see my changes
below - and pushed the patch to master.

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 8c70c108..b308924c 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -287,13 +287,8 @@ port_lua_create(struct port *port, struct lua_State *L)
 }
 
 struct execute_lua_ctx {
-	union {
-		struct {
-			const char *name;
-			uint32_t name_len;
-		};
-		struct mpstream *stream;
-	};
+	const char *name;
+	uint32_t name_len;
 	struct port *args;
 };
 
@@ -345,22 +340,26 @@ execute_lua_eval(lua_State *L)
 	return lua_gettop(L);
 }
 
+struct encode_lua_ctx {
+	struct port_lua *port;
+	struct mpstream *stream;
+};
+
 static int
 encode_lua_call(lua_State *L)
 {
-	struct execute_lua_ctx *ctx =
-		(struct execute_lua_ctx *) lua_topointer(L, 1);
+	struct encode_lua_ctx *ctx =
+		(struct encode_lua_ctx *) 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;
-	struct port_lua *port = (struct port_lua *) ctx->args;
-	int size = lua_gettop(port->L);
+	int size = lua_gettop(ctx->port->L);
 	for (int i = 1; i <= size; ++i)
-		luamp_encode(port->L, cfg, ctx->stream, i);
-	port->size = size;
+		luamp_encode(ctx->port->L, cfg, ctx->stream, i);
+	ctx->port->size = size;
 	mpstream_flush(ctx->stream);
 	return 0;
 }
@@ -368,16 +367,15 @@ encode_lua_call(lua_State *L)
 static int
 encode_lua_call_16(lua_State *L)
 {
-	struct execute_lua_ctx *ctx =
-		(struct execute_lua_ctx *) lua_topointer(L, 1);
+	struct encode_lua_ctx *ctx =
+		(struct encode_lua_ctx *) 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;
-	struct port_lua *port = (struct port_lua *) ctx->args;
-	port->size = luamp_encode_call_16(port->L, cfg, ctx->stream);
+	ctx->port->size = luamp_encode_call_16(ctx->port->L, cfg, ctx->stream);
 	mpstream_flush(ctx->stream);
 	return 0;
 }
@@ -392,8 +390,8 @@ port_lua_do_dump(struct port *base, struct mpstream *stream,
 	 * Use the same global state, assuming the encoder doesn't
 	 * yield.
 	 */
-	struct execute_lua_ctx ctx;
-	ctx.args = base;
+	struct encode_lua_ctx ctx;
+	ctx.port = port;
 	ctx.stream = stream;
 	struct lua_State *L = tarantool_L;
 	int top = lua_gettop(L);

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 5/6] box: refactor box_lua_find helper
  2019-06-23 13:57 ` [PATCH v4 5/6] box: refactor box_lua_find helper Kirill Shcherbatov
@ 2019-06-24 12:35   ` Vladimir Davydov
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Davydov @ 2019-06-24 12:35 UTC (permalink / raw)
  To: Kirill Shcherbatov; +Cc: tarantool-patches

Pushed to master.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 6/6] box: introduce Lua persistent functions
  2019-06-23 13:57 ` [PATCH v4 6/6] box: introduce Lua persistent functions Kirill Shcherbatov
@ 2019-06-24 12:38   ` Vladimir Davydov
  2019-06-25  8:22     ` [tarantool-patches] " Konstantin Osipov
  0 siblings, 1 reply; 14+ messages in thread
From: Vladimir Davydov @ 2019-06-24 12:38 UTC (permalink / raw)
  To: Kirill Shcherbatov; +Cc: tarantool-patches

On Sun, Jun 23, 2019 at 04:57:57PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index f98ab42ac..d3d874477 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -669,6 +870,22 @@ lbox_func_new(struct lua_State *L, struct func *func)
>  	lua_pushstring(L, "language");
>  	lua_pushstring(L, func_language_strs[func->def->language]);
>  	lua_settable(L, top);
> +	lua_pushstring(L, "is_deterministic");
> +	lua_pushboolean(L, func->def->is_deterministic);
> +	lua_settable(L, top);
> +	if (func->def->body != NULL) {
> +		lua_pushstring(L, "body");
> +		lua_pushstring(L, func->def->body);
> +		lua_settable(L, top);
> +		lua_pushstring(L, "is_sandboxed");
> +		lua_pushboolean(L, func->def->is_sandboxed);
> +		lua_settable(L, top);
> +	}
> +	if (func->def->comment != NULL) {
> +		lua_pushstring(L, "comment");
> +		lua_pushstring(L, func->def->comment);
> +		lua_settable(L, top);
> +	}

We must delete 'body' and 'is_sandboxed' in case the function isn't
persistent, otherwise it might inherit old values.

> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 9c3ee063c..455d1e18a 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -2138,7 +2138,10 @@ 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',
> +                              is_deterministic = 'boolean',
> +                              is_sandboxed = 'boolean', opts = 'table',

opts = 'table' not needed, apparently.

Pushed to master with the following changes:

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index d50e23bf..88110c43 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -868,22 +868,27 @@ lbox_func_new(struct lua_State *L, struct func *func)
 	lua_pushstring(L, "language");
 	lua_pushstring(L, func_language_strs[func->def->language]);
 	lua_settable(L, top);
+	lua_pushstring(L, "body");
+	if (func->def->body != NULL)
+		lua_pushstring(L, func->def->body);
+	else
+		lua_pushnil(L);
+	lua_settable(L, top);
+	lua_pushstring(L, "comment");
+	if (func->def->comment != NULL)
+		lua_pushstring(L, func->def->comment);
+	else
+		lua_pushnil(L);
+	lua_settable(L, top);
 	lua_pushstring(L, "is_deterministic");
 	lua_pushboolean(L, func->def->is_deterministic);
 	lua_settable(L, top);
-	if (func->def->body != NULL) {
-		lua_pushstring(L, "body");
-		lua_pushstring(L, func->def->body);
-		lua_settable(L, top);
-		lua_pushstring(L, "is_sandboxed");
+	lua_pushstring(L, "is_sandboxed");
+	if (func->def->body != NULL)
 		lua_pushboolean(L, func->def->is_sandboxed);
-		lua_settable(L, top);
-	}
-	if (func->def->comment != NULL) {
-		lua_pushstring(L, "comment");
-		lua_pushstring(L, func->def->comment);
-		lua_settable(L, top);
-	}
+	else
+		lua_pushnil(L);
+	lua_settable(L, top);
 
 	/* Bless func object. */
 	lua_getfield(L, LUA_GLOBALSINDEX, "box");
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 455d1e18..1ab97440 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2140,8 +2140,8 @@ box.schema.func.create = function(name, opts)
                               if_not_exists = 'boolean',
                               language = 'string', body = 'string',
                               is_deterministic = 'boolean',
-                              is_sandboxed = 'boolean', opts = 'table',
-                              comment = 'string'})
+                              is_sandboxed = 'boolean',
+                              comment = 'string' })
     local _func = box.space[box.schema.FUNC_ID]
     local _vfunc = box.space[box.schema.VFUNC_ID]
     local func = _vfunc.index.name:get{name}
diff --git a/test/box/function1.result b/test/box/function1.result
index 969798ea..7bea2d64 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -448,7 +448,6 @@ func
 - is_deterministic: false
   id: 2
   setuid: false
-  comment: Divide two values
   name: function1.divide
   language: C
 ...

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [tarantool-patches] Re: [PATCH v4 6/6] box: introduce Lua persistent functions
  2019-06-24 12:38   ` Vladimir Davydov
@ 2019-06-25  8:22     ` Konstantin Osipov
  0 siblings, 0 replies; 14+ messages in thread
From: Konstantin Osipov @ 2019-06-25  8:22 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Kirill Shcherbatov

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/06/24 15:40]:

The reasons I reverted this patch are:

- formally, RFC is not ready and not pushed
- the process reason why RFC is not pushed is that the contributor
  didn't care to push it to completion

- the technical reason RFC is not ready because nothing is done in
  the RFC to provision for persistent functions in SQL. How is a
  persistent function will ever be used in SQL? What if there is a
  name conflict with a built-in SQL function? What is the return
  type of such a function?  What are argument types and how are
  they pushed to the function's call stack? It's not necessary to 
  implement all of this at once, but the RFC should provision good
  answers for it.

-- 
Konstantin Osipov, Moscow, Russia

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-06-25  8:22 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-23 13:57 [PATCH v4 0/6] box: rework functions machinery Kirill Shcherbatov
2019-06-23 13:57 ` [PATCH v4 1/6] box: rework func cache update machinery Kirill Shcherbatov
2019-06-24 10:22   ` Vladimir Davydov
2019-06-23 13:57 ` [PATCH v4 2/6] box: rework box_lua_{call, eval} to use input port Kirill Shcherbatov
2019-06-24 10:23   ` Vladimir Davydov
2019-06-23 13:57 ` [PATCH v4 3/6] box: rework func object as a function frontend Kirill Shcherbatov
2019-06-24 10:23   ` Vladimir Davydov
2019-06-23 13:57 ` [PATCH v4 4/6] box: export registered functions in box.func folder Kirill Shcherbatov
2019-06-24 10:25   ` Vladimir Davydov
2019-06-23 13:57 ` [PATCH v4 5/6] box: refactor box_lua_find helper Kirill Shcherbatov
2019-06-24 12:35   ` Vladimir Davydov
2019-06-23 13:57 ` [PATCH v4 6/6] box: introduce Lua persistent functions Kirill Shcherbatov
2019-06-24 12:38   ` Vladimir Davydov
2019-06-25  8:22     ` [tarantool-patches] " Konstantin Osipov

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