* [PATCH v2] tarantoolctl: return an error on enter to a dead socket. @ 2018-08-21 6:56 Serge Petrenko 2018-08-21 12:28 ` Vladimir Davydov 0 siblings, 1 reply; 4+ messages in thread From: Serge Petrenko @ 2018-08-21 6:56 UTC (permalink / raw) To: vdavydov.dev; +Cc: kostja, tarantool-patches, Serge Petrenko Tarantoolctl enter didn't check whether connection to a socket was established if a socket file existed. It just executed a local console. Fix this by adding a check and an error, also add a test case. Closes #3364 --- https://github.com/tarantool/tarantool/issues/3364 https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3364-tarantoolctl-enter-fix Changes in v2: - alter testcase. - remove explicit deletion of unix socket from patch due to poor implementation. extra/dist/tarantoolctl.in | 19 +++++++++------ test/app-tap/tarantoolctl.test.lua | 48 +++++++++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in index 70747bd0c..6b6e78cd2 100755 --- a/extra/dist/tarantoolctl.in +++ b/extra/dist/tarantoolctl.in @@ -638,15 +638,20 @@ local function enter() end return 1 end - - local cmd = string.format( - "require('console').connect('%s', { connect_timeout = %s })", - console_sock, TIMEOUT_INFINITY - ) - - console.on_start(function(self) self:eval(cmd) end) + local status, ret + console.on_start(function(self) + status, ret = pcall(console.connect, console_sock, + {connect_timeout = TIMEOUT_INFINITY}) + if not status then + log.error("Can't connect to %s (%s)", console_sock_path, ret) + self.running = false + end + end) console.on_client_disconnect(function(self) self.running = false end) console.start() + if not status then + return 1 + end return 0 end diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua index ac9a208ca..c37c8f365 100755 --- a/test/app-tap/tarantoolctl.test.lua +++ b/test/app-tap/tarantoolctl.test.lua @@ -120,6 +120,14 @@ local function tctl_wait_stop(dir, name) end end +local function wait_delete(path) + if path then + while fio.path.exists(path) do + fiber.sleep(0.001) + end + end +end + local function tctl_command(dir, cmd, args, name) local pid = nil if not fio.stat(fio.pathjoin(dir, '.tarantoolctl')) then @@ -157,7 +165,7 @@ local function check_ok(test, dir, cmd, args, e_res, e_stdout, e_stderr) end local test = tap.test('tarantoolctl') -test:plan(6) +test:plan(7) -- basic start/stop test -- must be stopped afterwards @@ -258,6 +266,44 @@ do end end +-- check enter +do + local dir = fio.tempdir() + + local code = [[ box.cfg{} ]] + create_script(dir, 'script.lua', code) + + local status, err = pcall(function() + test:test("check error codes in case of enter", function(test_i) + test_i:plan(10) + check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to") + local console_sock = 'script.control' + console_sock = fio.pathjoin(dir, console_sock) + test_i:is(fio.path.exists(console_sock), false, "directory clean") + check_ok(test_i, dir, 'start', 'script', 0) + tctl_wait_start(dir, 'script') + test_i:is(fio.path.exists(console_sock), true, + "unix socket created") + check_ok(test_i, dir, 'stop', 'script', 0) + wait_delete(console_sock) + test_i:is(fio.path.exists(console_sock), false, + "remove unix socket upon exit") + fio.open(console_sock, 'O_CREAT') + test_i:is(fio.path.exists(console_sock), true, "file created") + check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to") + fio.unlink(console_sock) + end) + end) + + cleanup_instance(dir, 'script') + recursive_rmdir(dir) + + if status == false then + print(("Error: %s"):format(err)) + os.exit() + end +end + -- check basic help do local dir = fio.tempdir() -- 2.15.2 (Apple Git-101.1) ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] tarantoolctl: return an error on enter to a dead socket. 2018-08-21 6:56 [PATCH v2] tarantoolctl: return an error on enter to a dead socket Serge Petrenko @ 2018-08-21 12:28 ` Vladimir Davydov 2018-08-21 12:51 ` Serge Petrenko 0 siblings, 1 reply; 4+ messages in thread From: Vladimir Davydov @ 2018-08-21 12:28 UTC (permalink / raw) To: Serge Petrenko; +Cc: kostja, tarantool-patches On Tue, Aug 21, 2018 at 09:56:21AM +0300, Serge Petrenko wrote: > +-- check enter > +do > + local dir = fio.tempdir() > + > + local code = [[ box.cfg{} ]] > + create_script(dir, 'script.lua', code) > + > + local status, err = pcall(function() > + test:test("check error codes in case of enter", function(test_i) > + test_i:plan(10) > + check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to") > + local console_sock = 'script.control' > + console_sock = fio.pathjoin(dir, console_sock) > + test_i:is(fio.path.exists(console_sock), false, "directory clean") > + check_ok(test_i, dir, 'start', 'script', 0) > + tctl_wait_start(dir, 'script') > + test_i:is(fio.path.exists(console_sock), true, > + "unix socket created") > + check_ok(test_i, dir, 'stop', 'script', 0) > + wait_delete(console_sock) There's tctl_wait_stop() for this. Anyway, why do you need to start/stop instance before trying to enter it? AFAIU you just want to check that 'tarantoolctl enter' returns an error when called on a closed socket so the code below should be enough. > + test_i:is(fio.path.exists(console_sock), false, > + "remove unix socket upon exit") > + fio.open(console_sock, 'O_CREAT') > + test_i:is(fio.path.exists(console_sock), true, "file created") > + check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to") > + fio.unlink(console_sock) > + end) > + end) > + > + cleanup_instance(dir, 'script') > + recursive_rmdir(dir) > + > + if status == false then > + print(("Error: %s"):format(err)) > + os.exit() > + end > +end > + ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] tarantoolctl: return an error on enter to a dead socket. 2018-08-21 12:28 ` Vladimir Davydov @ 2018-08-21 12:51 ` Serge Petrenko 2018-08-21 13:26 ` Vladimir Davydov 0 siblings, 1 reply; 4+ messages in thread From: Serge Petrenko @ 2018-08-21 12:51 UTC (permalink / raw) To: Vladimir Davydov; +Cc: kostja, tarantool-patches > 21 авг. 2018 г., в 15:28, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а): > > On Tue, Aug 21, 2018 at 09:56:21AM +0300, Serge Petrenko wrote: >> +-- check enter >> +do >> + local dir = fio.tempdir() >> + >> + local code = [[ box.cfg{} ]] >> + create_script(dir, 'script.lua', code) >> + >> + local status, err = pcall(function() >> + test:test("check error codes in case of enter", function(test_i) >> + test_i:plan(10) >> + check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to") >> + local console_sock = 'script.control' >> + console_sock = fio.pathjoin(dir, console_sock) >> + test_i:is(fio.path.exists(console_sock), false, "directory clean") >> + check_ok(test_i, dir, 'start', 'script', 0) >> + tctl_wait_start(dir, 'script') >> + test_i:is(fio.path.exists(console_sock), true, >> + "unix socket created") >> + check_ok(test_i, dir, 'stop', 'script', 0) >> + wait_delete(console_sock) > > There's tctl_wait_stop() for this. Yes, fixed. New diff is below. > > Anyway, why do you need to start/stop instance before trying to enter > it? AFAIU you just want to check that 'tarantoolctl enter' returns an > error when called on a closed socket so the code below should be enough. I wanted to stress that I’m using a correct path to socket. Not just creating some Random file at some random place and then expecting `tarantoolctl enter` to fail. Also test that there is no socket after `tarantoolctl stop` was a request by Kostja For previous patch, but it is irrelevant now. We can remove it, if you want, but I believe it’s some extra coverage, nothing wrong with that. >> + test_i:is(fio.path.exists(console_sock), false, >> + "remove unix socket upon exit") >> + fio.open(console_sock, 'O_CREAT') >> + test_i:is(fio.path.exists(console_sock), true, "file created") >> + check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to") >> + fio.unlink(console_sock) >> + end) >> + end) >> + >> + cleanup_instance(dir, 'script') >> + recursive_rmdir(dir) >> + >> + if status == false then >> + print(("Error: %s"):format(err)) >> + os.exit() >> + end >> +end >> + extra/dist/tarantoolctl.in | 19 +++++++++++------- test/app-tap/tarantoolctl.test.lua | 40 +++++++++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in index 70747bd0c..6b6e78cd2 100755 --- a/extra/dist/tarantoolctl.in +++ b/extra/dist/tarantoolctl.in @@ -638,15 +638,20 @@ local function enter() end return 1 end - - local cmd = string.format( - "require('console').connect('%s', { connect_timeout = %s })", - console_sock, TIMEOUT_INFINITY - ) - - console.on_start(function(self) self:eval(cmd) end) + local status, ret + console.on_start(function(self) + status, ret = pcall(console.connect, console_sock, + {connect_timeout = TIMEOUT_INFINITY}) + if not status then + log.error("Can't connect to %s (%s)", console_sock_path, ret) + self.running = false + end + end) console.on_client_disconnect(function(self) self.running = false end) console.start() + if not status then + return 1 + end return 0 end diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua index ac9a208ca..64f2aa7e7 100755 --- a/test/app-tap/tarantoolctl.test.lua +++ b/test/app-tap/tarantoolctl.test.lua @@ -157,7 +157,7 @@ local function check_ok(test, dir, cmd, args, e_res, e_stdout, e_stderr) end local test = tap.test('tarantoolctl') -test:plan(6) +test:plan(7) -- basic start/stop test -- must be stopped afterwards @@ -258,6 +258,44 @@ do end end +-- check enter +do + local dir = fio.tempdir() + + local code = [[ box.cfg{} ]] + create_script(dir, 'script.lua', code) + + local status, err = pcall(function() + test:test("check error codes in case of enter", function(test_i) + test_i:plan(10) + check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to") + local console_sock = 'script.control' + console_sock = fio.pathjoin(dir, console_sock) + test_i:is(fio.path.exists(console_sock), false, "directory clean") + check_ok(test_i, dir, 'start', 'script', 0) + tctl_wait_start(dir, 'script') + test_i:is(fio.path.exists(console_sock), true, + "unix socket created") + check_ok(test_i, dir, 'stop', 'script', 0) + tctl_wait_stop(dir, 'script') + test_i:is(fio.path.exists(console_sock), false, + "remove unix socket upon exit") + fio.open(console_sock, 'O_CREAT') + test_i:is(fio.path.exists(console_sock), true, "file created") + check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to") + fio.unlink(console_sock) + end) + end) + + cleanup_instance(dir, 'script') + recursive_rmdir(dir) + + if status == false then + print(("Error: %s"):format(err)) + os.exit() + end +end + -- check basic help do local dir = fio.tempdir() -- 2.15.2 (Apple Git-101.1) ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] tarantoolctl: return an error on enter to a dead socket. 2018-08-21 12:51 ` Serge Petrenko @ 2018-08-21 13:26 ` Vladimir Davydov 0 siblings, 0 replies; 4+ messages in thread From: Vladimir Davydov @ 2018-08-21 13:26 UTC (permalink / raw) To: Serge Petrenko; +Cc: kostja, tarantool-patches On Tue, Aug 21, 2018 at 03:51:57PM +0300, Serge Petrenko wrote: > >> +-- check enter > >> +do > >> + local dir = fio.tempdir() > >> + > >> + local code = [[ box.cfg{} ]] > >> + create_script(dir, 'script.lua', code) > >> + > >> + local status, err = pcall(function() > >> + test:test("check error codes in case of enter", function(test_i) > >> + test_i:plan(10) > >> + check_ok(test_i, dir, 'enter', 'script', 1, nil, "Can't connect to") > >> + local console_sock = 'script.control' > >> + console_sock = fio.pathjoin(dir, console_sock) > >> + test_i:is(fio.path.exists(console_sock), false, "directory clean") > >> + check_ok(test_i, dir, 'start', 'script', 0) > >> + tctl_wait_start(dir, 'script') > >> + test_i:is(fio.path.exists(console_sock), true, > >> + "unix socket created") > >> + check_ok(test_i, dir, 'stop', 'script', 0) > >> + wait_delete(console_sock) > > > > There's tctl_wait_stop() for this. > > Yes, fixed. New diff is below. > > > > > Anyway, why do you need to start/stop instance before trying to enter > > it? AFAIU you just want to check that 'tarantoolctl enter' returns an > > error when called on a closed socket so the code below should be enough. > > I wanted to stress that I’m using a correct path to socket. Not just creating some > Random file at some random place and then expecting `tarantoolctl enter` to fail. > Also test that there is no socket after `tarantoolctl stop` was a request by Kostja > For previous patch, but it is irrelevant now. We can remove it, if you want, but > I believe it’s some extra coverage, nothing wrong with that. OK, pushed to 1.10. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-08-21 13:26 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-21 6:56 [PATCH v2] tarantoolctl: return an error on enter to a dead socket Serge Petrenko 2018-08-21 12:28 ` Vladimir Davydov 2018-08-21 12:51 ` Serge Petrenko 2018-08-21 13:26 ` Vladimir Davydov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox