From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f47.google.com (mail-lf1-f47.google.com [209.85.167.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0CB5745C304 for ; Sun, 20 Dec 2020 18:49:17 +0300 (MSK) Received: by mail-lf1-f47.google.com with SMTP id o13so17805974lfr.3 for ; Sun, 20 Dec 2020 07:49:17 -0800 (PST) Date: Sun, 20 Dec 2020 18:49:14 +0300 From: Cyrill Gorcunov Message-ID: <20201220154914.GA3139@grain> References: <20201210161832.729439-1-gorcunov@gmail.com> <20201210161832.729439-4-gorcunov@gmail.com> <2a91ad22-6cd3-0bdd-78ef-203bd4b48a8d@tarantool.org> <20201215081645.GB983198@grain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v4 3/4] crash: move fatal signal handling in List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: Mons Anderson , tml 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?