From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (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 545DF4429E1 for ; Wed, 1 Jul 2020 13:01:45 +0300 (MSK) From: Oleg Babin References: <20200630160209.279470-1-gorcunov@gmail.com> <20200630160209.279470-3-gorcunov@gmail.com> Message-ID: Date: Wed, 1 Jul 2020 13:01:43 +0300 MIME-Version: 1.0 In-Reply-To: <20200630160209.279470-3-gorcunov@gmail.com> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 2/3] lua/log: allow to use json formatter early List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Cc: yaroslav.dynnikov@gmail.com 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 > --- 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))