Re: [Tarantool-patches] [PATCH v2 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/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')
====================