From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) (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 ADCF145C305 for ; Sat, 5 Dec 2020 23:04:14 +0300 (MSK) Received: by mail-lf1-f66.google.com with SMTP id r24so12436095lfm.8 for ; Sat, 05 Dec 2020 12:04:14 -0800 (PST) Date: Sat, 5 Dec 2020 23:04:11 +0300 From: Cyrill Gorcunov Message-ID: <20201205200411.GD2303@grain> References: <20201204153003.175555-1-gorcunov@gmail.com> <20201204153003.175555-4-gorcunov@gmail.com> <43d75b36-ecc6-2722-2130-ccb7aa0ee32e@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <43d75b36-ecc6-2722-2130-ccb7aa0ee32e@tarantool.org> 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: Vladislav Shpilevoy Cc: Mons Anderson , tml On Sat, Dec 05, 2020 at 07:33:49PM +0100, Vladislav Shpilevoy wrote: > 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. Thanks! I'll try to implement. Looks reasonable to me. > > 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. Actually this comment left from previous version which I didn't send. There I was simply using execve, instead of vfork+execve. Thus once execve passed no more local abort() call happened. Thanks for pointing, need to drop this comment. > Btw, having errstat_exec_send_crash() with void args makes existence > of errstat_get() even stranger. Indeed. I'll think. Thanks for all your comments!