From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 14F6845C304 for ; Sun, 20 Dec 2020 17:48:13 +0300 (MSK) References: <20201210161832.729439-1-gorcunov@gmail.com> <20201210161832.729439-4-gorcunov@gmail.com> <2a91ad22-6cd3-0bdd-78ef-203bd4b48a8d@tarantool.org> <20201215081645.GB983198@grain> From: Vladislav Shpilevoy Message-ID: Date: Sun, 20 Dec 2020 15:48:11 +0100 MIME-Version: 1.0 In-Reply-To: <20201215081645.GB983198@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Cyrill Gorcunov Cc: Mons Anderson , tml >>> +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.