From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 EB195469719 for ; Sat, 22 Feb 2020 20:18:14 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Sat, 22 Feb 2020 18:18:12 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org Thanks for the patch! See 6 comments below. On 19/02/2020 15:16, Nikita Pettik wrote: > To achieve this let's refactor luaT_error_create() to return error > object instead of setting it via box_error_set(). > luaT_error_created() is used both to handle box.error() and 1. luaT_error_created() -> luaT_error_create(). > box.error.new() invocations, and box.error() is still expected to set > error to diagnostic area. So, luaT_error_call() which implements > box.error() processing at the end calls diag_set_error(). > It is worth mentioning that net.box module relied on the fact that > box.error.new() set error to diagnostic area: otherwise request errors > don't get to diagnostic area on client side. > > @TarantoolBot document > Title: Don't promote error created via box.error.new to diagnostic area > > Now box.error.new() only creates error object, but doesn't set it to > Tarantool's diagnostic area: > box.error.clear() > e = box.error.new({code = 111, reason = "cause"}) > assert(box.error.last() == nil) > --- > - true > ... 2. It is better to wrap the code into ```, because otherwise it will look like tickets created by Peter, with weird formatting, huge headers, etc. > To set error in diagnostic area explicitly box.error.set() has been > introduced. It accepts error object which is set as last system error > (i.e. becomes available via box.error.last()). > Finally, box.error.new() does not longer accept error object as an > argument (this was undocumented feature). > Note that patch does not affect box.error(), which still pushed error to > diagnostic area. This fact is reflected in docs: > ''' > Emulate a request error, with text based on one of the pre-defined > Tarantool errors... > ''' > > Needed for #1148 > Closes #4778 3. This should be above docbot request. Otherwise it will appear in the doc ticket. > --- > src/box/error.cc | 16 +++++++++++++++ > src/box/error.h | 8 ++++++++ > src/box/lua/error.cc | 56 ++++++++++++++++++++++++++++---------------------- > test/box/misc.result | 30 ++++++++++++++++++++++++++- > test/box/misc.test.lua | 16 +++++++++++++++ > 5 files changed, 101 insertions(+), 25 deletions(-) > > diff --git a/src/box/error.cc b/src/box/error.cc > index 7dfe1b3ee..824a4617c 100644 > --- a/src/box/error.cc > +++ b/src/box/error.cc > @@ -86,6 +86,22 @@ box_error_set(const char *file, unsigned line, uint32_t code, > return -1; > } > > +struct error * > +box_error_construct(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 != NULL) { > + client_error->m_errcode = code; 4. You can now make box_error_set() call box_error_construct() + diag_set(), to avoid code duplication. > + va_list ap; > + va_start(ap, fmt); > + error_vformat_msg(e, fmt, ap); > + va_end(ap); > + } > + return e; > +} > + > /* }}} */ > > struct rmean *rmean_error = NULL; > diff --git a/src/box/error.h b/src/box/error.h > index b8c7cf73d..42043ef80 100644 > --- a/src/box/error.h > +++ b/src/box/error.h > @@ -129,6 +129,14 @@ int > box_error_set(const char *file, unsigned line, uint32_t code, > const char *format, ...); > > +/** > + * Construct error object without setting it in the diagnostics > + * area. On the memory allocation fail returns NULL. > + */ > +struct error * > +box_error_construct(const char *file, unsigned line, uint32_t code, > + const char *fmt, ...); 5. You added the method to the public C API (it is inside 'cond public' comments). I don't think we should do that. At least until someone asked so. Also why not box_error_new()? 'Construct' seems to be a very strange name. > + > /** > * A backward-compatible API define. > */ > diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc > index 7311cb2ce..17d4de653 100644 > --- a/src/box/lua/error.cc > +++ b/src/box/lua/error.cc > @@ -42,7 +42,14 @@ extern "C" { > #include "lua/utils.h" > #include "box/error.h" > > -static void > +/** > + * Parse Lua arguments (they can come as single table > + * f({code : number, reason : string}) or as separate members > + * f(code, reason)) and construct struct error with given values. > + * In case one of arguments is missing it corresponding field 6. it -> its. > + * in struct error is filled with default value. > + */ > +static struct error * > luaT_error_create(lua_State *L, int top_base) > { > uint32_t code = 0;