| Message ID | 1641461121-2212-2-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, > diff --git a/doc/user-guide.txt b/doc/user-guide.txt ... > +| 'LTP_KCONFIG_DISABLE' | Switch for kernel config check functionality. > + 'y' or '1': disable kconfig check, > + 'n' or '0': enable kconfig check. Not sure if you got inspired by LTP_COLORIZE_OUTPUT, which also uses y/1 or n/0 input, but I'd really stick just to LTP_KCONFIG_DISABLE=1 to disable kconfig check. By default kconfig check would be kept. LTP_COLORIZE_OUTPUT has more complicated the default value, because it's on by default when print to stderr, but off when redirected (we should document it in doc/user-guide.txt). But there is no reason have way to configure the default value. But maybe others see it differently. And writing it today I'd have probably chosen for LTP_KCONFIG_DISABLE only one: y/n or 1/0. Kind regards, Petr
Hi Xu, > +| 'LTP_KCONFIG_DISABLE' | Switch for kernel config check functionality. > + 'y' or '1': disable kconfig check, > + 'n' or '0': enable kconfig check. Also, regardless if we keep it this way, it's worth to mention the default value. > int tst_kconfig_check(const char *const kconfigs[]) > { > size_t expr_cnt = array_len(kconfigs); > @@ -485,6 +506,11 @@ int tst_kconfig_check(const char *const kconfigs[]) > unsigned int i, var_cnt; > int ret = 0; > + if (tst_kconfig_disabled()) { > + tst_res(TINFO, "Kernel config check functionality has been disabled."); nit: please avoid the dot :). Kind regards, Petr
Hi! > diff --git a/doc/user-guide.txt b/doc/user-guide.txt > index 494652618..8d4435a28 100644 > --- a/doc/user-guide.txt > +++ b/doc/user-guide.txt > @@ -18,6 +18,9 @@ For running LTP network tests see `testcases/network/README.md`. > | 'LTP_SINGLE_FS_TYPE' | Testing only - specifies filesystem instead all > supported (for tests with '.all_filesystems'). > | 'LTP_DEV_FS_TYPE' | Filesystem used for testing (default: 'ext2'). > +| 'LTP_KCONFIG_DISABLE' | Switch for kernel config check functionality. > + 'y' or '1': disable kconfig check, > + 'n' or '0': enable kconfig check. Maybe it would be better named LTP_KCONFIG_SKIP or even KCONFIG_SKIP_CHECK we do have KCONFIG_PATH so it would make sense to keep the kernel config related variables prefixed with just KCONFIG_ I think that the point made by Tim Bird was that the KCONFIG_PATH should be standartized variable among testsuites, so it makes sense to have KCONFIG_SKIP_CHECK as a standartized variable as well. [CC: Tim should we start tracking common env variables like this somewhere?] > | 'LTP_TIMEOUT_MUL' | Multiply timeout, must be number >= 1 (> 1 is useful for > slow machines to avoid unexpected timeout). > Variable is also used in shell tests, but ceiled to int. > diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c > index dc7decff9..c37afd8c8 100644 > --- a/lib/tst_kconfig.c > +++ b/lib/tst_kconfig.c > @@ -478,6 +478,27 @@ static void dump_vars(const struct tst_expr *expr) > } > } > > +static int tst_kconfig_disabled(void) > +{ > + static int check; > + > + if (check) > + return check - 1; > + > + char *env = getenv("LTP_KCONFIG_DISABLE"); > + > + if (env) { > + if (!strcmp(env, "n") || !strcmp(env, "0")) > + check = 1; > + if (!strcmp(env, "y") || !strcmp(env, "1")) > + check = 2; > + return check - 1; > + } > + > + check = 1; > + return 0; > +} As pointed out by Peter, can we please keep it simple? I.e. just branch on variable defined/undefined? > int tst_kconfig_check(const char *const kconfigs[]) > { > size_t expr_cnt = array_len(kconfigs); > @@ -485,6 +506,11 @@ int tst_kconfig_check(const char *const kconfigs[]) > unsigned int i, var_cnt; > int ret = 0; > > + if (tst_kconfig_disabled()) { > + tst_res(TINFO, "Kernel config check functionality has been disabled."); I would try to make more clear what has happened here, something as: "Skipping kernel config check as requested" Or something along these lines. > + return 0; > + } > + > for (i = 0; i < expr_cnt; i++) { > exprs[i] = tst_bool_expr_parse(kconfigs[i]);
Hi Xu, Cyril, > Hi! > > diff --git a/doc/user-guide.txt b/doc/user-guide.txt > > index 494652618..8d4435a28 100644 > > --- a/doc/user-guide.txt > > +++ b/doc/user-guide.txt > > @@ -18,6 +18,9 @@ For running LTP network tests see `testcases/network/README.md`. > > | 'LTP_SINGLE_FS_TYPE' | Testing only - specifies filesystem instead all > > supported (for tests with '.all_filesystems'). > > | 'LTP_DEV_FS_TYPE' | Filesystem used for testing (default: 'ext2'). > > +| 'LTP_KCONFIG_DISABLE' | Switch for kernel config check functionality. > > + 'y' or '1': disable kconfig check, > > + 'n' or '0': enable kconfig check. > Maybe it would be better named LTP_KCONFIG_SKIP or even > KCONFIG_SKIP_CHECK we do have KCONFIG_PATH so it would make sense to > keep the kernel config related variables prefixed with just KCONFIG_ > I think that the point made by Tim Bird was that the KCONFIG_PATH should > be standartized variable among testsuites, so it makes sense to have > KCONFIG_SKIP_CHECK as a standartized variable as well. Is it too bad to have LTP_KCONFIG_SKIP_CHECK and LTP_KCONFIG_PATH ? Maybe we could change it even now. OK, we have few exceptions to LTP_ prefix LTPROOT (I'd keep it for historical reasons), TMPDIR (IMHO make sense), TST_NO_CLEANUP (IMHO should be changed to LTP_NO_CLEANUP). > [CC: Tim should we start tracking common env variables like this somewhere?] +1 > > + if (tst_kconfig_disabled()) { > > + tst_res(TINFO, "Kernel config check functionality has been disabled."); > I would try to make more clear what has happened here, something as: > "Skipping kernel config check as requested" +1 > Or something along these lines. Kind regards, Petr
Hi! > > > diff --git a/doc/user-guide.txt b/doc/user-guide.txt > > > index 494652618..8d4435a28 100644 > > > --- a/doc/user-guide.txt > > > +++ b/doc/user-guide.txt > > > @@ -18,6 +18,9 @@ For running LTP network tests see `testcases/network/README.md`. > > > | 'LTP_SINGLE_FS_TYPE' | Testing only - specifies filesystem instead all > > > supported (for tests with '.all_filesystems'). > > > | 'LTP_DEV_FS_TYPE' | Filesystem used for testing (default: 'ext2'). > > > +| 'LTP_KCONFIG_DISABLE' | Switch for kernel config check functionality. > > > + 'y' or '1': disable kconfig check, > > > + 'n' or '0': enable kconfig check. > > > Maybe it would be better named LTP_KCONFIG_SKIP or even > > KCONFIG_SKIP_CHECK we do have KCONFIG_PATH so it would make sense to > > keep the kernel config related variables prefixed with just KCONFIG_ > > > I think that the point made by Tim Bird was that the KCONFIG_PATH should > > be standartized variable among testsuites, so it makes sense to have > > KCONFIG_SKIP_CHECK as a standartized variable as well. > > Is it too bad to have LTP_KCONFIG_SKIP_CHECK and LTP_KCONFIG_PATH ? > Maybe we could change it even now. Yes, the whole reason not to prefix it with LTP_ is to have a standard among all the testsuites. The more variables are standartized the better. > TST_NO_CLEANUP (IMHO should be changed to LTP_NO_CLEANUP). Unless we want to have a standard for that one as well. Really all we need to is to create a page with the description of these variables and agree on a common subset. It's that simple, but someone has to actually do it.
Hi all, > Hi! > > > > diff --git a/doc/user-guide.txt b/doc/user-guide.txt > > > > index 494652618..8d4435a28 100644 > > > > --- a/doc/user-guide.txt > > > > +++ b/doc/user-guide.txt > > > > @@ -18,6 +18,9 @@ For running LTP network tests see `testcases/network/README.md`. > > > > | 'LTP_SINGLE_FS_TYPE' | Testing only - specifies filesystem instead all > > > > supported (for tests with '.all_filesystems'). > > > > | 'LTP_DEV_FS_TYPE' | Filesystem used for testing (default: 'ext2'). > > > > +| 'LTP_KCONFIG_DISABLE' | Switch for kernel config check functionality. > > > > + 'y' or '1': disable kconfig check, > > > > + 'n' or '0': enable kconfig check. > > > Maybe it would be better named LTP_KCONFIG_SKIP or even > > > KCONFIG_SKIP_CHECK we do have KCONFIG_PATH so it would make sense to > > > keep the kernel config related variables prefixed with just KCONFIG_ > > > I think that the point made by Tim Bird was that the KCONFIG_PATH should > > > be standartized variable among testsuites, so it makes sense to have > > > KCONFIG_SKIP_CHECK as a standartized variable as well. > > Is it too bad to have LTP_KCONFIG_SKIP_CHECK and LTP_KCONFIG_PATH ? > > Maybe we could change it even now. > Yes, the whole reason not to prefix it with LTP_ is to have a standard > among all the testsuites. The more variables are standartized the > better. Ah, thanks, I wasn't aware of this agreement. > > TST_NO_CLEANUP (IMHO should be changed to LTP_NO_CLEANUP). > Unless we want to have a standard for that one as well. Really all we > need to is to create a page with the description of these variables and > agree on a common subset. It's that simple, but someone has to actually > do it. I thought this is the page for user defined variables: https://github.com/linux-test-project/ltp/wiki/User-Guidelines But we should explain there the exceptions. Kind regards, Petr
Hi Cyril > Hi! >>>> diff --git a/doc/user-guide.txt b/doc/user-guide.txt >>>> index 494652618..8d4435a28 100644 >>>> --- a/doc/user-guide.txt >>>> +++ b/doc/user-guide.txt >>>> @@ -18,6 +18,9 @@ For running LTP network tests see `testcases/network/README.md`. >>>> | 'LTP_SINGLE_FS_TYPE' | Testing only - specifies filesystem instead all >>>> supported (for tests with '.all_filesystems'). >>>> | 'LTP_DEV_FS_TYPE' | Filesystem used for testing (default: 'ext2'). >>>> +| 'LTP_KCONFIG_DISABLE' | Switch for kernel config check functionality. >>>> + 'y' or '1': disable kconfig check, >>>> + 'n' or '0': enable kconfig check. >> >>> Maybe it would be better named LTP_KCONFIG_SKIP or even >>> KCONFIG_SKIP_CHECK we do have KCONFIG_PATH so it would make sense to >>> keep the kernel config related variables prefixed with just KCONFIG_ >> >>> I think that the point made by Tim Bird was that the KCONFIG_PATH should >>> be standartized variable among testsuites, so it makes sense to have >>> KCONFIG_SKIP_CHECK as a standartized variable as well. >> >> Is it too bad to have LTP_KCONFIG_SKIP_CHECK and LTP_KCONFIG_PATH ? >> Maybe we could change it even now. > > Yes, the whole reason not to prefix it with LTP_ is to have a standard > among all the testsuites. The more variables are standartized the > better. This make me remember xfstests testsuite also has KCONFIG_PATH environment variable. I will use KCONFIG_SKIP_CHECK name firstly, Unless Tim objects(may know other testsuite has used KCONFIG_ environment variable ) or has a better naming method. Best Regards Yang Xu > >> TST_NO_CLEANUP (IMHO should be changed to LTP_NO_CLEANUP). > > Unless we want to have a standard for that one as well. Really all we > need to is to create a page with the description of these variables and > agree on a common subset. It's that simple, but someone has to actually > do it. >
diff --git a/doc/user-guide.txt b/doc/user-guide.txt index 494652618..8d4435a28 100644 --- a/doc/user-guide.txt +++ b/doc/user-guide.txt @@ -18,6 +18,9 @@ For running LTP network tests see `testcases/network/README.md`. | 'LTP_SINGLE_FS_TYPE' | Testing only - specifies filesystem instead all supported (for tests with '.all_filesystems'). | 'LTP_DEV_FS_TYPE' | Filesystem used for testing (default: 'ext2'). +| 'LTP_KCONFIG_DISABLE' | Switch for kernel config check functionality. + 'y' or '1': disable kconfig check, + 'n' or '0': enable kconfig check. | 'LTP_TIMEOUT_MUL' | Multiply timeout, must be number >= 1 (> 1 is useful for slow machines to avoid unexpected timeout). Variable is also used in shell tests, but ceiled to int. diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c index dc7decff9..c37afd8c8 100644 --- a/lib/tst_kconfig.c +++ b/lib/tst_kconfig.c @@ -478,6 +478,27 @@ static void dump_vars(const struct tst_expr *expr) } } +static int tst_kconfig_disabled(void) +{ + static int check; + + if (check) + return check - 1; + + char *env = getenv("LTP_KCONFIG_DISABLE"); + + if (env) { + if (!strcmp(env, "n") || !strcmp(env, "0")) + check = 1; + if (!strcmp(env, "y") || !strcmp(env, "1")) + check = 2; + return check - 1; + } + + check = 1; + return 0; +} + int tst_kconfig_check(const char *const kconfigs[]) { size_t expr_cnt = array_len(kconfigs); @@ -485,6 +506,11 @@ int tst_kconfig_check(const char *const kconfigs[]) unsigned int i, var_cnt; int ret = 0; + if (tst_kconfig_disabled()) { + tst_res(TINFO, "Kernel config check functionality has been disabled."); + return 0; + } + for (i = 0; i < expr_cnt; i++) { exprs[i] = tst_bool_expr_parse(kconfigs[i]); @@ -530,6 +556,11 @@ char tst_kconfig_get(const char *confname) { struct tst_kconfig_var var; + if (tst_kconfig_disabled()) { + tst_res(TINFO, "Kernel config check functionality has been disabled."); + return 0; + } + var.id_len = strlen(confname); if (var.id_len >= sizeof(var.id)) diff --git a/lib/tst_test.c b/lib/tst_test.c index d5cefadaa..3e6f2ee1f 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -483,6 +483,7 @@ static void print_help(void) fprintf(stderr, "LTP_COLORIZE_OUTPUT Force colorized output behaviour (y/1 always, n/0: never)\n"); fprintf(stderr, "LTP_DEV Path to the block device to be used (for .needs_device)\n"); fprintf(stderr, "LTP_DEV_FS_TYPE Filesystem used for testing (default: %s)\n", DEFAULT_FS_TYPE); + fprintf(stderr, "LTP_KCONFIG_DISABLE Switch for kernel config check functionality (y/1: disable, n/0: enable)\n"); fprintf(stderr, "LTP_SINGLE_FS_TYPE Testing only - specifies filesystem instead all supported (for .all_filesystems)\n"); fprintf(stderr, "LTP_TIMEOUT_MUL Timeout multiplier (must be a number >=1)\n"); fprintf(stderr, "LTP_VIRT_OVERRIDE Overrides virtual machine detection (values: \"\"|kvm|microsoft|xen|zvm)\n");
This environment variables is designed to add kernel config check functionality switch. So we can disable kconfig check completely and it is useful especially for the embedded platforms that they don't have kernel config. Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- doc/user-guide.txt | 3 +++ lib/tst_kconfig.c | 31 +++++++++++++++++++++++++++++++ lib/tst_test.c | 1 + 3 files changed, 35 insertions(+)