[Tarantool-patches] [PATCH v21 1/6] box/func: fix modules functions restore
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Apr 10 02:31:06 MSK 2021
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;
}
More information about the Tarantool-patches
mailing list