From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 31 Jul 2018 15:45:17 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH v4 1/4] Configurable syslog destination Message-ID: <20180731124517.c32i4tvxqribouvj@esperanza> References: <20180731101833.55671-1-arkholga@tarantool.org> <20180731101833.55671-2-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180731101833.55671-2-arkholga@tarantool.org> To: Olga Arkhangelskaia Cc: tarantool-patches@freelists.org List-ID: On Tue, Jul 31, 2018 at 01:18:30PM +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. > > Closes #3487 > --- > src/say.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > src/say.h | 13 +++++++- > 2 files changed, 110 insertions(+), 6 deletions(-) > > diff --git a/src/say.c b/src/say.c > index ac221dd19..526b8862f 100644 > --- a/src/say.c > +++ b/src/say.c > @@ -45,6 +46,7 @@ > #include > #include > > +#define DEFAULT_PORT 514 We don't use macros when we can define a constant in a enum, e.g. enum { DEFAULT_PORT = 512 }; Also, DEFAULT_PORT is a too generic name. I think you should prefix it with SAY_SYSLOG_. > pid_t log_pid = 0; > int log_level = S_INFO; > enum say_format log_format = SF_PLAIN; > @@ -473,16 +475,85 @@ syslog_connect_unix(const char *path) > return fd; > } > > +/** > + * Connect to remote syslogd using server:port. > + * @param server address string of remote host. _ is missing between 'server' and 'address'. > + * @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; > + > + 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", DEFAULT_PORT); > + portnum = buf; > + } > + memset(&hints, 0, sizeof(hints)); > + hints.ai_family = AF_INET; > + hints.ai_socktype = SOCK_DGRAM; > + hints.ai_protocol = IPPROTO_UDP; > + > + if (getaddrinfo(remote, (const char*)portnum, NULL, &inf) < 0) { > + diag_set(SystemError, "syslog: getaddrinfo failed %s", > + gai_strerror(errno)); First, getaddrinfo() doesn't set errno. You are supposed to call gai_strerror() on the code it returned. Second, SystemError always appends strerror(errno) to the error message. Since getaddrinfo() doesn't do that, you should set errno manually. EIO seems to be a good choice. Take a look at coio_getaddrinfo() for an example. Third, there's no point to prepend the error message with "syslog:". diag_set() includes file name and string to the error message so finding a culprit shouldn't be a problem. > + 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, "syslog: socket failed %s", > + strerror(errno)); Again. No point to prepend "syslog:" and append strerror(errno). > + continue; > + } > + if (connect(fd, inf->ai_addr, inf->ai_addrlen) != 0) { > + close(fd); > + fd = -1; > + diag_set(SystemError, "syslog:connect failed %s", > + strerror(errno)); Ditto. > + continue; > + } > + break; > + } > + freeaddrinfo(inf); > +out: > + free(copy); > + return fd; > +} > diff --git a/src/say.h b/src/say.h > index 2c2395fe0..73697b1b7 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 { Let's rename this to 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,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; Missing comment to 'server_type'. Also, let's prefix it with 'syslog_', like other syslog-related options (syslog_identity, syslog_facility). > + /** Path to file if logging to file, socket > + * or server address in case of syslog. > + */ /** * Malformed comment format. * Multi-line comments should look like this. */