Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area
Date: Wed, 1 Apr 2020 22:24:56 +0000	[thread overview]
Message-ID: <20200401222456.GA27659@tarantool.org> (raw)
In-Reply-To: <20200401162618.GD26447@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
 

  reply	other threads:[~2020-04-01 22:24 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25  1:42 [Tarantool-patches] [PATCH v2 00/10] Stacked diagnostics Nikita Pettik
2020-03-25  1:42 ` [Tarantool-patches] [PATCH v2 01/10] box: rfc for stacked diagnostic area Nikita Pettik
2020-03-25  8:27   ` Konstantin Osipov
2020-03-25 14:08     ` Nikita Pettik
2020-03-25  1:42 ` [Tarantool-patches] [PATCH v2 02/10] box: rename diag_add_error to diag_set_error Nikita Pettik
2020-03-25  8:27   ` Konstantin Osipov
2020-03-26  0:22   ` Vladislav Shpilevoy
2020-03-26 12:31     ` Nikita Pettik
2020-03-25  1:42 ` [Tarantool-patches] [PATCH v2 03/10] test: move box.error tests to box/error.test.lua Nikita Pettik
2020-03-25  8:28   ` Konstantin Osipov
2020-03-26  0:22   ` Vladislav Shpilevoy
2020-03-26 12:31     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 04/10] box/error: introduce box.error.set() method Nikita Pettik
2020-03-25  8:33   ` Konstantin Osipov
2020-03-25 17:41     ` Nikita Pettik
2020-03-26  0:22   ` Vladislav Shpilevoy
2020-03-26 12:31     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 05/10] box/error: don't set error created via box.error.new to diag Nikita Pettik
2020-03-26 16:50   ` Konstantin Osipov
2020-03-26 17:59     ` Nikita Pettik
2020-03-26 18:06       ` Nikita Pettik
2020-03-26 18:07       ` Alexander Turenko
2020-03-27  0:19   ` Vladislav Shpilevoy
2020-03-27 13:09     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area Nikita Pettik
2020-03-26 16:54   ` Konstantin Osipov
2020-03-26 18:03     ` Nikita Pettik
2020-03-26 18:08       ` Konstantin Osipov
2020-03-28 18:40   ` Vladislav Shpilevoy
2020-04-01 16:09     ` Nikita Pettik
2020-04-02  0:29       ` Vladislav Shpilevoy
2020-04-02 17:42         ` Nikita Pettik
2020-04-02 22:20           ` Vladislav Shpilevoy
2020-04-03  1:54             ` Nikita Pettik
2020-04-03 23:17               ` Vladislav Shpilevoy
2020-03-28 18:59   ` Vladislav Shpilevoy
2020-03-31 17:44     ` Nikita Pettik
2020-04-02  0:29       ` Vladislav Shpilevoy
2020-04-02 14:16         ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 07/10] box: use stacked diagnostic area for functional indexes Nikita Pettik
2020-03-30 23:24   ` Vladislav Shpilevoy
2020-04-01 15:53     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 08/10] box/error: clarify purpose of reference counting in struct error Nikita Pettik
2020-03-30 23:24   ` Vladislav Shpilevoy
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 09/10] iproto: refactor error encoding with mpstream Nikita Pettik
2020-03-30 23:24   ` Vladislav Shpilevoy
2020-04-01 15:54     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area Nikita Pettik
2020-03-30 23:24   ` Vladislav Shpilevoy
2020-04-01 16:26     ` Nikita Pettik
2020-04-01 22:24       ` Nikita Pettik [this message]
2020-04-02  0:29         ` Vladislav Shpilevoy
2020-04-02 14:01           ` Nikita Pettik
2020-04-02 22:20             ` Vladislav Shpilevoy
2020-04-03  2:16               ` Nikita Pettik
2020-04-03 23:17                 ` Vladislav Shpilevoy
2020-04-06 11:07                   ` Nikita Pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200401222456.GA27659@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox