From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 1FA9B445320 for ; Wed, 15 Jul 2020 12:32:26 +0300 (MSK) Date: Wed, 15 Jul 2020 12:32:15 +0300 From: Sergey Kaplun Message-ID: <20200715093215.GA947@root> References: <9e6b2ec973a0dd0f2bb6da4915bf1f8f7ff58081.1592235478.git.skaplun@tarantool.org> <20200714154646.GL5559@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200714154646.GL5559@tarantool.org> 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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org, Alexander Turenko 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). 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. 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. > > > > 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 > > > > > > > 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? > > 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 > > @@ -0,0 +1,47 @@ > > +#include /* lua_*() */ > > +#include /* luaL_*() */ > > Minor: Since we use bundled LuaJIT you need to lookup "local" headers > prior to the system-wide ones[1]. But I've run make with more verbose > output and seen we don't use "-I-" in preprocessor flags[2], so I see no > difference how to specify such includes (IMHO it's not the proper way): > | [100%] Building C object test/unit/CMakeFiles/mplua.test.dir/mplua.c.o > | cd /tarantool/test/unit && /usr/bin/cc -DCORO_ASM > | -DLUAJIT_SMART_STRINGS=1 -DLUA_USE_APICHECK=1 -DLUA_USE_ASSERT=1 > | -DNVALGRIND=1 -DYAML_DECLARE_STATIC -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE > | -D__STDC_CONSTANT_MACROS=1 -D__STDC_FORMAT_MACROS=1 > | -D__STDC_LIMIT_MACROS=1 -I/tarantool/src -I/tarantool/src/lib > | -I/tarantool/src/lib/small -I/tarantool/src/lib/small/third_party > | -I/tarantool -I/tarantool/third_party/zstd/lib > | -I/tarantool/third_party/zstd/lib/common > | -I/tarantool/third_party/luajit/src -I/tarantool/src/lib/msgpuck > | -I/tarantool/src/box -I/tarantool/third_party > | -I/tarantool/build/curl/dest/include > | -I/tarantool/third_party/libyaml/include -fexceptions -funwind-tables > | -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2 > | -std=c11 -Wall -Wextra -Wno-strict-aliasing -Wno-char-subscripts > | -Wno-format-truncation -fno-gnu89-inline -Wno-cast-function-type -Werror > | -Wno-unused-parameter -Wno-unused -Wno-unused-result > | -Wno-tautological-compare -g -ggdb -O0 -o > | CMakeFiles/mplua.test.dir/mplua.c.o -c /tarantool/test/unit/mplua.c It's very important comment! Of course I should use "" here. Thanks! > > +#include /* luaL_openlibs() */ > > This include is excess. Thanks! Nice catch! > > +#include /* memcmp() */ > > + const char test_str[] = "\xd4\x0f\x00"; > > + const char *data = (char *)test_str; > > Why do you create a distinct pointer for to be passed to ? > You can just properly declare : > | const char *test_str = "\xd4\x0f\00"; > Ok! Sure! > > + luamp_decode(L, luaL_msgpack_default, &data); > > + return 0; > > +} > > + > > + > > +static void > > +test_luamp_encode_extension_default(struct lua_State *L) > > +{ > > + plan(2); > > + lua_pushcfunction(L, lua_encode_unknown_mp); > > + ok(lua_pcall(L, 0, 0, 0) != 0, > > You can check that exactly is yielded here. Thanks! Good idea! > > + "mplua_decode: unsupported extension raise error"); > > You can simply use here. Ok! Thanks! > > + 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
> 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 > uses it right in
. 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
as it's done in rlist.c Ok, I will move body of the test to the
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. > > +int > > +main(void) > > +{ > > + header(); > > + > > + struct lua_State *L = luaL_newstate(); > > + test_luamp_encode_extension_default(L); > > + > > + footer(); > > + check_plan(); > > The function should return 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? > > +} > > > > > -- > > 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