From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore Date: Sat, 10 Apr 2021 01:31:06 +0200 [thread overview] Message-ID: <98549c9b-2c93-684b-2b97-2e1359fa725e@tarantool.org> (raw) In-Reply-To: <20210408164151.1759348-2-gorcunov@gmail.com> 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 <stdio.h> > +#include <stdbool.h> 5. You don't need stdio and stdbool headers. > +#include <msgpuck.h> > + > +#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 <stdio.h> > +#include <stdbool.h> > +#include <msgpuck.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); 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 <stdio.h> -#include <stdbool.h> -#include <msgpuck.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); + 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 <stdio.h> -#include <stdbool.h> -#include <msgpuck.h> - +#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 <stdio.h> -#include <stdbool.h> -#include <msgpuck.h> - +#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 <stdio.h> -#include <stdbool.h> -#include <msgpuck.h> - +#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; }
next prev parent reply other threads:[~2021-04-09 23:31 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-08 16:41 [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches 2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore Cyrill Gorcunov via Tarantool-patches 2021-04-09 23:31 ` Vladislav Shpilevoy via Tarantool-patches [this message] 2021-04-10 15:02 ` Cyrill Gorcunov via Tarantool-patches 2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 2/6] box/func: module_reload -- drop redundant argument Cyrill Gorcunov via Tarantool-patches 2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 3/6] box/module_cache: introduce modules subsystem Cyrill Gorcunov via Tarantool-patches 2021-04-09 23:54 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-10 14:59 ` Cyrill Gorcunov via Tarantool-patches 2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 4/6] box/schema.func: switch to new module api Cyrill Gorcunov via Tarantool-patches 2021-04-09 23:55 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-10 15:00 ` Cyrill Gorcunov via Tarantool-patches 2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module Cyrill Gorcunov via Tarantool-patches 2021-04-11 15:38 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-11 22:38 ` Cyrill Gorcunov via Tarantool-patches 2021-04-12 22:08 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-12 22:34 ` Cyrill Gorcunov via Tarantool-patches 2021-04-08 16:41 ` [Tarantool-patches] [PATCH v21 6/6] test: add box.lib test Cyrill Gorcunov via Tarantool-patches 2021-04-08 18:53 ` Cyrill Gorcunov via Tarantool-patches 2021-04-11 15:43 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-11 21:56 ` Cyrill Gorcunov via Tarantool-patches 2021-04-11 22:36 ` Cyrill Gorcunov via Tarantool-patches 2021-04-12 22:08 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-13 7:10 ` Cyrill Gorcunov via Tarantool-patches 2021-04-13 21:53 ` [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module Vladislav Shpilevoy via Tarantool-patches 2021-04-14 8:07 ` Kirill Yukhin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=98549c9b-2c93-684b-2b97-2e1359fa725e@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox