From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: Re: [tarantool-patches] Re: [PATCH v2] xrow: print corrupted rows on decoding error. From: Serge Petrenko In-Reply-To: <20190403091827.qh5rxxpsbrkl35bp@esperanza> Date: Thu, 4 Apr 2019 14:52:25 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <338F2586-287A-4521-A6EE-F9E738FF6701@tarantool.org> References: <20190401130906.31356-1-sergepetrenko@tarantool.org> <20190403091827.qh5rxxpsbrkl35bp@esperanza> To: Vladimir Davydov Cc: tarantool-patches@freelists.org, Kirill Yukhin List-ID: Hi! > 3 =D0=B0=D0=BF=D1=80. 2019 =D0=B3., =D0=B2 12:18, Vladimir Davydov = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0= ): >=20 > On Mon, Apr 01, 2019 at 04:09:06PM +0300, Serge Petrenko wrote: >> Add row hex printing to log on verbose level. This would be useful >> during investigation of errors related to invalid msgpack packet >> arrival. >>=20 >> Here's some example output: >> ``` >> 2019-04-01 16:04:51.629 [31304] iproto V> Corrupted row is: 3D A5 41 = 55 99 18 A8 08 C2 40 2B BA 50 12 F6 C6 0B 27 66 1C >> 2019-04-01 16:04:51.629 [31304] iproto xrow.c:133 E> = ER_INVALID_MSGPACK: Invalid MsgPack - packet header >> ``` >>=20 >> Related to #4040 >> --- >> = https://github.com/tarantool/tarantool/tree/sp/xrow-decode-verbose-err >> https://github.com/tarantool/tarantool/issues/4040 >>=20 >> Changes in v2: >> - make xrow_on_decode_err a macro instead of a >> function to preserve line where error occurs. >> - add a test case. >> - always allocate buffer for hex dump with malloc >> - add logging on all errors related to invalid >> msgpack and missing request field. >>=20 >> src/box/xrow.c | 116 = ++++++++++++++++++++++++++------------ >> test/box/net.box.result | 34 +++++++++++ >> test/box/net.box.test.lua | 16 ++++++ >> 3 files changed, 131 insertions(+), 35 deletions(-) >>=20 >> diff --git a/src/box/xrow.c b/src/box/xrow.c >> index 73d9e739b..fdd15bef3 100644 >> --- a/src/box/xrow.c >> +++ b/src/box/xrow.c >> @@ -88,15 +88,49 @@ mp_decode_vclock(const char **data, struct vclock = *vclock) >> return 0; >> } >>=20 >> +/** >> + * Set diagnostics with an error and log the corrupted row >> + * which caused the error. >> + * Optionally, if log_level is 'verbose' or greater, >> + * dump the corrupted row contents in hex to the log. >> + * >> + * @param what - exception to set. >> + * @param desc_str - error description string. >> + */ >> +#define xrow_on_decode_err(_start, _end, _what, _desc_str) do { = \ >> + diag_set(ClientError, _what, _desc_str); = \ >> + = \ >> + if (!say_log_level_is_enabled(S_VERBOSE)) = \ >> + break; = \ >> + = \ >> + size_t _len =3D 3 * ((const char *)(_end) - (const char = *)(_start) + 1); \ >=20 > Nit: shouldn't +1 be outside the parentheses? Yes, I guess I meant the interval to be inclusive. But since it is not, the =E2=80=98+1=E2=80=99 should be outside. >=20 >> + char *_buf =3D (char *)malloc(_len); = \ >=20 > Nit: we don't usually prefix names with underscores, because this may > conflict with libc internal macro definitions. Ok. Do you want me to fix it? >=20 > Other than that this patch looks good to me. Do we need it in 1.10? Yes, we need it in 1.10.