From: Alexander Turenko <alexander.turenko@tarantool.org> To: Roman Khabibov <roman.habibov@tarantool.org> Cc: tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH] lua: return getaddrinfo() errors Date: Sun, 23 Jun 2019 23:31:43 +0300 [thread overview] Message-ID: <20190623203141.snnjgyqrntr6okp4@tkn_work_nb> (raw) In-Reply-To: <20190615165829.11888-1-roman.habibov@tarantool.org> Re C part of the commit (src/lib/core/coio_task.c). The origin of the (rc < 0) check is the following commit (marked relevant phrase with !!): commit ea1da04d5add51c308efb3fd2d71cdfabed8411c Author: Roman Tsisyk <roman@tsisyk.com> Date: Mon Dec 5 20:28:59 2016 +0300 Refactor coio_task * 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. Needed for #1954 I don't find any reason to ignoring 'unconventional' errors. getaddrinfo() returns negative values on Linux, however it is not so on Mac OS. Values and corresponding messages can be checked like so: $ gcc -Wall -Wextra -x c <(echo -e '#include <sys/types.h>\n#include <sys/socket.h>\n#include <netdb.h>\n#include <stdio.h>\nint main() { printf("%d\\n", EAI_NONAME); return 0; }') && ./a.out; rm a.out $ gcc -Wall -Wextra -x c <(echo -e '#include <sys/types.h>\n#include <sys/socket.h>\n#include <netdb.h>\n#include <stdio.h>\nint main() { printf("%s\\n", gai_strerror(EAI_NONAME)); return 0; }') && ./a.out; rm a.out The commit message should mention Roman's commit and cleanly state that (rc < 0) assumption does not work on Mac OS. 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). I would add relevant test case for coio_getaddrinfo() into test/unit/coio.cc. Consider comments re Lua part of the commit below. > Before this patch, branch when coio_getaddrinfo() returns > getaddrinfo() errors has never reached. Add this errors into the > socket and thenet.box modules. Typo: thenet.box -> net.box. Aside that I would add more investigation results and contracts change (see other comments in this email). > > Closes #4138 > --- > > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4138-getaddrinfo > Issue: https://github.com/tarantool/tarantool/issues/4138 > 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 */ > > /* Task finished */ > - if (task->rc < 0) { > + if (task->rc != 0) { See comments at the start of the email. > 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 <fiber.h> > #include "lua/utils.h" > #include "lua/fiber.h" > +#include "reflection.h" It is only needed to deference 'err->type', so I would remove it with this check. > > extern int coio_wait(int fd, int event, double timeout); > > @@ -816,6 +817,11 @@ lbox_socket_getaddrinfo(struct lua_State *L) > > if (dns_res != 0) { > lua_pushnil(L); > + struct error *err = diag_get()->last; > + if (strcmp(err->type->name, "SystemError") == 0) { I don't think that we should check a type here. Why not report any error as is? After that the convention for lbox_socket_getaddrinfo() will be simple: it returns a table of results when successul; `nil`, err_msg otherwise. Now it can return just `nil` or `nil`, err_msg -- the contract is more complex. Lua's code will be simpler if we'll simplify this contract. 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 <...>. > 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 = 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 > - boxerrno(boxerrno.EINVAL) > - return nil > + if not err then > + boxerrno(boxerrno.EINVAL) > + return nil > + end > + return nil, err Here we change the contract of socket.getaddrinfo() -- the user visible function. I would mention that in the commit message. > 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 > > -- test timeout > -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) > +socket.tcp_connect('127.0.0.1', 80, 0.00000000000001) It seems this change is not related to your commit. > +--gh-4138 Check getaddrinfo() error. > + > +test_run:cmd("setopt delimiter ';'") > +function check_err(err) > + if err == 'getaddrinfo: nodename nor servname provided, or not known' or > + err == 'getaddrinfo: Servname not supported for ai_socktype' or > + err == 'getaddrinfo: Name or service not known' then > + return true > + end > + return false > +end; Flush the delimiter to '' here. Please, comment (outside of semicolon-delimiter block) that these messages corresponds to the following codes: * EAI_NONAME (Mac OS) * EAI_SERVICE (Linux and Mac OS) * EAI_NONAME (Linux) 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). > + > +s, err = socket:connect('hostname:3301'); I would use less generic hostname like 'non_exists_hostname'. > 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:.*') > > +--gh-4138 Check getaddrinfo() error. > +test_run:cmd("setopt delimiter ';'") > + > +function check_err(err) > + if err == 'getaddrinfo: nodename nor servname provided, or not known' or > + err == 'getaddrinfo: Servname not supported for ai_socktype' or > + err == 'getaddrinfo: Name or service not known' then > + return true > + end > + return false > +end; The same comments as above are applicable for this test case too.
next prev parent reply other threads:[~2019-06-23 20:32 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-15 16:58 [tarantool-patches] " Roman Khabibov 2019-06-23 20:31 ` Alexander Turenko [this message] 2019-06-25 13:38 ` [tarantool-patches] Re: [PATCH v2 1/2] " Roman Khabibov 2019-07-09 8:04 ` Alexander Turenko 2019-07-10 2:16 ` Roman Khabibov 2019-07-23 12:39 ` Alexander Turenko 2019-07-26 13:48 ` Alexander Turenko 2019-08-05 13:36 ` Roman Khabibov 2019-08-29 0:45 ` Alexander Turenko [not found] ` <868EAF2C-A491-46C9-AD37-7512D6CAB213@tarantool.org> 2019-09-06 13:45 ` [tarantool-patches] Re: [server-dev] " Alexander Turenko 2019-09-10 12:54 ` Roman Khabibov 2019-11-01 14:39 ` [Tarantool-patches] [server-dev] Re: [tarantool-patches] " Alexander Turenko 2019-11-21 17:27 ` [Tarantool-patches] [tarantool-patches] [server-dev] " Roman Khabibov 2019-12-08 19:48 ` Alexander Turenko 2019-12-10 16:25 ` Roman Khabibov 2019-12-18 14:58 ` Alexander Turenko 2019-12-21 17:50 ` Roman Khabibov 2019-12-23 13:36 ` Alexander Turenko 2019-12-26 17:29 ` Roman Khabibov 2020-02-18 13:55 ` [Tarantool-patches] Fwd: " Roman Khabibov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190623203141.snnjgyqrntr6okp4@tkn_work_nb \ --to=alexander.turenko@tarantool.org \ --cc=roman.habibov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH] lua: return getaddrinfo() errors' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox