From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 594434696C6 for ; Fri, 10 Apr 2020 02:46:44 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) From: Serge Petrenko In-Reply-To: <4e444328-9007-ea9e-a462-2cc1bafb073e@tarantool.org> Date: Fri, 10 Apr 2020 02:46:43 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <4e444328-9007-ea9e-a462-2cc1bafb073e@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 3/4] box: add MsgPack encoding/decoding for UUID List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org > 6 =D0=B0=D0=BF=D1=80. 2020 =D0=B3., =D0=B2 00:26, Vladislav Shpilevoy = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0= =B0): >=20 > Thanks for the patch! >=20 Hi! Thanks for the review! > See 10 comments below. >=20 >> diff --git a/src/lib/core/mp_uuid.c b/src/lib/core/mp_uuid.c >> new file mode 100644 >> index 000000000..5b0cc3f1a >> --- /dev/null >> +++ b/src/lib/core/mp_uuid.c >> @@ -0,0 +1,75 @@ >> + >> +#include "mp_uuid.h" >> +#include "msgpuck.h" >> +#include "mp_extension_types.h" >> +#include "lib/uuid/tt_uuid.h" >=20 > 1. uuid is a separate library depending on core. Now core > depends on uuid too. This is a cyclic dependency. It does not > explode probably only because all needed functions are in the > tt_uuid.h header. So mp_uuid.c/.h should be moved to lib/uuid. > Is it possible? >=20 > I see we add MP_UUID to src/lib/core/mp_extension_types.h. > That is also a part of core lib. Maybe we should just merge core > and uuid libs. I remember we split them only because Kostja > insisted, but personally I don't see any reason in keeping then > separated. >=20 > However I am afraid this won't help forever. In future we may > want to add new mp_ext for something in box/ such as MP_TUPLE > or something. We already want something like that for custom > errors. Not related to this patch anyway. Thanks for pointing this out. I merged mp_uuid.h/.c into uuid lib, and also extracted mpstream, which depends on mp_uuid now, into a separate library. So now the dependencies are =20 mpstream =E2=80=94> uuid -> core \________->_______/ >=20 >> + >> +inline uint32_t >> +mp_sizeof_uuid() >> +{ >> + return mp_sizeof_ext(sizeof(struct tt_uuid)); >> +} >> + >> +struct tt_uuid * >> +uuid_unpack(const char **data, uint32_t len, struct tt_uuid *uuid) >> +{ >> + if (len !=3D sizeof(*uuid)) >> + return NULL; >> + memcpy(uuid, *data, sizeof(*uuid)); >> + if (tt_uuid_validate(uuid) !=3D 0) >> + return NULL; >> + *data +=3D sizeof(*uuid); >> + return uuid; >> +} >> + >> +struct tt_uuid * >> +mp_decode_uuid(const char **data, struct tt_uuid *uuid) >> +{ >> + if (mp_typeof(**data) !=3D MP_EXT) >> + return NULL; >> + int8_t type; >> + const char *const svp =3D *data; >> + >> + uint32_t len =3D mp_decode_extl(data, &type); >> + if (type !=3D MP_UUID || uuid_unpack(data, len, uuid) =3D=3D = NULL) { >> + *data =3D svp; >> + return NULL; >> + } >> + return uuid; >> +} >> + >> +char * >> +mp_encode_uuid(char *data, const struct tt_uuid *uuid) >> +{ >> + return mp_encode_ext(data, MP_UUID, (char *)uuid, = sizeof(*uuid)); >=20 > 2. Can we just copy it as is? Shouldn't it be big endian or > little endian or whatever the standard says? If the standard > says nothing, we should use big-endian (because all other msgpack > types use it), and we need to either convert every field of > uuid to big endian before encoding, or ensure it is already > big endian always, and copy as is. >=20 > In case tt_uuid fields are little endian now, I think no big > problem in storing them as big endian. Just keep tostring/fromstring > and other public functions doing the same as before. If we store > uuid in big-endian, we can use memcmp() to compare two 16byte = binaries. > Without conversions. >=20 Good point, thanks! It isn=E2=80=99t specified whether uuid fields = should be big or little endian. The standard does specify though that the fields must be encoded as big endian. So instead of memcpy I do mp_store_u32, = mp_store_u16, =E2=80=A6 and so on. (mp_load_u32(), =E2=80=A6 for unpack). mp_load/store_8/16/32 convert the integers to big endian for us, if = needed. >> +} >> diff --git a/src/lib/core/mp_uuid.h b/src/lib/core/mp_uuid.h >> new file mode 100644 >> index 000000000..fda0f3aed >> --- /dev/null >> +++ b/src/lib/core/mp_uuid.h >> @@ -0,0 +1,90 @@ >> +#ifndef TARANTOOL_LIB_CORE_MP_UUID_INCLUDED >> +#define TARANTOOL_LIB_CORE_MP_UUID_INCLUDED >=20 > 3. We use #pragma once now, no? >=20 Ok, fixed. >> +#include >> + >> +#if defined(__cplusplus) >> +extern "C" { >> +#endif /* defined(__cplusplus) */ >> + >> +struct tt_uuid; >> + >> +/** >> + * \brief Return the number of bytes an encoded uuid value takes. >> + */ >> +uint32_t >> +mp_sizeof_uuid(); >=20 > 4. Please, use 'void' when a function does not take any arguments. > Cyrill was saing that it is bad to leave empty (), because then > the compiler considers it a function having any number of arguments, > and generates some code for that. Done. >=20 >> + >> +/** >> + * Copy a uuid value from a buffer. Can be used in a combination >> + * with mp_decode_extl() instead of mp_decode_uuid() when multiple >> + * extension types are possible. >> + * >> + * \param data A buffer. >> + * \param len Length returned by mp_decode_extl, has to be equal >> + * to sizeof(struct tt_uuid), otherwise an error is >> + * returned. >> + * \param[out] uuid Uuid to be decoded. >> + * \return A pointer to the decoded uuid. >> + * NULL in case of an error. >> + * \post *data =3D *data + sizeof(struct tt_uuid). >> + */ >> +struct tt_uuid * >> +uuid_unpack(const char **data, uint32_t len, struct tt_uuid *uuid); >=20 > 5. Why is it in mp_uuid.h/.c? It does not depend on msgpack at all. > The similar function decimal_unpack(), for example, is in decimal.c. > Not in mp_decimal.c. After considering your comments re uuid_unpack I pack and unpack uuid fields in big endian style, using mp_store/load_u32/u16/u8. So let it be in mp_uuid.h/.c now. >=20 >> + >> +/** >> + * \brief Decode a uuid from MsgPack \a data. >> + * \param data A buffer. >> + * \param[out] uuid Uuid to be decoded. >> + * \return A pointer to the decoded uuid. >> + * NULL in case of an error. >> + * \post *data =3D *data + mp_sizeof_uuid(). >> + */ >> +struct tt_uuid * >> +mp_decode_uuid(const char **data, struct tt_uuid *uuid); >> + >> diff --git a/src/lib/msgpuck b/src/lib/msgpuck >> index 8ae606a16..0d273f95f 160000 >> --- a/src/lib/msgpuck >> +++ b/src/lib/msgpuck >> @@ -1 +1 @@ >> -Subproject commit 8ae606a1636dd89b2d61b154e5a1db03dce91657 >> +Subproject commit 0d273f95f8de64aeed698c354ca3bc7cb5ea461a >=20 > 6. I didn't see a patch for msgpuck repo. Did you send it? Yeah, you even LGTM=E2=80=99ed it ([PATCH] Add const qualifier to = mp_encode_ext argument) Alexander Turenko pushed it and I updated the reference in v2 of this = patchset. >=20 >> diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c >> index edbc15b72..fb49b1547 100644 >> --- a/src/lua/msgpack.c >> +++ b/src/lua/msgpack.c >> @@ -325,6 +336,10 @@ luamp_decode(struct lua_State *L, struct = luaL_serializer *cfg, >> break; >> } >> } >> +return; >=20 > 7. Bad indentation for 'return;'. >=20 Yep, fixed, ty. >> +ext_decode_err: >> + lua_pop(L, -1); >> + luaL_error(L, "msgpack.decode: invalid MsgPack"); >> } >>> diff --git a/test/app/uuid.result b/test/app/uuid.result >> index 0713614c6..013c51282 100644 >> --- a/test/app/uuid.result >> +++ b/test/app/uuid.result >> @@ -106,7 +106,7 @@ uu.node[5] >> -- invalid values >> uuid.fromstr(nil) >> --- >> -- error: 'builtin/uuid.lua:47: fromstr(str)' >> +- error: 'builtin/uuid.lua:38: fromstr(str)' >=20 > 8. Better add a test_run filter to avoid that change in > future when anything will be updated in uuid.lua again. >=20 Ok, done. >> ... >> uuid.fromstr('') >> --- >> diff --git a/test/unit/uuid.c b/test/unit/uuid.c >> index c43d93b4f..4f6b963ba 100644 >> --- a/test/unit/uuid.c >> +++ b/test/unit/uuid.c >> @@ -27,10 +29,28 @@ uuid_test(struct tt_uuid a, struct tt_uuid b, int = expected_result) >> "%s %s %s", a_str, sign, b_str); >> } >>=20 >> +static void >> +mp_uuid_test() >> +{ >> + plan(4); >> + char buf[18]; >> + char *data =3D buf; >> + const char *data1 =3D buf; >> + struct tt_uuid uu, ret; >> + random_init(); >> + tt_uuid_create(&uu); >> + char *end =3D mp_encode_uuid(data, &uu); >> + is(end - data, mp_sizeof_uuid(), "mp_sizeof_uuid() =3D=3D = encoded length"); >> + struct tt_uuid *rc =3D mp_decode_uuid(&data1, &ret); >> + is(rc, &ret, "mp_decode_uuid() return code"); >> + is(data1, end, "mp_sizeof_uuid() =3D=3D decoded length"); >> + is(tt_uuid_compare(&uu, &ret), 0, = "mp_decode_uuid(mp_encode_uuid(uu)) =3D=3D uu"); >=20 > 9. When you create a subset of tests, it should have at least > plan() + check_plan(). Also we usually add header() footer(), but > seems like it is too late for this test file. Fixed, thanks. >=20 >> +} >> + >> int >> main(void) >> { >> - plan(2); >> + plan(3); >> diff --git a/third_party/lua-yaml/lyaml.cc = b/third_party/lua-yaml/lyaml.cc >> index af4f2f5d5..411c56f71 100644 >> --- a/third_party/lua-yaml/lyaml.cc >> +++ b/third_party/lua-yaml/lyaml.cc >> @@ -697,10 +700,18 @@ static int dump_node(struct lua_yaml_dumper = *dumper) >> switch (field.ext_type) { >> case MP_DECIMAL: >> str =3D decimal_to_string(field.decval); >> - len =3D strlen(str); >> - break; >> + len =3D strlen(str); >> + break; >> + case MP_UUID: >> + { >> + char *buf =3D tt_static_buf(); >> + tt_uuid_to_string(field.uuidval, buf); >> + str =3D buf; >> + len =3D UUID_STR_LEN; >> + break; >> + } >> default: >> - assert(0); /* checked by luaL_checkfield() */ >> + assert(0); /* checked by luaL_checkfield() */ >=20 > 10. Better avoid the unnecessary diff. >=20 >> } >> break; >> } >>=20 -- Serge Petrenko sergepetrenko@tarantool.org