From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Serge Petrenko <sergepetrenko@tarantool.org> Subject: [tarantool-patches] Re: [PATCH] Replace net.box usage with console in tarantoolctl eval Date: Thu, 5 Jul 2018 14:29:36 +0300 [thread overview] Message-ID: <a50154e4-22b0-c650-ac30-3c05d3a6730e@tarantool.org> (raw) In-Reply-To: <20180704095902.58607-1-sergepetrenko@tarantool.org> 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.
next prev parent reply other threads:[~2018-07-05 11:29 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 ` Vladislav Shpilevoy [this message] 2018-07-06 13:02 ` [tarantool-patches] " Sergey Petrenko 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=a50154e4-22b0-c650-ac30-3c05d3a6730e@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=sergepetrenko@tarantool.org \ --cc=tarantool-patches@freelists.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