From: Sergey Petrenko <sergepetrenko@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH] Replace net.box usage with console in tarantoolctl eval Date: Fri, 6 Jul 2018 16:02:23 +0300 [thread overview] Message-ID: <31c3604f-5ca6-23fb-039b-b08382fa4c8a@tarantool.org> (raw) In-Reply-To: <a50154e4-22b0-c650-ac30-3c05d3a6730e@tarantool.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
next prev parent reply other threads:[~2018-07-06 13:02 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-04 9:59 [tarantool-patches] " Serge Petrenko 2018-07-05 11:29 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-06 13:02 ` Sergey Petrenko [this message] 2018-07-09 9:53 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=31c3604f-5ca6-23fb-039b-b08382fa4c8a@tarantool.org \ --to=sergepetrenko@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH] Replace net.box usage with console in tarantoolctl eval' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox