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 6114D4696C3 for ; Wed, 8 Apr 2020 02:20:58 +0300 (MSK) References: <1065f691482e681b779abd3698ec4489267c11fd.1579558507.git.v.shpilevoy@tarantool.org> <20200121002441.sysri34odzjrpjzu@tkn_work_nb> <20200122022537.tygaowsn5remjee4@tkn_work_nb> <5925f9e8-e148-9d68-eb39-db87e9e631eb@tarantool.org> <20200406231248.GB8215@tarantool.org> From: Vladislav Shpilevoy Message-ID: <88440e00-cda8-643e-8fa8-e79551bd7bdf@tarantool.org> Date: Wed, 8 Apr 2020 01:20:56 +0200 MIME-Version: 1.0 In-Reply-To: <20200406231248.GB8215@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 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/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 >> @@ -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? Tags are the standard YAML way of adding a type to an object. For details see lua_yaml_encode() comment, and http://yaml.org/spec/1.2/spec.html#tag/shorthand/. YAML allows to introduce your own tags, they just need to have a special syntax. For pushes we've chosen the tag you can see below. They can't be changed in future, since something may already depend on them. They are a part of the public API. Anyway, I now use the tag in only one place in v2 version, so no need to move it to a constant. >> + 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 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. 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 rewrote that part significantly in v2. > 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 > @@ -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; That is a bad idea, we still have garbage on tarantool_L. Need to drop it first. Anyway now I use cclosure, which automatically removes all leftovers from the stack. > } > assert(lua_gettop(L) - top == 1); > assert(lua_isstring(L, -1)); > ================================================================================ > >> + 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. YAML and Lua formatters escape 0 by writing '\0'. But anyway I agree, better use malloc + memcpy here. 'strndup' won't work, since it also stops when sees 0. Implemented in v2 and added a test. >> + 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. Agree, implemented in v2.