[Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

Igor Munkin imun at tarantool.org
Thu Jul 16 23:53:31 MSK 2020 

Sergey,

On 15.07.20, Sergey Kaplun wrote:
> Hi Igor! Thanks for the review!
> 
> On 14.07.20, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! Please consider my comments below.
> > 
> > On 15.06.20, Sergey Kaplun wrote:
> > > This patch adds decoding of mp_ext type and inserts it into error
> > > message when an error is raised
> > 
> > But what was the problem? I can read various binary sequences, but I'm
> > still not good at MessagePack. Could you please describe what exactly
> > was wrong prior to your fix?
> 
> Well, I had to provide more detailed desciption in commit message.
> Our msgpack library doesn't support deconding/encoding of ext types
> until commit 8ae606 (00d770 commit adds this functionality in
> tarantool). As you can see from issue [1] \xd4 or \xd5 are leading to
> the error at decoding.  Nevertheless they are valid msgpack keys that
> mean extension format family [2]. User should see a type of
> unsupported extension as 15 (\xf0).

Great, this part should be polished and added to the commit message.

> 
> 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.

So it's not hidden, but handled and the issue doesn't occur since 2.4.1.

> 
> 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?

> 
> > > 
> > > Closes #5017
> > > ---
> > >  src/lua/msgpack.c        |  5 +++--
> > >  test/unit/CMakeLists.txt |  5 +++++
> > >  test/unit/mplua.c        | 47 ++++++++++++++++++++++++++++++++++++++++
> > >  test/unit/mplua.result   |  5 +++++
> > >  4 files changed, 60 insertions(+), 2 deletions(-)
> > >  create mode 100644 test/unit/mplua.c
> > >  create mode 100644 test/unit/mplua.result
> > > 
> > 
> > <snipped>
> > 
> > > diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
> > > index 6dfb10f46..906422e82 100644
> > > --- a/test/unit/CMakeLists.txt
> > > +++ b/test/unit/CMakeLists.txt
> > > @@ -205,3 +205,8 @@ target_link_libraries(coll.test core unit ${ICU_LIBRARIES} misc)
> > >  
> > >  add_executable(tuple_bigref.test tuple_bigref.c)
> > >  target_link_libraries(tuple_bigref.test tuple unit)
> > > +
> > > +add_executable(mplua.test mplua.c)
> > > +target_link_libraries(mplua.test unit box server core
> > > +    ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES}
> > > +    ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES})
> > 
> > Why should this be done in such complex way? Do your test binary need
> > *all* mentioned libs? I see no usage for libcurl, libreadline, libicu,
> > libyaml in the test. Furthermore, IIRC LuaJIT symbols are provided by
> > libserver archive.
> 
> When I had tried to build my test I found out that lua/msgpack.c requires build
> with tarantool_lua_ibuf (from lua/utils.c).
> 
> | /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld:
> | CMakeFiles/mplua.test.dir/__/__/src/lua/msgpack.c.o: in function
> | `lua_msgpack_encode': msgpack.c:(.text+0x1673): undefined reference to
> | `tarantool_lua_ibuf'
> 
> Building utils requires build with httpc_lua (from lua/httpc.c). And it
> requires curl library.
> 
> | /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld:
> | CMakeFiles/mplua.test.dir/__/__/src/lua/init.c.o:(.data.rel+0x1b8): undefined
> | reference to `httpc_lua'
> 
> And so on. I suppose that building with libserver and its linkage with
> necessary libraries would be less bulky than list all necessary sources
> inside add_executable. It would be nice if you will offer a prettier
> approach.
> 
> Unfortunately libserver doesn't provides necessary symbols, but requires it to
> be built.
> | nm -g src/libserver.a | grep lua_type | sort | uniq
> |                  U lua_type
> |                  U lua_typename
> 
> > 
> > Well, I faced the following linkage error when the test is build in
> > Release mode:
> > | [ 87%] Linking CXX executable mplua.test
> > | /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: ../../src/libcore.a(evio.cc.o): in function `evio_service_bind(evio_service*, char const*)':
> > | evio.cc:(.text+0x9e2): undefined reference to `uri_parse'
> > | collect2: error: ld returned 1 exit status
> 
> Hmm, I build successfully with Debug/RelWithDebInfo/Release mode
> locally. How did you get this error? Did you build from my branch or
> applied diff to master?

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?

> 
> > > diff --git a/test/unit/mplua.c b/test/unit/mplua.c
> > > new file mode 100644
> > > index 000000000..3b7f4de76
> > > --- /dev/null
> > > +++ b/test/unit/mplua.c

<snipped>

> > > +	const char err_msg[] = "msgpack.decode: unsupported extension: 15";
> > > +	ok(memcmp(lua_tostring(L, -1), err_msg, sizeof(err_msg)) == 0,
> > > +	   "mplua_decode: unsupported extension correct type");
> > > +}
> > > +
> > > +
> > 
> > Well, it looks like you've mixed several approaches and it led to the
> > following mess:
> > * You set test plan right in the test case, but check it in the <main>
> >   routine. I guess these calls should be placed within the same
> >   function, otherwise they are hard maintained.
> > * There are lots of tests using header/footer but no one except
> >   <base64.c> uses it right in <main>. I don't know, what these macros
> >   are for, but most of the tests use them in the test case.
> > 
> > The test case is really small and I guess you can move everything right
> > to the <main> as it's done in rlist.c
> 
> Ok, I will move body of the test to the <main> directly.
> 
> > Besides, the main question is why do you use C test instead of writing a
> > small Lua test (similar to the one in the issue)? At least you can avoid
> > all linkage issues.
> 
> As I say above this error is hidden from lua at tarantool >= 2.4.1. So
> I suppose it would be nice to have one test to 1.10 and 2.4.1.

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.

However, if other reviewers insist on the test implemented in C, please
adjust it considering the comments I left in my previous reply.

> 
> > > +int
> > > +main(void)
> > > +{
> > > +	header();
> > > +
> > > +	struct lua_State *L = luaL_newstate();
> > > +	test_luamp_encode_extension_default(L);
> > > +
> > > +	footer();
> > > +	check_plan();
> > 
> > The function should return <int> but returns nothing. All other tests
> > have the following idiom:
> > | return check_plan();
> > 
> 
> It is correct if we are talking about master. But at 1.10 AFAIS test
> adhere to other way (I use for example test/unit/tuple_bigref.c as one
> of the oldest). Should I use approach from master here or I should use
> old code style here?

As we discussed offline, the most of the tests (23/58) in the unit
directory use this idiom (checked on 1.10). So if you finally decide to
leave the test implemented in C, please adjust it to the common idiom.

> 
> > > +}
> > 
> > <snipped>
> > 
> > > -- 
> > > 2.24.1
> > > 
> > 
> > [1]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Include-Syntax.html#Include-Syntax
> > [2]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Invocation.html#Invocation
> > 
> > -- 
> > Best regards,
> > IM
> 
> [1]: https://github.com/tarantool/tarantool/issues/5017
> [2]: https://github.com/msgpack/msgpack/blob/master/spec.md#ext-format-family
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

More information about the Tarantool-patches mailing list