[tarantool-patches] Re: [PATCH] Replace net.box usage with console in tarantoolctl eval
- From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
- To: tarantool-patches@xxxxxxxxxxxxx, Serge Petrenko <sergepetrenko@xxxxxxxxxxxxx>
- Date: Thu, 5 Jul 2018 14:29:36 +0300
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.
Other related posts:
