From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id BD28A228E3 for ; Tue, 7 Aug 2018 16:12:10 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 1VwQR51t58aQ for ; Tue, 7 Aug 2018 16:12:10 -0400 (EDT) Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 54E57221DA for ; Tue, 7 Aug 2018 16:12:10 -0400 (EDT) Received: by mail-lf1-f41.google.com with SMTP id j143-v6so12538524lfj.12 for ; Tue, 07 Aug 2018 13:12:10 -0700 (PDT) From: Olga Arkhangelskaia Subject: [tarantool-patches] [PATCH v3] Fixed non-informative error messages for log conf. Date: Tue, 7 Aug 2018 23:11:14 +0300 Message-Id: <20180807201114.43870-1-krishtal.olja@gmail.com> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Olga Arkhangelskaia From: Olga Arkhangelskaia 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 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. src/box/box.cc | 25 +++++++++++-------------- src/say.c | 42 ++++++++++++------------------------------ test/box-tap/cfg.test.lua | 7 +++++-- 3 files changed, 28 insertions(+), 46 deletions(-) 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); } 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); } 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)