From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 6A2B64429A8 for ; Thu, 17 Oct 2019 17:09:06 +0300 (MSK) Date: Thu, 17 Oct 2019 17:09:05 +0300 From: Sergey Ostanevich Message-ID: <20191017140905.GB115@tarantool.org> References: <20191017104912.31390-1-i.kosarev@tarantool.org> <20191017124050.GA22173@tarantool.org> <20191017125739.GB22173@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191017125739.GB22173@tarantool.org> Subject: Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2] refactoring: wrap lua_newthread using luaT_cpcall List-Id: Tarantool development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@freelists.org Cc: tarantool-patches@dev.tarantool.org Hi! Please, create a GH for follow-ups: - Rename lua_ and luaL_ interfaces in tarantool code base (utils.h, etc..) so that we will not interfere with luajit code base I also agree with Igor that there's no need in luaT_pushthread() since it has only one use and provides no more functionality than 2 lines of code. With above fixed this patch is LGTM. Regards, Sergos On 17 Oct 15:57, Igor Munkin wrote: > CCed Sergos for proceeding with the review. > > On 17.10.19, Igor Munkin wrote: > > Ilya, > > > > Thanks, the patch LGTM, however please consider my minor comments. > > > > On 17.10.19, Ilya Kosarev wrote: > > > Wrap throwing lua_newthread in luaT_newthread using luaT_cpcall > > > to process arising error properly. > > > > > > Closes #4556 > > > --- > > > https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4556-wrap-lua-newthread > > > https://github.com/tarantool/tarantool/issues/4556 > > > > > > Changes in v2: > > > - introduced luaT_newthread instead of repeated slocs > > > - renamed luaL_pushthread to luaT_pushthread due to namespaces agreement > > > > > > src/box/lua/call.c | 12 +++++++--- > > > src/lua/fiber.c | 4 +++- > > > src/lua/trigger.c | 12 ++++------ > > > src/lua/utils.h | 41 +++++++++++++++++++++++++++++++++++ > > > third_party/lua-yaml/lyaml.cc | 5 ++++- > > > 5 files changed, 61 insertions(+), 13 deletions(-) > > > > > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > > > index 631003c84..51cad286a 100644 > > > --- a/src/box/lua/call.c > > > +++ b/src/box/lua/call.c > > > @@ -527,7 +527,9 @@ static inline int > > > box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx, > > > struct port *ret) > > > { > > > - lua_State *L = lua_newthread(tarantool_L); > > > + lua_State *L = luaT_newthread(tarantool_L); > > > + if (L == NULL) > > > + return -1; > > > int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX); > > > port_lua_create(ret, L); > > > ((struct port_lua *) ret)->ref = coro_ref; > > > @@ -654,7 +656,9 @@ func_persistent_lua_load(struct func_lua *func) > > > * an arbitrary user-defined code > > > * (e.g. body = 'fiber.yield()'). > > > */ > > > - struct lua_State *coro_L = lua_newthread(tarantool_L); > > > + lua_State *coro_L = luaT_newthread(tarantool_L); > > > + if (coro_L == NULL) > > > + return -1; > > > if (!func->base.def->is_sandboxed) { > > > /* > > > * Keep the original env to apply to a non-sandboxed > > > @@ -811,7 +815,9 @@ lbox_func_call(struct lua_State *L) > > > * before the function call to pass it into the > > > * pcall-sandboxed tarantool_L handler. > > > */ > > > - lua_State *args_L = lua_newthread(tarantool_L); > > > + lua_State *args_L = luaT_newthread(tarantool_L); > > > + if (args_L == NULL) > > > + return luaT_error(L); > > > int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX); > > > lua_xmove(L, args_L, lua_gettop(L) - 1); > > > struct port args; > > > diff --git a/src/lua/fiber.c b/src/lua/fiber.c > > > index 336be60a2..124908a05 100644 > > > --- a/src/lua/fiber.c > > > +++ b/src/lua/fiber.c > > > @@ -388,7 +388,9 @@ lua_fiber_run_f(MAYBE_UNUSED va_list ap) > > > static struct fiber * > > > fiber_create(struct lua_State *L) > > > { > > > - struct lua_State *child_L = lua_newthread(L); > > > + lua_State *child_L = luaT_newthread(L); > > > + if (child_L == NULL) > > > + luaT_error(L); > > > int coro_ref = luaL_ref(L, LUA_REGISTRYINDEX); > > > > > > struct fiber *f = fiber_new("lua", lua_fiber_run_f); > > > diff --git a/src/lua/trigger.c b/src/lua/trigger.c > > > index 4803e85c5..6df048a8d 100644 > > > --- a/src/lua/trigger.c > > > +++ b/src/lua/trigger.c > > > @@ -73,17 +73,13 @@ lbox_trigger_run(struct trigger *ptr, void *event) > > > * trigger yields, so when it's time to clean > > > * up the coro, we wouldn't know which stack position > > > * it is on. > > > - * > > > - * XXX: lua_newthread() may throw if out of memory, > > > - * this needs to be wrapped with lua_pcall() as well. > > > - * Don't, since it's a stupid overhead on every trigger > > > - * invocation, and in future we plan to hack into Lua > > > - * C API to fix this. > > > */ > > > - struct lua_State *L; > > > + lua_State *L; > > > int coro_ref; > > > if (fiber()->storage.lua.stack == NULL) { > > > - L = lua_newthread(tarantool_L); > > > + L = luaT_newthread(tarantool_L); > > > + if (L == NULL) > > > + diag_raise(); > > > coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX); > > > } else { > > > L = fiber()->storage.lua.stack; > > > diff --git a/src/lua/utils.h b/src/lua/utils.h > > > index f8c34545a..f82b30fbc 100644 > > > --- a/src/lua/utils.h > > > +++ b/src/lua/utils.h > > > @@ -591,6 +591,47 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg, > > > luaL_error(L, "number must not be NaN or Inf"); > > > } > > > > > > > Note: the comment about lua_* namespace also relates to the wrapper > > below. I guess we need some follow-up activity for it. > > > > > +/** > > > + * @brief A wrapper for lua_newthread() to pass it into luaT_cpcall > > > + * @param L is a Lua State > > > + * @sa lua_newthread() > > > + */ > > > +static inline int > > > +lua_newthread_wrapper(lua_State *L) > > > +{ > > > + *(lua_State **)lua_touserdata(L, 1) = lua_newthread(L); > > > + return 0; > > > +} > > > + > > > > Minor: I see no reason to create a separate function for pushing one > > lua_State onto the anothers' stack. Feel free to ignore or to discuss it > > with the next reviewer. > > > > > +/** > > > + * @brief Push L1 thread onto the L stack > > > + * @param L is a Lua State whose stack we are pushing onto > > > + * @param L1 is a Lua State whose thread we are pushing > > > + */ > > > +static inline void > > > +luaT_pushthread(lua_State *L, lua_State *L1) > > > +{ > > > + setthreadV(L, L->top, L1); > > > + incr_top(L); > > > +} > > > + > > > +/** > > > + * @brief Safe wrapper for lua_newthread() > > > + * @param L is a Lua State > > > + * @sa lua_newthread() > > > + */ > > > +static inline lua_State * > > > +luaT_newthread(lua_State *L) > > > +{ > > > + lua_State *L1 = NULL; > > > + if (luaT_cpcall(L, lua_newthread_wrapper, &L1) != 0) { > > > + return NULL; > > > + } > > > + assert(L1 != NULL); > > > + luaT_pushthread(L, L1); > > > + return L1; > > > +} > > > + > > > /** > > > * Check if a value on @a L stack by index @a idx is an ibuf > > > * object. Both 'struct ibuf' and 'struct ibuf *' are accepted. > > > diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc > > > index 7485341fa..af4f2f5d5 100644 > > > --- a/third_party/lua-yaml/lyaml.cc > > > +++ b/third_party/lua-yaml/lyaml.cc > > > @@ -789,7 +789,10 @@ lua_yaml_encode(lua_State *L, struct luaL_serializer *serializer, > > > dumper.cfg = serializer; > > > dumper.error = 0; > > > /* create thread to use for YAML buffer */ > > > - dumper.outputL = lua_newthread(L); > > > + dumper.outputL = luaT_newthread(L); > > > + if (dumper.outputL == NULL) { > > > + return luaL_error(L, OOM_ERRMSG); > > > + } > > > luaL_buffinit(dumper.outputL, &dumper.yamlbuf); > > > > > > if (!yaml_emitter_initialize(&dumper.emitter)) > > > -- > > > 2.17.1 > > > > > > > -- > > Best regards, > > IM > > -- > Best regards, > IM >