[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