Re: [Tarantool-patches] [PATCH v12 8/8] test: box/cfunc -- add cmod test

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 5 comments below.

> diff --git a/test/box/cfunc2.c b/test/box/cfunc2.c
> new file mode 100644
> index 000000000..f6d826183
> --- /dev/null
> +++ b/test/box/cfunc2.c
> @@ -0,0 +1,132 @@
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <msgpuck.h>
> +
> +#include "module.h"
> +
> +/*
> + * Just make sure we've been called.
> + */
> +int
> +cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> +	(void)ctx;
> +	(void)args;
> +	(void)args_end;
> +	return 0;
> +}
> +
> +/*
> + * Fetch first N even numbers (just to make sure the order of
> + * arguments is not screwed).> + */
> +int
> +cfunc_fetch_evens(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> +	int arg_count = mp_decode_array(&args);
> +	if (arg_count != 1) {
> +		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
> +			"invalid argument count");

1. Broken alignment. Here and in other error places.

> +	}
> +	int field_count = mp_decode_array(&args);
> +	if (field_count < 1) {
> +		return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
> +			"invalid array size");
> +	}
> +
> +	for (int i = 0; i < field_count; i++) {
> +		int val = mp_decode_uint(&args);
> +		int needed = 2 * (i+1);

2. Please, use whitespaces before and after binary operators. I really
suggest you to revisit the code style guide.

Why do you need such a complex function? Why couldn't you simply make a
function like 'echo', which validates msgpack and returns it as is? You
would be able to check that the args are not corrupted, and are exactly
what you have passed.

> +		if (val != needed) {
> +			char res[128];
> +			snprintf(res, sizeof(res), "%s %d != %d",
> +				 "invalid argument", val, needed);
> +			return box_error_set(__FILE__, __LINE__,
> +					     ER_PROC_C, "%s", res);
> +		}
> +	}
> +
> +	return 0;
> +}
> diff --git a/test/box/cmod.result b/test/box/cmod.result
> new file mode 100644
> index 000000000..f8558c191
> --- /dev/null
> +++ b/test/box/cmod.result
> @@ -0,0 +1,199 @@
> +-- test-run result file version 2
> +build_path = os.getenv("BUILDDIR")
> + | ---
> + | ...
> +package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
> + | ---
> + | ...
> +
> +cmod = require('cmod')

3. In the beginning of a test we usually write a comment with the
issue number and its description.

> + | ---
> + | ...
> +fio = require('fio')
> + | ---
> + | ...
> +
> +ext = (jit.os == "OSX" and "dylib" or "so")
> + | ---
> + | ...
> +
> +cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext
> + | ---
> + | ...
> +cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext
> + | ---
> + | ...
> +cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext
> + | ---
> + | ...
> +
> +_ = pcall(fio.unlink(cfunc_path))
> + | ---
> + | ...
> +fio.symlink(cfunc1_path, cfunc_path)
> + | ---
> + | - true
> + | ...
> +
> +module, err = cmod.load('non-existing-cfunc')
> + | ---
> + | ...
> +assert(err ~= nil)
> + | ---
> + | - true
> + | ...
> +
> +--
> +-- They all are sitting in cfunc.so
> +module, err = cmod.load('cfunc')
> + | ---
> + | ...
> +assert(err == nil)
> + | ---
> + | - true
> + | ...
> +
> +cfunc_nop, err = module:load('no-such-func')
> + | ---
> + | ...
> +assert(err ~= nil)
> + | ---
> + | - true
> + | ...
> +cfunc_nop, err = module:load('cfunc_nop')
> + | ---
> + | ...
> +assert(err == nil)
> + | ---
> + | - true
> + | ...
> +cfunc_fetch_evens, err = module:load('cfunc_fetch_evens')
> + | ---
> + | ...
> +assert(err == nil)
> + | ---
> + | - true
> + | ...
> +cfunc_multireturn, err = module:load('cfunc_multireturn')
> + | ---
> + | ...
> +assert(err == nil)
> + | ---
> + | - true
> + | ...
> +cfunc_args, err = module:load('cfunc_args')
> + | ---
> + | ...
> +assert(err == nil)
> + | ---
> + | - true
> + | ...
> +cfunc_sum, err = module:load('cfunc_sum')
> + | ---
> + | ...
> +assert(err == nil)
> + | ---
> + | - true
> + | ...
> +
> +--
> +-- Make sure they all are callable
> +cfunc_nop()
> + | ---
> + | - true
> + | ...
> +cfunc_fetch_evens()
> + | ---
> + | - true
> + | ...
> +cfunc_multireturn()
> + | ---
> + | - true
> + | ...
> +cfunc_args()
> + | ---
> + | - true
> + | ...
> +
> +--
> +-- Clean old function references and reload a new one.
> +_ = pcall(fio.unlink(cfunc_path))
> + | ---
> + | ...
> +fio.symlink(cfunc2_path, cfunc_path)
> + | ---
> + | - true
> + | ...
> +
> +module:reload()
> + | ---
> + | - true
> + | ...
> +
> +cfunc_nop()
> + | ---
> + | - true
> + | ...
> +cfunc_multireturn()

4. I am not sure it is correct. The function wasn't
reloaded. I thought we decided that to get a new
function after reload you need to call load() on the
module again.

Reason for not changing the existing function objects was
that the reload is supposed to happen when the whole
application reloads. It means there can be some fibers
running the old code for a while after reload until they
notice that they must be restarted. If you do reload like
you did in this patch, the old fibers will get new functions
immediately right under their feet, and this may result
into unexpected behaviour for code in these fibers.

Another way to get the desired behaviour - use unload() + load()
- I assume in this case the old function objects will remain
unchanged, right?

I suggest to ask Mons what to do with this. And this is why
I asked for a proper RFC (or GitHub discussion) before starting
to rework the patch.

> + | ---
> + | - true
> + | - [1]
> + | - [1]
> + | ...
> +cfunc_fetch_evens({2,4,6})
> + | ---
> + | - true
> + | ...
> +cfunc_fetch_evens({1,2,3})  -- error
> + | ---
> + | - null
> + | - invalid argument 1 != 2
> + | ...
> +cfunc_args(1, "hello")
> + | ---
> + | - true
> + | - [1, 'hello']
> + | ...
> +cfunc_sum(1) -- error
> + | ---
> + | - null
> + | - invalid argument count
> + | ...
> +cfunc_sum(1,2)
> + | ---
> + | - true
> + | - 3
> + | ...
> +
> +--
> +-- Clean them up
> +cfunc_nop:unload()
> + | ---
> + | - true
> + | ...
> +cfunc_multireturn:unload()
> + | ---
> + | - true
> + | ...
> +cfunc_fetch_evens:unload()
> + | ---
> + | - true
> + | ...
> +cfunc_args:unload()
> + | ---
> + | - true
> + | ...
> +cfunc_sum:unload()
> + | ---
> + | - true
> + | ...
> +module:unload()
> + | ---
> + | - true
> + | ...
> +
> +--
> +-- Cleanup the generated symlink
> +_ = pcall(fio.unlink(cfunc_path))

5. You didn't test GC, multiple unload (should raise an
error), reload when the shared file was deleted, and
that the functions are kept loaded even if the module
was unloaded. Maybe more, but this is just what I thought
of right away.