From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 6E9574696C3 for ; Sat, 11 Apr 2020 17:14:50 +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: Date: Sat, 11 Apr 2020 17:14:49 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <60281b83d48279b708ad173d271e900943ce8e57.1586476073.git.sergepetrenko@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 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 > 10 =D0=B0=D0=BF=D1=80. 2020 =D0=B3., =D0=B2 19:56, Vladislav Shpilevoy = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0= =B0): >=20 > Thanks for the patch! Hi! Thanks for the review! >=20 > See 7 comments below. >=20 >> diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/uuid/mp_uuid.c >> new file mode 100644 >> index 000000000..7acfbc797 >> --- /dev/null >> +++ b/src/lib/uuid/mp_uuid.c >> @@ -0,0 +1,98 @@ >> + >> +#include "mp_uuid.h" >> +#include "msgpuck.h" >> +#include "mp_extension_types.h" >> +#include "lib/uuid/tt_uuid.h" >=20 > 1. Lib/, as well as core/, can be omitted. That paths anyway > are in -I. Ok, thanks. >=20 >> + >> +inline uint32_t >> +mp_sizeof_uuid(void) >> +{ >> + return mp_sizeof_ext(UUID_PACKED_LEN); >> +} >> diff --git a/src/lib/uuid/mp_uuid.h b/src/lib/uuid/mp_uuid.h >> new file mode 100644 >> index 000000000..430cb96d2 >> --- /dev/null >> +++ b/src/lib/uuid/mp_uuid.h >> @@ -0,0 +1,90 @@ >> + >> +#include >> + >> +#if defined(__cplusplus) >> +extern "C" { >> +#endif /* defined(__cplusplus) */ >> + >> +struct tt_uuid; >> + >> +#define UUID_PACKED_LEN sizeof(struct tt_uuid) >=20 > 2. Maybe better make it enum. When you do it via the macros, you > workaround the necessity to make include tt_uuid.h, but the macros > anyway can't be actually used without this include. So it is ok > to add the include here + change that to macros. Ok. >=20 >> + >> +/** >> + * \brief Return the number of bytes an encoded uuid value takes. >> + */ >=20 > 3. I would better use @ than \. The latter is used in the old code > only. Ok. >=20 >> +uint32_t >> +mp_sizeof_uuid(void); >> + >> diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c >> index 73ed3ece6..e4fb0cf43 100644 >> --- a/src/lua/msgpack.c >> +++ b/src/lua/msgpack.c >> @@ -43,6 +43,7 @@ >>=20 >> #include "lua/decimal.h" /* lua_pushdecimal() */ >> #include "lib/core/decimal.h" /* decimal_unpack() */ >> +#include "lib/uuid/mp_uuid.h" /* mp_decode_uuid() */ >=20 > 4. lib/ can be omitted. Don't know why they are used in other > includes. >=20 > diag.h is the most used header from core/, and we never write > core/diag.h. I see, ok. >=20 >> diff --git a/src/lua/utils.c b/src/lua/utils.c >> index 54d18ac89..bd6bfb008 100644 >> --- a/src/lua/utils.c >> +++ b/src/lua/utils.c >> @@ -1286,5 +1296,15 @@ tarantool_lua_utils_init(struct lua_State *L) >> assert(CTID_CHAR_PTR !=3D 0); >> CTID_CONST_CHAR_PTR =3D luaL_ctypeid(L, "const char *"); >> assert(CTID_CONST_CHAR_PTR !=3D 0); >> + rc =3D luaL_cdef(L, "struct tt_uuid {" >> + "uint32_t time_low;" >> + "uint16_t time_mid;" >> + "uint16_t time_hi_and_version;" >> + "uint8_t clock_seq_hi_and_reserved;" >> + "uint8_t clock_seq_low;" >> + "uint8_t node[6];" >> + "};"); >=20 > 5. It is worth adding assert(rc =3D=3D 0). Otherwise you can omit 'rc = =3D ' > at all. Yep, I missed it somehow. Thanks. >=20 >> + CTID_UUID =3D luaL_ctypeid(L, "struct tt_uuid"); >> + assert(CTID_UUID !=3D 0); >> return 0; >> } >> 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 >> @@ -50,6 +50,9 @@ extern "C" { >> } /* extern "C" */ >> #include "lua/utils.h" >> #include "lib/core/decimal.h" >> +#include "lib/core/tt_static.h" >> +#include "lib/core/mp_extension_types.h" /* MP_DECIMAL, MP_UUID */ >> +#include "lib/uuid/tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN = */ >=20 > 6. lib/ and core/ can be omitted. Done. >=20 >> #define LUAYAML_TAG_PREFIX "tag:yaml.org,2002:" >>=20 >> @@ -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; >> + } > 7. Consider the diff: >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > diff --git a/third_party/lua-yaml/lyaml.cc = b/third_party/lua-yaml/lyaml.cc > index 411c56f71..29dbe7211 100644 > --- a/third_party/lua-yaml/lyaml.cc > +++ b/third_party/lua-yaml/lyaml.cc > @@ -703,13 +703,9 @@ static int dump_node(struct lua_yaml_dumper = *dumper) > len =3D strlen(str); > break; > case MP_UUID: > - { > - char *buf =3D tt_static_buf(); > - tt_uuid_to_string(field.uuidval, buf); > - str =3D buf; > + str =3D tt_uuid_str(field.uuidval); > len =3D UUID_STR_LEN; > break; > - } > default: > assert(0); /* checked by luaL_checkfield() */ > } > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Thanks! Also applied to lua-cjson. Here=E2=80=99s the diff: diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/uuid/mp_uuid.c index 7acfbc797..1a9daf6d1 100644 --- a/src/lib/uuid/mp_uuid.c +++ b/src/lib/uuid/mp_uuid.c @@ -32,7 +32,6 @@ #include "mp_uuid.h" #include "msgpuck.h" #include "mp_extension_types.h" -#include "lib/uuid/tt_uuid.h" =20 inline uint32_t mp_sizeof_uuid(void) diff --git a/src/lib/uuid/mp_uuid.h b/src/lib/uuid/mp_uuid.h index 430cb96d2..fdc39f7ef 100644 --- a/src/lib/uuid/mp_uuid.h +++ b/src/lib/uuid/mp_uuid.h @@ -32,17 +32,17 @@ */ =20 #include +#include "tt_uuid.h" =20 #if defined(__cplusplus) extern "C" { #endif /* defined(__cplusplus) */ =20 -struct tt_uuid; =20 -#define UUID_PACKED_LEN sizeof(struct tt_uuid) +enum {UUID_PACKED_LEN =3D sizeof(struct tt_uuid)}; =20 /** - * \brief Return the number of bytes an encoded uuid value takes. + * @brief Return the number of bytes an encoded uuid value takes. */ uint32_t mp_sizeof_uuid(void); @@ -52,35 +52,35 @@ mp_sizeof_uuid(void); * 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 + * @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. + * @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). + * @post *data =3D *data + sizeof(struct tt_uuid). */ struct tt_uuid * uuid_unpack(const char **data, uint32_t len, struct tt_uuid *uuid); =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. + * @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(). + * @post *data =3D *data + mp_sizeof_uuid(). */ struct tt_uuid * mp_decode_uuid(const char **data, struct tt_uuid *uuid); =20 /** - * \brief Encode a uuid. - * \param data A buffer. - * \param uuid A uuid to encode. + * @brief Encode a uuid. + * @param data A buffer. + * @param uuid A uuid to encode. * - * \return \a data + mp_sizeof_uuid() + * @return @a data + mp_sizeof_uuid() */ char * mp_encode_uuid(char *data, const struct tt_uuid *uuid); diff --git a/src/lua/utils.c b/src/lua/utils.c index bd6bfb008..667365fdc 100644 --- a/src/lua/utils.c +++ b/src/lua/utils.c @@ -1287,7 +1287,6 @@ tarantool_lua_utils_init(struct lua_State *L) =20 int rc =3D luaL_cdef(L, "struct ibuf;"); assert(rc =3D=3D 0); - (void) rc; CTID_STRUCT_IBUF =3D luaL_ctypeid(L, "struct ibuf"); assert(CTID_STRUCT_IBUF !=3D 0); CTID_STRUCT_IBUF_PTR =3D luaL_ctypeid(L, "struct ibuf *"); @@ -1304,6 +1303,8 @@ tarantool_lua_utils_init(struct lua_State *L) "uint8_t clock_seq_low;" "uint8_t node[6];" "};"); + assert(rc =3D=3D 0); + (void) rc; CTID_UUID =3D luaL_ctypeid(L, "struct tt_uuid"); assert(CTID_UUID !=3D 0); return 0; diff --git a/third_party/lua-cjson/lua_cjson.c = b/third_party/lua-cjson/lua_cjson.c index 6e1793a59..d4b89ce0d 100644 --- a/third_party/lua-cjson/lua_cjson.c +++ b/third_party/lua-cjson/lua_cjson.c @@ -48,9 +48,9 @@ #include "strbuf.h" =20 #include "lua/utils.h" -#include "lib/core/mp_extension_types.h" /* MP_DECIMAL, MP_UUID */ -#include "lib/core/tt_static.h" -#include "lib/uuid/tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN */ +#include "mp_extension_types.h" /* MP_DECIMAL, MP_UUID */ +#include "tt_static.h" +#include "uuid/tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN */ =20 #define DEFAULT_ENCODE_KEEP_BUFFER 1 =20 @@ -431,11 +431,8 @@ static void json_append_data(lua_State *l, struct = luaL_serializer *cfg, return json_append_string(cfg, json, str, strlen(str)); } case MP_UUID: - { - char *str =3D tt_static_buf(); - tt_uuid_to_string(field.uuidval, str); - return json_append_string(cfg, json, str, UUID_STR_LEN); - } + return json_append_string(cfg, json, = tt_uuid_str(field.uuidval), + UUID_STR_LEN); default: assert(false); } diff --git a/third_party/lua-yaml/lyaml.cc = b/third_party/lua-yaml/lyaml.cc index 411c56f71..9c3a4a646 100644 --- a/third_party/lua-yaml/lyaml.cc +++ b/third_party/lua-yaml/lyaml.cc @@ -50,9 +50,9 @@ extern "C" { } /* extern "C" */ #include "lua/utils.h" #include "lib/core/decimal.h" -#include "lib/core/tt_static.h" -#include "lib/core/mp_extension_types.h" /* MP_DECIMAL, MP_UUID */ -#include "lib/uuid/tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN */ +#include "tt_static.h" +#include "mp_extension_types.h" /* MP_DECIMAL, MP_UUID */ +#include "uuid/tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN */ =20 #define LUAYAML_TAG_PREFIX "tag:yaml.org,2002:" =20 @@ -703,13 +703,9 @@ static int dump_node(struct lua_yaml_dumper = *dumper) len =3D strlen(str); break; case MP_UUID: - { - char *buf =3D tt_static_buf(); - tt_uuid_to_string(field.uuidval, buf); - str =3D buf; + str =3D tt_uuid_str(field.uuidval); len =3D UUID_STR_LEN; break; - } default: assert(0); /* checked by luaL_checkfield() */ } -- Serge Petrenko sergepetrenko@tarantool.org