From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 5 Apr 2019 11:06:08 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v2] xrow: print corrupted rows on decoding error. Message-ID: <20190405080608.uyuunti2tebak5wl@esperanza> References: <20190401130906.31356-1-sergepetrenko@tarantool.org> <20190403091827.qh5rxxpsbrkl35bp@esperanza> <338F2586-287A-4521-A6EE-F9E738FF6701@tarantool.org> <20190404120637.x7s5bulkubcaqweh@esperanza> <20190404145110.7mqil2sf6pmcfkn4@esperanza> <20190405061846.GA24274@chai> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190405061846.GA24274@chai> To: Konstantin Osipov Cc: tarantool-patches@freelists.org, Serge Petrenko , Kirill Yukhin List-ID: On Fri, Apr 05, 2019 at 09:18:46AM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [19/04/04 17:57]: > > Pushed to master, 2.1, 1.10. > > This was fast. The first version of this patch was submitted for review more than two weeks ago, on March 15. We haven't heard from you since then. Though you didn't care. > > First of all, we already have request_str() function, and there is > a good deal of code reuse that can take place. request_str() prints a well-formed request. Here we want to dump corrupted msgpack. > > This comment, basically, says it all. > > But provided you had to write your own implementation: > - why use a macro? So that diag_set() and say() preserve file/line. > - why use malloc? Because a corrupted request can easily be longer than 256 bytes (that's how many bytes fits in tt_static_buf). Since it's a slow path, which is only executed on decode error provided S_VERBOSE log level is enabled, I reckon using malloc() is fine. > - what's wrong with mp_snprint, and if the problem is that it > doesn't properly escape non-printable characters, why not fix it > to escape these or implement a version which does? Because data can be corrupted, e.g. an MP_ARRAY with N items in the header having only M < N items in the body. We can't print such data with mp_snprint.