From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 6642E4696C3 for ; Mon, 6 Apr 2020 14:07:53 +0300 (MSK) Date: Mon, 6 Apr 2020 11:07:51 +0000 From: Nikita Pettik Message-ID: <20200406110750.GB20849@tarantool.org> References: <3bb7c695b34356137897b953281543c46fe3bafa.1585097339.git.korablev@tarantool.org> <20200401162618.GD26447@tarantool.org> <20200401222456.GA27659@tarantool.org> <64be3c70-4ec9-0743-0b6d-e7453239db4a@tarantool.org> <20200402140100.GA30923@tarantool.org> <20200403021647.GD946@tarantool.org> <2cf09865-aec8-8c35-489a-25b05734fbaa@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2cf09865-aec8-8c35-489a-25b05734fbaa@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 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.