<HTML><BODY><div>
<div>Fixed minor comments.</div>
<div> </div>
<div>Sasha, you can proceed with the review. <br>
</div>
<blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Понедельник, 13 января 2020, 18:50 +03:00 от Igor Munkin <imun@tarantool.org>:<br>
<div id="">
<div class="js-helper js-readmsg-msg">
<style type="text/css">
</style>
<div>
<div id="style_15789306430622075400_BODY">Masha,<br>
<br>
Thanks, the patch LGTM, except a couple minor comments below. Please<br>
consider them.<br>
<br>
Sasha, please proceed with the patch.<br>
<br>
On 13.01.20, Maria Khaydich wrote:<br>
> Thank you for the review.<br>
><br>
> I’ve changed the comment describing box.execute method to explain why<br>
> an extra flag is needed.<br>
> And also added polishing fixes proposed by Igor.<br>
><br>
> Result:<br>
><br>
> Using box.execute method before explicitly configuring box<br>
> automatically invoked box.cfg nonetheless. Any further calls<br>
> to box.cfg caused its reconfiguration which then led to an<br>
> error when trying to use execute method again.<br>
><br>
> The patch introduces a fix making box.execute immutable.<br>
><br>
> Closes #4231<br>
> ---<br>
> Branch:<br>
> [1]<a href="https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.e" target="_blank">https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.e</a><br>
> xecute-immutable-function<br>
> Issue:<br>
> [2]<a href="https://github.com/tarantool/tarantool/issues/4231" target="_blank">https://github.com/tarantool/tarantool/issues/4231</a><br>
> src/box/lua/load_cfg.lua | 15 ++++++++-<br>
> .../gh-4231-immutable-box-execute.test.lua | 32 +++++++++++++++++++<br>
> 2 files changed, 46 insertions(+), 1 deletion(-)<br>
> create mode 100755 test/box-tap/gh-4231-immutable-box-execute.test.lua<br>
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua<br>
> index 85617c8f0..1f9b4392f 100644<br>
> --- a/src/box/lua/load_cfg.lua<br>
> +++ b/src/box/lua/load_cfg.lua<br>
> @@ -517,10 +517,14 @@ setmetatable(box, {<br>
> end<br>
> })<br>
><br>
> +-- Flag needed to keep immutable functions after box reconfiguration<br>
> +local box_loaded = false<br>
> +<br>
> local function load_cfg(cfg)<br>
> cfg = upgrade_cfg(cfg, translate_cfg)<br>
> cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)<br>
> apply_default_cfg(cfg, default_cfg);<br>
> + box_loaded = true<br>
> -- Save new box.cfg<br>
> box.cfg = cfg<br>
> if not pcall(private.cfg_check) then<br>
> @@ -564,7 +568,16 @@ box.cfg = locked(load_cfg)<br>
> -- metatable.<br>
> --<br>
> function box.execute(...)<br>
> - load_cfg()<br>
> + --<br>
> + -- Saving box.execute method before explicit calls to either<br>
> + -- box.execute() or box.cfg{} leads to implicit invocation of<br>
> + -- box.cfg nonetheless. The flag box_loaded makes sure execute<br>
> + -- is an immutable function meaning it won't be overwritten by<br>
> + -- any following attempts to reconfigure box.<br>
> + --<br>
> + if not box_loaded then<br>
> + load_cfg()<br>
> + end<br>
> return box.execute(...)<br>
> end<br>
><br>
> diff --git a/test/box-tap/gh-4231-immutable-box-execute.test.lua<br>
> b/test/box-tap/gh-4231-immutable-box-execute.test.lua<br>
> new file mode 100755<br>
> index 000000000..d8940a0b2<br>
> --- /dev/null<br>
> +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua<br>
> @@ -0,0 +1,32 @@<br>
> +#!/usr/bin/env tarantool<br>
> +local tap = require('tap')<br>
> +local test = tap.test('execute')<br>
> +test:plan(4)<br>
> +<br>
> +--<br>
> +-- gh-4231: box.execute should be immutable function meaning it's<br>
> +-- address doesn't change after first box.cfg implicit invocation<br>
> +--<br>
> +<br>
> +local function execute_is_immutable(execute, cmd, msg)<br>
> + local status, err = pcall(function()<br>
> + execute(cmd)<br>
> + end)<br>
> + test:ok(status and err == nil, msg)<br>
> +end<br>
> +<br>
> +local box_execute_stub = box.execute<br>
> +test:is(box_execute_stub, box.execute, "execute is not the same before<br>
> box.cfg")<br>
<br>
Minor: this test is a trivial and excess one. I suggest to drop it.<br>
<br>
> +execute_is_immutable(box_execute_stub, "SELECT 1",<br>
> + "execute does not work properly before box.cfg")<br>
> +<br>
> +local box_execute_actual = box.execute<br>
> +-- explicit call to load_cfg<br>
> +box.cfg{}<br>
> +-- checking the function was not reconfigured, i.e. adress stays the<br>
> same<br>
> +test:is(box_execute_actual, box.execute, "execute is not the same<br>
> after box.cfg")<br>
<br>
Minor: out of 80 chars<br>
<br>
> +execute_is_immutable(box_execute_actual,<br>
> + "CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));",<br>
> + "execute does not work properly after box.cfg")<br>
> +<br>
> +os.exit(test:check() and 0 or 1)<br>
> --<br>
> 2.24.0<br>
><br>
> Четверг, 26 декабря 2019, 17:08 +03:00 от Alexander Turenko<br>
> <<a href="/compose?To=alexander.turenko@tarantool.org">alexander.turenko@tarantool.org</a>>:<br>
><br>
> > function box.execute(...)<br>
> > - load_cfg()<br>
> > + --<br>
> > + -- Calling box.execute before explicitly configuring box led to<br>
> > + -- automatic box.cfg invocation nonetheless. Following explicit<br>
> > + -- attempts to configure box led to its reconfiguratin and as a<br>
> > + -- result - to an error when trying to use execute method<br>
> again.<br>
> > + -- The check makes sure box.execute is an immutable function.<br>
> > + --<br>
> I would describe all this hell with registering lbox_execute as<br>
> box.execute, moving it to box_cfg_guard_whitelist, adding this<br>
> box.execute function, then replacing back with lbox_execute at<br>
> load_cfg().<br>
> This description would make clear why everything is fine if we just<br>
> call<br>
> box.execute(), but extra flag is necessary if a user saved box.execute<br>
> before first box.execute() or box.cfg() call.<br>
><br>
><br>
><br>
> --<br>
> Maria Khaydich<br>
><br>
> References<br>
><br>
> 1. <a href="https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function" target="_blank">https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function</a><br>
> 2. <a href="https://github.com/tarantool/tarantool/issues/4231" target="_blank">https://github.com/tarantool/tarantool/issues/4231</a><br>
<br>
--<br>
Best regards,<br>
IM</div>
</div>
</div>
</div>
</blockquote>
<div> </div>
<div data-signature-widget="container">
<div data-signature-widget="content">
<div>--<br>
Maria Khaydich</div>
</div>
</div>
<div> </div>
</div>
</BODY></HTML>