From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Tue, 18 Jun 2019 16:58:06 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v3 2/6] box: rework box_lua_{call, eval} to use input port
Message-ID: <20190618135806.bg4e624m6aslr3hx@esperanza>
References: <cover.1560433806.git.kshcherbatov@tarantool.org>
 <dafa79859cc2d4c3320f2ecf0afcd027dcb9f9b6.1560433806.git.kshcherbatov@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <dafa79859cc2d4c3320f2ecf0afcd027dcb9f9b6.1560433806.git.kshcherbatov@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Thu, Jun 13, 2019 at 05:08:22PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/func.c b/src/box/func.c
> index d1b8879ad..740fad3d7 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -433,21 +436,39 @@ func_load(struct func *func)
>  }
>  
>  int
> -func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
> -	  const char *args_end)
> +func_call(struct func *func, struct port *in_port, struct port *out_port)
>  {
> +	assert(func != NULL && func->def->language == FUNC_LANGUAGE_C);
> +	/* Transaction is not started. */
> +	assert(!in_txn());
>  	if (func->func == NULL) {
>  		if (func_load(func) != 0)
>  			return -1;
>  	}
>  
> +	uint32_t args_sz;
> +	const char *args = port_get_msgpack(in_port, &args_sz);
> +	if (args == NULL)
> +		return -1;
> +
> +	port_tuple_create(out_port);
> +	box_function_ctx_t ctx = { out_port };
> +
>  	/* Module can be changed after function reload. */
>  	struct module *module = func->module;
>  	assert(module != NULL);
>  	++module->calls;
> -	int rc = func->func(ctx, args, args_end);
> +	int rc = func->func(&ctx, args, args + args_sz);
>  	--module->calls;
>  	module_gc(module);
> +	if (rc != 0) {
> +		if (diag_last_error(&fiber()->diag) == NULL) {
> +			/* Stored procedure forget to set diag  */
> +			diag_set(ClientError, ER_PROC_C, "unknown error");
> +		}
> +		port_destroy(out_port);
> +		return -1;
> +	}

Shouldn't you call region_truncate() here to clean up after the function
and possibly port_get_msgpack()?

>  	return rc;
>  }
>  
> diff --git a/src/lib/core/port.h b/src/lib/core/port.h
> index 8ace40fc5..2f1bc5ed1 100644
> --- a/src/lib/core/port.h
> +++ b/src/lib/core/port.h
> @@ -74,6 +75,8 @@ struct port_vtab {
>  	 * @retval not nil Plain text.
>  	 */
>  	const char *(*dump_plain)(struct port *port, uint32_t *size);
> +	/** Get the content of a port as a msgpack data. */
> +	const char *(*get_msgpack)(struct port *port, uint32_t *size);

Please mention what's the lifetime of the returned data. I guess you
should say that it's implementation dependent: it may either be returned
directly from the port, in which case the data will stay alive as long
as the port is alive, or it may be allocated on the fiber()->gc, in
which case the caller is responsible for cleaning up.