Re: [Tarantool-patches] [PATCH v4 3/4] crash: move fatal signal handling in

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

>>> +static struct crash_info {
>>> +	/**
>>> +	 * These two are mostly useless as being
>>> +	 * plain addresses but keep for backward
>>> +	 * compatibility.
>>
>> 3. Why don't you say the same about 'siaddr'? It is also
>> just a plain address.
> 
> The other members are exported to report while these two
> are printed in local console only. To be honest I don't
> see any reason in these two members but I kept them to
> not break backward compatibility.

This one also is printed to local console only. So the
question is the same, why don't you call it also a useless
plain address?

>>> +#ifdef ENABLE_BACKTRACE
>>> +	/*
>>
>> 4. We usually use /** in out-of-function comment's
>> first line.
> 
> This comment is not a part of doxygen, I left it this
> way intentionally. This comment for internal use.

So you seriously think everything else should go to doxygen?
Please, lets be consistent. The rule is simple - /** our of
functions, /* - inside. Please, just follow it.

>>> +	 * 4K of memory should be enough to keep the backtrace.
>>> +	 * In worst case it gonna be simply trimmed.
>>> +	 */
>>> +	char backtrace_buf[4096];
>>
>> 5. This is a functional change. Previously for the backtrace
>> we used the static buffer.
>>
>> 1) Why did you change it?
> 
> Because the buffer is used between the calls, iow it filled once
> and then passed to plain report to the console and then to
> json encoding. And keeping data in static buffer between the
> calls is very bad idea, it bounds calls to the context. I'm ready
> to spend 4K per instance for this. We can shrink the value down
> to 1K if you prefer but keeping static buffer between the calls
> definitely is not an option.
> 
>> 2) Why isn't it in a separate commit? As I told you, it is really
>> hard to extract what did you change in a ~460 lines patch titled as
>> 'move' to check if it does not break anything or is even needed.
>> Please, don't make it harder.
> 
> Vlad, I remember this. The problem is that if I would do interdiff
> the result will be simply unreadable (believe me, I tried). This
> is why I sent the whole new patch instead. I reworked the patch
> too much.

I don't know what is 'interdiff'. But I do know what is an atomic
commit. And this commit is not atomic. Also I know when a patch is
easy to follow and easy to review. This one isn't. Because I constantly
need to look for changes you did among hundreds of lines of refactoring.

Please, split the independent changes into separate commits so as they
could be properly reviewed and the changes could be justified in the
commit messages.

>> Also print_backtrace() becomes unused after your patch.
> 
> Not really
> 
> [cyrill@grain tarantool.git] git grep -n print_backtrace
> src/lib/core/backtrace.cc:436:print_backtrace(void)
> src/lib/core/backtrace.cc:449:  print_backtrace();
> src/lib/core/backtrace.h:46:void print_backtrace(void);
> src/lua/init.c:367:     print_backtrace();
> 
> It is still suitable own handler, no? 

Yes, my bad.