From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 1A7906EC56; Sun, 14 Mar 2021 15:43:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1A7906EC56 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1615725793; bh=WdIGXR5YY3U/Kdim1Ezwd5gpC4C/bLN5tcGEujs65B0=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=J8DmB1S/7Hs1Y8sxWJe2Og9PinA241rMIOgNVDFlVzdk3pBMTFZfamON4W3I5Cn99 18zxpu0KPNnB5Wo7OPIp3ugtTbKrWE79HNf/cApGxv39xPoqIbKQ11obtp2YOV7q79 mhDwt9N34Kv6+jAx+DpROSIQSQ9xPcEG/XSW6/UU= Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 039ED6EC56 for ; Sun, 14 Mar 2021 15:43:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 039ED6EC56 Received: by smtp32.i.mail.ru with esmtpa (envelope-from ) id 1lLQ5N-0005Zs-RC; Sun, 14 Mar 2021 15:43:10 +0300 Date: Sun, 14 Mar 2021 15:42:20 +0300 To: Roman Khabibov Message-ID: <20210314124220.GA16737@root> References: <20210311054949.91263-1-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210311054949.91263-1-roman.habibov@tarantool.org> X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9D3134714A9BDB69BC7A690B93B93DD09EFED8A8D520004C100894C459B0CD1B9B8D82FB0F27611E13A0B6F9E6626431D52774EE4D618BBD66B7995677FB1BAB6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75210414551E8CD62EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D07BBD2EBFB7BF888638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C7FF294EE7CC9FD50A39B691C9C8590818FD5AB5B83866363A471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658359CC434672EE6371117882F4460429728AD0CFFFB425014E868A13BD56FB6657A7F4EDE966BC389F9E8FC8737B5C22495B4C35AC65A386B3089D37D7C0E48F6CCF19DD082D7633A0E7DDDDC251EA7DABAAAE862A0553A39223F8577A6DFFEA7CE1AEB6AF2DA18B6243847C11F186F3C5E7DDDDC251EA7DABCC89B49CDF41148FA8EF81845B15A4842623479134186CDE6BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A53807FAF93921B490B7DCCD088E96B6F0E2EA3DE0B524A291D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75F04B387B5D7535DE410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344F2126FF337FA651949DDC6B9BA785D8CA061371845868C7EAC9F9E4A607788B5C4FF7CE01F307011D7E09C32AA3244C2F85252B82A62C4EBC705941BCE2072495A9E0DC41E9A4CFFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj4I/mOTH/3lG4h415VMXfDw== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB44E47F1499589D5D35E4F1E6D4FE296CCB28245CA303D9C28F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] serializer: serialize recursive structures X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! Side note: I've got into the swing of things by looking of previous series of patches ([1][2]). AFAICS, there was idea to throw an error on recursive serialization, is there discussion anywhere about this implementation instead the old one which I've missed? I have some questions about current version of serializer: Nit: we have no need to add a marker for structures that are not used: | Tarantool 2.8.0-108-g195d44ddc | tarantool> local a = {} local b = {a = a} b[b] = b a[b] = a setmetatable(b, {__serialize = function(...) return {a} end}) return a | --- | - &0 | ? &1 | - *0 | : *0 | ... And when serialize is omitted, it works like expected: | Tarantool 2.8.0-108-g195d44ddc | tarantool> local a = {} local b = {a = a} b[b] = b a[b] = a return a | --- | - &0 | ? &1 | a: *0 | *1: *1 | : *0 | ... In the old version it looks minimalistic and good: | Tarantool 2.8.0-0-gefc30ccf8 | tarantool> local a = {} local b = {a = a} b[b] = b a[b] = a setmetatable(b, {__serialize = function(...) return {a} end}) return a | --- | - &0 | ? - *0 | : *0 | ... Also, I found some inconsistency in showing repeated objects. For the new version: | Tarantool 2.8.0-108-g195d44ddc | local a = {} local b = {} a[b] = b setmetatable(b, {__serialize = function(...) return print end}) return a | --- | - 'function: builtin#29': *0 | ... But '&0' does not exist, so it looks confusing. Same story with threads. And for old version: | Tarantool 2.8.0-0-gefc30ccf8 | tarantool> local a = {} local b = {} a[b] = b setmetatable(b, {__serialize = function(...) return print end}) return a | --- | - 'function: builtin#29': 'function: builtin#29' | ... Side note: I'm not sure that the next example is adequate, so I CC-ed Sergos to judge me. The main idea is to change a serialization function inside serialization. It looks weird and useless, to be honest, but who knows. Here is the example: | Tarantool 2.8.0-108-g195d44ddc | tarantool> local function example() | > local a = {} | > local b = {} | > b[a] = a | > local reta | > local retb | > local function swap_ser(o) | > local newf | > local f = getmetatable(o).__serialize | > if f == reta then | > newf = retb | > else | > newf = reta | > end | > getmetatable(o).__serialize = newf | > end | > reta = function(o) swap_ser(o) return "a" end | > retb = function(o) swap_ser(o) return "b" end | > setmetatable(a, {__serialize = reta}) | > return b | > end return example() | --- | - a: *0 | ... Note, that '*0' looks even more confusing now :). For the old version: | Tarantool 2.8.0-0-gefc30ccf8 | tarantool> local function example() | > local a = {} | > local b = {} | > b[a] = a | > local reta | > local retb | > local function swap_ser(o) | > local newf | > local f = getmetatable(o).__serialize | > if f == reta then | > newf = retb | > else | > newf = reta | > end | > getmetatable(o).__serialize = newf | > end | > reta = function(o) swap_ser(o) return "a" end | > retb = function(o) swap_ser(o) return "b" end | > setmetatable(a, {__serialize = reta}) | > return b | > end return example() | --- | - a: b | ... Looks like we should check not only object itself, but that the serialization function is the same. Moreover, I don't know, what can we do with situations, when a serialize function depends on some context that can be changed inside it: The new version: | Tarantool 2.8.0-108-g195d44ddc | tarantool> local a = {} local b = {} b[a] = a local show_a = true setmetatable(a, {__serialize = function() show_a = not show_a if show_a then return "a" else return "b" end end}) return b | --- | - b: *0 | ... The old version works like expected: | Tarantool 2.8.0-0-gefc30ccf8 | tarantool> local a = {} local b = {} b[a] = a local show_a = true setmetatable(a, {__serialize = function() show_a = not show_a if show_a then return "a" else return "b" end end}) return b | --- | - b: a | ... Side note: It doesn't relate to issue and patch directly, but deep recursive calls still lead to core: | Tarantool 2.8.0-108-g195d44ddc | tarantool> local a = {} a[a] = a local recf recf = function(t) return setmetatable({}, {__serialize = recf}) end setmetatable(a, {__serialize = recf}) return a | Segmentation fault (core dumped) With the backtrace that looks like the following: | #8 0x000055fd217034d3 in lua_field_try_serialize (L=0x40ffb378, anchortable_index=3, cfg=0x41c6e8c0, idx=1375, field=0x7f29425fd760) | at /home/burii/reviews/tarantool/serialize/src/lua/utils.c:694 | #9 0x000055fd2170387d in lua_field_inspect_table (L=0x40ffb378, anchortable_index=3, cfg=0x41c6e8c0, idx=1375, field=0x7f29425fd760) | at /home/burii/reviews/tarantool/serialize/src/lua/utils.c:740 | #10 0x000055fd21704542 in luaL_tofield (L=0x40ffb378, cfg=0x41c6e8c0, anchortable_index=3, opts=0x0, index=1375, field=0x7f29425fd760) | at /home/burii/reviews/tarantool/serialize/src/lua/utils.c:944 | #11 0x000055fd217034f8 in lua_field_try_serialize (L=0x40ffb378, anchortable_index=3, cfg=0x41c6e8c0, idx=1374, field=0x7f29425fd760) | at /home/burii/reviews/tarantool/serialize/src/lua/utils.c:695 | #12 0x000055fd2170387d in lua_field_inspect_table (L=0x40ffb378, anchortable_index=3, cfg=0x41c6e8c0, idx=1374, field=0x7f29425fd760) | at /home/burii/reviews/tarantool/serialize/src/lua/utils.c:740 | #13 0x000055fd21704542 in luaL_tofield (L=0x40ffb378, cfg=0x41c6e8c0, anchortable_index=3, opts=0x0, index=1374, field=0x7f29425fd760) | at /home/burii/reviews/tarantool/serialize/src/lua/utils.c:944 | #14 0x000055fd217034f8 in lua_field_try_serialize (L=0x40ffb378, anchortable_index=3, cfg=0x41c6e8c0, idx=1373, field=0x7f29425fd760) | at /home/burii/reviews/tarantool/serialize/src/lua/utils.c:695 | #15 0x000055fd2170387d in lua_field_inspect_table (L=0x40ffb378, anchortable_index=3, cfg=0x41c6e8c0, idx=1373, field=0x7f29425fd760) | at /home/burii/reviews/tarantool/serialize/src/lua/utils.c:740 | <4K lines below> I suppose that it can be handled by checking of the serialize level of structure (table). If its to high we can either raise an error or return for the rest of the structure. At least it will be good to create an issue follow-ups this problem. Since the answers to the questions above are critical to evaluate the logic of the patch, I have not reviewed it yet. Most parts of the comments below is about to add more comments to code or some suggestions about codestile. On 11.03.21, Roman Khabibov wrote: > Fix bug with bus error during serializing of recursive > structures. Please provide a little bit more description about your changes. It helps me as a reviewer to check the patch and for other developers it will be easier to understand these changes in the future. > > Closes #3228 > --- > > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3228-visited-set > Issue: https://github.com/tarantool/tarantool/issues/3228 > > src/box/lua/call.c | 6 +- > src/box/lua/execute.c | 2 +- > src/box/lua/serialize_lua.c | 96 ++------ > src/box/lua/tuple.c | 2 +- > src/box/sql/func.c | 2 +- > src/lua/msgpack.c | 10 +- > src/lua/pickle.c | 2 +- > src/lua/utils.c | 226 ++++++++++++++++-- > src/lua/utils.h | 43 +++- > ...-3228-serializer-look-for-recursion.result | 67 ++++++ > ...228-serializer-look-for-recursion.test.lua | 17 ++ > test/swim/swim.result | 18 +- > third_party/lua-cjson/lua_cjson.c | 4 +- > third_party/lua-yaml/lyaml.cc | 88 +++---- > 14 files changed, 390 insertions(+), 193 deletions(-) > create mode 100644 test/app/gh-3228-serializer-look-for-recursion.result > create mode 100644 test/app/gh-3228-serializer-look-for-recursion.test.lua Please provide Changelog entry too. > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > index 0315e720c..e4907a3f7 100644 > --- a/src/box/lua/call.c > +++ b/src/box/lua/call.c > diff --git a/src/box/lua/serialize_lua.c b/src/box/lua/serialize_lua.c > index caa08a60f..ab612dfc2 100644 > --- a/src/box/lua/serialize_lua.c > +++ b/src/box/lua/serialize_lua.c > @@ -215,7 +215,7 @@ trace_node(struct lua_dumper *d) > @@ -339,41 +339,16 @@ emit_node(struct lua_dumper *d, struct node *nd, int indent, > static const char * > get_lua_anchor(struct lua_dumper *d) > { > - const char *s = ""; > - > - lua_pushvalue(d->L, -1); > - lua_rawget(d->L, d->anchortable_index); > - if (!lua_toboolean(d->L, -1)) { > - lua_pop(d->L, 1); > - return NULL; > - } > - > - if (lua_isboolean(d->L, -1)) { > - /* > - * This element is referenced more > - * than once but has not been named. > - */ > - char buf[32]; > - snprintf(buf, sizeof(buf), "%u", d->anchor_number++); > - lua_pop(d->L, 1); > - lua_pushvalue(d->L, -1); > - lua_pushstring(d->L, buf); > - s = lua_tostring(d->L, -1); > - lua_rawset(d->L, d->anchortable_index); > - trace_anchor(s, false); > - } else { > - /* > - * An aliased element. > - * > - * FIXME: Need an example to use. > - * > - * const char *str = lua_tostring(d->L, -1); > - */ > - const char *str = lua_tostring(d->L, -1); > - trace_anchor(str, true); > - lua_pop(d->L, 1); > + const char *anchor = NULL; > + int code = get_anchor(d->L, d->anchortable_index, &d->anchor_number, > + &anchor); > + if (code == 1) { Please avoid using magic numbers. Here and below. > + trace_anchor(anchor, false); > + } else if (code == 2) { > + trace_anchor(anchor, true); > + return ""; > } > - return s; > + return anchor; > } > > static void > @@ -783,7 +758,7 @@ dump_node(struct lua_dumper *d, struct node *nd, int indent) > @@ -881,49 +856,6 @@ dump_node(struct lua_dumper *d, struct node *nd, int indent) > return emit_node(d, nd, indent, str, len); > } > > -/** > - * Find references to tables, we use it > - * to find self references in tables. > - */ > -static void > -find_references(struct lua_dumper *d) > -{ > -} > - > /** > * Dump recursively from the root node. > */ > @@ -935,7 +867,7 @@ dump_root(struct lua_dumper *d) > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c > index 3e6f043b4..4d417ec34 100644 > --- a/src/box/lua/tuple.c > +++ b/src/box/lua/tuple.c > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index f15d27051..d4a7064f0 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c > index 24b0d2ccd..412736572 100644 > --- a/src/lua/msgpack.c > +++ b/src/lua/msgpack.c > diff --git a/src/lua/pickle.c b/src/lua/pickle.c > index 65208b5b3..86f0f77e8 100644 > --- a/src/lua/pickle.c > +++ b/src/lua/pickle.c > diff --git a/src/lua/utils.c b/src/lua/utils.c > index b5a6ca5b7..64404e3fc 100644 > --- a/src/lua/utils.c > +++ b/src/lua/utils.c > @@ -439,9 +439,54 @@ lua_gettable_wrapper(lua_State *L) > return 1; > } > > +/** > + * Check if node with @a idx serialized. > + */ > +static int > +is_serialized(struct lua_State *L, int anchortable_index, int idx) > +{ > + if (anchortable_index == 0) > + return 0; > + lua_pushvalue(L, idx); > + lua_rawget(L, anchortable_index); > + int is_serialized = 0; > + if (lua_isnil(L, -1) == 0) { Nit: This comparison is good according to current Tarantool's code style, but all old checks are using just `lua_is*()`. Personally, I prefer to keep code style consistence inside one translation unit. Feel free to ignore. Here and below. > + lua_pushstring(L, "serialized"); > + lua_rawget(L, -2); > + is_serialized = lua_toboolean(L, -1); > + lua_pop(L, 1); > + } > + lua_pop(L, 1); > + return is_serialized; > +} > + > +/** > + * Mark node with @a idx as serialized. > + */ > +static void > +mark_as_serialized(struct lua_State *L, int anchortable_index, int idx) > +{ > + if (anchortable_index == 0) > + return; > + /* Push copy of table. */ For what? Feel free to be more descriptive. > + lua_pushvalue(L, idx); > + lua_pushvalue(L, idx); > + lua_rawget(L, anchortable_index); > + if (lua_isnil(L, -1) == 1) { > + lua_pop(L, 1); > + lua_newtable(L); > + } > + int flags_index = lua_gettop(L); > + lua_pushstring(L, "serialized"); > + lua_pushboolean(L, true); > + lua_rawset(L, flags_index); > + lua_rawset(L, anchortable_index); > +} > + > +void > +find_references(struct lua_State *L, int anchortable_index, const char *mode) > +{ > + int type = lua_type(L, -1); > + if (type != LUA_TTABLE || anchortable_index == 0) > + return; > + > + int node_index = lua_gettop(L); > + lua_pushvalue(L, node_index); > + > + /* Get value with flags from anchor table. */ > + lua_pushvalue(L, node_index); > + lua_rawget(L, anchortable_index); > + if (lua_isnil(L, -1) == 1) { > + lua_pop(L, 1); > + lua_newtable(L); > + } > + int flags_index = lua_gettop(L); > + > + /* Get and increment counter. */ > + lua_pushstring(L, mode); > + lua_rawget(L, flags_index); > + > + int new_count = 0; > + if (lua_isnil(L, -1) == 1) { > + new_count = 1; > + lua_pop(L, 1); > + } else { > + lua_Integer number = lua_tointeger(L, -1); > + lua_pop(L, 1); > + if (number < 2) > + new_count = ++number; > + else > + new_count = -1; > + } > + > + /* Push new count to value with flags. */ > + if (new_count == -1) { > + lua_pop(L, 2); > + assert(node_index == lua_gettop(L)); > + return; > + } > + lua_pushstring(L, mode); > + lua_pushinteger(L, new_count); > + lua_rawset(L, flags_index); > + > + /* Check if node is already serialized. */ > + bool already_serialized = false; > + if (strcmp(mode, "after") == 0) { > + lua_pushstring(L, "serialized"); > + lua_rawget(L, flags_index); > + already_serialized = lua_toboolean(L, -1); > + lua_pop(L, 1); > + } > + lua_rawset(L, anchortable_index); > + assert(node_index == lua_gettop(L)); > + if (already_serialized == true) > + return; > + > + /* Recursively process other table values. */ > + lua_pushnil(L); > + while (lua_next(L, -2) != 0) { > + /* Find references on value. */ > + find_references(L, anchortable_index, mode); > + lua_pop(L, 1); > + /* Find references on key. */ > + find_references(L, anchortable_index, mode); > + } > +} > + > +int > +get_anchor(struct lua_State *L, int anchortable_index, > + unsigned int *anchor_number, const char **anchor) > +{ > + lua_pushvalue(L, -1); > + lua_rawget(L, anchortable_index); > + if (lua_isnil(L, -1) == 1) { > + lua_pop(L, 1); > + *anchor = NULL; > + return 0; > + } > + > + lua_pushstring(L, "before"); > + lua_rawget(L, -2); > + const char *str = NULL; > + if (lua_type(L, -1) == LUA_TSTRING) > + str = lua_tostring(L, -1); > + lua_Integer number_before = lua_tointeger(L, -1); > + lua_pop(L, 1); > + lua_Integer number_after = 0; > + if (str == NULL) { > + lua_pushstring(L, "after"); > + lua_rawget(L, -2); > + if (lua_type(L, -1) == LUA_TSTRING) > + str = lua_tostring(L, -1); > + number_after = lua_tointeger(L, -1); > + lua_pop(L, 1); > + } > + > + *anchor = ""; > + int ret = 0; > + if ((number_before > 1 || number_after > 1) && str == NULL) { > + /* > + * This element is referenced more than once but > + * has not been named. > + */ > + char buf[32]; > + snprintf(buf, sizeof(buf), "%u", (*anchor_number)++); According to your comment: | @param[out] anchor_number Ptr to the number anchors. Does it determines only by this function (as far as it is output value)? If yes, should you set *anchor_number to 0? And can it be more than 1? > + lua_pop(L, 1); > + lua_pushvalue(L, -1); > + > + lua_pushvalue(L, -1); > + lua_rawget(L, anchortable_index); > + if (number_before > 1) > + lua_pushstring(L, "before"); > + else > + lua_pushstring(L, "after"); Looks like `lua_pushstring(L, number_before > 1 ? "before" : "after");` is enough, isn't it? > + lua_pushstring(L, buf); > + *anchor = lua_tostring(L, -1); > + lua_rawset(L, -3); > + lua_rawset(L, anchortable_index); > + ret = 1; > + } else if (str != NULL) { > + /* This is an aliased element. */ > + lua_pop(L, 1); > + *anchor = str; > + ret = 2; > + } else { > + lua_pop(L, 1); > + *anchor = NULL; > + ret = 0; > + } > + return ret; > +} > + > /** > * Call __serialize method of a table object by index > * if the former exists. > @@ -496,11 +677,13 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg, > diff --git a/src/lua/utils.h b/src/lua/utils.h > index 37531676d..ef9c70dfb 100644 > --- a/src/lua/utils.h > +++ b/src/lua/utils.h > @@ -354,6 +354,29 @@ struct luaL_field { > bool compact; /* a flag used by YAML serializer */ > }; > > +/** > + * Find references to tables to look up self references in tables. > + * > + * @param L Lua stack. > + * @param anchortable_index Index of anchor table on stack. > + * @param mode When the function is called before or after dump > + function. > + */ Please add some information about how this function modifies table by `anchortable_index`. May be it makes sense to reference this function by `luaL_tofield()` and `luaL_convertfield()`. > +void > +find_references(struct lua_State *L, int anchortable_index, const char *mode); > + > +/** > + * Generate aliases and anchor numbers for self references. > + * > + * @param L Lua stack. > + * @param anchortable_index Index of anchor table on stack. > + * @param[out] anchor_number Ptr to the number anchors. > + * @param[out] anchor Ptr to anchor string. > + */ > +int > +get_anchor(struct lua_State *L, int anchortable_index, > + unsigned int *anchor_number, const char **anchor); > + > /** > * @brief Convert a value from the Lua stack to a lua_field structure. > * This function is designed for use with Lua bindings and data > @@ -388,6 +411,7 @@ struct luaL_field { > diff --git a/test/app/gh-3228-serializer-look-for-recursion.result b/test/app/gh-3228-serializer-look-for-recursion.result > new file mode 100644 > index 000000000..773470c8e > --- /dev/null > +++ b/test/app/gh-3228-serializer-look-for-recursion.result > diff --git a/test/app/gh-3228-serializer-look-for-recursion.test.lua b/test/app/gh-3228-serializer-look-for-recursion.test.lua > new file mode 100644 > index 000000000..1c9b0375f > --- /dev/null > +++ b/test/app/gh-3228-serializer-look-for-recursion.test.lua > diff --git a/test/swim/swim.result b/test/swim/swim.result > index bfc83c143..539131677 100644 > --- a/test/swim/swim.result > +++ b/test/swim/swim.result > diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c > index 38e999870..67a9762bc 100644 > --- a/third_party/lua-cjson/lua_cjson.c > +++ b/third_party/lua-cjson/lua_cjson.c > diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc > index 9c3a4a646..72f269e6c 100644 > --- a/third_party/lua-yaml/lyaml.cc > +++ b/third_party/lua-yaml/lyaml.cc > @@ -500,38 +500,23 @@ usage_error: > static int dump_node(struct lua_yaml_dumper *dumper); > > static yaml_char_t *get_yaml_anchor(struct lua_yaml_dumper *dumper) { > - const char *s = ""; > - lua_pushvalue(dumper->L, -1); > - lua_rawget(dumper->L, dumper->anchortable_index); > - if (!lua_toboolean(dumper->L, -1)) { > - lua_pop(dumper->L, 1); > - return NULL; > - } > - > - if (lua_isboolean(dumper->L, -1)) { > - /* this element is referenced more than once but has not been named */ > - char buf[32]; > - snprintf(buf, sizeof(buf), "%u", dumper->anchor_number++); > - lua_pop(dumper->L, 1); > - lua_pushvalue(dumper->L, -1); > - lua_pushstring(dumper->L, buf); > - s = lua_tostring(dumper->L, -1); > - lua_rawset(dumper->L, dumper->anchortable_index); > - } else { > - /* this is an aliased element */ > + const char *anchor = NULL; > + if (get_anchor(dumper->L, dumper->anchortable_index, &dumper->anchor_number, > + &anchor) == 2) { > yaml_event_t ev; > - const char *str = lua_tostring(dumper->L, -1); > - if (!yaml_alias_event_initialize(&ev, (yaml_char_t *) str) || > + if (!yaml_alias_event_initialize(&ev, (yaml_char_t *) anchor) || > !yaml_emitter_emit(&dumper->emitter, &ev)) > luaL_error(dumper->L, OOM_ERRMSG); > - lua_pop(dumper->L, 1); > + return (yaml_char_t *)""; > } > - return (yaml_char_t *)s; > + return (yaml_char_t *) anchor; Nit: s/(yaml_char_t *) anchor/(yaml_char_t *)anchor/, according to the previous change. > } > > -static int dump_table(struct lua_yaml_dumper *dumper, struct luaL_field *field){ > +static int dump_table(struct lua_yaml_dumper *dumper, struct luaL_field *field, > + yaml_char_t *anchor) { > yaml_event_t ev; > - yaml_char_t *anchor = get_yaml_anchor(dumper); > + if (anchor == NULL) > + anchor = get_yaml_anchor(dumper); > > if (anchor && !*anchor) return 1; > > @@ -556,10 +541,12 @@ static int dump_table(struct lua_yaml_dumper *dumper, struct luaL_field *field){ > yaml_emitter_emit(&dumper->emitter, &ev) != 0 ? 1 : 0; > } > > -static int dump_array(struct lua_yaml_dumper *dumper, struct luaL_field *field){ > +static int dump_array(struct lua_yaml_dumper *dumper, struct luaL_field *field, > + yaml_char_t *anchor) { > unsigned i; > yaml_event_t ev; > - yaml_char_t *anchor = get_yaml_anchor(dumper); > + if (anchor == NULL) > + anchor = get_yaml_anchor(dumper); > > if (anchor && !*anchor) > return 1; > @@ -621,8 +608,18 @@ static int dump_node(struct lua_yaml_dumper *dumper) > bool unused; > (void) unused; > > + yaml_char_t *anchor = NULL; > + > + /* > + * Keep anchor to print it if luaL_checkfield() push new value > + * on stack after serialization. > + */ > + if (lua_istable(dumper->L, -1) == 1) > + anchor = get_yaml_anchor(dumper); > + > int top = lua_gettop(dumper->L); > - luaL_checkfield(dumper->L, dumper->cfg, top, &field); > + luaL_checkfield(dumper->L, dumper->cfg, dumper->anchortable_index, top, > + &field); > switch(field.type) { > case MP_UINT: > snprintf(buf, sizeof(buf) - 1, "%" PRIu64, field.ival); > @@ -647,9 +644,9 @@ static int dump_node(struct lua_yaml_dumper *dumper) > len = strlen(buf); > break; > case MP_ARRAY: > - return dump_array(dumper, &field); > + return dump_array(dumper, &field, anchor); > case MP_MAP: > - return dump_table(dumper, &field); > + return dump_table(dumper, &field, anchor); > case MP_STR: > str = lua_tolstring(dumper->L, -1, &len); > if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) || > @@ -745,35 +742,6 @@ static int append_output(void *arg, unsigned char *buf, size_t len) { > return 1; > } > > -static void find_references(struct lua_yaml_dumper *dumper) { > -} > - > int > lua_yaml_encode(lua_State *L, struct luaL_serializer *serializer, > const char *tag_handle, const char *tag_prefix) > @@ -819,7 +787,7 @@ lua_yaml_encode(lua_State *L, struct luaL_serializer *serializer, > dumper.anchortable_index = lua_gettop(L); > dumper.anchor_number = 0; > lua_pushvalue(L, 1); /* push copy of arg we're processing */ > - find_references(&dumper); > + find_references(L, dumper.anchortable_index, "before"); Side note: looks like depends on now. Doesn't look like a real third party :). Feel free to ignore. > dump_document(&dumper); > if (dumper.error) > goto error; > -- > 2.24.3 (Apple Git-128) > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-November/020771.html [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018419.html -- Best regards, Sergey Kaplun