From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 5ECBA6EC5D; Mon, 5 Apr 2021 19:04:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5ECBA6EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617638684; bh=RWmga8sRswmnpLhCW5/1/v6otYYA5+7CFTGDJvp5wHo=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=bKaICjFklf5j5/6SZj6dPeY/ZWH5T2QC0THndiAqaD4ANhtktAxgZ+ni0lffWonvx LiCf+SGTCNlIDdcKwLxgA6bonsaOKxtfbm0AO4jDlIukgm2XiDAR+bmsq9Rb1AKWbv NNvCh/IHJBgGlh3HNgnCl4FWbaJNScBFWGhZtAPU= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 964B46EC5D for ; Mon, 5 Apr 2021 19:04:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 964B46EC5D Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lTRiU-0002Wb-Eu; Mon, 05 Apr 2021 19:04:43 +0300 To: Cyrill Gorcunov , tml References: <20210402123420.885834-1-gorcunov@gmail.com> <20210402123420.885834-7-gorcunov@gmail.com> Message-ID: <8d3a4d20-2887-7423-777f-2af808b65495@tarantool.org> Date: Mon, 5 Apr 2021 18:04:41 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <20210402123420.885834-7-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E32947A0146560F8BA709498CFB6209D8582A182A05F5380850404C2A19C24E36F2993754D488630F94A1DB6178942137E8E136E1FE858405D516 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78887611F2F2455C9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B23888C9749F3CAC8638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C16EE06F5A270FE6A07F655FAFE4641E905CF161A7AC3DC6EA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3E97D2AE7161E217F117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006373BC478629CBEC79DEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5A113106D41BEE965972ECE54E9539AF60876E3E42E58723CD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34A9A0A0BF1A2CAC6284AF6B1F4699922697B9F9EFEEBEB0C621A75770971D891862C08BD44F0CDD0C1D7E09C32AA3244C8A43658162DE4F69C2ACC21D1700E42B8A6D4CC6FBFAC251927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojM00ve/f+0olDo3rDIe99Zw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638220AA7B7BE11ABEC61ABBB77EA4F8467F33841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > +#include > +#include > + > +#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.