[tarantool-patches] Re: [server-dev] Re: Re: [PATCH v2 1/2] lua: return getaddrinfo() errors

[Alexander Turenko]

> +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
> +-- Error code and error message returned by getaddrinfo() depends
> +-- on system's gai_strerror().
> 
> >> +test_run:cmd("setopt delimiter ';'")
> >> +function check_err(err)
> >> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> >> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> >> +       err == 'getaddrinfo: Name or service not known' then
> >> +            return true
> >> +    end
> >> +    return false
> >> +end;
> >> +test_run:cmd("setopt delimiter ''");
> >> +
> >> +s, err = socket:connect('non_exists_hostname:3301')
> >> +check_err(err) == true
> >> +
> >> test_run:cmd("clear filter”)
> > 
> > It is better to verify it against ffi.C.gai_strerror(EAI_AGAIN)
> > ffi.C.gai_strerror(EAI_NONAME) and don't rely on the fact that those
> > messages don't changed across libc versions. The same for other tests.
> Don’t know how to export this macroses/enums to Lua. More, it is too
> complicated, because we need to covert cdata to Lua string.

We discussed it voicely with Roman. The plan was to add needed constants
to socket.c and expose into Lua as strings. We however don't expose
GAI_* error codes, just messages from gai_strerror(). So maybe adding
them just for tests isn't really worthful.

> >> +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
> >> +-- Error code and error message returned by getaddrinfo() depends
> >> +-- on system's gai_strerror(). So that there is no checking for
> >> +-- certain error message.
> >> +test_run:cmd("setopt delimiter ';'")
> >> +---
> >> +- true
> >> +...
> >> +function check_err(err)
> >> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> >> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> >> +       err == 'getaddrinfo: Name or service not known' then
> >> +            return true
> >> +    end
> >> +    return false
> >> +end;
> >> +---
> >> +...
> >> +test_run:cmd("setopt delimiter ''");
> >> +---
> >> +- true
> >> +...
> >> +s, err = socket:connect('non_exists_hostname:3301')
> > 
> > socket.connect() should be called, the colon here is the mistake.

Please, look again to the code. One call is socket.connect(...), another
is socket:connect().

Just in case, I suggest to add a patch to your patchset to keep youself
from such mistakes:

 | diff --git a/src/lua/socket.lua b/src/lua/socket.lua
 | index e4815adbb..a1c13d4db 100644
 | --- a/src/lua/socket.lua
 | +++ b/src/lua/socket.lua
 | @@ -1554,7 +1554,8 @@ local function lsocket_tcp()
 |  end
 |  
 |  local function lsocket_connect(host, port)
 | -    if host == nil or port == nil then
 | +    if type(host) ~= 'string' or (type(port) ~= 'string' and
 | +            type(port) ~= 'number') then
 | 		 error("Usage: luasocket.connect(host, port)")
 | 	 end
 | 	 local s, err = tcp_connect(host, port)
 | @@ -1569,7 +1570,8 @@ local function lsocket_connect(host, port)
 |  end
 |  
 |  local function lsocket_bind(host, port, backlog)
 | -    if host == nil or port == nil then
 | +    if type(host) ~= 'string' or (type(port) ~= 'string' and
 | +            type(port) ~= 'number') then
 | 		 error("Usage: luasocket.bind(host, port [, backlog])")
 | 	 end
 | 	 local function prepare(s) return backlog end

> > I think we should test both native and Lua Socket API, because both were
> > changed.
> > 
> > Should we ever change Lua Socket API? Please, verify it with a
> > documentation and an actual behaviour of the external module. We emulate
> > it, so we should be compatible.

Are you look into this?