[tarantool-patches] [PATCH v4] Fixed non-informative error messages for log conf.
Olga Arkhangelskaia
krishtal.olja at gmail.com
Sun Aug 12 12:56:08 MSK 2018
From: Olga Arkhangelskaia <arkholga at 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)
More information about the Tarantool-patches
mailing list