[tarantool-patches] [PATCH v2 0/3] Syslog destination

[tarantool-patches] [PATCH v2 0/3] Syslog destination

[Olga Arkhangelskaia]

This series adds support of configurable destionation for syslog.
The option is called server. Possible options are ip4 and unix socket:
syslog:server=unix:/path/to/socket,identity=myinstance
syslog:server=ip4:port,identity=tarantool_myinstance

If server option is not set, but syslog is used in log configuration - 
default sockets for syslogd are used: /dev/log or /var/run/syslog.

This series is upon fix #3205. That will be slightly changed later.

Issue: #3487
---
https://github.com/tarantool/tarantool/issues/3487

Branch: OKriw/gh-3487-syslog-conf-dest
---
https://github.com/tarantool/tarantool/tree/OKriw/gh-3487-syslog-conf-dest

v2:
- Changed tests. Use random port, no sever is started, used current dir
- Changed basic structs - now we have emum for syslog dst, reuse log->path

Olga Arkhangelskaia (3):
  Configurable syslog destination
  Syslog remote destination test
  Syslog destination test unix socket

 src/say.c                           | 106 +++++++++++++++++++++++++++++++++---
 src/say.h                           |  14 ++++-
 test/app-tap/syslog_remote.test.lua |  37 +++++++++++++
 test/app-tap/syslog_socket.test.lua |  40 ++++++++++++++
 4 files changed, 189 insertions(+), 8 deletions(-)
 create mode 100755 test/app-tap/syslog_remote.test.lua
 create mode 100755 test/app-tap/syslog_socket.test.lua

-- 
2.14.3 (Apple Git-98)

[tarantool-patches] [PATCH v2 1/3] Configurable syslog destination

[Olga Arkhangelskaia]

Added server option to syslog configuration.
Server option is responsible for log destination. At the momemt
there is two ways of usage:server=unix:/path/to/socket or
server=ipv4:port. If port is not set default udp port 514 is used.
If logging to syslog is set, however there is no sever options -
default location is used: Linux /dev/log and Mac /var/run/syslog.

Issue: #3487
---
https://github.com/tarantool/tarantool/issues/3487

Branch:https: OKriw/gh-3487-syslog-conf-dest
---
https://github.com/tarantool/tarantool/tree/OKriw/gh-3487-syslog-conf-dest

gh-3487-syslog-conf-dest 
 src/say.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 src/say.h |  14 ++++++++-
 2 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/src/say.c b/src/say.c
index 43124083c..50416c08d 100644
--- a/src/say.c
+++ b/src/say.c
@@ -36,6 +36,7 @@
 #include <stdarg.h>
 #include <stdio.h>
 #include <string.h>
+#include <netdb.h>
 #include <sys/types.h>
 #include <unistd.h>
 #include <fcntl.h>
@@ -52,6 +53,8 @@ enum say_format log_format = SF_PLAIN;
 /** List of logs to rotate */
 static RLIST_HEAD(log_rotate_list);
 
+static const char syslog_syntax_remainder[] =
+        "expecting syslog: or syslog:server=ip:potr or syslog:server=unix:/path";
 static const char logger_syntax_reminder[] =
 	"expecting a file name or a prefix, such as '|', 'pipe:', 'syslog:'";
 /**
@@ -473,16 +476,80 @@ syslog_connect_unix(const char *path)
 	return fd;
 }
 
+/**
+ * Connect to remote syslogd using server:port.
+ * @param ip address of remote host.
+ * @param port on remote host.
+ * @retval not 0 Socket descriptor.
+ * @retval    -1 Socket error.
+ */
+static int
+syslog_connect_remote(const char *server_address)
+{
+	/* IPv4 */
+	struct addrinfo *inf, hints, *ptr;
+	const char *remote;
+	char *portnum, *copy;
+	int fd = -1;
+
+	copy = strdup(server_address);
+	if (copy == NULL) {
+		diag_set(OutOfMemory, strlen(server_address), "malloc",
+			 copy);
+		return fd;
+	}
+	portnum = copy;
+	remote = strsep(&portnum, ":");
+	if (!remote) {
+		diag_set(IllegalParams,syslog_syntax_remainder);
+		free(copy);
+		return fd;
+	}
+	memset(&hints, 0, sizeof(hints));
+	hints.ai_family = AF_INET;
+	hints.ai_socktype = SOCK_DGRAM;
+	hints.ai_protocol = IPPROTO_UDP;
+	hints.ai_flags = AI_DEFAULT;
+
+	if (getaddrinfo(remote, portnum, NULL, &inf) < 0) {
+		free(copy);
+		return -1;
+	}
+	for (ptr = inf; ptr; ptr = ptr->ai_next) {
+		fd = socket(ptr->ai_family, ptr->ai_socktype, ptr->ai_protocol);
+		if (fd < 0) {
+			continue;
+		}
+
+		if (connect(fd, inf->ai_addr, inf->ai_addrlen) != 0) {
+			close(fd);
+			fd = -1;
+			continue;
+		}
+		break;
+	}
+	freeaddrinfo(inf);
+	free(copy);
+	return fd;
+}
+
 static inline int
 log_syslog_connect(struct log *log)
 {
+
 	/*
-	 * Try two locations: '/dev/log' for Linux and
+	 * If server option is not set use '/dev/log' for Linux and
 	 * '/var/run/syslog' for Mac.
 	 */
-	log->fd = syslog_connect_unix("/dev/log");
-	if (log->fd < 0)
-		log->fd = syslog_connect_unix("/var/run/syslog");
+	if (log->server_type == SAY_SYSLOG_LOCAL) {
+		log->fd = syslog_connect_unix("/dev/log");
+			if (log->fd < 0)
+				log->fd = syslog_connect_unix("/var/run/syslog");
+	} else if (log->server_type == SAY_SYSLOG_UNIX) {
+		log->fd = syslog_connect_unix(log->path);
+	} else {
+		log->fd = syslog_connect_remote(log->path);
+	}
 	return log->fd;
 }
 
@@ -498,6 +565,17 @@ log_syslog_init(struct log *log, const char *init_str)
 	if (say_parse_syslog_opts(init_str, &opts) < 0)
 		return -1;
 
+	log->server_type = opts.syslog_server;
+	if (log->server_type == SAY_SYSLOG_LOCAL)
+		log->path = NULL;
+	else {
+		log->path = strdup(opts.server);
+			if (log->path == NULL) {
+				diag_set(OutOfMemory, strlen(init_str),
+				         "malloc", "log->path");
+			return -1;
+		}
+	}
 	if (opts.identity == NULL)
 		log->syslog_ident = strdup("tarantool");
 	else
@@ -1044,6 +1122,8 @@ say_syslog_facility_by_name(const char *facility)
 int
 say_parse_syslog_opts(const char *init_str, struct say_syslog_opts *opts)
 {
+	opts->server = NULL;
+	opts->syslog_server = SAY_SYSLOG_LOCAL;
 	opts->identity = NULL;
 	opts->facility = syslog_facility_MAX;
 	opts->copy = strdup(init_str);
@@ -1051,8 +1131,8 @@ say_parse_syslog_opts(const char *init_str, struct say_syslog_opts *opts)
 		diag_set(OutOfMemory, strlen(init_str), "malloc", "opts->copy");
 		return -1;
 	}
-	char *ptr = opts->copy;
-	const char *option, *value;
+	char *ptr = opts->copy; 
+	const char *option, *value, *srv_str;
 
 	/* strsep() overwrites the separator with '\0' */
 	while ((option = strsep(&ptr, ","))) {
@@ -1060,7 +1140,19 @@ say_parse_syslog_opts(const char *init_str, struct say_syslog_opts *opts)
 			break;
 
 		value = option;
-		if (say_parse_prefix(&value, "identity=")) {
+		srv_str = say_parse_prefix(&value, "server=");
+		if (srv_str != NULL) {
+			if (opts->server != NULL ||
+			    opts->syslog_server != SAY_SYSLOG_LOCAL)
+				goto duplicate;
+			if (say_parse_prefix(&srv_str, "unix:")) {
+				opts->syslog_server = SAY_SYSLOG_UNIX;
+				opts->server = srv_str;
+			} else {
+				opts->syslog_server = SAY_SYSLOG_REMOTE;
+				opts->server = srv_str;
+			}
+		} else if (say_parse_prefix(&value, "identity=")) {
 			if (opts->identity != NULL)
 				goto duplicate;
 			opts->identity = value;
diff --git a/src/say.h b/src/say.h
index ad3ba3417..a1b6c2331 100644
--- a/src/say.h
+++ b/src/say.h
@@ -79,6 +79,12 @@ say_log_level_is_enabled(int level)
 
 extern enum say_format log_format;
 
+enum say_logger_syslog_server {
+	SAY_SYSLOG_LOCAL,
+	SAY_SYSLOG_UNIX,
+	SAY_SYSLOG_REMOTE
+};
+
 enum say_logger_type {
 	/**
 	 * Before the app server core is initialized, we do not
@@ -141,7 +147,10 @@ struct log {
 	/** The current log level. */
 	int level;
 	enum say_logger_type type;
-	/** path to file if logging to file. */
+	enum say_logger_syslog_server server_type;
+	/** path to file if logging to file
+	 * or server address in case of syslog
+	 * */
 	char *path;
 	bool nonblock;
 	log_format_func_t format_func;
@@ -162,6 +171,7 @@ struct log {
 	int rotating_threads;
 	enum syslog_facility syslog_facility;
 	struct rlist in_log_list;
+	/* Server options. Either ip/port pair or unix socket address.*/
 };
 
 /**
@@ -390,6 +400,8 @@ say_parse_logger_type(const char **str, enum say_logger_type *type);
 
 /** Syslog logger initialization params */
 struct say_syslog_opts {
+	enum say_logger_syslog_server syslog_server;
+	const char *server;
 	const char *identity;
 	enum syslog_facility facility;
 	/* Input copy (content unspecified). */
-- 
2.14.3 (Apple Git-98)

[tarantool-patches] [PATCH v2 2/3] Syslog remote destination test

[Olga Arkhangelskaia]

Adds syslog remote destination test in app-tap.
Test creates server, sets appropriate log level and redirects logs to it.
If log level received by the server is the same - test passes.

Usage: test-run.py syslog

Issue: #3487
---
https://github.com/tarantool/tarantool/issues/3487

Branch: OKriw/gh-3487-syslog-conf-dest 
---
https://github.com/tarantool/tarantool/tree/OKriw/gh-3487-syslog-conf-dest
 test/app-tap/syslog_remote.test.lua | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100755 test/app-tap/syslog_remote.test.lua

diff --git a/test/app-tap/syslog_remote.test.lua b/test/app-tap/syslog_remote.test.lua
new file mode 100755
index 000000000..01a75fa64
--- /dev/null
+++ b/test/app-tap/syslog_remote.test.lua
@@ -0,0 +1,37 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local socket = require('socket')
+local os = require('os')
+local test = tap.test("syslog_remote")
+local log = require('log')
+local errno = require('errno')
+
+local addr = '127.0.0.1'
+local port = 1000 + math.random(32768)
+
+test:plan(1)
+local function start_server()
+	test:diag("Starting server %s:%u", addr, port)
+	local sc = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+	sc:bind(addr, port)
+	return sc
+end
+
+local function find_log_str()
+	local opt = string.format("syslog:server=%s:%u,identity=tarantool", addr, port)
+	local res = false
+	box.cfg{log = opt, log_level = 5}
+	log.info('Test syslog destination')
+	while (sc:readable(1)) do
+		local buf = sc:recv(1000)
+		print (buf)
+		res = buf:match('Test syslog destination')
+	end
+		test:ok(res, "syslog_remote")
+end
+
+sc = start_server()
+test:test('syslog_remote',find_log_str(test))
+sc:close()
+os.exit(test:check() == true)
-- 
2.14.3 (Apple Git-98)

[tarantool-patches] [PATCH v2 3/3] Syslog destination test unix socket

[Olga Arkhangelskaia]

Adds syslog destination unix test in app-tap.
Test redirects logs to newly created unix socket and sets appropriate
log level. If log receives the same level - test passes.

Usage: test-run.py syslog

Issue:3487
---
https://github.com/tarantool/tarantool/issues/3487

Branch: OKriw/gh-3487-syslog-conf-dest
---
https://github.com/tarantool/tarantool/tree/OKriw/gh-3487-syslog-conf-dest

 test/app-tap/syslog_socket.test.lua | 40 +++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100755 test/app-tap/syslog_socket.test.lua

diff --git a/test/app-tap/syslog_socket.test.lua b/test/app-tap/syslog_socket.test.lua
new file mode 100755
index 000000000..febe106f5
--- /dev/null
+++ b/test/app-tap/syslog_socket.test.lua
@@ -0,0 +1,40 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local socket = require('socket')
+local os = require('os')
+local test = tap.test("syslog_unix")
+local log = require('log')
+local fio = require('fio')
+
+path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock')
+
+test:plan(1)
+local function start_server()
+    test:diag("Create unix socket at %s", path)
+    local unix_socket = socket('AF_UNIX', 'SOCK_DGRAM',0)
+    unix_socket:bind('unix/', path)
+    socket.tcp_connect('unix', path)
+    return unix_socket
+end
+
+local function find_log_str (test, socket)
+	local buf
+	local opt = string.format("syslog:server=unix:%s,identity=tarantool", path)
+	print (opt)
+	box.cfg{log = opt, log_level = 5}
+	log.info("Test remote syslog destination")
+	buf = socket:read("Test remote syslog destination", 30)
+	if buf ~= nil then
+		test:ok(buf:match('Test remote syslog destination'), "syslog_unix")
+	else
+		test:fail("syslog_unix")
+	end
+end
+
+sock = start_server()
+
+test:test('syslog_unix',find_log_str(test, sock))
+sock:close()
+os.remove(path)
+os.exit(test:check() == true)
-- 
2.14.3 (Apple Git-98)

Re: [tarantool-patches] [PATCH v2 1/3] Configurable syslog destination

[Vladimir Davydov]

On Tue, Jul 17, 2018 at 12:02:49PM +0300, Olga Arkhangelskaia wrote:
> Added server option to syslog configuration.
> Server option is responsible for log destination. At the momemt
> there is two ways of usage:server=unix:/path/to/socket or
> server=ipv4:port. If port is not set default udp port 514 is used.
> If logging to syslog is set, however there is no sever options -
> default location is used: Linux /dev/log and Mac /var/run/syslog.
> 
> Issue: #3487
> ---
> https://github.com/tarantool/tarantool/issues/3487
> 
> Branch:https: OKriw/gh-3487-syslog-conf-dest
> ---
> https://github.com/tarantool/tarantool/tree/OKriw/gh-3487-syslog-conf-dest
> 
> gh-3487-syslog-conf-dest 

Please see my reply to your other patch regarding how to format patches
for review.

>  src/say.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  src/say.h |  14 ++++++++-
>  2 files changed, 112 insertions(+), 8 deletions(-)
> 
> diff --git a/src/say.c b/src/say.c
> index 43124083c..50416c08d 100644
> --- a/src/say.c
> +++ b/src/say.c
> @@ -36,6 +36,7 @@
>  #include <stdarg.h>
>  #include <stdio.h>
>  #include <string.h>
> +#include <netdb.h>
>  #include <sys/types.h>
>  #include <unistd.h>
>  #include <fcntl.h>
> @@ -52,6 +53,8 @@ enum say_format log_format = SF_PLAIN;
>  /** List of logs to rotate */
>  static RLIST_HEAD(log_rotate_list);
>  
> +static const char syslog_syntax_remainder[] =

s/remainder/reminder

> +        "expecting syslog: or syslog:server=ip:potr or syslog:server=unix:/path";

s/potr/port

Please enable spell checker.

>  static const char logger_syntax_reminder[] =
>  	"expecting a file name or a prefix, such as '|', 'pipe:', 'syslog:'";
>  /**
> @@ -473,16 +476,80 @@ syslog_connect_unix(const char *path)
>  	return fd;
>  }
>  
> +/**
> + * Connect to remote syslogd using server:port.
> + * @param ip address of remote host.
> + * @param port on remote host.

This function now takes a single argument. Please fix the comment.

> + * @retval not 0 Socket descriptor.
> + * @retval    -1 Socket error.
> + */
> +static int
> +syslog_connect_remote(const char *server_address)
> +{
> +	/* IPv4 */

This comment is obviously pointless. Please remove.

> +	struct addrinfo *inf, hints, *ptr;
> +	const char *remote;
> +	char *portnum, *copy;
> +	int fd = -1;

JFYI: we're OK with defining variables anywhere in the code
(in the middle of a function, in the body of a loop, etc).

> +
> +	copy = strdup(server_address);
> +	if (copy == NULL) {
> +		diag_set(OutOfMemory, strlen(server_address), "malloc",
> +			 copy);

OutOfMemory error takes two strings - allocator and object names, e.g.

		diag_set(OutOfMemory, strlen(server_address) + 1,
			 "malloc", "syslog server address");

> +		return fd;
> +	}
> +	portnum = copy;
> +	remote = strsep(&portnum, ":");
> +	if (!remote) {

Nit: in tarantool we always compare pointers to NULL explicitly
(don't ask me why - I'm not the one who invented the rules :-):

	if (remote == NULL) {

> +		diag_set(IllegalParams,syslog_syntax_remainder);
> +		free(copy);
> +		return fd;
> +	}

I don't think we should fail in case the port is not specified. Let's
use some default port instead. BTW in the commit message, you wrote that
you use port 514 by default.

> +	memset(&hints, 0, sizeof(hints));
> +	hints.ai_family = AF_INET;
> +	hints.ai_socktype = SOCK_DGRAM;
> +	hints.ai_protocol = IPPROTO_UDP;
> +	hints.ai_flags = AI_DEFAULT;

This doesn't compile on Linux, because AI_DEFAULT is undefined
(see travis). Please fix.

> +
> +	if (getaddrinfo(remote, portnum, NULL, &inf) < 0) {
> +		free(copy);
> +		return -1;
> +	}
> +	for (ptr = inf; ptr; ptr = ptr->ai_next) {
> +		fd = socket(ptr->ai_family, ptr->ai_socktype, ptr->ai_protocol);
> +		if (fd < 0) {
> +			continue;
> +		}
> +
> +		if (connect(fd, inf->ai_addr, inf->ai_addrlen) != 0) {
> +			close(fd);
> +			fd = -1;
> +			continue;
> +		}
> +		break;
> +	}
> +	freeaddrinfo(inf);
> +	free(copy);
> +	return fd;
> +}
> +
>  static inline int
>  log_syslog_connect(struct log *log)
>  {
> +
>  	/*
> -	 * Try two locations: '/dev/log' for Linux and
> +	 * If server option is not set use '/dev/log' for Linux and
>  	 * '/var/run/syslog' for Mac.
>  	 */
> -	log->fd = syslog_connect_unix("/dev/log");
> -	if (log->fd < 0)
> -		log->fd = syslog_connect_unix("/var/run/syslog");
> +	if (log->server_type == SAY_SYSLOG_LOCAL) {
> +		log->fd = syslog_connect_unix("/dev/log");
> +			if (log->fd < 0)

Malformed indentation. Please fix.

> +				log->fd = syslog_connect_unix("/var/run/syslog");
> +	} else if (log->server_type == SAY_SYSLOG_UNIX) {
> +		log->fd = syslog_connect_unix(log->path);
> +	} else {
> +		log->fd = syslog_connect_remote(log->path);
> +	}

Nit: switch-case would be more appropriate here IMHO.

>  	return log->fd;
>  }
>  
> @@ -498,6 +565,17 @@ log_syslog_init(struct log *log, const char *init_str)
>  	if (say_parse_syslog_opts(init_str, &opts) < 0)
>  		return -1;
>  
> +	log->server_type = opts.syslog_server;
> +	if (log->server_type == SAY_SYSLOG_LOCAL)
> +		log->path = NULL;
> +	else {
> +		log->path = strdup(opts.server);
> +			if (log->path == NULL) {

Malformed indentation. Please fix.

> +				diag_set(OutOfMemory, strlen(init_str),

Should be strlen(opts.server) I guess.

> +				         "malloc", "log->path");

Nit: s/"log->path"/"server address"

> +			return -1;
> +		}
> +	}
>  	if (opts.identity == NULL)
>  		log->syslog_ident = strdup("tarantool");
>  	else
> @@ -1044,6 +1122,8 @@ say_syslog_facility_by_name(const char *facility)
>  int
>  say_parse_syslog_opts(const char *init_str, struct say_syslog_opts *opts)
>  {
> +	opts->server = NULL;
> +	opts->syslog_server = SAY_SYSLOG_LOCAL;
>  	opts->identity = NULL;
>  	opts->facility = syslog_facility_MAX;
>  	opts->copy = strdup(init_str);
> @@ -1051,8 +1131,8 @@ say_parse_syslog_opts(const char *init_str, struct say_syslog_opts *opts)
>  		diag_set(OutOfMemory, strlen(init_str), "malloc", "opts->copy");
>  		return -1;
>  	}
> -	char *ptr = opts->copy;
> -	const char *option, *value;
> +	char *ptr = opts->copy; 
> +	const char *option, *value, *srv_str;
>  
>  	/* strsep() overwrites the separator with '\0' */
>  	while ((option = strsep(&ptr, ","))) {
> @@ -1060,7 +1140,19 @@ say_parse_syslog_opts(const char *init_str, struct say_syslog_opts *opts)
>  			break;
>  
>  		value = option;
> -		if (say_parse_prefix(&value, "identity=")) {
> +		srv_str = say_parse_prefix(&value, "server=");
> +		if (srv_str != NULL) {

I'd rewrite this code without introducing a new local variable
'srv_str': say_parse_prefix advances value so you might as well
write:

		if (say_parse_prefix(&value, "server=")) {
			...
			opts->server = value;
		}

This would also be consistent with the code below.

> +			if (opts->server != NULL ||
> +			    opts->syslog_server != SAY_SYSLOG_LOCAL)
> +				goto duplicate;
> +			if (say_parse_prefix(&srv_str, "unix:")) {
> +				opts->syslog_server = SAY_SYSLOG_UNIX;
> +				opts->server = srv_str;
> +			} else {
> +				opts->syslog_server = SAY_SYSLOG_REMOTE;
> +				opts->server = srv_str;
> +			}
> +		} else if (say_parse_prefix(&value, "identity=")) {
>  			if (opts->identity != NULL)
>  				goto duplicate;
>  			opts->identity = value;
> diff --git a/src/say.h b/src/say.h
> index ad3ba3417..a1b6c2331 100644
> --- a/src/say.h
> +++ b/src/say.h
> @@ -79,6 +79,12 @@ say_log_level_is_enabled(int level)
>  
>  extern enum say_format log_format;
>  
> +enum say_logger_syslog_server {

Why not simply say_syslog_server?

> +	SAY_SYSLOG_LOCAL,

IMHO SAY_SYSLOG_LOCAL is kinda confusing, because SAY_SYSLOG_UNIX is
also sorta local. What about SAY_SYSLOG_DEFAULT?

> +	SAY_SYSLOG_UNIX,
> +	SAY_SYSLOG_REMOTE
> +};
> +
>  enum say_logger_type {
>  	/**
>  	 * Before the app server core is initialized, we do not
> @@ -141,7 +147,10 @@ struct log {
>  	/** The current log level. */
>  	int level;
>  	enum say_logger_type type;
> -	/** path to file if logging to file. */
> +	enum say_logger_syslog_server server_type;
> +	/** path to file if logging to file
> +	 * or server address in case of syslog
> +	 * */

Bad comment formatting. Please fix. BTW we always start comments with
a capital letter and end them with a dot. Kostja is very serious about
that.

>  	char *path;
>  	bool nonblock;
>  	log_format_func_t format_func;
> @@ -162,6 +171,7 @@ struct log {
>  	int rotating_threads;
>  	enum syslog_facility syslog_facility;
>  	struct rlist in_log_list;
> +	/* Server options. Either ip/port pair or unix socket address.*/

Stray comment. Please remove.

>  };
>  
>  /**
> @@ -390,6 +400,8 @@ say_parse_logger_type(const char **str, enum say_logger_type *type);
>  
>  /** Syslog logger initialization params */
>  struct say_syslog_opts {
> +	enum say_logger_syslog_server syslog_server;
> +	const char *server;
>  	const char *identity;
>  	enum syslog_facility facility;
>  	/* Input copy (content unspecified). */

Re: [tarantool-patches] [PATCH v2 2/3] Syslog remote destination test

[Vladimir Davydov]

On Tue, Jul 17, 2018 at 12:02:50PM +0300, Olga Arkhangelskaia wrote:
> Adds syslog remote destination test in app-tap.
> Test creates server, sets appropriate log level and redirects logs to it.
> If log level received by the server is the same - test passes.
> 

> Usage: test-run.py syslog

This line is pointless. We know how to run tests :-) Please remove.

> 
> Issue: #3487
> ---
> https://github.com/tarantool/tarantool/issues/3487
> 
> Branch: OKriw/gh-3487-syslog-conf-dest 
> ---
> https://github.com/tarantool/tarantool/tree/OKriw/gh-3487-syslog-conf-dest

Please see my reply to your other patch regarding patch formatting.

>  test/app-tap/syslog_remote.test.lua | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100755 test/app-tap/syslog_remote.test.lua

In review to v1 I asked you to fold this test in box-tap/cfg.
Any reason why this couldn't/shouldn't be done?

> 
> diff --git a/test/app-tap/syslog_remote.test.lua b/test/app-tap/syslog_remote.test.lua
> new file mode 100755
> index 000000000..01a75fa64
> --- /dev/null
> +++ b/test/app-tap/syslog_remote.test.lua
> @@ -0,0 +1,37 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local socket = require('socket')
> +local os = require('os')
> +local test = tap.test("syslog_remote")
> +local log = require('log')
> +local errno = require('errno')
> +
> +local addr = '127.0.0.1'
> +local port = 1000 + math.random(32768)

No, that's not what I meant. Using a random port like that won't save
your from conflicts. You should retry until 'bind' succeeds.

> +
> +test:plan(1)
> +local function start_server()
> +	test:diag("Starting server %s:%u", addr, port)
> +	local sc = socket('AF_INET', 'SOCK_DGRAM', 'udp')
> +	sc:bind(addr, port)
> +	return sc
> +end
> +
> +local function find_log_str()

Nit: I'd just inlined both of these functions.

> +	local opt = string.format("syslog:server=%s:%u,identity=tarantool", addr, port)
> +	local res = false
> +	box.cfg{log = opt, log_level = 5}
> +	log.info('Test syslog destination')
> +	while (sc:readable(1)) do
> +		local buf = sc:recv(1000)
> +		print (buf)
> +		res = buf:match('Test syslog destination')
> +	end
> +		test:ok(res, "syslog_remote")
> +end

Bad indentation. We use 4 spaces in Lua, never tabs. Please configure
your editor appropriately and fix this test.

> +
> +sc = start_server()
> +test:test('syslog_remote',find_log_str(test))
> +sc:close()
> +os.exit(test:check() == true)

Re[2]: [tarantool-patches] [PATCH v2 2/3] Syslog remote destination test

[Olga Arkhangelskaia]

[-- Attachment #1: Type: text/plain, Size: 2847 bytes --]




>Вторник, 17 июля 2018, 17:07 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:
>
>On Tue, Jul 17, 2018 at 12:02:50PM +0300, Olga Arkhangelskaia wrote:
>> Adds syslog remote destination test in app-tap.
>> Test creates server, sets appropriate log level and redirects logs to it.
>> If log level received by the server is the same - test passes.
>> 
>
>> Usage: test-run.py syslog
>
>This line is pointless. We know how to run tests :-) Please remove. 
ok
>
>
>> 
>> Issue: #3487
>> ---
>>  https://github.com/tarantool/tarantool/issues/3487
>> 
>> Branch: OKriw/gh-3487-syslog-conf-dest 
>> ---
>>  https://github.com/tarantool/tarantool/tree/OKriw/gh-3487-syslog-conf-dest
>
>Please see my reply to your other patch regarding patch formatting.
>
>>  test/app-tap/syslog_remote.test.lua | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>  create mode 100755 test/app-tap/syslog_remote.test.lua
>
>In review to v1 I asked you to fold this test in box-tap/cfg.
>Any reason why this couldn't/shouldn't be done?
Sorry, I did not notice that comment. However, why in  box-tap/cfg.test.lua and not separately?

>
>> 
>> diff --git a/test/app-tap/syslog_remote.test.lua b/test/app-tap/syslog_remote.test.lua
>> new file mode 100755
>> index 000000000..01a75fa64
>> --- /dev/null
>> +++ b/test/app-tap/syslog_remote.test.lua
>> @@ -0,0 +1,37 @@
>> +#!/usr/bin/env tarantool
>> +
>> +local tap = require('tap')
>> +local socket = require('socket')
>> +local os = require('os')
>> +local test = tap.test("syslog_remote")
>> +local log = require('log')
>> +local errno = require('errno')
>> +
>> +local addr = '127.0.0.1'
>> +local port = 1000 + math.random(32768)
>
>No, that's not what I meant. Using a random port like that won't save
>your from conflicts. You should retry until 'bind' succeeds. 
And how to choose number of attempts? Ten is enough?
>
>
>> +
>> +test:plan(1)
>> +local function start_server()
>> +	test:diag("Starting server %s:%u", addr, port)
>> +	local sc = socket('AF_INET', 'SOCK_DGRAM', 'udp')
>> +	sc:bind(addr, port)
>> +	return sc
>> +end
>> +
>> +local function find_log_str()
>
>Nit: I'd just inlined both of these functions.
>
>> +	local opt = string.format("syslog:server=%s:%u,identity=tarantool", addr, port)
>> +	local res = false
>> +	box.cfg{log = opt, log_level = 5}
>> +	log.info('Test syslog destination')
>> +	while (sc:readable(1)) do
>> +		local buf = sc:recv(1000)
>> +		print (buf)
>> +		res = buf:match('Test syslog destination')
>> +	end
>> +		test:ok(res, "syslog_remote")
>> +end
>
>Bad indentation. We use 4 spaces in Lua, never tabs. Please configure
>your editor appropriately and fix this test.
>
>> +
>> +sc = start_server()
>> +test:test('syslog_remote',find_log_str(test))
>> +sc:close()
>> +os.exit(test:check() == true)


-- 
Olga Arkhangelskaia

[-- Attachment #2: Type: text/html, Size: 4540 bytes --]

Re: [tarantool-patches] [PATCH v2 2/3] Syslog remote destination test

[Vladimir Davydov]

On Tue, Jul 17, 2018 at 05:15:24PM +0300, Olga Arkhangelskaia wrote:
> >>  test/app-tap/syslog_remote.test.lua | 37 +++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 37 insertions(+)
> >>  create mode 100755 test/app-tap/syslog_remote.test.lua
> >
> >In review to v1 I asked you to fold this test in box-tap/cfg.
> >Any reason why this couldn't/shouldn't be done?
> Sorry, I did not notice that comment. However, why in  box-tap/cfg.test.lua and not separately?

Hmm, good question. For one thing, what's you're testing here is not a
part of the application server, but is rather relevant to the box API so
the test should live in box/ or box-tap/. Since box-tap/cfg.test.lua is
supposed to test box.cfg() and already has all the infrastructure for
starting a new tarantool instance (I mean, run_code), it feels like a
good choice.

Anyway, we don't usually create a new test file for each bug/feature -
that'd be an overkill. We try to group tests by subsystem. I suppose, in
your particular case it would be acceptable to introduce a new test
file, but it should test both unix and udp sockets then IMHO, which
means that you need the run_code() from box-tap/cfg.test.lua, so why not
just fold the test cases there - they aren't that big, are they.

> 
> >
> >> 
> >> diff --git a/test/app-tap/syslog_remote.test.lua b/test/app-tap/syslog_remote.test.lua
> >> new file mode 100755
> >> index 000000000..01a75fa64
> >> --- /dev/null
> >> +++ b/test/app-tap/syslog_remote.test.lua
> >> @@ -0,0 +1,37 @@
> >> +#!/usr/bin/env tarantool
> >> +
> >> +local tap = require('tap')
> >> +local socket = require('socket')
> >> +local os = require('os')
> >> +local test = tap.test("syslog_remote")
> >> +local log = require('log')
> >> +local errno = require('errno')
> >> +
> >> +local addr = '127.0.0.1'
> >> +local port = 1000 + math.random(32768)
> >
> >No, that's not what I meant. Using a random port like that won't save
> >your from conflicts. You should retry until 'bind' succeeds. 
> And how to choose number of attempts? Ten is enough?

I guess so. Even if you have half of the ports busy, after ten attempts
the probability of repeatedly choosing a busy port is less than 0.1%,
which is acceptable, I think.

Also, I asked you to test invalid syslog destination (non-existing unix
socket or host address). Please do so as to cover the error paths.