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

Igor Munkin imun at tarantool.org
Tue Apr 7 02:12:48 MSK 2020


Vlad,

Thanks for the patch! Please consider the comments below.

On 23.01.20, Vladislav Shpilevoy wrote:

<snipped>

> 
> ================================================================================
> 
> Full patch:
> 
> ================================================================================
> 
> commit 34c2d8033bcfabcab95a9aeec2ca029169969133
> Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> Date:   Mon Jan 20 23:08:58 2020 +0100
> 
>     box: export box_session_push to the public C API
>     
>     API is different from box.session.push() - sync argument was
>     removed. It will disappear from Lua API as well, according to
>     a part of public API, and because it just is not needed here.
>     Session is omitted as well. Indeed, a user can't push to a foreign
>     session, and the current session can be obtained inside
>     box_session_push(). And anyway session is not in the public C API.
>     
>     Internally dump into iproto is done using obuf_dup(), just like
>     tuple_to_obuf() does. obuf_alloc() would be a bad call here,
>     because it wouldn't be able to split the pushed data into several
>     obuf chunks, and would cause obuf fragmentation.
>     
>     Dump into plain text behaves just like a Lua push - it returns a
>     YAML formatted text. But to turn MessagePack into YAML an
>     intermediate Lua representation is used, because there is no a
>     MessagePack -> YAML translator yet.
>     
>     Closes #4734
>     
>     @TarantoolBot document
>     Title: box_session_push() C API
>     
>     There is a new function in the public C API:
>     
>         int
>         box_session_push(const char *data, const char *data_end);
>     
>     It takes raw MessagePack, and behaves just like Lua
>     box.session.push().
> 

<snipped>

> 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

<snipped>

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

> +	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 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
@@ -430,20 +430,12 @@ 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);
-       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.
         */
+       lua_State *L = tarantool_L;
        int top = lua_gettop(L);
        lua_pushcfunction(L, lua_port_msgpack_dump_plain);
        lua_pushlightuserdata(L, (void *) port->data);
@@ -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;
        }
        assert(lua_gettop(L) - top == 1);
        assert(lua_isstring(L, -1));
@@ -480,15 +472,11 @@ port_msgpack_dump_plain(struct port *base, uint32_t *size)
        char *buffer = (char *) strdup(result);
        if (buffer == NULL) {
                diag_set(OutOfMemory, len + 1, "strdup", "buffer");
-               goto error;
+               return NULL;
        }
        assert(port->buffer == NULL);
        port->buffer = buffer;
-       luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
        return buffer;
-error:
-       luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
-       return NULL;
 }
 
 /**

================================================================================

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

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

> +	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
> +	return buffer;
> +error:
> +	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
> +	return NULL;
> +}
> +
>  /**
>   * Push a tagged YAML document into a console socket.
>   * @param session Console session.

<snipped>

-- 
Best regards,
IM


More information about the Tarantool-patches mailing list