[PATCH v3] Add MsgPack ext types handling.
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Jun 4 17:11:53 MSK 2019
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.
More information about the Tarantool-patches
mailing list