[Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message
Igor Munkin
imun at tarantool.org
Tue Jul 14 18:46:46 MSK 2020
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
>
<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.
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.h> /* lua_*() */
> +#include <lauxlib.h> /* 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 <lualib.h> /* luaL_openlibs() */
This include is excess.
> +#include <string.h> /* 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 <luamp_decode>?
You can just properly declare <test_str>:
| 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 <LUA_ERRRUN> is yielded here.
> + "mplua_decode: unsupported extension raise error");
You can simply use <lua_cpcall> 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 <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
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 <int> but returns nothing. All other tests
have the following idiom:
| return check_plan();
> +}
<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
More information about the Tarantool-patches
mailing list