| Message ID | 1641461121-2212-1-git-send-email-xuyang2018.jy@fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/4] lib/tst_kconfig: Modify the return type of tst_kconfig_check function | expand |
Hi Xu,
Reviewed-by: Petr Vorel <petr.vorel@gmail.com>
Obviously OK.
Kind regards,
Petr
Hi! > diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c > index d433b8cf6..dc7decff9 100644 > --- a/lib/tst_kconfig.c > +++ b/lib/tst_kconfig.c > @@ -478,22 +478,26 @@ static void dump_vars(const struct tst_expr *expr) > } > } > > -void tst_kconfig_check(const char *const kconfigs[]) > +int tst_kconfig_check(const char *const kconfigs[]) > { > size_t expr_cnt = array_len(kconfigs); > struct tst_expr *exprs[expr_cnt]; > unsigned int i, var_cnt; > - int abort_test = 0; > + int ret = 0; > > for (i = 0; i < expr_cnt; i++) { > exprs[i] = tst_bool_expr_parse(kconfigs[i]); > > - if (!exprs[i]) > - tst_brk(TBROK, "Invalid kconfig expression!"); > + if (!exprs[i]) { > + tst_res(TWARN, "Invalid kconfig expression!"); > + return 1; > + } > } > > - if (validate_vars(exprs, expr_cnt)) > - tst_brk(TBROK, "Invalid kconfig variables!"); > + if (validate_vars(exprs, expr_cnt)) { > + tst_res(TWARN, "Invalid kconfig variables!"); > + return 1; > + } I think that it would be actually better to keep the TBROK in these two checks because neither of these two will trigger unless there is a typo in the expressions and it makes sense to abort everything and stop in these cases. Other than that it looks good.
Hi Cyril > Hi! >> diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c >> index d433b8cf6..dc7decff9 100644 >> --- a/lib/tst_kconfig.c >> +++ b/lib/tst_kconfig.c >> @@ -478,22 +478,26 @@ static void dump_vars(const struct tst_expr *expr) >> } >> } >> >> -void tst_kconfig_check(const char *const kconfigs[]) >> +int tst_kconfig_check(const char *const kconfigs[]) >> { >> size_t expr_cnt = array_len(kconfigs); >> struct tst_expr *exprs[expr_cnt]; >> unsigned int i, var_cnt; >> - int abort_test = 0; >> + int ret = 0; >> >> for (i = 0; i< expr_cnt; i++) { >> exprs[i] = tst_bool_expr_parse(kconfigs[i]); >> >> - if (!exprs[i]) >> - tst_brk(TBROK, "Invalid kconfig expression!"); >> + if (!exprs[i]) { >> + tst_res(TWARN, "Invalid kconfig expression!"); >> + return 1; >> + } >> } >> >> - if (validate_vars(exprs, expr_cnt)) >> - tst_brk(TBROK, "Invalid kconfig variables!"); >> + if (validate_vars(exprs, expr_cnt)) { >> + tst_res(TWARN, "Invalid kconfig variables!"); >> + return 1; >> + } > > I think that it would be actually better to keep the TBROK in these two > checks because neither of these two will trigger unless there is a typo > in the expressions and it makes sense to abort everything and stop in > these cases. Sounds reasonable, will do it. Best Regards Yang Xu > > Other than that it looks good. >
diff --git a/include/tst_kconfig.h b/include/tst_kconfig.h index 1bb21fea8..cc0908ea8 100644 --- a/include/tst_kconfig.h +++ b/include/tst_kconfig.h @@ -44,7 +44,8 @@ void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len); /** * Checks if required kernel configuration options are set in the kernel - * config and exits the test with TCONF if at least one is missing. + * config. Return 0 if every config is satisfied and return 1 if at least + * one is missing. * * The config options can be passed in two different formats, either * "CONFIG_FOO" in which case the option has to be set in order to continue the @@ -53,6 +54,6 @@ void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len); * * @param kconfigs NULL-terminated array of config strings needed for the testrun. */ -void tst_kconfig_check(const char *const kconfigs[]); +int tst_kconfig_check(const char *const kconfigs[]); #endif /* TST_KCONFIG_H__ */ diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c index d433b8cf6..dc7decff9 100644 --- a/lib/tst_kconfig.c +++ b/lib/tst_kconfig.c @@ -478,22 +478,26 @@ static void dump_vars(const struct tst_expr *expr) } } -void tst_kconfig_check(const char *const kconfigs[]) +int tst_kconfig_check(const char *const kconfigs[]) { size_t expr_cnt = array_len(kconfigs); struct tst_expr *exprs[expr_cnt]; unsigned int i, var_cnt; - int abort_test = 0; + int ret = 0; for (i = 0; i < expr_cnt; i++) { exprs[i] = tst_bool_expr_parse(kconfigs[i]); - if (!exprs[i]) - tst_brk(TBROK, "Invalid kconfig expression!"); + if (!exprs[i]) { + tst_res(TWARN, "Invalid kconfig expression!"); + return 1; + } } - if (validate_vars(exprs, expr_cnt)) - tst_brk(TBROK, "Invalid kconfig variables!"); + if (validate_vars(exprs, expr_cnt)) { + tst_res(TWARN, "Invalid kconfig variables!"); + return 1; + } var_cnt = get_var_cnt(exprs, expr_cnt); struct tst_kconfig_var vars[var_cnt]; @@ -506,7 +510,7 @@ void tst_kconfig_check(const char *const kconfigs[]) int val = tst_bool_expr_eval(exprs[i], map); if (val != 1) { - abort_test = 1; + ret = 1; tst_res(TINFO, "Constraint '%s' not satisfied!", kconfigs[i]); dump_vars(exprs[i]); } @@ -519,8 +523,7 @@ void tst_kconfig_check(const char *const kconfigs[]) free(vars[i].val); } - if (abort_test) - tst_brk(TCONF, "Aborting due to unsuitable kernel config, see above!"); + return ret; } char tst_kconfig_get(const char *confname) diff --git a/lib/tst_test.c b/lib/tst_test.c index 9fea7263a..d5cefadaa 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -1025,8 +1025,8 @@ static void do_setup(int argc, char *argv[]) parse_opts(argc, argv); - if (tst_test->needs_kconfigs) - tst_kconfig_check(tst_test->needs_kconfigs); + if (tst_test->needs_kconfigs && tst_kconfig_check(tst_test->needs_kconfigs)) + tst_brk(TCONF, "Aborting due to unsuitable kernel config, see above!"); if (tst_test->needs_root && geteuid() != 0) tst_brk(TCONF, "Test needs to be run as root");
So this function can be used to detect whether the function succeeded in shell api by its return value. Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- include/tst_kconfig.h | 5 +++-- lib/tst_kconfig.c | 21 ++++++++++++--------- lib/tst_test.c | 4 ++-- 3 files changed, 17 insertions(+), 13 deletions(-)