From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3E8ED26C5B for ; Thu, 5 Jul 2018 07:29:39 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 3-7yYeb_DCE5 for ; Thu, 5 Jul 2018 07:29:39 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id EA82E26C4F for ; Thu, 5 Jul 2018 07:29:38 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] Replace net.box usage with console in tarantoolctl eval References: <20180704095902.58607-1-sergepetrenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 5 Jul 2018 14:29:36 +0300 MIME-Version: 1.0 In-Reply-To: <20180704095902.58607-1-sergepetrenko@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Serge Petrenko 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.