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

[Cyrill Gorcunov]

On Wed, Jul 01, 2020 at 01:01:43PM +0300, Oleg Babin wrote:
> 
> 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
> 
> ```

File a bug please. Lets be clear -- our config parsing code is
far from being user friendly, hopefully we will make it more
convenient with time. I'm pretty sure that using lua language
for parsing config params was wrong from the very beginning
(especially when we start to allow config procedure via
modules and to sync data between box config and module config
we've to provide some internal "api", this is utter crap but
imposible to fix without remake of the whole config engine.
I expect we will do so eventually).

> 
> 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:'''
> ...
> 
> ```

Yup.

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

The key moment is that Lua treats box.NULL in a special way.

tarantool> a={log=box.NULL}
---
...

tarantool> a
---
- log: null
...

tarantool> a={log=nil}
---
...

tarantool> a
---
- []
...


IOW, when you set up some key to nil it gets vanished.
But when configuration fails I need to restore log=nil
if it were so (because setting cfg entries doesn't go
in one pass but instead one by one). Thus imagine we
set up log="syslog:" and made some mistake in format
key, I need to walk over backed up values and setup
them back.

> 
> 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
> > +    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(...).

Indeed, thanks! I didn't know about this helper. Will update.

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

Sounds reasonable, I'll take a look, thanks!