[tarantool-patches] Re: [PATCH v2 1/2] lua: return getaddrinfo() errors
Roman Khabibov
roman.habibov at tarantool.org
Wed Jul 10 05:16:55 MSK 2019
> On Jul 9, 2019, at 11:04, Alexander Turenko <alexander.turenko at tarantool.org> wrote:
>
>>> 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.
lua: return getaddrinfo() errors
Before this patch, branch when getaddrinfo() returns error codes
couldn't be reached on Mac OS, because they are greater than 0 on
Mac OS (assumption "rc < 0" in commit ea1da04d5 is incorret for
Mac OS). Add this errors into the socket and the net.box modules.
Now getaddrinfo() can return a pair of values (nil and error
message) in case of error.
Closes #4138
>> + /* 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.
+ /* gh-4138: Check getaddrinfo() error. */
+ isnt(coio_getaddrinfo("non_exists_hostname", port, NULL, &i, 1), 0,
+ "getaddrinfo error");
+ isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
+ "getaddrinfo error message");
+
> 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.
>I would add relevant test case for coio_getaddrinfo() into
>test/unit/coio.cc.
I added this for this reason.
Why? Now, error message is proper, and I check it by grep:
+-- gh-4138 Check getaddrinfo() error. Error code and error message
+-- returned by getaddrinfo() depends on system's gai_strerror()
+-- and compiler. So that there is no checking for certain error
+-- message.
+
+s, err = socket:connect('non_exists_hostname:3301')
+string.find(err, 'getaddrinfo') ~= nil
+s, err = socket:bind('non_exists_hostname:3301')
+string.find(err, 'getaddrinfo') ~= nil
+
Same about net.box test.
>> + * 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).
+/**
+ * Wrap coio_getaddrinfo() and call it. Push returned values onto
+ * @a L Lua stack. In case of error raise a Lua error.
+ *
+ * @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 (first is nil, second is error
+ * message).
+ */
>>>> +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.
Done.
> (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.
@@ -1347,10 +1356,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)
+ return nil, err
end
for _, remote in ipairs(dns) do
timeout = deadline - fiber.clock()
>> @@ -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.
I think that if we remove this ‘if', the behaviour of these methods
(lsocket_connect(), lsocket_bind()) will be strange. In some cases error
message is present, in some cases it is not, because tcp_connect() can return
just nil without err.
> 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.
- return nil
+ -- Second arg added to do the behaviour of this
+ -- function symmetrical in case of timeout on both
+ -- Linux and Mac OS. On Mac OS error message 'timed
+ -- out' is thrown by getaddrinfo(). On Linux the
+ -- behaviour of getaddrinfo() in this case isn't same
+ -- and timeout is checked here.
+ return nil, 'timed out'
More information about the Tarantool-patches
mailing list