Re: [tarantool-patches] Re: [PATCH v2 04/10] console: use Lua C API to do formatting for console

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

On 10/05/2018 21:46, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/04/20 16:25]:
>> YAML formatting C API is needed for #2677, where it will be used
>> to send text pushes and prints as tagged YAML documents.
> 
> I don't understand why you need to move dump/format into C
> from this comment.
> 
> What was wrong with keeping it in Lua implementation?

New commit message:

     console: use Lua C API to do formatting for console
     
     YAML formatting C API is needed for #2677, where it will be used
     to send text pushes and prints as tagged YAML documents.
     
     Push in the next patches is implemented as a virtual C method of
     struct session, so it is necessary to be able format YAML from C.
     
     Needed for #2677

> 
>>
>> Needed for #2677
>> ---
>>   src/box/lua/console.c         | 48 +++++++++++++++++++++++++++++++++++++++++++
>>   src/box/lua/console.lua       | 32 ++++++-----------------------
>>   third_party/lua-yaml/lyaml.cc | 34 ++++++++++++------------------
>>   third_party/lua-yaml/lyaml.h  | 29 ++++++++++++++++++++++++++
>>   4 files changed, 96 insertions(+), 47 deletions(-)
>>
>> diff --git a/src/box/lua/console.c b/src/box/lua/console.c
>> index d27d7ecac..f6009d387 100644
>> --- a/src/box/lua/console.c
>> +++ b/src/box/lua/console.c
>> @@ -34,6 +34,7 @@
>>   #include "lua/fiber.h"
>>   #include "fiber.h"
>>   #include "coio.h"
>> +#include "lua-yaml/lyaml.h"
>>   #include <lua.h>
>>   #include <lauxlib.h>
>>   #include <lualib.h>
>> @@ -42,6 +43,8 @@
>>   #include <stdlib.h>
>>   #include <ctype.h>
>>   
>> +static struct luaL_serializer *luaL_yaml_default = NULL;
>> +
>>   /*
>>    * Completion engine (Mike Paul's).
>>    * Used internally when collecting completions locally. Also a Lua
>> @@ -328,6 +331,32 @@ lbox_console_add_history(struct lua_State *L)
>>   	return 0;
>>   }
>>   
>> +/**
>> + * Encode Lua object into YAML documents. Gets variable count of
>> + * parameters.
>> + * @retval String with YAML documents - one per parameter.
>> + */
>> +static int
>> +lbox_console_format(struct lua_State *L)
>> +{
>> +	int arg_count = lua_gettop(L);
>> +	if (arg_count == 0) {
>> +		lua_pushstring(L, "---\n...\n");
>> +		return 1;
>> +	}
>> +	lua_createtable(L, arg_count, 0);
>> +	for (int i = 0; i < arg_count; ++i) {
>> +		if (lua_isnil(L, i + 1))
>> +			luaL_pushnull(L);
>> +		else
>> +			lua_pushvalue(L, i + 1);
>> +		lua_rawseti(L, -2, i + 1);
>> +	}
>> +	lua_replace(L, 1);
>> +	lua_settop(L, 1);
>> +	return lua_yaml_encode(L, luaL_yaml_default);
>> +}
>> +
>>   void
>>   tarantool_lua_console_init(struct lua_State *L)
>>   {
>> @@ -336,6 +365,7 @@ tarantool_lua_console_init(struct lua_State *L)
>>   		{"save_history",       lbox_console_save_history},
>>   		{"add_history",        lbox_console_add_history},
>>   		{"completion_handler", lbox_console_completion_handler},
>> +		{"format",             lbox_console_format},
>>   		{NULL, NULL}
>>   	};
>>   	luaL_register_module(L, "console", consolelib);
>> @@ -344,6 +374,24 @@ tarantool_lua_console_init(struct lua_State *L)
>>   	lua_getfield(L, -1, "completion_handler");
>>   	lua_pushcclosure(L, lbox_console_readline, 1);
>>   	lua_setfield(L, -2, "readline");
>> +
>> +	luaL_yaml_default = lua_yaml_new_serializer(L);
>> +	luaL_yaml_default->encode_invalid_numbers = 1;
>> +	luaL_yaml_default->encode_load_metatables = 1;
>> +	luaL_yaml_default->encode_use_tostring = 1;
>> +	luaL_yaml_default->encode_invalid_as_nil = 1;
>> +	/*
>> +	 * Hold reference to a formatter (the Lua representation
>> +	 * of luaL_yaml_default).
>> It is not visible to a user
>> +	 * here, because require('console') returns modified
>> +	 * package with no formatter. This problem is absent in
>> +	 * similar places like lua/msgpack.c, because they are
>> +	 * serializers and nothing more - they hold
>> +	 * luaL_serializer in LUA_REGISTRYINDEX. Console does the
>> +	 * same, but here a YAML serializer is just a part of
>> +	 * console.
>> +	 */
> 
> I don't understand the comment beyond the first sentence.

I commiserate. Maybe the new comment is more understandable.

diff --git a/src/box/lua/console.c b/src/box/lua/console.c
index a85a43dbd..ffd1347de 100644
--- a/src/box/lua/console.c
+++ b/src/box/lua/console.c
@@ -383,15 +383,12 @@ tarantool_lua_console_init(struct lua_State *L)
         luaL_yaml_default->encode_use_tostring = 1;
         luaL_yaml_default->encode_invalid_as_nil = 1;
         /*
-        * Hold reference to a formatter (the Lua representation
-        * of luaL_yaml_default). It is not visible to a user
-        * here, because require('console') returns modified
-        * package with no formatter. This problem is absent in
-        * similar places like lua/msgpack.c, because they are
-        * serializers and nothing more - they hold
-        * luaL_serializer in LUA_REGISTRYINDEX. Console does the
-        * same, but here a YAML serializer is just a part of
-        * console.
+        * Hold reference to the formatter in module local
+        * variable.
+        *
+        * This member is not visible to a user, because
+        * console.lua modifies itself, holding the formatter in
+        * module local variable. Add_history, save_history,
+        * load_history work the same way.
          */
         lua_setfield(L, -2, "formatter");
  }

> 
>> +	lua_setfield(L, -2, "formatter");
>>   }
>>   
>>   /*
>> +/**
>> + * Encode a Lua object or objects into YAML documents onto Lua
>> + * stack.
>> + * @param L Lua stack to get arguments and push result.
>> + * @param serializer Lua YAML serializer.
>> + * @param tag_handle NULL, or a global tag handle. Handle becomes
>> + *        a synonym for prefix.
>> + * @param tag_prefix NULL, or a global tag prefix, to which @a
>> + *        handle is expanded.
>> + * @retval nil, error Error.
>> + * @retval not nil Lua string with dumped object.
>> + */
>> +int
>> +lua_yaml_encode_tagged(lua_State *L, struct luaL_serializer *serializer,
>> +		       const char *tag_handle, const char *tag_prefix);
> 
> No need to keep the comments in two places. We have decided to
> keep the comments around declarations, not definitions, a while
> ago (please make sure to update the server engineering guidelines
> if it is not reflected there or in the coding style).

I do not keep comments in two places. This comment is removed from lyaml.cc.
And you could see it in the diff above. Anyway in the new patch based on previous
review fixes this function is new, and is not just exposed into the header file. So
the comment is not moved or duplicated.

See the entire diff below:

=======================================================================

commit 52dbf38e8657b21d3870297611e439125dd12235
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Apr 5 20:20:24 2018 +0300

     console: use Lua C API to do formatting for console
     
     YAML formatting C API is needed for #2677, where it will be used
     to send text pushes and prints as tagged YAML documents.
     
     Push in the next patches is implemented as a virtual C method of
     struct session, so it is necessary to be able format YAML from C.
     
     Needed for #2677

diff --git a/src/box/lua/console.c b/src/box/lua/console.c
index 7a5ac5550..31614b4a8 100644
--- a/src/box/lua/console.c
+++ b/src/box/lua/console.c
@@ -34,6 +34,7 @@
  #include "lua/fiber.h"
  #include "fiber.h"
  #include "coio.h"
+#include "lua-yaml/lyaml.h"
  #include <lua.h>
  #include <lauxlib.h>
  #include <lualib.h>
@@ -42,6 +43,8 @@
  #include <stdlib.h>
  #include <ctype.h>
  
+static struct luaL_serializer *luaL_yaml_default = NULL;
+
  /*
   * Completion engine (Mike Paul's).
   * Used internally when collecting completions locally. Also a Lua
@@ -329,6 +332,33 @@ lbox_console_add_history(struct lua_State *L)
  	return 0;
  }
  
+/**
+ * Encode Lua object into YAML documents. Gets variable count of
+ * parameters.
+ * @retval 1 Pushes string with YAML documents - one per
+ *         parameter.
+ */
+static int
+lbox_console_format(struct lua_State *L)
+{
+	int arg_count = lua_gettop(L);
+	if (arg_count == 0) {
+		lua_pushstring(L, "---\n...\n");
+		return 1;
+	}
+	lua_createtable(L, arg_count, 0);
+	for (int i = 0; i < arg_count; ++i) {
+		if (lua_isnil(L, i + 1))
+			luaL_pushnull(L);
+		else
+			lua_pushvalue(L, i + 1);
+		lua_rawseti(L, -2, i + 1);
+	}
+	lua_replace(L, 1);
+	lua_settop(L, 1);
+	return lua_yaml_encode(L, luaL_yaml_default);
+}
+
  void
  tarantool_lua_console_init(struct lua_State *L)
  {
@@ -337,6 +367,7 @@ tarantool_lua_console_init(struct lua_State *L)
  		{"save_history",       lbox_console_save_history},
  		{"add_history",        lbox_console_add_history},
  		{"completion_handler", lbox_console_completion_handler},
+		{"format",             lbox_console_format},
  		{NULL, NULL}
  	};
  	luaL_register_module(L, "console", consolelib);
@@ -345,6 +376,22 @@ tarantool_lua_console_init(struct lua_State *L)
  	lua_getfield(L, -1, "completion_handler");
  	lua_pushcclosure(L, lbox_console_readline, 1);
  	lua_setfield(L, -2, "readline");
+
+	luaL_yaml_default = lua_yaml_new_serializer(L);
+	luaL_yaml_default->encode_invalid_numbers = 1;
+	luaL_yaml_default->encode_load_metatables = 1;
+	luaL_yaml_default->encode_use_tostring = 1;
+	luaL_yaml_default->encode_invalid_as_nil = 1;
+	/*
+	 * Hold reference to the formatter in module local
+	 * variable.
+	 *
+	 * This member is not visible to a user, because
+	 * console.lua modifies itself, holding the formatter in
+	 * module local variable. Add_history, save_history,
+	 * load_history work the same way.
+	 */
+	lua_setfield(L, -2, "formatter");
  }
  
  /*
diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index f1a8f4915..bc4e02bfc 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -12,34 +12,11 @@ local net_box = require('net.box')
  
  local YAML_TERM = '\n...\n'
  
--- admin formatter must be able to encode any Lua variable
-local formatter = yaml.new()
-formatter.cfg{
-    encode_invalid_numbers = true;
-    encode_load_metatables = true;
-    encode_use_tostring    = true;
-    encode_invalid_as_nil  = true;
-}
-
  local function format(status, ...)
-    -- When storing a nil in a Lua table, there is no way to
-    -- distinguish nil value from no value. This is a trick to
-    -- make sure yaml converter correctly
-    local function wrapnull(v)
-        return v == nil and formatter.NULL or v
-    end
      local err
      if status then
-        local count = select('#', ...)
-        if count == 0 then
-            return "---\n...\n"
-        end
-        local res = {}
-        for i=1,count,1 do
-            table.insert(res, wrapnull(select(i, ...)))
-        end
          -- serializer can raise an exception
-        status, err = pcall(formatter.encode, res)
+        status, err = pcall(internal.format, ...)
          if status then
              return err
          else
@@ -47,9 +24,12 @@ local function format(status, ...)
                  tostring(err)
          end
      else
-        err = wrapnull(...)
+        err = ...
+        if err == nil then
+            err = box.NULL
+        end
      end
-    return formatter.encode({{error = err }})
+    return internal.format({ error = err })
  end
  
  --
diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 1544e08b4..c66afd817 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -807,6 +807,19 @@ error:
     }
  }
  
+int lua_yaml_encode_tagged(lua_State *L, struct luaL_serializer *serializer,
+                           const char *tag_handle, const char *tag_prefix)
+{
+   assert(tag_handle != NULL && tag_prefix != NULL);
+   assert(lua_gettop(L) == 1);
+   return lua_yaml_encode_impl(L, serializer, tag_handle, tag_prefix);
+}
+
+int lua_yaml_encode(lua_State *L, struct luaL_serializer *serializer)
+{
+   return lua_yaml_encode_impl(L, serializer, NULL, NULL);
+}
+
  /**
   * Dump Lua objects into YAML string. It takes any argument count,
   * and dumps each in a separate document.
@@ -845,7 +858,11 @@ usage_error:
  }
  
  static int
-l_new(lua_State *L);
+l_new(lua_State *L)
+{
+   lua_yaml_new_serializer(L);
+   return 1;
+}
  
  static const luaL_Reg yamllib[] = {
     { "encode", l_dump },
@@ -855,12 +872,12 @@ static const luaL_Reg yamllib[] = {
     { NULL, NULL}
  };
  
-static int
-l_new(lua_State *L)
+struct luaL_serializer *
+lua_yaml_new_serializer(lua_State *L)
  {
     struct luaL_serializer *s = luaL_newserializer(L, NULL, yamllib);
     s->has_compact = 1;
-   return 1;
+   return s;
  }
  
  int
diff --git a/third_party/lua-yaml/lyaml.h b/third_party/lua-yaml/lyaml.h
index 650a2d747..ac3cb8ca7 100644
--- a/third_party/lua-yaml/lyaml.h
+++ b/third_party/lua-yaml/lyaml.h
@@ -7,9 +7,37 @@ extern "C" {
  
  #include <lua.h>
  
+struct luaL_serializer;
+
  LUALIB_API int
  luaopen_yaml(lua_State *L);
  
+/** @Sa luaL_newserializer(). */
+struct luaL_serializer *
+lua_yaml_new_serializer(lua_State *L);
+
+/**
+ * Encode a Lua object into YAML document onto Lua stack.
+ * @param L Lua stack to get an argument and push result.
+ * @param serializer Lua YAML serializer.
+ * @param tag_handle NULL, or a global tag handle. Handle becomes
+ *        a synonym for prefix.
+ * @param tag_prefix NULL, or a global tag prefix, to which @a
+ *        handle is expanded.
+ * @retval nil, error Error.
+ * @retval not nil Lua string with dumped object.
+ */
+int
+lua_yaml_encode_tagged(lua_State *L, struct luaL_serializer *serializer,
+		       const char *tag_handle, const char *tag_prefix);
+
+/**
+ * Same as lua_yaml_encode_tagged, but encode with no tags and
+ * with multiple arguments.
+ */
+int
+lua_yaml_encode(lua_State *L, struct luaL_serializer *serializer);
+
  #ifdef __cplusplus
  }
  #endif