[Tarantool-patches] [PATCH 2/3] lua/log: allow to use json formatter early
Oleg Babin
olegrok at tarantool.org
Wed Jul 1 13:01:43 MSK 2020
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<gorcunov at gmail.com>
> ---
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))
More information about the Tarantool-patches
mailing list