[tarantool-patches] Re: [PATCH v3] Add MsgPack ext types handling.
Serge Petrenko
sergepetrenko at tarantool.org
Tue Jun 4 18:24:53 MSK 2019
> 4 июня 2019 г., в 17:11, Vladimir Davydov <vdavydov.dev at gmail.com> написал(а):
>
> On Mon, Jun 03, 2019 at 04:14:25PM +0300, Serge Petrenko wrote:
>> diff --git a/msgpuck.c b/msgpuck.c
>> index d0ffb83..e5f7e25 100644
>> --- a/msgpuck.c
>> +++ b/msgpuck.c
>> @@ -33,6 +33,17 @@
>> #define MP_LIBRARY 1
>> #include "msgpuck.h"
>>
>> +const char *
>> +mp_ext_type_str(enum mp_ext_type type)
>> +{
>> + switch(type) {
>> + case MP_EXT_TIMESTAMP:
>> + return "timestamp";
>> + default:
>> + return "undefined";
>> + }
>> +}
>> +
>> size_t
>> mp_vformat(char *data, size_t data_size, const char *format, va_list vl)
>> {
>> @@ -298,9 +309,14 @@ next: \
>> PRINTF("%lg", mp_decode_double(&data)); \
>> break; \
>> case MP_EXT: \
>> - mp_next(&data); \
>> - PRINTF("undefined"); \
>> + { \
>> + int8_t type; \
>> + uint32_t len; \
>> + len = mp_decode_extl(&data, &type); \
>> + PRINTF("%s", mp_ext_type_str(type)); \
>> + data += len; \
>> break; \
>> + } \
>
> It isn't exactly helpful to see "timestamp" instead of a timestamp
> value. If we add MP_EXT_TIMESTAMP to the library, we should implement
> all methods needed to work with it:
>
> mp_sizeof_timestamp
> mp_encode_timestamp
> mp_decode_timestamp
> mp_vformat
> mp_snprint
>
> Since this would require a lot of effort and is out of the scope of this
> issue, I suggest dropping MP_EXT_TIMESTAMP altogether for now. Let's
> just simply add mp_ext infrastructure, like you did in this patch. In
> tests, you can pass an arbitrary number for MP_EXT type.
>
>> +/**
>> + * \brief Encode extension data of length \a len.
>> + * The function is equivalent to mp_encode_extl() + memcpy.
>> + * \param data - a buffer
>> + * \param ext_data - a pointer to extension data
>> + * \param type - extension type to encode
>> + * \param len - a extension data length
>> + * \return \a data + mp_sizeof_ext(\a len) ==
>> + * data + mp_sizeof_extl(\a len) + \a len
>> + * \sa mp_encode_strl
>> + */
>> +MP_PROTO char *
>> +mp_encode_ext(char *data, char *ext_data, int8_t type, uint32_t len);
>
> Nit:
>
> - IMO 'type' should go before 'ext_data' in the argument list as it
> defines what 'ext_data' is about.
>
> - I'd rename 'ext_data' argument to 'str' for consistency with
> mp_encode_bin/str. Besides, it's shorter. Not really important
> so this one is up to you.
>
> Other than that, the patch looks good to me.
Thank you!
All fixed. After some consideration, ’str’ is probably better than ‘ext_data’.
The diff’s below
diff --git a/msgpuck.c b/msgpuck.c
index e5f7e25..637755c 100644
--- a/msgpuck.c
+++ b/msgpuck.c
@@ -33,17 +33,6 @@
#define MP_LIBRARY 1
#include "msgpuck.h"
-const char *
-mp_ext_type_str(enum mp_ext_type type)
-{
- switch(type) {
- case MP_EXT_TIMESTAMP:
- return "timestamp";
- default:
- return "undefined";
- }
-}
-
size_t
mp_vformat(char *data, size_t data_size, const char *format, va_list vl)
{
@@ -313,7 +302,7 @@ next: \
int8_t type; \
uint32_t len; \
len = mp_decode_extl(&data, &type); \
- PRINTF("%s", mp_ext_type_str(type)); \
+ PRINTF("undefined"); \
data += len; \
break; \
} \
diff --git a/msgpuck.h b/msgpuck.h
index 12ae9c1..7cf59d2 100644
--- a/msgpuck.h
+++ b/msgpuck.h
@@ -382,13 +382,6 @@ enum mp_type {
MP_EXT
};
-/**
- * \brief MsgPack extension data types.
- */
-enum mp_ext_type {
- MP_EXT_TIMESTAMP = -1
-};
-
/**
* \brief Determine MsgPack type by a first byte \a c of encoded data.
*
@@ -569,15 +562,15 @@ mp_encode_extl(char *data, int8_t type, uint32_t len);
* \brief Encode extension data of length \a len.
* The function is equivalent to mp_encode_extl() + memcpy.
* \param data - a buffer
- * \param ext_data - a pointer to extension data
* \param type - extension type to encode
+ * \param str - a pointer to extension data
* \param len - a extension data length
* \return \a data + mp_sizeof_ext(\a len) ==
* data + mp_sizeof_extl(\a len) + \a len
* \sa mp_encode_strl
*/
MP_PROTO char *
-mp_encode_ext(char *data, char *ext_data, int8_t type, uint32_t len);
+mp_encode_ext(char *data, int8_t type, char *str, uint32_t len);
/**
* \brief Check that \a cur buffer has enough bytes to decode an ext header.
@@ -1584,10 +1577,10 @@ mp_encode_extl(char *data, int8_t type, uint32_t len)
}
MP_IMPL char *
-mp_encode_ext(char *data, char *ext_data, int8_t type, uint32_t len)
+mp_encode_ext(char *data, int8_t type, char *str, uint32_t len)
{
data = mp_encode_extl(data, type, len);
- memcpy(data, ext_data, len);
+ memcpy(data, str, len);
return data + len;
}
diff --git a/test/msgpuck.c b/test/msgpuck.c
index c2aa70f..ecc2b26 100644
--- a/test/msgpuck.c
+++ b/test/msgpuck.c
@@ -82,7 +82,7 @@ static char *data = buf + 1; /* use unaligned address to fail early */
s1[i] = 'a' + i % 26; \
} \
_ext(int8_t ext_type = 0); \
- const char *d1 = mp_encode_##_type(data, s1, _ext(ext_type COMMA) _vl);\
+ const char *d1 = mp_encode_##_type(data, _ext(ext_type COMMA) s1, _vl);\
const char *d2; \
uint32_t len2; \
d2 = data; \
@@ -884,7 +884,7 @@ test_mp_print()
d = mp_encode_uint(d, 100);
d = mp_encode_uint(d, 500);
/* let's pack zero-length ext */
- d = mp_encode_extl(d, MP_EXT_TIMESTAMP, 0);
+ d = mp_encode_extl(d, 0, 0);
char bin[] = "\x12test\x34\b\t\n\"bla\\-bla\"\f\r";
d = mp_encode_bin(d, bin, sizeof(bin));
d = mp_encode_map(d, 0);
@@ -893,7 +893,7 @@ test_mp_print()
const char *expected =
"[-5, 42, \"kill bill\", [], "
"{\"bool true\": true, \"bool false\": false, \"null\": null, "
- "\"float\": 3.14, \"double\": 3.14, 100: 500}, timestamp, "
+ "\"float\": 3.14, \"double\": 3.14, 100: 500}, undefined, "
"\"\\u0012test4\\b\\t\\n\\\"bla\\\\-bla\\\"\\f\\r\\u0000\", {}]";
int esize = strlen(expected);
More information about the Tarantool-patches
mailing list