[PATCH v2 2/3] lua: internal function to parse space format
imeevma at tarantool.org
imeevma at tarantool.org
Wed Jun 19 13:34:24 MSK 2019
Thank you for review! My answers, diff between versions and new
patch below.
On 6/18/19 11:55 AM, Vladimir Davydov wrote:
> 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.
>
Done.
> 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.
>
Fixed.
>> /** {{{ 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.
>
Fixed.
> 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.
>
Fixed.
>> + 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.
>
Fixed.
>> +{
>> + 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.
>
Fixed.
> Also, I think we should allow calling it without any arguments, in which
> case it should be equivalent to box.internal.new_tuple_format({}).
>
Fixed.
>> + 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 (\).
>
Fixed.
> 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
Fixed.
Diff between versions:
>From 9196d92c38772913e5ac0e078d6b9c1342e96bf3 Mon Sep 17 00:00:00 2001
Date: Wed, 19 Jun 2019 12:01:55 +0300
Subject: [PATCH] Review fix
diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index e62e2ba..a835d3d 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -42,7 +42,7 @@
#include "box/lua/tuple.h"
#include "mpstream.h"
-uint32_t CTID_STRUCT_TUPLE_FORMAT_PTR;
+static uint32_t CTID_STRUCT_TUPLE_FORMAT_PTR;
/** {{{ Miscellaneous utils **/
@@ -120,18 +120,27 @@ lbox_select(lua_State *L)
/* }}} */
-static int
-lbox_tuple_format_gc(struct lua_State *L)
+/** {{{ Utils to work with tuple_format. **/
+
+struct tuple_format *
+lbox_check_tuple_format(struct lua_State *L, int narg)
{
uint32_t ctypeid;
struct tuple_format *format =
- *(struct tuple_format **)luaL_checkcdata(L, 1, &ctypeid);
+ *(struct tuple_format **)luaL_checkcdata(L, narg, &ctypeid);
if (ctypeid != CTID_STRUCT_TUPLE_FORMAT_PTR) {
- luaL_error(L, "Invalid argument: 'struct tuple_format *' "\
+ luaL_error(L, "Invalid argument: 'struct tuple_format *' "
"expected, got %s)",
- lua_typename(L, lua_type(L, 1)));
+ lua_typename(L, lua_type(L, narg)));
}
- box_tuple_format_unref(format);
+ return format;
+}
+
+static int
+lbox_tuple_format_gc(struct lua_State *L)
+{
+ struct tuple_format *format = lbox_check_tuple_format(L, 1);
+ tuple_format_unref(format);
return 0;
}
@@ -141,17 +150,19 @@ 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);
+ 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)
+lbox_tuple_format_new(struct lua_State *L)
{
assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0);
- if (lua_gettop(L) != 1 || ! lua_istable(L, 1))
+ 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)
@@ -189,7 +200,7 @@ lbox_format_new(struct lua_State *L)
lua_pushstring(L, "name");
lua_gettable(L, -2);
if (lua_isnil(L, -1)) {
- return luaL_error(L, tt_sprintf("field %d name is not "\
+ return luaL_error(L, tt_sprintf("field %d name is not "
"specified", i + 1));
}
const char *name = lua_tolstring(L, -1, &len);
@@ -216,12 +227,14 @@ lbox_format_new(struct lua_State *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},
+ {"new_tuple_format", lbox_tuple_format_new},
{NULL, NULL}
};
diff --git a/src/box/lua/misc.h b/src/box/lua/misc.h
index dfedfe3..1c6f1f6 100644
--- a/src/box/lua/misc.h
+++ b/src/box/lua/misc.h
@@ -45,6 +45,9 @@ lbox_encode_tuple_on_gc(struct lua_State *L, int idx, size_t *p_len);
void
box_lua_misc_init(struct lua_State *L);
+struct tuple_format *
+lbox_check_tuple_format(struct lua_State *L, int narg);
+
#if defined(__cplusplus)
} /* extern "C" */
#endif /* defined(__cplusplus) */
diff --git a/test/box/misc.result b/test/box/misc.result
index ce39bbb..5c42e33 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1274,6 +1274,7 @@ s:drop()
--
-- gh-2978: Function to parse space format.
--
+-- Error: no field name:
format = {}
---
...
@@ -1283,10 +1284,11 @@ format[1] = {}
format[1].type = 'unsigned'
---
...
-box.internal.format_new(format)
+box.internal.new_tuple_format(format)
---
- error: field 1 name is not specified
...
+-- Error: duplicate field name:
format[1].name = 'aaa'
---
...
@@ -1299,10 +1301,11 @@ format[2].name = 'aaa'
format[2].type = 'any'
---
...
-box.internal.format_new(format)
+box.internal.new_tuple_format(format)
---
- error: Space field 'aaa' is duplicate
...
+-- Error: wrong field type:
format[2].name = 'bbb'
---
...
@@ -1315,7 +1318,44 @@ format[3].name = 'ccc'
format[3].type = 'something'
---
...
-box.internal.format_new(format)
+box.internal.new_tuple_format(format)
---
- error: field 3 has unknown field type
...
+-- Error: too many arguments:
+box.internal.new_tuple_format(format, format)
+---
+- error: 'Usage: box.internal.format_new(format)'
+...
+-- Error: wrong argument:
+box.internal.new_tuple_format(1)
+---
+- error: 'Usage: box.internal.format_new(format)'
+...
+--
+-- In next tests we should receive cdata("struct tuple_format *").
+-- We do not have a way to check cdata in Lua, but there should be
+-- no errors.
+--
+-- Without argument it is equivalent to new_tuple_format({})
+tuple_format = box.internal.new_tuple_format()
+---
+...
+-- If no type that type == "any":
+format[3].type = nil
+---
+...
+tuple_format = box.internal.new_tuple_format(format)
+---
+...
+-- Function space:format() without arguments returns valid format:
+tuple_format = box.internal.new_tuple_format(box.space._space:format())
+---
+...
+-- Check is_nullable option fo field
+format[2].is_nullable = true
+---
+...
+tuple_format = box.internal.new_tuple_format(format)
+---
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index 2d13b99..94a6450 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -357,19 +357,49 @@ s:drop()
--
-- gh-2978: Function to parse space format.
--
+
+-- Error: no field name:
format = {}
format[1] = {}
format[1].type = 'unsigned'
-box.internal.format_new(format)
+box.internal.new_tuple_format(format)
+-- Error: duplicate field name:
format[1].name = 'aaa'
format[2] = {}
format[2].name = 'aaa'
format[2].type = 'any'
-box.internal.format_new(format)
+box.internal.new_tuple_format(format)
+-- Error: wrong field type:
format[2].name = 'bbb'
format[3] = {}
format[3].name = 'ccc'
format[3].type = 'something'
-box.internal.format_new(format)
+box.internal.new_tuple_format(format)
+
+-- Error: too many arguments:
+box.internal.new_tuple_format(format, format)
+
+-- Error: wrong argument:
+box.internal.new_tuple_format(1)
+
+--
+-- In next tests we should receive cdata("struct tuple_format *").
+-- We do not have a way to check cdata in Lua, but there should be
+-- no errors.
+--
+
+-- Without argument it is equivalent to new_tuple_format({})
+tuple_format = box.internal.new_tuple_format()
+
+-- If no type that type == "any":
+format[3].type = nil
+tuple_format = box.internal.new_tuple_format(format)
+
+-- Function space:format() without arguments returns valid format:
+tuple_format = box.internal.new_tuple_format(box.space._space:format())
+
+-- Check is_nullable option fo field
+format[2].is_nullable = true
+tuple_format = box.internal.new_tuple_format(format)
New version:
>From a911d820ae46a4ac9151e649ee45f6df21e6120a 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..a835d3d 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"
+static uint32_t CTID_STRUCT_TUPLE_FORMAT_PTR;
+
/** {{{ Miscellaneous utils **/
char *
@@ -116,14 +120,130 @@ lbox_select(lua_State *L)
/* }}} */
+/** {{{ Utils to work with tuple_format. **/
+
+struct tuple_format *
+lbox_check_tuple_format(struct lua_State *L, int narg)
+{
+ uint32_t ctypeid;
+ struct tuple_format *format =
+ *(struct tuple_format **)luaL_checkcdata(L, narg, &ctypeid);
+ if (ctypeid != CTID_STRUCT_TUPLE_FORMAT_PTR) {
+ luaL_error(L, "Invalid argument: 'struct tuple_format *' "
+ "expected, got %s)",
+ lua_typename(L, lua_type(L, narg)));
+ }
+ return format;
+}
+
+static int
+lbox_tuple_format_gc(struct lua_State *L)
+{
+ struct tuple_format *format = lbox_check_tuple_format(L, 1);
+ tuple_format_unref(format);
+ 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;
+ tuple_format_ref(format);
+ lua_pushcfunction(L, lbox_tuple_format_gc);
+ luaL_setcdatagc(L, -2);
+ return 1;
+}
+
+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));
+ }
+ }
+ 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 "
+ "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},
+ {"new_tuple_format", lbox_tuple_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/src/box/lua/misc.h b/src/box/lua/misc.h
index dfedfe3..1c6f1f6 100644
--- a/src/box/lua/misc.h
+++ b/src/box/lua/misc.h
@@ -45,6 +45,9 @@ lbox_encode_tuple_on_gc(struct lua_State *L, int idx, size_t *p_len);
void
box_lua_misc_init(struct lua_State *L);
+struct tuple_format *
+lbox_check_tuple_format(struct lua_State *L, int narg);
+
#if defined(__cplusplus)
} /* extern "C" */
#endif /* defined(__cplusplus) */
diff --git a/test/box/misc.result b/test/box/misc.result
index 43b5a4a..5c42e33 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1271,3 +1271,91 @@ box.cfg{too_long_threshold = too_long_threshold}
s:drop()
---
...
+--
+-- gh-2978: Function to parse space format.
+--
+-- Error: no field name:
+format = {}
+---
+...
+format[1] = {}
+---
+...
+format[1].type = 'unsigned'
+---
+...
+box.internal.new_tuple_format(format)
+---
+- error: field 1 name is not specified
+...
+-- Error: duplicate field name:
+format[1].name = 'aaa'
+---
+...
+format[2] = {}
+---
+...
+format[2].name = 'aaa'
+---
+...
+format[2].type = 'any'
+---
+...
+box.internal.new_tuple_format(format)
+---
+- error: Space field 'aaa' is duplicate
+...
+-- Error: wrong field type:
+format[2].name = 'bbb'
+---
+...
+format[3] = {}
+---
+...
+format[3].name = 'ccc'
+---
+...
+format[3].type = 'something'
+---
+...
+box.internal.new_tuple_format(format)
+---
+- error: field 3 has unknown field type
+...
+-- Error: too many arguments:
+box.internal.new_tuple_format(format, format)
+---
+- error: 'Usage: box.internal.format_new(format)'
+...
+-- Error: wrong argument:
+box.internal.new_tuple_format(1)
+---
+- error: 'Usage: box.internal.format_new(format)'
+...
+--
+-- In next tests we should receive cdata("struct tuple_format *").
+-- We do not have a way to check cdata in Lua, but there should be
+-- no errors.
+--
+-- Without argument it is equivalent to new_tuple_format({})
+tuple_format = box.internal.new_tuple_format()
+---
+...
+-- If no type that type == "any":
+format[3].type = nil
+---
+...
+tuple_format = box.internal.new_tuple_format(format)
+---
+...
+-- Function space:format() without arguments returns valid format:
+tuple_format = box.internal.new_tuple_format(box.space._space:format())
+---
+...
+-- Check is_nullable option fo field
+format[2].is_nullable = true
+---
+...
+tuple_format = box.internal.new_tuple_format(format)
+---
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index 75d937e..94a6450 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -353,3 +353,53 @@ lsn == expected_lsn
box.cfg{too_long_threshold = too_long_threshold}
s:drop()
+
+--
+-- gh-2978: Function to parse space format.
+--
+
+-- Error: no field name:
+format = {}
+format[1] = {}
+format[1].type = 'unsigned'
+box.internal.new_tuple_format(format)
+
+-- Error: duplicate field name:
+format[1].name = 'aaa'
+format[2] = {}
+format[2].name = 'aaa'
+format[2].type = 'any'
+box.internal.new_tuple_format(format)
+
+-- Error: wrong field type:
+format[2].name = 'bbb'
+format[3] = {}
+format[3].name = 'ccc'
+format[3].type = 'something'
+box.internal.new_tuple_format(format)
+
+-- Error: too many arguments:
+box.internal.new_tuple_format(format, format)
+
+-- Error: wrong argument:
+box.internal.new_tuple_format(1)
+
+--
+-- In next tests we should receive cdata("struct tuple_format *").
+-- We do not have a way to check cdata in Lua, but there should be
+-- no errors.
+--
+
+-- Without argument it is equivalent to new_tuple_format({})
+tuple_format = box.internal.new_tuple_format()
+
+-- If no type that type == "any":
+format[3].type = nil
+tuple_format = box.internal.new_tuple_format(format)
+
+-- Function space:format() without arguments returns valid format:
+tuple_format = box.internal.new_tuple_format(box.space._space:format())
+
+-- Check is_nullable option fo field
+format[2].is_nullable = true
+tuple_format = box.internal.new_tuple_format(format)
More information about the Tarantool-patches
mailing list