Re: [Tarantool-patches] [PATCH 2/2] box: raise on raw modifications of box.cfg values

[Igor Munkin <imun@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