[Tarantool-patches] [PATCH v2 3/4] box: add MsgPack encoding/decoding for UUID
Serge Petrenko
sergepetrenko at tarantool.org
Sat Apr 11 17:14:49 MSK 2020
> 10 апр. 2020 г., в 19:56, Vladislav Shpilevoy <v.shpilevoy at 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 at tarantool.org
More information about the Tarantool-patches
mailing list