[Tarantool-patches] [PATCH 0/5] mp_snprint() and mp_fprint() for decimal, uuid, error

[Tarantool-patches] [PATCH 0/5] mp_snprint() and mp_fprint() for decimal, uuid, error

[Vladislav Shpilevoy]

The patchset makes msgpuck functions mp_snprint() and mp_fprint()
nicely serialize MP_DECIMAL, MP_UUID, and MP_ERROR objects.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4719-mp_print-ext
Issue: https://github.com/tarantool/tarantool/issues/4719

Vladislav Shpilevoy (5):
  msgpuck: bump version to enable extension printer
  decimal: provide MP_DECIMAL extension serializer
  uuid: provide MP_UUID extension serializer
  error: provide MP_ERROR extension serializer
  msgpuck: activate MP_EXT custom serializers

 src/box/CMakeLists.txt    |   1 +
 src/box/box.cc            |   2 +
 src/box/mp_error.cc       | 161 ++++++++++++++++++++++-
 src/box/mp_error.h        |  29 ++++
 src/box/msgpack.c         |  78 +++++++++++
 src/box/msgpack.h         |  41 ++++++
 src/lib/core/mp_decimal.c |  18 +++
 src/lib/core/mp_decimal.h |  27 ++++
 src/lib/msgpuck           |   2 +-
 src/lib/uuid/mp_uuid.c    |  18 +++
 src/lib/uuid/mp_uuid.h    |  27 ++++
 test/unit/decimal.c       |  63 ++++++++-
 test/unit/decimal.result  |  11 +-
 test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
 test/unit/mp_error.result |  72 +++++++++-
 test/unit/msgpack.result  |  17 ++-
 test/unit/uuid.c          |  64 ++++++++-
 test/unit/uuid.result     |  11 +-
 18 files changed, 900 insertions(+), 12 deletions(-)
 create mode 100644 src/box/msgpack.c
 create mode 100644 src/box/msgpack.h

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 1/5] msgpuck: bump version to enable extension printer

[Vladislav Shpilevoy]

In the new version an API is added to be able to customize MP_EXT
serializer for mp_snprint() and mp_fprint() functions.

Part of #4719
---
 src/lib/msgpuck          |  2 +-
 test/unit/msgpack.result | 17 +++++++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/lib/msgpuck b/src/lib/msgpuck
index e476ad192..1b15d2ad1 160000
--- a/src/lib/msgpuck
+++ b/src/lib/msgpuck
@@ -1 +1 @@
-Subproject commit e476ad19281b29d2ee3e11e74c564c0eceea37d5
+Subproject commit 1b15d2ad18263ed060088aa4f7e105b574c07ffa
diff --git a/test/unit/msgpack.result b/test/unit/msgpack.result
index 02c0b05fd..0b0c10ed9 100644
--- a/test/unit/msgpack.result
+++ b/test/unit/msgpack.result
@@ -1,4 +1,4 @@
-1..22
+1..23
     1..135
     # *** test_uints ***
     # uint 0U
@@ -2103,6 +2103,15 @@ ok 18 - subtests
     ok 12 - mp_snprint max nesting depth result
     # *** test_mp_print: done ***
 ok 19 - subtests
+    1..5
+    # *** test_mp_print_ext ***
+    ok 1 - mp_snrpint size match
+    ok 2 - str is correct
+    ok 3 - mp_fprint size match
+    ok 4 - mp_fprint written correct number of bytes
+    ok 5 - str is correct
+    # *** test_mp_print_ext: done ***
+ok 20 - subtests
     1..65
     # *** test_mp_check ***
     ok 1 - invalid fixmap 1
@@ -2171,7 +2180,7 @@ ok 19 - subtests
     ok 64 - invalid map32 2
     ok 65 - invalid map32 3
     # *** test_mp_check: done ***
-ok 20 - subtests
+ok 21 - subtests
     1..96
     # *** test_numbers ***
     ok 1 - mp_read_int32(mp_encode_uint(123)) check success
@@ -2271,7 +2280,7 @@ ok 20 - subtests
     ok 95 - mp_read_double(mp_encode_strl(100)) check fail
     ok 96 - mp_read_double(mp_encode_strl(100)) check pos unchanged
     # *** test_numbers: done ***
-ok 21 - subtests
+ok 22 - subtests
     1..4
     # *** test_overflow ***
     ok 1 - mp_check array overflow
@@ -2279,4 +2288,4 @@ ok 21 - subtests
     ok 3 - mp_check str overflow
     ok 4 - mp_check bin overflow
     # *** test_overflow: done ***
-ok 22 - subtests
+ok 23 - subtests
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/5] msgpuck: bump version to enable extension printer

[Cyrill Gorcunov]

On Tue, May 12, 2020 at 01:45:48AM +0200, Vladislav Shpilevoy wrote:
> In the new version an API is added to be able to customize MP_EXT
> serializer for mp_snprint() and mp_fprint() functions.
> 
> Part of #4719
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 2/5] decimal: provide MP_DECIMAL extension serializer

[Vladislav Shpilevoy]

Msgpuck functions mp_snprint() and mp_fprint() now support
customizable MP_EXT serializer. This patch adds such for
MP_DECIMAL. It is not activated yet, but will be later, when other
extensions also get their serializers.

Part of #4719
---
 src/lib/core/mp_decimal.c | 18 +++++++++++
 src/lib/core/mp_decimal.h | 27 +++++++++++++++++
 test/unit/decimal.c       | 63 ++++++++++++++++++++++++++++++++++++++-
 test/unit/decimal.result  | 11 ++++++-
 4 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/mp_decimal.c b/src/lib/core/mp_decimal.c
index 985e75291..ffc2c5773 100644
--- a/src/lib/core/mp_decimal.c
+++ b/src/lib/core/mp_decimal.c
@@ -70,3 +70,21 @@ mp_encode_decimal(char *data, const decimal_t *dec)
 	data = decimal_pack(data, dec);
 	return data;
 }
+
+int
+mp_snprint_decimal(char *buf, int size, const char **data, uint32_t len)
+{
+	decimal_t d;
+	if (decimal_unpack(data, len, &d) == NULL)
+		return -1;
+	return snprintf(buf, size, "%s", decimal_to_string(&d));
+}
+
+int
+mp_fprint_decimal(FILE *file, const char **data, uint32_t len)
+{
+	decimal_t d;
+	if (decimal_unpack(data, len, &d) == NULL)
+		return -1;
+	return fprintf(file, "%s", decimal_to_string(&d));
+}
diff --git a/src/lib/core/mp_decimal.h b/src/lib/core/mp_decimal.h
index 778529068..b8a327632 100644
--- a/src/lib/core/mp_decimal.h
+++ b/src/lib/core/mp_decimal.h
@@ -63,6 +63,33 @@ mp_decode_decimal(const char **data, decimal_t *dec);
 char *
 mp_encode_decimal(char *data, const decimal_t *dec);
 
+/**
+ * Print decimal's string representation into a given buffer.
+ * @param buf Target buffer to write string to.
+ * @param size Buffer size.
+ * @param data MessagePack encoded decimal, without MP_EXT header.
+ * @param len Length of @a data. If not all data is used, it is
+ *        an error.
+ * @retval <0 Error. Couldn't decode decimal.
+ * @retval >=0 How many bytes were written, or would have been
+ *        written, if there was enough buffer space.
+ */
+int
+mp_snprint_decimal(char *buf, int size, const char **data, uint32_t len);
+
+/**
+ * Print decimal's string representation into a stream.
+ * @param file Target stream to write string to.
+ * @param data MessagePack encoded decimal, without MP_EXT header.
+ * @param len Length of @a data. If not all data is used, it is
+ *        an error.
+ * @retval <0 Error. Couldn't decode decimal, or couldn't write to
+ *        the stream.
+ * @retval >=0 How many bytes were written.
+ */
+int
+mp_fprint_decimal(FILE *file, const char **data, uint32_t len);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/test/unit/decimal.c b/test/unit/decimal.c
index 254114b5f..179723d02 100644
--- a/test/unit/decimal.c
+++ b/test/unit/decimal.c
@@ -254,10 +254,70 @@ test_to_int(void)
 	return check_plan();
 }
 
+static int
+mp_fprint_ext_test(FILE *file, const char **data, int depth)
+{
+	(void)depth;
+	int8_t type;
+	uint32_t len = mp_decode_extl(data, &type);
+	if (type != MP_DECIMAL)
+		return fprintf(file, "undefined");
+	return mp_fprint_decimal(file, data, len);
+}
+
+static int
+mp_snprint_ext_test(char *buf, int size, const char **data, int depth)
+{
+	(void)depth;
+	int8_t type;
+	uint32_t len = mp_decode_extl(data, &type);
+	if (type != MP_DECIMAL)
+		return snprintf(buf, size, "undefined");
+	return mp_snprint_decimal(buf, size, data, len);
+}
+
+static void
+test_mp_print(void)
+{
+	plan(5);
+	header();
+
+	mp_snprint_ext = mp_snprint_ext_test;
+	mp_fprint_ext = mp_fprint_ext_test;
+
+	char buffer[1024];
+	char str[1024];
+	const char *expected = "1.234";
+	const int len = strlen(expected);
+	decimal_t d;
+	decimal_from_string(&d, expected);
+	mp_encode_decimal(buffer, &d);
+	int rc = mp_snprint(NULL, 0, buffer);
+	is(rc, len, "correct mp_snprint size with empty buffer");
+	rc = mp_snprint(str, sizeof(str), buffer);
+	is(rc, len, "correct mp_snprint size");
+	is(strcmp(str, expected), 0, "correct mp_snprint result");
+
+	FILE *f = tmpfile();
+	rc = mp_fprint(f, buffer);
+	is(rc, len, "correct mp_fprint size");
+	rewind(f);
+	rc = fread(str, 1, sizeof(str), f);
+	str[rc] = 0;
+	is(strcmp(str, expected), 0, "correct mp_fprint result");
+	fclose(f);
+
+	mp_snprint_ext = mp_snprint_ext_default;
+	mp_fprint_ext = mp_fprint_ext_default;
+
+	footer();
+	check_plan();
+}
+
 int
 main(void)
 {
-	plan(281);
+	plan(282);
 
 	dectest(314, 271, uint64, uint64_t);
 	dectest(65535, 23456, uint64, uint64_t);
@@ -322,6 +382,7 @@ main(void)
 	test_pack_unpack();
 
 	test_mp_decimal();
+	test_mp_print();
 
 	return check_plan();
 }
diff --git a/test/unit/decimal.result b/test/unit/decimal.result
index e8432fb36..f11396df3 100644
--- a/test/unit/decimal.result
+++ b/test/unit/decimal.result
@@ -1,4 +1,4 @@
-1..281
+1..282
 ok 1 - decimal(314)
 ok 2 - decimal(271)
 ok 3 - decimal(314) + decimal(271)
@@ -698,3 +698,12 @@ ok 280 - subtests
     ok 197 - decimal_unpack() after mp_decode_extl() value
     ok 198 - decimal_unpack() after mp_decode_extl() len
 ok 281 - subtests
+    1..5
+	*** test_mp_print ***
+    ok 1 - correct mp_snprint size with empty buffer
+    ok 2 - correct mp_snprint size
+    ok 3 - correct mp_snprint result
+    ok 4 - correct mp_fprint size
+    ok 5 - correct mp_fprint result
+	*** test_mp_print: done ***
+ok 282 - subtests
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 2/5] decimal: provide MP_DECIMAL extension serializer

[Cyrill Gorcunov]

On Tue, May 12, 2020 at 01:45:49AM +0200, Vladislav Shpilevoy wrote:
> +
> +int
> +mp_snprint_decimal(char *buf, int size, const char **data, uint32_t len)
> +{
> +	decimal_t d;
> +	if (decimal_unpack(data, len, &d) == NULL)
> +		return -1;
> +	return snprintf(buf, size, "%s", decimal_to_string(&d));
> +}

This looks suspicious -- if buffer size is not enough the snprintf
returns not the number of bytes really written but rather a number
of bytes needed to write the desired string. Are you sure it is
safe to return snprintf result here?

Re: [Tarantool-patches] [PATCH 2/5] decimal: provide MP_DECIMAL extension serializer

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 12/05/2020 17:13, Cyrill Gorcunov wrote:
> On Tue, May 12, 2020 at 01:45:49AM +0200, Vladislav Shpilevoy wrote:
>> +
>> +int
>> +mp_snprint_decimal(char *buf, int size, const char **data, uint32_t len)
>> +{
>> +	decimal_t d;
>> +	if (decimal_unpack(data, len, &d) == NULL)
>> +		return -1;
>> +	return snprintf(buf, size, "%s", decimal_to_string(&d));
>> +}
> 
> This looks suspicious -- if buffer size is not enough the snprintf
> returns not the number of bytes really written but rather a number
> of bytes needed to write the desired string. Are you sure it is
> safe to return snprintf result here?

Yes, I am sure it is safe and correct. mp_snprint() returns exactly this -
an snprintf-like result. So all printers, used inside of it, should do the
same. At least this is what is documented and covered by existing tests.

Re: [Tarantool-patches] [PATCH 2/5] decimal: provide MP_DECIMAL extension serializer

[Cyrill Gorcunov]

On Tue, May 12, 2020 at 10:30:24PM +0200, Vladislav Shpilevoy wrote:
> > 
> > This looks suspicious -- if buffer size is not enough the snprintf
> > returns not the number of bytes really written but rather a number
> > of bytes needed to write the desired string. Are you sure it is
> > safe to return snprintf result here?
> 
> Yes, I am sure it is safe and correct. mp_snprint() returns exactly this -
> an snprintf-like result. So all printers, used inside of it, should do the
> same. At least this is what is documented and covered by existing tests.

Yeah, I realized later than this is don on purpose, that's why I put
my ack in reply.

Re: [Tarantool-patches] [PATCH 2/5] decimal: provide MP_DECIMAL extension serializer

[Cyrill Gorcunov]

On Tue, May 12, 2020 at 01:45:49AM +0200, Vladislav Shpilevoy wrote:
> Msgpuck functions mp_snprint() and mp_fprint() now support
> customizable MP_EXT serializer. This patch adds such for
> MP_DECIMAL. It is not activated yet, but will be later, when other
> extensions also get their serializers.
> 
> Part of #4719
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 3/5] uuid: provide MP_UUID extension serializer

[Vladislav Shpilevoy]

Msgpuck functions mp_snprint() and mp_fprint() now support
customizable MP_EXT serializer. This patch adds such for
MP_UUID. It is not activated yet, but will be later, when last
extension - MP_ERROR - also gets its serializer.

Part of #4719
---
 src/lib/uuid/mp_uuid.c | 18 ++++++++++++
 src/lib/uuid/mp_uuid.h | 27 ++++++++++++++++++
 test/unit/uuid.c       | 64 +++++++++++++++++++++++++++++++++++++++++-
 test/unit/uuid.result  | 11 +++++++-
 4 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/uuid/mp_uuid.c
index 1a9daf6d1..b2341ae36 100644
--- a/src/lib/uuid/mp_uuid.c
+++ b/src/lib/uuid/mp_uuid.c
@@ -95,3 +95,21 @@ mp_encode_uuid(char *data, const struct tt_uuid *uuid)
 	data = mp_encode_extl(data, MP_UUID, UUID_PACKED_LEN);
 	return uuid_pack(data, uuid);
 }
+
+int
+mp_snprint_uuid(char *buf, int size, const char **data, uint32_t len)
+{
+	struct tt_uuid uuid;
+	if (uuid_unpack(data, len, &uuid) == NULL)
+		return -1;
+	return snprintf(buf, size, "%s", tt_uuid_str(&uuid));
+}
+
+int
+mp_fprint_uuid(FILE *file, const char **data, uint32_t len)
+{
+	struct tt_uuid uuid;
+	if (uuid_unpack(data, len, &uuid) == NULL)
+		return -1;
+	return fprintf(file, "%s", tt_uuid_str(&uuid));
+}
diff --git a/src/lib/uuid/mp_uuid.h b/src/lib/uuid/mp_uuid.h
index fdc39f7ef..3cf591848 100644
--- a/src/lib/uuid/mp_uuid.h
+++ b/src/lib/uuid/mp_uuid.h
@@ -85,6 +85,33 @@ mp_decode_uuid(const char **data, struct tt_uuid *uuid);
 char *
 mp_encode_uuid(char *data, const struct tt_uuid *uuid);
 
+/**
+ * Print uuid's string representation into a given buffer.
+ * @param buf Target buffer to write string to.
+ * @param size Buffer size.
+ * @param data MessagePack encoded uuid, without MP_EXT header.
+ * @param len Length of @a data. If not all data is used, it is
+ *        an error.
+ * @retval <0 Error. Couldn't decode uuid.
+ * @retval >=0 How many bytes were written, or would have been
+ *        written, if there was enough buffer space.
+ */
+int
+mp_snprint_uuid(char *buf, int size, const char **data, uint32_t len);
+
+/**
+ * Print uuid's string representation into a stream.
+ * @param file Target stream to write string to.
+ * @param data MessagePack encoded uuid, without MP_EXT header.
+ * @param len Length of @a data. If not all data is used, it is
+ *        an error.
+ * @retval <0 Error. Couldn't decode uuid, or couldn't write to
+ *        the stream.
+ * @retval >=0 How many bytes were written.
+ */
+int
+mp_fprint_uuid(FILE *file, const char **data, uint32_t len);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/test/unit/uuid.c b/test/unit/uuid.c
index b51d13cb8..fdf263092 100644
--- a/test/unit/uuid.c
+++ b/test/unit/uuid.c
@@ -2,6 +2,8 @@
 #include "uuid/tt_uuid.h"
 #include "uuid/mp_uuid.h"
 #include "core/random.h"
+#include "msgpuck/msgpuck.h"
+#include "mp_extension_types.h"
 #include <string.h>
 
 static void
@@ -48,10 +50,69 @@ mp_uuid_test()
         check_plan();
 }
 
+static int
+mp_fprint_ext_test(FILE *file, const char **data, int depth)
+{
+        (void)depth;
+        int8_t type;
+        uint32_t len = mp_decode_extl(data, &type);
+        if (type != MP_UUID)
+                return fprintf(file, "undefined");
+        return mp_fprint_uuid(file, data, len);
+}
+
+static int
+mp_snprint_ext_test(char *buf, int size, const char **data, int depth)
+{
+        (void)depth;
+        int8_t type;
+        uint32_t len = mp_decode_extl(data, &type);
+        if (type != MP_UUID)
+                return snprintf(buf, size, "undefined");
+        return mp_snprint_uuid(buf, size, data, len);
+}
+
+static void
+mp_print_test(void)
+{
+        plan(5);
+        header();
+
+        mp_snprint_ext = mp_snprint_ext_test;
+        mp_fprint_ext = mp_fprint_ext_test;
+
+        char buffer[1024];
+        char str[1024];
+        struct tt_uuid uuid;
+        tt_uuid_create(&uuid);
+
+        mp_encode_uuid(buffer, &uuid);
+        int rc = mp_snprint(NULL, 0, buffer);
+        is(rc, UUID_STR_LEN, "correct mp_snprint size with empty buffer");
+        rc = mp_snprint(str, sizeof(str), buffer);
+        is(rc, UUID_STR_LEN, "correct mp_snprint size");
+        is(strcmp(str, tt_uuid_str(&uuid)), 0, "correct mp_snprint result");
+
+        FILE *f = tmpfile();
+        rc = mp_fprint(f, buffer);
+        is(rc, UUID_STR_LEN, "correct mp_fprint size");
+        rewind(f);
+        rc = fread(str, 1, sizeof(str), f);
+        str[rc] = 0;
+        is(strcmp(str, tt_uuid_str(&uuid)), 0, "correct mp_fprint result");
+        fclose(f);
+
+        mp_snprint_ext = mp_snprint_ext_default;
+        mp_fprint_ext = mp_fprint_ext_default;
+
+        footer();
+        check_plan();
+}
+
 int
 main(void)
 {
-        plan(3);
+        plan(4);
 
         uuid_test(
                 (struct tt_uuid){.time_low = 1712399963,
@@ -85,6 +146,7 @@ main(void)
                 -1);
 
         mp_uuid_test();
+        mp_print_test();
 
         return check_plan();
 }
diff --git a/test/unit/uuid.result b/test/unit/uuid.result
index 50a1140c5..4899ae4a2 100644
--- a/test/unit/uuid.result
+++ b/test/unit/uuid.result
@@ -1,4 +1,4 @@
-1..3
+1..4
 ok 1 - 6611265b-8852-4832-af8b-4164d52c62eb > 186ebbf7-cf97-4e2e-8b1b-76154f6f3949
 ok 2 - 075b4148-8fb0-2e7f-af50-4164d52c62eb < 1fbc929f-5da8-28c5-8b36-76154f6f3949
     1..4
@@ -7,3 +7,12 @@ ok 2 - 075b4148-8fb0-2e7f-af50-4164d52c62eb < 1fbc929f-5da8-28c5-8b36-76154f6f39
     ok 3 - mp_sizeof_uuid() == decoded length
     ok 4 - mp_decode_uuid(mp_encode_uuid(uu)) == uu
 ok 3 - subtests
+    1..5
+	*** mp_print_test ***
+    ok 1 - correct mp_snprint size with empty buffer
+    ok 2 - correct mp_snprint size
+    ok 3 - correct mp_snprint result
+    ok 4 - correct mp_fprint size
+    ok 5 - correct mp_fprint result
+	*** mp_print_test: done ***
+ok 4 - subtests
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 3/5] uuid: provide MP_UUID extension serializer

[Cyrill Gorcunov]

On Tue, May 12, 2020 at 01:45:50AM +0200, Vladislav Shpilevoy wrote:
> Msgpuck functions mp_snprint() and mp_fprint() now support
> customizable MP_EXT serializer. This patch adds such for
> MP_UUID. It is not activated yet, but will be later, when last
> extension - MP_ERROR - also gets its serializer.
> 
> Part of #4719
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

[Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Vladislav Shpilevoy]

Msgpuck functions mp_snprint() and mp_fprint() now support
customizable MP_EXT serializer. This patch adds such for MP_ERROR.
All extension serializers will be activated in a separate commit.

Part of #4719
---
 src/box/mp_error.cc       | 161 ++++++++++++++++++++++-
 src/box/mp_error.h        |  29 ++++
 test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
 test/unit/mp_error.result |  72 +++++++++-
 4 files changed, 529 insertions(+), 3 deletions(-)

diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index 0491a7a50..fed2ce288 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -28,6 +28,8 @@
  * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
+#ifndef MP_ERROR_PRINT_DEFINITION
+
 #include "box/mp_error.h"
 #include "box/error.h"
 #include "mpstream/mpstream.h"
@@ -86,7 +88,8 @@ enum {
 	 * Type-specific fields stored as a map
 	 * {string key = value}.
 	 */
-	MP_ERROR_FIELDS = 0x06
+	MP_ERROR_FIELDS = 0x06,
+	MP_ERROR_MAX,
 };
 
 /**
@@ -552,3 +555,159 @@ error_unpack_unsafe(const char **data)
 	}
 	return err;
 }
+
+#define MP_ERROR_PRINT_DEFINITION
+#define MP_PRINT_FUNC snprintf
+#define MP_PRINT_SUFFIX snprint
+#define MP_PRINT_2(total, func, ...)						\
+	SNPRINT(total, func, buf, size, __VA_ARGS__)
+#define MP_PRINT_ARGS_DECL char *buf, int size
+#include __FILE__
+
+#define MP_ERROR_PRINT_DEFINITION
+#define MP_PRINT_FUNC fprintf
+#define MP_PRINT_SUFFIX fprint
+#define MP_PRINT_2(total, func, ...) do {							\
+	int bytes = func(file, __VA_ARGS__);					\
+	if (bytes < 0)								\
+		return -1;							\
+	total += bytes;								\
+} while (0)
+#define MP_PRINT_ARGS_DECL FILE *file
+#include __FILE__
+
+/* !defined(MP_ERROR_PRINT_DEFINITION) */
+#else
+/* defined(MP_ERROR_PRINT_DEFINITION) */
+
+/**
+ * MP_ERROR extension string serializer.
+ * There are two applications for string serialization - into a
+ * buffer, and into a file. Structure of both is exactly the same
+ * except for the copying/writing itself. To avoid code
+ * duplication the code is templated and expects some macros to do
+ * the actual output.
+ */
+
+#define MP_CONCAT4_R(a, b, c, d) a##b##c##d
+#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d)
+#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)
+
+#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
+#define mp_print_error_one mp_func_name(error_one)
+#define mp_print_error_stack mp_func_name(error_stack)
+#define mp_print_error mp_func_name(error)
+#define mp_print_common mp_func_name(recursion)
+
+static int
+mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
+{
+	int total = 0;
+	MP_PRINT(total, "{");
+	if (depth <= 0) {
+		MP_PRINT(total, "...}");
+		return total;
+	}
+	const char *field_to_key[MP_ERROR_MAX] = {
+		/* MP_ERROR_TYPE = */ "\"type\": ",
+		/* MP_ERROR_FILE = */ "\"file\": ",
+		/* MP_ERROR_LINE = */ "\"line\": ",
+		/* MP_ERROR_MESSAGE = */ "\"message\": ",
+		/* MP_ERROR_ERRNO = */ "\"errno\": ",
+		/* MP_ERROR_CODE = */ "\"code\": ",
+		/* MP_ERROR_FIELDS = */ "\"fields\": ",
+	};
+	--depth;
+	if (mp_typeof(**data) != MP_MAP)
+		return -1;
+	uint32_t map_size = mp_decode_map(data);
+	for (uint32_t i = 0; i < map_size; ++i) {
+		if (i != 0)
+			MP_PRINT(total, ", ");
+		if (mp_typeof(**data) != MP_UINT)
+			return -1;
+		uint64_t key = mp_decode_uint(data);
+		if (key < MP_ERROR_MAX)
+			MP_PRINT(total, "%s", field_to_key[key]);
+		else
+			MP_PRINT(total, "%llu: ", key);
+		MP_PRINT_2(total, mp_print_common, data, depth);
+	}
+	MP_PRINT(total, "}");
+	return total;
+}
+
+static int
+mp_print_error_stack(MP_PRINT_ARGS_DECL, const char **data, int depth)
+{
+	int total = 0;
+	MP_PRINT(total, "[");
+	if (depth <= 0) {
+		MP_PRINT(total, "...]");
+		return total;
+	}
+	--depth;
+	if (mp_typeof(**data) != MP_ARRAY)
+		return -1;
+	uint32_t arr_size = mp_decode_array(data);
+	for (uint32_t i = 0; i < arr_size; ++i) {
+		if (i != 0)
+			MP_PRINT(total, ", ");
+		MP_PRINT_2(total, mp_print_error_one, data, depth);
+	}
+	MP_PRINT(total, "]");
+	return total;
+}
+
+int
+mp_print_error(MP_PRINT_ARGS_DECL, const char **data, int depth)
+{
+	int total = 0;
+	MP_PRINT(total, "{");
+	if (depth <= 0) {
+		MP_PRINT(total, "...}");
+		return total;
+	}
+	--depth;
+	if (mp_typeof(**data) != MP_MAP)
+		return -1;
+	uint32_t map_size = mp_decode_map(data);
+	for (uint32_t i = 0; i < map_size; ++i) {
+		if (i != 0)
+			MP_PRINT(total, ", ");
+		if (mp_typeof(**data) != MP_UINT)
+			return -1;
+		uint64_t key = mp_decode_uint(data);
+		switch(key) {
+		case MP_ERROR_STACK: {
+			MP_PRINT(total, "\"stack\": ");
+			MP_PRINT_2(total, mp_print_error_stack, data, depth);
+			break;
+		}
+		default:
+			MP_PRINT(total, "%llu: ", key);
+			MP_PRINT_2(total, mp_print_common, data, depth);
+			break;
+		}
+	}
+	MP_PRINT(total, "}");
+	return total;
+}
+
+#undef MP_PRINT
+#undef MP_CONCAT4_R
+#undef MP_CONCAT4
+
+#undef mp_func_name
+#undef mp_print_error_one
+#undef mp_print_error_stack
+#undef mp_print_error
+#undef mp_print_common
+
+#undef MP_ERROR_PRINT_DEFINITION
+#undef MP_PRINT_FUNC
+#undef MP_PRINT_SUFFIX
+#undef MP_PRINT_2
+#undef MP_PRINT_ARGS_DECL
+
+#endif /* defined(MP_ERROR_PRINT_DEFINITION) */
diff --git a/src/box/mp_error.h b/src/box/mp_error.h
index f040e0fc7..a4a82ecb7 100644
--- a/src/box/mp_error.h
+++ b/src/box/mp_error.h
@@ -35,9 +35,11 @@ extern "C" {
 #endif /* defined(__cplusplus) */
 
 #include <stdint.h>
+#include <stdio.h>
 #include <assert.h>
 
 struct mpstream;
+struct error;
 
 /**
  * @brief Encode error to mpstream as MP_ERROR.
@@ -69,6 +71,33 @@ error_unpack(const char **data, uint32_t len)
 	return e;
 }
 
+/**
+ * Print error's string representation into a given buffer.
+ * @param buf Target buffer to write string to.
+ * @param size Buffer size.
+ * @param data MessagePack encoded error, without MP_EXT header.
+ * @param len Length of @a data. If not all data is used, it is
+ *        an error.
+ * @retval <0 Error. Couldn't decode error.
+ * @retval >=0 How many bytes were written, or would have been
+ *        written, if there was enough buffer space.
+ */
+int
+mp_snprint_error(char *buf, int size, const char **data, int depth);
+
+/**
+ * Print error's string representation into a stream.
+ * @param file Target stream to write string to.
+ * @param data MessagePack encoded error, without MP_EXT header.
+ * @param len Length of @a data. If not all data is used, it is
+ *        an error.
+ * @retval <0 Error. Couldn't decode error, or couldn't write to
+ *        the stream.
+ * @retval >=0 How many bytes were written.
+ */
+int
+mp_fprint_error(FILE *file, const char **data, int depth);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/test/unit/mp_error.cc b/test/unit/mp_error.cc
index becabb370..e85d282e7 100644
--- a/test/unit/mp_error.cc
+++ b/test/unit/mp_error.cc
@@ -33,7 +33,10 @@
 #include "fiber.h"
 #include "memory.h"
 #include "msgpuck.h"
+#include "mp_extension_types.h"
 #include "tt_static.h"
+#include "small/ibuf.h"
+#include "mpstream/mpstream.h"
 
 #include "box/error.h"
 #include "box/mp_error.h"
@@ -452,11 +455,275 @@ test_unknown_additional_fields()
 	footer();
 }
 
+static int
+mp_fprint_ext_test(FILE *file, const char **data, int depth)
+{
+	int8_t type;
+	mp_decode_extl(data, &type);
+	if (type != MP_ERROR)
+		return fprintf(file, "undefined");
+	return mp_fprint_error(file, data, depth);
+}
+
+static int
+mp_snprint_ext_test(char *buf, int size, const char **data, int depth)
+{
+	int8_t type;
+	mp_decode_extl(data, &type);
+	if (type != MP_ERROR)
+		return snprintf(buf, size, "undefined");
+	return mp_snprint_error(buf, size, data, depth);
+}
+
+static void
+test_mp_print_check_str(int depth, const char *str, int len,
+			const char *expected, const char *method)
+{
+	is(len, (int) strlen(str), "%s depth %d correct returned value", method,
+	   depth);
+	int expected_len = strlen(expected);
+	is(len, depth * 2 + expected_len, "%s depth %d correct length", method,
+	   depth);
+	bool is_error = false;
+	/*
+	 * Deep encoding is simulated with a number of nested
+	 * arrays. In string representation they look like:
+	 *
+	 *   [[[[[[[ ... object ... ]]]]]]]
+	 *
+	 */
+	for (int i = 0; i < depth && !is_error; ++i)
+		is_error = str[i] != '[' || str[len - 1 - i] != ']';
+	ok(!is_error, "%s depth %d correct prefix and suffix", method, depth);
+	str += depth;
+	is(strncmp(str, expected, expected_len), 0, "%s depth %d correct "
+	   "object in the middle", method, depth);
+}
+
+static void
+mpstream_error_test(void *ctx)
+{
+	*((bool *)ctx) = true;
+}
+
+static void
+test_mp_print_encode_error(struct ibuf *buf, struct error *e, int depth)
+{
+	struct mpstream stream;
+	bool is_error = false;
+	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
+		      mpstream_error_test, &is_error);
+	for (int i = 0; i < depth; ++i)
+		mpstream_encode_array(&stream, 1);
+	error_to_mpstream(e, &stream);
+	mpstream_flush(&stream);
+	fail_if(is_error);
+}
+
+static void
+test_mp_print_check(const char *data, int depth, const char *expected)
+{
+	char str[2048];
+	int rc = mp_snprint(str, sizeof(str), data);
+	test_mp_print_check_str(depth, str, rc, expected, "mp_snprint");
+	int rc2 = mp_snprint(NULL, 0, data);
+	is(rc, rc2, "mp_snprint depth %d correct with NULL buffer", depth);
+
+	FILE *f = tmpfile();
+	rc = mp_fprint(f, data);
+	rewind(f);
+	rc2 = fread(str, 1, TT_STATIC_BUF_LEN, f);
+	is(rc, rc2, "mp_fprint depth %d result and the actual file size "
+	   "are equal", depth);
+	str[rc] = 0;
+	fclose(f);
+	test_mp_print_check_str(depth, str, rc, expected, "mp_fprint");
+}
+
+static void
+test_mp_print(void)
+{
+	header();
+	plan(60);
+
+	mp_snprint_ext = mp_snprint_ext_test;
+	mp_fprint_ext = mp_fprint_ext_test;
+
+	struct error *e1 = BuildClientError("file1", 1, 0);
+	error_ref(e1);
+	struct error *e2 = BuildCustomError("file2", 4, "type", 5);
+	struct error *e3 = BuildAccessDeniedError("file3", 6, "field1",
+						  "field2", "field3", "field4");
+	error_set_prev(e1, e2);
+	error_set_prev(e2, e3);
+
+	struct ibuf buf;
+	ibuf_create(&buf, &cord()->slabc, 1024);
+
+	note("zero depth, normal error");
+	int depth = 0;
+	const char *expected = "{"
+		"\"stack\": ["
+			"{"
+				"\"type\": \"ClientError\", "
+				"\"line\": 1, "
+				"\"file\": \"file1\", "
+				"\"message\": \"Unknown error\", "
+				"\"errno\": 0, "
+				"\"code\": 0"
+			"}, "
+			"{"
+				"\"type\": \"CustomError\", "
+				"\"line\": 4, "
+				"\"file\": \"file2\", "
+				"\"message\": \"\", "
+				"\"errno\": 0, "
+				"\"code\": 5, "
+				"\"fields\": {"
+					"\"custom_type\": \"type\""
+				"}"
+			"}, "
+			"{"
+				"\"type\": \"AccessDeniedError\", "
+				"\"line\": 6, "
+				"\"file\": \"file3\", "
+				"\"message\": \"field1 access to field2 "
+					"'field3' is denied for user "
+					"'field4'\", "
+				"\"errno\": 0, "
+				"\"code\": 42, "
+				"\"fields\": {"
+					"\"object_type\": \"field2\", "
+					"\"object_name\": \"field3\", "
+					"\"access_type\": \"field1\""
+				"}"
+			"}"
+		"]"
+	"}";
+	test_mp_print_encode_error(&buf, e1, depth);
+	test_mp_print_check(buf.rpos, depth, expected);
+	ibuf_reset(&buf);
+
+	note("max depth, all is truncated");
+	depth = MP_PRINT_MAX_DEPTH;
+	expected = "{...}";
+	test_mp_print_encode_error(&buf, e1, depth);
+	test_mp_print_check(buf.rpos, depth, expected);
+	ibuf_reset(&buf);
+
+	note("max depth - 1, top level of keys is visible");
+	depth = MP_PRINT_MAX_DEPTH - 1;
+	expected = "{\"stack\": [...]}";
+	test_mp_print_encode_error(&buf, e1, depth);
+	test_mp_print_check(buf.rpos, depth, expected);
+	ibuf_reset(&buf);
+
+	note("max depth - 2, top level of keys and error count are visible");
+	depth = MP_PRINT_MAX_DEPTH - 2;
+	expected = "{\"stack\": [{...}, {...}, {...}]}";
+	test_mp_print_encode_error(&buf, e1, depth);
+	test_mp_print_check(buf.rpos, depth, expected);
+	ibuf_reset(&buf);
+
+	note("max depth - 3, all except additional fields is visible");
+	depth = MP_PRINT_MAX_DEPTH - 3;
+	expected = "{"
+		"\"stack\": ["
+			"{"
+				"\"type\": \"ClientError\", "
+				"\"line\": 1, "
+				"\"file\": \"file1\", "
+				"\"message\": \"Unknown error\", "
+				"\"errno\": 0, "
+				"\"code\": 0"
+			"}, "
+			"{"
+				"\"type\": \"CustomError\", "
+				"\"line\": 4, "
+				"\"file\": \"file2\", "
+				"\"message\": \"\", "
+				"\"errno\": 0, "
+				"\"code\": 5, "
+				"\"fields\": {...}"
+			"}, "
+			"{"
+				"\"type\": \"AccessDeniedError\", "
+				"\"line\": 6, "
+				"\"file\": \"file3\", "
+				"\"message\": \"field1 access to field2 "
+					"'field3' is denied for user "
+					"'field4'\", "
+				"\"errno\": 0, "
+				"\"code\": 42, "
+				"\"fields\": {...}"
+			"}"
+		"]"
+	"}";
+	test_mp_print_encode_error(&buf, e1, depth);
+	test_mp_print_check(buf.rpos, depth, expected);
+	ibuf_reset(&buf);
+
+	note("zero depth, error with unknown fields");
+	ibuf_reserve(&buf, 2048);
+	char *begin = buf.rpos + 10;
+	char *data = mp_encode_map(begin, 2);
+	data = mp_encode_uint(data, 4096);
+	data = mp_encode_double(data, 1.234);
+	data = mp_encode_uint(data, MP_ERROR_STACK);
+	data = mp_encode_array(data, 1);
+	struct mp_test_error err;
+	memset(&err, 0, sizeof(err));
+	err.code = 42;
+	err.line = 3;
+	err.saved_errno = 4;
+	err.type = "AccessDeniedError";
+	err.file = "File";
+	err.message = "Message";
+	err.ad_object_type = "ObjectType";
+	err.ad_object_name = "ObjectName";
+	err.ad_access_type = "AccessType";
+	err.unknown_uint_field = 300;
+	err.unknown_str_field = "unknown_field";
+	data = mp_encode_mp_error(&err, data);
+	uint32_t size = data - begin;
+	begin -= mp_sizeof_extl(size);
+	mp_encode_extl(begin, MP_ERROR, size);
+	expected = "{"
+		"4096: 1.234, "
+		"\"stack\": ["
+			"{"
+				"\"type\": \"AccessDeniedError\", "
+				"\"file\": \"File\", "
+				"\"line\": 3, "
+				"\"message\": \"Message\", "
+				"\"errno\": 4, "
+				"\"code\": 42, "
+				"18446744073709551615: 300, "
+				"\"fields\": {"
+					"\"object_type\": \"ObjectType\", "
+					"\"object_name\": \"ObjectName\", "
+					"\"access_type\": \"AccessType\", "
+					"\"unknown_field\": \"unknown_field\""
+				"}"
+			"}"
+		"]"
+	"}";
+	test_mp_print_check(begin, 0, expected);
+
+	error_unref(e1);
+	ibuf_destroy(&buf);
+	mp_snprint_ext = mp_snprint_ext_default;
+	mp_fprint_ext = mp_fprint_ext_default;
+
+	check_plan();
+	footer();
+}
+
 int
 main(void)
 {
 	header();
-	plan(5);
+	plan(6);
 	memory_init();
 	fiber_init(fiber_c_invoke);
 
@@ -465,6 +732,7 @@ main(void)
 	test_fail_not_enough_fields();
 	test_unknown_fields();
 	test_unknown_additional_fields();
+	test_mp_print();
 
 	fiber_free();
 	memory_free();
diff --git a/test/unit/mp_error.result b/test/unit/mp_error.result
index 6ef37b4d5..0582458d3 100644
--- a/test/unit/mp_error.result
+++ b/test/unit/mp_error.result
@@ -1,5 +1,5 @@
 	*** main ***
-1..5
+1..6
 	*** test_stack_error_decode ***
     1..17
     ok 1 - check CustomError
@@ -41,4 +41,74 @@ ok 4 - subtests
     ok 1 - check unknown additional field
 ok 5 - subtests
 	*** test_unknown_additional_fields: done ***
+	*** test_mp_print ***
+    1..60
+    # zero depth, normal error
+    ok 1 - mp_snprint depth 0 correct returned value
+    ok 2 - mp_snprint depth 0 correct length
+    ok 3 - mp_snprint depth 0 correct prefix and suffix
+    ok 4 - mp_snprint depth 0 correct object in the middle
+    ok 5 - mp_snprint depth 0 correct with NULL buffer
+    ok 6 - mp_fprint depth 0 result and the actual file size are equal
+    ok 7 - mp_fprint depth 0 correct returned value
+    ok 8 - mp_fprint depth 0 correct length
+    ok 9 - mp_fprint depth 0 correct prefix and suffix
+    ok 10 - mp_fprint depth 0 correct object in the middle
+    # max depth, all is truncated
+    ok 11 - mp_snprint depth 32 correct returned value
+    ok 12 - mp_snprint depth 32 correct length
+    ok 13 - mp_snprint depth 32 correct prefix and suffix
+    ok 14 - mp_snprint depth 32 correct object in the middle
+    ok 15 - mp_snprint depth 32 correct with NULL buffer
+    ok 16 - mp_fprint depth 32 result and the actual file size are equal
+    ok 17 - mp_fprint depth 32 correct returned value
+    ok 18 - mp_fprint depth 32 correct length
+    ok 19 - mp_fprint depth 32 correct prefix and suffix
+    ok 20 - mp_fprint depth 32 correct object in the middle
+    # max depth - 1, top level of keys is visible
+    ok 21 - mp_snprint depth 31 correct returned value
+    ok 22 - mp_snprint depth 31 correct length
+    ok 23 - mp_snprint depth 31 correct prefix and suffix
+    ok 24 - mp_snprint depth 31 correct object in the middle
+    ok 25 - mp_snprint depth 31 correct with NULL buffer
+    ok 26 - mp_fprint depth 31 result and the actual file size are equal
+    ok 27 - mp_fprint depth 31 correct returned value
+    ok 28 - mp_fprint depth 31 correct length
+    ok 29 - mp_fprint depth 31 correct prefix and suffix
+    ok 30 - mp_fprint depth 31 correct object in the middle
+    # max depth - 2, top level of keys and error count are visible
+    ok 31 - mp_snprint depth 30 correct returned value
+    ok 32 - mp_snprint depth 30 correct length
+    ok 33 - mp_snprint depth 30 correct prefix and suffix
+    ok 34 - mp_snprint depth 30 correct object in the middle
+    ok 35 - mp_snprint depth 30 correct with NULL buffer
+    ok 36 - mp_fprint depth 30 result and the actual file size are equal
+    ok 37 - mp_fprint depth 30 correct returned value
+    ok 38 - mp_fprint depth 30 correct length
+    ok 39 - mp_fprint depth 30 correct prefix and suffix
+    ok 40 - mp_fprint depth 30 correct object in the middle
+    # max depth - 3, all except additional fields is visible
+    ok 41 - mp_snprint depth 29 correct returned value
+    ok 42 - mp_snprint depth 29 correct length
+    ok 43 - mp_snprint depth 29 correct prefix and suffix
+    ok 44 - mp_snprint depth 29 correct object in the middle
+    ok 45 - mp_snprint depth 29 correct with NULL buffer
+    ok 46 - mp_fprint depth 29 result and the actual file size are equal
+    ok 47 - mp_fprint depth 29 correct returned value
+    ok 48 - mp_fprint depth 29 correct length
+    ok 49 - mp_fprint depth 29 correct prefix and suffix
+    ok 50 - mp_fprint depth 29 correct object in the middle
+    # zero depth, error with unknown fields
+    ok 51 - mp_snprint depth 0 correct returned value
+    ok 52 - mp_snprint depth 0 correct length
+    ok 53 - mp_snprint depth 0 correct prefix and suffix
+    ok 54 - mp_snprint depth 0 correct object in the middle
+    ok 55 - mp_snprint depth 0 correct with NULL buffer
+    ok 56 - mp_fprint depth 0 result and the actual file size are equal
+    ok 57 - mp_fprint depth 0 correct returned value
+    ok 58 - mp_fprint depth 0 correct length
+    ok 59 - mp_fprint depth 0 correct prefix and suffix
+    ok 60 - mp_fprint depth 0 correct object in the middle
+ok 6 - subtests
+	*** test_mp_print: done ***
 	*** main: done ***
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Cyrill Gorcunov]

On Tue, May 12, 2020 at 01:45:51AM +0200, Vladislav Shpilevoy wrote:
> Msgpuck functions mp_snprint() and mp_fprint() now support
> customizable MP_EXT serializer. This patch adds such for MP_ERROR.
> All extension serializers will be activated in a separate commit.
> 
> Part of #4719
> ---
>  src/box/mp_error.cc       | 161 ++++++++++++++++++++++-
>  src/box/mp_error.h        |  29 ++++
>  test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
>  test/unit/mp_error.result |  72 +++++++++-
>  4 files changed, 529 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
> index 0491a7a50..fed2ce288 100644
> --- a/src/box/mp_error.cc
> +++ b/src/box/mp_error.cc
> @@ -28,6 +28,8 @@
>   * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
> +#ifndef MP_ERROR_PRINT_DEFINITION
> +
>  #include "box/mp_error.h"
>  #include "box/error.h"
>  #include "mpstream/mpstream.h"
> @@ -86,7 +88,8 @@ enum {
>  	 * Type-specific fields stored as a map
>  	 * {string key = value}.
>  	 */
> -	MP_ERROR_FIELDS = 0x06
> +	MP_ERROR_FIELDS = 0x06,
> +	MP_ERROR_MAX,
>  };
>  
>  /**
> @@ -552,3 +555,159 @@ error_unpack_unsafe(const char **data)
>  	}
>  	return err;
>  }
> +
> +#define MP_ERROR_PRINT_DEFINITION
> +#define MP_PRINT_FUNC snprintf
> +#define MP_PRINT_SUFFIX snprint
> +#define MP_PRINT_2(total, func, ...)						\
> +	SNPRINT(total, func, buf, size, __VA_ARGS__)
> +#define MP_PRINT_ARGS_DECL char *buf, int size
> +#include __FILE__

Don't get it -- include current file?! This a good sign of
some kind of problem in the code structure... If we need
a template then it should be some .cc/.c file explicily
included instead of __FILE__, no?

> +#define MP_ERROR_PRINT_DEFINITION
> +#define MP_PRINT_FUNC fprintf
> +#define MP_PRINT_SUFFIX fprint
> +#define MP_PRINT_2(total, func, ...) do {							\
> +	int bytes = func(file, __VA_ARGS__);					\
> +	if (bytes < 0)								\
> +		return -1;							\
> +	total += bytes;								\
> +} while (0)
> +#define MP_PRINT_ARGS_DECL FILE *file
> +#include __FILE__
> +
> +/* !defined(MP_ERROR_PRINT_DEFINITION) */
> +#else
> +/* defined(MP_ERROR_PRINT_DEFINITION) */
> +
> +/**
> + * MP_ERROR extension string serializer.
> + * There are two applications for string serialization - into a
> + * buffer, and into a file. Structure of both is exactly the same
> + * except for the copying/writing itself. To avoid code
> + * duplication the code is templated and expects some macros to do
> + * the actual output.
> + */
> +
> +#define MP_CONCAT4_R(a, b, c, d) a##b##c##d
> +#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d)
> +#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)
> +
> +#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
> +#define mp_print_error_one mp_func_name(error_one)
> +#define mp_print_error_stack mp_func_name(error_stack)
> +#define mp_print_error mp_func_name(error)
> +#define mp_print_common mp_func_name(recursion)

Maybe we should align the assignments to be able to read it, like

#define MP_CONCAT4_R(a, b, c, d)	a##b##c##d
#define MP_CONCAT4(a, b, c, d)		MP_CONCAT4_R(a, b, c, d)
#define MP_PRINT(total, ...)		MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)

#define mp_func_name(name)	MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
#define mp_print_error_one	mp_func_name(error_one)
#define mp_print_error_stack	mp_func_name(error_stack)
#define mp_print_error		mp_func_name(error)
#define mp_print_common		mp_func_name(recursion)

> +
> +static int
> +mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
> +{
> +	int total = 0;
> +	MP_PRINT(total, "{");
> +	if (depth <= 0) {
> +		MP_PRINT(total, "...}");
> +		return total;
> +	}
> +	const char *field_to_key[MP_ERROR_MAX] = {
> +		/* MP_ERROR_TYPE = */ "\"type\": ",
> +		/* MP_ERROR_FILE = */ "\"file\": ",
> +		/* MP_ERROR_LINE = */ "\"line\": ",
> +		/* MP_ERROR_MESSAGE = */ "\"message\": ",
> +		/* MP_ERROR_ERRNO = */ "\"errno\": ",
> +		/* MP_ERROR_CODE = */ "\"code\": ",
> +		/* MP_ERROR_FIELDS = */ "\"fields\": ",
> +	};

Please use designited initializers in a readable way
instead of this bloody mess

	const char *field_to_key[] = {
		[MP_ERROR_TYPE]		= "\"type\": ",
		...
		[MP_ERROR_FILE]		= ...,
		...
		[MP_ERROR_FIELDS]	= "\"fields\": ",
	}

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Vladislav Shpilevoy]

Thanks for the review!

On 12/05/2020 19:52, Cyrill Gorcunov wrote:
> On Tue, May 12, 2020 at 01:45:51AM +0200, Vladislav Shpilevoy wrote:
>> Msgpuck functions mp_snprint() and mp_fprint() now support
>> customizable MP_EXT serializer. This patch adds such for MP_ERROR.
>> All extension serializers will be activated in a separate commit.
>>
>> Part of #4719
>> ---
>>  src/box/mp_error.cc       | 161 ++++++++++++++++++++++-
>>  src/box/mp_error.h        |  29 ++++
>>  test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
>>  test/unit/mp_error.result |  72 +++++++++-
>>  4 files changed, 529 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
>> index 0491a7a50..fed2ce288 100644
>> --- a/src/box/mp_error.cc
>> +++ b/src/box/mp_error.cc
>> @@ -552,3 +555,159 @@ error_unpack_unsafe(const char **data)
>>  	}
>>  	return err;
>>  }
>> +
>> +#define MP_ERROR_PRINT_DEFINITION
>> +#define MP_PRINT_FUNC snprintf
>> +#define MP_PRINT_SUFFIX snprint
>> +#define MP_PRINT_2(total, func, ...)						\
>> +	SNPRINT(total, func, buf, size, __VA_ARGS__)
>> +#define MP_PRINT_ARGS_DECL char *buf, int size
>> +#include __FILE__
> 
> Don't get it -- include current file?! This a good sign of
> some kind of problem in the code structure... If we need
> a template then it should be some .cc/.c file explicily
> included instead of __FILE__, no?

I don't want to introduce a new file just for these 2 functions
(mp_fprint_error() and mp_snprint_error()). This would be even
worse than it is now. So I used the same file. I wrote __FILE__
instead of mp_error.cc, so as to minimize diff, when mp_error.cc
will be converted to C and moved to lib/core (that is planned).
Also #include __FILE__ is more clear, when you want to say
"include self", IMO.

Alternative is to make mp_print_error_one(), mp_print_error_stack(),
mp_print_error() as huge macros, but I don't like writing big macros.
It is hard to edit, and much much harder to debug, since this all
becomes just one infinite line, and 'n' in gdb/lldb skips the whole
function.

If you don't like the whole template idea, there is one alternative
I described in the cover letter for the msgpuck patchset - virtual
printer. But it will be really slow. We could probably try to make it
buffered, but then it will be double-copying, also slow. I tried really
hard to find other solutions, but failed. Even looked at fmemopen(),
so as we would need to implement only mp_fprint(), but msgpuck library
is supposed to work on Windows too. Also fmemopen() is basically a
virtual printer as well (i.e. is slower, than direct snprint()).

So if you have anything in mind except templates, or a virtual printer,
or you know something about fmemopen() what I don't know, please, tell.
Because I don't like these solutions, but just couldn't find anything
better, with bearable complexity worth the feature. Which I thought would
be just one-day patch, but I was very very wrong here.

I could switch to the virtual printer in case you think its slowness is
worth the simplification.

As unbearable level of complexity I mean the non-binary tree solution
I proposed in the msgpuck patchset's cover letter.

>> +#define MP_ERROR_PRINT_DEFINITION
>> +#define MP_PRINT_FUNC fprintf
>> +#define MP_PRINT_SUFFIX fprint
>> +#define MP_PRINT_2(total, func, ...) do {							\
>> +	int bytes = func(file, __VA_ARGS__);					\
>> +	if (bytes < 0)								\
>> +		return -1;							\
>> +	total += bytes;								\
>> +} while (0)
>> +#define MP_PRINT_ARGS_DECL FILE *file
>> +#include __FILE__
>> +
>> +/* !defined(MP_ERROR_PRINT_DEFINITION) */
>> +#else
>> +/* defined(MP_ERROR_PRINT_DEFINITION) */
>> +
>> +/**
>> + * MP_ERROR extension string serializer.
>> + * There are two applications for string serialization - into a
>> + * buffer, and into a file. Structure of both is exactly the same
>> + * except for the copying/writing itself. To avoid code
>> + * duplication the code is templated and expects some macros to do
>> + * the actual output.
>> + */
>> +
>> +#define MP_CONCAT4_R(a, b, c, d) a##b##c##d
>> +#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d)
>> +#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)
>> +
>> +#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
>> +#define mp_print_error_one mp_func_name(error_one)
>> +#define mp_print_error_stack mp_func_name(error_stack)
>> +#define mp_print_error mp_func_name(error)
>> +#define mp_print_common mp_func_name(recursion)
> 
> Maybe we should align the assignments to be able to read it, like

I am pretty much able to read it as is, and moreover our code style
does not contain any kinds of such alignments. I explained that all
already in public chats, in private, in emails. Not going to describe
pros and cons again.

I applied the alignment, and faced the problems right away - with equal
alignment for MP_CONCAT4_R, MP_CONCAT4, and MP_PRINT one of the lines
becomes too big, out of 80 symbols. On the other hand with not equal
alignments it would not look better than now.

The mp_func_name() definitions with +2 tabs fall out of 80 as well, but
with +1 the space is too short - any a bit longer name will violate
the alignment in future causing either big unnecessary diff or alignment
violation.

> #define MP_CONCAT4_R(a, b, c, d)	a##b##c##d
> #define MP_CONCAT4(a, b, c, d)		MP_CONCAT4_R(a, b, c, d)
> #define MP_PRINT(total, ...)		MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)
> 
> #define mp_func_name(name)	MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
> #define mp_print_error_one	mp_func_name(error_one)
> #define mp_print_error_stack	mp_func_name(error_stack)
> #define mp_print_error		mp_func_name(error)
> #define mp_print_common		mp_func_name(recursion)
> 
>> +
>> +static int
>> +mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
>> +{
>> +	int total = 0;
>> +	MP_PRINT(total, "{");
>> +	if (depth <= 0) {
>> +		MP_PRINT(total, "...}");
>> +		return total;
>> +	}
>> +	const char *field_to_key[MP_ERROR_MAX] = {
>> +		/* MP_ERROR_TYPE = */ "\"type\": ",
>> +		/* MP_ERROR_FILE = */ "\"file\": ",
>> +		/* MP_ERROR_LINE = */ "\"line\": ",
>> +		/* MP_ERROR_MESSAGE = */ "\"message\": ",
>> +		/* MP_ERROR_ERRNO = */ "\"errno\": ",
>> +		/* MP_ERROR_CODE = */ "\"code\": ",
>> +		/* MP_ERROR_FIELDS = */ "\"fields\": ",
>> +	};
> 
> Please use designited initializers in a readable way
> instead of this bloody mess
> 
> 	const char *field_to_key[] = {
> 		[MP_ERROR_TYPE]		= "\"type\": ",
> 		...
> 		[MP_ERROR_FILE]		= ...,
> 		...
> 		[MP_ERROR_FIELDS]	= "\"fields\": ",
> 	}

Once again - this is not a mess, this is our codestyle. If you
don't like it, you should talk to somebody responsible for our
SOP to change it. We can't write our code in individual code
styles, and silently hope to push the old style away eventually.
If you want changes, they need to go through our documentation
and the responsible person. This is Kirill Y., as I remember.

Talking of [<number>] = <value> initializer - I didn't know you
can do that in C. Cool, thanks. Now it should be safer.

I applied the alignments, but I won't push it into the master until,
of course, the branch gets all ACKs, *and* until we agree to either
align all new code, or don't align such places at all. In the latter
case I will return the old version.

====================

diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index fed2ce288..0b5e6bc96 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -589,15 +589,16 @@ error_unpack_unsafe(const char **data)
  * the actual output.
  */
 
-#define MP_CONCAT4_R(a, b, c, d) a##b##c##d
-#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d)
-#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)
+#define MP_CONCAT4_R(a, b, c, d)	a##b##c##d
+#define MP_CONCAT4(a, b, c, d)		MP_CONCAT4_R(a, b, c, d)
+#define MP_PRINT(total, ...)		MP_PRINT_2(total, MP_PRINT_FUNC,	\
+						   __VA_ARGS__)
 
-#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
-#define mp_print_error_one mp_func_name(error_one)
-#define mp_print_error_stack mp_func_name(error_stack)
-#define mp_print_error mp_func_name(error)
-#define mp_print_common mp_func_name(recursion)
+#define mp_func_name(name)	MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
+#define mp_print_error_one	mp_func_name(error_one)
+#define mp_print_error_stack	mp_func_name(error_stack)
+#define mp_print_error		mp_func_name(error)
+#define mp_print_common		mp_func_name(recursion)
====================

I don't know why the line above is screwed. In source it looks ok.
Probably due to '+' in the beginning, added by git. One another
reason against alignments - they will look shitty in git diff, show,
etc.

====================
 
 static int
 mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
@@ -609,13 +610,13 @@ mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
 		return total;
 	}
 	const char *field_to_key[MP_ERROR_MAX] = {
-		/* MP_ERROR_TYPE = */ "\"type\": ",
-		/* MP_ERROR_FILE = */ "\"file\": ",
-		/* MP_ERROR_LINE = */ "\"line\": ",
-		/* MP_ERROR_MESSAGE = */ "\"message\": ",
-		/* MP_ERROR_ERRNO = */ "\"errno\": ",
-		/* MP_ERROR_CODE = */ "\"code\": ",
-		/* MP_ERROR_FIELDS = */ "\"fields\": ",
+		[MP_ERROR_TYPE] =	"\"type\": ",
+		[MP_ERROR_FILE] =	"\"file\": ",
+		[MP_ERROR_LINE] =	"\"line\": ",
+		[MP_ERROR_MESSAGE] =	"\"message\": ",
+		[MP_ERROR_ERRNO] =	"\"errno\": ",
+		[MP_ERROR_CODE] =	"\"code\": ",
+		[MP_ERROR_FIELDS] =	"\"fields\": ",
 	};
 	--depth;
 	if (mp_typeof(**data) != MP_MAP)

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Cyrill Gorcunov]

On Tue, May 12, 2020 at 10:38:25PM +0200, Vladislav Shpilevoy wrote:
...
> > 
> > Don't get it -- include current file?! This a good sign of
> > some kind of problem in the code structure... If we need
> > a template then it should be some .cc/.c file explicily
> > included instead of __FILE__, no?
> 
> I don't want to introduce a new file just for these 2 functions
> (mp_fprint_error() and mp_snprint_error()). This would be even
> worse than it is now. So I used the same file. I wrote __FILE__
> instead of mp_error.cc, so as to minimize diff, when mp_error.cc
> will be converted to C and moved to lib/core (that is planned).
> Also #include __FILE__ is more clear, when you want to say
> "include self", IMO.

OK, thanks for the long explanation, Vlad! I trimmed the mail
to not overquote. As to alignments and the rest -- up to you
(hopefully Sergey and Nikita will take look as well).

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Serge Petrenko]

Hi! Thanks for the patch!

12.05.2020 23:38, Vladislav Shpilevoy пишет:
> Thanks for the review!
>
> On 12/05/2020 19:52, Cyrill Gorcunov wrote:
>> On Tue, May 12, 2020 at 01:45:51AM +0200, Vladislav Shpilevoy wrote:
>>> Msgpuck functions mp_snprint() and mp_fprint() now support
>>> customizable MP_EXT serializer. This patch adds such for MP_ERROR.
>>> All extension serializers will be activated in a separate commit.
>>>
>>> Part of #4719
>>> ---
>>>   src/box/mp_error.cc       | 161 ++++++++++++++++++++++-
>>>   src/box/mp_error.h        |  29 ++++
>>>   test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
>>>   test/unit/mp_error.result |  72 +++++++++-
>>>   4 files changed, 529 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
>>> index 0491a7a50..fed2ce288 100644
>>> --- a/src/box/mp_error.cc
>>> +++ b/src/box/mp_error.cc
>>> @@ -552,3 +555,159 @@ error_unpack_unsafe(const char **data)
>>>   	}
>>>   	return err;
>>>   }
>>> +
>>> +#define MP_ERROR_PRINT_DEFINITION
>>> +#define MP_PRINT_FUNC snprintf
>>> +#define MP_PRINT_SUFFIX snprint
>>> +#define MP_PRINT_2(total, func, ...)						\
>>> +	SNPRINT(total, func, buf, size, __VA_ARGS__)
>>> +#define MP_PRINT_ARGS_DECL char *buf, int size
>>> +#include __FILE__
>> Don't get it -- include current file?! This a good sign of
>> some kind of problem in the code structure... If we need
>> a template then it should be some .cc/.c file explicily
>> included instead of __FILE__, no?
> I don't want to introduce a new file just for these 2 functions
> (mp_fprint_error() and mp_snprint_error()). This would be even
> worse than it is now. So I used the same file. I wrote __FILE__
> instead of mp_error.cc, so as to minimize diff, when mp_error.cc
> will be converted to C and moved to lib/core (that is planned).
> Also #include __FILE__ is more clear, when you want to say
> "include self", IMO.
>
> Alternative is to make mp_print_error_one(), mp_print_error_stack(),
> mp_print_error() as huge macros, but I don't like writing big macros.
> It is hard to edit, and much much harder to debug, since this all
> becomes just one infinite line, and 'n' in gdb/lldb skips the whole
> function.
>
> If you don't like the whole template idea, there is one alternative
> I described in the cover letter for the msgpuck patchset - virtual
> printer. But it will be really slow. We could probably try to make it
> buffered, but then it will be double-copying, also slow. I tried really
> hard to find other solutions, but failed. Even looked at fmemopen(),
> so as we would need to implement only mp_fprint(), but msgpuck library
> is supposed to work on Windows too. Also fmemopen() is basically a
> virtual printer as well (i.e. is slower, than direct snprint()).
>
> So if you have anything in mind except templates, or a virtual printer,
> or you know something about fmemopen() what I don't know, please, tell.
> Because I don't like these solutions, but just couldn't find anything
> better, with bearable complexity worth the feature. Which I thought would
> be just one-day patch, but I was very very wrong here.
>
> I could switch to the virtual printer in case you think its slowness is
> worth the simplification.
>
> As unbearable level of complexity I mean the non-binary tree solution
> I proposed in the msgpuck patchset's cover letter.
>
>>> +#define MP_ERROR_PRINT_DEFINITION
>>> +#define MP_PRINT_FUNC fprintf
>>> +#define MP_PRINT_SUFFIX fprint
>>> +#define MP_PRINT_2(total, func, ...) do {							\
>>> +	int bytes = func(file, __VA_ARGS__);					\
>>> +	if (bytes < 0)								\
>>> +		return -1;							\
>>> +	total += bytes;								\
>>> +} while (0)
>>> +#define MP_PRINT_ARGS_DECL FILE *file
>>> +#include __FILE__
>>> +
>>> +/* !defined(MP_ERROR_PRINT_DEFINITION) */
>>> +#else
>>> +/* defined(MP_ERROR_PRINT_DEFINITION) */
>>> +
>>> +/**
>>> + * MP_ERROR extension string serializer.
>>> + * There are two applications for string serialization - into a
>>> + * buffer, and into a file. Structure of both is exactly the same
>>> + * except for the copying/writing itself. To avoid code
>>> + * duplication the code is templated and expects some macros to do
>>> + * the actual output.
>>> + */
>>> +
>>> +#define MP_CONCAT4_R(a, b, c, d) a##b##c##d
>>> +#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d)
>>> +#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)
>>> +
>>> +#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
>>> +#define mp_print_error_one mp_func_name(error_one)
>>> +#define mp_print_error_stack mp_func_name(error_stack)
>>> +#define mp_print_error mp_func_name(error)
>>> +#define mp_print_common mp_func_name(recursion)
>> Maybe we should align the assignments to be able to read it, like
> I am pretty much able to read it as is, and moreover our code style
> does not contain any kinds of such alignments. I explained that all
> already in public chats, in private, in emails. Not going to describe
> pros and cons again.
>
> I applied the alignment, and faced the problems right away - with equal
> alignment for MP_CONCAT4_R, MP_CONCAT4, and MP_PRINT one of the lines
> becomes too big, out of 80 symbols. On the other hand with not equal
> alignments it would not look better than now.
>
> The mp_func_name() definitions with +2 tabs fall out of 80 as well, but
> with +1 the space is too short - any a bit longer name will violate
> the alignment in future causing either big unnecessary diff or alignment
> violation.
>
>> #define MP_CONCAT4_R(a, b, c, d)	a##b##c##d
>> #define MP_CONCAT4(a, b, c, d)		MP_CONCAT4_R(a, b, c, d)
>> #define MP_PRINT(total, ...)		MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)
>>
>> #define mp_func_name(name)	MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
>> #define mp_print_error_one	mp_func_name(error_one)
>> #define mp_print_error_stack	mp_func_name(error_stack)
>> #define mp_print_error		mp_func_name(error)
>> #define mp_print_common		mp_func_name(recursion)
>>
>>> +
>>> +static int
>>> +mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
>>> +{
>>> +	int total = 0;
>>> +	MP_PRINT(total, "{");
>>> +	if (depth <= 0) {
>>> +		MP_PRINT(total, "...}");
>>> +		return total;
>>> +	}
>>> +	const char *field_to_key[MP_ERROR_MAX] = {
>>> +		/* MP_ERROR_TYPE = */ "\"type\": ",
>>> +		/* MP_ERROR_FILE = */ "\"file\": ",
>>> +		/* MP_ERROR_LINE = */ "\"line\": ",
>>> +		/* MP_ERROR_MESSAGE = */ "\"message\": ",
>>> +		/* MP_ERROR_ERRNO = */ "\"errno\": ",
>>> +		/* MP_ERROR_CODE = */ "\"code\": ",
>>> +		/* MP_ERROR_FIELDS = */ "\"fields\": ",
>>> +	};
>> Please use designited initializers in a readable way
>> instead of this bloody mess
>>
>> 	const char *field_to_key[] = {
>> 		[MP_ERROR_TYPE]		= "\"type\": ",
>> 		...
>> 		[MP_ERROR_FILE]		= ...,
>> 		...
>> 		[MP_ERROR_FIELDS]	= "\"fields\": ",
>> 	}
> Once again - this is not a mess, this is our codestyle. If you
> don't like it, you should talk to somebody responsible for our
> SOP to change it. We can't write our code in individual code
> styles, and silently hope to push the old style away eventually.
> If you want changes, they need to go through our documentation
> and the responsible person. This is Kirill Y., as I remember.
>
> Talking of [<number>] = <value> initializer - I didn't know you
> can do that in C. Cool, thanks. Now it should be safer.
>
> I applied the alignments, but I won't push it into the master until,
> of course, the branch gets all ACKs, *and* until we agree to either
> align all new code, or don't align such places at all. In the latter
> case I will return the old version.
>
> ====================
>
> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
> index fed2ce288..0b5e6bc96 100644
> --- a/src/box/mp_error.cc
> +++ b/src/box/mp_error.cc
> @@ -589,15 +589,16 @@ error_unpack_unsafe(const char **data)
>    * the actual output.
>    */
>   
> -#define MP_CONCAT4_R(a, b, c, d) a##b##c##d
> -#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d)
> -#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)
> +#define MP_CONCAT4_R(a, b, c, d)	a##b##c##d
> +#define MP_CONCAT4(a, b, c, d)		MP_CONCAT4_R(a, b, c, d)
> +#define MP_PRINT(total, ...)		MP_PRINT_2(total, MP_PRINT_FUNC,	\
> +						   __VA_ARGS__)
>   
> -#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
> -#define mp_print_error_one mp_func_name(error_one)
> -#define mp_print_error_stack mp_func_name(error_stack)
> -#define mp_print_error mp_func_name(error)
> -#define mp_print_common mp_func_name(recursion)
> +#define mp_func_name(name)	MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
> +#define mp_print_error_one	mp_func_name(error_one)
> +#define mp_print_error_stack	mp_func_name(error_stack)
> +#define mp_print_error		mp_func_name(error)
> +#define mp_print_common		mp_func_name(recursion)
> ====================
>
> I don't know why the line above is screwed. In source it looks ok.
> Probably due to '+' in the beginning, added by git. One another
> reason against alignments - they will look shitty in git diff, show,
> etc.


I personally like the aligned version more, but it's up to you. LGTM


>
> ====================
>   
>   static int
>   mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
> @@ -609,13 +610,13 @@ mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
>   		return total;
>   	}
>   	const char *field_to_key[MP_ERROR_MAX] = {
> -		/* MP_ERROR_TYPE = */ "\"type\": ",
> -		/* MP_ERROR_FILE = */ "\"file\": ",
> -		/* MP_ERROR_LINE = */ "\"line\": ",
> -		/* MP_ERROR_MESSAGE = */ "\"message\": ",
> -		/* MP_ERROR_ERRNO = */ "\"errno\": ",
> -		/* MP_ERROR_CODE = */ "\"code\": ",
> -		/* MP_ERROR_FIELDS = */ "\"fields\": ",
> +		[MP_ERROR_TYPE] =	"\"type\": ",
> +		[MP_ERROR_FILE] =	"\"file\": ",
> +		[MP_ERROR_LINE] =	"\"line\": ",
> +		[MP_ERROR_MESSAGE] =	"\"message\": ",
> +		[MP_ERROR_ERRNO] =	"\"errno\": ",
> +		[MP_ERROR_CODE] =	"\"code\": ",
> +		[MP_ERROR_FIELDS] =	"\"fields\": ",
>   	};
>   	--depth;
>   	if (mp_typeof(**data) != MP_MAP)

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Nikita Pettik]

On 12 May 01:45, Vladislav Shpilevoy wrote:
>  src/box/mp_error.h        |  29 ++++
>  test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
>  test/unit/mp_error.result |  72 +++++++++-
>  4 files changed, 529 insertions(+), 3 deletions(-)
> +
> +#define MP_ERROR_PRINT_DEFINITION
> +#define MP_PRINT_FUNC snprintf
> +#define MP_PRINT_SUFFIX snprint
> +#define MP_PRINT_2(total, func, ...)						\
> +	SNPRINT(total, func, buf, size, __VA_ARGS__)
> +#define MP_PRINT_ARGS_DECL char *buf, int size
> +#include __FILE__
> +
> +#define MP_ERROR_PRINT_DEFINITION
> +#define MP_PRINT_FUNC fprintf
> +#define MP_PRINT_SUFFIX fprint
> +#define MP_PRINT_2(total, func, ...) do {							\
> +	int bytes = func(file, __VA_ARGS__);					\
> +	if (bytes < 0)								\
> +		return -1;							\
> +	total += bytes;								\
> +} while (0)
> +#define MP_PRINT_ARGS_DECL FILE *file
> +#include __FILE__

I've failed to understand how this is supposed to work.
Why do you have to include file itself? Why do you have to do it twice?
This macro related part definitely lacks some comments.

> +/* !defined(MP_ERROR_PRINT_DEFINITION) */
> +#else
> +/* defined(MP_ERROR_PRINT_DEFINITION) */
> +

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Vladislav Shpilevoy]

Thanks for the review!

On 13/05/2020 14:31, Nikita Pettik wrote:
> On 12 May 01:45, Vladislav Shpilevoy wrote:
>>  src/box/mp_error.h        |  29 ++++
>>  test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
>>  test/unit/mp_error.result |  72 +++++++++-
>>  4 files changed, 529 insertions(+), 3 deletions(-)
>> +
>> +#define MP_ERROR_PRINT_DEFINITION
>> +#define MP_PRINT_FUNC snprintf
>> +#define MP_PRINT_SUFFIX snprint
>> +#define MP_PRINT_2(total, func, ...)						\
>> +	SNPRINT(total, func, buf, size, __VA_ARGS__)
>> +#define MP_PRINT_ARGS_DECL char *buf, int size
>> +#include __FILE__
>> +
>> +#define MP_ERROR_PRINT_DEFINITION
>> +#define MP_PRINT_FUNC fprintf
>> +#define MP_PRINT_SUFFIX fprint
>> +#define MP_PRINT_2(total, func, ...) do {							\
>> +	int bytes = func(file, __VA_ARGS__);					\
>> +	if (bytes < 0)								\
>> +		return -1;							\
>> +	total += bytes;								\
>> +} while (0)
>> +#define MP_PRINT_ARGS_DECL FILE *file
>> +#include __FILE__
> 
> I've failed to understand how this is supposed to work.
> Why do you have to include file itself? Why do you have to do it twice?
> This macro related part definitely lacks some comments.

This is not supposed to work. It works already. Was a surprise even for me.
I wasn't sure this needs more explanation that I gave below:

> /**
>  * MP_ERROR extension string serializer.
>  * There are two applications for string serialization - into a
>  * buffer, and into a file. Structure of both is exactly the same
>  * except for the copying/writing itself. To avoid code
>  * duplication the code is templated and expects some macros to do
>  * the actual output.
>  */
This is basically a C template. The same as bps tree, rb tree, mhash. But
located in the same source file which needs it. Since this is not a common
enough feature to move it to a separate place.

I include self with pre-declared macros MP_ERROR_PRINT_DEFINITION. When it
is defined, the file skips everything except the 3 functions in the end:
mp_print_error_one(), mp_print_error_stack(), mp_print_error().

The skip uses mere #ifdef MP_ERROR_PRINT_DEFINITION to understand, that
currently the file is being included into self to customize the error
printers. So all is skipped except them.

I include the file twice, but with different parameters. Like if I would
customize one template with two parameter sets in C++. First inclusion
turns

    mp_print_error_one(), mp_print_error_stack(), mp_print_error()

into

    mp_snprint_error_one(), mp_snprint_error_stack(), mp_snprint_error()

and snprintf() calls inside. The second inclusion turns the functions into

    mp_fprint_error_one(), mp_fprint_error_stack(), mp_fprint_error()

and fprintf() calls inside.

I tried to extend the comment.

====================
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index 0b5e6bc96..a4a4ccaca 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -587,6 +587,21 @@ error_unpack_unsafe(const char **data)
  * except for the copying/writing itself. To avoid code
  * duplication the code is templated and expects some macros to do
  * the actual output.
+ *
+ * Templates are defined similarly to the usual C trick, when all
+ * the code is in one file and relies on some external macros to
+ * define customizable types, functions, parameters. Then other
+ * code can include this file with the macros defined. Even
+ * multiple times, in case structure and function names also are
+ * customizable.
+ *
+ * The tricky thing here is that the templates are defined in the
+ * same file, which needs them. So the file needs to include
+ * *self* with a few macros defined beforehand. That allows to
+ * hide this template from any external access, use mp_error
+ * internal functions, and keep all the code in one place without
+ * necessity to split it into different files just to be able to
+ * turn it into a template.
  */
 

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Nikita Pettik]

On 14 May 00:10, Vladislav Shpilevoy wrote:
> Thanks for the review!
> 
> On 13/05/2020 14:31, Nikita Pettik wrote:
> > On 12 May 01:45, Vladislav Shpilevoy wrote:
> >> +#define MP_ERROR_PRINT_DEFINITION
> >> +#define MP_PRINT_FUNC fprintf
> >> +#define MP_PRINT_SUFFIX fprint
> >> +#define MP_PRINT_2(total, func, ...) do {							\
> >> +	int bytes = func(file, __VA_ARGS__);					\
> >> +	if (bytes < 0)								\
> >> +		return -1;							\
> >> +	total += bytes;								\
> >> +} while (0)
> >> +#define MP_PRINT_ARGS_DECL FILE *file
> >> +#include __FILE__
> > 
> > I've failed to understand how this is supposed to work.
> > Why do you have to include file itself? Why do you have to do it twice?
> > This macro related part definitely lacks some comments.
> 
> This is not supposed to work. It works already. Was a surprise even for me.
> I wasn't sure this needs more explanation that I gave below:
> 
> > /**
> >  * MP_ERROR extension string serializer.
> >  * There are two applications for string serialization - into a
> >  * buffer, and into a file. Structure of both is exactly the same
> >  * except for the copying/writing itself. To avoid code
> >  * duplication the code is templated and expects some macros to do
> >  * the actual output.
> >  */
> This is basically a C template. The same as bps tree, rb tree, mhash. But
> located in the same source file which needs it. Since this is not a common
> enough feature to move it to a separate place.
> 
> I include self with pre-declared macros MP_ERROR_PRINT_DEFINITION. When it
> is defined, the file skips everything except the 3 functions in the end:
> mp_print_error_one(), mp_print_error_stack(), mp_print_error().

A-ha, it is what I missed. But personally I would anyway move these
macros/functions to a separate file. It would definitely cut extra
questions and would make code cleaner (IMHO) (I bet anybody who
sees '#include __FILE__' for the first time is like 'WTF?!').
 
> The skip uses mere #ifdef MP_ERROR_PRINT_DEFINITION to understand, that
> currently the file is being included into self to customize the error
> printers. So all is skipped except them.
> 
> I include the file twice, but with different parameters. Like if I would
> customize one template with two parameter sets in C++. First inclusion
> turns
> 
>     mp_print_error_one(), mp_print_error_stack(), mp_print_error()
> 
> into
> 
>     mp_snprint_error_one(), mp_snprint_error_stack(), mp_snprint_error()
> 
> and snprintf() calls inside. The second inclusion turns the functions into
> 
>     mp_fprint_error_one(), mp_fprint_error_stack(), mp_fprint_error()
> 
> and fprintf() calls inside.
> 
> I tried to extend the comment.

You gave cool explanation right in this message. Why not copy it to
the source file?

> ====================
> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
> index 0b5e6bc96..a4a4ccaca 100644
> --- a/src/box/mp_error.cc
> +++ b/src/box/mp_error.cc
> @@ -587,6 +587,21 @@ error_unpack_unsafe(const char **data)
>   * except for the copying/writing itself. To avoid code
>   * duplication the code is templated and expects some macros to do
>   * the actual output.
> + *
> + * Templates are defined similarly to the usual C trick, when all
> + * the code is in one file and relies on some external macros to
> + * define customizable types, functions, parameters. Then other
> + * code can include this file with the macros defined. Even
> + * multiple times, in case structure and function names also are
> + * customizable.
> + *
> + * The tricky thing here is that the templates are defined in the
> + * same file, which needs them. So the file needs to include
> + * *self* with a few macros defined beforehand. That allows to
> + * hide this template from any external access, use mp_error
> + * internal functions, and keep all the code in one place without
> + * necessity to split it into different files just to be able to
> + * turn it into a template.
>   */
>  

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Vladislav Shpilevoy]

Hi! Thanks for the review!

>>> /**
>>>  * MP_ERROR extension string serializer.
>>>  * There are two applications for string serialization - into a
>>>  * buffer, and into a file. Structure of both is exactly the same
>>>  * except for the copying/writing itself. To avoid code
>>>  * duplication the code is templated and expects some macros to do
>>>  * the actual output.
>>>  */
>> This is basically a C template. The same as bps tree, rb tree, mhash. But
>> located in the same source file which needs it. Since this is not a common
>> enough feature to move it to a separate place.
>>
>> I include self with pre-declared macros MP_ERROR_PRINT_DEFINITION. When it
>> is defined, the file skips everything except the 3 functions in the end:
>> mp_print_error_one(), mp_print_error_stack(), mp_print_error().
> 
> A-ha, it is what I missed. But personally I would anyway move these
> macros/functions to a separate file. It would definitely cut extra
> questions and would make code cleaner (IMHO) (I bet anybody who
> sees '#include __FILE__' for the first time is like 'WTF?!').

But! How cool the self inclusion is 😎. If someone would wrap these things
into neat macros, it could be a useful tool to replace C++ templates, which
are the only real problem on the way of getting rid of C++ from the code base.
For example, tuple comparators heavily depend on templates, and their move
to separate files is hardly possible - they depend on tuple_compare.cc
internals too much.

Jokes aside, self inclusion solves one important issue - non templated
dependencies. If I move the templates to a different file, it needs to include
"mp_error.h", at least for MP_ERROR_* enum constants. Strictly speaking. Also
it needs MessagePack headers, and can't use any mp_error.cc internal functions
even though these templates *are* internal too.

>> The skip uses mere #ifdef MP_ERROR_PRINT_DEFINITION to understand, that
>> currently the file is being included into self to customize the error
>> printers. So all is skipped except them.
>>
>> I include the file twice, but with different parameters. Like if I would
>> customize one template with two parameter sets in C++. First inclusion
>> turns
>>
>>     mp_print_error_one(), mp_print_error_stack(), mp_print_error()
>>
>> into
>>
>>     mp_snprint_error_one(), mp_snprint_error_stack(), mp_snprint_error()
>>
>> and snprintf() calls inside. The second inclusion turns the functions into
>>
>>     mp_fprint_error_one(), mp_fprint_error_stack(), mp_fprint_error()
>>
>> and fprintf() calls inside.
>>
>> I tried to extend the comment.
> 
> You gave cool explanation right in this message. Why not copy it to
> the source file?

Because it is too common. Can't put it in just one place, since the mentioned
things are spread on the file. I tried to split it in parts and put them
where they belong. In addition to the comment I added previously.

diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index a4a4ccaca..23a74d0a9 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -28,6 +28,11 @@
  * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
+/**
+ * When this macros is defined, it means the file included self in
+ * order to customize some templates. Skip everything except them
+ * then.
+ */
 #ifndef MP_ERROR_PRINT_DEFINITION
 
 #include "box/mp_error.h"
@@ -556,6 +561,11 @@ error_unpack_unsafe(const char **data)
 	return err;
 }
 
+/**
+ * Include this file into self with a few template parameters
+ * to create mp_snprint_error() and mp_fprint_error() functions
+ * and their helpers from a printer template.
+ */
 #define MP_ERROR_PRINT_DEFINITION
 #define MP_PRINT_FUNC snprintf
 #define MP_PRINT_SUFFIX snprint
@@ -675,6 +685,11 @@ mp_print_error_stack(MP_PRINT_ARGS_DECL, const char **data, int depth)
 	return total;
 }
 
+/**
+ * The main printer template. Depending on template parameters it
+ * is turned into mp_snprint_error() with snprintf() semantics or
+ * into mp_fprint_error() with fprintf() semantics.
+ */
 int
 mp_print_error(MP_PRINT_ARGS_DECL, const char **data, int depth)
 {

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Nikita Pettik]

On 14 May 23:28, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
> 
> >>> /**
> >>>  * MP_ERROR extension string serializer.
> >>>  * There are two applications for string serialization - into a
> >>>  * buffer, and into a file. Structure of both is exactly the same
> >>>  * except for the copying/writing itself. To avoid code
> >>>  * duplication the code is templated and expects some macros to do
> >>>  * the actual output.
> >>>  */
> >> This is basically a C template. The same as bps tree, rb tree, mhash. But
> >> located in the same source file which needs it. Since this is not a common
> >> enough feature to move it to a separate place.
> >>
> >> I include self with pre-declared macros MP_ERROR_PRINT_DEFINITION. When it
> >> is defined, the file skips everything except the 3 functions in the end:
> >> mp_print_error_one(), mp_print_error_stack(), mp_print_error().
> > 
> > A-ha, it is what I missed. But personally I would anyway move these
> > macros/functions to a separate file. It would definitely cut extra
> > questions and would make code cleaner (IMHO) (I bet anybody who
> > sees '#include __FILE__' for the first time is like 'WTF?!').
> 
> But! How cool the self inclusion is 😎. If someone would wrap these things
> into neat macros, it could be a useful tool to replace C++ templates, which
> are the only real problem on the way of getting rid of C++ from the code base.
> For example, tuple comparators heavily depend on templates, and their move
> to separate files is hardly possible - they depend on tuple_compare.cc
> internals too much.
> 
> Jokes aside, self inclusion solves one important issue - non templated
> dependencies. If I move the templates to a different file, it needs to include
> "mp_error.h", at least for MP_ERROR_* enum constants. Strictly speaking. Also
> it needs MessagePack headers, and can't use any mp_error.cc internal functions
> even though these templates *are* internal too.

Ok, as you wish.
 
> >> The skip uses mere #ifdef MP_ERROR_PRINT_DEFINITION to understand, that
> >> currently the file is being included into self to customize the error
> >> printers. So all is skipped except them.
> >>
> >> I include the file twice, but with different parameters. Like if I would
> >> customize one template with two parameter sets in C++. First inclusion
> >> turns
> >>
> >>     mp_print_error_one(), mp_print_error_stack(), mp_print_error()
> >>
> >> into
> >>
> >>     mp_snprint_error_one(), mp_snprint_error_stack(), mp_snprint_error()
> >>
> >> and snprintf() calls inside. The second inclusion turns the functions into
> >>
> >>     mp_fprint_error_one(), mp_fprint_error_stack(), mp_fprint_error()
> >>
> >> and fprintf() calls inside.
> >>
> >> I tried to extend the comment.
> > 
> > You gave cool explanation right in this message. Why not copy it to
> > the source file?
> 
> Because it is too common. Can't put it in just one place, since the mentioned
> things are spread on the file. I tried to split it in parts and put them
> where they belong. In addition to the comment I added previously.

Ok, thanks. LGTM.

> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
> index a4a4ccaca..23a74d0a9 100644
> --- a/src/box/mp_error.cc
> +++ b/src/box/mp_error.cc
> @@ -28,6 +28,11 @@
>   * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
> +/**
> + * When this macros is defined, it means the file included self in
> + * order to customize some templates. Skip everything except them
> + * then.
> + */
>  #ifndef MP_ERROR_PRINT_DEFINITION
>  
>  #include "box/mp_error.h"
> @@ -556,6 +561,11 @@ error_unpack_unsafe(const char **data)
>  	return err;
>  }
>  
> +/**
> + * Include this file into self with a few template parameters
> + * to create mp_snprint_error() and mp_fprint_error() functions
> + * and their helpers from a printer template.
> + */
>  #define MP_ERROR_PRINT_DEFINITION
>  #define MP_PRINT_FUNC snprintf
>  #define MP_PRINT_SUFFIX snprint
> @@ -675,6 +685,11 @@ mp_print_error_stack(MP_PRINT_ARGS_DECL, const char **data, int depth)
>  	return total;
>  }
>  
> +/**
> + * The main printer template. Depending on template parameters it
> + * is turned into mp_snprint_error() with snprintf() semantics or
> + * into mp_fprint_error() with fprintf() semantics.
> + */
>  int
>  mp_print_error(MP_PRINT_ARGS_DECL, const char **data, int depth)
>  {

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Vladislav Shpilevoy]

I added explicit casts when print uint64 in mp_error.cc,
since some compilers complain about %lld != uint64_t, as
noticed by Alexander T.

====================
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index badf7ee92..61cf5e1b8 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -657,7 +657,7 @@ mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
 		if (key < MP_ERROR_MAX)
 			MP_PRINT(total, "%s", mp_error_field_to_json_key[key]);
 		else
-			MP_PRINT(total, "%llu: ", key);
+			MP_PRINT(total, "%llu: ", (unsigned long long)key);
 		MP_PRINT_2(total, mp_print_common, data, depth);
 	}
 	MP_PRINT(total, "}");
@@ -717,7 +717,7 @@ mp_print_error(MP_PRINT_ARGS_DECL, const char **data, int depth)
 			break;
 		}
 		default:
-			MP_PRINT(total, "%llu: ", key);
+			MP_PRINT(total, "%llu: ", (unsigned long long)key);
 			MP_PRINT_2(total, mp_print_common, data, depth);
 			break;
 		}

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Alexander Turenko]

> +	const char *field_to_key[MP_ERROR_MAX] = {
> +		/* MP_ERROR_TYPE = */ "\"type\": ",
> +		/* MP_ERROR_FILE = */ "\"file\": ",
> +		/* MP_ERROR_LINE = */ "\"line\": ",
> +		/* MP_ERROR_MESSAGE = */ "\"message\": ",
> +		/* MP_ERROR_ERRNO = */ "\"errno\": ",
> +		/* MP_ERROR_CODE = */ "\"code\": ",
> +		/* MP_ERROR_FIELDS = */ "\"fields\": ",
> +	};

Should be static?

Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Vladislav Shpilevoy]

Thanks for the review!

On 19/05/2020 13:51, Alexander Turenko wrote:
>> +	const char *field_to_key[MP_ERROR_MAX] = {
>> +		/* MP_ERROR_TYPE = */ "\"type\": ",
>> +		/* MP_ERROR_FILE = */ "\"file\": ",
>> +		/* MP_ERROR_LINE = */ "\"line\": ",
>> +		/* MP_ERROR_MESSAGE = */ "\"message\": ",
>> +		/* MP_ERROR_ERRNO = */ "\"errno\": ",
>> +		/* MP_ERROR_CODE = */ "\"code\": ",
>> +		/* MP_ERROR_FIELDS = */ "\"fields\": ",
>> +	};
> 
> Should be static?

I can do it static, but only outside of the function and the
of the whole template then. Otherwise we will have 2 copies
of this array, because mp_print_error_one() is turned into 2
functions: mp_sprint_error_one() and mp_fprint_error_one().

====================
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index 23a74d0a9..badf7ee92 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -561,6 +561,16 @@ error_unpack_unsafe(const char **data)
 	return err;
 }
 
+static const char *const mp_error_field_to_json_key[MP_ERROR_MAX] = {
+	[MP_ERROR_TYPE] =	"\"type\": ",
+	[MP_ERROR_FILE] =	"\"file\": ",
+	[MP_ERROR_LINE] =	"\"line\": ",
+	[MP_ERROR_MESSAGE] =	"\"message\": ",
+	[MP_ERROR_ERRNO] =	"\"errno\": ",
+	[MP_ERROR_CODE] =	"\"code\": ",
+	[MP_ERROR_FIELDS] =	"\"fields\": ",
+};
+
 /**
  * Include this file into self with a few template parameters
  * to create mp_snprint_error() and mp_fprint_error() functions
@@ -634,15 +644,6 @@ mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
 		MP_PRINT(total, "...}");
 		return total;
 	}
-	const char *field_to_key[MP_ERROR_MAX] = {
-		[MP_ERROR_TYPE] =	"\"type\": ",
-		[MP_ERROR_FILE] =	"\"file\": ",
-		[MP_ERROR_LINE] =	"\"line\": ",
-		[MP_ERROR_MESSAGE] =	"\"message\": ",
-		[MP_ERROR_ERRNO] =	"\"errno\": ",
-		[MP_ERROR_CODE] =	"\"code\": ",
-		[MP_ERROR_FIELDS] =	"\"fields\": ",
-	};
 	--depth;
 	if (mp_typeof(**data) != MP_MAP)
 		return -1;
@@ -654,7 +655,7 @@ mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
 			return -1;
 		uint64_t key = mp_decode_uint(data);
 		if (key < MP_ERROR_MAX)
-			MP_PRINT(total, "%s", field_to_key[key]);
+			MP_PRINT(total, "%s", mp_error_field_to_json_key[key]);
 		else
 			MP_PRINT(total, "%llu: ", key);
 		MP_PRINT_2(total, mp_print_common, data, depth);

[Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers

[Vladislav Shpilevoy]

Msgpuck functions mp_snprint() and mp_fprint() now support
customizable MP_EXT serializer. This patch makes them able to
correctly serialize all tarantool's MP_EXT extensions: MP_DECIMAL,
MP_UUID, MP_ERROR.

Closes #4719
---
 src/box/CMakeLists.txt |  1 +
 src/box/box.cc         |  2 ++
 src/box/msgpack.c      | 78 ++++++++++++++++++++++++++++++++++++++++++
 src/box/msgpack.h      | 41 ++++++++++++++++++++++
 4 files changed, 122 insertions(+)
 create mode 100644 src/box/msgpack.c
 create mode 100644 src/box/msgpack.h

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index c931ecdfe..16a729590 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -71,6 +71,7 @@ add_library(xlog STATIC xlog.c)
 target_link_libraries(xlog core box_error crc32 ${ZSTD_LIBRARIES})
 
 add_library(box STATIC
+    msgpack.c
     iproto.cc
     error.cc
     xrow_io.cc
diff --git a/src/box/box.cc b/src/box/box.cc
index ae0907e0f..d95a47cc9 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -76,6 +76,7 @@
 #include "func.h"
 #include "sequence.h"
 #include "sql_stmt_cache.h"
+#include "msgpack.h"
 
 static char status[64] = "unknown";
 
@@ -2355,6 +2356,7 @@ on_wal_checkpoint_threshold(void)
 void
 box_init(void)
 {
+	msgpack_init();
 	fiber_cond_create(&ro_cond);
 
 	user_cache_init();
diff --git a/src/box/msgpack.c b/src/box/msgpack.c
new file mode 100644
index 000000000..37bb3920c
--- /dev/null
+++ b/src/box/msgpack.c
@@ -0,0 +1,78 @@
+/*
+ * Copyright 2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include "msgpack.h"
+#include "msgpuck/msgpuck.h"
+
+#include "mp_extension_types.h"
+#include "mp_decimal.h"
+#include "uuid/mp_uuid.h"
+#include "mp_error.h"
+
+static int
+msgpack_fprint_ext(FILE *file, const char **data, int depth)
+{
+	int8_t type;
+	uint32_t len = mp_decode_extl(data, &type);
+	switch(type) {
+	case MP_DECIMAL:
+		return mp_fprint_decimal(file, data, len);
+	case MP_UUID:
+		return mp_fprint_uuid(file, data, len);
+	case MP_ERROR:
+		return mp_fprint_error(file, data, depth);
+	default:
+		return fprintf(file, "undefined");
+	}
+}
+
+static int
+msgpack_snprint_ext(char *buf, int size, const char **data, int depth)
+{
+	int8_t type;
+	uint32_t len = mp_decode_extl(data, &type);
+	switch(type) {
+	case MP_DECIMAL:
+		return mp_snprint_decimal(buf, size, data, len);
+	case MP_UUID:
+		return mp_snprint_uuid(buf, size, data, len);
+	case MP_ERROR:
+		return mp_snprint_error(buf, size, data, depth);
+	default:
+		return snprintf(buf, size, "undefined");
+	}
+}
+
+void
+msgpack_init(void)
+{
+	mp_fprint_ext = msgpack_fprint_ext;
+	mp_snprint_ext = msgpack_snprint_ext;
+}
diff --git a/src/box/msgpack.h b/src/box/msgpack.h
new file mode 100644
index 000000000..a7ccc7f90
--- /dev/null
+++ b/src/box/msgpack.h
@@ -0,0 +1,41 @@
+#pragma once
+/*
+ * Copyright 2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+void
+msgpack_init(void);
+
+#if defined(__cplusplus)
+}
+#endif /* defined(__cplusplus) */
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers

[Cyrill Gorcunov]

On Tue, May 12, 2020 at 01:45:52AM +0200, Vladislav Shpilevoy wrote:
> Msgpuck functions mp_snprint() and mp_fprint() now support
> customizable MP_EXT serializer. This patch makes them able to
> correctly serialize all tarantool's MP_EXT extensions: MP_DECIMAL,
> MP_UUID, MP_ERROR.
> 
> Closes #4719
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers

[Nikita Pettik]

On 12 May 01:45, Vladislav Shpilevoy wrote:
> +
> +static int
> +msgpack_fprint_ext(FILE *file, const char **data, int depth)
> +{
> +	int8_t type;
> +	uint32_t len = mp_decode_extl(data, &type);
> +	switch(type) {
> +	case MP_DECIMAL:
> +		return mp_fprint_decimal(file, data, len);
> +	case MP_UUID:
> +		return mp_fprint_uuid(file, data, len);
> +	case MP_ERROR:
> +		return mp_fprint_error(file, data, depth);
> +	default:
> +		return fprintf(file, "undefined");

I'd come up with more sensible message in case of "undefined" mp_ type.
For instance: ("undefined mgpack extension (%d)", type).

> +	}
> +}
> +
> +static int
> +msgpack_snprint_ext(char *buf, int size, const char **data, int depth)
> +{
> +	int8_t type;
> +	uint32_t len = mp_decode_extl(data, &type);
> +	switch(type) {
> +	case MP_DECIMAL:
> +		return mp_snprint_decimal(buf, size, data, len);
> +	case MP_UUID:
> +		return mp_snprint_uuid(buf, size, data, len);
> +	case MP_ERROR:
> +		return mp_snprint_error(buf, size, data, depth);
> +	default:
> +		return snprintf(buf, size, "undefined");

Same here.

Re: [Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 13/05/2020 23:06, Nikita Pettik wrote:
> On 12 May 01:45, Vladislav Shpilevoy wrote:
>> +
>> +static int
>> +msgpack_fprint_ext(FILE *file, const char **data, int depth)
>> +{
>> +	int8_t type;
>> +	uint32_t len = mp_decode_extl(data, &type);
>> +	switch(type) {
>> +	case MP_DECIMAL:
>> +		return mp_fprint_decimal(file, data, len);
>> +	case MP_UUID:
>> +		return mp_fprint_uuid(file, data, len);
>> +	case MP_ERROR:
>> +		return mp_fprint_error(file, data, depth);
>> +	default:
>> +		return fprintf(file, "undefined");
> 
> I'd come up with more sensible message in case of "undefined" mp_ type.
> For instance: ("undefined mgpack extension (%d)", type).

This is not an error message. This is a JSON value (even though it is
not considered standard). Msgpuck prints it, when does not know what else
do with the type.

This was the case for all MP_EXT so far, they were printed as 'undefined'.
Now only unknown are 'undefined'.

Re: [Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers

[Nikita Pettik]

On 13 May 23:48, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
> 
> On 13/05/2020 23:06, Nikita Pettik wrote:
> > On 12 May 01:45, Vladislav Shpilevoy wrote:
> >> +
> >> +static int
> >> +msgpack_fprint_ext(FILE *file, const char **data, int depth)
> >> +{
> >> +	int8_t type;
> >> +	uint32_t len = mp_decode_extl(data, &type);
> >> +	switch(type) {
> >> +	case MP_DECIMAL:
> >> +		return mp_fprint_decimal(file, data, len);
> >> +	case MP_UUID:
> >> +		return mp_fprint_uuid(file, data, len);
> >> +	case MP_ERROR:
> >> +		return mp_fprint_error(file, data, depth);
> >> +	default:
> >> +		return fprintf(file, "undefined");
> > 
> > I'd come up with more sensible message in case of "undefined" mp_ type.
> > For instance: ("undefined mgpack extension (%d)", type).
> 
> This is not an error message.

I understand that. Still I stick to the point that simple "undefined"
looks poor.

> This is a JSON value (even though it is
> not considered standard). Msgpuck prints it, when does not know what else
> do with the type.
 
> This was the case for all MP_EXT so far, they were printed as 'undefined'.
> Now only unknown are 'undefined'.

Re: [Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers

[Vladislav Shpilevoy]

Thanks for the review!

>> On 13/05/2020 23:06, Nikita Pettik wrote:
>>> On 12 May 01:45, Vladislav Shpilevoy wrote:
>>>> +
>>>> +static int
>>>> +msgpack_fprint_ext(FILE *file, const char **data, int depth)
>>>> +{
>>>> +	int8_t type;
>>>> +	uint32_t len = mp_decode_extl(data, &type);
>>>> +	switch(type) {
>>>> +	case MP_DECIMAL:
>>>> +		return mp_fprint_decimal(file, data, len);
>>>> +	case MP_UUID:
>>>> +		return mp_fprint_uuid(file, data, len);
>>>> +	case MP_ERROR:
>>>> +		return mp_fprint_error(file, data, depth);
>>>> +	default:
>>>> +		return fprintf(file, "undefined");
>>>
>>> I'd come up with more sensible message in case of "undefined" mp_ type.
>>> For instance: ("undefined mgpack extension (%d)", type).
>>
>> This is not an error message.
> 
> I understand that. Still I stick to the point that simple "undefined"
> looks poor.

Ok, I just noticed that you wrapped your proposal into (). It is not just
a bare string or something else, that could be treated in 2 ways. Then I
guess it is ok. Anyway both 'undefined', and parenthesis are JS JSON parsers
features only. So by adding a few details we are not breaking normal JSON
parsers any further.

I changed this commit, and added a new commit to the msgpuck patchset.
The message is changed a little.

====================
diff --git a/src/box/msgpack.c b/src/box/msgpack.c
index 37bb3920c..4013aec4c 100644
--- a/src/box/msgpack.c
+++ b/src/box/msgpack.c
@@ -49,7 +49,8 @@ msgpack_fprint_ext(FILE *file, const char **data, int depth)
 	case MP_ERROR:
 		return mp_fprint_error(file, data, depth);
 	default:
-		return fprintf(file, "undefined");
+		return fprintf(file, "(extension: type %d, len %u)", (int)type,
+			       (unsigned)len);
 	}
 }
 
@@ -66,7 +67,8 @@ msgpack_snprint_ext(char *buf, int size, const char **data, int depth)
 	case MP_ERROR:
 		return mp_snprint_error(buf, size, data, depth);
 	default:
-		return snprintf(buf, size, "undefined");
+		return snprintf(buf, size, "(extension: type %d, len %u)",
+				(int)type, (unsigned)len);
 	}
 }
 

Re: [Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers

[Alexander Turenko]

> diff --git a/src/box/msgpack.c b/src/box/msgpack.c
> index 37bb3920c..4013aec4c 100644
> --- a/src/box/msgpack.c
> +++ b/src/box/msgpack.c
> @@ -49,7 +49,8 @@ msgpack_fprint_ext(FILE *file, const char **data, int depth)
>  	case MP_ERROR:
>  		return mp_fprint_error(file, data, depth);
>  	default:
> -		return fprintf(file, "undefined");
> +		return fprintf(file, "(extension: type %d, len %u)", (int)type,
> +			       (unsigned)len);
>  	}
>  }

I would reuse mp_fprint_ext_default() here.

>  
> @@ -66,7 +67,8 @@ msgpack_snprint_ext(char *buf, int size, const char **data, int depth)
>  	case MP_ERROR:
>  		return mp_snprint_error(buf, size, data, depth);
>  	default:
> -		return snprintf(buf, size, "undefined");
> +		return snprintf(buf, size, "(extension: type %d, len %u)",
> +				(int)type, (unsigned)len);
>  	}
>  }

Same.

Re: [Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers

[Vladislav Shpilevoy]

Thanks for the review!

On 19/05/2020 14:11, Alexander Turenko wrote:
>> diff --git a/src/box/msgpack.c b/src/box/msgpack.c
>> index 37bb3920c..4013aec4c 100644
>> --- a/src/box/msgpack.c
>> +++ b/src/box/msgpack.c
>> @@ -49,7 +49,8 @@ msgpack_fprint_ext(FILE *file, const char **data, int depth)
>>  	case MP_ERROR:
>>  		return mp_fprint_error(file, data, depth);
>>  	default:
>> -		return fprintf(file, "undefined");
>> +		return fprintf(file, "(extension: type %d, len %u)", (int)type,
>> +			       (unsigned)len);
>>  	}
>>  }
> 
> I would reuse mp_fprint_ext_default() here.

It will decode MP_EXT again, I wanted to avoid this. But I guess
perf is not so critical here, yeah.

====================
diff --git a/src/box/msgpack.c b/src/box/msgpack.c
index 4013aec4c..1723dea4c 100644
--- a/src/box/msgpack.c
+++ b/src/box/msgpack.c
@@ -39,6 +39,7 @@
 static int
 msgpack_fprint_ext(FILE *file, const char **data, int depth)
 {
+	const char **orig = data;
 	int8_t type;
 	uint32_t len = mp_decode_extl(data, &type);
 	switch(type) {
@@ -49,14 +50,14 @@ msgpack_fprint_ext(FILE *file, const char **data, int depth)
 	case MP_ERROR:
 		return mp_fprint_error(file, data, depth);
 	default:
-		return fprintf(file, "(extension: type %d, len %u)", (int)type,
-			       (unsigned)len);
+		return mp_fprint_ext_default(file, orig, depth);
 	}
 }
 
 static int
 msgpack_snprint_ext(char *buf, int size, const char **data, int depth)
 {
+	const char **orig = data;
 	int8_t type;
 	uint32_t len = mp_decode_extl(data, &type);
 	switch(type) {
@@ -67,8 +68,7 @@ msgpack_snprint_ext(char *buf, int size, const char **data, int depth)
 	case MP_ERROR:
 		return mp_snprint_error(buf, size, data, depth);
 	default:
-		return snprintf(buf, size, "(extension: type %d, len %u)",
-				(int)type, (unsigned)len);
+		return mp_snprint_ext_default(buf, size, orig, depth);
 	}
 }
 

Re: [Tarantool-patches] [PATCH 5/5] msgpuck: activate MP_EXT custom serializers

[Nikita Pettik]

On 14 May 23:27, Vladislav Shpilevoy wrote:
> Thanks for the review!
> 
> >> On 13/05/2020 23:06, Nikita Pettik wrote:
> >>> On 12 May 01:45, Vladislav Shpilevoy wrote:
> >>>> +
> >>>> +static int
> >>>> +msgpack_fprint_ext(FILE *file, const char **data, int depth)
> >>>> +{
> >>>> +	int8_t type;
> >>>> +	uint32_t len = mp_decode_extl(data, &type);
> >>>> +	switch(type) {
> >>>> +	case MP_DECIMAL:
> >>>> +		return mp_fprint_decimal(file, data, len);
> >>>> +	case MP_UUID:
> >>>> +		return mp_fprint_uuid(file, data, len);
> >>>> +	case MP_ERROR:
> >>>> +		return mp_fprint_error(file, data, depth);
> >>>> +	default:
> >>>> +		return fprintf(file, "undefined");
> >>>
> >>> I'd come up with more sensible message in case of "undefined" mp_ type.
> >>> For instance: ("undefined mgpack extension (%d)", type).
> >>
> >> This is not an error message.
> > 
> > I understand that. Still I stick to the point that simple "undefined"
> > looks poor.
> 
> Ok, I just noticed that you wrapped your proposal into (). It is not just
> a bare string or something else, that could be treated in 2 ways. Then I
> guess it is ok. Anyway both 'undefined', and parenthesis are JS JSON parsers
> features only. So by adding a few details we are not breaking normal JSON
> parsers any further.
> 
> I changed this commit, and added a new commit to the msgpuck patchset.
> The message is changed a little.
> 
> ====================
> diff --git a/src/box/msgpack.c b/src/box/msgpack.c
> index 37bb3920c..4013aec4c 100644
> --- a/src/box/msgpack.c
> +++ b/src/box/msgpack.c
> @@ -49,7 +49,8 @@ msgpack_fprint_ext(FILE *file, const char **data, int depth)
>  	case MP_ERROR:
>  		return mp_fprint_error(file, data, depth);
>  	default:
> -		return fprintf(file, "undefined");
> +		return fprintf(file, "(extension: type %d, len %u)", (int)type,
> +			       (unsigned)len);
>  	}
>  }
>  
> @@ -66,7 +67,8 @@ msgpack_snprint_ext(char *buf, int size, const char **data, int depth)
>  	case MP_ERROR:
>  		return mp_snprint_error(buf, size, data, depth);
>  	default:
> -		return snprintf(buf, size, "undefined");
> +		return snprintf(buf, size, "(extension: type %d, len %u)",
> +				(int)type, (unsigned)len);
>  	}
>  }
>  

LGTM.

Re: [Tarantool-patches] [PATCH 0/5] mp_snprint() and mp_fprint() for decimal, uuid, error

[Serge Petrenko]

Hi! Thanks for the patchset!

12.05.2020 02:45, Vladislav Shpilevoy пишет:
> The patchset makes msgpuck functions mp_snprint() and mp_fprint()
> nicely serialize MP_DECIMAL, MP_UUID, and MP_ERROR objects.
>
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4719-mp_print-ext
> Issue: https://github.com/tarantool/tarantool/issues/4719
>
> Vladislav Shpilevoy (5):
>    msgpuck: bump version to enable extension printer
>    decimal: provide MP_DECIMAL extension serializer
>    uuid: provide MP_UUID extension serializer
>    error: provide MP_ERROR extension serializer
>    msgpuck: activate MP_EXT custom serializers
>
>   src/box/CMakeLists.txt    |   1 +
>   src/box/box.cc            |   2 +
>   src/box/mp_error.cc       | 161 ++++++++++++++++++++++-
>   src/box/mp_error.h        |  29 ++++
>   src/box/msgpack.c         |  78 +++++++++++
>   src/box/msgpack.h         |  41 ++++++
>   src/lib/core/mp_decimal.c |  18 +++
>   src/lib/core/mp_decimal.h |  27 ++++
>   src/lib/msgpuck           |   2 +-
>   src/lib/uuid/mp_uuid.c    |  18 +++
>   src/lib/uuid/mp_uuid.h    |  27 ++++
>   test/unit/decimal.c       |  63 ++++++++-
>   test/unit/decimal.result  |  11 +-
>   test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
>   test/unit/mp_error.result |  72 +++++++++-
>   test/unit/msgpack.result  |  17 ++-
>   test/unit/uuid.c          |  64 ++++++++-
>   test/unit/uuid.result     |  11 +-
>   18 files changed, 900 insertions(+), 12 deletions(-)
>   create mode 100644 src/box/msgpack.c
>   create mode 100644 src/box/msgpack.h

LGTM

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 0/5] mp_snprint() and mp_fprint() for decimal, uuid, error

[Alexander Turenko]

On Tue, May 12, 2020 at 01:45:47AM +0200, Vladislav Shpilevoy wrote:
> The patchset makes msgpuck functions mp_snprint() and mp_fprint()
> nicely serialize MP_DECIMAL, MP_UUID, and MP_ERROR objects.

Pushed to master.

WBR, Alexander Turenko.