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 662F125DF9 for ; Wed, 16 Jan 2019 13:25:46 -0500 (EST) 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 UfV-dWRlrB86 for ; Wed, 16 Jan 2019 13:25:46 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 200B0257A4 for ; Wed, 16 Jan 2019 13:25:46 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield() References: From: Vladislav Shpilevoy Message-ID: <09d7de8e-0565-50aa-8243-5bb7ca4ee623@tarantool.org> Date: Wed, 16 Jan 2019 21:25:35 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed 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: tarantool-patches@freelists.org, imeevma@tarantool.org Hi! Thanks for the patch! See 4 comments below. On 15/01/2019 19:10, imeevma@tarantool.org wrote: > Before this commit, the luaL_tofield() function threw a Lua > exception when an error occurred. This behavior has been changed > in this commit, now it sets diag_set() and returns -1 in case of > an error. > > Needed for #3505 > --- > src/box/lua/call.c | 9 +++-- > src/box/lua/tuple.c | 3 +- > src/lua/msgpack.c | 12 ++++-- > src/lua/utils.c | 105 ++++++++++++++++++++++++++++++---------------------- > src/lua/utils.h | 8 +++- > 5 files changed, 83 insertions(+), 54 deletions(-) > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > index 52939ae..39df05d 100644 > --- a/src/box/lua/call.c > +++ b/src/box/lua/call.c > @@ -164,7 +164,8 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg, > */ > for (int i = 1; i <= nrets; ++i) { > struct luaL_field field; > - luaL_tofield(L, cfg, i, &field); > + if (luaL_tofield(L, cfg, i, &field) < 0) > + luaT_error(L); 1. I know, it is weird, but usually we write return luaT_error/luaL_error(). Difference is in 'return' usage. Do not ask me why, I do not know. Maybe to emphasize that this statement finishes the function. So please, write 'return' where possible. Here and in other places. > struct tuple *tuple; > if (field.type == MP_EXT && > (tuple = luaT_istuple(L, i)) != NULL) { > diff --git a/src/lua/utils.c b/src/lua/utils.c > index 978fe61..9a9fd3a 100644 > --- a/src/lua/utils.c > +++ b/src/lua/utils.c > @@ -37,6 +37,8 @@ > #include > #include > 2. Unnecessary empty line. > +#include "box/error.h" > + > int luaL_nil_ref = LUA_REFNIL; > int luaL_map_metatable_ref = LUA_REFNIL; > int luaL_array_metatable_ref = LUA_REFNIL; > @@ -408,10 +411,11 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg, > lua_call(L, 1, 1); > /* replace obj with the unpacked value */ > lua_replace(L, idx); 3. In my opinion there is still a problem with lua_* methods, throwing exceptions on OOM *and on a simple stack overflow*. Even fixed GC will not fix the problem if all memory is occupied by non-garbage memory. I adhere to my opinion that it is better to use lua_cpcall. There is nothing bad with it. *_pcall just remembers a couple of registers, it is not too expensive for such a vast function. Also lua_cpcall causes much less diff. We can do it, for example, by renaming luaL_tofield to luaL_tofield_xc and introducing new luaL_tofield, calling luaL_tofield_xc via lua_cpcall. Just like we used to work with C++ *_xc wrappers. Please, ask again in the server team chat about the proposal above. Do not forget about stack overflow error, which also is possible and does not mean panic. Moreover, it is worth noting that diff is going to be much less and simpler. If they decline the proposal, I give in. > - luaL_tofield(L, cfg, idx, field); > - return; > + return luaL_tofield(L, cfg, idx, field); > } else if (!lua_isstring(L, -1)) { > - luaL_error(L, "invalid " LUAL_SERIALIZE " value"); > + diag_set(ClientError, ER_PROC_LUA, > + "invalid " LUAL_SERIALIZE " value"); > + return -1; > } > > type = lua_tostring(L, -1); > @@ -525,94 +539,96 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index, > field->dval = num; > CHECK_NUMBER(num); > } > - return; > + break; 4. Why no to return 0? Anyway all your breaks lead to a simple 'return 0'. > @@ -620,14 +636,15 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index, > field->sval.len = 0; > if (lua_touserdata(L, index) == NULL) { > field->type = MP_NIL; > - return; > + break; > } > /* Fall through */ > default: > field->type = MP_EXT; > - return; > + break; > } > #undef CHECK_NUMBER > + return 0; > } > > void