[Tarantool-patches] [PATCH v2 1/1] box: export box_session_push to the public C API
Igor Munkin
imun at tarantool.org
Wed Apr 8 18:33:39 MSK 2020
Vlad,
Thanks, the patch is neat! I left several comments below, please
consider them.
On 08.04.20, Vladislav Shpilevoy wrote:
> API is different from box.session.push() - sync argument was
> removed. It will disappear from Lua API as well, 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 produces a
> YAML formatted text or Lua text depending on output format. But to
> turn MessagePack into YAML or Lua text an intermediate Lua
> representation is used, because there are no a MessagePack -> YAML
> and MessagePack -> Lua text translators yet.
>
> Closes #4734
>
> @TarantoolBot document
> Title: box_session_push() C API
>
> There is a new function in the public C API:
> ```C
> int
> box_session_push(const char *data, const char *data_end);
> ```
>
> It takes raw MessagePack, and behaves just like Lua
> `box.session.push()`.
> ---
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4734-export-box-session-push
> Issue: https://github.com/tarantool/tarantool/issues/4734
>
> Changes in v2:
> - Rebased on Chris' patch for box.session.push() Lua text format;
> - Fixed Igor's comments.
>
> @ChangeLog
> - box_session_push() new public C API function. It takes a
> const char * MessagePack, and returns it to the client
> out of order, like Lua analogue (box.session.push()) (gh-4734).
>
> extra/exports | 1 +
> src/box/box.cc | 14 +++++
> src/box/box.h | 14 +++++
> src/box/call.c | 44 +++++++++++++-
> src/box/lua/console.c | 75 ++++++++++++++++++++---
> src/box/port.h | 15 +++++
> test/box/function1.c | 7 +++
> test/box/push.result | 133 +++++++++++++++++++++++++++++++++++++++++
> test/box/push.test.lua | 51 ++++++++++++++++
> 9 files changed, 344 insertions(+), 10 deletions(-)
>
<snipped>
> diff --git a/src/box/call.c b/src/box/call.c
> index a46a61c3c..be4f71f59 100644
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -64,17 +64,55 @@ port_msgpack_get_msgpack(struct port *base, uint32_t *size)
> return port->data;
> }
>
<snipped>
> +
> +char *
> +port_mspack_set_plain(struct port *base, const char *plain, uint32_t len)
> +{
> + struct port_msgpack *port = (struct port_msgpack *) base;
> + assert(port->plain == NULL);
> + port->plain = (char *)malloc(len + 1);
Typo: here is some mess with cast style: here you omit the whitespace
prior to <malloc>, but there is a cast with whitespace prior to variable
few line above. I guess there is the one policy for it (at least within
a single translation unit).
Minor: AFAIR <void *> is casted considering the destination pointer type
*in plain C*. Is it a kinda Tarantool-wide rule to explicitly cast
pointers for such assignments?
> + if (port->plain == NULL) {
> + diag_set(OutOfMemory, len + 1, "malloc", "port->plain");
> + return NULL;
> + }
> + memcpy(port->plain, plain, len);
> + port->plain[len] = 0;
> + return port->plain;
> +}
> +
<snipped>
> diff --git a/src/box/lua/console.c b/src/box/lua/console.c
> index bd454c269..396cb87f2 100644
> --- a/src/box/lua/console.c
> +++ b/src/box/lua/console.c
> @@ -37,6 +37,7 @@
> #include "lua/fiber.h"
> #include "fiber.h"
> #include "coio.h"
> +#include "lua/msgpack.h"
> #include "lua-yaml/lyaml.h"
> #include <lua.h>
> #include <lauxlib.h>
> @@ -390,19 +391,17 @@ console_set_output_format(enum output_format output_format)
> }
>
> /**
> - * Dump port lua data with respect to output format:
> + * Dump Lua data to text with respect to output format:
> * YAML document tagged with !push! global tag or Lua string.
> - * @param port Port lua.
> + * @param L Lua state.
> * @param[out] size Size of the result.
> *
> - * @retval not NULL Tagged YAML document.
> + * @retval not NULL Tagged YAML document or Lua text.
> * @retval NULL Error.
> */
> -const char *
> -port_lua_dump_plain(struct port *port, uint32_t *size)
> +static const char *
> +lua_dump_plain(struct lua_State *L, uint32_t *size)
Please no... lua_* and luaL_* are Lua standart namespace prefixes, let's
stop spoiling it right here (even if the function is localized within a
translation unit). Furthermore, it doesn't respect the lua_CFunction
signature and lua_* prefix is definitely a misguiding one here. Let's
try to use another prefix here.
> {
> - struct port_lua *port_lua = (struct port_lua *) port;
> - struct lua_State *L = port_lua->L;
> enum output_format fmt = console_get_output_format();
> if (fmt == OUTPUT_FORMAT_YAML) {
> int rc = lua_yaml_encode(L, luaL_yaml_default, "!push!",
> @@ -435,6 +434,68 @@ port_lua_dump_plain(struct port *port, uint32_t *size)
> return result;
> }
>
> +/** Plain text converter for port Lua data. */
> +const char *
> +port_lua_dump_plain(struct port *base, uint32_t *size)
> +{
> + return lua_dump_plain(((struct port_lua *)base)->L, size);
Typo: one more time, it looks like a mess in type cast style within a
single translation unit.
> +}
> +
> +/**
> + * A helper for port_msgpack_dump_plain() to execute it safely
> + * regarding Lua errors.
> + */
> +static int
> +lua_port_msgpack_dump_plain(struct lua_State *L)
I was so deep into the patch semantics and totally missed it in v1...
Let's think about the prefix different from Lua standart one.
> +{
> + char **result = (char **)lua_touserdata(L, lua_upvalueindex(1));
> + struct port_msgpack *port =
> + (struct port_msgpack *)lua_touserdata(L, lua_upvalueindex(2));
> + uint32_t *size = (uint32_t *)lua_touserdata(L, lua_upvalueindex(3));
> + const char *data = port->data;
> + /*
> + * MessagePack -> Lua object -> YAML/Lua text. The middle
> + * is not really needed here, but there is no
> + * MessagePack -> YAML encoder yet. Neither
> + * MessagePack -> Lua text.
> + */
> + luamp_decode(L, luaL_msgpack_default, &data);
> + data = lua_dump_plain(L, size);
> + if (data == NULL) {
> + *result = NULL;
> + return 0;
> + }
> + *result = port_mspack_set_plain((struct port *)port, data, *size);
> + return 0;
> + }
> +
> +/** Plain text converter for raw MessagePack. */
> +const char *
> +port_msgpack_dump_plain(struct port *base, uint32_t *size)
> +{
> + struct lua_State *L = tarantool_L;
Moved our discussion from the previous thread here:
================================================================================
> 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.
When OOM occurs both host and guest stacks are unwinded to the closest
protected frame (i.e. the one created via <lua_pcall> below). Guest
stack base is restored to the corresponding slot, slots above the top
one won't be marked and GC objects will be collected sometime later. If
there are no unmanaged memory allocations underneath then no leaks
occur.
>
> 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 might be missing something, but I see no memory leak since you create
a protected frame via <lua_pcall>.
================================================================================
> + char *result = NULL;
> + int top = lua_gettop(L);
> + (void) top;
> + lua_pushlightuserdata(L, &result);
> + lua_pushlightuserdata(L, base);
> + lua_pushlightuserdata(L, size);
> + lua_pushcclosure(L, lua_port_msgpack_dump_plain, 3);
> + if (lua_pcall(L, 0, 0, 0) != 0) {
If <lua_port_msgpack_dump_plain> returns nothing via guest stack, you
can use more convenient <lua_cpcall>[1] here. It requires almost no
auxillary actions: just fill a structure to be passed via the void
pointer argument to the function.
> + /*
> + * Error string is pushed in case it was a Lua
> + * error.
> + */
> + assert(lua_isstring(L, -1));
> + diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1));
> + lua_pop(L, 1);
> + assert(lua_gettop(L) == top);
> + return NULL;
> + }
> + assert(lua_gettop(L) == top);
> + return result;
> +}
> +
> /**
> * Push a tagged YAML document or a Lua string into a console
> * socket.
<snipped>
> diff --git a/test/box/push.test.lua b/test/box/push.test.lua
> index 7ae6f4a86..d2dc31db6 100644
> --- a/test/box/push.test.lua
> +++ b/test/box/push.test.lua
> @@ -264,3 +264,54 @@ chan_push:put(true)
> chan_push:get()
> box.schema.func.drop('do_long_and_push')
> box.session.on_disconnect(nil, on_disconnect)
> +
> +--
> +-- gh-4734: C API for session push.
> +--
> +build_path = os.getenv("BUILDDIR")
> +old_cpath = package.cpath
Minor: I see no reason for saving original package.cpath, if you are not
restoring it further. Thereby this line looks to be excess.
> +package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..old_cpath
> +
> +box.schema.func.create('function1.test_push', {language = 'C'})
> +box.schema.user.grant('guest', 'super')
> +c = netbox.connect(box.cfg.listen)
> +messages = {}
> +c:call('function1.test_push', \
> + {1, 2, 3}, \
> + {on_push = table.insert, \
> + on_push_ctx = messages})
> +messages
> +c:close()
> +--
> +-- C can push to the console.
> +--
> +ffi = require('ffi')
> +cd = ffi.new('char[3]')
> +cd[0] = 65
> +cd[1] = 0
> +cd[2] = 67
> +-- A string having 0 byte inside. Check that it is handled fine.
> +s = ffi.string(cd, 3)
This can be done much simpler way that requires no FFI:
| s = '\x65\x00\x67'
> +
> +console = require('console')
> +fio = require('fio')
> +socket = require('socket')
> +sock_path = fio.pathjoin(fio.cwd(), 'console.sock')
> +_ = fio.unlink(sock_path)
> +server = console.listen(sock_path)
> +client = socket.tcp_connect('unix/', sock_path)
> +_ = client:read({chunk = 128})
> +_ = client:write("box.func['function1.test_push']:call({1, 2, 3, s})\n")
> +client:read("\n...\n")
> +_ = client:read("\n...\n")
> +-- Lua output format is supported too.
> +_ = client:write("\\set output lua\n")
> +_ = client:read(";")
> +_ = client:write("box.func['function1.test_push']:call({1, 2, 3, s})\n")
> +client:read(";")
> +_ = client:read(";")
> +client:close()
> +server:close()
> +
> +box.schema.user.revoke('guest', 'super')
> +box.schema.func.drop('function1.test_push')
> --
> 2.21.1 (Apple Git-122.3)
>
[1]: https://www.lua.org/manual/5.1/manual.html#lua_cpcall
--
Best regards,
IM
More information about the Tarantool-patches
mailing list