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

[Cyrill Gorcunov <gorcunov@gmail.com>]

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.

> >>> +#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?

Yes, I thought so.

> Please, lets be consistent. The rule is simple - /** our of
> functions, /* - inside. Please, just follow it.

Sure, will update.

> > 
> >> 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

interdiff shows changes on top of previous commit. the changes were
so intrusive that I posted a new version instead because interdiff
were unreadable.

> 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?