Re: [Tarantool-patches] [PATCH v5 1/2] fiber: use diag_ logger in fiber_madvise/mprotect failures

[Alexander Turenko <alexander.turenko@tarantool.org>]

Everything looks okay expect a couple of minor comments.

WBR, Alexader Turenko.

> fiber: use diag_ logger in fiber_madvise/mprotect failures

We usually try to fit a commit header within 50 symbols (this is however
should not be very strong requirement, IMHO). Say,

 | fiber: set diagnostic at mprotect/madvise failure

> diff --git a/test/box/errinj.result b/test/box/errinj.result
> index babe36b1b..c58e1d2e8 100644
> --- a/test/box/errinj.result
> +++ b/test/box/errinj.result
> @@ -1,1783 +1,1887 @@
> +-- test-run result file version 2

Please, don't change format of the result file. See [1], section 'New
result file format'.

In brief, it means to do

$ (cd test && ./test-run.py box/errinj.test.lua && mv box/errinj.{reject,result})

instead of

$ (cd test && rm box/errinj.result && ./test-run.py box/errinj.test.lua)

I was asked to fix it several times and so I will bring the issue [2]
into QA team attention at Friday's status meeting.

[1]: https://github.com/tarantool/test-run/commit/a04b5b096c607172ce4fc86a84e3531c9f3a7304
[2]: https://github.com/tarantool/test-run/issues/194

> diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
> index 4a57597e9..212a02d36 100644
> --- a/test/unit/CMakeLists.txt
> +++ b/test/unit/CMakeLists.txt
> @@ -74,6 +74,10 @@ add_executable(fiber.test fiber.cc)
>  set_source_files_properties(fiber.cc PROPERTIES COMPILE_FLAGS -O0)
>  target_link_libraries(fiber.test core unit)
>  
> +add_executable(fiber_stack.test fiber_stack.cc)
> +set_source_files_properties(fiber_stack.cc PROPERTIES COMPILE_FLAGS -O0)
> +target_link_libraries(fiber_stack.test core unit)
> +
>  if (NOT ENABLE_GCOV)
>      # This test is known to be broken with GCOV
>      add_executable(guard.test guard.cc)
> diff --git a/test/unit/fiber_stack.cc b/test/unit/fiber_stack.cc
> new file mode 100644
> index 000000000..b402e55d5
> --- /dev/null
> +++ b/test/unit/fiber_stack.cc
> @@ -0,0 +1,83 @@
> +#include "memory.h"
> +#include "fiber.h"
> +#include "unit.h"
> +#include "trivia/util.h"
> +#include "errinj.h"
> +
> +static struct fiber_attr default_attr;
> +
> +static int
> +noop_f(va_list ap)
> +{
> +	return 0;
> +}
> +
> +static void
> +fiber_stack_fail_test(void)
> +{
> +	struct errinj *inj;
> +	struct fiber *fiber;
> +
> +	header();
> +
> +	/*
> +        * Set non-default stack size to prevent reusing of an
> +        * existing fiber.
> +        */

Space/tab indent mix.

> +	struct fiber_attr *fiber_attr = fiber_attr_new();
> +	fiber_attr_setstacksize(fiber_attr, default_attr.stack_size * 2);
> +
> +	/*
> +        * Clear the fiber's diagnostics area to check that failed
> +        * fiber_new() sets an error.
> +        */

Same.

> diff --git a/test/unit/fiber_stack.result b/test/unit/fiber_stack.result
> new file mode 100644
> index 000000000..af10ab4f1
> --- /dev/null
> +++ b/test/unit/fiber_stack.result
> @@ -0,0 +1,6 @@
> +       *** fiber_stack_fail_test ***
> +ok 1 - mprotect: failed to setup fiber guard page
> +ok 2 - mprotect: diag is armed after error
> +ok 3 - madvise: non critical error on madvise hint
> +ok 4 - madvise: diag is armed after error
> +       *** fiber_stack_fail_test: done ***

It fails like so for me:

[001] Test failed! Result content mismatch:
[001] --- unit/fiber_stack.result	Thu Feb  6 10:36:02 2020
[001] +++ unit/fiber_stack.reject	Thu Feb  6 13:29:15 2020
[001] @@ -1,3 +1,4 @@
[001] +SystemError fiber mprotect failed: Cannot allocate memory
[001]  	*** fiber_stack_fail_test ***
[001]  ok 1 - mprotect: failed to setup fiber guard page
[001]  ok 2 - mprotect: diag is armed after error

It seems test-run mixes stdout and stderr for unit tests.

There is the common problem: 'release_disabled' tests are not run at all
for 'core = app' and 'core = unittest' test suites. I filed [3] for
this. It means for now that you should verify your test locally w/o
release_disabled, but then put it to release_disabled back before push.

[3]: https://github.com/tarantool/test-run/issues/199