From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 CF21A445320 for ; Fri, 17 Jul 2020 12:23:55 +0300 (MSK) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) From: "sergos@tarantool.org" In-Reply-To: <20200717083521.GA3979@root> Date: Fri, 17 Jul 2020 12:23:53 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <232C7A81-4104-4DBE-8985-3CBBDB007CC3@tarantool.org> References: <9e6b2ec973a0dd0f2bb6da4915bf1f8f7ff58081.1592235478.git.skaplun@tarantool.org> <20200714154646.GL5559@tarantool.org> <20200715093215.GA947@root> <20200716205331.GP5559@tarantool.org> <20200717083521.GA3979@root> Subject: Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: Alexander Turenko , tarantool-patches@dev.tarantool.org Hi! Thanks for the new patch, LGTM. I support Alexander Turenko in his contribution path: master + all = supported versions. Sergos > On 17 Jul 2020, at 11:35, Sergey Kaplun wrote: >=20 > Igor, >=20 > On 16.07.20, Igor Munkin wrote: >> Sergey, >>=20 >> On 15.07.20, Sergey Kaplun wrote: >=20 > >=20 >>>=20 >>> This bug is hidden at master because since 2.4.1 (commit 345877) we = have >>> additional error handler (luamp_decode_extension_box) that correctly = decode >>> extension type. >>=20 >> So it's not hidden, but handled and the issue doesn't occur since = 2.4.1. >=20 > AFAIK, yes. >=20 >>=20 >>>=20 >>> The main idea of the patch is to cherry-pick commits e476ad1 and = 8ae606a >>> to separate msgpack branch for tarantool-1.10 (only them because we >>> don't want to add a lot of commits to LTS version). Also we should >>> cherry-pick commit 00d770 to latest tarantool 1.10 and after that = add >>> corresponding decoding of extension type. For versions higher than = 1.10 >>> (IIRC) we can just cherry-pick this patch and a test. >>=20 >> As you mentioned above, the issue is gone (or masked, whatever) in = all >> versions above 2.4.1. AFAIR everything except these versions and LTS >> will be deprecated in a few days. So you need to fix the issue only = for >> 1.10 and nothing to be cherry-picked, right? >=20 > Hmm, I suppose that it will be enough to apply this patch only to = 1.10. > In that way we don't need C-implemented test. >=20 >>=20 > >>=20 >> My bad. I tried to reduce the amount of the required libs myself and = get >> the list quite similar to the one you've mentioned within your next >> reply. Does it successfully work for all build modes? >=20 > Yes, it does. >=20 > >=20 >>=20 >> Well, I still don't get the reason for it. You can simply check your >> patch via a tiny Lua chunk for 1.10, otherwise the patch and the test >> are not necessary for the reasons I mentioned above. >=20 > I rewrote test to simple lua chunk (see it below). >=20 >>=20 >> However, if other reviewers insist on the test implemented in C, = please >> adjust it considering the comments I left in my previous reply. >>=20 >=20 > >=20 >>=20 >> --=20 >> Best regards, >> IM >=20 > msgpack: fix wrong mp_ext type in an error message >=20 > While decoding msgpack's extension tarantool raises error as far as it > doesn't support extension types. Error contains message about wrong > extension type with wrong byte (the first instead of the second). >=20 > This patch adds decoding of mp_ext type and inserts it into error > message when error is raised. >=20 > Closes #5017 > --- > src/lua/msgpack.c | 5 +++-- > test/app-tap/msgpack.test.lua | 5 ++++- > 2 files changed, 7 insertions(+), 3 deletions(-) >=20 > diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c > index b7a2955fe..e3935d383 100644 > --- a/src/lua/msgpack.c > +++ b/src/lua/msgpack.c > @@ -224,8 +224,9 @@ = luamp_set_encode_extension(luamp_encode_extension_f handler) > static void > luamp_decode_extension_default(struct lua_State *L, const char **data) > { > - luaL_error(L, "msgpack.decode: unsupported extension: %u", > - (unsigned char) **data); > + int8_t ext_type; > + mp_decode_extl(data, &ext_type); > + luaL_error(L, "msgpack.decode: unsupported extension: %u", = ext_type); > } >=20 >=20 > diff --git a/test/app-tap/msgpack.test.lua = b/test/app-tap/msgpack.test.lua > index 6f7b46a15..1f3904837 100755 > --- a/test/app-tap/msgpack.test.lua > +++ b/test/app-tap/msgpack.test.lua > @@ -36,7 +36,7 @@ local function test_offsets(test, s) > end >=20 > local function test_misc(test, s) > - test:plan(4) > + test:plan(5) > local ffi =3D require('ffi') > local buffer =3D require('buffer') > local buf =3D ffi.cast("const char *", "\x91\x01") > @@ -47,6 +47,9 @@ local function test_misc(test, s) > test:is(buf + 2, bufend, 'ibuf_decode position') > test:is_deeply(result, {1}, "ibuf_decode result") > test:ok(not st and e:match("null"), "null ibuf") > + st, e =3D pcall(s.decode, "\xd4\x0f\x00") > + test:ok(not st and e:match("unsupported extension: 15"), > + "unsupported extension decode") > end >=20 > tap.test("msgpack", function(test) > --=20 > 2.24.1 >=20 > --=20 > Best regards, > Sergey Kaplun