Tarantool development patches archive
 help / color / mirror / Atom feed
From: Cyrill Gorcunov <gorcunov@gmail.com>
To: Oleg Babin <olegrok@tarantool.org>
Cc: tml <tarantool-patches@dev.tarantool.org>, yaroslav.dynnikov@gmail.com
Subject: Re: [Tarantool-patches] [PATCH 2/3] lua/log: allow to use json formatter early
Date: Thu, 2 Jul 2020 10:27:55 +0300	[thread overview]
Message-ID: <20200702072755.GE2256@grain> (raw)
In-Reply-To: <e1469b31-ac42-8a34-35bb-130c36276b00@tarantool.org>

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!

  reply	other threads:[~2020-07-02  7:27 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30 16:02 [Tarantool-patches] [PATCH v2 0/3] log: allow json formatter in boottime logger Cyrill Gorcunov
2020-06-30 16:02 ` [Tarantool-patches] [PATCH 1/3] core/say: allow to use json in boot logger Cyrill Gorcunov
2020-06-30 16:02 ` [Tarantool-patches] [PATCH 2/3] lua/log: allow to use json formatter early Cyrill Gorcunov
2020-07-01 10:01   ` Oleg Babin
2020-07-02  7:27     ` Cyrill Gorcunov [this message]
2020-07-02  7:35       ` Oleg Babin
2020-06-30 16:02 ` [Tarantool-patches] [PATCH 3/3] test: app-tap/logger -- test json in boottime logger Cyrill Gorcunov
  -- strict thread matches above, loose matches on Subject: below --
2020-06-29 11:23 [Tarantool-patches] [PATCH 0/3] log: allow json formatter " Cyrill Gorcunov
2020-06-29 11:23 ` [Tarantool-patches] [PATCH 2/3] lua/log: allow to use json formatter early Cyrill Gorcunov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200702072755.GE2256@grain \
    --to=gorcunov@gmail.com \
    --cc=olegrok@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=yaroslav.dynnikov@gmail.com \
    --subject='Re: [Tarantool-patches] [PATCH 2/3] lua/log: allow to use json formatter early' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox