From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 6D995452566 for ; Sun, 17 Nov 2019 21:57:22 +0300 (MSK) Date: Sun, 17 Nov 2019 21:56:25 +0300 From: Igor Munkin Message-ID: <20191117185625.GH31677@tarantool.org> References: <20191107140314.92871-1-arkholga@tarantool.org> <20191107140314.92871-3-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191107140314.92871-3-arkholga@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/2] box: raise on raw modifications of box.cfg values List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia Cc: tarantool-patches@dev.tarantool.org Olya, Thanks for the patch, it looks similar to the one, proposed by Mons some time ago. However, I look forward to the patchset to be made within tarantool gh-4521 to see whether it can be applied here. Nevertheless, I left several comments below, please consider them. On 07.11.19, Olga Arkhangelskaia wrote: > Prior this patch there was a possibility to change values throw > the raw table modification, like box.cfg.log_level = , or > box.cfg["log_level"] = . Now we store cfg values in separate table, > how ever the behaviour stays the same. > > Closes #2867 > --- > src/box/lua/load_cfg.lua | 18 +++++++++++-- > test/box-tap/cfg.test.lua | 14 +++++++++- > test/box/cfg.result | 55 +++++++++++++++++++++++++++++++++++++++ > test/box/cfg.test.lua | 3 +++ > 4 files changed, 87 insertions(+), 3 deletions(-) > > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > index 85617c8f0..602bf92c5 100644 > --- a/src/box/lua/load_cfg.lua > +++ b/src/box/lua/load_cfg.lua > @@ -533,13 +533,27 @@ local function load_cfg(cfg) > end > setmetatable(box, nil) > box_configured = nil > - box.cfg = setmetatable(cfg, > + box.cfg = {} This assignment is an excess one, box.cfg is initialized with a new empty table few lines below via setmetatable. > + local actual = cfg > + box.cfg = setmetatable({}, > { > __newindex = function(table, index) > error('Attempt to modify a read-only table') > end, > __call = locked(reload_cfg), > - }) > + __index = function (self, k) > + return actual[k] > + end, > + __serialize = function() return actual end, > + > + __pairs = function(self) > + local function iter(actual, k) > + local v This variable is an excess one. > + return next(actual, k) > + end > + return iter, actual, nil > + end > + }) > private.cfg_load() > for key, fun in pairs(dynamic_cfg) do > local val = cfg[key] > diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua > index b61073438..b76f28893 100755 > --- a/test/box-tap/cfg.test.lua > +++ b/test/box-tap/cfg.test.lua > @@ -6,7 +6,7 @@ local socket = require('socket') > local fio = require('fio') > local uuid = require('uuid') > local msgpack = require('msgpack') > -test:plan(104) > +test:plan(106) > > -------------------------------------------------------------------------------- > -- Invalid values > @@ -592,6 +592,18 @@ box.cfg{read_only=true} > ]] > test:is(run_script(code), PANIC, "panic on bootstrapping a read-only instance as master") > > +-- > +-- gf-2867 raise on raw modifications of box.cfg values > +-- > +code = [[ > +box.cfg{} > +box.cfg.read_only = true > +]] > +test:is(run_script(code), PANIC, "attempt to modify a read-only table") > > +code = [[ > +box.cfg["read_only"] = true > +]] > +test:is(run_script(code), PANIC, "attempt to modify a read-only table") This test is exactly the same as the prior one see Lua reference manual[1]: | The syntax var.Name is just syntactic sugar for var["Name"] > test:check() > os.exit(0) > diff --git a/test/box/cfg.result b/test/box/cfg.result > index 5370bb870..9031e1480 100644 > --- a/test/box/cfg.result > +++ b/test/box/cfg.result > @@ -236,6 +236,61 @@ box.cfg{replication = {}} > | --- > | ... > > +cfg = {} > + | --- > + | ... > +for k, v in pairs(box.cfg) do table.insert(cfg, k) end > + | --- > + | ... > +cfg > + | --- > + | - - pid_file > + | - checkpoint_count > + | - feedback_host > + | - readahead > + | - log_level > + | - checkpoint_interval > + | - vinyl_page_size > + | - coredump > + | - replication_sync_lag > + | - replication_timeout > + | - wal_dir_rescan_delay > + | - vinyl_memory > + | - replication_skip_conflict > + | - strip_core > + | - feedback_enabled > + | - wal_max_size > + | - too_long_threshold > + | - memtx_memory > + | - log > + | - background > + | - vinyl_dir > + | - vinyl_cache > + | - vinyl_read_threads > + | - vinyl_bloom_fpr > + | - vinyl_timeout > + | - net_msg_max > + | - listen > + | - force_recovery > + | - vinyl_max_tuple_size > + | - vinyl_run_count_per_level > + | - hot_standby > + | - log_format > + | - memtx_max_tuple_size > + | - memtx_min_tuple_size > + | - feedback_interval > + | - vinyl_write_threads > + | - wal_mode > + | - worker_pool_threads > + | - vinyl_run_size_ratio > + | - read_only > + | - memtx_dir > + | - wal_dir > + | - slab_alloc_factor > + | - checkpoint_wal_threshold > + | - replication_connect_timeout > + | - replication_sync_timeout > + | ... > -------------------------------------------------------------------------------- > -- Test of hierarchical cfg type check > -------------------------------------------------------------------------------- > diff --git a/test/box/cfg.test.lua b/test/box/cfg.test.lua > index 56ccb6767..275737e7a 100644 > --- a/test/box/cfg.test.lua > +++ b/test/box/cfg.test.lua > @@ -19,6 +19,9 @@ box.cfg{coredump = 'true'} > box.cfg{replication = {}} > box.cfg{replication = {}} > > +cfg = {} > +for k, v in pairs(box.cfg) do table.insert(cfg, k) end > +cfg > -------------------------------------------------------------------------------- > -- Test of hierarchical cfg type check > -------------------------------------------------------------------------------- > -- > 2.20.1 (Apple Git-117) > [1]: https://www.lua.org/manual/5.1/manual.html#2.3 -- Best regards, IM