Hi! Thank you for the review! > On 10 Mar 2020, at 02:13, Vladislav Shpilevoy wrote: > > 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(). Done: +enum output_format { + OUTPUT_FORMAT_YAML = 0, + OUTPUT_FORMAT_LUA_LINE, + OUTPUT_FORMAT_LUA_BLOCK, +}; (Here I forgot about ‘opts’ in the first version, it is possible to \set output lua,block, for example) > >> + >> /* >> * 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. I added 2 following functions: +enum output_format +console_get_output_format() +void +console_set_output_format(enum output_format output_format) > >> +{ >> + 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. Fixed in v2. > >> + 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. + assert(fmt == OUTPUT_FORMAT_LUA_LINE || + fmt == OUTPUT_FORMAT_LUA_BLOCK); > >> + 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. Thanks for clarification, fixed. + lua_pcall(L, 1, 1, 0); > >> } >> - assert(rc == 1); >> - assert(lua_isstring(L, -1)); > > 6. Why did you remove that? Does it return not a string for Lua format? It does return a string, fixed. > >> 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. Perhaps, this could do + * Push a tagged YAML document or a Lua string into a console + * socket. > >> * @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. I’m sorry, I was following the style from schema.lua:1289 box.internal.check_space_arg = check_space_arg -- for net.box Fixed: +-- Used by console_session_push. +box_internal.format_lua_value = function(value) > >> + 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. I agree, my first version does not work as intended, thanks for the catch! However I didn’t come up with anything better than your suggestion, at least now it works. It could be a temporary workaround, perhaps? I will resend v2.