Re: [Tarantool-patches] [PATCH v3 3/4] crash: use errstat code in fatal signals

[Cyrill Gorcunov <gorcunov@gmail.com>]

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 <gorcunov@gmail.com>
> > ---
> >  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!