On Tue, Jul 31, 2018 at 03:34:29PM +0300, Olga Arkhangelskaia wrote:
static voidI prefer not to use labels when I can. This place can be easily
box_check_say()
{
- const char *log = cfg_gets("log");
- if (log == NULL)
- return;
enum say_logger_type type;
+ const char *log = cfg_gets("log");
+ if (log == NULL) {
+ type = SAY_LOGGER_STDERR;
+ goto checks;
+ }
+
if (say_parse_logger_type(&log, &type) < 0) {
tnt_raise(ClientError, ER_CFG, "log",
diag_last_error(diag_get())->errmsg);
rewritten without the goto:
enum say_logger_type type = SAY_LOGGER_STDERR; /* default */
const char *log = cfg_gets("log");
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) {
I agree about labels.
However, in this particular I do insist. If we rewrite it like you
did - we miss two cases I mentioned in cover letter - setting format and
setting nonblog, without
setting the log type. Label help us to leave this possibility but to check
the input.
If no log is set - we need to check stderr type and other things that will
be set
sorry, we do not need second call@@ -745,6 +752,10 @@ voidsay_set_log_format() is called twice, which doesn't look right.
box_set_log_format(void)
{
enum say_format format =
box_check_log_format(cfg_gets("log_format"));
+ if (say_set_log_format(format) < 0) {
+ tnt_raise(ClientError, ER_CFG, "log_format",
+ diag_last_error(diag_get())->errmsg);
+ }
say_set_log_format(format);
Ideally, the error message should look exactly like the one emitted byWhy? Format can be changed, so we need to say about format types that does
box_check_say().
not exist.
May be, you could simply call box_check_say() from
box_set_log_format()?
Again, why? box_check_say() is used to check whole config, and check format
- checks only format. We do not need check things like nonblock in there.
I don't see why assertion is better.
}I guess if you called box_check_say() from box_set_log_format(), you
diff --git a/src/say.c b/src/say.c
index b4836d9ee..8753bb865 100644
--- a/src/say.c
+++ b/src/say.c
@@ -190,7 +190,7 @@ say_set_log_level(int new_level)
log_set_level(log_default, (enum say_level) new_level);
}
-void
+int
say_set_log_format(enum say_format format)
could simply add some assertions making sure that log format is
compatible with logger type instead of allowing this function to return
an error.
We need to say to user/admin that the thing that he/she doing is wrong.
no - panic means that tarantool is failed to initialize one of his systems,{panic() seems to be missing.
/*
@@ -204,22 +204,25 @@ say_set_log_format(enum say_format format)
case SF_JSON:
if (!allowed_to_change) {
- say_error("json log format is not supported when output
is '%s'",
+ diag_set(IllegalParams,
+ "json log format is incompatible with '%s'log",
say_logger_type_strs[log_default->type]);
- return;
+ return -1;
}
log_set_format(log_default, say_format_json);
break;
case SF_PLAIN:
if (!allowed_to_change) {
- return;
+ return 0;
}
log_set_format(log_default, say_format_plain);
break;
default:
- unreachable();
+ diag_set(IllegalParams, "unsupported log_format, expected plain
or json");
+ return -1;
}
log_format = format;
+ return 0;
}
static const char *say_format_strs[] = {
@@ -692,7 +695,8 @@ say_logger_init(const char *init_str, int level, int
nonblock,
say_set_log_level(level);
log_background = background;
log_pid = log_default->pid;
- say_set_log_format(say_format_by_name(format));
+ if (say_set_log_format(say_format_by_name(format)) < 0)
+ goto error;
if (background) {
fflush(stderr);
@@ -715,6 +719,8 @@ say_logger_init(const char *init_str, int level, int
nonblock,
fail:
diag_log();
panic("failed to initialize logging subsystem");
+error:
+ diag_log();
this is crash. However, error with the format - it is too much to crush,
don't you think? We can leave the old one.