From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (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 4376B445320 for ; Thu, 23 Jul 2020 17:12:12 +0300 (MSK) Date: Thu, 23 Jul 2020 17:12:10 +0300 From: Sergey Ostanevich Message-ID: <20200723141210.GD894@tarantool.org> References: <20200312102434.97300-1-roman.habibov@tarantool.org> <20200312102434.97300-2-roman.habibov@tarantool.org> <20200329085141.GC328@tarantool.org> <20200417093736.GB3110@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org, alexander.turenko@tarantool.org 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 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 > >> 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 > 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 *** >