[tarantool-patches] Re: [PATCH v2 09/12] box: introduce Lua persistent functions

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov at tarantool.org> [19/07/10 14:02]:
> diff --git a/src/box/func.c b/src/box/func.c
> index 8227527ec..8d93a83b2 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -34,7 +34,9 @@
>  #include "assoc.h"
>  #include "lua/utils.h"
>  #include "lua/call.h"
> +#include "lua/lua_sql.h"
>  #include "error.h"
> +#include "sql.h"

Why do you need these includes? Please watch for unnecessary
includes when you do a self-review.

>  #include "diag.h"
>  #include "port.h"
>  #include "schema.h"
> @@ -385,11 +387,18 @@ struct func *
>  func_new(struct func_def *def)
>  {
>  	struct func *func;
> -	if (def->language == FUNC_LANGUAGE_C) {
> +	switch (def->language) {
> +	case FUNC_LANGUAGE_C:
>  		func = func_c_new(def);
> -	} else {
> -		assert(def->language == FUNC_LANGUAGE_LUA);
> +		break;
> +	case FUNC_LANGUAGE_LUA:
>  		func = func_lua_new(def);
> +		break;
> +	case FUNC_LANGUAGE_SQL_BUILTIN:
> +		func = func_sql_builtin_new(def);
> +		break;
> +	default:
> +		unreachable();
>  	}
>  	if (func == NULL)
>  		return NULL;
> @@ -416,8 +425,13 @@ static struct func_vtab func_c_vtab;
>  static struct func *
>  func_c_new(struct func_def *def)
>  {
> -	(void) def;
>  	assert(def->language == FUNC_LANGUAGE_C);
> +	if (def->body != NULL || def->is_sandboxed) {
> +		diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
> +			 "body and is_sandboxed options are not compatible "
> +			 "with C language");
> +		return NULL;
> +	}
>  	struct func_c *func = (struct func_c *) malloc(sizeof(struct func_c));
>  	if (func == NULL) {
>  		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
> diff --git a/src/box/func_def.c b/src/box/func_def.c
> index 2b135e2d7..fb9f77df8 100644
> --- a/src/box/func_def.c
> +++ b/src/box/func_def.c
> +static int
> +func_persistent_lua_load(struct func_lua *func)
> +{
> +	int rc = -1;
> +	int top = lua_gettop(tarantool_L);
> +	struct region *region = &fiber()->gc;
> +	size_t region_svp = region_used(region);
> +	const char *load_pref = "return ";

load_prefix, pref is ambiguous

The patch is LGTM. I would take another couple of hours to review
it carefully and tidy up - including the comments. 
Unfortunately I don't have them the moment. I hope Vova can take
the patch from here.

-- 
Konstantin Osipov, Moscow, Russia