From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 18 Jun 2019 11:55:53 +0300 From: Vladimir Davydov Subject: Re: [PATCH v1 2/2] netbox: define formats for tuple from netbox Message-ID: <20190618085553.6nff2dzyl2ouzcrc@esperanza> References: <82d7940c4420df06b6e905304f27797d4d348329.1560160282.git.imeevma@gmail.com> <20190611085511.jto7yyuognolqg3b@esperanza> <20190614122920.GA3535@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190614122920.GA3535@tarantool.org> To: Mergen Imeev Cc: tarantool-patches@freelists.org List-ID: 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