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 1044025918 for ; Thu, 17 Jan 2019 06:41:10 -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 MYZM2byVQqF3 for ; Thu, 17 Jan 2019 06:41:09 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 BD08822027 for ; Thu, 17 Jan 2019 06:41:09 -0500 (EST) Date: Thu, 17 Jan 2019 14:41:07 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield() Message-ID: <20190117114107.GL28204@chai> References: <09d7de8e-0565-50aa-8243-5bb7ca4ee623@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <09d7de8e-0565-50aa-8243-5bb7ca4ee623@tarantool.org> 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 Cc: imeevma@tarantool.org * Vladislav Shpilevoy [19/01/16 21:29]: I don't mind if we have a cpcall even in scope of this patch - but let's test these errors, not just rewrite the code and hope for the best. Can we have any kind of test which runs in short time for lua errors? > 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 -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov