From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v3 2/6] box: rework box_lua_{call, eval} to use input port References: <20190618121256.hexjwmwh4tdjmas3@esperanza> From: Kirill Shcherbatov Message-ID: <9018c744-6018-23f4-bb36-1c144a8c4724@tarantool.org> Date: Wed, 19 Jun 2019 18:51:15 +0300 MIME-Version: 1.0 In-Reply-To: <20190618121256.hexjwmwh4tdjmas3@esperanza> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov List-ID: >> module_gc(module); >> + 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(out_port); >> + return -1; >> + } > >Shouldn't you call region_truncate() here to clean up after the function >and possibly port_get_msgpack()? No, the func_c_call implementation must clean region by it's own. ====================================================== 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. 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 | 30 +++++++++++++++--- 14 files changed, 194 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 e81cc32aa..a91c939fc 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 /** @@ -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 + #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 + 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 8de7401fa..3058d8dac 100644 --- a/src/box/lua/misc.cc +++ b/src/box/lua/misc.cc @@ -61,8 +61,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; @@ -72,6 +74,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 **/ @@ -109,7 +124,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 +#include #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..e25e8fc5e 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 #include #if defined(__cplusplus) @@ -62,8 +63,13 @@ 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. + */ + 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 +80,16 @@ 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. + * 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 +125,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 +136,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