From: Vladimir Davydov <vdavydov.dev@gmail.com> To: Olga Arkhangelskaia <arkholga@tarantool.org> Cc: tarantool-patches@freelists.org Subject: Re: [tarantool-patches] [PATCH 1/2] Ractoring in string validation of log parametrs Date: Wed, 18 Jul 2018 15:18:41 +0300 [thread overview] Message-ID: <20180718121841.wkuwvvoz3ccldhzg@esperanza> (raw) In-Reply-To: <20180717151521.34679-2-arkholga@tarantool.org> 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 */
next prev parent reply other threads:[~2018-07-18 12:18 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-17 15:15 [tarantool-patches] [PATCH v2 0/2] Refactor in box and say modules Olga Arkhangelskaia 2018-07-17 15:15 ` [tarantool-patches] [PATCH 1/2] Ractoring in string validation of log parametrs Olga Arkhangelskaia 2018-07-18 12:18 ` Vladimir Davydov [this message] 2018-07-17 15:15 ` [tarantool-patches] [PATCH 2/2] Added strdup fail checks in say Olga Arkhangelskaia 2018-07-18 12:21 ` Vladimir Davydov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180718121841.wkuwvvoz3ccldhzg@esperanza \ --to=vdavydov.dev@gmail.com \ --cc=arkholga@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='Re: [tarantool-patches] [PATCH 1/2] Ractoring in string validation of log parametrs' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox