Re: [Tarantool-patches] [PATCH 1/1] box: export box_session_push to the public C API

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the review!

>> diff --git a/src/box/lua/console.c b/src/box/lua/console.c
>> index 57e7e7f4f..ac353d012 100644
>> --- a/src/box/lua/console.c
>> +++ b/src/box/lua/console.c
>> @@ -410,6 +411,86 @@ port_lua_dump_plain(struct port *port, uint32_t *size)
>>  	return result;
>>  }
>>  
>> +/**
>> + * A helper for port_msgpack_dump_plain() to safely work with Lua,
>> + * being called via pcall().
>> + */
>> +static int
>> +lua_port_msgpack_dump_plain(struct lua_State *L)
>> +{
>> +	const char *data = lua_touserdata(L, 1);
>> +	lua_pop(L, 1);
>> +	luamp_decode(L, luaL_msgpack_default, &data);
> 
> I see both tag handle and prefix are the same as in port_lua_dump_plain,
> so IMHO it would be great to introduce corresponding constants for them.
> Furthermore, these values looks a bit cryptic to me, could you please
> drop a few words nearby describing them? Can these values be changed in
> the future?

Tags are the standard YAML way of adding a type to an object.
For details see lua_yaml_encode() comment, and
http://yaml.org/spec/1.2/spec.html#tag/shorthand/.

YAML allows to introduce your own tags, they just need to have a
special syntax. For pushes we've chosen the tag you can see below.

They can't be changed in future, since something may already depend on
them. They are a part of the public API.

Anyway, I now use the tag in only one place in v2 version, so no need
to move it to a constant.

>> +	return lua_yaml_encode(L, luaL_yaml_default, "!push!",
>> +			       "tag:tarantool.io/push,2018");
>> +}
>> +
>> +/** The same as port_lua plain dump, but from raw MessagePack. */
>> +const char *
>> +port_msgpack_dump_plain(struct port *base, uint32_t *size)
>> +{
>> +	struct port_msgpack *port = (struct port_msgpack *) base;
>> +	struct lua_State *L = luaT_newthread(tarantool_L);
> 
> It looks like an additional Lua coroutine is excess here and all
> conversion from MessagePack to Lua can be done using the main one (i.e.
> tarantool_L).

I was afraid that in case of an OOM we will leave garbage in tarantool_L.
But if you say we can use tarantool_L safely, then ok, I am not against
that. It is just one another reason to start panic when the heap of Lua
are out of memory, because otherwise data on tarantool_L will leak.

On the other hand, the new thread will also leak, so probably this is
a lose-lose situation, but with tarantool_L it is a little simpler.

I rewrote that part significantly in v2.

> I changed the patch a bit (diff is below) and tests
> successfully passed on my machine. Did I miss something (maybe a yield
> underneath luamp_decode / lua_yaml_encode)?
> 
> ================================================================================
> 
> diff --git a/src/box/lua/console.c b/src/box/lua/console.c
> index ac353d012..d5e0d193b 100644
> --- a/src/box/lua/console.c
> +++ b/src/box/lua/console.c
> @@ -457,7 +449,7 @@ port_msgpack_dump_plain(struct port *base, uint32_t *size)
>                  */
>                 assert(lua_isstring(L, -1));
>                 diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1));
> -               goto error;
> +               return NULL;

That is a bad idea, we still have garbage on tarantool_L. Need to
drop it first. Anyway now I use cclosure, which automatically
removes all leftovers from the stack.

>         }
>         assert(lua_gettop(L) - top == 1);
>         assert(lua_isstring(L, -1));
> ================================================================================
> 
>> +	if (L == NULL)
>> +		return NULL;
>> +	/*
>> +	 * Create a new thread and pop it immediately from the
>> +	 * main stack. So as it would not stay there in case
>> +	 * something would throw in this function.
>> +	 */
>> +	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
>> +	/*
>> +	 * MessagePack -> Lua -> YAML. The middle is not really
>> +	 * needed here, but there is no MessagePack -> YAML
>> +	 * encoder yet.
>> +	 */
>> +	int top = lua_gettop(L);
>> +	lua_pushcfunction(L, lua_port_msgpack_dump_plain);
>> +	lua_pushlightuserdata(L, (void *) port->data);
>> +	if (lua_pcall(L, 1, LUA_MULTRET, 0) != 0 ||
>> +	    lua_gettop(L) - top == 2) {
>> +		/*
>> +		 * Nil and error string are pushed onto the stack
>> +		 * if that was a YAML error. Only error string is
>> +		 * pushed in case it was a Lua error. In both
>> +		 * cases top of the stack is a string.
>> +		 */
>> +		assert(lua_isstring(L, -1));
>> +		diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1));
>> +		goto error;
>> +	}
>> +	assert(lua_gettop(L) - top == 1);
>> +	assert(lua_isstring(L, -1));
>> +	size_t len;
>> +	const char *result = lua_tolstring(L, -1, &len);
>> +	*size = (uint32_t) len;
>> +	/*
>> +	 * The result string should be copied, because the stack,
>> +	 * keeping the string, is going to be destroyed in the
>> +	 * next lines.
>> +	 * Can't use region, because somebody should free it, and
>> +	 * its purpose is to free many objects at once. In this
>> +	 * case only one string should be allocated and freed
>> +	 * right after usage. Heap is fine for that.
>> +	 * Can't use the global tarantool_lua_ibuf, because the
>> +	 * plain dumper is used by session push, which yields in
>> +	 * coio. And during that yield the ibuf can be reset by
>> +	 * another fiber.
>> +	 */
>> +	char *buffer = (char *) strdup(result);
> 
> Lua strings can contain NUL byte in the middle of the data area and it
> is a good reason for using strndup here, considering you already obtain
> the length earlier. Otherwise, it would be nice to drop a corresponding
> comment about a contract guaranteeing the NUL byte doesn't occur in the
> middle of the result value.

YAML and Lua formatters escape 0 by writing '\0'. But anyway I agree,
better use malloc + memcpy here. 'strndup' won't work, since it also
stops when sees 0. Implemented in v2 and added a test.

>> +	if (buffer == NULL) {
>> +		diag_set(OutOfMemory, len + 1, "strdup", "buffer");
>> +		goto error;
>> +	}
>> +	assert(port->buffer == NULL);
>> +	port->buffer = buffer;
> 
> It seems to be more convenient to move the lines below to a separate
> function (e.g port_msgpack_add_buffer). I have one rationale for it: you
> make a strdup right here, but release the allocated memory in
> port_msgpack_destroy. Such resource responsibility division seems to be
> complex for the further maintainence. If you're against my proposal and
> totally OK with the current approach, feel free to ignore.

Agree, implemented in v2.