Re: [PATCH v1 2/2] netbox: define formats for tuple from netbox

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

On Fri, Jun 14, 2019 at 03:29:21PM +0300, Mergen Imeev wrote:
> First new patch:

It looks like it's time to send v2 in a separate thread.

See a few minor comments inline.

> 
> From 63075deb8411643d4af0cf27be2c024b5a20222c Mon Sep 17 00:00:00 2001
> Date: Tue, 11 Jun 2019 15:21:39 +0300
> Subject: [PATCH] lua: internal function to parse space format
> 
> This patch defines a new function that parses space format and
> returns it to Lua as cdata. For this cdata destructor is included.
> 
> Needed for #2978
> 
> diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
> index 8de7401..e62e2ba 100644
> --- a/src/box/lua/misc.cc
> +++ b/src/box/lua/misc.cc
> @@ -37,9 +37,13 @@
>  
>  #include "box/box.h"
>  #include "box/port.h"
> +#include "box/tuple.h"
> +#include "box/tuple_format.h"
>  #include "box/lua/tuple.h"
>  #include "mpstream.h"
>  
> +uint32_t CTID_STRUCT_TUPLE_FORMAT_PTR;
> +

Should be static.

>  /** {{{ Miscellaneous utils **/
>  
>  char *
> @@ -116,14 +120,117 @@ lbox_select(lua_State *L)
>  
>  /* }}} */
>  
> +static int
> +lbox_tuple_format_gc(struct lua_State *L)
> +{
> +	uint32_t ctypeid;
> +	struct tuple_format *format =
> +		*(struct tuple_format **)luaL_checkcdata(L, 1, &ctypeid);
> +	if (ctypeid != CTID_STRUCT_TUPLE_FORMAT_PTR) {
> +		luaL_error(L, "Invalid argument: 'struct tuple_format *' "\

Backslash (\) is not necessary.

Anyway, I think we better factor this check out into a separate
function, like lboxk_check_tuple_format, so that we could easily
reuse it in case we introduce some new functions taking a format.

> +			   "expected, got %s)",
> +			   lua_typename(L, lua_type(L, 1)));
> +	}
> +	box_tuple_format_unref(format);

Please use tuple_format_ref/tuple_format_unref.

box_* functions exist for external modules. No need to use them here.

> +	return 0;
> +}
> +
> +static int
> +lbox_push_tuple_format(struct lua_State *L, struct tuple_format *format)
> +{
> +	struct tuple_format **ptr = (struct tuple_format **)
> +		luaL_pushcdata(L, CTID_STRUCT_TUPLE_FORMAT_PTR);
> +	*ptr = format;
> +	box_tuple_format_ref(format);
> +	lua_pushcfunction(L, lbox_tuple_format_gc);
> +	luaL_setcdatagc(L, -2);
> +	return 1;
> +}
> +
> +static int
> +lbox_format_new(struct lua_State *L)

Please be consistent and use tuple_format prefix everywhere.

> +{
> +	assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0);
> +	if (lua_gettop(L) != 1 || ! lua_istable(L, 1))
> +		return luaL_error(L, "Usage: box.internal.format_new(format)");

I would rather name this function box.internal.new_tuple_format.

Also, I think we should allow calling it without any arguments, in which
case it should be equivalent to box.internal.new_tuple_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));
> +			}
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_pushstring(L, "name");
> +		lua_gettable(L, -2);
> +		if (lua_isnil(L, -1)) {
> +			return luaL_error(L, tt_sprintf("field %d name is not "\

Useless backslash (\).

> +							"specified", i + 1));
> +		}
> +		const char *name = lua_tolstring(L, -1, &len);
> +		fields[i].name = (char *)region_alloc(region, len + 1);
> +		if (fields == NULL) {
> +			diag_set(OutOfMemory, size, "region_alloc",
> +				 "fields[i].name");
> +			return luaT_error(L);
> +		}
> +		memcpy(fields[i].name, name, len);
> +		fields[i].name[len] = '\0';
> +		lua_pop(L, 1);
> +		lua_pop(L, 1);
> +	}
> +	struct tuple_dictionary *dict = tuple_dictionary_new(fields, count);
> +	region_truncate(region, region_svp);
> +	if (dict == NULL)
> +		return luaT_error(L);
> +	struct tuple_format *format =
> +		tuple_format_new(&tuple_format_runtime->vtab, NULL, NULL, 0,
> +				 NULL, 0, 0, dict, false, false);
> +	if (format == NULL)
> +		return luaT_error(L);
> +	return lbox_push_tuple_format(L, format);
> +}
> +
>  void
>  box_lua_misc_init(struct lua_State *L)
>  {
>  	static const struct luaL_Reg boxlib_internal[] = {
>  		{"select", lbox_select},
> +		{"format_new", lbox_format_new},
>  		{NULL, NULL}
>  	};
>  
>  	luaL_register(L, "box.internal", boxlib_internal);
>  	lua_pop(L, 1);
> +
> +	int rc = luaL_cdef(L, "struct tuple_format;");
> +	assert(rc == 0);
> +	(void) rc;
> +	CTID_STRUCT_TUPLE_FORMAT_PTR = luaL_ctypeid(L, "struct tuple_format *");
> +	assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0);
>  }
> diff --git a/test/box/misc.result b/test/box/misc.result
> index 43b5a4a..ce39bbb 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -1271,3 +1271,51 @@ box.cfg{too_long_threshold = too_long_threshold}
>  s:drop()
>  ---
>  ...
> +--
> +-- gh-2978: Function to parse space format.
> +--
> +format = {}
> +---
> +...
> +format[1] = {}
> +---
> +...
> +format[1].type = 'unsigned'
> +---
> +...
> +box.internal.format_new(format)
> +---
> +- error: field 1 name is not specified
> +...
> +format[1].name = 'aaa'
> +---
> +...
> +format[2] = {}
> +---
> +...
> +format[2].name = 'aaa'
> +---
> +...
> +format[2].type = 'any'
> +---
> +...
> +box.internal.format_new(format)
> +---
> +- error: Space field 'aaa' is duplicate
> +...
> +format[2].name = 'bbb'
> +---
> +...
> +format[3] = {}
> +---
> +...
> +format[3].name = 'ccc'
> +---
> +...
> +format[3].type = 'something'
> +---
> +...
> +box.internal.format_new(format)
> +---
> +- error: field 3 has unknown field type
> +...

Please write more tests:
 - setting 'format' without 'type'
 - passing something different from a table to the function
 - passing the value returned by space:format() to this function
 - setting is_nullable in the input table