From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f68.google.com (mail-lf1-f68.google.com [209.85.167.68]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6727C469719 for ; Thu, 13 Feb 2020 11:19:53 +0300 (MSK) Received: by mail-lf1-f68.google.com with SMTP id t23so3616652lfk.6 for ; Thu, 13 Feb 2020 00:19:53 -0800 (PST) Date: Thu, 13 Feb 2020 11:19:50 +0300 From: Cyrill Gorcunov Message-ID: <20200213081950.GM21061@uranus> References: <20200206123114.8010-1-gorcunov@gmail.com> <20200206123114.8010-2-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v6 1/2] fiber: set diagnostics at madvise/mprotect failure List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml 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!