From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org> Cc: Mons Anderson <v.perepelitsa@corp.mail.ru> Subject: Re: [Tarantool-patches] [PATCH v3 3/4] crash: use errstat code in fatal signals Date: Sat, 5 Dec 2020 19:33:49 +0100 [thread overview] Message-ID: <43d75b36-ecc6-2722-2130-ccb7aa0ee32e@tarantool.org> (raw) In-Reply-To: <20201204153003.175555-4-gorcunov@gmail.com> 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(); >
next prev parent reply other threads:[~2020-12-05 18:33 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-04 15:29 [Tarantool-patches] [PATCH v3 0/4] crash dump: implement sending feedback Cyrill Gorcunov 2020-12-04 15:30 ` [Tarantool-patches] [PATCH v3 1/4] backtrace: allow to specify destination buffer Cyrill Gorcunov 2020-12-05 18:30 ` Vladislav Shpilevoy 2020-12-05 18:52 ` Cyrill Gorcunov 2020-12-04 15:30 ` [Tarantool-patches] [PATCH v3 2/4] errstat: add crash report base code Cyrill Gorcunov 2020-12-05 18:33 ` Vladislav Shpilevoy 2020-12-05 19:58 ` Cyrill Gorcunov 2020-12-06 15:50 ` Vladislav Shpilevoy 2020-12-04 15:30 ` [Tarantool-patches] [PATCH v3 3/4] crash: use errstat code in fatal signals Cyrill Gorcunov 2020-12-05 18:33 ` Vladislav Shpilevoy [this message] 2020-12-05 20:04 ` Cyrill Gorcunov 2020-12-04 15:30 ` [Tarantool-patches] [PATCH v3 4/4] cfg: configure crash report sending Cyrill Gorcunov 2020-12-05 18:34 ` Vladislav Shpilevoy 2020-12-05 18:30 ` [Tarantool-patches] [PATCH v3 0/4] crash dump: implement sending feedback Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=43d75b36-ecc6-2722-2130-ccb7aa0ee32e@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.perepelitsa@corp.mail.ru \ --subject='Re: [Tarantool-patches] [PATCH v3 3/4] crash: use errstat code in fatal signals' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox