[Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
Roman Khabibov
roman.habibov at tarantool.org
Tue Dec 10 19:25:05 MSK 2019
Hi! Thanks for the review.
> On Dec 8, 2019, at 22:48, Alexander Turenko <alexander.turenko at tarantool.org> wrote:
>
>> @@ -72,14 +72,28 @@ static void
>> test_getaddrinfo(void)
>> {
>> header();
>> - plan(1);
>> + plan(3);
>> const char *host = "127.0.0.1";
>> const char *port = "3333";
>> struct addrinfo *i;
>> /* NULL hints should work. It is a standard. */
>> int rc = coio_getaddrinfo(host, port, NULL, &i, 1);
>> is(rc, 0, "getaddrinfo");
>> - freeaddrinfo(i);
>
> Why do you remove freeing of the previous result? It will lead to a leak
> and so will require extra work to run unit tests under ASAN or Valgrind.
Sorry. I did it for debug and forgot to return this line.
@@ -81,6 +81,24 @@ 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";
+ bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 ||
+ strcmp(errmsg, exp_errmsg_2) == 0 ||
+ strcmp(errmsg, exp_errmsg_3) == 0;
+ is(is_match_with_exp, true, "getaddrinfo error message");
+
>> +
>> + /* gh-4138: Check getaddrinfo() retval. */
>
> Nit: retval and diagnostics area. You changed the code and the comment
> becomes inaccurate.
>
>> + 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";
>> + bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 ||
>> + strcmp(errmsg, exp_errmsg_2) == 0 ||
>> + strcmp(errmsg, exp_errmsg_3) == 0;
>> + is(is_match_with_exp, true, "getaddrinfo error message");
>>
>> /*
>> * gh-4209: 0 timeout should not be a special value and
>>
>>> The patch looks okay other then this.
>>
>> commit d9c23fde4c7c1fa2979954be83c1a2f6f8407347
>> Author: Roman Khabibov <roman.habibov at 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 then say that it should
>> be handler in a better way after #1148. Also, two diag_set() in
>
> 'and then say' -- looks as copy-paste from review notes.
>
> Typo: handler -> handled.
Done.
>> syslog_connect_unix() was added to avoid asserts in this
>> diag_log().
>>
>> Need for #4138
commit ff683ad948dd9404221979f399b155c63aa88e5a
Author: Roman Khabibov <roman.habibov at 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 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..aa38f5a99 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,24 @@ 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";
+ bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 ||
+ strcmp(errmsg, exp_errmsg_2) == 0 ||
+ strcmp(errmsg, exp_errmsg_3) == 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 ***
More information about the Tarantool-patches
mailing list