Re: [Tarantool-patches] [PATCH v4 4/4] crash: report crash data to the feedback server

[Cyrill Gorcunov <gorcunov@gmail.com>]

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.