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 B8C976EC5D; Mon, 5 Apr 2021 18:47:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B8C976EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617637655; bh=mm/fcrOhGxL8qle6e42QzYUOBc3F1t4wKRyBGAEo/38=; 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=sivrPLoyXAc/TaHGuUfc1qc/mtB2WOX0Kf8GfMuU9ah3L+UfbV3eRZkqJA+VBYDPy aiBHVzcNjQipVJdaiJG9h25rzQwkxj9GiQVazweYXvVuMvmzZkzMb4IG5dowjrcL5g IRO2/64hdDnxq+d+rMcdXPyk2UxkK9R58VlpfnTo= 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 502886EC5D for ; Mon, 5 Apr 2021 18:47:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 502886EC5D Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lTRRt-0004Kj-Qf; Mon, 05 Apr 2021 18:47:34 +0300 To: Cyrill Gorcunov , tml References: <20210402123420.885834-1-gorcunov@gmail.com> <20210402123420.885834-4-gorcunov@gmail.com> Message-ID: <5be31f35-b8f4-9800-6804-29957f7634bf@tarantool.org> Date: Mon, 5 Apr 2021 17:47:32 +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-4-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E3294DD38BE31D7797EF84C856624A09E4809182A05F538085040AD2F0C2BF24B7901474C23B08344AF87B22C2D1EA3832A540ADBFBA59F7DBF11 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7818460412E3A2163EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063767500BC4578134A08638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C16EE06F5A270FE6A654BBB663FC786FFECE128108E3FCA37A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3ED8438A78DFE0A9E117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006370251709C511DE2D0EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A597FB06B262418FEE5DCEACB3056F9839E77522F2DC490499D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344B4608D01B1CB59A662425689E61AD8F2187BE48C806A1BBF19909EBD7227751E1DD739432D4DFA51D7E09C32AA3244CB413C13B163A50A3F671B404E98018EF1DD47778AE04E04D729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojM00ve/f+0ol/x1NbcCCnVg== X-Mailru-Sender: 689FA8AB762F73936BC43F508A06382223DF14B627F641983038DCBF6D72482D3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore 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: > In commit 96938fafb an ability to hot reload of modules 1. Please, add the commit title in parentheses and quotes after the hash value. > has been introduced. When module is been reloaded his > functions are resolved to new symbols but if something > went wrong it is supposed to restore old symbols from > the old module. Actually current code restores only > one function and may crash if there a bunch of functions > to restore. Lets fix it. > > Part-of #4642 2. How is it a part of 4642? It is totally unrelated. It is a separate bug, existing before 4642, and which could exist after 4642 without this patch, and which does not block 4642 anyhow. > diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md > new file mode 100644 > index 000000000..7e189617f > --- /dev/null > +++ b/changelogs/unreleased/fix-module-reload.md > @@ -0,0 +1,4 @@ > +## bugfix/core > + > +* Fix module reloading procedure which may crash in case if > + new module is corrupted (gh-4642). 3. 'module' term is used not only for .so/.dylib files, but also for Lua modules. You need to be more specific that this is about compiled files. > diff --git a/src/box/func.c b/src/box/func.c > index 233696a4f..1cd7073de 100644 > --- a/src/box/func.c > +++ b/src/box/func.c > @@ -402,10 +403,10 @@ module_reload(const char *package, const char *package_end) > return 0; > restore: > /* > - * Some old-dso func can't be load from new module, restore old > - * functions. > + * Some old functions are not found int the new module, 4. int -> in. > + * thus restore all migrated functions back to original. > */ > diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt > index 06bfbbe9d..944831af2 100644 > --- a/test/box/CMakeLists.txt > +++ b/test/box/CMakeLists.txt > @@ -2,4 +2,6 @@ include_directories(${MSGPUCK_INCLUDE_DIRS}) > build_module(function1 function1.c) > build_module(reload1 reload1.c) > build_module(reload2 reload2.c) > +build_module(func_restore1 func_restore1.c) > +build_module(func_restore2 func_restore2.c) > build_module(tuple_bench tuple_bench.c) > diff --git a/test/box/func_restore.result b/test/box/func_restore.result 5. The test also passes if I just replace rlist_foreach_entry_safe with rlist_foreach_entry_safe_reverse in the original code. Which means it won't test anything in case we ever change how do we put the functions to the list, or how we walk the list on reload. I propose you to make the test harder to bypass. > diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c > new file mode 100644 > index 000000000..ffd69fd2b > --- /dev/null > +++ b/test/box/func_restore1.c > @@ -0,0 +1,34 @@ > +#include > +#include > +#include > + > +#include "module.h" > + > +static int > +echo_num(box_function_ctx_t *ctx, const char *args, > + const char *args_end, unsigned int num) > +{ > + char res[16]; > + char *end = mp_encode_uint(res, num); > + box_return_mp(ctx, res, end); > + return 0; > +} > + > + 6. Between functions we use a single empty line. The same for the other .c file.