Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Olga Arkhangelskaia <arkholga@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [tarantool-patches] [PATCH v6] Configurable syslog destination
Date: Wed, 1 Aug 2018 18:36:48 +0300	[thread overview]
Message-ID: <20180801153648.wn26gmh66jsvwgx5@esperanza> (raw)
In-Reply-To: <20180801150050.20937-1-arkholga@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.

      reply	other threads:[~2018-08-01 15:36 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-01 15:00 Olga Arkhangelskaia
2018-08-01 15:36 ` Vladimir Davydov [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180801153648.wn26gmh66jsvwgx5@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=arkholga@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [tarantool-patches] [PATCH v6] Configurable syslog destination' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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