From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module Date: Mon, 5 Apr 2021 18:04:41 +0200 [thread overview] Message-ID: <8d3a4d20-2887-7423-777f-2af808b65495@tarantool.org> (raw) In-Reply-To: <20210402123420.885834-7-gorcunov@gmail.com> Thanks for the patch! See 10 comments below. > @TarantoolBot document > Title: box.lib module > > Overview > ======== > > `box.lib` module provides a way to create, delete and execute > `C` procedures from shared libraries. Unlike `box.schema.func` > methods the functions created with `box.lib` help are not persistent > and live purely in memory. Once a node get turned off they are > vanished. An initial purpose for them is to execute them on > nodes which are running in read-only mode. > > Module functions > ================ > > `require('box.lib').load(path) -> obj | error` 1. Implementation in C should allow not to call 'require()' at all. For example, you don't call require('box.tuple') to use the tuples. The same for lib. However I don't know what is the current rule for the new modules - force to call require() or not. Please, ask Mons what should we do. In case we need to force to call require() - I have no idea how to do that TBH, since the module is in C really, not in Lua. It is loaded as a part of the executable file anyway. 2. Why can't I use box.lib without calling box.cfg{}? I does not depend on it anyhow. For example, box.tuple works fine, require() on .so files works fine, even ffi.load() works fine without box.cfg{}. What is the problem with box.lib? > Unloads a module. Returns `true` on success, otherwise an error > is thrown. Once the module is unloaded one can't load new > functions from this module instance. > > Possible errors: > > - IllegalParams: a module is not supplied. > - IllegalParams: a module is already unloaded. > > Example: > > ``` Lua > m = require('box.lib').load('path/to/library') > -- > -- do something with module > -- > m:unload() > ``` > > If there are functions from this module referenced somewhere > in other places of Lua code they still can be executed because > the module continue sitting in memory until the last reference > to it is closed. > > If the module become a target to the Lua's garbage collector > then unload is called implicitly. > > module:load(name) -> obj | error` 3. You need to add ` in the beginning too. Otherwise the document structure would be screwed. > --- > src/box/CMakeLists.txt | 1 + > src/box/lua/box_lib.c | 590 +++++++++++++++++++++++++++++++++++++++++ > src/box/lua/box_lib.h | 25 ++ 4. It is already in box/ folder, you don't need box_ prefix for the files. For example, we don't have box_tuple.h/box_tuple.c. We have just tuple.c and tuple.h, and so on. > src/box/lua/init.c | 2 + > test/box/misc.result | 1 + > 5 files changed, 619 insertions(+) > create mode 100644 src/box/lua/box_lib.c > create mode 100644 src/box/lua/box_lib.h > > diff --git a/src/box/lua/box_lib.c b/src/box/lua/box_lib.c > new file mode 100644 > index 000000000..ce2ef8902 > --- /dev/null > +++ b/src/box/lua/box_lib.c > @@ -0,0 +1,590 @@ > +/* > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file. > + */ > + > +#include <unistd.h> > +#include <string.h> > +#include <lua.h> > + > +#include "box/error.h" > +#include "box/port.h" > + > +#include "tt_static.h" > + > +#include "assoc.h" > +#include "box_lib.h" > +#include "diag.h" > +#include "module_cache.h" > + > +#include "lua/utils.h" > + > +/** > + * Function descriptor. > + */ > +struct box_module_func { > + /** C function to call. */ > + struct module_func mf; 5. You could call it 'base' like we normally do when inherit structures. > + /** Number of references. */ > + int64_t refs; > + /** Length of functon name in @a key. */ > + size_t sym_len; > + /** Length of @a key. */ > + size_t len; > + /** Function hash key. */ > + char key[0]; > +}; <...> > +/** Handle __index request for a module object. */ > +static int > +lbox_module_index(struct lua_State *L) > +{ > + lua_getmetatable(L, 1); > + lua_pushvalue(L, 2); > + lua_rawget(L, -2); > + if (!lua_isnil(L, -1)) > + return 1; 6. What is happening here in these 5 lines? > + > + struct module *m = get_udata(L, uname_lib); > + if (m == NULL) { > + lua_pushnil(L); > + return 1; > + } <...> > + > +/** > + * Load a function. > + * > + * This function takes a function name from the caller > + * stack @a L and either returns a cached function or > + * creates a new function object. > + * > + * Possible errors: > + * > + * - IllegalParams: function name is either not supplied > + * or not a string. > + * - SystemError: unable to open a module due to a system error. > + * - ClientError: a module does not exist. > + * - OutOfMemory: unable to allocate a module. > + * > + * @returns module object on success or throws an error. > + */ > +static int > +lbox_module_load_func(struct lua_State *L) > +{ > + const char *method = "function = module:load"; > + const char fmt_noname[] = "Expects %s(\'name\') but no name passed"; 7. Why do you randomly jump between const char * and const char []? Please, use const char * to be consistent, and because it looks simpler. > + > + if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) { 8. Here you use lua_isstring(), in some places you use lua_type() == LUA_TSTRING. Please, be consistent, and choose one way. > + diag_set(IllegalParams, fmt_noname, method); > + return luaT_error(L); > + } > + > + struct module *m = get_udata(L, uname_lib); > + if (m == NULL) { > + const char *fmt = > + "Expects %s(\'name\') but not module object passed"; > + diag_set(IllegalParams, fmt, method); > + return luaT_error(L); > + } > + > + size_t sym_len; > + const char *sym = lua_tolstring(L, 2, &sym_len); > + > + if (sym_len < 1) { > + diag_set(IllegalParams, fmt_noname, method); > + return luaT_error(L); > + } > + > + /* > + * Functions are bound to a module symbols, thus > + * since the hash is global it should be unique > + * per module. The symbol (function name) is the > + * last part of the hash key. > + */ > + const char *key = tt_sprintf("%p.%s.%s", (void *)m, > + m->package, sym); 9. I wouldn't use the static buffer in non-trivial code, and never in Lua and Lua C code. Especially after the recently discovered bug. Firstly, something inside box_module_func_new() might also try to use the static buffer and would overwrite it. Secondly, deep inside it accesses Lua to find package.* content, and that might trigger Lua GC, which also might use the static buffer for anything. > + size_t len = strlen(key); > + > + struct box_module_func *cf = cache_find(key, len); > + if (cf == NULL) { > + cf = box_module_func_new(m, key, len, sym_len); > + if (cf == NULL) > + return luaT_error(L); > + } else { > + box_module_func_ref(cf); > + } > + > + new_udata(L, uname_func, cf); > + return 1; > +} > + > +/** > + * Unload a function. > + * > + * Take a function object from the caller stack @a L and unload it. > + * > + * Possible errors: > + * > + * - IllegalParams: the function is not supplied. > + * - IllegalParams: the function already unloaded. > + * > + * @returns true on success or throwns an error. > + */ > +static int > +lbox_func_unload(struct lua_State *L) > +{ > + if (lua_gettop(L) != 1) { > + diag_set(IllegalParams, "Expects function:unload()"); > + return luaT_error(L); > + } > + > + struct box_module_func *cf = get_udata(L, uname_func); > + if (cf == NULL) { > + diag_set(IllegalParams, "The function is unloaded"); > + return luaT_error(L); > + } > + > + set_udata(L, uname_func, NULL); 10. By having get_udata and set_udata helpers you force yourself to do double-check for the cdata on each pair of these calls. While you could get void** first time, and simply *ptr = NULL it instead of set_udata(NULL). Up to you, just a note.
next prev parent reply other threads:[~2021-04-05 16:04 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-02 12:34 [Tarantool-patches] [PATCH v20 0/7] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches 2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 1/7] box/schema: make sure hashes are created Cyrill Gorcunov via Tarantool-patches 2021-04-05 9:28 ` Serge Petrenko via Tarantool-patches 2021-04-05 9:50 ` Cyrill Gorcunov via Tarantool-patches 2021-04-05 10:13 ` Serge Petrenko via Tarantool-patches 2021-04-05 15:45 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-06 7:44 ` Cyrill Gorcunov via Tarantool-patches 2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 2/7] box/func: module_reload -- drop redundant argument Cyrill Gorcunov via Tarantool-patches 2021-04-05 10:23 ` Serge Petrenko via Tarantool-patches 2021-04-05 10:26 ` Serge Petrenko via Tarantool-patches 2021-04-05 10:31 ` Cyrill Gorcunov via Tarantool-patches 2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore Cyrill Gorcunov via Tarantool-patches 2021-04-05 10:53 ` Serge Petrenko via Tarantool-patches 2021-04-05 11:26 ` Cyrill Gorcunov via Tarantool-patches 2021-04-05 11:42 ` Serge Petrenko via Tarantool-patches 2021-04-05 15:47 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-06 8:38 ` Cyrill Gorcunov via Tarantool-patches 2021-04-06 20:02 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-06 20:42 ` Cyrill Gorcunov via Tarantool-patches 2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem Cyrill Gorcunov via Tarantool-patches 2021-04-05 12:34 ` Serge Petrenko via Tarantool-patches 2021-04-05 15:52 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-06 14:33 ` Cyrill Gorcunov via Tarantool-patches 2021-04-06 20:09 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-06 22:05 ` Cyrill Gorcunov via Tarantool-patches 2021-04-06 23:43 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-07 7:03 ` Cyrill Gorcunov via Tarantool-patches 2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 5/7] box/schema.func: switch to new module api Cyrill Gorcunov via Tarantool-patches 2021-04-05 15:56 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module Cyrill Gorcunov via Tarantool-patches 2021-04-05 16:04 ` Vladislav Shpilevoy via Tarantool-patches [this message] 2021-04-07 16:59 ` Cyrill Gorcunov via Tarantool-patches 2021-04-07 20:22 ` Cyrill Gorcunov via Tarantool-patches 2021-04-07 20:28 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-07 20:37 ` Cyrill Gorcunov via Tarantool-patches 2021-04-07 20:45 ` Cyrill Gorcunov via Tarantool-patches 2021-04-07 21:04 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 7/7] test: add box.lib test Cyrill Gorcunov via Tarantool-patches 2021-04-05 16:04 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-05 15:45 ` [Tarantool-patches] [PATCH v20 0/7] box: implement box.lib Lua module Vladislav Shpilevoy via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8d3a4d20-2887-7423-777f-2af808b65495@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox