From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 22202469719 for ; Sat, 10 Oct 2020 00:46:23 +0300 (MSK) References: <20201008213608.1022476-1-gorcunov@gmail.com> <20201008213608.1022476-6-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: <935104b9-2ba6-6264-ef0b-fa9f587a736d@tarantool.org> Date: Fri, 9 Oct 2020 23:46:20 +0200 MIME-Version: 1.0 In-Reply-To: <20201008213608.1022476-6-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Thanks for the patch! See 10 comments below. > @TarantoolBot document > Title: cbox module > > Overview > ======== > > `cbox` module provides a way to create, delete and execute > `C` procedures. Unlinke `box.schema.func` functionality this > functions are not persistent in any space and live purely in > memory, thus once node get turned off they are vanished. Same > time such functions are allowed to be created and executed > when a node is in read-only mode. > > Module functions > ================ > > `cbox.func.create(name) -> obj | nil, err` > ------------------------------------------ > > Create a new function with name `name`. 1. I hope this is a draft. Because in the final version it would be better to explain what does it mean to 'create' a function (or 'load' in the next version, I guess). That it will load user's dynamic lib on the first function call. It does not create anything, just loads an existing function from a shared file. > Possible errors: > - IllegalParams: function name is either not supplied > or not a string. > - IllegalParams: a function is already created. > - OutOfMemory: unable to allocate a function. > > On success the new callable object is returned, otherwise > `nil,error` pair. > > Example: > > ``` Lua > f, err = require('cbox').create('func') > if not f then > print(err) > end > -- call the function > f(arg1,arg2,...) > ``` > > `cbox.func.drop(name) -> true | nil, err` > ----------------------------------------- > > Deletes a function with name `name`. > > Possible errors: > - IllegalParams: function name is either not supplied > or not a string. > - IllegalParams: the function does not exist. > > On success `true` is returned, otherwise `nil,error` pair. > > Example: > > ``` Lua > ok, err = require('cbox').func.drop('func') > if not ok then > print(err) > end > ``` > > `cbox.module.reload(name) -> true | nil, err` > --------------------------------------------- > > Reloads a module with name `name` and all functions which > were associated for the module. 2. What happens with the currently working functions? What happens, if there are still non-deleted refs to the old functions? What if all refs are deleted, but not on all functions was called unload()? > Possible errors: > - IllegalParams: module name is either not supplied > or not a string. > - IllegalParams: the function does not exist. > - ClientError: a module with the name provided does > not exist. > > On success `true` is returned, otherwise `nil,error` pair. > > Example: > > ``` Lua > ok, err = require('cbox').module.reload('func') > if not ok then > print(err) > end > ``` 3. It seems the function invocation itself is not documented? > diff --git a/src/box/lua/cbox.c b/src/box/lua/cbox.c > new file mode 100644 > index 000000000..58585b44d > --- /dev/null > +++ b/src/box/lua/cbox.c > @@ -0,0 +1,417 @@ > +/* > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file. > + */ > + > +#include > +#include > + > +#include "tt_static.h" 4. Why do you need tt_static.h? You don't use any of tt_* functions here. > +#include "assoc.h" > +#include "diag.h" > +#include "port.h" > +#include "say.h" 5. You don't use say_* functions either. > + > +#include "box/error.h" > +#include "box/func.h" > +#include "box/call.h" > +#include "box/port.h" > + > +#include "trivia/util.h" > +#include "lua/utils.h" > + > +/** Function names to descriptor map. */ > +static struct mh_strnptr_t *func_hash; > + > +/** > + * Function descriptor. > + */ > +struct cbox_func { > + /** > + * Symbol descriptor for the function in > + * an associated module. > + */ > + struct module_sym mod_sym; > + > + /** Function name length */ > + size_t name_len; > + > + /** Function name */ > + char name[0]; > +}; > + > +/** > + * Find function descriptor by its name. > + */ > +struct cbox_func * > +cbox_func_find(const char *name, size_t len) 6. I assume the function should be 'static'. It is never used out of cbox.c. > +{ > + if (name != NULL && len > 0) { > + mh_int_t e = mh_strnptr_find_inp(func_hash, name, len); > + if (e != mh_end(func_hash)) > + return mh_strnptr_node(func_hash, e)->val; > + } > + return NULL; > +} > + > +/** > + * Add function descriptor to the cache. > + */ > +static int > +cbox_func_add(struct cbox_func *cfunc) > +{ > + const struct mh_strnptr_node_t nd = { > + .str = cfunc->name, > + .len = cfunc->name_len, > + .hash = mh_strn_hash(cfunc->name, cfunc->name_len), > + .val = cfunc, > + }; > + > + mh_int_t h = mh_strnptr_put(func_hash, &nd, NULL, NULL); > + if (h == mh_end(func_hash)) { > + diag_set(OutOfMemory, sizeof(nd), __func__, "h"); > + return -1; > + } > + return 0; > +} > + > +/** > + * Setup a detailed error description into the diagnostics area. > + */ > +static void > +diag_set_param_error(struct lua_State *L, int idx, const char *func_name, > + const char *param, const char *exp) > +{ > + const char *typename = idx == 0 ? > + "" : lua_typename(L, lua_type(L, idx)); > + static const char *fmt = > + "%s: wrong parameter \"%s\": expected %s, got %s"; 7. Please, lets omit 'static' when it is not needed. It just raises questions. > + diag_set(IllegalParams, fmt, func_name, param, exp, typename); > +} > + > +/** > + * Call function by its name from the Lua code. > + */ > +static int > +lcbox_func_call(struct lua_State *L) > +{ > + int nr_args = lua_gettop(L); 8. It is not really a number of arguments. It includes the function object itself. > + > + /* > + * A function name is associated with the variable. > + */ > + lua_getfield(L, -nr_args, "name"); > + size_t name_len; > + const char *name = lua_tolstring(L, -1, &name_len); > + struct cbox_func *cf = cbox_func_find(name, name_len); > + /* > + * Do not pop the result early, since Lua's GC may > + * drop the name string found. > + */ > + lua_pop(L, 1); > + > + if (cf == NULL) { > + /* > + * This can happen if someone changed the > + * name of the function manually. > + */ > + const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS); > + diag_set(IllegalParams, fmt, name); > + return luaT_push_nil_and_error(L); > + } > + > + /* > + * FIXME: We should get rid of luaT_newthread but this > + * requires serious modifucations. In particular 9. modifucations -> modifications. > + * port_lua_do_dump uses tarantool_L reference and > + * coro_ref must be valid as well. > + */ > + lua_State *args_L = luaT_newthread(tarantool_L); > + if (args_L == NULL) > + return luaT_push_nil_and_error(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 (module_sym_call(&cf->mod_sym, &args, &ret) != 0) { > + port_destroy(&args); > + return luaT_push_nil_and_error(L); > + } > + > + int top = lua_gettop(L); > + lua_pushboolean(L, true); > + port_dump_lua(&ret, L, true); > + int cnt = lua_gettop(L) - top; > + > + port_destroy(&ret); > + port_destroy(&args); > + > + return cnt; > +} > + > +/** > + * Create a new function. > + * > + * This function takes a function name from the caller > + * stack @a L and creates a new function object. > + * > + * Possible errors: > + * > + * - IllegalParams: function name is either not supplied > + * or not a string. > + * - IllegalParams: a function is already created. > + * - OutOfMemory: unable to allocate a function. > + * > + * @returns function object on success or {nil,error} on error, > + * the error is set to the diagnostics area. > + */ > +static int > +lcbox_func_create(struct lua_State *L) > +{ > + const char *method = "cbox.func.create"; > + struct cbox_func *cf = NULL; > + > + if (lua_gettop(L) != 1) { > + const char *fmt = "Expects %s(\'name\')"; > + diag_set(IllegalParams, fmt, method); > + goto out; > + } > + > + if (lua_type(L, 1) != LUA_TSTRING) { > + diag_set_param_error(L, 1, method, > + lua_tostring(L, 1), > + "function name"); > + goto out; > + } > + > + size_t name_len; > + const char *name = lua_tolstring(L, 1, &name_len); > + > + if (cbox_func_find(name, name_len) != NULL) { > + const char *fmt = tnt_errcode_desc(ER_FUNCTION_EXISTS); > + diag_set(IllegalParams, fmt, name); > + goto out; > + } > + > + size_t size = sizeof(*cf) + name_len + 1; > + cf = malloc(size); > + if (cf == NULL) { > + diag_set(OutOfMemory, size, "malloc", "cf"); > + goto out; > + } > + > + cf->mod_sym.addr = NULL; > + cf->mod_sym.module = NULL; > + cf->mod_sym.name = cf->name; > + cf->name_len = name_len; > + > + memcpy(cf->name, name, name_len); > + cf->name[name_len] = '\0'; > + > + if (cbox_func_add(cf) != 0) > + goto out; > + > + /* > + * A new variable should be callable for > + * convenient use in Lua. > + */ > + lua_newtable(L); > + > + lua_pushstring(L, "name"); > + lua_pushstring(L, cf->name); > + lua_settable(L, -3); > + > + lua_newtable(L); > + > + lua_pushstring(L, "__call"); > + lua_pushcfunction(L, lcbox_func_call); > + lua_settable(L, -3); > + > + lua_setmetatable(L, -2); > + return 1; > + > +out: > + free(cf); > + return luaT_push_nil_and_error(L); > +} > + > +/** > + * Drop a function. > + * > + * This function takes a function name from the caller > + * stack @a L and drops a function object. > + * > + * Possible errors: > + * > + * - IllegalParams: function name is either not supplied > + * or not a string. > + * - IllegalParams: the function does not exist. > + * > + * @returns true on success or {nil,error} on error, > + * the error is set to the diagnostics area. > + */ > +static int > +lcbox_func_drop(struct lua_State *L) > +{ > + const char *method = "cbox.func.drop"; > + const char *name = NULL; > + > + if (lua_gettop(L) != 1) { > + const char *fmt = "expects %s(\'name\')"; > + diag_set(IllegalParams, fmt, method); > + return luaT_push_nil_and_error(L); > + } > + > + if (lua_type(L, 1) != LUA_TSTRING) { > + diag_set_param_error(L, 1, method, > + lua_tostring(L, 1), > + "function name or id"); > + return luaT_push_nil_and_error(L); > + } > + > + size_t name_len; > + name = lua_tolstring(L, 1, &name_len); > + > + mh_int_t e = mh_strnptr_find_inp(func_hash, name, name_len); > + if (e == mh_end(func_hash)) { > + const char *fmt = tnt_errcode_desc(ER_NO_SUCH_FUNCTION); > + diag_set(IllegalParams, fmt, name); > + return luaT_push_nil_and_error(L); > + } > + > + struct cbox_func *cf = mh_strnptr_node(func_hash, e)->val; > + mh_strnptr_del(func_hash, e, NULL); > + free(cf); 10. I would advise to wrap it into cbox_func_delete(). I am 100% sure we will add more deletion things to the function object soon. > + > + lua_pushboolean(L, true); > + return 1; > +}