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

[Cyrill Gorcunov <gorcunov@gmail.com>]

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!