From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 4333A445321 for ; Tue, 14 Jul 2020 18:57:03 +0300 (MSK) Date: Tue, 14 Jul 2020 18:46:46 +0300 From: Igor Munkin Message-ID: <20200714154646.GL5559@tarantool.org> References: <9e6b2ec973a0dd0f2bb6da4915bf1f8f7ff58081.1592235478.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <9e6b2ec973a0dd0f2bb6da4915bf1f8f7ff58081.1592235478.git.skaplun@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: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org, Alexander Turenko 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? > > 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. 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 I don't know what to say... > 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 > +#include /* luaL_openlibs() */ This include is excess. > +#include /* memcmp() */ > + > +#include "lua/msgpack.h" > +#include "unit.h" > + > +/* > + * test for https://github.com/tarantool/tarantool/issues/5017 > + */ > + > + > +static int > +lua_encode_unknown_mp(struct lua_State *L) > +{ > + 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"; > + 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. > + "mplua_decode: unsupported extension raise error"); You can simply use here. > + 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 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. > +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(); > +} > -- > 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