From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 094324696C2 for ; Tue, 2 Jun 2020 10:51:45 +0300 (MSK) From: Oleg Babin References: <20200601222507.560415-1-gorcunov@gmail.com> <20200601222507.560415-9-gorcunov@gmail.com> Message-ID: <3b0ffbf3-6cc1-773e-3358-271fc5fdd530@tarantool.org> Date: Tue, 2 Jun 2020 10:51:40 +0300 MIME-Version: 1.0 In-Reply-To: <20200601222507.560415-9-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v7 08/11] lua/log: allow to configure logging without a box List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Hi! Thanks for your patch. Looks OK. But still I think about inconsistency I noticed in previous message (to 7th patch). May be I was wrong and some validation is missed here. On 02/06/2020 01:25, Cyrill Gorcunov wrote: > We bring in an ability to configure logger early without > calling box.cfg{}. For this sake log.cfg({}) becomes a > callable table. One can provide arguments of the looger > which later will be propagated back to box.cfg table on > demand. > > Another notable change is that the settings in log module > become defaults value, thus if one configured log before > the box then new settings get propagated to box's default > logging settings (previously they are silently reset to > defaults). > > Fixes #689 > > @TarantoolBot documnent > Title: Module log > > To configure log module eary without initializing box module > at all call the `log.cfg({})` method, where the following > arguments are acceptable: > > - `log` to specify file, pipe or syslog (same as > `box.cfg{log}` option); the default value is nil > and stderr is used; > > - `level` to specify logging level (1-7); the default > value is 5; > > - `format` to specify output encoding ('plain' or 'json'); > the default value is 'plain'; > > - `nonblock` to open logging output in nonblocking mode > (`true` or `false`); the default value is `false`. > > Signed-off-by: Cyrill Gorcunov > --- > src/lua/log.lua | 105 +++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 104 insertions(+), 1 deletion(-) > > diff --git a/src/lua/log.lua b/src/lua/log.lua > index 33e98fd84..dfdc326fe 100644 > --- a/src/lua/log.lua > +++ b/src/lua/log.lua > @@ -119,6 +119,27 @@ local log2box_keys = { > ['format'] = 'log_format', > } > > +local box_module_cfg = nil > + > +-- Reflect changes in box.cfg (if > +-- box.cfg been initialized). > +local function update_box_cfg(k) > + if box_module_cfg ~= nil then > + local update = function(k, v) > + if box_module_cfg[v] ~= log_cfg[k] then > + box_module_cfg[v] = log_cfg[k] > + end > + end > + if k ~= nil then > + update(k, log2box_keys[k]) > + else > + for k, v in pairs(log2box_keys) do > + update(k, v) > + end > + end > + end > +end > + > local function say(level, fmt, ...) > if ffi.C.log_level < level then > -- don't waste cycles on debug.getinfo() > @@ -170,6 +191,7 @@ end > local function log_level(level) > ffi.C.say_set_log_level(level) > rawset(log_cfg, 'level', level) > + update_box_cfg('level') > end > > local function log_format(name) > @@ -190,6 +212,7 @@ local function log_format(name) > ffi.C.say_set_log_format(ffi.C.SF_PLAIN) > end > rawset(log_cfg, 'format', name) > + update_box_cfg('format') > end > > local function log_pid() > @@ -208,10 +231,87 @@ end > -- Update own values from box interface. > local function box_on_load_cfg(box_cfg) > for k, v in pairs(log2box_keys) do > - if box_cfg[v] ~= log_cfg[k] then > + if box_cfg[v] ~= nil and box_cfg[v] ~= log_cfg[k] then > log_cfg[k] = box_cfg[v] > end > end > + -- Remember the backreference to box > + -- module config table. > + box_module_cfg = box_cfg > +end > + > +local params_once = { > + 'log', > + 'log_nonblock', > +} > + > +-- Setup dynamic parameters. > +local function dynamic_cfg(cfg) > + for _, k in pairs(params_once) do > + if cfg[k] ~= nil and cfg[k] ~= log_cfg[k] then > + local m = "log: '%s' can't be set dynamically" > + error(m:format(k)) > + end > + end > + > + if cfg.level ~= nil then > + log_level(cfg.level) > + end > + > + if cfg.format ~= nil then > + log_format(cfg.format) > + end > + > + -- The log_x helpers above update box > + -- settings on their own, so no need > + -- for more update_box_cfg calls. > +end > + > +-- Load or reload configuration via log.cfg({}) call. > +local function load_cfg(oldcfg, cfg) > + cfg = cfg or {} > + > + if cfg.format ~= nil then > + if fmt_str2num[cfg.format] == nil then > + local m = "log: 'format' must be %s" > + error(m:format(fmt_list())) > + end > + end > + > + if cfg.level ~= nil then > + if type(cfg.level) ~= 'number' then > + error("log: 'level' option must be a number") > + end > + end > + > + if cfg.nonblock ~= nil then > + if type(cfg.nonblock) ~= 'boolean' then > + error("log: 'nonblock' option must be 'true' or 'false'") > + end > + end > + > + if ffi.C.say_logger_initialized() == true then > + return dynamic_cfg(cfg) > + end > + > + cfg.level = cfg.level or log_cfg.level > + cfg.format = cfg.format or log_cfg.format > + cfg.nonblock = cfg.nonblock or (log_cfg.nonblock or false) > + > + -- We never allow confgure the logger in background > + -- mode since we don't know how the box will be configured > + -- later. > + ffi.C.say_logger_init(cfg.log, cfg.level, > + cfg.nonblock, cfg.format, 0) > + > + -- Update log_cfg vars to show them in module > + -- configuration output. > + rawset(log_cfg, 'log', cfg.log) > + rawset(log_cfg, 'level', cfg.level) > + rawset(log_cfg, 'nonblock', cfg.nonblock) > + rawset(log_cfg, 'format', cfg.format) > + > + update_box_cfg() > end > > local compat_warning_said = false > @@ -235,6 +335,9 @@ local log = { > pid = log_pid, > level = log_level, > log_format = log_format, > + cfg = setmetatable(log_cfg, { > + __call = load_cfg, > + }), > > -- Internal API to box module, not for users, > -- names can be changed. >