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 688586EC5E; Sun, 11 Apr 2021 18:38:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 688586EC5E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618155535; bh=v46AHjBR0hCOo7d5z/a4d/2l98iryPbnF0VPUeU9lHs=; 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=EH6Hr31J6kVWLf48LBGKEBOQP3p6vq/9WdZU/iw5D1JU4qxmnLceQVNn8qsWKkJge D71NRtR0XVeoaCm3KVKL/Eu/DgJyQYkQIyP7pKl45zAWlXx3SxZYE3PUOTGjDfGENW NdO49Urmix9FTpfwWkvNywedmfvANIh1mQeJd+h8= 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 6CD706EC5E for ; Sun, 11 Apr 2021 18:38:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6CD706EC5E Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lVcAm-0000Wj-7J; Sun, 11 Apr 2021 18:38:52 +0300 To: Cyrill Gorcunov , tml References: <20210408164151.1759348-1-gorcunov@gmail.com> <20210408164151.1759348-6-gorcunov@gmail.com> Message-ID: <0868de76-3941-ed34-3d7e-2f7328e30934@tarantool.org> Date: Sun, 11 Apr 2021 17:38:51 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <20210408164151.1759348-6-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480257C85EA0BB7A95D0F00AE41BB9A5343182A05F5380850405F1F3E7A636A1FF9EE2C5B6DDBEE5F144DBBDA5F53C5954B8080351829FEA6C8 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7466896EF24E80F12EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378BCFB34D7DDF138E8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B21430FB47253EC8FEF438CF20C04B64CE40B83DF1158C6122D2E47CDBA5A96583C09775C1D3CA48CFE478A468B35FE767117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE74A95F4E53E8DCE969FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73542F54486E6D6388DC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5FC07E6A60D1FF849E3EDCD190DD51FF472EB6FD1610E9A0AD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34FD9114F69CA58D1E21DE04BCCE0FF7154632858F31E4AC833A604DBD064756501E9D92C17CAE095C1D7E09C32AA3244CA5EC8DCF1B5A80CC00C481CE032C7C35B4DF56057A86259F927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyKKJYJ15DtKVOLBT4uC1og== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822051866E6F9E8572714292AB0FF5B60133841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v21 5/6] 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! Please, try to respond to my comments in the original email, explicitly. When and how you fixed them, or why did not. Not with promises like "will do", "ok", and so on, they don't help much. Otherwise I see you tend to miss some of the comments. Also I don't remember all of my comments, so when you don't respond to them, I need to go to my old email, and match the comments with the places you was supposed to fix, and it takes time. See how Sergey and Mergen send their patches for examples. https://github.com/tarantool/tarantool/wiki/Code-review-procedure#during-the-review See 4 comments below. > module:load(name) -> obj | error` 1. In the previous review I said you missed ` in the beginning of the line here. Please, add it. Otherwise the formatting might be broken. > --------------------------------- > > Loads a new function with name `name` from the previously > loaded `module` and return a callable object instance > associated with the function. On failure an error is thrown. > > Possible errors: > - IllegalParams: function name is either not supplied > or not a string. > - IllegalParams: attempt to load a function but module > has been unloaded already. > - ClientError: no such function in the module. > - OutOfMemory: unable to allocate a function. > > Example: > > ``` Lua > -- Load a module if not been loaded yet. > m = box.lib.load('path/to/library') > -- Load a function with the `foo` name from the module `m`. > func = m:load('foo') > ``` > > diff --git a/src/box/lua/lib.c b/src/box/lua/lib.c > new file mode 100644 > index 000000000..78aee37b5 > --- /dev/null > +++ b/src/box/lua/lib.c > @@ -0,0 +1,606 @@ > + > +static int > +cache_put(struct box_module_func *cf) > +{ > + const struct mh_strnptr_node_t nd = { > + .str = cf->key, > + .len = cf->len, > + .hash = mh_strn_hash(cf->key, cf->len), > + .val = cf, > + }; > + mh_int_t e = mh_strnptr_put(func_hash, &nd, NULL, NULL); 2. As I said in the previous review, it would be good to get the old value and ensure it is NULL. So as not to replace something and not notice it. > + if (e == mh_end(func_hash)) { > + diag_set(OutOfMemory, sizeof(nd), "malloc", > + "box.lib: hash node"); > + return -1; > + } > + return 0; > +} <...> > +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"; > + > + if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) { > + 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); > + const size_t max_sym_len = 512; > + > + if (sym_len < 1) { > + diag_set(IllegalParams, fmt_noname, method); > + return luaT_error(L); > + } else if (sym_len > max_sym_len) { > + diag_set(IllegalParams, "Symbol \'%s\' is too long (max %zd)", > + sym, max_sym_len); > + 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. > + * > + * The key's buffer should be big enough to keep > + * the longest package path plus symbol name and > + * the pointer. > + */ > + char key[PATH_MAX + 2 * max_sym_len]; 3. +3 for the dots you added below. > + snprintf(key, sizeof(key), "%p.%s.%s", > + (void *)m, m->package, sym); 4. Is '(void *)' cast needed? It should work implicitly, no? > + 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; > +}