From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EAAF24696C3 for ; Thu, 2 Apr 2020 01:24:57 +0300 (MSK) Date: Wed, 1 Apr 2020 22:24:56 +0000 From: Nikita Pettik Message-ID: <20200401222456.GA27659@tarantool.org> References: <3bb7c695b34356137897b953281543c46fe3bafa.1585097339.git.korablev@tarantool.org> <20200401162618.GD26447@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200401162618.GD26447@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 01 Apr 16:26, Nikita Pettik wrote: > @@ -1169,7 +1166,7 @@ xrow_decode_error(struct xrow_header *row) > } else if (key == IPROTO_ERROR_STACK ) { > if (mp_check_array(pos, end) != 0) > goto error; > - if (iproto_decode_error_stack(&pos, end) != 0) > + if (iproto_decode_error_stack(&pos) != 0) > continue; > } else { > mp_next(&pos); > > > I don't see any tests on that. Are there some? Maybe at least > > unit tests, in xrow.cc? Although a proper test would try to > > send an error stack through applier. Because xrow_decode_error() > > is used only there. > > I'm a bit new to replication tests. I've been working on it. > Could you please review the rest of changes so far? Well, I've come up with following unit tests (and a bit more fixes of xrow_decode_error()): diff --git a/src/box/xrow.c b/src/box/xrow.c index 596c765b7..f33226dc5 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -1094,8 +1094,7 @@ iproto_decode_error_stack(const char **pos) * box_error_add() does not replace previous errors. */ box_error_clear(); - if (mp_typeof(**pos) != MP_ARRAY) - return -1; + assert(mp_typeof(**pos) == MP_ARRAY); uint32_t stack_sz = mp_decode_array(pos); for (uint32_t i = 0; i < stack_sz; i++) { uint32_t code = 0; @@ -1163,11 +1162,11 @@ xrow_decode_error(struct xrow_header *row) const char *str = mp_decode_str(&pos, &len); snprintf(error, sizeof(error), "%.*s", len, str); box_error_set(__FILE__, __LINE__, code, error); - } else if (key == IPROTO_ERROR_STACK ) { - if (mp_check_array(pos, end) != 0) + } else if (key == IPROTO_ERROR_STACK) { + if (mp_typeof(*pos) != MP_ARRAY) goto error; if (iproto_decode_error_stack(&pos) != 0) - continue; + goto error; } else { mp_next(&pos); continue; diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc index 68a334239..954f22f16 100644 --- a/test/unit/xrow.cc +++ b/test/unit/xrow.cc @@ -241,6 +241,140 @@ test_xrow_header_encode_decode() check_plan(); } +static char * +error_stack_entry_encode(char *pos, const char *err_str) +{ + pos = mp_encode_map(pos, 2); + pos = mp_encode_uint(pos, IPROTO_ERROR_CODE); + pos = mp_encode_uint(pos, 159); + pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE); + pos = mp_encode_str(pos, err_str, strlen(err_str)); + return pos; +} + +void +test_xrow_error_stack_decode() +{ + plan(14); + char buffer[2048]; + /* + * To start with, let's test the simplest and obsolete + * way of setting errors. + */ + char *pos = mp_encode_map(buffer, 1); + pos = mp_encode_uint(pos, IPROTO_ERROR); + pos = mp_encode_str(pos, "e1", strlen("e1")); + + struct xrow_header header; + header.bodycnt = 666; + header.type = 159 | IPROTO_TYPE_ERROR; + header.body[0].iov_base = buffer; + header.body[0].iov_len = pos - buffer; + + xrow_decode_error(&header); + struct error *last = diag_last_error(diag_get()); + isnt(last, NULL, "xrow_decode succeed: diag has been set"); + is(strcmp(last->errmsg, "e1"), 0, + "xrow_decode succeed: error is parsed"); + + pos = mp_encode_map(buffer, 1); + pos = mp_encode_uint(pos, IPROTO_ERROR_STACK); + pos = mp_encode_array(pos, 2); + pos = error_stack_entry_encode(pos, "e1"); + pos = error_stack_entry_encode(pos, "e2"); + header.body[0].iov_base = buffer; + header.body[0].iov_len = pos - buffer; + + diag_clear(diag_get()); + xrow_decode_error(&header); + last = diag_last_error(diag_get()); + isnt(last, NULL, "xrow_decode succeed: diag has been set"); + is(strcmp(last->errmsg, "e2"), 0, "xrow_decode error stack suceed: " + "e2 at the top of stack"); + last = last->cause; + isnt(last, NULL, "xrow_decode succeed: 'cause' is present in stack") + is(strcmp(last->errmsg, "e1"), 0, "xrow_decode succeed: " + "stack has been parsed"); + /* + * Let's try decode broken stack. Variations: + * 1. Stack is not encoded as an array; + * 2. Stack doesn't contain maps; + * 3. Stack's map keys are not uints; + * 4. Stack's map values have wrong types; + * In all these cases diag_last should contain emty err. + */ + /* Stack is encoded as map. */ + pos = mp_encode_map(buffer, 1); + pos = mp_encode_uint(pos, IPROTO_ERROR_STACK); + pos = mp_encode_map(pos, 1); + pos = error_stack_entry_encode(pos, "e1"); + header.body[0].iov_base = buffer; + header.body[0].iov_len = pos - buffer; + + diag_clear(diag_get()); + xrow_decode_error(&header); + last = diag_last_error(diag_get()); + isnt(last, NULL, "xrow_decode succeed: diag has been set"); + is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: " + "stack contains map instead of array"); + + /* Stack doesn't containt map(s) - array instead. */ + pos = mp_encode_map(buffer, 1); + pos = mp_encode_uint(pos, IPROTO_ERROR_STACK); + pos = mp_encode_array(pos, 2); + pos = mp_encode_uint(pos, IPROTO_ERROR_CODE); + pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE); + header.body[0].iov_base = buffer; + header.body[0].iov_len = pos - buffer; + + diag_clear(diag_get()); + xrow_decode_error(&header); + last = diag_last_error(diag_get()); + isnt(last, NULL, "xrow_decode succeed: diag has been set"); + is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: " + "stack contains array values instead of maps"); + + /* Stack's map keys are not uints. */ + pos = mp_encode_map(buffer, 1); + pos = mp_encode_uint(pos, IPROTO_ERROR_STACK); + pos = mp_encode_array(pos, 1); + pos = mp_encode_map(pos, 2); + pos = mp_encode_str(pos, "string instead of uint", + strlen("string instead of uint")); + pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE); + pos = mp_encode_uint(pos, IPROTO_ERROR_CODE); + pos = mp_encode_uint(pos, 159); + header.body[0].iov_base = buffer; + header.body[0].iov_len = pos - buffer; + + diag_clear(diag_get()); + xrow_decode_error(&header); + last = diag_last_error(diag_get()); + isnt(last, NULL, "xrow_decode succeed: diag has been set"); + is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: " + "stack's map keys are not uints"); + + /* Stack's map values have wrong types. */ + pos = mp_encode_map(buffer, 1); + pos = mp_encode_uint(pos, IPROTO_ERROR_STACK); + pos = mp_encode_array(pos, 1); + pos = mp_encode_map(pos, 2); + pos = mp_encode_uint(pos, IPROTO_ERROR_CODE); + pos = mp_encode_uint(pos, 159); + pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE); + pos = mp_encode_uint(pos, 666); + header.body[0].iov_base = buffer; + header.body[0].iov_len = pos - buffer; + + diag_clear(diag_get()); + xrow_decode_error(&header); + last = diag_last_error(diag_get()); + isnt(last, NULL, "xrow_decode succeed: diag has been set"); + is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: " + "stack's map wrong value type"); + check_plan(); +} + void test_request_str() { @@ -279,13 +413,14 @@ main(void) { memory_init(); fiber_init(fiber_c_invoke); - plan(3); + plan(4); random_init(); test_iproto_constants(); test_greeting(); test_xrow_header_encode_decode(); + test_xrow_error_stack_decode(); test_request_str(); random_free(); diff --git a/test/unit/xrow.result b/test/unit/xrow.result index 5ee92ad7b..1d07915a4 100644 --- a/test/unit/xrow.result +++ b/test/unit/xrow.result @@ -1,4 +1,4 @@ -1..3 +1..4 1..40 ok 1 - round trip ok 2 - roundtrip.version_id @@ -53,6 +53,22 @@ ok 1 - subtests ok 9 - decoded sync ok 10 - decoded bodycnt ok 2 - subtests + 1..14 + ok 1 - xrow_decode failed: no diag has been set + ok 2 - xrow_decode failed: wrong error is set + ok 3 - xrow_decode error stack failed: no diag has been set + ok 4 - xrow_decode error stack failed: e2 should come first + ok 5 - xrow_decode error stack failed: 'cause' is absent + ok 6 - xrow_decode error stack failed: wrong error is set + ok 7 - xrow_decode error stack failed: no diag has been set + ok 8 - xrow_decode corrupted stack: stack contains map instead of array + ok 9 - xrow_decode error stack failed: no diag has been set + ok 10 - xrow_decode corrupted stack: stack contains array values instead of maps + ok 11 - xrow_decode error stack failed: no diag has been set + ok 12 - xrow_decode corrupted stack: stack's map keys are not uints + ok 13 - xrow_decode error stack failed: no diag has been set + ok 14 - xrow_decode corrupted stack: stack's map wrong value type +ok 3 - subtests 1..1 ok 1 - request_str -ok 3 - subtests +ok 4 - subtests