[tarantool-patches] Re: [PATCH] Replace net.box usage with console in tarantoolctl eval

[Sergey Petrenko]

Hi! Thank you for the review.


05.07.2018 14:29, Vladislav Shpilevoy пишет:

> Hello. Thanks for the patch!
>
> See 2 comments below.
>
> On 04/07/2018 12:59, Serge Petrenko wrote:
>> Net.box usage for console is deprecated in 1.10,
>> replaced it with console.
>>
>> Closes: #3490
>> ---
>> https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3490-tarantoolctl-replace-netbox 
>>
>> https://github.com/tarantool/tarantool/issues/3490
>>
>>   extra/dist/tarantoolctl.in | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
>> index 507ebe8bf..463d3e0a1 100755
>> --- a/extra/dist/tarantoolctl.in
>> +++ b/extra/dist/tarantoolctl.in
>> @@ -647,13 +647,29 @@ local function stdin_isatty()
>>   end
>>     local function execute_remote(uri, code)
>> -    local remote = netbox.connect(uri, {
>> -        console = true, connect_timeout = TIMEOUT_INFINITY
>> -    })
>> -    if remote == nil then
>> -        return nil
>> +    local err, ret
>> +    console.on_start(function(self)
>> +        local cmd = string.format(
>> +            "require('console').connect('%s', { connect_timeout = %s 
>> })",
>> +            uri, TIMEOUT_INFINITY
>> +        )
>> +        err = self:eval(cmd)
>
> 1. Please, explain why do we connect in such complicated way? Why not 
> just
>
>     console.connect(uri, {connect_timeout = TIMEOUT_INFINITY})
I copied this connection method from function enter(), because 
console.connect()
didn't work for me for some reason (probably did something wrong).
I like your diff better, and I will put more thought into copying other 
code from
now on. You can find the new diff below, I ran the tests and it seems ok.
>
> ? I can not understand why we execute it via 'self:eval'.
>
>> +        err = yaml.decode(err)[1]
>> +        if type(err) == 'table' and err.error then
>
> 2. Console eval on error does not return a table like {..., error = 
> ..., }.
> It returns 'format(false, errmsg)' - see remote_eval and local_eval in
> console.lua. So this check makes no sense. I see, that you copied it from
> eval() function, but it uses this strange check only when the eval itself
> was ok (eval checks for 'true' as first returned value from
> execute_remote()).
>
> And to be honest I do not understand why this check is used on a eval
> result, even before your patch. Looks like I can deceive this check by
> simply evaling this: 'return {error = "my any error"}'.
>
> I propose this diff:
>
> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
>     index 463d3e0a1..eae87acb3 100755
>     --- a/extra/dist/tarantoolctl.in
>     +++ b/extra/dist/tarantoolctl.in
>     @@ -646,30 +646,19 @@ local function stdin_isatty()
>          return ffi.C.isatty(0) == 1
>      end
>
>     -local function execute_remote(uri, code)
>     -    local err, ret
>     +function execute_remote(uri, code)
>     +    local status, ret
>          console.on_start(function(self)
>     -        local cmd = string.format(
>     -            "require('console').connect('%s', { connect_timeout = 
> %s })",
>     -            uri, TIMEOUT_INFINITY
>     -        )
>     -        err = self:eval(cmd)
>     -        err = yaml.decode(err)[1]
>     -        if type(err) == 'table' and err.error then
>     -            err = false
>     -        else
>     -            ret = self:eval(code)
>     +        status, ret = pcall(console.connect, uri,
>     +                            {connect_timeout = TIMEOUT_INFINITY})
>     +        if status then
>     +            status, ret = pcall(self.eval, self, code)
>              end
>              self.running = false
>          end)
>          console.on_client_disconnect(function(self) self.running = 
> false end)
>     -
>          console.start()
>     -
>     -    if not err then
>     -        return false
>     -    end
>     -    return true, ret
>     +    return status, ret
>      end
>
>      local function connect()
>
> I did not run the tests though except a pair of my scripts. So please, 
> look at
> this diff thoughtful, squash with your commit if it is ok, fix tests 
> if needed.
> Or explain, why this diff does not work and fix my remarks yourself.
diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 507ebe8bf..3b1c38585 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -647,13 +647,19 @@ local function stdin_isatty()
  end

  local function execute_remote(uri, code)
-    local remote = netbox.connect(uri, {
-        console = true, connect_timeout = TIMEOUT_INFINITY
-    })
-    if remote == nil then
-        return nil
-    end
-    return true, remote:eval(code)
+    local status, ret
+    console.on_start(function(self)
+        status, ret = pcall(console.connect, uri,
+                            {connect_timeout = TIMEOUT_INFINITY})
+        if status then
+            status, ret = pcall(self.eval, self, code)
+        end
+        self.running = false
+    end)
+    console.on_client_disconnect(function(self) self.running = false end)
+
+    console.start()
+    return status, ret
  end