[Tarantool-patches] [PATCH msgpuck 0/2] MP_EXT virtual serializer

[Tarantool-patches] [PATCH msgpuck 0/2] MP_EXT virtual serializer

[Vladislav Shpilevoy]

The patchset makes MP_EXT serializer for mp_snprint() and
mp_fprint() functions virtual.

The patch implements one possible solution - return recursion
instead of mp_stack into the printers, and make external code
overload both mp_fprint() and mp_snprint(). The most
straightforward solution.

However it is possible to avoid overloading both mp_fprint() and
mp_snprint(). Instead, msgpuck could expose just one virtual
method, which takes an opaque object with method print() having
signature like snprintf() and fprintf() - format and arguments.

mp_snprint() would create that opaque object's print() method as
snprintf(). While mp_fprint() would create this object's print()
method as fprintf(). Drawback of the solution - ton of virtual
calls, because serializers tend to call print() a lot, on small
strings like ':', ', ', and so on. On the other hand, this should
simplify code in Tarantool, especially MP_ERROR serializer, where
code duplication was so big for mp_snprint_error() and
mp_fprint_error(), that I invented some kind of C template for it.

With one virtual method, which accepts a virtual object it would
be just one mp_print_error().

I decided to leave the final decision to a review discussion.
Should we keep it like it is done now, or make it even more
virtual like I described above, or probably there are other ideas.

Branch: http://github.com/tarantool/msgpuck/tree/gerold103/mp_print_ext
Issue: https://github.com/tarantool/tarantool/issues/4719

Vladislav Shpilevoy (2):
  Return recursion to mp_snprint() and mp_fprint()
  Make MP_EXT mp_snprint() and mp_fprint() customizable

 msgpuck.c      | 156 ++++++++++++++++++++++++++++++++-----------------
 msgpuck.h      |  50 ++++++++++++++++
 test/msgpuck.c |  83 +++++++++++++++++++++++++-
 3 files changed, 234 insertions(+), 55 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH msgpuck 1/2] Return recursion to mp_snprint() and mp_fprint()

[Vladislav Shpilevoy]

The commit partially reverts 5a1d91ae2830d9a1f1201c9026724ae3e32d5324
"Implement mp_stack class". The mp_stack struct is still alive,
but is not used for printing anymore.

Despite mp_stack printer looked nice, it still wasn't good enough
by a couple of reasons:

- On every print it allocated all 32 stack frames;

- It wasn't flexible. The solution stops being so simple, when
  number of states for every stack frame grows drastically. This
  is the case of the future MP_EXT serializer.

Lets elaborate the last problem. MP_EXT serializer in theory can
do 3 possible things:

- Return a plain text ready to print. For example, a decimal
  extension may return a ready-to-print string with a floating
  point number. Or UUID may return a UUID string.

- Return a pointer to non-MP_EXT MessagePack to continue
  serialization by msgpuck built-in means. For example, if an
  object is a mere MP_MAP behind MP_EXT, then the serializer could
  just remove MP_EXT header and let the other part be serialized
  normally.

- In case of a complex object underneath, with non-MessagePack
  elements in it, with some of its MessagePack internals needed to
  be substituted before printing, with necessity to avoid copying
  and recursion (otherwise mp_stack becomes useless anyway), the
  serializer can return a tree-like structure with pointers at
  raw MessagePack, at plain text, at more nested objects of this
  kind. For example, consider this:

  MP_EXT: MP_ERROR: <MP_MAP> {
    MP_ERROR_STACK 0x00: <MP_ARRAY> [
        <MP_MAP> {
            MP_ERROR_TYPE 0x00: <MP_STR>,
            MP_ERROR_FILE 0x01: <MP_STR>,
            MP_ERROR_PAYLOAD 0x02: <MP_MAP> {
                ...
            }
        },
        ...
    ],
    ...
  }

  If this would be printed using the previous way (trim MP_EXT and
  print normally), this would look like this:

  {0: [{0: 'clienterror', 1: 'test.c', 2: {...}}]}

  However it may be needed to serialize it like this:

  {'stack': [{'type': 'clienterror', 'file': 'test.c', 'payload': {...}}]}

  So the original MP_ERROR object should be split into a
  non-binary tree, where nodes can point at plain static data
  (strings 'type', 'file'), at the original MessagePack (payload
  should point at the original object, to avoid copying and
  recursion), at an array or map of these objects (like 'stack').

This runs out of control even quicker, when comes realization,
that all the 3 return ways may return something, what needs
destruction after printing.

However complexity level still would be bearable if the last
option wouldn't make the stack stop being a stack. Indeed,
whatever it returns, if it is not a scalar ready-to-print plain
object, it should be pushed on the stack. When push a tree on the
stack, it stops being a stack. Since we will start working not
with top of the stack.

The commit makes decision to just return recursion into
mp_snprint() and mp_fprint() functions. That allows to use
recursion in future mp_ext serializers as well, and makes to code
more similar to all the other serializers we have.

Part of https://github.com/tarantool/tarantool/issues/4719
---
 msgpuck.c | 133 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 79 insertions(+), 54 deletions(-)

diff --git a/msgpuck.c b/msgpuck.c
index d0ffb83..7ea86e8 100644
--- a/msgpuck.c
+++ b/msgpuck.c
@@ -232,31 +232,28 @@ mp_format(char *data, size_t data_size, const char *format, ...)
 	return res;
 }
 
-#define MP_PRINT(PRINTF) \
+#define MP_PRINT(SELF, PRINTF) \
 {										\
-	struct mp_frame frames[MP_PRINT_MAX_DEPTH];				\
-	struct mp_stack stack;							\
-	mp_stack_create(&stack, MP_PRINT_MAX_DEPTH, frames);			\
-next:										\
-	switch (mp_typeof(*data)) {						\
+	switch (mp_typeof(**data)) {						\
 	case MP_NIL:								\
-		mp_decode_nil(&data);						\
+		mp_decode_nil(data);						\
 		PRINTF("null");							\
 		break;								\
 	case MP_UINT:								\
-		PRINTF("%llu", (unsigned long long) mp_decode_uint(&data));	\
+		PRINTF("%llu", (unsigned long long) mp_decode_uint(data));	\
 		break;								\
 	case MP_INT:								\
-		PRINTF("%lld", (long long) mp_decode_int(&data));		\
+		PRINTF("%lld", (long long) mp_decode_int(data));		\
 		break;								\
 	case MP_STR:								\
 	case MP_BIN:								\
 	{									\
-		uint32_t len = mp_typeof(*data) == MP_STR ?			\
-			mp_decode_strl(&data) : mp_decode_binl(&data);		\
+		uint32_t len = mp_typeof(**data) == MP_STR ?			\
+			mp_decode_strl(data) : mp_decode_binl(data);		\
 		PRINTF("\"");							\
 		const char *s;							\
-		for (s = data; s < data + len; s++) {				\
+		const char *end = *data + len;					\
+		for (s = *data; s < end; s++) {					\
 			unsigned char c = (unsigned char ) *s;			\
 			if (c < 128 && mp_char2escape[c] != NULL) {		\
 				/* Escape character */				\
@@ -266,84 +263,102 @@ next:										\
 			}							\
 		}								\
 		PRINTF("\"");							\
-		data += len;							\
+		*data = end;							\
 		break;								\
 	}									\
 	case MP_ARRAY:								\
+	{									\
+		PRINTF("[");							\
+		if (depth <= 0) {						\
+			PRINTF("...");						\
+			mp_next(data);						\
+		} else {							\
+			--depth;						\
+			uint32_t count = mp_decode_array(data);			\
+			for (uint32_t i = 0; i < count; i++) {			\
+				if (i)						\
+					PRINTF(", ");				\
+				SELF(data);					\
+			}							\
+			++depth;						\
+		}								\
+		PRINTF("]");							\
+		break;								\
+	}									\
 	case MP_MAP:								\
 	{									\
-		enum mp_type type = mp_typeof(*data);				\
-		if (!mp_stack_is_full(&stack)) {				\
-			uint32_t count = type == MP_ARRAY ?			\
-					 mp_decode_array(&data) :		\
-					 2 * mp_decode_map(&data);		\
-			mp_stack_push(&stack, type, count);			\
+		PRINTF("{");							\
+		if (depth <= 0) {						\
+			PRINTF("...");						\
+			mp_next(data);						\
 		} else {							\
-			/*							\
-			* Skip msgpack items that do not			\
-			* fit onto the stack.					\
-			*/							\
-			PRINTF(type == MP_ARRAY ? "[...]" : "{...}");		\
-			mp_next(&data);						\
+			--depth;						\
+			uint32_t count = mp_decode_map(data);			\
+			for (uint32_t i = 0; i < count; i++) {			\
+				if (i)						\
+					PRINTF(", ");				\
+				SELF(data);					\
+				PRINTF(": ");					\
+				SELF(data);					\
+			}							\
+			++depth;						\
 		}								\
+		PRINTF("}");							\
 		break;								\
 	}									\
 	case MP_BOOL:								\
-		PRINTF(mp_decode_bool(&data) ? "true" : "false");		\
+		PRINTF(mp_decode_bool(data) ? "true" : "false");		\
 		break;								\
 	case MP_FLOAT:								\
-		PRINTF("%g", mp_decode_float(&data));				\
+		PRINTF("%g", mp_decode_float(data));				\
 		break;								\
 	case MP_DOUBLE:								\
-		PRINTF("%lg", mp_decode_double(&data));				\
+		PRINTF("%lg", mp_decode_double(data));				\
 		break;								\
 	case MP_EXT:								\
-		mp_next(&data);							\
+		mp_next(data);							\
 		PRINTF("undefined");						\
 		break;								\
 	default:								\
 		mp_unreachable();						\
 		return -1;							\
 	}									\
-	while (!mp_stack_is_empty(&stack)) {					\
-		struct mp_frame *frame = mp_stack_top(&stack);			\
-		if (frame->idx < 0)						\
-			PRINTF(frame->type == MP_ARRAY ? "[" : "{");		\
-		if (!mp_frame_advance(frame)) {					\
-			PRINTF(frame->type == MP_ARRAY ? "]" : "}");		\
-			mp_stack_pop(&stack);					\
-			continue;						\
-		} else if (frame->idx != 0) {					\
-			PRINTF(frame->type == MP_MAP && frame->idx % 2 == 1 ?	\
-			       ": " : ", ");					\
-		}								\
-		goto next;							\
-	}									\
 }
 
-int
-mp_fprint(FILE *file, const char *data)
+static int
+mp_fprint_recursion(FILE *file, const char **data, int depth)
 {
-	if (!file)
-		file = stdout;
 	int total_bytes = 0;
-#define PRINT(...) do {								\
-	int bytes = fprintf(file, __VA_ARGS__);					\
+#define HANDLE(FUN, ...) do {							\
+	int bytes = FUN(file, __VA_ARGS__);					\
 	if (mp_unlikely(bytes < 0))						\
 		return -1;							\
 	total_bytes += bytes;							\
 } while (0)
-	MP_PRINT(PRINT)
+#define PRINT(...) HANDLE(fprintf, __VA_ARGS__)
+#define SELF(...) HANDLE(mp_fprint_recursion, __VA_ARGS__, depth)
+MP_PRINT(SELF, PRINT)
+#undef HANDLE
+#undef SELF
 #undef PRINT
 	return total_bytes;
 }
 
 int
-mp_snprint(char *buf, int size, const char *data)
+mp_fprint(FILE *file, const char *data)
+{
+	if (!file)
+		file = stdout;
+	int res = mp_fprint_recursion(file, &data, MP_PRINT_MAX_DEPTH);
+	return res;
+}
+
+static int
+mp_snprint_recursion(char *buf, int size, const char **data, int depth)
 {
 	int total_bytes = 0;
-#define PRINT(...) do {								\
-	int bytes = snprintf(buf, size, __VA_ARGS__);				\
+#define HANDLE(FUN, ...) do {							\
+	int bytes = FUN(buf, size, __VA_ARGS__);				\
 	if (mp_unlikely(bytes < 0))						\
 		return -1;							\
 	total_bytes += bytes;							\
@@ -356,8 +371,18 @@ mp_snprint(char *buf, int size, const char *data)
 		size = 0;							\
 	}									\
 } while (0)
-	MP_PRINT(PRINT)
+#define PRINT(...) HANDLE(snprintf, __VA_ARGS__)
+#define SELF(...) HANDLE(mp_snprint_recursion, __VA_ARGS__, depth)
+MP_PRINT(SELF, PRINT)
+#undef HANDLE
+#undef SELF
 #undef PRINT
 	return total_bytes;
 }
 #undef MP_PRINT
+
+int
+mp_snprint(char *buf, int size, const char *data)
+{
+	return mp_snprint_recursion(buf, size, &data, MP_PRINT_MAX_DEPTH);
+}
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH msgpuck 2/2] Make MP_EXT mp_snprint() and mp_fprint() customizable

[Vladislav Shpilevoy]

MP_EXT in these functions was skipped and printed as 'undefined'.
In Tarantool there are already 3 extensions - decimals, UUID,
errors. It is time to make them being nicely printed too.

The patch makes it possible via virtual MP_EXT serializer, which
by default does the same as before. But in Tarantool is
substituted with correct printer for the mentioned extensions.

Part of https://github.com/tarantool/tarantool/issues/4719
---
 msgpuck.c      | 31 ++++++++++++++++---
 msgpuck.h      | 50 ++++++++++++++++++++++++++++++
 test/msgpuck.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 159 insertions(+), 5 deletions(-)

diff --git a/msgpuck.c b/msgpuck.c
index 7ea86e8..1ee234b 100644
--- a/msgpuck.c
+++ b/msgpuck.c
@@ -33,6 +33,26 @@
 #define MP_LIBRARY 1
 #include "msgpuck.h"
 
+int
+mp_fprint_ext_default(FILE *file, const char **data, int depth)
+{
+	(void) depth;
+	mp_next(data);
+	return fprintf(file, "undefined");
+}
+
+int
+mp_snprint_ext_default(char *buf, int size, const char **data, int depth)
+{
+	(void) depth;
+	mp_next(data);
+	return snprintf(buf, size, "undefined");
+}
+
+mp_fprint_ext_f mp_fprint_ext = mp_fprint_ext_default;
+
+mp_snprint_ext_f mp_snprint_ext = mp_snprint_ext_default;
+
 size_t
 mp_vformat(char *data, size_t data_size, const char *format, va_list vl)
 {
@@ -316,8 +336,7 @@ mp_format(char *data, size_t data_size, const char *format, ...)
 		PRINTF("%lg", mp_decode_double(data));				\
 		break;								\
 	case MP_EXT:								\
-		mp_next(data);							\
-		PRINTF("undefined");						\
+		PRINT_EXT(data);						\
 		break;								\
 	default:								\
 		mp_unreachable();						\
@@ -325,7 +344,7 @@ mp_format(char *data, size_t data_size, const char *format, ...)
 	}									\
 }
 
-static int
+int
 mp_fprint_recursion(FILE *file, const char **data, int depth)
 {
 	int total_bytes = 0;
@@ -335,12 +354,14 @@ mp_fprint_recursion(FILE *file, const char **data, int depth)
 		return -1;							\
 	total_bytes += bytes;							\
 } while (0)
+#define PRINT_EXT(...) HANDLE(mp_fprint_ext, __VA_ARGS__, depth)
 #define PRINT(...) HANDLE(fprintf, __VA_ARGS__)
 #define SELF(...) HANDLE(mp_fprint_recursion, __VA_ARGS__, depth)
 MP_PRINT(SELF, PRINT)
 #undef HANDLE
 #undef SELF
 #undef PRINT
+#undef PRINT_EXT
 	return total_bytes;
 }
 
@@ -353,7 +374,7 @@ mp_fprint(FILE *file, const char *data)
 	return res;
 }
 
-static int
+int
 mp_snprint_recursion(char *buf, int size, const char **data, int depth)
 {
 	int total_bytes = 0;
@@ -371,12 +392,14 @@ mp_snprint_recursion(char *buf, int size, const char **data, int depth)
 		size = 0;							\
 	}									\
 } while (0)
+#define PRINT_EXT(...) HANDLE(mp_snprint_ext, __VA_ARGS__, depth)
 #define PRINT(...) HANDLE(snprintf, __VA_ARGS__)
 #define SELF(...) HANDLE(mp_snprint_recursion, __VA_ARGS__, depth)
 MP_PRINT(SELF, PRINT)
 #undef HANDLE
 #undef SELF
 #undef PRINT
+#undef PRINT_EXT
 	return total_bytes;
 }
 #undef MP_PRINT
diff --git a/msgpuck.h b/msgpuck.h
index 5dfbbd9..91c7693 100644
--- a/msgpuck.h
+++ b/msgpuck.h
@@ -980,6 +980,30 @@ mp_vformat(char *data, size_t data_size, const char *format, va_list args);
 int
 mp_fprint(FILE *file, const char *data);
 
+/**
+ * \brief Print MsgPack data to \a file using JSON-like format.
+ * Works exactly like \sa mp_fprint(), but allows to specify max
+ * depth, and changes \a data parameter. Intended to be used for
+ * MsgPack serialization recursion.
+ */
+int
+mp_fprint_recursion(FILE *file, const char **data, int depth);
+
+typedef int (*mp_fprint_ext_f)(FILE *file, const char **data, int depth);
+
+/**
+ * \brief Function called when need to serialize MP_EXT into a
+ * file.
+ */
+extern mp_fprint_ext_f mp_fprint_ext;
+
+/**
+ * \brief Default MP_EXT serializer into a file. Skips the object,
+ * ignores all the other arguments, and writes 'undefined'.
+ */
+int
+mp_fprint_ext_default(FILE *file, const char **data, int depth);
+
 /**
  * \brief format MsgPack data to \a buf using JSON-like format.
  * \sa mp_fprint()
@@ -997,6 +1021,32 @@ mp_fprint(FILE *file, const char *data);
 int
 mp_snprint(char *buf, int size, const char *data);
 
+/**
+ * \brief Format MsgPack data to \a buf using JSON-like format.
+ * Works exactly like \sa mp_snprint(), but allows to specify max
+ * depth, and changes \a data parameter. Intended to be used for
+ * MsgPack serialization recursion.
+ */
+int
+mp_snprint_recursion(char *buf, int size, const char **data, int depth);
+
+typedef int (*mp_snprint_ext_f)(char *buf, int size, const char **data,
+				int depth);
+
+/**
+ * \brief Function called when need to serialize MP_EXT into a
+ * string.
+ */
+extern mp_snprint_ext_f mp_snprint_ext;
+
+/**
+ * \brief Default MP_EXT serializer into a string. Skips the
+ * object, ignores all the other arguments, and prints
+ * 'undefined'.
+ */
+int
+mp_snprint_ext_default(char *buf, int size, const char **data, int depth);
+
 /**
  * \brief Check that \a cur buffer has enough bytes to decode a string header
  * \param cur buffer
diff --git a/test/msgpuck.c b/test/msgpuck.c
index ecc2b26..1d8c726 100644
--- a/test/msgpuck.c
+++ b/test/msgpuck.c
@@ -969,6 +969,86 @@ test_mp_print()
 	return check_plan();
 }
 
+enum mp_ext_test_type {
+	MP_EXT_TEST_PLAIN,
+	MP_EXT_TEST_MSGPACK,
+};
+
+static int
+mp_fprint_ext_test(FILE *file, const char **data, int depth)
+{
+	int8_t type;
+	uint32_t len = mp_decode_extl(data, &type);
+	const char *ext = *data;
+	*data += len;
+	switch(type) {
+	case MP_EXT_TEST_PLAIN:
+		return fprintf(file, "%.*s", len, ext);
+	case MP_EXT_TEST_MSGPACK:
+		return mp_fprint_recursion(file, &ext, depth);
+	}
+	return fprintf(file, "undefined");
+}
+
+static int
+mp_snprint_ext_test(char *buf, int size, const char **data, int depth)
+{
+	int8_t type;
+	uint32_t len = mp_decode_extl(data, &type);
+	const char *ext = *data;
+	*data += len;
+	switch(type) {
+	case MP_EXT_TEST_PLAIN:
+		return snprintf(buf, size, "%.*s", len, ext);
+	case MP_EXT_TEST_MSGPACK:
+		return mp_snprint_recursion(buf, size, &ext, depth);
+	}
+	return snprintf(buf, size, "undefined");
+}
+
+static int
+test_mp_print_ext(void)
+{
+	plan(5);
+	header();
+	mp_snprint_ext = mp_snprint_ext_test;
+	mp_fprint_ext = mp_fprint_ext_test;
+
+	char *pos = buf;
+	const char *plain = "plain-str";
+	size_t plain_len = strlen(plain);
+	pos = mp_encode_array(pos, 4);
+	pos = mp_encode_uint(pos, 100);
+	pos = mp_encode_ext(pos, MP_EXT_TEST_PLAIN, plain, plain_len);
+	pos = mp_encode_extl(pos, MP_EXT_TEST_MSGPACK,
+			     mp_sizeof_str(plain_len));
+	pos = mp_encode_str(pos, plain, plain_len);
+	pos = mp_encode_uint(pos, 200);
+
+	int size = mp_snprint(NULL, 0, buf);
+	int real_size = mp_snprint(str, sizeof(str), buf);
+	is(size, real_size, "mp_snrpint size match");
+	const char *expected = "[100, plain-str, \"plain-str\", 200]";
+	is(strcmp(str, expected), 0, "str is correct");
+
+	FILE *tmpf = tmpfile();
+	if (tmpf == NULL)
+		abort();
+	real_size = mp_fprint(tmpf, buf);
+	is(size, real_size, "mp_fprint size match");
+	rewind(tmpf);
+	real_size = (int) fread(str, 1, sizeof(str), tmpf);
+	is(real_size, size, "mp_fprint written correct number of bytes");
+	str[real_size] = 0;
+	is(strcmp(str, expected), 0, "str is correct");
+	fclose(tmpf);
+
+	mp_snprint_ext = mp_snprint_ext_default;
+	mp_fprint_ext = mp_fprint_ext_default;
+	footer();
+	return check_plan();
+}
+
 int
 test_mp_check()
 {
@@ -1226,7 +1306,7 @@ test_overflow()
 
 int main()
 {
-	plan(22);
+	plan(23);
 	test_uints();
 	test_ints();
 	test_bools();
@@ -1246,6 +1326,7 @@ int main()
 	test_compare_uints();
 	test_format();
 	test_mp_print();
+	test_mp_print_ext();
 	test_mp_check();
 	test_numbers();
 	test_overflow();
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH msgpuck 1.5/2] Provide more details at MP_EXT mp_fprint/snprint()

[Vladislav Shpilevoy]

It was printed as 'undefined'. Which is not really
helpful. The very least what can be done - print type
and size.
---
 msgpuck.c      | 9 +++++++--
 test/msgpuck.c | 9 ++++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/msgpuck.c b/msgpuck.c
index 7ea86e8..bdaa4ae 100644
--- a/msgpuck.c
+++ b/msgpuck.c
@@ -316,9 +316,14 @@ mp_format(char *data, size_t data_size, const char *format, ...)
 		PRINTF("%lg", mp_decode_double(data));				\
 		break;								\
 	case MP_EXT:								\
-		mp_next(data);							\
-		PRINTF("undefined");						\
+	{									\
+		int8_t type;							\
+		uint32_t len;							\
+		mp_decode_ext(data, &type, &len);				\
+		PRINTF("(extension: type %d, len %u)", (int)type,		\
+		       (unsigned)len);						\
 		break;								\
+	}									\
 	default:								\
 		mp_unreachable();						\
 		return -1;							\
diff --git a/test/msgpuck.c b/test/msgpuck.c
index ecc2b26..eaf50e0 100644
--- a/test/msgpuck.c
+++ b/test/msgpuck.c
@@ -883,8 +883,10 @@ test_mp_print()
 	d = mp_encode_double(d, 3.14);
 	d = mp_encode_uint(d, 100);
 	d = mp_encode_uint(d, 500);
-	/* let's pack zero-length ext */
-	d = mp_encode_extl(d, 0, 0);
+	/* MP_EXT with type 123 and of size 3 bytes. */
+	d = mp_encode_extl(d, 123, 3);
+	memcpy(d, "str", 3);
+	d += 3;
 	char bin[] = "\x12test\x34\b\t\n\"bla\\-bla\"\f\r";
 	d = mp_encode_bin(d, bin, sizeof(bin));
 	d = mp_encode_map(d, 0);
@@ -893,7 +895,8 @@ test_mp_print()
 	const char *expected =
 		"[-5, 42, \"kill bill\", [], "
 		"{\"bool true\": true, \"bool false\": false, \"null\": null, "
-		"\"float\": 3.14, \"double\": 3.14, 100: 500}, undefined, "
+		"\"float\": 3.14, \"double\": 3.14, 100: 500}, "
+		"(extension: type 123, len 3), "
 		"\"\\u0012test4\\b\\t\\n\\\"bla\\\\-bla\\\"\\f\\r\\u0000\", {}]";
 	int esize = strlen(expected);
 
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH msgpuck 0/2] MP_EXT virtual serializer

[Serge Petrenko]

12.05.2020 02:46, Vladislav Shpilevoy пишет:
> The patchset makes MP_EXT serializer for mp_snprint() and
> mp_fprint() functions virtual.
>
> The patch implements one possible solution - return recursion
> instead of mp_stack into the printers, and make external code
> overload both mp_fprint() and mp_snprint(). The most
> straightforward solution.
>
> However it is possible to avoid overloading both mp_fprint() and
> mp_snprint(). Instead, msgpuck could expose just one virtual
> method, which takes an opaque object with method print() having
> signature like snprintf() and fprintf() - format and arguments.
>
> mp_snprint() would create that opaque object's print() method as
> snprintf(). While mp_fprint() would create this object's print()
> method as fprintf(). Drawback of the solution - ton of virtual
> calls, because serializers tend to call print() a lot, on small
> strings like ':', ', ', and so on. On the other hand, this should
> simplify code in Tarantool, especially MP_ERROR serializer, where
> code duplication was so big for mp_snprint_error() and
> mp_fprint_error(), that I invented some kind of C template for it.
>
> With one virtual method, which accepts a virtual object it would
> be just one mp_print_error().
>
> I decided to leave the final decision to a review discussion.
> Should we keep it like it is done now, or make it even more
> virtual like I described above, or probably there are other ideas.
>
> Branch: http://github.com/tarantool/msgpuck/tree/gerold103/mp_print_ext
> Issue: https://github.com/tarantool/tarantool/issues/4719
>
> Vladislav Shpilevoy (2):
>    Return recursion to mp_snprint() and mp_fprint()
>    Make MP_EXT mp_snprint() and mp_fprint() customizable
>
>   msgpuck.c      | 156 ++++++++++++++++++++++++++++++++-----------------
>   msgpuck.h      |  50 ++++++++++++++++
>   test/msgpuck.c |  83 +++++++++++++++++++++++++-
>   3 files changed, 234 insertions(+), 55 deletions(-)


Hi! Thanks for the patchset! LGTM(including patch 1.5/2)

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH msgpuck 0/2] MP_EXT virtual serializer

[Cyrill Gorcunov]

Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

On Mon, May 18, 2020 at 6:18 PM Serge Petrenko
<sergepetrenko@tarantool.org> wrote:
>
>
> 12.05.2020 02:46, Vladislav Shpilevoy пишет:
> > The patchset makes MP_EXT serializer for mp_snprint() and
> > mp_fprint() functions virtual.
> >
> > The patch implements one possible solution - return recursion
> > instead of mp_stack into the printers, and make external code
> > overload both mp_fprint() and mp_snprint(). The most
> > straightforward solution.
> >
> > However it is possible to avoid overloading both mp_fprint() and
> > mp_snprint(). Instead, msgpuck could expose just one virtual
> > method, which takes an opaque object with method print() having
> > signature like snprintf() and fprintf() - format and arguments.
> >
> > mp_snprint() would create that opaque object's print() method as
> > snprintf(). While mp_fprint() would create this object's print()
> > method as fprintf(). Drawback of the solution - ton of virtual
> > calls, because serializers tend to call print() a lot, on small
> > strings like ':', ', ', and so on. On the other hand, this should
> > simplify code in Tarantool, especially MP_ERROR serializer, where
> > code duplication was so big for mp_snprint_error() and
> > mp_fprint_error(), that I invented some kind of C template for it.
> >
> > With one virtual method, which accepts a virtual object it would
> > be just one mp_print_error().
> >
> > I decided to leave the final decision to a review discussion.
> > Should we keep it like it is done now, or make it even more
> > virtual like I described above, or probably there are other ideas.
> >
> > Branch: http://github.com/tarantool/msgpuck/tree/gerold103/mp_print_ext
> > Issue: https://github.com/tarantool/tarantool/issues/4719
> >
> > Vladislav Shpilevoy (2):
> >    Return recursion to mp_snprint() and mp_fprint()
> >    Make MP_EXT mp_snprint() and mp_fprint() customizable
> >
> >   msgpuck.c      | 156 ++++++++++++++++++++++++++++++++-----------------
> >   msgpuck.h      |  50 ++++++++++++++++
> >   test/msgpuck.c |  83 +++++++++++++++++++++++++-
> >   3 files changed, 234 insertions(+), 55 deletions(-)
>
>
> Hi! Thanks for the patchset! LGTM(including patch 1.5/2)
>
> --
> Serge Petrenko
>

Re: [Tarantool-patches] [PATCH msgpuck 1.5/2] Provide more details at MP_EXT mp_fprint/snprint()

[Alexander Turenko]

>  	case MP_EXT:								\
> -		mp_next(data);							\
> -		PRINTF("undefined");						\
> +	{									\
> +		int8_t type;							\
> +		uint32_t len;							\
> +		mp_decode_ext(data, &type, &len);				\
> +		PRINTF("(extension: type %d, len %u)", (int)type,		\
> +		       (unsigned)len);						\

The header file still contains this comment:

 | /**
 |  * \brief Default MP_EXT serializer into a file. Skips the object,
 |  * ignores all the other arguments, and writes 'undefined'.
 |                                         ^^^^^^^^^^^^^^^^^^
 |  */
 | int
 | mp_fprint_ext_default(FILE *file, const char **data, int depth);

(Same for mp_snprint_ext_default).

Re: [Tarantool-patches] [PATCH msgpuck 1.5/2] Provide more details at MP_EXT mp_fprint/snprint()

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 19/05/2020 14:14, Alexander Turenko wrote:
>>  	case MP_EXT:								\
>> -		mp_next(data);							\
>> -		PRINTF("undefined");						\
>> +	{									\
>> +		int8_t type;							\
>> +		uint32_t len;							\
>> +		mp_decode_ext(data, &type, &len);				\
>> +		PRINTF("(extension: type %d, len %u)", (int)type,		\
>> +		       (unsigned)len);						\
> 
> The header file still contains this comment:
> 
>  | /**
>  |  * \brief Default MP_EXT serializer into a file. Skips the object,
>  |  * ignores all the other arguments, and writes 'undefined'.
>  |                                         ^^^^^^^^^^^^^^^^^^
>  |  */
>  | int
>  | mp_fprint_ext_default(FILE *file, const char **data, int depth);
> 
> (Same for mp_snprint_ext_default).

Shame on me, I didn't notice it. Changed on the branch in this commit.
See below diff. Also I changed comments in the third commit. See in the
other email.

====================
diff --git a/msgpuck.h b/msgpuck.h
index 5dfbbd9..82e078b 100644
--- a/msgpuck.h
+++ b/msgpuck.h
@@ -970,7 +970,19 @@ mp_vformat(char *data, size_t data_size, const char *format, va_list args);
 
 /**
  * \brief print MsgPack data \a file using JSON-like format.
- * MP_EXT is printed as "undefined"
+ * MP_EXT is printed as a non-standard JSON 'list':
+ *
+ *     (extension: type <type>, len <len>)
+ *
+ * For example:
+ *
+ *     (extension: type 10, len 35)
+ *
+ * Type is the MP_EXT type. Length is of the MP_EXT body, not
+ * counting its header. Since the 'list' and what is in it is not
+ * a standard JSON, printing a MessagePack buffer, having MP_EXT
+ * in it, may lead to an invalid JSON.
+ *
  * \param file - pointer to file (or NULL for stdout)
  * \param data - pointer to buffer containing msgpack object
  * \retval >=0 - the number of bytes printed
@@ -982,6 +994,8 @@ mp_fprint(FILE *file, const char *data);
 
 /**
  * \brief format MsgPack data to \a buf using JSON-like format.
+ * Behaves the same as \sa mp_fprint(), but with snprintf()
+ * semantics.
  * \sa mp_fprint()
  * \param buf - buffer to use
  * \param size - buffer size. This function write at most size bytes

Re: [Tarantool-patches] [PATCH msgpuck 2/2] Make MP_EXT mp_snprint() and mp_fprint() customizable

[Vladislav Shpilevoy]

Changed some comments:

====================
diff --git a/msgpuck.h b/msgpuck.h
index a92f177..16d72d5 100644
--- a/msgpuck.h
+++ b/msgpuck.h
@@ -983,6 +983,9 @@ mp_vformat(char *data, size_t data_size, const char *format, va_list args);
  * a standard JSON, printing a MessagePack buffer, having MP_EXT
  * in it, may lead to an invalid JSON.
  *
+ * However MP_EXT may be printed differently in case a proper
+ * virtual serializer was installed. \sa mp_fprint_ext_f.
+ *
  * \param file - pointer to file (or NULL for stdout)
  * \param data - pointer to buffer containing msgpack object
  * \retval >=0 - the number of bytes printed
@@ -1011,7 +1014,11 @@ extern mp_fprint_ext_f mp_fprint_ext;
 
 /**
  * \brief Default MP_EXT serializer into a file. Skips the object,
- * ignores all the other arguments, and writes 'undefined'.
+ * ignores all the other arguments, and writes
+ *
+ *     (extension: type <type>, len <len>)
+ *
+ * \sa mp_fprint().
  */
 int
 mp_fprint_ext_default(FILE *file, const char **data, int depth);
@@ -1056,7 +1063,10 @@ extern mp_snprint_ext_f mp_snprint_ext;
 /**
  * \brief Default MP_EXT serializer into a string. Skips the
  * object, ignores all the other arguments, and prints
- * 'undefined'.
+ *
+ *     (extension: type <type>, len <len>)
+ *
+ * \sa mp_snprint().
  */
 int
 mp_snprint_ext_default(char *buf, int size, const char **data, int depth);

Re: [Tarantool-patches] [PATCH msgpuck 0/2] MP_EXT virtual serializer

[Alexander Turenko]

On Tue, May 12, 2020 at 01:46:00AM +0200, Vladislav Shpilevoy wrote:
> The patchset makes MP_EXT serializer for mp_snprint() and
> mp_fprint() functions virtual.

Pushed to msgpuck's master.

WBR, Alexander Turenko.