[LTP] [PATCH v1 2/3] shell: add kconfig parse api
xuyang2018.jy@fujitsu.com
xuyang2018.jy@fujitsu.com
Wed Jan 5 08:15:30 CET 2022
Hi Petr
> Hi Xu,
>
>> Use tst_check_kconfigs command to call tst_kconfig_check function in internal.
>> It introduces three variables in tst_test.sh
>> TST_NEEDS_KCONFIGS
>> TST_NEEDS_KCONFIG_IFS (default value is comma)
>> TST_TCONF_IF_KCONFIG (default value is 1, 0 means to use TWRAN and case continue)
>
>> Also, we can use tst_check_kconfigs in your shell case if you want to skip subtest
>> case instead the whole test.
>
>> ps:Don't use array in tst_require_kconfigs because dash doesn't support shell array.
> IFS instead of array is has been used since tst_test.sh creation, maybe we can
> omit this info.
Now, I know. Will remove this info.
>
>> Fixes:#891
>> Suggested-by: Petr Vorel<pvorel@suse.cz>
>> Suggested-by: Cyril Hrubis<chrubis@suse.cz>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>
>> +++ b/doc/shell-test-api.txt
>> @@ -193,22 +193,23 @@ simply by setting right '$TST_NEEDS_FOO'.
>
>> [options="header"]
>> |=============================================================================
>> -| Variable name | Action done
>> -| 'TST_NEEDS_ROOT' | Exit the test with 'TCONF' unless executed under root.
>> -| | Alternatively the 'tst_require_root' command can be used.
>> -| 'TST_NEEDS_TMPDIR' | Create test temporary directory and cd into it.
>> -| 'TST_NEEDS_DEVICE' | Prepare test temporary device, the path to testing
>> - device is stored in '$TST_DEVICE' variable.
>> - The option implies 'TST_NEEDS_TMPDIR'.
>> -| 'TST_NEEDS_CMDS' | String with command names that has to be present for
>> - the test (see below).
>> -| 'TST_NEEDS_MODULE' | Test module name needed for the test (see below).
>> -| 'TST_NEEDS_DRIVERS'| Checks kernel drivers support for the test.
>> -| 'TST_TIMEOUT' | Maximum timeout set for the test in sec. Must be int>= 1,
>> - or -1 (special value to disable timeout), default is 300.
>> - Variable is meant be set in tests, not by user.
>> - It's an equivalent of `tst_test.timeout` in C, can be set
>> - via 'tst_set_timeout(timeout)' after test has started.
>> +| Variable name | Action done
>> +| 'TST_NEEDS_ROOT' | Exit the test with 'TCONF' unless executed under root.
>> +| | Alternatively the 'tst_require_root' command can be used.
>> +| 'TST_NEEDS_TMPDIR' | Create test temporary directory and cd into it.
>> +| 'TST_NEEDS_DEVICE' | Prepare test temporary device, the path to testing
>> + device is stored in '$TST_DEVICE' variable.
>> + The option implies 'TST_NEEDS_TMPDIR'.
>> +| 'TST_NEEDS_CMDS' | String with command names that has to be present for
>> + the test (see below).
>> +| 'TST_NEEDS_MODULE' | Test module name needed for the test (see below).
>> +| 'TST_NEEDS_DRIVERS' | Checks kernel drivers support for the test.
>> +| 'TST_NEEDS_KCONFIGS'| Checks kernel kconfigs support for the test (see below).
> +1 for adding TST_NEEDS_KCONFIG_IFS, it's needed e.g. for: CONFIG_LSM="integrity,apparmor"
> Could you please mention TST_NEEDS_KCONFIG_IFS and TST_TCONF_IF_KCONFIG here in docs?
of course.
>
>> +| 'TST_TIMEOUT' | Maximum timeout set for the test in sec. Must be int>= 1,
>> + or -1 (special value to disable timeout), default is 300.
>> + Variable is meant be set in tests, not by user.
>> + It's an equivalent of `tst_test.timeout` in C, can be set
>> + via 'tst_set_timeout(timeout)' after test has started.
>> |=============================================================================
>
>> [options="header"]
>> @@ -742,3 +743,27 @@ TST_NEEDS_CHECKPOINTS=1
>> Since both the implementations are compatible, it's also possible to start
>> a child binary process from a shell test and synchronize with it. This process
>> must have checkpoints initialized by calling 'tst_reinit()'.
>> +
>> +1.7 Parsing kernel .config
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +The shell library provides an implementation of the kconfig parsing interface
>> +compatible with the C version.
>> +
>> +It's possible to pass kernel kconfig list for tst_require_kconfigs api with
> nit: s/api/API/ (it's an abbreviation). Also before merge please update git commit message.
>
>> +'$TST_NEEDS_KCONFIGS'.
>> +Optional '$TST_NEEDS_KCONFIG_IFS' is used for splitting, default value is comma.
>> +Optional '$TST_TCONF_IF_KCONFIG' is used for deciding how to exit the test if kernel
>> +.config doesn't meet test's requirement, default value is 1(TCONF). Otherwise, just
> I wonder if we need TST_TCONF_IF_KCONFIG functionality in the test or if it's an
> user request (i.e. user like variable LTP_TCONF_IF_KCONFIG doc/user-guide.txt).
> Because I'm not sure whether test would need it, but I can imagine user want to
> have test running even kernel config is not available (e.g. embedded platforms).
> Or maybe we need both user variable and test variable.
Oh, I misunderstand the usage.
I should use TST_TCONF_IF_KCONFIG for non-found kconfig file instead of
non-found kconfig list.
I think one variable is enough.
>
> Also not sure about TST_TCONF_IF_KCONFIG name, IMHO "IF" should be replaced to
> something which describes what it does.
Thinking a good name isn't a easy thing.
how about TCONF_IF_NO_KCONFIG?
>
> Also this patchset is about syncing C API functionality with shell API. But you
> introduce TST_TCONF_IF_KCONFIG only for shell. Shouldn't it be functionality for
> both parts?
Yes, code maybe as below:
void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len)
+static char kconfig_flag;
+
+int tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len)
{
char line[128];
unsigned int vars_found = 0;
+ const char *flag = getenv("TWARN_IF_NO_KCONFIG");
+
+ if (flag && !strcmp(flag,"y"))
+ kconfig_flag = 'y';
FILE *fp = open_kconfig();
- if (!fp)
+ if (!fp) {
+ if (kconfig_flag == 'y') {
+ tst_res(TWARN, "Cannot parse kernel .config");
+ return 1;
+ }
tst_brk(TBROK, "Cannot parse kernel .config");
-
+ }
while (fgets(line, sizeof(line), fp)) {
if (kconfig_parse_line(line, vars, vars_len))
vars_found++;
@@ -198,6 +210,7 @@ void tst_kconfig_read(struct tst_kconfig_var vars[],
size_t vars_len)
exit:
close_kconfig(fp);
+ return 0;
}
static size_t array_len(const char *const kconfigs[])
@@ -504,7 +517,9 @@ int tst_kconfig_check(const char *const kconfigs[])
var_cnt = populate_vars(exprs, expr_cnt, vars);
- tst_kconfig_read(vars, var_cnt);
+ ret = tst_kconfig_read(vars, var_cnt);
+ if (ret)
+ return kconfig_flag == 'y' ? 0 : 1;
>
> More notes about this variable also below.
>
> BTW github actions have probably kernel config on expected place, which means
> that most of the new tests TCONF, but tst_check_kconfig05.sh TFAIL.
I guess we can export the KCONFIG_PATH to solve this problem. But I
don't know the expected place on github actions.
>
> tst_rhost_run 1 TCONF: veth(null) 0 TWARN : /__w/ltp/ltp/lib/tst_kernel.c:110: expected file /lib/modules/5.11.0-1022-azure/modules.dep does not exist or not a file
> 320
> (null) 0 TWARN : /__w/ltp/ltp/lib/tst_kernel.c:110: expected file /lib/modules/5.11.0-1022-azure/modules.builtin does not exist or not a file driver not available
>
>> +use TWRAN and continue to run test.
>> +
>> +Now, we support the length of kconfig list is 10.
> Why 10? Cyril suggested that in PR, where he suggested to use separated
> variables:
> https://github.com/linux-test-project/ltp/issues/891#issuecomment-989712350
>
> But for string used like array there is no performance limitation, thus I'd use
> something like 50 or 100. Because for certain IMA tests there are at least 6
> kconfig requirements, thus> 10 could be hit.
If case needs more than 10 kconfigs, we can use & ie
"CONFIG_EX4_FS & CONFIG_XFS_FS & CONFIG_QUOTAL_FS, CONFIG_PROC_FS..."
>
> ...
>> diff --git a/lib/newlib_tests/shell/tst_check_kconfig01.sh b/lib/newlib_tests/shell/tst_check_kconfig01.sh
>> new file mode 100755
>> index 000000000..dc09b6092
>> --- /dev/null
>> +++ b/lib/newlib_tests/shell/tst_check_kconfig01.sh
>> @@ -0,0 +1,26 @@
>> +#!/bin/sh
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +# Copyright (c) 2021 FUJITSU LIMITED. All rights reserved.
>> +
>> +TST_TESTFUNC=do_test
>> +TST_NEEDS_CMDS="tst_check_kconfigs"
>> +TST_NEEDS_KCONFIGS="CONFIG_GENERIC_IRQ_PROBE=y,
>> +CONFIG_GENERIC_IRQ_SHOW=y,
>> +CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=y,
>> +CONFIG_GENERIC_PENDING_IRQ=y,
>> +CONFIG_GENERIC_IRQ_MIGRATION=y,
>> +CONFIG_IRQ_DOMAIN=y,
>> +CONFIG_IRQ_DOMAIN_HIERARCHY=y,
>> +CONFIG_GENERIC_MSI_IRQ=y,
>> +CONFIG_GENERIC_MSI_IRQ_DOMAIN=y,
>> +CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR=y,
>> +CONFIG_GENERIC_IRQ_RESERVATION_MODE=y"
>> +
>> +. tst_test.sh
>> +
>> +do_test()
>> +{
>> + tst_res TFAIL "Kconfig api only supports 10 kernel kconfigs!"
> s/api/API
OK
>
> ...
>> +++ b/lib/newlib_tests/shell/tst_check_kconfig05.sh
>> @@ -0,0 +1,26 @@
>> +#!/bin/sh
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +# Copyright (c) 2021 FUJITSU LIMITED. All rights reserved.
>> +
>> +TST_TESTFUNC=do_test
>> +TST_NEEDS_CMDS="tst_check_kconfigs"
>> +. tst_test.sh
>> +
>> +do_test()
>> +{
>> + tst_check_kconfigs "CONFIG_EXT4_FS"
>> + if [ $? -eq 0 ]; then
>> + tst_res TPASS "kernel .config has CONFIG_EXT4_fs."
> nit: I'd also remove dot at the end (we don't use it in tests).
OK
>
>> + else
>> + tst_res TFAIL "kerenl .config doesn't have CONFIG_EXT4_FS."
> s/kerenl/kernel/
Sorry.
>> + fi
>> +
>> + tst_check_kconfigs "CONFIG_EXT4"
>> + if [ $? -eq 0 ]; then
>> + tst_res TFAIL "kernel .config has CONFIG_EXT4."
>> + else
>> + tst_res TPASS "kernel .config doesn't have CONFIG_EXT4."
>> + fi
>> +}
>> +
>> +tst_run
>> diff --git a/lib/newlib_tests/shell/tst_check_kconfig06.sh b/lib/newlib_tests/shell/tst_check_kconfig06.sh
>> new file mode 100755
>> index 000000000..6a6d68fd7
>> --- /dev/null
>> +++ b/lib/newlib_tests/shell/tst_check_kconfig06.sh
>> @@ -0,0 +1,17 @@
>> +#!/bin/sh
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +# Copyright (c) 2021 FUJITSU LIMITED. All rights reserved.
>> +
>> +TST_TESTFUNC=do_test
>> +TST_NEEDS_CMDS="tst_check_kconfigs"
>> +TST_NEEDS_KCONFIGS="CONFIG_EXT4"
>> +TST_TCONF_IF_KCONFIG=0
> IMHO in shell is better to have empty value for "false", not zero.
> Also, we use it that way for shell variables.
>
OK
> But that would mean you'd have to allow to be empty, i.e. use : instead of :-:
> +TST_TCONF_IF_KCONFIG="${TST_TCONF_IF_KCONFIG:-1}"
> -TST_TCONF_IF_KCONFIG="${TST_TCONF_IF_KCONFIG:1}"
>
> But maybe it'd be better to have variable which is off by default (thus variable
> with reverse meaning).
OK
>
> ...
>> +++ b/testcases/lib/tst_check_kconfigs.c
>> @@ -0,0 +1,17 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/* Copyright (c) 2021 FUJITSU LIMITED. All rights reserved.*/
>> +
>> +#include<stdio.h>
>> +#include "tst_kconfig.h"
>> +
>> +int main(int argc, const char *argv[])
>> +{
>> + if (argc< 2) {
>> + fprintf(stderr, "Please provide kernel kconfig list\n");
>> + return 1;
>> + }
>> +
>> + if (tst_kconfig_check(argv+1))
>> + return 1;
> nit: I'd add space here (readability).
>> + return 0;
>> +}
>> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
>> index 2556b28f5..c8134692e 100644
> ...
>
>> --- a/testcases/lib/tst_test.sh
>> + tst_check_kconfigs $kconfig1 $kconfig2 $kconfig3 $kconfig4 $kconfig5 $kconfig6\
>> + $kconfig7 $kconfig8 $kconfig9 $kconfig10
>> + if [ $? -ne 0 ]; then
>> + if [ $TST_TCONF_IF_KCONFIG -eq 1 ]; then
>> + tst_brk TCONF "kconfig not available"
>
>> + else
>> + tst_res TWARN "kconfig not available"
> This is quite strong: either test "fails" due TWARN non-zero exit code or it's
> skipped. I'd prefer to have user variable for systems which are properly
> configured (user will make sure all kconfig options are set), but it's just
> missing kconfig due system configuration.
I plan to fix the variable usage for non-found kconfig path/file instead
of kconfig list.
Best Regards
Yang Xu
>
> Kind regards,
> Petr
More information about the ltp
mailing list