[Tarantool-patches] [PATCH v2 1/1] box: export box_session_push to the public C API
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Apr 8 23:35:57 MSK 2020
Hi! Thanks for the review!
>> 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).
Yes, we decided to remove whitespace after cast, since we don't apply
it for other unary operators. I still didn't use to it. Fixed. Although
probably not everywhere, so be attentive during the next review.
====================
diff --git a/src/box/box.cc b/src/box/box.cc
index 15e79df19..cfdf961c5 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1462,7 +1462,7 @@ box_session_push(const char *data, const char *data_end)
if (session == NULL)
return -1;
struct port_msgpack port;
- struct port *base = (struct port *) &port;
+ struct port *base = (struct port *)&port;
port_msgpack_create(base, data, data_end - data);
int rc = session_push(session, session_sync(session), base);
port_msgpack_destroy(base);
diff --git a/src/box/call.c b/src/box/call.c
index be4f71f59..811c44c65 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -67,7 +67,7 @@ port_msgpack_get_msgpack(struct port *base, uint32_t *size)
static int
port_msgpack_dump_msgpack(struct port *base, struct obuf *out)
{
- struct port_msgpack *port = (struct port_msgpack *) base;
+ struct port_msgpack *port = (struct port_msgpack *)base;
assert(port->vtab == &port_msgpack_vtab);
size_t size = port->data_sz;
if (obuf_dup(out, port->data, size) == size)
@@ -85,7 +85,7 @@ port_msgpack_dump_plain(struct port *base, uint32_t *size);
void
port_msgpack_destroy(struct port *base)
{
- struct port_msgpack *port = (struct port_msgpack *) base;
+ struct port_msgpack *port = (struct port_msgpack *)base;
assert(port->vtab == &port_msgpack_vtab);
free(port->plain);
}
@@ -93,7 +93,7 @@ port_msgpack_destroy(struct port *base)
char *
port_mspack_set_plain(struct port *base, const char *plain, uint32_t len)
{
- struct port_msgpack *port = (struct port_msgpack *) base;
+ struct port_msgpack *port = (struct port_msgpack *)base;
assert(port->plain == NULL);
port->plain = (char *)malloc(len + 1);
if (port->plain == NULL) {
diff --git a/test/box/function1.c b/test/box/function1.c
index b2ce752a9..a28431e86 100644
--- a/test/box/function1.c
+++ b/test/box/function1.c
@@ -249,6 +249,6 @@ test_sleep(box_function_ctx_t *ctx, const char *args, const char *args_end)
int
test_push(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- (void) ctx;
+ (void)ctx;
return box_session_push(args, args_end);
}
====================
> 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?
Yes, we cast in C too. Usually, in the new code. Still many old places
omit the cast, and Vladimir usually omitted it even in the new code.
Hard even to tell what is more - with cast or without.
>> + 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;
>> +}
>> +
>> 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
>> @@ -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).
Agree. I removed 'lua_' prefix from all new functions:
====================
diff --git a/src/box/lua/console.c b/src/box/lua/console.c
index 396cb87f2..886120eac 100644
--- a/src/box/lua/console.c
+++ b/src/box/lua/console.c
@@ -400,7 +400,7 @@ console_set_output_format(enum output_format output_format)
* @retval NULL Error.
*/
static const char *
-lua_dump_plain(struct lua_State *L, uint32_t *size)
+console_dump_plain(struct lua_State *L, uint32_t *size)
{
enum output_format fmt = console_get_output_format();
if (fmt == OUTPUT_FORMAT_YAML) {
@@ -438,7 +438,7 @@ lua_dump_plain(struct lua_State *L, uint32_t *size)
const char *
port_lua_dump_plain(struct port *base, uint32_t *size)
{
- return lua_dump_plain(((struct port_lua *)base)->L, size);
+ return console_dump_plain(((struct port_lua *)base)->L, size);
}
/**
@@ -446,7 +446,7 @@ port_lua_dump_plain(struct port *base, uint32_t *size)
* regarding Lua errors.
*/
static int
-lua_port_msgpack_dump_plain(struct lua_State *L)
+port_msgpack_dump_plain_via_lua(struct lua_State *L)
{
char **result = (char **)lua_touserdata(L, lua_upvalueindex(1));
struct port_msgpack *port =
@@ -460,7 +460,7 @@ lua_port_msgpack_dump_plain(struct lua_State *L)
* MessagePack -> Lua text.
*/
luamp_decode(L, luaL_msgpack_default, &data);
- data = lua_dump_plain(L, size);
+ data = console_dump_plain(L, size);
if (data == NULL) {
*result = NULL;
return 0;
@@ -480,7 +480,7 @@ port_msgpack_dump_plain(struct port *base, uint32_t *size)
lua_pushlightuserdata(L, &result);
lua_pushlightuserdata(L, base);
lua_pushlightuserdata(L, size);
- lua_pushcclosure(L, lua_port_msgpack_dump_plain, 3);
+ lua_pushcclosure(L, port_msgpack_dump_plain_via_lua, 3);
if (lua_pcall(L, 0, 0, 0) != 0) {
/*
* Error string is pushed in case it was a Lua
====================
> 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.
This is because it is not a lua_CFunction. It is not called
anywhere via lua_call/pcall/cpcall. But I got the point.
>> {
>> - 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.
Fixed above.
>> +}
>> +
>> +/**
>> + * 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.
Fixed above.
>> +{
>> + 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).
It will be unwinded, but only in case we got to the call. What if one
of lua_pushlightuserdata() would throw an OOM? Nothing will be removed,
because this function (port_dump_plain) does not work on tarantool_L. It
may be a pure C stack not intersecting with Lua anyhow. We use tarantool_L
as some kind of external buffer and a last resort to reach Lua objects from
pure C context.
So if a throw happens before lua_pcall(), we will leave something on
tarantool_L. Right?
> 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>.
It may not manage to get to it and fail during pushing the argument.
> ================================================================================
>
>> + 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.
Now we are talking :) Seems like this thing will protect from any
leaks 99%, right? (Except in case of a fail it may be not able to
push error object on the stack, is it possible?).
====================
diff --git a/src/box/call.c b/src/box/call.c
index 811c44c65..599df7f60 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -90,7 +90,7 @@ port_msgpack_destroy(struct port *base)
free(port->plain);
}
-char *
+int
port_mspack_set_plain(struct port *base, const char *plain, uint32_t len)
{
struct port_msgpack *port = (struct port_msgpack *)base;
@@ -98,11 +98,11 @@ port_mspack_set_plain(struct port *base, const char *plain, uint32_t len)
port->plain = (char *)malloc(len + 1);
if (port->plain == NULL) {
diag_set(OutOfMemory, len + 1, "malloc", "port->plain");
- return NULL;
+ return -1;
}
memcpy(port->plain, plain, len);
port->plain[len] = 0;
- return port->plain;
+ return 0;
}
static const struct port_vtab port_msgpack_vtab = {
diff --git a/src/box/lua/console.c b/src/box/lua/console.c
index 886120eac..ff2db5832 100644
--- a/src/box/lua/console.c
+++ b/src/box/lua/console.c
@@ -448,11 +448,15 @@ port_lua_dump_plain(struct port *base, uint32_t *size)
static int
port_msgpack_dump_plain_via_lua(struct lua_State *L)
{
- 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));
+ void **ctx = (void **)lua_touserdata(L, 1);
+ struct port_msgpack *port = (struct port_msgpack *)ctx[0];
+ uint32_t *size = (uint32_t *)ctx[1];
const char *data = port->data;
+ /*
+ * Need to pop, because YAML decoder will consume all what
+ * can be found on the stack.
+ */
+ lua_pop(L, 1);
/*
* MessagePack -> Lua object -> YAML/Lua text. The middle
* is not really needed here, but there is no
@@ -461,11 +465,10 @@ port_msgpack_dump_plain_via_lua(struct lua_State *L)
*/
luamp_decode(L, luaL_msgpack_default, &data);
data = console_dump_plain(L, size);
- if (data == NULL) {
- *result = NULL;
- return 0;
- }
- *result = port_mspack_set_plain((struct port *)port, data, *size);
+ if (data == NULL)
+ assert(port->plain == NULL);
+ else
+ port_mspack_set_plain((struct port *)port, data, *size);
return 0;
}
@@ -474,14 +477,13 @@ const char *
port_msgpack_dump_plain(struct port *base, uint32_t *size)
{
struct lua_State *L = tarantool_L;
- 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, port_msgpack_dump_plain_via_lua, 3);
- if (lua_pcall(L, 0, 0, 0) != 0) {
+ void *ctx[2] = {(void *)base, (void *)size};
+ /*
+ * lua_cpcall() protects from errors thrown from Lua which
+ * may break a caller, not knowing about Lua and not
+ * expecting any exceptions.
+ */
+ if (lua_cpcall(L, port_msgpack_dump_plain_via_lua, ctx) != 0) {
/*
* Error string is pushed in case it was a Lua
* error.
@@ -489,11 +491,9 @@ 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));
lua_pop(L, 1);
- assert(lua_gettop(L) == top);
return NULL;
}
- assert(lua_gettop(L) == top);
- return result;
+ return ((struct port_msgpack *)base)->plain;
}
/**
diff --git a/src/box/port.h b/src/box/port.h
index 7ef5a2c63..32d13f589 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -107,7 +107,7 @@ port_msgpack_destroy(struct port *base);
/**
* Set plain text version of data in the given port. It is copied.
*/
-char *
+int
port_mspack_set_plain(struct port *base, const char *plain, uint32_t len);
/** Port for storing the result of a Lua CALL/EVAL. */
====================
>> + /*
>> + * 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;
>> +}
>> 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.
That is a bug, I should restore it.
====================
diff --git a/test/box/push.result b/test/box/push.result
index f5d8fe563..23b420420 100644
--- a/test/box/push.result
+++ b/test/box/push.result
@@ -696,3 +696,6 @@ box.schema.user.revoke('guest', 'super')
box.schema.func.drop('function1.test_push')
---
...
+package.cpath = old_cpath
+---
+...
diff --git a/test/box/push.test.lua b/test/box/push.test.lua
index d2dc31db6..ffeeee3e4 100644
--- a/test/box/push.test.lua
+++ b/test/box/push.test.lua
@@ -315,3 +315,5 @@ server:close()
box.schema.user.revoke('guest', 'super')
box.schema.func.drop('function1.test_push')
+
+package.cpath = old_cpath
====================
>> +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'
Cool, thanks for the hint:
====================
diff --git a/test/box/push.result b/test/box/push.result
index 23b420420..7888e4d92 100644
--- a/test/box/push.result
+++ b/test/box/push.result
@@ -604,23 +604,8 @@ 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)
+s = '\x41\x00\x43'
---
...
console = require('console')
diff --git a/test/box/push.test.lua b/test/box/push.test.lua
index ffeeee3e4..f4518466e 100644
--- a/test/box/push.test.lua
+++ b/test/box/push.test.lua
@@ -282,16 +282,13 @@ c:call('function1.test_push', \
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)
+s = '\x41\x00\x43'
console = require('console')
fio = require('fio')
====================
More information about the Tarantool-patches
mailing list