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

[Alexander Turenko <alexander.turenko@tarantool.org>]

> >> @@ -1360,9 +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
> >> -        self._errno = boxerrno.EINVAL
> >> +    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
> >> +    if dns == nil then
> >> +        return nil, err
> >> +    end
> >> +    if #dns == 0 then
> >> +        boxerrno(self._errno)
> >>         return nil, socket_error(self)
> >>     end
> >>     for _, remote in ipairs(dns) do
> > 
> > EINVAL here is changed to self._errno, which is always `nil` at least
> > for typical usage of lua socket: `s = socket.tcp()`, then `s:connect()`.
> > 
> > Let's keep the old behaviour (EINVAL) when getaddrinfo() returns zero
> > amount of addresses.
> > 
> > RFC 2553 ('Basic Socket Interface Extensions for IPv6') states:
> > 
> > | Upon successful return a pointer to a linked list of one or more
> > | addrinfo structures is returned through the final argument.
> > 
> > RFC 3493 ('Basic Socket Interface Extensions for IPv6') states the same,
> > but more explicitly:
> > 
> > | Upon successful return of getaddrinfo(), the location to which res
> > | points shall refer to a linked list of addrinfo structures, each of
> > | which shall specify a socket address and information for use in
> > | creating a socket with which to use that socket address.  The list
> > | shall include at least one addrinfo structure.
> > 
> > So this situation should not occur. But anyway we should not miss to set
> > a certain errno in the case at least for consistency with tcp_connect().
> > 
> > Also I think that we should set errno to EINVAL when getaddrinfo() fails
> > to keep the code backward compatible: say, a user lean on this value
> > when tcp_connect() / :connect() returns `nil`.
>
> I understand so:
> 
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index a334ad45b..6c829a11c 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -995,7 +995,13 @@ local function getaddrinfo(host, port, timeout, opts)
>          end
>  
>      end
> -    return internal.getaddrinfo(host, port, timeout, ga_opts)
> +
> +    local ret, err = internal.getaddrinfo(host, port, timeout, ga_opts)
> +    if ret == nil then
> +        boxerrno(boxerrno.EINVAL)
> +        return nil, err
> +    end
> +    return ret
>  end
>  
>  -- tcp connector
> @@ -1041,9 +1047,12 @@ local function tcp_connect(host, port, timeout)
>      end
>      local timeout = timeout or TIMEOUT_INFINITY
>      local stop = fiber.clock() + timeout
> -    local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
> +    local dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
>          protocol = 'tcp' })
> -    if dns == nil or #dns == 0 then
> +    if dns == nil then
> +        return nil, err
> +    end
> +    if #dns == 0 then
>          boxerrno(boxerrno.EINVAL)
>          return nil
>      end
> @@ -1360,8 +1369,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()
> +        return nil, err
> +    end
> +    if #dns == 0 then
>          self._errno = boxerrno.EINVAL
>          return nil, socket_error(self)
>      end
> @@ -1547,9 +1560,12 @@ local function lsocket_connect(host, port)
>      if host == nil or port == nil then
>          error("Usage: luasocket.connect(host, port)")
>      end
> -    local s = tcp_connect(host, port)
> +    local s, err = tcp_connect(host, port)
>      if not s then
> -        return nil, boxerrno.strerror()
> +        if not err then
> +            return nil, boxerrno.strerror()
> +        end
> +        return nil, err
>      end
>      setmetatable(s, lsocket_tcp_client_mt)
>      return s

I meant that `self._errno = boxerrno.EINVAL` was changed to
`boxerrno(self._errno)` in `#dns == 0` case and it does not more affect
the second return value (an error message). I would return it back for
`#dns == 0` case (only for it).

The motivation is just to don't change things that we don't asked to
change.

I didn't mean changes in getaddrinfo() function itself.

> > Also I see that we don't document Lua Socket part of the API:
> > https://www.tarantool.io/en/doc/1.10/reference/reference_lua/socket/
> 
> Is this the part of this patchset? Maybe, it would be better to do doc
> request separately.

It was in context of your docbot request: there is no need to ask to
update Lua Socket API docs if we does not provide it at all.

Lua Socket has its own API documentation, so it is maybe okay to don't
document it again on our website.