Re: [LTP] [PATCH v4 3/5] shell: add kconfig parse api

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

Hi Cyril, Li, Petr

> Hi!
>> int main(int argc, char *argv[])
>> {
>>           char *str = argv[1];
>>           char *delim = argv[2];
>>           unsigned int i, cnt = 1;
>>           int ret = 0;
>>
>>           if (argc == 2) {
>>                   delim = ",";
>>           } else if (argc == 3) {
>>                   if (strlen(delim)>  1) {
>>                           fprintf(stderr, "The delim must be a single
>> character\n");
>>                           return 1;
>>                   }
>>           } else {
>>                   fprintf(stderr, "Please provide kernel kconfig list and
>> delim "
>>                                   "(optinal, default value is ',')\n");
>>                   return 1;
>>           }
>
> We can make the code a bit easier to read with a switch()
>
> 	switch (argc) {
> 	case 2:
> 		delim = ",";
> 	break;
> 	case 3:
> 		if (strlen(delim)>  1) {
> 			fprintf(stderr, "...");
> 			return 1;
> 		}
> 	break;
> 	default:
> 		fprintf(stderr, "...");
> 		return 1;
> 	}
>
>
>>           for (i = 0; str[i]; i++) {
>>                   if (str[i] == delim[0])
>>                           cnt++;
>>           }
>>
>>           char **kconfigs = malloc(++cnt * sizeof(char *));
>
> It's unlikely that this will fail the allocation, but for the sake of
> correctness we should also do:
>
> 	if (!kconfigs) {
> 		fprintf(stderr, "malloc failed");
> 		return 1;
> 	}
>
>>           for (i = 0; i<  cnt; i++)
>>                   kconfigs[i] = strtok_r(str, delim,&str);
>>
>>           if (tst_kconfig_check((const char * const*)kconfigs))
>>                   ret = 1;
>>
>>           free(kconfigs);
>>           return ret;
>> }
>
> Other than the two minor issues this version looks good to me:

I modify this patch with the above comment and also move tst_res into
kconfig_skip_check in the 2nd patch. Then I pushed this patchset.

Thanks for your patient review.

Best Regards
Yang Xu
>
> Reviewed-by: Cyril Hrubis<chrubis@suse.cz>
>

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