Re: [Tarantool-patches] [PATCH v7 08/11] lua/log: allow to configure logging without a box

[Oleg Babin <olegrok@tarantool.org>]

Hi! Thanks for your patch. Looks OK. But still I think about 
inconsistency I noticed in previous message (to 7th patch). May be I was 
wrong and some validation is missed here.

On 02/06/2020 01:25, Cyrill Gorcunov wrote:
> We bring in an ability to configure logger early without
> calling box.cfg{}. For this sake log.cfg({}) becomes a
> callable table. One can provide arguments of the looger
> which later will be propagated back to box.cfg table on
> demand.
> 
> Another notable change is that the settings in log module
> become defaults value, thus if one configured log before
> the box then new settings get propagated to box's default
> logging settings (previously they are silently reset to
> defaults).
> 
> Fixes #689
> 
> @TarantoolBot documnent
> Title: Module log
> 
> To configure log module eary without initializing box module
> at all call the `log.cfg({})` method, where the following
> arguments are acceptable:
> 
>   - `log` to specify file, pipe or syslog (same as
>     `box.cfg{log}` option); the default value is nil
>     and stderr is used;
> 
>   - `level` to specify logging level (1-7); the default
>     value is 5;
> 
>   - `format` to specify output encoding ('plain' or 'json');
>     the default value is 'plain';
> 
>   - `nonblock` to open logging output in nonblocking mode
>     (`true` or `false`); the default value is `false`.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lua/log.lua | 105 +++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 104 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index 33e98fd84..dfdc326fe 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -119,6 +119,27 @@ local log2box_keys = {
>       ['format']      = 'log_format',
>   }
>   
> +local box_module_cfg = nil
> +
> +-- Reflect changes in box.cfg (if
> +-- box.cfg been initialized).
> +local function update_box_cfg(k)
> +    if box_module_cfg ~= nil then
> +        local update = function(k, v)
> +            if box_module_cfg[v] ~= log_cfg[k] then
> +                box_module_cfg[v] = log_cfg[k]
> +            end
> +        end
> +        if k ~= nil then
> +            update(k, log2box_keys[k])
> +        else
> +            for k, v in pairs(log2box_keys) do
> +                update(k, v)
> +            end
> +        end
> +    end
> +end
> +
>   local function say(level, fmt, ...)
>       if ffi.C.log_level < level then
>           -- don't waste cycles on debug.getinfo()
> @@ -170,6 +191,7 @@ end
>   local function log_level(level)
>       ffi.C.say_set_log_level(level)
>       rawset(log_cfg, 'level', level)
> +    update_box_cfg('level')
>   end
>   
>   local function log_format(name)
> @@ -190,6 +212,7 @@ local function log_format(name)
>           ffi.C.say_set_log_format(ffi.C.SF_PLAIN)
>       end
>       rawset(log_cfg, 'format', name)
> +    update_box_cfg('format')
>   end
>   
>   local function log_pid()
> @@ -208,10 +231,87 @@ end
>   -- Update own values from box interface.
>   local function box_on_load_cfg(box_cfg)
>       for k, v in pairs(log2box_keys) do
> -        if box_cfg[v] ~= log_cfg[k] then
> +        if box_cfg[v] ~= nil and box_cfg[v] ~= log_cfg[k] then
>               log_cfg[k] = box_cfg[v]
>           end
>       end
> +    -- Remember the backreference to box
> +    -- module config table.
> +    box_module_cfg = box_cfg
> +end
> +
> +local params_once = {
> +    'log',
> +    'log_nonblock',
> +}
> +
> +-- Setup dynamic parameters.
> +local function dynamic_cfg(cfg)
> +    for _, k in pairs(params_once) do
> +        if cfg[k] ~= nil and cfg[k] ~= log_cfg[k] then
> +            local m = "log: '%s' can't be set dynamically"
> +            error(m:format(k))
> +        end
> +    end
> +
> +    if cfg.level ~= nil then
> +        log_level(cfg.level)
> +    end
> +
> +    if cfg.format ~= nil then
> +        log_format(cfg.format)
> +    end
> +
> +    -- The log_x helpers above update box
> +    -- settings on their own, so no need
> +    -- for more update_box_cfg calls.
> +end
> +
> +-- Load or reload configuration via log.cfg({}) call.
> +local function load_cfg(oldcfg, cfg)
> +    cfg = cfg or {}
> +
> +    if cfg.format ~= nil then
> +        if fmt_str2num[cfg.format] == nil then
> +            local m = "log: 'format' must be %s"
> +            error(m:format(fmt_list()))
> +        end
> +    end
> +
> +    if cfg.level ~= nil then
> +        if type(cfg.level) ~= 'number' then
> +            error("log: 'level' option must be a number")
> +        end
> +    end
> +
> +    if cfg.nonblock ~= nil then
> +        if type(cfg.nonblock) ~= 'boolean' then
> +            error("log: 'nonblock' option must be 'true' or 'false'")
> +        end
> +    end
> +
> +    if ffi.C.say_logger_initialized() == true then
> +        return dynamic_cfg(cfg)
> +    end
> +
> +    cfg.level = cfg.level or log_cfg.level
> +    cfg.format = cfg.format or log_cfg.format
> +    cfg.nonblock = cfg.nonblock or (log_cfg.nonblock or false)
> +
> +    -- We never allow confgure the logger in background
> +    -- mode since we don't know how the box will be configured
> +    -- later.
> +    ffi.C.say_logger_init(cfg.log, cfg.level,
> +                          cfg.nonblock, cfg.format, 0)
> +
> +    -- Update log_cfg vars to show them in module
> +    -- configuration output.
> +    rawset(log_cfg, 'log', cfg.log)
> +    rawset(log_cfg, 'level', cfg.level)
> +    rawset(log_cfg, 'nonblock', cfg.nonblock)
> +    rawset(log_cfg, 'format', cfg.format)
> +
> +    update_box_cfg()
>   end
>   
>   local compat_warning_said = false
> @@ -235,6 +335,9 @@ local log = {
>       pid = log_pid,
>       level = log_level,
>       log_format = log_format,
> +    cfg = setmetatable(log_cfg, {
> +        __call = load_cfg,
> +    }),
>   
>       -- Internal API to box module, not for users,
>       -- names can be changed.
>