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