From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 10 Jun 2019 12:17:37 +0300 From: Vladimir Davydov Subject: Re: [PATCH v2 1/9] box: refactor box_lua_find helper Message-ID: <20190610091737.mz3jhhlb5kivww3r@esperanza> References: <9f896499624289da849eebccd076f3029353ae8d.1559822429.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9f896499624289da849eebccd076f3029353ae8d.1559822429.git.kshcherbatov@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Thu, Jun 06, 2019 at 03:03:57PM +0300, Kirill Shcherbatov wrote: > The box_lua_find routine used to work with an empty stack only. > It is unacceptable in following patches because this helper > need to be reused in following patches for environment table > construction. > The definition also is moved to lua/utils.h. > > Needed for #4182, #1260 > --- > src/box/lua/call.c | 85 ++++++++-------------------------------------- > src/lua/utils.c | 59 ++++++++++++++++++++++++++++++++ > src/lua/utils.h | 11 ++++++ > 3 files changed, 85 insertions(+), 70 deletions(-) > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > index 04020ef6f..c729778c4 100644 > --- a/src/box/lua/call.c > +++ b/src/box/lua/call.c > @@ -44,73 +44,6 @@ > #include "trivia/util.h" > #include "mpstream.h" > > -/** > - * A helper to find a Lua function by name and put it > - * on top of the stack. > - */ > -static int > -box_lua_find(lua_State *L, const char *name, const char *name_end) > -{ > - int index = LUA_GLOBALSINDEX; > - int objstack = 0; > - const char *start = name, *end; > - > - while ((end = (const char *) memchr(start, '.', name_end - start))) { > - lua_checkstack(L, 3); > - lua_pushlstring(L, start, end - start); > - lua_gettable(L, index); > - if (! lua_istable(L, -1)) { > - diag_set(ClientError, ER_NO_SUCH_PROC, > - name_end - name, name); > - luaT_error(L); > - } > - start = end + 1; /* next piece of a.b.c */ > - index = lua_gettop(L); /* top of the stack */ > - } > - > - /* box.something:method */ > - if ((end = (const char *) memchr(start, ':', name_end - start))) { > - lua_checkstack(L, 3); > - lua_pushlstring(L, start, end - start); > - lua_gettable(L, index); > - if (! (lua_istable(L, -1) || > - lua_islightuserdata(L, -1) || lua_isuserdata(L, -1) )) { > - diag_set(ClientError, ER_NO_SUCH_PROC, > - name_end - name, name); > - luaT_error(L); > - } > - start = end + 1; /* next piece of a.b.c */ > - index = lua_gettop(L); /* top of the stack */ > - objstack = index; > - } > - > - > - lua_pushlstring(L, start, name_end - start); > - lua_gettable(L, index); > - if (!lua_isfunction(L, -1) && !lua_istable(L, -1)) { > - /* lua_call or lua_gettable would raise a type error > - * for us, but our own message is more verbose. */ > - diag_set(ClientError, ER_NO_SUCH_PROC, > - name_end - name, name); > - luaT_error(L); > - } > - /* setting stack that it would contain only > - * the function pointer. */ > - if (index != LUA_GLOBALSINDEX) { > - if (objstack == 0) { /* no object, only a function */ > - lua_replace(L, 1); > - } else if (objstack == 1) { /* just two values, swap them */ > - lua_insert(L, -2); > - } else { /* long path */ > - lua_insert(L, 1); > - lua_insert(L, 2); > - objstack = 1; > - } > - lua_settop(L, 1 + objstack); > - } > - return 1 + objstack; > -} > - > /** > * A helper to find lua stored procedures for box.call. > * box.call iteslf is pure Lua, to avoid issues > @@ -124,7 +57,12 @@ lbox_call_loadproc(struct lua_State *L) > const char *name; > size_t name_len; > name = lua_tolstring(L, 1, &name_len); > - return box_lua_find(L, name, name + name_len); > + int count; > + if (luaT_func_find(L, name, name + name_len, &count) != 0) { > + diag_set(ClientError, ER_NO_SUCH_PROC, name_len, name); > + return luaT_error(L); > + } > + return count; > } > > /* > @@ -292,9 +230,16 @@ execute_lua_call(lua_State *L) > const char *name = request->name; > uint32_t name_len = mp_decode_strl(&name); > > - int oc = 0; /* how many objects are on stack after box_lua_find */ > + /* > + * How many objects are on stack after > + * luaT_func_find call. > + */ > + int oc = 0; > /* Try to find a function by name in Lua */ > - oc = box_lua_find(L, name, name + name_len); > + if (luaT_func_find(L, name, name + name_len, &oc) != 0) { > + diag_set(ClientError, ER_NO_SUCH_PROC, name_len, name); > + return luaT_error(L); > + } > > /* Push the rest of args (a tuple). */ > const char *args = request->args; > diff --git a/src/lua/utils.c b/src/lua/utils.c > index 01a0cd894..27ff6b396 100644 > --- a/src/lua/utils.c > +++ b/src/lua/utils.c > @@ -1189,6 +1189,65 @@ void luaL_iterator_delete(struct luaL_iterator *it) > > /* }}} */ > > +int > +luaT_func_find(struct lua_State *L, const char *name, const char *name_end, > + int *count) > +{ > + int index = LUA_GLOBALSINDEX; > + int objstack = 0, top = lua_gettop(L); > + const char *start = name, *end; > + > + while ((end = (const char *) memchr(start, '.', name_end - start))) { > + lua_checkstack(L, 3); > + lua_pushlstring(L, start, end - start); > + lua_gettable(L, index); > + if (! lua_istable(L, -1)) > + return -1; > + start = end + 1; /* next piece of a.b.c */ > + index = lua_gettop(L); /* top of the stack */ > + } > + > + /* box.something:method */ > + if ((end = (const char *) memchr(start, ':', name_end - start))) { > + lua_checkstack(L, 3); > + lua_pushlstring(L, start, end - start); > + lua_gettable(L, index); > + if (! (lua_istable(L, -1) || > + lua_islightuserdata(L, -1) || lua_isuserdata(L, -1) )) > + return -1; > + start = end + 1; /* next piece of a.b.c */ > + index = lua_gettop(L); /* top of the stack */ > + objstack = index - top; > + } > + > + lua_pushlstring(L, start, name_end - start); > + lua_gettable(L, index); > + if (!lua_isfunction(L, -1) && !lua_istable(L, -1)) { > + /* lua_call or lua_gettable would raise a type error > + * for us, but our own message is more verbose. */ > + return -1; > + } > + > + /* setting stack that it would contain only > + * the function pointer. */ > + if (index != LUA_GLOBALSINDEX) { > + if (objstack == 0) { /* no object, only a function */ > + lua_replace(L, top + 1); > + lua_pop(L, lua_gettop(L) - top - 1); > + } else if (objstack == 1) { /* just two values, swap them */ > + lua_insert(L, -2); > + lua_pop(L, lua_gettop(L) - top - 2); > + } else { /* long path */ > + lua_insert(L, top + 1); > + lua_insert(L, top + 2); > + lua_pop(L, objstack - 1); > + objstack = 1; > + } > + } > + *count = 1 + objstack; > + return 0; > +} Moving code and patching it at the same time is bad, as it's very easy to overlook a mistake. Please try to avoid that. Anyway, I don't think we need to move this function anywhere - IMO we should define func_lua in src/box/lua/call.c instead. Also, patching this function is really trivial - since it's only needed for sandboxing I'd fold this change in the patch introducing persistent functions.