From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) (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 BCBC84765E0 for ; Wed, 23 Dec 2020 21:58:01 +0300 (MSK) Received: by mail-lf1-f43.google.com with SMTP id o19so42456581lfo.1 for ; Wed, 23 Dec 2020 10:58:01 -0800 (PST) Date: Wed, 23 Dec 2020 21:57:57 +0300 From: Cyrill Gorcunov Message-ID: <20201223185757.GF8261@grain> References: <20201223154155.234884-1-gorcunov@gmail.com> <20201223154155.234884-5-gorcunov@gmail.com> <30a781d7-4f4e-3543-f67e-2fb1f7cce172@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <30a781d7-4f4e-3543-f67e-2fb1f7cce172@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v5 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 Wed, Dec 23, 2020 at 07:47:44PM +0100, Vladislav Shpilevoy wrote: > > > > +/** > > + * Mark an environment that we're in crashinfo handling, this > > + * allows us to escape recursive attempts to send report, > > + * if the action of sending report is failing itself. > > + */ > > +static int > > +crash_mark_env_mode(void) > > 1. You don't need to change 'env'. Because in the new process > send_crashinfo will be false. Therefore it won't send the crash > anyway. Yeah, good catch! Will drop this. > > +/** > > + * Zap reserved symbols. > > + */ > > +static char * > > +json_zap(char *s) > > 2. Why can't you simply call json_escape(), which we > already have? Yes, it may add a few more bytes, but it > is not something super bad. json_escape() returns number > of written bytes, so you can use it quite easy. Ah, I didn't know that we have this helper. Thanks, Vlad! > > It would look even simpler, if you would use the existing > SNPRINT() macros. It returns -1 on fail, but you could patch > it or introduce SNPRINT_GO which would goto to a given label. > Or keep return -1, and do it like in sio_socketname_to_buffer() - > it has a separate function to do all the prints. > > I tried to do that in my diff. > > > +{ > > + if (s == NULL) > > + return NULL; > > + > > + /* > > + * Actually we should escape them but for > > + * now lets just zap without changing source > > + * size. > > + */ > > + for (size_t i = 0; i < strlen(s); i++) { > > 3. Strlen() is going to be called on each iteration. Please, save > the result into a variable. Or just use json_escape(). Nope, it won't. gcc is smart enough to move this bb out of cycle. This is known for decades already. Still if I gonna use json helper you pointed above I'll not need this function completely. > Consider my fixes below and on top of the branch in a separate > commit. I didn't test them though. So there are likely to be a > few typos. Yeah, thanks a lot. Will update and report you.