From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: [tarantool-patches] Re: [PATCH v3] Add MsgPack ext types handling. From: Serge Petrenko In-Reply-To: <20190604141153.63z7yefi2uituyw3@esperanza> Date: Tue, 4 Jun 2019 18:24:53 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <7C50C83F-5F11-494F-AD18-FC07465FC5DA@tarantool.org> References: <20190603131425.29405-1-sergepetrenko@tarantool.org> <20190604141153.63z7yefi2uituyw3@esperanza> To: Vladimir Davydov Cc: Georgy Kirichenko , Konstantin Osipov , tarantool-patches@freelists.org List-ID: > 4 =D0=B8=D1=8E=D0=BD=D1=8F 2019 =D0=B3., =D0=B2 17:11, Vladimir = Davydov =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB= (=D0=B0): >=20 > 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" >>=20 >> +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 =3D mp_decode_extl(&data, &type); = \ >> + PRINTF("%s", mp_ext_type_str(type)); = \ >> + data +=3D len; = \ >> break; = \ >> + } = \ >=20 > 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: >=20 > mp_sizeof_timestamp > mp_encode_timestamp > mp_decode_timestamp > mp_vformat > mp_snprint >=20 > 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. >=20 >> +/** >> + * \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) =3D=3D >> + * 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); >=20 > Nit: >=20 > - IMO 'type' should go before 'ext_data' in the argument list as it > defines what 'ext_data' is about. >=20 > - 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. >=20 > Other than that, the patch looks good to me. Thank you! All fixed. After some consideration, =E2=80=99str=E2=80=99 is probably = better than =E2=80=98ext_data=E2=80=99. The diff=E2=80=99s 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" =20 -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 =3D mp_decode_extl(&data, &type); = \ - PRINTF("%s", mp_ext_type_str(type)); = \ + PRINTF("undefined"); = \ data +=3D 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 }; =20 -/** - * \brief MsgPack extension data types. - */ -enum mp_ext_type { - MP_EXT_TIMESTAMP =3D -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) =3D=3D * 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); =20 /** * \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) } =20 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 =3D mp_encode_extl(data, type, len); - memcpy(data, ext_data, len); + memcpy(data, str, len); return data + len; } =20 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 =3D buf + 1; /* use unaligned = address to fail early */ s1[i] =3D 'a' + i % 26; = \ } = \ _ext(int8_t ext_type =3D 0); = \ - const char *d1 =3D mp_encode_##_type(data, s1, _ext(ext_type = COMMA) _vl);\ + const char *d1 =3D mp_encode_##_type(data, _ext(ext_type COMMA) = s1, _vl);\ const char *d2; = \ uint32_t len2; = \ d2 =3D data; = \ @@ -884,7 +884,7 @@ test_mp_print() d =3D mp_encode_uint(d, 100); d =3D mp_encode_uint(d, 500); /* let's pack zero-length ext */ - d =3D mp_encode_extl(d, MP_EXT_TIMESTAMP, 0); + d =3D mp_encode_extl(d, 0, 0); char bin[] =3D "\x12test\x34\b\t\n\"bla\\-bla\"\f\r"; d =3D mp_encode_bin(d, bin, sizeof(bin)); d =3D mp_encode_map(d, 0); @@ -893,7 +893,7 @@ test_mp_print() const char *expected =3D "[-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 =3D strlen(expected); =20