From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id DC8B52460F for ; Tue, 30 Jul 2019 16:42:58 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id esCHQRgrfMZC for ; Tue, 30 Jul 2019 16:42:58 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 46D49245A8 for ; Tue, 30 Jul 2019 16:42:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: rework error handling in box.execute() References: <3a3334974edb35becea5ca606d2943a230f097a4.1564495334.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Tue, 30 Jul 2019 22:45:05 +0200 MIME-Version: 1.0 In-Reply-To: <3a3334974edb35becea5ca606d2943a230f097a4.1564495334.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: imeevma@tarantool.org Cc: kostja@tarantool.org, tarantool-patches@freelists.org Hi! Thanks for the patch! On 30/07/2019 16:04, imeevma@tarantool.org wrote: > In accordance with the Lua coding style in Tarantool, all errors > returned in Lua should be returned using 'return nil, error'. > However, box.execute() throws an exception in case of an error. > This patch causes box.execute() to return an error, as described > in the coding style. > > Closes #4390 > --- > https://github.com/tarantool/tarantool/issues/4390 > https://github.com/tarantool/tarantool/tree/imeevma/gh-4390-box_execute-should-not-throw > > diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c > index 7b7c575..85c5738 100644 > --- a/src/box/lua/execute.c > +++ b/src/box/lua/execute.c > @@ -254,13 +254,25 @@ lbox_execute(struct lua_State *L) > if (! lua_istable(L, 2)) > return luaL_error(L, "Second argument must be a table"); > bind_count = lua_sql_bind_list_decode(L, &bind, 2); > - if (bind_count < 0) > - return luaT_error(L); > + if (bind_count < 0) { > + lua_pushnil(L); > + struct error *e = box_error_last(); > + if (e == NULL) > + return 1; > + luaT_pusherror(L, e); First, how is it possible, that e == NULL? It should not be so. I deleted that check, and the tests passed. So please, drop it. Secondly, there are already 4 places, which push exactly the same nil + diag error. And 13 more intricate places in lua/fio.c. I propose you to introduce a new function to push nil + last box error. Another place is lua_swim_new, which pushes nil implicitly, when in the expression: *(struct swim **) luaL_pushcdata(L, ctid_swim_ptr) = s; 's' is NULL. But in fact it is the same case, and here you can use than new function too. Please, introduce and use that new function in a separate commit before this one.