[Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area
Nikita Pettik
korablev at tarantool.org
Mon Apr 6 14:07:51 MSK 2020
On 04 Apr 01:17, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
>
> See 3 comments below.
>
> > iproto: support error stacked diagnostic area
> >
> > This patch introduces support of stacked errors in IProto protocol and
> > in net.box module.
> >
> > Closes #1148
> > ```
> > MAP{IPROTO_ERROR : string, IPROTO_ERROR_STACK : ARRAY[MAP{ERROR_CODE : uint, ERROR_MESSAGE : string}, MAP{...}, ...]}
> > ```
> > where IPROTO_ERROR is 0x31 key, IPROTO_ERROR_STACK is 0x51, ERROR_CODE
>
> 1. IPROTO_ERROR_STACK is actually 0x52.
Fixed (artifact after rebase on master).
> > is 0x01 and ERROR_MESSAGE is 0x02.
> On 03/04/2020 04:16, Nikita Pettik wrote:
> > On 03 Apr 00:20, Vladislav Shpilevoy wrote:
> >> Thanks for the fixes!
> >>
> >> diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
> >> index ae45f18b0..718ef15a2 100644
> >> --- a/test/unit/xrow.cc
> >> +++ b/test/unit/xrow.cc
> >> @@ -392,9 +393,51 @@ test_xrow_error_stack_decode()
> >> + /* Overflow error code. */
> >> + 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, (uint64_t)1 << 40);
> >> + pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
> >> + pos = mp_encode_str(pos, "test msg", strlen("test msg"));
> >> + 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(box_error_code(last), 159, "xrow_decode failed, took code from "
> >> + "header");
> >> + is(strcmp(last->errmsg, ""), 0, "xrow_decode failed, message is not "
> >> + "decoded");
> >> +
> >> + /* Overflow error key. */
> >> + 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, 0xff00000000 | IPROTO_ERROR_CODE);
> >> + pos = mp_encode_uint(pos, 159);
> >> + pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
> >> + pos = mp_encode_str(pos, "test msg", strlen("test msg"));
> >> + 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(box_error_code(last), 0, "xrow_decode last error code is default 0");
> >> + is(strcmp(last->errmsg, "test msg"), 0, "xrow_decode message is fine "
> >> + "even without the code");
> >
> > It is likely to be the same as "Bad key in the packet.". At least it
> > seems to cover the same execution path.
>
> 3. You can merge them if you want. But then you need to use
> 0xff00000000, not 0xff000000. To ensure, that it is truncated
> to 0 if cast to uint32.
Merged:
diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
index 7b138f1bf..9b76350af 100644
--- a/test/unit/xrow.cc
+++ b/test/unit/xrow.cc
@@ -256,7 +256,7 @@ error_stack_entry_encode(char *pos, const char *err_str)
void
test_xrow_error_stack_decode()
{
- plan(24);
+ plan(21);
char buffer[2048];
/*
* To start with, let's test the simplest and obsolete
@@ -306,7 +306,6 @@ test_xrow_error_stack_decode()
* 4. Stack's map values have wrong types;
* 5. Stack's map key is broken (doesn't fit into u8);
* 6. Stack's map contains overflowed (> 2^32) error code;
- * 7. Stack's map contains overflowed (> 2^32) key.
* In all these cases diag_last should contain empty err.
*/
/* Stack is encoded as map. */
@@ -384,7 +383,7 @@ test_xrow_error_stack_decode()
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, 0xff000000 | IPROTO_ERROR_CODE);
+ pos = mp_encode_uint(pos, 0xff00000000 | IPROTO_ERROR_CODE);
pos = mp_encode_uint(pos, 159);
pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
pos = mp_encode_str(pos, "test msg", strlen("test msg"));
@@ -420,26 +419,6 @@ test_xrow_error_stack_decode()
is(strcmp(last->errmsg, ""), 0, "xrow_decode failed, message is not "
"decoded");
- /* Overflow error key. */
- 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, 0xff00000000 | IPROTO_ERROR_CODE);
- pos = mp_encode_uint(pos, 159);
- pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
- pos = mp_encode_str(pos, "test msg", strlen("test msg"));
- 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(box_error_code(last), 0, "xrow_decode last error code is default 0");
- is(strcmp(last->errmsg, "test msg"), 0, "xrow_decode message is fine "
- "even without the code");
-
check_plan();
}
> The patchset LGTM, including the box.error() path to set diag
> with a struct error object. Except the wrong IPROTO_ERROR_STACK
> code in the commit message. Please, fix it, and send the patchset
> to a second reviewer, keeping me in copy just in case.
More information about the Tarantool-patches
mailing list