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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.

>     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!
>>
>> See 2 comments below.
>>
>>> diff --git a/src/box/error.cc b/src/box/error.cc
>>> index 8e77c2e9e..897aa9261 100644
>>> --- a/src/box/error.cc
>>> +++ b/src/box/error.cc
>>> @@ -102,6 +102,23 @@ box_error_new(const char *file, unsigned line, uint32_t code,
>>>  	return e;
>>>  }
>>>  
>>> +int
>>> +box_error_add(const char *file, unsigned line, uint32_t code,
>>> +	      const char *fmt, ...)
>>> +{
>>> +	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
>>> +	ClientError *client_error = type_cast(ClientError, e);
>>> +	if (client_error) {
>>> +		client_error->m_errcode = code;
>>> +		va_list ap;
>>> +		va_start(ap, fmt);
>>> +		error_vformat_msg(e, fmt, ap);
>>> +		va_end(ap);
>>> +	}
>>> +	diag_add_error(&fiber()->diag, e);
>>> +	return -1;
>>
>> 1. Why do we return -1 instead of the new error object?
>> box_error_new() returns an error, and this function does not.
>> Seems inconsistent.
> 
> box_error_new() is supposed to return new error according to
> its name (to be more precise - '_new' suffix). It allocates and
> fills in error struct. On the other hand, box_error_add() creates
> error and adds it to the diagnostic area. It is simply meaningless
> to return error, which is already set to diag.

2. Yeah, sorry. I mistakenly took box_error_new() for box_error_set().
You are right.

>> 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.

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.