From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 1 Aug 2018 18:36:48 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] [PATCH v6] Configurable syslog destination
Message-ID: <20180801153648.wn26gmh66jsvwgx5@esperanza>
References: <20180801150050.20937-1-arkholga@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180801150050.20937-1-arkholga@tarantool.org>
To: Olga Arkhangelskaia <arkholga@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

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.