[Tarantool-patches] [PATCH v4 4/4] crash: report crash data to the feedback server
Cyrill Gorcunov
gorcunov at gmail.com
Sun Dec 20 21:26:20 MSK 2020
On Sun, Dec 20, 2020 at 04:16:46PM +0100, Vladislav Shpilevoy wrote:
> I see you didn't push it on the branch. Please, do. Otherwise I can't
> validate if it works.
OK, I'll make a new branch and address your comments.
> See 6 comments below.
>
> > diff --git a/src/box/box.cc b/src/box/box.cc
> > index a8bc3471d..27079fd46 100644
> > --- a/src/box/box.cc
> > +++ b/src/box/box.cc
> > @@ -1213,6 +1214,21 @@ box_set_prepared_stmt_cache_size(void)
> > return 0;
> > }
> >
> > +void
> > +box_set_crash_params(void)
> > +{
> > + const char *host = cfg_gets("feedback_host");
> > + bool is_enabled_1 = cfg_getb("feedback_enabled");
> > + bool is_enabled_2 = cfg_getb("feedback_crashinfo");
> > +
> > + if (host != NULL && strlen(host) >= CRASH_FEEDBACK_HOST_MAX) {
> > + tnt_raise(ClientError, ER_CFG, "feedback_host",
> > + "the address is too long");
>
> 1. Please, don't use exceptions in the new code. We are getting rid
> of them, and such changes complicate it in future.
Sure!
> > + }
> > +
> > + crash_cfg_set_params(host, is_enabled_1 && is_enabled_2);
>
> 2. Just 'crash_cfg', please. We have lots of 'cfg' functions for
> various modules, and none of them has 'set_params' suffix. Probably
> because this is what 'cfg' means anyway.
OK
> > - char backtrace_buf[4096];
> > + char backtrace_buf[1024];
>
> 3. I am begging you. Please. Stop mixing independent changes with
> each other. Please. How can I ask otherwise? What can I do to
> explain this? How can I help to understand?
Sorry Vlad :(
> > +#define snprintf_safe(__end, __fmt, ...) \
> > + do { \
> > + size_t size = (char *)(void *)__end - p; \
>
> 4. Please, remove the double cast. You can use (char *) as is. Here and
> in other places.
OK
> > + /*
> > + * nodename might a sensitive information, skip.
>
> 5. 'might' what? You missed a verb.
ouch, might contain of course
>
> 6. I think now I understood all these buffer allocs. Except
> that I still don't understand why is tail initialized as
>
> char *tail = &p[SMALL_STATIC_SIZE - 8];
>
> instead of just `tail = end;`.
yes, redundant, will drop.
More information about the Tarantool-patches
mailing list