From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f51.google.com (mail-lf1-f51.google.com [209.85.167.51]) (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 107DA45C304 for ; Sun, 20 Dec 2020 21:26:22 +0300 (MSK) Received: by mail-lf1-f51.google.com with SMTP id h22so8879354lfu.2 for ; Sun, 20 Dec 2020 10:26:22 -0800 (PST) Date: Sun, 20 Dec 2020 21:26:20 +0300 From: Cyrill Gorcunov Message-ID: <20201220182620.GD3139@grain> References: <20201210161832.729439-1-gorcunov@gmail.com> <20201210161832.729439-5-gorcunov@gmail.com> <3d201857-f809-c758-297c-e3e896fbf06c@tarantool.org> <20201216111606.GA14556@grain> <20201216203106.GC14556@grain> <96e492a5-3866-91ac-801b-c26c1fce96e2@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <96e492a5-3866-91ac-801b-c26c1fce96e2@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 4/4] crash: report crash data to the feedback server List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: TML On Sun, Dec 20, 2020 at 04:16:46PM +0100, Vladislav Shpilevoy wrote: > I see you didn't push it on the branch. Please, do. Otherwise I can't > validate if it works. OK, I'll make a new branch and address your comments. > See 6 comments below. > > > diff --git a/src/box/box.cc b/src/box/box.cc > > index a8bc3471d..27079fd46 100644 > > --- a/src/box/box.cc > > +++ b/src/box/box.cc > > @@ -1213,6 +1214,21 @@ box_set_prepared_stmt_cache_size(void) > > return 0; > > } > > > > +void > > +box_set_crash_params(void) > > +{ > > + const char *host = cfg_gets("feedback_host"); > > + bool is_enabled_1 = cfg_getb("feedback_enabled"); > > + bool is_enabled_2 = cfg_getb("feedback_crashinfo"); > > + > > + if (host != NULL && strlen(host) >= CRASH_FEEDBACK_HOST_MAX) { > > + tnt_raise(ClientError, ER_CFG, "feedback_host", > > + "the address is too long"); > > 1. Please, don't use exceptions in the new code. We are getting rid > of them, and such changes complicate it in future. Sure! > > + } > > + > > + crash_cfg_set_params(host, is_enabled_1 && is_enabled_2); > > 2. Just 'crash_cfg', please. We have lots of 'cfg' functions for > various modules, and none of them has 'set_params' suffix. Probably > because this is what 'cfg' means anyway. OK > > - char backtrace_buf[4096]; > > + char backtrace_buf[1024]; > > 3. I am begging you. Please. Stop mixing independent changes with > each other. Please. How can I ask otherwise? What can I do to > explain this? How can I help to understand? Sorry Vlad :( > > +#define snprintf_safe(__end, __fmt, ...) \ > > + do { \ > > + size_t size = (char *)(void *)__end - p; \ > > 4. Please, remove the double cast. You can use (char *) as is. Here and > in other places. OK > > + /* > > + * nodename might a sensitive information, skip. > > 5. 'might' what? You missed a verb. ouch, might contain of course > > 6. I think now I understood all these buffer allocs. Except > that I still don't understand why is tail initialized as > > char *tail = &p[SMALL_STATIC_SIZE - 8]; > > instead of just `tail = end;`. yes, redundant, will drop.