From: "Maria Khaydich" <maria.khaydich@tarantool.org> To: "Alexander Turenko" <alexander.turenko@tarantool.org> Cc: tarantool-patches@dev.tarantool.org, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] box: make box.execute() immutable Date: Mon, 13 Jan 2020 15:13:40 +0300 [thread overview] Message-ID: <1578917620.195553363@f475.i.mail.ru> (raw) In-Reply-To: <20191226140834.urhg7ude4b5zkep4@tkn_work_nb> [-- Attachment #1: Type: text/plain, Size: 4502 bytes --] Thank you for the review. I’ve changed the comment describing box.execute method to explain why an extra flag is needed. And also added polishing fixes proposed by Igor. Result: Using box.execute method before explicitly configuring box automatically invoked box.cfg nonetheless. Any further calls to box.cfg caused its reconfiguration which then led to an error when trying to use execute method again. The patch introduces a fix making box.execute immutable. Closes #4231 --- Branch: https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function Issue: https://github.com/tarantool/tarantool/issues/4231 src/box/lua/load_cfg.lua | 15 ++++++++- .../gh-4231-immutable-box-execute.test.lua | 32 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) create mode 100755 test/box-tap/gh-4231-immutable-box-execute.test.lua diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua index 85617c8f0..1f9b4392f 100644 --- a/src/box/lua/load_cfg.lua +++ b/src/box/lua/load_cfg.lua @@ -517,10 +517,14 @@ setmetatable(box, { end }) +-- Flag needed to keep immutable functions after box reconfiguration +local box_loaded = false + local function load_cfg(cfg) cfg = upgrade_cfg(cfg, translate_cfg) cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg) apply_default_cfg(cfg, default_cfg); + box_loaded = true -- Save new box.cfg box.cfg = cfg if not pcall(private.cfg_check) then @@ -564,7 +568,16 @@ box.cfg = locked(load_cfg) -- metatable. -- function box.execute(...) - load_cfg() + -- + -- Saving box.execute method before explicit calls to either + -- box.execute() or box.cfg{} leads to implicit invocation of + -- box.cfg nonetheless. The flag box_loaded makes sure execute + -- is an immutable function meaning it won't be overwritten by + -- any following attempts to reconfigure box. + -- + if not box_loaded then + load_cfg() + end return box.execute(...) end diff --git a/test/box-tap/gh-4231-immutable-box-execute.test.lua b/test/box-tap/gh-4231-immutable-box-execute.test.lua new file mode 100755 index 000000000..d8940a0b2 --- /dev/null +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua @@ -0,0 +1,32 @@ +#!/usr/bin/env tarantool +local tap = require('tap') +local test = tap.test('execute') +test:plan(4) + +-- +-- gh-4231: box.execute should be immutable function meaning it's +-- address doesn't change after first box.cfg implicit invocation +-- + +local function execute_is_immutable(execute, cmd, msg) + local status, err = pcall(function() + execute(cmd) + end) + test:ok(status and err == nil, msg) +end + +local box_execute_stub = box.execute +test:is(box_execute_stub, box.execute, "execute is not the same before box.cfg") +execute_is_immutable(box_execute_stub, "SELECT 1", + "execute does not work properly before box.cfg") + +local box_execute_actual = box.execute +-- explicit call to load_cfg +box.cfg{} +-- checking the function was not reconfigured, i.e. adress stays the same +test:is(box_execute_actual, box.execute, "execute is not the same after box.cfg") +execute_is_immutable(box_execute_actual, + "CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));", + "execute does not work properly after box.cfg") + +os.exit(test:check() and 0 or 1) -- 2.24.0 >Четверг, 26 декабря 2019, 17:08 +03:00 от Alexander Turenko < alexander.turenko@tarantool.org >: > >> function box.execute(...) >> - load_cfg() >> + -- >> + -- Calling box.execute before explicitly configuring box led to >> + -- automatic box.cfg invocation nonetheless. Following explicit >> + -- attempts to configure box led to its reconfiguratin and as a >> + -- result - to an error when trying to use execute method again. >> + -- The check makes sure box.execute is an immutable function. >> + -- >I would describe all this hell with registering lbox_execute as >box.execute, moving it to box_cfg_guard_whitelist, adding this >box.execute function, then replacing back with lbox_execute at >load_cfg(). > >This description would make clear why everything is fine if we just call >box.execute(), but extra flag is necessary if a user saved box.execute >before first box.execute() or box.cfg() call. -- Maria Khaydich [-- Attachment #2: Type: text/html, Size: 7286 bytes --]
next prev parent reply other threads:[~2020-01-13 12:13 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-14 11:50 [Tarantool-patches] [PATCH] box.execute should be immutable function Maria 2019-11-14 16:51 ` Nikita Pettik 2019-12-17 14:39 ` Igor Munkin 2019-12-24 15:32 ` [Tarantool-patches] [PATCH] box: make box.execute() immutable Maria Khaydich 2019-12-25 1:30 ` Igor Munkin 2019-12-26 14:08 ` Alexander Turenko 2020-01-13 12:13 ` Maria Khaydich [this message] 2020-01-13 15:48 ` Igor Munkin 2020-01-18 10:56 ` Maria Khaydich 2020-02-20 17:51 ` Alexander Turenko 2020-02-20 21:15 ` Igor Munkin 2020-03-11 15:56 ` Maria Khaydich 2020-03-18 22:25 ` Igor Munkin 2020-05-02 14:52 ` Alexander Turenko 2020-05-12 16:16 ` Alexander Turenko 2020-03-11 15:57 ` [Tarantool-patches] [PATCH 1/2] box: make box.cfg idempotent function Maria Khaydich 2020-03-12 13:29 ` Konstantin Osipov 2020-03-12 19:25 ` Maria Khaydich 2020-03-12 20:00 ` Konstantin Osipov 2020-03-18 22:26 ` Igor Munkin 2020-03-19 7:19 ` Konstantin Osipov 2020-03-19 9:08 ` Igor Munkin 2020-03-19 10:06 ` Konstantin Osipov 2020-03-19 10:26 ` Igor Munkin 2020-05-06 11:17 ` Alexander Turenko 2020-05-06 11:49 ` Konstantin Osipov 2020-05-06 12:53 ` Alexander Turenko 2020-05-06 13:02 ` Konstantin Osipov 2020-05-06 13:13 ` Alexander Turenko 2020-03-18 22:26 ` Igor Munkin 2020-05-12 16:17 ` Alexander Turenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1578917620.195553363@f475.i.mail.ru \ --to=maria.khaydich@tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] box: make box.execute() immutable' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox