Re: [LTP] [PATCH v1 2/3] shell: add kconfig parse api

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

Hi Petr
> Hi Xu,
>
>>>> +'$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.
>
> OK, but I'd like to know others' opinion what's needed.
> Cyril, Li?
>
>>> 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?
>
> Well, I was not sure about "IF" part. For use in test code it should have "TST_"
> prefix, for users to set it should have "LTP_" prefix.
When I write this v2 patch, I think we may introuce a 
LTP_KCONFIG_DISABLE macro that can disable kconfig parser function whole 
for the situation that some embedded platforms doesn't have config. So 
we don't need to distinguish whether kconfig file doesn't exist or perms 
leak or invalid config expression.

code may below
static int tst_kconfig_disable()
{
         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);
         struct tst_expr *exprs[expr_cnt];
         unsigned int i, var_cnt;
         int ret = 0;

	if (tst_kconfig_disable())
		return 0
	...
}

Also, this macro is easy to understand and we don't need macro ie 
CONF_IF_NO_KCONFIG or TWARN_NO_IF_KCONFIG. Any idea?

Best Regards
Yang Xu
>
>>> 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;
>>    }
>
> Sure, once we agree what should be implemented.
>
>>    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.
>
> Sure, for github we can find config place.
> But this can happen to user who runs the test. IMHO test should not fail if
> user's system is without config. That's why I'd like to have a variable making
> errors non-fatal.
>
>>> 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..."
> Sure. I just meant there is no reason to put low number and then workaround it.
>
>>>> --- 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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp