[tarantool-patches] Re: [PATCH] tarantoolctl: remove metatable assumptions in start()

[Roman Khabibov <roman.habibov@tarantool.org>]

Hello! Thanks for review.
> On Apr 1, 2019, at 8:43 AM, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> Commented in the issue, cited here:
> 
>> @opomuc @olegrok I don't know what are you trying to do here. It seems
>> you just try to start an instance w/o box.cfg() call inside. I don't
>> see a reason to support this case. However more meaningful error
>> message from tarantoolctl start in the case would helpful.
>> 
>> Re config module errors: I don't know whether you use it correctly. If
>> so, file another issue to this module.
>> 
>> https://github.com/tarantool/tarantool/issues/3953#issuecomment-478439448
> 
> I propose to consider this issue as a problem with an error message.
> 
> WBR, Alexander Turenko.
> 
> On Wed, Mar 27, 2019 at 04:40:14PM +0300, Roman Khabibov wrote:
>> box.cfg{} metatables are uninitialized when start() called
>> before box.cfg{}. It led to unexpected error.
>> 
>> Closes #3953
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3953-tctl
>> Issue: https://github.com/tarantool/tarantool/issues/3953
>> 
>> extra/dist/tarantoolctl.in         | 7 -------
>> test/app-tap/tarantoolctl.test.lua | 4 +++-
>> 2 files changed, 3 insertions(+), 8 deletions(-)
>> 
>> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
>> index 47fcf895f..91747e3ba 100755
>> --- a/extra/dist/tarantoolctl.in
>> +++ b/extra/dist/tarantoolctl.in
>> @@ -547,13 +547,6 @@ local function start()
>>         end
>>         os.exit(1)
>>     end
>> -    local old_call = getmetatable(box.cfg).__call
>> -    getmetatable(box.cfg).__call = function(old_cfg, cfg)
>> -        if old_cfg.pid_file ~= nil and cfg ~= nil and cfg.pid_file ~= nil then
>> -            cfg.pid_file = old_cfg.pid_file
> 
> So if a user lean on that value (s)he now cannot?
> 
>> -        end
>> -        old_call(old_cfg, cfg)
> 
> So if a user set a metatable on box.cfg() with __call method, it will
> not more called?
> 
>> -    end
>>     return 0
>> end
>> 
>> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
>> index db046e03f..1755fcc66 100755
>> --- a/test/app-tap/tarantoolctl.test.lua
>> +++ b/test/app-tap/tarantoolctl.test.lua
>> @@ -165,10 +165,12 @@ do
>>     local dir = fio.tempdir()
>>     local code = [[ box.cfg{memtx_memory = 104857600} ]]
>>     create_script(dir, 'script.lua', code)
>> +    create_script(dir, 'init.lua', [[ print('Hi!') ]])
>> 
>>     local status, err = pcall(function()
>>         test:test("basic test", function(test_i)
>> -            test_i:plan(16)
>> +            test_i:plan(18)
>> +            check_ok(test_i, dir, 'start', 'init.lua', 0, nil, "Starting instance init…")
Reworked.
+    local m_table = getmetatable(box.cfg)
+    if m_table == nil then
+        error("Plaese, call box.cfg{} within your instance")
+    end
+    local old_call = m_table.__call

> So we're 'successfully' started an instance, it executes a script and
> then 'tarantoolctl status' and 'tarantoolctl enter' don't show it as
> running. It does not look as a right behaviour.
> 
>>             check_ok(test_i, dir, 'start',  'script', 0, nil, "Starting instance")
>>             tctl_wait_start(dir, 'script')
>>             check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")
>> -- 
>> 2.20.1 (Apple Git-117)
check_ok(test_i, dir, 'status', 'script', 0, nil, "is running”) - I think it’s OK. Instance ’script’ have already been started, so ’tarantoolctl status script’ returns 0.

commit 4cdf8e79bb530288098fb30e5014498fc0ad4262
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Mon Mar 25 20:52:05 2019 +0300

    tarantoolctl: error when box.cfg didn't called
    
    There was assumpted uninitialized metamethods of box.cfg{} if user
    did't call it in his instance.
    
    Closes #3953

diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 47fcf895f..396fa7957 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -547,7 +547,11 @@ local function start()
         end
         os.exit(1)
     end
-    local old_call = getmetatable(box.cfg).__call
+    local m_table = getmetatable(box.cfg)
+    if m_table == nil then
+        error("Plaese, call box.cfg{} within your instance")
+    end
+    local old_call = m_table.__call
     getmetatable(box.cfg).__call = function(old_cfg, cfg)
         if old_cfg.pid_file ~= nil and cfg ~= nil and cfg.pid_file ~= nil then
             cfg.pid_file = old_cfg.pid_file
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index db046e03f..428b074e2 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -165,10 +165,13 @@ do
     local dir = fio.tempdir()
     local code = [[ box.cfg{memtx_memory = 104857600} ]]
     create_script(dir, 'script.lua', code)
+    create_script(dir, 'init.lua', [[ print('Hi!') ]])
 
     local status, err = pcall(function()
         test:test("basic test", function(test_i)
-            test_i:plan(16)
+            test_i:plan(18)
+            check_ok(test_i, dir, 'start', 'init.lua', 1, nil, "Starting instance init...",
+                     "Plaese, call box.cfg{} within your instance")
             check_ok(test_i, dir, 'start',  'script', 0, nil, "Starting instance")
             tctl_wait_start(dir, 'script')
             check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")