From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 4435E445320 for ; Fri, 17 Jul 2020 13:22:28 +0300 (MSK) Date: Fri, 17 Jul 2020 13:12:10 +0300 From: Igor Munkin Message-ID: <20200717101210.GE18920@tarantool.org> References: <9e6b2ec973a0dd0f2bb6da4915bf1f8f7ff58081.1592235478.git.skaplun@tarantool.org> <20200714154646.GL5559@tarantool.org> <20200715093215.GA947@root> <20200716205331.GP5559@tarantool.org> <20200717083521.GA3979@root> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <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: tarantool-patches@dev.tarantool.org, Alexander Turenko Sergey, Thanks for the fixes you made! The patch LGTM now, except a single nit regarding the commit message. On 17.07.20, Sergey Kaplun wrote: > Igor, > > On 16.07.20, Igor Munkin wrote: > > Sergey, > > > > On 15.07.20, Sergey Kaplun wrote: > > > > > > 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. > > > > 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? > > 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. I see other reviewers have mentioned in their replies that it's worth to apply the patch for all supported versions. Since the output differs, please create two different branches to ease the merge procedure: one to be applied to LTS branch and another one -- for bleeding and active 2.x stable branches. AFAICS, only the error message need to be adjusted. > > > msgpack: fix wrong mp_ext type in an error message > > 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). > > This patch adds decoding of mp_ext type and inserts it into error > message when error is raised. I reworded the commit message to make it a bit clearer: | Since Tarantool doesn't support msgpack extension types, the error | occurs while its decoding. The error message is misleading, since | the wrong byte is used for representing the extension type. | | The patch adds mp_ext type decoding and the correct type value is used | in the message when the error is raised. Feel free to change it on your own. > > Closes #5017 > --- > src/lua/msgpack.c | 5 +++-- > test/app-tap/msgpack.test.lua | 5 ++++- > 2 files changed, 7 insertions(+), 3 deletions(-) > > -- > 2.24.1 > > -- > Best regards, > Sergey Kaplun -- Best regards, IM