From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: imeevma@tarantool.org Subject: [PATCH v2 2/3] lua: internal function to parse space format Date: Wed, 19 Jun 2019 13:34:24 +0300 Message-Id: In-Reply-To: References: To: vdavydov.dev@gmail.com Cc: tarantool-patches@freelists.org List-ID: 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)