Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH v2 0/2] take getaddrinfo() errors  into account
@ 2019-06-25 13:38 Roman Khabibov
  2019-06-25 13:38 ` [tarantool-patches] [PATCH v2 2/2] say: " Roman Khabibov
  0 siblings, 1 reply; 19+ messages in thread
From: Roman Khabibov @ 2019-06-25 13:38 UTC (permalink / raw)
  To: tarantool-patches; +Cc: alexander.turenko

Before this patch, branch when getaddrinfo() returns error codes
couldn't be reached on Mac OS, because they are greater than 0 on
Mac OS (checking for errors was rc < 0).

Roman Khabibov (2):
  lua: return getaddrinfo() errors
  say: take getaddrinfo() errors  into account

 src/box/lua/net_box.lua   |  7 +++++--
 src/lib/core/coio_task.c  |  2 +-
 src/lib/core/say.c        |  3 ++-
 src/lua/socket.c          | 17 ++++++++++++++++-
 src/lua/socket.lua        | 37 +++++++++++++++++++++++--------------
 test/app/socket.result    | 21 ++++++++++++++++++++-
 test/app/socket.test.lua  | 10 ++++++++++
 test/box-tap/cfg.test.lua | 19 ++++++++++++++++++-
 test/box/net.box.result   | 11 +++++++++++
 test/box/net.box.test.lua |  8 ++++++++
 test/unit/coio.cc         |  7 ++++++-
 test/unit/coio.result     |  4 +++-
 12 files changed, 123 insertions(+), 23 deletions(-)

-- 
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4138-getaddrinfo
Issue: https://github.com/tarantool/tarantool/issues/4138
2.20.1 (Apple Git-117)

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors  into account
  2019-06-25 13:38 [tarantool-patches] [PATCH v2 0/2] take getaddrinfo() errors into account Roman Khabibov
@ 2019-06-25 13:38 ` Roman Khabibov
  2019-07-23 14:52   ` [tarantool-patches] " Alexander Turenko
  0 siblings, 1 reply; 19+ messages in thread
From: Roman Khabibov @ 2019-06-25 13:38 UTC (permalink / raw)
  To: tarantool-patches; +Cc: alexander.turenko

Before this patch, branch when getaddrinfo() returns error codes
couldn't be reached on Mac OS, because they are greater than 0 on
Mac OS (checking for errors was rc < 0).

Part of #4138
---
 src/lib/core/say.c        |  3 ++-
 test/box-tap/cfg.test.lua | 19 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index 0b2cf2c34..c927f9fc0 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -506,7 +506,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));
@@ -594,6 +594,7 @@ log_syslog_init(struct log *log, const char *init_str)
 	log->fd = log_syslog_connect(log);
 	if (log->fd < 0) {
 		/* syslog indent is freed in atexit(). */
+		diag_log();
 		diag_set(SystemError, "syslog logger: %s", strerror(errno));
 		return -1;
 	}
diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index 55de5e41c..d92e9140d 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -6,7 +6,7 @@ local socket = require('socket')
 local fio = require('fio')
 local uuid = require('uuid')
 local msgpack = require('msgpack')
-test:plan(104)
+test:plan(105)
 
 --------------------------------------------------------------------------------
 -- Invalid values
@@ -566,6 +566,23 @@ os.exit(0)
 ]]
 test:is(run_script(code), 0, "log_nonblock")
 
+--
+-- gh-4138: check getaddrinfo() error and panic after that.
+--
+code=[[
+local socket = require('socket')
+local log = require('log')
+local fio = require('fio')
+
+path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock')
+unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0)
+unix_socket:bind('unix/', path)
+
+opt = string.format("syslog:server=non_exists_hostname:%s,identity=tarantool", path)
+box.cfg{log = opt, log_nonblock=true}
+]]
+test:is(run_script(code), PANIC, "log_nonblock")
+
 --
 -- Crash (instead of panic) when trying to recover a huge tuple.
 --
-- 
2.20.1 (Apple Git-117)

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [tarantool-patches] Re: [PATCH v2 2/2] say: take getaddrinfo() errors  into account
  2019-06-25 13:38 ` [tarantool-patches] [PATCH v2 2/2] say: " Roman Khabibov
@ 2019-07-23 14:52   ` Alexander Turenko
  2019-08-05 13:32     ` Roman Khabibov
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Turenko @ 2019-07-23 14:52 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

> @@ -594,6 +594,7 @@ log_syslog_init(struct log *log, const char *init_str)
>  	log->fd = log_syslog_connect(log);
>  	if (log->fd < 0) {
>  		/* syslog indent is freed in atexit(). */
> +		diag_log();

1. It is need to be properly commented: we need to log a diagnostics
here until stacked diagnostics will be implemented (#1148).

2. syslog_connect_unix() does not set a diag, diag_log() will lead to an
assertion fail.

3. I would mention this change in the commit message, because it is not
part of the problem with Mac OS you described in it.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [tarantool-patches] Re: [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-07-23 14:52   ` [tarantool-patches] " Alexander Turenko
@ 2019-08-05 13:32     ` Roman Khabibov
  2019-08-28 21:34       ` Alexander Turenko
  0 siblings, 1 reply; 19+ messages in thread
From: Roman Khabibov @ 2019-08-05 13:32 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Alexander Turenko



> On Jul 23, 2019, at 17:52, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
>> @@ -594,6 +594,7 @@ log_syslog_init(struct log *log, const char *init_str)
>> 	log->fd = log_syslog_connect(log);
>> 	if (log->fd < 0) {
>> 		/* syslog indent is freed in atexit(). */
>> +		diag_log();
> 
> 1. It is need to be properly commented: we need to log a diagnostics
> here until stacked diagnostics will be implemented (#1148).
> 
> 2. syslog_connect_unix() does not set a diag, diag_log() will lead to an
> assertion fail.
> 
> 3. I would mention this change in the commit message, because it is not
> part of the problem with Mac OS you described in it.
@@ -506,10 +506,15 @@ 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));
+		/*
+		 * We need to log a diagnostics here until stacked
+		 * diagnostics will be implemented (#1148).
+		 */
+		diag_log();
 		goto out;
 	}


commit f5b19e933fbf2eb3784a0c3cc28d999a3fa85abe
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Tue Jul 30 15:39:21 2019 +0300

    coio/say: take getaddrinfo() errors into account
    
    Before this patch, branch when getaddrinfo() returns error codes
    couldn't be reached on Mac OS, because they are greater than 0 on
    Mac OS (assumption "rc < 0" in commit ea1da04 is incorrect for
    Mac OS).
    
    * diag log() in say.c was added, because we need to log a
    diagnostics here until stacked diagnostics will be implemented
    (#1148).
    
    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 0b2cf2c34..a45595443 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -506,10 +506,15 @@ 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));
+		/*
+		 * We need to log a diagnostics here until stacked
+		 * diagnostics will be implemented (#1148).
+		 */
+		diag_log();
 		goto out;
 	}
 	for (ptr = inf; ptr; ptr = ptr->ai_next) {
diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index 55de5e41c..d92e9140d 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -6,7 +6,7 @@ local socket = require('socket')
 local fio = require('fio')
 local uuid = require('uuid')
 local msgpack = require('msgpack')
-test:plan(104)
+test:plan(105)
 
 --------------------------------------------------------------------------------
 -- Invalid values
@@ -566,6 +566,23 @@ os.exit(0)
 ]]
 test:is(run_script(code), 0, "log_nonblock")
 
+--
+-- gh-4138: check getaddrinfo() error and panic after that.
+--
+code=[[
+local socket = require('socket')
+local log = require('log')
+local fio = require('fio')
+
+path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock')
+unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0)
+unix_socket:bind('unix/', path)
+
+opt = string.format("syslog:server=non_exists_hostname:%s,identity=tarantool", path)
+box.cfg{log = opt, log_nonblock=true}
+]]
+test:is(run_script(code), PANIC, "log_nonblock")
+
 --
 -- Crash (instead of panic) when trying to recover a huge tuple.
 --
diff --git a/test/unit/coio.cc b/test/unit/coio.cc
index bb8bd7131..a70d3254d 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,12 @@ test_getaddrinfo(void)
 	is(rc, 0, "getaddrinfo");
 	freeaddrinfo(i);
 
+	/* gh-4138: Check getaddrinfo() error. */
+	isnt(coio_getaddrinfo("non_exists_hostname", port, NULL, &i, 1), 0,
+	     "getaddrinfo error");
+	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
+	     "getaddrinfo error message");
+
 	/*
 	 * gh-4209: 0 timeout should not be a special value and
 	 * detach a task. Before a fix it led to segfault
diff --git a/test/unit/coio.result b/test/unit/coio.result
index 5019fa48a..49759b747 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 error
+ok 3 - getaddrinfo error message
 	*** test_getaddrinfo: done ***

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [tarantool-patches] Re: [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-08-05 13:32     ` Roman Khabibov
@ 2019-08-28 21:34       ` Alexander Turenko
  2019-08-29  0:51         ` Alexander Turenko
       [not found]         ` <8E98F721-601F-436D-8F0A-5E399D8F7CAB@tarantool.org>
  0 siblings, 2 replies; 19+ messages in thread
From: Alexander Turenko @ 2019-08-28 21:34 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

This particular patch is mostly okay, but I would work a bit more on
tests and minor details.

Please, consider comments below.

WBR, Alexander Turenko.

On Mon, Aug 05, 2019 at 04:32:41PM +0300, Roman Khabibov wrote:
> 
> 
> > On Jul 23, 2019, at 17:52, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> > 
> >> @@ -594,6 +594,7 @@ log_syslog_init(struct log *log, const char *init_str)
> >> 	log->fd = log_syslog_connect(log);
> >> 	if (log->fd < 0) {
> >> 		/* syslog indent is freed in atexit(). */
> >> +		diag_log();
> > 
> > 1. It is need to be properly commented: we need to log a diagnostics
> > here until stacked diagnostics will be implemented (#1148).
> > 
> > 2. syslog_connect_unix() does not set a diag, diag_log() will lead to an
> > assertion fail.
> > 
> > 3. I would mention this change in the commit message, because it is not
> > part of the problem with Mac OS you described in it.
> @@ -506,10 +506,15 @@ 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));
> +		/*
> +		 * We need to log a diagnostics here until stacked
> +		 * diagnostics will be implemented (#1148).
> +		 */
> +		diag_log();

It is not the only error that is possible in this function, but others
will not be logged. I think the logging should be added before replacing
the diagnostic with the next one:

 | --- a/src/lib/core/say.c
 | +++ b/src/lib/core/say.c
 | @@ -593,6 +593,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) {
 | +			/* XXX: comment. */
 | +			diag_log();
 | 				/* syslog indent is freed in atexit(). */
 | 				diag_set(SystemError, "syslog logger: %s", strerror(errno));
 | 				return -1;

> commit f5b19e933fbf2eb3784a0c3cc28d999a3fa85abe
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Tue Jul 30 15:39:21 2019 +0300
> 
>     coio/say: take getaddrinfo() errors into account

I would say that this fix is for Mac OS in the commit header if
possible. Say, 'coio/say: fix getaddrinfo error handling on Mac OS'.

>     
>     Before this patch, branch when getaddrinfo() returns error codes
>     couldn't be reached on Mac OS, because they are greater than 0 on
>     Mac OS (assumption "rc < 0" in commit ea1da04 is incorrect for
>     Mac OS).
>     
>     * diag log() in say.c was added, because we need to log a
>     diagnostics here until stacked diagnostics will be implemented
>     (#1148).

I would say, because otherwise it will be hid by the following
diagnostic and then say that it should be handler in a better way after
#1148.

Asterisk here is redundant, because it is not a list.

>     
>     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 0b2cf2c34..a45595443 100644
> --- a/src/lib/core/say.c
> +++ b/src/lib/core/say.c
> @@ -506,10 +506,15 @@ 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));
> +		/*
> +		 * We need to log a diagnostics here until stacked
> +		 * diagnostics will be implemented (#1148).
> +		 */
> +		diag_log();
>  		goto out;
>  	}
>  	for (ptr = inf; ptr; ptr = ptr->ai_next) {
> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> index 55de5e41c..d92e9140d 100755
> --- a/test/box-tap/cfg.test.lua
> +++ b/test/box-tap/cfg.test.lua
> @@ -6,7 +6,7 @@ local socket = require('socket')
>  local fio = require('fio')
>  local uuid = require('uuid')
>  local msgpack = require('msgpack')
> -test:plan(104)
> +test:plan(105)
>  
>  --------------------------------------------------------------------------------
>  -- Invalid values
> @@ -566,6 +566,23 @@ os.exit(0)
>  ]]
>  test:is(run_script(code), 0, "log_nonblock")
>  
> +--
> +-- gh-4138: check getaddrinfo() error and panic after that.
> +--
> +code=[[
> +local socket = require('socket')
> +local log = require('log')
> +local fio = require('fio')
> +
> +path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock')
> +unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0)
> +unix_socket:bind('unix/', path)
> +
> +opt = string.format("syslog:server=non_exists_hostname:%s,identity=tarantool", path)
> +box.cfg{log = opt, log_nonblock=true}

log_nonblock is not needed here, so it is better to remove it.

box.cfg{log = 'syslog:server=non_exists_hostname:3301'} is enough, not need to
form a file path, no need identity, no need requiring socket, log and fio.

The test passes even before the patch, so what it is intended to test? I
think we should write a test that verifies stderr output to find all log
messages we expect to appear in the case:

Linux:

 | SystemError getaddrinfo: Temporary failure in name resolution: Input/output error
 | SystemError syslog logger: Input/output error: Input/output error
 | failed to initialize logging subsystem

gai_strerror() message corresponds to EAI_AGAIN.

Mac OS:

 | 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

gai_strerror() message corresponds to EAI_NONAME.

I propose to call ffi.C.gai_strerror() right from a test to form two
error messages and verify that the actual input match one of them.

If it is hard to catch stderr, then let's proceed w/o this test. However
I think it is doable.

I also propose to test error messages in the similar way (using
ffi.C.gai_strerror(GAI_AGAIN) and ffi.C.gai_strerror(GAI_NONAME)) in
test cases in second patch of the patchset.

> +]]
> +test:is(run_script(code), PANIC, "log_nonblock")
> +
>  --
>  -- Crash (instead of panic) when trying to recover a huge tuple.
>  --
> diff --git a/test/unit/coio.cc b/test/unit/coio.cc
> index bb8bd7131..a70d3254d 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,12 @@ test_getaddrinfo(void)
>  	is(rc, 0, "getaddrinfo");
>  	freeaddrinfo(i);
>  
> +	/* gh-4138: Check getaddrinfo() error. */
> +	isnt(coio_getaddrinfo("non_exists_hostname", port, NULL, &i, 1), 0,
> +	     "getaddrinfo error");

I would say 'getaddrinfo retval' instead 'getaddrinfo error'.

Use `rc = coio_getaddrinfo(<...>)` as above within this function, it
reads easier.

> +	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
> +	     "getaddrinfo error message");
> +

I propose to verify the entire error message using
gai_strerror(GAI_AGAIN) and gai_strerror(GAI_NONAME)—just as proposed
above for a log message.

>  	/*
>  	 * 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..49759b747 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 error
> +ok 3 - getaddrinfo error message
>  	*** test_getaddrinfo: done ***
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [tarantool-patches] Re: [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-08-28 21:34       ` Alexander Turenko
@ 2019-08-29  0:51         ` Alexander Turenko
       [not found]         ` <8E98F721-601F-436D-8F0A-5E399D8F7CAB@tarantool.org>
  1 sibling, 0 replies; 19+ messages in thread
From: Alexander Turenko @ 2019-08-29  0:51 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

> Linux:
> 
>  | SystemError getaddrinfo: Temporary failure in name resolution: Input/output error
>  | SystemError syslog logger: Input/output error: Input/output error
>  | failed to initialize logging subsystem
> 
> gai_strerror() message corresponds to EAI_AGAIN.
> 
> Mac OS:
> 
>  | 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
> 
> gai_strerror() message corresponds to EAI_NONAME.

I see now that Linux also can give EAI_NONAME in the case. It seems it
depends on network conditions. Anyway, let's check for both.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [tarantool-patches] Re: [PATCH v2 2/2] say: take getaddrinfo() errors into account
       [not found]         ` <8E98F721-601F-436D-8F0A-5E399D8F7CAB@tarantool.org>
@ 2019-09-06 13:44           ` Alexander Turenko
  2019-09-10 12:52             ` Roman Khabibov
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Turenko @ 2019-09-06 13:44 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

> >> +--
> >> +-- gh-4138: check getaddrinfo() error and panic after that.
> >> +--
> >> +code=[[
> >> +local socket = require('socket')
> >> +local log = require('log')
> >> +local fio = require('fio')
> >> +
> >> +path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock')
> >> +unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0)
> >> +unix_socket:bind('unix/', path)
> >> +
> >> +opt = string.format("syslog:server=non_exists_hostname:%s,identity=tarantool", path)
> >> +box.cfg{log = opt, log_nonblock=true}
> > 
> > log_nonblock is not needed here, so it is better to remove it.
> Removed.

Still no.

> > box.cfg{log = 'syslog:server=non_exists_hostname:3301'} is enough, not need to
> > form a file path, no need identity, no need requiring socket, log and fio.
> > 
> > The test passes even before the patch, so what it is intended to test? I
> > think we should write a test that verifies stderr output to find all log
> > messages we expect to appear in the case:
> > 
> > Linux:
> > 
> > | SystemError getaddrinfo: Temporary failure in name resolution: Input/output error
> > | SystemError syslog logger: Input/output error: Input/output error
> > | failed to initialize logging subsystem
> > 
> > gai_strerror() message corresponds to EAI_AGAIN.
> > 
> > Mac OS:
> > 
> > | 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
> > 
> > gai_strerror() message corresponds to EAI_NONAME.
> > 
> > I propose to call ffi.C.gai_strerror() right from a test to form two
> > error messages and verify that the actual input match one of them.
> > 
> > If it is hard to catch stderr, then let's proceed w/o this test. However
> > I think it is doable.
> > 
> > I also propose to test error messages in the similar way (using
> > ffi.C.gai_strerror(GAI_AGAIN) and ffi.C.gai_strerror(GAI_NONAME)) in
> > test cases in second patch of the patchset.
> We discussed that with Vova and we decided, that it is too difficult.
> It is better, to waste not time for this test. Test for PANIC is enough.

I don't see any reason to add a test case that checks that tarantool
exits in the case: it was so before the commit and nothing is changed
except the error message. I propose to delete the test case.

> >> +	/* gh-4138: Check getaddrinfo() error. */
> >> +	isnt(coio_getaddrinfo("non_exists_hostname", port, NULL, &i, 1), 0,
> >> +	     "getaddrinfo error");
> > 
> > I would say 'getaddrinfo retval' instead 'getaddrinfo error'.

I don't insist, but remind about that here if it was missed by a
mistake.

> >> +	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
> >> +	     "getaddrinfo error message");
> >> +
> > 
> > I propose to verify the entire error message using
> > gai_strerror(GAI_AGAIN) and gai_strerror(GAI_NONAME)—just as proposed
> > above for a log message.
> I have not found the way, how to carry this macros/enum from the libc header to Lua.

It is possible to add them into socket.c, but anyway it is not worth to
do this just for a test. Let's check for certain error messages (not for
just 'getaddrinfo: ' prefix).

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [tarantool-patches] Re: [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-09-06 13:44           ` Alexander Turenko
@ 2019-09-10 12:52             ` Roman Khabibov
  2019-11-01 15:19               ` [Tarantool-patches] " Alexander Turenko
  0 siblings, 1 reply; 19+ messages in thread
From: Roman Khabibov @ 2019-09-10 12:52 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Alexander Turenko



> On Sep 6, 2019, at 16:44, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
>>>> +--
>>>> +-- gh-4138: check getaddrinfo() error and panic after that.
>>>> +--
>>>> +code=[[
>>>> +local socket = require('socket')
>>>> +local log = require('log')
>>>> +local fio = require('fio')
>>>> +
>>>> +path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock')
>>>> +unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0)
>>>> +unix_socket:bind('unix/', path)
>>>> +
>>>> +opt = string.format("syslog:server=non_exists_hostname:%s,identity=tarantool", path)
>>>> +box.cfg{log = opt, log_nonblock=true}
>>> 
>>> log_nonblock is not needed here, so it is better to remove it.
>> Removed.
> 
> Still no.
> 
>>> box.cfg{log = 'syslog:server=non_exists_hostname:3301'} is enough, not need to
>>> form a file path, no need identity, no need requiring socket, log and fio.
>>> 
>>> The test passes even before the patch, so what it is intended to test? I
>>> think we should write a test that verifies stderr output to find all log
>>> messages we expect to appear in the case:
>>> 
>>> Linux:
>>> 
>>> | SystemError getaddrinfo: Temporary failure in name resolution: Input/output error
>>> | SystemError syslog logger: Input/output error: Input/output error
>>> | failed to initialize logging subsystem
>>> 
>>> gai_strerror() message corresponds to EAI_AGAIN.
>>> 
>>> Mac OS:
>>> 
>>> | 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
>>> 
>>> gai_strerror() message corresponds to EAI_NONAME.
>>> 
>>> I propose to call ffi.C.gai_strerror() right from a test to form two
>>> error messages and verify that the actual input match one of them.
>>> 
>>> If it is hard to catch stderr, then let's proceed w/o this test. However
>>> I think it is doable.
>>> 
>>> I also propose to test error messages in the similar way (using
>>> ffi.C.gai_strerror(GAI_AGAIN) and ffi.C.gai_strerror(GAI_NONAME)) in
>>> test cases in second patch of the patchset.
>> We discussed that with Vova and we decided, that it is too difficult.
>> It is better, to waste not time for this test. Test for PANIC is enough.
> 
> I don't see any reason to add a test case that checks that tarantool
> exits in the case: it was so before the commit and nothing is changed
> except the error message. I propose to delete the test case.
Ok. Removed.

>>>> +	/* gh-4138: Check getaddrinfo() error. */
>>>> +	isnt(coio_getaddrinfo("non_exists_hostname", port, NULL, &i, 1), 0,
>>>> +	     "getaddrinfo error");
>>> 
>>> I would say 'getaddrinfo retval' instead 'getaddrinfo error'.
> 
> I don't insist, but remind about that here if it was missed by a
> mistake.
+	/* gh-4138: Check getaddrinfo() retval. */
+	rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, 1);
+	isnt(rc, 0, "getaddrinfo retval");
+	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
+	     "getaddrinfo error message");
+

>>>> +	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
>>>> +	     "getaddrinfo error message");
>>>> +
>>> 
>>> I propose to verify the entire error message using
>>> gai_strerror(GAI_AGAIN) and gai_strerror(GAI_NONAME)—just as proposed
>>> above for a log message.
>> I have not found the way, how to carry this macros/enum from the libc header to Lua.
> 
> It is possible to add them into socket.c, but anyway it is not worth to
> do this just for a test. Let's check for certain error messages (not for
> just 'getaddrinfo: ' prefix).

commit 3e973c2203131ab663bdf1154be73ff6592568ea
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Tue Jul 30 15:39:21 2019 +0300

    coiox/say: fix getaddrinfo error handling on macOS
    
    Before this patch, branch when getaddrinfo() returns error codes
    couldn't be reached on macOS, because they are greater than 0 on
    macOS (assumption "rc < 0" in commit ea1da04 is incorrect for
    macOS).
    
    Note: diag_log() in say.c was added, because otherwise it will be
    hid by the following diagnostic and then say that it should
    be handler in a better way after #1148. Also, two diag_set() in
    syslog_connect_unix() was added to avoid asserts in this
    diag_log().
    
    Need for #4138

diff --git a/src/lib/core/coio_task.c b/src/lib/core/coio_task.c
index 908b336ed..83f669d05 100644
--- a/src/lib/core/coio_task.c
+++ b/src/lib/core/coio_task.c
@@ -413,7 +413,7 @@ coio_getaddrinfo(const char *host, const char *port,
 		return -1; /* timed out or cancelled */
 
 	/* Task finished */
-	if (task->rc < 0) {
+	if (task->rc != 0) {
 		/* getaddrinfo() failed */
 		errno = EIO;
 		diag_set(SystemError, "getaddrinfo: %s",
diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index 64a637c58..8ad88ad57 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -459,14 +459,17 @@ static inline int
 syslog_connect_unix(const char *path)
 {
 	int fd = socket(PF_UNIX, SOCK_DGRAM, 0);
-	if (fd < 0)
+	if (fd < 0) {
+		diag_set(SystemError, "socket");
 		return -1;
+	}
 	struct sockaddr_un un;
 	memset(&un, 0, sizeof(un));
 	snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
 	un.sun_family = AF_UNIX;
 	if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) {
 		close(fd);
+		diag_set(SystemError, "connect");
 		return -1;
 	}
 	return fd;
@@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address)
 	hints.ai_protocol = IPPROTO_UDP;
 
 	ret = getaddrinfo(remote, portnum, &hints, &inf);
-	if (ret < 0) {
+	if (ret != 0) {
 		errno = EIO;
 		diag_set(SystemError, "getaddrinfo: %s",
 			 gai_strerror(ret));
@@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str)
 	say_free_syslog_opts(&opts);
 	log->fd = log_syslog_connect(log);
 	if (log->fd < 0) {
+		/*
+		 * We need to log a diagnostics here until stacked
+		 * diagnostics will be implemented (#1148).
+		 */
+		diag_log();
 		/* syslog indent is freed in atexit(). */
 		diag_set(SystemError, "syslog logger: %s", strerror(errno));
 		return -1;
diff --git a/test/unit/coio.cc b/test/unit/coio.cc
index bb8bd7131..3758c8857 100644
--- a/test/unit/coio.cc
+++ b/test/unit/coio.cc
@@ -72,7 +72,7 @@ static void
 test_getaddrinfo(void)
 {
 	header();
-	plan(1);
+	plan(3);
 	const char *host = "127.0.0.1";
 	const char *port = "3333";
 	struct addrinfo *i;
@@ -81,6 +81,12 @@ test_getaddrinfo(void)
 	is(rc, 0, "getaddrinfo");
 	freeaddrinfo(i);
 
+	/* gh-4138: Check getaddrinfo() retval. */
+	rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i, 1);
+	isnt(rc, 0, "getaddrinfo retval");
+	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
+	     "getaddrinfo error message");
+
 	/*
 	 * gh-4209: 0 timeout should not be a special value and
 	 * detach a task. Before a fix it led to segfault
diff --git a/test/unit/coio.result b/test/unit/coio.result
index 5019fa48a..90b567140 100644
--- a/test/unit/coio.result
+++ b/test/unit/coio.result
@@ -7,6 +7,8 @@
 # call done with res 0
 	*** test_call_f: done ***
 	*** test_getaddrinfo ***
-1..1
+1..3
 ok 1 - getaddrinfo
+ok 2 - getaddrinfo retval
+ok 3 - getaddrinfo error message
 	*** test_getaddrinfo: done ***

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-09-10 12:52             ` Roman Khabibov
@ 2019-11-01 15:19               ` Alexander Turenko
  2019-11-21 17:28                 ` [Tarantool-patches] [tarantool-patches] " Roman Khabibov
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Turenko @ 2019-11-01 15:19 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches, tarantool-patches

> coiox/say: fix getaddrinfo error handling on macOS

Typo: coiox -> coio.

> >>>> +	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
> >>>> +	     "getaddrinfo error message");
> >>>> +
> >>> 
> >>> I propose to verify the entire error message using
> >>> gai_strerror(GAI_AGAIN) and gai_strerror(GAI_NONAME)—just as proposed
> >>> above for a log message.
> >> I have not found the way, how to carry this macros/enum from the libc header to Lua.
> > 
> > It is possible to add them into socket.c, but anyway it is not worth to
> > do this just for a test. Let's check for certain error messages (not for
> > just 'getaddrinfo: ' prefix).

Again: let's check for certain error messages (not for just
'getaddrinfo: ' prefix).

The patch looks okay other then this.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-11-01 15:19               ` [Tarantool-patches] " Alexander Turenko
@ 2019-11-21 17:28                 ` Roman Khabibov
  2019-12-08 19:48                   ` Alexander Turenko
  0 siblings, 1 reply; 19+ messages in thread
From: Roman Khabibov @ 2019-11-21 17:28 UTC (permalink / raw)
  To: tarantool-patches



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

>>>>>> +	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
>>>>>> +	     "getaddrinfo error message");
>>>>>> +
>>>>> 
>>>>> I propose to verify the entire error message using
>>>>> gai_strerror(GAI_AGAIN) and gai_strerror(GAI_NONAME)—just as proposed
>>>>> above for a log message.
>>>> I have not found the way, how to carry this macros/enum from the libc header to Lua.
>>> 
>>> It is possible to add them into socket.c, but anyway it is not worth to
>>> do this just for a test. Let's check for certain error messages (not for
>>> just 'getaddrinfo: ' prefix).
> 
> Again: let's check for certain error messages (not for just
> 'getaddrinfo: ' prefix).
I have changed the timeout to 15768000000, because last->errmsg was ’timed out’.

@@ -72,14 +72,28 @@ static void
 test_getaddrinfo(void)
 {
 	header();
-	plan(1);
+	plan(3);
 	const char *host = "127.0.0.1";
 	const char *port = "3333";
 	struct addrinfo *i;
 	/* NULL hints should work. It is a standard. */
 	int rc = coio_getaddrinfo(host, port, NULL, &i, 1);
 	is(rc, 0, "getaddrinfo");
-	freeaddrinfo(i);
+
+	/* gh-4138: Check getaddrinfo() retval. */
+	rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i,
+			      15768000000);
+	isnt(rc, 0, "getaddrinfo retval");
+	const char *errmsg = diag_get()->last->errmsg;
+	const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided"
+		", or not known";
+	const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for "
+		"ai_socktype";
+	const char *exp_errmsg_3 = "getaddrinfo: Name or service not known";
+	bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 ||
+		strcmp(errmsg, exp_errmsg_2) == 0 ||
+		strcmp(errmsg, exp_errmsg_3) == 0;
+	is(is_match_with_exp, true, "getaddrinfo error message");
 
 	/*
 	 * gh-4209: 0 timeout should not be a special value and

> The patch looks okay other then this.

commit d9c23fde4c7c1fa2979954be83c1a2f6f8407347
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Tue Jul 30 15:39:21 2019 +0300

    coio/say: fix getaddrinfo error handling on macOS
    
    Before this patch, branch when getaddrinfo() returns error codes
    couldn't be reached on macOS, because they are greater than 0 on
    macOS (assumption "rc < 0" in commit ea1da04 is incorrect for
    macOS).
    
    Note: diag_log() in say.c was added, because otherwise it will be
    hid by the following diagnostic and then say that it should
    be handler in a better way after #1148. Also, two diag_set() in
    syslog_connect_unix() was added to avoid asserts in this
    diag_log().
    
    Need for #4138

diff --git a/src/lib/core/coio_task.c b/src/lib/core/coio_task.c
index 908b336ed..83f669d05 100644
--- a/src/lib/core/coio_task.c
+++ b/src/lib/core/coio_task.c
@@ -413,7 +413,7 @@ coio_getaddrinfo(const char *host, const char *port,
 		return -1; /* timed out or cancelled */
 
 	/* Task finished */
-	if (task->rc < 0) {
+	if (task->rc != 0) {
 		/* getaddrinfo() failed */
 		errno = EIO;
 		diag_set(SystemError, "getaddrinfo: %s",
diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index 64a637c58..8ad88ad57 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -459,14 +459,17 @@ static inline int
 syslog_connect_unix(const char *path)
 {
 	int fd = socket(PF_UNIX, SOCK_DGRAM, 0);
-	if (fd < 0)
+	if (fd < 0) {
+		diag_set(SystemError, "socket");
 		return -1;
+	}
 	struct sockaddr_un un;
 	memset(&un, 0, sizeof(un));
 	snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
 	un.sun_family = AF_UNIX;
 	if (connect(fd, (struct sockaddr *) &un, sizeof(un)) != 0) {
 		close(fd);
+		diag_set(SystemError, "connect");
 		return -1;
 	}
 	return fd;
@@ -506,7 +509,7 @@ syslog_connect_remote(const char *server_address)
 	hints.ai_protocol = IPPROTO_UDP;
 
 	ret = getaddrinfo(remote, portnum, &hints, &inf);
-	if (ret < 0) {
+	if (ret != 0) {
 		errno = EIO;
 		diag_set(SystemError, "getaddrinfo: %s",
 			 gai_strerror(ret));
@@ -593,6 +596,11 @@ log_syslog_init(struct log *log, const char *init_str)
 	say_free_syslog_opts(&opts);
 	log->fd = log_syslog_connect(log);
 	if (log->fd < 0) {
+		/*
+		 * We need to log a diagnostics here until stacked
+		 * diagnostics will be implemented (#1148).
+		 */
+		diag_log();
 		/* syslog indent is freed in atexit(). */
 		diag_set(SystemError, "syslog logger: %s", strerror(errno));
 		return -1;
diff --git a/test/unit/coio.cc b/test/unit/coio.cc
index bb8bd7131..d5cbd2fb9 100644
--- a/test/unit/coio.cc
+++ b/test/unit/coio.cc
@@ -72,14 +72,28 @@ static void
 test_getaddrinfo(void)
 {
 	header();
-	plan(1);
+	plan(3);
 	const char *host = "127.0.0.1";
 	const char *port = "3333";
 	struct addrinfo *i;
 	/* NULL hints should work. It is a standard. */
 	int rc = coio_getaddrinfo(host, port, NULL, &i, 1);
 	is(rc, 0, "getaddrinfo");
-	freeaddrinfo(i);
+
+	/* gh-4138: Check getaddrinfo() retval. */
+	rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i,
+			      15768000000);
+	isnt(rc, 0, "getaddrinfo retval");
+	const char *errmsg = diag_get()->last->errmsg;
+	const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided"
+		", or not known";
+	const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for "
+		"ai_socktype";
+	const char *exp_errmsg_3 = "getaddrinfo: Name or service not known";
+	bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 ||
+		strcmp(errmsg, exp_errmsg_2) == 0 ||
+		strcmp(errmsg, exp_errmsg_3) == 0;
+	is(is_match_with_exp, true, "getaddrinfo error message");
 
 	/*
 	 * gh-4209: 0 timeout should not be a special value and
diff --git a/test/unit/coio.result b/test/unit/coio.result
index 5019fa48a..90b567140 100644
--- a/test/unit/coio.result
+++ b/test/unit/coio.result
@@ -7,6 +7,8 @@
 # call done with res 0
 	*** test_call_f: done ***
 	*** test_getaddrinfo ***
-1..1
+1..3
 ok 1 - getaddrinfo
+ok 2 - getaddrinfo retval
+ok 3 - getaddrinfo error message
 	*** test_getaddrinfo: done ***

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-11-21 17:28                 ` [Tarantool-patches] [tarantool-patches] " Roman Khabibov
@ 2019-12-08 19:48                   ` Alexander Turenko
  2019-12-10 16:25                     ` Roman Khabibov
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Turenko @ 2019-12-08 19:48 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

> @@ -72,14 +72,28 @@ static void
>  test_getaddrinfo(void)
>  {
>  	header();
> -	plan(1);
> +	plan(3);
>  	const char *host = "127.0.0.1";
>  	const char *port = "3333";
>  	struct addrinfo *i;
>  	/* NULL hints should work. It is a standard. */
>  	int rc = coio_getaddrinfo(host, port, NULL, &i, 1);
>  	is(rc, 0, "getaddrinfo");
> -	freeaddrinfo(i);

Why do you remove freeing of the previous result? It will lead to a leak
and so will require extra work to run unit tests under ASAN or Valgrind.

> +
> +	/* gh-4138: Check getaddrinfo() retval. */

Nit: retval and diagnostics area. You changed the code and the comment
becomes inaccurate.

> +	rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i,
> +			      15768000000);
> +	isnt(rc, 0, "getaddrinfo retval");
> +	const char *errmsg = diag_get()->last->errmsg;
> +	const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided"
> +		", or not known";
> +	const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for "
> +		"ai_socktype";
> +	const char *exp_errmsg_3 = "getaddrinfo: Name or service not known";
> +	bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 ||
> +		strcmp(errmsg, exp_errmsg_2) == 0 ||
> +		strcmp(errmsg, exp_errmsg_3) == 0;
> +	is(is_match_with_exp, true, "getaddrinfo error message");
>  
>  	/*
>  	 * gh-4209: 0 timeout should not be a special value and
> 
> > The patch looks okay other then this.
> 
> commit d9c23fde4c7c1fa2979954be83c1a2f6f8407347
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Tue Jul 30 15:39:21 2019 +0300
> 
>     coio/say: fix getaddrinfo error handling on macOS
>     
>     Before this patch, branch when getaddrinfo() returns error codes
>     couldn't be reached on macOS, because they are greater than 0 on
>     macOS (assumption "rc < 0" in commit ea1da04 is incorrect for
>     macOS).
>     
>     Note: diag_log() in say.c was added, because otherwise it will be
>     hid by the following diagnostic and then say that it should
>     be handler in a better way after #1148. Also, two diag_set() in

'and then say' -- looks as copy-paste from review notes.

Typo: handler -> handled.

>     syslog_connect_unix() was added to avoid asserts in this
>     diag_log().
>     
>     Need for #4138

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-12-08 19:48                   ` Alexander Turenko
@ 2019-12-10 16:25                     ` Roman Khabibov
  2019-12-18 15:01                       ` Alexander Turenko
  0 siblings, 1 reply; 19+ messages in thread
From: Roman Khabibov @ 2019-12-10 16:25 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: tarantool-patches

Hi! Thanks for the review.

> On Dec 8, 2019, at 22:48, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
>> @@ -72,14 +72,28 @@ static void
>> test_getaddrinfo(void)
>> {
>> 	header();
>> -	plan(1);
>> +	plan(3);
>> 	const char *host = "127.0.0.1";
>> 	const char *port = "3333";
>> 	struct addrinfo *i;
>> 	/* NULL hints should work. It is a standard. */
>> 	int rc = coio_getaddrinfo(host, port, NULL, &i, 1);
>> 	is(rc, 0, "getaddrinfo");
>> -	freeaddrinfo(i);
> 
> Why do you remove freeing of the previous result? It will lead to a leak
> and so will require extra work to run unit tests under ASAN or Valgrind.
Sorry. I did it for debug and forgot to return this line.

@@ -81,6 +81,24 @@ 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";
+	bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 ||
+		strcmp(errmsg, exp_errmsg_2) == 0 ||
+		strcmp(errmsg, exp_errmsg_3) == 0;
+	is(is_match_with_exp, true, "getaddrinfo error message");
+

>> +
>> +	/* gh-4138: Check getaddrinfo() retval. */
> 
> Nit: retval and diagnostics area. You changed the code and the comment
> becomes inaccurate.
> 
>> +	rc = coio_getaddrinfo("non_exists_hostname", port, NULL, &i,
>> +			      15768000000);
>> +	isnt(rc, 0, "getaddrinfo retval");
>> +	const char *errmsg = diag_get()->last->errmsg;
>> +	const char *exp_errmsg_1 = "getaddrinfo: nodename nor servname provided"
>> +		", or not known";
>> +	const char *exp_errmsg_2 = "getaddrinfo: Servname not supported for "
>> +		"ai_socktype";
>> +	const char *exp_errmsg_3 = "getaddrinfo: Name or service not known";
>> +	bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 ||
>> +		strcmp(errmsg, exp_errmsg_2) == 0 ||
>> +		strcmp(errmsg, exp_errmsg_3) == 0;
>> +	is(is_match_with_exp, true, "getaddrinfo error message");
>> 
>> 	/*
>> 	 * gh-4209: 0 timeout should not be a special value and
>> 
>>> The patch looks okay other then this.
>> 
>> commit d9c23fde4c7c1fa2979954be83c1a2f6f8407347
>> Author: Roman Khabibov <roman.habibov@tarantool.org>
>> Date:   Tue Jul 30 15:39:21 2019 +0300
>> 
>>    coio/say: fix getaddrinfo error handling on macOS
>> 
>>    Before this patch, branch when getaddrinfo() returns error codes
>>    couldn't be reached on macOS, because they are greater than 0 on
>>    macOS (assumption "rc < 0" in commit ea1da04 is incorrect for
>>    macOS).
>> 
>>    Note: diag_log() in say.c was added, because otherwise it will be
>>    hid by the following diagnostic and then say that it should
>>    be handler in a better way after #1148. Also, two diag_set() in
> 
> 'and then say' -- looks as copy-paste from review notes.
> 
> Typo: handler -> handled.
Done.

>>    syslog_connect_unix() was added to avoid asserts in this
>>    diag_log().
>> 
>>    Need for #4138

commit ff683ad948dd9404221979f399b155c63aa88e5a
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 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..aa38f5a99 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,24 @@ 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";
+	bool is_match_with_exp = strcmp(errmsg, exp_errmsg_1) == 0 ||
+		strcmp(errmsg, exp_errmsg_2) == 0 ||
+		strcmp(errmsg, exp_errmsg_3) == 0;
+	is(is_match_with_exp, true, "getaddrinfo error message");
+
 	/*
 	 * gh-4209: 0 timeout should not be a special value and
 	 * 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 ***

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-12-10 16:25                     ` Roman Khabibov
@ 2019-12-18 15:01                       ` Alexander Turenko
  2019-12-21 17:50                         ` Roman Khabibov
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Turenko @ 2019-12-18 15:01 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

Only one comment.

unit/coio fails on FreeBSD:

[006] Test failed! Result content mismatch:
[006] --- unit/coio.result	Wed Dec 11 09:58:11 2019
[006] +++ unit/coio.reject	Wed Sep 18 08:51:42 2019
[006] @@ -1,3 +1,6 @@
[006] +#   Failed test 'getaddrinfo error message'
[006] +#   in /home/vagrant/tarantool/test/unit/coio.cc at line 100
[006] +# Looks like you failed 1 test of 3 run.
[006]  	*** stat_timeout_test ***
[006]  	*** stat_timeout_test: done ***
[006]  	*** stat_notify_test ***
[006] @@ -10,5 +13,5 @@
[006]  1..3
[006]  ok 1 - getaddrinfo
[006]  ok 2 - getaddrinfo retval
[006] -ok 3 - getaddrinfo error message
[006] +not ok 3 - getaddrinfo error message
[006]  	*** test_getaddrinfo: done ***

https://gitlab.com/tarantool/tarantool/-/jobs/375844818

Please, also push a branch with -full-ci postfix: this will run the
whole testing matrix on the branch in GitLab-CI. It worth to verify this
just in case.

WBR, Alexander Turenko.

On Tue, Dec 10, 2019 at 07:25:05PM +0300, Roman Khabibov wrote:
> Hi! Thanks for the review.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-12-18 15:01                       ` Alexander Turenko
@ 2019-12-21 17:50                         ` Roman Khabibov
  2019-12-23 12:56                           ` Alexander Turenko
  0 siblings, 1 reply; 19+ messages in thread
From: Roman Khabibov @ 2019-12-21 17:50 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: tarantool-patches



> On Dec 18, 2019, at 18:01, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> Only one comment.
> 
> unit/coio fails on FreeBSD:
Fixed.

> [006] Test failed! Result content mismatch:
> [006] --- unit/coio.result	Wed Dec 11 09:58:11 2019
> [006] +++ unit/coio.reject	Wed Sep 18 08:51:42 2019
> [006] @@ -1,3 +1,6 @@
> [006] +#   Failed test 'getaddrinfo error message'
> [006] +#   in /home/vagrant/tarantool/test/unit/coio.cc at line 100
> [006] +# Looks like you failed 1 test of 3 run.
> [006]  	*** stat_timeout_test ***
> [006]  	*** stat_timeout_test: done ***
> [006]  	*** stat_notify_test ***
> [006] @@ -10,5 +13,5 @@
> [006]  1..3
> [006]  ok 1 - getaddrinfo
> [006]  ok 2 - getaddrinfo retval
> [006] -ok 3 - getaddrinfo error message
> [006] +not ok 3 - getaddrinfo error message
> [006]  	*** test_getaddrinfo: done ***
> 
> https://gitlab.com/tarantool/tarantool/-/jobs/375844818
> 
> Please, also push a branch with -full-ci postfix: this will run the
> whole testing matrix on the branch in GitLab-CI. It worth to verify this
> just in case.
> 
> WBR, Alexander Turenko.
> 
> On Tue, Dec 10, 2019 at 07:25:05PM +0300, Roman Khabibov wrote:
>> Hi! Thanks for the review.

commit ea8478bc3363cb9278d40f412dfccd9a43e70e0e
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 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..1b4ffa32a 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,27 @@ 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";
+	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;
+	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 ***

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-12-21 17:50                         ` Roman Khabibov
@ 2019-12-23 12:56                           ` Alexander Turenko
  2019-12-23 13:38                             ` Alexander Turenko
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Turenko @ 2019-12-23 12:56 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

LGTM. Please, proceed with the next reviewer (Sergey O.).

> > Please, also push a branch with -full-ci postfix: this will run the
> > whole testing matrix on the branch in GitLab-CI. It worth to verify this
> > just in case.

I still think that you should do that.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-12-23 12:56                           ` Alexander Turenko
@ 2019-12-23 13:38                             ` Alexander Turenko
  2019-12-26 17:29                               ` Roman Khabibov
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Turenko @ 2019-12-23 13:38 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

On Mon, Dec 23, 2019 at 03:56:58PM +0300, Alexander Turenko wrote:
> LGTM. Please, proceed with the next reviewer (Sergey O.).
> 
> > > Please, also push a branch with -full-ci postfix: this will run the
> > > whole testing matrix on the branch in GitLab-CI. It worth to verify this
> > > just in case.
> 
> I still think that you should do that.

I run tests and got EAI_AGAIN. Let's add it too to the list. See
https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/013260.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-12-23 13:38                             ` Alexander Turenko
@ 2019-12-26 17:29                               ` Roman Khabibov
  2019-12-26 21:05                                 ` Alexander Turenko
  0 siblings, 1 reply; 19+ messages in thread
From: Roman Khabibov @ 2019-12-26 17:29 UTC (permalink / raw)
  To: Sergey Ostanevich; +Cc: tarantool-patches



> On Dec 23, 2019, at 16:38, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> On Mon, Dec 23, 2019 at 03:56:58PM +0300, Alexander Turenko wrote:
>> LGTM. Please, proceed with the next reviewer (Sergey O.).
>> 
>>>> Please, also push a branch with -full-ci postfix: this will run the
>>>> whole testing matrix on the branch in GitLab-CI. It worth to verify this
>>>> just in case.
>> 
>> I still think that you should do that.
> 
> I run tests and got EAI_AGAIN. Let's add it too to the list. See
> https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/013260.html

commit 4ac352b12ee701898042808408eadcd24caf5bb9
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 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..22c2a9226 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,30 @@ 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";
+	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;
+	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 ***

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-12-26 17:29                               ` Roman Khabibov
@ 2019-12-26 21:05                                 ` Alexander Turenko
  2019-12-27 13:01                                   ` Roman Khabibov
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Turenko @ 2019-12-26 21:05 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

> > I run tests and got EAI_AGAIN. Let's add it too to the list. See
> > https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/013260.html

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

Linux:

$ gcc -Wall -Wextra -x c <(echo -e '#include <sys/types.h>\n#include <sys/socket.h>\n#include <netdb.h>\n#include <stdio.h>\nint main() { printf("%s\\n", gai_strerror(EAI_AGAIN)); return 0; }') && ./a.out; rm a.out
Temporary failure in name resolution

Mac OS:

$ clang -Wall -Wextra -x c <(echo -e '#include <sys/types.h>\n#include <sys/socket.h>\n#include <netdb.h>\n#include <stdio.h>\nint main() { printf("%s\\n", gai_strerror(EAI_AGAIN)); return 0; }') && ./a.out; rm a.out
Temporary failure in name resolution

FreeBSD:

% printf '#include <sys/types.h>\n#include <sys/socket.h>\n#include <netdb.h>\n#include <stdio.h>\nint main() { printf("%%s\\n", gai_strerror(EAI_AGAIN)); return 0; }' > tmp.c && gcc -Wall -Wextra -x c tmp.c && ./a.out ; rm a.out tmp.c
Name could not be resolved at this time

"temporary failure in name resolution" does not match any.

> +	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;
> +	is(is_match_with_exp, true, "getaddrinfo error message");

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] say: take getaddrinfo() errors into account
  2019-12-26 21:05                                 ` Alexander Turenko
@ 2019-12-27 13:01                                   ` Roman Khabibov
  0 siblings, 0 replies; 19+ messages in thread
From: Roman Khabibov @ 2019-12-27 13:01 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: tarantool-patches

Hi!

> Linux:
> 
> $ gcc -Wall -Wextra -x c <(echo -e '#include <sys/types.h>\n#include <sys/socket.h>\n#include <netdb.h>\n#include <stdio.h>\nint main() { printf("%s\\n", gai_strerror(EAI_AGAIN)); return 0; }') && ./a.out; rm a.out
> Temporary failure in name resolution
> 
> Mac OS:
> 
> $ clang -Wall -Wextra -x c <(echo -e '#include <sys/types.h>\n#include <sys/socket.h>\n#include <netdb.h>\n#include <stdio.h>\nint main() { printf("%s\\n", gai_strerror(EAI_AGAIN)); return 0; }') && ./a.out; rm a.out
> Temporary failure in name resolution
> 
> FreeBSD:
> 
> % printf '#include <sys/types.h>\n#include <sys/socket.h>\n#include <netdb.h>\n#include <stdio.h>\nint main() { printf("%%s\\n", gai_strerror(EAI_AGAIN)); return 0; }' > tmp.c && gcc -Wall -Wextra -x c tmp.c && ./a.out ; rm a.out tmp.c
> Name could not be resolved at this time
> 
> "temporary failure in name resolution" does not match any.
Fixed.

I looked here.
https://www.freebsd.org/cgi/man.cgi?query=gai_strerror&sektion=3&manpath=FreeBSD+5.4-RELEASE

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-12-27 13:01 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-25 13:38 [tarantool-patches] [PATCH v2 0/2] take getaddrinfo() errors into account Roman Khabibov
2019-06-25 13:38 ` [tarantool-patches] [PATCH v2 2/2] say: " Roman Khabibov
2019-07-23 14:52   ` [tarantool-patches] " Alexander Turenko
2019-08-05 13:32     ` Roman Khabibov
2019-08-28 21:34       ` Alexander Turenko
2019-08-29  0:51         ` Alexander Turenko
     [not found]         ` <8E98F721-601F-436D-8F0A-5E399D8F7CAB@tarantool.org>
2019-09-06 13:44           ` Alexander Turenko
2019-09-10 12:52             ` Roman Khabibov
2019-11-01 15:19               ` [Tarantool-patches] " Alexander Turenko
2019-11-21 17:28                 ` [Tarantool-patches] [tarantool-patches] " Roman Khabibov
2019-12-08 19:48                   ` Alexander Turenko
2019-12-10 16:25                     ` Roman Khabibov
2019-12-18 15:01                       ` Alexander Turenko
2019-12-21 17:50                         ` Roman Khabibov
2019-12-23 12:56                           ` Alexander Turenko
2019-12-23 13:38                             ` Alexander Turenko
2019-12-26 17:29                               ` Roman Khabibov
2019-12-26 21:05                                 ` Alexander Turenko
2019-12-27 13:01                                   ` Roman Khabibov

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