Logger: Use serpent instead of dump (#9588)

* Persist: support serpent, and use by default over dump (as we assume consistency > readability in Persist).
* Logger/Dbg: Use serpent instead of dump to dump tables (it's slightly more compact, honors __tostring, and will tag tables with their ref, which can come in handy when debugging).
* Dbg: Don't duplicate Logger's log function, just use it directly.
* Fontlist/ConfigDialog: Use serpent for the debug dump.
* Call `os.setlocale(C, "numeric")` on startup instead of peppering it around dump calls. It's process-wide, so it didn't make much sense.
* Trapper: Use LuaJIT's serde facilities instead of dump. They're more reliable in the face of funky input, much faster, and in this case, the data never makes it to human eyes, so a human-readable format didn't gain us anything.

base

@@ -1 +1 @@
-Subproject commit 7484668769f8e72d3be62762501aacf0a9b22870
+Subproject commit eda02706e6152bec9a3de9706c7ccb6d80f148d2

frontend/dbg.lua

@@ -19,8 +19,7 @@ These functions don't do anything when debugging is turned off.
 --]]--
 
 local logger = require("logger")
-local dump = require("dump")
-local isAndroid, android = pcall(require, "android")
+local util = require("util")
 
 local Dbg = {
     -- set to nil so first debug:turnOff call won't be skipped
@@ -30,40 +29,7 @@ local Dbg = {
 
 local Dbg_mt = {}
 
-local LvDEBUG
-if isAndroid then
-    LvDEBUG = function(...)
-        local line = {}
-        for _, v in ipairs({...}) do
-            if type(v) == "table" then
-                table.insert(line, dump(v, math.huge))
-            else
-                table.insert(line, tostring(v))
-            end
-        end
-        return android.LOGV(table.concat(line, " "))
-    end
-else
-    LvDEBUG = function(...)
-        local line = {
-            os.date("%x-%X DEBUG"),
-        }
-        for _, v in ipairs({...}) do
-            if type(v) == "table" then
-                table.insert(line, dump(v, math.huge))
-            else
-                table.insert(line, tostring(v))
-            end
-        end
-        table.insert(line, "\n")
-        return io.write(table.concat(line, " "))
-    end
-end
-
---- Helper function to help dealing with nils in Dbg:guard...
-local function pack_values(...)
-    return select("#", ...), {...}
-end
+local LvDEBUG = logger.LvDEBUG
 
 --- Turn on debug mode.
 -- This should only be used in tests and at the user's request.
@@ -80,11 +46,11 @@ function Dbg:turnOn()
             if pre_guard then
                 pre_guard(...)
             end
-            local n, values = pack_values(old_method(...))
+            local values = util.table_pack(old_method(...))
             if post_guard then
                 post_guard(...)
             end
-            return unpack(values, 1, n)
+            return unpack(values, 1, values.n)
         end
     end
     --- Use this instead of a regular Lua @{assert}().

frontend/docsettings.lua

@@ -188,7 +188,6 @@ function DocSettings:flush()
     end
     self:ensureSidecar(self.sidecar)
     local s_out = dump(self.data)
-    os.setlocale('C', 'numeric')
     for _, f in ipairs(serials) do
         local directory_updated = false
         if lfs.attributes(f, "mode") == "file" then

frontend/dump.lua

@@ -1,10 +1,9 @@
 --[[--
 A simple serialization function which won't do uservalues, functions, or loops.
 
-If we ever need a more full-featured variant, the consensus seems to be https://github.com/pkulchenko/serpent ;).
+If you need a more full-featured variant, serpent is available in ffi/serpent ;).
 ]]
 
-local isUbuntuTouch = os.getenv("UBUNTU_APPLICATION_ISOLATION") ~= nil
 local insert = table.insert
 local indent_prefix = "    "
 
@@ -49,15 +48,7 @@ local function _serialize(what, outt, indent, max_lv, history, pairs_func)
     elseif datatype == "string" then
         insert(outt, string.format("%q", what))
     elseif datatype == "number" then
-        if isUbuntuTouch then
-            --- @fixme The `SDL_CreateRenderer` function in Ubuntu touch somehow
-            -- use a strange locale that formats number like this: 1.10000000000000g+02
-            -- which cannot be recognized by loadfile after the number is dumped.
-            -- Here the workaround is to preserve enough precision in "%.13e" format.
-            insert(outt, string.format("%.13e", what))
-        else
-            insert(outt, tostring(what))
-        end
+        insert(outt, tostring(what))
     elseif datatype == "boolean" then
         insert(outt, tostring(what))
     elseif datatype == "function" then

frontend/fontlist.lua

@@ -219,15 +219,13 @@ function FontList:getFontList()
 end
 
 function FontList:dumpFontList()
-    local dump = require("dump")
+    local serpent = require("ffi/serpent")
 
     -- FontInfo
     local path = self.cachedir .. "/fontinfo_dump.lua"
     local f = io.open(path, "w")
     if f ~= nil then
-        os.setlocale('C', 'numeric')
-        f:write("return ")
-        f:write(dump(self.fontinfo, nil, true))
+        f:write(serpent.block(self.fontinfo, { indent = "  ", comment = false, nocode = true }))
         f:close()
     else
         return
@@ -237,9 +235,7 @@ function FontList:dumpFontList()
     path = self.cachedir .. "/fontlist_dump.lua"
     f = io.open(path, "w")
     if f ~= nil then
-        os.setlocale('C', 'numeric')
-        f:write("return ")
-        f:write(dump(self.fontlist, nil, true))
+        f:write(serpent.block(self.fontlist, { indent = "  ", comment = false, nocode = true }))
         f:close()
     else
         return

frontend/logger.lua

@@ -9,7 +9,7 @@ Example:
     logger.err("House is on fire!")
 ]]
 
-local dump = require("dump")
+local serpent = require("ffi/serpent")
 local isAndroid, android = pcall(require, "android")
 
 local DEFAULT_DUMP_LVL = 10
@@ -36,6 +36,12 @@ local LOG_PREFIX = {
 
 local noop = function() end
 
+local serpent_opts = {
+    maxlevel = DEFAULT_DUMP_LVL,
+    indent = "  ",
+    nocode = true,
+}
+
 local Logger = {
     levels = LOG_LVL,
 }
@@ -53,7 +59,7 @@ if isAndroid then
         local line = {}
         for _, v in ipairs({...}) do
             if type(v) == "table" then
-                table.insert(line, dump(v, DEFAULT_DUMP_LVL))
+                table.insert(line, serpent.block(v, serpent_opts))
             else
                 table.insert(line, tostring(v))
             end
@@ -68,7 +74,7 @@ else
         }
         for _, v in ipairs({...}) do
             if type(v) == "table" then
-                table.insert(line, dump(v, DEFAULT_DUMP_LVL))
+                table.insert(line, serpent.block(v, serpent_opts))
             else
                 table.insert(line, tostring(v))
             end
@@ -107,6 +113,11 @@ function Logger:setLevel(new_lvl)
     end
 end
 
+-- For dbg's sake
+function Logger.LvDEBUG(...)
+    return log("dbg", ...)
+end
+
 Logger:setLevel(LOG_LVL.info)
 
 return Logger

frontend/luadata.lua

@@ -140,7 +140,6 @@ function LuaData:append(data)
     if not self.file then return end
     local f_out = io.open(self.file, "a")
     if f_out ~= nil then
-        os.setlocale('C', 'numeric')
         -- NOTE: This is a function call, with a table as its single argument. Parentheses are elided.
         f_out:write(self.name.."Entry")
         f_out:write(dump(data))
@@ -177,7 +176,6 @@ function LuaData:flush()
     logger.dbg("LuaData: Write to", self.file)
     local f_out = io.open(self.file, "w")
     if f_out ~= nil then
-        os.setlocale('C', 'numeric')
         f_out:write("-- we can read Lua syntax here!\n")
         f_out:write(self.name.."Entry")
         f_out:write(dump(self.data))

frontend/luadefaults.lua

@@ -175,7 +175,6 @@ function LuaDefaults:flush()
     end
     local f_out = io.open(self.file, "w")
     if f_out ~= nil then
-        os.setlocale('C', 'numeric')
         f_out:write("-- we can read Lua syntax here!\nreturn ")
         f_out:write(dump(self.rw, nil, true))
         f_out:write("\n")

frontend/luasettings.lua

@@ -266,7 +266,6 @@ function LuaSettings:flush()
     end
     local f_out = io.open(self.file, "w")
     if f_out ~= nil then
-        os.setlocale('C', 'numeric')
         f_out:write("-- we can read Lua syntax here!\nreturn ")
         f_out:write(dump(self.data, nil, true))
         f_out:write("\n")

frontend/persist.lua

@@ -4,6 +4,7 @@ local dump = require("dump")
 local ffi = require("ffi")
 local lfs = require("libs/libkoreader-lfs")
 local logger = require("logger")
+local serpent = require("ffi/serpent")
 local zstd = require("ffi/zstd")
 
 local C = ffi.C
@@ -164,6 +165,29 @@ local codecs = {
             end
             return t()
         end,
+    },
+    -- serpent: human readable (-ish), more thorough than dump (in particular, supports serializing functions)
+    -- NOTE: if you want pretty printing, pass { sortkeys = true, compact = false, indent = "  " } to serpent's second arg.
+    serpent = {
+        id = "serpent",
+        reads_from_file = false,
+        writes_to_file = false,
+
+        serialize = function(t)
+            local ok, str = serpent.dump(t)
+            if not ok then
+                return nil, "cannot serialize " .. tostring(t) .. " (" .. str .. ")"
+            end
+            return str
+        end,
+
+        deserialize = function(str)
+            local ok, t = serpent.load(str)
+            if not ok then
+                return nil, "malformed serialized data (" .. t .. ")"
+            end
+            return t
+        end,
     }
 }
 
@@ -172,7 +196,7 @@ local Persist = {}
 function Persist:new(o)
     o = o or {}
     assert(type(o.path) == "string", "path is required")
-    o.codec = o.codec or "dump"
+    o.codec = o.codec or "serpent"
     setmetatable(o, self)
     self.__index = self
     return o
@@ -242,13 +266,12 @@ function Persist:delete()
 end
 
 function Persist.getCodec(name)
-    local fallback = codecs["dump"]
     for key, codec in pairs(codecs) do
-        if type(key) == "string" and key == name then
+        if key == name then
             return codec
         end
     end
-    return fallback
+    return codecs["serpent"]
 end
 
 return Persist

frontend/ui/trapper.lua

@@ -13,9 +13,10 @@ local ConfirmBox = require("ui/widget/confirmbox")
 local InfoMessage = require("ui/widget/infomessage")
 local TrapWidget = require("ui/widget/trapwidget")
 local UIManager = require("ui/uimanager")
-local dump = require("dump")
+local buffer = require("string.buffer")
 local ffiutil = require("ffi/util")
 local logger = require("logger")
+local util = require("util")
 local _ = require("gettext")
 
 local Trapper = {}
@@ -540,13 +541,13 @@ function Trapper:dismissableRunInSubprocess(task, trap_widget_or_string, task_re
     local check_num = 0
 
     local completed = false
-    local ret_values = nil
+    local ret_values
 
     local pid, parent_read_fd = ffiutil.runInSubProcess(function(pid, child_write_fd)
         local output_str = ""
         if task_returns_simple_string then
-            -- task is assumed to return only a string or nil, avoid
-            -- possibly expensive dump()/dofile()
+            -- task is assumed to return only a string or nil,
+            -- so avoid a possibly expensive ser/deser roundtrip.
             local result = task()
             if type(result) == "string" then
                 output_str = result
@@ -554,15 +555,16 @@ function Trapper:dismissableRunInSubprocess(task, trap_widget_or_string, task_re
                 logger.warn("returned value from task is not a string:", result)
             end
         else
-            -- task may return complex data structures, that we dump()
-            -- Note: be sure these data structures contain only classic types,
-            -- and no function (dofile() with fail if it meets
-            -- "function: 0x55949671c670"...)
-            -- task may also return multiple return values, so we
-            -- wrap them in a table (beware the { } construct may stop
-            -- at the first nil met)
-            local results = { task() }
-            output_str = "return "..dump(results).."\n"
+            -- task may return complex data structures, that we serialize.
+            -- NOTE: LuaJIT's serializer currently doesn't support:
+            --       functions, coroutines, non-numerical FFI cdata & full userdata.
+            local results = util.table_pack(task())
+            local ok, str = pcall(buffer.encode, results)
+            if not ok then
+                logger.warn("cannot serialize", tostring(results), "->", str)
+            else
+                output_str = str
+            end
         end
         ffiutil.writeToFD(child_write_fd, output_str, true)
     end, true) -- with_pipe = true
@@ -618,7 +620,7 @@ function Trapper:dismissableRunInSubprocess(task, trap_widget_or_string, task_re
             -- it may still be alive blocking on write() (if data exceeds
             -- the kernel pipe buffer)
             local subprocess_done = ffiutil.isSubProcessDone(pid)
-            local stuff_to_read = parent_read_fd and ffiutil.getNonBlockingReadSize(parent_read_fd) ~=0
+            local stuff_to_read = parent_read_fd and ffiutil.getNonBlockingReadSize(parent_read_fd) ~= 0
             logger.dbg("subprocess_done:", subprocess_done, " stuff_to_read:", stuff_to_read)
             if subprocess_done or stuff_to_read then
                 -- Subprocess is gone or nearly gone
@@ -626,13 +628,13 @@ function Trapper:dismissableRunInSubprocess(task, trap_widget_or_string, task_re
                 if stuff_to_read then
                     local ret_str = ffiutil.readAllFromFD(parent_read_fd)
                     if task_returns_simple_string then
-                        ret_values = { ret_str }
+                        ret_values = ret_str
                     else
-                        local ok, results = pcall(load(ret_str))
-                        if ok and results then
-                            ret_values = results
+                        local ok, t = pcall(buffer.decode, ret_str)
+                        if ok and t then
+                            ret_values = t
                         else
-                            logger.warn("load() failed:", results)
+                            logger.warn("malformed serialized data:", t)
                         end
                     end
                     if not subprocess_done then
@@ -670,7 +672,11 @@ function Trapper:dismissableRunInSubprocess(task, trap_widget_or_string, task_re
     end
     -- return what we got or not to our caller
     if ret_values then
-        return completed, unpack(ret_values)
+        if task_returns_simple_string then
+            return completed, ret_values
+        else
+            return completed, unpack(ret_values, 1, ret_values.n)
+        end
     end
     return completed
 end

frontend/ui/widget/configdialog.lua

@@ -27,8 +27,8 @@ local UIManager = require("ui/uimanager")
 local UnderlineContainer = require("ui/widget/container/underlinecontainer")
 local VerticalGroup = require("ui/widget/verticalgroup")
 local VerticalSpan = require("ui/widget/verticalspan")
-local dump = require("dump")
 local logger = require("logger")
+local serpent = require("ffi/serpent")
 local _ = require("gettext")
 local Screen = Device.screen
 local T = require("ffi/util").template
@@ -1398,7 +1398,7 @@ function ConfigDialog:onMakeDefault(name, name_text, values, labels, position)
     end
     -- generic fallback to support table values
     if type(display_value) == "table" then
-        display_value = dump(display_value)
+        display_value = serpent.block(display_value, { maxlevel = 6, indent = "  ", comment = false, nocode = true })
     end
 
     UIManager:show(ConfirmBox:new{
@@ -1434,7 +1434,7 @@ function ConfigDialog:onMakeFineTuneDefault(name, name_text, values, labels, dir
 ]]),
         current_value[1], current_value[2])
     elseif type(current_value) == "table" then
-        display_value = dump(current_value)
+        display_value = serpent.block(current_value, { maxlevel = 6, indent = "  ", comment = false, nocode = true })
     else
         display_value = current_value
     end

frontend/ui/wikipedia.lua

@@ -223,7 +223,7 @@ function Wikipedia:loadPage(text, lang, page_type, plain)
         error(content)
     end
 
-    if content ~= "" and string.sub(content, 1,1) == "{" then
+    if content ~= "" and string.sub(content, 1, 1) == "{" then
         local ok, result = pcall(JSON.decode, content)
         if ok and result then
             logger.dbg("wiki result json:", result)
@@ -395,7 +395,7 @@ local function image_load_bb_func(image, highres)
     logger.dbg("fetching", source)
     local Trapper = require("ui/trapper")
     -- We use dismissableRunInSubprocess with simple string return value to
-    -- avoid dump()/load() a long string of image bytes
+    -- avoid serialization/deserialization of a long string of image bytes
     local completed, data = Trapper:dismissableRunInSubprocess(function()
         local success, data = getUrlContent(source, timeout, maxtime)
         -- With simple string value, we're not able to return the failure

frontend/util.lua

@@ -1468,6 +1468,12 @@ local WrappedFunction_mt = {
     end,
 }
 
+--- Helper function to help dealing with nils in varargs, since we don't have table.pack w/o Lua 5.2 compat...
+--- Unpack w/ unpack(t, 1, t.n)
+function util.table_pack(...)
+    return { n = select("#", ...), ... }
+end
+
 --- Wrap (or replace) a table method with a custom method, in a revertable way.
 -- This allows you extend the features of an existing module by modifying its
 -- internal methods, and then revert them back to normal later if necessary.

reader.lua

@@ -9,6 +9,9 @@ void setlinebuf(struct _IO_FILE *);
 ]]
 C.setlinebuf(C.stdout)
 
+-- Enforce a reliable locale for numerical representations
+os.setlocale("C", "numeric")
+
 io.write([[
 ---------------------------------------------
                 launching...

spec/unit/persist_spec.lua

@@ -1,7 +1,7 @@
 describe("Persist module", function()
     local Persist
     local sample
-    local bitserInstance, luajitInstance, dumpInstance
+    local bitserInstance, luajitInstance, zstdInstance, dumpInstance, serpentInstance
     local ser, deser, str, tab
     local fail = { a = function() end, }
 
@@ -30,14 +30,18 @@ describe("Persist module", function()
         Persist = require("persist")
         bitserInstance = Persist:new{ path = "test.dat", codec = "bitser" }
         luajitInstance = Persist:new{ path = "testj.dat", codec = "luajit" }
-        dumpInstance = Persist:new { path = "test.txt", codec = "dump" }
+        zstdInstance = Persist:new{ path = "test.zst", codec = "zstd" }
+        dumpInstance = Persist:new{ path = "test.lua", codec = "dump" }
+        serpentInstance = Persist:new{ path = "tests.lua", codec = "serpent" }
         sample = arrayOf(1000)
     end)
 
     it("should save a table to file", function()
         assert.is_true(bitserInstance:save(sample))
         assert.is_true(luajitInstance:save(sample))
+        assert.is_true(zstdInstance:save(sample))
         assert.is_true(dumpInstance:save(sample))
+        assert.is_true(serpentInstance:save(sample))
     end)
 
     it("should generate a valid file", function()
@@ -48,26 +52,36 @@ describe("Persist module", function()
         assert.is_true(luajitInstance:exists())
         assert.is_true(luajitInstance:size() > 0)
         assert.is_true(type(luajitInstance:timestamp()) == "number")
+
+        assert.is_true(zstdInstance:exists())
+        assert.is_true(zstdInstance:size() > 0)
+        assert.is_true(type(zstdInstance:timestamp()) == "number")
     end)
 
     it("should load a table from file", function()
         assert.are.same(sample, bitserInstance:load())
         assert.are.same(sample, luajitInstance:load())
+        assert.are.same(sample, zstdInstance:load())
         assert.are.same(sample, dumpInstance:load())
+        assert.are.same(sample, serpentInstance:load())
     end)
 
     it("should delete the file", function()
         bitserInstance:delete()
         luajitInstance:delete()
+        zstdInstance:delete()
         dumpInstance:delete()
+        serpentInstance:delete()
         assert.is_nil(bitserInstance:exists())
         assert.is_nil(luajitInstance:exists())
+        assert.is_nil(zstdInstance:exists())
         assert.is_nil(dumpInstance:exists())
+        assert.is_nil(serpentInstance:exists())
     end)
 
     it("should return standalone serializers/deserializers", function()
         tab = sample
-        for _, codec in ipairs({"dump", "bitser", "luajit"}) do
+        for _, codec in ipairs({"dump", "serpent", "bitser", "luajit", "zstd"}) do
             assert.is_true(Persist.getCodec(codec).id == codec)
             ser = Persist.getCodec(codec).serialize
             deser = Persist.getCodec(codec).deserialize
@@ -78,7 +92,7 @@ describe("Persist module", function()
     end)
 
     it("should work with huge tables", function()
-        for _, codec in ipairs({"bitser", "luajit"}) do
+        for _, codec in ipairs({"bitser", "luajit", "zstd"}) do
             tab = arrayOf(100000)
             ser = Persist.getCodec(codec).serialize
             deser = Persist.getCodec(codec).deserialize
@@ -87,8 +101,18 @@ describe("Persist module", function()
         end
     end)
 
-    it ("should fail to serialize functions", function()
-        for _, codec in ipairs({"dump", "bitser", "luajit"}) do
+    it("should fail to serialize functions", function()
+        for _, codec in ipairs({"dump", "bitser", "luajit", "zstd"}) do
+            assert.is_true(Persist.getCodec(codec).id == codec)
+            ser = Persist.getCodec(codec).serialize
+            deser = Persist.getCodec(codec).deserialize
+            str = ser(fail)
+            assert.are_not.same(deser(str), fail)
+        end
+    end)
+
+    it("should successfully serialize functions", function()
+        for _, codec in ipairs({"serpent"}) do
             assert.is_true(Persist.getCodec(codec).id == codec)
             ser = Persist.getCodec(codec).serialize
             deser = Persist.getCodec(codec).deserialize