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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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