[Tarantool-patches] [PATCH v4 6/8] lua/log: use log module settings inside box.cfg

[Leonid Vasiliev]

Hi!

On 28.05.2020 20:34, Cyrill Gorcunov wrote:
> On Thu, May 28, 2020 at 08:07:02PM +0300, lvasiliev wrote:
>>> +local function apply_default_modules_cfg(cfg)
>>
>> Looks like apply_default_cfg(cfg, log.box_api.cfg).
>> Can we avoid code duplication?
> 
> The problem is keys naming. The module may have own mapping.
> For example in my latest version we use variables without
> log_ prefix but for box module sake we provide a map
> 
> log.lua
> =======
> 
> -- Name mapping from box to log module.
> -- Make sure all required fields are covered!
> local log2box_keys = {
>      ['log']         = 'log',
>      ['nonblock']    = 'log_nonblock',
>      ['level']       = 'log_level',
>      ['format']      = 'log_format',
> }
> 
> -- Apply defaut config to the box module
> local function box_cfg_apply_default(box_cfg)
>      for k, v in pairs(log_cfg) do
>          if box_cfg[log2box_keys[k]] == nil then
>              box_cfg[log2box_keys[k]] = v
>          end
>      end
> end
> 
> load_cfg.lua
> ============
> 
> -- Fetch default settings from modules.
> local function apply_default_modules_cfg(cfg)
>      log.box_api.cfg_apply_default(cfg)
> end
> 
> This isolate module specifics from box variables as
> it should be I believe.
> 

Ok.

>>
>>> +    --
>>> +    -- logging
>>> +    for k,v in pairs(log.box_api.cfg) do
>>
>> Add a space before v.
> 
> already reworked, thanks!
> 
>>> @@ -146,7 +162,8 @@ local function log_rotate()
>>>    end
>>>    local function log_level(level)
>>> -    return ffi.C.say_set_log_level(level)
>>> +    ffi.C.say_set_log_level(level)
>>> +    rawset(log_cfg, 'log_level', level)
>>
>> Maybe I'm wrong, but seems like after that change the ffi.C.log_level
>> is deprecated (log_cfg.log_level can be used instead). But now you
>> keep 'level' in two places (ffi.C.log_level and log_cfg.log_level).
> 
> Not sure I follow here. You mean to drop their usage in Lua's
> "function say()" in this module?
> 

Yes.

>>>    setmetatable(log, {
>>> +    __serialize = function(self)
>>
>> Can you add a test on __serizlize?
> 
> Good point, will do.
>