From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B0E7221905 for ; Tue, 10 Sep 2019 08:52:05 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rNPl9XDVAg3y for ; Tue, 10 Sep 2019 08:52:05 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B7653218DB for ; Tue, 10 Sep 2019 08:52:04 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 2/2] say: take getaddrinfo() errors into account From: Roman Khabibov In-Reply-To: <20190906134443.kc5anat2t3q26l3o@tkn_work_nb> Date: Tue, 10 Sep 2019 15:52:00 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <3603f7507651b37ddd549a8c247709cc7ff43f44.1561469272.git.roman.habibov@tarantool.org> <20190723145249.5xwc2td6omphwwzw@tkn_work_nb> <20190828213431.3yd4kwcahe2oizgs@tkn_work_nb> <8E98F721-601F-436D-8F0A-5E399D8F7CAB@tarantool.org> <20190906134443.kc5anat2t3q26l3o@tkn_work_nb> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Alexander Turenko > On Sep 6, 2019, at 16:44, Alexander Turenko = wrote: >=20 >>>> +-- >>>> +-- gh-4138: check getaddrinfo() error and panic after that. >>>> +-- >>>> +code=3D[[ >>>> +local socket =3D require('socket') >>>> +local log =3D require('log') >>>> +local fio =3D require('fio') >>>> + >>>> +path =3D fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock') >>>> +unix_socket =3D socket('AF_UNIX', 'SOCK_DGRAM', 0) >>>> +unix_socket:bind('unix/', path) >>>> + >>>> +opt =3D = string.format("syslog:server=3Dnon_exists_hostname:%s,identity=3Dtarantool= ", path) >>>> +box.cfg{log =3D opt, log_nonblock=3Dtrue} >>>=20 >>> log_nonblock is not needed here, so it is better to remove it. >> Removed. >=20 > Still no. >=20 >>> box.cfg{log =3D 'syslog:server=3Dnon_exists_hostname:3301'} is = enough, not need to >>> form a file path, no need identity, no need requiring socket, log = and fio. >>>=20 >>> The test passes even before the patch, so what it is intended to = test? I >>> think we should write a test that verifies stderr output to find all = log >>> messages we expect to appear in the case: >>>=20 >>> Linux: >>>=20 >>> | SystemError getaddrinfo: Temporary failure in name resolution: = Input/output error >>> | SystemError syslog logger: Input/output error: Input/output error >>> | failed to initialize logging subsystem >>>=20 >>> gai_strerror() message corresponds to EAI_AGAIN. >>>=20 >>> Mac OS: >>>=20 >>> | 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 >>>=20 >>> gai_strerror() message corresponds to EAI_NONAME. >>>=20 >>> I propose to call ffi.C.gai_strerror() right from a test to form two >>> error messages and verify that the actual input match one of them. >>>=20 >>> If it is hard to catch stderr, then let's proceed w/o this test. = However >>> I think it is doable. >>>=20 >>> I also propose to test error messages in the similar way (using >>> ffi.C.gai_strerror(GAI_AGAIN) and ffi.C.gai_strerror(GAI_NONAME)) in >>> test cases in second patch of the patchset. >> We discussed that with Vova and we decided, that it is too difficult. >> It is better, to waste not time for this test. Test for PANIC is = enough. >=20 > I don't see any reason to add a test case that checks that tarantool > exits in the case: it was so before the commit and nothing is changed > except the error message. I propose to delete the test case. Ok. Removed. >>>> + /* gh-4138: Check getaddrinfo() error. */ >>>> + isnt(coio_getaddrinfo("non_exists_hostname", port, NULL, &i, 1), = 0, >>>> + "getaddrinfo error"); >>>=20 >>> I would say 'getaddrinfo retval' instead 'getaddrinfo error'. >=20 > I don't insist, but remind about that here if it was missed by a > mistake. + /* gh-4138: Check getaddrinfo() retval. */ + rc =3D coio_getaddrinfo("non_exists_hostname", port, NULL, &i, = 1); + isnt(rc, 0, "getaddrinfo retval"); + isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL, + "getaddrinfo error message"); + >>>> + isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL, >>>> + "getaddrinfo error message"); >>>> + >>>=20 >>> I propose to verify the entire error message using >>> gai_strerror(GAI_AGAIN) and gai_strerror(GAI_NONAME)=E2=80=94just as = proposed >>> above for a log message. >> I have not found the way, how to carry this macros/enum from the libc = header to Lua. >=20 > It is possible to add them into socket.c, but anyway it is not worth = to > do this just for a test. Let's check for certain error messages (not = for > just 'getaddrinfo: ' prefix). commit 3e973c2203131ab663bdf1154be73ff6592568ea Author: Roman Khabibov Date: Tue Jul 30 15:39:21 2019 +0300 coiox/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 then say that it should be handler 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 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"); 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"); 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(); /* 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..3758c8857 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,12 @@ test_getaddrinfo(void) is(rc, 0, "getaddrinfo"); freeaddrinfo(i); =20 + /* gh-4138: Check getaddrinfo() retval. */ + rc =3D coio_getaddrinfo("non_exists_hostname", port, NULL, &i, = 1); + isnt(rc, 0, "getaddrinfo retval"); + isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL, + "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 ***