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

[Alexander Turenko]

Comments below are trivial. Let's fix and proceed with the next reviewer
(Sergey O.). Please, resend the patchset for him.

I still think you should run CI on all targets (push to ...-full-ci
branch).

LGTM except minor comments below.

WBR, Alexander Turenko.

> @@ -1360,8 +1363,12 @@ local function lsocket_tcp_connect(self, host, port)
>      -- This function is broken by design
>      local ga_opts = { family = 'AF_INET', type = 'SOCK_STREAM' }
>      local timeout = deadline - fiber.clock()
> -    local dns = getaddrinfo(host, port, timeout, ga_opts)
> -    if dns == nil or #dns == 0 then
> +    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
> +    if dns == nil then
> +        self._errno = boxerrno()

Master's socket _errno was set to EINVAL before this patch. Let's keep
this behaviour, but change the 2nd return value.

I mean:

 | if dns == nil then
 |     self._errno = boxerrno.EINVAL
 |     return nil, err
 | end
 | if #dns == 0 then
 |     self._errno = boxerrno.EINVAL
 |     return nil, socket_error(self)
 | end

> +        return nil, err
> +    end
> +    if #dns == 0 then
>          self._errno = boxerrno.EINVAL
>          return nil, socket_error(self)
>      end

> diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
> index c72d41763..7086e496b 100644
> --- a/test/app/socket.test.lua
> +++ b/test/app/socket.test.lua
> @@ -960,6 +968,27 @@ server:close()
>  
>  test_run:cmd("clear filter")
>  
> +-- 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;

You added the following error message to coio_getaddrinfo() unit test in
the first commit:

 | const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided"
 |         ", or not known";

So it worth to add it here too. We'll enable the test of FreeBSD sooner
or later.

Also I got EAI_AGAIN on my system now (maybe something was changed on
DNS server side). Let's add it too (for both commits).

> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
> index 8e65ff470..9c9aee1ad 100644
> --- a/test/box/net.box.test.lua
> +++ b/test/box/net.box.test.lua
> @@ -1582,4 +1582,21 @@ test_run:wait_log('default', '00000030:.*', nil, 10)
>  -- we expect nothing below, so don't wait
>  test_run:grep_log('default', '00000040:.*')
>  
> +-- gh-4138 Check getaddrinfo() error from 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 ''");

Same here.