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

[Igor Munkin <imun@tarantool.org>]

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