[tarantool-patches] Re: [PATCH v1 1/1] implement mp_stack class
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Jan 23 13:06:59 MSK 2019
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();
> }
More information about the Tarantool-patches
mailing list