* [Tarantool-patches] [PATCH v2 0/2] Return getaddrinfo() errors
@ 2020-03-12 10:24 Roman Khabibov
2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS Roman Khabibov
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Roman Khabibov @ 2020-03-12 10:24 UTC (permalink / raw)
To: tarantool-patches
Issue: https://github.com/tarantool/tarantool/issues/4138
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4138-getaddrinfo-full-ci
Roman Khabibov (2):
coio/say: fix getaddrinfo error handling on macOS
lua: return getaddrinfo() errors
src/box/lua/net_box.lua | 7 +++--
src/lib/core/coio_task.c | 2 +-
src/lib/core/say.c | 12 ++++++--
src/lua/socket.c | 16 +++++++++-
src/lua/socket.lua | 22 ++++++++++----
test/app/socket.result | 63 ++++++++++++++++++++++++++++++++++++---
test/app/socket.test.lua | 35 ++++++++++++++++++++--
test/box/net.box.result | 31 +++++++++++++++++++
test/box/net.box.test.lua | 20 +++++++++++++
test/unit/coio.cc | 29 +++++++++++++++++-
test/unit/coio.result | 4 ++-
11 files changed, 221 insertions(+), 20 deletions(-)
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 23+ messages in thread* [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-03-12 10:24 [Tarantool-patches] [PATCH v2 0/2] Return getaddrinfo() errors Roman Khabibov @ 2020-03-12 10:24 ` Roman Khabibov 2020-03-29 8:51 ` Sergey Ostanevich 2020-04-26 17:20 ` Vladislav Shpilevoy 2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors Roman Khabibov 2020-07-28 14:08 ` [Tarantool-patches] [PATCH v2 0/2] Return " Kirill Yukhin 2 siblings, 2 replies; 23+ messages in thread From: Roman Khabibov @ 2020-03-12 10:24 UTC (permalink / raw) To: tarantool-patches Before this patch, branch when getaddrinfo() returns error codes couldn't be reached on macOS, because they are greater than 0 on macOS (assumption "rc < 0" in commit ea1da04 is incorrect for macOS). Note: diag_log() in say.c was added, because otherwise it will be hid by the following diagnostic and it should be handled in a better way after #1148. Also, two diag_set() in syslog_connect_unix() was added to avoid asserts in this diag_log(). Need for #4138 --- src/lib/core/coio_task.c | 2 +- src/lib/core/say.c | 12 ++++++++++-- test/unit/coio.cc | 29 ++++++++++++++++++++++++++++- test/unit/coio.result | 4 +++- 4 files changed, 42 insertions(+), 5 deletions(-) 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) { /* getaddrinfo() failed */ errno = EIO; diag_set(SystemError, "getaddrinfo: %s", diff --git a/src/lib/core/say.c b/src/lib/core/say.c index 64a637c58..8ad88ad57 100644 --- a/src/lib/core/say.c +++ b/src/lib/core/say.c @@ -459,14 +459,17 @@ static inline int syslog_connect_unix(const char *path) { int fd = socket(PF_UNIX, SOCK_DGRAM, 0); - if (fd < 0) + if (fd < 0) { + diag_set(SystemError, "socket"); return -1; + } struct sockaddr_un un; memset(&un, 0, sizeof(un)); snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); un.sun_family = AF_UNIX; if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { close(fd); + diag_set(SystemError, "connect"); return -1; } return fd; @@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address) hints.ai_protocol = IPPROTO_UDP; ret = getaddrinfo(remote, portnum, &hints, &inf); - if (ret < 0) { + if (ret != 0) { errno = EIO; diag_set(SystemError, "getaddrinfo: %s", gai_strerror(ret)); @@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str) say_free_syslog_opts(&opts); log->fd = log_syslog_connect(log); if (log->fd < 0) { + /* + * We need to log a diagnostics here until stacked + * diagnostics will be implemented (#1148). + */ + diag_log(); /* syslog indent is freed in atexit(). */ diag_set(SystemError, "syslog logger: %s", strerror(errno)); return -1; diff --git a/test/unit/coio.cc b/test/unit/coio.cc index bb8bd7131..957c58ede 100644 --- a/test/unit/coio.cc +++ b/test/unit/coio.cc @@ -72,7 +72,7 @@ static void test_getaddrinfo(void) { header(); - plan(1); + plan(3); const char *host = "127.0.0.1"; const char *port = "3333"; struct addrinfo *i; @@ -81,6 +81,33 @@ test_getaddrinfo(void) is(rc, 0, "getaddrinfo"); freeaddrinfo(i); + /* + * gh-4138: Check getaddrinfo() retval and diagnostics + * area. + */ + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, + 15768000000); + isnt(rc, 0, "getaddrinfo retval"); + const char *errmsg = diag_get()->last->errmsg; + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" + ", or not known"; + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " + "ai_socktype"; + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" + ", or not known"; + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " + "resolution"; + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " + "this time"; + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || + strcmp(errmsg, exp_errmsg_2) == 0 || + strcmp(errmsg, exp_errmsg_3) == 0 || + strcmp(errmsg, exp_errmsg_4) == 0 || + strcmp(errmsg, exp_errmsg_5) == 0 || + strcmp(errmsg, exp_errmsg_6) == 0; + is(is_match_with_exp, true, "getaddrinfo error message"); + /* * gh-4209: 0 timeout should not be a special value and * detach a task. Before a fix it led to segfault diff --git a/test/unit/coio.result b/test/unit/coio.result index 5019fa48a..90b567140 100644 --- a/test/unit/coio.result +++ b/test/unit/coio.result @@ -7,6 +7,8 @@ # call done with res 0 *** test_call_f: done *** *** test_getaddrinfo *** -1..1 +1..3 ok 1 - getaddrinfo +ok 2 - getaddrinfo retval +ok 3 - getaddrinfo error message *** test_getaddrinfo: done *** -- 2.21.0 (Apple Git-122) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS Roman Khabibov @ 2020-03-29 8:51 ` Sergey Ostanevich 2020-03-29 9:12 ` Alexander Turenko 2020-04-06 2:07 ` Roman Khabibov 2020-04-26 17:20 ` Vladislav Shpilevoy 1 sibling, 2 replies; 23+ messages in thread From: Sergey Ostanevich @ 2020-03-29 8:51 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches Hi! Thanks for the patch! See below on my comments. Sergos. On 12 мар 13:24, Roman Khabibov wrote: > Before this patch, branch when getaddrinfo() returns error codes > couldn't be reached on macOS, because they are greater than 0 on > macOS (assumption "rc < 0" in commit ea1da04 is incorrect for > macOS). > > Note: diag_log() in say.c was added, because otherwise it will be > hid by the following diagnostic and it should be handled in a > better way after #1148. Also, two diag_set() in Please, notify owner of #1148 about follow-up that will be needed. > syslog_connect_unix() was added to avoid asserts in this > diag_log(). > > Need for #4138 > --- > src/lib/core/coio_task.c | 2 +- > src/lib/core/say.c | 12 ++++++++++-- > test/unit/coio.cc | 29 ++++++++++++++++++++++++++++- > test/unit/coio.result | 4 +++- > 4 files changed, 42 insertions(+), 5 deletions(-) > > 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) { > /* getaddrinfo() failed */ > errno = EIO; > diag_set(SystemError, "getaddrinfo: %s", > diff --git a/src/lib/core/say.c b/src/lib/core/say.c > index 64a637c58..8ad88ad57 100644 > --- a/src/lib/core/say.c > +++ b/src/lib/core/say.c > @@ -459,14 +459,17 @@ static inline int > syslog_connect_unix(const char *path) > { > int fd = socket(PF_UNIX, SOCK_DGRAM, 0); > - if (fd < 0) > + if (fd < 0) { > + diag_set(SystemError, "socket"); This error message gives nothing. Please, describe the error behind it using the strerror(errno) > return -1; > + } > struct sockaddr_un un; > memset(&un, 0, sizeof(un)); > snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > un.sun_family = AF_UNIX; > if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { > close(fd); > + diag_set(SystemError, "connect"); Ditto. > return -1; > } > return fd; > @@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address) > hints.ai_protocol = IPPROTO_UDP; > > ret = getaddrinfo(remote, portnum, &hints, &inf); > - if (ret < 0) { > + if (ret != 0) { > errno = EIO; > diag_set(SystemError, "getaddrinfo: %s", > gai_strerror(ret)); > @@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str) > say_free_syslog_opts(&opts); > log->fd = log_syslog_connect(log); > if (log->fd < 0) { > + /* > + * We need to log a diagnostics here until stacked > + * diagnostics will be implemented (#1148). > + */ > + diag_log(); Make a poniter about this in #1148 > /* syslog indent is freed in atexit(). */ > diag_set(SystemError, "syslog logger: %s", strerror(errno)); > return -1; > diff --git a/test/unit/coio.cc b/test/unit/coio.cc > index bb8bd7131..957c58ede 100644 > --- a/test/unit/coio.cc > +++ b/test/unit/coio.cc > @@ -72,7 +72,7 @@ static void > test_getaddrinfo(void) > { > header(); > - plan(1); > + plan(3); > const char *host = "127.0.0.1"; > const char *port = "3333"; > struct addrinfo *i; > @@ -81,6 +81,33 @@ test_getaddrinfo(void) > is(rc, 0, "getaddrinfo"); > freeaddrinfo(i); > > + /* > + * gh-4138: Check getaddrinfo() retval and diagnostics > + * area. > + */ > + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, > + 15768000000); > + isnt(rc, 0, "getaddrinfo retval"); > + const char *errmsg = diag_get()->last->errmsg; > + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" > + ", or not known"; > + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " > + "ai_socktype"; > + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; > + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" > + ", or not known"; > + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " > + "resolution"; > + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " > + "this time"; > + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || > + strcmp(errmsg, exp_errmsg_2) == 0 || > + strcmp(errmsg, exp_errmsg_3) == 0 || > + strcmp(errmsg, exp_errmsg_4) == 0 || > + strcmp(errmsg, exp_errmsg_5) == 0 || > + strcmp(errmsg, exp_errmsg_6) == 0; > + is(is_match_with_exp, true, "getaddrinfo error message"); > + Why did you made such a test - you're not sure which one will be triggered? Can you create a test that will check all possible errors? > /* > * gh-4209: 0 timeout should not be a special value and > * detach a task. Before a fix it led to segfault > diff --git a/test/unit/coio.result b/test/unit/coio.result > index 5019fa48a..90b567140 100644 > --- a/test/unit/coio.result > +++ b/test/unit/coio.result > @@ -7,6 +7,8 @@ > # call done with res 0 > *** test_call_f: done *** > *** test_getaddrinfo *** > -1..1 > +1..3 > ok 1 - getaddrinfo > +ok 2 - getaddrinfo retval > +ok 3 - getaddrinfo error message > *** test_getaddrinfo: done *** > -- > 2.21.0 (Apple Git-122) > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-03-29 8:51 ` Sergey Ostanevich @ 2020-03-29 9:12 ` Alexander Turenko 2020-04-06 2:07 ` Roman Khabibov 1 sibling, 0 replies; 23+ messages in thread From: Alexander Turenko @ 2020-03-29 9:12 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches > > diff --git a/src/lib/core/say.c b/src/lib/core/say.c > > index 64a637c58..8ad88ad57 100644 > > --- a/src/lib/core/say.c > > +++ b/src/lib/core/say.c > > @@ -459,14 +459,17 @@ static inline int > > syslog_connect_unix(const char *path) > > { > > int fd = socket(PF_UNIX, SOCK_DGRAM, 0); > > - if (fd < 0) > > + if (fd < 0) { > > + diag_set(SystemError, "socket"); > This error message gives nothing. Please, describe the error behind it > using the strerror(errno) | void | SystemError::log() const | { | say_file_line(S_SYSERROR, file, line, strerror(saved_errno), | "SystemError %s", errmsg); | } https://github.com/tarantool/tarantool/blob/eaa860885dc5708956094845e20f5bc81275ca26/src/lib/core/exception.cc#L153-L158 > > diff --git a/test/unit/coio.cc b/test/unit/coio.cc > > index bb8bd7131..957c58ede 100644 > > --- a/test/unit/coio.cc > > +++ b/test/unit/coio.cc > > @@ -72,7 +72,7 @@ static void > > test_getaddrinfo(void) > > { > > header(); > > - plan(1); > > + plan(3); > > const char *host = "127.0.0.1"; > > const char *port = "3333"; > > struct addrinfo *i; > > @@ -81,6 +81,33 @@ test_getaddrinfo(void) > > is(rc, 0, "getaddrinfo"); > > freeaddrinfo(i); > > > > + /* > > + * gh-4138: Check getaddrinfo() retval and diagnostics > > + * area. > > + */ > > + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, > > + 15768000000); > > + isnt(rc, 0, "getaddrinfo retval"); > > + const char *errmsg = diag_get()->last->errmsg; > > + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" > > + ", or not known"; > > + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " > > + "ai_socktype"; > > + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; > > + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" > > + ", or not known"; > > + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " > > + "resolution"; > > + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " > > + "this time"; > > + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || > > + strcmp(errmsg, exp_errmsg_2) == 0 || > > + strcmp(errmsg, exp_errmsg_3) == 0 || > > + strcmp(errmsg, exp_errmsg_4) == 0 || > > + strcmp(errmsg, exp_errmsg_5) == 0 || > > + strcmp(errmsg, exp_errmsg_6) == 0; > > + is(is_match_with_exp, true, "getaddrinfo error message"); > > + > Why did you made such a test - you're not sure which one will be > triggered? Can you create a test that will check all possible errors? I asked to check for certain errors rather than that some SystemError with '^getaddrinfo: .*$' message raised. Sure, the return value of getaddrinfo() depends on network conditions (I guess it depends of a DNS server: whether it reports temporary or persistent error; but also whether a network interface is up at all). gai_strerror() output depends of OS. I asked to comment messages with EAI_* constants that corresponds to them, but it seems it was missed. Sure, proper description is necessary here. Nope, we unable to control network conditions and OS here and cannot create a test case for each of those situations. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-03-29 8:51 ` Sergey Ostanevich 2020-03-29 9:12 ` Alexander Turenko @ 2020-04-06 2:07 ` Roman Khabibov 2020-04-17 9:37 ` Sergey Ostanevich 1 sibling, 1 reply; 23+ messages in thread From: Roman Khabibov @ 2020-04-06 2:07 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches Hi! Thanks for the review. > On Mar 29, 2020, at 11:51, Sergey Ostanevich <sergos@tarantool.org> wrote: > > Hi! > > Thanks for the patch! > See below on my comments. > > Sergos. > > On 12 мар 13:24, Roman Khabibov wrote: >> Before this patch, branch when getaddrinfo() returns error codes >> couldn't be reached on macOS, because they are greater than 0 on >> macOS (assumption "rc < 0" in commit ea1da04 is incorrect for >> macOS). >> >> Note: diag_log() in say.c was added, because otherwise it will be >> hid by the following diagnostic and it should be handled in a >> better way after #1148. Also, two diag_set() in > Please, notify owner of #1148 about follow-up that will be needed. > >> syslog_connect_unix() was added to avoid asserts in this >> diag_log(). >> >> Need for #4138 >> --- >> src/lib/core/coio_task.c | 2 +- >> src/lib/core/say.c | 12 ++++++++++-- >> test/unit/coio.cc | 29 ++++++++++++++++++++++++++++- >> test/unit/coio.result | 4 +++- >> 4 files changed, 42 insertions(+), 5 deletions(-) >> >> 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) { >> /* getaddrinfo() failed */ >> errno = EIO; >> diag_set(SystemError, "getaddrinfo: %s", >> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >> index 64a637c58..8ad88ad57 100644 >> --- a/src/lib/core/say.c >> +++ b/src/lib/core/say.c >> @@ -459,14 +459,17 @@ static inline int >> syslog_connect_unix(const char *path) >> { >> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >> - if (fd < 0) >> + if (fd < 0) { >> + diag_set(SystemError, "socket"); > This error message gives nothing. Please, describe the error behind it > using the strerror(errno) >> return -1; >> + } >> struct sockaddr_un un; >> memset(&un, 0, sizeof(un)); >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >> un.sun_family = AF_UNIX; >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >> close(fd); >> + diag_set(SystemError, "connect"); > Ditto. @@ -465,13 +465,16 @@ static inline int syslog_connect_unix(const char *path) { int fd = socket(PF_UNIX, SOCK_DGRAM, 0); - if (fd < 0) + if (fd < 0) { + diag_set(SystemError, strerror(errno)); return -1; + } struct sockaddr_un un; memset(&un, 0, sizeof(un)); snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); un.sun_family = AF_UNIX; if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { + diag_set(SystemError, strerror(errno)); close(fd); return -1; } >> return -1; >> } >> return fd; >> @@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address) >> hints.ai_protocol = IPPROTO_UDP; >> >> ret = getaddrinfo(remote, portnum, &hints, &inf); >> - if (ret < 0) { >> + if (ret != 0) { >> errno = EIO; >> diag_set(SystemError, "getaddrinfo: %s", >> gai_strerror(ret)); >> @@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str) >> say_free_syslog_opts(&opts); >> log->fd = log_syslog_connect(log); >> if (log->fd < 0) { >> + /* >> + * We need to log a diagnostics here until stacked >> + * diagnostics will be implemented (#1148). >> + */ >> + diag_log(); > Make a poniter about this in #1148 Ok. >> /* syslog indent is freed in atexit(). */ >> diag_set(SystemError, "syslog logger: %s", strerror(errno)); >> return -1; >> diff --git a/test/unit/coio.cc b/test/unit/coio.cc >> index bb8bd7131..957c58ede 100644 >> --- a/test/unit/coio.cc >> +++ b/test/unit/coio.cc >> @@ -72,7 +72,7 @@ static void >> test_getaddrinfo(void) >> { >> header(); >> - plan(1); >> + plan(3); >> const char *host = "127.0.0.1"; >> const char *port = "3333"; >> struct addrinfo *i; >> @@ -81,6 +81,33 @@ test_getaddrinfo(void) >> is(rc, 0, "getaddrinfo"); >> freeaddrinfo(i); >> >> + /* >> + * gh-4138: Check getaddrinfo() retval and diagnostics >> + * area. >> + */ >> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, >> + 15768000000); >> + isnt(rc, 0, "getaddrinfo retval"); >> + const char *errmsg = diag_get()->last->errmsg; >> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" >> + ", or not known"; >> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " >> + "ai_socktype"; >> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; >> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" >> + ", or not known"; >> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " >> + "resolution"; >> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " >> + "this time"; >> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || >> + strcmp(errmsg, exp_errmsg_2) == 0 || >> + strcmp(errmsg, exp_errmsg_3) == 0 || >> + strcmp(errmsg, exp_errmsg_4) == 0 || >> + strcmp(errmsg, exp_errmsg_5) == 0 || >> + strcmp(errmsg, exp_errmsg_6) == 0; >> + is(is_match_with_exp, true, "getaddrinfo error message"); >> + > Why did you made such a test - you're not sure which one will be > triggered? Can you create a test that will check all possible errors? See Alexander answer. I added comments about the constants. >> /* >> * gh-4209: 0 timeout should not be a special value and >> * detach a task. Before a fix it led to segfault >> diff --git a/test/unit/coio.result b/test/unit/coio.result >> index 5019fa48a..90b567140 100644 >> --- a/test/unit/coio.result >> +++ b/test/unit/coio.result >> @@ -7,6 +7,8 @@ >> # call done with res 0 >> *** test_call_f: done *** >> *** test_getaddrinfo *** >> -1..1 >> +1..3 >> ok 1 - getaddrinfo >> +ok 2 - getaddrinfo retval >> +ok 3 - getaddrinfo error message >> *** test_getaddrinfo: done *** >> -- >> 2.21.0 (Apple Git-122) commit f17e3e73ae2689dd2ec1dcd94d699636f19f93a5 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Tue Jul 30 15:39:21 2019 +0300 coio/say: fix getaddrinfo error handling on macOS Before this patch, branch when getaddrinfo() returns error codes couldn't be reached on macOS, because they are greater than 0 on macOS (assumption "rc < 0" in commit ea1da04 is incorrect for macOS). Note: diag_log() in say.c was added, because otherwise it will be hid by the following diagnostic and it should be handled in a better way after #1148. Also, two diag_set() in syslog_connect_unix() was added to avoid asserts in this diag_log(). Need for #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) { /* getaddrinfo() failed */ errno = EIO; diag_set(SystemError, "getaddrinfo: %s", diff --git a/src/lib/core/say.c b/src/lib/core/say.c index dd05285a6..0f8db4587 100644 --- a/src/lib/core/say.c +++ b/src/lib/core/say.c @@ -465,13 +465,16 @@ static inline int syslog_connect_unix(const char *path) { int fd = socket(PF_UNIX, SOCK_DGRAM, 0); - if (fd < 0) + if (fd < 0) { + diag_set(SystemError, strerror(errno)); return -1; + } struct sockaddr_un un; memset(&un, 0, sizeof(un)); snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); un.sun_family = AF_UNIX; if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { + diag_set(SystemError, strerror(errno)); close(fd); return -1; } @@ -512,7 +515,7 @@ syslog_connect_remote(const char *server_address) hints.ai_protocol = IPPROTO_UDP; ret = getaddrinfo(remote, portnum, &hints, &inf); - if (ret < 0) { + if (ret != 0) { errno = EIO; diag_set(SystemError, "getaddrinfo: %s", gai_strerror(ret)); @@ -599,6 +602,11 @@ log_syslog_init(struct log *log, const char *init_str) say_free_syslog_opts(&opts); log->fd = log_syslog_connect(log); if (log->fd < 0) { + /* + * We need to log a diagnostics here until stacked + * diagnostics will be implemented (#1148). + */ + diag_log(); /* syslog indent is freed in atexit(). */ diag_set(SystemError, "syslog logger: %s", strerror(errno)); return -1; diff --git a/test/unit/coio.cc b/test/unit/coio.cc index bb8bd7131..69f78829c 100644 --- a/test/unit/coio.cc +++ b/test/unit/coio.cc @@ -72,7 +72,7 @@ static void test_getaddrinfo(void) { header(); - plan(1); + plan(3); const char *host = "127.0.0.1"; const char *port = "3333"; struct addrinfo *i; @@ -81,6 +81,39 @@ test_getaddrinfo(void) is(rc, 0, "getaddrinfo"); freeaddrinfo(i); + /* + * gh-4138: Check getaddrinfo() retval and diagnostics + * area. + */ + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, + 15768000000); + isnt(rc, 0, "getaddrinfo retval"); + const char *errmsg = diag_get()->last->errmsg; + /* EAI_NONAME */ + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" + ", or not known"; + /* EAI_SERVICE */ + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " + "ai_socktype"; + /* EAI_NONAME */ + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; + /* EAI_NONAME */ + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" + ", or not known"; + /* EAI_AGAIN */ + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " + "resolution"; + /* EAI_AGAIN */ + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " + "this time"; + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || + strcmp(errmsg, exp_errmsg_2) == 0 || + strcmp(errmsg, exp_errmsg_3) == 0 || + strcmp(errmsg, exp_errmsg_4) == 0 || + strcmp(errmsg, exp_errmsg_5) == 0 || + strcmp(errmsg, exp_errmsg_6) == 0; + is(is_match_with_exp, true, "getaddrinfo error message"); + /* * gh-4209: 0 timeout should not be a special value and * detach a task. Before a fix it led to segfault diff --git a/test/unit/coio.result b/test/unit/coio.result index 5019fa48a..90b567140 100644 --- a/test/unit/coio.result +++ b/test/unit/coio.result @@ -7,6 +7,8 @@ # call done with res 0 *** test_call_f: done *** *** test_getaddrinfo *** -1..1 +1..3 ok 1 - getaddrinfo +ok 2 - getaddrinfo retval +ok 3 - getaddrinfo error message *** test_getaddrinfo: done *** ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-04-06 2:07 ` Roman Khabibov @ 2020-04-17 9:37 ` Sergey Ostanevich 2020-04-24 11:32 ` Roman Khabibov 2020-06-23 13:27 ` Roman Khabibov 0 siblings, 2 replies; 23+ messages in thread From: Sergey Ostanevich @ 2020-04-17 9:37 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches > >> Note: diag_log() in say.c was added, because otherwise it will be > >> hid by the following diagnostic and it should be handled in a > >> better way after #1148. Also, two diag_set() in > > Please, notify owner of #1148 about follow-up that will be needed. > > As I can see from #1148 comments, it is already closed. Can you address the problem now with a new gh issue? Otherwise LGTM. Sergos > >> syslog_connect_unix() was added to avoid asserts in this > >> diag_log(). > >> > >> Need for #4138 > >> --- > >> src/lib/core/coio_task.c | 2 +- > >> src/lib/core/say.c | 12 ++++++++++-- > >> test/unit/coio.cc | 29 ++++++++++++++++++++++++++++- > >> test/unit/coio.result | 4 +++- > >> 4 files changed, 42 insertions(+), 5 deletions(-) > >> > >> 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) { > >> /* getaddrinfo() failed */ > >> errno = EIO; > >> diag_set(SystemError, "getaddrinfo: %s", > >> diff --git a/src/lib/core/say.c b/src/lib/core/say.c > >> index 64a637c58..8ad88ad57 100644 > >> --- a/src/lib/core/say.c > >> +++ b/src/lib/core/say.c > >> @@ -459,14 +459,17 @@ static inline int > >> syslog_connect_unix(const char *path) > >> { > >> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); > >> - if (fd < 0) > >> + if (fd < 0) { > >> + diag_set(SystemError, "socket"); > > This error message gives nothing. Please, describe the error behind it > > using the strerror(errno) > >> return -1; > >> + } > >> struct sockaddr_un un; > >> memset(&un, 0, sizeof(un)); > >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > >> un.sun_family = AF_UNIX; > >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { > >> close(fd); > >> + diag_set(SystemError, "connect"); > > Ditto. > @@ -465,13 +465,16 @@ static inline int > syslog_connect_unix(const char *path) > { > int fd = socket(PF_UNIX, SOCK_DGRAM, 0); > - if (fd < 0) > + if (fd < 0) { > + diag_set(SystemError, strerror(errno)); > return -1; > + } > struct sockaddr_un un; > memset(&un, 0, sizeof(un)); > snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > un.sun_family = AF_UNIX; > if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { > + diag_set(SystemError, strerror(errno)); > close(fd); > return -1; > } > > >> return -1; > >> } > >> return fd; > >> @@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address) > >> hints.ai_protocol = IPPROTO_UDP; > >> > >> ret = getaddrinfo(remote, portnum, &hints, &inf); > >> - if (ret < 0) { > >> + if (ret != 0) { > >> errno = EIO; > >> diag_set(SystemError, "getaddrinfo: %s", > >> gai_strerror(ret)); > >> @@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str) > >> say_free_syslog_opts(&opts); > >> log->fd = log_syslog_connect(log); > >> if (log->fd < 0) { > >> + /* > >> + * We need to log a diagnostics here until stacked > >> + * diagnostics will be implemented (#1148). > >> + */ > >> + diag_log(); > > Make a poniter about this in #1148 > Ok. > > >> /* syslog indent is freed in atexit(). */ > >> diag_set(SystemError, "syslog logger: %s", strerror(errno)); > >> return -1; > >> diff --git a/test/unit/coio.cc b/test/unit/coio.cc > >> index bb8bd7131..957c58ede 100644 > >> --- a/test/unit/coio.cc > >> +++ b/test/unit/coio.cc > >> @@ -72,7 +72,7 @@ static void > >> test_getaddrinfo(void) > >> { > >> header(); > >> - plan(1); > >> + plan(3); > >> const char *host = "127.0.0.1"; > >> const char *port = "3333"; > >> struct addrinfo *i; > >> @@ -81,6 +81,33 @@ test_getaddrinfo(void) > >> is(rc, 0, "getaddrinfo"); > >> freeaddrinfo(i); > >> > >> + /* > >> + * gh-4138: Check getaddrinfo() retval and diagnostics > >> + * area. > >> + */ > >> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, > >> + 15768000000); > >> + isnt(rc, 0, "getaddrinfo retval"); > >> + const char *errmsg = diag_get()->last->errmsg; > >> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" > >> + ", or not known"; > >> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " > >> + "ai_socktype"; > >> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; > >> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" > >> + ", or not known"; > >> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " > >> + "resolution"; > >> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " > >> + "this time"; > >> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || > >> + strcmp(errmsg, exp_errmsg_2) == 0 || > >> + strcmp(errmsg, exp_errmsg_3) == 0 || > >> + strcmp(errmsg, exp_errmsg_4) == 0 || > >> + strcmp(errmsg, exp_errmsg_5) == 0 || > >> + strcmp(errmsg, exp_errmsg_6) == 0; > >> + is(is_match_with_exp, true, "getaddrinfo error message"); > >> + > > Why did you made such a test - you're not sure which one will be > > triggered? Can you create a test that will check all possible errors? > See Alexander answer. I added comments about the constants. > > >> /* > >> * gh-4209: 0 timeout should not be a special value and > >> * detach a task. Before a fix it led to segfault > >> diff --git a/test/unit/coio.result b/test/unit/coio.result > >> index 5019fa48a..90b567140 100644 > >> --- a/test/unit/coio.result > >> +++ b/test/unit/coio.result > >> @@ -7,6 +7,8 @@ > >> # call done with res 0 > >> *** test_call_f: done *** > >> *** test_getaddrinfo *** > >> -1..1 > >> +1..3 > >> ok 1 - getaddrinfo > >> +ok 2 - getaddrinfo retval > >> +ok 3 - getaddrinfo error message > >> *** test_getaddrinfo: done *** > >> -- > >> 2.21.0 (Apple Git-122) > > commit f17e3e73ae2689dd2ec1dcd94d699636f19f93a5 > Author: Roman Khabibov <roman.habibov@tarantool.org> > Date: Tue Jul 30 15:39:21 2019 +0300 > > coio/say: fix getaddrinfo error handling on macOS > > Before this patch, branch when getaddrinfo() returns error codes > couldn't be reached on macOS, because they are greater than 0 on > macOS (assumption "rc < 0" in commit ea1da04 is incorrect for > macOS). > > Note: diag_log() in say.c was added, because otherwise it will be > hid by the following diagnostic and it should be handled in a > better way after #1148. Also, two diag_set() in > syslog_connect_unix() was added to avoid asserts in this > diag_log(). > > Need for #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) { > /* getaddrinfo() failed */ > errno = EIO; > diag_set(SystemError, "getaddrinfo: %s", > diff --git a/src/lib/core/say.c b/src/lib/core/say.c > index dd05285a6..0f8db4587 100644 > --- a/src/lib/core/say.c > +++ b/src/lib/core/say.c > @@ -465,13 +465,16 @@ static inline int > syslog_connect_unix(const char *path) > { > int fd = socket(PF_UNIX, SOCK_DGRAM, 0); > - if (fd < 0) > + if (fd < 0) { > + diag_set(SystemError, strerror(errno)); > return -1; > + } > struct sockaddr_un un; > memset(&un, 0, sizeof(un)); > snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > un.sun_family = AF_UNIX; > if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { > + diag_set(SystemError, strerror(errno)); > close(fd); > return -1; > } > @@ -512,7 +515,7 @@ syslog_connect_remote(const char *server_address) > hints.ai_protocol = IPPROTO_UDP; > > ret = getaddrinfo(remote, portnum, &hints, &inf); > - if (ret < 0) { > + if (ret != 0) { > errno = EIO; > diag_set(SystemError, "getaddrinfo: %s", > gai_strerror(ret)); > @@ -599,6 +602,11 @@ log_syslog_init(struct log *log, const char *init_str) > say_free_syslog_opts(&opts); > log->fd = log_syslog_connect(log); > if (log->fd < 0) { > + /* > + * We need to log a diagnostics here until stacked > + * diagnostics will be implemented (#1148). > + */ > + diag_log(); > /* syslog indent is freed in atexit(). */ > diag_set(SystemError, "syslog logger: %s", strerror(errno)); > return -1; > diff --git a/test/unit/coio.cc b/test/unit/coio.cc > index bb8bd7131..69f78829c 100644 > --- a/test/unit/coio.cc > +++ b/test/unit/coio.cc > @@ -72,7 +72,7 @@ static void > test_getaddrinfo(void) > { > header(); > - plan(1); > + plan(3); > const char *host = "127.0.0.1"; > const char *port = "3333"; > struct addrinfo *i; > @@ -81,6 +81,39 @@ test_getaddrinfo(void) > is(rc, 0, "getaddrinfo"); > freeaddrinfo(i); > > + /* > + * gh-4138: Check getaddrinfo() retval and diagnostics > + * area. > + */ > + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, > + 15768000000); > + isnt(rc, 0, "getaddrinfo retval"); > + const char *errmsg = diag_get()->last->errmsg; > + /* EAI_NONAME */ > + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" > + ", or not known"; > + /* EAI_SERVICE */ > + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " > + "ai_socktype"; > + /* EAI_NONAME */ > + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; > + /* EAI_NONAME */ > + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" > + ", or not known"; > + /* EAI_AGAIN */ > + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " > + "resolution"; > + /* EAI_AGAIN */ > + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " > + "this time"; > + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || > + strcmp(errmsg, exp_errmsg_2) == 0 || > + strcmp(errmsg, exp_errmsg_3) == 0 || > + strcmp(errmsg, exp_errmsg_4) == 0 || > + strcmp(errmsg, exp_errmsg_5) == 0 || > + strcmp(errmsg, exp_errmsg_6) == 0; > + is(is_match_with_exp, true, "getaddrinfo error message"); > + > /* > * gh-4209: 0 timeout should not be a special value and > * detach a task. Before a fix it led to segfault > diff --git a/test/unit/coio.result b/test/unit/coio.result > index 5019fa48a..90b567140 100644 > --- a/test/unit/coio.result > +++ b/test/unit/coio.result > @@ -7,6 +7,8 @@ > # call done with res 0 > *** test_call_f: done *** > *** test_getaddrinfo *** > -1..1 > +1..3 > ok 1 - getaddrinfo > +ok 2 - getaddrinfo retval > +ok 3 - getaddrinfo error message > *** test_getaddrinfo: done *** > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-04-17 9:37 ` Sergey Ostanevich @ 2020-04-24 11:32 ` Roman Khabibov 2020-06-23 13:27 ` Roman Khabibov 1 sibling, 0 replies; 23+ messages in thread From: Roman Khabibov @ 2020-04-24 11:32 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches Hi! Yes. > On Apr 17, 2020, at 12:37, Sergey Ostanevich <sergos@tarantool.org> wrote: > >>>> Note: diag_log() in say.c was added, because otherwise it will be >>>> hid by the following diagnostic and it should be handled in a >>>> better way after #1148. Also, two diag_set() in >>> Please, notify owner of #1148 about follow-up that will be needed. >>> > > As I can see from #1148 comments, it is already closed. Can you address > the problem now with a new gh issue? > > Otherwise LGTM. > > Sergos > >>>> syslog_connect_unix() was added to avoid asserts in this >>>> diag_log(). >>>> >>>> Need for #4138 >>>> --- >>>> src/lib/core/coio_task.c | 2 +- >>>> src/lib/core/say.c | 12 ++++++++++-- >>>> test/unit/coio.cc | 29 ++++++++++++++++++++++++++++- >>>> test/unit/coio.result | 4 +++- >>>> 4 files changed, 42 insertions(+), 5 deletions(-) >>>> >>>> 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) { >>>> /* getaddrinfo() failed */ >>>> errno = EIO; >>>> diag_set(SystemError, "getaddrinfo: %s", >>>> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >>>> index 64a637c58..8ad88ad57 100644 >>>> --- a/src/lib/core/say.c >>>> +++ b/src/lib/core/say.c >>>> @@ -459,14 +459,17 @@ static inline int >>>> syslog_connect_unix(const char *path) >>>> { >>>> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >>>> - if (fd < 0) >>>> + if (fd < 0) { >>>> + diag_set(SystemError, "socket"); >>> This error message gives nothing. Please, describe the error behind it >>> using the strerror(errno) >>>> return -1; >>>> + } >>>> struct sockaddr_un un; >>>> memset(&un, 0, sizeof(un)); >>>> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >>>> un.sun_family = AF_UNIX; >>>> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >>>> close(fd); >>>> + diag_set(SystemError, "connect"); >>> Ditto. >> @@ -465,13 +465,16 @@ static inline int >> syslog_connect_unix(const char *path) >> { >> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >> - if (fd < 0) >> + if (fd < 0) { >> + diag_set(SystemError, strerror(errno)); >> return -1; >> + } >> struct sockaddr_un un; >> memset(&un, 0, sizeof(un)); >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >> un.sun_family = AF_UNIX; >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >> + diag_set(SystemError, strerror(errno)); >> close(fd); >> return -1; >> } >> >>>> return -1; >>>> } >>>> return fd; >>>> @@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address) >>>> hints.ai_protocol = IPPROTO_UDP; >>>> >>>> ret = getaddrinfo(remote, portnum, &hints, &inf); >>>> - if (ret < 0) { >>>> + if (ret != 0) { >>>> errno = EIO; >>>> diag_set(SystemError, "getaddrinfo: %s", >>>> gai_strerror(ret)); >>>> @@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str) >>>> say_free_syslog_opts(&opts); >>>> log->fd = log_syslog_connect(log); >>>> if (log->fd < 0) { >>>> + /* >>>> + * We need to log a diagnostics here until stacked >>>> + * diagnostics will be implemented (#1148). >>>> + */ >>>> + diag_log(); >>> Make a poniter about this in #1148 >> Ok. >> >>>> /* syslog indent is freed in atexit(). */ >>>> diag_set(SystemError, "syslog logger: %s", strerror(errno)); >>>> return -1; >>>> diff --git a/test/unit/coio.cc b/test/unit/coio.cc >>>> index bb8bd7131..957c58ede 100644 >>>> --- a/test/unit/coio.cc >>>> +++ b/test/unit/coio.cc >>>> @@ -72,7 +72,7 @@ static void >>>> test_getaddrinfo(void) >>>> { >>>> header(); >>>> - plan(1); >>>> + plan(3); >>>> const char *host = "127.0.0.1"; >>>> const char *port = "3333"; >>>> struct addrinfo *i; >>>> @@ -81,6 +81,33 @@ test_getaddrinfo(void) >>>> is(rc, 0, "getaddrinfo"); >>>> freeaddrinfo(i); >>>> >>>> + /* >>>> + * gh-4138: Check getaddrinfo() retval and diagnostics >>>> + * area. >>>> + */ >>>> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, >>>> + 15768000000); >>>> + isnt(rc, 0, "getaddrinfo retval"); >>>> + const char *errmsg = diag_get()->last->errmsg; >>>> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" >>>> + ", or not known"; >>>> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " >>>> + "ai_socktype"; >>>> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; >>>> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" >>>> + ", or not known"; >>>> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " >>>> + "resolution"; >>>> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " >>>> + "this time"; >>>> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || >>>> + strcmp(errmsg, exp_errmsg_2) == 0 || >>>> + strcmp(errmsg, exp_errmsg_3) == 0 || >>>> + strcmp(errmsg, exp_errmsg_4) == 0 || >>>> + strcmp(errmsg, exp_errmsg_5) == 0 || >>>> + strcmp(errmsg, exp_errmsg_6) == 0; >>>> + is(is_match_with_exp, true, "getaddrinfo error message"); >>>> + >>> Why did you made such a test - you're not sure which one will be >>> triggered? Can you create a test that will check all possible errors? >> See Alexander answer. I added comments about the constants. >> >>>> /* >>>> * gh-4209: 0 timeout should not be a special value and >>>> * detach a task. Before a fix it led to segfault >>>> diff --git a/test/unit/coio.result b/test/unit/coio.result >>>> index 5019fa48a..90b567140 100644 >>>> --- a/test/unit/coio.result >>>> +++ b/test/unit/coio.result >>>> @@ -7,6 +7,8 @@ >>>> # call done with res 0 >>>> *** test_call_f: done *** >>>> *** test_getaddrinfo *** >>>> -1..1 >>>> +1..3 >>>> ok 1 - getaddrinfo >>>> +ok 2 - getaddrinfo retval >>>> +ok 3 - getaddrinfo error message >>>> *** test_getaddrinfo: done *** >>>> -- >>>> 2.21.0 (Apple Git-122) >> >> commit f17e3e73ae2689dd2ec1dcd94d699636f19f93a5 >> Author: Roman Khabibov <roman.habibov@tarantool.org> >> Date: Tue Jul 30 15:39:21 2019 +0300 >> >> coio/say: fix getaddrinfo error handling on macOS >> >> Before this patch, branch when getaddrinfo() returns error codes >> couldn't be reached on macOS, because they are greater than 0 on >> macOS (assumption "rc < 0" in commit ea1da04 is incorrect for >> macOS). >> >> Note: diag_log() in say.c was added, because otherwise it will be >> hid by the following diagnostic and it should be handled in a >> better way after #1148. Also, two diag_set() in >> syslog_connect_unix() was added to avoid asserts in this >> diag_log(). >> >> Need for #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) { >> /* getaddrinfo() failed */ >> errno = EIO; >> diag_set(SystemError, "getaddrinfo: %s", >> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >> index dd05285a6..0f8db4587 100644 >> --- a/src/lib/core/say.c >> +++ b/src/lib/core/say.c >> @@ -465,13 +465,16 @@ static inline int >> syslog_connect_unix(const char *path) >> { >> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >> - if (fd < 0) >> + if (fd < 0) { >> + diag_set(SystemError, strerror(errno)); >> return -1; >> + } >> struct sockaddr_un un; >> memset(&un, 0, sizeof(un)); >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >> un.sun_family = AF_UNIX; >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >> + diag_set(SystemError, strerror(errno)); >> close(fd); >> return -1; >> } >> @@ -512,7 +515,7 @@ syslog_connect_remote(const char *server_address) >> hints.ai_protocol = IPPROTO_UDP; >> >> ret = getaddrinfo(remote, portnum, &hints, &inf); >> - if (ret < 0) { >> + if (ret != 0) { >> errno = EIO; >> diag_set(SystemError, "getaddrinfo: %s", >> gai_strerror(ret)); >> @@ -599,6 +602,11 @@ log_syslog_init(struct log *log, const char *init_str) >> say_free_syslog_opts(&opts); >> log->fd = log_syslog_connect(log); >> if (log->fd < 0) { >> + /* >> + * We need to log a diagnostics here until stacked >> + * diagnostics will be implemented (#1148). >> + */ >> + diag_log(); >> /* syslog indent is freed in atexit(). */ >> diag_set(SystemError, "syslog logger: %s", strerror(errno)); >> return -1; >> diff --git a/test/unit/coio.cc b/test/unit/coio.cc >> index bb8bd7131..69f78829c 100644 >> --- a/test/unit/coio.cc >> +++ b/test/unit/coio.cc >> @@ -72,7 +72,7 @@ static void >> test_getaddrinfo(void) >> { >> header(); >> - plan(1); >> + plan(3); >> const char *host = "127.0.0.1"; >> const char *port = "3333"; >> struct addrinfo *i; >> @@ -81,6 +81,39 @@ test_getaddrinfo(void) >> is(rc, 0, "getaddrinfo"); >> freeaddrinfo(i); >> >> + /* >> + * gh-4138: Check getaddrinfo() retval and diagnostics >> + * area. >> + */ >> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, >> + 15768000000); >> + isnt(rc, 0, "getaddrinfo retval"); >> + const char *errmsg = diag_get()->last->errmsg; >> + /* EAI_NONAME */ >> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" >> + ", or not known"; >> + /* EAI_SERVICE */ >> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " >> + "ai_socktype"; >> + /* EAI_NONAME */ >> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; >> + /* EAI_NONAME */ >> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" >> + ", or not known"; >> + /* EAI_AGAIN */ >> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " >> + "resolution"; >> + /* EAI_AGAIN */ >> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " >> + "this time"; >> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || >> + strcmp(errmsg, exp_errmsg_2) == 0 || >> + strcmp(errmsg, exp_errmsg_3) == 0 || >> + strcmp(errmsg, exp_errmsg_4) == 0 || >> + strcmp(errmsg, exp_errmsg_5) == 0 || >> + strcmp(errmsg, exp_errmsg_6) == 0; >> + is(is_match_with_exp, true, "getaddrinfo error message"); >> + >> /* >> * gh-4209: 0 timeout should not be a special value and >> * detach a task. Before a fix it led to segfault >> diff --git a/test/unit/coio.result b/test/unit/coio.result >> index 5019fa48a..90b567140 100644 >> --- a/test/unit/coio.result >> +++ b/test/unit/coio.result >> @@ -7,6 +7,8 @@ >> # call done with res 0 >> *** test_call_f: done *** >> *** test_getaddrinfo *** >> -1..1 >> +1..3 >> ok 1 - getaddrinfo >> +ok 2 - getaddrinfo retval >> +ok 3 - getaddrinfo error message >> *** test_getaddrinfo: done *** >> >> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-04-17 9:37 ` Sergey Ostanevich 2020-04-24 11:32 ` Roman Khabibov @ 2020-06-23 13:27 ` Roman Khabibov 2020-07-23 14:12 ` Sergey Ostanevich 1 sibling, 1 reply; 23+ messages in thread From: Roman Khabibov @ 2020-06-23 13:27 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches, alexander.turenko Hi! I decided to stay diag_log() as it is. I tried to use new diag_add() from the stacked diagnostics patch, but it don’t log this error. We have to log this error to print error message from getaddrinfo after panic with “say”. See the following reproducer: tarantool> socket = require('socket') --- ... tarantool> log = require('log') --- ... tarantool> fio = require('fio') --- ... tarantool> --- ... tarantool> path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock') --- ... tarantool> unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0) --- ... tarantool> unix_socket:bind('unix/', path) --- - false ... tarantool> --- ... tarantool> opt = string.format("syslog:server=non_exists_hostname:%s,identity=tarantool", path) --- ... tarantool> box.cfg{log = opt, log_nonblock=true} SystemError getaddrinfo: nodename nor servname provided, or not known: Input/output error SystemError syslog logger: Input/output error: Input/output error failed to initialize logging subsystem If we remove diag_log(), we will lose getaddrinfo error in the log after panic. I didn’t add it to test, because once upon a time with Vova we decided that panic is hard to test, and it's not worth it. > On Apr 17, 2020, at 12:37, Sergey Ostanevich <sergos@tarantool.org> wrote: > >>>> Note: diag_log() in say.c was added, because otherwise it will be >>>> hid by the following diagnostic and it should be handled in a >>>> better way after #1148. Also, two diag_set() in >>> Please, notify owner of #1148 about follow-up that will be needed. >>> > > As I can see from #1148 comments, it is already closed. Can you address > the problem now with a new gh issue? > > Otherwise LGTM. > > Sergos > >>>> syslog_connect_unix() was added to avoid asserts in this >>>> diag_log(). >>>> >>>> Need for #4138 >>>> --- >>>> src/lib/core/coio_task.c | 2 +- >>>> src/lib/core/say.c | 12 ++++++++++-- >>>> test/unit/coio.cc | 29 ++++++++++++++++++++++++++++- >>>> test/unit/coio.result | 4 +++- >>>> 4 files changed, 42 insertions(+), 5 deletions(-) >>>> >>>> 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) { >>>> /* getaddrinfo() failed */ >>>> errno = EIO; >>>> diag_set(SystemError, "getaddrinfo: %s", >>>> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >>>> index 64a637c58..8ad88ad57 100644 >>>> --- a/src/lib/core/say.c >>>> +++ b/src/lib/core/say.c >>>> @@ -459,14 +459,17 @@ static inline int >>>> syslog_connect_unix(const char *path) >>>> { >>>> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >>>> - if (fd < 0) >>>> + if (fd < 0) { >>>> + diag_set(SystemError, "socket"); >>> This error message gives nothing. Please, describe the error behind it >>> using the strerror(errno) >>>> return -1; >>>> + } >>>> struct sockaddr_un un; >>>> memset(&un, 0, sizeof(un)); >>>> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >>>> un.sun_family = AF_UNIX; >>>> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >>>> close(fd); >>>> + diag_set(SystemError, "connect"); >>> Ditto. >> @@ -465,13 +465,16 @@ static inline int >> syslog_connect_unix(const char *path) >> { >> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >> - if (fd < 0) >> + if (fd < 0) { >> + diag_set(SystemError, strerror(errno)); >> return -1; >> + } >> struct sockaddr_un un; >> memset(&un, 0, sizeof(un)); >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >> un.sun_family = AF_UNIX; >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >> + diag_set(SystemError, strerror(errno)); >> close(fd); >> return -1; >> } >> >>>> return -1; >>>> } >>>> return fd; >>>> @@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address) >>>> hints.ai_protocol = IPPROTO_UDP; >>>> >>>> ret = getaddrinfo(remote, portnum, &hints, &inf); >>>> - if (ret < 0) { >>>> + if (ret != 0) { >>>> errno = EIO; >>>> diag_set(SystemError, "getaddrinfo: %s", >>>> gai_strerror(ret)); >>>> @@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str) >>>> say_free_syslog_opts(&opts); >>>> log->fd = log_syslog_connect(log); >>>> if (log->fd < 0) { >>>> + /* >>>> + * We need to log a diagnostics here until stacked >>>> + * diagnostics will be implemented (#1148). >>>> + */ >>>> + diag_log(); >>> Make a poniter about this in #1148 >> Ok. >> >>>> /* syslog indent is freed in atexit(). */ >>>> diag_set(SystemError, "syslog logger: %s", strerror(errno)); >>>> return -1; >>>> diff --git a/test/unit/coio.cc b/test/unit/coio.cc >>>> index bb8bd7131..957c58ede 100644 >>>> --- a/test/unit/coio.cc >>>> +++ b/test/unit/coio.cc >>>> @@ -72,7 +72,7 @@ static void >>>> test_getaddrinfo(void) >>>> { >>>> header(); >>>> - plan(1); >>>> + plan(3); >>>> const char *host = "127.0.0.1"; >>>> const char *port = "3333"; >>>> struct addrinfo *i; >>>> @@ -81,6 +81,33 @@ test_getaddrinfo(void) >>>> is(rc, 0, "getaddrinfo"); >>>> freeaddrinfo(i); >>>> >>>> + /* >>>> + * gh-4138: Check getaddrinfo() retval and diagnostics >>>> + * area. >>>> + */ >>>> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, >>>> + 15768000000); >>>> + isnt(rc, 0, "getaddrinfo retval"); >>>> + const char *errmsg = diag_get()->last->errmsg; >>>> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" >>>> + ", or not known"; >>>> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " >>>> + "ai_socktype"; >>>> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; >>>> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" >>>> + ", or not known"; >>>> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " >>>> + "resolution"; >>>> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " >>>> + "this time"; >>>> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || >>>> + strcmp(errmsg, exp_errmsg_2) == 0 || >>>> + strcmp(errmsg, exp_errmsg_3) == 0 || >>>> + strcmp(errmsg, exp_errmsg_4) == 0 || >>>> + strcmp(errmsg, exp_errmsg_5) == 0 || >>>> + strcmp(errmsg, exp_errmsg_6) == 0; >>>> + is(is_match_with_exp, true, "getaddrinfo error message"); >>>> + >>> Why did you made such a test - you're not sure which one will be >>> triggered? Can you create a test that will check all possible errors? >> See Alexander answer. I added comments about the constants. >> >>>> /* >>>> * gh-4209: 0 timeout should not be a special value and >>>> * detach a task. Before a fix it led to segfault >>>> diff --git a/test/unit/coio.result b/test/unit/coio.result >>>> index 5019fa48a..90b567140 100644 >>>> --- a/test/unit/coio.result >>>> +++ b/test/unit/coio.result >>>> @@ -7,6 +7,8 @@ >>>> # call done with res 0 >>>> *** test_call_f: done *** >>>> *** test_getaddrinfo *** >>>> -1..1 >>>> +1..3 >>>> ok 1 - getaddrinfo >>>> +ok 2 - getaddrinfo retval >>>> +ok 3 - getaddrinfo error message >>>> *** test_getaddrinfo: done *** >>>> -- >>>> 2.21.0 (Apple Git-122) >> >> commit f17e3e73ae2689dd2ec1dcd94d699636f19f93a5 >> Author: Roman Khabibov <roman.habibov@tarantool.org> >> Date: Tue Jul 30 15:39:21 2019 +0300 >> >> coio/say: fix getaddrinfo error handling on macOS >> >> Before this patch, branch when getaddrinfo() returns error codes >> couldn't be reached on macOS, because they are greater than 0 on >> macOS (assumption "rc < 0" in commit ea1da04 is incorrect for >> macOS). >> >> Note: diag_log() in say.c was added, because otherwise it will be >> hid by the following diagnostic and it should be handled in a >> better way after #1148. Also, two diag_set() in >> syslog_connect_unix() was added to avoid asserts in this >> diag_log(). >> >> Need for #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) { >> /* getaddrinfo() failed */ >> errno = EIO; >> diag_set(SystemError, "getaddrinfo: %s", >> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >> index dd05285a6..0f8db4587 100644 >> --- a/src/lib/core/say.c >> +++ b/src/lib/core/say.c >> @@ -465,13 +465,16 @@ static inline int >> syslog_connect_unix(const char *path) >> { >> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >> - if (fd < 0) >> + if (fd < 0) { >> + diag_set(SystemError, strerror(errno)); >> return -1; >> + } >> struct sockaddr_un un; >> memset(&un, 0, sizeof(un)); >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >> un.sun_family = AF_UNIX; >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >> + diag_set(SystemError, strerror(errno)); >> close(fd); >> return -1; >> } >> @@ -512,7 +515,7 @@ syslog_connect_remote(const char *server_address) >> hints.ai_protocol = IPPROTO_UDP; >> >> ret = getaddrinfo(remote, portnum, &hints, &inf); >> - if (ret < 0) { >> + if (ret != 0) { >> errno = EIO; >> diag_set(SystemError, "getaddrinfo: %s", >> gai_strerror(ret)); >> @@ -599,6 +602,11 @@ log_syslog_init(struct log *log, const char *init_str) >> say_free_syslog_opts(&opts); >> log->fd = log_syslog_connect(log); >> if (log->fd < 0) { >> + /* >> + * We need to log a diagnostics here until stacked >> + * diagnostics will be implemented (#1148). >> + */ >> + diag_log(); >> /* syslog indent is freed in atexit(). */ >> diag_set(SystemError, "syslog logger: %s", strerror(errno)); >> return -1; >> diff --git a/test/unit/coio.cc b/test/unit/coio.cc >> index bb8bd7131..69f78829c 100644 >> --- a/test/unit/coio.cc >> +++ b/test/unit/coio.cc >> @@ -72,7 +72,7 @@ static void >> test_getaddrinfo(void) >> { >> header(); >> - plan(1); >> + plan(3); >> const char *host = "127.0.0.1"; >> const char *port = "3333"; >> struct addrinfo *i; >> @@ -81,6 +81,39 @@ test_getaddrinfo(void) >> is(rc, 0, "getaddrinfo"); >> freeaddrinfo(i); >> >> + /* >> + * gh-4138: Check getaddrinfo() retval and diagnostics >> + * area. >> + */ >> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, >> + 15768000000); >> + isnt(rc, 0, "getaddrinfo retval"); >> + const char *errmsg = diag_get()->last->errmsg; >> + /* EAI_NONAME */ >> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" >> + ", or not known"; >> + /* EAI_SERVICE */ >> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " >> + "ai_socktype"; >> + /* EAI_NONAME */ >> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; >> + /* EAI_NONAME */ >> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" >> + ", or not known"; >> + /* EAI_AGAIN */ >> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " >> + "resolution"; >> + /* EAI_AGAIN */ >> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " >> + "this time"; >> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || >> + strcmp(errmsg, exp_errmsg_2) == 0 || >> + strcmp(errmsg, exp_errmsg_3) == 0 || >> + strcmp(errmsg, exp_errmsg_4) == 0 || >> + strcmp(errmsg, exp_errmsg_5) == 0 || >> + strcmp(errmsg, exp_errmsg_6) == 0; >> + is(is_match_with_exp, true, "getaddrinfo error message"); >> + >> /* >> * gh-4209: 0 timeout should not be a special value and >> * detach a task. Before a fix it led to segfault >> diff --git a/test/unit/coio.result b/test/unit/coio.result >> index 5019fa48a..90b567140 100644 >> --- a/test/unit/coio.result >> +++ b/test/unit/coio.result >> @@ -7,6 +7,8 @@ >> # call done with res 0 >> *** test_call_f: done *** >> *** test_getaddrinfo *** >> -1..1 >> +1..3 >> ok 1 - getaddrinfo >> +ok 2 - getaddrinfo retval >> +ok 3 - getaddrinfo error message >> *** test_getaddrinfo: done *** >> >> commit cd5333e3acd35602e004a48eaefefd58dbd08cdd (HEAD) Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Tue Jul 30 15:39:21 2019 +0300 coio/say: fix getaddrinfo error handling on macOS Before this patch, branch when getaddrinfo() returns error codes couldn't be reached on macOS, because they are greater than 0 on macOS (assumption "rc < 0" in commit ea1da04 is incorrect for macOS). Note: diag_log() in say.c was added, because otherwise it will be hid in the case of panic(). Also, two diag_set() in syslog_connect_unix() was added to avoid asserts in this diag_log(). Needed for #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) { /* getaddrinfo() failed */ errno = EIO; diag_set(SystemError, "getaddrinfo: %s", diff --git a/src/lib/core/say.c b/src/lib/core/say.c index 791011e6f..9841ade25 100644 --- a/src/lib/core/say.c +++ b/src/lib/core/say.c @@ -485,13 +485,16 @@ static inline int syslog_connect_unix(const char *path) { int fd = socket(PF_UNIX, SOCK_DGRAM, 0); - if (fd < 0) + if (fd < 0) { + diag_set(SystemError, strerror(errno)); return -1; + } struct sockaddr_un un; memset(&un, 0, sizeof(un)); snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); un.sun_family = AF_UNIX; if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { + diag_set(SystemError, strerror(errno)); close(fd); return -1; } @@ -532,7 +535,7 @@ syslog_connect_remote(const char *server_address) hints.ai_protocol = IPPROTO_UDP; ret = getaddrinfo(remote, portnum, &hints, &inf); - if (ret < 0) { + if (ret != 0) { errno = EIO; diag_set(SystemError, "getaddrinfo: %s", gai_strerror(ret)); @@ -619,6 +622,7 @@ log_syslog_init(struct log *log, const char *init_str) say_free_syslog_opts(&opts); log->fd = log_syslog_connect(log); if (log->fd < 0) { + diag_log(); /* syslog indent is freed in atexit(). */ diag_set(SystemError, "syslog logger: %s", strerror(errno)); return -1; diff --git a/test/unit/coio.cc b/test/unit/coio.cc index bb8bd7131..69f78829c 100644 --- a/test/unit/coio.cc +++ b/test/unit/coio.cc @@ -72,7 +72,7 @@ static void test_getaddrinfo(void) { header(); - plan(1); + plan(3); const char *host = "127.0.0.1"; const char *port = "3333"; struct addrinfo *i; @@ -81,6 +81,39 @@ test_getaddrinfo(void) is(rc, 0, "getaddrinfo"); freeaddrinfo(i); + /* + * gh-4138: Check getaddrinfo() retval and diagnostics + * area. + */ + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, + 15768000000); + isnt(rc, 0, "getaddrinfo retval"); + const char *errmsg = diag_get()->last->errmsg; + /* EAI_NONAME */ + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" + ", or not known"; + /* EAI_SERVICE */ + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " + "ai_socktype"; + /* EAI_NONAME */ + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; + /* EAI_NONAME */ + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" + ", or not known"; + /* EAI_AGAIN */ + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " + "resolution"; + /* EAI_AGAIN */ + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " + "this time"; + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || + strcmp(errmsg, exp_errmsg_2) == 0 || + strcmp(errmsg, exp_errmsg_3) == 0 || + strcmp(errmsg, exp_errmsg_4) == 0 || + strcmp(errmsg, exp_errmsg_5) == 0 || + strcmp(errmsg, exp_errmsg_6) == 0; + is(is_match_with_exp, true, "getaddrinfo error message"); + /* * gh-4209: 0 timeout should not be a special value and * detach a task. Before a fix it led to segfault diff --git a/test/unit/coio.result b/test/unit/coio.result index 5019fa48a..90b567140 100644 --- a/test/unit/coio.result +++ b/test/unit/coio.result @@ -7,6 +7,8 @@ # call done with res 0 *** test_call_f: done *** *** test_getaddrinfo *** -1..1 +1..3 ok 1 - getaddrinfo +ok 2 - getaddrinfo retval +ok 3 - getaddrinfo error message *** test_getaddrinfo: done *** ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-06-23 13:27 ` Roman Khabibov @ 2020-07-23 14:12 ` Sergey Ostanevich 2020-07-28 9:26 ` Roman Khabibov 0 siblings, 1 reply; 23+ messages in thread From: Sergey Ostanevich @ 2020-07-23 14:12 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches, alexander.turenko Hi! Thanks for the update, LGTM. Sergos On 23 Jun 16:27, Roman Khabibov wrote: > Hi! > I decided to stay diag_log() as it is. I tried to use new diag_add() from > the stacked diagnostics patch, but it don’t log this error. We have to > log this error to print error message from getaddrinfo after panic with > “say”. > > See the following reproducer: > > tarantool> socket = require('socket') > --- > ... > > tarantool> log = require('log') > --- > ... > > tarantool> fio = require('fio') > --- > ... > > tarantool> > --- > ... > > tarantool> path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock') > --- > ... > > tarantool> unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0) > --- > ... > > tarantool> unix_socket:bind('unix/', path) > --- > - false > ... > > tarantool> > --- > ... > > tarantool> opt = string.format("syslog:server=non_exists_hostname:%s,identity=tarantool", path) > --- > ... > > tarantool> box.cfg{log = opt, log_nonblock=true} > SystemError getaddrinfo: nodename nor servname provided, or not known: Input/output error > SystemError syslog logger: Input/output error: Input/output error > failed to initialize logging subsystem > > If we remove diag_log(), we will lose getaddrinfo error in the log after > panic. I didn’t add it to test, because once upon a time with Vova > we decided that panic is hard to test, and it's not worth it. > > > On Apr 17, 2020, at 12:37, Sergey Ostanevich <sergos@tarantool.org> wrote: > > > >>>> Note: diag_log() in say.c was added, because otherwise it will be > >>>> hid by the following diagnostic and it should be handled in a > >>>> better way after #1148. Also, two diag_set() in > >>> Please, notify owner of #1148 about follow-up that will be needed. > >>> > > > > As I can see from #1148 comments, it is already closed. Can you address > > the problem now with a new gh issue? > > > > Otherwise LGTM. > > > > Sergos > > > >>>> syslog_connect_unix() was added to avoid asserts in this > >>>> diag_log(). > >>>> > >>>> Need for #4138 > >>>> --- > >>>> src/lib/core/coio_task.c | 2 +- > >>>> src/lib/core/say.c | 12 ++++++++++-- > >>>> test/unit/coio.cc | 29 ++++++++++++++++++++++++++++- > >>>> test/unit/coio.result | 4 +++- > >>>> 4 files changed, 42 insertions(+), 5 deletions(-) > >>>> > >>>> 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) { > >>>> /* getaddrinfo() failed */ > >>>> errno = EIO; > >>>> diag_set(SystemError, "getaddrinfo: %s", > >>>> diff --git a/src/lib/core/say.c b/src/lib/core/say.c > >>>> index 64a637c58..8ad88ad57 100644 > >>>> --- a/src/lib/core/say.c > >>>> +++ b/src/lib/core/say.c > >>>> @@ -459,14 +459,17 @@ static inline int > >>>> syslog_connect_unix(const char *path) > >>>> { > >>>> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); > >>>> - if (fd < 0) > >>>> + if (fd < 0) { > >>>> + diag_set(SystemError, "socket"); > >>> This error message gives nothing. Please, describe the error behind it > >>> using the strerror(errno) > >>>> return -1; > >>>> + } > >>>> struct sockaddr_un un; > >>>> memset(&un, 0, sizeof(un)); > >>>> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > >>>> un.sun_family = AF_UNIX; > >>>> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { > >>>> close(fd); > >>>> + diag_set(SystemError, "connect"); > >>> Ditto. > >> @@ -465,13 +465,16 @@ static inline int > >> syslog_connect_unix(const char *path) > >> { > >> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); > >> - if (fd < 0) > >> + if (fd < 0) { > >> + diag_set(SystemError, strerror(errno)); > >> return -1; > >> + } > >> struct sockaddr_un un; > >> memset(&un, 0, sizeof(un)); > >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > >> un.sun_family = AF_UNIX; > >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { > >> + diag_set(SystemError, strerror(errno)); > >> close(fd); > >> return -1; > >> } > >> > >>>> return -1; > >>>> } > >>>> return fd; > >>>> @@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address) > >>>> hints.ai_protocol = IPPROTO_UDP; > >>>> > >>>> ret = getaddrinfo(remote, portnum, &hints, &inf); > >>>> - if (ret < 0) { > >>>> + if (ret != 0) { > >>>> errno = EIO; > >>>> diag_set(SystemError, "getaddrinfo: %s", > >>>> gai_strerror(ret)); > >>>> @@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str) > >>>> say_free_syslog_opts(&opts); > >>>> log->fd = log_syslog_connect(log); > >>>> if (log->fd < 0) { > >>>> + /* > >>>> + * We need to log a diagnostics here until stacked > >>>> + * diagnostics will be implemented (#1148). > >>>> + */ > >>>> + diag_log(); > >>> Make a poniter about this in #1148 > >> Ok. > >> > >>>> /* syslog indent is freed in atexit(). */ > >>>> diag_set(SystemError, "syslog logger: %s", strerror(errno)); > >>>> return -1; > >>>> diff --git a/test/unit/coio.cc b/test/unit/coio.cc > >>>> index bb8bd7131..957c58ede 100644 > >>>> --- a/test/unit/coio.cc > >>>> +++ b/test/unit/coio.cc > >>>> @@ -72,7 +72,7 @@ static void > >>>> test_getaddrinfo(void) > >>>> { > >>>> header(); > >>>> - plan(1); > >>>> + plan(3); > >>>> const char *host = "127.0.0.1"; > >>>> const char *port = "3333"; > >>>> struct addrinfo *i; > >>>> @@ -81,6 +81,33 @@ test_getaddrinfo(void) > >>>> is(rc, 0, "getaddrinfo"); > >>>> freeaddrinfo(i); > >>>> > >>>> + /* > >>>> + * gh-4138: Check getaddrinfo() retval and diagnostics > >>>> + * area. > >>>> + */ > >>>> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, > >>>> + 15768000000); > >>>> + isnt(rc, 0, "getaddrinfo retval"); > >>>> + const char *errmsg = diag_get()->last->errmsg; > >>>> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" > >>>> + ", or not known"; > >>>> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " > >>>> + "ai_socktype"; > >>>> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; > >>>> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" > >>>> + ", or not known"; > >>>> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " > >>>> + "resolution"; > >>>> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " > >>>> + "this time"; > >>>> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || > >>>> + strcmp(errmsg, exp_errmsg_2) == 0 || > >>>> + strcmp(errmsg, exp_errmsg_3) == 0 || > >>>> + strcmp(errmsg, exp_errmsg_4) == 0 || > >>>> + strcmp(errmsg, exp_errmsg_5) == 0 || > >>>> + strcmp(errmsg, exp_errmsg_6) == 0; > >>>> + is(is_match_with_exp, true, "getaddrinfo error message"); > >>>> + > >>> Why did you made such a test - you're not sure which one will be > >>> triggered? Can you create a test that will check all possible errors? > >> See Alexander answer. I added comments about the constants. > >> > >>>> /* > >>>> * gh-4209: 0 timeout should not be a special value and > >>>> * detach a task. Before a fix it led to segfault > >>>> diff --git a/test/unit/coio.result b/test/unit/coio.result > >>>> index 5019fa48a..90b567140 100644 > >>>> --- a/test/unit/coio.result > >>>> +++ b/test/unit/coio.result > >>>> @@ -7,6 +7,8 @@ > >>>> # call done with res 0 > >>>> *** test_call_f: done *** > >>>> *** test_getaddrinfo *** > >>>> -1..1 > >>>> +1..3 > >>>> ok 1 - getaddrinfo > >>>> +ok 2 - getaddrinfo retval > >>>> +ok 3 - getaddrinfo error message > >>>> *** test_getaddrinfo: done *** > >>>> -- > >>>> 2.21.0 (Apple Git-122) > >> > >> commit f17e3e73ae2689dd2ec1dcd94d699636f19f93a5 > >> Author: Roman Khabibov <roman.habibov@tarantool.org> > >> Date: Tue Jul 30 15:39:21 2019 +0300 > >> > >> coio/say: fix getaddrinfo error handling on macOS > >> > >> Before this patch, branch when getaddrinfo() returns error codes > >> couldn't be reached on macOS, because they are greater than 0 on > >> macOS (assumption "rc < 0" in commit ea1da04 is incorrect for > >> macOS). > >> > >> Note: diag_log() in say.c was added, because otherwise it will be > >> hid by the following diagnostic and it should be handled in a > >> better way after #1148. Also, two diag_set() in > >> syslog_connect_unix() was added to avoid asserts in this > >> diag_log(). > >> > >> Need for #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) { > >> /* getaddrinfo() failed */ > >> errno = EIO; > >> diag_set(SystemError, "getaddrinfo: %s", > >> diff --git a/src/lib/core/say.c b/src/lib/core/say.c > >> index dd05285a6..0f8db4587 100644 > >> --- a/src/lib/core/say.c > >> +++ b/src/lib/core/say.c > >> @@ -465,13 +465,16 @@ static inline int > >> syslog_connect_unix(const char *path) > >> { > >> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); > >> - if (fd < 0) > >> + if (fd < 0) { > >> + diag_set(SystemError, strerror(errno)); > >> return -1; > >> + } > >> struct sockaddr_un un; > >> memset(&un, 0, sizeof(un)); > >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > >> un.sun_family = AF_UNIX; > >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { > >> + diag_set(SystemError, strerror(errno)); > >> close(fd); > >> return -1; > >> } > >> @@ -512,7 +515,7 @@ syslog_connect_remote(const char *server_address) > >> hints.ai_protocol = IPPROTO_UDP; > >> > >> ret = getaddrinfo(remote, portnum, &hints, &inf); > >> - if (ret < 0) { > >> + if (ret != 0) { > >> errno = EIO; > >> diag_set(SystemError, "getaddrinfo: %s", > >> gai_strerror(ret)); > >> @@ -599,6 +602,11 @@ log_syslog_init(struct log *log, const char *init_str) > >> say_free_syslog_opts(&opts); > >> log->fd = log_syslog_connect(log); > >> if (log->fd < 0) { > >> + /* > >> + * We need to log a diagnostics here until stacked > >> + * diagnostics will be implemented (#1148). > >> + */ > >> + diag_log(); > >> /* syslog indent is freed in atexit(). */ > >> diag_set(SystemError, "syslog logger: %s", strerror(errno)); > >> return -1; > >> diff --git a/test/unit/coio.cc b/test/unit/coio.cc > >> index bb8bd7131..69f78829c 100644 > >> --- a/test/unit/coio.cc > >> +++ b/test/unit/coio.cc > >> @@ -72,7 +72,7 @@ static void > >> test_getaddrinfo(void) > >> { > >> header(); > >> - plan(1); > >> + plan(3); > >> const char *host = "127.0.0.1"; > >> const char *port = "3333"; > >> struct addrinfo *i; > >> @@ -81,6 +81,39 @@ test_getaddrinfo(void) > >> is(rc, 0, "getaddrinfo"); > >> freeaddrinfo(i); > >> > >> + /* > >> + * gh-4138: Check getaddrinfo() retval and diagnostics > >> + * area. > >> + */ > >> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, > >> + 15768000000); > >> + isnt(rc, 0, "getaddrinfo retval"); > >> + const char *errmsg = diag_get()->last->errmsg; > >> + /* EAI_NONAME */ > >> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" > >> + ", or not known"; > >> + /* EAI_SERVICE */ > >> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " > >> + "ai_socktype"; > >> + /* EAI_NONAME */ > >> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; > >> + /* EAI_NONAME */ > >> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" > >> + ", or not known"; > >> + /* EAI_AGAIN */ > >> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " > >> + "resolution"; > >> + /* EAI_AGAIN */ > >> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " > >> + "this time"; > >> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || > >> + strcmp(errmsg, exp_errmsg_2) == 0 || > >> + strcmp(errmsg, exp_errmsg_3) == 0 || > >> + strcmp(errmsg, exp_errmsg_4) == 0 || > >> + strcmp(errmsg, exp_errmsg_5) == 0 || > >> + strcmp(errmsg, exp_errmsg_6) == 0; > >> + is(is_match_with_exp, true, "getaddrinfo error message"); > >> + > >> /* > >> * gh-4209: 0 timeout should not be a special value and > >> * detach a task. Before a fix it led to segfault > >> diff --git a/test/unit/coio.result b/test/unit/coio.result > >> index 5019fa48a..90b567140 100644 > >> --- a/test/unit/coio.result > >> +++ b/test/unit/coio.result > >> @@ -7,6 +7,8 @@ > >> # call done with res 0 > >> *** test_call_f: done *** > >> *** test_getaddrinfo *** > >> -1..1 > >> +1..3 > >> ok 1 - getaddrinfo > >> +ok 2 - getaddrinfo retval > >> +ok 3 - getaddrinfo error message > >> *** test_getaddrinfo: done *** > >> > >> > > commit cd5333e3acd35602e004a48eaefefd58dbd08cdd (HEAD) > Author: Roman Khabibov <roman.habibov@tarantool.org> > Date: Tue Jul 30 15:39:21 2019 +0300 > > coio/say: fix getaddrinfo error handling on macOS > > Before this patch, branch when getaddrinfo() returns error codes > couldn't be reached on macOS, because they are greater than 0 on > macOS (assumption "rc < 0" in commit ea1da04 is incorrect for > macOS). > > Note: diag_log() in say.c was added, because otherwise it will be > hid in the case of panic(). Also, two diag_set() in > syslog_connect_unix() was added to avoid asserts in this > diag_log(). > > Needed for #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) { > /* getaddrinfo() failed */ > errno = EIO; > diag_set(SystemError, "getaddrinfo: %s", > diff --git a/src/lib/core/say.c b/src/lib/core/say.c > index 791011e6f..9841ade25 100644 > --- a/src/lib/core/say.c > +++ b/src/lib/core/say.c > @@ -485,13 +485,16 @@ static inline int > syslog_connect_unix(const char *path) > { > int fd = socket(PF_UNIX, SOCK_DGRAM, 0); > - if (fd < 0) > + if (fd < 0) { > + diag_set(SystemError, strerror(errno)); > return -1; > + } > struct sockaddr_un un; > memset(&un, 0, sizeof(un)); > snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > un.sun_family = AF_UNIX; > if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { > + diag_set(SystemError, strerror(errno)); > close(fd); > return -1; > } > @@ -532,7 +535,7 @@ syslog_connect_remote(const char *server_address) > hints.ai_protocol = IPPROTO_UDP; > > ret = getaddrinfo(remote, portnum, &hints, &inf); > - if (ret < 0) { > + if (ret != 0) { > errno = EIO; > diag_set(SystemError, "getaddrinfo: %s", > gai_strerror(ret)); > @@ -619,6 +622,7 @@ log_syslog_init(struct log *log, const char *init_str) > say_free_syslog_opts(&opts); > log->fd = log_syslog_connect(log); > if (log->fd < 0) { > + diag_log(); > /* syslog indent is freed in atexit(). */ > diag_set(SystemError, "syslog logger: %s", strerror(errno)); > return -1; > diff --git a/test/unit/coio.cc b/test/unit/coio.cc > index bb8bd7131..69f78829c 100644 > --- a/test/unit/coio.cc > +++ b/test/unit/coio.cc > @@ -72,7 +72,7 @@ static void > test_getaddrinfo(void) > { > header(); > - plan(1); > + plan(3); > const char *host = "127.0.0.1"; > const char *port = "3333"; > struct addrinfo *i; > @@ -81,6 +81,39 @@ test_getaddrinfo(void) > is(rc, 0, "getaddrinfo"); > freeaddrinfo(i); > > + /* > + * gh-4138: Check getaddrinfo() retval and diagnostics > + * area. > + */ > + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, > + 15768000000); > + isnt(rc, 0, "getaddrinfo retval"); > + const char *errmsg = diag_get()->last->errmsg; > + /* EAI_NONAME */ > + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" > + ", or not known"; > + /* EAI_SERVICE */ > + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " > + "ai_socktype"; > + /* EAI_NONAME */ > + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; > + /* EAI_NONAME */ > + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" > + ", or not known"; > + /* EAI_AGAIN */ > + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " > + "resolution"; > + /* EAI_AGAIN */ > + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " > + "this time"; > + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || > + strcmp(errmsg, exp_errmsg_2) == 0 || > + strcmp(errmsg, exp_errmsg_3) == 0 || > + strcmp(errmsg, exp_errmsg_4) == 0 || > + strcmp(errmsg, exp_errmsg_5) == 0 || > + strcmp(errmsg, exp_errmsg_6) == 0; > + is(is_match_with_exp, true, "getaddrinfo error message"); > + > /* > * gh-4209: 0 timeout should not be a special value and > * detach a task. Before a fix it led to segfault > diff --git a/test/unit/coio.result b/test/unit/coio.result > index 5019fa48a..90b567140 100644 > --- a/test/unit/coio.result > +++ b/test/unit/coio.result > @@ -7,6 +7,8 @@ > # call done with res 0 > *** test_call_f: done *** > *** test_getaddrinfo *** > -1..1 > +1..3 > ok 1 - getaddrinfo > +ok 2 - getaddrinfo retval > +ok 3 - getaddrinfo error message > *** test_getaddrinfo: done *** > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-07-23 14:12 ` Sergey Ostanevich @ 2020-07-28 9:26 ` Roman Khabibov 0 siblings, 0 replies; 23+ messages in thread From: Roman Khabibov @ 2020-07-28 9:26 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches, Alexander Turenko Hi! Kirill, you can push. > On Jul 23, 2020, at 17:12, Sergey Ostanevich <sergos@tarantool.org> wrote: > > Hi! > > Thanks for the update, LGTM. > > Sergos > > On 23 Jun 16:27, Roman Khabibov wrote: >> Hi! >> I decided to stay diag_log() as it is. I tried to use new diag_add() from >> the stacked diagnostics patch, but it don’t log this error. We have to >> log this error to print error message from getaddrinfo after panic with >> “say”. >> >> See the following reproducer: >> >> tarantool> socket = require('socket') >> --- >> ... >> >> tarantool> log = require('log') >> --- >> ... >> >> tarantool> fio = require('fio') >> --- >> ... >> >> tarantool> >> --- >> ... >> >> tarantool> path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock') >> --- >> ... >> >> tarantool> unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0) >> --- >> ... >> >> tarantool> unix_socket:bind('unix/', path) >> --- >> - false >> ... >> >> tarantool> >> --- >> ... >> >> tarantool> opt = string.format("syslog:server=non_exists_hostname:%s,identity=tarantool", path) >> --- >> ... >> >> tarantool> box.cfg{log = opt, log_nonblock=true} >> SystemError getaddrinfo: nodename nor servname provided, or not known: Input/output error >> SystemError syslog logger: Input/output error: Input/output error >> failed to initialize logging subsystem >> >> If we remove diag_log(), we will lose getaddrinfo error in the log after >> panic. I didn’t add it to test, because once upon a time with Vova >> we decided that panic is hard to test, and it's not worth it. >> >>> On Apr 17, 2020, at 12:37, Sergey Ostanevich <sergos@tarantool.org> wrote: >>> >>>>>> Note: diag_log() in say.c was added, because otherwise it will be >>>>>> hid by the following diagnostic and it should be handled in a >>>>>> better way after #1148. Also, two diag_set() in >>>>> Please, notify owner of #1148 about follow-up that will be needed. >>>>> >>> >>> As I can see from #1148 comments, it is already closed. Can you address >>> the problem now with a new gh issue? >>> >>> Otherwise LGTM. >>> >>> Sergos >>> >>>>>> syslog_connect_unix() was added to avoid asserts in this >>>>>> diag_log(). >>>>>> >>>>>> Need for #4138 >>>>>> --- >>>>>> src/lib/core/coio_task.c | 2 +- >>>>>> src/lib/core/say.c | 12 ++++++++++-- >>>>>> test/unit/coio.cc | 29 ++++++++++++++++++++++++++++- >>>>>> test/unit/coio.result | 4 +++- >>>>>> 4 files changed, 42 insertions(+), 5 deletions(-) >>>>>> >>>>>> 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) { >>>>>> /* getaddrinfo() failed */ >>>>>> errno = EIO; >>>>>> diag_set(SystemError, "getaddrinfo: %s", >>>>>> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >>>>>> index 64a637c58..8ad88ad57 100644 >>>>>> --- a/src/lib/core/say.c >>>>>> +++ b/src/lib/core/say.c >>>>>> @@ -459,14 +459,17 @@ static inline int >>>>>> syslog_connect_unix(const char *path) >>>>>> { >>>>>> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >>>>>> - if (fd < 0) >>>>>> + if (fd < 0) { >>>>>> + diag_set(SystemError, "socket"); >>>>> This error message gives nothing. Please, describe the error behind it >>>>> using the strerror(errno) >>>>>> return -1; >>>>>> + } >>>>>> struct sockaddr_un un; >>>>>> memset(&un, 0, sizeof(un)); >>>>>> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >>>>>> un.sun_family = AF_UNIX; >>>>>> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >>>>>> close(fd); >>>>>> + diag_set(SystemError, "connect"); >>>>> Ditto. >>>> @@ -465,13 +465,16 @@ static inline int >>>> syslog_connect_unix(const char *path) >>>> { >>>> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >>>> - if (fd < 0) >>>> + if (fd < 0) { >>>> + diag_set(SystemError, strerror(errno)); >>>> return -1; >>>> + } >>>> struct sockaddr_un un; >>>> memset(&un, 0, sizeof(un)); >>>> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >>>> un.sun_family = AF_UNIX; >>>> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >>>> + diag_set(SystemError, strerror(errno)); >>>> close(fd); >>>> return -1; >>>> } >>>> >>>>>> return -1; >>>>>> } >>>>>> return fd; >>>>>> @@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address) >>>>>> hints.ai_protocol = IPPROTO_UDP; >>>>>> >>>>>> ret = getaddrinfo(remote, portnum, &hints, &inf); >>>>>> - if (ret < 0) { >>>>>> + if (ret != 0) { >>>>>> errno = EIO; >>>>>> diag_set(SystemError, "getaddrinfo: %s", >>>>>> gai_strerror(ret)); >>>>>> @@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str) >>>>>> say_free_syslog_opts(&opts); >>>>>> log->fd = log_syslog_connect(log); >>>>>> if (log->fd < 0) { >>>>>> + /* >>>>>> + * We need to log a diagnostics here until stacked >>>>>> + * diagnostics will be implemented (#1148). >>>>>> + */ >>>>>> + diag_log(); >>>>> Make a poniter about this in #1148 >>>> Ok. >>>> >>>>>> /* syslog indent is freed in atexit(). */ >>>>>> diag_set(SystemError, "syslog logger: %s", strerror(errno)); >>>>>> return -1; >>>>>> diff --git a/test/unit/coio.cc b/test/unit/coio.cc >>>>>> index bb8bd7131..957c58ede 100644 >>>>>> --- a/test/unit/coio.cc >>>>>> +++ b/test/unit/coio.cc >>>>>> @@ -72,7 +72,7 @@ static void >>>>>> test_getaddrinfo(void) >>>>>> { >>>>>> header(); >>>>>> - plan(1); >>>>>> + plan(3); >>>>>> const char *host = "127.0.0.1"; >>>>>> const char *port = "3333"; >>>>>> struct addrinfo *i; >>>>>> @@ -81,6 +81,33 @@ test_getaddrinfo(void) >>>>>> is(rc, 0, "getaddrinfo"); >>>>>> freeaddrinfo(i); >>>>>> >>>>>> + /* >>>>>> + * gh-4138: Check getaddrinfo() retval and diagnostics >>>>>> + * area. >>>>>> + */ >>>>>> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, >>>>>> + 15768000000); >>>>>> + isnt(rc, 0, "getaddrinfo retval"); >>>>>> + const char *errmsg = diag_get()->last->errmsg; >>>>>> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" >>>>>> + ", or not known"; >>>>>> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " >>>>>> + "ai_socktype"; >>>>>> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; >>>>>> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" >>>>>> + ", or not known"; >>>>>> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " >>>>>> + "resolution"; >>>>>> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " >>>>>> + "this time"; >>>>>> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || >>>>>> + strcmp(errmsg, exp_errmsg_2) == 0 || >>>>>> + strcmp(errmsg, exp_errmsg_3) == 0 || >>>>>> + strcmp(errmsg, exp_errmsg_4) == 0 || >>>>>> + strcmp(errmsg, exp_errmsg_5) == 0 || >>>>>> + strcmp(errmsg, exp_errmsg_6) == 0; >>>>>> + is(is_match_with_exp, true, "getaddrinfo error message"); >>>>>> + >>>>> Why did you made such a test - you're not sure which one will be >>>>> triggered? Can you create a test that will check all possible errors? >>>> See Alexander answer. I added comments about the constants. >>>> >>>>>> /* >>>>>> * gh-4209: 0 timeout should not be a special value and >>>>>> * detach a task. Before a fix it led to segfault >>>>>> diff --git a/test/unit/coio.result b/test/unit/coio.result >>>>>> index 5019fa48a..90b567140 100644 >>>>>> --- a/test/unit/coio.result >>>>>> +++ b/test/unit/coio.result >>>>>> @@ -7,6 +7,8 @@ >>>>>> # call done with res 0 >>>>>> *** test_call_f: done *** >>>>>> *** test_getaddrinfo *** >>>>>> -1..1 >>>>>> +1..3 >>>>>> ok 1 - getaddrinfo >>>>>> +ok 2 - getaddrinfo retval >>>>>> +ok 3 - getaddrinfo error message >>>>>> *** test_getaddrinfo: done *** >>>>>> -- >>>>>> 2.21.0 (Apple Git-122) >>>> >>>> commit f17e3e73ae2689dd2ec1dcd94d699636f19f93a5 >>>> Author: Roman Khabibov <roman.habibov@tarantool.org> >>>> Date: Tue Jul 30 15:39:21 2019 +0300 >>>> >>>> coio/say: fix getaddrinfo error handling on macOS >>>> >>>> Before this patch, branch when getaddrinfo() returns error codes >>>> couldn't be reached on macOS, because they are greater than 0 on >>>> macOS (assumption "rc < 0" in commit ea1da04 is incorrect for >>>> macOS). >>>> >>>> Note: diag_log() in say.c was added, because otherwise it will be >>>> hid by the following diagnostic and it should be handled in a >>>> better way after #1148. Also, two diag_set() in >>>> syslog_connect_unix() was added to avoid asserts in this >>>> diag_log(). >>>> >>>> Need for #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) { >>>> /* getaddrinfo() failed */ >>>> errno = EIO; >>>> diag_set(SystemError, "getaddrinfo: %s", >>>> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >>>> index dd05285a6..0f8db4587 100644 >>>> --- a/src/lib/core/say.c >>>> +++ b/src/lib/core/say.c >>>> @@ -465,13 +465,16 @@ static inline int >>>> syslog_connect_unix(const char *path) >>>> { >>>> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >>>> - if (fd < 0) >>>> + if (fd < 0) { >>>> + diag_set(SystemError, strerror(errno)); >>>> return -1; >>>> + } >>>> struct sockaddr_un un; >>>> memset(&un, 0, sizeof(un)); >>>> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >>>> un.sun_family = AF_UNIX; >>>> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >>>> + diag_set(SystemError, strerror(errno)); >>>> close(fd); >>>> return -1; >>>> } >>>> @@ -512,7 +515,7 @@ syslog_connect_remote(const char *server_address) >>>> hints.ai_protocol = IPPROTO_UDP; >>>> >>>> ret = getaddrinfo(remote, portnum, &hints, &inf); >>>> - if (ret < 0) { >>>> + if (ret != 0) { >>>> errno = EIO; >>>> diag_set(SystemError, "getaddrinfo: %s", >>>> gai_strerror(ret)); >>>> @@ -599,6 +602,11 @@ log_syslog_init(struct log *log, const char *init_str) >>>> say_free_syslog_opts(&opts); >>>> log->fd = log_syslog_connect(log); >>>> if (log->fd < 0) { >>>> + /* >>>> + * We need to log a diagnostics here until stacked >>>> + * diagnostics will be implemented (#1148). >>>> + */ >>>> + diag_log(); >>>> /* syslog indent is freed in atexit(). */ >>>> diag_set(SystemError, "syslog logger: %s", strerror(errno)); >>>> return -1; >>>> diff --git a/test/unit/coio.cc b/test/unit/coio.cc >>>> index bb8bd7131..69f78829c 100644 >>>> --- a/test/unit/coio.cc >>>> +++ b/test/unit/coio.cc >>>> @@ -72,7 +72,7 @@ static void >>>> test_getaddrinfo(void) >>>> { >>>> header(); >>>> - plan(1); >>>> + plan(3); >>>> const char *host = "127.0.0.1"; >>>> const char *port = "3333"; >>>> struct addrinfo *i; >>>> @@ -81,6 +81,39 @@ test_getaddrinfo(void) >>>> is(rc, 0, "getaddrinfo"); >>>> freeaddrinfo(i); >>>> >>>> + /* >>>> + * gh-4138: Check getaddrinfo() retval and diagnostics >>>> + * area. >>>> + */ >>>> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, >>>> + 15768000000); >>>> + isnt(rc, 0, "getaddrinfo retval"); >>>> + const char *errmsg = diag_get()->last->errmsg; >>>> + /* EAI_NONAME */ >>>> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" >>>> + ", or not known"; >>>> + /* EAI_SERVICE */ >>>> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " >>>> + "ai_socktype"; >>>> + /* EAI_NONAME */ >>>> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; >>>> + /* EAI_NONAME */ >>>> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" >>>> + ", or not known"; >>>> + /* EAI_AGAIN */ >>>> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " >>>> + "resolution"; >>>> + /* EAI_AGAIN */ >>>> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " >>>> + "this time"; >>>> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || >>>> + strcmp(errmsg, exp_errmsg_2) == 0 || >>>> + strcmp(errmsg, exp_errmsg_3) == 0 || >>>> + strcmp(errmsg, exp_errmsg_4) == 0 || >>>> + strcmp(errmsg, exp_errmsg_5) == 0 || >>>> + strcmp(errmsg, exp_errmsg_6) == 0; >>>> + is(is_match_with_exp, true, "getaddrinfo error message"); >>>> + >>>> /* >>>> * gh-4209: 0 timeout should not be a special value and >>>> * detach a task. Before a fix it led to segfault >>>> diff --git a/test/unit/coio.result b/test/unit/coio.result >>>> index 5019fa48a..90b567140 100644 >>>> --- a/test/unit/coio.result >>>> +++ b/test/unit/coio.result >>>> @@ -7,6 +7,8 @@ >>>> # call done with res 0 >>>> *** test_call_f: done *** >>>> *** test_getaddrinfo *** >>>> -1..1 >>>> +1..3 >>>> ok 1 - getaddrinfo >>>> +ok 2 - getaddrinfo retval >>>> +ok 3 - getaddrinfo error message >>>> *** test_getaddrinfo: done *** >>>> >>>> >> >> commit cd5333e3acd35602e004a48eaefefd58dbd08cdd (HEAD) >> Author: Roman Khabibov <roman.habibov@tarantool.org> >> Date: Tue Jul 30 15:39:21 2019 +0300 >> >> coio/say: fix getaddrinfo error handling on macOS >> >> Before this patch, branch when getaddrinfo() returns error codes >> couldn't be reached on macOS, because they are greater than 0 on >> macOS (assumption "rc < 0" in commit ea1da04 is incorrect for >> macOS). >> >> Note: diag_log() in say.c was added, because otherwise it will be >> hid in the case of panic(). Also, two diag_set() in >> syslog_connect_unix() was added to avoid asserts in this >> diag_log(). >> >> Needed for #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) { >> /* getaddrinfo() failed */ >> errno = EIO; >> diag_set(SystemError, "getaddrinfo: %s", >> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >> index 791011e6f..9841ade25 100644 >> --- a/src/lib/core/say.c >> +++ b/src/lib/core/say.c >> @@ -485,13 +485,16 @@ static inline int >> syslog_connect_unix(const char *path) >> { >> int fd = socket(PF_UNIX, SOCK_DGRAM, 0); >> - if (fd < 0) >> + if (fd < 0) { >> + diag_set(SystemError, strerror(errno)); >> return -1; >> + } >> struct sockaddr_un un; >> memset(&un, 0, sizeof(un)); >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >> un.sun_family = AF_UNIX; >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) { >> + diag_set(SystemError, strerror(errno)); >> close(fd); >> return -1; >> } >> @@ -532,7 +535,7 @@ syslog_connect_remote(const char *server_address) >> hints.ai_protocol = IPPROTO_UDP; >> >> ret = getaddrinfo(remote, portnum, &hints, &inf); >> - if (ret < 0) { >> + if (ret != 0) { >> errno = EIO; >> diag_set(SystemError, "getaddrinfo: %s", >> gai_strerror(ret)); >> @@ -619,6 +622,7 @@ log_syslog_init(struct log *log, const char *init_str) >> say_free_syslog_opts(&opts); >> log->fd = log_syslog_connect(log); >> if (log->fd < 0) { >> + diag_log(); >> /* syslog indent is freed in atexit(). */ >> diag_set(SystemError, "syslog logger: %s", strerror(errno)); >> return -1; >> diff --git a/test/unit/coio.cc b/test/unit/coio.cc >> index bb8bd7131..69f78829c 100644 >> --- a/test/unit/coio.cc >> +++ b/test/unit/coio.cc >> @@ -72,7 +72,7 @@ static void >> test_getaddrinfo(void) >> { >> header(); >> - plan(1); >> + plan(3); >> const char *host = "127.0.0.1"; >> const char *port = "3333"; >> struct addrinfo *i; >> @@ -81,6 +81,39 @@ test_getaddrinfo(void) >> is(rc, 0, "getaddrinfo"); >> freeaddrinfo(i); >> >> + /* >> + * gh-4138: Check getaddrinfo() retval and diagnostics >> + * area. >> + */ >> + rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, >> + 15768000000); >> + isnt(rc, 0, "getaddrinfo retval"); >> + const char *errmsg = diag_get()->last->errmsg; >> + /* EAI_NONAME */ >> + const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided" >> + ", or not known"; >> + /* EAI_SERVICE */ >> + const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for " >> + "ai_socktype"; >> + /* EAI_NONAME */ >> + const char *exp_errmsg_3 = "getaddrinfo: Name or service not known"; >> + /* EAI_NONAME */ >> + const char *exp_errmsg_4 = "getaddrinfo: hostname nor servname provided" >> + ", or not known"; >> + /* EAI_AGAIN */ >> + const char *exp_errmsg_5 = "getaddrinfo: Temporary failure in name " >> + "resolution"; >> + /* EAI_AGAIN */ >> + const char *exp_errmsg_6 = "getaddrinfo: Name could not be resolved at " >> + "this time"; >> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 || >> + strcmp(errmsg, exp_errmsg_2) == 0 || >> + strcmp(errmsg, exp_errmsg_3) == 0 || >> + strcmp(errmsg, exp_errmsg_4) == 0 || >> + strcmp(errmsg, exp_errmsg_5) == 0 || >> + strcmp(errmsg, exp_errmsg_6) == 0; >> + is(is_match_with_exp, true, "getaddrinfo error message"); >> + >> /* >> * gh-4209: 0 timeout should not be a special value and >> * detach a task. Before a fix it led to segfault >> diff --git a/test/unit/coio.result b/test/unit/coio.result >> index 5019fa48a..90b567140 100644 >> --- a/test/unit/coio.result >> +++ b/test/unit/coio.result >> @@ -7,6 +7,8 @@ >> # call done with res 0 >> *** test_call_f: done *** >> *** test_getaddrinfo *** >> -1..1 >> +1..3 >> ok 1 - getaddrinfo >> +ok 2 - getaddrinfo retval >> +ok 3 - getaddrinfo error message >> *** test_getaddrinfo: done *** >> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS 2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS Roman Khabibov 2020-03-29 8:51 ` Sergey Ostanevich @ 2020-04-26 17:20 ` Vladislav Shpilevoy 1 sibling, 0 replies; 23+ messages in thread From: Vladislav Shpilevoy @ 2020-04-26 17:20 UTC (permalink / raw) To: Roman Khabibov, tarantool-patches Hi! I didn't review the patch. Just noticed, that you need the stacked diag in one place, and I have a comment on that. > diff --git a/src/lib/core/say.c b/src/lib/core/say.c > index 64a637c58..8ad88ad57 100644 > --- a/src/lib/core/say.c > +++ b/src/lib/core/say.c > @@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str) > say_free_syslog_opts(&opts); > log->fd = log_syslog_connect(log); > if (log->fd < 0) { > + /* > + * We need to log a diagnostics here until stacked > + * diagnostics will be implemented (#1148). > + */ > + diag_log(); Stacked diagnostics is implemented now. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-03-12 10:24 [Tarantool-patches] [PATCH v2 0/2] Return getaddrinfo() errors Roman Khabibov 2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS Roman Khabibov @ 2020-03-12 10:24 ` Roman Khabibov 2020-03-29 9:07 ` Sergey Ostanevich 2020-07-28 14:08 ` [Tarantool-patches] [PATCH v2 0/2] Return " Kirill Yukhin 2 siblings, 1 reply; 23+ messages in thread From: Roman Khabibov @ 2020-03-12 10:24 UTC (permalink / raw) To: tarantool-patches Add getaddrinfo() errors into the several fuctions of socket. Now getaddrinfo() can return a pair of values (nil and error message) in case of error. Closes #4138 @TarantoolBot document Title: socket API changes * socket.getaddrinfo() Can return error message as second return value in case of internal getaddrinfo() error. Example: tarantool> socket.getaddrinfo('non_exists_hostname', 3301) --- - null - 'getaddrinfo: nodename nor servname provided, or not known' ... * socket.tcp_connect() Can return socket.getaddrinfo() error message as second return value. Example: tarantool> socket.tcp_connect('non_exists_hostname', 3301) --- - null - 'getaddrinfo: nodename nor servname provided, or not known' ... --- src/box/lua/net_box.lua | 7 +++-- src/lua/socket.c | 16 +++++++++- src/lua/socket.lua | 22 ++++++++++---- test/app/socket.result | 63 ++++++++++++++++++++++++++++++++++++--- test/app/socket.test.lua | 35 ++++++++++++++++++++-- test/box/net.box.result | 31 +++++++++++++++++++ test/box/net.box.test.lua | 20 +++++++++++++ 7 files changed, 179 insertions(+), 15 deletions(-) diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index 3f611c027..8068a8637 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -154,9 +154,12 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end local function establish_connection(host, port, timeout) local timeout = timeout or DEFAULT_CONNECT_TIMEOUT local begin = fiber.clock() - local s = socket.tcp_connect(host, port, timeout) + local s, err = socket.tcp_connect(host, port, timeout) if not s then - return nil, errno.strerror(errno()) + if not err then + return nil, errno.strerror(errno()) + end + return nil, err end local msg = s:read({chunk = IPROTO_GREETING_SIZE}, timeout - (fiber.clock() - begin)) diff --git a/src/lua/socket.c b/src/lua/socket.c index e75a8802e..ba683ad3a 100644 --- a/src/lua/socket.c +++ b/src/lua/socket.c @@ -775,6 +775,18 @@ lbox_getaddrinfo_result_wrapper(struct lua_State *L) return 1; } +/** + * 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 (first is nil, second is error + * message). + */ static int lbox_socket_getaddrinfo(struct lua_State *L) { @@ -817,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L) if (dns_res != 0) { lua_pushnil(L); - return 1; + struct error *err = diag_get()->last; + lua_pushstring(L, err->errmsg); + return 2; } /* no results */ diff --git a/src/lua/socket.lua b/src/lua/socket.lua index a334ad45b..ce6dab301 100644 --- a/src/lua/socket.lua +++ b/src/lua/socket.lua @@ -1041,9 +1041,12 @@ 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 + if dns == nil then + return nil, err + end + if #dns == 0 then boxerrno(boxerrno.EINVAL) return nil end @@ -1360,8 +1363,12 @@ 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) - if dns == nil or #dns == 0 then + local dns, err = getaddrinfo(host, port, timeout, ga_opts) + if dns == nil then + self._errno = boxerrno.EINVAL + return nil, err + end + if #dns == 0 then self._errno = boxerrno.EINVAL return nil, socket_error(self) end @@ -1547,9 +1554,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 end setmetatable(s, lsocket_tcp_client_mt) return s diff --git a/test/app/socket.result b/test/app/socket.result index 9f0ea0a5d..3f26bf2b0 100644 --- a/test/app/socket.result +++ b/test/app/socket.result @@ -941,10 +941,20 @@ sc:close() - true ... -- tcp_connect --- test timeout -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) +-- Test timeout. In this test, tcp_connect can return the second +-- output value from internal.getaddrinfo (usually on Mac OS, but +-- theoretically it can happen on Linux too). Sometimes +-- getaddrinfo() is timed out, sometimes connect. On Linux however +-- getaddrinfo is fast enough to never give timeout error in +-- the case. So, there are two sources of timeout errors that are +-- reported differently. This difference has appeared after +-- gh-4138 patch. +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001) --- -- null +... +s == nil +--- +- true ... -- AF_INET s = socket('AF_INET', 'SOCK_STREAM', 'tcp') @@ -1595,7 +1605,7 @@ fio.stat(path) == nil { socket.tcp_connect('abrakadabra#123') == nil, errno.strerror() } --- - - true - - Invalid argument + - Input/output error ... -- wrong options for getaddrinfo socket.getaddrinfo('host', 'port', { type = 'WRONG' }) == nil and errno() == errno.EINVAL @@ -2914,3 +2924,48 @@ srv:close() --- - true ... +-- gh-4138 Check getaddrinfo() error from socket:connect() only. +-- Error code and error message returned by getaddrinfo() depends +-- on system's gai_strerror(). +test_run:cmd("setopt delimiter ';'") +--- +- true +... +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' or + err == 'getaddrinfo: hostname nor servname provided, or not known' or + err == 'getaddrinfo: Temporary failure in name resolution' or + err == 'getaddrinfo: Name could not be resolved at this time' then + return true + end + return false +end; +--- +... +test_run:cmd("setopt delimiter ''"); +--- +- true +... +s, err = socket.getaddrinfo('non_exists_hostname', 3301) +--- +... +check_err(err) +--- +- true +... +s, err = socket.connect('non_exists_hostname', 3301) +--- +... +check_err(err) +--- +- true +... +s, err = socket.tcp_connect('non_exists_hostname', 3301) +--- +... +check_err(err) +--- +- true +... diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua index d1fe7ada6..c455d3ddb 100644 --- a/test/app/socket.test.lua +++ b/test/app/socket.test.lua @@ -300,8 +300,16 @@ sc:close() -- tcp_connect --- test timeout -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) +-- Test timeout. In this test, tcp_connect can return the second +-- output value from internal.getaddrinfo (usually on Mac OS, but +-- theoretically it can happen on Linux too). Sometimes +-- getaddrinfo() is timed out, sometimes connect. On Linux however +-- getaddrinfo is fast enough to never give timeout error in +-- the case. So, there are two sources of timeout errors that are +-- reported differently. This difference has appeared after +-- gh-4138 patch. +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001) +s == nil -- AF_INET s = socket('AF_INET', 'SOCK_STREAM', 'tcp') @@ -988,3 +996,26 @@ s:receive('*a') s:close() srv:close() +-- gh-4138 Check getaddrinfo() error from socket:connect() only. +-- Error code and error message returned by getaddrinfo() depends +-- on system's gai_strerror(). +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' or + err == 'getaddrinfo: hostname nor servname provided, or not known' or + err == 'getaddrinfo: Temporary failure in name resolution' or + err == 'getaddrinfo: Name could not be resolved at this time' then + return true + end + return false +end; +test_run:cmd("setopt delimiter ''"); + +s, err = socket.getaddrinfo('non_exists_hostname', 3301) +check_err(err) +s, err = socket.connect('non_exists_hostname', 3301) +check_err(err) +s, err = socket.tcp_connect('non_exists_hostname', 3301) +check_err(err) diff --git a/test/box/net.box.result b/test/box/net.box.result index e3dabf7d9..3e1ce6b11 100644 --- a/test/box/net.box.result +++ b/test/box/net.box.result @@ -3927,6 +3927,37 @@ test_run:grep_log('default', '00000040:.*') --- - null ... +-- gh-4138 Check getaddrinfo() error from connect() only. Error +-- code and error message returned by getaddrinfo() depends on +-- system's gai_strerror(). +test_run:cmd("setopt delimiter ';'") +--- +- true +... +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' or + err == 'getaddrinfo: hostname nor servname provided, or not known' or + err == 'getaddrinfo: Temporary failure in name resolution' or + err == 'getaddrinfo: Name could not be resolved at this time' then + return true + end + return false +end; +--- +... +test_run:cmd("setopt delimiter ''"); +--- +- true +... +s = remote.connect('non_exists_hostname:3301') +--- +... +check_err(s['error']) +--- +- true +... box.cfg{log_level=log_level} --- ... diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua index 8e65ff470..cad8754c3 100644 --- a/test/box/net.box.test.lua +++ b/test/box/net.box.test.lua @@ -1582,4 +1582,24 @@ test_run:wait_log('default', '00000030:.*', nil, 10) -- we expect nothing below, so don't wait test_run:grep_log('default', '00000040:.*') +-- gh-4138 Check getaddrinfo() error from connect() only. Error +-- code and error message returned by getaddrinfo() depends on +-- system's gai_strerror(). +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' or + err == 'getaddrinfo: hostname nor servname provided, or not known' or + err == 'getaddrinfo: Temporary failure in name resolution' or + err == 'getaddrinfo: Name could not be resolved at this time' then + return true + end + return false +end; +test_run:cmd("setopt delimiter ''"); + +s = remote.connect('non_exists_hostname:3301') +check_err(s['error']) + box.cfg{log_level=log_level} -- 2.21.0 (Apple Git-122) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors Roman Khabibov @ 2020-03-29 9:07 ` Sergey Ostanevich 2020-03-29 9:25 ` Alexander Turenko 2020-04-06 2:08 ` Roman Khabibov 0 siblings, 2 replies; 23+ messages in thread From: Sergey Ostanevich @ 2020-03-29 9:07 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches Hi! Thanks for the patch, please see below. Sergos On 12 мар 13:24, Roman Khabibov wrote: > Add getaddrinfo() errors into the several fuctions of socket. Now > getaddrinfo() can return a pair of values (nil and error message) > in case of error. > > Closes #4138 > > @TarantoolBot document > Title: socket API changes > > * socket.getaddrinfo() > > Can return error message as second return value in case of > internal getaddrinfo() error. I really don't like the case 'can return'. Can we work this out to always return a pair of values? > > Example: > tarantool> socket.getaddrinfo('non_exists_hostname', 3301) > --- > - null > - 'getaddrinfo: nodename nor servname provided, or not known' > ... > > * socket.tcp_connect() > > Can return socket.getaddrinfo() error message as second return > value. > > Example: > tarantool> socket.tcp_connect('non_exists_hostname', 3301) > --- > - null > - 'getaddrinfo: nodename nor servname provided, or not known' > ... > --- > src/box/lua/net_box.lua | 7 +++-- > src/lua/socket.c | 16 +++++++++- > src/lua/socket.lua | 22 ++++++++++---- > test/app/socket.result | 63 ++++++++++++++++++++++++++++++++++++--- > test/app/socket.test.lua | 35 ++++++++++++++++++++-- > test/box/net.box.result | 31 +++++++++++++++++++ > test/box/net.box.test.lua | 20 +++++++++++++ > 7 files changed, 179 insertions(+), 15 deletions(-) > > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua > index 3f611c027..8068a8637 100644 > --- a/src/box/lua/net_box.lua > +++ b/src/box/lua/net_box.lua > @@ -154,9 +154,12 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end > local function establish_connection(host, port, timeout) > local timeout = timeout or DEFAULT_CONNECT_TIMEOUT > local begin = fiber.clock() > - local s = socket.tcp_connect(host, port, timeout) > + local s, err = socket.tcp_connect(host, port, timeout) > if not s then > - return nil, errno.strerror(errno()) > + if not err then > + return nil, errno.strerror(errno()) > + end > + return nil, err > end > local msg = s:read({chunk = IPROTO_GREETING_SIZE}, > timeout - (fiber.clock() - begin)) > diff --git a/src/lua/socket.c b/src/lua/socket.c > index e75a8802e..ba683ad3a 100644 > --- a/src/lua/socket.c > +++ b/src/lua/socket.c > @@ -775,6 +775,18 @@ lbox_getaddrinfo_result_wrapper(struct lua_State *L) > return 1; > } > > +/** > + * 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 (first is nil, second is error > + * message). > + */ > static int > lbox_socket_getaddrinfo(struct lua_State *L) > { > @@ -817,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L) > > if (dns_res != 0) { > lua_pushnil(L); > - return 1; > + struct error *err = diag_get()->last; > + lua_pushstring(L, err->errmsg); > + return 2; > } > > /* no results */ > diff --git a/src/lua/socket.lua b/src/lua/socket.lua > index a334ad45b..ce6dab301 100644 > --- a/src/lua/socket.lua > +++ b/src/lua/socket.lua > @@ -1041,9 +1041,12 @@ 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 > + if dns == nil then > + return nil, err You explicitly put error here... > + end > + if #dns == 0 then > boxerrno(boxerrno.EINVAL) > return nil ... and not here. Why do you keep using two versions of error passing, not one? I would agree you need boxerrno for backward compatibility, but to start moving towards {res, err} you have to introduce it everywhere. > end > @@ -1360,8 +1363,12 @@ 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) > - if dns == nil or #dns == 0 then > + local dns, err = getaddrinfo(host, port, timeout, ga_opts) > + if dns == nil then > + self._errno = boxerrno.EINVAL > + return nil, err > + end > + if #dns == 0 then > self._errno = boxerrno.EINVAL > return nil, socket_error(self) > end > @@ -1547,9 +1554,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 > end > setmetatable(s, lsocket_tcp_client_mt) > return s > diff --git a/test/app/socket.result b/test/app/socket.result > index 9f0ea0a5d..3f26bf2b0 100644 > --- a/test/app/socket.result > +++ b/test/app/socket.result > @@ -941,10 +941,20 @@ sc:close() > - true > ... > -- tcp_connect > --- test timeout > -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) > +-- Test timeout. In this test, tcp_connect can return the second > +-- output value from internal.getaddrinfo (usually on Mac OS, but > +-- theoretically it can happen on Linux too). Sometimes > +-- getaddrinfo() is timed out, sometimes connect. On Linux however > +-- getaddrinfo is fast enough to never give timeout error in > +-- the case. So, there are two sources of timeout errors that are > +-- reported differently. This difference has appeared after > +-- gh-4138 patch. > +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001) > --- > -- null > +... > +s == nil > +--- > +- true > ... > -- AF_INET > s = socket('AF_INET', 'SOCK_STREAM', 'tcp') > @@ -1595,7 +1605,7 @@ fio.stat(path) == nil > { socket.tcp_connect('abrakadabra#123') == nil, errno.strerror() } > --- > - - true > - - Invalid argument > + - Input/output error > ... > -- wrong options for getaddrinfo > socket.getaddrinfo('host', 'port', { type = 'WRONG' }) == nil and errno() == errno.EINVAL > @@ -2914,3 +2924,48 @@ srv:close() > --- > - true > ... > +-- gh-4138 Check getaddrinfo() error from socket:connect() only. > +-- Error code and error message returned by getaddrinfo() depends > +-- on system's gai_strerror(). > +test_run:cmd("setopt delimiter ';'") > +--- > +- true > +... > +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' or > + err == 'getaddrinfo: hostname nor servname provided, or not known' or > + err == 'getaddrinfo: Temporary failure in name resolution' or > + err == 'getaddrinfo: Name could not be resolved at this time' then > + return true > + end > + return false > +end; > +--- > +... > +test_run:cmd("setopt delimiter ''"); > +--- > +- true > +... > +s, err = socket.getaddrinfo('non_exists_hostname', 3301) > +--- > +... > +check_err(err) > +--- > +- true > +... > +s, err = socket.connect('non_exists_hostname', 3301) > +--- > +... > +check_err(err) > +--- > +- true > +... > +s, err = socket.tcp_connect('non_exists_hostname', 3301) > +--- > +... > +check_err(err) > +--- > +- true > +... > diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua > index d1fe7ada6..c455d3ddb 100644 > --- a/test/app/socket.test.lua > +++ b/test/app/socket.test.lua > @@ -300,8 +300,16 @@ sc:close() > > -- tcp_connect > > --- test timeout > -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) > +-- Test timeout. In this test, tcp_connect can return the second > +-- output value from internal.getaddrinfo (usually on Mac OS, but > +-- theoretically it can happen on Linux too). Sometimes > +-- getaddrinfo() is timed out, sometimes connect. On Linux however > +-- getaddrinfo is fast enough to never give timeout error in > +-- the case. So, there are two sources of timeout errors that are > +-- reported differently. This difference has appeared after > +-- gh-4138 patch. > +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001) > +s == nil > > -- AF_INET > s = socket('AF_INET', 'SOCK_STREAM', 'tcp') > @@ -988,3 +996,26 @@ s:receive('*a') > s:close() > srv:close() > > +-- gh-4138 Check getaddrinfo() error from socket:connect() only. > +-- Error code and error message returned by getaddrinfo() depends > +-- on system's gai_strerror(). > +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' or > + err == 'getaddrinfo: hostname nor servname provided, or not known' or > + err == 'getaddrinfo: Temporary failure in name resolution' or > + err == 'getaddrinfo: Name could not be resolved at this time' then > + return true > + end > + return false > +end; I really doubt that different error messages from the set above will appear for the same error on different platforms. Could you please check for particular output for each case you trigger below? > +test_run:cmd("setopt delimiter ''"); > + > +s, err = socket.getaddrinfo('non_exists_hostname', 3301) > +check_err(err) > +s, err = socket.connect('non_exists_hostname', 3301) > +check_err(err) > +s, err = socket.tcp_connect('non_exists_hostname', 3301) > +check_err(err) > diff --git a/test/box/net.box.result b/test/box/net.box.result > index e3dabf7d9..3e1ce6b11 100644 > --- a/test/box/net.box.result > +++ b/test/box/net.box.result > @@ -3927,6 +3927,37 @@ test_run:grep_log('default', '00000040:.*') > --- > - null > ... > +-- gh-4138 Check getaddrinfo() error from connect() only. Error > +-- code and error message returned by getaddrinfo() depends on > +-- system's gai_strerror(). > +test_run:cmd("setopt delimiter ';'") > +--- > +- true > +... > +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' or > + err == 'getaddrinfo: hostname nor servname provided, or not known' or > + err == 'getaddrinfo: Temporary failure in name resolution' or > + err == 'getaddrinfo: Name could not be resolved at this time' then > + return true > + end > + return false > +end; > +--- > +... > +test_run:cmd("setopt delimiter ''"); > +--- > +- true > +... > +s = remote.connect('non_exists_hostname:3301') > +--- > +... > +check_err(s['error']) > +--- > +- true > +... > box.cfg{log_level=log_level} > --- > ... > diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua > index 8e65ff470..cad8754c3 100644 > --- a/test/box/net.box.test.lua > +++ b/test/box/net.box.test.lua > @@ -1582,4 +1582,24 @@ test_run:wait_log('default', '00000030:.*', nil, 10) > -- we expect nothing below, so don't wait > test_run:grep_log('default', '00000040:.*') > > +-- gh-4138 Check getaddrinfo() error from connect() only. Error > +-- code and error message returned by getaddrinfo() depends on > +-- system's gai_strerror(). > +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' or > + err == 'getaddrinfo: hostname nor servname provided, or not known' or > + err == 'getaddrinfo: Temporary failure in name resolution' or > + err == 'getaddrinfo: Name could not be resolved at this time' then > + return true > + end > + return false > +end; > +test_run:cmd("setopt delimiter ''"); > + > +s = remote.connect('non_exists_hostname:3301') > +check_err(s['error']) > + > box.cfg{log_level=log_level} > -- > 2.21.0 (Apple Git-122) > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-03-29 9:07 ` Sergey Ostanevich @ 2020-03-29 9:25 ` Alexander Turenko 2020-04-06 2:08 ` Roman Khabibov 1 sibling, 0 replies; 23+ messages in thread From: Alexander Turenko @ 2020-03-29 9:25 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches > > diff --git a/src/lua/socket.lua b/src/lua/socket.lua > > index a334ad45b..ce6dab301 100644 > > --- a/src/lua/socket.lua > > +++ b/src/lua/socket.lua > > @@ -1041,9 +1041,12 @@ 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 > > + if dns == nil then > > + return nil, err > You explicitly put error here... > > > + end > > + if #dns == 0 then > > boxerrno(boxerrno.EINVAL) > > return nil > ... and not here. Why do you keep using two versions of error passing, > not one? I would agree you need boxerrno for backward compatibility, but > to start moving towards {res, err} you have to introduce it everywhere. Seems logical. > > +-- gh-4138 Check getaddrinfo() error from socket:connect() only. > > +-- Error code and error message returned by getaddrinfo() depends > > +-- on system's gai_strerror(). > > +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' or > > + err == 'getaddrinfo: hostname nor servname provided, or not known' or > > + err == 'getaddrinfo: Temporary failure in name resolution' or > > + err == 'getaddrinfo: Name could not be resolved at this time' then > > + return true > > + end > > + return false > > +end; > I really doubt that different error messages from the set above will appear > for the same error on different platforms. Could you please check for particular > output for each case you trigger below? It is so. It varies even for the same system connected to different networks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-03-29 9:07 ` Sergey Ostanevich 2020-03-29 9:25 ` Alexander Turenko @ 2020-04-06 2:08 ` Roman Khabibov 2020-04-16 10:27 ` Sergey Ostanevich 1 sibling, 1 reply; 23+ messages in thread From: Roman Khabibov @ 2020-04-06 2:08 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches > On Mar 29, 2020, at 12:07, Sergey Ostanevich <sergos@tarantool.org> wrote: > > Hi! > > Thanks for the patch, please see below. > > Sergos > > On 12 мар 13:24, Roman Khabibov wrote: >> Add getaddrinfo() errors into the several fuctions of socket. Now >> getaddrinfo() can return a pair of values (nil and error message) >> in case of error. >> >> Closes #4138 >> >> @TarantoolBot document >> Title: socket API changes >> >> * socket.getaddrinfo() >> >> Can return error message as second return value in case of >> internal getaddrinfo() error. > I really don't like the case 'can return'. Can we work this out to > always return a pair of values? Done. >> >> Example: >> tarantool> socket.getaddrinfo('non_exists_hostname', 3301) >> --- >> - null >> - 'getaddrinfo: nodename nor servname provided, or not known' >> ... >> >> * socket.tcp_connect() >> >> Can return socket.getaddrinfo() error message as second return >> value. >> >> Example: >> tarantool> socket.tcp_connect('non_exists_hostname', 3301) >> --- >> - null >> - 'getaddrinfo: nodename nor servname provided, or not known' >> ... >> --- >> src/box/lua/net_box.lua | 7 +++-- >> src/lua/socket.c | 16 +++++++++- >> src/lua/socket.lua | 22 ++++++++++---- >> test/app/socket.result | 63 ++++++++++++++++++++++++++++++++++++--- >> test/app/socket.test.lua | 35 ++++++++++++++++++++-- >> test/box/net.box.result | 31 +++++++++++++++++++ >> test/box/net.box.test.lua | 20 +++++++++++++ >> 7 files changed, 179 insertions(+), 15 deletions(-) >> >> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua >> index 3f611c027..8068a8637 100644 >> --- a/src/box/lua/net_box.lua >> +++ b/src/box/lua/net_box.lua >> @@ -154,9 +154,12 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end >> local function establish_connection(host, port, timeout) >> local timeout = timeout or DEFAULT_CONNECT_TIMEOUT >> local begin = fiber.clock() >> - local s = socket.tcp_connect(host, port, timeout) >> + local s, err = socket.tcp_connect(host, port, timeout) >> if not s then >> - return nil, errno.strerror(errno()) >> + if not err then >> + return nil, errno.strerror(errno()) >> + end >> + return nil, err >> end >> local msg = s:read({chunk = IPROTO_GREETING_SIZE}, >> timeout - (fiber.clock() - begin)) >> diff --git a/src/lua/socket.c b/src/lua/socket.c >> index e75a8802e..ba683ad3a 100644 >> --- a/src/lua/socket.c >> +++ b/src/lua/socket.c >> @@ -775,6 +775,18 @@ lbox_getaddrinfo_result_wrapper(struct lua_State *L) >> return 1; >> } >> >> +/** >> + * 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 (first is nil, second is error >> + * message). >> + */ >> static int >> lbox_socket_getaddrinfo(struct lua_State *L) >> { >> @@ -817,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L) >> >> if (dns_res != 0) { >> lua_pushnil(L); >> - return 1; >> + struct error *err = diag_get()->last; >> + lua_pushstring(L, err->errmsg); >> + return 2; >> } >> >> /* no results */ >> diff --git a/src/lua/socket.lua b/src/lua/socket.lua >> index a334ad45b..ce6dab301 100644 >> --- a/src/lua/socket.lua >> +++ b/src/lua/socket.lua >> @@ -1041,9 +1041,12 @@ 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 >> + if dns == nil then >> + return nil, err > You explicitly put error here... > >> + end >> + if #dns == 0 then >> boxerrno(boxerrno.EINVAL) >> return nil > ... and not here. Why do you keep using two versions of error passing, > not one? I would agree you need boxerrno for backward compatibility, but > to start moving towards {res, err} you have to introduce it everywhere. @@ -1028,30 +1028,33 @@ local function tcp_connect(host, port, timeout) local s = socket_new('AF_UNIX', 'SOCK_STREAM', 0) if not s then -- Address family is not supported by the host - return nil + return nil, boxerrno.strerror() end if not socket_tcp_connect(s, host, port, timeout) then local save_errno = s._errno s:close() boxerrno(save_errno) - return nil + return nil, boxerrno.strerror() end boxerrno(0) return s 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 + if dns == nil then + return nil, err + end + if #dns == 0 then boxerrno(boxerrno.EINVAL) - return nil + return nil, boxerrno.strerror() end for i, remote in pairs(dns) do timeout = stop - fiber.clock() if timeout <= 0 then boxerrno(boxerrno.ETIMEDOUT) - return nil + return nil, boxerrno.strerror() end local s = socket_new(remote.family, remote.type, remote.protocol) if s then >> end >> @@ -1360,8 +1363,12 @@ 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) >> - if dns == nil or #dns == 0 then >> + local dns, err = getaddrinfo(host, port, timeout, ga_opts) >> + if dns == nil then >> + self._errno = boxerrno.EINVAL >> + return nil, err >> + end >> + if #dns == 0 then >> self._errno = boxerrno.EINVAL >> return nil, socket_error(self) >> end >> @@ -1547,9 +1554,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 >> end >> setmetatable(s, lsocket_tcp_client_mt) >> return s >> diff --git a/test/app/socket.result b/test/app/socket.result >> index 9f0ea0a5d..3f26bf2b0 100644 >> --- a/test/app/socket.result >> +++ b/test/app/socket.result >> @@ -941,10 +941,20 @@ sc:close() >> - true >> ... >> -- tcp_connect >> --- test timeout >> -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) >> +-- Test timeout. In this test, tcp_connect can return the second >> +-- output value from internal.getaddrinfo (usually on Mac OS, but >> +-- theoretically it can happen on Linux too). Sometimes >> +-- getaddrinfo() is timed out, sometimes connect. On Linux however >> +-- getaddrinfo is fast enough to never give timeout error in >> +-- the case. So, there are two sources of timeout errors that are >> +-- reported differently. This difference has appeared after >> +-- gh-4138 patch. >> +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001) >> --- >> -- null >> +... >> +s == nil >> +--- >> +- true >> ... >> -- AF_INET >> s = socket('AF_INET', 'SOCK_STREAM', 'tcp') >> @@ -1595,7 +1605,7 @@ fio.stat(path) == nil >> { socket.tcp_connect('abrakadabra#123') == nil, errno.strerror() } >> --- >> - - true >> - - Invalid argument >> + - Input/output error >> ... >> -- wrong options for getaddrinfo >> socket.getaddrinfo('host', 'port', { type = 'WRONG' }) == nil and errno() == errno.EINVAL >> @@ -2914,3 +2924,48 @@ srv:close() >> --- >> - true >> ... >> +-- gh-4138 Check getaddrinfo() error from socket:connect() only. >> +-- Error code and error message returned by getaddrinfo() depends >> +-- on system's gai_strerror(). >> +test_run:cmd("setopt delimiter ';'") >> +--- >> +- true >> +... >> +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' or >> + err == 'getaddrinfo: hostname nor servname provided, or not known' or >> + err == 'getaddrinfo: Temporary failure in name resolution' or >> + err == 'getaddrinfo: Name could not be resolved at this time' then >> + return true >> + end >> + return false >> +end; >> +--- >> +... >> +test_run:cmd("setopt delimiter ''"); >> +--- >> +- true >> +... >> +s, err = socket.getaddrinfo('non_exists_hostname', 3301) >> +--- >> +... >> +check_err(err) >> +--- >> +- true >> +... >> +s, err = socket.connect('non_exists_hostname', 3301) >> +--- >> +... >> +check_err(err) >> +--- >> +- true >> +... >> +s, err = socket.tcp_connect('non_exists_hostname', 3301) >> +--- >> +... >> +check_err(err) >> +--- >> +- true >> +... >> diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua >> index d1fe7ada6..c455d3ddb 100644 >> --- a/test/app/socket.test.lua >> +++ b/test/app/socket.test.lua >> @@ -300,8 +300,16 @@ sc:close() >> >> -- tcp_connect >> >> --- test timeout >> -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) >> +-- Test timeout. In this test, tcp_connect can return the second >> +-- output value from internal.getaddrinfo (usually on Mac OS, but >> +-- theoretically it can happen on Linux too). Sometimes >> +-- getaddrinfo() is timed out, sometimes connect. On Linux however >> +-- getaddrinfo is fast enough to never give timeout error in >> +-- the case. So, there are two sources of timeout errors that are >> +-- reported differently. This difference has appeared after >> +-- gh-4138 patch. >> +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001) >> +s == nil >> >> -- AF_INET >> s = socket('AF_INET', 'SOCK_STREAM', 'tcp') >> @@ -988,3 +996,26 @@ s:receive('*a') >> s:close() >> srv:close() >> >> +-- gh-4138 Check getaddrinfo() error from socket:connect() only. >> +-- Error code and error message returned by getaddrinfo() depends >> +-- on system's gai_strerror(). >> +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' or >> + err == 'getaddrinfo: hostname nor servname provided, or not known' or >> + err == 'getaddrinfo: Temporary failure in name resolution' or >> + err == 'getaddrinfo: Name could not be resolved at this time' then >> + return true >> + end >> + return false >> +end; > I really doubt that different error messages from the set above will appear > for the same error on different platforms. Could you please check for particular > output for each case you trigger below? Look at that: https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t >> +test_run:cmd("setopt delimiter ''"); >> + >> +s, err = socket.getaddrinfo('non_exists_hostname', 3301) >> +check_err(err) >> +s, err = socket.connect('non_exists_hostname', 3301) >> +check_err(err) >> +s, err = socket.tcp_connect('non_exists_hostname', 3301) >> +check_err(err) >> diff --git a/test/box/net.box.result b/test/box/net.box.result >> index e3dabf7d9..3e1ce6b11 100644 >> --- a/test/box/net.box.result >> +++ b/test/box/net.box.result >> @@ -3927,6 +3927,37 @@ test_run:grep_log('default', '00000040:.*') >> --- >> - null >> ... >> +-- gh-4138 Check getaddrinfo() error from connect() only. Error >> +-- code and error message returned by getaddrinfo() depends on >> +-- system's gai_strerror(). >> +test_run:cmd("setopt delimiter ';'") >> +--- >> +- true >> +... >> +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' or >> + err == 'getaddrinfo: hostname nor servname provided, or not known' or >> + err == 'getaddrinfo: Temporary failure in name resolution' or >> + err == 'getaddrinfo: Name could not be resolved at this time' then >> + return true >> + end >> + return false >> +end; >> +--- >> +... >> +test_run:cmd("setopt delimiter ''"); >> +--- >> +- true >> +... >> +s = remote.connect('non_exists_hostname:3301') >> +--- >> +... >> +check_err(s['error']) >> +--- >> +- true >> +... >> box.cfg{log_level=log_level} >> --- >> ... >> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua >> index 8e65ff470..cad8754c3 100644 >> --- a/test/box/net.box.test.lua >> +++ b/test/box/net.box.test.lua >> @@ -1582,4 +1582,24 @@ test_run:wait_log('default', '00000030:.*', nil, 10) >> -- we expect nothing below, so don't wait >> test_run:grep_log('default', '00000040:.*') >> >> +-- gh-4138 Check getaddrinfo() error from connect() only. Error >> +-- code and error message returned by getaddrinfo() depends on >> +-- system's gai_strerror(). >> +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' or >> + err == 'getaddrinfo: hostname nor servname provided, or not known' or >> + err == 'getaddrinfo: Temporary failure in name resolution' or >> + err == 'getaddrinfo: Name could not be resolved at this time' then >> + return true >> + end >> + return false >> +end; >> +test_run:cmd("setopt delimiter ''"); >> + >> +s = remote.connect('non_exists_hostname:3301') >> +check_err(s['error']) >> + >> box.cfg{log_level=log_level} >> -- >> 2.21.0 (Apple Git-122) >> commit 57d8b556b98a8b6de4d40f6ae1c5f48498022962 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Thu Nov 21 14:37:54 2019 +0300 lua: return getaddrinfo() errors Add getaddrinfo() errors into the several fuctions of socket. Now getaddrinfo() return a pair of values (nil and error message) in case of error. Closes #4138 @TarantoolBot document Title: socket API changes * socket.getaddrinfo() Return error message as the second return value. Example: tarantool> socket.getaddrinfo('non_exists_hostname', 3301) --- - null - 'getaddrinfo: nodename nor servname provided, or not known' ... * socket.tcp_connect() Return error message as the second return value. Example: tarantool> socket.tcp_connect('non_exists_hostname', 3301) --- - null - 'getaddrinfo: nodename nor servname provided, or not known' ... * socket.bind() Return error message as the second return value. Example: tarantool> socket.tcp_connect('non_exists_hostname', 3301) --- - null - 'getaddrinfo: nodename nor servname provided, or not known' ... * socket.tcp_server() Return error message as the second return value. Example: tarantool> socket.tcp_connect('non_exists_hostname', 3301) --- - null - 'getaddrinfo: nodename nor servname provided, or not known' ... diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index 3f611c027..b5d0dff4a 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -154,9 +154,9 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end local function establish_connection(host, port, timeout) local timeout = timeout or DEFAULT_CONNECT_TIMEOUT local begin = fiber.clock() - local s = socket.tcp_connect(host, port, timeout) + local s, err = socket.tcp_connect(host, port, timeout) if not s then - return nil, errno.strerror(errno()) + return nil, err end local msg = s:read({chunk = IPROTO_GREETING_SIZE}, timeout - (fiber.clock() - begin)) diff --git a/src/lua/socket.c b/src/lua/socket.c index e75a8802e..ba683ad3a 100644 --- a/src/lua/socket.c +++ b/src/lua/socket.c @@ -775,6 +775,18 @@ lbox_getaddrinfo_result_wrapper(struct lua_State *L) return 1; } +/** + * 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 (first is nil, second is error + * message). + */ static int lbox_socket_getaddrinfo(struct lua_State *L) { @@ -817,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L) if (dns_res != 0) { lua_pushnil(L); - return 1; + struct error *err = diag_get()->last; + lua_pushstring(L, err->errmsg); + return 2; } /* no results */ diff --git a/src/lua/socket.lua b/src/lua/socket.lua index a334ad45b..57fdc6213 100644 --- a/src/lua/socket.lua +++ b/src/lua/socket.lua @@ -963,7 +963,7 @@ local function getaddrinfo(host, port, timeout, opts) local itype = get_ivalue(internal.SO_TYPE, opts.type) if itype == nil then boxerrno(boxerrno.EINVAL) - return nil + return nil, boxerrno.strerror() end ga_opts.type = itype end @@ -972,7 +972,7 @@ local function getaddrinfo(host, port, timeout, opts) local ifamily = get_ivalue(internal.DOMAIN, opts.family) if ifamily == nil then boxerrno(boxerrno.EINVAL) - return nil + return nil, boxerrno.strerror() end ga_opts.family = ifamily end @@ -980,7 +980,7 @@ local function getaddrinfo(host, port, timeout, opts) if opts.protocol ~= nil then local p = getprotobyname(opts.protocol) if p == nil then - return nil + return nil, boxerrno.strerror() end ga_opts.protocol = p end @@ -990,7 +990,7 @@ local function getaddrinfo(host, port, timeout, opts) get_iflags(internal.AI_FLAGS, opts.flags) if ga_opts.flags == nil then boxerrno(boxerrno.EINVAL) - return nil + return nil, boxerrno.strerror() end end @@ -1028,30 +1028,33 @@ local function tcp_connect(host, port, timeout) local s = socket_new('AF_UNIX', 'SOCK_STREAM', 0) if not s then -- Address family is not supported by the host - return nil + return nil, boxerrno.strerror() end if not socket_tcp_connect(s, host, port, timeout) then local save_errno = s._errno s:close() boxerrno(save_errno) - return nil + return nil, boxerrno.strerror() end boxerrno(0) return s 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 + if dns == nil then + return nil, err + end + if #dns == 0 then boxerrno(boxerrno.EINVAL) - return nil + return nil, boxerrno.strerror() end for i, remote in pairs(dns) do timeout = stop - fiber.clock() if timeout <= 0 then boxerrno(boxerrno.ETIMEDOUT) - return nil + return nil, boxerrno.strerror() end local s = socket_new(remote.family, remote.type, remote.protocol) if s then @@ -1065,7 +1068,7 @@ local function tcp_connect(host, port, timeout) end end -- errno is set by socket_tcp_connect() - return nil + return nil, boxerrno.strerror() end local function tcp_server_handler(server, sc, from) @@ -1160,15 +1163,15 @@ end local function tcp_server_bind(host, port, prepare, timeout) timeout = timeout and tonumber(timeout) or TIMEOUT_INFINITY - local dns + local dns, err if host == 'unix/' then dns = {{host = host, port = port, family = 'AF_UNIX', protocol = 0, type = 'SOCK_STREAM' }} else - dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM', + dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM', flags = 'AI_PASSIVE'}) if dns == nil then - return nil + return nil, err end end @@ -1185,14 +1188,14 @@ local function tcp_server_bind(host, port, prepare, timeout) local save_errno = boxerrno() socket_close(s) boxerrno(save_errno) - return nil + return nil, boxerrno.strerror() end return s, addr end end -- DNS resolved successfully, but addresss family is not supported boxerrno(boxerrno.EAFNOSUPPORT) - return nil + return nil, boxerrno.strerror() end local function tcp_server(host, port, opts, timeout) @@ -1213,7 +1216,8 @@ local function tcp_server(host, port, opts, timeout) server.name = server.name or 'server' local s, addr = tcp_server_bind(host, port, server.prepare, timeout) if not s then - return nil + -- addr is error message now. + return nil, addr end fiber.create(tcp_server_loop, server, s, addr) return s, addr @@ -1360,8 +1364,12 @@ 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) - if dns == nil or #dns == 0 then + local dns, err = getaddrinfo(host, port, timeout, ga_opts) + if dns == nil then + self._errno = boxerrno.EINVAL + return nil, err + end + if #dns == 0 then self._errno = boxerrno.EINVAL return nil, socket_error(self) end @@ -1547,9 +1555,9 @@ 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() + return nil, err end setmetatable(s, lsocket_tcp_client_mt) return s @@ -1560,9 +1568,9 @@ local function lsocket_bind(host, port, backlog) error("Usage: luasocket.bind(host, port [, backlog])") end local function prepare(s) return backlog end - local s = tcp_server_bind(host, port, prepare) + local s, err = tcp_server_bind(host, port, prepare) if not s then - return nil, boxerrno.strerror() + return nil, err end return setmetatable(s, lsocket_tcp_server_mt) end diff --git a/test/app/socket.result b/test/app/socket.result index 9f0ea0a5d..6f91b32ee 100644 --- a/test/app/socket.result +++ b/test/app/socket.result @@ -941,10 +941,20 @@ sc:close() - true ... -- tcp_connect --- test timeout -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) +-- Test timeout. In this test, tcp_connect can return the second +-- output value from internal.getaddrinfo (usually on Mac OS, but +-- theoretically it can happen on Linux too). Sometimes +-- getaddrinfo() is timed out, sometimes connect. On Linux however +-- getaddrinfo is fast enough to never give timeout error in +-- the case. So, there are two sources of timeout errors that are +-- reported differently. This difference has appeared after +-- gh-4138 patch. +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001) --- -- null +... +s == nil +--- +- true ... -- AF_INET s = socket('AF_INET', 'SOCK_STREAM', 'tcp') @@ -1595,7 +1605,7 @@ fio.stat(path) == nil { socket.tcp_connect('abrakadabra#123') == nil, errno.strerror() } --- - - true - - Invalid argument + - Input/output error ... -- wrong options for getaddrinfo socket.getaddrinfo('host', 'port', { type = 'WRONG' }) == nil and errno() == errno.EINVAL @@ -2914,3 +2924,68 @@ srv:close() --- - true ... +-- gh-4138 Check getaddrinfo() error from socket:connect() only. +-- Error code and error message returned by getaddrinfo() depends +-- on system's gai_strerror(). +test_run:cmd("setopt delimiter ';'") +--- +- true +... +-- EAI_NONAME +-- EAI_SERVICE +-- EAI_NONAME +-- EAI_NONAME +-- EAI_AGAIN +-- EAI_AGAIN +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' or + err == 'getaddrinfo: hostname nor servname provided, or not known' or + err == 'getaddrinfo: Temporary failure in name resolution' or + err == 'getaddrinfo: Name could not be resolved at this time' then + return true + end + return false +end; +--- +... +test_run:cmd("setopt delimiter ''"); +--- +- true +... +s, err = socket.getaddrinfo('non_exists_hostname', 3301) +--- +... +check_err(err) +--- +- true +... +s, err = socket.connect('non_exists_hostname', 3301) +--- +... +check_err(err) +--- +- true +... +s, err = socket.tcp_connect('non_exists_hostname', 3301) +--- +... +check_err(err) +--- +- true +... +s, err = socket.bind('non_exists_hostname', 3301) +--- +... +check_err(err) +--- +- true +... +s, err = socket.tcp_server('non_exists_hostname', 3301, function() end) +--- +... +check_err(err) +--- +- true +... diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua index d1fe7ada6..9f5b3fea0 100644 --- a/test/app/socket.test.lua +++ b/test/app/socket.test.lua @@ -300,8 +300,16 @@ sc:close() -- tcp_connect --- test timeout -socket.tcp_connect('127.0.0.1', 80, 0.00000000001) +-- Test timeout. In this test, tcp_connect can return the second +-- output value from internal.getaddrinfo (usually on Mac OS, but +-- theoretically it can happen on Linux too). Sometimes +-- getaddrinfo() is timed out, sometimes connect. On Linux however +-- getaddrinfo is fast enough to never give timeout error in +-- the case. So, there are two sources of timeout errors that are +-- reported differently. This difference has appeared after +-- gh-4138 patch. +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001) +s == nil -- AF_INET s = socket('AF_INET', 'SOCK_STREAM', 'tcp') @@ -988,3 +996,36 @@ s:receive('*a') s:close() srv:close() +-- gh-4138 Check getaddrinfo() error from socket:connect() only. +-- Error code and error message returned by getaddrinfo() depends +-- on system's gai_strerror(). +test_run:cmd("setopt delimiter ';'") +function check_err(err) +-- EAI_NONAME + if err == 'getaddrinfo: nodename nor servname provided, or not known' or +-- EAI_SERVICE + err == 'getaddrinfo: Servname not supported for ai_socktype' or +-- EAI_NONAME + err == 'getaddrinfo: Name or service not known' or +-- EAI_NONAME + err == 'getaddrinfo: hostname nor servname provided, or not known' or +-- EAI_AGAIN + err == 'getaddrinfo: Temporary failure in name resolution' or +-- EAI_AGAIN + err == 'getaddrinfo: Name could not be resolved at this time' then + return true + end + return false +end; +test_run:cmd("setopt delimiter ''"); + +s, err = socket.getaddrinfo('non_exists_hostname', 3301) +check_err(err) +s, err = socket.connect('non_exists_hostname', 3301) +check_err(err) +s, err = socket.tcp_connect('non_exists_hostname', 3301) +check_err(err) +s, err = socket.bind('non_exists_hostname', 3301) +check_err(err) +s, err = socket.tcp_server('non_exists_hostname', 3301, function() end) +check_err(err) diff --git a/test/box/net.box.result b/test/box/net.box.result index e3dabf7d9..c5b32123b 100644 --- a/test/box/net.box.result +++ b/test/box/net.box.result @@ -3927,6 +3927,43 @@ test_run:grep_log('default', '00000040:.*') --- - null ... +-- gh-4138 Check getaddrinfo() error from connect() only. Error +-- code and error message returned by getaddrinfo() depends on +-- system's gai_strerror(). +test_run:cmd("setopt delimiter ';'") +--- +- true +... +-- EAI_NONAME +-- EAI_SERVICE +-- EAI_NONAME +-- EAI_NONAME +-- EAI_AGAIN +-- EAI_AGAIN +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' or + err == 'getaddrinfo: hostname nor servname provided, or not known' or + err == 'getaddrinfo: Temporary failure in name resolution' or + err == 'getaddrinfo: Name could not be resolved at this time' then + return true + end + return false +end; +--- +... +test_run:cmd("setopt delimiter ''"); +--- +- true +... +s = remote.connect('non_exists_hostname:3301') +--- +... +check_err(s['error']) +--- +- true +... box.cfg{log_level=log_level} --- ... diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua index 8e65ff470..24b996be4 100644 --- a/test/box/net.box.test.lua +++ b/test/box/net.box.test.lua @@ -1582,4 +1582,30 @@ test_run:wait_log('default', '00000030:.*', nil, 10) -- we expect nothing below, so don't wait test_run:grep_log('default', '00000040:.*') +-- gh-4138 Check getaddrinfo() error from connect() only. Error +-- code and error message returned by getaddrinfo() depends on +-- system's gai_strerror(). +test_run:cmd("setopt delimiter ';'") +function check_err(err) +-- EAI_NONAME + if err == 'getaddrinfo: nodename nor servname provided, or not known' or +-- EAI_SERVICE + err == 'getaddrinfo: Servname not supported for ai_socktype' or +-- EAI_NONAME + err == 'getaddrinfo: Name or service not known' or +-- EAI_NONAME + err == 'getaddrinfo: hostname nor servname provided, or not known' or +-- EAI_AGAIN + err == 'getaddrinfo: Temporary failure in name resolution' or +-- EAI_AGAIN + err == 'getaddrinfo: Name could not be resolved at this time' then + return true + end + return false +end; +test_run:cmd("setopt delimiter ''"); + +s = remote.connect('non_exists_hostname:3301') +check_err(s['error']) + box.cfg{log_level=log_level} ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-04-06 2:08 ` Roman Khabibov @ 2020-04-16 10:27 ` Sergey Ostanevich 2020-04-24 11:42 ` Roman Khabibov 0 siblings, 1 reply; 23+ messages in thread From: Sergey Ostanevich @ 2020-04-16 10:27 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 06 апр 05:08, Roman Khabibov wrote: > >> +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' or > >> + err == 'getaddrinfo: hostname nor servname provided, or not known' or > >> + err == 'getaddrinfo: Temporary failure in name resolution' or > >> + err == 'getaddrinfo: Name could not be resolved at this time' then > >> + return true > >> + end > >> + return false > >> +end; > > I really doubt that different error messages from the set above will appear > > for the same error on different platforms. Could you please check for particular > > output for each case you trigger below? > Look at that: > https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed > https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t > Exactly, this means for the error EAI_NONAME those OSes have differnet messages. But you've put different errors in one bunch - I meant it should never return 'Temporary failure in name resolution' as a result for EAI_SERVICE, right? Are you testing for correct error returned for a particular case? Otherwise looks good. Sergos ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-04-16 10:27 ` Sergey Ostanevich @ 2020-04-24 11:42 ` Roman Khabibov 2020-04-24 17:18 ` Sergey Ostanevich 0 siblings, 1 reply; 23+ messages in thread From: Roman Khabibov @ 2020-04-24 11:42 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches Hi! Thanks for the review. > On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote: > > On 06 апр 05:08, Roman Khabibov wrote: >>>> +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' or >>>> + err == 'getaddrinfo: hostname nor servname provided, or not known' or >>>> + err == 'getaddrinfo: Temporary failure in name resolution' or >>>> + err == 'getaddrinfo: Name could not be resolved at this time' then >>>> + return true >>>> + end >>>> + return false >>>> +end; >>> I really doubt that different error messages from the set above will appear >>> for the same error on different platforms. Could you please check for particular >>> output for each case you trigger below? >> Look at that: >> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed >> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t >> > > Exactly, this means for the error EAI_NONAME those OSes have differnet > messages. But you've put different errors in one bunch - I meant it > should never return 'Temporary failure in name resolution' as a result > for EAI_SERVICE, right? > > Are you testing for correct error returned for a particular case? Don’t understand the question. I just check error messages that appeared after travis/gitlab testing. > Otherwise looks good. > > Sergos ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-04-24 11:42 ` Roman Khabibov @ 2020-04-24 17:18 ` Sergey Ostanevich 2020-04-26 3:16 ` Roman Khabibov 0 siblings, 1 reply; 23+ messages in thread From: Sergey Ostanevich @ 2020-04-24 17:18 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On 24 апр 14:42, Roman Khabibov wrote: > Hi! Thanks for the review. > > > On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote: > > > > On 06 апр 05:08, Roman Khabibov wrote: > >>>> +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' or > >>>> + err == 'getaddrinfo: hostname nor servname provided, or not known' or > >>>> + err == 'getaddrinfo: Temporary failure in name resolution' or > >>>> + err == 'getaddrinfo: Name could not be resolved at this time' then > >>>> + return true > >>>> + end > >>>> + return false > >>>> +end; > >>> I really doubt that different error messages from the set above will appear > >>> for the same error on different platforms. Could you please check for particular > >>> output for each case you trigger below? > >> Look at that: > >> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed > >> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t > >> > > > > Exactly, this means for the error EAI_NONAME those OSes have differnet > > messages. But you've put different errors in one bunch - I meant it > > should never return 'Temporary failure in name resolution' as a result > > for EAI_SERVICE, right? > > > > Are you testing for correct error returned for a particular case? > Don’t understand the question. I just check error messages that appeared > after travis/gitlab testing. > Different errors makes different messages. Same error can make different messages on different platforms. You put both into one: you don't care about what exact error came to you, right? Is it something expected from this patch at all - to differentiate the errors? > > Otherwise looks good. > > > > Sergos > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-04-24 17:18 ` Sergey Ostanevich @ 2020-04-26 3:16 ` Roman Khabibov 2020-04-26 15:46 ` Alexander Turenko 0 siblings, 1 reply; 23+ messages in thread From: Roman Khabibov @ 2020-04-26 3:16 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches > On Apr 24, 2020, at 20:18, Sergey Ostanevich <sergos@tarantool.org> wrote: > > On 24 апр 14:42, Roman Khabibov wrote: >> Hi! Thanks for the review. >> >>> On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote: >>> >>> On 06 апр 05:08, Roman Khabibov wrote: >>>>>> +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' or >>>>>> + err == 'getaddrinfo: hostname nor servname provided, or not known' or >>>>>> + err == 'getaddrinfo: Temporary failure in name resolution' or >>>>>> + err == 'getaddrinfo: Name could not be resolved at this time' then >>>>>> + return true >>>>>> + end >>>>>> + return false >>>>>> +end; >>>>> I really doubt that different error messages from the set above will appear >>>>> for the same error on different platforms. Could you please check for particular >>>>> output for each case you trigger below? >>>> Look at that: >>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed >>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t >>>> >>> >>> Exactly, this means for the error EAI_NONAME those OSes have differnet >>> messages. But you've put different errors in one bunch - I meant it >>> should never return 'Temporary failure in name resolution' as a result >>> for EAI_SERVICE, right? >>> >>> Are you testing for correct error returned for a particular case? >> Don’t understand the question. I just check error messages that appeared >> after travis/gitlab testing. >> > Different errors makes different messages. > Same error can make different messages on different platforms. > > You put both into one: you don't care about what exact error came to > you, right? Right. The most important thing here is that the error is from getaddrinfo. > Is it something expected from this patch at all - to differentiate the > errors? The idea of this patch is to throw a internal getaddrinfo error instead of the obscure common I/O error. >>> Otherwise looks good. >>> >>> Sergos ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-04-26 3:16 ` Roman Khabibov @ 2020-04-26 15:46 ` Alexander Turenko 2020-04-26 16:26 ` Roman Khabibov 0 siblings, 1 reply; 23+ messages in thread From: Alexander Turenko @ 2020-04-26 15:46 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On Sun, Apr 26, 2020 at 06:16:29AM +0300, Roman Khabibov wrote: > > > > On Apr 24, 2020, at 20:18, Sergey Ostanevich <sergos@tarantool.org> wrote: > > > > On 24 апр 14:42, Roman Khabibov wrote: > >> Hi! Thanks for the review. > >> > >>> On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote: > >>> > >>> On 06 апр 05:08, Roman Khabibov wrote: > >>>>>> +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' or > >>>>>> + err == 'getaddrinfo: hostname nor servname provided, or not known' or > >>>>>> + err == 'getaddrinfo: Temporary failure in name resolution' or > >>>>>> + err == 'getaddrinfo: Name could not be resolved at this time' then > >>>>>> + return true > >>>>>> + end > >>>>>> + return false > >>>>>> +end; > >>>>> I really doubt that different error messages from the set above will appear > >>>>> for the same error on different platforms. Could you please check for particular > >>>>> output for each case you trigger below? > >>>> Look at that: > >>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed > >>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t > >>>> > >>> > >>> Exactly, this means for the error EAI_NONAME those OSes have differnet > >>> messages. But you've put different errors in one bunch - I meant it > >>> should never return 'Temporary failure in name resolution' as a result > >>> for EAI_SERVICE, right? > >>> > >>> Are you testing for correct error returned for a particular case? > >> Don’t understand the question. I just check error messages that appeared > >> after travis/gitlab testing. > >> > > Different errors makes different messages. > > Same error can make different messages on different platforms. > > > > You put both into one: you don't care about what exact error came to > > you, right? > Right. The most important thing here is that the error is from getaddrinfo. Here I disagree. We want to verify that an error from a specific set of errors occurs. AFAIR, three errors are possible when trying to resolve a non-existent hostname: EAI_NONAME, EAI_AGAIN (and, as I see from the messages, EAI_SERVICE; maybe it is when ipv6 is semi-configured like on travis-ci). Actual EAI_* error for the test depends on network conditions and host configuration, messages are platform dependent. See also: https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/003399.html Roman, I suggest to clarify things that Sergos asked in the comments near to the check: if it is not clear for a reviewer, then a future reader may hang with it too. And, please (AFAIR I already asked), comment which message is for which EAI_* constant. WBR, Alexander Turenko. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-04-26 15:46 ` Alexander Turenko @ 2020-04-26 16:26 ` Roman Khabibov 2020-07-13 9:51 ` sergos 0 siblings, 1 reply; 23+ messages in thread From: Roman Khabibov @ 2020-04-26 16:26 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches > On Apr 26, 2020, at 18:46, Alexander Turenko <alexander.turenko@tarantool.org> wrote: > > On Sun, Apr 26, 2020 at 06:16:29AM +0300, Roman Khabibov wrote: >> >> >>> On Apr 24, 2020, at 20:18, Sergey Ostanevich <sergos@tarantool.org> wrote: >>> >>> On 24 апр 14:42, Roman Khabibov wrote: >>>> Hi! Thanks for the review. >>>> >>>>> On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote: >>>>> >>>>> On 06 апр 05:08, Roman Khabibov wrote: >>>>>>>> +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' or >>>>>>>> + err == 'getaddrinfo: hostname nor servname provided, or not known' or >>>>>>>> + err == 'getaddrinfo: Temporary failure in name resolution' or >>>>>>>> + err == 'getaddrinfo: Name could not be resolved at this time' then >>>>>>>> + return true >>>>>>>> + end >>>>>>>> + return false >>>>>>>> +end; >>>>>>> I really doubt that different error messages from the set above will appear >>>>>>> for the same error on different platforms. Could you please check for particular >>>>>>> output for each case you trigger below? >>>>>> Look at that: >>>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed >>>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t >>>>>> >>>>> >>>>> Exactly, this means for the error EAI_NONAME those OSes have differnet >>>>> messages. But you've put different errors in one bunch - I meant it >>>>> should never return 'Temporary failure in name resolution' as a result >>>>> for EAI_SERVICE, right? >>>>> >>>>> Are you testing for correct error returned for a particular case? >>>> Don’t understand the question. I just check error messages that appeared >>>> after travis/gitlab testing. >>>> >>> Different errors makes different messages. >>> Same error can make different messages on different platforms. >>> >>> You put both into one: you don't care about what exact error came to >>> you, right? >> Right. The most important thing here is that the error is from getaddrinfo. > > Here I disagree. We want to verify that an error from a specific set of > errors occurs. AFAIR, three errors are possible when trying to resolve a > non-existent hostname: EAI_NONAME, EAI_AGAIN (and, as I see from the > messages, EAI_SERVICE; maybe it is when ipv6 is semi-configured like on > travis-ci). > > Actual EAI_* error for the test depends on network conditions and host > configuration, messages are platform dependent. > > See also: > https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/003399.html > > Roman, I suggest to clarify things that Sergos asked in the comments > near to the check: if it is not clear for a reviewer, then a future > reader may hang with it too. Ok, Sergos, can you just show me what is wrong in this code. Sorry, but I can’t figure out. There is the set of error messages, and there is the corresponding constants. If several error messages correspond to the same constant, then these messages are for different OS. +-- gh-4138 Check getaddrinfo() error from socket:connect() only. +-- Error code and error message returned by getaddrinfo() depends +-- on system's gai_strerror(). +test_run:cmd("setopt delimiter ';'") +function check_err(err) +-- EAI_NONAME + if err == 'getaddrinfo: nodename nor servname provided, or not known' or +-- EAI_SERVICE + err == 'getaddrinfo: Servname not supported for ai_socktype' or +-- EAI_NONAME + err == 'getaddrinfo: Name or service not known' or +-- EAI_NONAME + err == 'getaddrinfo: hostname nor servname provided, or not known' or +-- EAI_AGAIN + err == 'getaddrinfo: Temporary failure in name resolution' or +-- EAI_AGAIN + err == 'getaddrinfo: Name could not be resolved at this time' then + return true + end + return false +end; > And, please (AFAIR I already asked), comment which message is for which > EAI_* constant. > > WBR, Alexander Turenko. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors 2020-04-26 16:26 ` Roman Khabibov @ 2020-07-13 9:51 ` sergos 0 siblings, 0 replies; 23+ messages in thread From: sergos @ 2020-07-13 9:51 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches, Alexander Turenko Hi! Sorry for dropping out. My initial concern was that we put a bunch of errors, such as EAI_NONAME and another bunch of messages, which can appear differently depending on OS or network conditions, into one bowl and mix them up. I believe there should be a better approach, such as for each error a particular set of messages can appear. Now I tend to understand your statement as ‘an messages can appear for each error’. Is it right? Regards, Sergos > On 26 Apr 2020, at 19:26, Roman Khabibov <roman.habibov@tarantool.org> wrote: > > > >> On Apr 26, 2020, at 18:46, Alexander Turenko <alexander.turenko@tarantool.org> wrote: >> >> On Sun, Apr 26, 2020 at 06:16:29AM +0300, Roman Khabibov wrote: >>> >>> >>>> On Apr 24, 2020, at 20:18, Sergey Ostanevich <sergos@tarantool.org> wrote: >>>> >>>> On 24 апр 14:42, Roman Khabibov wrote: >>>>> Hi! Thanks for the review. >>>>> >>>>>> On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote: >>>>>> >>>>>> On 06 апр 05:08, Roman Khabibov wrote: >>>>>>>>> +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' or >>>>>>>>> + err == 'getaddrinfo: hostname nor servname provided, or not known' or >>>>>>>>> + err == 'getaddrinfo: Temporary failure in name resolution' or >>>>>>>>> + err == 'getaddrinfo: Name could not be resolved at this time' then >>>>>>>>> + return true >>>>>>>>> + end >>>>>>>>> + return false >>>>>>>>> +end; >>>>>>>> I really doubt that different error messages from the set above will appear >>>>>>>> for the same error on different platforms. Could you please check for particular >>>>>>>> output for each case you trigger below? >>>>>>> Look at that: >>>>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed >>>>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t >>>>>>> >>>>>> >>>>>> Exactly, this means for the error EAI_NONAME those OSes have differnet >>>>>> messages. But you've put different errors in one bunch - I meant it >>>>>> should never return 'Temporary failure in name resolution' as a result >>>>>> for EAI_SERVICE, right? >>>>>> >>>>>> Are you testing for correct error returned for a particular case? >>>>> Don’t understand the question. I just check error messages that appeared >>>>> after travis/gitlab testing. >>>>> >>>> Different errors makes different messages. >>>> Same error can make different messages on different platforms. >>>> >>>> You put both into one: you don't care about what exact error came to >>>> you, right? >>> Right. The most important thing here is that the error is from getaddrinfo. >> >> Here I disagree. We want to verify that an error from a specific set of >> errors occurs. AFAIR, three errors are possible when trying to resolve a >> non-existent hostname: EAI_NONAME, EAI_AGAIN (and, as I see from the >> messages, EAI_SERVICE; maybe it is when ipv6 is semi-configured like on >> travis-ci). >> >> Actual EAI_* error for the test depends on network conditions and host >> configuration, messages are platform dependent. >> >> See also: >> https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/003399.html >> >> Roman, I suggest to clarify things that Sergos asked in the comments >> near to the check: if it is not clear for a reviewer, then a future >> reader may hang with it too. > Ok, Sergos, can you just show me what is wrong in this code. Sorry, but I > can’t figure out. There is the set of error messages, and there is the > corresponding constants. If several error messages correspond to the same > constant, then these messages are for different OS. > > +-- gh-4138 Check getaddrinfo() error from socket:connect() only. > +-- Error code and error message returned by getaddrinfo() depends > +-- on system's gai_strerror(). > +test_run:cmd("setopt delimiter ';'") > +function check_err(err) > +-- EAI_NONAME > + if err == 'getaddrinfo: nodename nor servname provided, or not known' or > +-- EAI_SERVICE > + err == 'getaddrinfo: Servname not supported for ai_socktype' or > +-- EAI_NONAME > + err == 'getaddrinfo: Name or service not known' or > +-- EAI_NONAME > + err == 'getaddrinfo: hostname nor servname provided, or not known' or > +-- EAI_AGAIN > + err == 'getaddrinfo: Temporary failure in name resolution' or > +-- EAI_AGAIN > + err == 'getaddrinfo: Name could not be resolved at this time' then > + return true > + end > + return false > +end; > >> And, please (AFAIR I already asked), comment which message is for which >> EAI_* constant. >> >> WBR, Alexander Turenko. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/2] Return getaddrinfo() errors 2020-03-12 10:24 [Tarantool-patches] [PATCH v2 0/2] Return getaddrinfo() errors Roman Khabibov 2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS Roman Khabibov 2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors Roman Khabibov @ 2020-07-28 14:08 ` Kirill Yukhin 2 siblings, 0 replies; 23+ messages in thread From: Kirill Yukhin @ 2020-07-28 14:08 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches Hello, On 12 мар 13:24, Roman Khabibov wrote: > Issue: https://github.com/tarantool/tarantool/issues/4138 > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4138-getaddrinfo-full-ci > > Roman Khabibov (2): > coio/say: fix getaddrinfo error handling on macOS > lua: return getaddrinfo() errors I've checked your patchset into 2.5 and master. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-07-28 14:08 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-12 10:24 [Tarantool-patches] [PATCH v2 0/2] Return getaddrinfo() errors Roman Khabibov 2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS Roman Khabibov 2020-03-29 8:51 ` Sergey Ostanevich 2020-03-29 9:12 ` Alexander Turenko 2020-04-06 2:07 ` Roman Khabibov 2020-04-17 9:37 ` Sergey Ostanevich 2020-04-24 11:32 ` Roman Khabibov 2020-06-23 13:27 ` Roman Khabibov 2020-07-23 14:12 ` Sergey Ostanevich 2020-07-28 9:26 ` Roman Khabibov 2020-04-26 17:20 ` Vladislav Shpilevoy 2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors Roman Khabibov 2020-03-29 9:07 ` Sergey Ostanevich 2020-03-29 9:25 ` Alexander Turenko 2020-04-06 2:08 ` Roman Khabibov 2020-04-16 10:27 ` Sergey Ostanevich 2020-04-24 11:42 ` Roman Khabibov 2020-04-24 17:18 ` Sergey Ostanevich 2020-04-26 3:16 ` Roman Khabibov 2020-04-26 15:46 ` Alexander Turenko 2020-04-26 16:26 ` Roman Khabibov 2020-07-13 9:51 ` sergos 2020-07-28 14:08 ` [Tarantool-patches] [PATCH v2 0/2] Return " Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox