From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 8 Aug 2018 16:01:45 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] [PATCH v3] Fixed non-informative error
 messages for log conf.
Message-ID: <20180808130000.oova73yhnk7fttw7@esperanza>
References: <20180807201114.43870-1-krishtal.olja@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180807201114.43870-1-krishtal.olja@gmail.com>
To: Olga Arkhangelskaia <krishtal.olja@gmail.com>
Cc: tarantool-patches@freelists.org, Olga Arkhangelskaia <arkholga@tarantool.org>
List-ID: <tarantool-patches.dev.tarantool.org>

On Tue, Aug 07, 2018 at 11:11:14PM +0300, Olga Arkhangelskaia wrote:
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 62fd05468..9601697f6 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -354,15 +354,12 @@ apply_initial_join_row(struct xstream *stream, struct xrow_header *row)
>  static void
>  box_check_say()
>  {
> +	enum say_logger_type type = SAY_LOGGER_STDERR; /* default */
>  	const char *log = cfg_gets("log");
> -	if (log == NULL)
> -		return;
> -	enum say_logger_type type;
> -	if (say_parse_logger_type(&log, &type) < 0) {
> +	if (log != NULL && say_parse_logger_type(&log, &type) < 0) {
>  		tnt_raise(ClientError, ER_CFG, "log",
>  			  diag_last_error(diag_get())->errmsg);
>  	}
> -
>  	if (type == SAY_LOGGER_SYSLOG) {
>  		struct say_syslog_opts opts;
>  		if (say_parse_syslog_opts(log, &opts) < 0) {
> @@ -379,25 +376,24 @@ box_check_say()
>  	const char *log_format = cfg_gets("log_format");
>  	enum say_format format = say_format_by_name(log_format);
>  	if (format == say_format_MAX)
> -		diag_set(ClientError, ER_CFG, "log_format",
> +		tnt_raise(ClientError, ER_CFG, "log_format",
>  			 "expected 'plain' or 'json'");
>  	if (type == SAY_LOGGER_SYSLOG && format == SF_JSON) {
> -		tnt_raise(ClientError, ER_ILLEGAL_PARAMS, "log, log_format");
> +		tnt_raise(ClientError, ER_CFG, "log_format",
> +			  "'json' can't be used with syslog logger");
>  	}
>  	int log_nonblock = cfg_getb("log_nonblock");
> -	if (log_nonblock == 1 && type == SAY_LOGGER_FILE) {
> -		tnt_raise(ClientError, ER_ILLEGAL_PARAMS, "log, log_nonblock");
> +	if (log_nonblock == 1 &&
> +	    (type == SAY_LOGGER_FILE || type == SAY_LOGGER_STDERR)) {
> +		tnt_raise(ClientError, ER_CFG, "log_nonblock",
> +			  "the option is incompatible with file/stderr logger");
>  	}
>  }
>  
>  static enum say_format
>  box_check_log_format(const char *log_format)
>  {
> -	enum say_format format = say_format_by_name(log_format);
> -	if (format == say_format_MAX)
> -		tnt_raise(ClientError, ER_CFG, "log_format",
> -			  "expected 'plain' or 'json'");
> -	return format;
> +	return say_format_by_name(log_format);
>  }

Apparently, you don't need box_check_log_format() anymore as it simply
calls say_format_by_name() with the same argument. Please remove it and
call say_format_by_name() in box_set_log_format() directly.

Other than that, the patch looks good to me.

>  
>  static void
> @@ -745,6 +741,7 @@ box_set_log_level(void)
>  void
>  box_set_log_format(void)
>  {
> +	box_check_say();
>  	enum say_format format = box_check_log_format(cfg_gets("log_format"));
>  	say_set_log_format(format);
>  }