Re: [tarantool-patches] Re: [PATCH v1 1/1] implement mp_stack class

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Wed, Jan 23, 2019 at 11:23:51AM +0300, Kirill Shcherbatov wrote:
> Introduced a new mp_stack class to make complex msgpack parsing.
> The structure is needed in order to facilitate the decoding
> nested MP_ARRAY and MP_MAP msgpack containers without recursion.
> This allows you to determine that the parsing of the current
> container is complete.
> Reworked mp_snprint and mp_fprint routines to use mp_stack
> instead of recursion.
> 
> Needed for #1012

There's no ticket #1012 in the msgpuck repository. Please use a full
link to the github issue.

> ---
>  msgpuck.c      |  63 ++++++++++---------
>  msgpuck.h      | 167 +++++++++++++++++++++++++++++++++++++++++++++++++
>  test/msgpuck.c |  35 ++++++++++-
>  3 files changed, 235 insertions(+), 30 deletions(-)
> 
> diff --git a/msgpuck.c b/msgpuck.c
> index 72b800c..e77ad5b 100644
> --- a/msgpuck.c
> +++ b/msgpuck.c
> @@ -232,8 +232,12 @@ mp_format(char *data, size_t data_size, const char *format, ...)
>  	return res;
>  }
>  
> -#define MP_PRINT(SELF, PRINTF) \
> +#define MP_PRINT(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)) {						\
>  	case MP_NIL:								\
>  		mp_decode_nil(data);						\
> @@ -266,31 +270,21 @@ mp_format(char *data, size_t data_size, const char *format, ...)
>  		break;								\
>  	}									\
>  	case MP_ARRAY:								\
> -	{									\
> -		uint32_t count = mp_decode_array(data);				\
> -		PRINTF("[");							\
> -		uint32_t i;							\
> -		for (i = 0; i < count; i++) {					\
> -			if (i)							\
> -				PRINTF(", ");					\
> -			SELF(data);						\
> -		}								\
> -		PRINTF("]");							\
> -		break;								\
> -	}									\
>  	case MP_MAP:								\
>  	{									\
> -		uint32_t count = mp_decode_map(data);				\
> -		PRINTF("{");							\
> -		uint32_t i;							\
> -		for (i = 0; i < count; i++) {					\
> -			if (i)							\
> -				PRINTF(", ");					\
> -			SELF(data);						\
> -			PRINTF(": ");						\
> -			SELF(data);						\
> +		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);	\

Bad formatting: missing spaces around '*'.

> +			mp_stack_push(&stack, type, count);			\
> +		} else {							\
> +			/*							\
> +			* Skip msgpack items that do not			\
> +			* fit onto the stack.					\
> +			*/							\
> +			PRINTF(type == MP_ARRAY ? "[...]" : "{...}");		\
> +			mp_next(data);						\
>  		}								\
> -		PRINTF("}");							\
>  		break;								\
>  	}									\
>  	case MP_BOOL:								\
> @@ -310,6 +304,21 @@ mp_format(char *data, size_t data_size, const char *format, ...)
>  		mp_unreachable();						\
>  		return -1;							\
>  	}									\
> +	while (!mp_stack_is_empty(&stack)) {					\
> +		struct mp_frame *frame = mp_stack_top(&stack);			\
> +		if (!mp_stack_advance(&stack)) {				\
> +			PRINTF(frame->type == MP_ARRAY ? "]" : "}");		\
> +			mp_stack_pop(&stack);					\
> +		} else {							\
> +			if (frame->curr == 1)					\
> +				PRINTF(frame->type == MP_ARRAY ? "[" : "{");	\
> +			else if (frame->type == MP_MAP && frame->curr % 2 == 0)	\
> +				PRINTF(": ");					\
> +			else							\
> +				PRINTF(", ");					\
> +			goto next;						\
> +		}								\
> +	}									\

This doesn't work for empty maps/arrays. Please fix and extend
test_mp_print:expected to check that.

>  }
>  
>  static inline int
> @@ -323,10 +332,8 @@ mp_fprint_internal(FILE *file, const char **data)
>  	total_bytes += bytes;							\
>  } while (0)
>  #define PRINT(...) HANDLE(fprintf, __VA_ARGS__)
> -#define SELF(...) HANDLE(mp_fprint_internal, __VA_ARGS__)
> -MP_PRINT(SELF, PRINT)
> +MP_PRINT(PRINT)

I asked you to get rid of mp_fprint_internal and mp_snprint_internal in
comments to the previous version.

>  #undef HANDLE
> -#undef SELF
>  #undef PRINT
>  	return total_bytes;
>  }
> @@ -359,10 +366,8 @@ mp_snprint_internal(char *buf, int size, const char **data)
>  	}									\
>  } while (0)
>  #define PRINT(...) HANDLE(snprintf, __VA_ARGS__)
> -#define SELF(...) HANDLE(mp_snprint_internal, __VA_ARGS__)
> -MP_PRINT(SELF, PRINT)
> +MP_PRINT(PRINT)
>  #undef HANDLE
> -#undef SELF
>  #undef PRINT
>  	return total_bytes;
>  }
> diff --git a/test/msgpuck.c b/test/msgpuck.c
> index 9265453..73ace19 100644
> --- a/test/msgpuck.c
> +++ b/test/msgpuck.c
> @@ -764,7 +764,7 @@ test_format(void)
>  int
>  test_mp_print()
>  {
> -	plan(10);
> +	plan(12);
>  	header();
>  
>  	char msgpack[128];
> @@ -838,6 +838,39 @@ test_mp_print()
>  	int rc = mp_fprint(stdin, msgpack);
>  	is(rc, -1, "mp_fprint I/O error");
>  
> +	/* Test mp_snprint max nesting depth. */
> +	int mp_buff_sz = MP_PRINT_MAX_DEPTH * mp_sizeof_array(1) +
> +			 mp_sizeof_uint(1);
> +	int template_sz = 2 * (MP_PRINT_MAX_DEPTH + 1) + 3 + 1;
> +	char *mp_buff = malloc(mp_buff_sz);
> +	char *template = malloc(template_sz);
> +	char *decoded = malloc(template_sz);
> +	if (mp_buff == NULL || template == NULL || decoded == NULL) {
> +		fail("Failed to malloc memory");
> +		goto end;
> +	}

I wouldn't bother checking malloc() return value - after all it's just a
test. If malloc() fails, the test will crash, which is OK.

> +	char *buf_wptr = mp_buff;

mp_buff, but buf_wptr

I understand that it's merely a test, but I just can't help asking: why
can't you be consistent in variable naming?

> +	char *template_wptr = template;
> +	for (int i = 0; i <= 2 * (MP_PRINT_MAX_DEPTH + 1); i++) {
> +		if (i < MP_PRINT_MAX_DEPTH + 1) {
> +			buf_wptr = mp_encode_array(buf_wptr, 1);
> +			*(template_wptr++) = '[';

'template' is a confusing name: it's not a template, it's the expected
output. Sorry, but it's difficult to understand the code when variable
names are misleading.

> +		} else if (i == MP_PRINT_MAX_DEPTH + 1) {
> +			buf_wptr = mp_encode_uint(buf_wptr, 1);
> +			template_wptr += sprintf(template_wptr, "...");

Please use snprintf.

> +		} else {
> +			*(template_wptr++) = ']';
> +		}
> +	}

Please add an assertion making sure that we haven't exceeded the buffer
size.

> +	*(template_wptr++) = '\0';
> +	rc = mp_snprint(decoded, template_sz, mp_buff);
> +	ok(rc == template_sz - 1, "mp_snprint max nesting depth return value");
> +	ok(memcmp(decoded, template, template_sz - 1) == 0,
> +	   "mp_snprint max nesting depth result");

You should use strcmp() to make sure \0 has been written by mp_snprint.

> +end:
> +	free(decoded);
> +	free(template);
> +	free(mp_buff);
>  	footer();
>  	return check_plan();
>  }