From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 C6E7043E89B for ; Fri, 27 Mar 2020 16:09:37 +0300 (MSK) Date: Fri, 27 Mar 2020 13:09:36 +0000 From: Nikita Pettik Message-ID: <20200327130936.GA9287@tarantool.org> References: <1816245c768216733779be5c7420cc60725287ea.1585097339.git.korablev@tarantool.org> <536126ef-fef2-9f01-2d34-e0d1440d8eee@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <536126ef-fef2-9f01-2d34-e0d1440d8eee@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 05/10] 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 27 Mar 01:19, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 2 comments below. > > On 25/03/2020 02:43, 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_create() is used both to handle box.error() and > > 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. > > > > Needed for #1148 > > Closes #4778 > > > > @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 > > ... > > ``` > > 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 > > 1. 'pushed' -> 'pushes'. > > > diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc > > index 640e33910..ff285d7eb 100644 > > --- a/src/box/lua/error.cc > > +++ b/src/box/lua/error.cc > > @@ -139,9 +144,12 @@ luaT_error_new(lua_State *L) > > { > > if (lua_gettop(L) == 0) > > return luaL_error(L, "Usage: box.error.new(code, args)"); > > - luaT_error_create(L, 1); > > + struct error *e = luaT_error_create(L, 1); > > + if (e == NULL) > > + return luaL_error(L, "box.error.new(): bad arguments"); > > 2. I think it would be better to return the same 'Usage' error > in case of a problem with arguments. Not two different messages > meaning essentially the same. Or both should return 'bad arguments'. > However with both 'Usage' the diff would be 1 line smaller. > > Other than these comments, LGTM. If you fix them both - then push right > away. Pushed to master, updated changelog.