[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