From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 23 Jan 2019 13:06:59 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v1 1/1] implement mp_stack class Message-ID: <20190123100659.emxmwzoivrk4e3lh@esperanza> References: <20190117115843.nqellm5mmsglqnlf@tkn_work_nb> <842c55cd-5d3f-e857-42f4-94d648c88273@tarantool.org> <20190117163409.hvga32kxxgcc5sat@tkn_work_nb> <1ba698d1-8450-f2d3-7717-11adbf0d5f4f@tarantool.org> <20190121112541.hkqylj7hwcjeq526@tkn_work_nb> <2259a346-76dd-f721-176f-bfad39b25b5e@gmail.com> <20190121202529.r7wwnzcesdh34sda@esperanza> <09a7ccb3-fa8b-8691-ea26-c3146c42d839@tarantool.org> <20190122202136.uag7irkm2ltg3dkh@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: 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(); > }