From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <lvasiliev@tarantool.org>
Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id D49F1469710
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 29 May 2020 11:43:17 +0300 (MSK)
References: <20200528100738.221911-1-gorcunov@gmail.com>
 <20200528100738.221911-7-gorcunov@gmail.com>
 <6ec85e2f-93a6-250d-63a9-a18c4caddb7d@tarantool.org>
 <20200528173423.GG215590@grain>
From: Leonid Vasiliev <lvasiliev@tarantool.org>
Message-ID: <8ca886d5-a281-c88b-66b9-bf67edb2517a@tarantool.org>
Date: Fri, 29 May 2020 11:43:16 +0300
MIME-Version: 1.0
In-Reply-To: <20200528173423.GG215590@grain>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v4 6/8] lua/log: use log module
 settings inside box.cfg
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: tml <tarantool-patches@dev.tarantool.org>

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.
>