From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 18 Jul 2018 15:18:41 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH 1/2] Ractoring in string validation of log parametrs Message-ID: <20180718121841.wkuwvvoz3ccldhzg@esperanza> References: <20180717151521.34679-1-arkholga@tarantool.org> <20180717151521.34679-2-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180717151521.34679-2-arkholga@tarantool.org> To: Olga Arkhangelskaia Cc: tarantool-patches@freelists.org List-ID: On Tue, Jul 17, 2018 at 06:15:20PM +0300, Olga Arkhangelskaia wrote: > Removed unnecessary double check of log configuration string. > Instead all log configuration ckecks were moved to separate function. > > v2: > - Introduced new function say_check_cfg that encapsulates all checks. > --- > src/box/box.cc | 29 +---------------------------- > src/say.c | 36 +++++++++++++++++++++++------------- > src/say.h | 9 ++++----- > 3 files changed, 28 insertions(+), 46 deletions(-) > > diff --git a/src/box/box.cc b/src/box/box.cc > index 9252e82c1..9ef923616 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -362,36 +362,9 @@ box_check_say() > diag_last_error(diag_get())->errmsg); > } > > - if (say_check_init_str(log) == -1) { > - > - diag_raise(); > - } > - > - if (type == SAY_LOGGER_SYSLOG) { > - struct say_syslog_opts opts; > - if (say_parse_syslog_opts(log, &opts) < 0) { > - if (diag_last_error(diag_get())->type == > - &type_IllegalParams) { > - tnt_raise(ClientError, ER_CFG, "log", > - diag_last_error(diag_get())->errmsg); > - } > - diag_raise(); > - } > - say_free_syslog_opts(&opts); > - } > - > 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", > - "expected 'plain' or 'json'"); > - if (type == SAY_LOGGER_SYSLOG && format == SF_JSON) { > - tnt_raise(ClientError, ER_ILLEGAL_PARAMS, "log, log_format"); > - } > int log_nonblock = cfg_getb("log_nonblock"); > - if (log_nonblock == 1 && type == SAY_LOGGER_FILE) { > - tnt_raise(ClientError, ER_ILLEGAL_PARAMS, "log, log_nonblock"); > - } > + say_check_cfg(log, log_format, log_nonblock, type); Please fold the call to say_parse_logger_type() in say_check_cfg() as well. > } > > static enum say_format > diff --git a/src/say.c b/src/say.c > index 43124083c..501095b3e 100644 > --- a/src/say.c > +++ b/src/say.c > @@ -952,25 +952,35 @@ write_to_syslog(struct log *log, int total) > /** Loggers }}} */ > > /* > - * Init string parser(s) > + * Checks logger configuration > */ > - > -int > -say_check_init_str(const char *str) > +void > +say_check_cfg(const char *log, const char *log_format, > + int log_nonblock, enum say_logger_type type) > { > - enum say_logger_type type; > - if (say_parse_logger_type(&str, &type)) { > - diag_set(IllegalParams, logger_syntax_reminder); > - return -1; > - } > if (type == SAY_LOGGER_SYSLOG) { > struct say_syslog_opts opts; > - > - if (say_parse_syslog_opts(str, &opts) < 0) > - return -1; > + int ret = say_parse_syslog_opts(log, &opts); > say_free_syslog_opts(&opts); > + if (ret < 0) { > + diag_set(IllegalParams, "Incorrect option '%s'", log); > + diag_raise(); We don't raise exceptions from C files. Instead we return -1 after diag_set(). Please move diag_raise() to box.cc. Also, note box_check_say() should raise ClientError(ER_CFG), i.e. it should convert IllegalParams error to ClientError, like here (this is 1.10): https://github.com/tarantool/tarantool/blob/9f1e0f445d2c42d0901baa3eb084a57b036435e2/src/box/box.cc#L357 > + } > + } > + > + enum say_format format = say_format_by_name(log_format); > + if (format == say_format_MAX) { > + diag_set(IllegalParams, > + "for log_format expected 'plain' or 'json'"); > + } > + if (type == SAY_LOGGER_SYSLOG && format == SF_JSON) { > + diag_set(IllegalParams, "Illegal parametrs log, log_format"); > + diag_raise(); > + } > + if (log_nonblock == 1 && type == SAY_LOGGER_FILE) { > + diag_set(IllegalParams, "Illegal parametrs log, log_nonblock"); > + diag_raise(); > } > - return 0; > } > > /** > diff --git a/src/say.h b/src/say.h > index ad3ba3417..88cf9bd78 100644 > --- a/src/say.h > +++ b/src/say.h > @@ -371,12 +371,11 @@ CFORMAT(printf, 5, 0) extern sayfunc_t _say; > log_say_level(log, S_SYSERROR, strerror(errno), format, ##__VA_ARGS__) > > /** > - * validates logger init string; > - * @returns 0 if validation passed or -1 > - * with an error message written to diag > + * Validates logger configuration; Nit: s/;/. > */ > -int > -say_check_init_str(const char *str); > +void > +say_check_cfg(const char *log, const char *log_format, > + int log_nonblock, enum say_logger_type type); > > /* internals, for unit testing */