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 CBEB9469719 for ; Tue, 10 Mar 2020 02:13:43 +0300 (MSK) References: <20200306140334.22908-1-k.sosnin@tarantool.org> From: Vladislav Shpilevoy Message-ID: <64bfd4fa-7ea2-6168-2757-5af8aee2e816@tarantool.org> Date: Tue, 10 Mar 2020 00:13:42 +0100 MIME-Version: 1.0 In-Reply-To: <20200306140334.22908-1-k.sosnin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] box: fix formatting in session.push List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin , tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 9 comments below. On 06/03/2020 15:03, Chris Sosnin wrote: > box.session.push() encodes data as a YAML document independent on > the current console output format. This patch handles lua case in the > following way: > > tarantool>box.session.push() > > true; > > Closes #4686 > --- > issue: https://github.com/tarantool/tarantool/issues/4686 > branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4686-session-push-fmt > > src/box/lua/console.c | 62 ++++++++++++++++++++++++++--------- > src/box/lua/console.lua | 6 ++++ > test/app-tap/console.test.lua | 14 +++++++- > 3 files changed, 66 insertions(+), 16 deletions(-) > > diff --git a/src/box/lua/console.c b/src/box/lua/console.c > index 57e7e7f4f..a7573c306 100644 > --- a/src/box/lua/console.c > +++ b/src/box/lua/console.c > @@ -50,6 +50,11 @@ extern char serpent_lua[]; > > static struct luaL_serializer *luaL_yaml_default = NULL; > > +enum { > + OUTPUT_FORMAT_YAML, > + OUTPUT_FORMAT_LUA, > +}; 1. Please, give it a type and use it as a result of console_current_output(). > + > /* > * Completion engine (Mike Paul's). > * Used internally when collecting completions locally. Also a Lua > @@ -377,9 +382,28 @@ console_session_fd(struct session *session) > return session->meta.fd; > } > > +static int > +console_current_output(struct lua_State *L) 2. console_output_format() name would look better here, IMO. > +{ > + lua_getfield(L, LUA_GLOBALSINDEX, "box"); > + lua_getfield(L, -1, "session"); > + lua_getfield(L, -1, "storage"); > + lua_getfield(L, -1, "console_output_format"); > + if (lua_isnil(L, -1)) { > + lua_pop(L, 4); > + return OUTPUT_FORMAT_YAML; > + } > + lua_getfield(L, -1, "fmt"); 3. I think we should not use box.session.storage for internal information, such as output format. I would rather move it to session_meta in struct session in a separate preparatory patch, (or as a follow-up patch). box.session.storage is a documented table allowed to be used by users in any way. Some code easily may contain something like 'box.session.storage = {}', which will erase the format settings. > + int cmp = strcmp(lua_tostring(L, -1), "lua"); > + lua_pop(L, 5); > + if (cmp == 0) > + return OUTPUT_FORMAT_LUA; > + return OUTPUT_FORMAT_YAML; > +} > + > /** > - * Dump port lua data as a YAML document tagged with !push! global > - * tag. > + * Dump port lua data with respect to output format: > + * YAML document tagged with !push! global tag or lua string. > * @param port Port lua. > * @param[out] size Size of the result. > * > @@ -391,19 +415,27 @@ port_lua_dump_plain(struct port *port, uint32_t *size) > { > struct port_lua *port_lua = (struct port_lua *) port; > struct lua_State *L = port_lua->L; > - int rc = lua_yaml_encode(L, luaL_yaml_default, "!push!", > - "tag:tarantool.io/push,2018"); > - if (rc == 2) { > - /* > - * Nil and error object are pushed onto the stack. > - */ > - assert(lua_isnil(L, -2)); > + int fmt = console_current_output(L); > + if (fmt == OUTPUT_FORMAT_YAML) { > + int rc = lua_yaml_encode(L, luaL_yaml_default, "!push!", > + "tag:tarantool.io/push,2018"); > + if (rc == 2) { > + /* > + * Nil and error object are pushed onto the stack. > + */ > + assert(lua_isnil(L, -2)); > + assert(lua_isstring(L, -1)); > + diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1)); > + return NULL; > + } > + assert(rc == 1); > assert(lua_isstring(L, -1)); > - diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1)); > - return NULL; > + } else { /* OUTPUT_FORMAT_LUA */ 4. Better make it an assertion. That the format is 'Lua' here. > + luaL_findtable(L, LUA_GLOBALSINDEX, "box.internal", 1); > + lua_getfield(L, -1, "format_lua_value"); > + lua_pushvalue(L, -3); > + lua_call(L, 1, 1); 5. Should be pcall(). Otherwise any error will break C code, which called box_session_push(). lua_yaml_encode() is also unsafe, but it uses Lua deeply inside. On the contrary, this place is easy to protect. Also add a test case, please. Seems like serpent can fail on some input. At least, I see 2 error() calls in its sources. > } > - assert(rc == 1); > - assert(lua_isstring(L, -1)); 6. Why did you remove that? Does it return not a string for Lua format? > size_t len; > const char *result = lua_tolstring(L, -1, &len); > *size = (uint32_t) len; > @@ -411,10 +443,10 @@ port_lua_dump_plain(struct port *port, uint32_t *size) > } > > /** > - * Push a tagged YAML document into a console socket. > + * Push a tagged YAML document or a plain text into a console socket. 7. The line is out of 66 symbols now. YAML document is also plain text. The comment should be more specific, probably. > * @param session Console session. > * @param sync Unused request sync. > - * @param port Port with YAML to push. > + * @param port Port with the data to push. > * > * @retval 0 Success. > * @retval -1 Error. > diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua > index 17e2c91b2..cdf64b063 100644 > --- a/src/box/lua/console.lua > +++ b/src/box/lua/console.lua > @@ -12,6 +12,7 @@ local errno = require('errno') > local urilib = require('uri') > local yaml = require('yaml') > local net_box = require('net.box') > +local box_internal = require('box.internal') > > local PUSH_TAG_HANDLE = '!push!' > > @@ -96,6 +97,11 @@ local function format_lua_value(value, opts) > return serpent.line(value, serpent_opts) > end > > +box_internal.format_lua_value = function(value) -- for console_session_push 8. Please, - Don't write comments on the same lines as code; - Use single whitespace between words; - Use a capital letter in the beginning of sentence; - Use the dot in sentence's end. > + value = format_lua_value(value) > + return value .. '\n' > +end > + > output_handlers["lua"] = function(status, opts, ...) > local collect = {} > -- 9. Generally, I see that the patch prints pushes exactly like result of a function. I am not sure this is ok. For example, take YAML. The whole point in having YAML tags for YAML push output was in being able to separate pushes from a normal result and from each other. In that way pushes could be returned earlier than function returns. What is the whole purpose of pushes really. With the current Lua implementation I am not sure this is possible. Take this simple example. Tarantool1: require('console').listen(3313) Tarantool2: require('console').connect(3313) Now first case, when everything works fine: localhost:3313> box.session.push(100) require('fiber').sleep(100) %TAG !push! tag:tarantool.io/push,2018 --- 100 ... As you can see, the fiber sleeps. It didn't return anything yet. But the push is already executed and propagated to my console. Now do the same with Lua: localhost:3313> \set output lua true; localhost:3313> box.session.push(100) require('fiber').sleep(100) And no output. Push 100 should have been printed right away, but it is not. This is because console waits for ';' before printing anything when in Lua mode. So you need to design a way how to distinguish pushes from normal results, and print them earlier. But don't treat them as a final result. This was huge pain in the ass, when was being designed for YAML. I hope you will find something simple for Lua. For example, AFAIU, we could try to use Lua comments as some kind of hidden meta. When you send a push, you first sent a comment saying -- Push begin And in the end of the push you send -- Push end Or something like that. Also I would investigate whether Lua has some tag-like thing similar to what YAML has. I remember, we tried to use YAML comments instead of tags, but don't remember why we refused to use them.