[Tarantool-patches] [PATCH 1/1] console: fix usage of an undeclared variable

[Tarantool-patches] [PATCH 1/1] console: fix usage of an undeclared variable

[Vladislav Shpilevoy]

Console client's eval() method in case of an error at
reading from a socket was trying to return a variable
declared in a different view scope. Instead, the
error should be raised to drop the connection.
---
Branch: https://github.com/tarantool/tarantool/tree/gerold103/console-undefined-variable

No test, because can be reproduced only in repl mode of a client.

 src/box/lua/console.lua | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index f70ed830a..52df67465 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -451,7 +451,7 @@ local text_connection_mt = {
                     self.print_f(rc)
                 end
             end
-            return rc
+            return error(self:set_error())
         end,
         --
         -- Make the connection be in error state, set error
-- 
2.21.0 (Apple Git-122)

Re: [Tarantool-patches] [PATCH 1/1] console: fix usage of an undeclared variable

[Cyrill Gorcunov]

On Thu, Oct 17, 2019 at 09:50:44PM +0200, Vladislav Shpilevoy wrote:
> Console client's eval() method in case of an error at
> reading from a socket was trying to return a variable
> declared in a different view scope. Instead, the
> error should be raised to drop the connection.
> ---
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/console-undefined-variable
> 
> No test, because can be reproduced only in repl mode of a client.
> 
>  src/box/lua/console.lua | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index f70ed830a..52df67465 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -451,7 +451,7 @@ local text_connection_mt = {
>                      self.print_f(rc)
>                  end
>              end
> -            return rc
> +            return error(self:set_error())

Wait
...
                local rc = self:read()
                if not rc then
                    break
                end
...

We're reading from a socket and there could be EOF, which will
give us zero return code. But now we will start to yielding
an error in this case.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH 1/1] console: fix usage of an undeclared variable

[Vladislav Shpilevoy]

On 17/10/2019 22:00, Cyrill Gorcunov wrote:
> On Thu, Oct 17, 2019 at 09:50:44PM +0200, Vladislav Shpilevoy wrote:
>> Console client's eval() method in case of an error at
>> reading from a socket was trying to return a variable
>> declared in a different view scope. Instead, the
>> error should be raised to drop the connection.
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/gerold103/console-undefined-variable
>>
>> No test, because can be reproduced only in repl mode of a client.
>>
>>  src/box/lua/console.lua | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
>> index f70ed830a..52df67465 100644
>> --- a/src/box/lua/console.lua
>> +++ b/src/box/lua/console.lua
>> @@ -451,7 +451,7 @@ local text_connection_mt = {
>>                      self.print_f(rc)
>>                  end
>>              end
>> -            return rc
>> +            return error(self:set_error())
> 
> Wait
> ...
>                 local rc = self:read()
>                 if not rc then
>                     break
>                 end
> ...
> 
> We're reading from a socket and there could be EOF, which will
> give us zero return code. But now we will start to yielding
> an error in this case.
> 

set_error is aware of that. It says 'Peer closed' on EOF.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH 1/1] console: fix usage of an undeclared variable

[Cyrill Gorcunov]

On Thu, Oct 17, 2019 at 10:31:00PM +0200, Vladislav Shpilevoy wrote:
> > 
> > We're reading from a socket and there could be EOF, which will
> > give us zero return code. But now we will start to yielding
> > an error in this case.
> > 
> 
> set_error is aware of that. It says 'Peer closed' on EOF.

Looks reasonable enough. Thanks!

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH 1/1] console: fix usage of an undeclared variable

[Cyrill Gorcunov]

On Fri, Oct 18, 2019 at 12:05:05AM +0300, Cyrill Gorcunov wrote:
> On Thu, Oct 17, 2019 at 10:31:00PM +0200, Vladislav Shpilevoy wrote:
> > > 
> > > We're reading from a socket and there could be EOF, which will
> > > give us zero return code. But now we will start to yielding
> > > an error in this case.
> > > 
> > 
> > set_error is aware of that. It says 'Peer closed' on EOF.
> 
> Looks reasonable enough. Thanks!

Still (if only I'm not missing something obvious) this changes
api a bit -- previously we've been returning empty string for
this case but now we will print 'Peer closed', hopefully this
won't be a problem though.

	Cyrill

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH 1/1] console: fix usage of an undeclared variable

[Vladislav Shpilevoy]

On 17/10/2019 23:08, Cyrill Gorcunov wrote:
> On Fri, Oct 18, 2019 at 12:05:05AM +0300, Cyrill Gorcunov wrote:
>> On Thu, Oct 17, 2019 at 10:31:00PM +0200, Vladislav Shpilevoy wrote:
>>>>
>>>> We're reading from a socket and there could be EOF, which will
>>>> give us zero return code. But now we will start to yielding
>>>> an error in this case.
>>>>
>>>
>>> set_error is aware of that. It says 'Peer closed' on EOF.
>>
>> Looks reasonable enough. Thanks!
> 
> Still (if only I'm not missing something obvious) this changes
> api a bit -- previously we've been returning empty string for
> this case but now we will print 'Peer closed', hopefully this
> won't be a problem though.
> 
> 	Cyrill
> 

Previously there was an error about undeclared variable.

    'return rc'

Here 'rc' was not declared.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH 1/1] console: fix usage of an undeclared variable

[Cyrill Gorcunov]

On Thu, Oct 17, 2019 at 11:18:22PM +0200, Vladislav Shpilevoy wrote:
> Previously there was an error about undeclared variable.
> 
>     'return rc'
> 
> Here 'rc' was not declared.

Yes, and it means we will get nil here, and the caller code
threat it as no-error and return empty string upward.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH 1/1] console: fix usage of an undeclared variable

[Vladislav Shpilevoy]

On 17/10/2019 23:15, Cyrill Gorcunov wrote:
> On Thu, Oct 17, 2019 at 11:18:22PM +0200, Vladislav Shpilevoy wrote:
>> Previously there was an error about undeclared variable.
>>
>>     'return rc'
>>
>> Here 'rc' was not declared.
> 
> Yes, and it means we will get nil here, and the caller code
> threat it as no-error and return empty string upward.
> 

No, it was not nil. It was:

localhost:3313> 
---
- error: 'builtin/box/console.lua:454: variable ''rc'' is not declared'
...

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH 1/1] console: fix usage of an undeclared variable

[Cyrill Gorcunov]

On Thu, Oct 17, 2019 at 11:22:56PM +0200, Vladislav Shpilevoy wrote:
> > Yes, and it means we will get nil here, and the caller code
> > threat it as no-error and return empty string upward.
> 
> No, it was not nil. It was:
> 
> localhost:3313> 
> ---
> - error: 'builtin/box/console.lua:454: variable ''rc'' is not declared'
> ...

Aha! I happen to miss that. Thanks for the point!

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH 1/1] console: fix usage of an undeclared variable

[Vladislav Shpilevoy]

On 17/10/2019 23:19, Cyrill Gorcunov wrote:
> On Thu, Oct 17, 2019 at 11:22:56PM +0200, Vladislav Shpilevoy wrote:
>>> Yes, and it means we will get nil here, and the caller code
>>> threat it as no-error and return empty string upward.
>>
>> No, it was not nil. It was:
>>
>> localhost:3313> 
>> ---
>> - error: 'builtin/box/console.lua:454: variable ''rc'' is not declared'
>> ...
> 
> Aha! I happen to miss that. Thanks for the point!
> 

It would be nil in case a user turned strict mode off.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH 1/1] console: fix usage of an undeclared variable

[Cyrill Gorcunov]

On Thu, Oct 17, 2019 at 11:26:04PM +0200, Vladislav Shpilevoy wrote:
> >> localhost:3313> 
> >> ---
> >> - error: 'builtin/box/console.lua:454: variable ''rc'' is not declared'
> >> ...
> > 
> > Aha! I happen to miss that. Thanks for the point!
> 
> It would be nil in case a user turned strict mode off.

I see, thanks for explanation.

Re: [Tarantool-patches] [PATCH 1/1] console: fix usage of an undeclared variable

[Alexander Turenko]

LGTM.

Pushed to master, 2.2, 2.1 and 1.10.

WBR, Alexander Turenko.

On Thu, Oct 17, 2019 at 09:50:44PM +0200, Vladislav Shpilevoy wrote:
> Console client's eval() method in case of an error at
> reading from a socket was trying to return a variable
> declared in a different view scope. Instead, the
> error should be raised to drop the connection.
> ---
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/console-undefined-variable
> 
> No test, because can be reproduced only in repl mode of a client.
> 
>  src/box/lua/console.lua | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index f70ed830a..52df67465 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -451,7 +451,7 @@ local text_connection_mt = {
>                      self.print_f(rc)
>                  end
>              end
> -            return rc
> +            return error(self:set_error())
>          end,
>          --
>          -- Make the connection be in error state, set error
> -- 
> 2.21.0 (Apple Git-122)
>