From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f196.google.com (mail-lj1-f196.google.com [209.85.208.196]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B807C42F4AD for ; Thu, 2 Jul 2020 10:27:58 +0300 (MSK) Received: by mail-lj1-f196.google.com with SMTP id s9so30449544ljm.11 for ; Thu, 02 Jul 2020 00:27:58 -0700 (PDT) Date: Thu, 2 Jul 2020 10:27:55 +0300 From: Cyrill Gorcunov Message-ID: <20200702072755.GE2256@grain> References: <20200630160209.279470-1-gorcunov@gmail.com> <20200630160209.279470-3-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: 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: Oleg Babin Cc: tml , yaroslav.dynnikov@gmail.com On Wed, Jul 01, 2020 at 01:01:43PM +0300, Oleg Babin wrote: >=20 > tarantool> log.cfg{log =3D ' syslog:identity=3Dtest', format =3D 'json'} > IllegalParams: expecting a file name or a prefix, such as '|', 'pipe:', > 'syslog:' > failed to initialize logging subsystem >=20 > ``` 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). >=20 > It should be a simple error but not panic. I've just add a space in log > parameter. >=20 > At the same time: >=20 > ``` >=20 > tarantool> box.cfg{log =3D ' syslog:'} > --- > - error: 'Incorrect value for option ''log'': expecting a file name or a > prefix, such > =A0=A0=A0 as ''|'', ''pipe:'', ''syslog:''' > ... >=20 > ``` Yup. > > @@ -459,13 +459,13 @@ local function prepare_cfg(cfg, default_cfg, temp= late_cfg, > > module_cfg[k], modify_cfg[k], readable_na= me) > > elseif template_cfg[k] =3D=3D 'module' then > > local old_value =3D module_cfg[k].cfg_get(k, v) > > - module_cfg_backup[k] =3D old_value > > + module_cfg_backup[k] =3D old_value or box.NULL >=20 > Could you please clarify this change? I see below you check "value =3D=3D > box.NULL" but if value is simple nil then such condition is true. >=20 > ``` The key moment is that Lua treats box.NULL in a special way. tarantool> a=3D{log=3Dbox.NULL} --- ... tarantool> a --- - log: null ... tarantool> a=3D{log=3Dnil} --- ... tarantool> a --- - [] ... IOW, when you set up some key to nil it gets vanished. But when configuration fails I need to restore log=3Dnil 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=3D"syslog:" and made some mistake in format key, I need to walk over backed up values and setup them back. >=20 > tarantool> box.NULL =3D=3D nil > --- > - true > ... >=20 > tarantool> box.NULL =3D=3D box.NULL > --- > - true > ... >=20 > ``` >=20 > Why is simple "nil" not enough? >=20 > > - local ok, msg =3D module_cfg[k].cfg_set(k, v) > > + local ok, msg =3D 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 =3D 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 ~=3D nil then > > + if string.sub(init_str, 1, 7) =3D=3D "syslog:" then > > + log_type =3D ffi.C.SAY_LOGGER_SYSLOG > > + end > > + end > > + >=20 > 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 ~=3D nil and log_key =3D=3D 'format' then > > + aux_data =3D cfg['log'] > > + end >=20 > May be it's better to pass the whole "cfg" not single parameter to verify_ > functions. I think it's more extendable. >=20 > Feel free to disagree with me, but I think that verify_format should extr= act > "log" from configuration and consider them - it should not be done on top > level. Sounds reasonable, I'll take a look, thanks!