From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 698C324003 for ; Tue, 9 Jul 2019 22:17:00 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 3QsMWtfXU0sF for ; Tue, 9 Jul 2019 22:17:00 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B539C23F1F for ; Tue, 9 Jul 2019 22:16:59 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 1/2] lua: return getaddrinfo() errors From: Roman Khabibov In-Reply-To: <20190709080407.sv5quzrxpbnijwp4@tkn_work_nb> Date: Wed, 10 Jul 2019 05:16:55 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20190615165829.11888-1-roman.habibov@tarantool.org> <20190623203141.snnjgyqrntr6okp4@tkn_work_nb> <8C0820C4-7F5E-40A9-A634-E449E700D816@tarantool.org> <20190709080407.sv5quzrxpbnijwp4@tkn_work_nb> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Alexander Turenko > On Jul 9, 2019, at 11:04, Alexander Turenko = wrote: >=20 >>> The commit message should mention Roman's commit and cleanly state = that >>> (rc < 0) assumption does not work on Mac OS. >=20 > This is not done. lua: return getaddrinfo() errors =20 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. =20 Closes #4138 >> + /* gh-4138: Check getaddrinfo() error. */ >> + isnt(coio_getaddrinfo("hostname", port, NULL, &i, 1), 0, = "getaddrinfo error"); >=20 > The line is longer then 80 symbols >> + isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL, >> + "getaddrinfo error"); >=20 > 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 =3D socket:connect('non_exists_hostname:3301') +string.find(err, 'getaddrinfo') ~=3D nil +s, err =3D socket:bind('non_exists_hostname:3301') +string.find(err, 'getaddrinfo') ~=3D 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 >=20 > Typo: fist -> first. >=20 >> + * message). >> + * @retval One of luaT_error() codes. >=20 > '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 =3D socket:connect('hostname:3301'); >>>=20 >>> I would use less generic hostname like 'non_exists_hostname=E2=80=99. >=20 > The comment is still actual for test/unit/coio.cc. Done. > (I have copied parts of your patch below to comment them.) >=20 >> @@ -1347,10 +1350,10 @@ local function lsocket_tcp_connect(self, = host, port) >> -- This function is broken by design >> local ga_opts =3D { family =3D 'AF_INET', type =3D 'SOCK_STREAM' = } >> local timeout =3D deadline - fiber.clock() >> - local dns =3D getaddrinfo(host, port, timeout, ga_opts) >> + local dns, err =3D getaddrinfo(host, port, timeout, ga_opts) >> if dns =3D=3D nil or #dns =3D=3D 0 then >> - self._errno =3D boxerrno.EINVAL >> - return nil, socket_error(self) >> + self._errno =3D boxerrno.EINVAL >=20 > Broken indent. @@ -1347,10 +1356,10 @@ local function lsocket_tcp_connect(self, host, = port) -- This function is broken by design local ga_opts =3D { family =3D 'AF_INET', type =3D 'SOCK_STREAM' } local timeout =3D deadline - fiber.clock() - local dns =3D getaddrinfo(host, port, timeout, ga_opts) + local dns, err =3D getaddrinfo(host, port, timeout, ga_opts) if dns =3D=3D nil or #dns =3D=3D 0 then self._errno =3D boxerrno.EINVAL - return nil, socket_error(self) + return nil, err end for _, remote in ipairs(dns) do timeout =3D deadline - fiber.clock() >> @@ -1534,9 +1537,12 @@ local function lsocket_connect(host, port) >> if host =3D=3D nil or port =3D=3D nil then >> error("Usage: luasocket.connect(host, port)") >> end >> - local s =3D tcp_connect(host, port) >> + local s, err =3D tcp_connect(host, port) >> if not s then >> - return nil, boxerrno.strerror() >> + if not err then >=20 >> + return nil, boxerrno.strerror() >> + end >> + return nil, err >=20 > 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 =E2=80=98if', 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. >=20 >> for i, remote in pairs(dns) do >> timeout =3D stop - fiber.clock() >> if timeout <=3D 0 then >> boxerrno(boxerrno.ETIMEDOUT) >> - return nil >> + return nil, 'timed out' >> end >> local s =3D socket_new(remote.family, remote.type, = remote.protocol) >> if s then >=20 > Do you really think this change is self-explanatory and it is obvious > why this is needed (and why this was not needed before)? >=20 > 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'