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 9A17D2719F for ; Fri, 6 Jul 2018 09:02:27 -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 dlldSLf8xc8K for ; Fri, 6 Jul 2018 09:02:27 -0400 (EDT) Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 ED09726C57 for ; Fri, 6 Jul 2018 09:02:26 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] Replace net.box usage with console in tarantoolctl eval References: <20180704095902.58607-1-sergepetrenko@tarantool.org> From: Sergey Petrenko Message-ID: <31c3604f-5ca6-23fb-039b-b08382fa4c8a@tarantool.org> Date: Fri, 6 Jul 2018 16:02:23 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: ru 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: Vladislav Shpilevoy , tarantool-patches@freelists.org 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