From: Serge Petrenko <sergepetrenko@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 3/4] box: add MsgPack encoding/decoding for UUID Date: Sat, 11 Apr 2020 17:14:49 +0300 [thread overview] Message-ID: <DE6BB730-7863-45B5-BC9E-7FD8C5691D53@tarantool.org> (raw) In-Reply-To: <ee80d2d9-af43-250c-b7cf-ae2b92c9e456@tarantool.org> > 10 апр. 2020 г., в 19:56, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а): > > Thanks for the patch! Hi! Thanks for the review! > > See 7 comments below. > >> 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" > > 1. Lib/, as well as core/, can be omitted. That paths anyway > are in -I. Ok, thanks. > >> + >> +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 <stdint.h> >> + >> +#if defined(__cplusplus) >> +extern "C" { >> +#endif /* defined(__cplusplus) */ >> + >> +struct tt_uuid; >> + >> +#define UUID_PACKED_LEN sizeof(struct tt_uuid) > > 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. > >> + >> +/** >> + * \brief Return the number of bytes an encoded uuid value takes. >> + */ > > 3. I would better use @ than \. The latter is used in the old code > only. Ok. > >> +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 @@ >> >> #include "lua/decimal.h" /* lua_pushdecimal() */ >> #include "lib/core/decimal.h" /* decimal_unpack() */ >> +#include "lib/uuid/mp_uuid.h" /* mp_decode_uuid() */ > > 4. lib/ can be omitted. Don't know why they are used in other > includes. > > diag.h is the most used header from core/, and we never write > core/diag.h. I see, ok. > >> 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 != 0); >> CTID_CONST_CHAR_PTR = luaL_ctypeid(L, "const char *"); >> assert(CTID_CONST_CHAR_PTR != 0); >> + rc = 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];" >> + "};"); > > 5. It is worth adding assert(rc == 0). Otherwise you can omit 'rc = ' > at all. Yep, I missed it somehow. Thanks. > >> + CTID_UUID = luaL_ctypeid(L, "struct tt_uuid"); >> + assert(CTID_UUID != 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 */ > > 6. lib/ and core/ can be omitted. Done. > >> #define LUAYAML_TAG_PREFIX "tag:yaml.org,2002:" >> >> @@ -697,10 +700,18 @@ static int dump_node(struct lua_yaml_dumper *dumper) >> switch (field.ext_type) { >> case MP_DECIMAL: >> str = decimal_to_string(field.decval); >> - len = strlen(str); >> - break; >> + len = strlen(str); >> + break; >> + case MP_UUID: >> + { >> + char *buf = tt_static_buf(); >> + tt_uuid_to_string(field.uuidval, buf); >> + str = buf; >> + len = UUID_STR_LEN; >> + break; >> + } > 7. Consider the diff: > > ==================== > 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 = strlen(str); > break; > case MP_UUID: > - { > - char *buf = tt_static_buf(); > - tt_uuid_to_string(field.uuidval, buf); > - str = buf; > + str = tt_uuid_str(field.uuidval); > len = UUID_STR_LEN; > break; > - } > default: > assert(0); /* checked by luaL_checkfield() */ > } > ==================== Thanks! Also applied to lua-cjson. Here’s 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" 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 @@ */ #include <stdint.h> +#include "tt_uuid.h" #if defined(__cplusplus) extern "C" { #endif /* defined(__cplusplus) */ -struct tt_uuid; -#define UUID_PACKED_LEN sizeof(struct tt_uuid) +enum {UUID_PACKED_LEN = sizeof(struct tt_uuid)}; /** - * \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 = *data + sizeof(struct tt_uuid). + * @post *data = *data + sizeof(struct tt_uuid). */ struct tt_uuid * uuid_unpack(const char **data, uint32_t len, struct tt_uuid *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. + * @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 = *data + mp_sizeof_uuid(). + * @post *data = *data + mp_sizeof_uuid(). */ struct tt_uuid * mp_decode_uuid(const char **data, struct tt_uuid *uuid); /** - * \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) int rc = luaL_cdef(L, "struct ibuf;"); assert(rc == 0); - (void) rc; CTID_STRUCT_IBUF = luaL_ctypeid(L, "struct ibuf"); assert(CTID_STRUCT_IBUF != 0); CTID_STRUCT_IBUF_PTR = 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 == 0); + (void) rc; CTID_UUID = luaL_ctypeid(L, "struct tt_uuid"); assert(CTID_UUID != 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" #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 */ #define DEFAULT_ENCODE_KEEP_BUFFER 1 @@ -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 = 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 */ #define LUAYAML_TAG_PREFIX "tag:yaml.org,2002:" @@ -703,13 +703,9 @@ static int dump_node(struct lua_yaml_dumper *dumper) len = strlen(str); break; case MP_UUID: - { - char *buf = tt_static_buf(); - tt_uuid_to_string(field.uuidval, buf); - str = buf; + str = tt_uuid_str(field.uuidval); len = UUID_STR_LEN; break; - } default: assert(0); /* checked by luaL_checkfield() */ } -- Serge Petrenko sergepetrenko@tarantool.org
next prev parent reply other threads:[~2020-04-11 14:14 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-09 23:50 [Tarantool-patches] [PATCH v2 0/4] introduce indices over UUID Serge Petrenko 2020-04-09 23:50 ` [Tarantool-patches] [PATCH v2 1/4] refactoring: extract mpstream into a separate library Serge Petrenko 2020-04-10 16:56 ` Vladislav Shpilevoy 2020-04-11 13:12 ` Serge Petrenko 2020-04-09 23:50 ` [Tarantool-patches] [PATCH v2 2/4] uuid: expose tt_uuid_validate method Serge Petrenko 2020-04-09 23:50 ` [Tarantool-patches] [PATCH v2 3/4] box: add MsgPack encoding/decoding for UUID Serge Petrenko 2020-04-10 16:56 ` Vladislav Shpilevoy 2020-04-11 14:14 ` Serge Petrenko [this message] 2020-04-09 23:50 ` [Tarantool-patches] [PATCH v2 4/4] box: introduce indices by UUID Serge Petrenko 2020-04-10 16:56 ` Vladislav Shpilevoy 2020-04-11 14:14 ` Serge Petrenko 2020-04-10 12:27 ` [Tarantool-patches] [PATCH v2 0/4] introduce indices over UUID Serge Petrenko 2020-04-11 18:01 ` Vladislav Shpilevoy 2020-04-13 13:52 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=DE6BB730-7863-45B5-BC9E-7FD8C5691D53@tarantool.org \ --to=sergepetrenko@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 3/4] box: add MsgPack encoding/decoding for UUID' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox