From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 EC39845C304 for ; Fri, 11 Dec 2020 12:31:19 +0300 (MSK) References: <20201210161832.729439-1-gorcunov@gmail.com> <20201210161832.729439-4-gorcunov@gmail.com> From: Serge Petrenko Message-ID: Date: Fri, 11 Dec 2020 12:31:18 +0300 MIME-Version: 1.0 In-Reply-To: <20201210161832.729439-4-gorcunov@gmail.com> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v4 3/4] crash: move fatal signal handling in List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Cc: Mons Anderson , Vladislav Shpilevoy 10.12.2020 19:18, Cyrill Gorcunov пишет: > When SIGSEGV or SIGFPE reaches the tarantool we try to gather > all information related to the crash and print it out to the > console (well, stderr actually). Still there is a request > to not just show this info locally but send it out to the > feedback server. > > Thus to keep gathering crash related information in one module, > we move fatal signal handling into the separate crash.c file. > This allows us to collect the data we need in one place and > reuse it when we need to send reports to stderr (and to the > feedback server, which will be implemented in next patch). > > Part-of #5261 > > Signed-off-by: Cyrill Gorcunov Thanks for the patch! Please find a couple of style comments below. Other than that the patch looks good. > --- > src/lib/core/CMakeLists.txt | 1 + > src/lib/core/crash.c | 291 ++++++++++++++++++++++++++++++++++++ > src/lib/core/crash.h | 32 ++++ > src/main.cc | 138 +---------------- > 4 files changed, 329 insertions(+), 133 deletions(-) > create mode 100644 src/lib/core/crash.c > create mode 100644 src/lib/core/crash.h > > diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt > index 13ed1e7ab..06b2b91e1 100644 > --- a/src/lib/core/CMakeLists.txt > +++ b/src/lib/core/CMakeLists.txt > @@ -1,5 +1,6 @@ > set(core_sources > diag.c > + crash.c > say.c > memory.c > clock.c > diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c > new file mode 100644 > index 000000000..9572a023c > --- /dev/null > +++ b/src/lib/core/crash.c > @@ -0,0 +1,291 @@ > +/* > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file. I haven't seen us using such a license before. It must be fine, I'm just not familiar with this. > + */ > + > +#include > +#include > +#include > + > +/** > + * Report crash information to the stderr > + * (usually a current console). > + */ > +static void > +crash_report_stderr(struct crash_info *cinfo) > +{ > + if (cinfo->signo == SIGSEGV) { > + fprintf(stderr, "Segmentation fault\n"); > + const char *signal_code_repr = 0; Please use NULL for pointers. > + > + switch (cinfo->sicode) { > + case SEGV_MAPERR: > + signal_code_repr = "SEGV_MAPERR"; > + break; > + case SEGV_ACCERR: > + signal_code_repr = "SEGV_ACCERR"; > + break; > + } > + > + if (signal_code_repr) Please add `!= NULL` to the condition. -- Serge Petrenko