Re: [Tarantool-patches] [PATCH 2/3] lua/log: allow to use json formatter early

[Oleg Babin <olegrok@tarantool.org>]

Hi! Thanks for your patch see my four comments below.

On 30/06/2020 19:02, Cyrill Gorcunov wrote:
> There is no reason to not allow for json formatter
> on early logging stage.
>
> We add verification that
>
> 	box.cfg{log="syslog:", log_format="json"}
> or
> 	require('log').cfg{log="syslog:", format="json"}
>
> is triggering error since syslog output requires
> predefined structure and can't use json.
>
> Fixes #5121
>
> Signed-off-by: Cyrill Gorcunov<gorcunov@gmail.com>
> ---

May be it's not directly related to the patch but:

```

tarantool> log.cfg{log = ' syslog:identity=test', format = 'json'}
IllegalParams: expecting a file name or a prefix, such as '|', 'pipe:', 
'syslog:'
failed to initialize logging subsystem

```

It should be a simple error but not panic. I've just add a space in log 
parameter.

At the same time:

```

tarantool> box.cfg{log = ' syslog:'}
---
- error: 'Incorrect value for option ''log'': expecting a file name or a 
prefix, such
     as ''|'', ''pipe:'', ''syslog:'''
...

```

>   src/box/lua/load_cfg.lua |  6 +++---
>   src/lua/log.lua          | 43 ++++++++++++++++++++++++++++++----------
>   2 files changed, 36 insertions(+), 13 deletions(-)
>
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index f2f2df6f8..549c14cf3 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -459,13 +459,13 @@ local function prepare_cfg(cfg, default_cfg, template_cfg,
>                               module_cfg[k], modify_cfg[k], readable_name)
>           elseif template_cfg[k] == 'module' then
>               local old_value = module_cfg[k].cfg_get(k, v)
> -            module_cfg_backup[k] = old_value
> +            module_cfg_backup[k] = old_value or box.NULL
>   

Could you please clarify this change? I see below you check "value == 
box.NULL" but if value is simple nil then such condition is true.

```

tarantool> box.NULL == nil
---
- true
...

tarantool> box.NULL == box.NULL
---
- true
...

```

Why is simple "nil" not enough?

> -            local ok, msg = module_cfg[k].cfg_set(k, v)
> +            local ok, msg = module_cfg[k].cfg_set(cfg, k, v)
>               if not ok then
>                   -- restore back the old values for modules
>                   for module_k, module_v in pairs(module_cfg_backup) do
> -                    module_cfg[module_k].cfg_set(module_k, module_v)
> +                    module_cfg[module_k].cfg_set(nil, module_k, module_v)
>                   end
>                   box.error(box.error.CFG, readable_name, msg)
>               end
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index bed690526..44414bbd7 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -194,7 +194,7 @@ local function verify_static(k, v)
>   end
>   
>   -- Test if format is valid.
> -local function verify_format(key, name)
> +local function verify_format(key, name, init_str)
>       assert(log_cfg[key] ~= nil)
>   
>       if not fmt_str2num[name] then
> @@ -202,11 +202,20 @@ local function verify_format(key, name)
>           return false, m:format(fmt_list())
>       end
>   
> +    local log_type = ffi.C.log_type()
> +
> +    -- When comes from log.cfg{} or box.cfg{}
> +    -- initial call we might be asked to setup
> +    -- syslog with json which is not allowed.
> +    if init_str ~= nil then
> +        if string.sub(init_str, 1, 7) == "syslog:" then
> +            log_type = ffi.C.SAY_LOGGER_SYSLOG
> +        end
> +    end
> +

I believe that string.startswith(str, 'syslog:') is better here than 
string.sub(...).

>       if fmt_str2num[name] == ffi.C.SF_JSON then
> -        if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG or
> -            ffi.C.log_type() == ffi.C.SAY_LOGGER_BOOT then
> -            local m = "%s can't be used with " ..
> -            "syslog or boot-time logger"
> +        if log_type == ffi.C.SAY_LOGGER_SYSLOG then
> +            local m = "%s can't be used with syslog logger"
>               return false, m:format(fmt_num2str[ffi.C.SF_JSON])
>           end
>       end
> @@ -239,11 +248,11 @@ local verify_ops = {
>   }
>   
>   -- Verify a value for the particular key.
> -local function verify_option(k, v)
> +local function verify_option(k, v, ...)
>       assert(k ~= nil)
>   
>       if verify_ops[k] ~= nil then
> -        return verify_ops[k](k, v)
> +        return verify_ops[k](k, v, ...)
>       end
>   
>       return true
> @@ -372,10 +381,24 @@ local function box_api_cfg_get(key)
>   end
>   
>   -- Set value to log from box.cfg{}.
> -local function box_api_cfg_set(key, value)
> +local function box_api_cfg_set(cfg, key, value)
>       local log_key = box2log_keys[key]
> +    local aux_data
> +
> +    -- a special case where we need to restore
> +    -- nil value from previous setup attempt.
> +    if value == box.NULL then
> +        log_cfg[log_key] = nil
> +        return true
> +    end
> +
> +    -- 'format' option requires auxilary data
> +    -- for verification sake.
> +    if cfg ~= nil and log_key == 'format' then
> +        aux_data = cfg['log']
> +    end
>   

May be it's better to pass the whole "cfg" not single parameter to 
verify_ functions. I think it's more extendable.

Feel free to disagree with me, but I think that verify_format should 
extract "log" from configuration and consider them - it should not be 
done on top level.

> -    local ok, msg = verify_option(log_key, value)
> +    local ok, msg = verify_option(log_key, value, aux_data)
>       if not ok then
>           return false, msg
>       end
> @@ -450,7 +473,7 @@ local function load_cfg(oldcfg, cfg)
>       end
>   
>       if cfg.format ~= nil then
> -        local ok, msg = verify_option('format', cfg.format)
> +        local ok, msg = verify_option('format', cfg.format, cfg.log)
>           if not ok then
>               local m = "log.cfg: \'%s\' %s"
>               error(m:format('format', msg))