[tarantool-patches] [PATCH v6] Configurable syslog destination

[tarantool-patches] [PATCH v6] 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.

To monitor that the option is valid there is two tests in box-tap.

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

v1:
https://www.freelists.org/post/tarantool-patches/PATCH-03-Syslog-destination

v2:
https://www.freelists.org/post/tarantool-patches/PATCH-v2-03-Syslog-destination

v3:
https://www.freelists.org/post/tarantool-patches/PATCH-v3-04-Syslog-destination

v4:
https://www.freelists.org/post/tarantool-patches/PATCH-v4-04-Syslog-destination

v5:
https://www.freelists.org/post/tarantool-patches/PATCH-v5-04-Syslog-destination

Changes in 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->pathi

Changes in v3:
- moved test to box-tap
- used pcall in case of stopped/unconfigured syslogd
- added syslog check syslog configuration test 

Changes in v4:
- fixed memory leak
- changed variables name
- removed unnecessary libs in tests
- removed pcall
- fixed identation   

Changes in v5:
-changed variable names
-fixed indentation
-changed test unix socket, now it looks the same as remote test
-fixed spelling error

Changes in v6:
-squashed patches into one
-deleted default syslog config. test
-fixed indentation 

 src/say.c                 | 104 +++++++++++++++++++++++++++++++++++++++++++---
 src/say.h                 |  15 ++++++-
 test/box-tap/cfg.test.lua |  67 ++++++++++++++++++++++++++++-
 3 files changed, 179 insertions(+), 7 deletions(-)

diff --git a/src/say.c b/src/say.c
index ac221dd19..8fe38e32d 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>
@@ -48,6 +49,7 @@
 pid_t log_pid = 0;
 int log_level = S_INFO;
 enum say_format log_format = SF_PLAIN;
+enum { SAY_SYSLOG_DEFAULT_PORT = 512 };
 
 /** List of logs to rotate */
 static RLIST_HEAD(log_rotate_list);
@@ -473,16 +475,86 @@ syslog_connect_unix(const char *path)
 	return fd;
 }
 
+/**
+ * Connect to remote syslogd using server:port.
+ * @param server_address string of remote host.
+ * @retval not 0 Socket descriptor.
+ * @retval    -1 Socket error.
+ */
+static int
+syslog_connect_remote(const char *server_address)
+{
+	struct addrinfo *inf, hints, *ptr;
+	const char *remote;
+	char buf[10];
+	char *portnum, *copy;
+	int fd = -1;
+	int ret;
+
+	copy = strdup(server_address);
+	if (copy == NULL) {
+		diag_set(OutOfMemory, strlen(server_address), "malloc",
+			 "stslog server address");
+		return fd;
+	}
+	portnum = copy;
+	remote = strsep(&portnum, ":");
+	if (portnum == NULL) {
+		snprintf(buf, sizeof(buf), "%d", SAY_SYSLOG_DEFAULT_PORT);
+		portnum = buf;
+	}
+	memset(&hints, 0, sizeof(hints));
+	hints.ai_family = AF_INET;
+	hints.ai_socktype = SOCK_DGRAM;
+	hints.ai_protocol = IPPROTO_UDP;
+
+	ret = getaddrinfo(remote, (const char*)portnum, NULL, &inf);
+	if (ret < 0) {
+		errno = EIO;
+		diag_set(SystemError, "getaddrinfo:%s",
+			 gai_strerror(ret));
+		goto out;
+	}
+	for (ptr = inf; ptr; ptr = ptr->ai_next) {
+		fd = socket(ptr->ai_family, ptr->ai_socktype, ptr->ai_protocol);
+		if (fd < 0) {
+			diag_set(SystemError, "socket");
+			continue;
+		}
+		if (connect(fd, inf->ai_addr, inf->ai_addrlen) != 0) {
+			close(fd);
+			fd = -1;
+			diag_set(SystemError, "connect");
+			continue;
+		}
+		break;
+	}
+	freeaddrinfo(inf);
+out:
+	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");
+	switch (log->syslog_server_type) {
+	case SAY_SYSLOG_UNIX:
+		log->fd = syslog_connect_unix(log->path);
+		break;
+	case SAY_SYSLOG_REMOTE:
+		log->fd = syslog_connect_remote(log->path);
+		break;
+	default:
+		log->fd = syslog_connect_unix("/dev/log");
+		if (log->fd < 0)
+			log->fd = syslog_connect_unix("/var/run/syslog");
+
+	}
 	return log->fd;
 }
 
@@ -498,6 +570,15 @@ log_syslog_init(struct log *log, const char *init_str)
 	if (say_parse_syslog_opts(init_str, &opts) < 0)
 		return -1;
 
+	log->syslog_server_type = opts.server_type;
+	if (log->syslog_server_type != SAY_SYSLOG_DEFAULT) {
+		log->path = strdup(opts.server_path);
+		if (log->path == NULL) {
+			diag_set(OutOfMemory, strlen(opts.server_path),
+				 "malloc", "server address");
+			return -1;
+		}
+	}
 	if (opts.identity == NULL)
 		log->syslog_ident = strdup("tarantool");
 	else
@@ -1031,6 +1112,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_path = NULL;
+	opts->server_type = SAY_SYSLOG_DEFAULT;
 	opts->identity = NULL;
 	opts->facility = syslog_facility_MAX;
 	opts->copy = strdup(init_str);
@@ -1047,7 +1130,18 @@ say_parse_syslog_opts(const char *init_str, struct say_syslog_opts *opts)
 			break;
 
 		value = option;
-		if (say_parse_prefix(&value, "identity=")) {
+		if (say_parse_prefix(&value, "server=")) {
+			if (opts->server_path != NULL ||
+			    opts->server_type != SAY_SYSLOG_DEFAULT)
+				goto duplicate;
+			if (say_parse_prefix(&value, "unix:")) {
+				opts->server_type = SAY_SYSLOG_UNIX;
+				opts->server_path = value;
+			} else {
+				opts->server_type = SAY_SYSLOG_REMOTE;
+				opts->server_path = value;
+			}
+		} 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 2c2395fe0..3cff02e96 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_syslog_server_type {
+	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,12 @@ struct log {
 	/** The current log level. */
 	int level;
 	enum say_logger_type type;
-	/** path to file if logging to file. */
+	/* Type of syslog destination. */
+	enum say_syslog_server_type syslog_server_type;
+	/** 
+	 * Path to file if logging to file, socket
+	 * or server address in case of syslog.
+	 */
 	char *path;
 	bool nonblock;
 	log_format_func_t format_func;
@@ -384,6 +395,8 @@ say_parse_logger_type(const char **str, enum say_logger_type *type);
 
 /** Syslog logger initialization params */
 struct say_syslog_opts {
+	enum say_syslog_server_type server_type;
+	const char *server_path;
 	const char *identity;
 	enum syslog_facility facility;
 	/* Input copy (content unspecified). */
diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index ffafdbe42..b1bbe79fe 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(95)
+test:plan(97)
 
 --------------------------------------------------------------------------------
 -- Invalid values
@@ -464,5 +464,70 @@ code = string.format(code_fmt, dir, instance_uuid, uuid.new())
 test:is(run_script(code), PANIC, "replicaset_uuid mismatch")
 fio.rmdir(dir)
 
+--
+-- Check syslog unix socket configuration
+--
+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=unix:%s,identity=tarantool", path)
+local res = 1
+local buf = 'Started\n'
+box.cfg{log = opt, log_level = 5}
+log.info("Test socket syslog destination")
+while unix_socket:readable(1) do
+    buf = buf .. unix_socket:recv(1000)
+    if buf:match('Test socket syslog destination') then res = 0 end
+end
+
+unix_socket:close()                                                         
+os.remove(path) 
+os.exit(res)
+]]
+test:is(run_script(code), 0, "unix socket syslog log configuration")
+
+--
+-- Check syslog remote configuration
+--
+code = [[
+local socket = require('socket')
+local log = require('log')
+
+addr = '127.0.0.1'
+port = 1000 + math.random(32768)
+
+sc = socket('AF_INET', 'SOCK_DGRAM', 'udp')
+local attempt = 0
+while attempt < 10 do
+    if not sc:bind (addr, port) then
+        port = 1000 + math.random(32768)
+        attempt = attempt + 1
+    else
+        break
+    end
+end
+sc:bind(addr, port)
+
+local opt = string.format("syslog:server=%s:%u,identity=tarantool", addr, port)
+local res = 1
+local buf = 'Started\n'
+box.cfg{log = opt, log_level = 5}
+log.info('Test syslog destination')
+while sc:readable(1) do
+    buf = buf .. sc:recv(1000)
+    if buf:match('Test syslog destination') then res = 0 end
+end
+
+sc:close()
+os.exit(res)
+]]
+test:is(run_script(code), 0, "remote syslog log configuration")
+
 test:check()
 os.exit(0)
-- 
2.14.3 (Apple Git-98)

Re: [tarantool-patches] [PATCH v6] Configurable syslog destination

[Vladimir Davydov]

On Wed, Aug 01, 2018 at 06:00:50PM +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 -

A typo: s/sever/server

> default location is used: Linux /dev/log and Mac /var/run/syslog.
> 
> To monitor that the option is valid there is two tests in box-tap.

This sentence is pointless. It is incumbent on the developer to write
a functional test for each implemented feature. No point to mention it
in the commit message. We only mention tests in the commit message if
there's something peculiar about them. Please remove this sentence
from the commit message.

> +/**
> + * Connect to remote syslogd using server:port.
> + * @param server_address string of remote host.

Please replace 'string' with 'address' in the comment
("string of remote host" doesn't sound right).

> + * @retval not 0 Socket descriptor.
> + * @retval    -1 Socket error.
> + */
> +static int
> +syslog_connect_remote(const char *server_address)
> +{
> +	struct addrinfo *inf, hints, *ptr;
> +	const char *remote;
> +	char buf[10];
> +	char *portnum, *copy;
> +	int fd = -1;
> +	int ret;
> +
> +	copy = strdup(server_address);
> +	if (copy == NULL) {
> +		diag_set(OutOfMemory, strlen(server_address), "malloc",
> +			 "stslog server address");
> +		return fd;
> +	}
> +	portnum = copy;
> +	remote = strsep(&portnum, ":");
> +	if (portnum == NULL) {
> +		snprintf(buf, sizeof(buf), "%d", SAY_SYSLOG_DEFAULT_PORT);
> +		portnum = buf;
> +	}
> +	memset(&hints, 0, sizeof(hints));
> +	hints.ai_family = AF_INET;
> +	hints.ai_socktype = SOCK_DGRAM;
> +	hints.ai_protocol = IPPROTO_UDP;

'hints' are initialized, but never used (I assume they are supposed to
be passed to getaddrinfo below).

> +
> +	ret = getaddrinfo(remote, (const char*)portnum, NULL, &inf);

A space is missing between 'const char' and the asterisk (*). Anyway,
there's no need to convert 'portnum' to 'const char *', neither do you
need to define 'remote' as 'const char *'. Please remove those const
classifiers.

> +	if (ret < 0) {
> +		errno = EIO;
> +		diag_set(SystemError, "getaddrinfo:%s",

A space is missing between the colon (:) and '%s'.

Also, the issue is assigned to 1.10.2 (see the ticket) and hence is
supposed to be pushed to branch 1.10 while your branch is based on 2.0.
Please rebase on top of 1.10 and push - I want to make sure that tests
pass on Travis CI before pushing it to the trunk.