From 5c24470ea96c7617432a390d634c0292dff92e07 Mon Sep 17 00:00:00 2001 From: NiLuJe Date: Thu, 6 Oct 2022 02:21:03 +0200 Subject: [PATCH] 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 | 2 +- frontend/dbg.lua | 42 +++----------------------- frontend/docsettings.lua | 1 - frontend/dump.lua | 13 ++------ frontend/fontlist.lua | 10 ++----- frontend/logger.lua | 17 +++++++++-- frontend/luadata.lua | 2 -- frontend/luadefaults.lua | 1 - frontend/luasettings.lua | 1 - frontend/persist.lua | 31 ++++++++++++++++--- frontend/ui/trapper.lua | 46 ++++++++++++++++------------- frontend/ui/widget/configdialog.lua | 6 ++-- frontend/ui/wikipedia.lua | 4 +-- frontend/util.lua | 6 ++++ reader.lua | 3 ++ spec/unit/persist_spec.lua | 36 ++++++++++++++++++---- 16 files changed, 121 insertions(+), 100 deletions(-) diff --git a/base b/base index 748466876..eda02706e 160000 --- a/base +++ b/base @@ -1 +1 @@ -Subproject commit 7484668769f8e72d3be62762501aacf0a9b22870 +Subproject commit eda02706e6152bec9a3de9706c7ccb6d80f148d2 diff --git a/frontend/dbg.lua b/frontend/dbg.lua index 404ca52b1..2fca467a0 100644 --- a/frontend/dbg.lua +++ b/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}(). diff --git a/frontend/docsettings.lua b/frontend/docsettings.lua index d4a777277..618e69374 100644 --- a/frontend/docsettings.lua +++ b/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 diff --git a/frontend/dump.lua b/frontend/dump.lua index 515e0a8c3..083ec20e1 100644 --- a/frontend/dump.lua +++ b/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 diff --git a/frontend/fontlist.lua b/frontend/fontlist.lua index 65f952fa9..b28f843a8 100644 --- a/frontend/fontlist.lua +++ b/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 diff --git a/frontend/logger.lua b/frontend/logger.lua index 49b94528f..f712e2acd 100644 --- a/frontend/logger.lua +++ b/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 diff --git a/frontend/luadata.lua b/frontend/luadata.lua index d9c3561ab..9bb6384fe 100644 --- a/frontend/luadata.lua +++ b/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)) diff --git a/frontend/luadefaults.lua b/frontend/luadefaults.lua index 08ae0fcf1..955ad3ec5 100644 --- a/frontend/luadefaults.lua +++ b/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") diff --git a/frontend/luasettings.lua b/frontend/luasettings.lua index f20d54962..5094f2391 100644 --- a/frontend/luasettings.lua +++ b/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") diff --git a/frontend/persist.lua b/frontend/persist.lua index fedac5937..d69038973 100644 --- a/frontend/persist.lua +++ b/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 diff --git a/frontend/ui/trapper.lua b/frontend/ui/trapper.lua index e23c529d2..6ee43386a 100644 --- a/frontend/ui/trapper.lua +++ b/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 diff --git a/frontend/ui/widget/configdialog.lua b/frontend/ui/widget/configdialog.lua index dcc80743f..953d81be2 100644 --- a/frontend/ui/widget/configdialog.lua +++ b/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 diff --git a/frontend/ui/wikipedia.lua b/frontend/ui/wikipedia.lua index 8de6d7424..c3eba1789 100644 --- a/frontend/ui/wikipedia.lua +++ b/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 diff --git a/frontend/util.lua b/frontend/util.lua index 706929776..f364862d1 100644 --- a/frontend/util.lua +++ b/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. diff --git a/reader.lua b/reader.lua index 8350e17e7..cd2a2f553 100755 --- a/reader.lua +++ b/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... diff --git a/spec/unit/persist_spec.lua b/spec/unit/persist_spec.lua index 0fc656a7c..c43085aaf 100644 --- a/spec/unit/persist_spec.lua +++ b/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