From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Subject: Re: [tarantool-patches] [PATCH v2 2/3] lua: internal function to
 parse space format
References: <cover.1561130584.git.imeevma@gmail.com>
 <bb85acfd88ba9a39c4431b82d2e25f7978889062.1561130584.git.imeevma@gmail.com>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <5a5beb84-0bee-2a9c-41f7-b4b275c0fd42@tarantool.org>
Date: Fri, 21 Jun 2019 22:39:40 +0200
MIME-Version: 1.0
In-Reply-To: <bb85acfd88ba9a39c4431b82d2e25f7978889062.1561130584.git.imeevma@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
To: tarantool-patches@freelists.org, imeevma@tarantool.org
Cc: vdavydov.dev@gmail.com
List-ID: <tarantool-patches.dev.tarantool.org>

Thanks for the patch!

> +
> +static int
> +lbox_tuple_format_new(struct lua_State *L)
> +{
> +	assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0);
> +	if (lua_gettop(L) == 0)
> +		return lbox_push_tuple_format(L, tuple_format_runtime);
> +	if (lua_gettop(L) > 1 || ! lua_istable(L, 1))
> +		return luaL_error(L, "Usage: box.internal.format_new(format)");
> +	uint32_t count = lua_objlen(L, 1);
> +	if (count == 0)
> +		return lbox_push_tuple_format(L, tuple_format_runtime);
> +	size_t size = count * sizeof(struct field_def);
> +	struct region *region = &fiber()->gc;
> +	size_t region_svp = region_used(region);
> +	struct field_def *fields =
> +		(struct field_def *)region_alloc(region, size);
> +	if (fields == NULL) {
> +		diag_set(OutOfMemory, size, "region_alloc", "fields");
> +		return luaT_error(L);
> +	}
> +	for (uint32_t i = 0; i < count; ++i) {
> +		size_t len;
> +
> +		fields[i] = field_def_default;
> +
> +		lua_pushinteger(L, i + 1);
> +		lua_gettable(L, 1);
> +
> +		lua_pushstring(L, "type");
> +		lua_gettable(L, -2);
> +		if (! lua_isnil(L, -1)) {
> +			const char *type_name = lua_tolstring(L, -1, &len);
> +			fields[i].type = field_type_by_name(type_name, len);
> +			if (fields[i].type == field_type_MAX) {
> +				const char *err =
> +					"field %d has unknown field type";
> +				return luaL_error(L, tt_sprintf(err, i + 1));

Here and in all other 'return on error' the region leaks. But there is
another point. How can there be errors, if the method is internal? If I
had implemented that patch, I would panic/assert on any error here, except
region OOM.