From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id ACD5546970E for ; Thu, 6 Feb 2020 14:09:54 +0300 (MSK) Date: Thu, 6 Feb 2020 14:10:08 +0300 From: Alexander Turenko Message-ID: <20200206111007.a3wszxoquovsjuyk@tkn_work_nb> References: <20200205220624.8764-1-gorcunov@gmail.com> <20200205220624.8764-2-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200205220624.8764-2-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v5 1/2] fiber: use diag_ logger in fiber_madvise/mprotect failures List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml 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