Tarantool development patches archive
 help / color / mirror / Atom feed
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;
 }

  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