Re: [PATCH v2 1/9] box: refactor box_lua_find helper

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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.