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

[Vladimir Davydov]

On Tue, Jul 31, 2018 at 03:34:29PM +0300, Olga Arkhangelskaia wrote:
> > >   static void
> > >   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);
> > I prefer not to use labels when I can. This place can be easily
> > 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

The code I posted above does exactly that. Note, there's no 'return'.

> > > @@ -745,6 +752,10 @@ void
> > >   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);
> > say_set_log_format() is called twice, which doesn't look right.
> sorry, we do not need second call
> > Ideally, the error message should look exactly like the one emitted by
> > box_check_say().
> Why? Format can be changed, so we need to say about format types that does
> not exist.

Yep, and box_check_say() can do that for us. My point is that we can
avoid duplicating the code that performs the checks by reusing
box_check_say(), and it would result in consistent error messages, too.

> > 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.

It's a slow path so we can do some extra job there for the sake of
simplifying the code, I guess.

> 
> > 
> > >   }
> > > 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)
> > I guess if you called box_check_say() from box_set_log_format(), you
> > could simply add some assertions making sure that log format is
> > compatible with logger type instead of allowing this function to return
> > an error.
> I don't see why assertion is better.
> We need to say to user/admin that the thing that he/she doing is wrong.

Yep, we do, but if this function was called only after box_check_say(),
which already performs the checks, assertion would be OK.

> 
> 
> > >   {
> > >   	/*
> > > @@ -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();
> > panic() seems to be missing.
> no - panic means that tarantool is failed to initialize one of his systems,
> this is crash. However, error with the format - it is too much to crush,
> don't you think? We can leave the old one.

I guess you're right. It's just those 'fail' label with 'panic' and
'error' without look weird.

Come to think of it, you can't get here anyway, because the validity of
the configuration should've been checked by box_check_config() is
called, no? Confusing...

That's why I'd prefer to have all validity checks consolidated in
one place - box_check_say(), and use assertions anywhere else (in
say_logger_init, say_logger_set_format, etc).