From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 25 Jul 2018 19:27:32 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH v3 1/4] Configurable syslog destination Message-ID: <20180725162732.k2gca2k3bxf5b4vb@esperanza> References: <20180724171152.24921-1-arkholga@tarantool.org> <20180724171152.24921-2-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180724171152.24921-2-arkholga@tarantool.org> To: Olga Arkhangelskaia Cc: tarantool-patches@freelists.org List-ID: On Tue, Jul 24, 2018 at 08:11: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. > > Closes #3487 > --- > src/say.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- > src/say.h | 13 +++++++- > 2 files changed, 113 insertions(+), 7 deletions(-) > > diff --git a/src/say.c b/src/say.c > index ac221dd19..b4836d9ee 100644 > --- a/src/say.c > +++ b/src/say.c > @@ -36,6 +36,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -473,16 +474,86 @@ syslog_connect_unix(const char *path) > return fd; > } > > +/** > + * Connect to remote syslogd using server:port. > + * @param ip:port address string of remote host. This function now takes a single argument, 'server_address'. Please fix the comment. > + * @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 *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 (remote == NULL) { > + remote = strdup("514"); The default port should be defined as a numeric constant and converted to a string here with snprintf(). Also, shouldn't you use 'portnum' instead of 'remote' here? Also, it looks like you leak 'remote' in this function. Why don't you use a buffer on stack for storing port number? > + if (remote == NULL) { > + diag_set(OutOfMemory, strlen(remote), "strdup", > + "port"); > + free(copy); > + return fd; Please use labels to free 'copy' (like 'goto out', 'out' points to the 'free(copy); return fd;' at the end of this function). That would look neater IMHO. > + } > + } > + memset(&hints, 0, sizeof(hints)); > + hints.ai_family = AF_INET; > + hints.ai_socktype = SOCK_DGRAM; > + hints.ai_protocol = IPPROTO_UDP; > + hints.ai_flags = 0; Nit: this line is pointless as you zero the whole struct anyway. > + > + if (getaddrinfo(remote, portnum, NULL, &inf) < 0) { You should set diag here. Take a look at SystemError. > + 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) { ... and here > + continue; > + } > + > + if (connect(fd, inf->ai_addr, inf->ai_addrlen) != 0) { ... and here, too > + close(fd); > + fd = -1; > + continue; > + } > + break; > + } > + freeaddrinfo(inf); > + free(copy); > + return fd; > +} > + > static inline int > log_syslog_connect(struct log *log) > { > + Nit: please don't add an empty line here. > /* > - * 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->server_type) { > + case SAY_SYSLOG_UNIX: Malformed indentation: 'switch' and 'case' should be at the same level. > + 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 +569,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_DEFAULT) > + log->path = NULL; Nit: no need to set 'log->path' to NULL - it's already been done by 'log_create'. > + else { > + log->path = strdup(opts.server); > + if (log->path == NULL) { Malformed indentation. > + diag_set(OutOfMemory, strlen(opts.server), > + "malloc", "server address"); > + return -1; > + } > + } > if (opts.identity == NULL) > log->syslog_ident = strdup("tarantool"); > else > @@ -1031,6 +1113,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_DEFAULT; Hmm, 'server', 'syslog_server', this looks weird... May be, 'server_path' and 'server_type' or simply 'path' and 'type'? > opts->identity = NULL; > opts->facility = syslog_facility_MAX; > opts->copy = strdup(init_str); > @@ -1038,7 +1122,7 @@ 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; > + char *ptr = opts->copy; What's this doing? Adding a space at the end of the line? Please remove. > const char *option, *value; > > /* strsep() overwrites the separator with '\0' */ > @@ -1047,7 +1131,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 != NULL || > + opts->syslog_server != SAY_SYSLOG_DEFAULT) > + goto duplicate; > + if (say_parse_prefix(&value, "unix:")) { > + opts->syslog_server = SAY_SYSLOG_UNIX; > + opts->server = value; > + } else { > + opts->syslog_server = SAY_SYSLOG_REMOTE; > + opts->server = 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..6681aae98 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 { This sounds awkward. May be, say_syslog_server or say_syslog_server_type or simply say_syslog_type? Whatever name you choose, please be consistent throughout the code. That is, if you choose to call the enum say_syslog_type, then other names should be log->syslog_type and say_syslog_opts->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; Let's prefix this 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 style. > char *path; > bool nonblock; > log_format_func_t format_func; > @@ -384,6 +393,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; Here you don't need syslog_ prefix, because the struct is already named after syslog. > + const char *server; > const char *identity; > enum syslog_facility facility; > /* Input copy (content unspecified). */