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 3AC436EC59; Tue, 9 Mar 2021 01:51:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3AC436EC59 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1615243908; bh=yoZQvm+v/EiEUAm5nXQbgqn3yWZ82+2y0NKRzwq2V/4=; 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=ft+BUwtdJTe+8R1pJPbp+N6Dj5W2ajs9ehiFeCHE0BG2ga6rgB9eWz6cJpsIveQ8t jSw7fGZglWjNgFxbAkp1fU7HKdaHI6Wg2AzJXdFc9k/w2jL/DRi40la5r5WBTUieWt ixZTIZ3i8qng4wbe0NRQXjeFL03ySbPj4Aqho2lU= 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 2CE586EC59 for ; Tue, 9 Mar 2021 01:51:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2CE586EC59 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lJOj4-0000uz-Gh; Tue, 09 Mar 2021 01:51:47 +0300 To: Cyrill Gorcunov , tml References: <20210301212343.422372-1-gorcunov@gmail.com> <20210301212343.422372-6-gorcunov@gmail.com> Message-ID: <3c03510f-7398-224d-7226-706cac13b0cb@tarantool.org> Date: Mon, 8 Mar 2021 23:51:45 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <20210301212343.422372-6-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9D3134714A9BDB69B7D7089AC2F541D13D72A77FF59123A9B00894C459B0CD1B9CDF369C674F915FA6AEFDACE4FABF359A4066BDE562F54EED4BD60608253162F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73F300A97FDD4E158EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F832FB01FC7F589C8638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C1F4A82545730C7619CCAEB834696C3717A149B9E78E9BF6FA471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23DF004C9065253843057739F23D657EF2B13377AFFFEAFD26923F8577A6DFFEA7CB24F08513AFFAC7993EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B6F6B3B5B76D0D00FF089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E70227F01F88B6EF2528BD9CCCA9EDD067B1EDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2BBE337FB72E923155C0AF1600DCBC20B11E32FFC24AE6A491336C0913C274E25 X-C1DE0DAB: 0D63561A33F958A53F5A3318A37A75C22EF44A472A2A03E8C9702FB89752AE8FD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349A949488F6BF46DD69C8D9E150D8214E442B69DE1E730F3F5622A8BF6015B1F3B61525F69611E17E1D7E09C32AA3244C5402E767EEF090DCF862B76B0FD3DDECC86C126E7119A0FE729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojUqxEkzHTt/jh0+PNdfG/yA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822F99AF1F08DEA75D5122D7D268326402D3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v19 5/6] box/cmod: implement cmod Lua 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, add a changelog file. See 8 comments below. > diff --git a/src/box/lua/cmod.c b/src/box/lua/cmod.c > new file mode 100644 > index 000000000..ea5acd510 > --- /dev/null > +++ b/src/box/lua/cmod.c > @@ -0,0 +1,605 @@ > +/* > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file. > + */ > + > +#include > +#include > +#include 1. You don't need dlfcn.h. I suspect some other headers as well. For instance, eio.h, fcntl.h. Please, remove the not needed headers. > +#include > +#include > + > +#include "box/error.h" > +#include "box/port.h" > +#include "box/func_def.h" > + > +#include "tt_static.h" > + > +#include "assoc.h" > +#include "cmod.h" > +#include "fiber.h" > +#include "module_cache.h" > + > +#include "lua/utils.h" > +#include "libeio/eio.h" > + > +/** > + * Function descriptor. > + */ > +struct cmod_func { > + /** > + * C function to call. > + */ > + struct module_func mf; > + /** 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]; > +}; > + > +/** Function name to cmod_func hash. */ > +static struct mh_strnptr_t *cmod_func_hash = NULL; > + > +/** A type to find a module from an object. */ > +static const char *uname_cmod = "tt_uname_cmod"; > + > +/** A type to find a function from an object. */ > +static const char *uname_func = "tt_uname_cmod_func"; > + > +/** Get data associated with an object. */ > +static void * > +get_udata(struct lua_State *L, const char *uname) > +{ > + void **pptr = luaL_testudata(L, 1, uname); > + return pptr != NULL ? *pptr : NULL; > +} > + > +/** Set data to a new value. */ > +static void > +set_udata(struct lua_State *L, const char *uname, void *ptr) > +{ > + void **pptr = luaL_testudata(L, 1, uname); > + assert(pptr != NULL); > + *pptr = ptr; > +} > + > +/** Setup a new data and associate it with an object. */ > +static void > +new_udata(struct lua_State *L, const char *uname, void *ptr) > +{ > + *(void **)lua_newuserdata(L, sizeof(void *)) = ptr; > + luaL_getmetatable(L, uname); > + lua_setmetatable(L, -2); > +} > + > +/** > + * Helpers for function cache. > + */ > +static void * > +cf_cache_find(const char *str, size_t len) 2. What is 'cf', and why did you use simply cache_find/add/del in other patches but now you start adding prefixes? > +{ > + mh_int_t e = mh_strnptr_find_inp(cmod_func_hash, str, len); > + if (e == mh_end(cmod_func_hash)) > + return NULL; > + return mh_strnptr_node(cmod_func_hash, e)->val; > +} > + > +static int > +cf_cache_add(struct cmod_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(cmod_func_hash, &nd, NULL, NULL); > + if (e == mh_end(cmod_func_hash)) { > + diag_set(OutOfMemory, sizeof(nd), "malloc", > + "cmod: hash node"); > + return -1; > + } > + return 0; > +} > + > +static void > +cache_del(struct cmod_func *cf) > +{ > + mh_int_t e = mh_strnptr_find_inp(cmod_func_hash, > + cf->key, cf->len); > + if (e != mh_end(cmod_func_hash)) > + mh_strnptr_del(cmod_func_hash, e, NULL); > +} > + > +/** > + * Load a module. > + * > + * This function takes a module path from the caller > + * stack @a L and returns cached module instance or > + * creates a new module object. > + * > + * Possible errors: > + * > + * - IllegalParams: module path 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 > +lcmod_load(struct lua_State *L) 3. Why do you use 'struct lua_State' in this patch, and just 'lua_State' in the module_cache patch? > +{ > + const char msg_noname[] = "Expects cmod.load(\'name\') " > + "but no name passed"; > + > + if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) { > + diag_set(IllegalParams, msg_noname); > + return luaT_error(L); > + } > + > + size_t name_len; > + const char *name = lua_tolstring(L, 1, &name_len); > + > + if (name_len < 1) { > + diag_set(IllegalParams, msg_noname); > + return luaT_error(L); > + } > + > + struct module *m = module_load(name, name_len); > + if (m == NULL) > + return luaT_error(L); > + > + new_udata(L, uname_cmod, m); > + return 1; > +} > + > +/** > + * Unload a module. > + * > + * Take a module object from the caller stack @a L and unload it. > + * > + * Possible errors: > + * > + * - IllegalParams: module is not supplied. > + * - IllegalParams: the module is unloaded. > + * > + * @returns true on success or throwns an error. > + */ > +static int > +lcmod_unload(struct lua_State *L) > +{ > + if (lua_gettop(L) != 1) { > + diag_set(IllegalParams, "Expects module:unload()"); > + return luaT_error(L); > + } > + > + struct module *m = get_udata(L, uname_cmod); > + if (m == NULL) { > + diag_set(IllegalParams, "The module is unloaded"); > + return luaT_error(L); > + } > + > + set_udata(L, uname_cmod, NULL); > + module_unload(m); > + lua_pushboolean(L, true); > + return 1; > +} > + > +/** Handle __index request for a module object. */ > +static int > +lcmod_index(struct lua_State *L) > +{ > + lua_getmetatable(L, 1); > + lua_pushvalue(L, 2); > + lua_rawget(L, -2); > + if (!lua_isnil(L, -1)) > + return 1; > + > + struct module *m = get_udata(L, uname_cmod); > + if (m == NULL) { > + lua_pushnil(L); > + return 1; > + } > + > + const char *key = lua_tostring(L, 2); > + if (key == NULL || lua_type(L, 2) != LUA_TSTRING) { > + diag_set(IllegalParams, > + "Bad params, use __index(obj, )"); 4. Nobody ever uses __index explicitly like this. Better tell to use "[key]". > + return luaT_error(L); > + } > + > + if (strcmp(key, "path") == 0) { > + lua_pushstring(L, m->package); > + return 1; > + } > + > + /* > + * Internal keys for debug only, not API. > + */ > + if (strncmp(key, "tt_dev.", 7) == 0) { 5. This is a cool idea about retrieving info for the tests, but we already have ERRINJ_DYN_MODULE_COUNT. It helps to find if the modules are correctly allocated and deleted, which makes not necessary to introduce 'refs'. Talking of 'ptr', you don't need it either, if you can see the module count. You just check the count before and after module load. If it is incremented, a new module was loaded. If it is decremented, a module was unloaded. For testing a single module used both from cmod and box.schema.func you could add a global variable to your C module and see that both APIs work with the same global variable. For instance, add a function which increments it and returns the old value. Then call it both from cmod and box.schema.func, and see it is 2 in the end. Or keep using your hacks, but at least make them easier to use. By forcing to use `.` you make impossible to write the indexes without ['...']. Make it easier, and no need for 'tt' thing. For instance module.debug_refs. func.debug_module_ptr, and so on. ['...'] looks bulky. > + const char *subkey = &key[7]; > + if (strcmp(subkey, "refs") == 0) { > + lua_pushnumber(L, m->refs); > + return 1; > + } else if (strcmp(subkey, "ptr") == 0) { > + const char *s = tt_sprintf("%p", m); > + lua_pushstring(L, s); > + return 1; > + } > + } > + return 0; > +} > + > +/** Module representation for REPL (console). */ > +static int > +lcmod_serialize(struct lua_State *L) > +{ > + struct module *m = get_udata(L, uname_cmod); > + if (m == NULL) { > + lua_pushnil(L); > + return 1; > + } > + > + lua_createtable(L, 0, 1); > + lua_pushstring(L, m->package); > + lua_setfield(L, -2, "path"); > + return 1; > +} > + > +/** Collect a module. */ > +static int > +lcmod_gc(struct lua_State *L) > +{ > + struct module *m = get_udata(L, uname_cmod); > + if (m != NULL) { > + set_udata(L, uname_cmod, NULL); > + module_unload(m); > + } > + return 0; > +} > + > +/** Increase reference to a function. */ > +static void > +cmod_func_ref(struct cmod_func *cf) > +{ > + assert(cf->refs >= 0); > + ++cf->refs; > +} > + > +/** Free function memory. */ > +static void > +cmod_func_delete(struct cmod_func *cf) > +{ 6. Please, add an assertion it does not keep a reference at module * anymore. > + TRASH(cf); > + free(cf); > +} > + > +/** Unreference a function and free if last one. */ > +static void > +cmod_func_unref(struct cmod_func *cf) > +{ > + assert(cf->refs > 0); > + if (--cf->refs == 0) { > + module_func_unload(&cf->mf); > + cache_del(cf); > + cmod_func_delete(cf); > + } > +} > + > +/** Function name from a hash key. */ > +static char * > +cmod_func_name(struct cmod_func *cf) > +{ > + return &cf->key[cf->len - cf->sym_len]; > +} > + > +/** > + * Allocate a new function instance and resolve its address. > + * > + * @param m a module the function should be loaded from. > + * @param key function hash key, ie "addr.module.foo". > + * @param len length of @a key. > + * @param sym_len function symbol name length, ie 3 for "foo". > + * > + * @returns function instance on success, NULL otherwise (diag is set). > + */ > +static struct cmod_func * > +cmod_func_new(struct module *m, const char *key, size_t len, size_t sym_len) > +{ > + size_t size = sizeof(struct cmod_func) + len + 1; > + struct cmod_func *cf = malloc(size); > + if (cf == NULL) { > + diag_set(OutOfMemory, size, "malloc", "cf"); > + return NULL; > + } > + > + cf->len = len; > + cf->sym_len = sym_len; > + cf->refs = 0; > + > + module_func_create(&cf->mf); > + memcpy(cf->key, key, len); > + cf->key[len] = '\0'; > + > + if (module_func_load(m, cmod_func_name(cf), &cf->mf) != 0) { > + diag_set(ClientError, ER_LOAD_FUNCTION, > + cmod_func_name(cf), dlerror()); > + cmod_func_delete(cf); > + return NULL; > + } > + > + if (cf_cache_add(cf) != 0) { > + cmod_func_delete(cf); 7. You didn't unload the function. > + return NULL; > + } > + > + /* > + * Each new function depends on module presence. > + * Module will reside even if been unload > + * explicitly after the function creation. > + */ > + cmod_func_ref(cf); > + return cf; > +} > + > +/** Initialize cmod. */ > +void > +box_lua_cmod_init(struct lua_State *L) > +{ > + cmod_func_hash = mh_strnptr_new(); > + if (cmod_func_hash == NULL) > + panic("cmod: Can't allocate func hash table"); > + > + (void)L; 8. No need to cast to void. It is used. > + static const struct luaL_Reg top_methods[] = { > + { "load", lcmod_load }, > + { NULL, NULL }, > + }; > + luaL_register_module(L, "cmod", top_methods); > + lua_pop(L, 1); > + > + static const struct luaL_Reg lcmod_methods[] = { > + { "unload", lcmod_unload }, > + { "load", lcmod_load_func }, > + { "__index", lcmod_index }, > + { "__serialize", lcmod_serialize }, > + { "__gc", lcmod_gc }, > + { NULL, NULL }, > + }; > + luaL_register_type(L, uname_cmod, lcmod_methods); > + > + static const struct luaL_Reg lfunc_methods[] = { > + { "unload", lfunc_unload }, > + { "__index", lfunc_index }, > + { "__serialize", lfunc_serialize }, > + { "__gc", lfunc_gc }, > + { "__call", lfunc_call }, > + { NULL, NULL }, > + }; > + luaL_register_type(L, uname_func, lfunc_methods); > +}