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

[Vladislav Shpilevoy]

On 20.12.2020 16:49, Cyrill Gorcunov wrote:
> On Sun, Dec 20, 2020 at 03:48:11PM +0100, Vladislav Shpilevoy wrote:
>>>>> +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?
> 
> Because context_addr and siginfo_addr comes from a signal frame
> and they are uselesss if you have no real crash dump under your
> hands. You simply cant do anything with them and I don't understand
> why we're printing them and who needs them.
> 
> In turn siaddr points to the memory caused the signal to trigger
> and this address a way more suitable for understanding what
> is happeneing than context and siginfo.
> 
> I can simply drop the comment if you dont like it.

No. I am trying to understand why one plain address matters,
and the others are not. But now I think I understood, thanks.
Keep the comment.

>> 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.
> 
> Currently the commit is big enough because I jump into signal handling
> in one pass. If you prefer I can split the series in more small changes.
> Or you mean the updates over previsous series should be changes in small
> steps as a separate commits?

No, I don't mean commits with review fixes. I mean the patch commits.
Currently you have a patch that moves signal handling to crash.c AND it
also changes backtrace to use a 4KB buffer instead of the static buffer.
These 2 changes (move and backtrace buffer source change) are not related.
Therefore it is not correct to put them into 1 commit. It does not matter
in which order you will do these 2 changes, but one should be about move
only, and the other one about backtrace buffer update only.