[Tarantool-patches] [PATCH v6 1/2] fiber: set diagnostics at madvise/mprotect failure

[Cyrill Gorcunov]

On Thu, Feb 13, 2020 at 01:08:18AM +0100, Vladislav Shpilevoy wrote:
> > +			 *
> > +			 * Note that in case if we're called from
> > +			 * fiber_stack_create() the @mprotect_flags is
> 
> 1. Nit: we use Doxygen comment style, so any word right after @ is
> treated as a Doxygen command. If you want to mention some parameter,
> use command @a. Like this: '@a mprotect_flags'.

OK

> > +			 * the same as the slab been created with, so
> > +			 * calling mprotect for VMA with same flags
> > +			 * won't fail.
> > +			 */
> 
> 2. Yeah, good idea. We can do it not only in background though, but
> do similar to pthread and how it manages stacks to free. We can put
> stacks into a list, and try to free something from that list on each
> next creation of a fiber. Or even reuse right away, without freeing.

Nod, this is a task for near future.

> > diff --git a/test/box/errinj.result b/test/box/errinj.result
> > index babe36b1b..16f6b40c2 100644
> > --- a/test/box/errinj.result
> > +++ b/test/box/errinj.result
> 
> 3. So big diff for such a small change. I propose to fix this. We
> can sort result of errinj.info before printing it. It would make all
> changes to errinj produce much smaller diff. The patch should be
> trivial. Could you please create it?

Wait, first I don't really understand what you mean by "sort". The
output is in yaml format, you propose to sort nodes by names?

> > diff --git a/test/unit/fiber_stack.cc b/test/unit/fiber_stack.cc
> > new file mode 100644
> > index 000000000..de7fe90e3
> > --- /dev/null
> > +++ b/test/unit/fiber_stack.cc
> 
> 4. Do you need C++ here?

The C++ was choosen because we already have fiber_XXX.cc tests
so I simply tried to follow.

> 5. I would call it fiber_errinj.c. In future we may want to
> add more tests here, which use error injections. Up to you.

>From one point of view having one big test which would cover
various aspects of tarantool might help discovering unexpectedly
connected bugs. On the other hands having a set of small tests
seems to be more neat in terms of their managing. I must confess
I don't see strong preferrence here.

> > +#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();
> 
> 6. After header() there should be a plan() call saying how
> many tests you are going to do. In the end there should be
> check_plan() call, which prints a summary. Could you,
> please, add them?

Will take a look, thanks!

> > +
> > +static int
> > +main_f(va_list ap)
> > +{
> 
> 7. Main caller of test functions usually also has header(),
> plan(), check_plan(), and footer(). This is useful when
> you have many tests, and each has lots of cases.
> 
> Here you have only one test, so far. Probably these calls
> can be omitted in main_f(). Up to you.

OK, I'll review, thanks!