[Tarantool-patches] [PATCH v2 0/2] Return getaddrinfo() errors

[Tarantool-patches] [PATCH v2 0/2] Return getaddrinfo() errors

[Roman Khabibov]

Issue: https://github.com/tarantool/tarantool/issues/4138
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4138-getaddrinfo-full-ci

Roman Khabibov (2):
  coio/say: fix getaddrinfo error handling on macOS
  lua: return getaddrinfo() errors

 src/box/lua/net_box.lua   |  7 +++--
 src/lib/core/coio_task.c  |  2 +-
 src/lib/core/say.c        | 12 ++++++--
 src/lua/socket.c          | 16 +++++++++-
 src/lua/socket.lua        | 22 ++++++++++----
 test/app/socket.result    | 63 ++++++++++++++++++++++++++++++++++++---
 test/app/socket.test.lua  | 35 ++++++++++++++++++++--
 test/box/net.box.result   | 31 +++++++++++++++++++
 test/box/net.box.test.lua | 20 +++++++++++++
 test/unit/coio.cc         | 29 +++++++++++++++++-
 test/unit/coio.result     |  4 ++-
 11 files changed, 221 insertions(+), 20 deletions(-)

-- 
2.21.0 (Apple Git-122)

[Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Roman Khabibov]

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
---
 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");
 		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..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");
+
 	/*
 	 * 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)

[Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Roman Khabibov]

Add getaddrinfo() errors into the several fuctions of socket. Now
getaddrinfo() can return a pair of values (nil and error message)
in case of error.

Closes #4138

@TarantoolBot document
Title: socket API changes

* socket.getaddrinfo()

Can return error message as second return value in case of
internal getaddrinfo() error.

Example:
tarantool> socket.getaddrinfo('non_exists_hostname', 3301)
---
- null
- 'getaddrinfo: nodename nor servname provided, or not known'
...

* socket.tcp_connect()

Can return socket.getaddrinfo() error message as second return
value.

Example:
tarantool> socket.tcp_connect('non_exists_hostname', 3301)
---
- null
- 'getaddrinfo: nodename nor servname provided, or not known'
...
---
 src/box/lua/net_box.lua   |  7 +++--
 src/lua/socket.c          | 16 +++++++++-
 src/lua/socket.lua        | 22 ++++++++++----
 test/app/socket.result    | 63 ++++++++++++++++++++++++++++++++++++---
 test/app/socket.test.lua  | 35 ++++++++++++++++++++--
 test/box/net.box.result   | 31 +++++++++++++++++++
 test/box/net.box.test.lua | 20 +++++++++++++
 7 files changed, 179 insertions(+), 15 deletions(-)

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 3f611c027..8068a8637 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -154,9 +154,12 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
 local function establish_connection(host, port, timeout)
     local timeout = timeout or DEFAULT_CONNECT_TIMEOUT
     local begin = fiber.clock()
-    local s = socket.tcp_connect(host, port, timeout)
+    local s, err = socket.tcp_connect(host, port, timeout)
     if not s then
-        return nil, errno.strerror(errno())
+        if not err then
+            return nil, errno.strerror(errno())
+        end
+        return nil, err
     end
     local msg = s:read({chunk = IPROTO_GREETING_SIZE},
                         timeout - (fiber.clock() - begin))
diff --git a/src/lua/socket.c b/src/lua/socket.c
index e75a8802e..ba683ad3a 100644
--- a/src/lua/socket.c
+++ b/src/lua/socket.c
@@ -775,6 +775,18 @@ lbox_getaddrinfo_result_wrapper(struct lua_State *L)
 	return 1;
 }
 
+/**
+ * Wrap coio_getaddrinfo() and call it. Push returned values onto
+ * @a L Lua stack.
+ *
+ * @param L Lua stack.
+ *
+ * @retval 1 Number of returned values by Lua function if
+ * coio_getaddrinfo() success.
+ * @retval 2 Number of returned values by Lua function if
+ * coio_getaddrinfo() is failed (first is nil, second is error
+ * message).
+ */
 static int
 lbox_socket_getaddrinfo(struct lua_State *L)
 {
@@ -817,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L)
 
 	if (dns_res != 0) {
 		lua_pushnil(L);
-		return 1;
+		struct error *err = diag_get()->last;
+		lua_pushstring(L, err->errmsg);
+		return 2;
 	}
 
 	/* no results */
diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index a334ad45b..ce6dab301 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -1041,9 +1041,12 @@ local function tcp_connect(host, port, timeout)
     end
     local timeout = timeout or TIMEOUT_INFINITY
     local stop = fiber.clock() + timeout
-    local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
+    local dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
         protocol = 'tcp' })
-    if dns == nil or #dns == 0 then
+    if dns == nil then
+        return nil, err
+    end
+    if #dns == 0 then
         boxerrno(boxerrno.EINVAL)
         return nil
     end
@@ -1360,8 +1363,12 @@ local function lsocket_tcp_connect(self, host, port)
     -- This function is broken by design
     local ga_opts = { family = 'AF_INET', type = 'SOCK_STREAM' }
     local timeout = deadline - fiber.clock()
-    local dns = getaddrinfo(host, port, timeout, ga_opts)
-    if dns == nil or #dns == 0 then
+    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
+    if dns == nil then
+        self._errno = boxerrno.EINVAL
+        return nil, err
+    end
+    if #dns == 0 then
         self._errno = boxerrno.EINVAL
         return nil, socket_error(self)
     end
@@ -1547,9 +1554,12 @@ local function lsocket_connect(host, port)
     if host == nil or port == nil then
         error("Usage: luasocket.connect(host, port)")
     end
-    local s = tcp_connect(host, port)
+    local s, err = tcp_connect(host, port)
     if not s then
-        return nil, boxerrno.strerror()
+        if not err then
+            return nil, boxerrno.strerror()
+        end
+        return nil, err
     end
     setmetatable(s, lsocket_tcp_client_mt)
     return s
diff --git a/test/app/socket.result b/test/app/socket.result
index 9f0ea0a5d..3f26bf2b0 100644
--- a/test/app/socket.result
+++ b/test/app/socket.result
@@ -941,10 +941,20 @@ sc:close()
 - true
 ...
 -- tcp_connect
--- test timeout
-socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
+-- Test timeout. In this test, tcp_connect can return the second
+-- output value from internal.getaddrinfo (usually on Mac OS, but
+-- theoretically it can happen on Linux too). Sometimes
+-- getaddrinfo() is timed out, sometimes connect. On Linux however
+-- getaddrinfo is fast enough to never give timeout error in
+-- the case. So, there are two sources of timeout errors that are
+-- reported differently. This difference has appeared after
+-- gh-4138 patch.
+s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
 ---
-- null
+...
+s == nil
+---
+- true
 ...
 -- AF_INET
 s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
@@ -1595,7 +1605,7 @@ fio.stat(path) == nil
 { socket.tcp_connect('abrakadabra#123') == nil, errno.strerror() }
 ---
 - - true
-  - Invalid argument
+  - Input/output error
 ...
 -- wrong options for getaddrinfo
 socket.getaddrinfo('host', 'port', { type = 'WRONG' }) == nil and errno() == errno.EINVAL
@@ -2914,3 +2924,48 @@ srv:close()
 ---
 - true
 ...
+-- gh-4138 Check getaddrinfo() error from socket:connect() only.
+-- Error code and error message returned by getaddrinfo() depends
+-- on system's gai_strerror().
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+function check_err(err)
+    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
+       err == 'getaddrinfo: Servname not supported for ai_socktype' or
+       err == 'getaddrinfo: Name or service not known' or
+       err == 'getaddrinfo: hostname nor servname provided, or not known' or
+       err == 'getaddrinfo: Temporary failure in name resolution' or
+       err == 'getaddrinfo: Name could not be resolved at this time' then
+        return true
+    end
+    return false
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+s, err = socket.getaddrinfo('non_exists_hostname', 3301)
+---
+...
+check_err(err)
+---
+- true
+...
+s, err = socket.connect('non_exists_hostname', 3301)
+---
+...
+check_err(err)
+---
+- true
+...
+s, err = socket.tcp_connect('non_exists_hostname', 3301)
+---
+...
+check_err(err)
+---
+- true
+...
diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
index d1fe7ada6..c455d3ddb 100644
--- a/test/app/socket.test.lua
+++ b/test/app/socket.test.lua
@@ -300,8 +300,16 @@ sc:close()
 
 -- tcp_connect
 
--- test timeout
-socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
+-- Test timeout. In this test, tcp_connect can return the second
+-- output value from internal.getaddrinfo (usually on Mac OS, but
+-- theoretically it can happen on Linux too). Sometimes
+-- getaddrinfo() is timed out, sometimes connect. On Linux however
+-- getaddrinfo is fast enough to never give timeout error in
+-- the case. So, there are two sources of timeout errors that are
+-- reported differently. This difference has appeared after
+-- gh-4138 patch.
+s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
+s == nil
 
 -- AF_INET
 s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
@@ -988,3 +996,26 @@ s:receive('*a')
 s:close()
 srv:close()
 
+-- gh-4138 Check getaddrinfo() error from socket:connect() only.
+-- Error code and error message returned by getaddrinfo() depends
+-- on system's gai_strerror().
+test_run:cmd("setopt delimiter ';'")
+function check_err(err)
+    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
+       err == 'getaddrinfo: Servname not supported for ai_socktype' or
+       err == 'getaddrinfo: Name or service not known' or
+       err == 'getaddrinfo: hostname nor servname provided, or not known' or
+       err == 'getaddrinfo: Temporary failure in name resolution' or
+       err == 'getaddrinfo: Name could not be resolved at this time' then
+        return true
+    end
+    return false
+end;
+test_run:cmd("setopt delimiter ''");
+
+s, err = socket.getaddrinfo('non_exists_hostname', 3301)
+check_err(err)
+s, err = socket.connect('non_exists_hostname', 3301)
+check_err(err)
+s, err = socket.tcp_connect('non_exists_hostname', 3301)
+check_err(err)
diff --git a/test/box/net.box.result b/test/box/net.box.result
index e3dabf7d9..3e1ce6b11 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3927,6 +3927,37 @@ test_run:grep_log('default', '00000040:.*')
 ---
 - null
 ...
+-- gh-4138 Check getaddrinfo() error from connect() only. Error
+-- code and error message returned by getaddrinfo() depends on
+-- system's gai_strerror().
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+function check_err(err)
+    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
+       err == 'getaddrinfo: Servname not supported for ai_socktype' or
+       err == 'getaddrinfo: Name or service not known' or
+       err == 'getaddrinfo: hostname nor servname provided, or not known' or
+       err == 'getaddrinfo: Temporary failure in name resolution' or
+       err == 'getaddrinfo: Name could not be resolved at this time' then
+            return true
+    end
+    return false
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+s = remote.connect('non_exists_hostname:3301')
+---
+...
+check_err(s['error'])
+---
+- true
+...
 box.cfg{log_level=log_level}
 ---
 ...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 8e65ff470..cad8754c3 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1582,4 +1582,24 @@ test_run:wait_log('default', '00000030:.*', nil, 10)
 -- we expect nothing below, so don't wait
 test_run:grep_log('default', '00000040:.*')
 
+-- gh-4138 Check getaddrinfo() error from connect() only. Error
+-- code and error message returned by getaddrinfo() depends on
+-- system's gai_strerror().
+test_run:cmd("setopt delimiter ';'")
+function check_err(err)
+    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
+       err == 'getaddrinfo: Servname not supported for ai_socktype' or
+       err == 'getaddrinfo: Name or service not known' or
+       err == 'getaddrinfo: hostname nor servname provided, or not known' or
+       err == 'getaddrinfo: Temporary failure in name resolution' or
+       err == 'getaddrinfo: Name could not be resolved at this time' then
+            return true
+    end
+    return false
+end;
+test_run:cmd("setopt delimiter ''");
+
+s = remote.connect('non_exists_hostname:3301')
+check_err(s['error'])
+
 box.cfg{log_level=log_level}
-- 
2.21.0 (Apple Git-122)

Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Sergey Ostanevich]

Hi!

Thanks for the patch!
See below on my comments.

Sergos.

On 12 мар 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).
> 
> 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.

> 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.

>  		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

>  		/* 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?

>  	/*
>  	 * 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)
> 

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Sergey Ostanevich]

Hi!

Thanks for the patch, please see below.

Sergos

On 12 мар 13:24, Roman Khabibov wrote:
> Add getaddrinfo() errors into the several fuctions of socket. Now
> getaddrinfo() can return a pair of values (nil and error message)
> in case of error.
> 
> Closes #4138
> 
> @TarantoolBot document
> Title: socket API changes
> 
> * socket.getaddrinfo()
> 
> Can return error message as second return value in case of
> internal getaddrinfo() error.
I really don't like the case 'can return'. Can we work this out to
always return a pair of values?

> 
> Example:
> tarantool> socket.getaddrinfo('non_exists_hostname', 3301)
> ---
> - null
> - 'getaddrinfo: nodename nor servname provided, or not known'
> ...
> 
> * socket.tcp_connect()
> 
> Can return socket.getaddrinfo() error message as second return
> value.
> 
> Example:
> tarantool> socket.tcp_connect('non_exists_hostname', 3301)
> ---
> - null
> - 'getaddrinfo: nodename nor servname provided, or not known'
> ...
> ---
>  src/box/lua/net_box.lua   |  7 +++--
>  src/lua/socket.c          | 16 +++++++++-
>  src/lua/socket.lua        | 22 ++++++++++----
>  test/app/socket.result    | 63 ++++++++++++++++++++++++++++++++++++---
>  test/app/socket.test.lua  | 35 ++++++++++++++++++++--
>  test/box/net.box.result   | 31 +++++++++++++++++++
>  test/box/net.box.test.lua | 20 +++++++++++++
>  7 files changed, 179 insertions(+), 15 deletions(-)
> 
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 3f611c027..8068a8637 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -154,9 +154,12 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
>  local function establish_connection(host, port, timeout)
>      local timeout = timeout or DEFAULT_CONNECT_TIMEOUT
>      local begin = fiber.clock()
> -    local s = socket.tcp_connect(host, port, timeout)
> +    local s, err = socket.tcp_connect(host, port, timeout)
>      if not s then
> -        return nil, errno.strerror(errno())
> +        if not err then
> +            return nil, errno.strerror(errno())
> +        end
> +        return nil, err
>      end
>      local msg = s:read({chunk = IPROTO_GREETING_SIZE},
>                          timeout - (fiber.clock() - begin))
> diff --git a/src/lua/socket.c b/src/lua/socket.c
> index e75a8802e..ba683ad3a 100644
> --- a/src/lua/socket.c
> +++ b/src/lua/socket.c
> @@ -775,6 +775,18 @@ lbox_getaddrinfo_result_wrapper(struct lua_State *L)
>  	return 1;
>  }
>  
> +/**
> + * Wrap coio_getaddrinfo() and call it. Push returned values onto
> + * @a L Lua stack.
> + *
> + * @param L Lua stack.
> + *
> + * @retval 1 Number of returned values by Lua function if
> + * coio_getaddrinfo() success.
> + * @retval 2 Number of returned values by Lua function if
> + * coio_getaddrinfo() is failed (first is nil, second is error
> + * message).
> + */
>  static int
>  lbox_socket_getaddrinfo(struct lua_State *L)
>  {
> @@ -817,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L)
>  
>  	if (dns_res != 0) {
>  		lua_pushnil(L);
> -		return 1;
> +		struct error *err = diag_get()->last;
> +		lua_pushstring(L, err->errmsg);
> +		return 2;
>  	}
>  
>  	/* no results */
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index a334ad45b..ce6dab301 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -1041,9 +1041,12 @@ local function tcp_connect(host, port, timeout)
>      end
>      local timeout = timeout or TIMEOUT_INFINITY
>      local stop = fiber.clock() + timeout
> -    local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
> +    local dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
>          protocol = 'tcp' })
> -    if dns == nil or #dns == 0 then
> +    if dns == nil then
> +        return nil, err
You explicitly put error here...

> +    end
> +    if #dns == 0 then
>          boxerrno(boxerrno.EINVAL)
>          return nil
... and not here. Why do you keep using two versions of error passing,
not one? I would agree you need boxerrno for backward compatibility, but
to start moving towards {res, err} you have to introduce it everywhere. 

>      end
> @@ -1360,8 +1363,12 @@ local function lsocket_tcp_connect(self, host, port)
>      -- This function is broken by design
>      local ga_opts = { family = 'AF_INET', type = 'SOCK_STREAM' }
>      local timeout = deadline - fiber.clock()
> -    local dns = getaddrinfo(host, port, timeout, ga_opts)
> -    if dns == nil or #dns == 0 then
> +    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
> +    if dns == nil then
> +        self._errno = boxerrno.EINVAL
> +        return nil, err
> +    end
> +    if #dns == 0 then
>          self._errno = boxerrno.EINVAL
>          return nil, socket_error(self)
>      end
> @@ -1547,9 +1554,12 @@ local function lsocket_connect(host, port)
>      if host == nil or port == nil then
>          error("Usage: luasocket.connect(host, port)")
>      end
> -    local s = tcp_connect(host, port)
> +    local s, err = tcp_connect(host, port)
>      if not s then
> -        return nil, boxerrno.strerror()
> +        if not err then
> +            return nil, boxerrno.strerror()
> +        end
> +        return nil, err
>      end
>      setmetatable(s, lsocket_tcp_client_mt)
>      return s
> diff --git a/test/app/socket.result b/test/app/socket.result
> index 9f0ea0a5d..3f26bf2b0 100644
> --- a/test/app/socket.result
> +++ b/test/app/socket.result
> @@ -941,10 +941,20 @@ sc:close()
>  - true
>  ...
>  -- tcp_connect
> --- test timeout
> -socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
> +-- Test timeout. In this test, tcp_connect can return the second
> +-- output value from internal.getaddrinfo (usually on Mac OS, but
> +-- theoretically it can happen on Linux too). Sometimes
> +-- getaddrinfo() is timed out, sometimes connect. On Linux however
> +-- getaddrinfo is fast enough to never give timeout error in
> +-- the case. So, there are two sources of timeout errors that are
> +-- reported differently. This difference has appeared after
> +-- gh-4138 patch.
> +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
>  ---
> -- null
> +...
> +s == nil
> +---
> +- true
>  ...
>  -- AF_INET
>  s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
> @@ -1595,7 +1605,7 @@ fio.stat(path) == nil
>  { socket.tcp_connect('abrakadabra#123') == nil, errno.strerror() }
>  ---
>  - - true
> -  - Invalid argument
> +  - Input/output error
>  ...
>  -- wrong options for getaddrinfo
>  socket.getaddrinfo('host', 'port', { type = 'WRONG' }) == nil and errno() == errno.EINVAL
> @@ -2914,3 +2924,48 @@ srv:close()
>  ---
>  - true
>  ...
> +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
> +-- Error code and error message returned by getaddrinfo() depends
> +-- on system's gai_strerror().
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +function check_err(err)
> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> +       err == 'getaddrinfo: Name or service not known' or
> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> +        return true
> +    end
> +    return false
> +end;
> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +s, err = socket.getaddrinfo('non_exists_hostname', 3301)
> +---
> +...
> +check_err(err)
> +---
> +- true
> +...
> +s, err = socket.connect('non_exists_hostname', 3301)
> +---
> +...
> +check_err(err)
> +---
> +- true
> +...
> +s, err = socket.tcp_connect('non_exists_hostname', 3301)
> +---
> +...
> +check_err(err)
> +---
> +- true
> +...
> diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
> index d1fe7ada6..c455d3ddb 100644
> --- a/test/app/socket.test.lua
> +++ b/test/app/socket.test.lua
> @@ -300,8 +300,16 @@ sc:close()
>  
>  -- tcp_connect
>  
> --- test timeout
> -socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
> +-- Test timeout. In this test, tcp_connect can return the second
> +-- output value from internal.getaddrinfo (usually on Mac OS, but
> +-- theoretically it can happen on Linux too). Sometimes
> +-- getaddrinfo() is timed out, sometimes connect. On Linux however
> +-- getaddrinfo is fast enough to never give timeout error in
> +-- the case. So, there are two sources of timeout errors that are
> +-- reported differently. This difference has appeared after
> +-- gh-4138 patch.
> +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
> +s == nil
>  
>  -- AF_INET
>  s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
> @@ -988,3 +996,26 @@ s:receive('*a')
>  s:close()
>  srv:close()
>  
> +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
> +-- Error code and error message returned by getaddrinfo() depends
> +-- on system's gai_strerror().
> +test_run:cmd("setopt delimiter ';'")
> +function check_err(err)
> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> +       err == 'getaddrinfo: Name or service not known' or
> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> +        return true
> +    end
> +    return false
> +end;
I really doubt that different error messages from the set above will appear 
for the same error on different platforms. Could you please check for particular 
output for each case you trigger below?

> +test_run:cmd("setopt delimiter ''");
> +
> +s, err = socket.getaddrinfo('non_exists_hostname', 3301)
> +check_err(err)
> +s, err = socket.connect('non_exists_hostname', 3301)
> +check_err(err)
> +s, err = socket.tcp_connect('non_exists_hostname', 3301)
> +check_err(err)
> diff --git a/test/box/net.box.result b/test/box/net.box.result
> index e3dabf7d9..3e1ce6b11 100644
> --- a/test/box/net.box.result
> +++ b/test/box/net.box.result
> @@ -3927,6 +3927,37 @@ test_run:grep_log('default', '00000040:.*')
>  ---
>  - null
>  ...
> +-- gh-4138 Check getaddrinfo() error from connect() only. Error
> +-- code and error message returned by getaddrinfo() depends on
> +-- system's gai_strerror().
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +function check_err(err)
> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> +       err == 'getaddrinfo: Name or service not known' or
> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> +            return true
> +    end
> +    return false
> +end;
> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +s = remote.connect('non_exists_hostname:3301')
> +---
> +...
> +check_err(s['error'])
> +---
> +- true
> +...
>  box.cfg{log_level=log_level}
>  ---
>  ...
> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
> index 8e65ff470..cad8754c3 100644
> --- a/test/box/net.box.test.lua
> +++ b/test/box/net.box.test.lua
> @@ -1582,4 +1582,24 @@ test_run:wait_log('default', '00000030:.*', nil, 10)
>  -- we expect nothing below, so don't wait
>  test_run:grep_log('default', '00000040:.*')
>  
> +-- gh-4138 Check getaddrinfo() error from connect() only. Error
> +-- code and error message returned by getaddrinfo() depends on
> +-- system's gai_strerror().
> +test_run:cmd("setopt delimiter ';'")
> +function check_err(err)
> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> +       err == 'getaddrinfo: Name or service not known' or
> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> +            return true
> +    end
> +    return false
> +end;
> +test_run:cmd("setopt delimiter ''");
> +
> +s = remote.connect('non_exists_hostname:3301')
> +check_err(s['error'])
> +
>  box.cfg{log_level=log_level}
> -- 
> 2.21.0 (Apple Git-122)
>

Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Alexander Turenko]

> > 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)

 | void
 | SystemError::log() const
 | {
 | 	say_file_line(S_SYSERROR, file, line, strerror(saved_errno),
 | 		      "SystemError %s", errmsg);
 | }

https://github.com/tarantool/tarantool/blob/eaa860885dc5708956094845e20f5bc81275ca26/src/lib/core/exception.cc#L153-L158

> > 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?

I asked to check for certain errors rather than that some SystemError
with '^getaddrinfo: .*$' message raised. Sure, the return value of
getaddrinfo() depends on network conditions (I guess it depends of a DNS
server: whether it reports temporary or persistent error; but also
whether a network interface is up at all). gai_strerror() output
depends of OS.

I asked to comment messages with EAI_* constants that corresponds to
them, but it seems it was missed.

Sure, proper description is necessary here.

Nope, we unable to control network conditions and OS here and cannot
create a test case for each of those situations.

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Alexander Turenko]

> > diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> > index a334ad45b..ce6dab301 100644
> > --- a/src/lua/socket.lua
> > +++ b/src/lua/socket.lua
> > @@ -1041,9 +1041,12 @@ local function tcp_connect(host, port, timeout)
> >      end
> >      local timeout = timeout or TIMEOUT_INFINITY
> >      local stop = fiber.clock() + timeout
> > -    local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
> > +    local dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
> >          protocol = 'tcp' })
> > -    if dns == nil or #dns == 0 then
> > +    if dns == nil then
> > +        return nil, err
> You explicitly put error here...
> 
> > +    end
> > +    if #dns == 0 then
> >          boxerrno(boxerrno.EINVAL)
> >          return nil
> ... and not here. Why do you keep using two versions of error passing,
> not one? I would agree you need boxerrno for backward compatibility, but
> to start moving towards {res, err} you have to introduce it everywhere. 

Seems logical.

> > +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
> > +-- Error code and error message returned by getaddrinfo() depends
> > +-- on system's gai_strerror().
> > +test_run:cmd("setopt delimiter ';'")
> > +function check_err(err)
> > +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> > +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> > +       err == 'getaddrinfo: Name or service not known' or
> > +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> > +       err == 'getaddrinfo: Temporary failure in name resolution' or
> > +       err == 'getaddrinfo: Name could not be resolved at this time' then
> > +        return true
> > +    end
> > +    return false
> > +end;
> I really doubt that different error messages from the set above will appear 
> for the same error on different platforms. Could you please check for particular 
> output for each case you trigger below?

It is so. It varies even for the same system connected to different
networks.

Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Roman Khabibov]

Hi! Thanks for the review.

> On Mar 29, 2020, at 11:51, Sergey Ostanevich <sergos@tarantool.org> wrote:
> 
> Hi!
> 
> Thanks for the patch!
> See below on my comments.
> 
> Sergos.
> 
> On 12 мар 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).
>> 
>> 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.
> 
>> 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 <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 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 ***

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Roman Khabibov]

> On Mar 29, 2020, at 12:07, Sergey Ostanevich <sergos@tarantool.org> wrote:
> 
> Hi!
> 
> Thanks for the patch, please see below.
> 
> Sergos
> 
> On 12 мар 13:24, Roman Khabibov wrote:
>> Add getaddrinfo() errors into the several fuctions of socket. Now
>> getaddrinfo() can return a pair of values (nil and error message)
>> in case of error.
>> 
>> Closes #4138
>> 
>> @TarantoolBot document
>> Title: socket API changes
>> 
>> * socket.getaddrinfo()
>> 
>> Can return error message as second return value in case of
>> internal getaddrinfo() error.
> I really don't like the case 'can return'. Can we work this out to
> always return a pair of values?
Done.
>> 
>> Example:
>> tarantool> socket.getaddrinfo('non_exists_hostname', 3301)
>> ---
>> - null
>> - 'getaddrinfo: nodename nor servname provided, or not known'
>> ...
>> 
>> * socket.tcp_connect()
>> 
>> Can return socket.getaddrinfo() error message as second return
>> value.
>> 
>> Example:
>> tarantool> socket.tcp_connect('non_exists_hostname', 3301)
>> ---
>> - null
>> - 'getaddrinfo: nodename nor servname provided, or not known'
>> ...
>> ---
>> src/box/lua/net_box.lua   |  7 +++--
>> src/lua/socket.c          | 16 +++++++++-
>> src/lua/socket.lua        | 22 ++++++++++----
>> test/app/socket.result    | 63 ++++++++++++++++++++++++++++++++++++---
>> test/app/socket.test.lua  | 35 ++++++++++++++++++++--
>> test/box/net.box.result   | 31 +++++++++++++++++++
>> test/box/net.box.test.lua | 20 +++++++++++++
>> 7 files changed, 179 insertions(+), 15 deletions(-)
>> 
>> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
>> index 3f611c027..8068a8637 100644
>> --- a/src/box/lua/net_box.lua
>> +++ b/src/box/lua/net_box.lua
>> @@ -154,9 +154,12 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
>> local function establish_connection(host, port, timeout)
>>     local timeout = timeout or DEFAULT_CONNECT_TIMEOUT
>>     local begin = fiber.clock()
>> -    local s = socket.tcp_connect(host, port, timeout)
>> +    local s, err = socket.tcp_connect(host, port, timeout)
>>     if not s then
>> -        return nil, errno.strerror(errno())
>> +        if not err then
>> +            return nil, errno.strerror(errno())
>> +        end
>> +        return nil, err
>>     end
>>     local msg = s:read({chunk = IPROTO_GREETING_SIZE},
>>                         timeout - (fiber.clock() - begin))
>> diff --git a/src/lua/socket.c b/src/lua/socket.c
>> index e75a8802e..ba683ad3a 100644
>> --- a/src/lua/socket.c
>> +++ b/src/lua/socket.c
>> @@ -775,6 +775,18 @@ lbox_getaddrinfo_result_wrapper(struct lua_State *L)
>> 	return 1;
>> }
>> 
>> +/**
>> + * Wrap coio_getaddrinfo() and call it. Push returned values onto
>> + * @a L Lua stack.
>> + *
>> + * @param L Lua stack.
>> + *
>> + * @retval 1 Number of returned values by Lua function if
>> + * coio_getaddrinfo() success.
>> + * @retval 2 Number of returned values by Lua function if
>> + * coio_getaddrinfo() is failed (first is nil, second is error
>> + * message).
>> + */
>> static int
>> lbox_socket_getaddrinfo(struct lua_State *L)
>> {
>> @@ -817,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L)
>> 
>> 	if (dns_res != 0) {
>> 		lua_pushnil(L);
>> -		return 1;
>> +		struct error *err = diag_get()->last;
>> +		lua_pushstring(L, err->errmsg);
>> +		return 2;
>> 	}
>> 
>> 	/* no results */
>> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
>> index a334ad45b..ce6dab301 100644
>> --- a/src/lua/socket.lua
>> +++ b/src/lua/socket.lua
>> @@ -1041,9 +1041,12 @@ local function tcp_connect(host, port, timeout)
>>     end
>>     local timeout = timeout or TIMEOUT_INFINITY
>>     local stop = fiber.clock() + timeout
>> -    local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
>> +    local dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
>>         protocol = 'tcp' })
>> -    if dns == nil or #dns == 0 then
>> +    if dns == nil then
>> +        return nil, err
> You explicitly put error here...
> 
>> +    end
>> +    if #dns == 0 then
>>         boxerrno(boxerrno.EINVAL)
>>         return nil
> ... and not here. Why do you keep using two versions of error passing,
> not one? I would agree you need boxerrno for backward compatibility, but
> to start moving towards {res, err} you have to introduce it everywhere.
@@ -1028,30 +1028,33 @@ local function tcp_connect(host, port, timeout)
         local s = socket_new('AF_UNIX', 'SOCK_STREAM', 0)
         if not s then
             -- Address family is not supported by the host
-            return nil
+            return nil, boxerrno.strerror()
         end
         if not socket_tcp_connect(s, host, port, timeout) then
             local save_errno = s._errno
             s:close()
             boxerrno(save_errno)
-            return nil
+            return nil, boxerrno.strerror()
         end
         boxerrno(0)
         return s
     end
     local timeout = timeout or TIMEOUT_INFINITY
     local stop = fiber.clock() + timeout
-    local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
+    local dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
         protocol = 'tcp' })
-    if dns == nil or #dns == 0 then
+    if dns == nil then
+        return nil, err
+    end
+    if #dns == 0 then
         boxerrno(boxerrno.EINVAL)
-        return nil
+        return nil, boxerrno.strerror()
     end
     for i, remote in pairs(dns) do
         timeout = stop - fiber.clock()
         if timeout <= 0 then
             boxerrno(boxerrno.ETIMEDOUT)
-            return nil
+            return nil, boxerrno.strerror()
         end
         local s = socket_new(remote.family, remote.type, remote.protocol)
         if s then


>>     end
>> @@ -1360,8 +1363,12 @@ local function lsocket_tcp_connect(self, host, port)
>>     -- This function is broken by design
>>     local ga_opts = { family = 'AF_INET', type = 'SOCK_STREAM' }
>>     local timeout = deadline - fiber.clock()
>> -    local dns = getaddrinfo(host, port, timeout, ga_opts)
>> -    if dns == nil or #dns == 0 then
>> +    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
>> +    if dns == nil then
>> +        self._errno = boxerrno.EINVAL
>> +        return nil, err
>> +    end
>> +    if #dns == 0 then
>>         self._errno = boxerrno.EINVAL
>>         return nil, socket_error(self)
>>     end
>> @@ -1547,9 +1554,12 @@ local function lsocket_connect(host, port)
>>     if host == nil or port == nil then
>>         error("Usage: luasocket.connect(host, port)")
>>     end
>> -    local s = tcp_connect(host, port)
>> +    local s, err = tcp_connect(host, port)
>>     if not s then
>> -        return nil, boxerrno.strerror()
>> +        if not err then
>> +            return nil, boxerrno.strerror()
>> +        end
>> +        return nil, err
>>     end
>>     setmetatable(s, lsocket_tcp_client_mt)
>>     return s
>> diff --git a/test/app/socket.result b/test/app/socket.result
>> index 9f0ea0a5d..3f26bf2b0 100644
>> --- a/test/app/socket.result
>> +++ b/test/app/socket.result
>> @@ -941,10 +941,20 @@ sc:close()
>> - true
>> ...
>> -- tcp_connect
>> --- test timeout
>> -socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
>> +-- Test timeout. In this test, tcp_connect can return the second
>> +-- output value from internal.getaddrinfo (usually on Mac OS, but
>> +-- theoretically it can happen on Linux too). Sometimes
>> +-- getaddrinfo() is timed out, sometimes connect. On Linux however
>> +-- getaddrinfo is fast enough to never give timeout error in
>> +-- the case. So, there are two sources of timeout errors that are
>> +-- reported differently. This difference has appeared after
>> +-- gh-4138 patch.
>> +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
>> ---
>> -- null
>> +...
>> +s == nil
>> +---
>> +- true
>> ...
>> -- AF_INET
>> s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
>> @@ -1595,7 +1605,7 @@ fio.stat(path) == nil
>> { socket.tcp_connect('abrakadabra#123') == nil, errno.strerror() }
>> ---
>> - - true
>> -  - Invalid argument
>> +  - Input/output error
>> ...
>> -- wrong options for getaddrinfo
>> socket.getaddrinfo('host', 'port', { type = 'WRONG' }) == nil and errno() == errno.EINVAL
>> @@ -2914,3 +2924,48 @@ srv:close()
>> ---
>> - true
>> ...
>> +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
>> +-- Error code and error message returned by getaddrinfo() depends
>> +-- on system's gai_strerror().
>> +test_run:cmd("setopt delimiter ';'")
>> +---
>> +- true
>> +...
>> +function check_err(err)
>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
>> +       err == 'getaddrinfo: Name or service not known' or
>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
>> +        return true
>> +    end
>> +    return false
>> +end;
>> +---
>> +...
>> +test_run:cmd("setopt delimiter ''");
>> +---
>> +- true
>> +...
>> +s, err = socket.getaddrinfo('non_exists_hostname', 3301)
>> +---
>> +...
>> +check_err(err)
>> +---
>> +- true
>> +...
>> +s, err = socket.connect('non_exists_hostname', 3301)
>> +---
>> +...
>> +check_err(err)
>> +---
>> +- true
>> +...
>> +s, err = socket.tcp_connect('non_exists_hostname', 3301)
>> +---
>> +...
>> +check_err(err)
>> +---
>> +- true
>> +...
>> diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
>> index d1fe7ada6..c455d3ddb 100644
>> --- a/test/app/socket.test.lua
>> +++ b/test/app/socket.test.lua
>> @@ -300,8 +300,16 @@ sc:close()
>> 
>> -- tcp_connect
>> 
>> --- test timeout
>> -socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
>> +-- Test timeout. In this test, tcp_connect can return the second
>> +-- output value from internal.getaddrinfo (usually on Mac OS, but
>> +-- theoretically it can happen on Linux too). Sometimes
>> +-- getaddrinfo() is timed out, sometimes connect. On Linux however
>> +-- getaddrinfo is fast enough to never give timeout error in
>> +-- the case. So, there are two sources of timeout errors that are
>> +-- reported differently. This difference has appeared after
>> +-- gh-4138 patch.
>> +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
>> +s == nil
>> 
>> -- AF_INET
>> s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
>> @@ -988,3 +996,26 @@ s:receive('*a')
>> s:close()
>> srv:close()
>> 
>> +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
>> +-- Error code and error message returned by getaddrinfo() depends
>> +-- on system's gai_strerror().
>> +test_run:cmd("setopt delimiter ';'")
>> +function check_err(err)
>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
>> +       err == 'getaddrinfo: Name or service not known' or
>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
>> +        return true
>> +    end
>> +    return false
>> +end;
> I really doubt that different error messages from the set above will appear 
> for the same error on different platforms. Could you please check for particular 
> output for each case you trigger below?
Look at that:
https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed
https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t


>> +test_run:cmd("setopt delimiter ''");
>> +
>> +s, err = socket.getaddrinfo('non_exists_hostname', 3301)
>> +check_err(err)
>> +s, err = socket.connect('non_exists_hostname', 3301)
>> +check_err(err)
>> +s, err = socket.tcp_connect('non_exists_hostname', 3301)
>> +check_err(err)
>> diff --git a/test/box/net.box.result b/test/box/net.box.result
>> index e3dabf7d9..3e1ce6b11 100644
>> --- a/test/box/net.box.result
>> +++ b/test/box/net.box.result
>> @@ -3927,6 +3927,37 @@ test_run:grep_log('default', '00000040:.*')
>> ---
>> - null
>> ...
>> +-- gh-4138 Check getaddrinfo() error from connect() only. Error
>> +-- code and error message returned by getaddrinfo() depends on
>> +-- system's gai_strerror().
>> +test_run:cmd("setopt delimiter ';'")
>> +---
>> +- true
>> +...
>> +function check_err(err)
>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
>> +       err == 'getaddrinfo: Name or service not known' or
>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
>> +            return true
>> +    end
>> +    return false
>> +end;
>> +---
>> +...
>> +test_run:cmd("setopt delimiter ''");
>> +---
>> +- true
>> +...
>> +s = remote.connect('non_exists_hostname:3301')
>> +---
>> +...
>> +check_err(s['error'])
>> +---
>> +- true
>> +...
>> box.cfg{log_level=log_level}
>> ---
>> ...
>> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
>> index 8e65ff470..cad8754c3 100644
>> --- a/test/box/net.box.test.lua
>> +++ b/test/box/net.box.test.lua
>> @@ -1582,4 +1582,24 @@ test_run:wait_log('default', '00000030:.*', nil, 10)
>> -- we expect nothing below, so don't wait
>> test_run:grep_log('default', '00000040:.*')
>> 
>> +-- gh-4138 Check getaddrinfo() error from connect() only. Error
>> +-- code and error message returned by getaddrinfo() depends on
>> +-- system's gai_strerror().
>> +test_run:cmd("setopt delimiter ';'")
>> +function check_err(err)
>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
>> +       err == 'getaddrinfo: Name or service not known' or
>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
>> +            return true
>> +    end
>> +    return false
>> +end;
>> +test_run:cmd("setopt delimiter ''");
>> +
>> +s = remote.connect('non_exists_hostname:3301')
>> +check_err(s['error'])
>> +
>> box.cfg{log_level=log_level}
>> -- 
>> 2.21.0 (Apple Git-122)
>> 

commit 57d8b556b98a8b6de4d40f6ae1c5f48498022962
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Thu Nov 21 14:37:54 2019 +0300

    lua: return getaddrinfo() errors
    
    Add getaddrinfo() errors into the several fuctions of socket. Now
    getaddrinfo() return a pair of values (nil and error message) in
    case of error.
    
    Closes #4138
    
    @TarantoolBot document
    Title: socket API changes
    
    * socket.getaddrinfo()
    
    Return error message as the second return value.
    
    Example:
    tarantool> socket.getaddrinfo('non_exists_hostname', 3301)
    ---
    - null
    - 'getaddrinfo: nodename nor servname provided, or not known'
    ...
    
    * socket.tcp_connect()
    
    Return error message as the second return value.
    
    Example:
    tarantool> socket.tcp_connect('non_exists_hostname', 3301)
    ---
    - null
    - 'getaddrinfo: nodename nor servname provided, or not known'
    ...
    
    * socket.bind()
    
    Return error message as the second return value.
    
    Example:
    tarantool> socket.tcp_connect('non_exists_hostname', 3301)
    ---
    - null
    - 'getaddrinfo: nodename nor servname provided, or not known'
    ...
    
    * socket.tcp_server()
    
    Return error message as the second return value.
    
    Example:
    tarantool> socket.tcp_connect('non_exists_hostname', 3301)
    ---
    - null
    - 'getaddrinfo: nodename nor servname provided, or not known'
    ...

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 3f611c027..b5d0dff4a 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -154,9 +154,9 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
 local function establish_connection(host, port, timeout)
     local timeout = timeout or DEFAULT_CONNECT_TIMEOUT
     local begin = fiber.clock()
-    local s = socket.tcp_connect(host, port, timeout)
+    local s, err = socket.tcp_connect(host, port, timeout)
     if not s then
-        return nil, errno.strerror(errno())
+        return nil, err
     end
     local msg = s:read({chunk = IPROTO_GREETING_SIZE},
                         timeout - (fiber.clock() - begin))
diff --git a/src/lua/socket.c b/src/lua/socket.c
index e75a8802e..ba683ad3a 100644
--- a/src/lua/socket.c
+++ b/src/lua/socket.c
@@ -775,6 +775,18 @@ lbox_getaddrinfo_result_wrapper(struct lua_State *L)
 	return 1;
 }
 
+/**
+ * Wrap coio_getaddrinfo() and call it. Push returned values onto
+ * @a L Lua stack.
+ *
+ * @param L Lua stack.
+ *
+ * @retval 1 Number of returned values by Lua function if
+ * coio_getaddrinfo() success.
+ * @retval 2 Number of returned values by Lua function if
+ * coio_getaddrinfo() is failed (first is nil, second is error
+ * message).
+ */
 static int
 lbox_socket_getaddrinfo(struct lua_State *L)
 {
@@ -817,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L)
 
 	if (dns_res != 0) {
 		lua_pushnil(L);
-		return 1;
+		struct error *err = diag_get()->last;
+		lua_pushstring(L, err->errmsg);
+		return 2;
 	}
 
 	/* no results */
diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index a334ad45b..57fdc6213 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -963,7 +963,7 @@ local function getaddrinfo(host, port, timeout, opts)
             local itype = get_ivalue(internal.SO_TYPE, opts.type)
             if itype == nil then
                 boxerrno(boxerrno.EINVAL)
-                return nil
+                return nil, boxerrno.strerror()
             end
             ga_opts.type = itype
         end
@@ -972,7 +972,7 @@ local function getaddrinfo(host, port, timeout, opts)
             local ifamily = get_ivalue(internal.DOMAIN, opts.family)
             if ifamily == nil then
                 boxerrno(boxerrno.EINVAL)
-                return nil
+                return nil, boxerrno.strerror()
             end
             ga_opts.family = ifamily
         end
@@ -980,7 +980,7 @@ local function getaddrinfo(host, port, timeout, opts)
         if opts.protocol ~= nil then
             local p = getprotobyname(opts.protocol)
             if p == nil then
-                return nil
+                return nil, boxerrno.strerror()
             end
             ga_opts.protocol = p
         end
@@ -990,7 +990,7 @@ local function getaddrinfo(host, port, timeout, opts)
                 get_iflags(internal.AI_FLAGS, opts.flags)
             if ga_opts.flags == nil then
                 boxerrno(boxerrno.EINVAL)
-                return nil
+                return nil, boxerrno.strerror()
             end
         end
 
@@ -1028,30 +1028,33 @@ local function tcp_connect(host, port, timeout)
         local s = socket_new('AF_UNIX', 'SOCK_STREAM', 0)
         if not s then
             -- Address family is not supported by the host
-            return nil
+            return nil, boxerrno.strerror()
         end
         if not socket_tcp_connect(s, host, port, timeout) then
             local save_errno = s._errno
             s:close()
             boxerrno(save_errno)
-            return nil
+            return nil, boxerrno.strerror()
         end
         boxerrno(0)
         return s
     end
     local timeout = timeout or TIMEOUT_INFINITY
     local stop = fiber.clock() + timeout
-    local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
+    local dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
         protocol = 'tcp' })
-    if dns == nil or #dns == 0 then
+    if dns == nil then
+        return nil, err
+    end
+    if #dns == 0 then
         boxerrno(boxerrno.EINVAL)
-        return nil
+        return nil, boxerrno.strerror()
     end
     for i, remote in pairs(dns) do
         timeout = stop - fiber.clock()
         if timeout <= 0 then
             boxerrno(boxerrno.ETIMEDOUT)
-            return nil
+            return nil, boxerrno.strerror()
         end
         local s = socket_new(remote.family, remote.type, remote.protocol)
         if s then
@@ -1065,7 +1068,7 @@ local function tcp_connect(host, port, timeout)
         end
     end
     -- errno is set by socket_tcp_connect()
-    return nil
+    return nil, boxerrno.strerror()
 end
 
 local function tcp_server_handler(server, sc, from)
@@ -1160,15 +1163,15 @@ end
 
 local function tcp_server_bind(host, port, prepare, timeout)
     timeout = timeout and tonumber(timeout) or TIMEOUT_INFINITY
-    local dns
+    local dns, err
     if host == 'unix/' then
         dns = {{host = host, port = port, family = 'AF_UNIX', protocol = 0,
             type = 'SOCK_STREAM' }}
     else
-        dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
+        dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
             flags = 'AI_PASSIVE'})
         if dns == nil then
-            return nil
+            return nil, err
         end
     end
 
@@ -1185,14 +1188,14 @@ local function tcp_server_bind(host, port, prepare, timeout)
                 local save_errno = boxerrno()
                 socket_close(s)
                 boxerrno(save_errno)
-                return nil
+                return nil, boxerrno.strerror()
             end
             return s, addr
        end
     end
     -- DNS resolved successfully, but addresss family is not supported
     boxerrno(boxerrno.EAFNOSUPPORT)
-    return nil
+    return nil, boxerrno.strerror()
 end
 
 local function tcp_server(host, port, opts, timeout)
@@ -1213,7 +1216,8 @@ local function tcp_server(host, port, opts, timeout)
     server.name = server.name or 'server'
     local s, addr = tcp_server_bind(host, port, server.prepare, timeout)
     if not s then
-        return nil
+        -- addr is error message now.
+        return nil, addr
     end
     fiber.create(tcp_server_loop, server, s, addr)
     return s, addr
@@ -1360,8 +1364,12 @@ local function lsocket_tcp_connect(self, host, port)
     -- This function is broken by design
     local ga_opts = { family = 'AF_INET', type = 'SOCK_STREAM' }
     local timeout = deadline - fiber.clock()
-    local dns = getaddrinfo(host, port, timeout, ga_opts)
-    if dns == nil or #dns == 0 then
+    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
+    if dns == nil then
+        self._errno = boxerrno.EINVAL
+        return nil, err
+    end
+    if #dns == 0 then
         self._errno = boxerrno.EINVAL
         return nil, socket_error(self)
     end
@@ -1547,9 +1555,9 @@ local function lsocket_connect(host, port)
     if host == nil or port == nil then
         error("Usage: luasocket.connect(host, port)")
     end
-    local s = tcp_connect(host, port)
+    local s, err = tcp_connect(host, port)
     if not s then
-        return nil, boxerrno.strerror()
+        return nil, err
     end
     setmetatable(s, lsocket_tcp_client_mt)
     return s
@@ -1560,9 +1568,9 @@ local function lsocket_bind(host, port, backlog)
         error("Usage: luasocket.bind(host, port [, backlog])")
     end
     local function prepare(s) return backlog end
-    local s = tcp_server_bind(host, port, prepare)
+    local s, err = tcp_server_bind(host, port, prepare)
     if not s then
-        return nil, boxerrno.strerror()
+        return nil, err
     end
     return setmetatable(s, lsocket_tcp_server_mt)
 end
diff --git a/test/app/socket.result b/test/app/socket.result
index 9f0ea0a5d..6f91b32ee 100644
--- a/test/app/socket.result
+++ b/test/app/socket.result
@@ -941,10 +941,20 @@ sc:close()
 - true
 ...
 -- tcp_connect
--- test timeout
-socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
+-- Test timeout. In this test, tcp_connect can return the second
+-- output value from internal.getaddrinfo (usually on Mac OS, but
+-- theoretically it can happen on Linux too). Sometimes
+-- getaddrinfo() is timed out, sometimes connect. On Linux however
+-- getaddrinfo is fast enough to never give timeout error in
+-- the case. So, there are two sources of timeout errors that are
+-- reported differently. This difference has appeared after
+-- gh-4138 patch.
+s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
 ---
-- null
+...
+s == nil
+---
+- true
 ...
 -- AF_INET
 s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
@@ -1595,7 +1605,7 @@ fio.stat(path) == nil
 { socket.tcp_connect('abrakadabra#123') == nil, errno.strerror() }
 ---
 - - true
-  - Invalid argument
+  - Input/output error
 ...
 -- wrong options for getaddrinfo
 socket.getaddrinfo('host', 'port', { type = 'WRONG' }) == nil and errno() == errno.EINVAL
@@ -2914,3 +2924,68 @@ srv:close()
 ---
 - true
 ...
+-- gh-4138 Check getaddrinfo() error from socket:connect() only.
+-- Error code and error message returned by getaddrinfo() depends
+-- on system's gai_strerror().
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+-- EAI_NONAME
+-- EAI_SERVICE
+-- EAI_NONAME
+-- EAI_NONAME
+-- EAI_AGAIN
+-- EAI_AGAIN
+function check_err(err)
+    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
+       err == 'getaddrinfo: Servname not supported for ai_socktype' or
+       err == 'getaddrinfo: Name or service not known' or
+       err == 'getaddrinfo: hostname nor servname provided, or not known' or
+       err == 'getaddrinfo: Temporary failure in name resolution' or
+       err == 'getaddrinfo: Name could not be resolved at this time' then
+        return true
+    end
+    return false
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+s, err = socket.getaddrinfo('non_exists_hostname', 3301)
+---
+...
+check_err(err)
+---
+- true
+...
+s, err = socket.connect('non_exists_hostname', 3301)
+---
+...
+check_err(err)
+---
+- true
+...
+s, err = socket.tcp_connect('non_exists_hostname', 3301)
+---
+...
+check_err(err)
+---
+- true
+...
+s, err = socket.bind('non_exists_hostname', 3301)
+---
+...
+check_err(err)
+---
+- true
+...
+s, err = socket.tcp_server('non_exists_hostname', 3301, function() end)
+---
+...
+check_err(err)
+---
+- true
+...
diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
index d1fe7ada6..9f5b3fea0 100644
--- a/test/app/socket.test.lua
+++ b/test/app/socket.test.lua
@@ -300,8 +300,16 @@ sc:close()
 
 -- tcp_connect
 
--- test timeout
-socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
+-- Test timeout. In this test, tcp_connect can return the second
+-- output value from internal.getaddrinfo (usually on Mac OS, but
+-- theoretically it can happen on Linux too). Sometimes
+-- getaddrinfo() is timed out, sometimes connect. On Linux however
+-- getaddrinfo is fast enough to never give timeout error in
+-- the case. So, there are two sources of timeout errors that are
+-- reported differently. This difference has appeared after
+-- gh-4138 patch.
+s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
+s == nil
 
 -- AF_INET
 s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
@@ -988,3 +996,36 @@ s:receive('*a')
 s:close()
 srv:close()
 
+-- gh-4138 Check getaddrinfo() error from socket:connect() only.
+-- Error code and error message returned by getaddrinfo() depends
+-- on system's gai_strerror().
+test_run:cmd("setopt delimiter ';'")
+function check_err(err)
+-- EAI_NONAME
+    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
+-- EAI_SERVICE
+       err == 'getaddrinfo: Servname not supported for ai_socktype' or
+-- EAI_NONAME
+       err == 'getaddrinfo: Name or service not known' or
+-- EAI_NONAME
+       err == 'getaddrinfo: hostname nor servname provided, or not known' or
+-- EAI_AGAIN
+       err == 'getaddrinfo: Temporary failure in name resolution' or
+-- EAI_AGAIN
+       err == 'getaddrinfo: Name could not be resolved at this time' then
+        return true
+    end
+    return false
+end;
+test_run:cmd("setopt delimiter ''");
+
+s, err = socket.getaddrinfo('non_exists_hostname', 3301)
+check_err(err)
+s, err = socket.connect('non_exists_hostname', 3301)
+check_err(err)
+s, err = socket.tcp_connect('non_exists_hostname', 3301)
+check_err(err)
+s, err = socket.bind('non_exists_hostname', 3301)
+check_err(err)
+s, err = socket.tcp_server('non_exists_hostname', 3301, function() end)
+check_err(err)
diff --git a/test/box/net.box.result b/test/box/net.box.result
index e3dabf7d9..c5b32123b 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3927,6 +3927,43 @@ test_run:grep_log('default', '00000040:.*')
 ---
 - null
 ...
+-- gh-4138 Check getaddrinfo() error from connect() only. Error
+-- code and error message returned by getaddrinfo() depends on
+-- system's gai_strerror().
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+-- EAI_NONAME
+-- EAI_SERVICE
+-- EAI_NONAME
+-- EAI_NONAME
+-- EAI_AGAIN
+-- EAI_AGAIN
+function check_err(err)
+    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
+       err == 'getaddrinfo: Servname not supported for ai_socktype' or
+       err == 'getaddrinfo: Name or service not known' or
+       err == 'getaddrinfo: hostname nor servname provided, or not known' or
+       err == 'getaddrinfo: Temporary failure in name resolution' or
+       err == 'getaddrinfo: Name could not be resolved at this time' then
+            return true
+    end
+    return false
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+s = remote.connect('non_exists_hostname:3301')
+---
+...
+check_err(s['error'])
+---
+- true
+...
 box.cfg{log_level=log_level}
 ---
 ...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 8e65ff470..24b996be4 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1582,4 +1582,30 @@ test_run:wait_log('default', '00000030:.*', nil, 10)
 -- we expect nothing below, so don't wait
 test_run:grep_log('default', '00000040:.*')
 
+-- gh-4138 Check getaddrinfo() error from connect() only. Error
+-- code and error message returned by getaddrinfo() depends on
+-- system's gai_strerror().
+test_run:cmd("setopt delimiter ';'")
+function check_err(err)
+-- EAI_NONAME
+    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
+-- EAI_SERVICE
+       err == 'getaddrinfo: Servname not supported for ai_socktype' or
+-- EAI_NONAME
+       err == 'getaddrinfo: Name or service not known' or
+-- EAI_NONAME
+       err == 'getaddrinfo: hostname nor servname provided, or not known' or
+-- EAI_AGAIN
+       err == 'getaddrinfo: Temporary failure in name resolution' or
+-- EAI_AGAIN
+       err == 'getaddrinfo: Name could not be resolved at this time' then
+            return true
+    end
+    return false
+end;
+test_run:cmd("setopt delimiter ''");
+
+s = remote.connect('non_exists_hostname:3301')
+check_err(s['error'])
+
 box.cfg{log_level=log_level}

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Sergey Ostanevich]

On 06 апр 05:08, Roman Khabibov wrote:
> >> +test_run:cmd("setopt delimiter ';'")
> >> +function check_err(err)
> >> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> >> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> >> +       err == 'getaddrinfo: Name or service not known' or
> >> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> >> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> >> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> >> +        return true
> >> +    end
> >> +    return false
> >> +end;
> > I really doubt that different error messages from the set above will appear 
> > for the same error on different platforms. Could you please check for particular 
> > output for each case you trigger below?
> Look at that:
> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed
> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t
> 

Exactly, this means for the error EAI_NONAME those OSes have differnet
messages. But you've put different errors in one bunch - I meant it
should never return 'Temporary failure in name resolution' as a result
for EAI_SERVICE, right? 

Are you testing for correct error returned for a particular case?

Otherwise looks good.

Sergos

Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Sergey Ostanevich]

> >> 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 <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 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 ***
> 
> 

Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Roman Khabibov]

Hi! Yes.

> On Apr 17, 2020, at 12:37, Sergey Ostanevich <sergos@tarantool.org> 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 <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 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 ***
>> 
>> 

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Roman Khabibov]

Hi! Thanks for the review.

> On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote:
> 
> On 06 апр 05:08, Roman Khabibov wrote:
>>>> +test_run:cmd("setopt delimiter ';'")
>>>> +function check_err(err)
>>>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
>>>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
>>>> +       err == 'getaddrinfo: Name or service not known' or
>>>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
>>>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
>>>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
>>>> +        return true
>>>> +    end
>>>> +    return false
>>>> +end;
>>> I really doubt that different error messages from the set above will appear 
>>> for the same error on different platforms. Could you please check for particular 
>>> output for each case you trigger below?
>> Look at that:
>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed
>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t
>> 
> 
> Exactly, this means for the error EAI_NONAME those OSes have differnet
> messages. But you've put different errors in one bunch - I meant it
> should never return 'Temporary failure in name resolution' as a result
> for EAI_SERVICE, right? 
> 
> Are you testing for correct error returned for a particular case?
Don’t understand the question. I just check error messages that appeared
after travis/gitlab testing.

> Otherwise looks good.
> 
> Sergos

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Sergey Ostanevich]

On 24 апр 14:42, Roman Khabibov wrote:
> Hi! Thanks for the review.
> 
> > On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote:
> > 
> > On 06 апр 05:08, Roman Khabibov wrote:
> >>>> +test_run:cmd("setopt delimiter ';'")
> >>>> +function check_err(err)
> >>>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> >>>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> >>>> +       err == 'getaddrinfo: Name or service not known' or
> >>>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> >>>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> >>>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> >>>> +        return true
> >>>> +    end
> >>>> +    return false
> >>>> +end;
> >>> I really doubt that different error messages from the set above will appear 
> >>> for the same error on different platforms. Could you please check for particular 
> >>> output for each case you trigger below?
> >> Look at that:
> >> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed
> >> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t
> >> 
> > 
> > Exactly, this means for the error EAI_NONAME those OSes have differnet
> > messages. But you've put different errors in one bunch - I meant it
> > should never return 'Temporary failure in name resolution' as a result
> > for EAI_SERVICE, right? 
> > 
> > Are you testing for correct error returned for a particular case?
> Don’t understand the question. I just check error messages that appeared
> after travis/gitlab testing.
> 
Different errors makes different messages. 
Same error can make different messages on different platforms. 

You put both into one: you don't care about what exact error came to
you, right? 

Is it something expected from this patch at all - to differentiate the
errors?

> > Otherwise looks good.
> > 
> > Sergos
> 

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Roman Khabibov]

> On Apr 24, 2020, at 20:18, Sergey Ostanevich <sergos@tarantool.org> wrote:
> 
> On 24 апр 14:42, Roman Khabibov wrote:
>> Hi! Thanks for the review.
>> 
>>> On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote:
>>> 
>>> On 06 апр 05:08, Roman Khabibov wrote:
>>>>>> +test_run:cmd("setopt delimiter ';'")
>>>>>> +function check_err(err)
>>>>>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
>>>>>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
>>>>>> +       err == 'getaddrinfo: Name or service not known' or
>>>>>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
>>>>>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
>>>>>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
>>>>>> +        return true
>>>>>> +    end
>>>>>> +    return false
>>>>>> +end;
>>>>> I really doubt that different error messages from the set above will appear 
>>>>> for the same error on different platforms. Could you please check for particular 
>>>>> output for each case you trigger below?
>>>> Look at that:
>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed
>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t
>>>> 
>>> 
>>> Exactly, this means for the error EAI_NONAME those OSes have differnet
>>> messages. But you've put different errors in one bunch - I meant it
>>> should never return 'Temporary failure in name resolution' as a result
>>> for EAI_SERVICE, right? 
>>> 
>>> Are you testing for correct error returned for a particular case?
>> Don’t understand the question. I just check error messages that appeared
>> after travis/gitlab testing.
>> 
> Different errors makes different messages. 
> Same error can make different messages on different platforms. 
> 
> You put both into one: you don't care about what exact error came to
> you, right? 
Right. The most important thing here is that the error is from getaddrinfo.

> Is it something expected from this patch at all - to differentiate the
> errors?
The idea of this patch is to throw a internal getaddrinfo error instead
of the obscure common  I/O error.

>>> Otherwise looks good.
>>> 
>>> Sergos

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Alexander Turenko]

On Sun, Apr 26, 2020 at 06:16:29AM +0300, Roman Khabibov wrote:
> 
> 
> > On Apr 24, 2020, at 20:18, Sergey Ostanevich <sergos@tarantool.org> wrote:
> > 
> > On 24 апр 14:42, Roman Khabibov wrote:
> >> Hi! Thanks for the review.
> >> 
> >>> On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote:
> >>> 
> >>> On 06 апр 05:08, Roman Khabibov wrote:
> >>>>>> +test_run:cmd("setopt delimiter ';'")
> >>>>>> +function check_err(err)
> >>>>>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> >>>>>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> >>>>>> +       err == 'getaddrinfo: Name or service not known' or
> >>>>>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> >>>>>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> >>>>>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> >>>>>> +        return true
> >>>>>> +    end
> >>>>>> +    return false
> >>>>>> +end;
> >>>>> I really doubt that different error messages from the set above will appear 
> >>>>> for the same error on different platforms. Could you please check for particular 
> >>>>> output for each case you trigger below?
> >>>> Look at that:
> >>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed
> >>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t
> >>>> 
> >>> 
> >>> Exactly, this means for the error EAI_NONAME those OSes have differnet
> >>> messages. But you've put different errors in one bunch - I meant it
> >>> should never return 'Temporary failure in name resolution' as a result
> >>> for EAI_SERVICE, right? 
> >>> 
> >>> Are you testing for correct error returned for a particular case?
> >> Don’t understand the question. I just check error messages that appeared
> >> after travis/gitlab testing.
> >> 
> > Different errors makes different messages. 
> > Same error can make different messages on different platforms. 
> > 
> > You put both into one: you don't care about what exact error came to
> > you, right? 
> Right. The most important thing here is that the error is from getaddrinfo.

Here I disagree. We want to verify that an error from a specific set of
errors occurs. AFAIR, three errors are possible when trying to resolve a
non-existent hostname: EAI_NONAME, EAI_AGAIN (and, as I see from the
messages, EAI_SERVICE; maybe it is when ipv6 is semi-configured like on
travis-ci).

Actual EAI_* error for the test depends on network conditions and host
configuration, messages are platform dependent.

See also:
https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/003399.html

Roman, I suggest to clarify things that Sergos asked in the comments
near to the check: if it is not clear for a reviewer, then a future
reader may hang with it too.

And, please (AFAIR I already asked), comment which message is for which
EAI_* constant.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Roman Khabibov]

> On Apr 26, 2020, at 18:46, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> On Sun, Apr 26, 2020 at 06:16:29AM +0300, Roman Khabibov wrote:
>> 
>> 
>>> On Apr 24, 2020, at 20:18, Sergey Ostanevich <sergos@tarantool.org> wrote:
>>> 
>>> On 24 апр 14:42, Roman Khabibov wrote:
>>>> Hi! Thanks for the review.
>>>> 
>>>>> On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote:
>>>>> 
>>>>> On 06 апр 05:08, Roman Khabibov wrote:
>>>>>>>> +test_run:cmd("setopt delimiter ';'")
>>>>>>>> +function check_err(err)
>>>>>>>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
>>>>>>>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
>>>>>>>> +       err == 'getaddrinfo: Name or service not known' or
>>>>>>>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
>>>>>>>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
>>>>>>>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
>>>>>>>> +        return true
>>>>>>>> +    end
>>>>>>>> +    return false
>>>>>>>> +end;
>>>>>>> I really doubt that different error messages from the set above will appear 
>>>>>>> for the same error on different platforms. Could you please check for particular 
>>>>>>> output for each case you trigger below?
>>>>>> Look at that:
>>>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed
>>>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t
>>>>>> 
>>>>> 
>>>>> Exactly, this means for the error EAI_NONAME those OSes have differnet
>>>>> messages. But you've put different errors in one bunch - I meant it
>>>>> should never return 'Temporary failure in name resolution' as a result
>>>>> for EAI_SERVICE, right? 
>>>>> 
>>>>> Are you testing for correct error returned for a particular case?
>>>> Don’t understand the question. I just check error messages that appeared
>>>> after travis/gitlab testing.
>>>> 
>>> Different errors makes different messages. 
>>> Same error can make different messages on different platforms. 
>>> 
>>> You put both into one: you don't care about what exact error came to
>>> you, right? 
>> Right. The most important thing here is that the error is from getaddrinfo.
> 
> Here I disagree. We want to verify that an error from a specific set of
> errors occurs. AFAIR, three errors are possible when trying to resolve a
> non-existent hostname: EAI_NONAME, EAI_AGAIN (and, as I see from the
> messages, EAI_SERVICE; maybe it is when ipv6 is semi-configured like on
> travis-ci).
> 
> Actual EAI_* error for the test depends on network conditions and host
> configuration, messages are platform dependent.
> 
> See also:
> https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/003399.html
> 
> Roman, I suggest to clarify things that Sergos asked in the comments
> near to the check: if it is not clear for a reviewer, then a future
> reader may hang with it too.
Ok, Sergos, can you just show me what is wrong in this code. Sorry, but I
can’t figure out. There is the set of error messages, and there is the
corresponding constants. If several error messages correspond to the same
constant, then these messages are for different OS.

+-- gh-4138 Check getaddrinfo() error from socket:connect() only.
+-- Error code and error message returned by getaddrinfo() depends
+-- on system's gai_strerror().
+test_run:cmd("setopt delimiter ';'")
+function check_err(err)
+-- EAI_NONAME
+    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
+-- EAI_SERVICE
+       err == 'getaddrinfo: Servname not supported for ai_socktype' or
+-- EAI_NONAME
+       err == 'getaddrinfo: Name or service not known' or
+-- EAI_NONAME
+       err == 'getaddrinfo: hostname nor servname provided, or not known' or
+-- EAI_AGAIN
+       err == 'getaddrinfo: Temporary failure in name resolution' or
+-- EAI_AGAIN
+       err == 'getaddrinfo: Name could not be resolved at this time' then
+        return true
+    end
+    return false
+end;

> And, please (AFAIR I already asked), comment which message is for which
> EAI_* constant.
> 
> WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Vladislav Shpilevoy]

Hi! I didn't review the patch. Just noticed, that you need the
stacked diag in one place, and I have a comment on that.

> 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
> @@ -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();

Stacked diagnostics is implemented now.

Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Roman Khabibov]

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 <sergos@tarantool.org> 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 <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 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 <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 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 ***

Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[sergos]

Hi!

Sorry for dropping out. 

My initial concern was that we put a bunch of errors, such as EAI_NONAME and 
another bunch of messages, which can appear differently depending on OS or 
network conditions, into one bowl and mix them up.

I believe there should be a better approach, such as for each error a particular 
set of messages can appear. 

Now I tend to understand your statement as ‘an messages can appear for each
error’. Is it right? 

Regards,
Sergos


> On 26 Apr 2020, at 19:26, Roman Khabibov <roman.habibov@tarantool.org> wrote:
> 
> 
> 
>> On Apr 26, 2020, at 18:46, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
>> 
>> On Sun, Apr 26, 2020 at 06:16:29AM +0300, Roman Khabibov wrote:
>>> 
>>> 
>>>> On Apr 24, 2020, at 20:18, Sergey Ostanevich <sergos@tarantool.org> wrote:
>>>> 
>>>> On 24 апр 14:42, Roman Khabibov wrote:
>>>>> Hi! Thanks for the review.
>>>>> 
>>>>>> On Apr 16, 2020, at 13:27, Sergey Ostanevich <sergos@tarantool.org> wrote:
>>>>>> 
>>>>>> On 06 апр 05:08, Roman Khabibov wrote:
>>>>>>>>> +test_run:cmd("setopt delimiter ';'")
>>>>>>>>> +function check_err(err)
>>>>>>>>> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
>>>>>>>>> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
>>>>>>>>> +       err == 'getaddrinfo: Name or service not known' or
>>>>>>>>> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
>>>>>>>>> +       err == 'getaddrinfo: Temporary failure in name resolution' or
>>>>>>>>> +       err == 'getaddrinfo: Name could not be resolved at this time' then
>>>>>>>>> +        return true
>>>>>>>>> +    end
>>>>>>>>> +    return false
>>>>>>>>> +end;
>>>>>>>> I really doubt that different error messages from the set above will appear 
>>>>>>>> for the same error on different platforms. Could you please check for particular 
>>>>>>>> output for each case you trigger below?
>>>>>>> Look at that:
>>>>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115892#L3581 - Linux failed
>>>>>>> https://travis-ci.org/github/tarantool/tarantool/jobs/546115893#L3100 - macOS isn’t
>>>>>>> 
>>>>>> 
>>>>>> Exactly, this means for the error EAI_NONAME those OSes have differnet
>>>>>> messages. But you've put different errors in one bunch - I meant it
>>>>>> should never return 'Temporary failure in name resolution' as a result
>>>>>> for EAI_SERVICE, right? 
>>>>>> 
>>>>>> Are you testing for correct error returned for a particular case?
>>>>> Don’t understand the question. I just check error messages that appeared
>>>>> after travis/gitlab testing.
>>>>> 
>>>> Different errors makes different messages. 
>>>> Same error can make different messages on different platforms. 
>>>> 
>>>> You put both into one: you don't care about what exact error came to
>>>> you, right? 
>>> Right. The most important thing here is that the error is from getaddrinfo.
>> 
>> Here I disagree. We want to verify that an error from a specific set of
>> errors occurs. AFAIR, three errors are possible when trying to resolve a
>> non-existent hostname: EAI_NONAME, EAI_AGAIN (and, as I see from the
>> messages, EAI_SERVICE; maybe it is when ipv6 is semi-configured like on
>> travis-ci).
>> 
>> Actual EAI_* error for the test depends on network conditions and host
>> configuration, messages are platform dependent.
>> 
>> See also:
>> https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/003399.html
>> 
>> Roman, I suggest to clarify things that Sergos asked in the comments
>> near to the check: if it is not clear for a reviewer, then a future
>> reader may hang with it too.
> Ok, Sergos, can you just show me what is wrong in this code. Sorry, but I
> can’t figure out. There is the set of error messages, and there is the
> corresponding constants. If several error messages correspond to the same
> constant, then these messages are for different OS.
> 
> +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
> +-- Error code and error message returned by getaddrinfo() depends
> +-- on system's gai_strerror().
> +test_run:cmd("setopt delimiter ';'")
> +function check_err(err)
> +-- EAI_NONAME
> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> +-- EAI_SERVICE
> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> +-- EAI_NONAME
> +       err == 'getaddrinfo: Name or service not known' or
> +-- EAI_NONAME
> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> +-- EAI_AGAIN
> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> +-- EAI_AGAIN
> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> +        return true
> +    end
> +    return false
> +end;
> 
>> And, please (AFAIR I already asked), comment which message is for which
>> EAI_* constant.
>> 
>> WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Sergey Ostanevich]

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 <sergos@tarantool.org> 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 <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 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 <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 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 ***
> 

Re: [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS

[Roman Khabibov]

Hi! Kirill, you can push.

> On Jul 23, 2020, at 17:12, Sergey Ostanevich <sergos@tarantool.org> wrote:
> 
> 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 <sergos@tarantool.org> 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 <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 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 <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 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 ***
>> 

Re: [Tarantool-patches] [PATCH v2 0/2] Return getaddrinfo() errors

[Kirill Yukhin]

Hello,

On 12 мар 13:24, Roman Khabibov wrote:
> Issue: https://github.com/tarantool/tarantool/issues/4138
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4138-getaddrinfo-full-ci
> 
> Roman Khabibov (2):
>   coio/say: fix getaddrinfo error handling on macOS
>   lua: return getaddrinfo() errors

I've checked your patchset into 2.5 and master.

--
Regards, Kirill Yukhin