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

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Wed, Dec 23, 2020 at 07:47:44PM +0100, Vladislav Shpilevoy wrote:
> >  
> > +/**
> > + * Mark an environment that we're in crashinfo handling, this
> > + * allows us to escape recursive attempts to send report,
> > + * if the action of sending report is failing itself.
> > + */
> > +static int
> > +crash_mark_env_mode(void)
> 
> 1. You don't need to change 'env'. Because in the new process
> send_crashinfo will be false. Therefore it won't send the crash
> anyway.

Yeah, good catch! Will drop this.

> > +/**
> > + * Zap reserved symbols.
> > + */
> > +static char *
> > +json_zap(char *s)
> 
> 2. Why can't you simply call json_escape(), which we
> already have? Yes, it may add a few more bytes, but it
> is not something super bad. json_escape() returns number
> of written bytes, so you can use it quite easy.

Ah, I didn't know that we have this helper. Thanks, Vlad!

> 
> It would look even simpler, if you would use the existing
> SNPRINT() macros. It returns -1 on fail, but you could patch
> it or introduce SNPRINT_GO which would goto to a given label.
> Or keep return -1, and do it like in sio_socketname_to_buffer() -
> it has a separate function to do all the prints.
> 
> I tried to do that in my diff.
> 
> > +{
> > +	if (s == NULL)
> > +		return NULL;
> > +
> > +	/*
> > +	 * Actually we should escape them but for
> > +	 * now lets just zap without changing source
> > +	 * size.
> > +	 */
> > +	for (size_t i = 0; i < strlen(s); i++) {
> 
> 3. Strlen() is going to be called on each iteration. Please, save
> the result into a variable. Or just use json_escape().

Nope, it won't. gcc is smart enough to move this bb
out of cycle. This is known for decades already. Still
if I gonna use json helper you pointed above I'll not
need this function completely.

> Consider my fixes below and on top of the branch in a separate
> commit. I didn't test them though. So there are likely to be a
> few typos.

Yeah, thanks a lot. Will update and report you.