[tarantool-patches] [PATCH v1 1/1] box: fix custom delimiter for telnet connection

[tarantool-patches] [PATCH v1 1/1] box: fix custom delimiter for telnet connection

[Kirill Shcherbatov]

Because test-run uses a console connection to run tests, the
actual string delimiter was the user-specified delimiter
delim + "\n".
Since telnet sends \r\n on line break, the updated expression
delim + "\n"could not be found in a sequence data+delim+"\r\n",
so delimiter feature did not work at all.
Added delim + "\r" check along with delim + "\n", that solves the
described problem and does not violate backward compatibility.

Closes #2027

https://github.com/tarantool/tarantool/tree/kshch/gh-2027-telnet-alternative-delimiter
https://github.com/tarantool/tarantool/issues/2027
---
 src/box/lua/console.lua | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 028001127..bdedcb393 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -344,8 +344,9 @@ end
 -- Read command from connected client console.listen()
 --
 local function client_read(self)
-    local delim = self.delimiter .. "\n"
-    local buf = self.client:read(delim)
+    local delim_lf = self.delimiter .. "\n"
+    local delim_cr = self.delimiter .. "\r"
+    local buf = self.client:read({delimiter = {delim_lf, delim_cr}})
     if buf == nil then
         return nil
     elseif buf == "" then
@@ -355,7 +356,7 @@ local function client_read(self)
         return nil
     end
     -- remove trailing delimiter
-    return buf:sub(1, -#delim-1)
+    return buf:sub(1, -#delim_lf-1)
 end
 
 --
-- 
2.20.1

[tarantool-patches] Re: [PATCH v1 1/1] box: fix custom delimiter for telnet connection

[Alexander Turenko]

This patch looks good for me, except several minor things.

Can we write a test for the new behaviour?

WBR, Alexander Turenko.

On Thu, Feb 21, 2019 at 10:34:55AM +0300, Kirill Shcherbatov wrote:
> Because test-run uses a console connection to run tests, the
> actual string delimiter was the user-specified delimiter
> delim + "\n".

'test-run uses a console connection' says nothing about why a newline is
added to a delimiter. I would say:

In order to give a user ability to use a delimiter symbol within a
code the real delimiter is user-provided 'delim' plus '\n'.

> Since telnet sends \r\n on line break, the updated expression
> delim + "\n"could not be found in a sequence data+delim+"\r\n",

Forgotten whitespace after the first "\n".

> so delimiter feature did not work at all.
> Added delim + "\r" check along with delim + "\n", that solves the
> described problem and does not violate backward compatibility.
> 
> Closes #2027
> 
> https://github.com/tarantool/tarantool/tree/kshch/gh-2027-telnet-alternative-delimiter
> https://github.com/tarantool/tarantool/issues/2027
> ---
>  src/box/lua/console.lua | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index 028001127..bdedcb393 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -344,8 +344,9 @@ end
>  -- Read command from connected client console.listen()
>  --
>  local function client_read(self)
> -    local delim = self.delimiter .. "\n"
> -    local buf = self.client:read(delim)
> +    local delim_lf = self.delimiter .. "\n"
> +    local delim_cr = self.delimiter .. "\r"

I would comment here why we need delim_cr (in short).

> +    local buf = self.client:read({delimiter = {delim_lf, delim_cr}})
>      if buf == nil then
>          return nil
>      elseif buf == "" then
> @@ -355,7 +356,7 @@ local function client_read(self)
>          return nil
>      end
>      -- remove trailing delimiter
> -    return buf:sub(1, -#delim-1)
> +    return buf:sub(1, -#delim_lf-1)

I would use `-#self.delimiter` to make the code simmetric btw delim_cr /
delim_lf.

>  end
>  
>  --
> -- 
> 2.20.1
> 

[tarantool-patches] Re: [PATCH v1 1/1] box: fix custom delimiter for telnet connection

[Kirill Shcherbatov]

On 05.03.2019 16:43, Alexander Turenko wrote:
> This patch looks good for me, except several minor things.
Hi! Thank you for review. Fixed everything.

============================================

In order to give a user ability to use a delimiter symbol within a
code the real delimiter is user-provided 'delim' plus "\n".
Since telnet sends "\r\n" on line break, the updated expression
delim + "\n" could not be found in a sequence data+delim+"\r\n",
so delimiter feature did not work at all.
Added delim + "\r" check along with delim + "\n", that solves the
described problem and does not violate backward compatibility.

Closes #2027
---
 src/box/lua/console.lua       | 12 +++++++++---
 test/app-tap/console.test.lua | 14 +++++++++++++-
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 028001127..74e9addad 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -344,8 +344,14 @@ end
 -- Read command from connected client console.listen()
 --
 local function client_read(self)
-    local delim = self.delimiter .. "\n"
-    local buf = self.client:read(delim)
+    --
+    -- Byte sequences that come over the network and come from
+    -- the local client console may have a different terminal
+    -- character. We support bouth possible options: CR and LF.
+    --
+    local delim_cr = self.delimiter .. "\r"
+    local delim_lf = self.delimiter .. "\n"
+    local buf = self.client:read({delimiter = {delim_lf, delim_cr}})
     if buf == nil then
         return nil
     elseif buf == "" then
@@ -355,7 +361,7 @@ local function client_read(self)
         return nil
     end
     -- remove trailing delimiter
-    return buf:sub(1, -#delim-1)
+    return buf:sub(1, -#self.delimiter-2)
 end
 
 --
diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
index 4f915afd7..ccd33a2d0 100755
--- a/test/app-tap/console.test.lua
+++ b/test/app-tap/console.test.lua
@@ -21,7 +21,7 @@ local EOL = "\n...\n"
 
 test = tap.test("console")
 
-test:plan(59)
+test:plan(61)
 
 -- Start console and connect to it
 local server = console.listen(CONSOLE_SOCKET)
@@ -255,6 +255,18 @@ box.session.on_connect(nil, console_on_connect)
 box.session.on_disconnect(nil, console_on_disconnect)
 box.session.on_auth(nil, console_on_auth)
 
+
+--
+-- gh-2027: Fix custom delimiter for console connection.
+--
+client = socket.tcp_connect("unix/", CONSOLE_SOCKET)
+_ = client:read(128)
+client:write("console = require('console'); console.delimiter('#');\n")
+test:is(yaml.decode(client:read(EOL))[1], nil, "session type")
+client:write("box.NULL#\r\n")
+test:is(yaml.decode(client:read(EOL))[1], box.NULL, "test new delimiter")
+client:close()
+
 --
 -- gh-2642 "box.session.type()"
 --
-- 
2.21.0

[tarantool-patches] Re: [PATCH v1 1/1] box: fix custom delimiter for telnet connection

[Alexander Turenko]

We discussed it a bit more voicely and Kirill fixed some comments. I'll
put them below for the record.

LGTM then.

Vlad, can you, please, look at the patch?

On Tue, Mar 05, 2019 at 05:41:10PM +0300, Kirill Shcherbatov wrote:
> On 05.03.2019 16:43, Alexander Turenko wrote:
> > This patch looks good for me, except several minor things.
> Hi! Thank you for review. Fixed everything.
> 
> ============================================
> 
> In order to give a user ability to use a delimiter symbol within a
> code the real delimiter is user-provided 'delim' plus "\n".
> Since telnet sends "\r\n" on line break, the updated expression
> delim + "\n" could not be found in a sequence data+delim+"\r\n",
> so delimiter feature did not work at all.
> Added delim + "\r" check along with delim + "\n", that solves the
> described problem and does not violate backward compatibility.
> 
> Closes #2027
> ---
>  src/box/lua/console.lua       | 12 +++++++++---
>  test/app-tap/console.test.lua | 14 +++++++++++++-
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index 028001127..74e9addad 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -344,8 +344,14 @@ end
>  -- Read command from connected client console.listen()
>  --
>  local function client_read(self)
> -    local delim = self.delimiter .. "\n"
> -    local buf = self.client:read(delim)
> +    --
> +    -- Byte sequences that come over the network and come from
> +    -- the local client console may have a different terminal
> +    -- character. We support bouth possible options: CR and LF.
> +    --

Typo: bouth -> both.

CR and LF -> LF and CRLF.

> +    local delim_cr = self.delimiter .. "\r"
> +    local delim_lf = self.delimiter .. "\n"
> +    local buf = self.client:read({delimiter = {delim_lf, delim_cr}})
>      if buf == nil then
>          return nil
>      elseif buf == "" then
> @@ -355,7 +361,7 @@ local function client_read(self)
>          return nil
>      end
>      -- remove trailing delimiter
> -    return buf:sub(1, -#delim-1)
> +    return buf:sub(1, -#self.delimiter-2)
>  end
>  
>  --
> diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
> index 4f915afd7..ccd33a2d0 100755
> --- a/test/app-tap/console.test.lua
> +++ b/test/app-tap/console.test.lua
> @@ -21,7 +21,7 @@ local EOL = "\n...\n"
>  
>  test = tap.test("console")
>  
> -test:plan(59)
> +test:plan(61)
>  
>  -- Start console and connect to it
>  local server = console.listen(CONSOLE_SOCKET)
> @@ -255,6 +255,18 @@ box.session.on_connect(nil, console_on_connect)
>  box.session.on_disconnect(nil, console_on_disconnect)
>  box.session.on_auth(nil, console_on_auth)
>  
> +
> +--
> +-- gh-2027: Fix custom delimiter for console connection.

console -> telnet

> +--
> +client = socket.tcp_connect("unix/", CONSOLE_SOCKET)
> +_ = client:read(128)
> +client:write("console = require('console'); console.delimiter('#');\n")
> +test:is(yaml.decode(client:read(EOL))[1], nil, "session type")
> +client:write("box.NULL#\r\n")
> +test:is(yaml.decode(client:read(EOL))[1], box.NULL, "test new delimiter")
> +client:close()
> +
>  --
>  -- gh-2642 "box.session.type()"
>  --
> -- 
> 2.21.0

[tarantool-patches] Re: [PATCH v1 1/1] box: fix custom delimiter for telnet connection

[Kirill Yukhin]

Hello,

On 21 Feb 10:34, Kirill Shcherbatov wrote:
> Because test-run uses a console connection to run tests, the
> actual string delimiter was the user-specified delimiter
> delim + "\n".
> Since telnet sends \r\n on line break, the updated expression
> delim + "\n"could not be found in a sequence data+delim+"\r\n",
> so delimiter feature did not work at all.
> Added delim + "\r" check along with delim + "\n", that solves the
> described problem and does not violate backward compatibility.
> 
> Closes #2027
> 
> https://github.com/tarantool/tarantool/tree/kshch/gh-2027-telnet-alternative-delimiter
> https://github.com/tarantool/tarantool/issues/2027

I've committed your patch into 2.1 branch.

--
Regards, Kirill Yukhin