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!
next prev parent 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