From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5FB8B4696C3 for ; Wed, 8 Apr 2020 23:35:59 +0300 (MSK) References: <20200408153339.GF5713@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9c602d2d-9963-b8f3-46a9-eacaf1a94278@tarantool.org> Date: Wed, 8 Apr 2020 22:35:57 +0200 MIME-Version: 1.0 In-Reply-To: <20200408153339.GF5713@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 1/1] box: export box_session_push to the public C API List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.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; >> } >> > > > >> + >> +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 , 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 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 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 . 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 returns nothing via guest stack, you > can use more convenient [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') ====================