From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 6F1034696C3 for ; Tue, 7 Apr 2020 02:19:51 +0300 (MSK) Date: Tue, 7 Apr 2020 02:12:48 +0300 From: Igor Munkin Message-ID: <20200406231248.GB8215@tarantool.org> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5925f9e8-e148-9d68-eb39-db87e9e631eb@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Vlad, Thanks for the patch! Please consider the comments below. On 23.01.20, Vladislav Shpilevoy wrote: > > ================================================================================ > > Full patch: > > ================================================================================ > > commit 34c2d8033bcfabcab95a9aeec2ca029169969133 > Author: Vladislav Shpilevoy > Date: Mon Jan 20 23:08:58 2020 +0100 > > box: export box_session_push to the public C API > > API is different from box.session.push() - sync argument was > removed. It will disappear from Lua API as well, according to > a part of public API, and 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 returns a > YAML formatted text. But to turn MessagePack into YAML an > intermediate Lua representation is used, because there is no a > MessagePack -> YAML translator yet. > > Closes #4734 > > @TarantoolBot document > Title: box_session_push() C API > > There is a new function in the public C API: > > int > box_session_push(const char *data, const char *data_end); > > It takes raw MessagePack, and behaves just like Lua > box.session.push(). > > 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? > + 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 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 @@ -430,20 +430,12 @@ 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); - 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. */ + lua_State *L = tarantool_L; int top = lua_gettop(L); lua_pushcfunction(L, lua_port_msgpack_dump_plain); lua_pushlightuserdata(L, (void *) port->data); @@ -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; } assert(lua_gettop(L) - top == 1); assert(lua_isstring(L, -1)); @@ -480,15 +472,11 @@ port_msgpack_dump_plain(struct port *base, uint32_t *size) char *buffer = (char *) strdup(result); if (buffer == NULL) { diag_set(OutOfMemory, len + 1, "strdup", "buffer"); - goto error; + return NULL; } assert(port->buffer == NULL); port->buffer = buffer; - luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref); return buffer; -error: - luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref); - return NULL; } /** ================================================================================ > + 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. > + 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. > + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref); > + return buffer; > +error: > + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref); > + return NULL; > +} > + > /** > * Push a tagged YAML document into a console socket. > * @param session Console session. -- Best regards, IM