Tarantool development patches archive
 help / color / mirror / Atom feed
From: Roman Khabibov <roman.habibov@tarantool.org>
To: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
Date: Thu, 21 Nov 2019 20:28:06 +0300	[thread overview]
Message-ID: <E1252366-59AF-47C1-AB36-F24A2A1DF4AF@tarantool.org> (raw)
In-Reply-To: <20191101151939.lewxpitqxzyn6dz6@tkn_work_nb>



> On Nov 1, 2019, at 18:19, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
>> coiox/say: fix getaddrinfo error handling on macOS
> 
> Typo: coiox -> coio.
Ok.

>>>>>> +	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
>>>>>> +	     "getaddrinfo error message");
>>>>>> +
>>>>> 
>>>>> I propose to verify the entire error message using
>>>>> gai_strerror(GAI_AGAIN) and gai_strerror(GAI_NONAME)—just 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.
>>> 
>>> 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).
> 
> Again: let's check for certain error messages (not for just
> 'getaddrinfo: ' prefix).
I have changed the timeout to 15768000000, because last->errmsg was ’timed out’.

@@ -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);
+
+	/* gh-4138: Check getaddrinfo() retval. */
+	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@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
    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..d5cbd2fb9 100644
--- a/test/unit/coio.cc
+++ b/test/unit/coio.cc
@@ -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);
+
+	/* gh-4138: Check getaddrinfo() retval. */
+	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
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 ***

  reply	other threads:[~2019-11-21 17:28 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-25 13:38 [tarantool-patches] [PATCH v2 0/2] " Roman Khabibov
2019-06-25 13:38 ` [tarantool-patches] [PATCH v2 2/2] say: " Roman Khabibov
2019-07-23 14:52   ` [tarantool-patches] " Alexander Turenko
2019-08-05 13:32     ` Roman Khabibov
2019-08-28 21:34       ` Alexander Turenko
2019-08-29  0:51         ` Alexander Turenko
     [not found]         ` <8E98F721-601F-436D-8F0A-5E399D8F7CAB@tarantool.org>
2019-09-06 13:44           ` Alexander Turenko
2019-09-10 12:52             ` Roman Khabibov
2019-11-01 15:19               ` [Tarantool-patches] " Alexander Turenko
2019-11-21 17:28                 ` Roman Khabibov [this message]
2019-12-08 19:48                   ` [Tarantool-patches] [tarantool-patches] " Alexander Turenko
2019-12-10 16:25                     ` Roman Khabibov
2019-12-18 15:01                       ` Alexander Turenko
2019-12-21 17:50                         ` Roman Khabibov
2019-12-23 12:56                           ` Alexander Turenko
2019-12-23 13:38                             ` Alexander Turenko
2019-12-26 17:29                               ` Roman Khabibov
2019-12-26 21:05                                 ` Alexander Turenko
2019-12-27 13:01                                   ` Roman Khabibov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=E1252366-59AF-47C1-AB36-F24A2A1DF4AF@tarantool.org \
    --to=roman.habibov@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox