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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.

>  	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();
>