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 171126EC5B; Sat, 10 Apr 2021 02:31:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 171126EC5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618011071; bh=QuWQNCUim2RRsH7C6kM2iPwtMGudUtLJ5SKOYTX/l3U=; 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=Tokl95Yt65WxR9NUwchIrDyhUXegIOLIl8fOHZKba/a/9JLhinR2ypVDcCAm2384g C8LQD4vf+dz5eTNUymvUaXNRWeGTBYa4KH6AxPM5NuzIt/syY7DL12ZdtInNu24r6B xPGrNYMkwqV7cjVLbSkrxDhfXX1iEQcF96MtPt6Y= 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 BFC1E6EC5B for ; Sat, 10 Apr 2021 02:31:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BFC1E6EC5B Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lV0ah-0005hj-Ng; Sat, 10 Apr 2021 02:31:08 +0300 To: Cyrill Gorcunov , tml References: <20210408164151.1759348-1-gorcunov@gmail.com> <20210408164151.1759348-2-gorcunov@gmail.com> Message-ID: <98549c9b-2c93-684b-2b97-2e1359fa725e@tarantool.org> Date: Sat, 10 Apr 2021 01:31:06 +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-2-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480257C85EA0BB7A95D5E28B957962BB550182A05F538085040E5A0393D6EA08389AD456B1E65AFDD2B27C300CBA0A72F1F9D9BC54227CACC63 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F544D30F1A6FA191EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063795AFAF91F541EBCE8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2E5545BEAA41D48EECBEE6550BBAF404F4A8204C501E24153D2E47CDBA5A96583C09775C1D3CA48CFCA5A41EBD8A3A0199FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3CA5A41EBD8A3A0199FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E7356C9A9530EBF72002C4224003CC83647689D4C264860C145E X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C9783160CF997DC24542930D9CEC0EA6A0B831 X-C1DE0DAB: 0D63561A33F958A53EE41936081B0E4E9EF0A6591AB2B11BFDDFB356F49CB1F7D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3453037B5638665721663B25E2E8395FF93DFD9215D5D0C0CFD77279F16A12E8CF8687A05CBA7D05451D7E09C32AA3244CF0D7F370B2BBF74975DC5E22A74308837C0C08F7987826B9729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojqcJA+pXcDul+Mw8jR7jJoA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822888BB8D649919333909A8E4790D588493841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v21 1/6] 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" Hi! Thanks for working on this! See 7 comments below and my diff in the end of the email. > diff --git a/test/box/func_restore.result b/test/box/func_restore.result > new file mode 100644 > index 000000000..7cd9e67c4 > --- /dev/null > +++ b/test/box/func_restore.result 1. Please, name the file according to the rules of tests related to tickets: 'gh-5968-func-restore.test.lua'. > @@ -0,0 +1,95 @@ > +-- test-run result file version 2 > +-- > +-- Test that compiled C function can be restored 2. There should be a reference to the ticket: 'gh-5968: ...'. > +-- to the former values when new module can't be > +-- loaded for some reason (say there some > +-- missing functions). > +-- > +build_path = os.getenv("BUILDDIR") > + | --- > + | ... > +package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath > + | --- > + | ... > + > +fio = require('fio') > + | --- > + | ... > + > +ext = (jit.os == "OSX" and "dylib" or "so") > + | --- > + | ... > + > +path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext > + | --- > + | ... > +path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext > + | --- > + | ... > + > +_ = pcall(fio.unlink(path_func_restore)) > + | --- > + | ... > +fio.symlink(path_func_good, path_func_restore) > + | --- > + | - true > + | ... > + > +box.schema.func.create('func_restore.echo_1', {language = "C"}) > + | --- > + | ... > +box.schema.func.create('func_restore.echo_2', {language = "C"}) > + | --- > + | ... > +box.schema.func.create('func_restore.echo_3', {language = "C"}) > + | --- > + | ... > + > +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3') > + | --- > + | ... > +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2') > + | --- > + | ... > +box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1') > + | --- > + | ... > + > +box.func['func_restore.echo_3']:call() > + | --- > + | - 3 > + | ... > +box.func['func_restore.echo_2']:call() > + | --- > + | - 2 > + | ... > +box.func['func_restore.echo_1']:call() > + | --- > + | - 1 > + | ... > + > +function run_restore(path) \ > + _ = pcall(fio.unlink(path_func_restore)) \ > + fio.symlink(path, path_func_restore) \ > + \ > + local ok, _ = pcall(box.schema.func.reload, "func_restore") \ > + assert(not ok) \ > + \ > + box.func['func_restore.echo_1']:call() \ > + box.func['func_restore.echo_2']:call() \ > + box.func['func_restore.echo_3']:call() \ 3. It would be good to have assertions here that the functions still return what they used to. And the functions in the bad modules should return something different. So we would see all the functions didn't change and still return good values. > +end > + | --- > + | ... > + > +bad_modules = { \ > + fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \ > + fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \ > + fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \ > +} > + | --- > + | ... > + > +for k, v in ipairs(bad_modules) do run_restore(v) end > + | --- > + | ... 4. You didn't drop the functions. > diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c > new file mode 100644 > index 000000000..891ec0136 > --- /dev/null > +++ b/test/box/func_restore1.c > @@ -0,0 +1,33 @@ > +#include > +#include 5. You don't need stdio and stdbool headers. > +#include > + > +#include "module.h" > + > +static int > +echo_num(box_function_ctx_t *ctx, const char *args, > + const char *args_end, unsigned int num) 6. args and args_end are unused. > +{ > + char res[16]; > + char *end = mp_encode_uint(res, num); > + box_return_mp(ctx, res, end); > + return 0; > +} > + > +int > +echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end) > +{ > + return echo_num(ctx, args, args_end, 1); > +} > + > +int > +echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end) > +{ > + return echo_num(ctx, args, args_end, 2); > +} > + > +int > +echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end) > +{ > + return echo_num(ctx, args, args_end, 3); > +} > diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c > new file mode 100644 > index 000000000..d9d6de8df > --- /dev/null > +++ b/test/box/func_restore2.c > @@ -0,0 +1,27 @@ > +#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; > +} > + > +int > +echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end) > +{ > + return echo_num(ctx, args, args_end, 1); 7. It is better to make these functions return something different from the valid module. For example, make them all return 0. Or even make them crash. See below the diff I suggest you to apply. I didn't rename the test files though. Please, do it locally. ==================== diff --git a/test/box/func_restore.result b/test/box/func_restore.result index 7cd9e67c4..6c3c7aebc 100644 --- a/test/box/func_restore.result +++ b/test/box/func_restore.result @@ -1,14 +1,16 @@ -- test-run result file version 2 -- --- Test that compiled C function can be restored --- to the former values when new module can't be --- loaded for some reason (say there some --- missing functions). +-- gh-5968: test that compiled C function can be restored to the former values +-- when new module can't be loaded for some reason (say there some missing +-- functions). -- build_path = os.getenv("BUILDDIR") | --- | ... -package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath +old_cpath = package.cpath + | --- + | ... +package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..old_cpath | --- | ... @@ -16,14 +18,14 @@ fio = require('fio') | --- | ... -ext = (jit.os == "OSX" and "dylib" or "so") +ext = (jit.os == "OSX" and ".dylib" or ".so") | --- | ... -path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext +path_func_restore = fio.pathjoin(build_path, "test/box/func_restore") .. ext | --- | ... -path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext +path_func_good = fio.pathjoin(build_path, "test/box/func_restore1") .. ext | --- | ... @@ -55,37 +57,36 @@ box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1') | --- | ... -box.func['func_restore.echo_3']:call() +assert(box.func['func_restore.echo_1']:call() == 1) | --- - | - 3 + | - true | ... -box.func['func_restore.echo_2']:call() +assert(box.func['func_restore.echo_2']:call() == 2) | --- - | - 2 + | - true | ... -box.func['func_restore.echo_1']:call() +assert(box.func['func_restore.echo_3']:call() == 3) | --- - | - 1 + | - true | ... -function run_restore(path) \ - _ = pcall(fio.unlink(path_func_restore)) \ - fio.symlink(path, path_func_restore) \ - \ - local ok, _ = pcall(box.schema.func.reload, "func_restore") \ - assert(not ok) \ - \ - box.func['func_restore.echo_1']:call() \ - box.func['func_restore.echo_2']:call() \ - box.func['func_restore.echo_3']:call() \ +function run_restore(path) \ + pcall(fio.unlink(path_func_restore)) \ + fio.symlink(path, path_func_restore) \ + \ + assert(not pcall(box.schema.func.reload, "func_restore")) \ + \ + assert(box.func['func_restore.echo_1']:call() == 1) \ + assert(box.func['func_restore.echo_2']:call() == 2) \ + assert(box.func['func_restore.echo_3']:call() == 3) \ end | --- | ... -bad_modules = { \ - fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \ - fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \ - fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \ +bad_modules = { \ + fio.pathjoin(build_path, "test/box/func_restore2") .. ext, \ + fio.pathjoin(build_path, "test/box/func_restore3") .. ext, \ + fio.pathjoin(build_path, "test/box/func_restore4") .. ext, \ } | --- | ... @@ -93,3 +94,16 @@ bad_modules = { \ for k, v in ipairs(bad_modules) do run_restore(v) end | --- | ... + +box.schema.func.drop('func_restore.echo_1') + | --- + | ... +box.schema.func.drop('func_restore.echo_2') + | --- + | ... +box.schema.func.drop('func_restore.echo_3') + | --- + | ... +package.cpath = old_cpath + | --- + | ... diff --git a/test/box/func_restore.test.lua b/test/box/func_restore.test.lua index 4bd05769d..478f83631 100644 --- a/test/box/func_restore.test.lua +++ b/test/box/func_restore.test.lua @@ -1,18 +1,18 @@ -- --- Test that compiled C function can be restored --- to the former values when new module can't be --- loaded for some reason (say there some --- missing functions). +-- gh-5968: test that compiled C function can be restored to the former values +-- when new module can't be loaded for some reason (say there some missing +-- functions). -- build_path = os.getenv("BUILDDIR") -package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath +old_cpath = package.cpath +package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..old_cpath fio = require('fio') -ext = (jit.os == "OSX" and "dylib" or "so") +ext = (jit.os == "OSX" and ".dylib" or ".so") -path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext -path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext +path_func_restore = fio.pathjoin(build_path, "test/box/func_restore") .. ext +path_func_good = fio.pathjoin(build_path, "test/box/func_restore1") .. ext _ = pcall(fio.unlink(path_func_restore)) fio.symlink(path_func_good, path_func_restore) @@ -25,26 +25,30 @@ box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3') box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2') box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1') -box.func['func_restore.echo_3']:call() -box.func['func_restore.echo_2']:call() -box.func['func_restore.echo_1']:call() - -function run_restore(path) \ - _ = pcall(fio.unlink(path_func_restore)) \ - fio.symlink(path, path_func_restore) \ - \ - local ok, _ = pcall(box.schema.func.reload, "func_restore") \ - assert(not ok) \ - \ - box.func['func_restore.echo_1']:call() \ - box.func['func_restore.echo_2']:call() \ - box.func['func_restore.echo_3']:call() \ +assert(box.func['func_restore.echo_1']:call() == 1) +assert(box.func['func_restore.echo_2']:call() == 2) +assert(box.func['func_restore.echo_3']:call() == 3) + +function run_restore(path) \ + pcall(fio.unlink(path_func_restore)) \ + fio.symlink(path, path_func_restore) \ + \ + assert(not pcall(box.schema.func.reload, "func_restore")) \ + \ + assert(box.func['func_restore.echo_1']:call() == 1) \ + assert(box.func['func_restore.echo_2']:call() == 2) \ + assert(box.func['func_restore.echo_3']:call() == 3) \ end -bad_modules = { \ - fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \ - fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \ - fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \ +bad_modules = { \ + fio.pathjoin(build_path, "test/box/func_restore2") .. ext, \ + fio.pathjoin(build_path, "test/box/func_restore3") .. ext, \ + fio.pathjoin(build_path, "test/box/func_restore4") .. ext, \ } for k, v in ipairs(bad_modules) do run_restore(v) end + +box.schema.func.drop('func_restore.echo_1') +box.schema.func.drop('func_restore.echo_2') +box.schema.func.drop('func_restore.echo_3') +package.cpath = old_cpath diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c index 891ec0136..f2d0aaf78 100644 --- a/test/box/func_restore1.c +++ b/test/box/func_restore1.c @@ -1,33 +1,22 @@ -#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; -} - int echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end) { - return echo_num(ctx, args, args_end, 1); + const char *mp1 = "\x01"; + return box_return_mp(ctx, mp1, mp1 + 1); } int echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end) { - return echo_num(ctx, args, args_end, 2); + const char *mp1 = "\x02"; + return box_return_mp(ctx, mp1, mp1 + 1); } int echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end) { - return echo_num(ctx, args, args_end, 3); + const char *mp1 = "\x03"; + return box_return_mp(ctx, mp1, mp1 + 1); } diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c index d9d6de8df..ef34be12b 100644 --- a/test/box/func_restore2.c +++ b/test/box/func_restore2.c @@ -1,27 +1,16 @@ -#include -#include -#include - +#include "stdlib.h" #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; -} - int echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end) { - return echo_num(ctx, args, args_end, 1); + abort(); + return 0; } int echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end) { - return echo_num(ctx, args, args_end, 2); + abort(); + return 0; } diff --git a/test/box/func_restore3.c b/test/box/func_restore3.c index e38b44400..fd6f6f1e3 100644 --- a/test/box/func_restore3.c +++ b/test/box/func_restore3.c @@ -1,27 +1,16 @@ -#include -#include -#include - +#include "stdlib.h" #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; -} - int echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end) { - return echo_num(ctx, args, args_end, 2); + abort(); + return 0; } int echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end) { - return echo_num(ctx, args, args_end, 3); + abort(); + return 0; } diff --git a/test/box/func_restore4.c b/test/box/func_restore4.c index 4e97ff0a0..2ebd448f0 100644 --- a/test/box/func_restore4.c +++ b/test/box/func_restore4.c @@ -1,27 +1,16 @@ -#include -#include -#include - +#include "stdlib.h" #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; -} - int echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end) { - return echo_num(ctx, args, args_end, 1); + abort(); + return 0; } int echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end) { - return echo_num(ctx, args, args_end, 3); + abort(); + return 0; }