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 4119D3111A for ; Tue, 25 Jun 2019 09:38:37 -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 vm29tvLVvxWL for ; Tue, 25 Jun 2019 09:38:37 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 C9F0930CEB for ; Tue, 25 Jun 2019 09:38:36 -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: <20190623203141.snnjgyqrntr6okp4@tkn_work_nb> Date: Tue, 25 Jun 2019 16:38:34 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8C0820C4-7F5E-40A9-A634-E449E700D816@tarantool.org> References: <20190615165829.11888-1-roman.habibov@tarantool.org> <20190623203141.snnjgyqrntr6okp4@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 Jun 23, 2019, at 11:31 PM, Alexander Turenko = wrote: >=20 > Re C part of the commit (src/lib/core/coio_task.c). >=20 > The origin of the (rc < 0) check is the following commit (marked > relevant phrase with !!): >=20 > commit ea1da04d5add51c308efb3fd2d71cdfabed8411c > Author: Roman Tsisyk > Date: Mon Dec 5 20:28:59 2016 +0300 >=20 > Refactor coio_task >=20 > * Add coio_task_create() and coio_task_destroy() > * Rename coio_task() to coio_task_post() > * Fix the core invariant of coio_task_post(): > - On timeout or when fiber was cancelled set diag, return -1 and > guarantee that on_timeout will be called somewhen. > - Otherwise don't touch diag, don't call on_timeout callback, > return 0. Please check task->base.result and task->diag > to get the original return code and diag from the callback. > * Change the return value of coio_task_post() to "int". > * Add diag to coio_getaddrinfo() and fix a possible bug in = replication; > !! ignore uncoventional getaddrinfo(3) error codes for now. > * Fix buggy box.snapshot() tests. >=20 > Needed for #1954 >=20 > I don't find any reason to ignoring 'unconventional' errors. >=20 > getaddrinfo() returns negative values on Linux, however it is not so = on > Mac OS. Values and corresponding messages can be checked like so: >=20 > $ gcc -Wall -Wextra -x c <(echo -e '#include \n#include = \n#include \n#include \nint main() { = printf("%d\\n", EAI_NONAME); return 0; }') && ./a.out; rm a.out > $ gcc -Wall -Wextra -x c <(echo -e '#include \n#include = \n#include \n#include \nint main() { = printf("%s\\n", gai_strerror(EAI_NONAME)); return 0; }') && ./a.out; rm = a.out >=20 > The commit message should mention Roman's commit and cleanly state = that > (rc < 0) assumption does not work on Mac OS. >=20 > Aside of that there is nothing bad in calling gai_strerror() with some > unusual value, it just return "Unknown error" (I checked both Linux = and > Mac OS). >=20 > I would add relevant test case for coio_getaddrinfo() into > test/unit/coio.cc. @@ -72,7 +72,7 @@ static void test_getaddrinfo(void) { header(); - plan(1); + plan(3); const char *host =3D "127.0.0.1"; const char *port =3D "3333"; struct addrinfo *i; @@ -81,6 +81,11 @@ test_getaddrinfo(void) is(rc, 0, "getaddrinfo"); freeaddrinfo(i); =20 + /* gh-4138: Check getaddrinfo() error. */ + isnt(coio_getaddrinfo("hostname", port, NULL, &i, 1), 0, = "getaddrinfo error"); + isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL, + "getaddrinfo error"); + /* * gh-4209: 0 timeout should not be a special value and * detach a task. Before a fix it led to segfault > Consider comments re Lua part of the commit below. >=20 >> Before this patch, branch when coio_getaddrinfo() returns >> getaddrinfo() errors has never reached. Add this errors into the >> socket and thenet.box modules. >=20 > Typo: thenet.box -> net.box. 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 (checking for errors was rc < 0). 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 > Aside that I would add more investigation results and contracts change > (see other comments in this email). >=20 >>=20 >> Closes #4138 >> --- >>=20 >> Branch: = https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4138-getaddrin= fo >> Issue: https://github.com/tarantool/tarantool/issues/4138 >=20 >> diff --git a/src/lib/core/coio_task.c b/src/lib/core/coio_task.c >> index 908b336ed..83f669d05 100644 >> --- a/src/lib/core/coio_task.c >> +++ b/src/lib/core/coio_task.c >> @@ -413,7 +413,7 @@ coio_getaddrinfo(const char *host, const char = *port, >> return -1; /* timed out or cancelled */ >>=20 >> /* Task finished */ >> - if (task->rc < 0) { >> + if (task->rc !=3D 0) { >=20 > See comments at the start of the email. >=20 >> diff --git a/src/lua/socket.c b/src/lua/socket.c >> index 130378caf..5a8469ddf 100644 >> --- a/src/lua/socket.c >> +++ b/src/lua/socket.c >> @@ -54,6 +54,7 @@ >> #include >> #include "lua/utils.h" >> #include "lua/fiber.h" >> +#include "reflection.h" >=20 > It is only needed to deference 'err->type', so I would remove it with > this check. @@ -816,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L) =20 if (dns_res !=3D 0) { lua_pushnil(L); - return 1; + struct error *err =3D diag_get()->last; + lua_pushstring(L, err->errmsg); + return 2; } >>=20 >> extern int coio_wait(int fd, int event, double timeout); >>=20 >> @@ -816,6 +817,11 @@ lbox_socket_getaddrinfo(struct lua_State *L) >>=20 >> if (dns_res !=3D 0) { >> lua_pushnil(L); >> + struct error *err =3D diag_get()->last; >> + if (strcmp(err->type->name, "SystemError") =3D=3D 0) { >=20 > I don't think that we should check a type here. Why not report any = error > as is? >=20 > After that the convention for lbox_socket_getaddrinfo() will be = simple: > it returns a table of results when successul; `nil`, err_msg = otherwise. >=20 > Now it can return just `nil` or `nil`, err_msg -- the contract is more > complex. >=20 > Lua's code will be simpler if we'll simplify this contract. >=20 > I suggest to add a comment (in free form, but other reviewers can > enforce specific format) to the function that shows it contract > explicitly, like so: return <...> at success, otherwise return <=E2=80=A6= >. @@ -774,6 +774,19 @@ lbox_getaddrinfo_result_wrapper(struct lua_State = *L) return 1; } =20 +/** + * 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 + * message). + * @retval One of luaT_error() codes. + */ static int lbox_socket_getaddrinfo(struct lua_State *L) { >> diff --git a/src/lua/socket.lua b/src/lua/socket.lua >> index 2dba0a8d2..f0b432925 100644 >> --- a/src/lua/socket.lua >> +++ b/src/lua/socket.lua >> @@ -1028,11 +1028,14 @@ local function tcp_connect(host, port, = timeout) >> end >> local timeout =3D timeout or TIMEOUT_INFINITY >> local stop =3D fiber.clock() + timeout >> - local dns =3D getaddrinfo(host, port, timeout, { type =3D = 'SOCK_STREAM', >> + local dns, err =3D getaddrinfo(host, port, timeout, { type =3D = 'SOCK_STREAM', >> protocol =3D 'tcp' }) >> if dns =3D=3D nil or #dns =3D=3D 0 then >> - boxerrno(boxerrno.EINVAL) >> - return nil >> + if not err then >> + boxerrno(boxerrno.EINVAL) >> + return nil >> + end >> + return nil, err >=20 > Here we change the contract of socket.getaddrinfo() -- the user = visible > function. I would mention that in the commit message. Added. >> diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua >> index dab168f90..140baf22f 100644 >> --- a/test/app/socket.test.lua >> +++ b/test/app/socket.test.lua >> @@ -301,7 +301,7 @@ sc:close() >> -- tcp_connect >>=20 >> -- test timeout >> -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) >> +socket.tcp_connect('127.0.0.1', 80, 0.00000000000001) >=20 > It seems this change is not related to your commit. Removed. >> +--gh-4138 Check getaddrinfo() error. >> + >> +test_run:cmd("setopt delimiter ';'") >> +function check_err(err) >> + if err =3D=3D 'getaddrinfo: nodename nor servname provided, or = not known' or >> + err =3D=3D 'getaddrinfo: Servname not supported for = ai_socktype' or >> + err =3D=3D 'getaddrinfo: Name or service not known' then >> + return true >> + end >> + return false >> +end; >=20 > Flush the delimiter to '' here. >=20 > Please, comment (outside of semicolon-delimiter block) that these > messages corresponds to the following codes: >=20 > * EAI_NONAME (Mac OS) > * EAI_SERVICE (Linux and Mac OS) > * EAI_NONAME (Linux) >=20 > It seems that you reveice either EAI_NONAME or EAI_SERVICE, not? = Please, > elaborate. I guess it worth to see which certain error is returned by > getaddrinfo() here and check only for it (but consider difference > between Linux and Mac OS gai_strerror() messages if it is EAI_NONAME). >=20 >> + >> +s, err =3D socket:connect('hostname:3301'); >=20 > I would use less generic hostname like 'non_exists_hostname=E2=80=99. @@ -982,4 +982,14 @@ fiber.cancel(echo_fiber) client:read(1, 5) =3D=3D '' server:close() =20 +-- 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 + >> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua >> index bea43002d..13ed4e200 100644 >> --- a/test/box/net.box.test.lua >> +++ b/test/box/net.box.test.lua >> @@ -1538,4 +1538,19 @@ test_run:grep_log('default', '00000020:.*') >> test_run:grep_log('default', '00000030:.*') >> test_run:grep_log('default', '00000040:.*') >>=20 >> +--gh-4138 Check getaddrinfo() error. >> +test_run:cmd("setopt delimiter ';'") >> + >> +function check_err(err) >> + if err =3D=3D 'getaddrinfo: nodename nor servname provided, or = not known' or >> + err =3D=3D 'getaddrinfo: Servname not supported for = ai_socktype' or >> + err =3D=3D 'getaddrinfo: Name or service not known' then >> + return true >> + end >> + return false >> +end; >=20 > The same comments as above are applicable for this test case too. @@ -1538,4 +1538,12 @@ test_run:grep_log('default', '00000020:.*') test_run:grep_log('default', '00000030:.*') test_run:grep_log('default', '00000040:.*') =20 +-- 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 =3D remote.connect('non_exists_hostname:3301') +string.find(s['error'], 'getaddrinfo') ~=3D nil + box.cfg{log_level=3Dlog_level}=