From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5BD604696C3 for ; Mon, 6 Apr 2020 05:07:59 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3594.4.19\)) From: Roman Khabibov In-Reply-To: <20200329085141.GC328@tarantool.org> Date: Mon, 6 Apr 2020 05:07:57 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20200312102434.97300-1-roman.habibov@tarantool.org> <20200312102434.97300-2-roman.habibov@tarantool.org> <20200329085141.GC328@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Mar 29, 2020, at 11:51, Sergey Ostanevich = wrote: >=20 > Hi! >=20 > Thanks for the patch! > See below on my comments. >=20 > Sergos. >=20 > On 12 =D0=BC=D0=B0=D1=80 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). >>=20 >> 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. >=20 >> syslog_connect_unix() was added to avoid asserts in this >> diag_log(). >>=20 >> 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(-) >>=20 >> diff --git a/src/lib/core/coio_task.c b/src/lib/core/coio_task.c >> index 908b336ed..83f669d05 100644 >> --- a/src/lib/core/coio_task.c >> +++ b/src/lib/core/coio_task.c >> @@ -413,7 +413,7 @@ coio_getaddrinfo(const char *host, const char = *port, >> return -1; /* timed out or cancelled */ >>=20 >> /* Task finished */ >> - if (task->rc < 0) { >> + if (task->rc !=3D 0) { >> /* getaddrinfo() failed */ >> errno =3D 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 =3D 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 =3D AF_UNIX; >> if (connect(fd, (struct sockaddr *) &un, sizeof(un)) !=3D 0) { >> close(fd); >> + diag_set(SystemError, "connect"); > Ditto. @@ -465,13 +465,16 @@ static inline int syslog_connect_unix(const char *path) { int fd =3D 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 =3D AF_UNIX; if (connect(fd, (struct sockaddr *) &un, sizeof(un)) !=3D 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 =3D IPPROTO_UDP; >>=20 >> ret =3D getaddrinfo(remote, portnum, &hints, &inf); >> - if (ret < 0) { >> + if (ret !=3D 0) { >> errno =3D 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 =3D 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 =3D "127.0.0.1"; >> const char *port =3D "3333"; >> struct addrinfo *i; >> @@ -81,6 +81,33 @@ test_getaddrinfo(void) >> is(rc, 0, "getaddrinfo"); >> freeaddrinfo(i); >>=20 >> + /* >> + * gh-4138: Check getaddrinfo() retval and diagnostics >> + * area. >> + */ >> + rc =3D coio_getaddrinfo("non_exists_hostname", port, NULL, &i, >> + 15768000000); >> + isnt(rc, 0, "getaddrinfo retval"); >> + const char *errmsg =3D diag_get()->last->errmsg; >> + const char *exp_errmsg_1 =3D "getaddrinfo: nodename nor servname = provided" >> + ", or not known"; >> + const char *exp_errmsg_2 =3D "getaddrinfo: Servname not = supported for " >> + "ai_socktype"; >> + const char *exp_errmsg_3 =3D "getaddrinfo: Name or service not = known"; >> + const char *exp_errmsg_4 =3D "getaddrinfo: hostname nor servname = provided" >> + ", or not known"; >> + const char *exp_errmsg_5 =3D "getaddrinfo: Temporary failure in = name " >> + "resolution"; >> + const char *exp_errmsg_6 =3D "getaddrinfo: Name could not be = resolved at " >> + "this time"; >> + bool is_match_with_exp =3D strcmp(errmsg, exp_errmsg_1) =3D=3D 0 = || >> + strcmp(errmsg, exp_errmsg_2) =3D=3D 0 || >> + strcmp(errmsg, exp_errmsg_3) =3D=3D 0 || >> + strcmp(errmsg, exp_errmsg_4) =3D=3D 0 || >> + strcmp(errmsg, exp_errmsg_5) =3D=3D 0 || >> + strcmp(errmsg, exp_errmsg_6) =3D=3D 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 *** >> --=20 >> 2.21.0 (Apple Git-122) commit f17e3e73ae2689dd2ec1dcd94d699636f19f93a5 Author: Roman Khabibov Date: Tue Jul 30 15:39:21 2019 +0300 coio/say: fix getaddrinfo error handling on macOS =20 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). =20 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(). =20 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 */ =20 /* Task finished */ - if (task->rc < 0) { + if (task->rc !=3D 0) { /* getaddrinfo() failed */ errno =3D 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 =3D 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 =3D AF_UNIX; if (connect(fd, (struct sockaddr *) &un, sizeof(un)) !=3D 0) { + diag_set(SystemError, strerror(errno)); close(fd); return -1; } @@ -512,7 +515,7 @@ syslog_connect_remote(const char *server_address) hints.ai_protocol =3D IPPROTO_UDP; =20 ret =3D getaddrinfo(remote, portnum, &hints, &inf); - if (ret < 0) { + if (ret !=3D 0) { errno =3D 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 =3D 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 =3D "127.0.0.1"; const char *port =3D "3333"; struct addrinfo *i; @@ -81,6 +81,39 @@ test_getaddrinfo(void) is(rc, 0, "getaddrinfo"); freeaddrinfo(i); =20 + /* + * gh-4138: Check getaddrinfo() retval and diagnostics + * area. + */ + rc =3D coio_getaddrinfo("non_exists_hostname", port, NULL, &i, + 15768000000); + isnt(rc, 0, "getaddrinfo retval"); + const char *errmsg =3D diag_get()->last->errmsg; + /* EAI_NONAME */ + const char *exp_errmsg_1 =3D "getaddrinfo: nodename nor servname = provided" + ", or not known"; + /* EAI_SERVICE */ + const char *exp_errmsg_2 =3D "getaddrinfo: Servname not = supported for " + "ai_socktype"; + /* EAI_NONAME */ + const char *exp_errmsg_3 =3D "getaddrinfo: Name or service not = known"; + /* EAI_NONAME */ + const char *exp_errmsg_4 =3D "getaddrinfo: hostname nor servname = provided" + ", or not known"; + /* EAI_AGAIN */ + const char *exp_errmsg_5 =3D "getaddrinfo: Temporary failure in = name " + "resolution"; + /* EAI_AGAIN */ + const char *exp_errmsg_6 =3D "getaddrinfo: Name could not be = resolved at " + "this time"; + bool is_match_with_exp =3D strcmp(errmsg, exp_errmsg_1) =3D=3D 0 = || + strcmp(errmsg, exp_errmsg_2) =3D=3D 0 || + strcmp(errmsg, exp_errmsg_3) =3D=3D 0 || + strcmp(errmsg, exp_errmsg_4) =3D=3D 0 || + strcmp(errmsg, exp_errmsg_5) =3D=3D 0 || + strcmp(errmsg, exp_errmsg_6) =3D=3D 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 ***