Re: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area

[Nikita Pettik <korablev@tarantool.org>]

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.