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

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

> > The commit message should mention Roman's commit and cleanly state that
> > (rc < 0) assumption does not work on Mac OS.

This is not done.

> +	/* gh-4138: Check getaddrinfo() error. */
> +	isnt(coio_getaddrinfo("hostname", port, NULL, &i, 1), 0, "getaddrinfo error");

The line is longer then 80 symbols.

> +	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
> +	     "getaddrinfo error");

It would be good to have different case names.

The problem that stated in the issue is about improper error message. I don't
understand the test in this context: it does not check whether it is so. It is
applicable to lua tests too.

> + * Wrap coio_getaddrinfo() and call it. Push returned values onto
> + * @a L Lua stack.
> + *
> + * @param L Lua stack.
> + *
> + * @retval 1 Number of returned values by Lua function if
> + * coio_getaddrinfo() success.
> + * @retval 2 Number of returned values by Lua function if
> + * coio_getaddrinfo() is failed (fist is nil, second is error

Typo: fist -> first.

> + * message).
> + * @retval One of luaT_error() codes.

'luaT_error() code' is the strange phrase. I guess you want to say that
the function can raise a lua error. It is not a return value, because a
function does not return at all in the case (it is longjmp to be
precise).

> >> +s, err = socket:connect('hostname:3301');
> > 
> > I would use less generic hostname like 'non_exists_hostname’.

The comment is still actual for test/unit/coio.cc.

(I have copied parts of your patch below to comment them.)

> @@ -1347,10 +1350,10 @@ 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)
> +    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
>      if dns == nil or #dns == 0 then
> -        self._errno = boxerrno.EINVAL
> -        return nil, socket_error(self)
> +         self._errno = boxerrno.EINVAL

Broken indent.

> @@ -1534,9 +1537,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

Why not always set errno to EINVAL and return err, nil in case on an error? It
seems the 'if' here is redundant.

Also applicable to similar changes in other lua functions.

>      for i, remote in pairs(dns) do
>          timeout = stop - fiber.clock()
>          if timeout <= 0 then
>              boxerrno(boxerrno.ETIMEDOUT)
> -            return nil
> +            return nil, 'timed out'
>          end
>          local s = socket_new(remote.family, remote.type, remote.protocol)
>          if s then

Do you really think this change is self-explanatory and it is obvious
why this is needed (and why this was not needed before)?

Yep, we discussed it voicely, but I'm not a last reader of your code.