[tarantool-patches] [PATCH v4] Fixed non-informative error messages for log conf.

[tarantool-patches] [PATCH v4] Fixed non-informative error messages for log conf.

[Olga Arkhangelskaia]

From: Olga Arkhangelskaia <arkholga@tarantool.org>

In case of bad or erroneous options for log configurations
errors had ambiguous or absent messages. In some cases it lead to
app crashes.

Closes #3553
---
https://github.com/tarantool/tarantool/issues/3553
https://github.com/tarantool/tarantool/tree/OKriw/gh-3553-non-inf-error


v1:
https://www.freelists.org/post/tarantool-patches/PATCH-01-Fixed-noninformative-error-messages-for-log-conf

v2:
https://www.freelists.org/post/tarantool-patches/PATCH-v2-02-logger-non-inf-error

v3:
https://www.freelists.org/post/tarantool-patches/PATCH-v3-Fixed-noninformative-error-messages-for-log-conf

Changes in v2:
- added tests
- changed the way of checks, now all checks is left in box
- added re-usage of some checks 

Changes in v3:
- added asserts
- box_check_say is used to check all input cfg
- checked possibility to say box.cfg = nill and than to set some partametrs. It
is impossible, so there is no need to take log cfg from say.

Changes in v4:
- deleted unnecessary function
  
 src/box/box.cc            | 31 +++++++++++--------------------
 src/say.c                 | 42 ++++++++++++------------------------------
 test/box-tap/cfg.test.lua |  7 +++++--
 3 files changed, 28 insertions(+), 52 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 62fd05468..ee6a6e701 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,27 +376,20 @@ 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;
-}
-
 static void
 box_check_uri(const char *source, const char *option_name)
 {
@@ -745,7 +735,8 @@ box_set_log_level(void)
 void
 box_set_log_format(void)
 {
-	enum say_format format = box_check_log_format(cfg_gets("log_format"));
+	box_check_say();
+	enum say_format format = say_format_by_name(cfg_gets("log_format"));
 	say_set_log_format(format);
 }
 
diff --git a/src/say.c b/src/say.c
index ac221dd19..5ff4d3463 100644
--- a/src/say.c
+++ b/src/say.c
@@ -126,14 +126,6 @@ static const char *level_strs[] = {
 	[S_DEBUG] = "DEBUG",
 };
 
-static const char *say_logger_type_strs[] = {
-	[SAY_LOGGER_BOOT] = "stdout",
-	[SAY_LOGGER_STDERR] = "stderr",
-	[SAY_LOGGER_FILE] = "file",
-	[SAY_LOGGER_PIPE] = "pipe",
-	[SAY_LOGGER_SYSLOG] = "syslog",
-};
-
 static const char*
 level_to_string(int level)
 {
@@ -192,32 +184,21 @@ say_set_log_level(int new_level)
 void
 say_set_log_format(enum say_format format)
 {
-	/*
-	 * For syslog, default or boot log type the log format can
-	 * not be changed.
-	 */
-	bool allowed_to_change = log_default->type == SAY_LOGGER_STDERR ||
-				 log_default->type == SAY_LOGGER_PIPE ||
-				 log_default->type == SAY_LOGGER_FILE;
+	log_format_func_t format_func;
+
 	switch (format) {
 	case SF_JSON:
-
-		if (!allowed_to_change) {
-			say_error("json log format is not supported when output is '%s'",
-				  say_logger_type_strs[log_default->type]);
-			return;
-		}
-		log_set_format(log_default, say_format_json);
+		assert(log_default->type != SAY_LOGGER_SYSLOG);
+		format_func = say_format_json;
 		break;
 	case SF_PLAIN:
-		if (!allowed_to_change) {
-			return;
-		}
-		log_set_format(log_default, say_format_plain);
+		format_func = say_format_plain;
 		break;
 	default:
 		unreachable();
 	}
+
+	log_set_format(log_default, format_func);
 	log_format = format;
 }
 
@@ -540,10 +521,9 @@ log_create(struct log *log, const char *init_str, int nonblock)
 
 	if (init_str != NULL) {
 		enum say_logger_type type;
-		if (say_parse_logger_type(&init_str, &type)) {
-			diag_set(IllegalParams, logger_syntax_reminder);
+		if (say_parse_logger_type(&init_str, &type))
 			return -1;
-		}
+
 		int rc;
 		switch (type) {
 		case SAY_LOGGER_PIPE:
@@ -989,8 +969,10 @@ say_parse_logger_type(const char **str, enum say_logger_type *type)
 		*type = SAY_LOGGER_SYSLOG;
 	else if (strchr(*str, ':') == NULL)
 		*type = SAY_LOGGER_FILE;
-	else
+	else {
+		diag_set(IllegalParams, logger_syntax_reminder);
 		return -1;
+	}
 	return 0;
 }
 
diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index ffafdbe42..e3f904743 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -6,7 +6,7 @@ local socket = require('socket')
 local fio = require('fio')
 local uuid = require('uuid')
 local msgpack = require('msgpack')
-test:plan(95)
+test:plan(98)
 
 --------------------------------------------------------------------------------
 -- Invalid values
@@ -39,6 +39,7 @@ invalid('listen', '//!')
 invalid('log', ':')
 invalid('log', 'syslog:xxx=')
 invalid('log_level', 'unknown')
+invalid('log', ':test:')
 invalid('vinyl_memory', -1)
 invalid('vinyl_read_threads', 0)
 invalid('vinyl_write_threads', 1)
@@ -51,11 +52,12 @@ invalid('vinyl_bloom_fpr', 1.1)
 
 local function invalid_combinations(name, val)
     local status, result = pcall(box.cfg, val)
-    test:ok(not status and result:match('Illegal'), 'invalid '..name)
+    test:ok(not status and result:match('Incorrect'), 'invalid '..name)
 end
 
 invalid_combinations("log, log_nonblock", {log = "1.log", log_nonblock = true})
 invalid_combinations("log, log_format", {log = "syslog:identity=tarantool", log_format = 'json'})
+invalid_combinations("log, log_nonblock", {log_nonblock = true})
 
 test:is(type(box.cfg), 'function', 'box is not started')
 
@@ -105,6 +107,7 @@ test:ok(status and result == 'table', 'configured box')
 --------------------------------------------------------------------------------
 
 invalid('log_level', 'unknown')
+invalid('log_format', 'xxx')
 
 --------------------------------------------------------------------------------
 -- gh-534: Segmentation fault after two bad wal_mode settings
-- 
2.14.3 (Apple Git-98)

Re: [tarantool-patches] [PATCH v4] Fixed non-informative error messages for log conf.

[Vladimir Davydov]

Pushed to 2.0

Please prefix the subject line with a subystem name in future.
I did it by myself this time.