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

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

[Serge Petrenko]

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)
+        err = yaml.decode(err)[1]
+        if type(err) == 'table' and err.error then
+            err = false
+        else
+            ret = self:eval(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, remote:eval(code)
+    return true, ret
 end
 
 local function connect()
-- 
2.15.2 (Apple Git-101.1)

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

[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 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.

[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

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

[Vladislav Shpilevoy]

Hello. Thanks for the fixes! See my comment below.

The patch LGTM now. Vova, please, make a second review.

>>> 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).

Console.connect works only when a console is started. See console.connect source.
Console.start creates internal console object and puts it into the fiber local
storage. Then this object is used by console.connect().

In 'tarantool>' interactive console you do not execute console.start because it
is started already, but in scripts you should start it explicitly, like that:

     console.start()
     console.on_start(do something with console and stop it)

> 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