Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Roman Khabibov <roman.habibov@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] serializer: serialize recursive structures
Date: Sun, 14 Mar 2021 15:42:20 +0300	[thread overview]
Message-ID: <20210314124220.GA16737@root> (raw)
In-Reply-To: <20210311054949.91263-1-roman.habibov@tarantool.org>

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 <snipped> 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

<snipped>

> 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)

<snipped>

> @@ -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)

<snipped>

> @@ -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)
> -{

<snipped>

> -}
> -
>  /**
>   * Dump recursively from the root node.
>   */
> @@ -935,7 +867,7 @@ dump_root(struct lua_dumper *d)

<snipped>

> 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

<snipped>

> 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

<snipped>

> 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

<snipped>

> 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

<snipped>

> 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);
> +}
> +

<snipped>

> +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,

<snipped>

> 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 {

<snipped>

> 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

<snipped>

> 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

<snipped>

> 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

<snipped>

> 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

<snipped>

> 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) {

<snipped>

> -}
> -
>  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 <third_party/lua-yaml> depends on
<src/lua/utils.c> 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

  reply	other threads:[~2021-03-14 12:43 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-11  5:49 Roman Khabibov via Tarantool-patches
2021-03-14 12:42 ` Sergey Kaplun via Tarantool-patches [this message]
2021-04-05 22:05   ` Roman Khabibov via Tarantool-patches
2021-04-21  9:27     ` Sergey Kaplun via Tarantool-patches
2021-04-21 13:12       ` Sergey Bronnikov via Tarantool-patches
2021-04-21 18:20         ` Sergey Kaplun via Tarantool-patches
2021-04-13 15:54 ` Sergey Bronnikov via Tarantool-patches
2021-04-15 20:39   ` Roman Khabibov via Tarantool-patches
2021-04-21 12:34 ` Sergey Bronnikov via Tarantool-patches
2021-07-07 22:42 ` Alexander Turenko via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210314124220.GA16737@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=roman.habibov@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] serializer: serialize recursive structures' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox