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 1420245C305 for ; Sat, 5 Dec 2020 21:33:51 +0300 (MSK) References: <20201204153003.175555-1-gorcunov@gmail.com> <20201204153003.175555-4-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: <43d75b36-ecc6-2722-2130-ccb7aa0ee32e@tarantool.org> Date: Sat, 5 Dec 2020 19:33:49 +0100 MIME-Version: 1.0 In-Reply-To: <20201204153003.175555-4-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v3 3/4] crash: use errstat code in fatal signals List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Cc: Mons Anderson Thanks for the patch! See 2 comments below. On 04.12.2020 16:30, Cyrill Gorcunov via Tarantool-patches wrote: > In errstat code we fetch the signal statistic and > generate a backtrace for report. We don't send this > data right now but can reuse this code to not decode > registers and generate backtrace twice. > > Part-of #5261 > > Signed-off-by: Cyrill Gorcunov > --- > src/main.cc | 83 ++++++++++++++++++++++++++++++----------------------- > 1 file changed, 47 insertions(+), 36 deletions(-) > > diff --git a/src/main.cc b/src/main.cc > index 2f48f474c..260b9a0ff 100644 > --- a/src/main.cc > +++ b/src/main.cc> @@ -253,6 +253,10 @@ sig_fatal_cb(int signo, siginfo_t *siginfo, void *context) > } > > in_cb = 1; > + /* > + * Notify errstat engine about the crash. > + */ > + errstat_collect_crash(signo, siginfo, context); 1. So I probably see now. You have collect and send separated, because there is also printing below. In that case I can propose to make it more explicit somehow. Because when I firstly looked at that, it took quite some time to realize, that 'cinfo' variable you obtain above it filled inside errstat_collect_crash. It just is not passed to there. For example, you can delete errstat_get(), and make collect_crash() return struct errstat_crash*, which you will use below for the printing. Another option - move entire signal handler to your module, and then access any internals of it. This option looks the best for me. Having platform-dependent code both in the new module and in main.cc doing basically the same but for stderr vs network looks strange. > if (signo == SIGSEGV) { > fdprintf(fd, "Segmentation fault\n"); > @@ -279,8 +283,8 @@ sig_fatal_cb(int signo, siginfo_t *siginfo, void *context) > fprintf(stderr, " context: %p\n", context); > fprintf(stderr, " siginfo: %p\n", siginfo); > > -#if defined(__linux__) && defined(__amd64) > - dump_x86_64_registers((ucontext_t *)context); > +#ifdef TARGET_OS_LINUX > + dump_registers(cinfo); > #endif > > fdprintf(fd, "Current time: %u\n", (unsigned) time(0)); > @@ -290,8 +294,14 @@ sig_fatal_cb(int signo, siginfo_t *siginfo, void *context) > #ifdef ENABLE_BACKTRACE > fdprintf(fd, "Attempting backtrace... Note: since the server has " > "already crashed, \nthis may fail as well\n"); > - print_backtrace(); > + fdprintf(STDERR_FILENO, "%s", cinfo->backtrace_buf); > #endif > + /* > + * If sending crash report to the feedback server is > + * allowed we won't be generating local core dump but > + * rather try to send data and exit. 2. But why? If as a dump you mean the text printed above, it is still printed. If you mean the dump with the data, your module does not send it, and it is still dumped locally, below. Btw, having errstat_exec_send_crash() with void args makes existence of errstat_get() even stranger. > + */ > + errstat_exec_send_crash(); > end: > /* Try to dump core. */ > memset(&sa, 0, sizeof(sa)); > @@ -815,6 +825,7 @@ main(int argc, char **argv) > title_set_script_name(argv[0]); > } > > + errstat_init(tarantool_bin); > export_syms(); > > random_init(); >