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 E6B806EC5D; Mon, 5 Apr 2021 18:56:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E6B806EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617638213; bh=3+E412GICYhuxRpaO/B6fVZpVCQur+dWGVgChmGNJqk=; 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=nakSETsJ89KMy/9wnnxJ/0UyMFh31H2FDaD2ogGHFIMIb9lgJc0P61bzco3LBYimx +4CTL/yo5g6Mioc166nyqIpn3fZlmlsyd2hrcuEdhCqT1AVh2hqhLzcsTVJzDcknOK KyVJPpNEkV5oYBkTwzm0/E0we8wenSzMR7lLzXAM= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 40A906EC5D for ; Mon, 5 Apr 2021 18:56:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 40A906EC5D Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lTRas-0000CV-Ka; Mon, 05 Apr 2021 18:56:51 +0300 To: Cyrill Gorcunov , tml References: <20210402123420.885834-1-gorcunov@gmail.com> <20210402123420.885834-6-gorcunov@gmail.com> Message-ID: <31095f87-03f7-f7b4-6e4c-1e331dbe7ee8@tarantool.org> Date: Mon, 5 Apr 2021 17:56:49 +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-6-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E3294DD38BE31D7797EF84C856624A09E4809182A05F538085040ABC4D981C35859616350DEAE1E3288B19380E0575F3892A8C57603342359E887 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE788AD3E9728F968ABEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D70459434292EC88638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C16EE06F5A270FE6A2A54C4EB911EAC0F61523686073059B1A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE7328B01A8D746D8839FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3CF36E64A7E3F8E58117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006373BC478629CBEC79DEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C97831AB990DD67A192DAEEE3E100BA2433E01 X-C1DE0DAB: 0D63561A33F958A5B168283221B01ABBDF538263D2A752F6F32FB9F568FACE31D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3442AF01057B6BF9971E5E7B5C8A623246603B883CEF39330DF17696F771CFFBA43B1F70EAAB47A3E41D7E09C32AA3244CBDBBA68C6A2C9617BBD4DE63C8BBB135E3D93501275E802F729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojM00ve/f+0omZrHrMSbroAw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638222FE78F950D3E19F11308C93CFAD14D573841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v20 5/7] box/schema.func: switch to new module api 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 6 comments below. On 02.04.2021 14:34, Cyrill Gorcunov via Tarantool-patches wrote: > Since we have low level module api now we can switch > the box.schema.func code to use it. In particular we > define schema_module structure as a wrapper over the > modules api -- it carries a pointer to general module > structure. > > Because of modules reload functionality (which was actually > a big mistake to introduce in a first place, because it is too > fragile and in case of unintended misuse may simply crash the > application in a best case) the schema modules carry own cache > of modules instances. Thus to make overall code somehow close > to modules api we do: > > 1) every schema_module variable should be named as `mod`. this > allows to distinguish this kind of modules from general > `module` variables; 1. For base objects we often use name 'base'. So the module_cache module would be called 'base', and the schema_module would be called 'module' like it was before your patch. 'mod' and 'module' don't give any hint how are they different, since you want to separate them so hard everywhere. > 2) cache operations are renamed to cache_find/put/update/del; > 3) C functions are switched to use module_func low level api; > 4) because low level modules api carry own references we can > take no explicit reference when calling a function. > > diff --git a/src/box/func.c b/src/box/func.c > index 08918e6db..54d0cbc68 100644 > --- a/src/box/func.c > +++ b/src/box/func.c > @@ -227,190 +169,214 @@ module_cache_put(struct module *module) <...> > +static struct schema_module * > +__schema_module_load(const char *name, size_t len, bool force) 2. You can't use the double underscore unless it is absolutely necessary and can't be avoided. https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html > +{ > + struct schema_module *mod = malloc(sizeof(*mod)); > + if (mod != NULL) { > + mod->module = NULL; > + mod->refs = 0; > + rlist_create(&mod->funcs); > + } else { > + diag_set(OutOfMemory, sizeof(*mod), > + "malloc", "struct schema_module"); > + return NULL; > } > > - module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL); > - if (unlink(load_name) != 0) > - say_warn("failed to unlink dso link %s", load_name); > - if (rmdir(dir_name) != 0) > - say_warn("failed to delete temporary dir %s", dir_name); > - if (module->handle == NULL) { > - diag_set(ClientError, ER_LOAD_MODULE, package_len, > - package, dlerror()); > - goto error; > + if (force) > + mod->module = module_load_force(name, len); > + else > + mod->module = module_load(name, len); > + > + if (!mod->module) { 3. Please, use explicit == NULL. > + free(mod); > + return NULL; > } <...> > +static int > +schema_func_c_load(struct schema_module *mod, const char *func_name, > + struct func_c *func) 4. There is a simple rule for method naming and the order of their arguments: always start the method name with the struct name, and pass the object in the first argument, like C++. Here it should be func_c_load_from(), func_c_unload(), and 'func' must be the first argument. func_c_load_from() so as to emphasize it takes the module to load from explicitly, on the contrary with func_c_load(). Using the prefix 'schema' for that does not make it easier to understand the difference between schema_func_c_load and func_c_load. > { > - box_function_f f = (box_function_f)dlsym(module->handle, name); > - if (f == NULL) { > - diag_set(ClientError, ER_LOAD_FUNCTION, name, dlerror()); > - return NULL; > + assert(module_func_is_empty(&func->mf)); > + > + if (module_func_load(mod->module, func_name, &func->mf) != 0) > + return -1; > + > + func->mod = mod; > + rlist_move(&mod->funcs, &func->item); > + schema_module_ref(mod); > + > + return 0; > +} > + > +static void > +schema_func_c_unload(struct func_c *func) > +{ > + if (!module_func_is_empty(&func->mf)) { > + rlist_del(&func->item); > + schema_module_unref(func->mod); > + module_func_unload(&func->mf); > + func_c_create(func); > } > - return f; > } > > int > schema_module_reload(const char *package, const char *package_end) > { > - struct module *old_module = module_cache_find(package, package_end); > - if (old_module == NULL) { > + struct schema_module *old = cache_find(package, package_end); > + if (old == NULL) { > /* Module wasn't loaded - do nothing. */ > diag_set(ClientError, ER_NO_SUCH_MODULE, package); > return -1; > } > > - struct module *new_module = module_load(package, package_end); > - if (new_module == NULL) > + struct schema_module *new = schema_module_load_force(package, package_end); > + if (new == NULL) > return -1; > > + /* > + * Keep an extra reference to the old module > + * so it won't be freed until reload is complete, > + * otherwise we might free old module then fail > + * on some function loading and in result won't > + * be able to restore old symbols. > + */ > + schema_module_ref(old); > struct func_c *func, *tmp_func; > - rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) { > - /* Move immediately for restore sake. */ > - rlist_move(&new_module->funcs, &func->item); > + rlist_foreach_entry_safe(func, &old->funcs, item, tmp_func) { > struct func_name name; > func_split_name(func->base.def->name, &name); > - func->func = module_sym(new_module, name.sym); > - if (func->func == NULL) > + schema_func_c_unload(func); > + if (schema_func_c_load(new, name.sym, func) != 0) { > + /* > + * WARN: A hack accessing new->funcs directly > + * to start restore from this failing function. > + */ 5. Please, drop the hack. You can restore this function individually right here, and proceed to the restoration of the other functions. There might be other solutions, but this looks the easiest. > + rlist_move(&new->funcs, &func->item); > goto restore; > - func->module = new_module; > + } > } > diff --git a/test/box/gh-4648-func-load-unload.result b/test/box/gh-4648-func-load-unload.result > index e695dd365..91b78d083 100644 > --- a/test/box/gh-4648-func-load-unload.result > +++ b/test/box/gh-4648-func-load-unload.result > @@ -71,7 +71,8 @@ check_module_count_diff(-1) > | ... > > -- A not finished invocation of a function from a module prevents > --- its unload. Until the call is finished. > +-- low level module intance unload while schema level module is > +-- free to unload immediately when dropped. 6. As you can see from the issue description in this test, it was exactly about unloading the dlopen's handle. Not about the module object deletion. After your change, even if the modules are never truly unloaded, the test will pass which is obviously not what is supposed to happen. You need to move the error injection to module_cache.c to make it work like before and fail if the module wasn't fully unloaded.